Make sdn pod teardown robust #14893

pravisankar · 2017-06-26T23:24:02Z

We have seen bugs where podIP is not valid/empty for teardown operation,
in this case ovs flows are left out. This change will fix this case
by picking the podIP from ovs flows (using 'note' field that was derived from
pod sandbox ID)

pravisankar · 2017-06-26T23:25:04Z

@openshift/networking @dcbw PTAL

pravisankar · 2017-06-26T23:25:08Z

[test]

danwinship · 2017-06-27T13:15:42Z

pkg/sdn/plugin/ovscontroller_test.go

@@ -215,7 +215,7 @@ func TestOVSService(t *testing.T) {
 }

 const (
-	containerID         string = "bcb5d8d287fcf97458c48ad643b101079e3bc265a94e097e7407440716112f69"
+	sandboxID           string = "bcb5d8d287fcf97458c48ad643b101079e3bc265a94e097e7407440716112f69"


belongs in the previous PR

danwinship · 2017-06-27T13:19:12Z

pkg/sdn/plugin/ovscontroller.go

-func getPodDetailsByContainerID(flows []string, containerID string) (int, string, string, string, error) {
-	note, err := getPodNote(containerID)
+func getPodDetailsBySandboxID(ovsif ovs.Interface, sandboxID string) (int, string, string, string, error) {
+	flows, err := ovsif.DumpFlows()


Maybe we should rebase the real pkg/util/ovs implementation on top of the fake one, so that, eg, when you do an AddFlow(), it calls ovs-ofctl and also updates its own internal array of flows, so that we can implement code like this without actually having to call dump-flows all the time?

Currently DumpFlows is used in during startup, update pod and teardown case where podIP is empty. We can't optimize during startup, update pod may not be a frequent operation. May be we can introduce this idea when dumpFlows() is called frequently.

dcbw · 2017-06-27T20:00:45Z

pkg/sdn/plugin/pod_linux.go

+					ktypes.KubernetesPodNameLabel:      req.PodName,
+				},
+			}
+			if id, err := m.getPodSandboxID(filter, req); err == nil {


req.containerId should already be the pod sandbox ID... I'm pretty sure we don't need to re-request it, since at Teardown time the runtime is calling us directly with the sandboxID.

Okay, I will update.

dcbw · 2017-06-27T20:02:12Z

pkg/sdn/plugin/pod_linux.go

 	filter := &kruntimeapi.PodSandboxFilter{
 		LabelSelector: map[string]string{ktypes.KubernetesPodUIDLabel: string(pod.UID)},
 	}
-	podSandboxList, err := runtimeService.ListPodSandbox(filter)
+	podSandboxID, err := m.getPodSandboxID(filter, req)


As I mentioned in the other PR, I think the sandbox ID detection stuff for update() should go into node.go or update.go. Then we just pass the actual sandbox ID into the PodRequest which gets set up in node.go::Update(), and we'll get it already here.

pravisankar · 2017-06-28T00:13:27Z

@dcbw @danwinship Updated, ptal

danwinship · 2017-06-28T12:58:39Z

pkg/sdn/plugin/ovscontroller.go

+			return err
+		}
+		if _, ip, _, _, err := getPodDetailsBySandboxID(flows, sandboxID); err == nil {
+			podIP = ip


need to return an error here if this fails

We may get an error here if the corresponding ovs flow rule doesn't exists which means tearDown already happened. Don't we want to ignore in this case?

ok, in that case you need to return; right now it's just continuing on with podIP=""

pravisankar · 2017-06-30T08:21:20Z

re[test]

openshift-bot · 2017-07-05T17:39:30Z

Evaluated for origin test up to 084e376

openshift-bot · 2017-07-05T19:23:31Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2975/) (Base Commit: 22888b2) (PR Branch Commit: 084e376)

smarterclayton · 2017-07-12T00:31:30Z

Was this slated for 3.6?

pravisankar · 2017-07-12T23:23:34Z

On Tue, Jul 11, 2017 at 5:31 PM, Clayton Coleman ***@***.***> wrote: Was this slated for 3.6?

No, targeted for 3.6.1

…

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#14893 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABM0hqV7pag4e3mCPZga9zjw83lmmQPyks5sNBPlgaJpZM4OF-MH> .

deads2k · 2017-07-26T19:49:17Z

No, targeted for 3.6.1

Can you make another pull against the 3.6 branch (and an issue) and free this up to merge against master?

deads2k · 2017-07-26T19:49:23Z

/unassign

dcbw · 2017-08-04T18:23:52Z

/approve
/lgtm

though you'll want to rebase when #14894 lands so that we don't need yet another approval from clayton/liggit/etc

deads2k · 2017-08-07T13:38:32Z

/approve

openshift-bot · 2017-08-07T15:51:55Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2017-08-07T19:37:55Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2017-08-07T23:39:55Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2017-08-08T03:41:54Z

/retest

Please review the full test history for this PR and help us cut down flakes.

smarterclayton · 2017-08-08T03:51:01Z

I haven't seen the unit tests pass yet. Have they actually passed and I just missed it?

danwinship · 2017-08-08T13:33:49Z

[WARNING] `go test` had the following output to stderr:
# github.com/openshift/origin/pkg/sdn/plugin
pkg/sdn/plugin/ovscontroller_test.go:303: not enough arguments in call to oc.TearDownPod

We have seen bugs where podIP is not valid/empty for teardown operation, in this case ovs flows are left out. This change will fix this case by picking the podIP from ovs flows (using 'note' field that was derived from pod sandbox ID).

pravisankar · 2017-08-08T21:55:12Z

@danwinship @dcbw fixed ovscontroller_test.go, ptal

pravisankar · 2017-08-08T21:55:25Z

/retest

dcbw · 2017-08-09T17:53:59Z

/retest

All the errors are flakes attempting to either get the RPM packages or to pull the openshift docker images.

dcbw · 2017-08-09T17:54:10Z

changes /lgtm

pravisankar · 2017-08-17T23:11:29Z

/retest

danwinship · 2017-08-18T13:24:38Z

changes /lgtm

@dcbw FYI looks like that didn't work. I guess the command needs to be at the start of the line?
/lgtm

openshift-merge-robot · 2017-08-18T13:24:49Z

/test all [submit-queue is verifying that this PR is safe to merge]

openshift-merge-robot · 2017-08-18T13:24:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, dcbw, deads2k, 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/sdn/OWNERS~~ [danwinship,dcbw,deads2k,pravisankar]

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

pravisankar · 2017-08-18T17:51:19Z

/test extended_conformance_gce

openshift-merge-robot · 2017-08-18T19:05:46Z

/test all [submit-queue is verifying that this PR is safe to merge]

openshift-merge-robot · 2017-08-18T21:02:34Z

Automatic merge from submit-queue

danwinship reviewed Jun 27, 2017

View reviewed changes

dcbw suggested changes Jun 27, 2017

View reviewed changes

danwinship mentioned this pull request Jun 27, 2017

Bug 1453190 - Fix pod update operation #14892

Merged

pravisankar force-pushed the make-teardown-robust branch from cfdf928 to 7afe292 Compare June 28, 2017 00:12

danwinship reviewed Jun 28, 2017

View reviewed changes

pravisankar force-pushed the make-teardown-robust branch 2 times, most recently from 8f88ae0 to 4c82dd3 Compare June 28, 2017 22:22

dcbw approved these changes Jun 29, 2017

View reviewed changes

pravisankar force-pushed the make-teardown-robust branch from 4c82dd3 to d9a1d53 Compare June 30, 2017 08:21

pravisankar force-pushed the make-teardown-robust branch 2 times, most recently from a87117b to 084e376 Compare July 5, 2017 17:36

pravisankar added the component/networking label Jul 5, 2017

openshift-merge-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 24, 2017

openshift-merge-robot assigned deads2k and dcbw Jul 25, 2017

pravisankar force-pushed the make-teardown-robust branch from 084e376 to ce9aa5a Compare July 26, 2017 19:21

openshift-merge-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 Jul 26, 2017

openshift-ci-robot unassigned deads2k Jul 26, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 28, 2017

openshift-merge-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 28, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 4, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 7, 2017

smarterclayton removed the lgtm Indicates that a PR is ready to be merged. label Aug 8, 2017

Make sdn pod teardown robust

a4130be

We have seen bugs where podIP is not valid/empty for teardown operation, in this case ovs flows are left out. This change will fix this case by picking the podIP from ovs flows (using 'note' field that was derived from pod sandbox ID).

pravisankar force-pushed the make-teardown-robust branch from ce9aa5a to a4130be Compare August 8, 2017 21:52

openshift-merge-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 8, 2017

openshift-ci-robot assigned danwinship Aug 18, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 18, 2017

openshift-merge-robot merged commit 0dfa88a into openshift:master Aug 18, 2017

Make sdn pod teardown robust #14893

Make sdn pod teardown robust #14893

Conversation

pravisankar commented Jun 26, 2017 • edited

pravisankar commented Jun 26, 2017

pravisankar commented Jun 26, 2017

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

pravisankar commented Jun 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pravisankar commented Jun 30, 2017

openshift-bot commented Jul 5, 2017

openshift-bot commented Jul 5, 2017

smarterclayton commented Jul 12, 2017

pravisankar commented Jul 12, 2017 via email

deads2k commented Jul 26, 2017

deads2k commented Jul 26, 2017

dcbw commented Aug 4, 2017

deads2k commented Aug 7, 2017

openshift-bot commented Aug 7, 2017

openshift-bot commented Aug 7, 2017

openshift-bot commented Aug 7, 2017

openshift-bot commented Aug 8, 2017

smarterclayton commented Aug 8, 2017

danwinship commented Aug 8, 2017

pravisankar commented Aug 8, 2017

pravisankar commented Aug 8, 2017

dcbw commented Aug 9, 2017

dcbw commented Aug 9, 2017

pravisankar commented Aug 17, 2017

danwinship commented Aug 18, 2017

openshift-merge-robot commented Aug 18, 2017

openshift-merge-robot commented Aug 18, 2017

pravisankar commented Aug 18, 2017

openshift-merge-robot commented Aug 18, 2017

openshift-merge-robot commented Aug 18, 2017

pravisankar commented Jun 26, 2017 •

edited