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

Remove mount propagation feature gate #68230

Merged
merged 1 commit into from
Nov 2, 2018
Merged

Remove mount propagation feature gate #68230

merged 1 commit into from
Nov 2, 2018

Conversation

bertinatto
Copy link
Member

@bertinatto bertinatto commented Sep 4, 2018 •
edited by liggitt
Loading

What this PR does / why we need it:

This PR removes the logic around the mount propagation feature gate deprecated in v1.12. It's a follow-up to #67255.

Release note:

The `MountPropagation` feature is unconditionally enabled in v1.13, and can no longer be disabled.

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 4, 2018
@k8s-ci-robot k8s-ci-robot added area/kubelet sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/node Categorizes an issue or PR as relevant to SIG Node. labels Sep 4, 2018
@bertinatto
Copy link
Member Author

bertinatto commented Sep 4, 2018 •
edited
Loading

/sig storage
/assign jsafrane

@k8s-ci-robot k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Sep 4, 2018
@bertinatto
Copy link
Member Author

/hold

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 4, 2018
@bertinatto bertinatto force-pushed the remove_mount_propagation_gate branch from 878abda to 5545cf4 Compare October 2, 2018 08:20
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 2, 2018
@bertinatto
Copy link
Member Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 2, 2018
@bertinatto
Copy link
Member Author

/assign @liggitt

@liggitt
Copy link
Member

liggitt commented Oct 3, 2018

/ok-to-test
/lgtm

@k8s-ci-robot k8s-ci-robot 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 needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. release-note-none Denotes a PR that doesn't merit a release note. labels Oct 3, 2018
@bertinatto bertinatto force-pushed the remove_mount_propagation_gate branch from 5545cf4 to 4d03809 Compare October 3, 2018 07:31
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 3, 2018
@bertinatto
Copy link
Member Author

/retest

1 similar comment
@bertinatto
Copy link
Member Author

/retest

@bertinatto
Copy link
Member Author

/test pull-kubernetes-local-e2e-containerized

@bertinatto bertinatto force-pushed the remove_mount_propagation_gate branch from 4d03809 to 34bb086 Compare October 4, 2018 13:48
@bertinatto
Copy link
Member Author

/retest

@bertinatto
Copy link
Member Author

There's a failing test (pull-kubernetes-local-e2e-containerized) preventing this PR from being merged.

#69465 needs to be fixed first.

@bertinatto
Copy link
Member Author

/retest

@bertinatto
Copy link
Member Author

@liggitt, could you take another look, please? I had forgotten to include the changes in pkg/kubelet/kubelet_pods_linux_test.go, so I lost the lgtm label.

Also,

/assign @tallclair

for changes in Kubelet.

@tallclair
Copy link
Member

See kubernetes/website#10294

Looks like the current policy is that the feature gate must continue to function for at least 2 releases after GA, but can be a no-op (i.e. you can remove the logic, but need to keep the gate until 1.14).

@bertinatto
Copy link
Member Author

@tallclair, I updated the PR to keep the gate and remove the logic. Please let me know if I'm missing something.

@tallclair
Copy link
Member

/lgtm
If you're up for it, can you also update the warning here and the comment here to specify which release the feature gate will be removed in (I think 1.14 according to the latest iteration of the policy).

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 29, 2018
@tallclair
Copy link
Member

Please update the release note too.

@liggitt
Copy link
Member

liggitt commented Oct 30, 2018

I think that warning should be escalated to an error if they attempt to disable the feature, now that disabling it has no effect. Agree on updating comments to indicate removal in 1.14.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 30, 2018
@bertinatto
Copy link
Member Author

@tallclair, @liggitt: thanks, updated PR.

liggitt
liggitt reviewed Oct 31, 2018
View reviewed changes
pkg/kubelet/kubelet.go Outdated Show resolved Hide resolved
@liggitt
Copy link
Member

liggitt commented Oct 31, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 31, 2018
@liggitt
Copy link
Member

liggitt commented Oct 31, 2018

/approve

@tallclair tallclair mentioned this pull request Oct 31, 2018
Merged
@tallclair
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bertinatto, liggitt, 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-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 1, 2018
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

1 similar comment
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 08351b6 into kubernetes:master Nov 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@tallclair tallclair tallclair left review comments

@david-mcmahon david-mcmahon Awaiting requested review from david-mcmahon

@lavalamp lavalamp Awaiting requested review from lavalamp

Assignees

@liggitt liggitt

@jsafrane jsafrane

@tallclair tallclair

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet 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. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@bertinatto @liggitt @tallclair @k8s-ci-robot @fejta-bot @jsafrane