Use inner volume name instead of outer volume name for subpath directory #61373

msau42 · 2018-03-19T23:25:39Z

What this PR does / why we need it:
Fixes volume reconstruction for PVCs with subpath

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #61372

Special notes for your reviewer:

Release note:

ACTION REQUIRED: In-place node upgrades to this release from versions 1.7.14, 1.8.9, and 1.9.4 are not supported if using subpath volumes with PVCs.  Such pods should be drained from the node first.

msau42 · 2018-03-19T23:26:33Z

/assign @jsafrane @jingxu97
/cc @liggitt @gnufied @saad-ali
@kubernetes/sig-storage-pr-reviews

msau42 · 2018-03-21T01:08:58Z

This is ready to review.

I ran the new e2e tests against an old cluster to verify that they failed, and that the test cluster with the fix passes. Currently running all subpath tests to test for regression.

msau42 · 2018-03-21T01:12:53Z

test/e2e/storage/subpath.go

@@ -307,6 +308,20 @@ var _ = utils.SIGDescribe("Subpath", func() {

 				testPodContainerRestart(f, pod, filePathInVolume, filePathInSubpath)
 			})
+
+			It("should unmount if pod is gracefully deleted while kubelet is down [Disruptive][Slow]", func() {
+				if curVolType == "hostPath" || curVolType == "hostPathSymlink" {


There is one side issue here that hostpath volume plugin doesn't support reconstruction. We probably need to figure out how to implement support for it otherwise the subpath bind mounts are not going to get cleaned up for hostpath volumes in the reconstruction window (although I still question why do you need to use subpath with hostpath volumes?).

regardless of reconstruction after newest bit of refactoring - Teardown should be called for all volume types that are not in use, including hostpaths. Shouldn't that cleanup subpaths too?

Yes hostpath subpath is cleaned up in normal cases. But in the reconstruction case, it is not. And this PR won't fix that

I think graceful pod delete should still work for hostpath. Only forceful delete will fail, so this check can be removed.

gnufied · 2018-03-21T20:34:48Z

/lgtm

msau42 · 2018-03-21T21:17:58Z

/assign @tallclair for kubelet approval

tallclair · 2018-03-21T22:58:09Z

/lgtm

jingxu97 · 2018-03-22T00:05:16Z

pkg/kubelet/container/runtime.go

@@ -470,6 +470,9 @@ type VolumeInfo struct {
 	// Whether the volume permission is set to read-only or not
 	// This value is passed from volume.spec
 	ReadOnly bool
+	// Inner volume name, which is the PV name if used, otherwise
+	// it is the same as the outer volume name.


Maybe change to "InnerVolumeSpecName" for consistency.
also change outer volume name to "OuterVolumeSpecName" in the comment?

msau42 · 2018-03-22T00:46:03Z

/test pull-kubernetes-integration

saad-ali · 2018-03-22T01:29:21Z

/approve

saad-ali · 2018-03-22T01:31:33Z

/lgtm

k8s-ci-robot · 2018-03-22T01:32:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gnufied, msau42, saad-ali, tallclair

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/kubelet/OWNERS~~ [tallclair]
~~pkg/util/mount/OWNERS~~ [saad-ali]
~~pkg/volume/util/operationexecutor/OWNERS~~ [gnufied,saad-ali]
~~test/e2e/storage/OWNERS~~ [gnufied,msau42,saad-ali]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-github-robot · 2018-03-22T01:34:20Z

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@gnufied @jingxu97 @jsafrane @msau42 @saad-ali @tallclair

Pull Request Labels

sig/storage: Pull Request will be escalated to these SIGs if needed.
priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
kind/bug: Fixes a bug discovered during the current release.

Help

k8s-github-robot · 2018-03-22T13:20:25Z

Automatic merge from submit-queue (batch tested with PRs 61453, 61393, 61379, 61373, 61494). If you want to cherry-pick this change to another branch, please follow the instructions here.

…-upstream-release-1.10 Automatic merge from submit-queue. [1.10] Automated cherry pick of #61373: Use inner volume name instead of outer volume name for subpath directory Cherry pick of #61373 on release-1.10. #61373: Use inner volume name instead of outer volume name for subpath directory **Release note**: ```release-note ACTION REQUIRED: In-place node upgrades to this release from versions 1.7.14, 1.8.9, and 1.9.4 are not supported if using subpath volumes with PVCs. Such pods should be drained from the node first. ```

…-upstream-release-1.9 Automatic merge from submit-queue. [1.9] Automated cherry pick of #61373: Use inner volume name instead of outer volume name for subpath directory Cherry pick of #61373 on release-1.9. #61373: Use inner volume name instead of outer volume name for subpath directory **Release note**: ```release-note ACTION REQUIRED: In-place node upgrades to this release from versions 1.7.14, 1.8.9, and 1.9.4 are not supported if using subpath volumes with PVCs. Such pods should be drained from the node first. ```

…-upstream-release-1.8 Automatic merge from submit-queue. [1.8] Automated cherry pick of #61373: Use inner volume name instead of outer volume name for subpath directory Cherry pick of #61373 on release-1.8. #61373: Use inner volume name instead of outer volume name for subpath directory **Release note**: ```release-note ACTION REQUIRED: In-place node upgrades to this release from versions 1.7.14 and 1.8.9 are not supported if using subpath volumes with PVCs. Such pods should be drained from the node first. ```

…-upstream-release-1.7 Automatic merge from submit-queue. [1.7] Automated cherry pick of #61373: Use inner volume name instead of outer volume name for subpath directory Cherry pick of #61373 on release-1.7. #61373: Use inner volume name instead of outer volume name for subpath directory **Release note**: ```release-note ACTION REQUIRED: In-place node upgrades to this release from versions 1.7.14 are not supported if using subpath volumes with PVCs. Such pods should be drained from the node first. ```

k8s-ci-robot requested review from davidz627, jingxu97 and yujuhong March 19, 2018 23:25

k8s-ci-robot assigned jingxu97 and jsafrane Mar 19, 2018

k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Mar 19, 2018

k8s-ci-robot requested review from gnufied, liggitt and saad-ali March 19, 2018 23:26

msau42 force-pushed the subpath-reconstruct branch 2 times, most recently from b3f5261 to 779b650 Compare March 20, 2018 18:07

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 20, 2018

msau42 force-pushed the subpath-reconstruct branch from 779b650 to a12828f Compare March 21, 2018 01:07

msau42 changed the title ~~WIP: Use inner volume name instead of outer volume name for subpath directory~~ Use inner volume name instead of outer volume name for subpath directory Mar 21, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 21, 2018

msau42 commented Mar 21, 2018

View reviewed changes

msau42 mentioned this pull request Mar 21, 2018

hostPath volumes used with subPath volume mounts don't support reconstruction #61446

Open

saad-ali added this to the v1.10 milestone Mar 21, 2018

saad-ali added kind/bug Categorizes issue or PR as related to a bug. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. status/approved-for-milestone labels Mar 21, 2018

jingxu97 mentioned this pull request Mar 21, 2018

pvc volumes used with subPath volume mounts don't support reconstruction #61372

Closed

5 tasks

k8s-ci-robot assigned gnufied Mar 21, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 21, 2018

k8s-ci-robot assigned tallclair Mar 21, 2018

jingxu97 reviewed Mar 22, 2018

View reviewed changes

Use inner volume name instead of outer volume name for subpath directory

91c557f

msau42 force-pushed the subpath-reconstruct branch from 8e2e7af to 91c557f Compare March 22, 2018 00:13

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 22, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 22, 2018

k8s-ci-robot assigned saad-ali Mar 22, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 22, 2018

k8s-github-robot merged commit d23066e into kubernetes:master Mar 22, 2018

jdumars added the cherrypick-candidate label Mar 22, 2018

msau42 deleted the subpath-reconstruct branch November 17, 2018 21:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use inner volume name instead of outer volume name for subpath directory #61373

Use inner volume name instead of outer volume name for subpath directory #61373

msau42 commented Mar 19, 2018 •

edited

Loading

msau42 commented Mar 19, 2018

msau42 commented Mar 21, 2018

msau42 Mar 21, 2018 •

edited

Loading

gnufied Mar 21, 2018

msau42 Mar 21, 2018

msau42 Mar 21, 2018

gnufied commented Mar 21, 2018

msau42 commented Mar 21, 2018

tallclair commented Mar 21, 2018

jingxu97 Mar 22, 2018 •

edited

Loading

msau42 Mar 22, 2018

msau42 commented Mar 22, 2018

saad-ali commented Mar 22, 2018

saad-ali commented Mar 22, 2018

k8s-ci-robot commented Mar 22, 2018

k8s-github-robot commented Mar 22, 2018

k8s-github-robot commented Mar 22, 2018

Use inner volume name instead of outer volume name for subpath directory #61373

Use inner volume name instead of outer volume name for subpath directory #61373

Conversation

msau42 commented Mar 19, 2018 • edited Loading

msau42 commented Mar 19, 2018

msau42 commented Mar 21, 2018

msau42 Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

gnufied Mar 21, 2018

Choose a reason for hiding this comment

msau42 Mar 21, 2018

Choose a reason for hiding this comment

msau42 Mar 21, 2018

Choose a reason for hiding this comment

gnufied commented Mar 21, 2018

msau42 commented Mar 21, 2018

tallclair commented Mar 21, 2018

jingxu97 Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

msau42 Mar 22, 2018

Choose a reason for hiding this comment

msau42 commented Mar 22, 2018

saad-ali commented Mar 22, 2018

saad-ali commented Mar 22, 2018

k8s-ci-robot commented Mar 22, 2018

k8s-github-robot commented Mar 22, 2018

k8s-github-robot commented Mar 22, 2018

msau42 commented Mar 19, 2018 •

edited

Loading

msau42 Mar 21, 2018 •

edited

Loading

jingxu97 Mar 22, 2018 •

edited

Loading