From efd6f4aec66b7b70665a92d7ead8d550a3285758 Mon Sep 17 00:00:00 2001 From: Jason DeTiberus Date: Thu, 28 May 2020 16:09:36 -0400 Subject: [PATCH] Update group identifier to use for Cluster API annotations - Also add backwards compatibility for the previously used deprecated annotations --- .../cloudprovider/clusterapi/README.md | 4 +- .../clusterapi/clusterapi_controller.go | 7 +- .../clusterapi/clusterapi_controller_test.go | 292 ++++++++++++------ .../clusterapi_machinedeployment.go | 1 + .../clusterapi/clusterapi_machineset.go | 1 + .../clusterapi/clusterapi_nodegroup.go | 10 +- .../clusterapi/clusterapi_nodegroup_test.go | 111 ++++--- .../clusterapi/clusterapi_utils.go | 12 +- .../clusterapi/clusterapi_utils_test.go | 18 ++ 9 files changed, 309 insertions(+), 147 deletions(-) diff --git a/cluster-autoscaler/cloudprovider/clusterapi/README.md b/cluster-autoscaler/cloudprovider/clusterapi/README.md index 944ff3bb8b02..f41437750dda 100644 --- a/cluster-autoscaler/cloudprovider/clusterapi/README.md +++ b/cluster-autoscaler/cloudprovider/clusterapi/README.md @@ -33,12 +33,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. diff --git a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller.go b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller.go index 507682e210d2..42c3ef4c8c15 100644 --- a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller.go +++ b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller.go @@ -245,7 +245,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 { diff --git a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller_test.go b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller_test.go index 3269a39bed2c..de710cf58681 100644 --- a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller_test.go +++ b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller_test.go @@ -130,20 +130,20 @@ func mustCreateTestController(t *testing.T, testConfigs ...*testConfig) (*machin } } -func createMachineSetTestConfig(namespace string, nodeCount int, annotations map[string]string) *testConfig { - return createTestConfigs(createTestSpecs(namespace, 1, nodeCount, false, annotations)...)[0] +func createMachineSetTestConfig(namespace string, nodeCount int, annotations map[string]string, useDeprecatedAnnotation bool) *testConfig { + return createTestConfigs(useDeprecatedAnnotation, createTestSpecs(namespace, 1, nodeCount, false, annotations)...)[0] } -func createMachineSetTestConfigs(namespace string, configCount, nodeCount int, annotations map[string]string) []*testConfig { - return createTestConfigs(createTestSpecs(namespace, configCount, nodeCount, false, annotations)...) +func createMachineSetTestConfigs(namespace string, configCount, nodeCount int, annotations map[string]string, useDeprecatedAnnotation bool) []*testConfig { + return createTestConfigs(useDeprecatedAnnotation, createTestSpecs(namespace, configCount, nodeCount, false, annotations)...) } -func createMachineDeploymentTestConfig(namespace string, nodeCount int, annotations map[string]string) *testConfig { - return createTestConfigs(createTestSpecs(namespace, 1, nodeCount, true, annotations)...)[0] +func createMachineDeploymentTestConfig(namespace string, nodeCount int, annotations map[string]string, useDeprecatedAnnotation bool) *testConfig { + return createTestConfigs(useDeprecatedAnnotation, createTestSpecs(namespace, 1, nodeCount, true, annotations)...)[0] } -func createMachineDeploymentTestConfigs(namespace string, configCount, nodeCount int, annotations map[string]string) []*testConfig { - return createTestConfigs(createTestSpecs(namespace, configCount, nodeCount, true, annotations)...) +func createMachineDeploymentTestConfigs(namespace string, configCount, nodeCount int, annotations map[string]string, useDeprecatedAnnotation bool) []*testConfig { + return createTestConfigs(useDeprecatedAnnotation, createTestSpecs(namespace, configCount, nodeCount, true, annotations)...) } func createTestSpecs(namespace string, scalableResourceCount, nodeCount int, isMachineDeployment bool, annotations map[string]string) []testSpec { @@ -163,7 +163,7 @@ func createTestSpecs(namespace string, scalableResourceCount, nodeCount int, isM return specs } -func createTestConfigs(specs ...testSpec) []*testConfig { +func createTestConfigs(useDeprecatedAnnotation bool, specs ...testSpec) []*testConfig { var result []*testConfig for i, spec := range specs { @@ -220,7 +220,7 @@ func createTestConfigs(specs ...testSpec) []*testConfig { } for j := 0; j < spec.nodeCount; j++ { - config.nodes[j], config.machines[j] = makeLinkedNodeAndMachine(j, spec.namespace, machineOwner) + config.nodes[j], config.machines[j] = makeLinkedNodeAndMachine(j, spec.namespace, machineOwner, useDeprecatedAnnotation) } result = append(result, config) @@ -232,22 +232,26 @@ func createTestConfigs(specs ...testSpec) []*testConfig { // makeLinkedNodeAndMachine creates a node and machine. The machine // has its NodeRef set to the new node and the new machine's owner // reference is set to owner. -func makeLinkedNodeAndMachine(i int, namespace string, owner v1.OwnerReference) (*corev1.Node, *Machine) { +func makeLinkedNodeAndMachine(i int, namespace string, owner v1.OwnerReference, useDeprecatedAnnotation bool) (*corev1.Node, *Machine) { node := &corev1.Node{ TypeMeta: v1.TypeMeta{ Kind: "Node", }, ObjectMeta: v1.ObjectMeta{ - Name: fmt.Sprintf("%s-%s-node-%d", namespace, owner.Name, i), - Annotations: map[string]string{ - machineAnnotationKey: fmt.Sprintf("%s/%s-%s-machine-%d", namespace, namespace, owner.Name, i), - }, + Name: fmt.Sprintf("%s-%s-node-%d", namespace, owner.Name, i), + Annotations: map[string]string{}, }, Spec: corev1.NodeSpec{ ProviderID: fmt.Sprintf("test:////%s-%s-nodeid-%d", namespace, owner.Name, i), }, } + annotationKey := machineAnnotationKey + if useDeprecatedAnnotation { + annotationKey = deprecatedMachineAnnotationKey + } + node.Annotations[annotationKey] = fmt.Sprintf("%s/%s-%s-machine-%d", namespace, namespace, owner.Name, i) + machine := &Machine{ TypeMeta: v1.TypeMeta{ APIVersion: fmt.Sprintf("%s/v1alpha3", defaultCAPIGroup), @@ -335,24 +339,32 @@ func deleteTestConfigs(t *testing.T, controller *machineController, testConfigs func TestControllerFindMachineByID(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) { @@ -380,10 +392,15 @@ func TestControllerFindMachineByID(t *testing.T) { for _, tc := range testCases { t.Run(tc.description, func(t *testing.T) { - testConfig := createMachineSetTestConfig(testNamespace, 1, map[string]string{ - nodeGroupMinSizeAnnotationKey: "1", - nodeGroupMaxSizeAnnotationKey: "10", - }) + testConfig := createMachineSetTestConfig( + testNamespace, + 1, + map[string]string{ + nodeGroupMinSizeAnnotationKey: "1", + nodeGroupMaxSizeAnnotationKey: "10", + }, + tc.useDeprecatedAnnotation, + ) if tc.name == "" { tc.name = testConfig.machines[0].Name } @@ -396,10 +413,15 @@ func TestControllerFindMachineByID(t *testing.T) { } func TestControllerFindMachineOwner(t *testing.T) { - testConfig := createMachineSetTestConfig(testNamespace, 1, map[string]string{ - nodeGroupMinSizeAnnotationKey: "1", - nodeGroupMaxSizeAnnotationKey: "10", - }) + testConfig := createMachineSetTestConfig( + testNamespace, + 1, + map[string]string{ + nodeGroupMinSizeAnnotationKey: "1", + nodeGroupMaxSizeAnnotationKey: "10", + }, + false, + ) controller, stop := mustCreateTestController(t, testConfig) defer stop() @@ -441,10 +463,15 @@ func TestControllerFindMachineOwner(t *testing.T) { } func TestControllerFindMachineByProviderID(t *testing.T) { - testConfig := createMachineSetTestConfig(testNamespace, 1, map[string]string{ - nodeGroupMinSizeAnnotationKey: "1", - nodeGroupMaxSizeAnnotationKey: "10", - }) + testConfig := createMachineSetTestConfig( + testNamespace, + 1, + map[string]string{ + nodeGroupMinSizeAnnotationKey: "1", + nodeGroupMaxSizeAnnotationKey: "10", + }, + false, + ) controller, stop := mustCreateTestController(t, testConfig) defer stop() @@ -499,10 +526,15 @@ func TestControllerFindMachineByProviderID(t *testing.T) { } func TestControllerFindNodeByNodeName(t *testing.T) { - testConfig := createMachineSetTestConfig(testNamespace, 1, map[string]string{ - nodeGroupMinSizeAnnotationKey: "1", - nodeGroupMaxSizeAnnotationKey: "10", - }) + testConfig := createMachineSetTestConfig( + testNamespace, + 1, + map[string]string{ + nodeGroupMinSizeAnnotationKey: "1", + nodeGroupMaxSizeAnnotationKey: "10", + }, + false, + ) controller, stop := mustCreateTestController(t, testConfig) defer stop() @@ -527,10 +559,15 @@ func TestControllerFindNodeByNodeName(t *testing.T) { } func TestControllerMachinesInMachineSet(t *testing.T) { - testConfig1 := createMachineSetTestConfig("testConfig1", 5, map[string]string{ - nodeGroupMinSizeAnnotationKey: "1", - nodeGroupMaxSizeAnnotationKey: "10", - }) + testConfig1 := createMachineSetTestConfig( + "testConfig1", + 5, + map[string]string{ + nodeGroupMinSizeAnnotationKey: "1", + nodeGroupMaxSizeAnnotationKey: "10", + }, + false, + ) controller, stop := mustCreateTestController(t, testConfig1) defer stop() @@ -539,10 +576,15 @@ func TestControllerMachinesInMachineSet(t *testing.T) { // nodes and the additional machineset to the existing set of // test objects in the controller. This gives us two // machinesets, each with their own machines and linked nodes. - testConfig2 := createMachineSetTestConfig("testConfig2", 5, map[string]string{ - nodeGroupMinSizeAnnotationKey: "1", - nodeGroupMaxSizeAnnotationKey: "10", - }) + testConfig2 := createMachineSetTestConfig( + "testConfig2", + 5, + map[string]string{ + nodeGroupMinSizeAnnotationKey: "1", + nodeGroupMaxSizeAnnotationKey: "10", + }, + false, + ) if err := addTestConfigs(t, controller, testConfig2); err != nil { t.Fatalf("unexpected error: %v", err) @@ -600,10 +642,15 @@ func TestControllerMachinesInMachineSet(t *testing.T) { } func TestControllerLookupNodeGroupForNonExistentNode(t *testing.T) { - testConfig := createMachineSetTestConfig(testNamespace, 1, map[string]string{ - nodeGroupMinSizeAnnotationKey: "1", - nodeGroupMaxSizeAnnotationKey: "10", - }) + testConfig := createMachineSetTestConfig( + testNamespace, + 1, + map[string]string{ + nodeGroupMinSizeAnnotationKey: "1", + nodeGroupMaxSizeAnnotationKey: "10", + }, + false, + ) controller, stop := mustCreateTestController(t, testConfig) defer stop() @@ -646,18 +693,28 @@ func TestControllerNodeGroupForNodeWithMissingMachineOwner(t *testing.T) { } t.Run("MachineSet", func(t *testing.T) { - testConfig := createMachineSetTestConfig(testNamespace, 1, map[string]string{ - nodeGroupMinSizeAnnotationKey: "1", - nodeGroupMaxSizeAnnotationKey: "10", - }) + testConfig := createMachineSetTestConfig( + testNamespace, + 1, + map[string]string{ + nodeGroupMinSizeAnnotationKey: "1", + nodeGroupMaxSizeAnnotationKey: "10", + }, + false, + ) test(t, testConfig) }) t.Run("MachineDeployment", func(t *testing.T) { - testConfig := createMachineDeploymentTestConfig(testNamespace, 1, map[string]string{ - nodeGroupMinSizeAnnotationKey: "1", - nodeGroupMaxSizeAnnotationKey: "10", - }) + testConfig := createMachineDeploymentTestConfig( + testNamespace, + 1, + map[string]string{ + nodeGroupMinSizeAnnotationKey: "1", + nodeGroupMaxSizeAnnotationKey: "10", + }, + false, + ) test(t, testConfig) }) } @@ -679,18 +736,28 @@ func TestControllerNodeGroupForNodeWithPositiveScalingBounds(t *testing.T) { } t.Run("MachineSet", func(t *testing.T) { - testConfig := createMachineSetTestConfig(testNamespace, 1, map[string]string{ - nodeGroupMinSizeAnnotationKey: "1", - nodeGroupMaxSizeAnnotationKey: "1", - }) + testConfig := createMachineSetTestConfig( + testNamespace, + 1, + map[string]string{ + nodeGroupMinSizeAnnotationKey: "1", + nodeGroupMaxSizeAnnotationKey: "1", + }, + false, + ) test(t, testConfig) }) t.Run("MachineDeployment", func(t *testing.T) { - testConfig := createMachineDeploymentTestConfig(testNamespace, 1, map[string]string{ - nodeGroupMinSizeAnnotationKey: "1", - nodeGroupMaxSizeAnnotationKey: "1", - }) + testConfig := createMachineDeploymentTestConfig( + testNamespace, + 1, + map[string]string{ + nodeGroupMinSizeAnnotationKey: "1", + nodeGroupMaxSizeAnnotationKey: "1", + }, + false, + ) test(t, testConfig) }) } @@ -719,14 +786,14 @@ func TestControllerNodeGroups(t *testing.T) { assertNodegroupLen(t, controller, 0) // Test #2: add 5 machineset-based nodegroups - machineSetConfigs := createMachineSetTestConfigs("MachineSet", 5, 1, annotations) + machineSetConfigs := createMachineSetTestConfigs("MachineSet", 5, 1, annotations, false) if err := addTestConfigs(t, controller, machineSetConfigs...); err != nil { t.Fatalf("unexpected error: %v", err) } assertNodegroupLen(t, controller, 5) // Test #2: add 2 machinedeployment-based nodegroups - machineDeploymentConfigs := createMachineDeploymentTestConfigs("MachineDeployment", 2, 1, annotations) + machineDeploymentConfigs := createMachineDeploymentTestConfigs("MachineDeployment", 2, 1, annotations, false) if err := addTestConfigs(t, controller, machineDeploymentConfigs...); err != nil { t.Fatalf("unexpected error: %v", err) } @@ -750,14 +817,14 @@ func TestControllerNodeGroups(t *testing.T) { } // Test #5: machineset with no scaling bounds results in no nodegroups - machineSetConfigs = createMachineSetTestConfigs("MachineSet", 5, 1, annotations) + machineSetConfigs = createMachineSetTestConfigs("MachineSet", 5, 1, annotations, false) if err := addTestConfigs(t, controller, machineSetConfigs...); err != nil { t.Fatalf("unexpected error: %v", err) } assertNodegroupLen(t, controller, 0) // Test #6: machinedeployment with no scaling bounds results in no nodegroups - machineDeploymentConfigs = createMachineDeploymentTestConfigs("MachineDeployment", 2, 1, annotations) + machineDeploymentConfigs = createMachineDeploymentTestConfigs("MachineDeployment", 2, 1, annotations, false) if err := addTestConfigs(t, controller, machineDeploymentConfigs...); err != nil { t.Fatalf("unexpected error: %v", err) } @@ -769,7 +836,7 @@ func TestControllerNodeGroups(t *testing.T) { } // Test #7: machineset with bad scaling bounds results in an error and no nodegroups - machineSetConfigs = createMachineSetTestConfigs("MachineSet", 5, 1, annotations) + machineSetConfigs = createMachineSetTestConfigs("MachineSet", 5, 1, annotations, false) if err := addTestConfigs(t, controller, machineSetConfigs...); err != nil { t.Fatalf("unexpected error: %v", err) } @@ -778,7 +845,7 @@ func TestControllerNodeGroups(t *testing.T) { } // Test #8: machinedeployment with bad scaling bounds results in an error and no nodegroups - machineDeploymentConfigs = createMachineDeploymentTestConfigs("MachineDeployment", 2, 1, annotations) + machineDeploymentConfigs = createMachineDeploymentTestConfigs("MachineDeployment", 2, 1, annotations, false) if err := addTestConfigs(t, controller, machineDeploymentConfigs...); err != nil { t.Fatalf("unexpected error: %v", err) } @@ -842,22 +909,27 @@ func TestControllerNodeGroupsNodeCount(t *testing.T) { t.Run("MachineSet", func(t *testing.T) { for _, tc := range testCases { - test(t, tc, createMachineSetTestConfigs(testNamespace, tc.nodeGroups, tc.nodesPerGroup, annotations)) + test(t, tc, createMachineSetTestConfigs(testNamespace, tc.nodeGroups, tc.nodesPerGroup, annotations, false)) } }) t.Run("MachineDeployment", func(t *testing.T) { for _, tc := range testCases { - test(t, tc, createMachineDeploymentTestConfigs(testNamespace, tc.nodeGroups, tc.nodesPerGroup, annotations)) + test(t, tc, createMachineDeploymentTestConfigs(testNamespace, tc.nodeGroups, tc.nodesPerGroup, annotations, false)) } }) } func TestControllerFindMachineFromNodeAnnotation(t *testing.T) { - testConfig := createMachineSetTestConfig(testNamespace, 1, map[string]string{ - nodeGroupMinSizeAnnotationKey: "1", - nodeGroupMaxSizeAnnotationKey: "10", - }) + testConfig := createMachineSetTestConfig( + testNamespace, + 1, + map[string]string{ + nodeGroupMinSizeAnnotationKey: "1", + nodeGroupMaxSizeAnnotationKey: "10", + }, + false, + ) controller, stop := mustCreateTestController(t, testConfig) defer stop() @@ -901,10 +973,15 @@ func TestControllerFindMachineFromNodeAnnotation(t *testing.T) { } func TestControllerMachineSetNodeNamesWithoutLinkage(t *testing.T) { - testConfig := createMachineSetTestConfig(testNamespace, 3, map[string]string{ - nodeGroupMinSizeAnnotationKey: "1", - nodeGroupMaxSizeAnnotationKey: "10", - }) + testConfig := createMachineSetTestConfig( + testNamespace, + 3, + map[string]string{ + nodeGroupMinSizeAnnotationKey: "1", + nodeGroupMaxSizeAnnotationKey: "10", + }, + false, + ) controller, stop := mustCreateTestController(t, testConfig) defer stop() @@ -945,10 +1022,15 @@ func TestControllerMachineSetNodeNamesWithoutLinkage(t *testing.T) { } func TestControllerMachineSetNodeNamesUsingProviderID(t *testing.T) { - testConfig := createMachineSetTestConfig(testNamespace, 3, map[string]string{ - nodeGroupMinSizeAnnotationKey: "1", - nodeGroupMaxSizeAnnotationKey: "10", - }) + testConfig := createMachineSetTestConfig( + testNamespace, + 3, + map[string]string{ + nodeGroupMinSizeAnnotationKey: "1", + nodeGroupMaxSizeAnnotationKey: "10", + }, + false, + ) controller, stop := mustCreateTestController(t, testConfig) defer stop() @@ -994,10 +1076,15 @@ func TestControllerMachineSetNodeNamesUsingProviderID(t *testing.T) { } func TestControllerMachineSetNodeNamesUsingStatusNodeRefName(t *testing.T) { - testConfig := createMachineSetTestConfig(testNamespace, 3, map[string]string{ - nodeGroupMinSizeAnnotationKey: "1", - nodeGroupMaxSizeAnnotationKey: "10", - }) + testConfig := createMachineSetTestConfig( + testNamespace, + 3, + map[string]string{ + nodeGroupMinSizeAnnotationKey: "1", + nodeGroupMaxSizeAnnotationKey: "10", + }, + false, + ) controller, stop := mustCreateTestController(t, testConfig) defer stop() @@ -1062,10 +1149,15 @@ func TestControllerGetAPIVersionGroup(t *testing.T) { } func TestControllerGetAPIVersionGroupWithMachineDeployments(t *testing.T) { - testConfig := createMachineDeploymentTestConfig(testNamespace, 1, map[string]string{ - nodeGroupMinSizeAnnotationKey: "1", - nodeGroupMaxSizeAnnotationKey: "1", - }) + testConfig := createMachineDeploymentTestConfig( + testNamespace, + 1, + map[string]string{ + nodeGroupMinSizeAnnotationKey: "1", + nodeGroupMaxSizeAnnotationKey: "1", + }, + false, + ) if err := os.Setenv(CAPIGroupEnvVar, customCAPIGroup); err != nil { t.Fatalf("unexpected error: %v", err) } diff --git a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_machinedeployment.go b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_machinedeployment.go index 100c1ff6bbf8..b25706fa728c 100644 --- a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_machinedeployment.go +++ b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_machinedeployment.go @@ -116,6 +116,7 @@ func (r machineDeploymentScalableResource) MarkMachineForDeletion(machine *Machi annotations = map[string]string{} } annotations[machineDeleteAnnotationKey] = time.Now().String() + annotations[deprecatedMachineDeleteAnnotationKey] = time.Now().String() u.SetAnnotations(annotations) _, updateErr := r.controller.dynamicclient.Resource(*r.controller.machineResource).Namespace(u.GetNamespace()).Update(context.TODO(), u, metav1.UpdateOptions{}) diff --git a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_machineset.go b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_machineset.go index 98655bfaf3b5..bfd41fef9208 100644 --- a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_machineset.go +++ b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_machineset.go @@ -101,6 +101,7 @@ func (r machineSetScalableResource) MarkMachineForDeletion(machine *Machine) err annotations = map[string]string{} } annotations[machineDeleteAnnotationKey] = time.Now().String() + annotations[deprecatedMachineDeleteAnnotationKey] = time.Now().String() u.SetAnnotations(annotations) _, updateErr := r.controller.dynamicclient.Resource(*r.controller.machineResource).Namespace(u.GetNamespace()).Update(context.TODO(), u, metav1.UpdateOptions{}) diff --git a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup.go b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup.go index bfc7499fdd4f..00e7bb628488 100644 --- a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup.go +++ b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup.go @@ -25,9 +25,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 { diff --git a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup_test.go b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup_test.go index 74da6b97f182..3cb5f4e75411 100644 --- a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup_test.go +++ b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup_test.go @@ -194,7 +194,7 @@ func TestNodeGroupNewNodeGroupConstructor(t *testing.T) { t.Run("MachineSet", func(t *testing.T) { for _, tc := range testCases { t.Run(tc.description, func(t *testing.T) { - test(t, tc, createMachineSetTestConfig(testNamespace, tc.nodeCount, tc.annotations)) + test(t, tc, createMachineSetTestConfig(testNamespace, tc.nodeCount, tc.annotations, false)) }) } }) @@ -202,7 +202,7 @@ func TestNodeGroupNewNodeGroupConstructor(t *testing.T) { t.Run("MachineDeployment", func(t *testing.T) { for _, tc := range testCases { t.Run(tc.description, func(t *testing.T) { - test(t, tc, createMachineDeploymentTestConfig(testNamespace, tc.nodeCount, tc.annotations)) + test(t, tc, createMachineDeploymentTestConfig(testNamespace, tc.nodeCount, tc.annotations, false)) }) } }) @@ -297,7 +297,7 @@ func TestNodeGroupIncreaseSizeErrors(t *testing.T) { nodeGroupMinSizeAnnotationKey: "1", nodeGroupMaxSizeAnnotationKey: "10", } - test(t, &tc, createMachineSetTestConfig(testNamespace, int(tc.initial), annotations)) + test(t, &tc, createMachineSetTestConfig(testNamespace, int(tc.initial), annotations, false)) }) } }) @@ -309,7 +309,7 @@ func TestNodeGroupIncreaseSizeErrors(t *testing.T) { nodeGroupMinSizeAnnotationKey: "1", nodeGroupMaxSizeAnnotationKey: "10", } - test(t, &tc, createMachineDeploymentTestConfig(testNamespace, int(tc.initial), annotations)) + test(t, &tc, createMachineDeploymentTestConfig(testNamespace, int(tc.initial), annotations, false)) }) } }) @@ -386,7 +386,7 @@ func TestNodeGroupIncreaseSize(t *testing.T) { expected: 4, delta: 1, } - test(t, &tc, createMachineSetTestConfig(testNamespace, int(tc.initial), annotations)) + test(t, &tc, createMachineSetTestConfig(testNamespace, int(tc.initial), annotations, false)) }) t.Run("MachineDeployment", func(t *testing.T) { @@ -396,7 +396,7 @@ func TestNodeGroupIncreaseSize(t *testing.T) { expected: 4, delta: 1, } - test(t, &tc, createMachineDeploymentTestConfig(testNamespace, int(tc.initial), annotations)) + test(t, &tc, createMachineDeploymentTestConfig(testNamespace, int(tc.initial), annotations, false)) }) } @@ -496,7 +496,7 @@ func TestNodeGroupDecreaseTargetSize(t *testing.T) { delta: -1, expectedError: true, } - test(t, &tc, createMachineSetTestConfig(testNamespace, int(tc.initial), annotations)) + test(t, &tc, createMachineSetTestConfig(testNamespace, int(tc.initial), annotations, false)) }) t.Run("MachineSet", func(t *testing.T) { @@ -507,7 +507,7 @@ func TestNodeGroupDecreaseTargetSize(t *testing.T) { expected: 3, delta: -1, } - test(t, &tc, createMachineSetTestConfig(testNamespace, int(tc.initial), annotations)) + test(t, &tc, createMachineSetTestConfig(testNamespace, int(tc.initial), annotations, false)) }) t.Run("MachineDeployment", func(t *testing.T) { @@ -519,7 +519,7 @@ func TestNodeGroupDecreaseTargetSize(t *testing.T) { delta: -1, expectedError: true, } - test(t, &tc, createMachineDeploymentTestConfig(testNamespace, int(tc.initial), annotations)) + test(t, &tc, createMachineDeploymentTestConfig(testNamespace, int(tc.initial), annotations, false)) }) } @@ -612,7 +612,7 @@ func TestNodeGroupDecreaseSizeErrors(t *testing.T) { nodeGroupMinSizeAnnotationKey: "1", nodeGroupMaxSizeAnnotationKey: "10", } - test(t, &tc, createMachineSetTestConfig(testNamespace, int(tc.initial), annotations)) + test(t, &tc, createMachineSetTestConfig(testNamespace, int(tc.initial), annotations, false)) }) } }) @@ -624,7 +624,7 @@ func TestNodeGroupDecreaseSizeErrors(t *testing.T) { nodeGroupMinSizeAnnotationKey: "1", nodeGroupMaxSizeAnnotationKey: "10", } - test(t, &tc, createMachineDeploymentTestConfig(testNamespace, int(tc.initial), annotations)) + test(t, &tc, createMachineDeploymentTestConfig(testNamespace, int(tc.initial), annotations, false)) }) } }) @@ -676,6 +676,9 @@ func TestNodeGroupDeleteNodes(t *testing.T) { if _, found := machine.Annotations[machineDeleteAnnotationKey]; !found { t.Errorf("expected annotation %q on machine %s", machineDeleteAnnotationKey, machine.Name) } + if _, found := machine.Annotations[deprecatedMachineDeleteAnnotationKey]; !found { + t.Errorf("expected annotation %q on machine %s", deprecatedMachineDeleteAnnotationKey, machine.Name) + } } switch v := (ng.scalableResource).(type) { @@ -706,17 +709,27 @@ func TestNodeGroupDeleteNodes(t *testing.T) { // sorting and the expected semantics in test() will fail. t.Run("MachineSet", func(t *testing.T) { - test(t, createMachineSetTestConfig(testNamespace, 10, map[string]string{ - nodeGroupMinSizeAnnotationKey: "1", - nodeGroupMaxSizeAnnotationKey: "10", - })) + test(t, createMachineSetTestConfig( + testNamespace, + 10, + map[string]string{ + nodeGroupMinSizeAnnotationKey: "1", + nodeGroupMaxSizeAnnotationKey: "10", + }, + false, + )) }) t.Run("MachineDeployment", func(t *testing.T) { - test(t, createMachineDeploymentTestConfig(testNamespace, 10, map[string]string{ - nodeGroupMinSizeAnnotationKey: "1", - nodeGroupMaxSizeAnnotationKey: "10", - })) + test(t, createMachineDeploymentTestConfig( + testNamespace, + 10, + map[string]string{ + nodeGroupMinSizeAnnotationKey: "1", + nodeGroupMaxSizeAnnotationKey: "10", + }, + false, + )) }) } @@ -793,14 +806,14 @@ func TestNodeGroupMachineSetDeleteNodesWithMismatchedNodes(t *testing.T) { } t.Run("MachineSet", func(t *testing.T) { - testConfig0 := createMachineSetTestConfigs(testNamespace+"0", 1, 2, annotations) - testConfig1 := createMachineSetTestConfigs(testNamespace+"1", 1, 2, annotations) + testConfig0 := createMachineSetTestConfigs(testNamespace+"0", 1, 2, annotations, false) + testConfig1 := createMachineSetTestConfigs(testNamespace+"1", 1, 2, annotations, false) test(t, 2, append(testConfig0, testConfig1...)) }) t.Run("MachineDeployment", func(t *testing.T) { - testConfig0 := createMachineDeploymentTestConfigs(testNamespace+"0", 1, 2, annotations) - testConfig1 := createMachineDeploymentTestConfigs(testNamespace+"1", 1, 2, annotations) + testConfig0 := createMachineDeploymentTestConfigs(testNamespace+"0", 1, 2, annotations, false) + testConfig1 := createMachineDeploymentTestConfigs(testNamespace+"1", 1, 2, annotations, false) test(t, 2, append(testConfig0, testConfig1...)) }) } @@ -930,17 +943,27 @@ func TestNodeGroupDeleteNodesTwice(t *testing.T) { // sorting and the expected semantics in test() will fail. t.Run("MachineSet", func(t *testing.T) { - test(t, createMachineSetTestConfig(testNamespace, 10, map[string]string{ - nodeGroupMinSizeAnnotationKey: "1", - nodeGroupMaxSizeAnnotationKey: "10", - })) + test(t, createMachineSetTestConfig( + testNamespace, + 10, + map[string]string{ + nodeGroupMinSizeAnnotationKey: "1", + nodeGroupMaxSizeAnnotationKey: "10", + }, + false, + )) }) t.Run("MachineDeployment", func(t *testing.T) { - test(t, createMachineDeploymentTestConfig(testNamespace, 10, map[string]string{ - nodeGroupMinSizeAnnotationKey: "1", - nodeGroupMaxSizeAnnotationKey: "10", - })) + test(t, createMachineDeploymentTestConfig( + testNamespace, + 10, + map[string]string{ + nodeGroupMinSizeAnnotationKey: "1", + nodeGroupMaxSizeAnnotationKey: "10", + }, + false, + )) }) } @@ -1010,16 +1033,26 @@ func TestNodeGroupWithFailedMachine(t *testing.T) { // sorting and the expected semantics in test() will fail. t.Run("MachineSet", func(t *testing.T) { - test(t, createMachineSetTestConfig(testNamespace, 10, map[string]string{ - nodeGroupMinSizeAnnotationKey: "1", - nodeGroupMaxSizeAnnotationKey: "10", - })) + test(t, createMachineSetTestConfig( + testNamespace, + 10, + map[string]string{ + nodeGroupMinSizeAnnotationKey: "1", + nodeGroupMaxSizeAnnotationKey: "10", + }, + false, + )) }) t.Run("MachineDeployment", func(t *testing.T) { - test(t, createMachineDeploymentTestConfig(testNamespace, 10, map[string]string{ - nodeGroupMinSizeAnnotationKey: "1", - nodeGroupMaxSizeAnnotationKey: "10", - })) + test(t, createMachineDeploymentTestConfig( + testNamespace, + 10, + map[string]string{ + nodeGroupMinSizeAnnotationKey: "1", + nodeGroupMaxSizeAnnotationKey: "10", + }, + false, + )) }) } diff --git a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_utils.go b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_utils.go index 0e0f00151b0c..92a08d2fdcdd 100644 --- a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_utils.go +++ b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_utils.go @@ -25,8 +25,10 @@ import ( ) const ( - nodeGroupMinSizeAnnotationKey = "cluster.k8s.io/cluster-api-autoscaler-node-group-min-size" - nodeGroupMaxSizeAnnotationKey = "cluster.k8s.io/cluster-api-autoscaler-node-group-max-size" + 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" ) var ( @@ -57,6 +59,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 } @@ -73,6 +78,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 } diff --git a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_utils_test.go b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_utils_test.go index 41e852ab75fb..cff97052ccf1 100644 --- a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_utils_test.go +++ b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_utils_test.go @@ -110,6 +110,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 := MachineSet{