Skip to content

Make subnets reconcile/delete async#1914

Merged
k8s-ci-robot merged 1 commit into
kubernetes-sigs:mainfrom
Jont828:async-subnets
Feb 9, 2022
Merged

Make subnets reconcile/delete async#1914
k8s-ci-robot merged 1 commit into
kubernetes-sigs:mainfrom
Jont828:async-subnets

Conversation

@Jont828
Copy link
Copy Markdown
Contributor

@Jont828 Jont828 commented Dec 10, 2021 •
edited
Loading

What type of PR is this?

/kind feature
What this PR does / why we need it: Implementation of an async service for subnets as a follow up for #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 #1708

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

@k8s-ci-robot 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. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 10, 2021
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Dec 10, 2021

Hi @Jont828. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot 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/XL Denotes a PR that changes 500-999 lines, ignoring generated files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 10, 2021
@CecileRobertMichon
Copy link
Copy Markdown
Contributor

CecileRobertMichon commented Dec 13, 2021

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 13, 2021
@Jont828 Jont828 force-pushed the async-subnets branch 3 times, most recently from 7c0135e to 23de3d7 Compare December 15, 2021 03:41
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 15, 2021
@Jont828 Jont828 changed the title Make subnets reconcile/delete async [WIP] Make subnets reconcile/delete async Dec 17, 2021
@Jont828 Jont828 mentioned this pull request Dec 22, 2021
Closed
7 tasks
@CecileRobertMichon
Copy link
Copy Markdown
Contributor

CecileRobertMichon commented Jan 8, 2022

comment from @shysank about using named returns from another PR that is relevant here:
#1686 (comment)

Here is the commit that does this in the route tables PR: b2302ea

@Jont828 Jont828 force-pushed the async-subnets branch 2 times, most recently from 5375bbd to 645c4d2 Compare January 10, 2022 19:08
mboersma
mboersma reviewed Jan 10, 2022
View reviewed changes
Comment thread azure/scope/cluster.go
mboersma
mboersma reviewed Jan 10, 2022
View reviewed changes
Comment thread azure/scope/managedcontrolplane.go
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 11, 2022
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 11, 2022
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 1, 2022
@Jont828
Copy link
Copy Markdown
Contributor Author

Jont828 commented Feb 2, 2022

/retest

1 similar comment
@Jont828
Copy link
Copy Markdown
Contributor Author

Jont828 commented Feb 2, 2022

/retest

@Jont828
Copy link
Copy Markdown
Contributor Author

Jont828 commented Feb 2, 2022

/retest

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 2, 2022
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 2, 2022
@Jont828
Copy link
Copy Markdown
Contributor Author

Jont828 commented Feb 3, 2022

/retest

CecileRobertMichon
CecileRobertMichon reviewed Feb 4, 2022
View reviewed changes
Comment thread azure/services/subnets/spec.go Outdated
@Jont828 Jont828 force-pushed the async-subnets branch 2 times, most recently from 0ccd1e6 to b1c164e Compare February 4, 2022 23:29
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 4, 2022
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 7, 2022
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Feb 7, 2022 •
edited
Loading

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

@Jont828
Copy link
Copy Markdown
Contributor Author

Jont828 commented Feb 7, 2022

@CecileRobertMichon @shysank I believe the e2e tests are passing now, does this look good to you?

CecileRobertMichon
CecileRobertMichon approved these changes Feb 8, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/assign @shysank

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 8, 2022
@shysank
Copy link
Copy Markdown
Contributor

shysank commented Feb 9, 2022

/lgtm
/approve
Thanks for being patient with the reviews :)

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Feb 9, 2022

[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:

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 9, 2022
@k8s-ci-robot k8s-ci-robot merged commit 461953a into kubernetes-sigs:main Feb 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mboersma mboersma mboersma left review comments

+3 more reviewers

@shysank shysank shysank requested changes

@devigned devigned devigned approved these changes

@CecileRobertMichon CecileRobertMichon CecileRobertMichon approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@CecileRobertMichon CecileRobertMichon

@shysank shysank

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/provider/azure Issues or PRs related to azure provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Milestone

v1.1

Development

Successfully merging this pull request may close these issues.

async subnets

6 participants

@Jont828 @k8s-ci-robot @CecileRobertMichon @devigned @shysank @mboersma