From bb015b26a17ed1692d2a051e22adc6dfa09748e1 Mon Sep 17 00:00:00 2001 From: Michael McCune Date: Fri, 14 Oct 2022 14:06:57 -0400 Subject: [PATCH] remove unsupported functionality from cluster-api provider this change removes the code for the `Labels` and `Taints` interface functions of the clusterapi provider when scaling from zero. The body of these functions was added erronesouly and the Cluster API community is still deciding on how these values will be expose to the autoscaler. also updates the tests and readme to be more clear about the usage of labels and taints when scaling from zero. --- .../cloudprovider/clusterapi/README.md | 7 ++ .../clusterapi/clusterapi_nodegroup_test.go | 66 ++++++------------- .../clusterapi/clusterapi_unstructured.go | 26 ++------ 3 files changed, 34 insertions(+), 65 deletions(-) diff --git a/cluster-autoscaler/cloudprovider/clusterapi/README.md b/cluster-autoscaler/cloudprovider/clusterapi/README.md index ca4468b6362f..33a0a4d48643 100644 --- a/cluster-autoscaler/cloudprovider/clusterapi/README.md +++ b/cluster-autoscaler/cloudprovider/clusterapi/README.md @@ -228,6 +228,13 @@ rules: - list ``` +#### Pre-defined labels and taints on nodes scaled from zero + +The Cluster API provider currently does not support the addition of pre-defined +labels and taints for node groups that are scaling from zero. This work is on-going +and will be included in a future release once the API for specifying those +labels and taints has been accepted by the community. + ## Specifying a Custom Resource Group By default all Kubernetes resources consumed by the Cluster API provider will diff --git a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup_test.go b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup_test.go index a39fccc4a52c..2ddcff465e3a 100644 --- a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup_test.go +++ b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup_test.go @@ -1281,7 +1281,6 @@ func TestNodeGroupTemplateNodeInfo(t *testing.T) { type testCaseConfig struct { nodeLabels map[string]string - nodegroupLabels map[string]string includeNodes bool expectedErr error expectedCapacity map[corev1.ResourceName]int64 @@ -1309,44 +1308,24 @@ func TestNodeGroupTemplateNodeInfo(t *testing.T) { }, config: testCaseConfig{ expectedErr: nil, - expectedCapacity: map[corev1.ResourceName]int64{ - corev1.ResourceCPU: 2, - corev1.ResourceMemory: 2048 * 1024 * 1024, - corev1.ResourcePods: 110, - gpuapis.ResourceNvidiaGPU: 1, - }, - expectedNodeLabels: map[string]string{ + nodeLabels: map[string]string{ "kubernetes.io/os": "linux", "beta.kubernetes.io/os": "linux", "kubernetes.io/arch": "amd64", "beta.kubernetes.io/arch": "amd64", }, - }, - }, - { - name: "When the NodeGroup can scale from zero and the nodegroup adds labels to the Node", - nodeGroupAnnotations: map[string]string{ - memoryKey: "2048Mi", - cpuKey: "2", - }, - config: testCaseConfig{ - expectedErr: nil, - nodegroupLabels: map[string]string{ - "nodeGroupLabel": "value", - "anotherLabel": "anotherValue", - }, expectedCapacity: map[corev1.ResourceName]int64{ - corev1.ResourceCPU: 2, - corev1.ResourceMemory: 2048 * 1024 * 1024, - corev1.ResourcePods: 110, + corev1.ResourceCPU: 2, + corev1.ResourceMemory: 2048 * 1024 * 1024, + corev1.ResourcePods: 110, + gpuapis.ResourceNvidiaGPU: 1, }, expectedNodeLabels: map[string]string{ "kubernetes.io/os": "linux", "beta.kubernetes.io/os": "linux", "kubernetes.io/arch": "amd64", "beta.kubernetes.io/arch": "amd64", - "nodeGroupLabel": "value", - "anotherLabel": "anotherValue", + "kubernetes.io/hostname": "random value", }, }, }, @@ -1361,13 +1340,10 @@ func TestNodeGroupTemplateNodeInfo(t *testing.T) { expectedErr: nil, nodeLabels: map[string]string{ "kubernetes.io/os": "windows", + "beta.kubernetes.io/os": "windows", "kubernetes.io/arch": "arm64", + "beta.kubernetes.io/arch": "arm64", "node.kubernetes.io/instance-type": "instance1", - "anotherLabel": "nodeValue", // This should not be copied as it is not a well known label - }, - nodegroupLabels: map[string]string{ - "nodeGroupLabel": "value", - "anotherLabel": "nodeGroupValue", }, expectedCapacity: map[corev1.ResourceName]int64{ corev1.ResourceCPU: 2, @@ -1375,12 +1351,11 @@ func TestNodeGroupTemplateNodeInfo(t *testing.T) { corev1.ResourcePods: 110, }, expectedNodeLabels: map[string]string{ + "kubernetes.io/hostname": "random value", "kubernetes.io/os": "windows", - "beta.kubernetes.io/os": "linux", + "beta.kubernetes.io/os": "windows", "kubernetes.io/arch": "arm64", - "beta.kubernetes.io/arch": "amd64", - "nodeGroupLabel": "value", - "anotherLabel": "nodeGroupValue", + "beta.kubernetes.io/arch": "arm64", "node.kubernetes.io/instance-type": "instance1", }, }, @@ -1388,12 +1363,6 @@ func TestNodeGroupTemplateNodeInfo(t *testing.T) { } test := func(t *testing.T, testConfig *testConfig, config testCaseConfig) { - if testConfig.machineDeployment != nil { - unstructured.SetNestedStringMap(testConfig.machineDeployment.Object, config.nodegroupLabels, "spec", "template", "spec", "metadata", "labels") - } else { - unstructured.SetNestedStringMap(testConfig.machineSet.Object, config.nodegroupLabels, "spec", "template", "spec", "metadata", "labels") - } - if config.includeNodes { for i := range testConfig.nodes { testConfig.nodes[i].SetLabels(config.nodeLabels) @@ -1439,15 +1408,20 @@ func TestNodeGroupTemplateNodeInfo(t *testing.T) { } } - // expectedNodeLabels won't have the hostname label as it is randomized, so +1 to its length - if len(nodeInfo.Node().GetLabels()) != len(config.expectedNodeLabels)+1 { - t.Errorf("Expected node labels to have len: %d, but got: %d", len(config.expectedNodeLabels)+1, len(nodeInfo.Node().GetLabels())) + if len(nodeInfo.Node().GetLabels()) != len(config.expectedNodeLabels) { + t.Errorf("Expected node labels to have len: %d, but got: %d, labels are: %v", len(config.expectedNodeLabels), len(nodeInfo.Node().GetLabels()), nodeInfo.Node().GetLabels()) } for key, value := range nodeInfo.Node().GetLabels() { // Exclude the hostname label as it is randomized if key != corev1.LabelHostname { + if expected, ok := config.expectedNodeLabels[key]; ok { + if value != expected { + t.Errorf("Expected node label %q: %q, Got: %q", key, config.expectedNodeLabels[key], value) + } + } else { + t.Errorf("Expected node label %q to exist in node", key) + } if value != config.expectedNodeLabels[key] { - t.Errorf("Expected node label %q: %q, Got: %q", key, config.expectedNodeLabels[key], value) } } } diff --git a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_unstructured.go b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_unstructured.go index f8ad7f4e215b..853d80174861 100644 --- a/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_unstructured.go +++ b/cluster-autoscaler/cloudprovider/clusterapi/clusterapi_unstructured.go @@ -169,28 +169,16 @@ func (r unstructuredScalableResource) MarkMachineForDeletion(machine *unstructur } func (r unstructuredScalableResource) Labels() map[string]string { - labels, found, err := unstructured.NestedStringMap(r.unstructured.Object, "spec", "template", "spec", "metadata", "labels") - if !found || err != nil { - return nil - } - return labels + // TODO implement this once the community has decided how they will handle labels + // this issue is related, https://github.com/kubernetes-sigs/cluster-api/issues/7006 + + return nil } func (r unstructuredScalableResource) Taints() []apiv1.Taint { - taints, found, err := unstructured.NestedSlice(r.unstructured.Object, "spec", "template", "spec", "taints") - if !found || err != nil { - return nil - } - ret := make([]apiv1.Taint, len(taints)) - for i, t := range taints { - if v, ok := t.(apiv1.Taint); ok { - ret[i] = v - } else { - // if we cannot convert the interface to a Taint, return early with zero value - return nil - } - } - return ret + // TODO implement this once the community has decided how they will handle taints + + return nil } // A node group can scale from zero if it can inform about the CPU and memory