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.

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

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

Merged
merged 1 commit into from Mar 22, 2018

Conversation

@msau42
Member

msau42 commented Mar 19, 2018

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

This comment has been minimized.

@k8s-ci-robot k8s-ci-robot requested review from gnufied, liggitt and saad-ali Mar 19, 2018

@k8s-ci-robot k8s-ci-robot added size/L and removed size/XS labels Mar 20, 2018

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

@msau42

This comment has been minimized.

Member

msau42 commented Mar 21, 2018

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.

@@ -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" {

This comment has been minimized.

@msau42

msau42 Mar 21, 2018

Member

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?).

This comment has been minimized.

@gnufied

gnufied Mar 21, 2018

Member

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?

This comment has been minimized.

@msau42

msau42 Mar 21, 2018

Member

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

This comment has been minimized.

@msau42

msau42 Mar 21, 2018

Member

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

@msau42

This comment has been minimized.

Member

msau42 commented Mar 21, 2018

/test pull-kubernetes-integration

1 similar comment
@msau42

This comment has been minimized.

Member

msau42 commented Mar 21, 2018

/test pull-kubernetes-integration

@gnufied

This comment has been minimized.

Member

gnufied commented Mar 21, 2018

/lgtm

@msau42

This comment has been minimized.

Member

msau42 commented Mar 21, 2018

/assign @tallclair for kubelet approval

@tallclair

This comment has been minimized.

Member

tallclair commented Mar 21, 2018

/lgtm

@@ -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.

This comment has been minimized.

@jingxu97

jingxu97 Mar 22, 2018

Contributor

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

This comment has been minimized.

@msau42

@k8s-ci-robot k8s-ci-robot removed the lgtm label Mar 22, 2018

@msau42

This comment has been minimized.

Member

msau42 commented Mar 22, 2018

/test pull-kubernetes-integration

@saad-ali

This comment has been minimized.

Member

saad-ali commented Mar 22, 2018

/approve

@saad-ali

This comment has been minimized.

Member

saad-ali commented Mar 22, 2018

/lgtm

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Mar 22, 2018

[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:

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Mar 22, 2018

[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-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Mar 22, 2018

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.

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

14 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation msau42 authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

k8s-merge-robot added a commit that referenced this pull request Mar 23, 2018

Merge pull request #61518 from msau42/automated-cherry-pick-of-#61373-…
…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.
```

k8s-merge-robot added a commit that referenced this pull request Mar 24, 2018

Merge pull request #61519 from msau42/automated-cherry-pick-of-#61373-…
…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.
```

k8s-merge-robot added a commit that referenced this pull request Mar 26, 2018

Merge pull request #61520 from msau42/automated-cherry-pick-of-#61373-…
…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.
```

k8s-merge-robot added a commit that referenced this pull request Mar 27, 2018

Merge pull request #61521 from msau42/automated-cherry-pick-of-#61373-…
…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.
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment