Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix photon controller plugin to construct with correct PdID #37167

Merged
merged 1 commit into from
Dec 1, 2016
Merged

Fix photon controller plugin to construct with correct PdID #37167

merged 1 commit into from
Dec 1, 2016

Conversation

luomiao
Copy link

@luomiao luomiao commented Nov 19, 2016 •
edited
Loading

What this PR does / why we need it:
This PR is to fix a mismatching of unmount path in photon volume plugin, which is resulted from the assigning volume spec name to persistent disk ID. Without this path, unmounting process is stalling in reconciler when a pod is deleted. Restart the same pod will see a mount failure because the previous unmounting is still going on.

The input variable of function ConstructVolumeSpec is the volume spec name instead of persistent disk ID. Previously the function directly construct new volume spec by assigning volume spec name to persistent disk ID, which will result in mismatching of mount path. The fix will find the pdID according to mount path and construct volume spec with the correct pdID.

I have tested the patch with back-to-back pod creation/deletion and mounting/unmounting of photon persistent disk volume source performs normal now.

This need to be cherry-picked to 1.5 release branch.

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 19, 2016
@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Nov 19, 2016
pdhamdhere
pdhamdhere approved these changes Nov 19, 2016
View reviewed changes
Copy link

@pdhamdhere pdhamdhere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. This need to be cherry picked to 1.5.x.

Can you add a note to PR how you tested the fix?

@saad-ali saad-ali added this to the v1.5 milestone Nov 21, 2016
@saad-ali
Copy link
Member

Bug fix ok for post-code-freeze merge. We are planning on lifting code freeze soon. If this gets merged before code freeze is lifted it will automatically be picked up otherwise it'll need to be cherry picked to 1.5. Keep an eye on the kubernetes-dev group for communication about when code freeze is lifted.

@dims
Copy link
Member

dims commented Nov 23, 2016

@brendandburns @rootfs @luomiao : we really need to get reviews for this or push it off to 1.6

@rootfs
Copy link
Contributor

rootfs commented Nov 23, 2016

LGTM. @jingxu97 can you review?

@luomiao
Copy link
Author

luomiao commented Nov 24, 2016

@brendandburns @jingxu97 @rootfs
Can any one give a LGTM label if the PR looks good so we can have it in 1.5 on time?
Thanks! :)

@saad-ali saad-ali added lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Nov 24, 2016
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 24, 2016
Fix photon controller plugin to construct with correct PdID
c240042 
The input variable of function ConstructVolumeSpec is the volume spec
name instead of persistent disk ID. Previously the function directly
construct new volume spec by assigning volume spec name to persistent
disk ID, which will result in mismatching of mount path. The fix will
find the pdID according to mount path and construct volume spec with the
correct pdID.
@luomiao luomiao force-pushed the fix-photon-plugin-ConstructVolumeSpec branch from fb3c1e1 to c240042 Compare November 24, 2016 02:14
@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 24, 2016
@luomiao
Copy link
Author

luomiao commented Nov 24, 2016

@k8s-bot cvm gke e2e test this

@jingxu97 jingxu97 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 24, 2016
@rootfs rootfs mentioned this pull request Nov 28, 2016
Merged
@dims
Copy link
Member

dims commented Nov 28, 2016

@k8s-bot test this issue: #IGNORE

@luomiao
Copy link
Author

luomiao commented Nov 29, 2016

@k8s-bot unit test this

@luomiao
Copy link
Author

luomiao commented Nov 29, 2016

@k8s-bot kops aws e2e test this

@luomiao
Copy link
Author

luomiao commented Nov 29, 2016

@k8s-bot gci gce e2e test this

@luomiao
Copy link
Author

luomiao commented Nov 30, 2016

@dims @saad-ali
Hi all, the tests seems are already succeeded but somehow they are not reported back here.

@saad-ali
Copy link
Member

@k8s-bot test this

@rmmh
Copy link
Contributor

rmmh commented Dec 1, 2016

@k8s-bot test this issue kubernetes/test-infra#1253 (also, deleting spurious failure comments)

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 4c0781e into kubernetes:master Dec 1, 2016
@luomiao luomiao mentioned this pull request Dec 1, 2016
Merged
@saad-ali saad-ali added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Dec 1, 2016
saad-ali added a commit that referenced this pull request Dec 1, 2016
@saad-ali
Merge pull request #37779 from luomiao/automated-cherry-pick-of-#3716…
5c6a932 
…7-upstream-release-1.5

Automated cherry pick of #37167
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pdhamdhere pdhamdhere pdhamdhere approved these changes

Assignees

@brendandburns brendandburns

@rootfs rootfs

Labels
cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.5
Development

Successfully merging this pull request may close these issues.
10 participants
@luomiao @saad-ali @dims @rootfs @rmmh @k8s-github-robot @pdhamdhere @brendandburns @jingxu97 @k8s-ci-robot