Make scalesetvms delete async by Jont828 · Pull Request #3799 · kubernetes-sigs/cluster-api-provider-azure

Jont828 · 2023-08-04T01:12:30Z

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

Special notes for your reviewer:

cherry-pick candidate

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

Make scalesetvms delete async

codecov · 2023-08-04T20:35:12Z

Codecov Report

Patch coverage: 47.54% and project coverage change: +1.50% 🎉

Comparison is base (8893daa) 54.77% compared to head (ff61b9a) 56.27%.
Report is 167 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3799      +/-   ##
==========================================
+ Coverage   54.77%   56.27%   +1.50%     
==========================================
  Files         187      191       +4     
  Lines       19098    19383     +285     
==========================================
+ Hits        10460    10908     +448     
+ Misses       8070     7846     -224     
- Partials      568      629      +61

Files Changed	Coverage Δ
azure/services/scalesetvms/client.go	`0.00% <0.00%> (-16.95%)`	⬇️
azure/services/scalesetvms/spec.go	`0.00% <0.00%> (ø)`
azure/services/scalesetvms/scalesetvms.go	`65.55% <69.84%> (+2.14%)`	⬆️
azure/scope/machinepoolmachine.go	`33.94% <77.77%> (+3.80%)`	⬆️

... and 95 files with indirect coverage changes

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

Jont828 · 2023-08-08T21:31:12Z

@CecileRobertMichon @mboersma Should be ready for review, PTAL!

mboersma · 2023-08-10T18:01:04Z

/assign

willie-yao · 2023-08-16T23:16:39Z

Hey @Jont828, is this PR still blocked? If not, I'll be happy to review it!

Jont828 · 2023-08-17T22:14:05Z

@willie-yao I think I got confused, I meant that this PR is blocked because I need reviews. Just changed the label to "needs review!"

willie-yao

This looks great! Just some comments mostly for my own understanding

Jont828 · 2023-09-05T19:12:11Z

@willie-yao @mboersma @CecileRobertMichon Can we LGTM or is there anything else I should change?

CecileRobertMichon · 2023-09-05T19:57:24Z

@Jont828 I think we were mostly waiting on #3875 to be fixed (it now is)

Aside from that, I see a few unresolved threads still, so we still need to resolve those. I'll try to give this another round of review later today.

CecileRobertMichon · 2023-09-05T23:42:55Z

/test ls

k8s-ci-robot · 2023-09-05T23:42:57Z

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

/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-apiversion-upgrade
/test pull-cluster-api-provider-azure-capi-e2e
/test pull-cluster-api-provider-azure-conformance
/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-windows-containerd-upstream-with-ci-artifacts
/test pull-cluster-api-provider-azure-windows-containerd-upstream-with-ci-artifacts-serial-slow

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-ci-entrypoint
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

Details

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/test-infra repository.

CecileRobertMichon · 2023-09-05T23:43:12Z

/test pull-cluster-api-provider-azure-apiversion-upgrade
/test pull-cluster-api-provider-azure-capi-e2e
/test pull-cluster-api-provider-azure-conformance
/test pull-cluster-api-provider-azure-e2e-optional
/test pull-cluster-api-provider-azure-e2e-workload-upgrade

CecileRobertMichon · 2023-09-05T23:48:53Z

