Revert 20202. Use other measures to prevent race in test-cmd.sh #21175

caesarxuchao · 2016-02-12T19:34:13Z

#20202 skips the update when a pod is deleted with grace period = 0. That breaks the non-graceful deletion, pods will get a default deletion grace period even if the grace period is set to 0. This PR adds back the update. So the race between API server and nodecontroller/kubelet in a non-graceful deletion comes back. I haven't taken any measure to fix this race in this PR, because this race only occurs if API server is context-switched after it has done the "update" but before it does the "delete", i.e. here, which is unlikely to happen.

Reverting #20202 also lets another kind of race come back: kubelet and nodecontroller might send a deletion request that ends up deleting a deleted and then recreated pod. Kubelet has taken measures to prevent such a race, ~~I made the node controller to do the same in the second commit.~~ [Update] this is too risky for 1.2, I removed this commit.

cc @mikedanese.

I also revised hack/test-cmd.sh to switch namespace before recreating a deleted pod to completely avoid the latter race condition when running that script.

@smarterclayton @yujuhong @lavalamp

k8s-github-robot · 2016-02-12T19:35:39Z

Labelling this PR as size/M

k8s-bot · 2016-02-12T20:07:16Z

GCE e2e test build/test passed for commit f757bf7eb39c575accdafc6a1581b7d2c3d20e6d.

lavalamp · 2016-02-18T20:22:32Z

pkg/controller/node/nodecontroller.go

+		return
+	}
+	// This doesn't entirely avoid the race condition, pod can be deleted and recreated after this check
+	// but before the deletion request sent to the server.


Hm. Is it worth it to add this? ( @smarterclayton, opinions?)

It'd be better to add an optional UID precondition to delete. Like, add UID to the DeleteOptions type.

Can we do that for all pod operations? In kubelet, we also have to check if the pod we're getting from the apiserver before a status update is actually the pod we mean to update. The same applies to deletion. Since we use the pod UID as the key, it seems to make sense to be able to send a request to the apiserver with UID.

It'd be better to add an optional UID precondition to delete. Like, add UID to the DeleteOptions type.

I can do that. I thought it's an post 1.2 goal. Shall we do it for 1.2?

I think it's too risky for 1.2 right now. It's not a regression, it's just undesirable.

Ok. I'll revert the change to node controller.

@yujuhong I added your concern with "Get" operations to #20572.

caesarxuchao · 2016-02-19T18:20:33Z

@lavalamp I removed the node controller change. PTAL. Thanks.

k8s-bot · 2016-02-19T18:47:30Z

GCE e2e build/test failed for commit dae6091b565096511177abfbbc971bbcd807f3ec.

k8s-bot · 2016-02-19T20:41:57Z

GCE e2e test build/test passed for commit 314a6ab.

lavalamp · 2016-02-19T22:54:26Z

LGTM, thanks!

k8s-github-robot · 2016-02-20T13:56:41Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-02-20T14:21:31Z

GCE e2e test build/test passed for commit 314a6ab.

k8s-github-robot · 2016-02-22T05:00:18Z

@caesarxuchao
You must link to the test flake issue which caused you to request this manual re-test.
Re-test requests should be in the form of: k8s-bot test this issue: #<number>
Here is the list of open test flakes.

k8s-github-robot · 2016-02-22T05:00:18Z

@caesarxuchao
You must link to the test flake issue which caused you to request this manual re-test.
Re-test requests should be in the form of: k8s-bot test this issue: #<number>
Here is the list of open test flakes.

k8s-bot · 2016-02-22T05:16:58Z

GCE e2e test build/test passed for commit 314a6ab.

caesarxuchao · 2016-02-22T18:13:43Z

@k8s-bot unit test this issue: #21451

caesarxuchao · 2016-02-23T22:24:45Z

Still the same issue: /registry/Error deleting container: Error response from daemon: Conflict, You cannot remove a running container. Stop the container before attempting removal or use -f
Unrecognized input header

caesarxuchao · 2016-02-24T06:39:45Z

@k8s-bot unit test this issue: #21451

caesarxuchao · 2016-02-24T18:00:16Z

@k8s-bot unit test this issue: #21451

k8s-github-robot · 2016-02-25T11:08:45Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-02-25T11:38:13Z

GCE e2e test build/test passed for commit 314a6ab.

yujuhong · 2016-02-25T16:08:23Z

@k8s-bot unit test this issue: #21451

k8s-github-robot · 2016-02-25T17:02:32Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-02-25T17:37:12Z

GCE e2e test build/test passed for commit 314a6ab.

k8s-github-robot · 2016-02-25T20:00:29Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-02-25T20:31:31Z

GCE e2e build/test failed for commit 314a6ab.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

caesarxuchao · 2016-02-25T21:29:02Z

@k8s-bot test this please, issue: #20904, #21487

k8s-bot · 2016-02-25T22:12:22Z

GCE e2e build/test failed for commit 314a6ab.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

caesarxuchao · 2016-02-26T19:49:43Z

@k8s-bot e2e test this, issue: #21753

k8s-bot · 2016-02-26T20:21:07Z

GCE e2e build/test passed for commit 314a6ab.

k8s-github-robot · 2016-02-26T20:52:44Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-02-26T21:24:05Z

GCE e2e build/test failed for commit 314a6ab.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

caesarxuchao · 2016-02-26T23:59:32Z

@k8s-bot e2e test this, issue: #21753

k8s-bot · 2016-02-27T00:30:14Z

GCE e2e build/test failed for commit 314a6ab.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

caesarxuchao · 2016-02-27T01:01:16Z

@k8s-bot e2e test this, issue: #21484

k8s-bot · 2016-02-27T01:37:19Z

GCE e2e build/test passed for commit 314a6ab.

k8s-github-robot · 2016-02-27T02:00:34Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-02-27T02:33:17Z

GCE e2e build/test passed for commit 314a6ab.

k8s-github-robot · 2016-02-27T02:33:42Z

Automatic merge from submit-queue

Auto commit by PR queue bot

fabioy · 2016-02-27T03:25:05Z

I'm going to have to revert this change. After this went in, kubernetes-go-test started failing:

