Landscape Client

Bug #2006402
Comment #11

Comment 11 for bug 2006402

Revision history for this message

Steve Langasek (vorlon) wrote on 2023-06-23:

#11

> In addition, it's usually required to explicitly mark out all the fixed bugs
> in the changelog entry via a LP: #XXXXXXX stanza so that their status can be
> automatically updated as the package goes through the motions.

I'm sorry, but this is upside-down advice for SRUs of packages with documented SRU exception processes. Linking all of the bugs in the changelog means that the SRU process will enforce verification of each of those bugs, instead of using this single bug report for the SRU as expected. There is an ongoing discussion about certain cases where additional launchpad bug refs naturally wind up in the changelog of packages with SRU exceptions and what to do there <https://lists.ubuntu.com/archives/ubuntu-release/2023-June/005656.html>, but this isn't one of those cases, you've explicitly gone back and added them.

I would just edit the changelog to remove the launchpad bug refs and upload, *except*, that there are changes in the packaging under debian/. There is some discussion on https://wiki.ubuntu.com/LandscapeUpdates about packaging, which is good, but it's my position as a member of the SRU team (a position I believe is widely shared by the SRU team despite not being explicitly documented) that packaging changes never fall under the SRU exception policy but require their own validation in the SRU process.

In particular:

override_dh_installsystemd:
- dh_installsystemd --no-enable --no-start
+ dh_installsystemd --no-enable --no-start --no-stop-on-upgrade

Unfortunately dh_installsystemd is, in a word, a trainwreck. Most of the options have semantics that are unintuitive from their names, and many have inscrutable side effects. To sponsor this, I would need a separate bug that explains why you are making this change, what you expect the effect to be, and what testing will be done to confirm correct behavior across the various relevant scenarios (new install; upgrade with landscape client running; upgrade with landscape client not running).

At a glance, I would *infer* that the reason for this change is that the upgrade of landscape-client may be happening under landscape-client itself, therefore stopping it on upgrade could leave the system in an inconsistent state. And that makes sense, but also if we're not restarting the client on upgrade that means we don't get security updates applied to the running process... Ideally we would have something like openssh, where a service restart will restart the daemon but not any running sessions.

Then you also have:

- [ -f /var/run/landscape/landscape-client.pid ] && kill -s USR1 `cat /var/run/landscape/landscape-client.pid` > /dev/null 2>&1 || :
+ systemctl kill --signal=USR1 --kill-who=main landscape-client > /dev/null 2>&1 || :

That's straightforward and explained by the linked bug #1968189. However, 'systemctl kill' is a rather surprising interface to use. I see that SIGUSR1 is used to tell landscape-client to reopen its logs after rotation, so it makes sense that you don't want to use 'systemctl restart' to restart the whole service; but this is fairly non-obvious. Could you add a comment explaining this rationale for not using 'systemctl restart', either to the changelog or to the linked bug?

I believe with those two points addressed I would be happy to sponsor this (I've only looked at the jammy debdiff so far but I'm assuming the focal debdiff is equivalent). Unsubscribing ubuntu-sponsors for now, please re-subscribe when ready for re-review.

I'm sorry, but this is upside-down advice for SRUs of packages with documented SRU exception processes.  Linking all of the bugs in the changelog means that the SRU process will enforce verification of each of those bugs, instead of using this single bug report for the SRU as expected.  There is an ongoing discussion about certain cases where additional launchpad bug refs naturally wind up in the changelog of packages with SRU exceptions and what to do there <https://lists.ubuntu.com/archives/ubuntu-release/2023-June/005656.html>, but this isn't one of those cases, you've explicitly gone back and added them.

I would just edit the changelog to remove the launchpad bug refs and upload, *except*, that there are changes in the packaging under debian/.  There is some discussion on https://wiki.ubuntu.com/LandscapeUpdates about packaging, which is good, but it's my position as a member of the SRU team (a position I believe is widely shared by the SRU team despite not being explicitly documented) that packaging changes never fall under the SRU exception policy but require their own validation in the SRU process.

In particular:

override_dh_installsystemd:
-       dh_installsystemd --no-enable --no-start
+       dh_installsystemd --no-enable --no-start --no-stop-on-upgrade

Unfortunately dh_installsystemd is, in a word, a trainwreck.  Most of the options have semantics that are unintuitive from their names, and many have inscrutable side effects.  To sponsor this, I would need a separate bug that explains why you are making this change, what you expect the effect to be, and what testing will be done to confirm correct behavior across the various relevant scenarios (new install; upgrade with landscape client running; upgrade with landscape client not running).

At a glance, I would *infer* that the reason for this change is that the upgrade of landscape-client may be happening under landscape-client itself, therefore stopping it on upgrade could leave the system in an inconsistent state.  And that makes sense, but also if we're not restarting the client on upgrade that means we don't get security updates applied to the running process... Ideally we would have something like openssh, where a service restart will restart the daemon but not any running sessions.

Then you also have:

-        [ -f /var/run/landscape/landscape-client.pid ] && kill -s USR1 `cat /var/run/landscape/landscape-client.pid` > /dev/null 2>&1 || :
+        systemctl kill --signal=USR1 --kill-who=main landscape-client > /dev/null 2>&1 || :

That's straightforward and explained by the linked bug #1968189.  However, 'systemctl kill' is a rather surprising interface to use.  I see that SIGUSR1 is used to tell landscape-client to reopen its logs after rotation, so it makes sense that you don't want to use 'systemctl restart' to restart the whole service; but this is fairly non-obvious.  Could you add a comment explaining this rationale for not using 'systemctl restart', either to the changelog or to the linked bug?

I believe with those two points addressed I would be happy to sponsor this (I've only looked at the jammy debdiff so far but I'm assuming the focal debdiff is equivalent).  Unsubscribing ubuntu-sponsors for now, please re-subscribe when ready for re-review.