-func (ac *azureClient) GetResultIfDone(ctx context.Context, future *infrav1.Future) (compute.VirtualMachineScaleSetVM, error) {
-	ctx, _, spanDone := tele.StartSpanWithLogger(ctx, "scalesetvms.azureClient.GetResultIfDone")
-	defer spanDone()
+// CreateOrUpdateAsync is a dummy implementation to fulfill the async.Reconciler interface.


you shouldn't need to do this, you should be able to do async.New(scope, nil, client), when creating the service in scalesetvms.go so this client doesn't need to implement the Deleter interface

check out the disks client for a similar example

If I understand correctly, in the disks service we don't attempt to call CreateOrUpdateResource() so we can pass in a nil to the Creator interface. In this service, we want to get the resource if it exists and so I'm trying to leverage the CreateOrUpdateResource() in the async interface to fetch the resource and handle the not found/transient errors. Alternatively, I could try to construct the client as well, but I felt that it would be clunky to declare a Reconciler and a client for each type of VM.

Now that I'm thinking about this more, I wonder if it might make more sense to add a Getter (https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/main/azure/services/async/interfaces.go#L41) in the Service and simply call getter.Get() in the Reconcile func instead of using CreateOrUpdateResource in a way it's not meant to be used.

My thinking was to be able to take advantage of the error handling in this block of CreateOrUpdateResource() so we don't have to duplicate that logic. Maybe we could add a Get() function to the Reconciler iface to give us the error handling?

@mboersma what do you think? I'm concerned this won't work the same way with the async poller framework when we try to migrade scaleset vms to sdk v2

Actually, I think this will act the same way in asyncpoller; the CreateOrUpdateResource code isn't really changed. I think we can merge this as-is and it won't present problems for refactoring SDKv2 on top of it.

CecileRobertMichon · 2023-09-06T21:44:09Z

/test pull-cluster-api-provider-azure-apiversion-upgrade
/test pull-cluster-api-provider-azure-capi-e2e
/test pull-cluster-api-provider-azure-conformance
/test pull-cluster-api-provider-azure-e2e-optional
/test pull-cluster-api-provider-azure-e2e-workload-upgrade

Jont828 · 2023-09-07T20:21:34Z

/retest

CecileRobertMichon · 2023-09-08T00:17:26Z

/test pull-cluster-api-provider-azure-apiversion-upgrade
/test pull-cluster-api-provider-azure-capi-e2e
/test pull-cluster-api-provider-azure-conformance
/test pull-cluster-api-provider-azure-e2e-optional
/test pull-cluster-api-provider-azure-e2e-workload-upgrade

CecileRobertMichon

/lgtm
/assign @mboersma

k8s-ci-robot · 2023-09-08T17:20:08Z

LGTM label has been added.

Details

Git tree hash: 84618d271f474d4cb77fae91906576ac300dc9bf

mboersma

/lgtm
/approve

I also wanted to see that VMSS Flex still works and it looks like -e2e-optional passed. 👍🏻

k8s-ci-robot · 2023-09-08T19:15:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mboersma

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~~ [mboersma]

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 added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 4, 2023

k8s-ci-robot requested review from nojnhuh and willie-yao August 4, 2023 01:12

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

Jont828 force-pushed the async-scalesetvms branch from b1adac3 to cea845e Compare August 4, 2023 01:16

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

Jont828 force-pushed the async-scalesetvms branch 3 times, most recently from 5073e01 to 725db25 Compare August 4, 2023 20:24

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

Jont828 force-pushed the async-scalesetvms branch from 725db25 to 8e1ef08 Compare August 8, 2023 18:26

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

k8s-ci-robot assigned mboersma Aug 10, 2023

mboersma reviewed Aug 10, 2023

View reviewed changes

Comment thread azure/services/scalesetvms/scalesetvms.go Outdated

Comment thread azure/scope/machinepoolmachine.go

Comment thread azure/services/scalesetvms/spec.go

Jont828 commented Aug 10, 2023

View reviewed changes

Comment thread azure/services/scalesetvms/spec.go

Jont828 force-pushed the async-scalesetvms branch from 8e1ef08 to eafe6b7 Compare August 11, 2023 20:47

Jont828 assigned Jont828 and unassigned mboersma Aug 14, 2023

willie-yao reviewed Aug 23, 2023

View reviewed changes

Comment thread azure/services/scalesetvms/client.go

Comment thread azure/services/scalesetvms/scalesetvms.go

mboersma added this to the v1.11 milestone Aug 24, 2023

CecileRobertMichon reviewed Sep 5, 2023

View reviewed changes

Jont828 force-pushed the async-scalesetvms branch from eafe6b7 to c4e5448 Compare September 6, 2023 20:38

Make scalesetvms delete async

ff61b9a

Jont828 force-pushed the async-scalesetvms branch from c4e5448 to ff61b9a Compare September 7, 2023 22:03

CecileRobertMichon reviewed Sep 8, 2023

View reviewed changes

k8s-ci-robot assigned mboersma and CecileRobertMichon Sep 8, 2023

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

mboersma approved these changes Sep 8, 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 Sep 8, 2023

k8s-ci-robot merged commit ee7a843 into kubernetes-sigs:main Sep 8, 2023

Conversation

Jont828 commented Aug 4, 2023

Uh oh!

codecov Bot commented Aug 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Jont828 commented Aug 8, 2023

Uh oh!

mboersma commented Aug 10, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

willie-yao commented Aug 16, 2023

Uh oh!

Jont828 commented Aug 17, 2023

Uh oh!

willie-yao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

CecileRobertMichon commented Sep 5, 2023

Uh oh!

CecileRobertMichon commented Sep 5, 2023

Uh oh!

k8s-ci-robot commented Sep 5, 2023

Uh oh!

CecileRobertMichon commented Sep 5, 2023

Uh oh!

CecileRobertMichon Sep 5, 2023

Choose a reason for hiding this comment

Uh oh!

Jont828 Sep 6, 2023

Choose a reason for hiding this comment

Uh oh!

CecileRobertMichon Sep 6, 2023

Choose a reason for hiding this comment

Uh oh!

Jont828 Sep 7, 2023

Choose a reason for hiding this comment

Uh oh!

CecileRobertMichon Sep 7, 2023

Choose a reason for hiding this comment

Uh oh!

mboersma Sep 7, 2023

Choose a reason for hiding this comment

Uh oh!

CecileRobertMichon commented Sep 6, 2023

Uh oh!

Jont828 commented Sep 7, 2023

Uh oh!

CecileRobertMichon commented Sep 8, 2023

Uh oh!

CecileRobertMichon left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Sep 8, 2023

Uh oh!

mboersma left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Sep 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov Bot commented Aug 4, 2023 •

edited

Loading

Jont828 commented Sep 5, 2023 •

edited

Loading