sdn: try cleaning up OVS rules even if sandbox is gone #18166

dcbw · 2018-01-18T20:20:17Z

If a sandbox is deleted underneath kubernetes its netns will
be gone and its veth interface will be deleted by the kernel.
That means we can't inspect the veth for its IP address and
other details, which are used to remove OVS flows for the
interface.

But we've already got code to find out the IP using the
sandbox ID which kubelet passes down to us. Let's use
that code to at least delete the stale OVS flows.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1518684

@danwinship @openshift/networking @knobunc

danwinship · 2018-01-18T20:32:06Z

Hm... we could find the veth name in this case too; given the OVS flow, we can extract the port number and use ovs-vsctl to get the corresponding port name (ovs-vsctl --no-heading --data=bare --columns=name find Interface ofport=${PORT})...

dcbw · 2018-01-18T21:44:01Z

/test unit

dcbw · 2018-01-19T06:36:32Z

Hm... we could find the veth name in this case too; given the OVS flow, we can extract the port number and use ovs-vsctl to get the corresponding port name (ovs-vsctl --no-heading --data=bare --columns=name find Interface ofport=${PORT})...

@danwinship ok, how about now? Let's just stuff the sandbox ID into external-ids and use that to clean things up when we can. The one case where we need the hostVeth name is deleting the port, and we can get that if we have the sandbox ID. (open question, can we just delete the port & interface records from the OVSDB directly, or does 'del-port' do something special?)

As a further optimization, we could stuff the IP address into external-ids too and kill the flow note stuff perhaps? Much nicer parsing OVSDB output than dump-flows.

knobunc

LGTM

knobunc · 2018-01-19T14:24:40Z

/retest

danwinship

Generally LATM ("looks awesome to me").

(open question, can we just delete the port & interface records from the OVSDB directly, or does 'del-port' do something special?)

