-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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
User namespaces doc changes for 1.30 #45178
Conversation
👷 Deploy Preview for kubernetes-io-vnext-staging processing.
|
/retitle [WIP] User namespaces doc changes for 1.30 |
/sig node |
Hello @rata 👋 please take a look at Documenting for a release - PR Ready for Review to get your PR ready for review before Tuesday March 12th 2024 18:00 PST. Thank you! |
a8ab40e
to
f3656a9
Compare
PTAL, this is ready for review |
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.
As this is graduating to beta, please align with the style guide a bit more (see inline feedback).
In addition, support is needed in the container runtime and in the OCI container | ||
runtime. |
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.
Readers are likely to not know what you mean here. Can you clarify?
We might say “CRI-level runtime” and “OCI container runtime”, but then I think we'd do well to add an explanation about the two (maybe to our glossary?)
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.
Nah, that seems to complex and we are already using all over the place (not just here) the term "container runtime", I don't think we should change it.
I've adjusted to refer to the "OCI runtime" and added a clarification here too.
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.
We've found a place where we need to explain two kinds of container runtimes. You and I know the difference, but plenty of readers will not.
If we don't explain it here, we should expect to be explaining it in GitHub issues (and I'd like to avoid that happening).
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.
@sftim What? I've changed it and explained, have you read what I changed? I don't think using "CRI runtime" is worth it, for the things I explained. It is just that.
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.
Reviewing on behalf of SIG Docs, I'm disagreeing - I think we should help readers more. Can you convince me / us that we won't have confused readers here?
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.
@sftim sure. What do you think is not clear or could cause confusion? It would be great if you share that, to explain.
Let me explain in general, though, what has been already done:
First, I don't think we should use "CRI container runtime". We already use the term "container runtime" and we have been using it for several years, I don't see why we would change that. Furthermore, it is all over the place, it is not easy to change.
Secondly, the OCI runtime is only relevant for documentation in the userns context and maybe even for a few months only. So using a very specific term, instead of changing "container runtime" to "CRI container runtime" seems better.
Third, you asked to clarify and I chose a better name and clarified here what it is already.
What do you think is not clear and could cause confusion?
The kubelet can use a custom range for pods too. To configure a custom range, | ||
the node needs to have: | ||
|
||
* A user `kubelet` in the system |
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.
* A user `kubelet` in the system | |
* A local POSIX user named `kubelet` (you cannot use any other username here) |
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 not true, it is not a "local" user. Why would you think that?
Added the part about you cannot use another username here.
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'd expect to find kubelet
listed as a local user using getent
. The first column in /etc/subuid is a login name that you can find using NSS on a typical Linux system; that's different from a username that you'd find in your cluster's audit logs.
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.
What do you mean with "a local user using getent
"? And why is it relevant?
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 mean that you could run getent passwd | grep kubelet
and I would expect to learn the UID for kubelet
by doing that. Is that the case?
This is because /etc/subuid and /etc/subgid map subordinate ranges to users, and if there isn't a user, what are we mapping it to?
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.
Yes, that is the case. However, those are not local users. That command includes local users and, for example, LDAP users.
content/en/docs/tasks/configure-pod-container/user-namespaces.md
Outdated
Show resolved
Hide resolved
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.
@sftim fixed and answered to all your comments. But it's quite exhausting to answer to your reviews. Let me explain why.
It is exhausting because of the following (not all of them happened in this PR, but all of them happened already, I think several times):
- You suggest changes to things you suggested to write in the first place. We can all change our mind, that is okay, but when the content is the same and you keep doing it, it is really not helpful nor easy to address
- You make suggestions that applying them will do the opposite of another comment you did, so it is really not helpful the suggestion in the first place. It would be better if you suggest things that would just address your concerns.
- If you don't know something, ask. I can't stress this enough. Don't assume it works in a very specific way (when there are no pointers to even suggest that AFAIK), make tens of suggestion to change things in that way and most of the times you don't even answer when people ask why you thought it was that way. Answering that would be very helpful to understand if something in the doc might suggest that.
- If you don't know something, instead of making assumptions and tens of comments/suggestions, do it once and ask. This will make it way more simpler to act on it. If I missed to update something, you can mention it later.
- Explain why you suggest a change. As your suggestions are loaded with assumptions you for things didn't ask, it is very hard to understand why you suggested a change: was it because you assumed something and wanted to clarify your assumption? was it to clarify something else and you added your assumption while changing the text?
The kubelet can use a custom range for pods too. To configure a custom range, | ||
the node needs to have: | ||
|
||
* A user `kubelet` in the system |
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 not true, it is not a "local" user. Why would you think that?
Added the part about you cannot use another username here.
In addition, support is needed in the container runtime and in the OCI container | ||
runtime. |
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.
Nah, that seems to complex and we are already using all over the place (not just here) the term "container runtime", I don't think we should change it.
I've adjusted to refer to the "OCI runtime" and added a clarification here too.
Thanks for the feedback. Here's what's important for the documentation side: we want beta-quality (or better) documentation. This means taking docs that met the criteria for an alpha release, and improving them. In places, this might mean revising text I suggested a while ago - and I get that this can be frustrating. We're short on reviewer capacity so it might well be me that provides further reviews. I'll try to provide more context about the suggestions and to explain my thinking, inline. |
I don't know of any place where Kubernetes hard codes So far as I know, runc is very popular but Kubernetes is agnostic about which OCI runtime you use. |
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.
Hello @rata 👋, it looks like you've already engaged a lot of feedback. Great job! Keep this up. 🚀
@giuseppe, @saschagrunert, @mrunalp, @SergeyKanzhelev, @thockin, @derekwaynecarr When one of y'all have a moment, please give a review for technical accuracy. A friendly reminder that this PR needs doc review complete by Doc Freeze on March 26th 18:00 PT to get this into the release.
Thank you @sftim for a thoroughly review detailed review, even with a shortage of reviewer capacity. Here are a few other suggestions I have for conciseness, comprehension and active voice:
@@ -46,7 +46,28 @@ tmpfs, Secrets use a tmpfs, etc.) | |||
Some popular filesystems that support idmap mounts in Linux 6.3 are: btrfs, |
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'm curious, what do we think about presenting the concept of idmap mounts for newcomers that aren't familiar with this from Linux?
https://lwn.net/Articles/896255/
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.
How I'd cover that:
- have a blog article that explains Kubernetes and idmapped mounts
- publish that article a week or so before the release
- mark the article as
evergreen
(ie: we'll maintain it; usually we don't maintain blog articles once they reach a year old) - link from these user namespaces docs to that evergreen blog article
An example of an evergreen article: https://kubernetes.io/blog/2020/09/03/warnings/
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.
IMHO that seems like too much overhead. It's not clear the value it would have to have such an ongoing investment to keep it up to date either, as it is suggested. There are other places that do document idmap mounts, not sure I see the point of a k8s specific documentation (there isn't anything k8s-specific that I can think of).
Furthermore, Kubernetes users just need to know if a filesystem supports idmap mounts or not. Nothing else is really relevant.
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.
Just two nits, otherwise LGTM especially content-wise.
041082a
to
d3ecab4
Compare
@drewhagen Thanks! I've addressed all the comments, PTAL |
content/en/docs/reference/command-line-tools-reference/feature-gates/user-namespaces-support.md
Outdated
Show resolved
Hide resolved
content/en/docs/tasks/configure-pod-container/user-namespaces.md
Outdated
Show resolved
Hide resolved
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
We were showing 4294967295 for the uid_map file, that is how it looks on the host (not the container). Let's fix that. While we are there, let's improve the explanation too. Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
Now the kubelet asks the runtime for the features it supports and if it doesn't report user namespaces support, then the kubelet will fail the pod creation. Therefore, it is no longer possible for the field to be ignored. Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
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.
/approve
This is OK to publish for beta
(I judge that the) changes since #45178 (review) don't affect the technical accuracy.
/lgtm
Thanks for sticking at it @rata. The journey to GA should be smoother for this effort.
LGTM label has been added. Git tree hash: 43606d45ecf81fad61f7ab97f028d09e5a47aa68
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sftim 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 |
🎉 |
/milestone 1.30 |
This PR updates the documentation for changes coming in 1.30 and some other small fixes that I'll backport once this is merged.
cc @giuseppe