Fix race condition in setting node statusUpdateNeeded flag #32807

jingxu97 · 2016-09-15T18:19:22Z

This PR fixes the race condition in setting node statusUpdateNeeded flag
in master's attachdetach controller. This flag is used to indicate
whether a node status has been updated by the node_status_updater or
not. When updater finishes update a node status, it is set to false.
When the node status is changed such as volume is detached or new volume
is attached to the node, the flag is set to true so that updater can
update the status again. The previous workflow has a race condition as
follows

updater gets the currently attached volume list from the node which needs to be
updated.
A new volume A is attached to the same node right after 1 and set the
flag to TRUE
updater updates the node attached volume list (which does not include volume A) and then set the flag to FALSE.
The result is that volume A will be never added to the attached volume
list so at node side, this volume is never attached.

So in this PR, the flag is set to FALSE when updater tries to get the
attached volume list (as in an atomic operation). So in the above
example, after step 2, the flag will be TRUE again, in step 3, updater
does not set the flag if updates is sucessful. So after that, flag is
still TRUE and in next round of update, the node status will be updated.

Fix race condition in setting node statusUpdateNeeded flag

This change is

saad-ali · 2016-09-15T21:01:56Z

Removing this from 1.4 milestone per offline discussion. We will get it merged, give it time to bake, and merge it into a 1.4.x release.

k8s-bot · 2016-09-16T00:47:36Z

GCE e2e build/test passed for commit 25a0678.

saad-ali · 2016-09-19T23:21:58Z

pkg/controller/volume/attachdetach/cache/actual_state_of_world.go

-	ResetNodeStatusUpdateNeeded(nodeName string) error
+	// node to true indicating the AttachedVolume field of the Node's Status
+	// object needs to be updated by the node updater again.
+	ResetNodeStatusUpdateNeeded(nodeName string)


Revert the If no node with the... portion of the comment since it is still applicable.

Here I remove the returned error because I could not see the use of it.

This method can result in a failed to setNodeStatusUpdateNeeded error if nodeName does not exist. I like documentation comments to capture 1) what the input is, 2) what the normal output is, and 3) what results in an error. So I would leave the comment as is. That said, I'll leave it up to you to decide.

update the comments

saad-ali · 2016-09-19T23:29:00Z

pkg/controller/volume/attachdetach/cache/actual_state_of_world.go

-	ResetNodeStatusUpdateNeeded(nodeName string) error
+	// node to true indicating the AttachedVolume field of the Node's Status
+	// object needs to be updated by the node updater again.
+	ResetNodeStatusUpdateNeeded(nodeName string)


SInce this method now sets the value of statusUpdatedNeeded to true instead of false, change the name to reflect the behavior: SetNodeStatusUpdateNeeded(...)

saad-ali

A couple minor comments, otherwise LGTM

jingxu97 · 2016-09-21T21:03:37Z

@saad-ali PTAL

jingxu97 · 2016-09-22T17:53:24Z

@saad-ali PTAL

saad-ali · 2016-09-22T18:35:34Z

pkg/controller/volume/attachdetach/cache/actual_state_of_world.go

-			"failed to ResetNodeStatusUpdateNeeded(nodeName=%q) nodeName does not exist",
+		// should not happen
+		glog.Errorf(
+			"failed to setNodeStatusUpdateNeeded(nodeName=%q) nodeName does not exist",


Add needed bool to error message for clarity.

saad-ali · 2016-09-22T18:37:48Z

pkg/controller/volume/attachdetach/cache/actual_state_of_world.go

-	ResetNodeStatusUpdateNeeded(nodeName string) error
+	// node to true indicating the AttachedVolume field of the Node's Status
+	// object needs to be updated by the node updater again.
+	ResetNodeStatusUpdateNeeded(nodeName string)


This method can result in a failed to setNodeStatusUpdateNeeded error if nodeName does not exist. I like documentation comments to capture 1) what the input is, 2) what the normal output is, and 3) what results in an error. So I would leave the comment as is. That said, I'll leave it up to you to decide.

saad-ali

A few more comments

saad-ali · 2016-09-22T18:38:39Z

pkg/controller/volume/attachdetach/cache/actual_state_of_world.go

