Kube Play: Add argument for default pod spec

[ygalblum]

The new argument pod-defaults is a path to a Pod Spec file for Podman to use to set default values for keys that were not set in the provided YAML file

Does this PR introduce a user-facing change?

Kube Play: Add the argument pod-defaults for default pod spec to allow the caller to set default values for fields that are not set in the manifest file

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ygalblum
Once this PR has been reviewed and has the lgtm label, please assign rhatdan for approval by writing /assign @rhatdan in a comment. 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

[ygalblum]

@alexlarsson @rhatdan @vrothberg following up on my comment in the --read-only PR. This is how I envisioned it.
Currently, the code only takes the annotations from the default pod spec and applies the ones in the actual pod spec and the ones passed as arguments. But, the idea is to do the same for the entire Pod spec (and also to the Deployment spec). This will provide the means to set the default value of any field without having to specify an argument for every field
WDYT?

[vrothberg]

In general, I am a fan of discussing designs in issues not in PRs. Looking at code can lead to focusing on the code instead of the problem.

Is it accurate to rephrase the problem to: How can we define global defaults for kube play?

[ygalblum]

Sure, I want ever you prefer is fine by me. Will you open a discussion, or do you want me?

Is it accurate to rephrase the problem to: How can we define global defaults for kube play?

Yes, I think so.

[vrothberg]

Sure, I want ever you prefer is fine by me. Will you open a discussion, or do you want me?

We can continue the conversation here.

Is it accurate to rephrase the problem to: How can we define global defaults for kube play?

Yes, I think so.

I think defaults should go into containers.conf. We can already control a huge number of knobs and attributes there. If there's something missing for the automotive use case, I think we should consider that first.

[ygalblum]

I understand your point about containers.conf. But, this means that the defaults are at the system level whereas the argument option sets the default at each process level. I don't know what's the "right" level, but I think it is worth noting it.

Furthermore, this work started from Quadlet which is a little more opinionated. Having the defaults in containers.conf means that it is up to the user to set the desired values.

[vrothberg]

I understand your point about containers.conf. But, this means that the defaults are at the system level whereas the argument option sets the default at each process level. I don't know what's the "right" level, but I think it is worth noting it.

Individual processes can be pointed to a containers.conf via an environment variable (CONTAINERS_CONF=...).

Furthermore, this work started from Quadlet which is a little more opinionated.

There's a risk in getting too opinionated. There's much work going into the auto use cases and I want to enable such features for as many users as possible. At the present, I think that containers.conf can address your needs. If some knobs are missing, we can add them to containers.conf and all users benefit.

[rhatdan]

I agree this should go into the containers.conf. in github.com/containers/common

Although I am not sure we can easily specify a pod.spec in yaml?

[alexlarsson]

I don't understand this focus on containers.conf. I didn't look at the code (yet), but the basic requirement from quadlet (which led to this PR) is that we have a pod, which may define multiple containers, and we want to set some default values for container properties for the containers in the pod.

For example, suppose we want to set the default value of the io.containers.sdnotify property to "conmon", such that any one specific container in the pod can override this to "container", but for the other containers in the pod we use "conmon".

[alexlarsson]

And, for quadlet, these would generally not be global defaults, but for that particular run. Other podman play (for example outside the quadlet generated service files) should not apply this default value for io.containers.sdnotify.

[vrothberg]

I don't understand this focus on containers.conf. I didn't look at the code (yet), but the basic requirement from quadlet (which led to this PR) is that we have a pod, which may define multiple containers, [...]

[...] and we want to set some default values for container properties for the containers in the pod.

That is a nice description of what containers.conf can do, among other things.

For example, suppose we want to set the default value of the io.containers.sdnotify property to "conmon", such that any one specific container in the pod can override this to "container", but for the other containers in the pod we use "conmon".

That is already supported in the K8s YAML. There's no knob in containers.conf though but that can easily be added.

I want to encourage to open issues before PRs. As mentioned above, I want to generalize features in way they're usable beyond the initially motivating use case, if possible.

[alexlarsson]

For example, suppose we want to set the default value of the io.containers.sdnotify property to "conmon", such that any one specific container in the pod can override this to "container", but for the other containers in the pod we use "conmon".

