[PATCH] mdadm/systemd: change KillMode from none to mixed in service files

[PATCH] mdadm/systemd: change KillMode from none to mixed in service files

[Coly Li]

For mdadm's systemd configuration, current systemd KillMode is "none" in
following service files,
- mdadm-grow-continue@.service
- mdmon@.service

This "none" mode is strongly recommended againsted by systemd developers
(see man 5 system.kill for "KillMode=" section), and is considering to
remove in future systemd version.

A proper KillMode is "mixed", which is explained in the system.kill man
page as,
"If set to mixed, the SIGTERM signal (see below) is sent to the main
process while the subsequent SIGKILL signal (see below) is sent to all
remaining processes of the unit's control group."

This patch changes KillMode in above listed service files from "none"
to "mixed", to follow systemd recommendation and avoid potential
unnecessary issue.

Suggested-by: Benjamin Brunner <bbrunner@suse.com>
Cc: Neil Brown <neilb@suse.de>
Cc: Jes Sorensen <jsorensen@fb.com>
---
 systemd/mdadm-grow-continue@.service | 2 +-
 systemd/mdmon@.service               | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/systemd/mdadm-grow-continue@.service b/systemd/mdadm-grow-continue@.service
index 5c667d2..5fe9d99 100644
--- a/systemd/mdadm-grow-continue@.service
+++ b/systemd/mdadm-grow-continue@.service
@@ -14,4 +14,4 @@ ExecStart=BINDIR/mdadm --grow --continue /dev/%I
 StandardInput=null
 StandardOutput=null
 StandardError=null
-KillMode=none
+KillMode=mixed
diff --git a/systemd/mdmon@.service b/systemd/mdmon@.service
index 85a3a7c..5ef1bf5 100644
--- a/systemd/mdmon@.service
+++ b/systemd/mdmon@.service
@@ -25,4 +25,4 @@ Type=forking
 # it out) and systemd will remove it when transitioning from
 # initramfs to rootfs.
 #PIDFile=/run/mdadm/%I.pid
-KillMode=none
+KillMode=mixed
-- 
2.31.1

Re: [PATCH] mdadm/systemd: change KillMode from none to mixed in service files

[mtkaczyk]

Hi Coly,

> This patch changes KillMode in above listed service files from "none"
> to "mixed", to follow systemd recommendation and avoid potential
> unnecessary issue.
What about mdmonitor.service? Should we add it there too? Now it is not
defined.

> Cc: Jes Sorensen <jsorensen@fb.com>
Not sure if it a problem but Jes uses: jes@trained-monkey.org for
communication with linux-raid.

Thanks,
Mariusz

Re: [PATCH] mdadm/systemd: change KillMode from none to mixed in service files

[Coly Li]

On 12/2/21 12:08 AM, mtkaczyk wrote:
> Hi Coly,
>
>> This patch changes KillMode in above listed service files from "none"
>> to "mixed", to follow systemd recommendation and avoid potential
>> unnecessary issue.
> What about mdmonitor.service? Should we add it there too? Now it is not
> defined.

It was overlooked when I did grep KillMode. Yes, I agree 
mdmonitor.service should have a KillMode key word as well.

Let me post a v2 version.

>> Cc: Jes Sorensen <jsorensen@fb.com>
> Not sure if it a problem but Jes uses: jes@trained-monkey.org for
> communication with linux-raid.

Copied. Thanks for the hint.

Coly Li

Re: [PATCH] mdadm/systemd: change KillMode from none to mixed in service files

[Benjamin Brunner]

Hi all,

On 01.12.21 17:23, Coly Li wrote:
> On 12/2/21 12:08 AM, mtkaczyk wrote:
>> Hi Coly,
>>
>>> This patch changes KillMode in above listed service files from "none"
>>> to "mixed", to follow systemd recommendation and avoid potential
>>> unnecessary issue.
>> What about mdmonitor.service? Should we add it there too? Now it is not
>> defined.
> 
> It was overlooked when I did grep KillMode. Yes, I agree 
> mdmonitor.service should have a KillMode key word as well.
> 
> Let me post a v2 version.
> 
JFYI, when KillMode is not set, it defaults to KillMode=control-group, 
see https://www.freedesktop.org/software/systemd/man/systemd.kill.html.

Therefore, it shouldn't be necessary to explicitly add it (as long as 
control-group is working in case of mdmonitor.service, of course).

Best
Benjamin

