occm: implement a support for atomic routes update by kayrus · Pull Request #2134 · kubernetes/cloud-provider-openstack

kayrus · 2023-02-27T23:04:54Z

What this PR does / why we need it:

An extra improvement for the route reconciliation logic. This PR allows to use a single API call to add or remove routes instead of fetching existing routes and combining them with new or removing old routes with a further update. This change allows concurrent calls without consistency issues.

Which issue this PR fixes(if applicable):
fixes #2089
based on #2090

Special notes for reviewers:

Release note:

Implement a support for concurrent atomic routes updates.

k8s-ci-robot · 2023-02-27T23:05:04Z

Hi @kayrus. Thanks for your PR.

I'm waiting for a kubernetes 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.

jichenjc · 2023-02-28T01:51:01Z

/ok-to-test

k8s-ci-robot · 2023-02-28T02:14:14Z

@kayrus: 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
openstack-cloud-csi-manila-e2e-test	`362b0cc`	link	true	`/test openstack-cloud-csi-manila-e2e-test`

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.

zetaab · 2023-03-02T21:34:37Z

pkg/openstack/openstack.go

 // RouterOpts is used for Neutron routes
 type RouterOpts struct {
-	RouterID string `gcfg:"router-id"` // required
+	RouterID         string `gcfg:"router-id"` // required


in my opinion this required should be removed because its not really required. Like mentioned in another PR - we have been using OCCM for 7 years without using this at all.

zetaab · 2023-03-02T21:38:55Z

please rebase this after that another PR is merged

kayrus · 2023-03-02T21:46:03Z

This is still WIP. I'm in contact with our neutron team. I'd like to remove the manual config toggle, based on extensions API. So there should be no need for an extra option.
I'll also remove the required mark from the struct member.

zetaab · 2023-03-02T22:01:58Z

@kayrus if you have time could you explain why this routes feature is needed? Our architecture has always been that we have router in (project) network with dhcp and router will handle the traffic automatically, no need to add routes. Could we get some benefits of using it?

kayrus · 2023-03-03T07:58:37Z

@zetaab if you have time could you explain why this routes feature is needed?

k8s has a global PODs CIDR, and each node serves its own segment, e.g. 10.16.0.0/16 will be served by multiple nodes with 10.16.1.0/24, 10.16.2.0/24, 10.16.3.0/24, etc... Routes interface tells to a cloud provider router which POD CIDR segment should be served by which node and adds static routes to a router.

Therefore VMs or Loadbalancers from the same private network can access a pod's network using static routes:

10.16.1.0/24 is routed to node1
10.16.2.0/24 is routed to node2
etc...

This approach is used in GCP, AWS, Azure and other providers as well. Basically it adds an ability for loadbalancers to access PODs network directly avoiding nodePorts.

Initially there was no atomic routes update, and routes were updated with the following logic: get all routes from a router, append a new route to the list of existing routes, update the router. And this logic worked in concurrent manner, thus there were a lot of inconsistent updates, which overwrote routes, submitted by parallel threads.

With an atomic update it should be possible to update routes in concurrent manner and reduce an amount of API calls. Which should significantly reduce the already improved route update logic.

Currently I'm trying to figure out whether it's possible to get extraroute-atomic extension info from the neutron API to avoid config option toggle, therefore the PR is still WIP.

kayrus · 2023-03-03T19:16:15Z

@zetaab I updated my comment above. See the bold text.

jichenjc · 2023-03-06T01:42:38Z

docs/openstack-cloud-controller-manager/using-openstack-cloud-controller-manager.md

+
+* `router-id`
+  Required. Specifies the Neutron router ID to manage Kubernetes cluster routes.
+* `extraroute-atomic`


maybe in the future we can check through neutron ext-list to detect whether it's enabled or not ?

that's what I'm investigating right now.

done. my tests show overall routes reconciliation latency improvements from 2-5 minutes down to 10 seconds for a cluster of 16 nodes.

ready for review.

kayrus · 2023-03-07T19:06:43Z

@zetaab @jichenjc ready for review

kayrus · 2023-03-09T07:39:13Z

@zetaab @jichenjc kindly ping
see also #2150 and #2152.

#2134) (#2150) * occm: implement an optional atomic routes update * Fix IPv6 detection logic in routes

#2134) (#2152) * occm: implement an optional atomic routes update * Fix IPv6 detection logic in routes

zetaab

/approve
/lgtm

k8s-ci-robot · 2023-03-09T12:56:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zetaab

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

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 do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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. labels Feb 27, 2023

k8s-ci-robot requested review from FengyunPan2 and ramineni February 27, 2023 23:05

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 27, 2023

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 27, 2023

kayrus mentioned this pull request Feb 27, 2023

Optimize route reconciliation logic #2090

Merged

kayrus force-pushed the atomic-routes branch from 362b0cc to 3c0444e Compare February 28, 2023 06:40

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

kayrus mentioned this pull request Mar 1, 2023

REQUEST: New membership for kayrus kubernetes/org#4056

Closed

9 tasks

zetaab reviewed Mar 2, 2023

View reviewed changes

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

kayrus force-pushed the atomic-routes branch from 3c0444e to fc6c831 Compare March 3, 2023 06:50

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

jichenjc reviewed Mar 6, 2023

View reviewed changes

kayrus force-pushed the atomic-routes branch from fc6c831 to 43ea456 Compare March 7, 2023 14:04

kayrus changed the title ~~occm: implement an optional atomic routes update~~ occm: implement a support for atomic routes update Mar 7, 2023

kayrus marked this pull request as ready for review March 7, 2023 14:06

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 Mar 7, 2023

k8s-ci-robot requested review from Fedosin and anguslees March 7, 2023 14:06

occm: implement an optional atomic routes update

1f037cf

kayrus force-pushed the atomic-routes branch from 43ea456 to 1f037cf Compare March 8, 2023 15:13

This was referenced Mar 8, 2023

occm: implement a support for atomic routes update cherry-pick to 1.25 (#2134) #2150

Merged

occm: implement a support for atomic routes update cherry-pick to 1.26 (#2134) #2152

Merged

Fix IPv6 detection logic in routes

93bbf28

kayrus force-pushed the atomic-routes branch from 874c5e3 to 93bbf28 Compare March 9, 2023 12:02

k8s-ci-robot pushed a commit that referenced this pull request Mar 9, 2023

occm: implement a support for atomic routes update cherry-pick to 1.25 (

4828146

#2134) (#2150) * occm: implement an optional atomic routes update * Fix IPv6 detection logic in routes

k8s-ci-robot pushed a commit that referenced this pull request Mar 9, 2023

occm: implement a support for atomic routes update cherry-pick to 1.26 (

c80cf38

#2134) (#2152) * occm: implement an optional atomic routes update * Fix IPv6 detection logic in routes

zetaab approved these changes Mar 9, 2023

View reviewed changes

k8s-ci-robot assigned zetaab Mar 9, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 9, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 9, 2023

k8s-ci-robot merged commit 9484739 into kubernetes:master Mar 9, 2023

kayrus deleted the atomic-routes branch March 9, 2023 12:59

jknipper mentioned this pull request Mar 16, 2023

Bump occm to v1.25.5 sapcc/kubernikus#777

Merged

kayrus mentioned this pull request Jul 7, 2025

Detection of missing router routes is broken terraform-provider-openstack/terraform-provider-openstack#1917

Closed

Conversation

kayrus commented Feb 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Feb 27, 2023

Uh oh!

jichenjc commented Feb 28, 2023

Uh oh!

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

Uh oh!

Uh oh!

zetaab Mar 2, 2023

Choose a reason for hiding this comment

Uh oh!

zetaab commented Mar 2, 2023

Uh oh!

kayrus commented Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zetaab commented Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kayrus commented Mar 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kayrus commented Mar 3, 2023

Uh oh!

jichenjc Mar 6, 2023

Choose a reason for hiding this comment

Uh oh!

kayrus Mar 6, 2023

Choose a reason for hiding this comment

Uh oh!

kayrus Mar 7, 2023

Choose a reason for hiding this comment

Uh oh!

kayrus commented Mar 7, 2023

Uh oh!

kayrus commented Mar 9, 2023

Uh oh!

zetaab left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Mar 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kayrus commented Feb 27, 2023 •

edited

Loading

k8s-ci-robot commented Feb 28, 2023 •

edited

Loading

kayrus commented Mar 2, 2023 •

edited

Loading

zetaab commented Mar 2, 2023 •

edited

Loading

kayrus commented Mar 3, 2023 •

edited

Loading