9:40 a.m.
Hi,
this requires a bit of background, so please bear with me :)
Earlier this year, the libvirt project changed its RPM package to be
more granular[1] as a step towards a future where the legacy
monolithic daemon (libvirtd) is completely gone. This change has
happened upstream, but the Fedora package uses the same spec file so
it was quickly reflected in Rawhide.
As part of this change virtproxyd, a service which has historically
been part of the libvirt-daemon package, has been moved to the
newly-introduced libvirt-daemon-proxy package.
More recently we have realized that, despite our care in trying to
ensure smooth upgrades, splitting the package this way has
unfortunately caused unintended consequences[2][3]. Specifically,
since the libvirt-daemon-proxy package is a completely new one as far
as RPM is concerned, all its systemd units will get presets applied
during %post. In at least two completely valid deployment scenarios,
this results in some amount of brokenness due to local configuration
changes made by the admin getting discarded.
In order to avoid this problem, I have implemented a new set of RPM
macros[4][5] that base the decision of whether presets should be
applied for a systemd unit not on whether the package that contains
them is being newly installed, but rather on whether the unit itself
existed on the system before package installation. This should
address not only the scenario of services being split off to separate
packages, but also the one where an existing package starts shipping
additional services.
Now, since this is clearly not a libvirt-specific issue, I believe
this approach should be adopted across Fedora by way of these macros
(or comparable ones) being added to systemd. Packages would have to
migrate from the old macros over time, and at some point it should be
possible to get rid of them entirely.
Note that openSUSE already has its own distro-specific RPM macros for
dealing with systemd units, and in fact the approach that I have
implemented is partially inspired by theirs. Ideally, we'd be able to
collaborate with them to create a single set of macros that lives in
systemd and satisfies the needs of both Fedora and openSUSE.
The problem is that Fedora 39 and RHEL 9.3 are fast approaching and,
if we don't do anything about this issue before then, a subset of
libvirt users will see their deployments broken after upgrade. In the
interest of avoiding that, I would prefer to get the libvirt-specific
version of the macros merged as soon as possible, and focus on
upstreaming the work all the way to systemd as a follow-up.
Does this plan sound reasonable to the Fedora community? Are there
any serious concerns regarding the approach taken for the macros that
would cause them to be considered a complete no-go? Any scenarios
that I might have missed while implementing them?
Thanks for your patience in reading this far :) and looking forward
to your feedback!
[1] https://listman.redhat.com/archives/libvir-list/2023-January/237059.html
[2] https://bugzilla.redhat.com/show_bug.cgi?id=2210058
[3] https://listman.redhat.com/archives/libvir-list/2023-June/240226.html
[4] https://listman.redhat.com/archives/libvir-list/2023-July/240723.html
[5] https://listman.redhat.com/archives/libvir-list/2023-July/240729.html
--
Andrea Bolognani / Red Hat / Virtualization
4:01 p.m.
On Thu, Jul 20, 2023 at 07:40:08AM -0700, Andrea Bolognani wrote:
Hi,
this requires a bit of background, so please bear with me :)
Thanks for all the background... :)
...snip reasonable stuff...
I agree that an upstream systemd solution is what we should strive for.
The problem is that Fedora 39 and RHEL 9.3 are fast approaching and,
if we don't do anything about this issue before then, a subset of
libvirt users will see their deployments broken after upgrade. In the
interest of avoiding that, I would prefer to get the libvirt-specific
version of the macros merged as soon as possible, and focus on
upstreaming the work all the way to systemd as a follow-up.
Do you have a patch/PR we could look at to show what exactly these
macros look like?
Does this plan sound reasonable to the Fedora community? Are there
any serious concerns regarding the approach taken for the macros that
would cause them to be considered a complete no-go? Any scenarios
that I might have missed while implementing them?
Not sure, but possibly. :)
If for some reason this didn't turn out to be something we wanted to
ship, what does the 'manual fix' look like? Complex?
kevin
8:45 a.m.
On Thu, Jul 20, 2023 at 02:01:37PM -0700, Kevin Fenzi wrote:
On Thu, Jul 20, 2023 at 07:40:08AM -0700, Andrea Bolognani wrote:
> The problem is that Fedora 39 and RHEL 9.3 are fast approaching and,
> if we don't do anything about this issue before then, a subset of
> libvirt users will see their deployments broken after upgrade. In the
> interest of avoiding that, I would prefer to get the libvirt-specific
> version of the macros merged as soon as possible, and focus on
> upstreaming the work all the way to systemd as a follow-up.
Do you have a patch/PR we could look at to show what exactly these
macros look like?
The full series[4] and the patch adding the new macros[5] were both
linked in the original message. I guess you must have missed them.
I've repeated the links below for your convenience.
Of course that's for the short-term, libvirt-specific solution, not
for the more generic one that we'd want to land in systemd.
> Does this plan sound reasonable to the Fedora community? Are
there
> any serious concerns regarding the approach taken for the macros that
> would cause them to be considered a complete no-go? Any scenarios
> that I might have missed while implementing them?
Not sure, but possibly. :)
If for some reason this didn't turn out to be something we wanted to
ship, what does the 'manual fix' look like? Complex?
What do you mean by "manual fix"? The steps that an admin that ends
up with a broken installation after the update would have to follow
to return it to a working state? If so, not really that bad, just a
few calls to systemctl.
However, realizing that the deployment is broken in the first place
is its own challenge (the failure mode might not be immediately
obvious), as would be figuring out the exact systemctl calls one
needs: there are at least two possible failures that I'm aware of,
and the fix is different based on which one you hit.
[4] https://listman.redhat.com/archives/libvir-list/2023-July/240723.html
[5] https://listman.redhat.com/archives/libvir-list/2023-July/240729.html
--
Andrea Bolognani / Red Hat / Virtualization
12:07 p.m.
On Mon, Jul 24, 2023 at 06:45:12AM -0700, Andrea Bolognani wrote:
On Thu, Jul 20, 2023 at 02:01:37PM -0700, Kevin Fenzi wrote:
> On Thu, Jul 20, 2023 at 07:40:08AM -0700, Andrea Bolognani wrote:
> > The problem is that Fedora 39 and RHEL 9.3 are fast approaching and,
> > if we don't do anything about this issue before then, a subset of
> > libvirt users will see their deployments broken after upgrade. In the
> > interest of avoiding that, I would prefer to get the libvirt-specific
> > version of the macros merged as soon as possible, and focus on
> > upstreaming the work all the way to systemd as a follow-up.
>
> Do you have a patch/PR we could look at to show what exactly these
> macros look like?
The full series[4] and the patch adding the new macros[5] were both
linked in the original message. I guess you must have missed them.
I've repeated the links below for your convenience.
Oops. I guess I did. Sorry about that.
Of course that's for the short-term, libvirt-specific solution,
not
for the more generic one that we'd want to land in systemd.
Right.
> > Does this plan sound reasonable to the Fedora community?
Are there
> > any serious concerns regarding the approach taken for the macros that
> > would cause them to be considered a complete no-go? Any scenarios
> > that I might have missed while implementing them?
>
> Not sure, but possibly. :)
>
> If for some reason this didn't turn out to be something we wanted to
> ship, what does the 'manual fix' look like? Complex?
What do you mean by "manual fix"? The steps that an admin that ends
up with a broken installation after the update would have to follow
to return it to a working state? If so, not really that bad, just a
few calls to systemctl.
However, realizing that the deployment is broken in the first place
is its own challenge (the failure mode might not be immediately
obvious), as would be figuring out the exact systemctl calls one
needs: there are at least two possible failures that I'm aware of,
and the fix is different based on which one you hit.
ok. Good to know.
kevin
3:20 a.m.
Hi Andrea,
On Thu, Jul 20, 2023 at 07:40:08AM -0700, Andrea Bolognani wrote:
since the libvirt-daemon-proxy package is a completely new one as
far
as RPM is concerned, all its systemd units will get presets applied
during %post. In at least two completely valid deployment scenarios,
this results in some amount of brokenness due to local configuration
changes made by the admin getting discarded.
This is a known problem. We've
been usually dealing with it in an ad-hoc
manner using a %trigger. I'll paste some docs from another email below,
but that is complex and generally far from ideal. It'd be nice to have
a clean solution for this.
Another issue is that our guidelines don't mention the issue at all,
so if people don't know about it, it's easy to fall into the trap.
So there's certainly space for improvement here ;)
In order to avoid this problem, I have implemented a new set of RPM
macros[4][5] that base the decision of whether presets should be
applied for a systemd unit not on whether the package that contains
them is being newly installed, but rather on whether the unit itself
existed on the system before package installation. This should
address not only the scenario of services being split off to separate
packages, but also the one where an existing package starts shipping
additional services.
Now, since this is clearly not a libvirt-specific issue, I believe
this approach should be adopted across Fedora by way of these macros
(or comparable ones) being added to systemd. Packages would have to
migrate from the old macros over time, and at some point it should be
possible to get rid of them entirely.
Systemd macros are maintained upstream. So if
you have
patches/suggestions/etc., they should be filed upstream.
I looked briefly at your links [4,5] but it seems very libvirt-specific and
it's hard to see the full plan without context. Please file a pull request
upstream and it'll be easier to discuss there. There're also people from
other distros there.
One immediate comment is that systemd macros have been reworked to restart
everything in %posttrans using a single operation. I.e. various per-package
macros mark services with "needs-restart", and then in %posttrans
a single 'systemctl reload-or-restart --marked' does the deed.
It seems your macros reimplement the previous behaviour of individual
restarts, and we wouldn't want to go back to that.
--
From
https://lists.fedoraproject.org/archives/list/kexec@lists.fedoraproject.o...:
-%post
+%post -n kdump-tools
# Initial installation
%systemd_post kdump.service
This one is tricky. %systemd_post presets the service
on "first installation",
which is actually the first the time package is installed. I.e. it unfortunately
also would execute the preset on upgrades that split out a subpackage, because
as far as rpm is concerned, this is the initial installation of that subpackage.
The righteous way to solve this would be something like this:
%triggerprein -n kdump-tools -- kexec-tools < 2.0.26-8
touch %{_localstatedir}/lib/rpm-state/kexec-tools.no-preset
%post -n kdump-tools
rm %{_localstatedir}/lib/rpm-state/kexec-tools.no-preset 2>/dev/null && return
0
# Initial installation
%systemd_post kdump.service
A bit of a bother, but at least nobody will be suprised by
kdump.service changing state.
--
Zbyszek
9:36 a.m.
On Fri, Jul 21, 2023 at 08:20:21AM +0000, Zbigniew Jędrzejewski-Szmek wrote:
On Thu, Jul 20, 2023 at 07:40:08AM -0700, Andrea Bolognani wrote:
> Now, since this is clearly not a libvirt-specific issue, I believe
> this approach should be adopted across Fedora by way of these macros
> (or comparable ones) being added to systemd. Packages would have to
> migrate from the old macros over time, and at some point it should be
> possible to get rid of them entirely.
Systemd macros are maintained upstream. So if you have
patches/suggestions/etc., they should be filed upstream.
I looked briefly at your links [4,5] but it seems very libvirt-specific and
it's hard to see the full plan without context. Please file a pull request
upstream and it'll be easier to discuss there. There're also people from
other distros there.
Making this generic enough that it would work with arbitrary
services, and polished enough that I would consider submitting it as
a systemd change, would require a non-trivial amount of additional
work.
That's work that I'm absolutely willing and intending to do[1], but
even under the most optimistic circumstances I see no chance of it
being completed in time for Fedora 39 and RHEL 9.3, which is when we
would need it in order to avoid breaking users' deployments.
So what I'm looking for here is validation that the overall approach
is reasonably sound, the current implementation is not completely
broken, and in general Fedora would not reject a libvirt.spec file
that contained those changes.
Does that seem fair?
One immediate comment is that systemd macros have been reworked to
restart
everything in %posttrans using a single operation. I.e. various per-package
macros mark services with "needs-restart", and then in %posttrans
a single 'systemctl reload-or-restart --marked' does the deed.
It seems your macros reimplement the previous behaviour of individual
restarts, and we wouldn't want to go back to that.
Each service is restarted/reloaded only once, but yeah there's a
separate call to systemctl for each of those reloads/restarts which I
agree is suboptimal.
For the short-term implementation that lives in libvirt.spec I
thought it would be best to rely as little as possible on external
bits of machinery, such as the call to reload-or-restart, in order to
keep things self-contained and easy to inspect. Especially since the
reload-or-restart bit is part of systemd.spec's %transfiletriggerin,
which at least AFAICT is kind of opaque when looking at an installed
system.
Note that systemd-update-helper also doesn't currently support
marking services as needs-reload, only needs-restart, so that would
have to be added.
> +%post -n kdump-tools
> # Initial installation
> %systemd_post kdump.service
This one is tricky. %systemd_post presets the service on "first installation",
which is actually the first the time package is installed. I.e. it unfortunately
also would execute the preset on upgrades that split out a subpackage, because
as far as rpm is concerned, this is the initial installation of that subpackage.
The righteous way to solve this would be something like this:
%triggerprein -n kdump-tools -- kexec-tools < 2.0.26-8
touch %{_localstatedir}/lib/rpm-state/kexec-tools.no-preset
%post -n kdump-tools
rm %{_localstatedir}/lib/rpm-state/kexec-tools.no-preset 2>/dev/null && return
0
# Initial installation
%systemd_post kdump.service
A bit of a bother, but at least nobody will be suprised by
kdump.service changing state.
This obviously works, but also requires a lot more involvement from
the package maintainer: specifying the last version number that's not
affected by the split[2] making sure that each bit of logic is in the
scriptlet for the correct package...
The beauty of the "check at %pre time whether the unit already exists
on disk" approach is that it should work out of the box in all
scenarios, with no further information needed. Do you see a problem
with it that I have failed to consider?
[1] I definitely don't want libvirt.spec to keep carrying around its
own systemd macros forever!
[2] Or is that the first one that is? O:-)
--
Andrea Bolognani / Red Hat / Virtualization
5:51 a.m.
On Mon, Jul 24, 2023 at 07:36:41AM -0700, Andrea Bolognani wrote:
On Fri, Jul 21, 2023 at 08:20:21AM +0000, Zbigniew Jędrzejewski-Szmek
wrote:
> On Thu, Jul 20, 2023 at 07:40:08AM -0700, Andrea Bolognani wrote:
> > Now, since this is clearly not a libvirt-specific issue, I believe
> > this approach should be adopted across Fedora by way of these macros
> > (or comparable ones) being added to systemd. Packages would have to
> > migrate from the old macros over time, and at some point it should be
> > possible to get rid of them entirely.
>
> Systemd macros are maintained upstream. So if you have
> patches/suggestions/etc., they should be filed upstream.
>
> I looked briefly at your links [4,5] but it seems very libvirt-specific and
> it's hard to see the full plan without context. Please file a pull request
> upstream and it'll be easier to discuss there. There're also people from
> other distros there.
Making this generic enough that it would work with arbitrary
services, and polished enough that I would consider submitting it as
a systemd change, would require a non-trivial amount of additional
work.
That's work that I'm absolutely willing and intending to do[1], but
even under the most optimistic circumstances I see no chance of it
being completed in time for Fedora 39 and RHEL 9.3, which is when we
would need it in order to avoid breaking users' deployments.
So what I'm looking for here is validation that the overall approach
is reasonably sound, the current implementation is not completely
broken, and in general Fedora would not reject a libvirt.spec file
that contained those changes.
I don't think Fedora would reject those changes, because in general
we don't reject things unless they really break stuff for other people.
Nevertheless, I agree with what Daniel P. Berrangé wrote in another reply:
My concern is about where the solution is applied and divergance
from
Fedora guidelines.
The long term direction of Fedora / RPM has been to reduce the number
of scriptlets required to be explicitly listed in package specfiles, by
having RPM globally apply script logic for all content in certain given
directories. If we're using standard Fedora macros, then we'd not expect
to see problems if the macros get changed to adapt to new approaches,
but if we're going our own way all bets are off.
The current macros we're using are specified in the Fedora packaging
guidelines:
https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_sy...
and their impl is provided by systemd upstream itself:
https://github.com/systemd/systemd/blob/main/src/rpm/macros.systemd.in
The problems described do not appear to be things unique to libvirt.
IOW, I think the problem needs to be raised & addressed in context of
the Fedora and systemd communities, rather than having libvirt diverge
from normal Feora packaging practice.
You're putting in quite a lot of work to create some very specific
machinery. From the POV of the individual package this may look like
the most efficient solution, but from the POV of the distro, such
unique per-package solutions have the downside that they require special
knowledge and make it harder for other maintainers to understand the
package and contribute to it.
I think it'd be more effiecient to go with the (slightly ugly, no
disagreement here) seven line %trigger scriptlet. And and then work
with upstream to implement a generic solution that can be used by all
packages. It seems to me that the work put into implementing a custom
solution in libvirt would be wasted if you want to work with upstream
on a completely different general solution soon after.
> One immediate comment is that systemd macros have been reworked
to restart
> everything in %posttrans using a single operation. I.e. various per-package
> macros mark services with "needs-restart", and then in %posttrans
> a single 'systemctl reload-or-restart --marked' does the deed.
> It seems your macros reimplement the previous behaviour of individual
> restarts, and we wouldn't want to go back to that.
Each service is restarted/reloaded only once, but yeah there's a
separate call to systemctl for each of those reloads/restarts which I
agree is suboptimal.
For the short-term implementation that lives in libvirt.spec I
thought it would be best to rely as little as possible on external
bits of machinery, such as the call to reload-or-restart, in order to
keep things self-contained and easy to inspect. Especially since the
reload-or-restart bit is part of systemd.spec's %transfiletriggerin,
which at least AFAICT is kind of opaque when looking at an installed
system.
Note that systemd-update-helper also doesn't currently support
marking services as needs-reload, only needs-restart, so that would
have to be added.
So far, the assumption was that almost everything would be restarted
(because we want the new code to be running). Stuff that cannot be
restarted is effectively ignored by the packaging macros, we have
%systemd_postun == %nil. In the general case, for services that do
not support restarting, it's not clear that they should be reloaded.
They old code might not support the new configuration format, or some
of the auxiliary files might not exist in the new version, or even if
the reload is supported, it's not clear why it should happen.
So packages are generally expected to do the reload, if appropriate,
on their own. There is really no harm in having
%postun
systemctl try-reload foobar.service || :
We *could* wrap this in a macro, but it's trivial and package-specific
anyway.
> > +%post -n kdump-tools
> > # Initial installation
> > %systemd_post kdump.service
>
> This one is tricky. %systemd_post presets the service on "first
installation",
> which is actually the first the time package is installed. I.e. it unfortunately
> also would execute the preset on upgrades that split out a subpackage, because
> as far as rpm is concerned, this is the initial installation of that subpackage.
>
> The righteous way to solve this would be something like this:
>
> %triggerprein -n kdump-tools -- kexec-tools < 2.0.26-8
> touch %{_localstatedir}/lib/rpm-state/kexec-tools.no-preset
>
> %post -n kdump-tools
> rm %{_localstatedir}/lib/rpm-state/kexec-tools.no-preset 2>/dev/null &&
return 0
> # Initial installation
> %systemd_post kdump.service
>
> A bit of a bother, but at least nobody will be suprised by
> kdump.service changing state.
This obviously works, but also requires a lot more involvement from
the package maintainer: specifying the last version number that's not
affected by the split[2] making sure that each bit of logic is in the
scriptlet for the correct package...
The beauty of the "check at %pre time whether the unit already exists
on disk" approach is that it should work out of the box in all
scenarios, with no further information needed. Do you see a problem
with it that I have failed to consider?
I think it's an interesting approach and it sounds like it should
work. Nevertheless, it's a big departure from what we used before, so
there might be some corner cases I'm not seeing.
I wonder if it would be possible to invert the approach, and have a
%transfiletriggerun that'd mark various using to not require a preset,
and then just do the preset for anything that was added later.
Zbyszek
9:45 a.m.
On Tue, Jul 25, 2023 at 10:51:04AM +0000, Zbigniew Jędrzejewski-Szmek wrote:
I don't think Fedora would reject those changes, because in
general
we don't reject things unless they really break stuff for other people.
Nevertheless, I agree with what Daniel P. Berrangé wrote in another reply:
> My concern is about where the solution is applied and divergance from
> Fedora guidelines.
>
> The long term direction of Fedora / RPM has been to reduce the number
> of scriptlets required to be explicitly listed in package specfiles, by
> having RPM globally apply script logic for all content in certain given
> directories. If we're using standard Fedora macros, then we'd not expect
> to see problems if the macros get changed to adapt to new approaches,
> but if we're going our own way all bets are off.
>
> The current macros we're using are specified in the Fedora packaging
> guidelines:
>
> https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_sy...
>
> and their impl is provided by systemd upstream itself:
>
> https://github.com/systemd/systemd/blob/main/src/rpm/macros.systemd.in
>
> The problems described do not appear to be things unique to libvirt.
>
> IOW, I think the problem needs to be raised & addressed in context of
> the Fedora and systemd communities, rather than having libvirt diverge
> from normal Feora packaging practice.
You're putting in quite a lot of work to create some very specific
machinery. From the POV of the individual package this may look like
the most efficient solution, but from the POV of the distro, such
unique per-package solutions have the downside that they require special
knowledge and make it harder for other maintainers to understand the
package and contribute to it.
No disagreement from me here :)
I think it'd be more effiecient to go with the (slightly ugly,
no
disagreement here) seven line %trigger scriptlet. And and then work
with upstream to implement a generic solution that can be used by all
packages. It seems to me that the work put into implementing a custom
solution in libvirt would be wasted if you want to work with upstream
on a completely different general solution soon after.
Another way to look at it is that I *already* put all that work in,
and the solution I've proposed for libvirt has been tested quite
thoroughly in a number of install and upgrade scenarios. In order to
adopt the %trigger based solution, I will have to spend more time on
developing and testing another implementation.
I'll give it a shot, but as I mentioned time is not on our side with
this one. My primary concern is having a working solution in place so
that users' deployment don't break on upgrade.
> Note that systemd-update-helper also doesn't currently
support
> marking services as needs-reload, only needs-restart, so that would
> have to be added.
So far, the assumption was that almost everything would be restarted
(because we want the new code to be running). Stuff that cannot be
restarted is effectively ignored by the packaging macros, we have
%systemd_postun == %nil. In the general case, for services that do
not support restarting, it's not clear that they should be reloaded.
They old code might not support the new configuration format, or some
of the auxiliary files might not exist in the new version, or even if
the reload is supported, it's not clear why it should happen.
So packages are generally expected to do the reload, if appropriate,
on their own. There is really no harm in having
%postun
systemctl try-reload foobar.service || :
We *could* wrap this in a macro, but it's trivial and package-specific
anyway.
I feel that having packages call systemctl directly instead of going
through the %systemd_* macros is an anti-pattern that shouldn't be
encouraged.
The current set of macros already includes both %systemd_postun and
%systemd_postun_with_restart, so adding %systemd_postun_with_reload
or something along those lines doesn't seem like a stretch.
Of course it would be the package's responsibility to choose the
right macro for each unit. The maintainer should know enough about
the services to make the correct call.
> > > +%post -n kdump-tools
> > > # Initial installation
> > > %systemd_post kdump.service
> >
> > This one is tricky. %systemd_post presets the service on "first
installation",
> > which is actually the first the time package is installed. I.e. it
unfortunately
> > also would execute the preset on upgrades that split out a subpackage, because
> > as far as rpm is concerned, this is the initial installation of that
subpackage.
> >
> > The righteous way to solve this would be something like this:
> >
> > %triggerprein -n kdump-tools -- kexec-tools < 2.0.26-8
> > touch %{_localstatedir}/lib/rpm-state/kexec-tools.no-preset
> >
> > %post -n kdump-tools
> > rm %{_localstatedir}/lib/rpm-state/kexec-tools.no-preset 2>/dev/null
&& return 0
> > # Initial installation
> > %systemd_post kdump.service
> >
> > A bit of a bother, but at least nobody will be suprised by
> > kdump.service changing state.
Looking at this again, I can rely on the fact that %triggerprein will
run before %post, right? It looks like that based on what I know
about scriptlets execution order and your example, but I want to be
sure.
What version number should I use in my case? The package split
happened in 9.1.0-1, but this handling is going to land in 9.6.0-1 at
the earliest so I think think using this latter version number makes
more sense, right? If you have somehow already upgraded to, say,
9.3.0-1 in the meantime, then your deployment is already broken and
there's not much that we can do about it anyway.
> The beauty of the "check at %pre time whether the unit
already exists
> on disk" approach is that it should work out of the box in all
> scenarios, with no further information needed. Do you see a problem
> with it that I have failed to consider?
I think it's an interesting approach and it sounds like it should
work. Nevertheless, it's a big departure from what we used before, so
there might be some corner cases I'm not seeing.
That's already encouraging :)
I wonder if it would be possible to invert the approach, and have a
%transfiletriggerun that'd mark various using to not require a preset,
and then just do the preset for anything that was added later.
I'm not sure I quite understand what you're suggesting.
Wouldn't that part be under systemd's control? AFAICT the current
implementation uses %transfiletriggerpostun to restart services that
have been marked as needing a restart, but the task of marking them
as such still falls on the package.
For presets, we don't have a --marked option that would allow
applying everything in one go. I actually don't think that we would
want that to happen anyway: in the case of libvirt, for example, we
need presets for virtqemud.socket to be applied before presets for
virtqemud.service, because virtqemud.socket is disabled as per the
system-wide default and virtqemud.service is explicitly enabled, but
at the same time virtqemud.service has Also=virtqemud.socket in its
[Install] section and we want the result to be both being enabled.
In general, wouldn't this kind of approach require marking units with
preset-already-applied (or similar) and carrying that annotation for
the lifetime of the system? That seems worse than marking units as
preset-needed (or similar) for just the short time between %pre and
%posttrans.
But I might have simply misunderstood your proposal :)
--
Andrea Bolognani / Red Hat / Virtualization
2:15 a.m.
On Tue, Jul 25, 2023 at 10:45:30AM -0400, Andrea Bolognani wrote:
On Tue, Jul 25, 2023 at 10:51:04AM +0000, Zbigniew Jędrzejewski-Szmek
wrote:
> I don't think Fedora would reject those changes, because in general
> we don't reject things unless they really break stuff for other people.
> Nevertheless, I agree with what Daniel P. Berrangé wrote in another reply:
>
> > My concern is about where the solution is applied and divergance from
> > Fedora guidelines.
> >
> > The long term direction of Fedora / RPM has been to reduce the number
> > of scriptlets required to be explicitly listed in package specfiles, by
> > having RPM globally apply script logic for all content in certain given
> > directories. If we're using standard Fedora macros, then we'd not
expect
> > to see problems if the macros get changed to adapt to new approaches,
> > but if we're going our own way all bets are off.
> >
> > The current macros we're using are specified in the Fedora packaging
> > guidelines:
> >
> >
https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_sy...
> >
> > and their impl is provided by systemd upstream itself:
> >
> > https://github.com/systemd/systemd/blob/main/src/rpm/macros.systemd.in
> >
> > The problems described do not appear to be things unique to libvirt.
> >
> > IOW, I think the problem needs to be raised & addressed in context of
> > the Fedora and systemd communities, rather than having libvirt diverge
> > from normal Feora packaging practice.
>
> You're putting in quite a lot of work to create some very specific
> machinery. From the POV of the individual package this may look like
> the most efficient solution, but from the POV of the distro, such
> unique per-package solutions have the downside that they require special
> knowledge and make it harder for other maintainers to understand the
> package and contribute to it.
No disagreement from me here :)
> I think it'd be more effiecient to go with the (slightly ugly, no
> disagreement here) seven line %trigger scriptlet. And and then work
> with upstream to implement a generic solution that can be used by all
> packages. It seems to me that the work put into implementing a custom
> solution in libvirt would be wasted if you want to work with upstream
> on a completely different general solution soon after.
Another way to look at it is that I *already* put all that work in,
and the solution I've proposed for libvirt has been tested quite
thoroughly in a number of install and upgrade scenarios. In order to
adopt the %trigger based solution, I will have to spend more time on
developing and testing another implementation.
That's fine. I only saw some links to emails, and it didn't know that
those have already been applied and tested. My goal was to minimize
overall effort, not to increase it ;)
I'll give it a shot, but as I mentioned time is not on our side
with
this one. My primary concern is having a working solution in place so
that users' deployment don't break on upgrade.
> > Note that systemd-update-helper also doesn't currently support
> > marking services as needs-reload, only needs-restart, so that would
> > have to be added.
>
> So far, the assumption was that almost everything would be restarted
> (because we want the new code to be running). Stuff that cannot be
> restarted is effectively ignored by the packaging macros, we have
> %systemd_postun == %nil. In the general case, for services that do
> not support restarting, it's not clear that they should be reloaded.
> They old code might not support the new configuration format, or some
> of the auxiliary files might not exist in the new version, or even if
> the reload is supported, it's not clear why it should happen.
>
> So packages are generally expected to do the reload, if appropriate,
> on their own. There is really no harm in having
> %postun
> systemctl try-reload foobar.service || :
>
> We *could* wrap this in a macro, but it's trivial and package-specific
> anyway.
I feel that having packages call systemctl directly instead of going
through the %systemd_* macros is an anti-pattern that shouldn't be
encouraged.
That's a valid point.
The current set of macros already includes both %systemd_postun and
%systemd_postun_with_restart, so adding %systemd_postun_with_reload
or something along those lines doesn't seem like a stretch.
→ https://github.com/systemd/systemd/pull/28521
> > > > +%post -n kdump-tools
> > > > # Initial installation
> > > > %systemd_post kdump.service
> > >
> > > This one is tricky. %systemd_post presets the service on "first
installation",
> > > which is actually the first the time package is installed. I.e. it
unfortunately
> > > also would execute the preset on upgrades that split out a subpackage,
because
> > > as far as rpm is concerned, this is the initial installation of that
subpackage.
> > >
> > > The righteous way to solve this would be something like this:
> > >
> > > %triggerprein -n kdump-tools -- kexec-tools < 2.0.26-8
> > > touch %{_localstatedir}/lib/rpm-state/kexec-tools.no-preset
> > >
> > > %post -n kdump-tools
> > > rm %{_localstatedir}/lib/rpm-state/kexec-tools.no-preset 2>/dev/null
&& return 0
> > > # Initial installation
> > > %systemd_post kdump.service
> > >
> > > A bit of a bother, but at least nobody will be suprised by
> > > kdump.service changing state.
Looking at this again, I can rely on the fact that %triggerprein will
run before %post, right? It looks like that based on what I know
about scriptlets execution order and your example, but I want to be
sure.
Yes, I think so.
https://rpm-software-management.github.io/rpm/manual/triggers.html#order-...
seems to say so too.
What version number should I use in my case? The package split
happened in 9.1.0-1, but this handling is going to land in 9.6.0-1 at
the earliest so I think think using this latter version number makes
more sense, right? If you have somehow already upgraded to, say,
9.3.0-1 in the meantime, then your deployment is already broken and
there's not much that we can do about it anyway.
Yes.
> > The beauty of the "check at %pre time whether the unit
already exists
> > on disk" approach is that it should work out of the box in all
> > scenarios, with no further information needed. Do you see a problem
> > with it that I have failed to consider?
>
> I think it's an interesting approach and it sounds like it should
> work. Nevertheless, it's a big departure from what we used before, so
> there might be some corner cases I'm not seeing.
That's already encouraging :)
> I wonder if it would be possible to invert the approach, and have a
> %transfiletriggerun that'd mark various using to not require a preset,
> and then just do the preset for anything that was added later.
I'm not sure I quite understand what you're suggesting.
Wouldn't that part be under systemd's control? AFAICT the current
implementation uses %transfiletriggerpostun to restart services that
have been marked as needing a restart, but the task of marking them
as such still falls on the package.
For presets, we don't have a --marked option that would allow
applying everything in one go. I actually don't think that we would
want that to happen anyway: in the case of libvirt, for example, we
need presets for virtqemud.socket to be applied before presets for
virtqemud.service, because virtqemud.socket is disabled as per the
system-wide default and virtqemud.service is explicitly enabled, but
at the same time virtqemud.service has Also=virtqemud.socket in its
[Install] section and we want the result to be both being enabled.
In general, wouldn't this kind of approach require marking units with
preset-already-applied (or similar) and carrying that annotation for
the lifetime of the system? That seems worse than marking units as
preset-needed (or similar) for just the short time between %pre and
%posttrans.
My idea was that we'd preset all units. The guidelines say that units
should be enabled through presets, so effectively, all units should
either use presets or have no enablement, in which case the preset has
no effect. But looking at this again, I don't think it's worth the
trouble, because we'd still need some of the other scriptlets, so
the spec files wouldn't become much simpler.
Zbyszek
5:06 a.m.
On Wed, Jul 26, 2023 at 07:15:42AM +0000, Zbigniew Jędrzejewski-Szmek wrote:
On Tue, Jul 25, 2023 at 10:45:30AM -0400, Andrea Bolognani wrote:
> On Tue, Jul 25, 2023 at 10:51:04AM +0000, Zbigniew Jędrzejewski-Szmek wrote:
> > I think it'd be more effiecient to go with the (slightly ugly, no
> > disagreement here) seven line %trigger scriptlet. And and then work
> > with upstream to implement a generic solution that can be used by all
> > packages. It seems to me that the work put into implementing a custom
> > solution in libvirt would be wasted if you want to work with upstream
> > on a completely different general solution soon after.
>
> Another way to look at it is that I *already* put all that work in,
> and the solution I've proposed for libvirt has been tested quite
> thoroughly in a number of install and upgrade scenarios. In order to
> adopt the %trigger based solution, I will have to spend more time on
> developing and testing another implementation.
That's fine. I only saw some links to emails, and it didn't know that
those have already been applied and tested. My goal was to minimize
overall effort, not to increase it ;)
They've been tested and, as of yesterday, fully reviewed, but not yet
applied.
If I understand correctly and you have no strong objections to them
being merged, I think I will stick to my original plan of addressing
the immediate issue in libvirt by pushing those patches, and work on
a more long-term solution that lives in systemd as a follow-up
instead of investigating the %trigger route further.
The lack of strong objections on the Fedora side will hopefully
assuage Daniel's concerns as well.
> I feel that having packages call systemctl directly instead of
going
> through the %systemd_* macros is an anti-pattern that shouldn't be
> encouraged.
That's a valid point.
> The current set of macros already includes both %systemd_postun and
> %systemd_postun_with_restart, so adding %systemd_postun_with_reload
> or something along those lines doesn't seem like a stretch.
→ https://github.com/systemd/systemd/pull/28521
Neat!
--
Andrea Bolognani / Red Hat / Virtualization
3:34 a.m.
On Wed, Jul 26, 2023 at 06:06:45AM -0400, Andrea Bolognani wrote:
On Wed, Jul 26, 2023 at 07:15:42AM +0000, Zbigniew Jędrzejewski-Szmek
wrote:
> > %systemd_postun_with_restart, so adding %systemd_postun_with_reload
> > or something along those lines doesn't seem like a stretch.
>
> → https://github.com/systemd/systemd/pull/28521
This is now merged. The new macros are:
%systemd_postun_with_reload, %systemd_user_postun_with_reload,
%systemd_user_daemon_reexec. That last one is now used in
the systemd.spec file in rawhide to properly reexec user@.service
instances after package upgrade.
Somebody needs to file an FPC pull request now ;)
Zbyszek
6:01 a.m.
On Wed, Aug 09, 2023 at 08:34:59AM +0000, Zbigniew Jędrzejewski-Szmek wrote:
On Wed, Jul 26, 2023 at 06:06:45AM -0400, Andrea Bolognani wrote:
> On Wed, Jul 26, 2023 at 07:15:42AM +0000, Zbigniew Jędrzejewski-Szmek wrote:
> > > %systemd_postun_with_restart, so adding %systemd_postun_with_reload
> > > or something along those lines doesn't seem like a stretch.
> >
> > → https://github.com/systemd/systemd/pull/28521
This is now merged. The new macros are:
%systemd_postun_with_reload, %systemd_user_postun_with_reload,
%systemd_user_daemon_reexec. That last one is now used in
the systemd.spec file in rawhide to properly reexec user@.service
instances after package upgrade.
Somebody needs to file an FPC pull request now ;)
https://pagure.io/packaging-committee/pull-request/1301
Reviews welcome ;)
Zbyszek
11:04 a.m.
On Mon, Sep 11, 2023 at 11:01:17AM +0000, Zbigniew Jędrzejewski-Szmek wrote:
On Wed, Aug 09, 2023 at 08:34:59AM +0000, Zbigniew Jędrzejewski-Szmek
wrote:
> This is now merged. The new macros are:
> %systemd_postun_with_reload, %systemd_user_postun_with_reload,
> %systemd_user_daemon_reexec. That last one is now used in
> the systemd.spec file in rawhide to properly reexec user@.service
> instances after package upgrade.
>
> Somebody needs to file an FPC pull request now ;)
https://pagure.io/packaging-committee/pull-request/1301
Reviews welcome ;)
FWIW, changes look reasonable to me.
I still intend to extract the libvirt macros, make them more generic,
polish them and submit the result to systemd upstream. I just haven't
been able to carve time to do that yet, but it's reasonably high on
my TODO list :)
--
Andrea Bolognani / Red Hat / Virtualization
248
days inactive
302
days old
12 comments
3 participants
participants (3)
-
Andrea Bolognani -
Kevin Fenzi -
Zbigniew Jędrzejewski-Szmek