Implement ScaleSetPriority for AzureManagedMachinePool #2604

jackfrancis · 2022-08-25T17:45:59Z

What type of PR is this?

/kind feature
/kind bug

What this PR does / why we need it:

This PR implements the ScaleSetPriority feature for AzureManagedMachinePool. See the official AKS documentation for Spot node pools here:

https://docs.microsoft.com/en-us/azure/aks/spot-node-pool

This PR ensures that we don't override (e.g., accidentally delete) any AKS-enforced labels that may make it to the user-configurable part of the the node pool labels configuration surface area.

Until this AKS issue is addressed, capz must have this adaptive update behavior:

[BUG] kubernetes.azure.com/scalesetpriority:spot is user-overrideable Azure/AKS#3152

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #2460

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

Implement ScaleSetPriority for AzureManagedMachinePool

k8s-ci-robot · 2022-08-25T17:46:00Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

jackfrancis · 2022-08-25T23:01:02Z

exp/api/v1beta1/azuremanagedmachinepool_webhook.go

@@ -319,6 +336,21 @@ func (m *AzureManagedMachinePool) validateName() error {
 	return nil
 }

+func (m *AzureManagedMachinePool) validateNodeLabels() error {


I don't think it's possible to do this validation in the kubebuilder API type spec (e.g., using the Pattern= interface to define a required regular expression match) because the API type is a map[string]string.

zmalik · 2022-08-29T19:48:37Z

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


@jackfrancis if you send the Regular value. AKS isn't setting it to Regular but returns the empty.

Later any other change to nodepool will fail as Azure API returns the error that it cannot change from "" to Regular

I wasn't able to reproduce this with labels changes to the node pool. Which node pool changes were you seeing this on when ScaleSetPriority was set to "Regular"?

Again I did some more manual tests and this looks fine. Creating a cluster with an explicit scaleSetPriority: Regular AzureManagedMachinePool configuration does not have any apparent negative side-effects.

I think this PR is ready to gol

still doesn't work for me.

steps to reproduce:

create an agentpool with explicit ScaleSetPriority set to Regular

verify in az aks nodepool show --cluster-name cluster-name -n nodepool-name that it has "scaleSetPriority": null,

try to change a label in agentpool and see reconcile error Message=\"Changing property 'properties.ScaleSetPriority' is not allowed.

Are you testing using a build from this branch? In this current PR implementation, I definitely can't reproduce. I do observe what you're seeing from the AKS API:

$ az aks nodepool show --cluster-name capz-e2e-l9d26h-aks -n pool1 -g capz-e2e-l9d26h-aks | grep 'scaleSetPriority' "scaleSetPriority": null,

However, I'm able to continually update the node pool using capz (using node labels below as an example):

$ k get azuremanagedmachinepool/pool1 -n capz-e2e-l9d26h -o yaml | grep -A 5 ' nodeLabels' nodeLabels: foo: bar osDiskSizeGB: 40 osDiskType: Ephemeral osType: Linux providerIDList:

You can see I've already applied a foo: bar node label. I'll now add a 2nd label:

$ k edit azuremanagedmachinepool/pool1 -n capz-e2e-l9d26h azuremanagedmachinepool.infrastructure.cluster.x-k8s.io/pool1 edited $ k get azuremanagedmachinepool/pool1 -n capz-e2e-l9d26h -o yaml | grep -A 5 ' nodeLabels' nodeLabels: foo: bar hello: world osDiskSizeGB: 40 osDiskType: Ephemeral osType: Linux

Now if I get the node pool data from the AKS API I see the updated label:

$ az aks nodepool show --cluster-name capz-e2e-l9d26h-aks -n pool1 -g capz-e2e-l9d26h-aks | grep -A 5 'nodeLabels' "nodeLabels": { "foo": "bar", "hello": "world" }, "nodePublicIpPrefixId": null, "nodeTaints": [

As expected, ScaleSetPriority is unchanged from either source:

$ az aks nodepool show --cluster-name capz-e2e-l9d26h-aks -n pool1 -g capz-e2e-l9d26h-aks | grep 'scaleSetPriority' "scaleSetPriority": null, $ k get azuremanagedmachinepool/pool1 -n capz-e2e-l9d26h -o yaml | grep ' scaleSetPriority' scaleSetPriority: Regular

I think this PR has overcome this particular pathology. We should ship add'l E2E tests with this PR to prove it, but I'm confident we're good here.

definitely its working now, does Azure API changed something?
in any case I agree to merge this

jackfrancis · 2022-09-28T23:16:47Z

@nojnhuh @CecileRobertMichon I'd like to advocate we move forward with this after further reviewing the existing code for vulnerabilities to the introduction of lots of requests (and thus API throttling). For the record, the scenario I'm concerned about it:

Scale out a spot pool successfully
Azure recalculates spot pricing and deletes the vms that underlie our cluster nodes
capz continually initiates a PUT against Azure APIs to restore its source of goal state truth

I think there is possibly some work to do to streamline how capz and Azure communicate in spot scenarios, but I'm not concerned about exponential request leakage to Azure.

tl;dr ready for final review

nojnhuh

left a couple nits, overall lgtm

exp/api/v1beta1/azuremanagedmachinepool_webhook.go

nojnhuh · 2022-09-30T15:46:49Z

exp/api/v1beta1/azuremanagedmachinepool_webhook_test.go

 	}
 	var client client.Client
 	for _, tc := range tests {
+		tc := tc


FWIW I don't think doing this is necessary here because we're never taking the address of tc in the subtest (like with &tc). Certainly doesn't hurt to keep it here though in case this test needs to do that in the future.

Two points:

It's correct that we do this as a best-practice to avoid the non-deterministic UT outcomes that result from not doing this when we need to.

Doing it all the time as a rote practice tends to make the developer forget the exact edge case root cause that makes this pattern a necessary thing and it becomes a kind of incantation.

🤷

exp/api/v1beta1/azuremanagedmachinepool_webhook.go

jackfrancis · 2022-10-05T21:50:20Z

/retest

azure/services/agentpools/spec.go

CecileRobertMichon · 2022-10-05T22:38:57Z

azure/services/agentpools/spec.go

+
+// 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 {


does this intentionally drop any labels that are not system labels (ie. no kubernetes.azure.com prefix) but aren't part of the CAPZ spec? for example if they were added by some external process

Yes. I think the idea here is that capz is the "exclusive" maintainer of labels, and the update interface has been defined such that every time you push a labels change you need to include the entire set of labels. Ref:

https://learn.microsoft.com/en-us/azure/aks/use-labels#updating-labels-on-existing-node-pools

The think about the kubernetes.azure.com-prefixed labels is (IMO) and AKS bug, and hopefully we can drop this foo at some point in the future.

Azure/AKS#3152

CecileRobertMichon

/lgtm
/assign @nojnhuh

k8s-ci-robot · 2022-10-06T23:32:18Z

@CecileRobertMichon: GitHub didn't allow me to assign the following users: nojnhuh.

Note that only kubernetes-sigs members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/lgtm
/assign @nojnhuh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

nojnhuh · 2022-10-07T05:12:51Z

/assign

jackfrancis · 2022-10-07T16:36:37Z

/retest

nojnhuh

LGTM

CecileRobertMichon · 2022-10-10T20:32:51Z

/approve

k8s-ci-robot · 2022-10-10T20:33:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [CecileRobertMichon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested review from devigned and mboersma August 25, 2022 17:46

jackfrancis force-pushed the synthesize-aks-capz-nodepool-labels branch from 66f13f0 to 9afb6b4 Compare August 25, 2022 21:56

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 25, 2022

jackfrancis marked this pull request as ready for review August 25, 2022 21:56

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 25, 2022

k8s-ci-robot requested a review from alexeldeib August 25, 2022 21:57

jackfrancis changed the title ~~synthesize node label ownership between capz and AKS~~ don't override AKS system node pool labels Aug 25, 2022

jackfrancis force-pushed the synthesize-aks-capz-nodepool-labels branch 2 times, most recently from a7d6791 to 3b77f73 Compare August 25, 2022 22:54

jackfrancis commented Aug 25, 2022

View reviewed changes

jackfrancis force-pushed the synthesize-aks-capz-nodepool-labels branch 2 times, most recently from 27355ce to 4548d7f Compare August 29, 2022 18:45

jackfrancis changed the title ~~don't override AKS system node pool labels~~ WIP: Implement ScaleSetPriority for AzureManagedMachinePool Aug 29, 2022

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 29, 2022

jackfrancis added area/managedclusters Issues related to managed AKS clusters created through the CAPZ ManagedCluster Type and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Aug 29, 2022

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 29, 2022

zmalik reviewed Aug 29, 2022

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 31, 2022

jackfrancis force-pushed the synthesize-aks-capz-nodepool-labels branch from 4548d7f to efc23fc Compare August 31, 2022 22:38

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 31, 2022

jackfrancis force-pushed the synthesize-aks-capz-nodepool-labels branch from efc23fc to a5b45c8 Compare August 31, 2022 22:57

jackfrancis mentioned this pull request Sep 15, 2022

standardize scope local cache #2657

Closed

3 tasks

jackfrancis requested a review from CecileRobertMichon September 26, 2022 18:59

jackfrancis added this to the v1.6 milestone Sep 29, 2022

nojnhuh reviewed Sep 30, 2022

View reviewed changes

jackfrancis force-pushed the synthesize-aks-capz-nodepool-labels branch from 7937424 to daf9414 Compare September 30, 2022 17:19

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 5, 2022

ScaleSetPriority for AzureManagedMachinePool

d498079

jackfrancis force-pushed the synthesize-aks-capz-nodepool-labels branch from daf9414 to d498079 Compare October 5, 2022 20:39

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 5, 2022

CecileRobertMichon reviewed Oct 5, 2022

View reviewed changes

azure/services/agentpools/spec.go Show resolved Hide resolved

CecileRobertMichon reviewed Oct 5, 2022

View reviewed changes

jackfrancis added kind/feature Categorizes issue or PR as related to a new feature. and removed kind/bug Categorizes issue or PR as related to a bug. labels Oct 6, 2022

CecileRobertMichon reviewed Oct 6, 2022

View reviewed changes

k8s-ci-robot assigned CecileRobertMichon Oct 6, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 6, 2022

k8s-ci-robot assigned nojnhuh Oct 7, 2022

nojnhuh approved these changes Oct 10, 2022

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 10, 2022

k8s-ci-robot merged commit 724a4e7 into kubernetes-sigs:main Oct 10, 2022

jackfrancis mentioned this pull request Dec 7, 2022

[AKS] node labels update remove the AKS system labels #2603

Closed

jackfrancis deleted the synthesize-aks-capz-nodepool-labels branch December 9, 2022 22:33

CecileRobertMichon mentioned this pull request Jan 23, 2023

SpotVMOptions with AzureManagedMachinePool Supported? #3070

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement ScaleSetPriority for AzureManagedMachinePool #2604

Implement ScaleSetPriority for AzureManagedMachinePool #2604

jackfrancis commented Aug 25, 2022 •

edited

Loading

k8s-ci-robot commented Aug 25, 2022

jackfrancis Aug 25, 2022

zmalik Aug 29, 2022

jackfrancis Aug 29, 2022

jackfrancis Sep 6, 2022

zmalik Sep 7, 2022

jackfrancis Sep 9, 2022

zmalik Sep 13, 2022

jackfrancis commented Sep 28, 2022

nojnhuh left a comment

nojnhuh Sep 30, 2022

jackfrancis Sep 30, 2022

jackfrancis commented Oct 5, 2022

CecileRobertMichon Oct 5, 2022

jackfrancis Oct 5, 2022

jackfrancis Oct 5, 2022

CecileRobertMichon left a comment

k8s-ci-robot commented Oct 6, 2022

nojnhuh commented Oct 7, 2022

jackfrancis commented Oct 7, 2022

nojnhuh left a comment

CecileRobertMichon commented Oct 10, 2022

k8s-ci-robot commented Oct 10, 2022

Implement ScaleSetPriority for AzureManagedMachinePool #2604

Implement ScaleSetPriority for AzureManagedMachinePool #2604

Conversation

jackfrancis commented Aug 25, 2022 • edited Loading

k8s-ci-robot commented Aug 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackfrancis commented Sep 28, 2022

nojnhuh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackfrancis commented Oct 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CecileRobertMichon left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 6, 2022

nojnhuh commented Oct 7, 2022

jackfrancis commented Oct 7, 2022

nojnhuh left a comment

Choose a reason for hiding this comment

CecileRobertMichon commented Oct 10, 2022

k8s-ci-robot commented Oct 10, 2022

jackfrancis commented Aug 25, 2022 •

edited

Loading