Move pod-namespace calls out of process #18355

danwinship · 2018-01-30T19:46:47Z

As discussed in #15991, we need to move all operations in the pod's network namespace out of process, due to a golang issue that allows setns() calls in a locked thread to leak into other threads, causing random lossage as operations intended for the main network namespace end up running in other namespaces instead. (This is fixed in golang 1.10 but we need a fix before then.)

Fixes #15991
Fixes #14385
Fixes #13108
Fixes #18317
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1539987

knobunc · 2018-01-31T14:18:48Z

I hate that we have to do this, but the code looks as reasonable as it can be.

It was only being used in one remaining case (to get the pod IP when tearing down a pod), but we can just always use the fallback code to get that information from OVS instead.

This requires two OVS commands rather than one but it saves us having to parse all that dump-flows output.

danwinship · 2018-01-31T16:40:12Z

/test extended_networking

danwinship · 2018-02-01T13:51:21Z

/test extended_networking
/test unit

danwinship · 2018-02-01T14:02:43Z

@dcbw I think this is ready for review.

smarterclayton · 2018-02-01T17:30:01Z

/test gcp

smarterclayton · 2018-02-01T19:46:16Z

/test gcp

The DNS flake is currently undiagnosed, but doesn't appear to be network related.

However, I notice when that test fails I see the following things in the logs on that node for the specific pod (dns-test):

Feb 01 18:20:44 ci-prtest-5a37c28-15429-ig-n-qzmw origin-node[10940]: I0201 18:20:44.622016   10940 kubelet.go:1852] SyncLoop (DELETE, "api"): "dns-test-8a31e45e-077c-11e8-9376-0e8c3420a542_e2e-tests-dns-g9vhj(8a3a5941-077c-11e8-b172-42010a8e0005)"                                                                   
Feb 01 18:20:44 ci-prtest-5a37c28-15429-ig-n-qzmw origin-node[10940]: I0201 18:20:44.622162   10940 kubelet.go:1846] SyncLoop (REMOVE, "api"): "dns-test-8a31e45e-077c-11e8-9376-0e8c3420a542_e2e-tests-dns-g9vhj(8a3a5941-077c-11e8-b172-42010a8e0005)"                                                                   
Feb 01 18:20:44 ci-prtest-5a37c28-15429-ig-n-qzmw origin-node[10940]: W0201 18:20:44.688273   10940 docker_sandbox.go:340] failed to read pod IP from plugin/docker: NetworkPlugin cni failed on the status hook for pod "dns-test-8a31e45e-077c-11e8-9376-0e8c3420a542_e2e-tests-dns-g9vhj": Unexpected command output nsenter: cannot open /proc/49301/ns/net: No such file or directory
Feb 01 18:20:44 ci-prtest-5a37c28-15429-ig-n-qzmw origin-node[10940]: with error: exit status 1

and then a little later for a different pod:

Feb 01 18:20:46 ci-prtest-5a37c28-15429-ig-n-qzmw origin-node[10940]: W0201 18:20:46.019187   10940 cni.go:242] CNI failed to retrieve network namespace path: cannot find network namespace for the terminated container "b4042e74db575730b2de71c100638609f216e87c625dd6267e11d9b3a5fbc207"                               
Feb 01 18:20:46 ci-prtest-5a37c28-15429-ig-n-qzmw origin-node[10940]: I0201 18:20:46.053343   10940 ovs.go:147] Error executing ovs-ofctl: 2018-02-01T18:20:46Z|00001|ofp_util|WARN|Negative value -1 is not a valid port number.                                                                                          
Feb 01 18:20:46 ci-prtest-5a37c28-15429-ig-n-qzmw origin-node[10940]: ovs-ofctl: -1: port value out of range for in_port                                                                                                                                                                                                   
Feb 01 18:20:46 ci-prtest-5a37c28-15429-ig-n-qzmw ovs-vsctl[56743]: ovs|00001|vsctl|INFO|Called as ovs-vsctl --may-exist add-port br0 veth94082da1 -- set Interface veth94082da1 external-ids=sandbox=35d61949b758ff0d3479e24da1bc26406b577cb30430b23bfd75a09d4c414a1c                                                     
Feb 01 18:20:46 ci-prtest-5a37c28-15429-ig-n-qzmw ovs-vsctl[56850]: ovs|00001|vsctl|INFO|Called as ovs-vsctl --if-exists clear port veth94082da1 qos

