-
Notifications
You must be signed in to change notification settings - Fork 38.6k
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]: fixed container restart due to pod spec field changes #124220
Conversation
Please note that we're already in Test Freeze for the Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Sun Apr 7 13:42:56 UTC 2024. |
@liggitt Could you please tell me if I'm on the right track? |
b87dde2
to
cee9837
Compare
/cc @tallclair @ndixita |
/triage accepted |
// Note: this list must be updated if ever kubelet wants to allow mutations to other fields. | ||
func pickFieldToHash(container *v1.Container) map[string]string { | ||
retval := map[string]string{ | ||
"name": container.Name, |
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.
why not include fields like ID, or ImageID? These should also be immutable. Are you finding the smallest set of fields that don't require a restart?
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 picking the smallest set of mutable fields in the REST API v1.Container that do require a restart.
ID and ImageID are not in the v1.Container type
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.
why not include fields like ID, or ImageID?
We already had some discussion above about not include the pod ID.
ref: #124220 (comment) #124220 (comment)
Are you finding the smallest set of fields that don't require a restart?
Yes, we would like your help in reviewing and verifying whether other fields in v1.Container
need to be covered.
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.
gotcha, thanks!
/lgtm |
LGTM label has been added. Git tree hash: 6be440dc41e15951e9dd8427c3a5f8ff908ab020
|
Thanks for the review @haircommander /lgtm |
/cc @AkihiroSuda @mrunalp @dchen1107 @SergeyKanzhelev for approval And do we need to discuss adding gates to control risk proposed by @thockin? |
@HirazawaUi: GitHub didn't allow me to request PR reviews from the following users: for, approve. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. 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-sigs/prow repository. |
I am making a pass for node approval. |
6453b9c
to
4badc54
Compare
4badc54
to
3ec13c5
Compare
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.
/lgtm
LGTM label has been added. Git tree hash: 88b44d0862a0c27bc7550a10c1732a65ee428895
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: HirazawaUi, mrunalp, vinaykul 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 feature
What this PR does / why we need it:
The kubelet should hash on fields that it consider as mutable for the life of the container.
This can prevent pod restarts caused by modification of the pod spec default value in the following two situations:
According to kubernetes/website#12326 (comment), we know that pods will be drained when upgrading kubelet, so we can change this mechanism on a minor version boundary.
Which issue(s) this PR fixes:
Fixes #122028 #63814
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: