diff --git a/azure/converters/managedagentpool.go b/azure/converters/managedagentpool.go index 3f7fae2fd95e..b73cc74405a5 100644 --- a/azure/converters/managedagentpool.go +++ b/azure/converters/managedagentpool.go @@ -42,5 +42,6 @@ func AgentPoolToManagedClusterAgentPoolProfile(pool containerservice.AgentPool) OsDiskType: properties.OsDiskType, NodeLabels: properties.NodeLabels, EnableUltraSSD: properties.EnableUltraSSD, + ScaleSetPriority: properties.ScaleSetPriority, } } diff --git a/azure/scope/managedmachinepool.go b/azure/scope/managedmachinepool.go index fb63d7e09efc..6a7b9e86f6fc 100644 --- a/azure/scope/managedmachinepool.go +++ b/azure/scope/managedmachinepool.go @@ -175,6 +175,7 @@ func buildAgentPoolSpec(managedControlPlane *infrav1exp.AzureManagedControlPlane OsDiskType: managedMachinePool.Spec.OsDiskType, EnableUltraSSD: managedMachinePool.Spec.EnableUltraSSD, Headers: maps.FilterByKeyPrefix(agentPoolAnnotations, azure.CustomHeaderPrefix), + ScaleSetPriority: managedMachinePool.Spec.ScaleSetPriority, } if managedMachinePool.Spec.OSDiskSizeGB != nil { diff --git a/azure/services/agentpools/agentpools_test.go b/azure/services/agentpools/agentpools_test.go index 728889a6ad6f..53fd2768b5be 100644 --- a/azure/services/agentpools/agentpools_test.go +++ b/azure/services/agentpools/agentpools_test.go @@ -193,3 +193,82 @@ func TestDeleteAgentPools(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)) + }) + } +} diff --git a/azure/services/agentpools/spec.go b/azure/services/agentpools/spec.go index 4599fa69a878..ae3a9e071006 100644 --- a/azure/services/agentpools/spec.go +++ b/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. @@ -89,6 +90,9 @@ type AgentPoolSpec struct { // Headers is the list of headers to add to the HTTP requests to update this resource. Headers map[string]string + + // 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. @@ -113,6 +117,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) { + var nodeLabels map[string]*string if existing != nil { existingPool, ok := existing.(containerservice.AgentPool) if !ok { @@ -164,6 +169,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 @@ -197,15 +208,29 @@ 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, }, }, 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 +} diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_azuremanagedmachinepools.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_azuremanagedmachinepools.yaml index 86a93e931c07..2aca70599c4a 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_azuremanagedmachinepools.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_azuremanagedmachinepools.yaml @@ -258,6 +258,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. diff --git a/exp/api/v1alpha3/azuremanagedmachinepool_conversion.go b/exp/api/v1alpha3/azuremanagedmachinepool_conversion.go index 8ac08cc9da00..74ac709d3c00 100644 --- a/exp/api/v1alpha3/azuremanagedmachinepool_conversion.go +++ b/exp/api/v1alpha3/azuremanagedmachinepool_conversion.go @@ -45,6 +45,7 @@ func (src *AzureManagedMachinePool) ConvertTo(dstRaw conversion.Hub) error { dst.Spec.OSType = restored.Spec.OSType dst.Spec.NodeLabels = restored.Spec.NodeLabels dst.Spec.EnableUltraSSD = restored.Spec.EnableUltraSSD + dst.Spec.ScaleSetPriority = restored.Spec.ScaleSetPriority dst.Status.LongRunningOperationStates = restored.Status.LongRunningOperationStates dst.Status.Conditions = restored.Status.Conditions diff --git a/exp/api/v1alpha3/zz_generated.conversion.go b/exp/api/v1alpha3/zz_generated.conversion.go index 89c8bdc9bff4..3e56fd35d640 100644 --- a/exp/api/v1alpha3/zz_generated.conversion.go +++ b/exp/api/v1alpha3/zz_generated.conversion.go @@ -890,6 +890,7 @@ func autoConvert_v1beta1_AzureManagedMachinePoolSpec_To_v1alpha3_AzureManagedMac // WARNING: in.OsDiskType requires manual conversion: does not exist in peer-type // WARNING: in.EnableUltraSSD requires manual conversion: does not exist in peer-type // WARNING: in.OSType requires manual conversion: does not exist in peer-type + // WARNING: in.ScaleSetPriority requires manual conversion: does not exist in peer-type return nil } diff --git a/exp/api/v1alpha4/zz_generated.conversion.go b/exp/api/v1alpha4/zz_generated.conversion.go index 3436d4ec2c67..122f8fa1c431 100644 --- a/exp/api/v1alpha4/zz_generated.conversion.go +++ b/exp/api/v1alpha4/zz_generated.conversion.go @@ -1195,6 +1195,7 @@ func autoConvert_v1beta1_AzureManagedMachinePoolSpec_To_v1alpha4_AzureManagedMac // WARNING: in.OsDiskType requires manual conversion: does not exist in peer-type // WARNING: in.EnableUltraSSD requires manual conversion: does not exist in peer-type // WARNING: in.OSType requires manual conversion: does not exist in peer-type + // WARNING: in.ScaleSetPriority requires manual conversion: does not exist in peer-type return nil } diff --git a/exp/api/v1beta1/azuremanagedmachinepool_types.go b/exp/api/v1beta1/azuremanagedmachinepool_types.go index 568402cc6da6..2e47d66bc0d5 100644 --- a/exp/api/v1beta1/azuremanagedmachinepool_types.go +++ b/exp/api/v1beta1/azuremanagedmachinepool_types.go @@ -98,6 +98,11 @@ type AzureManagedMachinePoolSpec struct { // +kubebuilder:validation:Enum=Linux;Windows // +optional OSType *string `json:"osType,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. diff --git a/exp/api/v1beta1/azuremanagedmachinepool_webhook.go b/exp/api/v1beta1/azuremanagedmachinepool_webhook.go index a08ca79c88b8..78996463825f 100644 --- a/exp/api/v1beta1/azuremanagedmachinepool_webhook.go +++ b/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 @@ -223,6 +225,48 @@ func (m *AzureManagedMachinePool) ValidateUpdate(oldRaw runtime.Object, client c "field is immutable, unsetting is not allowed")) } } + + if old.Spec.ScaleSetPriority != nil { + // Prevent ScaleSetPriority modification if it was already set to some value + if m.Spec.ScaleSetPriority == nil { + // unsetting the field is not allowed + allErrs = append(allErrs, + field.Invalid( + field.NewPath("Spec", "ScaleSetPriority"), + m.Spec.ScaleSetPriority, + "field is immutable, unsetting is not allowed")) + } else if *m.Spec.ScaleSetPriority != *old.Spec.ScaleSetPriority { + // changing the field is not allowed + allErrs = append(allErrs, + field.Invalid( + field.NewPath("Spec", "ScaleSetPriority"), + m.Spec.ScaleSetPriority, + "field is immutable")) + } + } else { + if m.Spec.ScaleSetPriority != nil { + allErrs = append(allErrs, + field.Invalid( + field.NewPath("Spec", "ScaleSetPriority"), + m.Spec.ScaleSetPriority, + "field is immutable, unsetting is not allowed")) + } + } + + updateValidators := []func() error{ + m.validateNodeLabels, + } + + for _, validator := range updateValidators { + if err := validator(); err != nil { + allErrs = append(allErrs, + field.Invalid( + field.NewPath("Spec", "NodeLabels"), + m.Spec.NodeLabels, + fmt.Sprintf("Node pool labels must not start with %s", azureutil.AzureSystemNodeLabelPrefix))) + } + } + if len(allErrs) != 0 { return apierrors.NewInvalid(GroupVersion.WithKind("AzureManagedMachinePool").GroupKind(), m.Name, allErrs) } @@ -319,6 +363,21 @@ func (m *AzureManagedMachinePool) validateName() error { return nil } +func (m *AzureManagedMachinePool) validateNodeLabels() error { + if m.Spec.NodeLabels != nil { + for key := range m.Spec.NodeLabels { + if azureutil.IsAzureSystemNodeLabelKey(key) { + return field.Invalid( + field.NewPath("Spec", "NodeLabels"), + m.Spec.NodeLabels[key], + fmt.Sprintf("Node pool label must not start with %s", azureutil.AzureSystemNodeLabelPrefix)) + } + } + } + + return nil +} + func ensureStringSlicesAreEqual(a []string, b []string) bool { if len(a) != len(b) { return false diff --git a/exp/api/v1beta1/azuremanagedmachinepool_webhook_test.go b/exp/api/v1beta1/azuremanagedmachinepool_webhook_test.go index 8b708d55997d..bcf4082fed6f 100644 --- a/exp/api/v1beta1/azuremanagedmachinepool_webhook_test.go +++ b/exp/api/v1beta1/azuremanagedmachinepool_webhook_test.go @@ -463,6 +463,25 @@ func TestAzureManagedMachinePoolUpdatingWebhook(t *testing.T) { }, wantErr: true, }, + { + 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 { @@ -581,6 +600,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 { diff --git a/exp/api/v1beta1/zz_generated.deepcopy.go b/exp/api/v1beta1/zz_generated.deepcopy.go index 3f3cf8bb8607..76fe1d8412a9 100644 --- a/exp/api/v1beta1/zz_generated.deepcopy.go +++ b/exp/api/v1beta1/zz_generated.deepcopy.go @@ -844,6 +844,11 @@ func (in *AzureManagedMachinePoolSpec) DeepCopyInto(out *AzureManagedMachinePool *out = new(string) **out = **in } + if in.ScaleSetPriority != nil { + in, out := &in.ScaleSetPriority, &out.ScaleSetPriority + *out = new(string) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new AzureManagedMachinePoolSpec. diff --git a/templates/cluster-template-aks.yaml b/templates/cluster-template-aks.yaml index e141f84ed14d..cb40bb96744a 100644 --- a/templates/cluster-template-aks.yaml +++ b/templates/cluster-template-aks.yaml @@ -97,6 +97,7 @@ metadata: spec: mode: User osDiskSizeGB: 40 + scaleSetPriority: Spot sku: ${AZURE_NODE_MACHINE_TYPE} --- apiVersion: infrastructure.cluster.x-k8s.io/v1beta1 diff --git a/templates/flavors/aks/cluster-template.yaml b/templates/flavors/aks/cluster-template.yaml index c4f0a18134f6..30f082b693a3 100644 --- a/templates/flavors/aks/cluster-template.yaml +++ b/templates/flavors/aks/cluster-template.yaml @@ -104,3 +104,4 @@ spec: mode: User osDiskSizeGB: 40 sku: "${AZURE_NODE_MACHINE_TYPE}" + scaleSetPriority: Spot diff --git a/templates/test/ci/cluster-template-prow-aks.yaml b/templates/test/ci/cluster-template-prow-aks.yaml index d6a17e89dcff..88ba731bb5bf 100644 --- a/templates/test/ci/cluster-template-prow-aks.yaml +++ b/templates/test/ci/cluster-template-prow-aks.yaml @@ -114,6 +114,7 @@ spec: type: shared osDiskSizeGB: 40 osDiskType: Ephemeral + scaleSetPriority: Spot sku: ${AZURE_NODE_MACHINE_TYPE} taints: - effect: NoSchedule diff --git a/util/azure/azure.go b/util/azure/azure.go index 79b9f5eedd28..4083de41c1af 100644 --- a/util/azure/azure.go +++ b/util/azure/azure.go @@ -24,6 +24,8 @@ import ( var azureResourceGroupNameRE = regexp.MustCompile(`.*/subscriptions/(?:.*)/resourceGroups/(.+)/providers/(?:.*)`) +const AzureSystemNodeLabelPrefix = "kubernetes.azure.com" + // ConvertResourceGroupNameToLower converts the resource group name in the resource ID to be lowered. // Inspired by https://github.com/kubernetes-sigs/cloud-provider-azure/blob/88c9b89611e7c1fcbd39266928cce8406eb0e728/pkg/provider/azure_wrap.go#L409 func ConvertResourceGroupNameToLower(resourceID string) (string, error) { @@ -35,3 +37,8 @@ func ConvertResourceGroupNameToLower(resourceID string) (string, error) { resourceGroup := matches[1] return strings.Replace(resourceID, resourceGroup, strings.ToLower(resourceGroup), 1), nil } + +// IsAzureSystemNodeLabelKey is a helper function that determines whether a node label key is an Azure "system" label. +func IsAzureSystemNodeLabelKey(labelKey string) bool { + return strings.HasPrefix(labelKey, AzureSystemNodeLabelPrefix) +}