Check future regardless of error in async helpers and return VM result for existing VM by CecileRobertMichon · Pull Request #1837 · kubernetes-sigs/cluster-api-provider-azure

CecileRobertMichon · 2021-11-05T20:36:19Z

What type of PR is this?

What this PR does / why we need it: Small followups on #1697 and comments in #1684

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:

NONE

CecileRobertMichon · 2021-11-05T20:39:22Z

-			}
-			scope.SetLongRunningOperationState(future)
-			return nil, azure.WithTransientError(azure.NewOperationNotDoneError(future), retryAfter(sdkFuture))
+	if sdkFuture != nil {


@karuppiah7890 please take a look. I believe this addresses your comment in #1684 (comment). I think it makes sense to not require an error (that way this will work whether an error is returned or not). However, I still think we shouldn't silence the error returned by WaitForCompletionRef in the clients since eventually it might return a different error besides context deadline exceeded.

CecileRobertMichon · 2021-11-05T20:40:06Z

+	if !ok {
+		if params == nil {
+			// nothing to do here.
+			return existingVM, nil, nil


this addresses #1697 (comment)

can we move this to line 86 and invoke spec.Parameters() can be only for new vms? wdyt?

in this specific VM service we technically could because we never want to update existing VMs but this pattern works best for all services in general: we pass in the existing resource to spec.Parameters(existing) and that function determines if the resource needs to be created or updated based on the existing resource. For example, for NSGs we update the group the security rules are missing. IMO it's best to use the same pattern throughout all services so it's easier to refactor them later to have even more code in common.

CecileRobertMichon · 2021-11-05T21:59:40Z

/hold

need to investigate why e2e tests are failing

…d delete

CecileRobertMichon · 2021-11-08T17:31:00Z

/hold cancel

CecileRobertMichon · 2021-11-08T17:31:06Z

/retest

CecileRobertMichon · 2021-11-08T17:58:47Z

/assign @shysank @Jont828 @karuppiah7890

k8s-ci-robot · 2021-11-08T17:58:49Z

@CecileRobertMichon: GitHub didn't allow me to assign the following users: Jont828, karuppiah7890.

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

Details

In response to this:

/assign @shysank @Jont828 @karuppiah7890

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.

shysank · 2021-11-09T20:07:52Z

+	var existingVM interface{}
+
+	if existing, err := ac.Get(ctx, spec.ResourceGroupName(), spec.ResourceName()); err != nil && !azure.ResourceNotFound(err) {
+		return nil, nil, errors.Wrapf(err, "failed to get virtual machine %s in %s", spec.ResourceName(), spec.ResourceGroupName())


shouldn't we handle errors that are not ResourceNotFound?

if it's a ResourceNotFound error, that means the VM doesn't exist, so we don't set existingVM. It's not an error we want to return, it's an expected error whenever we create the resource for the first time.

oh yeah, this used to be in a different form in virtualmachines.go. Perhaps I should be more in sync with the new code, but alas the code itself is not in sync anymore.

shysank · 2021-11-09T20:15:38Z

+	if !ok {
+		if params == nil {
+			// nothing to do here.
+			return existingVM, nil, nil


can we move this to line 86 and invoke spec.Parameters() can be only for new vms? wdyt?

shysank · 2021-11-10T01:35:27Z

/lgtm

devigned

/lgtm
/approve

k8s-ci-robot · 2021-11-10T15:35:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: devigned

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

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-none Denotes a PR that doesn't merit a release note. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 5, 2021

k8s-ci-robot requested review from juan-lee and nader-ziada November 5, 2021 20:36

k8s-ci-robot added area/provider/azure Issues or PRs related to azure provider sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 5, 2021

CecileRobertMichon commented Nov 5, 2021

View reviewed changes

CecileRobertMichon force-pushed the vm-fixes branch from d7a2ab4 to cfd085a Compare November 5, 2021 20:41

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

Cecile Robert-Michon added 2 commits November 5, 2021 18:18

Return existing VM as result if VM does not need to be updated

70fe322

Check if a future is returned regardless of error for async create an…

8a4b357

…d delete

CecileRobertMichon force-pushed the vm-fixes branch from cfd085a to 8a4b357 Compare November 6, 2021 01:18

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

k8s-ci-robot assigned shysank Nov 8, 2021

CecileRobertMichon mentioned this pull request Nov 8, 2021

Make VNet and NSGs reconcile/delete async #1684

Closed

3 tasks

shysank reviewed Nov 9, 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 10, 2021

devigned approved these changes Nov 10, 2021

View reviewed changes

k8s-ci-robot assigned devigned Nov 10, 2021

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

k8s-ci-robot merged commit 2d4c465 into kubernetes-sigs:main Nov 10, 2021

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

CecileRobertMichon deleted the vm-fixes branch February 17, 2023 23:24

Conversation

CecileRobertMichon commented Nov 5, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CecileRobertMichon commented Nov 5, 2021

Uh oh!

CecileRobertMichon commented Nov 8, 2021

Uh oh!

CecileRobertMichon commented Nov 8, 2021

Uh oh!

CecileRobertMichon commented Nov 8, 2021

Uh oh!

k8s-ci-robot commented Nov 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shysank commented Nov 10, 2021

Uh oh!

devigned left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Nov 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants