Add support for ipc namespace modes "none, private, sharable" #13583

rhatdan · 2022-03-21T18:17:50Z

Signed-off-by: Daniel J Walsh [email protected]

rhatdan · 2022-03-21T18:19:10Z

@mheon Currently we don't record the IPCMode of a container, but in order to get --ipcmod=private to work properly, we need to check if the container we are trying to share IPCMode with is marked as private, and then reject the connection.

How would you like me to record this information? Should we just add it to the config?

rhatdan · 2022-03-21T20:49:54Z

Requires: containers/common#972

mheon · 2022-03-21T22:34:13Z

Why do we need to record it? For other namespaces we just reverse-engineer it from the OCI spec

rhatdan · 2022-03-22T10:47:51Z

I don't think they can regenerate it from the spec. "private" means that the container can not be shared with another container. There is nothing in the spec to indicate this as different then "sharable".

Luap99 · 2022-03-22T11:08:59Z

pkg/specgen/generate/namespaces.go

@@ -134,9 +135,13 @@ func namespaceOptions(ctx context.Context, s *specgen.SpecGenerator, rt *libpod.
 		if err != nil {
 			return nil, errors.Wrapf(err, "error looking up container to share ipc namespace with")
 		}
+		if ipcCtr.Config().IPCMode.IsPrivate() {


Should use .ConfigNoCopy() or however this function was called.

mheon · 2022-03-22T13:16:51Z

@rhatdan If the only thing we're worried about is shareable IPC, I think a *bool would be good? Nil = use what's in the OCI spec, true = create a shareable private IPC namespace, false = create a non-shareable private IPC namespace.

mheon · 2022-03-22T13:22:31Z

Actually, I wonder if we even need the 3 states. Just a bool is probably file - false will be "use what was set in the OCI spec", true will be "configure a shareable, private IPC namespace"/

rhatdan · 2022-03-22T14:06:46Z

@mheon We care about whether the IPC Mode was none as well. Sharable, Private, None. I think we end up being Mode, so I am not sure why we don't just record it.

mheon · 2022-03-22T14:57:43Z

None is easy, just check that the bool is false (not creating shareable IPC) and the IPC namespace is missing from the OCI spec

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

TomSweeneyRedHat · 2022-03-22T23:09:16Z

docs/source/markdown/podman-run.1.md

 - **ns:**_path_: path to an IPC namespace to join.
+- **private**: private IPC namespace.
+= **shareable**: private IPC with a possibility to share it with  other containers.


rhatdan · 2022-03-24T17:35:51Z

@mheon None does not mean don't create the IPC namespace, it means create the ipc namespace but don't mount tmpfs on /dev/shm. --ipc=host means don't create the ipc namespace. Thus we have to save the mode.

mheon · 2022-03-24T17:38:41Z

Then we probably want a separate "NoCreateSHM" bool. I really do not want to store the literal mode in the DB, we did that with network and it has not worked out well for us - string-comparing against a user-provided string in the DB to determine what mode we are in is complicated.

rhatdan · 2022-03-24T17:55:49Z

Do you want noshareshm, which would indicate what we are doing. We still could not tell the difference in the podman contanier inspect though. Or do you want two booleans.

mheon · 2022-03-24T18:08:19Z

@rhatdan Two bools, one for "do not create any SHM", one for "create a shareable SHM"

giuseppe

/lgtm

openshift-ci · 2022-04-08T14:54:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, 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~~ [giuseppe,rhatdan]

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

giuseppe · 2022-04-08T14:54:36Z

/hold

TomSweeneyRedHat · 2022-04-08T15:21:18Z

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

+a private IPC namespace.
+
+- "": Use Podman's default, defined in containers.conf.
+- **container:**_id_: reuses another container's shared memory, semaphores, and message queues


nit

Suggested change

- **container:**_id_: reuses another container's shared memory, semaphores, and message queues

- **container:**_id_: reuses another container's shared memory, semaphores, and message queues.

TomSweeneyRedHat · 2022-04-08T15:21:39Z

docs/source/markdown/podman-run.1.md

@@ -528,9 +528,13 @@ To specify multiple static IPv6 addresses per container, set multiple networks u
 Set the IPC namespace mode for a container. The default is to create
 a private IPC namespace.

+- "": Use Podman's default, defined in containers.conf.
 - **container:**_id_: reuses another container shared memory, semaphores and message queues


same period nit

TomSweeneyRedHat · 2022-04-08T15:26:51Z

A couple of missing periods that can be addressed later. LGTM, but I would like a head nod from @mheon as he had a couple of comments in previous passes.

mheon · 2022-04-08T15:39:25Z

Will review after lunch

mheon · 2022-04-12T17:10:44Z

libpod/container_inspect.go

-	} else if ctrSpec.Linux != nil {
+	switch {
+	case !c.config.ShmShare:
+		hostConfig.IpcMode = "private"


Won't this be true for ipc=container: as well?

ShmShare would be set for them.

mheon · 2022-04-12T17:11:48Z

libpod/container_config.go

+	// ShmShare indicates whether /dev/shm can be shared with other containers
+	ShmShare bool `json:"ShmShare,omitempty"`
+	// ShmMount indicates whether a tmpfs should be created and mounted on  /dev/shm
+	ShmMount bool `json:"ShmMount,omitempty"`


This could break old containers, which won't have this set to true - maybe NoShmMount instead?

@mheon How about now?

Fixes: containers#13265 Signed-off-by: Daniel J Walsh <[email protected]>

mheon · 2022-04-16T16:28:21Z

/lgtm
/hold cancel

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 21, 2022

rhatdan force-pushed the ipc branch from bcd4054 to 9983dc1 Compare March 21, 2022 20:37

rhatdan changed the title ~~Add support for ipc namespace modes "none, private, sharable"~~ [wip] Add support for ipc namespace modes "none, private, sharable" Mar 21, 2022

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 Mar 21, 2022

Luap99 reviewed Mar 22, 2022

View reviewed changes

rhatdan force-pushed the ipc branch from 9983dc1 to 70fe929 Compare March 22, 2022 11:28

rhatdan force-pushed the ipc branch from 70fe929 to 542bd54 Compare March 22, 2022 14:03

rhatdan changed the title ~~[wip] Add support for ipc namespace modes "none, private, sharable"~~ Add support for ipc namespace modes "none, private, sharable" Mar 22, 2022

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 22, 2022

rhatdan force-pushed the ipc branch from 542bd54 to 70528cd Compare March 22, 2022 14:09

TomSweeneyRedHat reviewed Mar 22, 2022

View reviewed changes

docs/source/markdown/podman-create.1.md Outdated Show resolved Hide resolved

TomSweeneyRedHat reviewed Mar 22, 2022

View reviewed changes

docs/source/markdown/podman-create.1.md Outdated Show resolved Hide resolved

TomSweeneyRedHat reviewed Mar 22, 2022

View reviewed changes

docs/source/markdown/podman-create.1.md Outdated Show resolved Hide resolved

TomSweeneyRedHat reviewed Mar 22, 2022

View reviewed changes

rhatdan force-pushed the ipc branch 2 times, most recently from 1e12746 to f3c7883 Compare March 23, 2022 17:13

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 28, 2022

rhatdan force-pushed the ipc branch from f3c7883 to db58434 Compare March 28, 2022 21:09

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 28, 2022

rhatdan force-pushed the ipc branch 3 times, most recently from 85bf788 to 3478fe3 Compare March 30, 2022 13:05

rhatdan force-pushed the ipc branch from 3478fe3 to 43446d0 Compare April 5, 2022 17:50

giuseppe approved these changes Apr 8, 2022

View reviewed changes

openshift-ci bot assigned giuseppe Apr 8, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 8, 2022

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 8, 2022

TomSweeneyRedHat reviewed Apr 8, 2022

View reviewed changes

mheon reviewed Apr 12, 2022

View reviewed changes

Add support for ipc namespace modes "none, private, sharable"

3987c52

Fixes: containers#13265 Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan force-pushed the ipc branch from 43446d0 to 3987c52 Compare April 12, 2022 17:35

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 12, 2022

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 16, 2022

openshift-ci bot assigned mheon Apr 16, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 16, 2022

openshift-merge-robot merged commit 8d3075e into containers:main Apr 16, 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

Add support for ipc namespace modes "none, private, sharable" #13583

Add support for ipc namespace modes "none, private, sharable" #13583

rhatdan commented Mar 21, 2022

rhatdan commented Mar 21, 2022

rhatdan commented Mar 21, 2022

mheon commented Mar 21, 2022

rhatdan commented Mar 22, 2022

Luap99 Mar 22, 2022

mheon commented Mar 22, 2022

mheon commented Mar 22, 2022

rhatdan commented Mar 22, 2022

mheon commented Mar 22, 2022

TomSweeneyRedHat Mar 22, 2022

rhatdan commented Mar 24, 2022

mheon commented Mar 24, 2022

rhatdan commented Mar 24, 2022

mheon commented Mar 24, 2022

giuseppe left a comment

openshift-ci bot commented Apr 8, 2022

giuseppe commented Apr 8, 2022

TomSweeneyRedHat Apr 8, 2022

TomSweeneyRedHat Apr 8, 2022

TomSweeneyRedHat commented Apr 8, 2022

mheon commented Apr 8, 2022

mheon Apr 12, 2022

rhatdan Apr 12, 2022

mheon Apr 12, 2022

rhatdan Apr 13, 2022

mheon commented Apr 16, 2022

	- container:_id_: reuses another container's shared memory, semaphores, and message queues
	- container:_id_: reuses another container's shared memory, semaphores, and message queues.

Add support for ipc namespace modes "none, private, sharable" #13583

Add support for ipc namespace modes "none, private, sharable" #13583

Conversation

rhatdan commented Mar 21, 2022

rhatdan commented Mar 21, 2022

rhatdan commented Mar 21, 2022

mheon commented Mar 21, 2022

rhatdan commented Mar 22, 2022

Choose a reason for hiding this comment

mheon commented Mar 22, 2022

mheon commented Mar 22, 2022

rhatdan commented Mar 22, 2022

mheon commented Mar 22, 2022

Choose a reason for hiding this comment

rhatdan commented Mar 24, 2022

mheon commented Mar 24, 2022

rhatdan commented Mar 24, 2022

mheon commented Mar 24, 2022

giuseppe left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Apr 8, 2022

giuseppe commented Apr 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomSweeneyRedHat commented Apr 8, 2022

mheon commented Apr 8, 2022

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

mheon commented Apr 16, 2022