Make vnets reconcile/delete async by CecileRobertMichon · Pull Request #1921 · kubernetes-sigs/cluster-api-provider-azure

CecileRobertMichon · 2021-12-14T01:15:54Z

What type of PR is this?
/kind feature
/kind cleanup

What this PR does / why we need it: Follow up on #1610 and implementation of #1541 for virtual networks.

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: This PR replaces #1684

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 vnets reconcile/delete async

CecileRobertMichon · 2021-12-14T22:04:39Z

/assign @Jont828 @mboersma

k8s-ci-robot · 2021-12-14T22:04:41Z

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

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 @Jont828 @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.

CecileRobertMichon · 2022-01-19T01:21:42Z

rebased, this is ready for review again

/assign @shysank @mboersma

CecileRobertMichon · 2022-01-21T19:31:06Z

/retest

shysank

looks good overall with one minor comment. Will do another review after rebase.

CecileRobertMichon · 2022-01-28T19:50:45Z

@shysank @Jont828 rebased and addressed comments, PTAL

CecileRobertMichon · 2022-01-29T00:06:17Z

/retest

aks flake

shysank

looks good overall with a couple of nits and a clarification on vnet peerings

shysank · 2022-01-29T00:50:22Z

-		CIDRs:         s.Vnet().CIDRBlocks,
+func (s *ClusterScope) VNetSpec() azure.ResourceSpecGetter {
+	return &virtualnetworks.VNetSpec{
+		ResourceGroup:  s.Vnet().ResourceGroup,


I see that we have not copied Peerings from the old spec. Is this a conscious decision? we were setting it previously here

yes, what was happening before was that we were doing a full deep copy into the vnet stored in AzureCluster.Spec which meant we had to include every single field that had been set before otherwise we would risk overwriting it. This is not good because if a new feature gets added like vnet peerings there could be a bug where the vnet write erases the user configuration (and in fact we did run into that issue during vnet peering implementation). This is hopefully simplifying the code while keeping it functionally equivalent. We only set the vnet, ID, tags and cidrs from the existing vnet while keeping the rest of the VnetSpec the same.

Oh okay, makes sense. Thanks for the explanation!

Co-Authored-By: Shyam Sankaran <sankarans@vmware.com>

devigned

lgtm outside of the failure in e2e when ensuring the cluster is deleted.

CecileRobertMichon · 2022-01-31T17:31:59Z

Operation could not be completed as it results in exceeding approved standardDSv3Family Cores quota

/retest

shysank · 2022-01-31T18:45:33Z

/lgtm

CecileRobertMichon · 2022-01-31T19:25:33Z

AKS test is broken on main (see https://kubernetes.slack.com/archives/CEX9HENG7/p1643648764688369)

all other tests passed

/cc @devigned

CecileRobertMichon · 2022-01-31T22:54:59Z

/retest

exp test fixed in #2025

CecileRobertMichon · 2022-02-01T00:30:18Z

/retest

AKS timesync flake

k8s-ci-robot · 2022-02-01T00:31:49Z

@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	`021814e`	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.

shysank · 2022-02-01T17:11:04Z

/lgtm
/approve

k8s-ci-robot · 2022-02-01T17:11:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shysank

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

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. 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 Dec 14, 2021

k8s-ci-robot requested review from devigned and shysank December 14, 2021 01:16

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. labels Dec 14, 2021

CecileRobertMichon mentioned this pull request Dec 14, 2021

Make VNet and NSGs reconcile/delete async #1684

Closed

3 tasks

CecileRobertMichon force-pushed the async-vnet-2 branch from 0afe14e to 31e48d1 Compare December 14, 2021 21:58

k8s-ci-robot assigned mboersma Dec 14, 2021

CecileRobertMichon force-pushed the async-vnet-2 branch from 5bb5f80 to 20a99d9 Compare December 14, 2021 22:19

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

CecileRobertMichon force-pushed the async-vnet-2 branch from 20a99d9 to 9514c8b Compare December 16, 2021 18:08

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

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

CecileRobertMichon force-pushed the async-vnet-2 branch from 9514c8b to f6df036 Compare January 19, 2022 00:28

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

CecileRobertMichon force-pushed the async-vnet-2 branch 2 times, most recently from 1e2b278 to b1b5a8f Compare January 19, 2022 00:54

k8s-ci-robot assigned shysank Jan 19, 2022

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

shysank reviewed Jan 28, 2022

View reviewed changes

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

CecileRobertMichon force-pushed the async-vnet-2 branch from b1b5a8f to 50883c7 Compare January 28, 2022 18:33

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

CecileRobertMichon force-pushed the async-vnet-2 branch 2 times, most recently from e0129f3 to a3bed43 Compare January 28, 2022 19:49

CecileRobertMichon force-pushed the async-vnet-2 branch 5 times, most recently from 5188237 to df39987 Compare January 28, 2022 21:52

shysank reviewed Jan 29, 2022

View reviewed changes

Cecile Robert-Michon and others added 2 commits January 28, 2022 17:28

Make vnets reconcile/delete async

a03b9e2

Co-Authored-By: Shyam Sankaran <sankarans@vmware.com>

Add Getter interface for getting resources in other services

021814e

CecileRobertMichon force-pushed the async-vnet-2 branch from 427f3ce to 021814e Compare January 29, 2022 01:28

devigned reviewed Jan 29, 2022

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 31, 2022

k8s-ci-robot requested a review from devigned January 31, 2022 19:25

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 1, 2022

k8s-ci-robot merged commit a38d26a into kubernetes-sigs:main Feb 1, 2022

k8s-ci-robot added this to the v1.1 milestone Feb 1, 2022

CecileRobertMichon deleted the async-vnet-2 branch February 17, 2023 23:25

Conversation

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

Uh oh!

Uh oh!

CecileRobertMichon commented Dec 14, 2021

Uh oh!

k8s-ci-robot commented Dec 14, 2021

Uh oh!

CecileRobertMichon commented Jan 19, 2022

Uh oh!

CecileRobertMichon commented Jan 21, 2022

Uh oh!

shysank left a comment • 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!

CecileRobertMichon commented Jan 28, 2022

Uh oh!

CecileRobertMichon commented Jan 29, 2022

Uh oh!

shysank left a comment

Choose a reason for hiding this comment

Uh oh!

shysank Jan 29, 2022

Choose a reason for hiding this comment

Uh oh!

CecileRobertMichon Jan 29, 2022

Choose a reason for hiding this comment

Uh oh!

shysank Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

devigned left a comment

Choose a reason for hiding this comment

Uh oh!

CecileRobertMichon commented Jan 31, 2022

Uh oh!

shysank commented Jan 31, 2022

Uh oh!

CecileRobertMichon commented Jan 31, 2022

Uh oh!

CecileRobertMichon commented Jan 31, 2022

Uh oh!

CecileRobertMichon commented Feb 1, 2022

Uh oh!

k8s-ci-robot commented Feb 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shysank commented Feb 1, 2022

Uh oh!

k8s-ci-robot commented Feb 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

CecileRobertMichon commented Dec 14, 2021 •

edited

Loading

shysank left a comment •

edited

Loading

k8s-ci-robot commented Feb 1, 2022 •

edited

Loading