Merge pull request #8713 from elmiko/capi-managed-node-labels-fix

allow clusterapi provider to process managed labels

Author: k8s-ci-robot

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup_test.go

@@ -1494,6 +1494,7 @@ func TestNodeGroupTemplateNodeInfo(t *testing.T) {
 
 	type testCaseConfig struct {
 		nodeLabels            map[string]string
+		managedLabels         map[string]string
 		includeNodes          bool
 		expectedErr           error
 		expectedCapacity      map[corev1.ResourceName]int64
@@ -1618,6 +1619,37 @@ func TestNodeGroupTemplateNodeInfo(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "When the NodeGroup can scale from zero, and the scalable resource contains managed labels",
+			nodeGroupAnnotations: map[string]string{
+				memoryKey:   "2048Mi",
+				cpuKey:      "2",
+				gpuTypeKey:  gpuapis.ResourceNvidiaGPU,
+				gpuCountKey: "1",
+			},
+			config: testCaseConfig{
+				expectedErr: nil,
+				nodeLabels: map[string]string{
+					"kubernetes.io/os":   "linux",
+					"kubernetes.io/arch": "amd64",
+				},
+				managedLabels: map[string]string{
+					"node-role.kubernetes.io/test": "test",
+				},
+				expectedCapacity: map[corev1.ResourceName]int64{
+					corev1.ResourceCPU:        2,
+					corev1.ResourceMemory:     2048 * 1024 * 1024,
+					corev1.ResourcePods:       110,
+					gpuapis.ResourceNvidiaGPU: 1,
+				},
+				expectedNodeLabels: map[string]string{
+					"kubernetes.io/os":             "linux",
+					"kubernetes.io/arch":           "amd64",
+					"kubernetes.io/hostname":       "random value",
+					"node-role.kubernetes.io/test": "test",
+				},
+			},
+		},
 	}
 
 	test := func(t *testing.T, testConfig *TestConfig, config testCaseConfig) {
@@ -1704,6 +1736,7 @@ func TestNodeGroupTemplateNodeInfo(t *testing.T) {
 					WithNamespace(testNamespace).
 					WithNodeCount(10).
 					WithAnnotations(cloudprovider.JoinStringMaps(enableScaleAnnotations, tc.nodeGroupAnnotations)).
+					WithManagedLabels(tc.config.managedLabels).
 					Build()
 				test(t, testConfig, tc.config)
 			})
@@ -1714,6 +1747,7 @@ func TestNodeGroupTemplateNodeInfo(t *testing.T) {
 					WithNamespace(testNamespace).
 					WithNodeCount(10).
 					WithAnnotations(cloudprovider.JoinStringMaps(enableScaleAnnotations, tc.nodeGroupAnnotations)).
+					WithManagedLabels(tc.config.managedLabels).
 					Build()
 				test(t, testConfig, tc.config)
 			})

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_test_framework.go

@@ -53,25 +53,27 @@ const (
 )
 
 type testConfigBuilder struct {
-	scalableType scalableTestType
-	namespace    string
-	clusterName  string
-	namePrefix   string
-	nodeCount    int
-	annotations  map[string]string
-	capacity     map[string]string
-	nodeInfo     map[string]string
+	scalableType  scalableTestType
+	namespace     string
+	clusterName   string
+	namePrefix    string
+	nodeCount     int
+	annotations   map[string]string
+	capacity      map[string]string
+	nodeInfo      map[string]string
+	managedLabels map[string]string
 }
 
 // NewTestConfigBuilder returns a builder for dynamically constructing mock ClusterAPI resources for testing.
 func NewTestConfigBuilder() *testConfigBuilder {
 	return &testConfigBuilder{
-		namespace:   RandomString(6),
-		clusterName: RandomString(6),
-		namePrefix:  RandomString(6),
-		nodeCount:   0,
-		annotations: nil,
-		capacity:    nil,
+		namespace:     RandomString(6),
+		clusterName:   RandomString(6),
+		namePrefix:    RandomString(6),
+		nodeCount:     0,
+		annotations:   nil,
+		capacity:      nil,
+		managedLabels: nil,
 	}
 }
 
