Add warnings for cgroup related options (--cgroup*) when --pod or --pod-id-file is set

[ruihe774]

Does this PR introduce a user-facing change?

Added warnings for cgroup related options (`--cgroup*`) when `--pod` or `--pod-id-file` is set.

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ruihe774
Once this PR has been reviewed and has the lgtm label, please assign rhatdan for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

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

[ruihe774]

cc @rhatdan

[ruihe774]

Giving setting --userns and --pod together does not cause serious problem, I have changed my mind to make it a warning instead of error.

[mheon]

I thought there was a reason the two were incompatible... @giuseppe @Luap99 Do you remember what it was?

[giuseppe]

we must use the same user namespace for all the containers in a pod, otherwise we can't share namespaces.

[ruihe774]

we must use the same user namespace for all the containers in a pod, otherwise we can't share namespaces.

I cannot find this guarantee in doc. With different user namespaces the pod do not crash and runs well. I think a warning is enough. Also, it's difficult to ensure this guarantee.

[giuseppe]

we must use the same user namespace for all the containers in a pod, otherwise we can't share namespaces.

I cannot find this guarantee in doc. With different user namespaces the pod do not crash and runs well. I think a warning is enough. Also, it's difficult to ensure this guarantee.

if you don't share any namespace, why would you need to use a pod?

[rhatdan]

Using a Pod allows you to treat the service as a single application. I don't see a requirement to share namespaces.

[giuseppe]

Using a Pod allows you to treat the service as a single application. I don't see a requirement to share namespaces.

in that case we need to enchance the check and verify that the list of shared namespaces is empty. By default pods share the ipc, net, and uts namespaces, so something like: podman pod create foo && podman run --pod foo --userns ... won't work

[rhatdan]

Yes I think you would need to change the namespaces shared on the created pod to "".

[ruihe774]

Using a Pod allows you to treat the service as a single application. I don't see a requirement to share namespaces.

in that case we need to enchance the check and verify that the list of shared namespaces is empty. By default pods share the ipc, net, and uts namespaces, so something like: podman pod create foo && podman run --pod foo --userns ... won't work

I think it is too complicated to implement such checks. And IMO sharing these namespaces or not is orthogonal to each other.

[Luap99]

Doing these checks in the front end is fundamentally flawed. We simply do not have enough knowledge there to know what namespaces are used together and what conflicts,etc... These checks really must be moved into specgen instead, because cli level checks do not cover the API.

And overall I agree that we should not throw warnings if the namespace is not shared in the first place, this would be easy to know in specgen.

I cannot find this guarantee in doc. With different user namespaces the pod do not crash and runs well

Well if you do not care about the "owning" user namespaces sure. User namespaces are special in terms of how they work with capabilities and permissions in general, i.e. https://blog.podman.io/2023/12/interaction-between-user-namespaces-and-capabilities/
So yes it can work thus I agree the hard error always seemed a bit harsh but on the other side for most users they will not understand these details which will lead to confusing issues.

[ruihe774]

Doing these checks in the front end is fundamentally flawed. We simply do not have enough knowledge there to know what namespaces are used together and what conflicts,etc... These checks really must be moved into specgen instead, because cli level checks do not cover the API.

And overall I agree that we should not throw warnings if the namespace is not shared in the first place, this would be easy to know in specgen.

Sorry but I'm not familiar with the codebase. Making such change is too difficult for me. Maybe I shall drop the userns commit out of this PR and only keep the cgroups one. Hope that someone can work on that.

[Luap99]

IMO this is still the wrong place to throw such a warning even for cgroups.

If the pod was created without any cgroup limits then it now always throws warnings to users that didn't need it to begin with. I think structurally it is important to only throw warnings when they are actual warnings, i.e. unexpected things.
Just consider than any quadlet unit using pods will not throw these warnings by default even when no limit was set on the pod which just doesn't make sense for users.

Overall we can only check these things in specgen. The current solution will not work for any API user, it will not work when defaults are set via config file as you only check the flags.

[ruihe774]

Overall we can only check these things in specgen. The current solution will not work for any API user, it will not work when defaults are set via config file as you only check the flags.

The checks were orignally placed here. I just extended them. Despite that, I agree with you that they should be moved into specgen, which is out of my knowledge.

If the pod was created without any cgroup limits then it now always throws warnings to users that didn't need it to begin with. I think structurally it is important to only throw warnings when they are actual warnings, i.e. unexpected things.

I do not agree. IMO users expect cgroups of containers are descendants of the cgroup of the pod.