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

Default host user namespace via experimental flag #31169

Merged
merged 1 commit into from Nov 14, 2016

Conversation

pweil-
Copy link
Contributor

@pweil- pweil- commented Aug 22, 2016

@vishh @ncdc @pmorie @smarterclayton @thockin

Initial thought on the implementation #30684 (comment) wasn't quite right. Since we need to dereference a PVC in some cases the defaulting code didn't fit nicely in the docker manager code (would've coupled it with a kube client and would've been messy). I think passing this in via the runtime config turned out cleaner. PTAL


This change is Reviewable

@@ -211,4 +211,7 @@ func (s *KubeletServer) AddFlags(fs *pflag.FlagSet) {
fs.StringVar(&s.EvictionMinimumReclaim, "eviction-minimum-reclaim", s.EvictionMinimumReclaim, "A set of minimum reclaims (e.g. imagefs.available=2Gi) that describes the minimum amount of resource the kubelet will reclaim when performing a pod eviction if that resource is under pressure.")
fs.Int32Var(&s.PodsPerCore, "pods-per-core", s.PodsPerCore, "Number of Pods per core that can run on this Kubelet. The total number of Pods on this Kubelet cannot exceed max-pods, so max-pods will be used if this calculation results in a larger number of Pods allowed on the Kubelet. A value of 0 disables this limit.")
fs.BoolVar(&s.ProtectKernelDefaults, "protect-kernel-defaults", s.ProtectKernelDefaults, "Default kubelet behaviour for kernel tuning. If set, kubelet errors if any of kernel tunables is different than kubelet defaults.")

// experimental flags
fs.BoolVar(&s.ExperimentalHostUserNamespaceDefaulting, "experimental-host-user-namespace-defaulting", s.ExperimentalHostUserNamespaceDefaulting, "Default the value for UsernsMode for containers that are using other host namespaces, host mounts, or specific non-namespaced capabilities (MKNOD, SYS_MODULE, SYS_TIME). This should only be enabled is user namespace remapping is enabled in the docker daemon. [default=false]")
Copy link
Contributor

Choose a reason for hiding this comment

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

In the comment, explain what you are defaulting the value to. This description did not explain it.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 23, 2016
@@ -415,6 +415,9 @@ type KubeletConfiguration struct {
// iptablesDropBit is the bit of the iptables fwmark space to use for dropping packets. Kubelet will ensure iptables mark and drop rules.
// Values must be within the range [0, 31]. Must be different from IPTablesMasqueradeBit
IPTablesDropBit int32 `json:"iptablesDropBit"`
// experimentalHostUserNamespaceDefaulting is an experimental flag to enable defaulting of the UsernsMode for
// a container.
Copy link
Member

Choose a reason for hiding this comment

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

Comment about this requiring remapping to be enabled in the daemon (or, ahem, container runtime?)

@pweil- pweil- force-pushed the userns-experimental branch 3 times, most recently from c675e0d to 8dfdec5 Compare August 23, 2016 12:50
limitations under the License.
*/

package kubelet
Copy link
Member

Choose a reason for hiding this comment

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

note to other reviewers: I asked @pweil- to name this file as such, going to put a ton of other stuff into it in a planned refactor.

@pmorie pmorie added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Aug 23, 2016
@pmorie
Copy link
Member

pmorie commented Aug 23, 2016

This LGTM, I'll give others a chance to chime in.

@pmorie
Copy link
Member

pmorie commented Aug 23, 2016

@smarterclayton @thockin is this going to be 1.4?

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE e2e failed for commit bdbdcdfd660e80cdcdc503e725ef2c30e4364bc3. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@pweil-
Copy link
Contributor Author

pweil- commented Nov 7, 2016

@k8s-bot cvm gce e2e test this

@pweil-
Copy link
Contributor Author

pweil- commented Nov 7, 2016

rebased tests are green again, care to re-apply LGTM?

@smarterclayton @derekwaynecarr

@pweil-
Copy link
Contributor Author

pweil- commented Nov 7, 2016

@vishh ^

@pweil-
Copy link
Contributor Author

pweil- commented Nov 8, 2016

@vishh bump

@pweil-
Copy link
Contributor Author

pweil- commented Nov 8, 2016

@saad-ali FYI this has had LGTM and has been in rebase cycles since (if you need to do any release czar maintenance)

@saad-ali
Copy link
Member

saad-ali commented Nov 8, 2016

@saad-ali FYI this has had LGTM and has been in rebase cycles since (if you need to do any release czar maintenance)

Ack. Thanks for the heads up. Are there any concerns about this PR in terms of risk to the 1.5 release? I see that the initial plan was to get it merged early in the 1.5 dev cycle to give it time to bake.

@vishh
Copy link
Contributor

vishh commented Nov 8, 2016

Saad, this PR is expected to be turned off by default. So from a risk
perspective, it should be low.

On Tue, Nov 8, 2016 at 12:27 PM, Saad Ali notifications@github.com wrote:

@saad-ali https://github.com/saad-ali FYI this has had LGTM and has
been in rebase cycles since (if you need to do any release czar maintenance)

Ack. Thanks for the heads up. Are there any concerns about this PR in
terms of risk to the 1.5 release? I see that the initial plan was to get it
merged early in the 1.5 dev cycle to give it time to bake.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#31169 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGvIKEq31lgRSY-p5m22zJLbQlGEvFMIks5q8NtOgaJpZM4JqWln
.

@pweil-
Copy link
Contributor Author

pweil- commented Nov 8, 2016

Saad, this PR is expected to be turned off by default. So from a risk
perspective, it should be low.

Agree, the feature is guarded by a feature gate and is turned off by default

@saad-ali
Copy link
Member

saad-ali commented Nov 8, 2016

Saad, this PR is expected to be turned off by default. So from a risk
perspective, it should be low.

Agree, the feature is guarded by a feature gate and is turned off by default

Great, thanks.

@deads2k
Copy link
Contributor

deads2k commented Nov 9, 2016

Saad, this PR is expected to be turned off by default. So from a risk
perspective, it should be low.
Agree, the feature is guarded by a feature gate and is turned off by default
Great, thanks.

Looks like this has approval. Retagging.

@deads2k deads2k added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 9, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins unit/integration failed for commit bdbdcdfd660e80cdcdc503e725ef2c30e4364bc3. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE Node e2e failed for commit bdbdcdfd660e80cdcdc503e725ef2c30e4364bc3. Full PR test history.

The magic incantation to run this job again is @k8s-bot node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 10, 2016
@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 14, 2016
@dims
Copy link
Member

dims commented Nov 14, 2016

@deads2k @saad-ali - looks like one of you have to reapply lgtm tag as the bot removed it

@pweil-
Copy link
Contributor Author

pweil- commented Nov 14, 2016

@deads2k @saad-ali - looks like one of you have to reapply lgtm tag as the bot removed it

yes, please. PR needed rebase. Thanks!

@smarterclayton smarterclayton added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 14, 2016
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet