-
Notifications
You must be signed in to change notification settings - Fork 39.8k
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
kubelet: remove the --bootstrap-checkpoint-path feature #91577
Conversation
Hi @knabben. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
/assign @timothysc We are writing the possible alternatives in the deprecation warning, lmk if you have something to enhace the comment. |
We should remove the associated code as well that is plumbed through. /cc @yastij |
The process for deprecating these flags is to wait 2-4 releases before the final cleanup, first we add the deprecation warning. So, this is mostly planned for 1.23, does it works for you? |
cmd/kubelet/app/options/options.go
Outdated
@@ -394,6 +393,8 @@ func (f *KubeletFlags) AddFlags(mainfs *pflag.FlagSet) { | |||
fs.MarkDeprecated("chaos-chance", "will be removed in a future version.") | |||
fs.StringVar(&f.SeccompProfileRoot, "seccomp-profile-root", f.SeccompProfileRoot, "<Warning: Alpha feature> Directory path for seccomp profiles.") | |||
fs.MarkDeprecated("seccomp-profile-root", "will be removed in 1.23, in favor of using the `<root-dir>/seccomp` directory") | |||
fs.StringVar(&f.BootstrapCheckpointPath, "bootstrap-checkpoint-path", f.BootstrapCheckpointPath, "<Warning: Alpha feature> Path to the directory where the checkpoints are stored") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is marked as an alpha feature, as per our deprecation policy https://kubernetes.io/docs/reference/using-api/deprecation-policy/#deprecating-a-flag-or-cli this can be removed immediately as pointed by @timothysc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree... let's not carry deprecated alpha-level functionality
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing in the docs, I will remove the code/flag in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rolling back:
- Initial basic bootstrap-checkpoint support #50984, the PR introduced the flag and the initial implementation of pod checkpoint.
- parts of Node-level Checkpointing manager: Migrate dockershim and device plugin manager checkpointing #56040 since the original checkpoint ended using the Checkpoint Manager.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll defer to sig-node reviewers on the partial rollback
cc @liggitt - as we discussed this a while ago |
dd18470
to
abb8867
Compare
/remove-sig apps |
abb8867
to
79e4c7f
Compare
/lgtm |
/test pull-kubernetes-e2e-gce-storage-slow |
ping @kubernetes/sig-node-api-reviews |
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
/remove-label api-review |
@yastij: Those labels are not set on the issue: In response to this:
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. |
@yastij: The label(s) In response to this:
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. |
lgtm, will defer to node approvers |
/retitle kubelet: remove the --bootstrap-checkpoint-path feature |
/assign @derekwaynecarr |
reviewing now. |
/approve ironically, we are likely to require some alternate form of checkpoint for pod resource consumption to facilitate vertical autoscaling, but we can safely remove this particular use of checkpointing. thanks for the pr. |
@liggitt can you approve api change? |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: derekwaynecarr, knabben, liggitt 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 |
What type of PR is this?
/kind cleanup
/area kubelet
/area kubelet-api
What this PR does / why we need it:
Removing --bootstrap-checkpoint-path flag and related code.
Which issue(s) this PR fixes:
Fixes #92763
Refs #86843 #72202 (comment)
Special notes for your reviewer:
Does this PR introduce a user-facing change?: