make virtual machine reconcile async by CecileRobertMichon · Pull Request #1697 · kubernetes-sigs/cluster-api-provider-azure

CecileRobertMichon · 2021-09-17T23:44:22Z

What type of PR is this?
/kind feature

What this PR does / why we need it: implements #1541 for virtual machines.

Modify CreateResource to return the resulting resource so we can store info like providerID and the IP address of the node (before we were doing this with getExisting on the next reconcile loop)
Change the VM create and delete to be async using async helpers, adapt the client to implement the async interfaces
Moves a bunch of code from virtualmachines.go into spec.go and splits it into smaller functions
Added unit tests and restructured existing tests to test smaller chunks

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 #

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 virtual machine reconcile async

CecileRobertMichon · 2021-09-17T23:44:34Z

Tests, lint are still todo.

CecileRobertMichon · 2021-09-27T23:27:15Z

Removing [WIP] as this is ready for review but going to add a hold so it doesn't merge until after the release since it's a massive change

/hold

CecileRobertMichon · 2021-09-27T23:36:47Z

I know this seems like a lot of changes but... here are the main things going on:

Modify CreateResource to return the resulting resource so we can store info like providerID and the IP address of the node (before we were doing this with getExisting on the next reconcile loop)
Change the VM create and delete to be async using async helpers, adapt the client to implement the async interfaces
Moves a bunch of code from virtualmachines.go into spec.go and splits it into smaller functions
Added unit tests and restructured existing tests to test smaller chunks

And FWIW, this PR overall removes code while maintaining test coverage:

before: https://prow.k8s.io/view/gs/kubernetes-jenkins/logs/periodic-cluster-api-provider-azure-coverage/1442449517906497536
after: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-azure/1697/pull-cluster-api-provider-azure-coverage/1442626055931170816

CecileRobertMichon · 2021-09-27T23:39:07Z

-
-		osProfile, err := s.generateOSProfile(ctx, vmSpec)
+		// Discover addresses for NICs associated with the VM
+		addresses, err := s.getAddresses(ctx, vm, vmSpec.ResourceGroupName())


future work I'm considering: figure out how to do this without needing to get the NICs and IPs again (we created them earlier), but this would be another refactor so holding off for now since this PR already does a lot. Kept that function as-is.

CecileRobertMichon · 2021-09-28T17:24:40Z

/retest

CecileRobertMichon · 2021-09-28T17:24:50Z

/assign @shysank @devigned

CecileRobertMichon · 2021-10-18T22:58:27Z

/cc @devigned @shysank @karuppiah7890 @mboersma

k8s-ci-robot · 2021-10-18T22:58:29Z

@CecileRobertMichon: GitHub didn't allow me to request PR reviews from the following users: karuppiah7890.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @devigned @shysank @karuppiah7890 @mboersma

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.

devigned

/lgtm

CecileRobertMichon · 2021-11-02T21:38:59Z

rebased + squashed

this is ready to go @devigned @shysank

CecileRobertMichon · 2021-11-03T18:49:03Z

-		return err
+		return nil, nil, errors.Wrapf(err, "failed to get desired parameters for virtual machine %s", spec.ResourceName())
+	} else if vm == nil {
+		// nothing to do here.


self-review: maybe this should return the result as the existing vm instead of returning nil so we can act upon the existing VM even if it already exists

Could be added in the future if needed.

devigned

/lgtm

shysank

/lgtm overall with one small clarification

nader-ziada · 2021-11-03T21:56:14Z

went through it again now and lgtm

nader-ziada · 2021-11-03T22:13:47Z

/approve

k8s-ci-robot · 2021-11-03T22:14:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nader-ziada

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~~ [nader-ziada]

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

shysank · 2021-11-03T23:20:31Z

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

k8s-ci-robot · 2021-11-04T00:20:10Z

@CecileRobertMichon: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-provider-azure-apidiff	`930c3ca`	link	false	`/test pull-cluster-api-provider-azure-apidiff`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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. I understand the commands that are listed here.

nader-ziada · 2021-11-04T13:23:38Z

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

k8s-ci-robot requested review from nader-ziada and shysank September 17, 2021 23:44

k8s-ci-robot added the sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. label Sep 17, 2021

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 17, 2021

shysank mentioned this pull request Sep 20, 2021

Make azure services async #1702

Closed

23 tasks

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

CecileRobertMichon force-pushed the async-vms branch 3 times, most recently from ba7876c to 49a86d5 Compare September 27, 2021 22:58

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

CecileRobertMichon force-pushed the async-vms branch 2 times, most recently from d8ed261 to 3ce3007 Compare September 27, 2021 23:03

CecileRobertMichon changed the title ~~[WIP] make virtual machine reconcile async~~ make virtual machine reconcile async Sep 27, 2021

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 Sep 27, 2021

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 27, 2021

CecileRobertMichon commented Sep 27, 2021

View reviewed changes

k8s-ci-robot assigned devigned and shysank Sep 28, 2021

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

CecileRobertMichon force-pushed the async-vms branch from 3ce3007 to c0fc969 Compare October 11, 2021 21:29

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

k8s-ci-robot requested a review from devigned October 18, 2021 22:58

k8s-ci-robot requested a review from mboersma October 18, 2021 22:58

devigned approved these changes Oct 21, 2021

View reviewed changes

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 21, 2021

Cecile Robert-Michon added 2 commits November 2, 2021 14:36

make virtual machine reconcile async

1a2aeea

don't print "failure" for transient errors

930c3ca

CecileRobertMichon force-pushed the async-vms branch from cf6740a to 930c3ca Compare November 2, 2021 21:37

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 2, 2021

CecileRobertMichon mentioned this pull request Nov 2, 2021

Make publicips reconcile/delete async #1745

Closed

3 tasks

CecileRobertMichon commented Nov 3, 2021

View reviewed changes

devigned approved these changes Nov 3, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 3, 2021

shysank reviewed Nov 3, 2021

View reviewed changes

Comment thread azure/scope/machine.go

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 3, 2021

CecileRobertMichon mentioned this pull request Nov 3, 2021

Cache refresh may lead to terminal error #1828

Closed

k8s-ci-robot merged commit ebf1a1c into kubernetes-sigs:main Nov 4, 2021

k8s-ci-robot added this to the v1.1 milestone Nov 4, 2021

CecileRobertMichon mentioned this pull request Nov 5, 2021

Check future regardless of error in async helpers and return VM result for existing VM #1837

Merged

3 tasks

CecileRobertMichon deleted the async-vms branch February 17, 2023 23:24

Conversation

CecileRobertMichon commented Sep 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CecileRobertMichon commented Sep 17, 2021

Uh oh!

CecileRobertMichon commented Sep 27, 2021

Uh oh!

CecileRobertMichon commented Sep 27, 2021

Uh oh!

CecileRobertMichon Sep 27, 2021

Choose a reason for hiding this comment

Uh oh!

CecileRobertMichon commented Sep 28, 2021

Uh oh!

CecileRobertMichon commented Sep 28, 2021

Uh oh!

CecileRobertMichon commented Oct 18, 2021

Uh oh!

k8s-ci-robot commented Oct 18, 2021

Uh oh!

devigned left a comment

Choose a reason for hiding this comment

Uh oh!

CecileRobertMichon commented Nov 2, 2021

Uh oh!

CecileRobertMichon Nov 3, 2021

Choose a reason for hiding this comment

Uh oh!

devigned Nov 3, 2021

Choose a reason for hiding this comment

Uh oh!

devigned left a comment

Choose a reason for hiding this comment

Uh oh!

shysank left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nader-ziada commented Nov 3, 2021

Uh oh!

nader-ziada commented Nov 3, 2021

Uh oh!

k8s-ci-robot commented Nov 3, 2021

Uh oh!

shysank commented Nov 3, 2021

Uh oh!

k8s-ci-robot commented Nov 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nader-ziada commented Nov 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

CecileRobertMichon commented Sep 17, 2021 •

edited

Loading

k8s-ci-robot commented Nov 4, 2021 •

edited

Loading