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

[minuk-dev]

#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

• moby/moby/daemon/oci_linux.go#L294-L321
• man pages - pid_namespaces

[minuk-dev]

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

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?

Here are my manual tests. In these cases, docker and nerdctl work the same.

• restart base container test

$ docker run -d ubuntu sleep infinity # run a base container
9bd0d57b85dcbbe7eeabf72f867bb130ddaf35ca716b80f15f6612b92705124f

$ docker run --pid=container:9bd0d -d ubuntu sleep infinity # run shared container
cfe162ae92814624e44f06db81ecba3f016508a72708ee917ad9e60fe0fe663c

$ docker ps # check containers' status
CONTAINER ID   IMAGE     COMMAND            CREATED          STATUS          PORTS     NAMES
cfe162ae9281   ubuntu    "sleep infinity"   7 seconds ago    Up 6 seconds              gallant_galois
9bd0d57b85dc   ubuntu    "sleep infinity"   45 seconds ago   Up 44 seconds             peaceful_mendel

$ docker restart 9bd0d # restart the base container
9bd0d

$ docker ps # only base container survived
CONTAINER ID   IMAGE     COMMAND            CREATED              STATUS        PORTS     NAMES
9bd0d57b85dc   ubuntu    "sleep infinity"   About a minute ago   Up 1 second             peaceful_mendel

• restart shared container test

$ docker run -d ubuntu sleep infinity # run a base container
57f18c82ab5ebbf49d6adbded478abf85d71172ee06de5e0fb88acf552493957

$ docker run --pid=container:57f18 -d ubuntu sleep infinity # run a shared container
9de0f487563cb709f0065aecc6a02c4eccb2c2c5089ad5bcf885250ac993a4a1

$ docker restart 9de0f # restart the shared container
9de0f

$ docker ps 
CONTAINER ID   IMAGE     COMMAND            CREATED          STATUS         PORTS     NAMES
9de0f487563c   ubuntu    "sleep infinity"   16 seconds ago   Up 2 seconds             gracious_dewdney
57f18c82ab5e   ubuntu    "sleep infinity"   2 minutes ago    Up 2 minutes             beautiful_almeida

# all survived and keep sharing.
$ docker exec -it 9de0f bash
root@9de0f487563c:/# ps -ef
UID          PID    PPID  C STIME TTY          TIME CMD
root           1       0  0 15:21 ?        00:00:00 sleep infinity
root          13       0  0 15:24 ?        00:00:00 sleep infinity
root          36       0  0 16:08 pts/0    00:00:00 bash
root          44      36  0 16:08 pts/0    00:00:00 ps -ef

nit:

And please update the doc.

I updated. Thank you very much for your review.

[junnplus]

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.

[minuk-dev]

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 7a6d4250619c76eda7d2d2d748d9cf2fbc24313e.

Summary of updates:

• add setPlatformContainerOptions() at 82e3d04723bc3bd86f7dd075da5ff091e6432022
• 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.

[minuk-dev]

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.

[AkihiroSuda]

@junnplus LGTY?

[AkihiroSuda]

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 👍