That doesn't appear to be documented. (Note that you can't just ovs-vsctl destroy most objects anyway; you have to remove references to them (from br0's ports property in this case) and let OVS GC them.)

As a further optimization, we could stuff the IP address into external-ids too and kill the flow note stuff perhaps? Much nicer parsing OVSDB output than dump-flows.

Yes

danwinship · 2018-01-19T14:57:33Z

pkg/network/node/ovscontroller.go

-func (oc *ovsController) ensureOvsPort(hostVeth string) (int, error) {
-	return oc.ovs.AddPort(hostVeth, -1)
+func (oc *ovsController) ensureOvsPort(hostVeth, sandboxID string) (int, error) {
+	return oc.ovs.AddPort(hostVeth, -1, "external-ids=sandbox="+sandboxID, "--", "set", "Port", hostVeth, "external-ids=sandbox="+sandboxID)


Ugh.

Fortunately, I don't think there's any need to bother with Port properties. Commands that act on Ports will allow a Port name rather than a UUID (like we used to do in ClearPodBandwidth()), and the Port name is the same as the Interface name, so if you changed ovsif.Find() to let you specify the column you wanted returned, then you could have getPortsForSandbox() return a list of Interface names rather than a list of Port UUIDs, and that would work everywhere. In fact, it would simplify TearDownPod().

danwinship · 2018-01-19T15:05:56Z

pkg/network/node/ovscontroller.go

+		return err
+	}
+
+	portUUIDs, err := oc.getPortsForSandbox(sandboxID)


If we now absolutely depend on every port having a sandbox ID in its external-ids then we need to make AlreadySetUp() fail for bridges with non-external-id'ed ports attached. So change ruleVersion at the top of the file. (In which case you'll also want danwinship@dbbf89ac.)

UpdatePod() will also need to be updated to call ensureOvsPort().

danwinship · 2018-01-19T15:09:14Z

pkg/util/ovs/ovs.go

+	uuids := make([]string, 0)
+	for _, line := range strings.Split(output, "\n") {
+		parts := strings.Split(line, ":")
+		if len(parts) >= 2 && strings.TrimSpace(parts[0]) == "_uuid" {


You can force ovs-vsctl to do all this for you. --no-heading --columns=_uuid find ...

And then use strings.Fields to get the trimmed array.

danwinship · 2018-01-19T15:13:57Z

pkg/network/node/ovscontroller.go

-func (oc *ovsController) SetPodBandwidth(hostVeth string, ingressBPS, egressBPS int64) error {
-	// note pod ingress == OVS egress and vice versa
+func (oc *ovsController) getPortsForSandbox(sandboxID string) ([]string, error) {
+	return oc.ovs.Find("port", "external-ids=sandbox="+sandboxID)


That does a full match on external-ids (ie, it will fail if external-ids also contains other keys). Changing the = between external-ids and sandbox to a : will change it to a subkey match (checking only that external-ids contains a matching sandbox key)

danwinship · 2018-01-19T15:49:00Z

pkg/network/node/ovscontroller.go

+func (oc *ovsController) SetPodBandwidth(hostVeth, sandboxID string, ingressBPS, egressBPS int64) error {
+	// note pod ingress == OVS egress and vice versa
+
+	if err := oc.ClearPodBandwidth(nil, sandboxID); err != nil {


You could remove the hostVeth arg to SetPodBandwidth() and just call getPortsForSandbox() and then pass that list to ClearPodBandwidth() (which then no longer needs its own call-getPortsForSandbox() special case), and change the remaining hostVeth references in SetPodBandwidth() to use the portsList instead. (I'm not totally sure if this would be a net improvement.)

@danwinship I'd rather still pass it in so we know what the most-recently-created veth will be for actually setting the bandwidth. Otherwise we'd have to assume it's the first item in the ports array, or do it for all ports. We already have the info in the veth name here.

But I agree we don't need ClearPodBandwidth() to do its own getPortsForSandboxID().

dcbw · 2018-01-23T02:43:37Z

@danwinship @rajatchopra PTAL thanks! Addressed all your comments.

dcbw · 2018-01-23T15:58:15Z

/test extended_conformance_install issue #17882

danwinship · 2018-01-23T16:04:49Z

lgtm except I think you still need to update UpdatePod? Right now the changed ruleVersion means it will run UpdatePod on all the pods, but UpdatePod won't actually set the external-id

dcbw · 2018-01-23T17:04:03Z

lgtm except I think you still need to update UpdatePod? Right now the changed ruleVersion means it will run UpdatePod on all the pods, but UpdatePod won't actually set the external-id

@danwinship do we? If we bump the flow version, on restart that out to blows the entire bridge away on upgrade including all the flows, ports, interfaces, qos, etc. That makes Update() fail and the pod's sandbox gets killed and then completely restarted.

danwinship · 2018-01-23T17:11:11Z

duh, right. (not bothering to /lgtm yet since you have to rebase)

dcbw · 2018-01-23T17:30:40Z

@danwinship rebased

danwinship · 2018-01-23T17:42:41Z

/lgtm

dcbw · 2018-01-23T17:53:13Z

/test cmd

dcbw · 2018-01-23T19:04:52Z

/test extended_conformance_install issue #17556

dcbw · 2018-01-23T22:20:47Z

/approve

If a sandbox is deleted underneath kubernetes its netns will be gone and its veth interface will be deleted by the kernel. That means we can't inspect the veth for its IP address and other details, which are used to remove OVS flows for the interface. But we've already got code to find out the IP using the sandbox ID which kubelet passes down to us. Let's use that code to at least delete the stale OVS flows. Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1518684

Store the sandbox ID in the OVSDB as an external-id and use that to find things we need to clean up. Previously these were leaked if the network namespace was removed underneath kubernetes.

dcbw · 2018-01-23T23:49:07Z

/test unit

dcbw · 2018-01-24T02:21:18Z

@danwinship @knobunc can I get another LGTM? nothing has changed in the code, but I mistakenly repushed because I thought that might trigger re-evaluation of the pkg/util/ovs/OWNERS changes

pravisankar · 2018-01-24T07:58:16Z

pkg/network/node/ovscontroller.go

-	// note pod ingress == OVS egress and vice versa
+// Returned list can also be used for port names
+func (oc *ovsController) getInterfacesForSandbox(sandboxID string) ([]string, error) {
+	return oc.ovs.Find("interface", "name", "external-ids:sandbox="+sandboxID)


ovs AddPort() and Create() has 'external-ids=' and Find() has 'external-ids:', initially thought ':' is a typo but looking at the man page, column[:key]=value is the correct syntax.

@pravisankar yeah we want a substring match in Find, while Add/Create set it.

pravisankar · 2018-01-24T07:58:51Z

/lgtm

openshift-ci-robot · 2018-01-24T07:58:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, dcbw, 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:

~~pkg/network/OWNERS~~ [danwinship,dcbw,pravisankar]
~~pkg/util/ovs/OWNERS~~ [danwinship,dcbw,pravisankar]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

dcbw · 2018-01-24T15:56:16Z

/test extended_conformance_install issue #17556

pravisankar · 2018-01-24T19:16:00Z

/retest

openshift-merge-robot · 2018-01-25T01:07:02Z

Automatic merge from submit-queue.

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 18, 2018

openshift-ci-robot requested review from knobunc and smarterclayton January 18, 2018 20:20

dcbw force-pushed the ovs-cleanup-when-sandbox-is-gone branch from b71decb to db7b59d Compare January 18, 2018 20:23

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 18, 2018

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 19, 2018

knobunc approved these changes Jan 19, 2018

View reviewed changes

knobunc requested review from danwinship, rajatchopra and pravisankar and removed request for smarterclayton January 19, 2018 14:24

knobunc self-assigned this Jan 19, 2018

knobunc added the component/networking label Jan 19, 2018

danwinship suggested changes Jan 19, 2018

View reviewed changes

dcbw force-pushed the ovs-cleanup-when-sandbox-is-gone branch from c17cd2b to 22f3e02 Compare January 23, 2018 02:33

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 23, 2018

danwinship approved these changes Jan 23, 2018

View reviewed changes

dcbw force-pushed the ovs-cleanup-when-sandbox-is-gone branch from 22f3e02 to db19d04 Compare January 23, 2018 17:27

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 23, 2018

openshift-ci-robot assigned danwinship Jan 23, 2018

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

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

dcbw and others added 3 commits January 23, 2018 16:21

futureproof ovscontroller_test.go

36c26d5

sdn: clean up ports and QoS by matching on sandbox ID

87fdfb0

Store the sandbox ID in the OVSDB as an external-id and use that to find things we need to clean up. Previously these were leaked if the network namespace was removed underneath kubernetes.

dcbw force-pushed the ovs-cleanup-when-sandbox-is-gone branch from db19d04 to 87fdfb0 Compare January 23, 2018 22:21

openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 23, 2018

pravisankar reviewed Jan 24, 2018

View reviewed changes

pravisankar approved these changes Jan 24, 2018

View reviewed changes

openshift-ci-robot assigned pravisankar Jan 24, 2018

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

openshift-merge-robot merged commit ae43279 into openshift:master Jan 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sdn: try cleaning up OVS rules even if sandbox is gone #18166

sdn: try cleaning up OVS rules even if sandbox is gone #18166

dcbw commented Jan 18, 2018

danwinship commented Jan 18, 2018

dcbw commented Jan 18, 2018

dcbw commented Jan 19, 2018 •

edited

Loading

knobunc left a comment

knobunc commented Jan 19, 2018

danwinship left a comment

danwinship Jan 19, 2018

danwinship Jan 19, 2018

danwinship Jan 19, 2018

rajatchopra Jan 19, 2018

danwinship Jan 19, 2018

rajatchopra Jan 20, 2018

danwinship Jan 19, 2018

dcbw Jan 22, 2018

dcbw commented Jan 23, 2018

dcbw commented Jan 23, 2018

danwinship commented Jan 23, 2018

dcbw commented Jan 23, 2018

danwinship commented Jan 23, 2018

dcbw commented Jan 23, 2018

danwinship commented Jan 23, 2018

dcbw commented Jan 23, 2018

dcbw commented Jan 23, 2018

dcbw commented Jan 23, 2018

dcbw commented Jan 23, 2018

dcbw commented Jan 24, 2018

pravisankar Jan 24, 2018

dcbw Jan 24, 2018

pravisankar commented Jan 24, 2018

openshift-ci-robot commented Jan 24, 2018

dcbw commented Jan 24, 2018

pravisankar commented Jan 24, 2018

openshift-merge-robot commented Jan 25, 2018

sdn: try cleaning up OVS rules even if sandbox is gone #18166

sdn: try cleaning up OVS rules even if sandbox is gone #18166

Conversation

dcbw commented Jan 18, 2018

danwinship commented Jan 18, 2018

dcbw commented Jan 18, 2018

dcbw commented Jan 19, 2018 • edited Loading

knobunc left a comment

Choose a reason for hiding this comment

knobunc commented Jan 19, 2018

danwinship left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcbw commented Jan 23, 2018

dcbw commented Jan 23, 2018

danwinship commented Jan 23, 2018

dcbw commented Jan 23, 2018

danwinship commented Jan 23, 2018

dcbw commented Jan 23, 2018

danwinship commented Jan 23, 2018

dcbw commented Jan 23, 2018

dcbw commented Jan 23, 2018

dcbw commented Jan 23, 2018

dcbw commented Jan 23, 2018

dcbw commented Jan 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pravisankar commented Jan 24, 2018

openshift-ci-robot commented Jan 24, 2018

dcbw commented Jan 24, 2018

pravisankar commented Jan 24, 2018

openshift-merge-robot commented Jan 25, 2018

dcbw commented Jan 19, 2018 •

edited

Loading