That is already supported in the K8s YAML. There's no knob in containers.conf though but that can easily be added.

I think there is some miscommunication here. Lemme take a step back and restate the problem.

Suppose quadlet wants to generate a service file that starts a pod based on some yaml. To integrate well with systemd it wants to set some particular options, such as io.podman.sdnotify by default (because most k8s users would not even know that they shouls set it). However, we still want to allow the user to override the option in one or more containers in the pod. Thus, we want the ability to change the default of some container options in this particular run (i.e. not globally) of podman play.

Changing containers.conf to set a global io.podman.sdnotify value is not a good idea, because instances of play kube outside a systemd service file should not use that value.

Another example option we might want to set is imagePullPolicy. The non-kube quadlet services always use --pull=never. We probably would like to similarly set imagePullPolicy to default to false when starting the pod from the service file. Again, this is not something you want for a global property though.

[vrothberg]

Suppose quadlet wants to generate a service file that starts a pod based on some yaml. To integrate well with systemd it wants to set some particular options, such as io.podman.sdnotify by default (because most k8s users would not even know that they shouls set it).

It's probably just an example but why would Quadlet need to tweak the sdnotify policy? If the workload needs something other than the default, the workload needs to support it, so changing it automagically won't work.

Changing containers.conf to set a global io.podman.sdnotify value is not a good idea, because instances of play kube outside a systemd service file should not use that value.

As mentioned above, there's a CONTAINERS_CONF env variable which won't apply globally.

Another example option we might want to set is imagePullPolicy. The non-kube quadlet services always use --pull=never. We probably would like to similarly set imagePullPolicy to default to false when starting the pod from the service file. Again, this is not something you want for a global property though.

What I see is that we will reimplement parts of containers.conf for one specific use case. But all this stuff has to be maintained.

I'd love to see a list of concrete requirements and use cases. Then we can go through them one-by-one but I am afraid of deciding for the big hammer too quickly.

[alexlarsson]

It's probably just an example but why would Quadlet need to tweak the sdnotify policy? If the workload needs something other than the default, the workload needs to support it, so changing it automagically won't work.

I think we discussed this before, and it turns out that the current default values for sdnotify will work fine for the case of running from a systemd file. Essentially the default if we do nothing is default=conmon. This means it is always safe for quadlet to generate a service file with Type=notify, as we're guaranteed that there will be a notify. This is not the case for regular "podman run", where quadlet needs to pass in --sdnotify=conmon.

So, earlier in the PR i was using sdnotify as an example of something behaving like this, even though its not needed, because I don't offhand know of a better example.

Changing containers.conf to set a global io.podman.sdnotify value is not a good idea, because instances of play kube outside a systemd service file should not use that value.

As mentioned above, there's a CONTAINERS_CONF env variable which won't apply globally.

But the goal is not to override all containers.conf options in the system, just the ones needed?

Another example option we might want to set is imagePullPolicy. The non-kube quadlet services always use --pull=never. We probably would like to similarly set imagePullPolicy to default to false when starting the pod from the service file. Again, this is not something you want for a global property though.

What I see is that we will reimplement parts of containers.conf for one specific use case. But all this stuff has to be maintained.

I'd love to see a list of concrete requirements and use cases. Then we can go through them one-by-one but I am afraid of deciding for the big hammer too quickly.

Ok, lets drop this for now and look at the actual usecases that are needed for quadlet. Then we can revisit the generic idea if needed, or add more specialized overrides.

[vrothberg]

But the goal is not to override all containers.conf options in the system, just the ones needed?

That's a fair concern. CONTAINERS_CONF will ignore /usr/share/container if I recall correctly. containers.conf supports drop-in configs, so we could add a CONTAINERS_CONF_DROP_IN or something like that which will be loaded last. This way, we make sure that we consume system defaults but can override them where needed.

I see usefulness for that beyond Quadlet, so we could break it into an issue. @rhatdan WDYT?

Ok, lets drop this for now and look at the actual usecases that are needed for quadlet. Then we can revisit the generic idea if needed, or add more specialized overrides.

Thank you, I appreciate it.

[ygalblum]

Following the discussion, I'm closing this PR