move allowedcpu logic to pre-start validation

[dougsland]

Fixes #284

[dougsland]

@alexlarsson is it something like that are you looking to see ? I moved to draft as I need to fully test it.

[alexlarsson]

This is never ever going to work. You can't just modify a generator source from a service. It is problematic in many ways:

You shouldn't modify the rpm-owned file it will break its verification and break updates, and in fact you often can't if your using ostree or some other image system

The .container file is read during generators which is far before any service is started, so it will not pick up the change unless you start daemon-reload. And you absolutely should not start daemon-reload from a service.

And anyway, the point of this AllowedCPUs line is unclear. It seems to me like an example how you would change what cpus the qm partition should run on, so that each user can tweak it to what the actual safetly argument is for the device in question. Not some complicated code that always computes nproc/2.

What I meant was a dropin like:

# Contents of qm.continer.d/allowedcpus.conf
[Service]
AllowedCPUs=1 

This would override the AllowedCPUs for the generated service file. And I don't meant that we should have a complex system to automatically compute this from nproc/2, that is not correct. Instead it is just something that is available to each integrator to override things like AllowedCPUs to match what the policy is for particular devices without having to replace all of qm.container.

[alexlarsson]

Sorry, the filename in the above comment is wrong, the dropin file would be in qm.service.d, not qm.container.d if we're using a systemd dropins. With containers/podman#20828 it would be in qm.container.d.

[dougsland]

This is never ever going to work. You can't just modify a generator source from a service. It is problematic in many ways:

You shouldn't modify the rpm-owned file it will break its verification and break updates, and in fact you often can't if your using ostree or some other image system

The .container file is read during generators which is far before any service is started, so it will not pick up the change > unless you start daemon-reload. And you absolutely should not start daemon-reload from a service.

Gotcha

And anyway, the point of this AllowedCPUs line is unclear. It seems to me like an example how you would change what
cpus the qm partition should run on, so that each user can tweak it to what the actual safetly argument is for the device in
question. Not some complicated code that always computes nproc/2.

@rhatdan @lsm5 do you guys remember why this AllowedCPU was added into the spec file? Are you okay to set it to 1?

What I meant was a dropin like:

# Contents of qm.continer.d/allowedcpus.conf
[Service]
AllowedCPUs=1 

Yes, it's more simple if we don't need that logic.
Just double checking @alexlarsson , it will be: /etc/containers/systemd/qm.service.d/allowedcpus.conf

This would override the AllowedCPUs for the generated service file. And I don't meant that we should have a complex
system to automatically compute this from nproc/2, that is not correct. Instead it is just something that is available to each
integrator to override things like AllowedCPUs to match what the policy is for particular devices without having to replace > all of qm.container.

@alexlarsson Let's wait your patch go into podman code so we can use it. containers/podman#20828

[alexlarsson]

Just double checking @alexlarsson , it will be: /etc/containers/systemd/qm.service.d/allowedcpus.conf

No, if you're using the systemd dropins that would be a dropin to the qm.service file that quadlet generates, so it would be /etc/systemd/system/qm.service.d/allowedcpus.conf

[alexlarsson]

@rhatdan @lsm5 do you guys remember why this AllowedCPU was added into the spec file? Are you okay to set it to 1?

For the record, I don't mean that we should always set this to one. That would be very limiting. What I mean is that if a customer has looked at their requirements and decided that they want the QM partition to use a specific set of CPUs, they would drop a file like this to enable that configuration. I don't think we should limit CPU at all by default.

[dougsland]

@rhatdan @lsm5 do you guys remember why this AllowedCPU was added into the spec file? Are you okay to set it to 1?

For the record, I don't mean that we should always set this to one. That would be very limiting. What I mean is
that if a customer has looked at their requirements and decided that they want the QM partition to use a
specific set of CPUs, they would drop a file like this to enable that configuration. I don't think we should limit
CPU at all by default.

@alexlarsson Okay, so this patch is about removing the existing logic from spec and create a documentation instead?

[alexlarsson]

@rhatdan @lsm5 do you guys remember why this AllowedCPU was added into the spec file? Are you okay to set it to 1?

For the record, I don't mean that we should always set this to one. That would be very limiting. What I mean is
that if a customer has looked at their requirements and decided that they want the QM partition to use a
specific set of CPUs, they would drop a file like this to enable that configuration. I don't think we should limit
CPU at all by default.

@alexlarsson Okay, so this patch is about removing the existing logic from spec and create a documentation instead?

Yeah, maybe an example too.

[rhatdan]

Why not put it in and comment it out of qm.container, so it is easily discoverable but not on by default?

[alexlarsson]

Why not put it in and comment it out of qm.container, so it is easily discoverable but not on by default?

Yeah, that would work for me. Although to actually apply the change using a dropin may be preferable to modifying the qm.container file that is owned by the rpm. (Or making a modified copy of it in /etc, that doesn't then get updates, etc)

[rhatdan]

I agree I like the drop in, but discovering these flags is not easy. So having one in the default commented out helps users know capabities. Perhaps document in qm.container that better to modify flag in drop in directory.

[dougsland]

I agree I like the drop in, but discovering these flags is not easy. So having one in the default commented out helps users
know capabities. Perhaps document in qm.container that better to modify flag in drop in directory.

I agree, commented seems easy to find.

[dougsland]

@alexlarsson @rhatdan WDYT?

[alexlarsson]

looks great to me!

[rhatdan]

LGTM

[pypingou]

On Wed, Nov 29, 2023 at 05:02:35PM -0800, Daniel J Walsh wrote: Why not put it in and comment it out of qm.container, so it is easily discoverable but not on by default?

You could also do it the other way around, have the qm.container refer to an existing file, but that file would be completely commented out (and thus have no effect)