Merge pull request #3161 from detiber/fixCAPIAnnotations

Update group identifier to use for Cluster API annotations

cluster-autoscaler/cloudprovider/clusterapi/README.md

@@ -101,12 +101,12 @@ resources depending on the type of cluster-api mechanism that you are using.
 
 There are two annotations that control how a cluster resource should be scaled:
 
-* `cluster.k8s.io/cluster-api-autoscaler-node-group-min-size` - This specifies
+* `cluster.x-k8s.io/cluster-api-autoscaler-node-group-min-size` - This specifies
   the minimum number of nodes for the associated resource group. The autoscaler
   will not scale the group below this number. Please note that currently the
   cluster-api provider will not scale down to zero nodes.
 
-* `cluster.k8s.io/cluster-api-autoscaler-node-group-max-size` - This specifies
+* `cluster.x-k8s.io/cluster-api-autoscaler-node-group-max-size` - This specifies
   the maximum number of nodes for the associated resource group. The autoscaler
   will not scale the group above this number.
 

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller.go

@@ -258,7 +258,12 @@ func (c *machineController) findMachineByProviderID(providerID normalizedProvide
 	if node == nil {
 		return nil, nil
 	}
-	return c.findMachine(node.Annotations[machineAnnotationKey])
+
+	machineID, ok := node.Annotations[machineAnnotationKey]
+	if !ok {
+		machineID = node.Annotations[deprecatedMachineAnnotationKey]
+	}
+	return c.findMachine(machineID)
 }
 
 func isFailedMachineProviderID(providerID normalizedProviderID) bool {

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller_test.go

@@ -486,24 +486,32 @@ func deleteTestConfigs(t *testing.T, controller *machineController, testConfigs
 
 func TestControllerFindMachine(t *testing.T) {
 	type testCase struct {
-		description    string
-		name           string
-		namespace      string
-		lookupSucceeds bool
+		description             string
+		name                    string
+		namespace               string
+		useDeprecatedAnnotation bool
+		lookupSucceeds          bool
 	}
 
 	var testCases = []testCase{{
-		description:    "lookup fails",
-		lookupSucceeds: false,
-		name:           "machine-does-not-exist",
-		namespace:      "namespace-does-not-exist",
+		description:             "lookup fails",
+		lookupSucceeds:          false,
+		useDeprecatedAnnotation: false,
+		name:                    "machine-does-not-exist",
+		namespace:               "namespace-does-not-exist",
 	}, {
-		description:    "lookup fails in valid namespace",
-		lookupSucceeds: false,
-		name:           "machine-does-not-exist-in-existing-namespace",
+		description:             "lookup fails in valid namespace",
+		lookupSucceeds:          false,
+		useDeprecatedAnnotation: false,
+		name:                    "machine-does-not-exist-in-existing-namespace",
 	}, {
-		description:    "lookup succeeds",
-		lookupSucceeds: true,
+		description:             "lookup succeeds",
+		lookupSucceeds:          true,
+		useDeprecatedAnnotation: false,
+	}, {
+		description:             "lookup succeeds with deprecated annotation",
+		lookupSucceeds:          true,
+		useDeprecatedAnnotation: true,
 	}}
 
 	test := func(t *testing.T, tc testCase, testConfig *testConfig) {
@@ -541,6 +549,19 @@ func TestControllerFindMachine(t *testing.T) {
 			if tc.namespace == "" {
 				tc.namespace = testConfig.machines[0].GetNamespace()
 			}
+			if tc.useDeprecatedAnnotation {
+				for i := range testConfig.machines {
+					n := testConfig.nodes[i]
+					annotations := n.GetAnnotations()
+					val, ok := annotations[machineAnnotationKey]
+					if !ok {
+						t.Fatal("node did not contain machineAnnotationKey")
+					}
+					delete(annotations, machineAnnotationKey)
+					annotations[deprecatedMachineAnnotationKey] = val
+					n.SetAnnotations(annotations)
+				}
+			}
 			test(t, tc, testConfig)
 		})
 	}

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup.go

@@ -27,9 +27,13 @@ import (
 )
 
 const (
-	machineDeleteAnnotationKey = "cluster.k8s.io/delete-machine"
-	machineAnnotationKey       = "cluster.k8s.io/machine"
-	debugFormat                = "%s (min: %d, max: %d, replicas: %d)"
+	// deprecatedMachineDeleteAnnotationKey should not be removed until minimum cluster-api support is v1alpha3
+	deprecatedMachineDeleteAnnotationKey = "cluster.k8s.io/delete-machine"
+	// TODO: determine what currently relies on deprecatedMachineAnnotationKey to determine when it can be removed
+	deprecatedMachineAnnotationKey = "cluster.k8s.io/machine"
+	machineDeleteAnnotationKey     = "cluster.x-k8s.io/delete-machine"
+	machineAnnotationKey           = "cluster.x-k8s.io/machine"
+	debugFormat                    = "%s (min: %d, max: %d, replicas: %d)"
 )
 
 type nodegroup struct {

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup_test.go

@@ -649,6 +649,9 @@ func TestNodeGroupDeleteNodes(t *testing.T) {
 			if _, found := machine.GetAnnotations()[machineDeleteAnnotationKey]; !found {
 				t.Errorf("expected annotation %q on machine %s", machineDeleteAnnotationKey, machine.GetName())
 			}
+			if _, found := machine.GetAnnotations()[deprecatedMachineDeleteAnnotationKey]; !found {
+				t.Errorf("expected annotation %q on machine %s", deprecatedMachineDeleteAnnotationKey, machine.GetName())
+			}
 		}
 
 		gvr, err := ng.scalableResource.GroupVersionResource()

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_unstructured.go

@@ -129,15 +129,12 @@ func (r unstructuredScalableResource) UnmarkMachineForDeletion(machine *unstruct
 	}
 
 	annotations := u.GetAnnotations()
-	if _, ok := annotations[machineDeleteAnnotationKey]; ok {
-		delete(annotations, machineDeleteAnnotationKey)
-		u.SetAnnotations(annotations)
-		_, updateErr := r.controller.managementClient.Resource(r.controller.machineResource).Namespace(u.GetNamespace()).Update(context.TODO(), u, metav1.UpdateOptions{})
-
-		return updateErr
-	}
+	delete(annotations, machineDeleteAnnotationKey)
+	delete(annotations, deprecatedMachineDeleteAnnotationKey)
+	u.SetAnnotations(annotations)
+	_, updateErr := r.controller.managementClient.Resource(r.controller.machineResource).Namespace(u.GetNamespace()).Update(context.TODO(), u, metav1.UpdateOptions{})
 
-	return nil
+	return updateErr
 }
 
 func (r unstructuredScalableResource) MarkMachineForDeletion(machine *unstructured.Unstructured) error {
@@ -154,6 +151,7 @@ func (r unstructuredScalableResource) MarkMachineForDeletion(machine *unstructur
 	}
 
 	annotations[machineDeleteAnnotationKey] = time.Now().String()
+	annotations[deprecatedMachineDeleteAnnotationKey] = time.Now().String()
 	u.SetAnnotations(annotations)
 
 	_, updateErr := r.controller.managementClient.Resource(r.controller.machineResource).Namespace(u.GetNamespace()).Update(context.TODO(), u, metav1.UpdateOptions{})

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_utils.go

@@ -26,10 +26,12 @@ import (
 )
 
 const (
-	nodeGroupMinSizeAnnotationKey = "cluster.k8s.io/cluster-api-autoscaler-node-group-min-size"
-	nodeGroupMaxSizeAnnotationKey = "cluster.k8s.io/cluster-api-autoscaler-node-group-max-size"
-	clusterNameLabel              = "cluster.x-k8s.io/cluster-name"
-	deprecatedClusterNameLabel    = "cluster.k8s.io/cluster-name"
+	deprecatedNodeGroupMinSizeAnnotationKey = "cluster.k8s.io/cluster-api-autoscaler-node-group-min-size"
+	deprecatedNodeGroupMaxSizeAnnotationKey = "cluster.k8s.io/cluster-api-autoscaler-node-group-max-size"
+	nodeGroupMinSizeAnnotationKey           = "cluster.x-k8s.io/cluster-api-autoscaler-node-group-min-size"
+	nodeGroupMaxSizeAnnotationKey           = "cluster.x-k8s.io/cluster-api-autoscaler-node-group-max-size"
+	clusterNameLabel                        = "cluster.x-k8s.io/cluster-name"
+	deprecatedClusterNameLabel              = "cluster.k8s.io/cluster-name"
 )
 
 var (
@@ -60,6 +62,9 @@ type normalizedProviderID string
 // value is not of type int.
 func minSize(annotations map[string]string) (int, error) {
 	val, found := annotations[nodeGroupMinSizeAnnotationKey]
+	if !found {
+		val, found = annotations[deprecatedNodeGroupMinSizeAnnotationKey]
+	}
 	if !found {
 		return 0, errMissingMinAnnotation
 	}
@@ -76,6 +81,9 @@ func minSize(annotations map[string]string) (int, error) {
 // value is not of type int.
 func maxSize(annotations map[string]string) (int, error) {
 	val, found := annotations[nodeGroupMaxSizeAnnotationKey]
+	if !found {
+		val, found = annotations[deprecatedNodeGroupMaxSizeAnnotationKey]
+	}
 	if !found {
 		return 0, errMissingMaxAnnotation
 	}

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_utils_test.go

@@ -112,6 +112,24 @@ func TestUtilParseScalingBounds(t *testing.T) {
 		},
 		min: 0,
 		max: 1,
+	}, {
+		description: "deprecated min/max annotations still work, result is min 0, max 1",
+		annotations: map[string]string{
+			deprecatedNodeGroupMinSizeAnnotationKey: "0",
+			deprecatedNodeGroupMaxSizeAnnotationKey: "1",
+		},
+		min: 0,
+		max: 1,
+	}, {
+		description: "deprecated min/max annotations do not take precedence over non-deprecated annotations, result is min 1, max 2",
+		annotations: map[string]string{
+			deprecatedNodeGroupMinSizeAnnotationKey: "0",
+			deprecatedNodeGroupMaxSizeAnnotationKey: "1",
+			nodeGroupMinSizeAnnotationKey:           "1",
+			nodeGroupMaxSizeAnnotationKey:           "2",
+		},
+		min: 1,
+		max: 2,
 	}} {
 		t.Run(tc.description, func(t *testing.T) {
 			machineSet := unstructured.Unstructured{