+// Update the flag statusUpdateNeeded to indicate whether node status is already updated or
+// needs to be updated again by the node status updater.
+// This is an internal function and caller should acquire and release the lock
+func (asw *actualStateOfWorld) setNodeStatusUpdateNeeded(nodeName string, needed bool) {


nit: to avoid confusing this with the SetNodeStatusUpdateNeeded which sets the value to true, maybe rename it to modifyNodeStatusUpdateNeeded?

This PR fixes the race condition in setting node statusUpdateNeeded flag in master's attachdetach controller. This flag is used to indicate whether a node status has been updated by the node_status_updater or not. When updater finishes update a node status, it is set to false. When the node status is changed such as volume is detached or new volume is attached to the node, the flag is set to true so that updater can update the status again. The previous workflow has a race condition as follows 1. updater gets the currently attached volume list from the node which needs to be updated. 2. A new volume A is attached to the same node right after 1 and set the flag to TRUE 3. updater updates the node attached volume list (which does not include volume A) and then set the flag to FALSE. The result is that volume A will be never added to the attached volume list so at node side, this volume is never attached. So in this PR, the flag is set to FALSE when updater tries to get the attached volume list (as in an atomic operation). So in the above example, after step 2, the flag will be TRUE again, in step 3, updater does not set the flag if updates is sucessful. So after that, flag is still TRUE and in next round of update, the node status will be updated. This PR also changes a unit test due to the workflow changes

jingxu97 · 2016-09-22T21:04:21Z

@saad-ali PTAL

saad-ali

LGTM

saad-ali · 2016-09-22T21:06:43Z

This PR should be cherry-picked to v1.4.1.

k8s-ci-robot · 2016-09-23T16:30:00Z

Jenkins GCE e2e failed for commit 14cad20. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

jingxu97 · 2016-09-23T16:48:56Z

@k8s-bot gce e2e test this

k8s-github-robot · 2016-09-23T17:46:02Z

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

k8s-github-robot · 2016-09-23T18:25:16Z

Automatic merge from submit-queue

saad-ali · 2016-09-26T20:03:31Z

Adding cherrypick-candidate and v1.4 milestone to have this picked up for v1.4.1

…07-upstream-release-1.4 Automatic merge from submit-queue Automated cherry pick of #32807 Cherry pick of #32807 on release-1.4.

k8s-cherrypick-bot · 2016-10-04T20:02:26Z

Commit found in the "release-1.4" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

saad-ali · 2016-10-04T20:12:36Z

Now that this has been cherry-picked to the 1.4 branch (for 1.4.1), let's also cherry-pick it to the 1.3 branch (for 1.3.9).

…ck-of-#32807-upstream-release-1.4 Automatic merge from submit-queue Automated cherry pick of kubernetes#32807 Cherry pick of kubernetes#32807 on release-1.4.

k8s-cherrypick-bot · 2017-01-13T16:30:01Z

Commit found in the "release-1.3" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

googlebot added the cla: yes label Sep 15, 2016

jingxu97 added this to the v1.4 milestone Sep 15, 2016

jingxu97 assigned saad-ali, thockin and justinsb Sep 15, 2016

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Sep 15, 2016

k8s-github-robot mentioned this pull request Sep 15, 2016

[k8s.io] Kubectl client [k8s.io] Kubectl run --quiet job should not output anything except the command when --quiet is set {Kubernetes e2e suite} #32548

Closed

saad-ali removed this from the v1.4 milestone Sep 15, 2016

jingxu97 changed the title ~~Fix race conditino in setting node statusUpdateNeeded flag~~ Fix race condition in setting node statusUpdateNeeded flag Sep 15, 2016

jingxu97 force-pushed the stateupdateNeeded-9-15 branch 2 times, most recently from dfeb259 to 25a0678 Compare September 16, 2016 00:07

saad-ali reviewed Sep 19, 2016

View reviewed changes

saad-ali suggested changes Sep 19, 2016

View reviewed changes

jingxu97 mentioned this pull request Sep 21, 2016

Mounting/unmounting volume error on GKE #29903

Closed

jingxu97 force-pushed the stateupdateNeeded-9-15 branch from 25a0678 to d696d45 Compare September 22, 2016 18:27

saad-ali reviewed Sep 22, 2016

View reviewed changes

saad-ali suggested changes Sep 22, 2016

View reviewed changes

jingxu97 force-pushed the stateupdateNeeded-9-15 branch from d696d45 to 14cad20 Compare September 22, 2016 21:02

saad-ali approved these changes Sep 22, 2016

View reviewed changes

saad-ali added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed release-note-label-needed labels Sep 22, 2016

k8s-github-robot added the release-note-label-needed label Sep 22, 2016

k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Sep 22, 2016

jingxu97 added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. release-note-label-needed labels Sep 22, 2016

jingxu97 added this to the v1.4 milestone Sep 22, 2016

jingxu97 removed the release-note-none Denotes a PR that doesn't merit a release note. label Sep 22, 2016

jingxu97 removed this from the v1.4 milestone Sep 22, 2016

jingxu97 added the release-note-none Denotes a PR that doesn't merit a release note. label Sep 22, 2016

k8s-github-robot merged commit 0a4316f into kubernetes:master Sep 23, 2016

saad-ali added this to the v1.4 milestone Sep 26, 2016

saad-ali added the cherrypick-candidate label Sep 26, 2016

jingxu97 mentioned this pull request Oct 4, 2016

Automated cherry pick of #32807 #34038

Merged

jessfraz added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Oct 4, 2016

k8s-github-robot pushed a commit that referenced this pull request Oct 4, 2016

Merge pull request #34038 from jingxu97/automated-cherry-pick-of-#328…

1ca4a63

…07-upstream-release-1.4 Automatic merge from submit-queue Automated cherry pick of #32807 Cherry pick of #32807 on release-1.4.

k8s-cherrypick-bot removed the cherrypick-candidate label Oct 4, 2016

saad-ali modified the milestones: v1.3, v1.4 Oct 4, 2016

saad-ali added the cherrypick-candidate label Oct 5, 2016

k8s-cherrypick-bot removed the cherrypick-candidate label Jan 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix race condition in setting node statusUpdateNeeded flag #32807

Fix race condition in setting node statusUpdateNeeded flag #32807

jingxu97 commented Sep 15, 2016 •

edited by jessfraz

Loading

saad-ali commented Sep 15, 2016

k8s-bot commented Sep 16, 2016

saad-ali Sep 19, 2016

jingxu97 Sep 21, 2016

saad-ali Sep 22, 2016

jingxu97 Sep 22, 2016

saad-ali Sep 19, 2016

jingxu97 Sep 21, 2016

saad-ali left a comment

jingxu97 commented Sep 21, 2016

jingxu97 commented Sep 22, 2016

saad-ali Sep 22, 2016

jingxu97 Sep 22, 2016

jingxu97 Sep 22, 2016

saad-ali Sep 22, 2016

saad-ali left a comment

saad-ali Sep 22, 2016

jingxu97 Sep 22, 2016

jingxu97 Sep 22, 2016

jingxu97 commented Sep 22, 2016

saad-ali left a comment

saad-ali commented Sep 22, 2016

k8s-ci-robot commented Sep 23, 2016

jingxu97 commented Sep 23, 2016

k8s-github-robot commented Sep 23, 2016

k8s-github-robot commented Sep 23, 2016

saad-ali commented Sep 26, 2016

k8s-cherrypick-bot commented Oct 4, 2016

saad-ali commented Oct 4, 2016 •

edited

Loading

k8s-cherrypick-bot commented Jan 13, 2017

Fix race condition in setting node statusUpdateNeeded flag #32807

Fix race condition in setting node statusUpdateNeeded flag #32807

Conversation

jingxu97 commented Sep 15, 2016 • edited by jessfraz Loading

saad-ali commented Sep 15, 2016

k8s-bot commented Sep 16, 2016

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

saad-ali left a comment

Choose a reason for hiding this comment

jingxu97 commented Sep 21, 2016

jingxu97 commented Sep 22, 2016

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

saad-ali 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

jingxu97 commented Sep 22, 2016

saad-ali left a comment

Choose a reason for hiding this comment

saad-ali commented Sep 22, 2016

k8s-ci-robot commented Sep 23, 2016

jingxu97 commented Sep 23, 2016

k8s-github-robot commented Sep 23, 2016

k8s-github-robot commented Sep 23, 2016

saad-ali commented Sep 26, 2016

k8s-cherrypick-bot commented Oct 4, 2016

saad-ali commented Oct 4, 2016 • edited Loading

k8s-cherrypick-bot commented Jan 13, 2017

jingxu97 commented Sep 15, 2016 •

edited by jessfraz

Loading

saad-ali commented Oct 4, 2016 •

edited

Loading