Make scaleset reconcile/delete async by Jont828 · Pull Request #3111 · kubernetes-sigs/cluster-api-provider-azure

Jont828 · 2023-01-27T21:00:07Z

What type of PR is this?

/kind feature
What this PR does / why we need it: Implementation of an async service for managed agent pools as part of an effort to make all services async. See #1610 and #1541.

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

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:

Make scalesets reconcile/delete async

k8s-ci-robot · 2023-01-27T21:00:09Z

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

azure/scope/machinepool.go

codecov · 2023-06-12T23:23:31Z

Codecov Report

Patch coverage: 56.42% and project coverage change: -0.80% ⚠️

Comparison is base (8893daa) 54.77% compared to head (975ba45) 53.97%.
Report is 42 commits behind head on main.

❗ Current head 975ba45 differs from pull request most recent head 373518e. Consider uploading reports for the commit 373518e to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3111      +/-   ##
==========================================
- Coverage   54.77%   53.97%   -0.80%     
==========================================
  Files         187      188       +1     
  Lines       19098    18853     -245     
==========================================
- Hits        10460    10176     -284     
- Misses       8070     8135      +65     
+ Partials      568      542      -26

Files Changed	Coverage Δ
azure/scope/machinepool.go	`29.31% <0.00%> (-2.13%)`	⬇️
azure/services/scalesets/client.go	`0.00% <0.00%> (ø)`
exp/controllers/azuremachinepool_controller.go	`30.41% <0.00%> (-0.13%)`	⬇️
azure/services/roleassignments/roleassignments.go	`68.81% <63.63%> (-0.24%)`	⬇️
azure/services/scalesets/scalesets.go	`66.11% <70.88%> (-3.73%)`	⬇️
azure/services/scalesets/spec.go	`74.59% <74.59%> (ø)`
azure/converters/vmss.go	`93.64% <100.00%> (ø)`

... and 16 files with indirect coverage changes

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

Jont828 · 2023-08-10T20:59:08Z

@mboersma I've opened #3825 and removed the TODO.

mboersma · 2023-08-10T21:17:43Z

@Jont828 looks good! There are still a few outstanding comments AFAICT, hidden in the collapsed middle section of this epic PR:
#3111 (comment)
#3111 (comment)
#3111 (comment)

willie-yao · 2023-08-11T20:07:07Z

/lgtm

k8s-ci-robot · 2023-08-11T20:07:13Z

LGTM label has been added.

Details

Git tree hash: 24fa3fe276e692de983ff1b346338ddcc190636d

nawazkh

Great work @Jont828! Thanks for putting this together and working this out!
Also, I am sorry for the last-minute reviews. I had started the review earlier and then get diverted and had to shift my focus.
I had some minor nits and have jotted them below.

azure/scope/machinepool.go

azure/services/roleassignments/roleassignments.go

nawazkh · 2023-08-11T20:16:22Z

azure/services/roleassignments/roleassignments.go

 	if err != nil {
 		return nil, errors.Wrap(err, "failed to get principal ID for VMSS")
 	}


Should this be updated to below?

Suggested change

if err != nil {

return nil, errors.Wrap(err, "failed to get principal ID for VMSS")

}

if err != nil {

return nil, errors.Wrap(err, "failed to get resultVMSSIface")

}

On second thought, I think we want to leave it as is. Before, we were giving the error when we failed to get the VM, which we're still doing here. The error err should explain that we failed the Get() call while the wrapper message gives context as to why we needed to fetch the VMSS to begin with.

azure/services/scalesets/scalesets.go

azure/scope/machinepool.go

nawazkh · 2023-08-14T23:03:25Z

Just one nit 😬
Everything else looks good to me pending tests 🌚

nawazkh

Great work @Jont828 🚀
Thank you for going over the comments so patiently.
/lgtm

k8s-ci-robot · 2023-08-16T02:56:34Z

LGTM label has been added.

Details

Git tree hash: 13bae01c0dd22606daefe1fa94d7f2cc0a45169f

nawazkh · 2023-08-16T02:56:49Z

/test pull-cluster-api-provider-azure-e2e-optional

CecileRobertMichon

/lgtm
/approve

🚀

k8s-ci-robot · 2023-08-16T19:00:31Z

[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

Details

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

Jont828 · 2023-08-16T21:49:33Z

🚀 🚀 🚀

nawazkh · 2023-08-16T21:59:36Z

🚀

devigned · 2023-08-16T22:22:09Z

Awesome work!! 🚀

willie-yao · 2023-08-17T06:07:55Z

🚀 💯

k8s-ci-robot requested review from marosset and mboersma January 27, 2023 21:00

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 27, 2023

jackfrancis added this to the next milestone Mar 2, 2023

Jont828 force-pushed the async-scalesets branch from 5dd7ac2 to 741e9cd Compare March 2, 2023 21:44

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

mboersma modified the milestones: next, v1.9 Mar 9, 2023

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

mweibel reviewed Mar 21, 2023

View reviewed changes

azure/scope/machinepool.go Outdated Show resolved Hide resolved

jackfrancis modified the milestones: v1.9, next Apr 13, 2023

jackfrancis modified the milestone: next May 11, 2023

Jont828 force-pushed the async-scalesets branch 2 times, most recently from 65f4b5d to 83aa946 Compare May 23, 2023 17:29

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

Jont828 force-pushed the async-scalesets branch from 8182e36 to 6fb90d2 Compare May 24, 2023 21:37

Jont828 marked this pull request as ready for review June 7, 2023 16:39

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 Jun 7, 2023

k8s-ci-robot requested review from jackfrancis and jsturtevant June 7, 2023 16:39

Jont828 force-pushed the async-scalesets branch from ddffcd2 to 4f2cd6c Compare June 15, 2023 00:06

k8s-ci-robot requested a review from mboersma August 10, 2023 20:59

Jont828 force-pushed the async-scalesets branch from 766ce25 to d544d79 Compare August 11, 2023 19:55

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 11, 2023

nawazkh reviewed Aug 11, 2023

View reviewed changes

Jont828 force-pushed the async-scalesets branch from d544d79 to 2fc5b48 Compare August 14, 2023 22:39

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 14, 2023

Jont828 force-pushed the async-scalesets branch from 2fc5b48 to 236428f Compare August 14, 2023 22:41

nawazkh requested changes Aug 14, 2023

View reviewed changes

azure/scope/machinepool.go Outdated Show resolved Hide resolved

Make scaleset service async

373518e

Jont828 force-pushed the async-scalesets branch from 236428f to 373518e Compare August 15, 2023 21:02

nawazkh approved these changes Aug 16, 2023

View reviewed changes

k8s-ci-robot assigned nawazkh Aug 16, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 16, 2023

CecileRobertMichon reviewed Aug 16, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 16, 2023

k8s-ci-robot merged commit 6308a38 into kubernetes-sigs:main Aug 16, 2023

Jont828 deleted the async-scalesets branch August 16, 2023 21:49

Jont828 mentioned this pull request Sep 19, 2023

Removing a MachinePool resources lead capz-controller-manager to crash loop #4012

Closed

Comments

Conversation

Jont828 commented Jan 27, 2023

Uh oh!

k8s-ci-robot commented Jan 27, 2023

Uh oh!

Uh oh!

codecov bot commented Jun 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Jont828 commented Aug 10, 2023

Uh oh!

mboersma commented Aug 10, 2023

Uh oh!

willie-yao commented Aug 11, 2023

Uh oh!

k8s-ci-robot commented Aug 11, 2023

Uh oh!

nawazkh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nawazkh Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

Jont828 Aug 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nawazkh commented Aug 14, 2023

Uh oh!

nawazkh left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Aug 16, 2023

Uh oh!

nawazkh commented Aug 16, 2023

Uh oh!

CecileRobertMichon left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Aug 16, 2023

Uh oh!

Jont828 commented Aug 16, 2023

Uh oh!

nawazkh commented Aug 16, 2023

Uh oh!

devigned commented Aug 16, 2023

Uh oh!

willie-yao commented Aug 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

codecov bot commented Jun 12, 2023 •

edited

Loading

Jont828 Aug 15, 2023 •

edited

Loading