>>> Cc: Jes Sorensen <jsorensen@fb.com>
>> Not sure if it a problem but Jes uses: jes@trained-monkey.org for
>> communication with linux-raid.
> 
> Copied. Thanks for the hint.
> 
> Coly Li
> 

-- 
Benjamin Brunner
Engineering Manager System Boot and Init
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5
90409 Nürnberg
Germany

HRB 36809, AG Nürnberg
Geschäftsführer: Ivo Totev

Re: [PATCH] mdadm/systemd: change KillMode from none to mixed in service files

[Coly Li]

On 12/2/21 12:28 AM, Benjamin Brunner wrote:
> Hi all,
>
> On 01.12.21 17:23, Coly Li wrote:
>> On 12/2/21 12:08 AM, mtkaczyk wrote:
>>> Hi Coly,
>>>
>>>> This patch changes KillMode in above listed service files from "none"
>>>> to "mixed", to follow systemd recommendation and avoid potential
>>>> unnecessary issue.
>>> What about mdmonitor.service? Should we add it there too? Now it is not
>>> defined.
>>
>> It was overlooked when I did grep KillMode. Yes, I agree 
>> mdmonitor.service should have a KillMode key word as well.
>>
>> Let me post a v2 version.
>>
> JFYI, when KillMode is not set, it defaults to KillMode=control-group, 
> see https://www.freedesktop.org/software/systemd/man/systemd.kill.html.
>
> Therefore, it shouldn't be necessary to explicitly add it (as long as 
> control-group is working in case of mdmonitor.service, of course).

Hi Benjamin,

Please correct me if I am wrong, I see the difference of the KillMode is,
-- KillMode=mixed stops the processes more gentally, it kill the main 
process with SIGTERM and the remaining processes with SIGKILL.
-- KillMode=control-group kills all in-cgroup processes with SIGKILL, 
which I feel a bit cruel for the main process.

IMHO both method (explicit mixed mode or implicit control-group mode) 
should works for the fixing issue, but I feel removing the KillMode 
lines might be better as you indicated since the files can be a bit 
simpler. Do I understand you correct ?

Thanks.

Coly Li

Re: [PATCH] mdadm/systemd: change KillMode from none to mixed in service files

[NeilBrown]

On Thu, 02 Dec 2021, Coly Li wrote:
> On 12/2/21 12:28 AM, Benjamin Brunner wrote:
> > Hi all,
> >
> > On 01.12.21 17:23, Coly Li wrote:
> >> On 12/2/21 12:08 AM, mtkaczyk wrote:
> >>> Hi Coly,
> >>>
> >>>> This patch changes KillMode in above listed service files from "none"
> >>>> to "mixed", to follow systemd recommendation and avoid potential
> >>>> unnecessary issue.
> >>> What about mdmonitor.service? Should we add it there too? Now it is not
> >>> defined.
> >>
> >> It was overlooked when I did grep KillMode. Yes, I agree 
> >> mdmonitor.service should have a KillMode key word as well.
> >>
> >> Let me post a v2 version.
> >>
> > JFYI, when KillMode is not set, it defaults to KillMode=control-group, 
> > see https://www.freedesktop.org/software/systemd/man/systemd.kill.html.
> >
> > Therefore, it shouldn't be necessary to explicitly add it (as long as 
> > control-group is working in case of mdmonitor.service, of course).
> 
> Hi Benjamin,
> 
> Please correct me if I am wrong, I see the difference of the KillMode is,
> -- KillMode=mixed stops the processes more gentally, it kill the main 
> process with SIGTERM and the remaining processes with SIGKILL.
> -- KillMode=control-group kills all in-cgroup processes with SIGKILL, 
> which I feel a bit cruel for the main process.

There is no point sending SIGTERM to a process which doesn't respond to
it.  mdmon is the only mdadm service which handles SIGTERM.  So it might
make sense to uise KillMode=mixed for that.
For anything else, SIGKILL via KillMode=control-group is perfectly
acceptable. 

NeilBrown


> 
> IMHO both method (explicit mixed mode or implicit control-group mode) 
> should works for the fixing issue, but I feel removing the KillMode 
> lines might be better as you indicated since the files can be a bit 
> simpler. Do I understand you correct ?
> 
> Thanks.
> 
> Coly Li
> 
> 

Re: [PATCH] mdadm/systemd: change KillMode from none to mixed in service files

[Coly Li]

On 12/2/21 5:51 AM, NeilBrown wrote:
> On Thu, 02 Dec 2021, Coly Li wrote:
>> On 12/2/21 12:28 AM, Benjamin Brunner wrote:
>>> Hi all,
>>>
>>> On 01.12.21 17:23, Coly Li wrote:
>>>> On 12/2/21 12:08 AM, mtkaczyk wrote:
>>>>> Hi Coly,
>>>>>
>>>>>> This patch changes KillMode in above listed service files from "none"
>>>>>> to "mixed", to follow systemd recommendation and avoid potential
>>>>>> unnecessary issue.
>>>>> What about mdmonitor.service? Should we add it there too? Now it is not
>>>>> defined.
>>>> It was overlooked when I did grep KillMode. Yes, I agree
>>>> mdmonitor.service should have a KillMode key word as well.
>>>>
>>>> Let me post a v2 version.
>>>>
>>> JFYI, when KillMode is not set, it defaults to KillMode=control-group,
>>> see https://www.freedesktop.org/software/systemd/man/systemd.kill.html.
>>>
>>> Therefore, it shouldn't be necessary to explicitly add it (as long as
>>> control-group is working in case of mdmonitor.service, of course).
>> Hi Benjamin,
>>
>> Please correct me if I am wrong, I see the difference of the KillMode is,
>> -- KillMode=mixed stops the processes more gentally, it kill the main
>> process with SIGTERM and the remaining processes with SIGKILL.
>> -- KillMode=control-group kills all in-cgroup processes with SIGKILL,
>> which I feel a bit cruel for the main process.
> There is no point sending SIGTERM to a process which doesn't respond to
> it.  mdmon is the only mdadm service which handles SIGTERM.  So it might
> make sense to uise KillMode=mixed for that.
> For anything else, SIGKILL via KillMode=control-group is perfectly
> acceptable.

Hi Neil,

Thanks for the hint. So it seems remove the KillMode= lines from the 
systemd files might be best choice.

Hi Benjamin,

How do you think about removing the KillMode= lines to use SIGKILL as 
default action ?

Thanks.

Coly Li

Re: [PATCH] mdadm/systemd: change KillMode from none to mixed in service files

[Benjamin Brunner]

Hi,

On 05.12.21 14:42, Coly Li wrote:
> On 12/2/21 5:51 AM, NeilBrown wrote:
>> On Thu, 02 Dec 2021, Coly Li wrote:
>>> On 12/2/21 12:28 AM, Benjamin Brunner wrote:
>>>> Hi all,
>>>>
>>>> On 01.12.21 17:23, Coly Li wrote:
>>>>> On 12/2/21 12:08 AM, mtkaczyk wrote:
>>>>>> Hi Coly,
>>>>>>
>>>>>>> This patch changes KillMode in above listed service files from 
>>>>>>> "none"
>>>>>>> to "mixed", to follow systemd recommendation and avoid potential
>>>>>>> unnecessary issue.
>>>>>> What about mdmonitor.service? Should we add it there too? Now it 
>>>>>> is not
>>>>>> defined.
>>>>> It was overlooked when I did grep KillMode. Yes, I agree
>>>>> mdmonitor.service should have a KillMode key word as well.
>>>>>
>>>>> Let me post a v2 version.
>>>>>
>>>> JFYI, when KillMode is not set, it defaults to KillMode=control-group,
>>>> see https://www.freedesktop.org/software/systemd/man/systemd.kill.html.
>>>>
>>>> Therefore, it shouldn't be necessary to explicitly add it (as long as
>>>> control-group is working in case of mdmonitor.service, of course).
>>> Hi Benjamin,
>>>
>>> Please correct me if I am wrong, I see the difference of the KillMode 
>>> is,
>>> -- KillMode=mixed stops the processes more gentally, it kill the main
>>> process with SIGTERM and the remaining processes with SIGKILL.
>>> -- KillMode=control-group kills all in-cgroup processes with SIGKILL,
>>> which I feel a bit cruel for the main process.
>> There is no point sending SIGTERM to a process which doesn't respond to
>> it.  mdmon is the only mdadm service which handles SIGTERM.  So it might
>> make sense to uise KillMode=mixed for that.
>> For anything else, SIGKILL via KillMode=control-group is perfectly
>> acceptable.
> 
> Hi Neil,
> 
> Thanks for the hint. So it seems remove the KillMode= lines from the 
> systemd files might be best choice.
> 
> Hi Benjamin,
> 
> How do you think about removing the KillMode= lines to use SIGKILL as 
> default action ?
> 
 From my PoV, this should be fine. To be on the safe side, I've added 
Franck Bui, our systemd maintainer, to the list of recipients.

