From a375b0aa3679d165d64129596e5e098cc12710d9 Mon Sep 17 00:00:00 2001 From: Baptiste Girard-Carrabin Date: Tue, 4 Mar 2025 15:29:42 +0100 Subject: [PATCH 1/5] Ensure CSINode belongs to current node on init When a CSI plugin attempts to register on a node, the node checks if the CSINode object exists. If it does but the ownerReference of the CSINode does not match the UID of the current node, possibly because the node object was recreated with the same name, we end up in a race condition where the CSINode object will be updated but subsequently deleted by the GC controller. In this situation, the CSINode object will be gone and won't be recreated unless the CSI plugin or the kubelet are restarted. This commit fixes this race by checking that the CSINode object belong to the current node during initialization. If it doesn't, it means that the CSINode object is left over from a previous node and it must be removed first. Once removed, registration can progress as usual. --- .../csi/nodeinfomanager/nodeinfomanager.go | 45 ++++++++++++++++--- 1 file changed, 38 insertions(+), 7 deletions(-) diff --git a/pkg/volume/csi/nodeinfomanager/nodeinfomanager.go b/pkg/volume/csi/nodeinfomanager/nodeinfomanager.go index 1bff173c5fe..e03af9d08e6 100644 --- a/pkg/volume/csi/nodeinfomanager/nodeinfomanager.go +++ b/pkg/volume/csi/nodeinfomanager/nodeinfomanager.go @@ -63,6 +63,7 @@ var ( // the Node and CSINode objects. type nodeInfoManager struct { nodeName types.NodeName + nodeID types.UID volumeHost volume.VolumeHost migratedPlugins map[string](func() bool) // lock protects changes to node. @@ -430,6 +431,12 @@ func (nim *nodeInfoManager) tryInitializeCSINodeWithAnnotation(csiKubeClient cli nim.lock.Lock() defer nim.lock.Unlock() + node, err := csiKubeClient.CoreV1().Nodes().Get(context.TODO(), string(nim.nodeName), metav1.GetOptions{}) + if err != nil { + return err + } + nim.nodeID = node.UID + nodeInfo, err := csiKubeClient.StorageV1().CSINodes().Get(context.TODO(), string(nim.nodeName), metav1.GetOptions{}) if nodeInfo == nil || errors.IsNotFound(err) { // CreateCSINode will set the annotation @@ -439,6 +446,16 @@ func (nim *nodeInfoManager) tryInitializeCSINodeWithAnnotation(csiKubeClient cli return err } + if !nim.nodeOwnsCSINode(nodeInfo) { + klog.V(2).Infof("existing CSINode %q is owned by different node, cleaning up...", nodeInfo.Name) + err = nim.DeleteCSINode() + if err != nil { + return fmt.Errorf("error deleting existing CSINode %q: %w", nodeInfo.Name, err) + } + // Returning now so that the next attempt can create a new CSINode object + return fmt.Errorf("CSINode %q was owned by different node, deleted it", nodeInfo.Name) + } + annotationModified := setMigrationAnnotation(nim.migratedPlugins, nodeInfo) if annotationModified { @@ -449,6 +466,15 @@ func (nim *nodeInfoManager) tryInitializeCSINodeWithAnnotation(csiKubeClient cli } +func (nim *nodeInfoManager) nodeOwnsCSINode(nodeInfo *storagev1.CSINode) bool { + for _, ownerRef := range nodeInfo.OwnerReferences { + if ownerRef.Kind == nodeKind.Kind && ownerRef.Name == string(nim.nodeName) && ownerRef.UID == nim.nodeID { + return true + } + } + return false +} + func (nim *nodeInfoManager) CreateCSINode() (*storagev1.CSINode, error) { csiKubeClient := nim.volumeHost.GetKubeClient() @@ -456,11 +482,6 @@ func (nim *nodeInfoManager) CreateCSINode() (*storagev1.CSINode, error) { return nil, fmt.Errorf("error getting CSI client") } - node, err := csiKubeClient.CoreV1().Nodes().Get(context.TODO(), string(nim.nodeName), metav1.GetOptions{}) - if err != nil { - return nil, err - } - nodeInfo := &storagev1.CSINode{ ObjectMeta: metav1.ObjectMeta{ Name: string(nim.nodeName), @@ -468,8 +489,8 @@ func (nim *nodeInfoManager) CreateCSINode() (*storagev1.CSINode, error) { { APIVersion: nodeKind.Version, Kind: nodeKind.Kind, - Name: node.Name, - UID: node.UID, + Name: string(nim.nodeName), + UID: nim.nodeID, }, }, }, @@ -483,6 +504,16 @@ func (nim *nodeInfoManager) CreateCSINode() (*storagev1.CSINode, error) { return csiKubeClient.StorageV1().CSINodes().Create(context.TODO(), nodeInfo, metav1.CreateOptions{}) } +func (nim *nodeInfoManager) DeleteCSINode() error { + + csiKubeClient := nim.volumeHost.GetKubeClient() + if csiKubeClient == nil { + return fmt.Errorf("error getting CSI client") + } + + return csiKubeClient.StorageV1().CSINodes().Delete(context.TODO(), string(nim.nodeName), metav1.DeleteOptions{}) +} + func setMigrationAnnotation(migratedPlugins map[string](func() bool), nodeInfo *storagev1.CSINode) (modified bool) { if migratedPlugins == nil { return false From 328f9da729dc2e461bc3028e3be4c55c72da9e22 Mon Sep 17 00:00:00 2001 From: Baptiste Girard-Carrabin Date: Tue, 7 Oct 2025 10:03:13 +0200 Subject: [PATCH 2/5] Add current and old nodeIDs in log messages Refactor the nodeOwnsCSINode function to also outputs the current owner ID so that it can be logged afterwards. --- .../csi/nodeinfomanager/nodeinfomanager.go | 28 ++++++++++++++----- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/pkg/volume/csi/nodeinfomanager/nodeinfomanager.go b/pkg/volume/csi/nodeinfomanager/nodeinfomanager.go index e03af9d08e6..a8ef330a743 100644 --- a/pkg/volume/csi/nodeinfomanager/nodeinfomanager.go +++ b/pkg/volume/csi/nodeinfomanager/nodeinfomanager.go @@ -446,14 +446,14 @@ func (nim *nodeInfoManager) tryInitializeCSINodeWithAnnotation(csiKubeClient cli return err } - if !nim.nodeOwnsCSINode(nodeInfo) { - klog.V(2).Infof("existing CSINode %q is owned by different node, cleaning up...", nodeInfo.Name) + if ok, csiNodeOwnerID := nim.nodeOwnsCSINode(nodeInfo); !ok { + klog.V(2).Infof("existing CSINode %q is owned by different node (oldNodeID=%q, newNodeID=%q), cleaning up...", nodeInfo.Name, csiNodeOwnerID, nim.nodeID) err = nim.DeleteCSINode() if err != nil { return fmt.Errorf("error deleting existing CSINode %q: %w", nodeInfo.Name, err) } // Returning now so that the next attempt can create a new CSINode object - return fmt.Errorf("CSINode %q was owned by different node, deleted it", nodeInfo.Name) + return fmt.Errorf("CSINode %q was owned by different node (oldNodeID=%q, newNodeID=%q), deleted it", nodeInfo.Name, csiNodeOwnerID, nim.nodeID) } annotationModified := setMigrationAnnotation(nim.migratedPlugins, nodeInfo) @@ -466,13 +466,27 @@ func (nim *nodeInfoManager) tryInitializeCSINodeWithAnnotation(csiKubeClient cli } -func (nim *nodeInfoManager) nodeOwnsCSINode(nodeInfo *storagev1.CSINode) bool { +// nodeOwnsCSINode checks if the CSINode object is owned by the node represented by this nodeInfoManager. +// It also returns the current owner UID. +func (nim *nodeInfoManager) nodeOwnsCSINode(nodeInfo *storagev1.CSINode) (bool, types.UID) { + var csiNodeOwnerID types.UID + var found bool for _, ownerRef := range nodeInfo.OwnerReferences { - if ownerRef.Kind == nodeKind.Kind && ownerRef.Name == string(nim.nodeName) && ownerRef.UID == nim.nodeID { - return true + if ownerRef.Kind != nodeKind.Kind { + continue + } + + csiNodeOwnerID = ownerRef.UID + if ownerRef.Name != string(nim.nodeName) { + continue + } + + if csiNodeOwnerID == nim.nodeID { + found = true + break } } - return false + return found, csiNodeOwnerID } func (nim *nodeInfoManager) CreateCSINode() (*storagev1.CSINode, error) { From cb01a28df40235645c969eca9acbaae5e76e8900 Mon Sep 17 00:00:00 2001 From: Baptiste Girard-Carrabin Date: Tue, 7 Oct 2025 11:00:55 +0200 Subject: [PATCH 3/5] Ensure CSINode is owned by node when installing CSI drivers CSI drivers can be installed while the node is still initializing since the daemonsets usually tolerate all taints. As a result, we also need to make sure the outdated CSINode object has been removed before installing a new CSI driver. --- .../csi/nodeinfomanager/nodeinfomanager.go | 29 ++++++++++++----- .../nodeinfomanager/nodeinfomanager_test.go | 31 +++++++++++++++++++ 2 files changed, 52 insertions(+), 8 deletions(-) diff --git a/pkg/volume/csi/nodeinfomanager/nodeinfomanager.go b/pkg/volume/csi/nodeinfomanager/nodeinfomanager.go index a8ef330a743..ba1bde76a2d 100644 --- a/pkg/volume/csi/nodeinfomanager/nodeinfomanager.go +++ b/pkg/volume/csi/nodeinfomanager/nodeinfomanager.go @@ -403,6 +403,10 @@ func (nim *nodeInfoManager) tryUpdateCSINode( return err } + if err = nim.ensureNodeOwnsCSINode(nodeInfo); err != nil { + return err + } + return nim.installDriverToCSINode(nodeInfo, driverName, driverNodeID, maxAttachLimit, topology) } @@ -446,14 +450,8 @@ func (nim *nodeInfoManager) tryInitializeCSINodeWithAnnotation(csiKubeClient cli return err } - if ok, csiNodeOwnerID := nim.nodeOwnsCSINode(nodeInfo); !ok { - klog.V(2).Infof("existing CSINode %q is owned by different node (oldNodeID=%q, newNodeID=%q), cleaning up...", nodeInfo.Name, csiNodeOwnerID, nim.nodeID) - err = nim.DeleteCSINode() - if err != nil { - return fmt.Errorf("error deleting existing CSINode %q: %w", nodeInfo.Name, err) - } - // Returning now so that the next attempt can create a new CSINode object - return fmt.Errorf("CSINode %q was owned by different node (oldNodeID=%q, newNodeID=%q), deleted it", nodeInfo.Name, csiNodeOwnerID, nim.nodeID) + if err = nim.ensureNodeOwnsCSINode(nodeInfo); err != nil { + return err } annotationModified := setMigrationAnnotation(nim.migratedPlugins, nodeInfo) @@ -466,6 +464,21 @@ func (nim *nodeInfoManager) tryInitializeCSINodeWithAnnotation(csiKubeClient cli } +// ensureNodeOwnsCSINode will ensure that the current CSINode object is owned by the node represented by this nodeInfoManager. +// If not, it will delete the existing CSINode object and return an error. +func (nim *nodeInfoManager) ensureNodeOwnsCSINode(nodeInfo *storagev1.CSINode) error { + if ok, csiNodeOwnerID := nim.nodeOwnsCSINode(nodeInfo); !ok { + klog.V(2).Infof("existing CSINode %q is owned by different node (oldNodeID=%q, newNodeID=%q), cleaning up...", nodeInfo.Name, csiNodeOwnerID, nim.nodeID) + err := nim.DeleteCSINode() + if err != nil { + return fmt.Errorf("error deleting existing CSINode %q: %w", nodeInfo.Name, err) + } + // Returning now so that the next attempt can create a new CSINode object + return fmt.Errorf("CSINode %q was owned by different node (oldNodeID=%q, newNodeID=%q), deleted it", nodeInfo.Name, csiNodeOwnerID, nim.nodeID) + } + return nil +} + // nodeOwnsCSINode checks if the CSINode object is owned by the node represented by this nodeInfoManager. // It also returns the current owner UID. func (nim *nodeInfoManager) nodeOwnsCSINode(nodeInfo *storagev1.CSINode) (bool, types.UID) { diff --git a/pkg/volume/csi/nodeinfomanager/nodeinfomanager_test.go b/pkg/volume/csi/nodeinfomanager/nodeinfomanager_test.go index 60b4e8c6f69..be00b0969a0 100644 --- a/pkg/volume/csi/nodeinfomanager/nodeinfomanager_test.go +++ b/pkg/volume/csi/nodeinfomanager/nodeinfomanager_test.go @@ -307,6 +307,37 @@ func TestInstallCSIDriver(t *testing.T) { }, }, }, + { + name: "pre-existing node info, but owned by previous node", + existingNode: func() *v1.Node { + node := generateNode(nil /*nodeIDs*/, nil /*labels*/, nil /*capacity*/) + node.UID = types.UID("node1") + return node + }(), + existingCSINode: func() *storage.CSINode { + csiNode := generateCSINode(nil /*nodeIDs*/, nil /*volumeLimits*/, nil /*topologyKeys*/) + csiNode.OwnerReferences[0].UID = types.UID("node2") + return csiNode + }(), + inputNodeID: "com.example.csi/csi-node1", + expectedNode: &v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node1", + UID: types.UID("node1"), + Annotations: map[string]string{annotationKeyNodeID: marshall(nodeIDMap{"": "com.example.csi/csi-node1"})}, + }, + }, + expectedCSINode: &storage.CSINode{ + ObjectMeta: getCSINodeObjectMeta(), + Spec: storage.CSINodeSpec{ + Drivers: []storage.CSINodeDriver{ + { + NodeID: "com.example.csi/csi-node1", + }, + }, + }, + }, + }, { name: "nil topology, empty node", driverName: "com.example.csi.driver1", From a14230ae82da9fb65081f9be59c778608a618469 Mon Sep 17 00:00:00 2001 From: Baptiste Girard-Carrabin Date: Thu, 23 Oct 2025 10:28:51 +0200 Subject: [PATCH 4/5] [test] Validate migration annotation are still applied --- .../nodeinfomanager/nodeinfomanager_test.go | 26 ++++++++++++------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/pkg/volume/csi/nodeinfomanager/nodeinfomanager_test.go b/pkg/volume/csi/nodeinfomanager/nodeinfomanager_test.go index be00b0969a0..b211c3eba4c 100644 --- a/pkg/volume/csi/nodeinfomanager/nodeinfomanager_test.go +++ b/pkg/volume/csi/nodeinfomanager/nodeinfomanager_test.go @@ -56,6 +56,7 @@ type testcase struct { expectedCSINode *storage.CSINode expectFail bool hasModified bool + migratedPlugins map[string](func() bool) } type nodeIDMap map[string]string @@ -319,6 +320,9 @@ func TestInstallCSIDriver(t *testing.T) { csiNode.OwnerReferences[0].UID = types.UID("node2") return csiNode }(), + migratedPlugins: map[string](func() bool){ + "com.example.csi.driver1": func() bool { return true }, + }, inputNodeID: "com.example.csi/csi-node1", expectedNode: &v1.Node{ ObjectMeta: metav1.ObjectMeta{ @@ -327,16 +331,20 @@ func TestInstallCSIDriver(t *testing.T) { Annotations: map[string]string{annotationKeyNodeID: marshall(nodeIDMap{"": "com.example.csi/csi-node1"})}, }, }, - expectedCSINode: &storage.CSINode{ - ObjectMeta: getCSINodeObjectMeta(), - Spec: storage.CSINodeSpec{ - Drivers: []storage.CSINodeDriver{ - { - NodeID: "com.example.csi/csi-node1", + expectedCSINode: func() *storage.CSINode { + csiNode := &storage.CSINode{ + ObjectMeta: getCSINodeObjectMeta(), + Spec: storage.CSINodeSpec{ + Drivers: []storage.CSINodeDriver{ + { + NodeID: "com.example.csi/csi-node1", + }, }, }, - }, - }, + } + csiNode.Annotations = map[string]string{v1.MigratedPluginsAnnotationKey: "com.example.csi.driver1"} + return csiNode + }(), }, { name: "nil topology, empty node", @@ -1060,7 +1068,7 @@ func test(t *testing.T, addNodeInfo bool, testcases []testcase) { nil, nil, ) - nim := NewNodeInfoManager(types.NodeName(nodeName), host, nil) + nim := NewNodeInfoManager(types.NodeName(nodeName), host, tc.migratedPlugins) //// Act nim.CreateCSINode() From 28507d0e9213d5f716160ce8248fdee3d090ec87 Mon Sep 17 00:00:00 2001 From: Baptiste Girard-Carrabin Date: Thu, 23 Oct 2025 10:44:55 +0200 Subject: [PATCH 5/5] [test] Add case for existing driver on old csi node The driver on the old object should not appear on the new one because it represented a different node --- .../nodeinfomanager/nodeinfomanager_test.go | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/pkg/volume/csi/nodeinfomanager/nodeinfomanager_test.go b/pkg/volume/csi/nodeinfomanager/nodeinfomanager_test.go index b211c3eba4c..c57e4d69a88 100644 --- a/pkg/volume/csi/nodeinfomanager/nodeinfomanager_test.go +++ b/pkg/volume/csi/nodeinfomanager/nodeinfomanager_test.go @@ -346,6 +346,45 @@ func TestInstallCSIDriver(t *testing.T) { return csiNode }(), }, + { + name: "pre-existing node info with driver, but owned by previous node", + existingNode: func() *v1.Node { + node := generateNode(nil /*nodeIDs*/, nil /*labels*/, nil /*capacity*/) + node.UID = types.UID("node1") + return node + }(), + existingCSINode: func() *storage.CSINode { + csiNode := generateCSINode( + nodeIDMap{ + "com.example.csi.old-driver": "com.example.csi/csi-node2", + }, + nil /*volumeLimits*/, nil, /*topologyKeys*/ + ) + csiNode.OwnerReferences[0].UID = types.UID("node2") + return csiNode + }(), + driverName: "com.example.csi.driver1", + inputNodeID: "com.example.csi/csi-node1", + expectedNode: &v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node1", + UID: types.UID("node1"), + Annotations: map[string]string{annotationKeyNodeID: marshall(nodeIDMap{"com.example.csi.driver1": "com.example.csi/csi-node1"})}, + }, + }, + expectedCSINode: &storage.CSINode{ + ObjectMeta: getCSINodeObjectMeta(), + Spec: storage.CSINodeSpec{ + Drivers: []storage.CSINodeDriver{ + { + // Only the new driver should be present because the old CSINode represented a previous node. + Name: "com.example.csi.driver1", + NodeID: "com.example.csi/csi-node1", + }, + }, + }, + }, + }, { name: "nil topology, empty node", driverName: "com.example.csi.driver1",