Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Make route tables reconcile/delete async #1686

Merged
merged 2 commits into from
Jan 11, 2022
Merged

Make route tables reconcile/delete async #1686

merged 2 commits into from
Jan 11, 2022

Conversation

CecileRobertMichon
Copy link
Contributor

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

What this PR does / why we need it: Implements async reconciliation and delete for the route tables service. Follow up on #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 #

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 route tables 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/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. 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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 14, 2021
@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. labels Sep 14, 2021
@CecileRobertMichon
Copy link
Contributor Author

/hold

to rebase on top of #1684

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 14, 2021
@shysank shysank mentioned this pull request Sep 20, 2021
Closed
23 tasks
@CecileRobertMichon CecileRobertMichon mentioned this pull request Nov 5, 2021
Closed
3 tasks
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 5, 2021
@CecileRobertMichon CecileRobertMichon force-pushed the async-routetables branch 4 times, most recently from 62085f0 to 57c0b46 Compare November 8, 2021 18:18
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 10, 2021
@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 13, 2021
@CecileRobertMichon CecileRobertMichon force-pushed the async-routetables branch 3 times, most recently from 103d872 to b20c04c Compare December 14, 2021 22:10
@CecileRobertMichon
Copy link
Contributor Author

/hold cancel
/assign @Jont828 @mboersma @shysank

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 14, 2021
This was referenced Jan 8, 2022
Merged
Merged
Merged
@CecileRobertMichon CecileRobertMichon force-pushed the async-routetables branch 3 times, most recently from 9e6748c to e8e8f94 Compare January 10, 2022 23:34
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jan 11, 2022 •
edited
Loading

@CecileRobertMichon: The following tests 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-e2e-exp 9121e36 link false /test pull-cluster-api-provider-azure-e2e-exp
pull-cluster-api-provider-azure-apidiff 229f425 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.

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.

devigned
devigned approved these changes Jan 11, 2022
View reviewed changes
Copy link
Contributor

@devigned devigned left a comment

Choose a reason for hiding this comment

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

Beyond the comments from @mboersma, I have nothing more to add. lgtm!

azure/interfaces.go
@@ -110,5 +110,5 @@ type ResourceSpecGetter interface {
// Parameters takes the existing resource and returns the desired parameters of the resource.
// If the resource does not exist, or we do not care about existing parameters to update the resource, existing should be nil.
// If no update is needed on the resource, Parameters should return nil.
Parameters(existing interface{}) (interface{}, error)
Parameters(existing interface{}) (params interface{}, err error)
Copy link
Contributor

Choose a reason for hiding this comment

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

func Parameters[T any](existing T) (params T, err error)

Will be kind of nice. No action to take from this, just commenting.

@CecileRobertMichon
Copy link
Contributor Author

/assign @shysank @Jont828

@shysank
Copy link
Contributor

shysank commented Jan 11, 2022

/lgtm

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

shysank commented Jan 11, 2022

failed to solve with frontend dockerfile.v0: failed to solve with frontend gateway.v0: frontend grpc server closed unexpectedly

that's a new one...

just curious, was this a flake?

@CecileRobertMichon
Copy link
Contributor Author

yes it seemed like a prow infra flake, the test didn't even start running

@CecileRobertMichon
Copy link
Contributor Author

CecileRobertMichon commented Jan 11, 2022 •
edited
Loading

@shysank @devigned could one of you give it an approve as well please?

@shysank
Copy link
Contributor

shysank commented Jan 11, 2022

/approve

@k8s-ci-robot
Copy link
Contributor

[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

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 Jan 11, 2022
@k8s-ci-robot k8s-ci-robot merged commit 42cbf86 into kubernetes-sigs:main Jan 11, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.1 milestone Jan 11, 2022
@CecileRobertMichon CecileRobertMichon deleted the async-routetables branch February 17, 2023 23:25
@jackfrancis jackfrancis mentioned this pull request Jan 31, 2024
Merged
4 tasks
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@Jont828 Jont828 Jont828 left review comments

@karuppiah7890 karuppiah7890 karuppiah7890 left review comments

@shysank shysank shysank left review comments

@devigned devigned devigned approved these changes

@alexeldeib alexeldeib Awaiting requested review from alexeldeib

Assignees

@mboersma mboersma

@shysank shysank

@Jont828 Jont828

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/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. 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. 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.
7 participants
@CecileRobertMichon @k8s-ci-robot @shysank @devigned @karuppiah7890 @Jont828 @mboersma