Trivial fix to do fewer allocations in OVS healthcheck #17313
Conversation
openshift-ci-robot
added
the
size/XS
openshift-merge-robot
added
the
approved
pkg/network/node/ovscontroller.go
Outdated
| parsed, err := ovs.ParseFlow(ovs.ParseForDump, flow) | ||
| if err == nil && parsed.Table == ruleVersionTable && parsed.NoteHasPrefix(expectedVersionNote) { | ||
| return true | ||
| for i := len(flows)-1; i >= 0; i-- { |
There was a problem hiding this comment.
Adding a comment here will be helpful: Processing the flows in the reverse order will find the flow rule quicker as version table is on the highest numbered table
pkg/network/node/ovscontroller.go
Outdated
| if err == nil && parsed.Table == ruleVersionTable && parsed.NoteHasPrefix(expectedVersionNote) { | ||
| return true | ||
| for i := len(flows)-1; i >= 0; i-- { | ||
| parsed, err := ovs.ParseFlow(ovs.ParseForDump, flows[i]) |
There was a problem hiding this comment.
Is there a go bindings for ovs that we can use instead of calling ovs CLI commands?
There was a problem hiding this comment.
No. There are bindings for the ovs-vsctl side of things, but not ovs-ofctl.
But this patch was mostly just a quick fix for 3.7.z. I have ideas on further improvements already.
|
Sgtm, algorithmic >> mechanical any day |
danwinship
force-pushed
the
speed-up-ovs-healthcheck
branch
from
5250fc0 to
add3c37
Compare
danwinship
force-pushed
the
speed-up-ovs-healthcheck
branch
from
add3c37 to
67a57a3
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danwinship, pravisankar The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
/retest |
|
Automatic merge from submit-queue. |
|
@danwinship: The following test failed, say
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. |
|
/cherrypick release-3.7 |
|
@smarterclayton: New pull request created: #17333 In 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. |
Automatic merge from submit-queue. OVS parsing improvements The more-complicated version of #17313. This takes advantage of the fact that "ovs-ofctl dump-flows" lets you pass a filter, so rather than getting back the whole list of flows and parsing each one, we can just ask for a much smaller set of flows to begin with. In particular: - in AlreadySetUp() we can `DumpFlows("table=%d", ruleVersionTable)` and get back only the single flow we're looking for - in getPodDetailsBySandboxID(), we can `DumpFlows("table=20,arp")` and only get back the flows that *might* be the one we're looking for. (You can't match on the `actions` field, so there's no way to request only the flow that has the specific `note` that we're looking for. We *could* store the first 4 bytes of the sandbox ID in the cookie and then add that to the filter, I guess, but this code doesn't get run much anyway...)
Make
ovsController.AlreadySetUp()scan the flows in reverse order; the flow we're looking for is in the highest-numbered table, andovs-ofctl dump-flowsreturns the flows sorted by table number, so if we parse them in forward order, we'll end up unnecessarily parsing every single flow, while parsing them in reverse means we only parse one.(Also stop parsing further flows once we found the one we were looking for, even if it has the wrong value.)
Fixes #17312
(We can also make improvements to ParseFlow itself later, but this just seemed like the simplest fix for the immediate problem.)