Re: [PATCH] Don't execute final_action if failure_action terminates the system

[Kairui Song]

On Tue, Nov 5, 2019 at 8:37 PM d.hatayama(a)fujitsu.com
<d.hatayama(a)fujitsu.com&gt; wrote:
>  > -----Original Message-----
>  > From: Kairui Song [mailto:[e-mail]]
>  > Sent: Wednesday, October 30, 2019 6:35 PM
>  > To: kexec(a)lists.fedoraproject.org
>  > Cc: Pingfan Liu <piliu(a)redhat.com>; Dave Young <dyoung(a)redhat.com>;
> Hatayama,
>  > Daisuke/畑山 大輔 <d.hatayama(a)fujitsu.com>; Kairui Song <kasong(a)redhat.com&gt;
>  > Subject: [PATCH] Don't execute final_action if failure_action terminates the
>  > system
>  >
>  > If failure_action is shutdown/reboot/halt, final_action is pointless as
>  > the system will be already stopping. And if final_action is different
>  > from failure_action, it will trigger a systemd race problem and cause
>  > unexpected behavior to occur.
>  >
>  > So let the error handler stop and exit after performing failure_action
>  > successfully if failure_action is one of shutdown/reboot/halt.
>  > This way, final_action will not be executed.
>  >
>  > Signed-off-by: Kairui Song <kasong(a)redhat.com&gt;
>  > ---
>  >  kdump-lib-initramfs.sh | 6 +++---
>  >  1 file changed, 3 insertions(+), 3 deletions(-)
>  >
>  > diff --git a/kdump-lib-initramfs.sh b/kdump-lib-initramfs.sh
>  > index c409dce..8530556 100755
>  > --- a/kdump-lib-initramfs.sh
>  > +++ b/kdump-lib-initramfs.sh
>  > @@ -56,13 +56,13 @@ get_kdump_confs()
>  >                          FAILURE_ACTION="kdump_emergency_shell"
>  >                      ;;
>  >                      reboot)
>  > -                        FAILURE_ACTION="systemctl reboot -f"
>  > +                        FAILURE_ACTION="systemctl reboot -f &&
> exit"
>
>  Can -f option here also be removed?
>
>  >                      ;;
>  >                      halt)
>  > -                        FAILURE_ACTION="halt"
>  > +                        FAILURE_ACTION="halt && exit"
>
>  It's better to replace halt with systemctl halt as suggested by Dave.
>
>  >                      ;;
>  >                      poweroff)
>  > -                        FAILURE_ACTION="systemctl poweroff -f"
>  > +                        FAILURE_ACTION="systemctl poweroff -f &&
> exit"
>
>  Ditto.
>
>  I also checked the behavior when FAILURE_ACTION="exit" for in case, i.e.
>  the situation where shutdown/halt/reboot doesn't work well for some reason
>  even though it terminates with successful exit status.
>  The result is infinite loop like:
>
>     kdump-capture.serviec pulls in emergency.target on failrue
>     => emergency.target pulls in emergency.service
>     => emergency.service isolates kdump-error-handler.service
>     => kdump-error-handler.service isolates default.target (= initrd.target) on
> ExecStopPost=
>     => initrd.target pulls in kdump-capture.service
>
>  I think there is a room to improve this design in another work.
>  
Good suggestions.

Maye it's a good idea to let ExecStopPost= repeat the final_action again?

-- 
Best Regards,
Kairui Song

[PATCH] Don't execute final_action if failure_action terminates the system