@@ -93,6 +95,7 @@ func (b *testConfigBuilder) Build() *TestConfig {
 			b.annotations,
 			b.capacity,
 			b.nodeInfo,
+			b.managedLabels,
 		)[0],
 	)[0]
 }
@@ -114,6 +117,7 @@ func (b *testConfigBuilder) BuildMultiple(configCount int) []*TestConfig {
 			b.annotations,
 			b.capacity,
 			b.nodeInfo,
+			b.managedLabels,
 		)...,
 	)
 }
@@ -186,6 +190,19 @@ func (b *testConfigBuilder) WithNodeInfo(n map[string]string) *testConfigBuilder
 	return b
 }
 
+func (b *testConfigBuilder) WithManagedLabels(l map[string]string) *testConfigBuilder {
+	if l == nil {
+		// explicitly setting managed labels to nil
+		b.managedLabels = nil
+	} else {
+		if b.managedLabels == nil {
+			b.managedLabels = map[string]string{}
+		}
+		maps.Insert(b.managedLabels, maps.All(l))
+	}
+	return b
+}
+
 // TestConfig contains clusterspecific information about a single test configuration.
 type TestConfig struct {
 	spec              *TestSpec
@@ -235,6 +252,9 @@ func createTestConfigs(specs ...TestSpec) []*TestConfig {
 								"kind":       machineTemplateKind,
 								"name":       "TestMachineTemplate",
 							},
+							"metadata": map[string]interface{}{
+								"labels": map[string]interface{}{},
+							},
 						},
 					},
 				},
@@ -244,6 +264,12 @@ func createTestConfigs(specs ...TestSpec) []*TestConfig {
 
 		config.machineSet.SetAnnotations(make(map[string]string))
 
+		if spec.managedLabels != nil {
+			if err := unstructured.SetNestedStringMap(config.machineSet.Object, spec.managedLabels, "spec", "template", "spec", "metadata", "labels"); err != nil {
+				panic(err)
+			}
+		}
+
 		if !spec.rootIsMachineDeployment {
 			config.machineSet.SetAnnotations(spec.annotations)
 		} else {
@@ -273,6 +299,9 @@ func createTestConfigs(specs ...TestSpec) []*TestConfig {
 									"kind":     machineTemplateKind,
 									"name":     "TestMachineTemplate",
 								},
+								"metadata": map[string]interface{}{
+									"labels": map[string]interface{}{},
+								},
 							},
 						},
 					},
@@ -293,6 +322,12 @@ func createTestConfigs(specs ...TestSpec) []*TestConfig {
 				},
 			}
 			config.machineSet.SetOwnerReferences(ownerRefs)
