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.

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

Doc update for userns in 1.28 #41908

Merged
merged 6 commits into from Aug 8, 2023

Conversation

rata
Copy link
Member

@rata rata commented Jul 6, 2023

Commits 1-3 are ports of what we just merged in main, for 1.27. They improve the wording, fix a typo and use a timeless way for some sentences.

On top of that is the new commit that just removes mentiones to stateless pods, as we support all pods (stateful and stateless) now.

@k8s-ci-robot k8s-ci-robot added this to the 1.28 milestone Jul 6, 2023
@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 6, 2023
@rata
Copy link
Member Author

rata commented Jul 6, 2023

cc @giuseppe

@k8s-ci-robot k8s-ci-robot added the language/en Issues or PRs related to English language label Jul 6, 2023
@k8s-ci-robot k8s-ci-robot added the sig/docs Categorizes an issue or PR as relevant to SIG Docs. label Jul 6, 2023
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 17, 2023
@netlify
Copy link

netlify bot commented Jul 17, 2023

👷 Deploy Preview for kubernetes-io-vnext-staging processing.

Name Link
🔨 Latest commit c250efb
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/64d10b097e1f4c0008798f84

@rata rata marked this pull request as ready for review July 21, 2023 17:15
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 21, 2023
@rata
Copy link
Member Author

rata commented Jul 21, 2023

This is ready for review, PTAL :)

Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@natalisucks
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 25, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: f090ff8f77650d7fedd9361331f2b9fe5e1fd301

sftim
sftim previously requested changes Aug 5, 2023
Copy link
Contributor

@sftim sftim left a 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 see a change to https://kubernetes.io/docs/reference/command-line-tools-reference/feature-gates/

Is that still needed? If not, why not?

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 7, 2023
@rata rata requested a review from sftim August 7, 2023 13:51
@rata
Copy link
Member Author

rata commented Aug 7, 2023

@sftim I think that page is auto-generated, right? I think it is because the KEP didn't update the yaml (if this is auto generated). I've just opened a PR to fix the kep yaml: https://github.com/kubernetes/enhancements/pull/4147/files

Do you know if that is enough? Or anything else we need to do to fix it?

@rata rata force-pushed the rata/userns-1.28 branch 2 times, most recently from 528252e to 0be42ca Compare August 7, 2023 14:29
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 7, 2023
The variable expansion is wrong: it currently expands to 1.27.3 on the
rendered website, so it says it is supported in 1.27 and that it is not.

Let's just re-work this paragraph so it is cleaerer and the variable
expansion is what we want (1.27 and not 1.27.3)-

Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
The note is no longer valid (the branch was already merged and the
merged was done correctly).

Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
@rata rata force-pushed the rata/userns-1.28 branch 3 times, most recently from ee88d1c to 05f6207 Compare August 7, 2023 14:38
@rata
Copy link
Member Author

rata commented Aug 7, 2023

@sftim all fixed now. PTAL. CI is not running for some reason (but not failing either, just not running at all).

@sftim
Copy link
Contributor

sftim commented Aug 7, 2023

I recommend rebasing against latest dev-1.28 and squashing to 1 commit.

@sftim sftim dismissed their stale review August 7, 2023 14:43

Feedback was addressed

Please note that containerd v1.7 supports user namespaces for containers,
compatible with Kubernetes {{< skew currentPatchVersion >}}. It should not be used
with Kubernetes 1.27 (and later).
containerd v1.7 is not compatible with the userns support in Kubernetes v1.27 to v{{< skew latestVersion >}}.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?
What does this mean?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tengqm that you can't use userns with containerd 1.7 and those k8s versions

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It won't work, it will throw an error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me try understand the situation.
containerd 1.7 + k8s 1.25/1.26 -> worked;
containerd 1.7 + k8s 1.27 -> this doesn't work;

If my understanding is correct, how about adjust the order of the two statements?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tengqm Your understanding is correct. But can you make a specific suggestion here on github? I mean, just changing the order doesn't make much sense IMHO, as the second sentence highlights that some version do support it. It would be weird to say that before sayin some version does NOT support it.

I can do the change that you want, but give me github suggestion to avoid misunderstandings :)

The feature gate to enable user namespaces was previously named
`UserNamespacesStatelessPodsSupport`, when only stateless pods were supported.
Only Kubernetes v1.25 through to v1.27 recognise `UserNamespacesStatelessPodsSupport`; for
Kubernetes `{{< skew currentVersion >}}`, you need to use `UserNamespacesSupport`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an abuse of the skew shortcode.
The 'currentVersion' is a moving target, so this statement will become obsolete soon.
We will eventually remove this statement, either because 1.28 becomes an ancient release or because the gate has graduated/deprecated and removed. We are not sure which one will happen first. So .. instead you may want to clearly call out the version number rather than playing the skew magic.

"for Kubernetes start from v1.28, you need to ..."

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"for Kubernetes start from v1.28, you need to ..."

If we forget to update the text for some future release, that proposed text becomes equally incorrect.
The trick is not to forget! Maybe one day though we'll be able to warn - or fail a built - when people forget (I can see how to write that).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed everything after the ;. This way, this sentence is always correct.

Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
@rata
Copy link
Member Author

rata commented Aug 7, 2023

@sftim @tengqm PTAL.

@katcosgrove
Copy link
Contributor

Hey y'all! I'm swinging by for the v1.28 Docs team. Today is the docs deadline. Is there anything we can help you with to get this merged?

@sftim
Copy link
Contributor

sftim commented Aug 8, 2023

I'm happy with the changes since #41908 (comment)

/lgtm
/approve

We have some time before the release @tengqm if you want to propose a revision.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 8, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: e60b83195b7a55efd130a62d718f3aed2bba257f

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, saschagrunert, 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 /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 Aug 8, 2023
@k8s-ci-robot k8s-ci-robot merged commit b23511b into kubernetes:dev-1.28 Aug 8, 2023
6 of 7 checks passed
Rishit-dagli pushed a commit to Rishit-dagli/website that referenced this pull request Aug 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants