Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pause process should only be started if pid namespace is specified #1066

Open
haircommander opened this issue Jan 31, 2023 · 5 comments
Open

Comments

@haircommander
Copy link
Collaborator

It's possible we do this now, but the pause process should only be created and tracked if the pod has a pod level pid namespace. Otherwise, we're needlessly creating the infra container again (with less overhead, but still).

@saschagrunert
Copy link
Member

saschagrunert commented Feb 1, 2023

@haircommander
Copy link
Collaborator Author

we could, though the code could be simpler if we didn't need to differentiate there, and only needed to in conmon-rs. then, we could actually move away from pinns and have conmon-rs create namespaces unconditionally for the pod runtime

@saschagrunert
Copy link
Member

@haircommander in CRI-O, the check would look like:

sb.NamespaceOptions().GetPid() == types.NamespaceMode_POD

Should we pass the sb.NamespaceOptions().GetPid() to the conmon-rs client and no-op if the above condition is met? Or how should that look like interface wise?

@haircommander
Copy link
Collaborator Author

conmon-rs already knows what the pid namespace will be because it's created if the user specifies pid in the RPC call. Even if we're bind mounting the pid namespace from the host, there's already a process holding it open (systemd) so we don't need to spawn a pause process. We only need the pause process if the user requested a pod level pid namespace

@haircommander
Copy link
Collaborator Author

so it is my thinking that we conditionally create the pause process, but then in cri-o always call into conmon-rs to create our namespaces for the pod.

saschagrunert added a commit to saschagrunert/conmon-rs that referenced this issue Feb 6, 2023
We do not have to create the pause process on `CreateNamespaces` if no
PID namespace should be unshared. In this case we now return a dedicated
error and let the users decide what to do with it.

Fixes containers#1066

Signed-off-by: Sascha Grunert <[email protected]>
saschagrunert added a commit to saschagrunert/conmon-rs that referenced this issue Feb 6, 2023
We do not have to create the pause process on `CreateNamespaces` if no
PID namespace should be unshared. In this case we now return a dedicated
error and let the users decide what to do with it.

Fixes containers#1066

Signed-off-by: Sascha Grunert <[email protected]>
saschagrunert added a commit to saschagrunert/conmon-rs that referenced this issue Feb 6, 2023
We do not have to create the pause process on `CreateNamespaces` if no
PID namespace should be unshared. In this case we now return a dedicated
error and let the users decide what to do with it.

Fixes containers#1066

Signed-off-by: Sascha Grunert <[email protected]>
saschagrunert added a commit to saschagrunert/conmon-rs that referenced this issue Feb 6, 2023
We do not have to create the pause process on `CreateNamespaces` if no
PID namespace should be unshared. In this case we now return a dedicated
error and let the users decide what to do with it.

Fixes containers#1066

Signed-off-by: Sascha Grunert <[email protected]>
saschagrunert added a commit to saschagrunert/conmon-rs that referenced this issue Feb 7, 2023
We do not have to create the pause process on `CreateNamespaces` if no
PID namespace should be unshared. In this case we now return a dedicated
error and let the users decide what to do with it.

Fixes containers#1066

Signed-off-by: Sascha Grunert <[email protected]>
saschagrunert added a commit to saschagrunert/conmon-rs that referenced this issue Feb 7, 2023
We do not have to create the pause process on `CreateNamespaces` if no
PID namespace should be unshared. In this case we now return a dedicated
error and let the users decide what to do with it.

Fixes containers#1066

Signed-off-by: Sascha Grunert <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants