Hi Andrea, On Wed, Aug 28, 2024 at 12:17:05AM +0200, Andrea Bolognani wrote: > I read up a bit on the various problems and mitigations documented as > part of DEP17 yesterday and figured that our changes would probably > hit P1. I hoped that M7 might be enough to take care of that, but I > trust your expertise when you say that M7+M8 is a better fit for our > specific case.
I confirm. M7 should work most of the time. In particular, when the tool used to perform the upgrades is apt or based on apt (such as aptitude), which is probably like 99% of the upgrades. However, there is a chance of Conflicts not working as expected (DEP17 P12) when upgrading using dpkg. There also is a thin chance of apt invoking dpkg in that bad way in more complex upgrade scenarios. Hence, we recommend to combine M8 for packages with important functions. > The fact that the diversions will only exist for the duration of the > upgrade, instead of sticking around afterwards, makes this approach > seem particularly attractive to me. In other cases where we want to avoid Conflicts (e.g. in the essential set), the M7 protective diversions have to persist longer. Hence the proposal of combining. > So this part is as simple as replacing the existing > > Breaks: libvirt-daemon-system (<< 10.6.0-2~) > Replaces: libvirt-daemon-system (<< 10.6.0-2~) > > with > > Conflicts: libvirt-daemon-system (<< 10.6.0-2~) > > in the case of libvirt-daemon-log, right? Or are there additional > considerations? This looks good. Will you do the same change for libvirt-daemon, libvirt-daemon-common, and libvirt-daemon-lock? > I don't fully understand how that would help avoid the file loss > scenario, but once again I trust your expertise here so thankfull I > don't really need to O:-) In case, you are interested let me add detail. Otherwise skip. When dpkg is faced with unpacking a package that declares Conflicts with another package that is scheduled for removal, it may proceed with the unpack before the other package is actually removed. Thus there is a brief moment when both are unpacked concurrently despite having declared Conflicts. When the other package is removed, dpkg observes that e.g. /lib/systemd/system/virtlogd.socket is not installed by any other package and hence removes this file not realizing that it aliases /usr/lib/systemd/system/virtlogd.socket, which should be kept. The diversion is installed in preinst (hence before unpacking) and removed in postinst (hence after removing the other package) and thus covers this brief concurrency window. The diversion then redirects the removal of /lib/systemd/system/virtlogd.socket to a location that usually does not exist and thus saves /usr/lib/systemd/system/virtlogd.socket. > > Hence, lintian may be annoyed. > > That's fine, we can install overrides. That'd be more than warranted > in this case. I appreciate if you handle adding the relevant overrides. > I believe you're better positioned to get the details of the > implementation right, but I obviously have my own preferences when it > comes to what the code that gets committed to libvirt looks like. > > So for now I've sketched out an implementation, which you can find > below. The calls to dpkg-divert are most likely completely wrong, and > I'd appreciate it if you could fix them. Actually, that looks like it mostly just works. > Up to you whether you want to go all the way and prepare a MR with > the changes, or leave that last bit to me. You'll obviously be > credited in debian/changelog either way :) Let me do the review part here given that you already did most of the work and given that a number of remarks have multiple options and I don't happen to know your preference. > diff --git a/debian/libvirt-daemon-log.postinst.in > b/debian/libvirt-daemon-log.postinst.in > index 21723d4307..d75b30d930 100644 > --- a/debian/libvirt-daemon-log.postinst.in > +++ b/debian/libvirt-daemon-log.postinst.in > @@ -15,6 +15,7 @@ set -e > # the debian-policy package > > #FINISH_CONFFILE_TRANSFER# > +#DELETE_PROTECTIVE_DIVERSION# If you happen to enjoy merge requests originating from the Debian Janitor, you may wrap all the added code like this: # begin-remove-after: released:trixie #DELETE_PROTECTIVE_DIVERSION# # end-remove-after This serves both as a human-readable reminder for removing the code eventually, but it also is machine-readable and will make the Debian Janitor propose a MR for removing this code at the desired time. I'll leave it up to you whether you want to add support for this or not. If you do, please wrap all relevant code to avoid a partial removal. > DAEMON_SYSTEM_TO_DAEMON_LOG=" > /etc/default/virtlogd > @@ -24,6 +25,12 @@ DAEMON_SYSTEM_SYSV_TO_DAEMON_LOG=" > /etc/init.d/virtlogd > " > > +VIRTLOGD_UNITS=" > + virtlogd-admin.socket > + virtlogd.service > + virtlogd.socket > +" > + > case "$1" in > configure) > for conf in $DAEMON_SYSTEM_TO_DAEMON_LOG; do > @@ -46,6 +53,13 @@ case "$1" in > -- \ > "$@" > done > + for unit in $VIRTLOGD_UNITS; do > + delete_protective_diversion \ > + "/lib/systemd/system/$unit" \ > + "10.6.0-3~" \ > + -- \ > + "$@" > + done I suggest running this snippet also in the abort-upgrade case. Also add these calls to postrm matching the arguments failed-upgrade, abort-install, and abort-upgrade. Matching all of these should ensure that the protective diversions are always removed at the end of a dpkg transaction and thus you do not have to clean them up in a later upload. > ;; > > abort-upgrade|abort-remove|abort-deconfigure) > diff --git a/debian/libvirt-daemon-log.preinst.in > b/debian/libvirt-daemon-log.preinst.in > new file mode 100644 > index 0000000000..f5dbf73de9 > --- /dev/null > +++ b/debian/libvirt-daemon-log.preinst.in > @@ -0,0 +1,45 @@ > +#!/bin/sh > + > +set -e > + > +# summary of how this script can be called: > +# > +# * <new-preinst> `install' > +# * <new-preinst> `install' <old-version> <new-version> > +# * <new-preinst> `upgrade' <old-version> <new-version> > +# * <old-preinst> `abort-upgrade' <new-version> > +# > +# for details, see https://www.debian.org/doc/debian-policy/ or > +# the debian-policy package > + > +#CREATE_PROTECTIVE_DIVERSION# > + > +VIRTLOGD_UNITS=" > + virtlogd-admin.socket > + virtlogd.service > + virtlogd.socket > +" > + > +case "$1" in > + install|upgrade) > + for unit in $VIRTLOGD_UNITS; do > + create_protective_diversion \ > + "/lib/systemd/system/$unit" \ > + "10.6.0-3~" \ > + -- \ > + "$@" > + done > + ;; > + > + abort-upgrade) > + ;; > + > + *) > + echo "preinst called with unknown argument \`$1'" >&2 > + exit 1 > + ;; > +esac > + > +#DEBHELPER# > + > +exit 0 You have handled the libvirt-daemon-log units. The dumat report mentioned three other packages libvirt-daemon, libvirt-daemon-common, and libvirt-daemon-lock. Will you repeat the maintainer scripts for them and their mentioned units in the dumat report? I hope it is fairly obvious how to apply this. > diff --git a/debian/snippets.sh b/debian/snippets.sh > index 6b03f242d6..ad86432332 100644 > --- a/debian/snippets.sh > +++ b/debian/snippets.sh > @@ -194,6 +194,54 @@ remove_config_from_template() { > } > #END REMOVE_CONFIG_FROM_TEMPLATE > > +#BEGIN CREATE_PROTECTIVE_DIVERSION > +create_protective_diversion() { > + local usrfile="$1" > + local firstver="$2" > + > + if [ "$2" != "--" ]; then Do you mean $3 here? I think $2 is firstver. > + echo "create_protective_diversion called with the wrong number of > arguments" >&2 > + return 1 > + fi > + for _ in $(seq 1 2); do > + shift > + done > + > + if [ -z "$2" ] || dpkg --compare-versions -- "$2" gt "$lastver"; then > + return 0 > + fi Please remove this check. For instance, libvirt-daemon-common does not exist in bookworm. Hence $2 will be empty and you'd skip the addition of these diversions when installing libvirt-daemon-common, but they're really needed on such a first installation. What you could check here is the version of libvirt-daemon-system (the package we are moving the units from), but the added value in doing so does not warrant the added complexity in my opinion. > + > + dpkg-divert \ > + --no-rename \ > + --divert "$usrfile.usr-is-merged" \ > + --add "$usrfile" > +} > +#END CREATE_PROTECTIVE_DIVERSION > + > +#BEGIN DELETE_PROTECTIVE_DIVERSION > +delete_protective_diversion() { > + local usrfile="$1" > + local firstver="$2" > + > + if [ "$2" != "--" ]; then Do you mean $3 here? I think $2 is firstver. > + echo "delete_protective_diversion called with the wrong number of > arguments" >&2 > + return 1 > + fi > + for _ in $(seq 1 2); do > + shift > + done > + > + if [ ! -e "$usrfile.usr-is-merged" ]; then > + return 0 > + fi Please remove this check. The diverted file is expected to never exist. The sole point of the diversion is to redirect the removal of the file to a harmless location. This check would result in the diversions never getting removed. If you feel like you need to add a condition, consider using this: if [ -z "$(dpkg-divert --list "$usrfile")" ]; then return 0 fi > + dpkg-divert \ > + --no-rename \ > + --divert "$usrfile.usr-is-merged" \ > + --remove "$usrfile" > +} > +#END DELETE_PROTECTIVE_DIVERSION > + > #BEGIN SYSTEMD_DAEMON_RELOAD > systemd_daemon_reload() { > if [ -z "${DPKG_ROOT:-}" ] && [ -d /run/systemd/system ]; then Does your salsa-ci pipeline run piuparts? If not, consider running it locally before uploading to experimental. It should catch the worst of issues. Helmut