smarterclayton · 2018-02-01T22:49:28Z

/retest

smarterclayton · 2018-02-02T00:54:18Z

/test gcp

smarterclayton · 2018-02-02T00:57:02Z

I launched 4 jobs for GCP in parallel, that should give good coverage on the original flake issue.

smarterclayton · 2018-02-02T03:06:53Z

One of them failed on a web console startup failure - that could be network related (web console is the only component that in theory needs pod network started during the install). Rest of the runs went fine.

knobunc

/lgtm

openshift-ci-robot · 2018-02-02T14:56:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, knobunc

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,knobunc]
~~pkg/util/ovs/OWNERS~~ [danwinship,knobunc]

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

danwinship · 2018-02-02T15:36:02Z

Feb 01 18:20:44 ci-prtest-5a37c28-15429-ig-n-qzmw origin-node[10940]: W0201 18:20:44.688273 10940 docker_sandbox.go:340] failed to read pod IP from plugin/docker: NetworkPlugin cni failed on the status hook for pod "dns-test-8a31e45e-077c-11e8-9376-0e8c3420a542_e2e-tests-dns-g9vhj": Unexpected command output nsenter: cannot open /proc/49301/ns/net: No such file or directory

That's not a new thing and I don't think it indicates anything actually going wrong with the pod: it's just kubelet trying to get a status update on a pod that it has already started tearing down. (We ought to fix it to be better about that...)

openshift-merge-robot · 2018-02-02T16:22:48Z

Automatic merge from submit-queue (batch tested with PRs 18376, 18355).

smarterclayton · 2018-02-02T16:46:02Z

Can you open a bug for that and assign it to Seth? I think it happens on every pod termination now.

danwinship · 2018-02-02T18:45:20Z

filed #18414 for the pod termination vs pod status error

dcbw · 2018-02-02T19:11:16Z

pkg/network/sdn-cni-plugin/openshift-sdn.go

