From 7005f4751dce4cb002cb47b124e76bca90b3722a Mon Sep 17 00:00:00 2001 From: Baptiste Girard-Carrabin Date: Tue, 23 Apr 2024 18:55:54 +0200 Subject: [PATCH 1/2] [node-cleanup] Check node label to know if pv is linked to a deleted node --- pkg/common/common.go | 18 ++++++++++++------ pkg/common/common_test.go | 6 ++++-- pkg/node-cleanup/controller/controller_test.go | 6 +++--- pkg/node-cleanup/deleter/deleter_test.go | 4 ++-- 4 files changed, 21 insertions(+), 13 deletions(-) diff --git a/pkg/common/common.go b/pkg/common/common.go index c1cf80e28..2efd3e734 100644 --- a/pkg/common/common.go +++ b/pkg/common/common.go @@ -33,9 +33,10 @@ import ( "sigs.k8s.io/yaml" v1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/selection" "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes" corelisters "k8s.io/client-go/listers/core/v1" @@ -488,12 +489,17 @@ func GetVolumeMode(volUtil util.VolumeUtil, fullPath string) (v1.PersistentVolum } // NodeExists checks to see if a Node exists in the Indexer of a NodeLister. -func NodeExists(nodeLister corelisters.NodeLister, nodeName string) (bool, error) { - _, err := nodeLister.Get(nodeName) - if errors.IsNotFound(err) { - return false, nil +// It uses the well known label `kubernetes.io/hostname` to find the Node. +func NodeExists(nodeLister corelisters.NodeLister, nodeLabelValue string) (bool, error) { + req, err := labels.NewRequirement(NodeLabelKey, selection.Equals, []string{nodeLabelValue}) + if err != nil { + return false, err + } + nodes, err := nodeLister.List(labels.NewSelector().Add(*req)) + if err != nil { + return false, err } - return err == nil, err + return len(nodes) > 0, nil } // NodeAttachedToLocalPV gets the name of the Node that a local PV has a NodeAffinity to. diff --git a/pkg/common/common_test.go b/pkg/common/common_test.go index b10df208f..4bf3295f2 100644 --- a/pkg/common/common_test.go +++ b/pkg/common/common_test.go @@ -480,7 +480,9 @@ func TestGetVolumeMode(t *testing.T) { func TestNodeExists(t *testing.T) { node := &v1.Node{ ObjectMeta: metav1.ObjectMeta{ - Name: "test-node", + Labels: map[string]string{ + NodeLabelKey: "test-node", + }, }, } @@ -510,7 +512,7 @@ func TestNodeExists(t *testing.T) { nodeInformer.Informer().GetStore().Add(test.nodeAdded) } - exists, err := NodeExists(nodeInformer.Lister(), test.nodeQueried.Name) + exists, err := NodeExists(nodeInformer.Lister(), test.nodeQueried.Labels[NodeLabelKey]) if err != nil { t.Errorf("Got unexpected error: %s", err.Error()) } diff --git a/pkg/node-cleanup/controller/controller_test.go b/pkg/node-cleanup/controller/controller_test.go index 4a2604e58..257394959 100644 --- a/pkg/node-cleanup/controller/controller_test.go +++ b/pkg/node-cleanup/controller/controller_test.go @@ -330,7 +330,7 @@ func pvWithPVCAndNode(pvc *v1.PersistentVolumeClaim, node *v1.Node) *v1.Persiste { Key: common.NodeLabelKey, Operator: v1.NodeSelectorOpIn, - Values: []string{node.Name}, + Values: []string{node.Labels[common.NodeLabelKey]}, }, }, }, @@ -351,7 +351,7 @@ func pvWithNode(node *v1.Node) *v1.PersistentVolume { { Key: common.NodeLabelKey, Operator: v1.NodeSelectorOpIn, - Values: []string{node.Name}, + Values: []string{node.Labels[common.NodeLabelKey]}, }, }, }, @@ -386,7 +386,7 @@ func pvcWithUID(uid string) *v1.PersistentVolumeClaim { func node() *v1.Node { return &v1.Node{ ObjectMeta: metav1.ObjectMeta{ - Name: defaultNodeName, + Labels: map[string]string{common.NodeLabelKey: defaultNodeName}, }, } } diff --git a/pkg/node-cleanup/deleter/deleter_test.go b/pkg/node-cleanup/deleter/deleter_test.go index 754cfec02..e3733b47c 100644 --- a/pkg/node-cleanup/deleter/deleter_test.go +++ b/pkg/node-cleanup/deleter/deleter_test.go @@ -211,7 +211,7 @@ func localPV(node *v1.Node, phase v1.PersistentVolumePhase, reclaimPolicy v1.Per { Key: common.NodeLabelKey, Operator: v1.NodeSelectorOpIn, - Values: []string{node.Name}, + Values: []string{node.Labels[common.NodeLabelKey]}, }, }, }, @@ -230,7 +230,7 @@ func localPV(node *v1.Node, phase v1.PersistentVolumePhase, reclaimPolicy v1.Per func node() *v1.Node { return &v1.Node{ ObjectMeta: metav1.ObjectMeta{ - Name: testNodeName, + Labels: map[string]string{common.NodeLabelKey: testNodeName}, }, } } From 09efb948091985712e60b28f23695730eba4990f Mon Sep 17 00:00:00 2001 From: Baptiste Girard-Carrabin Date: Thu, 8 Aug 2024 19:13:57 +0200 Subject: [PATCH 2/2] [node-cleanup] Keep the original get when checking for node Only fallback ont the list call with the label if the node is not found in the get. This should be safer. --- pkg/common/common.go | 26 ++++++++++++------- pkg/common/common_test.go | 23 +++++++++++----- .../controller/controller_test.go | 6 ++--- pkg/node-cleanup/deleter/deleter_test.go | 4 +-- 4 files changed, 38 insertions(+), 21 deletions(-) diff --git a/pkg/common/common.go b/pkg/common/common.go index 2efd3e734..56daf93b2 100644 --- a/pkg/common/common.go +++ b/pkg/common/common.go @@ -33,6 +33,7 @@ import ( "sigs.k8s.io/yaml" v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" @@ -489,17 +490,22 @@ func GetVolumeMode(volUtil util.VolumeUtil, fullPath string) (v1.PersistentVolum } // NodeExists checks to see if a Node exists in the Indexer of a NodeLister. -// It uses the well known label `kubernetes.io/hostname` to find the Node. -func NodeExists(nodeLister corelisters.NodeLister, nodeLabelValue string) (bool, error) { - req, err := labels.NewRequirement(NodeLabelKey, selection.Equals, []string{nodeLabelValue}) - if err != nil { - return false, err - } - nodes, err := nodeLister.List(labels.NewSelector().Add(*req)) - if err != nil { - return false, err +// It tries to get the node and if it fails, it uses the well known label +// `kubernetes.io/hostname` to find the Node. +func NodeExists(nodeLister corelisters.NodeLister, nodeName string) (bool, error) { + _, err := nodeLister.Get(nodeName) + if errors.IsNotFound(err) { + req, err := labels.NewRequirement(NodeLabelKey, selection.Equals, []string{nodeName}) + if err != nil { + return false, err + } + nodes, err := nodeLister.List(labels.NewSelector().Add(*req)) + if err != nil { + return false, err + } + return len(nodes) > 0, nil } - return len(nodes) > 0, nil + return err == nil, err } // NodeAttachedToLocalPV gets the name of the Node that a local PV has a NodeAffinity to. diff --git a/pkg/common/common_test.go b/pkg/common/common_test.go index 4bf3295f2..14896b867 100644 --- a/pkg/common/common_test.go +++ b/pkg/common/common_test.go @@ -478,10 +478,16 @@ func TestGetVolumeMode(t *testing.T) { } func TestNodeExists(t *testing.T) { - node := &v1.Node{ + nodeName := "test-node" + nodeWithName := &v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: nodeName, + }, + } + nodeWithLabel := &v1.Node{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ - NodeLabelKey: "test-node", + NodeLabelKey: nodeName, }, }, } @@ -493,12 +499,17 @@ func TestNodeExists(t *testing.T) { expectedResult bool }{ { - nodeAdded: node, - nodeQueried: node, + nodeAdded: nodeWithName, + nodeQueried: nodeWithName, + expectedResult: true, + }, + { + nodeAdded: nodeWithLabel, + nodeQueried: nodeWithName, expectedResult: true, }, { - nodeQueried: node, + nodeQueried: nodeWithName, expectedResult: false, }, } @@ -512,7 +523,7 @@ func TestNodeExists(t *testing.T) { nodeInformer.Informer().GetStore().Add(test.nodeAdded) } - exists, err := NodeExists(nodeInformer.Lister(), test.nodeQueried.Labels[NodeLabelKey]) + exists, err := NodeExists(nodeInformer.Lister(), test.nodeQueried.Name) if err != nil { t.Errorf("Got unexpected error: %s", err.Error()) } diff --git a/pkg/node-cleanup/controller/controller_test.go b/pkg/node-cleanup/controller/controller_test.go index 257394959..4a2604e58 100644 --- a/pkg/node-cleanup/controller/controller_test.go +++ b/pkg/node-cleanup/controller/controller_test.go @@ -330,7 +330,7 @@ func pvWithPVCAndNode(pvc *v1.PersistentVolumeClaim, node *v1.Node) *v1.Persiste { Key: common.NodeLabelKey, Operator: v1.NodeSelectorOpIn, - Values: []string{node.Labels[common.NodeLabelKey]}, + Values: []string{node.Name}, }, }, }, @@ -351,7 +351,7 @@ func pvWithNode(node *v1.Node) *v1.PersistentVolume { { Key: common.NodeLabelKey, Operator: v1.NodeSelectorOpIn, - Values: []string{node.Labels[common.NodeLabelKey]}, + Values: []string{node.Name}, }, }, }, @@ -386,7 +386,7 @@ func pvcWithUID(uid string) *v1.PersistentVolumeClaim { func node() *v1.Node { return &v1.Node{ ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{common.NodeLabelKey: defaultNodeName}, + Name: defaultNodeName, }, } } diff --git a/pkg/node-cleanup/deleter/deleter_test.go b/pkg/node-cleanup/deleter/deleter_test.go index e3733b47c..754cfec02 100644 --- a/pkg/node-cleanup/deleter/deleter_test.go +++ b/pkg/node-cleanup/deleter/deleter_test.go @@ -211,7 +211,7 @@ func localPV(node *v1.Node, phase v1.PersistentVolumePhase, reclaimPolicy v1.Per { Key: common.NodeLabelKey, Operator: v1.NodeSelectorOpIn, - Values: []string{node.Labels[common.NodeLabelKey]}, + Values: []string{node.Name}, }, }, }, @@ -230,7 +230,7 @@ func localPV(node *v1.Node, phase v1.PersistentVolumePhase, reclaimPolicy v1.Per func node() *v1.Node { return &v1.Node{ ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{common.NodeLabelKey: testNodeName}, + Name: testNodeName, }, } }