fix issue in converting aws volume id from mount paths #36840

jingxu97 · 2016-11-15T21:08:59Z

This PR is to fix the issue in converting aws volume id from mount
paths. Currently there are three aws volume id formats supported. The
following lists example of those three formats and their corresponding
global mount paths:

aws:///vol-123456
(/var/lib/kubelet/plugins/kubernetes.io/aws-ebs/mounts/aws/vol-123456)
aws://us-east-1/vol-123456
(/var/lib/kubelet/plugins/kubernetes.io/mounts/aws/us-est-1/vol-123455)
vol-123456
(/var/lib/kubelet/plugins/kubernetes.io/mounts/aws/us-est-1/vol-123455)

For the first two cases, we need to check the mount path and convert
them back to the original format.

This PR fixes #36269

Fix issue in converting AWS volume ID from mount paths

This change is

saad-ali · 2016-11-15T21:16:40Z

pkg/controller/volume/attachdetach/reconciler/reconciler.go

@@ -192,11 +192,11 @@ func (rc *reconciler) reconcile() {
 		if rc.actualStateOfWorld.VolumeNodeExists(
 			volumeToAttach.VolumeName, volumeToAttach.NodeName) {
 			// Volume/Node exists, touch it to reset detachRequestedTime
-			glog.V(1).Infof("Volume %q/Node %q is attached--touching.", volumeToAttach.VolumeName, volumeToAttach.NodeName)
+			glog.V(5).Infof("Volume %q/Node %q is attached--touching.", volumeToAttach.VolumeName, volumeToAttach.NodeName)


Why lower the priority on these messages? Are they happening very frequently?

yes, it prints out log for every reconciler loop

saad-ali · 2016-11-15T21:19:22Z

pkg/util/mount/mount.go

+			// aws://us-east-1/vol-1234 (../aws-ebs/mounts/aws/us-east-1/vol-1234)
+			// vol-1234 (../aws-ebs/mounts/vol-1234)
+			// This code is for dealing with the first two cases.
+			if strings.HasPrefix(volumeID, "aws/") {


Having code to work around an AWS issue in common util/mount is pretty bad. Is there any way this can be isolated to the aws ebs volume plugin?

justinsb · 2016-11-15T21:20:52Z

pkg/util/mount/mount.go

+				} else {
+					volumeID = strings.Replace(volumeID, "aws/", "aws:///", 1)
+				}
+			}


I wonder if we should put this only into the AWS provider?

jingxu97 · 2016-11-15T22:54:35Z

@justinsb @saad-ali addressed the comment, PTAL. Thanks!

k8s-ci-robot · 2016-11-15T23:05:02Z

Jenkins Kubemark GCE e2e failed for commit 5a64cd6. 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.

k8s-ci-robot · 2016-11-15T23:05:04Z

Jenkins GCI GKE smoke e2e failed for commit 5a64cd6. 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.

k8s-ci-robot · 2016-11-15T23:05:32Z

Jenkins GCI GCE e2e failed for commit 5a64cd6. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce 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-11-15T23:05:33Z

Jenkins GCE e2e failed for commit 5a64cd6. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce 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-11-15T23:05:33Z

Jenkins GCE etcd3 e2e failed for commit 5a64cd6. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce etcd3 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-11-15T23:06:04Z

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

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

justinsb · 2016-11-15T23:15:38Z

pkg/volume/aws_ebs/aws_ebs.go

+			sourceName = strings.Replace(volumeID, "aws/", "aws:///", 1)
+		}
+		glog.V(4).Info("Convert aws volume name from %q to %q ", volumeID, sourceName)
+	}


This looks right.

If I'm going to nit-pick, it might be safer to split the string, and then glog.Warning if it doesn't match one of the forms we expect.

i.e. we expect either aws/vol-12345678 or aws/us-east-1b/vol-12345678, but we should log if it doesn't look like that.

Bonus: code might be easier to understand also!

justinsb · 2016-11-15T23:18:38Z

pkg/util/mount/mount.go

+				glog.Errorf("Failed to get volume id from mount %s - %v", mountPath, err)
+				return "", err
+			}
+			return volumeID, nil
 		}


I wonder if we should return a struct with the mount information. I see there's another overload which returns the ref count, so it's difficult to know exactly where this is used by pure grep. But this is my go-to trick (when in doubt, create a type!); not sure it's needed.

Yes, I plan to change the function name to avoid the confusion in another PR.

justinsb · 2016-11-15T23:19:23Z

I think this looks right; I'll try to test on AWS this evening (once tests are green I guess). @jingxu97 any hints on how to trigger the code path?

jingxu97 · 2016-11-16T19:13:32Z

@k8s-bot verify test this

k8s-ci-robot · 2016-11-16T21:13:23Z

Jenkins verification failed for commit 5349593fb2ba20ea3662be1ee7f9683a7e10f6bb. Full PR test history.

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

justinsb

One question, but otherwise LGTM

