[1/2] package/docker-engine: rewrite dockerd init script

From: "Fiona Klute (WIWA)" <fiona.klute@gmx.de>

This brings the dockerd init script in line with the standard
Buildroot init script pattern.

Reload using SIGHUP is also supported now, note that the Docker
documentation cautions that not all parameters can be changed at
runtime (without a full restart).

Signed-off-by: Fiona Klute (WIWA) <fiona.klute@gmx.de>
---
Please note that while the second patch in this series is an RFC, this
is not and in my opinion can be merged with or without the syslog fix.

 .checkpackageignore              |   1 -
 package/docker-engine/S60dockerd | 103 ++++++++++++++++++++++---------
 2 files changed, 74 insertions(+), 30 deletions(-)

--
2.45.2

Hello Fiona,

Thanks for working on this. One question below.

On Mon, 29 Jul 2024 14:20:10 +0200
Fiona Klute via buildroot <buildroot@buildroot.org> wrote:

> +start() {
> +	printf 'Starting %s: ' "$DAEMON"
> +	# shellcheck disable=SC2086 # we need word splitting for DOCKERD_ARGS
> +	start-stop-daemon --start --background --pidfile "$PIDFILE" \
> +		--exec "/usr/bin/$DAEMON" \
> +		-- --pidfile "$PIDFILE" $DOCKERD_ARGS
> +	# We have to use --background because dockerd doesn't
> +	# daemonize on its own. Unfortunately that means
> +	# start-stop-daemon cannot check the return status, so at
> +	# least check the process is actually running.

How is this different than the other init scripts for which we use
--background?

package/at/S99at
package/busybox/S01syslogd (which was recently reworked)
package/busybox/S41ifplugd (same)
package/busybox/S50crond (same)

While would need the while loop waiting for the daemon to actually be
started in the docker-engine init script and not the other services?

Thanks!

Thomas

Hi Thomas!

Am 29.07.24 um 22:50 schrieb Thomas Petazzoni:
> Hello Fiona,
>
> Thanks for working on this. One question below.
>
> On Mon, 29 Jul 2024 14:20:10 +0200
> Fiona Klute via buildroot <buildroot@buildroot.org> wrote:
>
>> +start() {
>> +	printf 'Starting %s: ' "$DAEMON"
>> +	# shellcheck disable=SC2086 # we need word splitting for DOCKERD_ARGS
>> +	start-stop-daemon --start --background --pidfile "$PIDFILE" \
>> +		--exec "/usr/bin/$DAEMON" \
>> +		-- --pidfile "$PIDFILE" $DOCKERD_ARGS
>> +	# We have to use --background because dockerd doesn't
>> +	# daemonize on its own. Unfortunately that means
>> +	# start-stop-daemon cannot check the return status, so at
>> +	# least check the process is actually running.
>
> How is this different than the other init scripts for which we use
> --background?
>
> package/at/S99at
> package/busybox/S01syslogd (which was recently reworked)
> package/busybox/S41ifplugd (same)
> package/busybox/S50crond (same)
>
> While would need the while loop waiting for the daemon to actually be
> started in the docker-engine init script and not the other services?

There isn't a fundamental difference, I might have over-engineered a bit
while thinking about experiments with the Docker config and how it'd be
annoying if I change the config and restart, see "OK", and then the
service isn't running. Effectively the loop only confirms that the
service reaches the point where it writes its PID file (that's something
those other services don't do, note the absence of --make-pidfile), but
it might still crash after.

It should be fine to drop the loop and accept the same limitation as in
those other scripts (if the binary can be executed and then crashes,
output still says "OK").

I can respin, I'm just hoping to get an opinion on the second patch
first (the log capture one), so I know whether to modify or drop that in v2.

Best regards,
Fiona

Hello Fiona,

On Tue, 30 Jul 2024 17:33:07 +0200
Fiona Klute <fiona.klute@gmx.de> wrote:

> > While would need the while loop waiting for the daemon to actually be
> > started in the docker-engine init script and not the other services?  
> 
> There isn't a fundamental difference, I might have over-engineered a bit
> while thinking about experiments with the Docker config and how it'd be
> annoying if I change the config and restart, see "OK", and then the
> service isn't running. Effectively the loop only confirms that the
> service reaches the point where it writes its PID file (that's something
> those other services don't do, note the absence of --make-pidfile), but
> it might still crash after.
> 
> It should be fine to drop the loop and accept the same limitation as in
> those other scripts (if the binary can be executed and then crashes,
> output still says "OK").

To me, the whole goal of the effort you have started (and which is
super nice!) is to have consistency among the init scripts, so I would
really prefer that a given situation is handled in exactly the same way
in all init scripts.

BTW, in another e-mail, I had suggested to improve a bit our
documentation to describe the different situations, and how we intend
to handle each situation in our init script.

> I can respin, I'm just hoping to get an opinion on the second patch
> first (the log capture one), so I know whether to modify or drop that in v2.

I don't have a super super strong opinion PATCH 2/2. Wrapper scripts
always tend to make things a bit more "obscure". My initial thought
was: but if all what's missing is --no-close support in Busybox, why
don't we implement it? On the other hand, your wrapper script allows
the logging to go into syslog (which is configurable), while with
--no-close, it simply gets redirected to a file, and that's it (which I
suppose we can say is less configurable than having logs going through
syslog).

Thomas

Am 30.07.24 um 17:50 schrieb Thomas Petazzoni:
> Hello Fiona,
>
> On Tue, 30 Jul 2024 17:33:07 +0200
> Fiona Klute <fiona.klute@gmx.de> wrote:
>
>>> While would need the while loop waiting for the daemon to actually be
>>> started in the docker-engine init script and not the other services?
>>
>> There isn't a fundamental difference, I might have over-engineered a bit
>> while thinking about experiments with the Docker config and how it'd be
>> annoying if I change the config and restart, see "OK", and then the
>> service isn't running. Effectively the loop only confirms that the
>> service reaches the point where it writes its PID file (that's something
>> those other services don't do, note the absence of --make-pidfile), but
>> it might still crash after.
>>
>> It should be fine to drop the loop and accept the same limitation as in
>> those other scripts (if the binary can be executed and then crashes,
>> output still says "OK").
>
> To me, the whole goal of the effort you have started (and which is
> super nice!) is to have consistency among the init scripts, so I would
> really prefer that a given situation is handled in exactly the same way
> in all init scripts.

Makes sense, I'll respin in that way. :-)

> BTW, in another e-mail, I had suggested to improve a bit our
> documentation to describe the different situations, and how we intend
> to handle each situation in our init script.

I saw that, and agree it makes sense. I'll have to see if/when I can fit
a documentation update in.

>> I can respin, I'm just hoping to get an opinion on the second patch
>> first (the log capture one), so I know whether to modify or drop that in v2.
>
> I don't have a super super strong opinion PATCH 2/2. Wrapper scripts
> always tend to make things a bit more "obscure". My initial thought
> was: but if all what's missing is --no-close support in Busybox, why
> don't we implement it? On the other hand, your wrapper script allows
> the logging to go into syslog (which is configurable), while with
> --no-close, it simply gets redirected to a file, and that's it (which I
> suppose we can say is less configurable than having logs going through
> syslog).

Yes, exactly, and the log_proxy that Alpine uses has some extra handling
to avoid losing some lines during (outside) log rotation. I'd very much
prefer keeping the redirection to syslog. The proper fix in my eyes
would be syslog support in the Docker engine itself (which ironically
already exists for containers).

Best regards,
Fiona