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

Windows: Fixes subpath volume persistance on container restart #92869

Closed
wants to merge 1 commit into from
Closed

Windows: Fixes subpath volume persistance on container restart #92869

wants to merge 1 commit into from

Conversation

claudiubelu
Copy link
Contributor

@claudiubelu claudiubelu commented Jul 7, 2020 •
edited

What type of PR is this?

/kind bug

/sig windows
/sig storage

/priority important-soon

What this PR does / why we need it:

Pods can have volume subpaths mounted inside the containers. Those subpaths can be based on expressions based on expanded environment variables, which can also be based on Pod annotations.

Kubernetes expects that the mounted volume subpaths to remain consistent across container restarts even if the Pod annotation it was based on changed. This behaviour is checked by the E2E test should not change the subpath mount on a container restart if the environment variable change.

This test fails on Windows because the subpath itself is always mounted (which is based on the Pod Annotation), while on Linux, on the first first, the volume subpath is created and mounted under kubelet/pods/<uid>/volume-subpaths/<volume>/<container-name> and cached, then on restart, it is going to use the same mount path that already exists, keeping the same subpath mounted in the container.

This commit addresses the issue on Windows, following a similar approach to the Linux implementation.

Refactors some of the code, which can be used on both Linux and Windows.

Which issue(s) this PR fixes:

Fixes #90336

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Fixed issue on changing volume subpaths based on expanded environment variables on Windows hosts.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jul 7, 2020
@claudiubelu claudiubelu changed the title Windows/subpath fix WIP: Windows/subpath fix Jul 7, 2020
@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. area/test sig/network Categorizes an issue or PR as relevant to SIG Network. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 7, 2020
@claudiubelu
Windows: Fixes subpath volume persistance on container restart
5836efb 
Pods can have volume subpaths mounted inside the containers. Those subpaths can be
based on expressions based on expanded environment variables, which can also be based
on Pod annotations.

Kubernetes expects that the mounted volume subpaths to remain consistent across container
restarts even if the Pod annotation it was based on changed. This behaviour is checked by the
E2E test "should not change the subpath mount on a container restart if the environment variable change"

This test fails on Windows because the subpath itself is always mounted (which is based on the Pod
Annotation), while on Linux, on the first first, the volume subpath is created and mounted under
kubelet/pods/<uid>/volume-subpaths/<volume>/<container-name> and cached, then on restart, it is going
to use the same mount path that already exists, keeping the same subpath mounted in the container.

This commit addresses the issue on Windows, following a similar approach to the Linux implementation.

Refactors some of the code, which can be used on both Linux and Windows.
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 8, 2020
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jul 8, 2020
@claudiubelu claudiubelu changed the title WIP: Windows/subpath fix Windows/subpath fix Jul 8, 2020
@k8s-ci-robot 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 Jul 8, 2020
@claudiubelu claudiubelu changed the title Windows/subpath fix Windows: Fixes subpath volume persistance on container restart Jul 8, 2020
@adelina-t
Copy link
Contributor

/cc @jsturtevant
/cc @michmike
/cc @ddebroy

/sig-windows

@k8s-ci-robot
Copy link
Contributor

@adelina-t: GitHub didn't allow me to request PR reviews from the following users: jsturtevant.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @jsturtevant
/cc @michmike
/cc @ddebroy

/sig-windows

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot requested a review from ddebroy July 8, 2020 14:28
@claudiubelu claudiubelu force-pushed the windows/subpath-fix branch 4 times, most recently from 684947e to 5836efb Compare July 9, 2020 15:57
@k8s-ci-robot
Copy link
Contributor

@claudiubelu: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 9, 2020
@k8s-ci-robot
Copy link
Contributor

@claudiubelu: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kubernetes-integration 5836efb link /test pull-kubernetes-integration
pull-kubernetes-e2e-gce-storage-slow 5836efb link /test pull-kubernetes-e2e-gce-storage-slow
pull-kubernetes-kubemark-e2e-gce-big 5836efb link /test pull-kubernetes-kubemark-e2e-gce-big
pull-kubernetes-e2e-azure-file-windows 5836efb link /test pull-kubernetes-e2e-azure-file-windows
pull-kubernetes-typecheck 5836efb link /test pull-kubernetes-typecheck

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

michmike
michmike approved these changes Jul 15, 2020
View reviewed changes
Copy link
Contributor

@michmike michmike left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve
@ddebroy can you PTAL?

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 15, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: claudiubelu, michmike
To complete the pull request process, please assign jsafrane
You can assign the PR to them by writing /assign @jsafrane in a comment when ready.

The full list of commands accepted by this bot can be found 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

@michmike
Copy link
Contributor

/assign @jingxu97

@jsturtevant
Copy link
Contributor

I think this PR is no longer relevant due to #89629 which changed the behavior of mounting subpath's on restarts and removed this requirement from the conformance tests. @claudiubelu can you confirm?

@claudiubelu
Copy link
Contributor Author

That is correct. The behaviour that this PR was adding is no longer valid in Kubernetes. The test that was verifying this behaviour was also removed in the PR mentioned above. Because of this, we no longer need this, we are already doing what is currently expected regarding volume subpaths on container restart.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michmike michmike michmike approved these changes

@aojea aojea Awaiting requested review from aojea

@saad-ali saad-ali Awaiting requested review from saad-ali

@ddebroy ddebroy Awaiting requested review from ddebroy

Assignees

@michmike michmike

@jingxu97 jingxu97

Labels
area/test 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. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Windows: subpath mount changes on a container restart if the environment variable changes
6 participants
@claudiubelu @adelina-t @k8s-ci-robot @michmike @jsturtevant @jingxu97