Bug 968405

This is indeed correct and I don't think this behaviour has been there before.
We do set 'export DISABLE_RESTART_ON_UPDATE=yes' as this this issue has been fixed with bsc#886641 already and I don't see any changes in xdm recently which may have broken this.
So maybe there is an issue with the %postun macro?

Maybe, but it's definitely not a rpm bug. So assigning back to you...

I meant to say the %service_del_postun macro defined in /usr/lib/rpm/macros.
The reason why DISABLE_RESTART_ON_UPDATE=yes has no effect at all is that this variable is set by a script /etc/sysconfig/services which is sourced before the variable is evaluated.
Since the intended use of this variable is tantamount to not to execute %restart_on_update and thus %service_del_postun at all, let's remove this line entirely.

Submitted for X11:XOrg, Factory.
MR ID  for 13.2 and Leap 42.1: 361763

This is an autogenerated message for OBS integration:
This bug (968405) was mentioned in
https://build.opensuse.org/request/show/361706 Factory / xdm
https://build.opensuse.org/request/show/361763 13.2+42.1 / xdm

(In reply to Egbert Eich from comment #3)
> I meant to say the %service_del_postun macro defined in /usr/lib/rpm/macros.
> The reason why DISABLE_RESTART_ON_UPDATE=yes has no effect at all is that
> this variable is set by a script /etc/sysconfig/services which is sourced
> before the variable is evaluated.
> Since the intended use of this variable is tantamount to not to execute
> %restart_on_update and thus %service_del_postun at all, let's remove this
> line entirely.

This only matches reality in the update case, but not in a possible 'uninstall' case, which is also handled by the macro line.

I'd much rather see the macro fixed rather than packages going on and removing the macros.

Reopening - assigning to systemd maintainer.

The current implementation of the script change is 'flawed' imho.

Some packages just 'know' that restarting their service during update is a bad idea, whatever any random config file might say (services like display-manager or PackageKit).

Those already properly export the variable - in both cases for a good reason. Overruling this with a config file, giving one entry for the entire system, is not a good idea.

The regression was introduced with https://build.opensuse.org/package/rdiff/openSUSE:Factory/systemd-rpm-macros?linkrev=base&rev=14 (thanks fcrozat for finding it)

This is an autogenerated message for OBS integration:
This bug (968405) was mentioned in
https://build.opensuse.org/request/show/361832 42.1+13.2 / xdm

(In reply to Dominique Leuenberger from comment #9)

Then explain how we solve both this bug and bsc#955996

If one use DISABLE_STOP_ON_REMOVAL != yes the nhe gets what has been requested.

I'll not change back to solve this bug and open the other here.

IHMO, this macro line was just for the update case, not for the unistall case. The uninstall case should be handled by 
%preun
%if !0%{?suse_version} < 1315
  %service_del_preun display-manager.service
%endif
Ie. we terminate the service before we remove the package. 
I'm not sure why this would not trigger in case of update also - at least according to 
http://www.rpm.org/max-rpm/s1-rpm-specref-scripts.html
%preun/%postun run before/after the erase.

However judging from
%service_del_preun() %{stop_on_removal %{1}}
%service_del_postun() %{restart_on_update %{1}}
taken from /usr/lib/rpm/macros 'preun' is related to removal while 'postun' is related to an update scenario.

(In reply to Dr. Werner Fink from comment #11)
> (In reply to Dominique Leuenberger from comment #9)
> 
> Then explain how we solve both this bug and bsc#955996
> 
> If one use DISABLE_STOP_ON_REMOVAL != yes the nhe gets what has been
> requested.
> 
> I'll not change back to solve this bug and open the other here.

Possibly with two macros then? You can have a system wide default (which is in normal instances to RESTART THE SERVICE) and a per -package override...

As for the other bug: if anybody would have told the customer to export DISABLE_RESTART_ON_UPDATE=yes' instead of =1, the whole thing would probably have been solved too :)

(In reply to Dominique Leuenberger from comment #13)

> As for the other bug: if anybody would have told the customer to export
> DISABLE_RESTART_ON_UPDATE=yes' instead of =1, the whole thing would probably
> have been solved too :)

yes, the wiki is wrong - the code is right... the code only compares(/compared) the value against 'yes', '1' is not evaluated to true

(In reply to Dr. Werner Fink from comment #11)
> (In reply to Dominique Leuenberger from comment #9)
> 
> Then explain how we solve both this bug and bsc#955996
> 
> If one use DISABLE_STOP_ON_REMOVAL != yes the nhe gets what has been
> requested.
> 
> I'll not change back to solve this bug and open the other here.

I don't think we can. No matter what, if %preun and %postun are run both during update and removal, we just cannot take down the display manager.
In fact, I'd argue that even during removal it would be unfriendly to kill a running session when the xdm package is removed. So the only time when you can safely terminate it is when no user is logged in.
Once the display-manager is terminated it won't restart if xdm is gone, but as long as it is running it will stay running. Even when the user logs out, a DM isn't restarted, thus even though xdm has been removed or updated, the old state remains running.

(In reply to Dominique Leuenberger from comment #14)

/etc/sysconfig/services is the only true documentation:

 # Type:        yesno
 ## Default:     no
 #
 # Do you want to disable the automatic restart of services when
 # a new version gets installed?
 #
 DISABLE_RESTART_ON_UPDATE="no"

 ## Type:        yesno
 ## Default:     no
 #
 # Do you want to disable the automatic shutdown of services when
 # the corresponding package gets erased?
 #
 DISABLE_STOP_ON_REMOVAL="no"

Don't know which wiki says something different here.

(In reply to Egbert Eich from comment #15)
> I don't think we can. No matter what, if %preun and %postun are run both
> during update and removal, we just cannot take down the display manager.
> In fact, I'd argue that even during removal it would be unfriendly to kill a
> running session when the xdm package is removed. So the only time when you
> can safely terminate it is when no user is logged in.
> Once the display-manager is terminated it won't restart if xdm is gone, but
> as long as it is running it will stay running. Even when the user logs out,
> a DM isn't restarted, thus even though xdm has been removed or updated, the
> old state remains running.

Note: this bug reaches further than XDM - see also the bug I marked as blockedby.
 Let's not invent solution per package, but let's make standard that work

(In reply to Dr. Werner Fink from comment #16)
> (In reply to Dominique Leuenberger from comment #14)
> 
> /etc/sysconfig/services is the only true documentation:
> 
>  # Type:        yesno
>  ## Default:     no
>  #
>  # Do you want to disable the automatic restart of services when
>  # a new version gets installed?
>  #
>  DISABLE_RESTART_ON_UPDATE="no"
> 
>  ## Type:        yesno
>  ## Default:     no
>  #
>  # Do you want to disable the automatic shutdown of services when
>  # the corresponding package gets erased?
>  #
>  DISABLE_STOP_ON_REMOVAL="no"
> 
> Don't know which wiki says something different here.

You're trying hard, aren't you?

YOU referenced bug https://bugzilla.suse.com/show_bug.cgi?id=955996#c3 (I now linked to the comment for you - sorry folks, it's a SLE bug)

A file is no wiki - you know the difference very well.

And the wiki article in question (referenced in https://bugzilla.suse.com/show_bug.cgi?id=955996#c3 ) is https://en.opensuse.org/openSUSE:Systemd_packaging_guidelines

*** Bug 963318 has been marked as a duplicate of this bug. ***

(In reply to Dominique Leuenberger from comment #17)
> (In reply to Egbert Eich from comment #15)
> > I don't think we can. No matter what, if %preun and %postun are run both
> > during update and removal, we just cannot take down the display manager.
> > In fact, I'd argue that even during removal it would be unfriendly to kill a
> > running session when the xdm package is removed. So the only time when you
> > can safely terminate it is when no user is logged in.
> > Once the display-manager is terminated it won't restart if xdm is gone, but
> > as long as it is running it will stay running. Even when the user logs out,
> > a DM isn't restarted, thus even though xdm has been removed or updated, the
> > old state remains running.
> 
> Note: this bug reaches further than XDM - see also the bug I marked as
> blockedby.
>  Let's not invent solution per package, but let's make standard that work

Dominique, please, let's discuss what we really need for xdm and then see if this applies to other packages as well.

(In reply to Dominique Leuenberger from comment #18)

> You're trying hard, aren't you?

Indeed I do!

Beside this, the wiki has to be fixed!

(In reply to Dr. Werner Fink from comment #21)

> Beside this, the wiki has to be fixed!

THAT point we agree on! Thank you (now I will take care of fixing the wiki)

(In reply to Dominique Leuenberger from comment #22)
> (In reply to Dr. Werner Fink from comment #21)
> 
> > Beside this, the wiki has to be fixed!
> 
> THAT point we agree on! Thank you (now I will take care of fixing the wiki)

(actually, with the current marcos, the paragraph would need to be removed completely, as the package has no chance of overriding the sysconfig value - Let's first come to a conclusion of this part and the fixup the documentation)

(In reply to Egbert Eich from comment #20)

> > Note: this bug reaches further than XDM - see also the bug I marked as
> > blockedby.
> >  Let's not invent solution per package, but let's make standard that work
> 
> Dominique, please, let's discuss what we really need for xdm and then see if
> this applies to other packages as well.

Egbert, I would change the nature of this bug to what it is: systemd no longer honors the documented (on the wiki) way for a package to instruct it not to restart a service on update

(In reply to Dominique Leuenberger from comment #24)
> (In reply to Egbert Eich from comment #20)
> 
> > > Note: this bug reaches further than XDM - see also the bug I marked as
> > > blockedby.
> > >  Let's not invent solution per package, but let's make standard that work
> > 
> > Dominique, please, let's discuss what we really need for xdm and then see if
> > this applies to other packages as well.
> 
> Egbert, I would change the nature of this bug to what it is: systemd no
> longer honors the documented (on the wiki) way for a package to instruct it
> not to restart a service on update

This could be fixed easily - ie by testing the env variable also before we source the syconfig/services file. 

However the macros you mentioned in comment #9 (ie the ones from systemd-rpm-macros) did not play a role in this ticket at all. 
The macros which play a role here are from the rpm package and admittedly exhibit the same behaviour as the ones from comment #9.

We need to address all of these cases therefore I'm going to include the maintainer of RPM again.

Besides this - you are talking about 'update' - how do you distinguish 'update' from 'removal' using the '%pre/%postun' scriptlets?
This may not even matter for the case at hand but since you are looking at the 'general' case, you may have to.

(In reply to Egbert Eich from comment #25)
> This could be fixed easily - ie by testing the env variable also before we
> source the syconfig/services file. 

agreed - one solution
 
> However the macros you mentioned in comment #9 (ie the ones from
> systemd-rpm-macros) did not play a role in this ticket at all. 
> The macros which play a role here are from the rpm package and admittedly
> exhibit the same behaviour as the ones from comment #9.

Good pointer - thanks, but:

> We need to address all of these cases therefore I'm going to include the
> maintainer of RPM again.

Indeed, we have two places failing through the same loop:

%service_del_postun is defined in two places:
/etc/rpm/macros.systemd
/usr/lib/rpm/macros

Looking at xdm, you got the one from /etc/rpm/macros.systemd (thus, the entire discussion so far stays true)

> Besides this - you are talking about 'update' - how do you distinguish
> 'update' from 'removal' using the '%pre/%postun' scriptlets?
> This may not even matter for the case at hand but since you are looking at
> the 'general' case, you may have to.

/etc/rpm/macros.systemd's %service_del_postun expands to:
# On uninstall, tell systemd to reload its unit files
%service_del_postun()                                                                   \
test -n "$FIRST_ARG" || FIRST_ARG="$1"                                                  \
if [ "$FIRST_ARG" -ge 1 ]; then                                                         \
        # Package upgrade, not uninstall                                                \
        if [ -x /usr/bin/systemctl ]; then                                              \
                /usr/bin/systemctl daemon-reload || :                                   \
                ( %_restart_check_systemctl %{?*} || : )                                \
        fi                                                                              \
else # package uninstall                                                                \
        for service in %{?*} ; do                                                       \
                sysv_service="${service%.*}"                                            \
                rm -f "/var/lib/systemd/migrated/$sysv_service" || :                    \
        done                                                                            \
        if [ -x /usr/bin/systemctl ]; then                                              \
                /usr/bin/systemctl daemon-reload || :                                   \
        fi                                                                              \
fi                                                                                      \
%{nil}


Updated vs uninstall is handled in the script.

for reference, %_restart_check_systemctl expands to:

%_restart_check_systemctl                                                               \
        test -f /etc/sysconfig/services && . /etc/sysconfig/services                    \
        test "$YAST_IS_RUNNING" = instsys       && exit 0                               \
        test "$DISABLE_RESTART_ON_UPDATE" = yes && exit 0                               \
        /usr/bin/systemctl try-restart %{nil}

And here lies the problem - export DUSABLE_RESTART_ON_UPDATE=yes (as set by xdm.spec) is overwritten by sourcing /etc/sysconfig/services and the system default being taken into account - resulting in the issue we now discuss / hopefully try to fix.

I'm also in favor of checking another environment variable, e.g.
DISABLE_RESTART_ON_UPDATE_OVERRIDE

(In reply to Michael Schröder from comment #27)
> I'm also in favor of checking another environment variable, e.g.
> DISABLE_RESTART_ON_UPDATE_OVERRIDE

Just for referenceL

> osc/openSUSE:Factory> find -maxdepth 2 -name '*.spec' -exec grep -H "export.*DISABLE_RESTART_ON_UPDATE" {} \;
./apparmor/apparmor.spec:export DISABLE_RESTART_ON_UPDATE="yes"
./firewalld/firewalld.spec:export DISABLE_RESTART_ON_UPDATE=1
./xen/xen.spec:export DISABLE_RESTART_ON_UPDATE=yes
./PackageKit/PackageKit.spec:export DISABLE_RESTART_ON_UPDATE=yes
./xdm/xdm.spec:export DISABLE_RESTART_ON_UPDATE=yes

Those packages need to be touched if the variable name changed - and we have to be careful to keep them working for older releases in case some of the maintainers care.

(I wonder who came up with that export DISABLE_RESTART_ON_UPDATE idea. It never worked with the pre-systemd macros and only worked with systemd by mistake. The scriptlets always have to source the /etc/sysconfig/services file, which will overwrite the env var...)

(In reply to Dominique Leuenberger from comment #28)
> ./xen/xen.spec:export DISABLE_RESTART_ON_UPDATE=yes

Nothing in xen-tools can be restarted, as long as there are active domUs and backends.

(In reply to Dominique Leuenberger from comment #26)
> 
> %service_del_postun is defined in two places:
> /etc/rpm/macros.systemd
> /usr/lib/rpm/macros

Ah, I didn't even notice that. It would be nice if we had the chance to consolidate this in the process.

> Looking at xdm, you got the one from /etc/rpm/macros.systemd (thus, the
> entire discussion so far stays true)

Right.

> > Besides this - you are talking about 'update' - how do you distinguish
> > 'update' from 'removal' using the '%pre/%postun' scriptlets?
> > This may not even matter for the case at hand but since you are looking at
> > the 'general' case, you may have to.
> 
> /etc/rpm/macros.systemd's %service_del_postun expands to:
> # On uninstall, tell systemd to reload its unit files
> %service_del_postun()                                                       
> \
> test -n "$FIRST_ARG" || FIRST_ARG="$1"                                      
> \
> if [ "$FIRST_ARG" -ge 1 ]; then                                             
> \
>         # Package upgrade, not uninstall                                    
> \
>         if [ -x /usr/bin/systemctl ]; then                                  
> \
>                 /usr/bin/systemctl daemon-reload || :                       
> \
>                 ( %_restart_check_systemctl %{?*} || : )                    
> \
>         fi                                                                  
> \
> else # package uninstall                                                    
> \
>         for service in %{?*} ; do                                           
> \
>                 sysv_service="${service%.*}"                                
> \
>                 rm -f "/var/lib/systemd/migrated/$sysv_service" || :        
> \
>         done                                                                
> \
>         if [ -x /usr/bin/systemctl ]; then                                  
> \
>                 /usr/bin/systemctl daemon-reload || :                       
> \
>         fi                                                                  
> \
> fi                                                                          
> \
> %{nil}
> 
> 
> Updated vs uninstall is handled in the script.

Ok, I now faintly remember that there is an argument passed to the %pre/%postun scripts. 

> 
> for reference, %_restart_check_systemctl expands to:
> 
> %_restart_check_systemctl                                                   
> \
>         test -f /etc/sysconfig/services && . /etc/sysconfig/services        
> \
>         test "$YAST_IS_RUNNING" = instsys       && exit 0                   
> \
>         test "$DISABLE_RESTART_ON_UPDATE" = yes && exit 0                   
> \
>         /usr/bin/systemctl try-restart %{nil}
> 
> And here lies the problem - export DUSABLE_RESTART_ON_UPDATE=yes (as set by
> xdm.spec) is overwritten by sourcing /etc/sysconfig/services and the system
> default being taken into account - resulting in the issue we now discuss /
> hopefully try to fix.

Which I mentioned already more than once. So agreed.

(In reply to Michael Schröder from comment #27)
> I'm also in favor of checking another environment variable, e.g.
> DISABLE_RESTART_ON_UPDATE_OVERRIDE

Do we really need this? IHM we just need to 'or' the user and spec file choice.
We don't want a restart if the spec file or the user doesn't want it.

Thus:
%_restart_check_systemctl  \
        test "$DISABLE_RESTART_ON_UPDATE" = yes && exit 0 \
        test "$YAST_IS_RUNNING" = instsys       && exit 0 \
        test -f /etc/sysconfig/services && . /etc/sysconfig/services && \
        test "$DISABLE_RESTART_ON_UPDATE" = yes && exit 0 \
        /usr/bin/systemctl try-restart %{nil}

and:
%_stop_check_systemctl \
        test "$DISABLE_STOP_ON_REMOVAL" = yes && exit 0 \
        test "$YAST_IS_RUNNING" = instsys     && exit 0 \
        test -f /etc/sysconfig/services && . /etc/sysconfig/services  && \
        test "$DISABLE_STOP_ON_REMOVAL" = yes && exit 0 \
        /usr/bin/systemctl stop %{nil}

Michael, can we get rid of the version of %service_del_postun and %service_del_preun in /usr/lib/rpm/macros, should we rather move these macros there or should they be renamed?

Created attachment 667084 [details]
Fix.

I've tested this fix in OBS in the project home:eeich:systemd-rpm-marcos-test.

As tested with the package 'xdm' the variable 'DISABLE_RESTART_ON_UPDATE' set in the spec file before calling the macro in the %postun rule is now honoured.

The same should be true for 'DISABLE_STOP_ON_REMOVAL'.

NOTE: Having %service_del_postun and %service_del_preun defined in two packages (rpm and systemd-rpm-macros) is really confusing - espeicially, since 
/var/adm/fillup-templates/sysconfig.services needs to part of 'rpm' - therefore I'd suggest to remove the definition of %service_del_preun and %service_del_postun from macrosin.diff and replace it with note.
Likewise, %restart_on_update and %stop_on_removal can be reomved from rpm-suse_macros. 
Check home:eeich:systemd-rpm-marcos-test on OBS.

IMHO a list of services which should not be restarted may added to /etc/sysconfig/services then this list can be used to be skip exactly this services from getting restarted.  Currently there are xdm and other display managers ... maybe sshd, and some others .. xen?

(In reply to Michael Schröder from comment #29)

Even this can be avoided with some scripting ;)

(In reply to Dominique Leuenberger from comment #23)

Sorry but I prefere to get SLES and openSUSE in sync at this point.  Even if this requires a veto from my side.

We could also discuss about the defaults in /etc/sysconfig/services ... nevertheless for a security fix for e.g. postfix or any other network based daemon you really like to have a restart in case of an update.  And this was the reason for this feature. And this is alos useful even for openSUSE based systems out there.

(In reply to Dr. Werner Fink from comment #34)
> IMHO a list of services which should not be restarted may added to
> /etc/sysconfig/services then this list can be used to be skip exactly this
> services from getting restarted.  Currently there are xdm and other display
> managers ... maybe sshd, and some others .. xen?

This would probably the most flexible solution:
- For services that don't allow a restart, this could be added on install,
  and removed on deinstall. 
- How do we handle package updates? 
  * If we add it, we may override the users choice.
  * If we don't, we will never be able to 'fix' the setting for an installed
    package.
- Do we have a fillup scriptlet which is able to safely add and remove entries 
  to/from a list in the sysconfig files?

This concept bring up a number of interesting questions which need to be addressed before we can implement this.

> (In reply to Michael Schröder from comment #29)
> 
> Even this can be avoided with some scripting ;)
> 
> (In reply to Dominique Leuenberger from comment #23)
> 
> Sorry but I prefere to get SLES and openSUSE in sync at this point.  Even if
> this requires a veto from my side.
> 
> We could also discuss about the defaults in /etc/sysconfig/services ...
> nevertheless for a security fix for e.g. postfix or any other network based
> daemon you really like to have a restart in case of an update.  And this was
> the reason for this feature. And this is alos useful even for openSUSE based
> systems out there.

Sure, sshd and the DMs need to be restarted, but this needs to be scheduled by the user. We already have a mechanism to tell the user to log out from his desktop session or reboot.

(In reply to Dr. Werner Fink from comment #34)
> IMHO a list of services which should not be restarted may added to
> /etc/sysconfig/services then this list can be used to be skip exactly this
> services from getting restarted.  Currently there are xdm and other display
> managers ... maybe sshd, and some others .. xen?

You favor a static list in a 'random' package over a package having the chance to specify by itself that it knows its service must not be restarted (the solution we had in place before with the exported variable).

> (In reply to Dominique Leuenberger from comment #23)
> 
> Sorry but I prefere to get SLES and openSUSE in sync at this point.  Even if
> this requires a veto from my side.

ath 'this' point or 'one' point again once we actually have a working solution? I agree of course that syncing it up is certainly the right thing to do. But so far we don't even have a clear answer what we want to achieve exactly.

From what I understood:

a) Per default, we want services to be restarted on updated / stopped on uninstall
b) A system admin might want to change one or both behaviors (changing the values in /etc/sysconfig/services)
c) Some packages KNOW that restarting their service has very severe side effects (xen tears down the VMs, apparmor leaves all running apps unprotected, PackageKit loses the running update task, xdm loses the running Desktop sessions). This list of packages is non-exhaustive and can/will change over time.

*a* was never disputed and the existing macros clearly pointed into this direction.

*b* did not work prior to the systemd-rpm-macros change of 'end of 2015' (the one referenced in comment 9)

*c* did work until the same change, some argue by accident, some argue it was intentional (as some packages make use of it, one could also elevate the accident to a welcome behavior)

The goal, imho, is to satisfy all three usecases.

(In reply to Dominique Leuenberger from comment #36)
> (In reply to Dr. Werner Fink from comment #34)
> > IMHO a list of services which should not be restarted may added to
> > /etc/sysconfig/services then this list can be used to be skip exactly this
> > services from getting restarted.  Currently there are xdm and other display
> > managers ... maybe sshd, and some others .. xen?
> 
> You favor a static list in a 'random' package over a package having the
> chance to specify by itself that it knows its service must not be restarted
> (the solution we had in place before with the exported variable).
> 

I don't think this is the idea, at least I hope it isn't.
Each service package should be able to add itself to this list.
To me the main concern here would be how to handle user overrides.

> > (In reply to Dominique Leuenberger from comment #23)
> > 
> > Sorry but I prefere to get SLES and openSUSE in sync at this point.  Even if
> > this requires a veto from my side.
> 
> ath 'this' point or 'one' point again once we actually have a working
> solution? I agree of course that syncing it up is certainly the right thing
> to do. But so far we don't even have a clear answer what we want to achieve
> exactly.
> 
> From what I understood:
> 
> a) Per default, we want services to be restarted on updated / stopped on
> uninstall
> b) A system admin might want to change one or both behaviors (changing the
> values in /etc/sysconfig/services)
> c) Some packages KNOW that restarting their service has very severe side
> effects (xen tears down the VMs, apparmor leaves all running apps
> unprotected, PackageKit loses the running update task, xdm loses the running
> Desktop sessions). This list of packages is non-exhaustive and can/will
> change over time.
> 
> *a* was never disputed and the existing macros clearly pointed into this
> direction.
> 
> *b* did not work prior to the systemd-rpm-macros change of 'end of 2015'
> (the one referenced in comment 9)

This change was introduced to fix a SLES issue where a partner had real issues.
The partner (an ISV) tried to fix it in the rpm spec file using DISABLE_RESTART_ON_UPDATE=1 (probably misguided due to faulty documentation) which of course did not work.
The change introduced broke the existing behaviour. Luckily, it has not made its way into SLE, yet.

> *c* did work until the same change, some argue by accident, some argue it
> was intentional (as some packages make use of it, one could also elevate the
> accident to a welcome behavior)
> 
> The goal, imho, is to satisfy all three usecases.

Werner's solution would do this. It introduces other questions, though.

guys, you're making this way too complicated. Introducing a new list is way too confusing. For our use cases it's much better to have the package know what to do.

Please keep it simple.

As for comment#32: yes, we do need this. Hijacking a already existing variable is a bad idea. We can also add a switch to the macros if you prefer this.

(In reply to Michael Schröder from comment #38)
> guys, you're making this way too complicated. Introducing a new list is way
> too confusing. For our use cases it's much better to have the package know
> what to do.
> 
> Please keep it simple.
> 
> As for comment#32: yes, we do need this. Hijacking a already existing
> variable is a bad idea. We can also add a switch to the macros if you prefer
> this.

What would an additional variable give you? 

The proposal from comment #33 only allows to override the default *do* update to *do not* update.
The spec file could override a users global choice to *do not* update to *do* update the respective service.

Do we see a need for this?

The additional variable gives me the good feeling that it's not a bad hack. You can't override all the other sysconfig vars with the environment, so why should it be different in this case?

(But a macro option would be even more clean, I admit.)

Sure, accepted - with noting the downside that every consumer of this needs to modify the respective package for any update that is done after this update has been released. 
I do assume that our build system picks up the latest updates of any package as soon as released.

(In reply to Egbert Eich from comment #35)
> - Do we have a fillup scriptlet which is able to safely add and remove
>   entries to/from a list in the sysconfig files?

You can use   sysconf_addword   (not saying that I like the idea of having a 
/etc/sysconfig/do-not-restart list)


(In reply to Dominique Leuenberger from comment #36)
> c) Some packages KNOW that restarting their service has very severe side
> effects (xen tears down the VMs, apparmor leaves all running apps
> unprotected, 

Just for the records - the AppArmor packages use "reload" instead of the default macros to avoid this. However, users can still accidently use restart.

> *c* did work until the same change, some argue by accident, some argue it
> was intentional (as some packages make use of it, one could also elevate the
> accident to a welcome behavior)

At least for AppArmor, I have to blame systemd ;-) - see bug 853019

Comment#42 and #43: Your macro change will also only take effect when the corresponding packages are released again, as the macros are already expanded in the binary packages. So I don't see any difference.

(In reply to Michael Schröder from comment #45)

This is always true!

Created attachment 667148 [details]
Small chnage which adds a rpm macro option as well as makes

the DISABLE_RESTART_ON_UPDATE=1 or DISABLE_STOP_ON_REMOVAL=1 work as well

(In reply to Dr. Werner Fink from comment #47)

Maybe a notifier should be added as well as now the (super) user has to restart or stop the service!

(In reply to Dr. Werner Fink from comment #47)
> Created attachment 667148 [details]
> Small chnage which adds a rpm macro option as well as makes
> 
> the DISABLE_RESTART_ON_UPDATE=1 or DISABLE_STOP_ON_REMOVAL=1 work as well

I like the solution! - Let's make sure that we get this fully documented on what works (seems exporting the variable OR passing -d is fine) and which solution we prefer (presumably -d, but this will only work on openSUSE Tumbleweed and SLE12SP2)

 %_restart_check_systemctl								\
-	test "$DISABLE_RESTART_ON_UPDATE" = yes && exit 0				\
+	test "$DISABLE_RESTART_ON_UPDATE" != no && exit 0				\

But as /etc/sysconfig/service is not yet sourced at that moment, ANY package not specifying it =no will NOT restart now (their value of DISABLE+RESTART_ON_UPDATE is "", not 'no'

(In reply to Michael Schröder from comment #45)
> Comment#42 and #43: Your macro change will also only take effect when the
> corresponding packages are released again, as the macros are already
> expanded in the binary packages. So I don't see any difference.

Of course. For any package released after the broken macros was released it is too late.
The difference is: when you change the semantics, you will have to notify each maintainer of a consumer of the macro of the change and make sure any possible update is properly serialized with the update to systemd-rpm-macros.

Of course it would be possible to have both the new (macro option driven) and old deprecated env variable driven semantics around in parallel for a while.

So DISABLE_RESTART_ON_UPDATE=0 will also disable the restart? Doesn't look like a good idea.

(And I don't like that it looks at the environment, this means that the env of the person running rpm is used to check this as well. That's why I preferred the option...)

(In reply to Dominique Leuenberger from comment #50)
>  %_restart_check_systemctl								\
> -	test "$DISABLE_RESTART_ON_UPDATE" = yes && exit 0				\
> +	test "$DISABLE_RESTART_ON_UPDATE" != no && exit 0				\
> 
> But as /etc/sysconfig/service is not yet sourced at that moment, ANY package
> not specifying it =no will NOT restart now (their value of
> DISABLE+RESTART_ON_UPDATE is "", not 'no'

Exactly. I did propose a patch in comment #33 which sources /etc/sysconfig/service but so far this has been ignored completely.

(In reply to Dominique Leuenberger from comment #50)

In /etc/sysconfig/services there are clear declaration *what+ values are used:

 ## Path:        System/Services
 ## Type:        yesno
 ## Default:     no

... sorry but if a sytem admin edits this without reading I can not help. And it should be clear that this works only for the case

  DISABLE_RESTART_ON_UPDATE=1 rpm -Uhv ...

or

  DISABLE_RESTART_ON_UPDATE=1 zypper up ...

(In reply to Egbert Eich from comment #53)

> > But as /etc/sysconfig/service is not yet sourced at that moment, ANY package
> > not specifying it =no will NOT restart now (their value of
> > DISABLE+RESTART_ON_UPDATE is "", not 'no'
> 
> Exactly. I did propose a patch in comment #33 which sources
> /etc/sysconfig/service but so far this has been ignored completely.

Maybe I should have given more context... the sourcing of /etc/sysconfig/services would happen right after.. so the approach is equal to what you proposed in comment 33... first check the 'env', then source and recheck the env variable (now coming from the file).

The difference being:
comment ee checked if DISABLE_RESTART_ON_UPDATE=yes && exit 0
and Werner's new solution doing DISABLE_RESTART_ON_UPDATE != no, which sounds very appealing at first, but fails, as outlined above, as unless specified by the package/user/anything else, the variable is not set at all.

(In reply to Michael Schröder from comment #52)

I also prefere options ... nevertheless the environment variable is already described even if with value "1" instead of "yes".

Beside this and with my change the option does override the environment variable.  If the environment variable should win that more work has to done.

(In reply to Dr. Werner Fink from comment #54)
> (In reply to Dominique Leuenberger from comment #50)
> 
> In /etc/sysconfig/services there are clear declaration *what+ values are
> used:
> 
>  ## Path:        System/Services
>  ## Type:        yesno
>  ## Default:     no
> 
> ... sorry but if a sytem admin edits this without reading I can not help.
> And it should be clear that this works only for the case
> 
>   DISABLE_RESTART_ON_UPDATE=1 rpm -Uhv ...
> 
> or
> 
>   DISABLE_RESTART_ON_UPDATE=1 zypper up ...

and what in case of simply:

rpm -Uhv <package>?

Sorry Werner: your script would not execute a restart of the service in this case, as UPDATE_ON_RESTART!=no is satisfied and the script exits.

(In reply to Dominique Leuenberger from comment #55)

OK ... then something like

  test -n "$DISABLE_RESTART_ON_UPDATE" -a "$DISABLE_RESTART_ON_UPDATE" != no && exit 0

Created attachment 667154 [details]
Fix using a macro argument while retaining the old semantics.

How about this? It retains the old (deprecated) semantics but adds a command line argument. We could make the command line argument work both ways ie. override the user provided default setting altogether as well.

Created attachment 667263 [details]
Patch for %_restart_check_systemctl using macro argument

Proposal using a macro argument. %_stop_check_systemctl can be handled likewise.
It's ugly but generates only the script code we need.

Created attachment 667293 [details]
Full patch for macros.systemd

Created attachment 667365 [details]
Yet an other change

(In reply to Egbert Eich from comment #61)

Hmmm ... what do you think about this?

(In reply to Dr. Werner Fink from comment #62)

Now there are two options for the servcie macros ... -f for force and -n for never ... beside the default.

The code:

%if "%{stop_on_remove}"=="default" \
	test "$DISABLE_STOP_ON_REMOVAL" = yes -o     \
	     "$DISABLE_STOP_ON_REMOVAL" = 1 && exit 0	  \
 	test -f /etc/sysconfig/services && . /etc/sysconfig/services	\
	test "$DISABLE_STOP_ON_REMOVAL" = yes -o	\
	     "$DISABLE_STOP_ON_REMOVAL" = 1 && exit 0	\
%endif	

implies, that in 'default' mode, we want to honour $DISABLE_STOP_ON_REMOVAL not only set in /etc/sysconfig/services but coming from other sources (spec-file, global environment) as well. 
Do we want this - or do we want two separate modes ('legacy' and 'default')? 
Ie: 
'legacy' - (no option flag) with the described behaviour and 
'default' - (option flag -d) where we ignore $DISABLE_STOP_ON_REMOVAL from 
            any other sources than /etc/sysconfig/services

In the light of comments #38, #40 and #41 I'd like to get Michael's opinion.

I think having legacy and default makes things too complicated for the packagers. I'm ok with Werner's proposal.

Two nitpicks:
1) I guess it should be %stop_on_removal for consistency
2) Please use %define instead of %global so that multiple subpacks can have different settings (i.e. the macro reverts to the old value after expansion).

Cheers,
  Michael.

Created attachment 667474 [details]
Using lua interpreter to avoid define or global command

This one does produce bash code for the default (no option):

        /usr/bin/systemctl --no-reload disable display-manager.service || :                             
        (
                test "$YAST_IS_RUNNING" = instsys  && exit 0
                test -f /etc/sysconfig/services -a -z "$DISABLE_STOP_ON_REMOVAL" && . /etc/sysconfig/services
                test "$DISABLE_STOP_ON_REMOVAL" = yes -o "$DISABLE_STOP_ON_REMOVAL" = 1 && exit 0
                /usr/bin/systemctl try-restart display-manager.service || :
        )

for the force option -f:


                (
                        test "$YAST_IS_RUNNING" = instsys  && exit 0
                        /usr/bin/systemctl try-restart display-manager.service || :
                )

and for the never option -n simply nothing.

Note that I've used a check if the variable DISABLE_STOP_ON_REMOVAL (or DISABLE_RESTART_ON_UPDATE) is already set that is that the file /etc/sysconfig/services is only sourced if the variable is not set.

Created attachment 667586 [details]
Using options only, no lua

(In reply to Egbert Eich)

OK, OK ...

it seems that within %{ ... } the line ending has not to be escaped.

(In reply to Dr. Werner Fink from comment #71)

The only question I've: does rpm on Leap42.1 and SLES12 SP0, SP1, and also SP2 do this in the same way that is no escaped newline is required witin %{ ... }?

Only problem I see is that rpm includes the options into the %{*} of the expanded macro

        (
                test "$YAST_IS_RUNNING" = instsys && exit 0
                /usr/bin/systemctl try-restart -f  display-manager.service
        )

... how can I avoid this?

(In reply to Dr. Werner Fink from comment #72)

... no expansion of a macro with options but macro expansion with options works.

SR#365189

This is an autogenerated message for OBS integration:
This bug (968405) was mentioned in
https://build.opensuse.org/request/show/365189 Factory / systemd-rpm-macros

#72: processed options are not included in %{*}

I would simply do it like this (untested):

%_stop_check_systemctl(fn) \
%{-n:exit 0} \
%{!-n:\
test "$YAST_IS_RUNNING" = instsys   && exit 0 \
%{!-f:\
test -f /etc/sysconfig/services && . /etc/sysconfig/services \
test "$DISABLE_STOP_ON_REMOVAL" = yes -o \
     "$DISABLE_STOP_ON_REMOVAL" = 1 && exit \
} \
} \
/usr/bin/systemctl stop %nil



No extra macros, no %if statement, just option processing.

This is an autogenerated message for OBS integration:
This bug (968405) was mentioned in
https://build.opensuse.org/request/show/365517 Factory / xdm

This is an autogenerated message for OBS integration:
This bug (968405) was mentioned in
https://build.opensuse.org/request/show/365553 Factory / xdm

I updated TW to current snapshot and was kicked out of GUI. I'm using XFCE installation pattern with lightdm; I do not see anything related to service restart in lightdm, but I see that xdm was updated (1.1.11-17.1 -> 1.1.11-18.1). Is it (loss of GUI session) expected in this case?

Even if this is fallout of %postuninstall of previous version - I'm using lightdm as display manager, why updating of *xdm* restarts it?

(In reply to Andrei Borzenkov from comment #78)
> I updated TW to current snapshot and was kicked out of GUI. I'm using XFCE
> installation pattern with lightdm; I do not see anything related to service
> restart in lightdm, but I see that xdm was updated (1.1.11-17.1 ->
> 1.1.11-18.1). Is it (loss of GUI session) expected in this case?
> 
> Even if this is fallout of %postuninstall of previous version - I'm using
> lightdm as display manager, why updating of *xdm* restarts it?

Andrei, Since the faulty code is in the uninstall scriptlet of the *old* package this will happen until a *fixed* package is updated ie gets uninstalled. The uninstall scriptlets of a package(-version) are stored in the rpm database until this package(-version) is removed completely.
So even though the issue is fixed, you will suffer from it once more. Only the next time around, when this package is updated *again* you should be fine.

This is an autogenerated message for OBS integration:
This bug (968405) was mentioned in
https://build.opensuse.org/request/show/369630 42.1 / systemd-rpm-macros

openSUSE-RU-2016:0864-1: An update that has three recommended fixes can now be installed.

Category: recommended (moderate)
Bug References: 955996,956849,968405
CVE References: 
Sources used:
openSUSE Leap 42.1 (src):    systemd-rpm-macros-2.1-3.1

SUSE-RU-2016:0974-1: An update that has three recommended fixes can now be installed.

Category: recommended (low)
Bug References: 955996,956849,968405
CVE References: 
Sources used:
SUSE Linux Enterprise Server 12-SP1 (src):    systemd-rpm-macros-2.1-4.1
SUSE Linux Enterprise Desktop 12-SP1 (src):    systemd-rpm-macros-2.1-4.1

SUSE-RU-2016:0975-1: An update that has 6 recommended fixes can now be installed.

Category: recommended (low)
Bug References: 900935,921075,946216,955996,956849,968405
CVE References: 
Sources used:
SUSE Linux Enterprise Server 12 (src):    systemd-rpm-macros-2.1-8.3.1
SUSE Linux Enterprise Desktop 12 (src):    systemd-rpm-macros-2.1-8.3.1

Hey, I don't mean to be a killjoy, and I may have missed something in the rather long discussion of this bug - but with tumbleweed, updating xdm *still* terminates the user's session.

May 02 09:33:35 hermes [RPM][1966]: install xdm-1.1.11-19.2.x86_64: success
May 02 09:33:35 hermes systemd[1]: Reloading.
May 02 09:33:35 hermes systemd[1]: nss-lookup.target: Dependency Before=nss-lookup.target dropped
May 02 09:33:35 hermes systemd[1]: Started CUPS Scheduler.
May 02 09:33:35 hermes [RPM][1966]: erase xdm-1.1.11-19.1.x86_64: success
May 02 09:33:35 hermes systemd[1]: Reloading.
May 02 09:33:35 hermes systemd[1]: nss-lookup.target: Dependency Before=nss-lookup.target dropped
May 02 09:33:35 hermes systemd[1]: Started CUPS Scheduler.
May 02 09:33:35 hermes systemd[1]: Stopping X Display Manager...
May 02 09:33:35 hermes org.gtk.vfs.Daemon[2740]: A connection to the bus can't be made
May 02 09:33:35 hermes su[2902]: pam_unix(su-l:session): session closed for user root
May 02 09:33:35 hermes kdm[2689]: :0[2689]: pam_unix(xdm:session): session closed for user lmb
...
May 02 09:33:37 hermes systemd[1]: Stopped X Display Manager.
May 02 09:33:37 hermes systemd[1]: Starting X Display Manager...


This is rather consistently annoying ;-) Is there any chance to avoid this without modifying /etc/sysconfig/services globally?

Lars, did you see comment #79, i.e. can you do us a favor and re-update the xdm package once more (with --force or --oldpackage or whatever will be required) in a running session in order to verify the issue is fixed? Thanks!

OK, so "zypper in --force xdm" does not reproduce this, but I had already updated several times since the fixes mentioned up to comment 84 were added. So I'd have thought I've already got the package update.

I tried downloading the package with -d and then used rpm -U --force directly. But that doesn't trigger the "erase" transaction either, since the new package version and the installed one are identical.

I'll try installing xdm-1.1.11-19.1 and then re-upgrading to xdm-1.1.11-19.2 again.

OK, so my session is still here, seems to have worked ;-)

Thanks. Sorry for the noise, just somewhat surprised it took so long for the fix to hit tumbleweed. Let's hope for the best.

This is an autogenerated message for OBS integration:
This bug (968405) was mentioned in
https://build.opensuse.org/request/show/544180 Factory / systemd-rpm-macros

This is an autogenerated message for OBS integration:
This bug (968405) was mentioned in
https://build.opensuse.org/request/show/547058 Factory / systemd-rpm-macros

This is an autogenerated message for OBS integration:
This bug (968405) was mentioned in
https://build.opensuse.org/request/show/576778 Factory / systemd-rpm-macros

This is an autogenerated message for OBS integration:
This bug (968405) was mentioned in
https://build.opensuse.org/request/show/611238 Factory / systemd-rpm-macros