fix(machinepool): clear VirtualMachineProfile if no model/custom data update #5164

mweibel · 2024-10-07T14:24:12Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
Scale up/down with MachinePool always updates the VM image model to use. This change sets the VirtualMachineProfile to nil when no change is necessary and ensures therefore less churn on scale up/downs leading to model updates which may require manual fixing

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 #4958

Special notes for your reviewer:

cherry-pick candidate

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

improves MachinePool scale up/down to avoid unnecessary VMSS VM model udpates

k8s-ci-robot · 2024-10-07T14:24:19Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign justinsb for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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 · 2024-10-07T14:24:23Z

Hi @mweibel. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

mweibel · 2024-10-07T14:26:06Z

azure/services/scalesets/spec_test.go

-	existingVMSS.Properties.AdditionalCapabilities = &armcompute.AdditionalCapabilities{UltraSSDEnabled: ptr.To(true)}

 	clone := newDefaultExistingVMSS("VM_SIZE")
 	clone.SKU.Capacity = ptr.To[int64](3)
 	clone.Properties.AdditionalCapabilities = &armcompute.AdditionalCapabilities{UltraSSDEnabled: ptr.To(true)}
 	clone.Properties.VirtualMachineProfile.NetworkProfile = nil

 	clone.Properties.VirtualMachineProfile.StorageProfile.ImageReference.Version = ptr.To("2.0")
-	clone.Properties.VirtualMachineProfile.NetworkProfile = nil


FYI those two lines I removed because they were duplicates.

nawazkh · 2024-10-07T16:18:53Z

/ok-to-test

codecov · 2024-10-07T21:47:02Z

Codecov Report

Attention: Patch coverage is 63.63636% with 4 lines in your changes missing coverage. Please review.

Project coverage is 51.36%. Comparing base (4fec820) to head (19d3251).
Report is 12 commits behind head on main.

Files with missing lines	Patch %	Lines
azure/scope/machinepool.go	0.00%	4 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5164   +/-   ##
=======================================
  Coverage   51.35%   51.36%           
=======================================
  Files         273      273           
  Lines       24671    24676    +5     
=======================================
+ Hits        12670    12675    +5     
  Misses      11212    11212           
  Partials      789      789

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

willie-yao · 2024-10-08T20:37:33Z

azure/scope/machinepool.go

