podman pod create --pid flag #10894

cdoern · 2021-07-09T16:20:02Z

added support for --pid flag. User can specify ns:file, pod, private, or host pid namespace values. container returns an error since you cannot point the ns of the pods infra container
to a container outside of the pod.

Signed-off-by: cdoern [email protected]

rhatdan · 2021-07-09T17:35:02Z

What does "pod" mean? Are you allowing pod's to join other pods pid namespace?

docs/source/markdown/podman-pod-create.1.md

libpod/define/pod_inspect.go

libpod/options.go

mheon · 2021-07-09T17:39:40Z

libpod/options.go

+		if pod.valid {
+			return define.ErrPodFinalized
+		}
+		pod.config.Pid = inp


We need to require that UsePodPID is set in the pod configuration - we have to be sharing the pod PID namespace for this to be set.

We need to require that UsePodPID is set in the pod configuration - we have to be sharing the pod PID namespace for this to be set.

Should I require it by checking and erroring out if not set, or warn the user and set it for them?

Good question. I was leaning towards just erroring out unless the user passes --share as well to specify that the PID namespace is shared, but that seems like a bad user experience. @rhatdan Should setting --pid automatically cause the pod to share the PID namespace?

libpod/pod.go

mheon · 2021-07-09T17:41:08Z

libpod/pod.go

@@ -170,6 +174,16 @@ func (p *Pod) CPUQuota() int64 {
 	return 0
 }

+// Pid returns the pod process id namespace value


This isn't getting the PID, but rather the path of the infra container's PID file?

It's also not really related to the PID namespace

Yeah I was unsure here of what would be helpful here @mheon, because the namespace just contains pod or host or whatever the user passed or a ns file if specified. What would this be expected to return?

Returning the mode of the namespace is what I would expect - we want to tell the user how the pod was configured. The PID of the pod's init container isn't that valuable.

Similar comment here for me, I'd change the variable to indicate it's pidmode and not the pid to help make the support of this more clear months down the line.

good idea @TomSweeneyRedHat. I will change to make it more explicit that it returns the mode passed by the user.

libpod/pod_api.go

mheon · 2021-07-09T17:43:27Z

libpod/runtime_pod_infra_linux.go

+			ns := spec.LinuxNamespace{}
+			pidVal := p.config.Pid.NSMode
+			if pidVal != "" {
+				if pidVal == "container" {


This validation should be done in the option (WithPid() so we only have to do it once and we can reject invalid configurations at creation time, not runtime.

Okay @mheon , to streamline this and preserve the originally passed values I might make another InfraContainerConfig value that is an actual specs.LinuxNameSpace{}. This will allow me to be able to print the proper mode in podInspect but also not have to parse anything again in runtime_pod_infra_linux.go

libpod/runtime_pod_infra_linux.go

libpod/runtime_pod_linux.go

mheon · 2021-07-09T17:48:00Z

pkg/domain/entities/pods.go

@@ -146,6 +147,17 @@ func (p *PodCreateOptions) CPULimits() *specs.LinuxCPU {
 	return cpu
 }

+func setNamespaces(p *PodCreateOptions) (specgen.Namespace, error) {


Suggest renaming to "getPIDNamespace" or, alternatively, making this return a struct which identifies which namespace is which - right now this is only useful for setting the PID namespace, but eventually we'll end up with four-ish settable namespaces.

mheon · 2021-07-09T17:48:35Z

pkg/specgen/generate/pod_create.go

@@ -102,6 +102,13 @@ func createPodOptions(p *specgen.PodSpecGenerator, rt *libpod.Runtime) ([]libpod
 		options = append(options, libpod.WithInfraCommand(p.InfraCommand))
 	}

+	if !p.Pid.IsDefault() {
+		options = append(options, libpod.WithPid(p.Pid))
+		if p.Pid.NSMode == "pod" {


This bit doesn't seem necessary?

pkg/domain/entities/pods.go

pkg/specgen/podspecgen.go

libpod/options.go

mheon · 2021-07-09T19:52:32Z

libpod/options.go

+		}
+		if p.config.UsePodPID {
+			ns := specs.LinuxNamespace{}
+			ns.Type = "pid"


Is there a reason we need to recreate the namespace?

I think specs.LinuxNameSpace{} is the type that the infra generator takes versus specgen.NameSpace, we need to switch over at some point and this is behind the scenes enough.

Ahhh, I missed the type change. Hm. My concern would be forwards-compat - we need to make sure that pods from before this change, without a valid namespace in the DB, continue to work.

well, the specgen.ParseNamespace() function I use in pkg/domain/entities/pods.go takes the type specgen.NameSpace so the actual validation and parsing relies more on the specgen version this is just a conversion. This switch will only execute when someone explicitly says --pid=... so would forwards-compat be a problem?

libpod/options.go

cdoern · 2021-07-09T20:18:41Z

What does "pod" mean? Are you allowing pod's to join other pods pid namespace?

pod means that the containers will default to using the pod's pid ns which is similar to private (the default). I added this because the specgen.parsenamespace() function utilizes this value so we are going to accept.

mheon · 2021-07-13T19:25:53Z

libpod/pod.go

@@ -97,6 +98,8 @@ type InfraContainerConfig struct {
 	HasInfraContainer  bool                  `json:"makeInfraContainer"`
 	NoNetwork          bool                  `json:"noNetwork,omitempty"`
 	HostNetwork        bool                  `json:"infraHostNetwork,omitempty"`
+	Pid                specgen.Namespace     `json:"infraPid,omitempty"`
+	LinuxPidNS         specs.LinuxNamespace  `json:"LinuxPIDNS,omitempty"`


Do we need both of these? I would imagine we can get the mode just by looking at the specs.LinuxNamespace - or, alternatively, generate the specs.LinuxNamespace from the specgen.Namespace when creating the infra container?

@mheon I kept both of them in order to parse the Pid NS before runtime. The types are different as specgen.Namespace contains info like host or pod which I do use again at runtime but specs.LinuxNamespace is the actual NS struct containing pid and the proper path. Not sure if I can parse before runtime and not have both of these but I will look.

TomSweeneyRedHat · 2021-07-14T20:08:38Z

docs/source/markdown/podman-pod-create.1.md

@@ -120,6 +120,14 @@ Add a DNS alias for the container. When the container is joined to a CNI network

 Disable creation of /etc/hosts for the pod.

+#### **--pid**=*pid*
+
+Set the PID mode for the pod Default is to create a private PID namespace for the pod. Requires PID NS to be shared.


I'm having a hard time parsing this. Maybe?

Suggested change

Set the PID mode for the pod Default is to create a private PID namespace for the pod. Requires PID NS to be shared.

Set the PID mode for the pod. The default is to create a private PID namespace for the pod. Requires the PID namespace to be shared.

TomSweeneyRedHat · 2021-07-14T20:15:29Z

docs/source/markdown/podman-pod-create.1.md

@@ -120,6 +120,14 @@ Add a DNS alias for the container. When the container is joined to a CNI network

 Disable creation of /etc/hosts for the pod.

+#### **--pid**=*pid*


I was thinking "process ID (pid) when reading this. I'll let others over-rule me, but I'd rather the option be named something else, in case we ever manipulate the pid later down the road. Perhaps --pid-mode, regardless, at least change the variable:

Suggested change

#### **--pid**=*pid*

#### **--pid**=*pid-mode*

I feel like --pid-ns might be more accurate if anything @TomSweeneyRedHat. Only thing is, container create calls it --pid

Ugh. They should be the same in both places, and it looks like Docker has gone with --pid too, so we probably ought to stay with --pid. Rather than *pid-mode* at line 123, I'd be fine with *pid-ns* if you prefer for the variable

okay @TomSweeneyRedHat I am good with keeping it as --pid, probably best so people don't get confused as to what this flag is compared to container create and docker

vrothberg

LGTM in general. Just two comments.

vrothberg · 2021-07-15T11:52:38Z

libpod/pod.go

@@ -97,6 +98,7 @@ type InfraContainerConfig struct {
 	HasInfraContainer  bool                  `json:"makeInfraContainer"`
 	NoNetwork          bool                  `json:"noNetwork,omitempty"`
 	HostNetwork        bool                  `json:"infraHostNetwork,omitempty"`
+	Pid                specgen.Namespace     `json:"infraPid,omitempty"`


I think this should be called PidNS to prevent mistaking it for a process ID. Same at other places in the code.

But I do agree that --pid makes sense on the CLI to remain consistent with container create.

vrothberg · 2021-07-15T11:54:52Z

test/e2e/pod_create_test.go

+		podCreate = podmanTest.Podman([]string{"pod", "create", "--pid", ns, "--share", "pid"})
+		podCreate.WaitWithDefaultTimeout()
+		Expect(podCreate).Should(ExitWithError())
+


Will the pid ns now be displayed in podman inspect? If so, it would be nice to exercise that here in the test.

@vrothberg we decided to output the NSMode the user gave us as it is more helpful, aka: path, pod, private... I will check for that here, thanks for calling that out!

baude · 2021-07-15T13:31:05Z

/hold

baude · 2021-07-15T13:31:12Z

/lgtm

added support for --pid flag. User can specify ns:file, pod, private, or host. container returns an error since you cannot point the ns of the pods infra container to a container outside of the pod. Signed-off-by: cdoern <[email protected]>

cdoern · 2021-07-15T15:34:33Z

sorry had to re-push to fix a test, ran locally and all green. should be good now @baude

rhatdan · 2021-07-15T19:17:46Z

/approve
/lgtm

openshift-ci · 2021-07-15T19:17:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cdoern, rhatdan

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:

~~OWNERS~~ [rhatdan]

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

ashley-cui · 2021-07-15T21:29:35Z

/hold cancel

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 9, 2021

cdoern force-pushed the pidPod branch 2 times, most recently from bcc4d84 to 9474e69 Compare July 9, 2021 16:35