From 99572119c8fed920aa03e31313df3b01eda92eda Mon Sep 17 00:00:00 2001 From: Jason DeTiberus Date: Mon, 21 Sep 2020 10:42:46 -0400 Subject: [PATCH] UPSTREAM: : openshift: 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 | 47 ++++++++++++++----- .../clusterapi/clusterapi_nodegroup.go | 5 ++ .../clusterapi/clusterapi_nodegroup_test.go | 3 ++ .../clusterapi/clusterapi_unstructured.go | 14 +++--- .../clusterapi/clusterapi_utils.go | 16 +++++-- .../clusterapi/clusterapi_utils_test.go | 18 +++++++ 8 files changed, 86 insertions(+), 28 deletions(-) diff --git a/cluster-autoscaler/cloudprovider/clusterapi/README.md b/cluster-autoscaler/cloudprovider/clusterapi/README.md index ef0277f00228..50d686aa681b 100644 --- a/cluster-autoscaler/cloudprovider/clusterapi/README.md +++ b/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. diff --git a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller.go b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller.go index 1bffec3e372d..49af685abef1 100644 --- a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller.go +++ b/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 { diff --git a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller_test.go b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller_test.go index 6a841e42b908..fea1f2a15de8 100644 --- a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller_test.go +++ b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller_test.go @@ -514,24 +514,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) { @@ -570,6 +578,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) }) } diff --git a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup.go b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup.go index 4fa942e2ab01..615bd9be9473 100644 --- a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup.go +++ b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup.go @@ -39,6 +39,11 @@ const ( // This default for the maximum number of pods comes from the machine-config-operator // see https://github.com/openshift/machine-config-operator/blob/2f1bd6d99131fa4471ed95543a51dec3d5922b2b/templates/worker/01-worker-kubelet/_base/files/kubelet.yaml#L19 defaultMaxPods = 250 + + // 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" ) type nodegroup struct { diff --git a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup_test.go b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup_test.go index fcfb2b3ceea6..82b6644a2a9c 100644 --- a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup_test.go +++ b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup_test.go @@ -648,6 +648,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() diff --git a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_unstructured.go b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_unstructured.go index 166eb97fe979..ec283497f701 100644 --- a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_unstructured.go +++ b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_unstructured.go @@ -131,15 +131,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 { @@ -156,6 +153,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{}) diff --git a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_utils.go b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_utils.go index c4cbed381368..c1ceeba70fd8 100644 --- a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_utils.go +++ b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_utils.go @@ -29,10 +29,12 @@ import ( ) const ( - nodeGroupMinSizeAnnotationKey = "machine.openshift.io/cluster-api-autoscaler-node-group-min-size" - nodeGroupMaxSizeAnnotationKey = "machine.openshift.io/cluster-api-autoscaler-node-group-max-size" - clusterNameLabel = "machine.openshift.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 = "machine.openshift.io/cluster-api-autoscaler-node-group-min-size" + nodeGroupMaxSizeAnnotationKey = "machine.openshift.io/cluster-api-autoscaler-node-group-max-size" + clusterNameLabel = "machine.openshift.io/cluster-name" + deprecatedClusterNameLabel = "cluster.k8s.io/cluster-name" cpuKey = "machine.openshift.io/vCPU" memoryKey = "machine.openshift.io/memoryMb" @@ -70,6 +72,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 } @@ -86,6 +91,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 a1aa09660c77..4686f647ef28 100644 --- a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_utils_test.go +++ b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_utils_test.go @@ -114,6 +114,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{