Support --pid=container:xxx for `nerdctl run` cmd #1411

minuk-dev · 2022-10-04T12:08:07Z

#1293
Signed-off-by: Min Uk Lee [email protected]

Changes

support --pid=container:xxx for nerdctl run command
extract generatePIDOpts() function for pid
(Updated) update setPlatformOptions()
(Updated) add setPlatformContainerOptions() - Details 1
(Updated) add PIDContainer label - Details 2

Details

1. add `setPlatformContainerOptions()`

Some functions only works on a specific platform (mostly linux). For persistency, they need to save information into container's labels.
Previously, it was processed on runtime. (e.g. hostname, logURI). It caused too many number of withInternalLabels()'s parameters
In my opinion, to keep up with it, if the number of retrun values of setPlatformOptions() increase, it would be difficult to maintain the code quality.
Therefore, I suggest adding addPlatformContainerOptions().

2. add `PIDContainer` label

Thanks, but we need to deal with shared container restarts, it will change pid namesapce.

For --pid flag's persistency, save the based container's id into the shared container's label.
When the container restart, reconfigPIDContainer() will recover it in startContainer()

a difference with docker

Because docker supports to isolate a container with a user namespace, it should share user namespace between containers when containers share their pid namespace.
- docker docs
- moby/moby/daemon/oci_linux.go#L231-L240
But, nerdctl does not support the user ns isolation.

references

cmd/nerdctl/run_linux_test.go

cmd/nerdctl/run_linux.go

junnplus

Thanks, but we need to deal with shared container restarts, it will change pid namesapce.

And please update the doc.

junnplus · 2022-10-04T16:32:41Z

Could I get an usecase for it? I could not find it because of my little knowledge.
In my opinion, if containers restart, their namespaces should be reset, isn't it?

You can test restart the base container and then restart the shared container.

cmd/nerdctl/run_linux.go

minuk-dev · 2022-10-10T12:15:03Z

Could I get an usecase for it? I could not find it because of my little knowledge.
In my opinion, if containers restart, their namespaces should be reset, isn't it?

You can test restart the base container and then restart the shared container.

I fixed it using labels at 7a6d425.

Summary of updates:

add setPlatformContainerOptions() at 82e3d04
add reconfigPIDContainer()
add PIDContainer label
test this feature in TestRestartPIDContainer.

Can you review it again? @junnplus

trivial: I updated the doc of pr. It is marked as updated.

cmd/nerdctl/restart_linux_test.go

cmd/nerdctl/run_linux.go

junnplus · 2022-10-11T07:29:43Z

cmd/nerdctl/run_linux.go

