Shepherd doesn't restart previously running dependent services

Done

Details

4 participants

Carlo Zancanaro
Clément Lassieur
Thompson, David
Ludovic Courtès

Owner: unassigned

Submitted by: Thompson, David

Severity: normal

Thompson, David wrote on 31 Mar 2016 15:23

Recipients:(address . bug-guix@gnu.org)

Message-ID:CAJ=RwfZdqc6iqsRsay95oPZDd-hrqkaERYA5PYiXp_FwOO1-Qg@mail.gmail.com

If service 'foo' is depended on by service 'bar', and both are
running, and one runs 'herd restart foo', both 'foo' and 'bar' are
stopped, but then only 'foo' is restarted.  I think that the dependent
services that were stopped should also be restarted, otherwise the
user has to manually start them back up one at a time.

- Dave

Carlo Zancanaro wrote on 25 Aug 2018 13:33

[PATCH shepherd] Restart dependent services on service restart

Recipients:(address . 23170@debbugs.gnu.org)

Message-ID:874lfi65rv.fsf@zancanaro.id.au

I've written a patch to fix this. It's not super smart, but it

should do the job.

It currently targets the branch after my patch in #32408[1], but

it's technically an independent change.

[1]: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=32408

Attachment: 0001-service-Restart-dependent-services-on-service-restar.patch

Ludovic Courtès wrote on 25 Aug 2018 16:41

Recipients:(name . Carlo Zancanaro)(address . carlo@zancanaro.id.au)(address . 23170@debbugs.gnu.org)

Message-ID:87bm9qbjb4.fsf@gnu.org

Hi again! :-)

Carlo Zancanaro <carlo@zancanaro.id.au> skribis:

Toggle quote (3 lines)

> I've written a patch to fix this. It's not super smart, but it should

> do the job.

I wonder if there are cases where one might want to restart a service

without restarting its dependent services. We can probably ignore it

for now, but perhaps we’ll need to add a flag or a separate action later.

Thoughts?

Toggle quote (13 lines)> From 50dd3ef4888b04ea3b869da893b23ad69fad8971 Mon Sep 17 00:00:00 2001
> From: Carlo Zancanaro <carlo@zancanaro.id.au>
> Date: Sat, 25 Aug 2018 20:32:11 +1000
> Subject: [PATCH] service: Restart dependent services on service restart
>
> * modules/shepherd/service.scm (required-by?): New procedure.
> (stop): Return a list of canonical-names for stopped dependent services,
> including transitive dependencies.
> (action)[restart]: Start services based on the return value of stop.
> (fold-services): New procedure.
> * tests/restart.sh: New file.
> * Makefile.am (TESTS): Add tests/restart.sh.

[...]

Toggle quote (16 lines)> +# Restart test1 and make sure that both services are still running (ie. that
> +# test2 hasn't been stopped)
> +$herd restart test1
> +$herd status test1 | grep started
> +$herd status test2 | grep started
> +
> +# Now let's test with a transitive dependency
> +$herd start test3
> +$herd status test3 | grep started
> +
> +# After restarting test1 we want test3 to still be running
> +$herd restart test1
> +$herd status test1 | grep started
> +$herd status test2 | grep started
> +$herd status test3 | grep started

For clarity, should we do an explicit “herd stop test1” followed by

“herd start test1”? I know it’s currently equivalent under the hood,

but it might be slightly clearer. WDYT?

Otherwise it LGTM. Thanks for addressing these longstanding issues!

Ludo’.

Carlo Zancanaro wrote on 26 Aug 2018 00:48

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 23170@debbugs.gnu.org)

Message-ID:871samqcz4.fsf@zancanaro.id.au

On Sun, Aug 26 2018, Ludovic Courtès wrote:

Toggle quote (7 lines)

> I wonder if there are cases where one might want to restart a

> service without restarting its dependent services. We can

> probably ignore it for now, but perhaps we’ll need to add a flag

> or a separate action later.

> Thoughts?

I think this is best served by 'herd stop', followed by 'herd

start'. This patch just special-cases the 'restart' action, so

manually stopping then starting a service will behave as the old

restart used to.

Toggle quote (4 lines)

> For clarity, should we do an explicit “herd stop test1” followed

> by “herd start test1”? I know it’s currently equivalent under

> the hood, but it might be slightly clearer. WDYT?

Hopefully the above also answers this, too. I did consider whether 
it was worth adding a test for 'herd stop' to make sure it still 
stops dependent services, and 'herd start' to make sure it doesn't 
start dependent services, but in the end I decided not to. I'm 
happy to send through another patch to test these cases, though, 
if you think it would be worthwhile.

Carlo

Ludovic Courtès wrote on 26 Aug 2018 23:08

Recipients:(name . Carlo Zancanaro)(address . carlo@zancanaro.id.au)(address . 23170@debbugs.gnu.org)

Message-ID:87ftz07s4t.fsf@gnu.org

Carlo Zancanaro <carlo@zancanaro.id.au> skribis:

Toggle quote (13 lines)> On Sun, Aug 26 2018, Ludovic Courtès wrote:
>> I wonder if there are cases where one might want to restart a
>> service without restarting its dependent services.  We can probably
>> ignore it for now, but perhaps we’ll need to add a flag or a
>> separate action later.
>>
>> Thoughts?
>
> I think this is best served by 'herd stop', followed by 'herd
> start'. This patch just special-cases the 'restart' action, so
> manually stopping then starting a service will behave as the old
> restart used to.

Great, I had overlooked this.

Toggle quote (6 lines)

>> For clarity, should we do an explicit “herd stop test1” followed by

>> “herd start test1”? I know it’s currently equivalent under the

>> hood, but it might be slightly clearer. WDYT?

> Hopefully the above also answers this, too.

It does, thanks!

Toggle quote (6 lines)

> I did consider whether it was worth adding a test for 'herd stop' to

> make sure it still stops dependent services, and 'herd start' to make

> sure it doesn't start dependent services, but in the end I decided not

> to. I'm happy to send through another patch to test these cases,

> though, if you think it would be worthwhile.

No, that’s fine.

I forgot if this was already done, but perhaps you can add a bit in the

manual to insist that ‘restart’ is not quite the same as ‘stop’ +

‘start’.

Anyway, it all LGTM, thanks!

Ludo’.

Carlo Zancanaro wrote on 27 Aug 2018 00:05

Recipients:(address . ludo@gnu.org)(address . 23170@debbugs.gnu.org)

Message-ID:7A4A9C8A-E937-44A9-B941-B8263B00093A@zancanaro.id.au

Hey Ludo’!

On 27 August 2018 7:08:34 am AEST, ludo@gnu.org wrote:

Toggle quote (2 lines)

>I forgot if this was already done, but perhaps you can add a bit in the manual to insist that ‘restart’ is not quite the same as ‘stop’ + ‘start’.

I hadn't done that, but I have now. There aren't many mentions of restart in the manual, but I changed the one that seemed most relevant.

Toggle quote (2 lines)

>Anyway, it all LGTM, thanks!

Pushed! Thanks for the review.

Carlo

Attachment: file

Carlo Zancanaro wrote on 27 Aug 2018 00:05

Recipients:(address . ludo@gnu.org)(address . 23170@debbugs.gnu.org)

Message-ID:BC9EC36A-3916-4DB8-B70F-785B692BC4A4@zancanaro.id.au

Hey Ludo’!

On 27 August 2018 7:08:34 am AEST, ludo@gnu.org wrote:

Toggle quote (2 lines)

>I forgot if this was already done, but perhaps you can add a bit in the manual to insist that ‘restart’ is not quite the same as ‘stop’ + ‘start’.

I hadn't done that, but I have now. There aren't many mentions of restart in the manual, but I changed the one that seemed most relevant.

Toggle quote (2 lines)

>Anyway, it all LGTM, thanks!

Pushed! Thanks for the review.

Carlo

Ludovic Courtès wrote on 27 Aug 2018 13:05

Recipients:(name . Carlo Zancanaro)(address . carlo@zancanaro.id.au)(address . 23170@debbugs.gnu.org)

Message-ID:87o9docboc.fsf@gnu.org

Hi,

Carlo Zancanaro <carlo@zancanaro.id.au> skribis:

Toggle quote (2 lines)

> Pushed! Thanks for the review.

Great!

I see that you also reverted the patch that removed the ‘EINTR-safe’

workaround. Could you explain why that was necessary? (It should not

be necessary with current Guile versions.)

Thank you,

Ludo’.

Carlo Zancanaro wrote on 27 Aug 2018 14:42

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 23170@debbugs.gnu.org)

Message-ID:87mut8dlq7.fsf@zancanaro.id.au

On Mon, Aug 27 2018, Ludovic Courtès wrote:

Toggle quote (5 lines)

> I see that you also reverted the patch that removed the

> ‘EINTR-safe’ workaround. Could you explain why that was

> necessary? (It should not be necessary with current Guile

> versions.)

I'm not really sure of the details, but as I mentioned on IRC, 
that commit stopped me from being able to boot. I grafted a new 
version of the Shepherd and reconfigured my system, and when it 
came up my screen was filled with messages complaining that it 
couldn't create things in /dev because they already existed. I 
tested the commits since the previous release and that was the one 
that caused my problem.

I'd like to investigate further, but I have no idea what could be 
causing it. All I have to go on so far is that I think the udev 
service is getting respawned repeatedly, but I don't know why that 
commit would cause that problem. After reverting that commit I 
could successfully boot back into my system and everything is 
working properly again.

Clément Lassieur wrote on 27 Aug 2018 19:53

Recipients:(name . Carlo Zancanaro)(address . carlo@zancanaro.id.au)

Message-ID:8736uzbssi.fsf@lassieur.org

Carlo Zancanaro <carlo@zancanaro.id.au> writes:

Toggle quote (6 lines)

>>Anyway, it all LGTM, thanks!

> Pushed! Thanks for the review.

> Carlo

Awesome! Thanks a lot Carlo for working on this :-)

Ludovic Courtès wrote on 5 Jan 2019 14:53

control message for bug #23170

Recipients:(address . control@debbugs.gnu.org)

Message-ID:87y37z5hyn.fsf@gnu.org

tags 23170 fixed

close 23170

Your comment

This issue is archived.

To comment on this conversation send an email to 23170@debbugs.gnu.org

is:open	open issues
is:done	closed issues
submitter:<who>	search issue submitter
author:<who>	search by message author
date:yesterday..now	search by issue date
mdate:3m..2d	search by message date