Merge pull request #2604 from jackfrancis/synthesize-aks-capz-nodepoo…

…l-labels

Implement ScaleSetPriority for AzureManagedMachinePool

azure/converters/managedagentpool.go

@@ -43,5 +43,6 @@ func AgentPoolToManagedClusterAgentPoolProfile(pool containerservice.AgentPool)
 		NodeLabels:          properties.NodeLabels,
 		EnableUltraSSD:      properties.EnableUltraSSD,
 		EnableNodePublicIP:  properties.EnableNodePublicIP,
+		ScaleSetPriority:    properties.ScaleSetPriority,
 	}
 }

azure/scope/managedmachinepool.go

@@ -176,6 +176,7 @@ func buildAgentPoolSpec(managedControlPlane *infrav1exp.AzureManagedControlPlane
 		EnableUltraSSD:     managedMachinePool.Spec.EnableUltraSSD,
 		Headers:            maps.FilterByKeyPrefix(agentPoolAnnotations, azure.CustomHeaderPrefix),
 		EnableNodePublicIP: managedMachinePool.Spec.EnableNodePublicIP,
+		ScaleSetPriority:   managedMachinePool.Spec.ScaleSetPriority,
 	}
 
 	if managedMachinePool.Spec.OSDiskSizeGB != nil {

azure/services/agentpools/spec.go

@@ -26,6 +26,7 @@ import (
 	"github.com/pkg/errors"
 	infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1"
 	"sigs.k8s.io/cluster-api-provider-azure/azure"
+	azureutil "sigs.k8s.io/cluster-api-provider-azure/util/azure"
 )
 
 // AgentPoolSpec contains agent pool specification details.
@@ -92,6 +93,9 @@ type AgentPoolSpec struct {
 
 	// EnableNodePublicIP controls whether or not nodes in the agent pool each have a public IP address.
 	EnableNodePublicIP *bool `json:"enableNodePublicIP,omitempty"`
+
+	// ScaleSetPriority specifies the ScaleSetPriority for the node pool. Allowed values are 'Spot' and 'Regular'
+	ScaleSetPriority *string `json:"scaleSetPriority,omitempty"`
 }
 
 // ResourceName returns the name of the agent pool.
@@ -116,6 +120,7 @@ func (s *AgentPoolSpec) CustomHeaders() map[string]string {
 
 // Parameters returns the parameters for the agent pool.
 func (s *AgentPoolSpec) Parameters(existing interface{}) (params interface{}, err error) {
+	nodeLabels := s.NodeLabels
 	if existing != nil {
 		existingPool, ok := existing.(containerservice.AgentPool)
 		if !ok {
@@ -169,6 +174,12 @@ func (s *AgentPoolSpec) Parameters(existing interface{}) (params interface{}, er
 			// agent pool is up to date, nothing to do
 			return nil, nil
 		}
+		// We do a just-in-time merge of existent kubernetes.azure.com-prefixed labels
+		// So that we don't unintentionally delete them
+		// See https://github.com/Azure/AKS/issues/3152
+		if normalizedProfile.NodeLabels != nil {
+			nodeLabels = mergeSystemNodeLabels(normalizedProfile.NodeLabels, existingPool.NodeLabels)
+		}
 	}
 
 	var availabilityZones *[]string
@@ -202,16 +213,30 @@ func (s *AgentPoolSpec) Parameters(existing interface{}) (params interface{}, er
 			MaxPods:             s.MaxPods,
 			MinCount:            s.MinCount,
 			Mode:                containerservice.AgentPoolMode(s.Mode),
-			NodeLabels:          s.NodeLabels,
+			NodeLabels:          nodeLabels,
 			NodeTaints:          nodeTaints,
 			OrchestratorVersion: s.Version,
 			OsDiskSizeGB:        &s.OSDiskSizeGB,
 			OsDiskType:          containerservice.OSDiskType(to.String(s.OsDiskType)),
 			OsType:              containerservice.OSType(to.String(s.OSType)),
+			ScaleSetPriority:    containerservice.ScaleSetPriority(to.String(s.ScaleSetPriority)),
 			Type:                containerservice.AgentPoolTypeVirtualMachineScaleSets,
 			VMSize:              sku,
 			VnetSubnetID:        vnetSubnetID,
 			EnableNodePublicIP:  s.EnableNodePublicIP,
 		},
 	}, nil
 }
+
+// mergeSystemNodeLabels appends any kubernetes.azure.com-prefixed labels from the AKS label set
+// into the local capz label set.
+func mergeSystemNodeLabels(capz, aks map[string]*string) map[string]*string {
+	ret := capz
+	// Look for labels returned from the AKS node pool API that begin with kubernetes.azure.com
+	for aksNodeLabelKey := range aks {
+		if azureutil.IsAzureSystemNodeLabelKey(aksNodeLabelKey) {
+			ret[aksNodeLabelKey] = aks[aksNodeLabelKey]
+		}
+	}
+	return ret
+}

azure/services/agentpools/spec_test.go

@@ -373,3 +373,82 @@ func TestParameters(t *testing.T) {
 		})
 	}
 }
+
+func TestMergeSystemNodeLabels(t *testing.T) {
+	testcases := []struct {
+		name       string
+		capzLabels map[string]*string
+		aksLabels  map[string]*string
+		expected   map[string]*string
+	}{
+		{
+			name: "update an existing label",
+			capzLabels: map[string]*string{
+				"foo": to.StringPtr("bar"),
+			},
+			aksLabels: map[string]*string{
+				"foo": to.StringPtr("baz"),
+			},
+			expected: map[string]*string{
+				"foo": to.StringPtr("bar"),
+			},
+		},
+		{
+			name:       "delete labels",
+			capzLabels: map[string]*string{},
+			aksLabels: map[string]*string{
+				"foo":   to.StringPtr("bar"),
+				"hello": to.StringPtr("world"),
+			},
+			expected: map[string]*string{},
+		},
+		{
+			name: "delete one label",
+			capzLabels: map[string]*string{
+				"foo": to.StringPtr("bar"),
+			},
+			aksLabels: map[string]*string{
+				"foo":   to.StringPtr("bar"),
+				"hello": to.StringPtr("world"),
+			},
+			expected: map[string]*string{
+				"foo": to.StringPtr("bar"),
+			},
+		},
+		{
+			name: "retain system label during update",
+			capzLabels: map[string]*string{
+				"foo": to.StringPtr("bar"),
+			},
+			aksLabels: map[string]*string{
+				"kubernetes.azure.com/scalesetpriority": to.StringPtr("spot"),
+			},
+			expected: map[string]*string{
+				"foo":                                   to.StringPtr("bar"),
+				"kubernetes.azure.com/scalesetpriority": to.StringPtr("spot"),
+			},
+		},
+		{
+			name:       "retain system label during delete",
+			capzLabels: map[string]*string{},
+			aksLabels: map[string]*string{
+				"kubernetes.azure.com/scalesetpriority": to.StringPtr("spot"),
+			},
+			expected: map[string]*string{
+				"kubernetes.azure.com/scalesetpriority": to.StringPtr("spot"),
+			},
+		},
+	}
+
+	for _, tc := range testcases {
+		t.Logf("Testing " + tc.name)
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			g := NewWithT(t)
+			t.Parallel()
+
+			ret := mergeSystemNodeLabels(tc.capzLabels, tc.aksLabels)
+			g.Expect(ret).To(Equal(tc.expected))
+		})
+	}
+}

config/crd/bases/infrastructure.cluster.x-k8s.io_azuremanagedmachinepools.yaml

@@ -262,6 +262,13 @@ spec:
                 items:
                   type: string
                 type: array
+              scaleSetPriority:
+                description: 'ScaleSetPriority specifies the ScaleSetPriority value.
+                  Default to Regular. Possible values include: ''Regular'', ''Spot'''
+                enum:
+                - Regular
+                - Spot
+                type: string
               scaling:
                 description: Scaling specifies the autoscaling parameters for the
                   node pool.

exp/api/v1alpha3/azuremanagedmachinepool_conversion.go

@@ -46,6 +46,7 @@ func (src *AzureManagedMachinePool) ConvertTo(dstRaw conversion.Hub) error {
 	dst.Spec.NodeLabels = restored.Spec.NodeLabels
 	dst.Spec.EnableUltraSSD = restored.Spec.EnableUltraSSD
 	dst.Spec.EnableNodePublicIP = restored.Spec.EnableNodePublicIP
+	dst.Spec.ScaleSetPriority = restored.Spec.ScaleSetPriority
 
 	dst.Status.LongRunningOperationStates = restored.Status.LongRunningOperationStates
 	dst.Status.Conditions = restored.Status.Conditions

exp/api/v1alpha4/azuremanagedmachinepool_conversion.go

@@ -46,6 +46,7 @@ func (src *AzureManagedMachinePool) ConvertTo(dstRaw conversion.Hub) error {
 	dst.Spec.NodeLabels = restored.Spec.NodeLabels
 	dst.Spec.EnableUltraSSD = restored.Spec.EnableUltraSSD
 	dst.Spec.EnableNodePublicIP = restored.Spec.EnableNodePublicIP
+	dst.Spec.ScaleSetPriority = restored.Spec.ScaleSetPriority
 
 	dst.Status.LongRunningOperationStates = restored.Status.LongRunningOperationStates
 	dst.Status.Conditions = restored.Status.Conditions

exp/api/v1beta1/azuremanagedmachinepool_types.go

@@ -102,6 +102,11 @@ type AzureManagedMachinePoolSpec struct {
 	// EnableNodePublicIP controls whether or not nodes in the pool each have a public IP address.
 	// +optional
 	EnableNodePublicIP *bool `json:"enableNodePublicIP,omitempty"`
+
+	// ScaleSetPriority specifies the ScaleSetPriority value. Default to Regular. Possible values include: 'Regular', 'Spot'
+	// +kubebuilder:validation:Enum=Regular;Spot
+	// +optional
+	ScaleSetPriority *string `json:"scaleSetPriority,omitempty"`
 }
 
 // ManagedMachinePoolScaling specifies scaling options.

exp/api/v1beta1/azuremanagedmachinepool_webhook.go

@@ -28,6 +28,7 @@ import (
 	kerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"sigs.k8s.io/cluster-api-provider-azure/azure"
+	azureutil "sigs.k8s.io/cluster-api-provider-azure/util/azure"
 	"sigs.k8s.io/cluster-api-provider-azure/util/maps"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -59,6 +60,7 @@ func (m *AzureManagedMachinePool) ValidateCreate(client client.Client) error {
 		m.validateMaxPods,
 		m.validateOSType,
 		m.validateName,
+		m.validateNodeLabels,
 	}
 
 	var errs []error
@@ -76,6 +78,14 @@ func (m *AzureManagedMachinePool) ValidateUpdate(oldRaw runtime.Object, client c
 	old := oldRaw.(*AzureManagedMachinePool)
 	var allErrs field.ErrorList
 
+	if err := m.validateNodeLabels(); err != nil {
+		allErrs = append(allErrs,
+			field.Invalid(
+				field.NewPath("Spec", "NodeLabels"),
+				m.Spec.NodeLabels,
+				err.Error()))
+	}
+
 	if old.Spec.OSType != nil {
 		// Prevent OSType modification if it was already set to some value
 		if m.Spec.OSType == nil {
@@ -189,6 +199,13 @@ func (m *AzureManagedMachinePool) ValidateUpdate(oldRaw runtime.Object, client c
 		}
 	}
 
+	if !reflect.DeepEqual(m.Spec.ScaleSetPriority, old.Spec.ScaleSetPriority) {
+		allErrs = append(allErrs,
+			field.Invalid(field.NewPath("Spec", "ScaleSetPriority"),
+				m.Spec.ScaleSetPriority, "field is immutable"),
+		)
+	}
+
 	if err := validateBoolPtrImmutable(
 		field.NewPath("Spec", "EnableUltraSSD"),
 		old.Spec.EnableUltraSSD,
@@ -298,6 +315,19 @@ func (m *AzureManagedMachinePool) validateName() error {
 	return nil
 }
 
+func (m *AzureManagedMachinePool) validateNodeLabels() error {
+	for key := range m.Spec.NodeLabels {
+		if azureutil.IsAzureSystemNodeLabelKey(key) {
+			return field.Invalid(
+				field.NewPath("Spec", "NodeLabels"),
+				key,
+				fmt.Sprintf("Node pool label key must not start with %s", azureutil.AzureSystemNodeLabelPrefix))
+		}
+	}
+
+	return nil
+}
+
 func ensureStringSlicesAreEqual(a []string, b []string) bool {
 	if len(a) != len(b) {
 		return false

exp/api/v1beta1/azuremanagedmachinepool_webhook_test.go

@@ -462,10 +462,31 @@ func TestAzureManagedMachinePoolUpdatingWebhook(t *testing.T) {
 			},
 			wantErr: false,
 		},
+		{
+			name: "Can't add a node label that begins with kubernetes.azure.com",
+			new: &AzureManagedMachinePool{
+				Spec: AzureManagedMachinePoolSpec{
+					NodeLabels: map[string]string{
+						"foo":                                   "bar",
+						"kubernetes.azure.com/scalesetpriority": "spot",
+					},
+				},
+			},
+			old: &AzureManagedMachinePool{
+				Spec: AzureManagedMachinePoolSpec{
+					NodeLabels: map[string]string{
+						"foo": "bar",
+					},
+				},
+			},
+			wantErr: true,
+		},
 	}
 	var client client.Client
 	for _, tc := range tests {
+		tc := tc
 		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
 			err := tc.new.ValidateUpdate(tc.old, client)
 			if tc.wantErr {
 				g.Expect(err).To(HaveOccurred())
@@ -650,6 +671,33 @@ func TestAzureManagedMachinePool_ValidateCreate(t *testing.T) {
 			wantErr:  true,
 			errorLen: 1,
 		},
+		{
+			name: "valid label",
+			ammp: &AzureManagedMachinePool{
+				Spec: AzureManagedMachinePoolSpec{
+					Mode:   "User",
+					OSType: to.StringPtr(azure.LinuxOS),
+					NodeLabels: map[string]string{
+						"foo": "bar",
+					},
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "kubernetes.azure.com label",
+			ammp: &AzureManagedMachinePool{
+				Spec: AzureManagedMachinePoolSpec{
+					Mode:   "User",
+					OSType: to.StringPtr(azure.LinuxOS),
+					NodeLabels: map[string]string{
+						"kubernetes.azure.com/scalesetpriority": "spot",
+					},
+				},
+			},
+			wantErr:  true,
+			errorLen: 1,
+		},
 	}
 	var client client.Client
 	for _, tc := range tests {

templates/flavors/aks/cluster-template.yaml

@@ -106,3 +106,4 @@ spec:
   osDiskSizeGB: 40
   sku: "${AZURE_NODE_MACHINE_TYPE}"
   enableNodePublicIP: false
+  scaleSetPriority: Regular