19:23:25 �(B+++ [0226 19:23:25] Testing resource aliasing
19:23:25 replicationcontroller "cassandra" created
19:23:25 I0226 19:23:25.736468 7531 event.go:211] Event(api.ObjectReference{Kind:"ReplicationController", Namespace:"default", Name:"cassandra", UID:"767d73df-dd01-11e5-befa-0242ac110003", APIVersion:"v1", ResourceVersion:"1034", FieldPath:""}): type: 'Normal' reason: 'SuccessfulCreate' Created pod: cassandra-2wxmq
19:23:25 I0226 19:23:25.736613 7531 event.go:211] Event(api.ObjectReference{Kind:"ReplicationController", Namespace:"default", Name:"cassandra", UID:"767d73df-dd01-11e5-befa-0242ac110003", APIVersion:"v1", ResourceVersion:"1034", FieldPath:""}): type: 'Normal' reason: 'SuccessfulCreate' Created pod: cassandra-hsdrt
19:23:26 replicationcontroller "cassandra" scaled
19:23:26 service "cassandra" created
19:23:26
19:23:26 FAIL!
19:23:26 Get all -l'app=cassandra' {{range.items}}{{range .metadata.labels}}{{.}}:{{end}}{{end}}
19:23:26 Expected: cassandra:cassandra:cassandra:
19:23:26 Got: cassandra:cassandra:cassandra:cassandra:
19:23:26 �(B
19:23:26 1563 ./hack/test-cmd.sh
19:23:26 �(B
19:23:26 !!! Error in ./hack/test-cmd.sh:51
19:23:26 'return 1' exited with status 1
19:23:26 Call stack:
19:23:26 1: ./hack/test-cmd.sh:51 runTests(...)
19:23:26 2: ./hack/test-cmd.sh:1636 main(...)
19:23:26 Exiting with status 1

fabioy · 2016-02-27T03:59:07Z

Reverting made Jenkins go green again. My apologies, but you'll have to redo this PR.

caesarxuchao · 2016-02-27T04:05:11Z

@fabioy Thanks for taking care of the builds. I think Jenkins didn't re-run the unit tests before merge so the failure was not caught. I'll patch the PR and submit it again.

caesarxuchao added the 0 - Backlog label Feb 12, 2016

googlebot added the cla: yes label Feb 12, 2016

caesarxuchao removed 0 - Backlog labels Feb 12, 2016

k8s-github-robot assigned lavalamp Feb 12, 2016

k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 12, 2016

caesarxuchao mentioned this pull request Feb 12, 2016

adding back the update when grace-period=0 #20763

Closed

lavalamp reviewed Feb 18, 2016
View reviewed changes

caesarxuchao force-pushed the revert-20202 branch from f757bf7 to dae6091 Compare February 19, 2016 18:18

googlebot added the cla: yes label Feb 19, 2016

Chao Xu added 2 commits February 19, 2016 11:37

revert 20202

4342a8c

switch namespace before recreate a pod in test-cmd.sh

314a6ab

caesarxuchao force-pushed the revert-20202 branch from dae6091 to 314a6ab Compare February 19, 2016 20:14

lavalamp added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 19, 2016

smarterclayton mentioned this pull request Feb 20, 2016

Finalize Kube items 3.2 openshift/origin#6766

Closed

85 tasks

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 22, 2016

yujuhong added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Feb 25, 2016

k8s-github-robot pushed a commit that referenced this pull request Feb 27, 2016

Merge pull request #21175 from caesarxuchao/revert-20202

a5ceafc

Auto commit by PR queue bot

k8s-github-robot merged commit a5ceafc into kubernetes:master Feb 27, 2016

fabioy mentioned this pull request Feb 27, 2016

Revert "Revert 20202. Use other measures to prevent race in test-cmd.sh" #22119

Merged

caesarxuchao mentioned this pull request Feb 29, 2016

Revert 20202 #22156

Merged

Revert 20202. Use other measures to prevent race in test-cmd.sh #21175

Revert 20202. Use other measures to prevent race in test-cmd.sh #21175

Conversation

caesarxuchao commented Feb 12, 2016

k8s-github-robot commented Feb 12, 2016

k8s-bot commented Feb 12, 2016

lavalamp Feb 18, 2016

Choose a reason for hiding this comment

yujuhong Feb 18, 2016

Choose a reason for hiding this comment

caesarxuchao Feb 18, 2016

Choose a reason for hiding this comment

smarterclayton Feb 18, 2016

Choose a reason for hiding this comment

caesarxuchao Feb 19, 2016

Choose a reason for hiding this comment

caesarxuchao Feb 19, 2016

Choose a reason for hiding this comment

caesarxuchao commented Feb 19, 2016

k8s-bot commented Feb 19, 2016

k8s-bot commented Feb 19, 2016

lavalamp commented Feb 19, 2016

k8s-github-robot commented Feb 20, 2016

k8s-bot commented Feb 20, 2016

k8s-github-robot commented Feb 22, 2016

k8s-github-robot commented Feb 22, 2016

k8s-bot commented Feb 22, 2016

caesarxuchao commented Feb 22, 2016

caesarxuchao commented Feb 23, 2016

caesarxuchao commented Feb 24, 2016

caesarxuchao commented Feb 24, 2016

k8s-github-robot commented Feb 25, 2016

k8s-bot commented Feb 25, 2016

yujuhong commented Feb 25, 2016

k8s-github-robot commented Feb 25, 2016

k8s-bot commented Feb 25, 2016

k8s-github-robot commented Feb 25, 2016

k8s-bot commented Feb 25, 2016

caesarxuchao commented Feb 25, 2016

k8s-bot commented Feb 25, 2016

caesarxuchao commented Feb 26, 2016

k8s-bot commented Feb 26, 2016

k8s-github-robot commented Feb 26, 2016

k8s-bot commented Feb 26, 2016

caesarxuchao commented Feb 26, 2016

k8s-bot commented Feb 27, 2016

caesarxuchao commented Feb 27, 2016

k8s-bot commented Feb 27, 2016

k8s-github-robot commented Feb 27, 2016

k8s-bot commented Feb 27, 2016

k8s-github-robot commented Feb 27, 2016

fabioy commented Feb 27, 2016

fabioy commented Feb 27, 2016

caesarxuchao commented Feb 27, 2016