Skip to content

Commit

Permalink
[node-cleanup] Keep the original get when checking for node
Browse files Browse the repository at this point in the history
Only fallback ont the list call with the label if the node is not found in the get. This should be safer.
  • Loading branch information
Fricounet committed Aug 8, 2024
1 parent 7005f47 commit 09efb94
Show file tree
Hide file tree
Showing 4 changed files with 38 additions and 21 deletions.
26 changes: 16 additions & 10 deletions pkg/common/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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.
Expand Down
23 changes: 17 additions & 6 deletions pkg/common/common_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
},
}
Expand All @@ -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,
},
}
Expand All @@ -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())
}
Expand Down
6 changes: 3 additions & 3 deletions pkg/node-cleanup/controller/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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},
},
},
},
Expand All @@ -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},
},
},
},
Expand Down Expand Up @@ -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,
},
}
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/node-cleanup/deleter/deleter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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},
},
},
},
Expand All @@ -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,
},
}
}
Expand Down

0 comments on commit 09efb94

Please sign in to comment.