-func (p *cniPlugin) CmdAdd(args *skel.CmdArgs) (types.Result, error) {
-	body, err := p.doCNI("http://dummy/", newCNIRequest(args))
+func (p *cniPlugin) doCNIServerAdd(req *cniserver.CNIRequest, hostVeth string) (types.Result, error) {
+	req.Env["OSDN_HOSTVETH"] = hostVeth


Any reason to do this in the environment rather than just stuff it into the JSON by extending CNIRequest?

dcbw · 2018-02-02T19:13:09Z

pkg/network/sdn-cni-plugin/openshift-sdn.go

+
+func (p *cniPlugin) CmdAdd(args *skel.CmdArgs) error {
+	req := newCNIRequest(args)
+	ifname := req.Env["CNI_IFNAME"]


args.IfName and args.Netns. But you can ignore the "" check if you want as these are already checked for you in the 'skel' package.

dcbw · 2018-02-02T19:14:57Z

pkg/network/sdn-cni-plugin/openshift-sdn.go

+		},
+	}
+	index := 0
+	result030.IPs[0].Interface = &index


This could be " = current.Int(0)" now.

dcbw · 2018-02-02T19:23:57Z

pkg/network/node/ovscontroller.go

@@ -323,64 +329,58 @@ func (oc *ovsController) SetPodBandwidth(hostVeth, sandboxID string, ingressBPS,
 	return nil
 }

-func (oc *ovsController) getPodDetailsBySandboxID(sandboxID string) (int, string, string, error) {
+func (oc *ovsController) getPodDetailsBySandboxID(sandboxID string) (int, net.IP, error) {


So what if we just stuff the PodIP into external-ids? Then we don't need to dump and parse flows all the time.

(edit: removed bit about getting ofport via ovs-vsctl which was wrong...)

Automatic merge from submit-queue (batch tested with PRs 18737, 18418). Minor cleanups to sdn-cni-plugin Based on belated comments on #18355: > So what if we just stuff the PodIP into external-ids? Then we don't need to dump and parse flows all the time. I had thought about that, but then we'd have to change ovs.Find() to return multiple columns (`ofport` and `external-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. > Any reason to do [hostVeth] in the environment rather than just stuff it into the JSON by extending CNIRequest? 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.

danwinship added kind/bug Categorizes issue or PR as related to a bug. component/networking sig/networking labels Jan 30, 2018

danwinship assigned dcbw Jan 30, 2018

danwinship requested a review from dcbw January 30, 2018 19:46

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 30, 2018

openshift-ci-robot requested review from knobunc and pravisankar January 30, 2018 19:46

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 30, 2018

danwinship added 3 commits January 31, 2018 11:38

Drop getVethInfo()

03997df

It was only being used in one remaining case (to get the pod IP when tearing down a pod), but we can just always use the fallback code to get that information from OVS instead.

Drop sandboxID as OVS flow note, use external-ids on OVS port instead

ddb2b45

This requires two OVS commands rather than one but it saves us having to parse all that dump-flows output.

Don't read back veth MAC; we already know it's based on the IP

2f29592

danwinship force-pushed the sdn-out-of-process branch from 7269ac1 to 5d62e09 Compare January 31, 2018 16:39

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 31, 2018

Pass directory rather than socket filename to CNIServer.Start()

3c843d7

danwinship force-pushed the sdn-out-of-process branch from 5d62e09 to b556452 Compare January 31, 2018 18:11

Move remaining pod-namespace operations out of the main process

dd7a754

danwinship force-pushed the sdn-out-of-process branch from b556452 to dd7a754 Compare January 31, 2018 22:43

danwinship changed the title ~~[WIP] Move pod-namespace calls out of process~~ Move pod-namespace calls out of process Feb 1, 2018

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 1, 2018

openshift-ci-robot assigned knobunc Feb 2, 2018

knobunc approved these changes Feb 2, 2018

View reviewed changes

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

openshift-merge-robot merged commit 1927cae into openshift:master Feb 2, 2018

dcbw reviewed Feb 2, 2018

View reviewed changes

danwinship mentioned this pull request Feb 2, 2018

Minor cleanups to sdn-cni-plugin #18418

Merged

danwinship deleted the sdn-out-of-process branch March 7, 2018 14:29

danwinship mentioned this pull request Mar 30, 2018

Use a dummy ns.NetNS in sdn_cni_plugin_test.go #19166

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move pod-namespace calls out of process #18355

Move pod-namespace calls out of process #18355

danwinship commented Jan 30, 2018 •

edited

Loading

knobunc commented Jan 31, 2018

danwinship commented Jan 31, 2018

danwinship commented Feb 1, 2018

danwinship commented Feb 1, 2018

smarterclayton commented Feb 1, 2018

smarterclayton commented Feb 1, 2018

smarterclayton commented Feb 1, 2018

smarterclayton commented Feb 2, 2018

smarterclayton commented Feb 2, 2018

smarterclayton commented Feb 2, 2018

knobunc left a comment

openshift-ci-robot commented Feb 2, 2018

danwinship commented Feb 2, 2018

openshift-merge-robot commented Feb 2, 2018

smarterclayton commented Feb 2, 2018

danwinship commented Feb 2, 2018

dcbw Feb 2, 2018

dcbw Feb 2, 2018

dcbw Feb 2, 2018

dcbw Feb 2, 2018 •

edited

Loading

Move pod-namespace calls out of process #18355

Move pod-namespace calls out of process #18355

Conversation

danwinship commented Jan 30, 2018 • edited Loading

knobunc commented Jan 31, 2018

danwinship commented Jan 31, 2018

danwinship commented Feb 1, 2018

danwinship commented Feb 1, 2018

smarterclayton commented Feb 1, 2018

smarterclayton commented Feb 1, 2018

smarterclayton commented Feb 1, 2018

smarterclayton commented Feb 2, 2018

smarterclayton commented Feb 2, 2018

smarterclayton commented Feb 2, 2018

knobunc left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Feb 2, 2018

danwinship commented Feb 2, 2018

openshift-merge-robot commented Feb 2, 2018

smarterclayton commented Feb 2, 2018

danwinship commented Feb 2, 2018

dcbw Feb 2, 2018

Choose a reason for hiding this comment

dcbw Feb 2, 2018

Choose a reason for hiding this comment

dcbw Feb 2, 2018

Choose a reason for hiding this comment

dcbw Feb 2, 2018 • edited Loading

Choose a reason for hiding this comment

danwinship commented Jan 30, 2018 •

edited

Loading

dcbw Feb 2, 2018 •

edited

Loading