Share cgroup namespace in pod #12310

hshiina · 2021-11-16T02:43:51Z

Signed-off-by: Hironori Shiina shiina.hironori@jp.fujitsu.com

What this PR does / why we need it:

podman help pod create displays cgroup as one of default values of
--share flag, which specifies shared namespaces in a pod. However,
cgroup is not listed in SharedNameSpaces of a pod with default
values. Even if cgroup is explicitly specified in --share flag,
cgroup is not listed in pod inspection and actually containers in a
pod don't share a cgreoup namespace.

There are two similar parameters for cgroups in a pod. When cgroup
is specified in --share, PodConfig.UsePodCgroup is set to true.
For SharedNameSpaces in pod inspection, PodConfig.UsePodCgroupNS is
referred to.

In order to share cgroup namespace expectedly,
PodConfig.UsePodCgroupNS should be set to true via
WithPodCgroup(). Regarding PodConfig.UsePodCgroup, this should be
always true via WithPodCgroups() as it was in Podman 3.3.

How to verify it

Run added integration tests.

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

openshift-ci · 2021-11-16T02:43:54Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: hshiina
To complete the pull request process, please assign saschagrunert after the PR has been reviewed.
You can assign the PR to them by writing /assign @saschagrunert in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mheon · 2021-11-16T04:42:29Z

pkg/specgen/namespaces.go

