Skip to content

Conversation

@rata
Copy link
Member

@rata rata commented Mar 23, 2023

containerd 1.7 was just released with user namespaces support. Let's mention this.

Also, let's emphasize in the task page that only supported runtimes work with user namespaces.

Fixes: #40233

@sftim doing only one PR against main is enough for this to show for 1.25 and 1.26? Let me know if I should open another PR for the other branch or what

cc @SergeyKanzhelev

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 23, 2023
@k8s-ci-robot k8s-ci-robot added the language/en Issues or PRs related to English language label Mar 23, 2023
@k8s-ci-robot k8s-ci-robot added the sig/docs Categorizes an issue or PR as relevant to SIG Docs. label Mar 23, 2023
@netlify
Copy link

netlify bot commented Mar 23, 2023

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit 05400ef
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/641c304d102eab0008687028
😎 Deploy Preview https://deploy-preview-40264--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Member

@SergeyKanzhelev SergeyKanzhelev left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 24, 2023
@rata rata force-pushed the rata/userns-1.25 branch from 761442b to 1332cf9 Compare March 24, 2023 11:02
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 24, 2023
@rata
Copy link
Member Author

rata commented Mar 24, 2023

Rebased and pushed to see if the CI works now. @SergeyKanzhelev can you re-LGTM? :)

@netlify
Copy link

netlify bot commented Mar 24, 2023

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit d2cd6ca
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/6421a6a059b61d0007020a07
😎 Deploy Preview https://deploy-preview-40264--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Member

@SergeyKanzhelev SergeyKanzhelev left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 24, 2023
sftim
sftim previously requested changes Mar 24, 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 very strongly recommend revising this before we merge it.

@sftim
Copy link
Contributor

sftim commented Mar 25, 2023

In fact, with #40264 (review) in mind:

/lgtm cancel

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 25, 2023
@rata rata force-pushed the rata/userns-1.25 branch 2 times, most recently from fb71333 to b00862e Compare March 27, 2023 10:38
* containerd: support is planned for the 1.7 release. See containerd
issue [#7063][containerd-userns-issue] for more details.
* containerd: version 1.7 supports user namespaces for containers, compatible
with Kubernetes {{< skew currentVersion >}}. If you are running a different
Copy link
Member Author

Choose a reason for hiding this comment

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

This will break if we apply this to 1.25. This will point to kubernetes 1.24, IIUC, which simply doesn't support user namespaces

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, I don't recommend backporting this line to the v1.25 docs

Copy link
Member Author

Choose a reason for hiding this comment

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

IMHO it will be simpler if we just use the versions, as they are not relative to any k8s version. In fact, here just expands to k8s 1.26, but this is true with k8s 1.25 too.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer calling out the specific version in this context.
We say "the WWII ended in 1945" instead of "in {{ current year }}, the WWII has ended." with a constant refreshing of the {{ current year }}.

Copy link
Contributor

Choose a reason for hiding this comment

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

Historical events aren't quite the right analogy. It's about a commutative relationship between (releases of) two things. The containerd docs might want to express the reverse relationship for example.

The text I've proposed for v1.27 also uses a skew shortcode (for a different reason).

I guess if we're sure that v1.7 of containerd is compatible specifically with v1.26 of Kubernetes and no other minor version of this project, we should say exactly that (and keep the “If you are running a different version of Kubernetes, check the documentation for that Kubernetes release.”)

However, this is an unusual case because of when we're making the change, ie after the code freeze for v1.27

Copy link
Member Author

Choose a reason for hiding this comment

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

@sftim what change is after code freeze? This doc PR?

First, this doc isn't for 1.27. This is for 1.25 and 1.26.

Secondly, that statement is true for 1.25 and 1.26, I don't understand why you insist in mentioning only one release.

And yes, we re-worked the implementation in 1.27 and what we need other requirements from the runtime. That is why it is not recommended to use 1.7 for userns with k8s >= 1.27.

But to return to more practical matter, how do you want me to phrase it here? So we can merge this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@sftim that link is not working (github is having hiccups, maybe is due to that). Can you please explain here again? Or a suggestion is even better ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, try #40264 (review)

Copy link
Member Author

Choose a reason for hiding this comment

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

@sftim thanks not sure what you mean, though. Yes, I'll open another PR for 1.25, but what change in the content here do you expect to see? I don't follow, sorry

@rata
Copy link
Member Author

rata commented Mar 27, 2023

@sftim PTAL, all comments are addressed now.

How will this work once this PR and PRs for 1.27 are merged? (#39860 and #40335) I think merging the main branch with the dev-1.27 might have conflicts, what happens in that case?

Also, I applied the suggestion to use the k8s skew version template thingy, but I think that will break when this is merged with the 1.25 branch (see the comment I added on that line for more details). What should we do? Can't we just use the hardcoded k8s versions (1.25 and 1.26) as what we really want is those versions and not something relative to the current k8s version?

@sftim
Copy link
Contributor

sftim commented Mar 27, 2023

We have a team that resolve merge conflicts. Consider putting in an HTML comment that explains your intent about the merge conflict that will arise.

I don't understand the shortcoming that we have with using {{< skew currentVersion >}} for the change to main. What worry do you have?

@rata
Copy link
Member Author

rata commented Mar 27, 2023

@sftim I'll add an html comment, thanks

My concern is that using version skrew does not expend to 1.26 AND 1.25 (which is what is accurate in this context) and can't be backported. If you want to keep it, I'm fine with it...

@rata rata force-pushed the rata/userns-1.25 branch from b00862e to 136ffd5 Compare March 27, 2023 11:33
@rata
Copy link
Member Author

rata commented Mar 27, 2023

@sftim added the comment, thanks. PTAL

@rata rata requested review from sftim and removed request for SergeyKanzhelev, divya-mohan0209 and mengjiao-liu March 27, 2023 12:05
containerd 1.7 was just released with user namespaces support. Let's
mention which kubernetes versions should work with container 1.7.

While we are there, let's clarify the CRI-O version and not duplicate
the requirements in the concept and task pages and just add a link

Signed-off-by: Rodrigo Campos <[email protected]>
@rata rata force-pushed the rata/userns-1.25 branch from 136ffd5 to d2cd6ca Compare March 27, 2023 14:21
@rata
Copy link
Member Author

rata commented Mar 27, 2023

@sftim Fixed, PTAL

@sftim sftim dismissed their stale review March 27, 2023 15:32

Review was stale

@SergeyKanzhelev
Copy link
Member

/lgtm

thank you @sftim for all the feedback.

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

LGTM label has been added.

Git tree hash: 491469ab6dbe3c37912cf396113498ac0a05467d

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: reylejano, SergeyKanzhelev

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 Apr 4, 2023
@k8s-ci-robot k8s-ci-robot merged commit 9360bc5 into kubernetes:main Apr 4, 2023
@rata rata deleted the rata/userns-1.25 branch April 4, 2023 09:46
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/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"document that containerd 1.7 is needed to use the userns feature (KEP 127)"

6 participants