Merge pull request #5 from gridscale/fix/node-taint-not-respecting-pools

Fix node taint not respecting pools

Author: fabiante

.github/workflows/ci.yaml

@@ -27,9 +27,12 @@ jobs:
         with:
           path: ${{ env.GOPATH }}/src/k8s.io/autoscaler
 
-      - name: Test
+      - name: Test cloudprovider/gridscale
         working-directory: ${{ env.GOPATH }}/src/k8s.io/autoscaler/cluster-autoscaler/cloudprovider/gridscale
         run: go test ./...
+      - name: Test utils/taints
+        working-directory: ${{ env.GOPATH }}/src/k8s.io/autoscaler/cluster-autoscaler/utils/taints
+        run: go test ./...
 
   test-and-verify:
     runs-on: ubuntu-latest

cluster-autoscaler/utils/taints/taints.go

@@ -35,8 +35,8 @@ import (
 	klog "k8s.io/klog/v2"
 )
 
-// gridscaleNode0SuffixName is the suffix of the gridscale node 0's name
-const gridscaleNode0SuffixName = "-node-pool0-0"
+// gridscaleNode0SuffixName is the suffix of the first node of every gridscale node pool.
+const gridscaleNode0SuffixName = "-0"
 
 const (
 	// ToBeDeletedTaint is a taint used to make the node unschedulable.
@@ -182,16 +182,25 @@ func MarkDeletionCandidate(node *apiv1.Node, client kube_client.Interface) error
 	return AddTaints(node, client, []apiv1.Taint{taint}, false)
 }
 
+// skipTaintingGridscaleNode checks if a node should not be tainted. This is gridscale specific.
+func skipTaintingGridscaleNode(node *apiv1.Node) (reason string, skip bool) {
+	// The first node should never be tainted
+	if strings.HasSuffix(node.Name, gridscaleNode0SuffixName) {
+		return fmt.Sprintf("Skipping tainting of node %v, because the first pool node should never be tainted", node.Name), true
+	}
+	return "", false
+}
+
 // AddTaints sets the specified taints on the node.
 func AddTaints(node *apiv1.Node, client kube_client.Interface, taints []apiv1.Taint, cordonNode bool) error {
 	retryDeadline := time.Now().Add(maxRetryDeadline)
 	freshNode := node.DeepCopy()
 	var err error
 	refresh := false
 	for {
-		// skip tainting gridscale node 0
-		if strings.HasSuffix(node.Name, gridscaleNode0SuffixName) {
-			klog.V(1).Infof("Skipping tainting of node %v, because it is a gridscale node 0", node.Name)
+		// skip tainting gridscale nodes when we don't want to
+		if reason, skip := skipTaintingGridscaleNode(node); skip {
+			klog.V(1).Infof(reason)
 			return nil
 		}
 		if refresh {

cluster-autoscaler/utils/taints/taints_gridscale_test.go

@@ -0,0 +1,59 @@
+package taints
+
+import (
+	"github.com/stretchr/testify/assert"
+	apiv1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"testing"
+)
+
+func Test_skipTaintingGridscaleNode(t *testing.T) {
+	type Test struct {
+		Name        string
+		NodeName    string
+		ExpectsSkip bool
+	}
+
+	tests := []Test{
+		{
+			Name:        "skips node if it is the first node",
+			NodeName:    "prod-my-pool-pool0-0",
+			ExpectsSkip: true,
+		},
+		{
+			Name:        "does not skip second node",
+			NodeName:    "prod-my-pool-pool0-1",
+			ExpectsSkip: false,
+		},
+		{
+			Name:        "skips node with pre-node-pool name if it is the first node",
+			NodeName:    "prod-node-pool0-0",
+			ExpectsSkip: true,
+		},
+		{
+			Name:        "does not skip node with pre-node-pool name if it is the second node",
+			NodeName:    "prod-node-pool0-1",
+			ExpectsSkip: false,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.Name, func(t *testing.T) {
+			node := &apiv1.Node{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: test.NodeName,
+				},
+			}
+
+			reason, skip := skipTaintingGridscaleNode(node)
+
+			if test.ExpectsSkip {
+				assert.True(t, skip, "Node should be skipped")
+				assert.NotEmpty(t, reason, "Skipping nodes must return a reason")
+			} else {
+				assert.False(t, skip, "Node should not be skipped")
+				assert.Empty(t, reason, "Non-skipped nodes should not return a reason")
+			}
+		})
+	}
+}