@@ -54,7 +54,7 @@ const (

 	// DefaultKernelNamespaces is a comma-separated list of default kernel
 	// namespaces.
-	DefaultKernelNamespaces = "cgroup,ipc,net,uts"
+	DefaultKernelNamespaces = "ipc,net,uts"


Why drop cgroup from this?

Though I posted this fix because values in --share and a result of pod inspection are inconsistent, I'm not sure if it's worth sharing a cgroup namespace in a pod.Containers in a pod have their own cgroups under a pod cgroup by default.
If a container enters the cgroup namespace of the infra container like other shared namespaces, the container cannot see its own cgroup.Because this behavior doesn't seem so useful, I dropped 'cgroup' from the default shared namespaces.

It also causes me to do it that crun cannot put a container into a specified cgroup namespace currently (containers/crun#781).

I would rather wait until crun is updated and retain the cgroup sharing by default, if possible.

I agree, lets remove the change and retest with latest crun.

Sure. I will remove this change.

When a new container joins a cgroup namespace of another container, if the given container uses the host's namespace, the new container doesn't have to re-enter the host's namespace. Especially, if the new container is rootless, the container cannot re-enter the host's namespace. Signed-off-by: Hironori Shiina <shiina.hironori@jp.fujitsu.com>

`podman help pod create` displays `cgroup` as one of default values of `--share` flag, which specifies shared namespaces in a pod. However, `cgroup` is not listed in `SharedNameSpaces` of a pod with default values. Even if `cgroup` is explicitly specified in `--share` flag, `cgroup` is not listed in pod inspection and actually containers in a pod don't share a cgroup namespace. There are two similar parameters for cgroups in a pod. When `cgroup` is specified in `--share`, `PodConfig.UsePodCgroup` is set to `true`. For `SharedNameSpaces` in pod inspection, `PodConfig.UsePodCgroupNS` is referred to. In order to share `cgroup` namespace expectedly, `PodConfig.UsePodCgroupNS` should be set to `true` via `WithPodCgroup()`. Regarding `PodConfig.UsePodCgroup`, this should be always `true` via `WithPodCgroups()` as it was in Podman 3.3. Signed-off-by: Hironori Shiina <shiina.hironori@jp.fujitsu.com>

cdoern · 2021-12-15T18:49:48Z

I am going to give this a look later today, I am skeptical of how this is just being applied on a ctr by ctr level. Not sure thought because I just glanced at this.

cdoern

@hshiina I have some concerns about when this change is being applied. Does this make it so it is impossible to not share cgroupns w/in a pod?

cdoern · 2021-12-19T18:17:15Z

libpod/options.go

@@ -1024,8 +1024,16 @@ func WithCgroupNSFrom(nsCtr *Container) CtrCreateOption {
 			return err
 		}

-		ctr.config.CgroupNsCtr = nsCtr.ID()
+		for _, ns := range nsCtr.config.Spec.Linux.Namespaces {


this might result in some nil pointers, no?

Yup, is this not checked elsewhere?

Thank you for the comments.
I will use Container.hasNamespace(), which checks nil pointers:
https://github.com/containers/podman/blob/main/libpod/container_internal.go#L2321

cdoern · 2021-12-19T18:18:47Z

pkg/specgen/generate/namespaces.go

@@ -454,7 +454,7 @@ func GetNamespaceOptions(ns []string, netnsIsHost bool) ([]libpod.PodCreateOptio
 	for _, toShare := range ns {
 		switch toShare {
 		case "cgroup":
-			options = append(options, libpod.WithPodCgroups())
+			options = append(options, libpod.WithPodCgroup())


WithPodCgroup or Cgroups?

I think this should be WithPodCgroup, which sets pod.config.UsePodCgroupNS to True. It might be better to rename these functions to make their roles clear.

cdoern · 2021-12-19T18:18:53Z

pkg/specgen/generate/pod_create.go

@@ -176,6 +176,7 @@ func createPodOptions(p *specgen.PodSpecGenerator, rt *libpod.Runtime, infraSpec
 			options = append(options, libpod.WithPodUser())
 		}
 	}
+	options = append(options, libpod.WithPodCgroups())


WithPodCgroup or Cgroups?

also, should this always be done?? what if we do not want to share?

Agreed we should only do this is the pod shares cgroups.

WithPodCgroups used to be always called in v3.3:
https://github.com/containers/podman/blob/v3.3/pkg/specgen/generate/pod_create.go#L142

As far as I investigated, it seems that this was unintentionally removed:

https://github.com/containers/podman/pull/11102/files#diff-edb3bda86436d2a7bc55b34fa48c71156dc5e02d5f452012b1750efb15b3158fR160

https://github.com/containers/podman/pull/12271/files#diff-edb3bda86436d2a7bc55b34fa48c71156dc5e02d5f452012b1750efb15b3158fL261

This is why I just restored this call.

I would consider introducing a new flag to specify whether cgroup is shared in a pod or not though it might be better to work in another PR.

IT should be handled via the --shares command, if the user wants to eliminate it, then using the shares command should handle it.

I think it should always be called by default as well @hshiina but the issue here is that --share=none here will not take affect since line is executed always, right?

I should have clarified which is expected to be shared by pod create --share cgroup, cgroup or cgroup namespace, at first. Though this PR assumes cgroup namespace is shared based on the help message, I would reconsider.

cdoern · 2022-01-06T14:39:41Z

@hshiina any updates on this PR? I just want to make sure that --share=none still works as expected.

hshiina · 2022-01-06T23:05:31Z

--share=none works as expected with assuming a cgroup namespace is shared by '--share cgroup'. However, I'm leaning towards reverting to the original behavior so that --share cgroup shares a cgroup parent.

This PR is more complicated than I expected at first. I filed a issue to elaborate on the concern: #12765

rhatdan · 2022-01-27T12:54:30Z

I think this PR has been taken over in a different PR and is not longer necessary. Closing, reopen if I am mistaken.

mheon reviewed Nov 16, 2021

View reviewed changes

Hironori Shiina added 2 commits November 23, 2021 17:06

hshiina force-pushed the pod-cgroup branch from e183bdb to 10d4bb3 Compare November 23, 2021 22:35

cdoern reviewed Dec 19, 2021

View reviewed changes

rhatdan closed this Jan 27, 2022

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 21, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Share cgroup namespace in pod #12310

Share cgroup namespace in pod #12310

hshiina commented Nov 16, 2021 •

edited

Loading

openshift-ci bot commented Nov 16, 2021

mheon Nov 16, 2021 •

edited by TomSweeneyRedHat

Loading

hshiina Nov 18, 2021

mheon Nov 18, 2021

rhatdan Nov 19, 2021

hshiina Nov 20, 2021

cdoern commented Dec 15, 2021

cdoern left a comment •

edited

Loading

cdoern Dec 19, 2021

rhatdan Dec 20, 2021

hshiina Dec 20, 2021

cdoern Dec 19, 2021

hshiina Dec 20, 2021

cdoern Dec 19, 2021

cdoern Dec 19, 2021

rhatdan Dec 20, 2021

hshiina Dec 20, 2021

rhatdan Dec 21, 2021

cdoern Dec 21, 2021

hshiina Dec 22, 2021

cdoern commented Jan 6, 2022

hshiina commented Jan 6, 2022

rhatdan commented Jan 27, 2022

Share cgroup namespace in pod #12310

Share cgroup namespace in pod #12310

Conversation

hshiina commented Nov 16, 2021 • edited Loading

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

openshift-ci bot commented Nov 16, 2021

mheon Nov 16, 2021 • edited by TomSweeneyRedHat Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdoern commented Dec 15, 2021

cdoern left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdoern commented Jan 6, 2022

hshiina commented Jan 6, 2022

rhatdan commented Jan 27, 2022

hshiina commented Nov 16, 2021 •

edited

Loading

mheon Nov 16, 2021 •

edited by TomSweeneyRedHat

Loading

cdoern left a comment •

edited

Loading