+	}
+
+	containerName := parsed[1]
+	walker := &containerwalker.ContainerWalker{


Why do we need to find again?

junnplus · 2022-10-11T07:32:14Z

cmd/nerdctl/run_linux.go

+			cOpts = append(cOpts, containerd.WithAdditionalContainerLabels(map[string]string{
+				labels.PIDContainer: containerName,
+			}))


We already have withInternalLabels, should be put together

When a new label is needed, the best way is to add a new parameter of withInternalLabels()?
And if so, --pid flags is processed in setPlatformOptions(). In order to pass into the withInternalLabels(), the number of setPlatformOptions()'s return value should be increased.

When a new label is needed, the best way is to add a new parameter of withInternalLabels()?

Uniformity is better than fragmentation.

And if so, --pid flags is processed in setPlatformOptions().

I think the --pid flags should be platform independent, why not consider moving it out of setPlatformOptions?

Previsouly, --pid only works on linux.

nerdctl/cmd/nerdctl/run_linux.go

Lines 135 to 144 in c6517c7

pidNs = strings.ToLower(pidNs)

if pidNs != "" {

if pidNs != "host" {

return nil, fmt.Errorf("invalid pid namespace. Set --pid=host to enable host pid namespace")

} else {

opts = append(opts, oci.WithHostNamespace(specs.PIDNamespace))

if rootlessutil.IsRootless() {

opts = append(opts, withBindMountHostProcfs)

}

}

The reason, why it only works on the linux, is that it's too sensitive with namespace suppport.

nerdctl/cmd/nerdctl/run.go

Lines 1184 to 1187 in d15ada7

ns := specs.LinuxNamespace{

Type: specs.PIDNamespace,

Path: fmt.Sprintf("/proc/%d/ns/pid", task.Pid()),

}

// WithHostNamespace allows a task to run inside the host's linux namespace func WithHostNamespace(ns specs.LinuxNamespaceType) SpecOpts { return func(_ context.Context, _ Client, _ *containers.Container, s *Spec) error { setLinux(s) for i, n := range s.Linux.Namespaces { if n.Type == ns { s.Linux.Namespaces = append(s.Linux.Namespaces[:i], s.Linux.Namespaces[i+1:]...) return nil } } return nil } }

To avoid a compile error, I placed reconfigPIDContainer() in cmd/nerdctl/start.go but I think it caused confusing.
So, I added a condition to check runtime os now.

nerdctl/cmd/nerdctl/start.go

Line 231 in 8680bdb

if runtime.GOOS != "linux" {

Anyway, in my opinion, --pid currently can work on linux only.
Because of it, --pid should be processed in setPlatformOptions(). But, I really agree with you. setPlatformContainerOptions() can make a fragmentation and cause to find a target container twice.
I tried to find a better way to improve it according to your review, but I couldn't. I'm really sorry.

I think the --pid flags should be platform independent, why not consider moving it out of setPlatformOptions?

And I don't understand how to move it out?

The PID namespace (as well as other namespaces) is specific to Linux.
I don't think it will ever be platform-independent in the foreseeable future.

junnplus

Please squash commits

AkihiroSuda

Thanks

minuk-dev · 2022-10-18T08:11:14Z

Thanks

I'm sorry that I didn't apply reviews from jun. I'm looking forward the better way than setplatformcontaineropts().
In my opinion, that's too hard to me now.
So, could you decide how to deal with this pr (e.g. because of low code quality, just close).
If pr is closed, then I'll try to improve the quality and retry pr.

junnplus

Sorry, no LGTM, I guess the code can be optimized again.

junnplus · 2022-10-18T08:55:58Z

cmd/nerdctl/run_linux.go

+			cOpts = append(cOpts, containerd.WithAdditionalContainerLabels(map[string]string{
+				labels.PIDContainer: containerName,
+			}))


And if so, --pid flags is processed in setPlatformOptions().

I think the --pid flags should be platform independent, why not consider moving it out of setPlatformOptions?

- Add context and client to `setPlatformOptions()` - Add branch for platform-specific container opts - Add PIDContainer label into container's labels. It only works on linux platform. - Add reconfigPIDContainer() into container's start process. It recovers the pid namespace from its PIDContainer label. Signed-off-by: Min Uk Lee <[email protected]>

AkihiroSuda · 2022-10-21T00:37:31Z

@junnplus LGTY?

AkihiroSuda · 2022-10-21T12:15:27Z

Let me merge this, as I don't think the --pid flag can be platform-independent in the foreseeable future, but please feel free to submit follow-up issues/PRs.

Thank you @minuk-dev @junnplus 👍

junnplus reviewed Oct 4, 2022

View reviewed changes

cmd/nerdctl/run_linux_test.go Outdated Show resolved Hide resolved

junnplus reviewed Oct 4, 2022

View reviewed changes

cmd/nerdctl/run_linux.go Outdated Show resolved Hide resolved

junnplus reviewed Oct 4, 2022

View reviewed changes

cmd/nerdctl/run_linux.go Outdated Show resolved Hide resolved

junnplus suggested changes Oct 4, 2022

View reviewed changes

This comment was marked as resolved.

Sign in to view

fahedouch reviewed Oct 5, 2022

View reviewed changes

cmd/nerdctl/run_linux.go Outdated Show resolved Hide resolved

cmd/nerdctl/run_linux.go Outdated Show resolved Hide resolved

cmd/nerdctl/run_linux.go Outdated Show resolved Hide resolved

AkihiroSuda added this to the v0.23.1 milestone Oct 6, 2022

minuk-dev changed the title ~~support --pid=container:xxx for nerdctl run cmd~~ WIP: support --pid=container:xxx for nerdctl run cmd Oct 9, 2022

minuk-dev requested a review from junnplus October 10, 2022 12:15

minuk-dev changed the title ~~WIP: support --pid=container:xxx for nerdctl run cmd~~ Support --pid=container:xxx for nerdctl run cmd Oct 10, 2022

fahedouch self-requested a review October 10, 2022 12:21

junnplus reviewed Oct 11, 2022

View reviewed changes

cmd/nerdctl/restart_linux_test.go Outdated Show resolved Hide resolved

junnplus reviewed Oct 11, 2022

View reviewed changes

cmd/nerdctl/run_linux.go Outdated Show resolved Hide resolved

junnplus reviewed Oct 11, 2022

View reviewed changes

cmd/nerdctl/run_linux.go

}

containerName := parsed[1]

walker := &containerwalker.ContainerWalker{

Copy link

Member

junnplus Oct 11, 2022

Choose a reason for hiding this comment

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

Why do we need to find again?

junnplus reviewed Oct 11, 2022

View reviewed changes

junnplus suggested changes Oct 11, 2022

View reviewed changes

AkihiroSuda requested a review from junnplus October 16, 2022 11:32

AkihiroSuda approved these changes Oct 18, 2022

View reviewed changes

junnplus suggested changes Oct 18, 2022

View reviewed changes

AkihiroSuda requested a review from junnplus October 19, 2022 02:35

AkihiroSuda merged commit 12e3e4e into containerd:master Oct 21, 2022

minuk-dev mentioned this pull request Nov 22, 2022

Refactor internalLabels #1537

Merged

minuk-dev mentioned this pull request Feb 5, 2024

nerdctl run not support --pid=container:xxxx and --ipc=container:xxx but docker is supported #1293

Closed

	pidNs = strings.ToLower(pidNs)
	if pidNs != "" {
	if pidNs != "host" {
	return nil, fmt.Errorf("invalid pid namespace. Set --pid=host to enable host pid namespace")
	} else {
	opts = append(opts, oci.WithHostNamespace(specs.PIDNamespace))
	if rootlessutil.IsRootless() {
	opts = append(opts, withBindMountHostProcfs)
	}
	}

	ns := specs.LinuxNamespace{
	Type: specs.PIDNamespace,
	Path: fmt.Sprintf("/proc/%d/ns/pid", task.Pid()),
	}

Support --pid=container:xxx for nerdctl run cmd #1411

Support --pid=container:xxx for nerdctl run cmd #1411

Uh oh!

Conversation

minuk-dev commented Oct 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Details

1. add setPlatformContainerOptions()

2. add PIDContainer label

a difference with docker

references

Uh oh!

Uh oh!

Uh oh!

Uh oh!

junnplus left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

junnplus commented Oct 4, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

minuk-dev commented Oct 10, 2022

Uh oh!

Uh oh!

Uh oh!

junnplus Oct 11, 2022

Choose a reason for hiding this comment

Uh oh!

junnplus Oct 11, 2022

Choose a reason for hiding this comment

Uh oh!

minuk-dev Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

junnplus Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

junnplus Oct 18, 2022

Choose a reason for hiding this comment

Uh oh!

minuk-dev Oct 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Oct 21, 2022

Choose a reason for hiding this comment

Uh oh!

junnplus left a comment

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

minuk-dev commented Oct 18, 2022

Uh oh!

junnplus left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junnplus Oct 18, 2022

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Oct 21, 2022

Uh oh!

AkihiroSuda commented Oct 21, 2022

Uh oh!

Uh oh!

Support --pid=container:xxx for `nerdctl run` cmd #1411

Support --pid=container:xxx for `nerdctl run` cmd #1411

minuk-dev commented Oct 4, 2022 •

edited

Loading

1. add `setPlatformContainerOptions()`

2. add `PIDContainer` label

junnplus left a comment •

edited

Loading

minuk-dev Oct 18, 2022 •

edited

Loading

junnplus left a comment •

edited

Loading