Bug 2092012 - specfile: improper environment check in scriptlet, please use ostree API instead
Summary: specfile: improper environment check in scriptlet, please use ostree API instead
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: kexec-tools
Version: 37
Hardware: Unspecified
OS: Unspecified
unspecified
medium
Target Milestone: ---
Assignee: Coiby
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2022-05-31 14:32 UTC by Luca BRUNO
Modified: 2022-10-26 01:56 UTC (History)
8 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2022-10-26 01:56:33 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)


Links
System ID Private Priority Status Summary Last Updated
Red Hat Issue Tracker FC-459 0 None None None 2022-05-31 14:32:49 UTC

Description Luca BRUNO 2022-05-31 14:32:27 UTC
The kexec-tools.spec has some logic in %pre and in %posttrans which is checking for an ostree environment by grep-ing /proc/cmdline:
 * https://src.fedoraproject.org/rpms/kexec-tools/blob/f36/f/kexec-tools.spec#_266
 * https://src.fedoraproject.org/rpms/kexec-tools/blob/f36/f/kexec-tools.spec#_341

This does not really work as intended, because those scripts can be executed at compose time on a different machine.
Inspecting the kernel arguments of the compose machine does not tell anything about the target environment, and can give wrong results.

The proper way to check for an ostree-based machine is to check for the presence of `/run/ostree-booted` instead.

Comment 1 Coiby 2022-07-14 07:20:10 UTC
Hi Luca,

Thanks for reporting this issue! The check in %pre and %posstrans is supposed to prevent the code that automatically updates the kernel crashkernel cmdline parameter from being executed as this feature doesn't support ostree. `/run/ostree-booted` seems to imply the ostreem system has been booted and is running, am I correct?

Comment 2 Coiby 2022-07-14 07:26:26 UTC
Another question is will package upgrade happens during compose time? Current the code will not be executed when  [ $1 == 2 ] i.e. during package upgrade.

Comment 3 Luca BRUNO 2022-07-14 07:45:36 UTC
> `/run/ostree-booted` seems to imply the ostree system has been booted and is running, am I correct?

Generally speaking yes, it is the most stable way to signal/check that a system has been booted in ostree-style.
It is also used by rpm-ostree at compose/install time in the sandboxed environment where scriptlets run, in order to signal that the package is being installed/composed into an ostree commit (i.e. not directly on a live system).
See https://github.com/coreos/rpm-ostree/blob/8ddf5f40d9cbbd9d3668cc75b703316e0a89ab11/src/libpriv/rpmostree-scripts.cxx#L350-L353 for reference.

> Another question is will package upgrade happens during compose time? Current the code will not be executed when  [ $1 == 2 ] i.e. during package upgrade.

I think that's not the case (normally).
I spotted a `kdumpctl` error at compose time, and I guess that was coming from the %posstrans logic indeed.
While looking into the specfile I noticed there was another similar check in %pre logic.
It would be good to fix both at once while at it, even if only one is actually triggering.

Comment 4 Coiby 2022-07-15 08:02:15 UTC
(In reply to Luca BRUNO from comment #3)
> > `/run/ostree-booted` seems to imply the ostree system has been booted and is running, am I correct?
> 
> Generally speaking yes, it is the most stable way to signal/check that a
> system has been booted in ostree-style.
> It is also used by rpm-ostree at compose/install time in the sandboxed
> environment where scriptlets run, in order to signal that the package is
> being installed/composed into an ostree commit (i.e. not directly on a live
> system).
> See
> https://github.com/coreos/rpm-ostree/blob/
> 8ddf5f40d9cbbd9d3668cc75b703316e0a89ab11/src/libpriv/rpmostree-scripts.
> cxx#L350-L353 for reference.

Thanks for the explanation! Btw, previously I used osbuild to create OSTree commits following
https://www.osbuild.org/news/2020-06-01-how-to-ostree-anaconda.html. Does /run/ostree-booted also exist during this type of compose?


> 
> > Another question is will package upgrade happens during compose time? Current the code will not be executed when  [ $1 == 2 ] i.e. during package upgrade.
> 
> I think that's not the case (normally).
> I spotted a `kdumpctl` error at compose time, and I guess that was coming
> from the %posstrans logic indeed.
> While looking into the specfile I noticed there was another similar check in
> %pre logic.
> It would be good to fix both at once while at it, even if only one is
> actually triggering.

Oh, I forgot RPM posttrans scriptlet doesn't provide way to distinguish between install and upgrade ([ $1 == 1] applies to both install and upgrade). That's why there is only  an error from %posttrans.

Comment 5 Luca BRUNO 2022-07-15 16:24:04 UTC
> Does /run/ostree-booted also exist during this type of compose?

That depends on how osbuild runs those scriptlets, but no I think that right now they aren't sandboxed in the same way and they don't see that file (at least right now).
But in that case too checking the kernel command-line is not useful, as it won't tell anything about the target system being composed.

Comment 6 Christian Kellner 2022-07-19 09:55:11 UTC
(In reply to Coiby from comment #4)

> Thanks for the explanation! Btw, previously I used osbuild to create OSTree
> commits following
> https://www.osbuild.org/news/2020-06-01-how-to-ostree-anaconda.html. Does
> /run/ostree-booted also exist during this type of compose?

osbuild currently does not set `/run/ostree-booted`, but we totally could. You can detect running osbuild via the `container` environment variable. kdumpctl already does so:

https://src.fedoraproject.org/rpms/kexec-tools/blob/f36/f/kdumpctl#_1647

Comment 7 Coiby 2022-07-19 13:35:19 UTC
(In reply to Luca BRUNO from comment #5)
> > Does /run/ostree-booted also exist during this type of compose?
> 
> That depends on how osbuild runs those scriptlets, but no I think that right
> now they aren't sandboxed in the same way and they don't see that file (at
> least right now).
> But in that case too checking the kernel command-line is not useful, as it
> won't tell anything about the target system being composed.

Thanks for the explanation! Christian's comment reminds me of another task which is kexec-tools tries to set up the kernel crashkernel parameter when the system is installed. For ananaconda, we have a dedicated addon to do that.  For osbuild, it's done in the posttrans scriptlet.   Does ostree want kexec-tools to set up the kernel crashkernel parameter? If yes, can we do it when packages are installed/composed into an ostree commit?

Comment 8 Coiby 2022-07-19 13:48:31 UTC
(In reply to Christian Kellner from comment #6)
> (In reply to Coiby from comment #4)
> 
> > Thanks for the explanation! Btw, previously I used osbuild to create OSTree
> > commits following
> > https://www.osbuild.org/news/2020-06-01-how-to-ostree-anaconda.html. Does
> > /run/ostree-booted also exist during this type of compose?
> 
> osbuild currently does not set `/run/ostree-booted`, but we totally could.
> You can detect running osbuild via the `container` environment variable.
> kdumpctl already does so:
> 
> https://src.fedoraproject.org/rpms/kexec-tools/blob/f36/f/kdumpctl#_1647

Yes, kdumpctl have already made use of the bwrap-osbuild environmental variable to to set up crashkernel for osbuild. Note the use case is different from ostree for which kexec-tools tries to disable the feature of auto-updating crashkernel as it currently doesn't support  ostree. 

Btw, when implementing the feature of setting up crashkernel for osbuild, I haven't took the case of osbuild+ostree as documented in https://www.osbuild.org/news/2020-06-01-how-to-ostree-anaconda.html into consideration into consideration. Do you know if current kexec-tools could work  for the osbuild+ostree+anaconda case?

Comment 9 Timothée Ravier 2022-07-27 10:58:24 UTC
> Does ostree want kexec-tools to set up the kernel crashkernel parameter? If yes, can we do it when packages are installed/composed into an ostree commit?

During composes on rpm-ostree based systems, RPMs can not set kernel arguments. The kexec-tools should thus skip setting kernel arguments during scriptlets by checking for the presence of `/run/ostree-booted`.

Note that osbuild does not currently use rpm-ostree to do the compose and thus behaves a little bit differently from other rpm-ostree based variants of Fedora such as CoreOS, Silverblue, etc.

Comment 10 Coiby 2022-07-29 12:54:58 UTC
(In reply to Timothée Ravier from comment #9)
> > Does ostree want kexec-tools to set up the kernel crashkernel parameter? If yes, can we do it when packages are installed/composed into an ostree commit?
> 
> During composes on rpm-ostree based systems, RPMs can not set kernel
> arguments. The kexec-tools should thus skip setting kernel arguments during
> scriptlets by checking for the presence of `/run/ostree-booted`.
> 
> Note that osbuild does not currently use rpm-ostree to do the compose and
> thus behaves a little bit differently from other rpm-ostree based variants
> of Fedora such as CoreOS, Silverblue, etc.

Thanks for the explanation! I've posted a patch to skip setting kernel arguments for ostree system
https://lists.fedoraproject.org/archives/list/kexec@lists.fedoraproject.org/thread/E72BIU7NITNMY643GOD2Q7L33SKK32IB/

Comment 11 Ben Cotton 2022-08-09 13:41:41 UTC
This bug appears to have been reported against 'rawhide' during the Fedora Linux 37 development cycle.
Changing version to 37.

Comment 12 Timothée Ravier 2022-10-19 15:59:00 UTC
According to https://lists.fedoraproject.org/archives/list/kexec@lists.fedoraproject.org/message/4OHV4WGOZD6JNSL25FQZMOVDEIGHGGMQ/ this should be fixed. Can you update the status? Thanks

Comment 13 Coiby 2022-10-26 01:56:33 UTC
(In reply to Timothée Ravier from comment #12)
> According to
> https://lists.fedoraproject.org/archives/list/kexec@lists.fedoraproject.org/
> message/4OHV4WGOZD6JNSL25FQZMOVDEIGHGGMQ/ this should be fixed. Can you
> update the status? Thanks

Thanks for the reminder!


Note You need to log in before you can comment on or make changes to this bug.