justinsb · 2016-11-17T06:11:20Z

pkg/volume/aws_ebs/aws_ebs.go

+			sourceName = awsURLNamePrefix + "" + "/" + volName // empty zone label
+		}
+		if length == 3 {
+			sourceName = awsURLNamePrefix + "/" + names[1] + volName // names[1] is the zone label


Are we missing a '/' here? i.e. awsURLNamePrefix + "/" + names[1] + "/" + volName ?

Fixed. Thanks for the review!

This PR is to fix the issue in converting aws volume id from mount paths. Currently there are three aws volume id formats supported. The following lists example of those three formats and their corresponding global mount paths: 1. aws:///vol-123456 (/var/lib/kubelet/plugins/kubernetes.io/aws-ebs/mounts/aws/vol-123456) 2. aws://us-east-1/vol-123456 (/var/lib/kubelet/plugins/kubernetes.io/mounts/aws/us-est-1/vol-123455) 3. vol-123456 (/var/lib/kubelet/plugins/kubernetes.io/mounts/aws/us-est-1/vol-123455) For the first two cases, we need to check the mount path and convert them back to the original format.

dims · 2016-11-18T21:39:34Z

@saad-ali : can you please reapply lgtm? looks like the bot did not pick up your last "/lgtm"

k8s-github-robot · 2016-11-18T22:25:57Z

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

k8s-github-robot · 2016-11-18T23:17:19Z

Automatic merge from submit-queue

justinsb · 2016-11-22T18:35:34Z

@jingxu97 are you going to cherry-pick this to 1.4 or have you done so? Would be great...

jingxu97 · 2016-11-22T19:21:55Z

@justinsb Sure, it is here #37302

Raffo · 2016-11-23T11:52:03Z

@jingxu97 Any information regarding when the next 1.4 release will be?

jingxu97 · 2016-11-24T00:01:17Z

I think it will be one week after thanksgiving.

…

On Wed, Nov 23, 2016 at 3:52 AM, Raffaele Di Fazio ***@***.*** > wrote: @jingxu97 <https://github.com/jingxu97> Any information regarding when the next 1.4 release will be? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#36840 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ASSNxf-XDNMztvULe9zuyVd8NDM_TqDuks5rBCj3gaJpZM4KzBhh> .

-- - Jing

…36840-upstream-release-1.4 Automated cherry pick of #37413 #36840 upstream release 1.4

sharms · 2016-12-07T20:46:38Z

Any chance this can be pushed out? Currently those running 1.4 release don't unmount AWS volumes which is a tough scenario for operations.

mrook · 2016-12-08T13:54:47Z

Yeah we'd love to see 1.4.7 with the fix pushed, we're fighting this issue on a production cluster ATM.

jingxu97 · 2016-12-08T16:50:57Z

Yes, the fix will definitely be available in release 1.4.7.

…

On Thu, Dec 8, 2016 at 5:55 AM, Michiel Rook ***@***.***> wrote: Yeah we'd love to see 1.4.7 with the fix pushed, we're fighting this issue on a production cluster ATM. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#36840 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ASSNxVFU7dLQFj01m_TYFOtR7e59kr7qks5rGAw9gaJpZM4KzBhh> .

-- - Jing

Automatic merge from submit-queue fix issue in converting aws volume id from mount paths This PR is to fix the issue in converting aws volume id from mount paths. Currently there are three aws volume id formats supported. The following lists example of those three formats and their corresponding global mount paths: 1. aws:///vol-123456 (/var/lib/kubelet/plugins/kubernetes.io/aws-ebs/mounts/aws/vol-123456) 2. aws://us-east-1/vol-123456 (/var/lib/kubelet/plugins/kubernetes.io/mounts/aws/us-est-1/vol-123455) 3. vol-123456 (/var/lib/kubelet/plugins/kubernetes.io/mounts/aws/us-est-1/vol-123455) For the first two cases, we need to check the mount path and convert them back to the original format. This PR fixes kubernetes#36269

googlebot added the cla: yes label Nov 15, 2016

jingxu97 assigned justinsb and saad-ali Nov 15, 2016

jingxu97 added sig/storage Categorizes an issue or PR as relevant to SIG Storage. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. labels Nov 15, 2016

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

saad-ali suggested changes Nov 15, 2016

View reviewed changes

justinsb reviewed Nov 15, 2016

View reviewed changes

jingxu97 force-pushed the Nov/aws-volumeid branch from 9761157 to 5a64cd6 Compare November 15, 2016 22:53

justinsb reviewed Nov 15, 2016

View reviewed changes

jingxu97 force-pushed the Nov/aws-volumeid branch from 5a64cd6 to 6c66b6a Compare November 16, 2016 18:34

jingxu97 force-pushed the Nov/aws-volumeid branch from 6c66b6a to 5349593 Compare November 16, 2016 20:39

jingxu97 force-pushed the Nov/aws-volumeid branch from 5349593 to 90b92ce Compare November 16, 2016 21:19

justinsb reviewed Nov 17, 2016

View reviewed changes

jingxu97 force-pushed the Nov/aws-volumeid branch from 90b92ce to 3d3e44e Compare November 17, 2016 06:36

jingxu97 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 17, 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 Nov 17, 2016

jingxu97 added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Nov 18, 2016

jingxu97 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 18, 2016

k8s-github-robot merged commit 95ab806 into kubernetes:master Nov 18, 2016

jingxu97 mentioned this pull request Nov 22, 2016

Automated cherry pick of #36840 #37302

Closed

saad-ali mentioned this pull request Nov 23, 2016

Volume stuck in attaching state when using multiple PersistentVolumeClaim #36450

Closed

BaluDontu mentioned this pull request Nov 23, 2016

kubernetes attempts to unmount a wrong vSphere volume and stops making any progress after that #37332

Closed

childsb mentioned this pull request Nov 28, 2016

Add volume reconstruct/cleanup logic in kubelet volume manager openshift/origin#12024

Merged

jingxu97 mentioned this pull request Nov 30, 2016

EBS volumes cannot reattach to PetSet after unexpected detachment #37662

Closed

BaluDontu mentioned this pull request Dec 1, 2016

Automated cherry pick of #37413 #36840 upstream release 1.4 #37867

Merged

jessfraz added cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. cherrypick-candidate labels Dec 1, 2016

jessfraz modified the milestones: v1.4, v1.5 Dec 1, 2016

jessfraz added a commit that referenced this pull request Dec 2, 2016

Merge pull request #37867 from vmware/automated-cherry-pick-of-#37413-#…

bb80362

…36840-upstream-release-1.4 Automated cherry pick of #37413 #36840 upstream release 1.4

jessfraz removed the cherrypick-candidate label Dec 2, 2016

mikesplain mentioned this pull request Dec 3, 2016

Wrong AWS volume can be mounted #29324

Closed

chentao1596 mentioned this pull request Dec 5, 2016

WIP:kubelet: support multi-headers when getting pod from HTTP source #38089

Closed

This was referenced Dec 12, 2016

Pods using EBS-backed PVC sometimes get stuck. #38301

Closed

EBS volumes remains in 'attaching' state when attached with a previously used device name #31891

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix issue in converting aws volume id from mount paths #36840

fix issue in converting aws volume id from mount paths #36840

jingxu97 commented Nov 15, 2016 •

edited by jessfraz

Loading

saad-ali Nov 15, 2016

jingxu97 Nov 15, 2016

saad-ali Nov 15, 2016

justinsb Nov 15, 2016

jingxu97 commented Nov 15, 2016

k8s-ci-robot commented Nov 15, 2016

k8s-ci-robot commented Nov 15, 2016

k8s-ci-robot commented Nov 15, 2016

k8s-ci-robot commented Nov 15, 2016

k8s-ci-robot commented Nov 15, 2016

k8s-ci-robot commented Nov 15, 2016

justinsb Nov 15, 2016

justinsb Nov 15, 2016

jingxu97 Nov 16, 2016

justinsb commented Nov 15, 2016

jingxu97 commented Nov 16, 2016

k8s-ci-robot commented Nov 16, 2016

justinsb left a comment

justinsb Nov 17, 2016

jingxu97 Nov 17, 2016

dims commented Nov 18, 2016

k8s-github-robot commented Nov 18, 2016

k8s-github-robot commented Nov 18, 2016

justinsb commented Nov 22, 2016

jingxu97 commented Nov 22, 2016

Raffo commented Nov 23, 2016

jingxu97 commented Nov 24, 2016 via email

sharms commented Dec 7, 2016

mrook commented Dec 8, 2016

jingxu97 commented Dec 8, 2016 via email

fix issue in converting aws volume id from mount paths #36840

fix issue in converting aws volume id from mount paths #36840

Conversation

jingxu97 commented Nov 15, 2016 • edited by jessfraz 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

jingxu97 commented Nov 15, 2016

k8s-ci-robot commented Nov 15, 2016

k8s-ci-robot commented Nov 15, 2016

k8s-ci-robot commented Nov 15, 2016

k8s-ci-robot commented Nov 15, 2016

k8s-ci-robot commented Nov 15, 2016

k8s-ci-robot commented Nov 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb commented Nov 15, 2016

jingxu97 commented Nov 16, 2016

k8s-ci-robot commented Nov 16, 2016

justinsb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dims commented Nov 18, 2016

k8s-github-robot commented Nov 18, 2016

k8s-github-robot commented Nov 18, 2016

justinsb commented Nov 22, 2016

jingxu97 commented Nov 22, 2016

Raffo commented Nov 23, 2016

jingxu97 commented Nov 24, 2016 via email

sharms commented Dec 7, 2016

mrook commented Dec 8, 2016

jingxu97 commented Dec 8, 2016 via email

jingxu97 commented Nov 15, 2016 •

edited by jessfraz

Loading