Skip to content

Conversation

@cgwalters
Copy link
Member

Add ConditionKernelCommandLine=ostree to prevent zincati from running on systems that aren't using ostree, where the update agent wouldn't be applicable.

This is motivated by having the service not run in bcvk ephemeral as well as podman run /sbin/init.

Add ConditionKernelCommandLine=ostree to prevent zincati from running
on systems that aren't using ostree, where the update agent wouldn't
be applicable.

This is motivated by having the service not run in `bcvk ephemeral`
as well as `podman run /sbin/init`.

Signed-off-by: Colin Walters <[email protected]>
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds a systemd condition to ensure the Zincati agent only runs on ostree-based systems, using ConditionKernelCommandLine=ostree. My review points out that this approach can cause issues in containerized environments on an ostree host. I've suggested using ConditionPathExists=/ostree/repo instead, as it's a more robust and standard method for this check, aligning with other core ostree services.

Comment on lines +6 to +8
# This ensures compatibility with `bcvk ephemeral` today; compatibility
# with a bootc+composefs future is still TBD.
ConditionKernelCommandLine=ostree

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using ConditionKernelCommandLine=ostree to detect an ostree-based system might not be the most robust method. A key issue is that when running in a container on an ostree host (e.g. via podman run /sbin/init), this condition will be met because it reads the host's kernel command line. This could lead to the agent unexpectedly running inside the container, which is likely not the desired behavior.

A more standard and reliable way to check for an ostree system is ConditionPathExists=/ostree/repo. This is the approach used by other core ostree services like rpm-ostreed.service. It would correctly identify the environment and avoid running inside a container unless the ostree repository is explicitly available.

For better robustness, I recommend using this condition instead.

ConditionPathExists=/ostree/repo

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah, wow it's right. docker/podman not masking the kernel commandline is really a giant footgun in all of this (unlike LXC among others IIRC).

But this isn't a new problem, and really wants a nicer system in general.

A more standard and reliable way to check for an ostree system is ConditionPathExists=/ostree/repo.

Yeah, though in some corner cases this will be wrong, such as having co-installed bootc/ostree alongside a package system and doing a rollback.

I guess we probably do need a consistent /run/bootc-booted or so that's always set even in the bootc+composefs case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a consensus on the path forward?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge this as is and we'll come back to the composefs problem later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

specifically for podman run /sbin/init (not sure about bcvk) would ConditionVirtualization=!container work?

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 this pull request may close these issues.

2 participants