Fix issue in updating device path when volume is attached multiple times #33796

jingxu97 · 2016-09-29T22:35:43Z

When volume is attached, it is possible that the actual state
already has this volume object (e.g., the volume is attached to multiple
nodes, or volume was detached and attached again). We need to update the
device path in such situation, otherwise, the device path would be stale
information and cause kubelet mount to the wrong device.

Fix issue in updating device path when volume is attached multiple times

This PR partially fixes issue #29324

This change is

k8s-ci-robot · 2016-09-29T23:01:10Z

Jenkins unit/integration failed for commit a897f308d0975a69016d146395a07dc5f6db6664. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit 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-30T20:20:33Z

@saad-ali PTAL. This is the fix for PR #29324

k8s-ci-robot · 2016-09-30T23:23:05Z

Jenkins GCI GKE smoke e2e failed for commit db79bcd. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke 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-30T23:33:17Z

@k8s-bot gke e2e test this

justinsb · 2016-09-30T23:42:27Z

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

@@ -279,8 +279,13 @@ func (asw *actualStateOfWorld) AddVolumeNode(
 			nodesAttachedTo: make(map[types.NodeName]nodeAttachedTo),
 			devicePath:      devicePath,
 		}
-		asw.attachedVolumes[volumeName] = volumeObj
+	} else {
+		// If volume object already exists, it indicates that the information would be out of date.


Do you think we should glog.Info or glog.V(2).Info, so we can have some indication in the logs?

added the log

justinsb · 2016-09-30T23:47:50Z

pkg/kubelet/volumemanager/cache/actual_state_of_world.go

@@ -367,6 +367,10 @@ func (asw *actualStateOfWorld) addVolume(
 			devicePath:         devicePath,
 		}
 		asw.attachedVolumes[volumeName] = volumeObj
+	} else {
+		// If volume object already exists, update the fields such as device path


And here as well I guess?

added the log

justinsb

LGTM, though I don't know this code other than what you have taught me! Did propose some additional logging points, if you think that would be helpful if we have to diagnose this further.

k8s-ci-robot · 2016-09-30T23:52:03Z

Jenkins GKE smoke e2e failed for commit db79bcd. Full PR test history.

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

k8s-ci-robot · 2016-09-30T23:52:07Z

Jenkins Kubemark GCE e2e failed for commit db79bcd. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark 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-10-03T17:25:54Z

pkg/kubelet/volumemanager/cache/actual_state_of_world.go

@@ -367,6 +367,10 @@ func (asw *actualStateOfWorld) addVolume(
 			devicePath:         devicePath,
 		}
 		asw.attachedVolumes[volumeName] = volumeObj
+	} else {
+		// If volume object already exists, update the fields such as device path


added the log

jingxu97 · 2016-10-03T17:33:11Z

addressed comments. @justinsb @saad-ali PTAL

k8s-ci-robot · 2016-10-03T18:14:35Z

Jenkins GCE e2e failed for commit 2135002. 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-10-03T18:48:14Z

@k8s-bot gce e2e test this

jingxu97 · 2016-10-03T22:49:09Z

@k8s-bot gce e2e test this

saad-ali · 2016-10-03T22:34:41Z

pkg/kubelet/volumemanager/cache/actual_state_of_world.go

+	} else {
+		// If volume object already exists, update the fields such as device path
+		volumeObj.devicePath = devicePath
+		volumeObj.spec = volumeSpec


This will not update the devicePath/volumeSpec for asw.attachedVolumes[volumeName]. It only updates the temporary volumeObj object. To update the value in the map, you have to write the temp object back:

asw.attachedVolumes[volumeName] = volumeObj

nit: also add a unit test verifying new behavior please.

I tried to add the unit test, but this device path stored in attachedVolumes does not used anywhere. So right now there is no public API to retrieve this piece of information. If we want to test it, it requires to add a new API which we don't really need right now.

I changed this part of code mainly for consistency.

saad-ali · 2016-10-03T22:38:49Z

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

@@ -26,6 +26,36 @@ import (
 	volumetesting "k8s.io/kubernetes/pkg/volume/testing"
 )

+func verifyAttachedVolume(


nit: Why move the helper method to the top of the file? I'd prefer to leave the helper method near the bottom where it was (makes the diff easier too).

Before it was in the middle of the code. If I move it to the bottom, it will still has the same effect, I think

Got it. Let's keep Tests grouped together at the top, and helper methods at the bottom.

Changed the order

saad-ali · 2016-10-03T22:40:03Z

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

+	verifyAttachedVolume(t, attachedVolumes, generatedVolumeName1, string(volumeName), node1Name, devicePath, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */)
+}
+
+func Test_OneVolumeTwoNodes_TwoDevicePaths(t *testing.T) {


nit: move this up after the last Test_... so all tests are grouped together.

When volume is attached, it is possible that the actual state already has this volume object (e.g., the volume is attached to multiple nodes, or volume was detached and attached again). We need to update the device path in such situation, otherwise, the device path would be stale information and cause kubelet mount to the wrong device. This PR partially fixes issue kubernetes#29324

saad-ali

LGTM Thanks @jingxu97

k8s-github-robot · 2016-10-04T05:24:54Z

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

k8s-github-robot · 2016-10-04T06:01:08Z

Automatic merge from submit-queue

saad-ali · 2016-10-05T23:39:48Z

Adding cherrypick-candidate and v1.4 milestone -- @jingxu97 please create cherrypick for this to ensure it is in v1.4.1

CC @jessfraz

jessfraz · 2016-10-06T00:37:35Z

@jingxu97 can you open the cherry-pick by tomorrow and ping me please, thanks!

jingxu97 · 2016-10-06T01:21:15Z

Sure. Will do it tonight.

On Wed, Oct 5, 2016 at 5:38 PM, Jess Frazelle notifications@github.com
wrote:

@jingxu97 https://github.com/jingxu97 can you open the cherry-pick by
tomorrow and ping me please, thanks!

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#33796 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ASSNxaYJT8oNHM9YAnvI9yM0oUMunnLOks5qxEL8gaJpZM4KKjcf
.

Jing

jingxu97 · 2016-10-06T08:25:44Z

cherrypick PR is here #34215
Please let me know if there is any issue. Thanks!

On Wed, Oct 5, 2016 at 6:20 PM, Jing Xu jinxu@google.com wrote:

Sure. Will do it tonight.

On Wed, Oct 5, 2016 at 5:38 PM, Jess Frazelle notifications@github.com
wrote:

@jingxu97 https://github.com/jingxu97 can you open the cherry-pick by
tomorrow and ping me please, thanks!

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#33796 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ASSNxaYJT8oNHM9YAnvI9yM0oUMunnLOks5qxEL8gaJpZM4KKjcf
.

Jing

Jing

…96-upstream-release-1.4 Automatic merge from submit-queue Automated cherry pick of #33796 Cherry pick of #33796 on release-1.4. #33796: Fix issue in updating device path when volume is attached

k8s-cherrypick-bot · 2016-10-06T20:02:36Z

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-06T20:26:41Z

Thanks @jingxu97. 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-#33796-upstream-release-1.4 Automatic merge from submit-queue Automated cherry pick of kubernetes#33796 Cherry pick of kubernetes#33796 on release-1.4. kubernetes#33796: Fix issue in updating device path when volume is attached

googlebot added the cla: yes label Sep 29, 2016

jingxu97 assigned saad-ali and justinsb Sep 29, 2016

jingxu97 added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Sep 29, 2016

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

jingxu97 changed the title ~~Fix issue in updating device path when volume is attached~~ Fix issue in updating device path when volume is attached multiple times Sep 29, 2016

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Sep 29, 2016

jingxu97 force-pushed the quickfix-aws-9-28 branch from a897f30 to e6c9947 Compare September 30, 2016 17:47

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

[k8s.io] Kubectl client [k8s.io] Guestbook application should create and stop a working application [Conformance] {Kubernetes e2e suite} #33672

Closed

jingxu97 mentioned this pull request Sep 30, 2016

Wrong AWS volume can be mounted #29324

Closed

jingxu97 force-pushed the quickfix-aws-9-28 branch from e6c9947 to db79bcd Compare September 30, 2016 23:05

justinsb reviewed Sep 30, 2016

View reviewed changes

jingxu97 commented Oct 3, 2016

View reviewed changes

jingxu97 force-pushed the quickfix-aws-9-28 branch from db79bcd to 2135002 Compare October 3, 2016 17:29

saad-ali suggested changes Oct 3, 2016

View reviewed changes

jingxu97 force-pushed the quickfix-aws-9-28 branch from 2135002 to 9e8edf6 Compare October 4, 2016 00:14

saad-ali approved these changes Oct 4, 2016

View reviewed changes

saad-ali added lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Oct 4, 2016

k8s-github-robot merged commit 1dc8277 into kubernetes:master Oct 4, 2016

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

jessfraz added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Oct 6, 2016

jingxu97 mentioned this pull request Oct 6, 2016

Automated cherry pick of #33796 #34215

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 6, 2016

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

saad-ali added cherrypick-candidate and removed cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. labels Oct 6, 2016

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

This was referenced Oct 10, 2016

Automated cherry pick of #33796 #34486

Closed

wrong volume mounted on AWS when EBS volume changes device node as part of detach from nodeX and attach to nodeY #34437

Closed

Automated cherry pick of #33796 #34557

Closed

jingxu97 mentioned this pull request Dec 7, 2016

AWS volume attached but not mounted, service started anyway #32295

Closed

k8s-cherrypick-bot removed the cherrypick-candidate label Jul 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue in updating device path when volume is attached multiple times #33796

Fix issue in updating device path when volume is attached multiple times #33796

jingxu97 commented Sep 29, 2016 •

edited by jessfraz

Loading

k8s-ci-robot commented Sep 29, 2016

jingxu97 commented Sep 30, 2016

k8s-ci-robot commented Sep 30, 2016

jingxu97 commented Sep 30, 2016

justinsb Sep 30, 2016

jingxu97 Oct 3, 2016

justinsb Sep 30, 2016

jingxu97 Oct 3, 2016

justinsb left a comment

k8s-ci-robot commented Sep 30, 2016

k8s-ci-robot commented Sep 30, 2016

jingxu97 Oct 3, 2016

jingxu97 commented Oct 3, 2016

k8s-ci-robot commented Oct 3, 2016

jingxu97 commented Oct 3, 2016

jingxu97 commented Oct 3, 2016

saad-ali Oct 3, 2016

saad-ali Oct 3, 2016

jingxu97 Oct 4, 2016 •

edited

Loading

saad-ali Oct 3, 2016

jingxu97 Oct 3, 2016

saad-ali Oct 3, 2016

jingxu97 Oct 3, 2016

saad-ali Oct 3, 2016

saad-ali left a comment

k8s-github-robot commented Oct 4, 2016

k8s-github-robot commented Oct 4, 2016

saad-ali commented Oct 5, 2016

jessfraz commented Oct 6, 2016

jingxu97 commented Oct 6, 2016

jingxu97 commented Oct 6, 2016

k8s-cherrypick-bot commented Oct 6, 2016

saad-ali commented Oct 6, 2016 •

edited

Loading

Fix issue in updating device path when volume is attached multiple times #33796

Fix issue in updating device path when volume is attached multiple times #33796

Conversation

jingxu97 commented Sep 29, 2016 • edited by jessfraz Loading

k8s-ci-robot commented Sep 29, 2016

jingxu97 commented Sep 30, 2016

k8s-ci-robot commented Sep 30, 2016

jingxu97 commented Sep 30, 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

justinsb left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 30, 2016

k8s-ci-robot commented Sep 30, 2016

Choose a reason for hiding this comment

jingxu97 commented Oct 3, 2016

k8s-ci-robot commented Oct 3, 2016

jingxu97 commented Oct 3, 2016

jingxu97 commented Oct 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jingxu97 Oct 4, 2016 • edited Loading

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

k8s-github-robot commented Oct 4, 2016

k8s-github-robot commented Oct 4, 2016

saad-ali commented Oct 5, 2016

jessfraz commented Oct 6, 2016

jingxu97 commented Oct 6, 2016

jingxu97 commented Oct 6, 2016

k8s-cherrypick-bot commented Oct 6, 2016

saad-ali commented Oct 6, 2016 • edited Loading

jingxu97 commented Sep 29, 2016 •

edited by jessfraz

Loading

jingxu97 Oct 4, 2016 •

edited

Loading

saad-ali commented Oct 6, 2016 •

edited

Loading