Optimize route reconciliation logic#2090
Conversation
|
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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
|
/ok-to-test |
|
still fixing gate, we can continue the code review before that.. |
e8feac3 to
9fef59e
Compare
9fef59e to
f93f0da
Compare
|
/retest-required |
f93f0da to
a413699
Compare
|
/test openstack-cloud-csi-manila-e2e-test |
|
/override openstack-cloud-csi-manila-e2e-test manila is broken currently |
|
@zetaab: Overrode contexts on behalf of zetaab: openstack-cloud-csi-manila-e2e-test DetailsIn response to this:
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. |
|
/test openstack-cloud-csi-manila-e2e-test |
|
Manila e2e tests are broken. What is the process for the PR review? |
|
/override openstack-cloud-csi-manila-e2e-test that's something we need fix later, for now, let's ignore this |
|
@jichenjc: Overrode contexts on behalf of jichenjc: openstack-cloud-csi-manila-e2e-test DetailsIn response to this:
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 could you please review this PR? Or do I need to wait for assigned reviewers approval? |
|
no, I will read this along with other reviewers, will do soon, thanks for the reminder |
|
@jichenjc: Overrode contexts on behalf of jichenjc: openstack-cloud-csi-manila-e2e-test DetailsIn response to this:
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. |
|
/lgtm |
zetaab
left a comment
There was a problem hiding this comment.
some comments, but like mentioned - I do not use this feature so I have not 100% understanding what is even needed
| return nil, err | ||
| } | ||
|
|
||
| var networkIDs []string |
There was a problem hiding this comment.
this does not contain unique values for networkid? is it possible to have duplicate values?
There was a problem hiding this comment.
duplicate values are less probable, since router cannot handle the same network more than once.
|
|
||
| // detect router's private network ID for further VM ports filtering | ||
| r.Lock() | ||
| r.networkIDs, err = getRouterNetworkIDs(r.network, r.os.routeOpts.RouterID) |
There was a problem hiding this comment.
is this really changing so often that we need to fetch this all the time? In general networks are not changing quite often? (spoiler: I am not using this feature at all)
There was a problem hiding this comment.
There is a low probability that this may happen. Nevertheless, I decided to be on the safe side, since it's just one API call.
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
hm. IMHO this is used in every openstack based k8s cluster, and the |
|
its not? example from kops https://github.com/kubernetes/kops/blob/master/upup/pkg/fi/cloudup/openstack/cloud.go#L835-L892 like I said we are not using any "router" things from occm. Instead we do have CNIs |
|
I was confused by and comment.And apologies, I've never used kops. |
|
Interesting, why that is required? Why anyone should use it? What is the purpose of it? I have used cloud provider openstack for 7 years without that. but everything works like should? actually after that in next row https://github.com/kubernetes/cloud-provider-openstack/blob/master/pkg/openstack/openstack.go#L467 it just disables the |
in my test environment the amount of API calls has been decreased by an order of magnitude, if not by two. Could you please skip |
|
/override openstack-cloud-csi-manila-e2e-test yeah but I mean, the interesting question is that why that is marked as required if its not really needed. |
|
@zetaab: Overrode contexts on behalf of zetaab: openstack-cloud-csi-manila-e2e-test DetailsIn response to this:
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'll cover this in my next PR: #2134. Thank you for paying attention on this. |
|
just cherry pick ,but we ususally backport a bug instead of a feature .. |
| if needIPv6 && ip.To4() == nil { | ||
| return v.Address | ||
| } | ||
| return v.Address |
There was a problem hiding this comment.
This if-branch is ineffective because the return value is the same regardless of whether the condition is true.
There was a problem hiding this comment.
Thanks, good catch!
Fixed in atomic routes PRs.
What this PR does / why we need it:
the
openstack.Routes'ListRoutesmethod must not loop over all project servers just to resolve a couple of route's next hops to node names. This PR resolves the node name only when it's found in the route's next hop using cached k8s API calls for nodes listing.Which issue this PR fixes(if applicable):
fixes #2089
Special notes for reviewers:
Release note: