Skip to content

Conversation

danwinship
Copy link
Contributor

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...)

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 19, 2017
Rename "ParseForDelete" to "ParseForFilter" (since it's about parsing
flow descriptions that are being used as filters, not specifically
about deleting), and fix up the logic around default values in
ParseForAdd vs ParseForFilter. (Among other things, this fixes the
code so that you can match on table=0, priority=0, or cookie=0.)
@smarterclayton
Copy link
Contributor

Nice

@danwinship
Copy link
Contributor Author

/retest

getPodDetailsBySandboxID() was written to have the list of flows
passed into it, for ease of testing. But that requires the caller to
know what flows it is looking for, and anyway, ovsController is
already mockable anyway so we can test the function just as easily as
an ovsController method.
@danwinship
Copy link
Contributor Author

/retest

1 similar comment
@danwinship
Copy link
Contributor Author

/retest

@danwinship
Copy link
Contributor Author

@dcbw PTAL
(and @smarterclayton can you /approve this for pkg/util?)

@smarterclayton
Copy link
Contributor

/approve

but not reviewed

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 19, 2018
@pravisankar
Copy link

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 22, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, pravisankar, smarterclayton

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 /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue.

@openshift-merge-robot openshift-merge-robot merged commit db27648 into openshift:master Jan 23, 2018
@danwinship danwinship deleted the ovs-parsing branch February 6, 2018 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/networking lgtm Indicates that a PR is ready to be merged. sig/networking size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants