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

127: Add KEP for user namespaces support #3065

Merged
merged 1 commit into from
Mar 8, 2022

Conversation

rata
Copy link
Member

@rata rata commented Nov 30, 2021

Here is the high level work we discussed during Nov 2 SIG-node meeting.

Let me know if this KEP effectively reflects what we discussed and IIUC agreed. I'd like to know of any concerns or show-stoppers ASAP rather than last minute, after we already worked a lot of time on this.

If there are no concerns and this plan is agreed by all parties and this is merged, with @giuseppe we will start a PoC implementation to fill-in the rest of the details this KEP needs (CRI changes, etc.).

To the best of my knowledge, this incorporates all the feedback in the PR #2101 that @thockin @mtaufen and others gave (see basically that almost all is automatic and the room for improvements in phase 3, in particular the per-sa/per-ns strategies are in scope, with some others).

Please let us know what you think and thanks again for your time! :)

Tagging people that might want to get their eyes on this:
@giuseppe @mrunalp @derekwaynecarr @endocrimes @rhatdan @alban @mauriciovasquezbernal

I created this with @giuseppe, who has already reviewed this.

Bellow c&p from the commit msg (see the slides linked below for a quick overview, if you prefer that! :)).


This commit adds the high level overview I proposed in the SIG-node
meeting on Nov 2. The work divided in phases and intial support (phase 1
and 2) is disentangled from further improvements that community members
wanted to see (phase 3).

This incorporates the valuable feedback in the discussion at PR #2101,
making things as automatic as possible and adding a phase 3 for such
improvements, while it also leaves room for future improvements too.

Slides used in the Nov 2 SIG-node meeting are here:
https://docs.google.com/presentation/d/1z4oiZ7v4DjWpZQI2kbFbI8Q6botFaA07KJYaKA-vZpg/edit#slide=id.gc6f73a04f_0_0

Closes: #2101

Signed-off-by: Rodrigo Campos rodrigo@kinvolk.io


  • One-line PR description: Add user namespaces KEP with its skeleton and phases
  • Issue link:
  • Other comments:

@k8s-ci-robot k8s-ci-robot added do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 30, 2021
@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/node Categorizes an issue or PR as relevant to SIG Node. labels Nov 30, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Nov 30, 2021

### Graduation Criteria

Graduation for each pod.spec field we introduce will be separate.
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 want to highlight this. I'd like the different pod.spec changes to graduate separate, so phase 1&2 can be in GA but phase 3 still in alpha/beta, as we gather more info and feedback (phase 3 will probably need quite a lot of feedback, I don't want that to block the progress of the rest of the features)

Copy link
Contributor

Choose a reason for hiding this comment

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

That will be through separate feature flags, right? Do we have proposed names? If we don't have one for later phases, we should call it out as a TBD.

Copy link
Member

@ddebroy ddebroy left a comment

Choose a reason for hiding this comment

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

A couple of thoughts to have a broader set of volume types - especially CSI plugin backed volumes - considered in Phase 1.

keps/sig-node/127-user-namespaces/README.md Show resolved Hide resolved
keps/sig-node/127-user-namespaces/README.md Show resolved Hide resolved
@rata rata force-pushed the rata/userns branch 2 times, most recently from 5a721e8 to 5a5f0fd Compare December 3, 2021 16:40
keps/sig-node/127-user-namespaces/README.md Outdated Show resolved Hide resolved
keps/sig-node/127-user-namespaces/README.md Outdated Show resolved Hide resolved
keps/sig-node/127-user-namespaces/README.md Outdated Show resolved Hide resolved
keps/sig-node/127-user-namespaces/README.md Outdated Show resolved Hide resolved
keps/sig-node/127-user-namespaces/README.md Outdated Show resolved Hide resolved
@rata
Copy link
Member Author

rata commented Dec 9, 2021

@ehashman Can we move this to "Needs reviewer"? I've answered all the open comments and my understanding from the last SIG-node meeting is that I'm waiting on @mrunalp and @derekwaynecarr to take a closer look. And I will ping @thockin and @mtaufen too.

@rata
Copy link
Member Author

rata commented Dec 9, 2021

Added @SergeyKanzhelev as reviewer now, as he asked me to do in the last sig-node meeting :)

Copy link
Contributor

@mtaufen mtaufen left a comment

Choose a reason for hiding this comment

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

Thanks @rata for writing this up. The overall breakdown looks good and I'm eager to see this work get started, so all the low level/behind the scenes groundwork can make progress as you mentioned.

It looks like there are maybe still some details to fill out in the KEP but this LGTM as a high level plan of how to approach the work.

keps/sig-node/127-user-namespaces/README.md Show resolved Hide resolved
keps/sig-node/127-user-namespaces/README.md Outdated Show resolved Hide resolved
@rata
Copy link
Member Author

rata commented Dec 20, 2021

@mtaufen Thanks a lot for the review!

@ehashman can we move this to needs review? (as I mentioned here #3065 (comment)). A review from @mrunalp and @derekwaynecarr is pending. @thockin also told me on slack that he will review during this break, hopefully :)

@mrunalp
Copy link
Contributor

mrunalp commented Feb 22, 2022

This is looking fine to me overall for phase 1. I left a few nits.

@rata
Copy link
Member Author

rata commented Feb 23, 2022

@SergeyKanzhelev @mrunalp thanks! We have addressed all the review comments. PTAL, let me know if anything is missing to merge this in with status provisional

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

thank you. I think it has enough information to kick off the phase 1

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 24, 2022
@mrunalp
Copy link
Contributor

mrunalp commented Feb 25, 2022

/lgtm
I think we have enough for Phase 1. Thanks!

@giuseppe
Copy link
Member

could we get /approve as well? :-)

/assign @derekwaynecarr

approvers:
- "@derekwaynecarr"
feature-gates:
- name: UserNamespacesPhase1
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Do you want to rename this back to UserNamespacesSupport as discussed in the thread before merge?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, sure. I didn't do it in case pushing will lose the LGTM (I'm not sure what is the config on this repo). Will push in a few minutes :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeap, we lost the lgtm label with the push :(

This commit adds the high level overview I proposed in the SIG-node
meeting on Nov 2. The work divided in phases and intial support (phase 1
and 2) is disentangled from further improvements that community members
wanted to see (phase 3).

This incorporates the valuable feedback in the discussion at PR 2101,
making things as automatic as possible and adding a phase 3 for such
improvements, while it also leaves room for future improvements too.

Slides used in the Nov 2 SIG-node meeting are here:
	https://docs.google.com/presentation/d/1z4oiZ7v4DjWpZQI2kbFbI8Q6botFaA07KJYaKA-vZpg/edit#slide=id.gc6f73a04f_0_0

Signed-off-by: Rodrigo Campos <rodrigo@kinvolk.io>
Co-authored-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 1, 2022
@rata
Copy link
Member Author

rata commented Mar 1, 2022

@mrunalp pushed changing only the gate-feature name as you requested. Can you LGTM again? :)

@mrunalp
Copy link
Contributor

mrunalp commented Mar 2, 2022

/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 2, 2022
@rata
Copy link
Member Author

rata commented Mar 3, 2022

@derekwaynecarr ? :)

@derekwaynecarr
Copy link
Member

Phase 1 looks great.

/approve
/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, rata, 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 Mar 8, 2022
@k8s-ci-robot k8s-ci-robot merged commit e69a9bf into kubernetes:master Mar 8, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.24 milestone Mar 8, 2022
@rata rata deleted the rata/userns branch March 9, 2022 14:53
ahmedtd pushed a commit to ahmedtd/enhancements that referenced this pull request Jun 8, 2022
This commit adds the high level overview I proposed in the SIG-node
meeting on Nov 2. The work divided in phases and intial support (phase 1
and 2) is disentangled from further improvements that community members
wanted to see (phase 3).

This incorporates the valuable feedback in the discussion at PR 2101,
making things as automatic as possible and adding a phase 3 for such
improvements, while it also leaves room for future improvements too.

Slides used in the Nov 2 SIG-node meeting are here:
	https://docs.google.com/presentation/d/1z4oiZ7v4DjWpZQI2kbFbI8Q6botFaA07KJYaKA-vZpg/edit#slide=id.gc6f73a04f_0_0

Signed-off-by: Rodrigo Campos <rodrigo@kinvolk.io>
Co-authored-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Co-authored-by: Giuseppe Scrivano <gscrivan@redhat.com>
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. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet