From d498079f5a4174d1adecbea0047718b84111c92d Mon Sep 17 00:00:00 2001 From: Jack Francis Date: Wed, 5 Oct 2022 13:39:34 -0700 Subject: [PATCH] ScaleSetPriority for AzureManagedMachinePool --- azure/converters/managedagentpool.go | 1 + azure/scope/managedmachinepool.go | 1 + azure/services/agentpools/spec.go | 27 ++++++- azure/services/agentpools/spec_test.go | 79 +++++++++++++++++++ ...ter.x-k8s.io_azuremanagedmachinepools.yaml | 7 ++ .../azuremanagedmachinepool_conversion.go | 1 + exp/api/v1alpha3/zz_generated.conversion.go | 1 + .../azuremanagedmachinepool_conversion.go | 1 + exp/api/v1alpha4/zz_generated.conversion.go | 1 + .../v1beta1/azuremanagedmachinepool_types.go | 5 ++ .../azuremanagedmachinepool_webhook.go | 30 +++++++ .../azuremanagedmachinepool_webhook_test.go | 48 +++++++++++ exp/api/v1beta1/zz_generated.deepcopy.go | 5 ++ templates/cluster-template-aks.yaml | 1 + templates/flavors/aks/cluster-template.yaml | 1 + .../test/ci/cluster-template-prow-aks.yaml | 1 + util/azure/azure.go | 8 ++ 17 files changed, 217 insertions(+), 1 deletion(-) diff --git a/azure/converters/managedagentpool.go b/azure/converters/managedagentpool.go index 76719332c37..46720635ed7 100644 --- a/azure/converters/managedagentpool.go +++ b/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, } } diff --git a/azure/scope/managedmachinepool.go b/azure/scope/managedmachinepool.go index add451ba5fb..72fcb91c935 100644 --- a/azure/scope/managedmachinepool.go +++ b/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 { diff --git a/azure/services/agentpools/spec.go b/azure/services/agentpools/spec.go index 9abe89d41e3..a2235aeba1d 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. @@ -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,12 +213,13 @@ 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, @@ -215,3 +227,16 @@ func (s *AgentPoolSpec) Parameters(existing interface{}) (params interface{}, er }, }, 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/azure/services/agentpools/spec_test.go b/azure/services/agentpools/spec_test.go index 5064d7db33b..2f86d355263 100644 --- a/azure/services/agentpools/spec_test.go +++ b/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)) + }) + } +} 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 e18b1049380..c3ec0e8e5cc 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_azuremanagedmachinepools.yaml +++ b/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. diff --git a/exp/api/v1alpha3/azuremanagedmachinepool_conversion.go b/exp/api/v1alpha3/azuremanagedmachinepool_conversion.go index 7f0e6ffee14..d7b30f45a2f 100644 --- a/exp/api/v1alpha3/azuremanagedmachinepool_conversion.go +++ b/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 diff --git a/exp/api/v1alpha3/zz_generated.conversion.go b/exp/api/v1alpha3/zz_generated.conversion.go index e6e001a507b..92866660887 100644 --- a/exp/api/v1alpha3/zz_generated.conversion.go +++ b/exp/api/v1alpha3/zz_generated.conversion.go @@ -892,6 +892,7 @@ func autoConvert_v1beta1_AzureManagedMachinePoolSpec_To_v1alpha3_AzureManagedMac // 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.EnableNodePublicIP 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/azuremanagedmachinepool_conversion.go b/exp/api/v1alpha4/azuremanagedmachinepool_conversion.go index 8717a54983f..ebeaadbbb91 100644 --- a/exp/api/v1alpha4/azuremanagedmachinepool_conversion.go +++ b/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 diff --git a/exp/api/v1alpha4/zz_generated.conversion.go b/exp/api/v1alpha4/zz_generated.conversion.go index c34c29438d4..ffa5d9d1702 100644 --- a/exp/api/v1alpha4/zz_generated.conversion.go +++ b/exp/api/v1alpha4/zz_generated.conversion.go @@ -1192,6 +1192,7 @@ func autoConvert_v1beta1_AzureManagedMachinePoolSpec_To_v1alpha4_AzureManagedMac // 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.EnableNodePublicIP 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 a433741494b..235f1d4dc84 100644 --- a/exp/api/v1beta1/azuremanagedmachinepool_types.go +++ b/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. diff --git a/exp/api/v1beta1/azuremanagedmachinepool_webhook.go b/exp/api/v1beta1/azuremanagedmachinepool_webhook.go index c17a833118d..b79434065a1 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 @@ -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 diff --git a/exp/api/v1beta1/azuremanagedmachinepool_webhook_test.go b/exp/api/v1beta1/azuremanagedmachinepool_webhook_test.go index 67194e13103..b449b84a6f6 100644 --- a/exp/api/v1beta1/azuremanagedmachinepool_webhook_test.go +++ b/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 { diff --git a/exp/api/v1beta1/zz_generated.deepcopy.go b/exp/api/v1beta1/zz_generated.deepcopy.go index 03250a54dc9..b5460bf39cd 100644 --- a/exp/api/v1beta1/zz_generated.deepcopy.go +++ b/exp/api/v1beta1/zz_generated.deepcopy.go @@ -856,6 +856,11 @@ func (in *AzureManagedMachinePoolSpec) DeepCopyInto(out *AzureManagedMachinePool *out = new(bool) **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 ac166157c88..4c158595846 100644 --- a/templates/cluster-template-aks.yaml +++ b/templates/cluster-template-aks.yaml @@ -99,6 +99,7 @@ spec: enableNodePublicIP: false mode: User osDiskSizeGB: 40 + scaleSetPriority: Regular 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 2da2216f41a..20003f1b75c 100644 --- a/templates/flavors/aks/cluster-template.yaml +++ b/templates/flavors/aks/cluster-template.yaml @@ -106,3 +106,4 @@ spec: osDiskSizeGB: 40 sku: "${AZURE_NODE_MACHINE_TYPE}" enableNodePublicIP: false + scaleSetPriority: Regular diff --git a/templates/test/ci/cluster-template-prow-aks.yaml b/templates/test/ci/cluster-template-prow-aks.yaml index 60098f0f049..1a629f4abfc 100644 --- a/templates/test/ci/cluster-template-prow-aks.yaml +++ b/templates/test/ci/cluster-template-prow-aks.yaml @@ -116,6 +116,7 @@ spec: type: shared osDiskSizeGB: 40 osDiskType: Ephemeral + scaleSetPriority: Regular sku: ${AZURE_NODE_MACHINE_TYPE} taints: - effect: NoSchedule diff --git a/util/azure/azure.go b/util/azure/azure.go index 79b9f5eedd2..a5801d3126b 100644 --- a/util/azure/azure.go +++ b/util/azure/azure.go @@ -24,6 +24,9 @@ import ( var azureResourceGroupNameRE = regexp.MustCompile(`.*/subscriptions/(?:.*)/resourceGroups/(.+)/providers/(?:.*)`) +// AzureSystemNodeLabelPrefix is a standard node label prefix for Azure features, e.g., kubernetes.azure.com/scalesetpriority. +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 +38,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) +}