+
+			if spec.managedLabels != nil {
+				if err := unstructured.SetNestedStringMap(config.machineDeployment.Object, spec.managedLabels, "spec", "template", "spec", "metadata", "labels"); err != nil {
+					panic(err)
+				}
+			}
 		}
 		config.machineSet.SetLabels(machineSetLabels)
 		if err := unstructured.SetNestedStringMap(config.machineSet.Object, machineSetLabels, "spec", "selector", "matchLabels"); err != nil {
@@ -352,6 +387,7 @@ type TestSpec struct {
 	annotations             map[string]string
 	capacity                map[string]string
 	nodeInfo                map[string]string
+	managedLabels           map[string]string
 	machineDeploymentName   string
 	machineSetName          string
 	machinePoolName         string
@@ -361,20 +397,42 @@ type TestSpec struct {
 	rootIsMachineDeployment bool
 }
 
-func createTestSpecs(namespace, clusterName, namePrefix string, scalableResourceCount, nodeCount int, isMachineDeployment bool, annotations map[string]string, capacity map[string]string, nodeInfo map[string]string) []TestSpec {
+func createTestSpecs(
+	namespace string,
+	clusterName string,
+	namePrefix string,
+	scalableResourceCount int,
+	nodeCount int,
+	isMachineDeployment bool,
+	annotations map[string]string,
+	capacity map[string]string,
+	nodeInfo map[string]string,
+	managedLabels map[string]string,
+) []TestSpec {
 	var specs []TestSpec
 
 	for i := 0; i < scalableResourceCount; i++ {
-		specs = append(specs, createTestSpec(namespace, clusterName, fmt.Sprintf("%s-%d", namePrefix, i), nodeCount, isMachineDeployment, annotations, capacity, nodeInfo))
+		specs = append(specs, createTestSpec(namespace, clusterName, fmt.Sprintf("%s-%d", namePrefix, i), nodeCount, isMachineDeployment, annotations, capacity, nodeInfo, managedLabels))
 	}
 
 	return specs
 }
 
-func createTestSpec(namespace, clusterName, name string, nodeCount int, isMachineDeployment bool, annotations map[string]string, capacity map[string]string, nodeInfo map[string]string) TestSpec {
+func createTestSpec(
+	namespace string,
+	clusterName string,
+	name string,
+	nodeCount int,
+	isMachineDeployment bool,
+	annotations map[string]string,
+	capacity map[string]string,
+	nodeInfo map[string]string,
+	managedLabels map[string]string,
+) TestSpec {
 	return TestSpec{
 		annotations:             annotations,
 		capacity:                capacity,
+		managedLabels:           managedLabels,
 		machineDeploymentName:   name,
 		machineSetName:          name,
 		clusterName:             clusterName,

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_unstructured.go

@@ -36,6 +36,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/validation"
+	"k8s.io/autoscaler/cluster-autoscaler/cloudprovider"
 	klog "k8s.io/klog/v2"
 	"k8s.io/utils/ptr"
 )
@@ -200,20 +201,29 @@ func (r unstructuredScalableResource) MarkMachineForDeletion(machine *unstructur
 }
 
 func (r unstructuredScalableResource) Labels() map[string]string {
+	allLabels := map[string]string{}
+
+	// get the managed labels from the scalable resource, if they exist.
+	if labels, found, err := unstructured.NestedStringMap(r.unstructured.UnstructuredContent(), "spec", "template", "spec", "metadata", "labels"); found && err == nil {
+		managedLabels := getManagedNodeLabelsFromLabels(labels)
+		allLabels = cloudprovider.JoinStringMaps(allLabels, managedLabels)
+	}
+
+	// annotation labels are supplied as an override to other values, we process them last.
 	annotations := r.unstructured.GetAnnotations()
 	// annotation value of the form "key1=value1,key2=value2"
 	if val, found := annotations[labelsKey]; found {
 		labels := strings.Split(val, ",")
-		kv := make(map[string]string, len(labels))
+		annotationLabels := make(map[string]string, len(labels))
 		for _, label := range labels {
 			split := strings.SplitN(label, "=", 2)
 			if len(split) == 2 {
-				kv[split[0]] = split[1]
+				annotationLabels[split[0]] = split[1]
 			}
 		}
-		return kv
+		allLabels = cloudprovider.JoinStringMaps(allLabels, annotationLabels)
 	}
-	return nil
+	return allLabels
 }
 
 func (r unstructuredScalableResource) Taints() []apiv1.Taint {

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_utils.go

@@ -59,6 +59,14 @@ const (
 	scaleUpFromZeroDefaultArchEnvVar = "CAPI_SCALE_ZERO_DEFAULT_ARCH"
 	// GpuDeviceType is used if DRA device is GPU
 	GpuDeviceType = "gpu"
+
+	// Cluster API constants, copied from cluster-api/api/core/v1beta1/machine_types.go
+	// nodeRoleLabelPrefix is one of the CAPI managed Node label prefixes.
+	nodeRoleLabelPrefix = "node-role.kubernetes.io"
+	// nodeRestrictionLabelDomain is one of the CAPI managed Node label domains.
+	nodeRestrictionLabelDomain = "node-restriction.kubernetes.io"
+	// managedNodeLabelDomain is one of the CAPI managed Node label domains.
+	managedNodeLabelDomain = "node.cluster.x-k8s.io"
 )
 
 var (
@@ -407,3 +415,33 @@ func GetDefaultScaleFromZeroArchitecture() SystemArchitecture {
 	})
 	return *systemArchitecture
 }
+
+// getManagedNodeLabelsFromLabels returns a map of labels that will be propagated
+// to nodes based on the Cluster API metadata propagation rules.
+func getManagedNodeLabelsFromLabels(labels map[string]string) map[string]string {
+	// TODO elmiko, add a user configuration to inject a string with their `--additional-sync-machine-labels` string.
+	// ref: https://cluster-api.sigs.k8s.io/reference/api/metadata-propagation#machine
+	managedLabels := map[string]string{}
+	for key, value := range labels {
+		if isManagedLabel(key) {
+			managedLabels[key] = value
+		}
+
+	}
+
+	return managedLabels
+}
+
+func isManagedLabel(key string) bool {
+	dnsSubdomainOrName := strings.Split(key, "/")[0]
+	if dnsSubdomainOrName == nodeRoleLabelPrefix {
+		return true
+	}
+	if dnsSubdomainOrName == nodeRestrictionLabelDomain || strings.HasSuffix(dnsSubdomainOrName, "."+nodeRestrictionLabelDomain) {
+		return true
+	}
+	if dnsSubdomainOrName == managedNodeLabelDomain || strings.HasSuffix(dnsSubdomainOrName, "."+managedNodeLabelDomain) {
+		return true
+	}
+	return false
+}

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_utils_test.go

@@ -18,12 +18,13 @@ package clusterapi
 
 import (
 	"fmt"
-	"github.com/google/go-cmp/cmp"
 	"reflect"
 	"strings"
 	"sync"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
+
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -33,6 +34,11 @@ import (
 const (
 	uuid1 = "ec21c5fb-a3d5-a45f-887b-6b49aa8fc218"
 	uuid2 = "ec23ebb0-bc60-443f-d139-046ec5046283"
+
+	managedLabel1 = "node-restriction.kubernetes.io/some-thing"
+	managedLabel2 = "prefixed.node-restriction.kubernetes.io/some-other-thing"
+	managedLabel3 = "node.cluster.x-k8s.io/another-thing"
+	managedLabel4 = "prefixed.node.cluster.x-k8s.io/another-thing"
 )
 
 func TestUtilParseScalingBounds(t *testing.T) {
@@ -937,3 +943,27 @@ func TestGetSystemArchitectureFromEnvOrDefault(t *testing.T) {
 		})
 	}
 }
+
+func TestGetManagedNodeLabelsFromLabels(t *testing.T) {
+	actualLabels := map[string]string{
+		// these labels should propagate
+		managedLabel1: "1",
+		managedLabel2: "2",
+		managedLabel3: "3",
+		managedLabel4: "4",
+		// the following should NOT propagate
+		"my.special.label/should-not-propagate":         "bar",
+		"prefixed.node-role.kubernetes.io/no-propagate": "special-role",
+	}
+
+	observedLabels := getManagedNodeLabelsFromLabels(actualLabels)
+	if len(observedLabels) != 4 {
+		t.Errorf("expected observedLabels length to be 4, actual: %d", len(observedLabels))
+	}
+	expectedLabels := []string{managedLabel1, managedLabel2, managedLabel3, managedLabel4}
+	for _, l := range expectedLabels {
+		if _, ok := observedLabels[l]; !ok {
+			t.Errorf("expected observedLabels to contain %q", l)
+		}
+	}
+}