Franck, could you have a look as well, please?

Thanks,
Benjamin

> Thanks.
> 
> Coly Li
> 

-- 
Benjamin Brunner
Engineering Manager System Boot and Init
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5
90409 Nürnberg
Germany

HRB 36809, AG Nürnberg
Geschäftsführer: Ivo Totev

Re: [PATCH] mdadm/systemd: change KillMode from none to mixed in service files

[Franck Bui]

Hi,

On 12/7/21 1:34 PM, Benjamin Brunner wrote:
>>>> Please correct me if I am wrong, I see the difference of the KillMode is,
>>>> -- KillMode=mixed stops the processes more gentally, it kill the main
>>>> process with SIGTERM and the remaining processes with SIGKILL.
>>>> -- KillMode=control-group kills all in-cgroup processes with SIGKILL,
>>>> which I feel a bit cruel for the main process.

I think there's a miss-understanding here.

Regardless of whether "mixed" or "control-group" mode (or any other mode
actually) is used, the process used to kill processes part of a service is
always the same, only the list of the killed processes differs.

The process is as follow:

 1. send the signal specified by KillSignal= to the list of processes (if
    any), TERM is the default
 2. wait until either the target of process(es) exit or a timeout expires
 3. if the timeout expires send the signal specified by FinalKillSignal=,
    KILL is the default

For "control-group", all remaining processes will receive the SIGTERM signal (by
default) and if there are still processes after a period f time, they will get
the SIGKILL signal.

For "mixed", only the main process will receive the SIGTERM signal, and if there
are still processes after a period of time, all remaining processes (including
the main one) will receive the SIGKILL signal.

>>> There is no point sending SIGTERM to a process which doesn't respond to
>>> it.  mdmon is the only mdadm service which handles SIGTERM.  So it might
>>> make sense to uise KillMode=mixed for that.
>>> For anything else, SIGKILL via KillMode=control-group is perfectly
>>> acceptable.

I don't know enough mdadm to suggest a mode but maybe the clarification above
will help you figuring this out.

That said it sounds a bit strange that some processes don't respond to SIGTERM.
Is that done because some services need to run lately during the shutdown process ?

Thanks.

Re: [PATCH] mdadm/systemd: change KillMode from none to mixed in service files

[NeilBrown]

On Thu, 09 Dec 2021, Franck Bui wrote:
> Hi,
> 
> On 12/7/21 1:34 PM, Benjamin Brunner wrote:
> >>>> Please correct me if I am wrong, I see the difference of the KillMode is,
> >>>> -- KillMode=mixed stops the processes more gentally, it kill the main
> >>>> process with SIGTERM and the remaining processes with SIGKILL.
> >>>> -- KillMode=control-group kills all in-cgroup processes with SIGKILL,
> >>>> which I feel a bit cruel for the main process.
> 
> I think there's a miss-understanding here.
> 
> Regardless of whether "mixed" or "control-group" mode (or any other mode
> actually) is used, the process used to kill processes part of a service is
> always the same, only the list of the killed processes differs.
> 
> The process is as follow:
> 
>  1. send the signal specified by KillSignal= to the list of processes (if
>     any), TERM is the default
>  2. wait until either the target of process(es) exit or a timeout expires
>  3. if the timeout expires send the signal specified by FinalKillSignal=,
>     KILL is the default
> 
> For "control-group", all remaining processes will receive the SIGTERM signal (by
> default) and if there are still processes after a period f time, they will get
> the SIGKILL signal.
> 
> For "mixed", only the main process will receive the SIGTERM signal, and if there
> are still processes after a period of time, all remaining processes (including
> the main one) will receive the SIGKILL signal.
> 
> >>> There is no point sending SIGTERM to a process which doesn't respond to
> >>> it.  mdmon is the only mdadm service which handles SIGTERM.  So it might
> >>> make sense to uise KillMode=mixed for that.
> >>> For anything else, SIGKILL via KillMode=control-group is perfectly
> >>> acceptable.
> 
> I don't know enough mdadm to suggest a mode but maybe the clarification above
> will help you figuring this out.
> 
> That said it sounds a bit strange that some processes don't respond to SIGTERM.
> Is that done because some services need to run lately during the shutdown process ?

When I wrote that they don't respond to SIGTERM, I meant that they don't
take any special action.  They don't ignore SIGTERM, so they will exit
when they are sent it.  i.e.  it doesn't matter whether they get SIGTERM
or SIGKILL - either way they will exit.

NeilBrown