>  -----Original Message-----
>  From: Kairui Song [mailto:[e-mail]]
>  Sent: Thursday, November 7, 2019 6:20 PM
>  To: Hatayama, Daisuke/畑山 大輔 <d.hatayama(a)fujitsu.com&gt;
>  Cc: kexec(a)lists.fedoraproject.org; Pingfan Liu <piliu(a)redhat.com>; Dave Young
>  <dyoung(a)redhat.com&gt;
>  Subject: Re: [PATCH] Don't execute final_action if failure_action terminates
>  the system
>  
>  On Tue, Nov 5, 2019 at 8:37 PM d.hatayama(a)fujitsu.com
>  <d.hatayama(a)fujitsu.com&gt; wrote:
>  > > -----Original Message-----
>  > > From: Kairui Song [mailto:[e-mail]]
>  > > Sent: Wednesday, October 30, 2019 6:35 PM
>  > > To: kexec(a)lists.fedoraproject.org
>  > > Cc: Pingfan Liu <piliu(a)redhat.com>; Dave Young
> <dyoung(a)redhat.com>;
>  Hatayama,
>  > > Daisuke/畑山 大輔 <d.hatayama(a)fujitsu.com>; Kairui Song
>  <kasong(a)redhat.com&gt;
>  > > Subject: [PATCH] Don't execute final_action if failure_action terminates
>  the
>  > > system
>  > >
>  > > If failure_action is shutdown/reboot/halt, final_action is pointless as
>  > > the system will be already stopping. And if final_action is different
>  > > from failure_action, it will trigger a systemd race problem and cause
>  > > unexpected behavior to occur.
>  > >
>  > > So let the error handler stop and exit after performing failure_action
>  > > successfully if failure_action is one of shutdown/reboot/halt.
>  > > This way, final_action will not be executed.
>  > >
>  > > Signed-off-by: Kairui Song <kasong(a)redhat.com&gt;
>  > > ---
>  > >  kdump-lib-initramfs.sh | 6 +++---
>  > >  1 file changed, 3 insertions(+), 3 deletions(-)
>  > >
>  > > diff --git a/kdump-lib-initramfs.sh b/kdump-lib-initramfs.sh
>  > > index c409dce..8530556 100755
>  > > --- a/kdump-lib-initramfs.sh
>  > > +++ b/kdump-lib-initramfs.sh
>  > > @@ -56,13 +56,13 @@ get_kdump_confs()
>  > >                          FAILURE_ACTION="kdump_emergency_shell"
>  > >                      ;;
>  > >                      reboot)
>  > > -                        FAILURE_ACTION="systemctl reboot -f"
>  > > +                        FAILURE_ACTION="systemctl reboot -f &&
> exit"
>  >
>  > Can -f option here also be removed?
>  >
>  > >                      ;;
>  > >                      halt)
>  > > -                        FAILURE_ACTION="halt"
>  > > +                        FAILURE_ACTION="halt && exit"
>  >
>  > It's better to replace halt with systemctl halt as suggested by Dave.
>  >
>  > >                      ;;
>  > >                      poweroff)
>  > > -                        FAILURE_ACTION="systemctl poweroff -f"
>  > > +                        FAILURE_ACTION="systemctl poweroff -f &&
> exit"
>  >
>  > Ditto.
>  >
>  > I also checked the behavior when FAILURE_ACTION="exit" for in case, i.e.
>  > the situation where shutdown/halt/reboot doesn't work well for some reason
>  > even though it terminates with successful exit status.
>  > The result is infinite loop like:
>  >
>  >    kdump-capture.serviec pulls in emergency.target on failrue
>  >    => emergency.target pulls in emergency.service
>  >    => emergency.service isolates kdump-error-handler.service
>  >    => kdump-error-handler.service isolates default.target (=
>  initrd.target) on ExecStopPost=
>  >    => initrd.target pulls in kdump-capture.service
>  >
>  > I think there is a room to improve this design in another work.
>  >
>  
>  Good suggestions.
>  
>  Maye it's a good idea to let ExecStopPost= repeat the final_action again?
>  
I don't have clear idea now, but it's at least necessary to remove the
circularity
of the shutdown process and maybe let the shutdown process end with forcible
shutdown as is done in poweroff.target/reboot.target
with JobTimeoutAction= directive?

    # grep -vE "(^#|^$)" /usr/lib/systemd/system/poweroff.target
/usr/lib/systemd/system/reboot.target
    /usr/lib/systemd/system/poweroff.target:[Unit]
    /usr/lib/systemd/system/poweroff.target:Description=Power-Off
    /usr/lib/systemd/system/poweroff.target:Documentation=man:systemd.special(7)
    /usr/lib/systemd/system/poweroff.target:DefaultDependencies=no
    /usr/lib/systemd/system/poweroff.target:Requires=systemd-poweroff.service
    /usr/lib/systemd/system/poweroff.target:After=systemd-poweroff.service
    /usr/lib/systemd/system/poweroff.target:AllowIsolate=yes
    /usr/lib/systemd/system/poweroff.target:JobTimeoutSec=30min
    /usr/lib/systemd/system/poweroff.target:JobTimeoutAction=poweroff-force
    /usr/lib/systemd/system/poweroff.target:[Install]
    /usr/lib/systemd/system/poweroff.target:Alias=ctrl-alt-del.target
    /usr/lib/systemd/system/reboot.target:[Unit]
    /usr/lib/systemd/system/reboot.target:Description=Reboot
    /usr/lib/systemd/system/reboot.target:Documentation=man:systemd.special(7)
    /usr/lib/systemd/system/reboot.target:DefaultDependencies=no
    /usr/lib/systemd/system/reboot.target:Requires=systemd-reboot.service
    /usr/lib/systemd/system/reboot.target:After=systemd-reboot.service
    /usr/lib/systemd/system/reboot.target:AllowIsolate=yes
    /usr/lib/systemd/system/reboot.target:JobTimeoutSec=30min
    /usr/lib/systemd/system/reboot.target:JobTimeoutAction=reboot-force
    /usr/lib/systemd/system/reboot.target:[Install]
    /usr/lib/systemd/system/reboot.target:Alias=ctrl-alt-del.target

Also, I think, for the issue of failure_action halt, your patch is good.
I'd like this improvement to be made in another work.
The infinite loop occurs when systemctl halt doesn't work but
normally, it doesn't occur.

>  
>  --
>  Best Regards,
>  Kairui Song
>