-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Minor cleanups to sdn-cni-plugin #18418
Minor cleanups to sdn-cni-plugin #18418
Conversation
@danwinship hmm, just doing:
doesn't seem to kill unit tests in pkg/network for me... |
6937f4f
to
3fead8c
Compare
@dcbw: Hm... not sure what the problem was before, but yeah, it seems to work fine. Updated |
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danwinship, dcbw 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 |
Automatic merge from submit-queue (batch tested with PRs 18737, 18418). |
Based on belated comments on #18355:
I had thought about that, but then we'd have to change ovs.Find() to return multiple columns (
ofport
andexternal-ids
) and we'd have to parse the external-ids to separate out the sandboxID from the podIP. Though, admittedly, there's already code to parse external-ids in fake_ovs now anyway...Also, we're only parsing a single flow now, because we request "in_port=%d" in the dump-flows.
I actually did do it that way first, and then changed it. Something about adding a field to CNIRequest had weird unexpected side effects. IIRC, it would have required a whole bunch of changes to the unit tests or something like that.
But I can change it if you prefer.