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/#_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. 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 _______________________________________________ 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