@@ -222,10 +222,8 @@ func (m *MachinePoolScope) ScaleSetSpec(ctx context.Context) azure.ResourceSpecG
 	}

 	if m.cache != nil {
-		if m.HasReplicasExternallyManaged(ctx) {
-			spec.ShouldPatchCustomData = m.cache.HasBootstrapDataChanges


For my own understanding: why are we removing the check for m.HasReplicasExternallyManaged(ctx)?

further below in ScaleSetSpec#existingParameters we determine whether we need to set VirtualMachineProfile to nil based on whether there are model changes or the ShouldPatchCustomData flag is being set. This flag will now be used for all kind of autoscaling, not just externally managed. We don't know otherwise if the custom data changed or not (since, when fetching a VMSS, you don't receive the customData for security reasons).

… update Scale up/down with MachinePool always updates the VM image model to use. This change sets the VirtualMachineProfile to nil when no change is necessary and ensures therefore less churn on scale up/downs leading to model updates which may require manual fixing Fixes some linting errors related to unnecessary params, too.

mweibel · 2024-10-09T11:17:33Z

/retest

^ looks like a flake

willie-yao

/lgtm
/test all

k8s-ci-robot · 2024-10-09T16:55:04Z

LGTM label has been added.

Git tree hash: 37ebd22eb8d86832791cd02014e08a101e29b9b4

willie-yao · 2024-10-09T16:55:38Z

/assign @nawazkh

nawazkh · 2024-10-09T17:34:44Z

/test ls

k8s-ci-robot · 2024-10-09T17:34:46Z

@nawazkh: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test pull-cluster-api-provider-azure-apiversion-upgrade
/test pull-cluster-api-provider-azure-build
/test pull-cluster-api-provider-azure-ci-entrypoint
/test pull-cluster-api-provider-azure-e2e
/test pull-cluster-api-provider-azure-e2e-aks
/test pull-cluster-api-provider-azure-test
/test pull-cluster-api-provider-azure-verify

The following commands are available to trigger optional jobs:

/test pull-cluster-api-provider-azure-apidiff
/test pull-cluster-api-provider-azure-capi-e2e
/test pull-cluster-api-provider-azure-conformance
/test pull-cluster-api-provider-azure-conformance-custom-builds
/test pull-cluster-api-provider-azure-conformance-dual-stack-with-ci-artifacts
/test pull-cluster-api-provider-azure-conformance-ipv6-with-ci-artifacts
/test pull-cluster-api-provider-azure-conformance-with-ci-artifacts
/test pull-cluster-api-provider-azure-e2e-optional
/test pull-cluster-api-provider-azure-e2e-workload-upgrade
/test pull-cluster-api-provider-azure-load-test-custom-builds
/test pull-cluster-api-provider-azure-windows-custom-builds
/test pull-cluster-api-provider-azure-windows-with-ci-artifacts

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-provider-azure-apidiff
pull-cluster-api-provider-azure-build
pull-cluster-api-provider-azure-e2e
pull-cluster-api-provider-azure-e2e-aks
pull-cluster-api-provider-azure-test
pull-cluster-api-provider-azure-verify

In response to this:

/test ls

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-sigs/prow repository.

nawazkh · 2024-10-09T19:50:11Z

I will need some time understanding this change, I hope it is ok to hold for a day more.

mweibel · 2024-10-09T19:51:23Z

sure, take your time and let me know if there are questions.

k8s-ci-robot requested review from Jont828 and mboersma October 7, 2024 14:24

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 7, 2024

mweibel force-pushed the fix-vmss-update-models branch from 06f39bd to 19d3251 Compare October 7, 2024 14:24

k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Oct 7, 2024

mweibel commented Oct 7, 2024

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 7, 2024

willie-yao reviewed Oct 8, 2024

View reviewed changes

mweibel force-pushed the fix-vmss-update-models branch from 19d3251 to c8cf42a Compare October 9, 2024 09:24

willie-yao approved these changes Oct 9, 2024

View reviewed changes

k8s-ci-robot assigned willie-yao Oct 9, 2024

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

k8s-ci-robot assigned nawazkh Oct 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(machinepool): clear VirtualMachineProfile if no model/custom data update #5164

fix(machinepool): clear VirtualMachineProfile if no model/custom data update #5164

mweibel commented Oct 7, 2024

k8s-ci-robot commented Oct 7, 2024

k8s-ci-robot commented Oct 7, 2024

mweibel Oct 7, 2024

nawazkh commented Oct 7, 2024

codecov bot commented Oct 7, 2024

willie-yao Oct 8, 2024

mweibel Oct 9, 2024

mweibel commented Oct 9, 2024

willie-yao left a comment

k8s-ci-robot commented Oct 9, 2024

willie-yao commented Oct 9, 2024

nawazkh commented Oct 9, 2024

k8s-ci-robot commented Oct 9, 2024

nawazkh commented Oct 9, 2024

mweibel commented Oct 9, 2024

fix(machinepool): clear VirtualMachineProfile if no model/custom data update #5164

Are you sure you want to change the base?

fix(machinepool): clear VirtualMachineProfile if no model/custom data update #5164

Conversation

mweibel commented Oct 7, 2024

k8s-ci-robot commented Oct 7, 2024

k8s-ci-robot commented Oct 7, 2024

mweibel Oct 7, 2024

Choose a reason for hiding this comment

nawazkh commented Oct 7, 2024

codecov bot commented Oct 7, 2024

Codecov Report

willie-yao Oct 8, 2024

Choose a reason for hiding this comment

mweibel Oct 9, 2024

Choose a reason for hiding this comment

mweibel commented Oct 9, 2024

willie-yao left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 9, 2024

willie-yao commented Oct 9, 2024

nawazkh commented Oct 9, 2024

k8s-ci-robot commented Oct 9, 2024

nawazkh commented Oct 9, 2024

mweibel commented Oct 9, 2024