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/#_systemd > > 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 _______________________________________________ devel mailing list -- devel@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/devel@xxxxxxxxxxxxxxxxxxxxxxx Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
