-
Notifications
You must be signed in to change notification settings - Fork 1.6k
KEP-2033: KubeletInUserNamespace: postpone beta to v1.36 #5694
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
base: master
Are you sure you want to change the base?
Conversation
a83c03a to
7e71dd9
Compare
SergeyKanzhelev
left a comment
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
/approve
since it just missed a deadline a little, I suggest we go ahead with this KEP in 1.36
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: AkihiroSuda, SergeyKanzhelev 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 |
|
hm, actually, let's take this chance to review API. /lgtm cancel Please update the KEP to change the name of a field in NodeStatus the same way it changed in code PR. Let's take this chance to review API ahead of coding stage. As per kubernetes/kubernetes#134639 (comment) adding @thockin here to help review the API. /assign @thockin |
7e71dd9 to
9db35c9
Compare
Done |
| --> | ||
|
|
||
| [`NodeSystemInfo`](https://pkg.go.dev/k8s.io/api/core/v1#NodeSystemInfo) will have `RunningInUserNS *bool` | ||
| [`NodeSystemInfo`](https://pkg.go.dev/k8s.io/api/core/v1#NodeSystemInfo) will have `RunningInUserNamespace *bool` |
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.
Right now the KEP suggested this node status field to address the PRR questions (mostly) regarding transparency for end user.
I like the idea of a strongly typed field. However we may need to think and address the following:
- Will users want to taint or select based on this information? If so, why not express it as a taint or label.
- Is this field mutable or node must be re-created in a different state?
- is boolean value enough long term? Do we envision more modes of operation to expose?
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.
- Will users want to taint or select based on this information? If so, why not express it as a taint or label.
Yes, label seems to make a sense here.
e.g.,
kubernetes.io/running-in-user-namespace: "true"
- Is this field mutable or node must be re-created in a different state?
Must be re-created.
- is boolean value enough long term? Do we envision more modes of operation to expose?
Additional labels can be added on demand
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.
Updated the PR to use a label.
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.
@SergeyKanzhelev For consistency with os and arch labels, NodeSystemInfo.RunningInUserNamespace should be kept too, or just a label suffices?
9db35c9 to
b00a39b
Compare
kubernetes/kubernetes PR 134639 was approved and got the `lgtm` label, however, it was not merged in v1.35. Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
b00a39b to
fdf1682
Compare
KEP-2033: promote KubeletInUserNamespace feature to beta kubernetes#134639 was approved and got the
lgtmlabel, however, it was not merged in v1.35.