* [Buildroot] [PATCH 1/2] package/docker-engine: rewrite dockerd init script @ 2024-07-29 12:20 Fiona Klute via buildroot 2024-07-29 12:20 ` [Buildroot] [RFC PATCH 2/2] package/docker-engine: add wrapper script for logging to syslog Fiona Klute via buildroot 2024-07-29 20:50 ` [Buildroot] [PATCH 1/2] package/docker-engine: rewrite dockerd init script Thomas Petazzoni via buildroot 0 siblings, 2 replies; 6+ messages in thread From: Fiona Klute via buildroot @ 2024-07-29 12:20 UTC (permalink / raw) To: buildroot; +Cc: Fiona Klute (WIWA), Christian Stewart 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(-) diff --git a/.checkpackageignore b/.checkpackageignore index 4fb65d231a..a9711f484f 100644 --- a/.checkpackageignore +++ b/.checkpackageignore @@ -460,7 +460,6 @@ package/dmalloc/0004-Makefile-use-the-configure-detected-or-user-supplied.patch package/dmalloc/0005-configure-use-LD-instead-of-hard-coding-ld.patch lib_patch.Upstream package/dmraid/0001-fix-compilation-under-musl.patch lib_patch.Upstream package/dmraid/S20dmraid lib_sysv.Variables -package/docker-engine/S60dockerd Shellcheck lib_sysv.Indent lib_sysv.Variables package/docopt-cpp/0001-only-build-one-target-use-BUILD_SHARED_LIBS-where-appropriate.patch lib_patch.Upstream package/domoticz/S99domoticz Shellcheck package/dovecot/0001-auth-Fix-handling-passdbs-with-identical-driver-args.patch lib_patch.Upstream diff --git a/package/docker-engine/S60dockerd b/package/docker-engine/S60dockerd index def8bea149..6850092eb2 100644 --- a/package/docker-engine/S60dockerd +++ b/package/docker-engine/S60dockerd @@ -1,38 +1,83 @@ #!/bin/sh -NAME=dockerd -DAEMON=/usr/bin/$NAME -PIDFILE=/var/run/$NAME.pid -DAEMON_ARGS="" - -[ -r /etc/default/$NAME ] && . /etc/default/$NAME $1 - -do_start() { - echo -n "Starting $NAME: " - start-stop-daemon --start --quiet --background --make-pidfile \ - --pidfile $PIDFILE --exec $DAEMON -- $DAEMON_ARGS \ - && echo "OK" || echo "FAIL" +DAEMON="dockerd" +PIDFILE="/var/run/$DAEMON.pid" +# expect dockerd to write its PID file within this number of seconds +# after start +STARTUP_TIMEOUT="5" + +DOCKERD_ARGS="" + +# shellcheck source=/dev/null +[ -r "/etc/default/$DAEMON" ] && . "/etc/default/$DAEMON" + +dockerd_running() { + start-stop-daemon --stop --test --quiet --pidfile "$PIDFILE" \ + --exec "/usr/bin/$DAEMON" + return $? +} + +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. + timeout="$(($(date +%s) + STARTUP_TIMEOUT))" + while [ "$(date +%s)" -lt "$timeout" ]; do + if dockerd_running; then + echo "OK" + return 0 + fi + sleep 0.1 + done + echo "FAIL" + return 1 +} + +stop() { + printf 'Stopping %s: ' "$DAEMON" + start-stop-daemon --stop --pidfile "$PIDFILE" --exec "/usr/bin/$DAEMON" + status=$? + if [ "$status" -eq 0 ]; then + echo "OK" + else + echo "FAIL" + return "$status" + fi + while dockerd_running; do + sleep 0.1 + done + rm -f "$PIDFILE" + return "$status" +} + +restart() { + stop + start } -do_stop() { - echo -n "Stopping $NAME: " - start-stop-daemon --stop --quiet --pidfile $PIDFILE \ - && echo "OK" || echo "FAIL" +reload() { + printf "Reloading %s config: " "$DAEMON" + start-stop-daemon --stop --signal HUP -q --pidfile "$PIDFILE" \ + --exec "/usr/bin/$DAEMON" + status=$? + if [ "$status" -eq 0 ]; then + echo "OK" + else + echo "FAIL" + fi + return "$status" } case "$1" in - start) - do_start - ;; - stop) - do_stop - ;; - restart) - do_stop - sleep 1 - do_start - ;; + start|stop|restart|reload) + "$1";; *) - echo "Usage: $0 {start|stop|restart}" - exit 1 + echo "Usage: $0 {start|stop|restart|reload}" + exit 1 esac -- 2.45.2 _______________________________________________ buildroot mailing list buildroot@buildroot.org https://lists.buildroot.org/mailman/listinfo/buildroot ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Buildroot] [RFC PATCH 2/2] package/docker-engine: add wrapper script for logging to syslog 2024-07-29 12:20 [Buildroot] [PATCH 1/2] package/docker-engine: rewrite dockerd init script Fiona Klute via buildroot @ 2024-07-29 12:20 ` Fiona Klute via buildroot 2024-07-29 20:50 ` [Buildroot] [PATCH 1/2] package/docker-engine: rewrite dockerd init script Thomas Petazzoni via buildroot 1 sibling, 0 replies; 6+ messages in thread From: Fiona Klute via buildroot @ 2024-07-29 12:20 UTC (permalink / raw) To: buildroot; +Cc: Fiona Klute (WIWA), Christian Stewart From: "Fiona Klute (WIWA)" <fiona.klute@gmx.de> Dockerd logs only to stdout/stderr [1], which is lost with --background. The upstream SysV init script [2] logs to a file by passing --no-close to start-stop-daemon and redirecting the output, but that option is not supported by Busybox' start-stop-daemon. The wrapper script added with this commit captures the output of dockerd (or whatever other command it is given) and forwards each line to syslog. [1] https://github.com/moby/moby/discussions/48260 [2] https://github.com/moby/moby/blob/50c3d19179e69f9e7ff01f688c4dbf32c5129ced/contrib/init/sysvinit-debian/docker Signed-off-by: Fiona Klute (WIWA) <fiona.klute@gmx.de> --- I'm not sure if this is the best solution, but I'm confident it's better than simply losing all the dockerd logs. The dockerd init script in Alpine uses a separate log_proxy tool, which is not packaged in Buildroot (and also focused on logging to files). If the wrapper script is acceptable, it might make sense to package it in a more generic way, so it can be used for other services that might have the same issue. Are there better options? Or could this approach be merged? package/docker-engine/S60dockerd | 7 +++++-- package/docker-engine/docker-engine.mk | 2 ++ package/docker-engine/dockerd-syslog-wrapper.sh | 4 ++++ 3 files changed, 11 insertions(+), 2 deletions(-) create mode 100644 package/docker-engine/dockerd-syslog-wrapper.sh diff --git a/package/docker-engine/S60dockerd b/package/docker-engine/S60dockerd index 6850092eb2..5814c14676 100644 --- a/package/docker-engine/S60dockerd +++ b/package/docker-engine/S60dockerd @@ -19,10 +19,13 @@ dockerd_running() { start() { printf 'Starting %s: ' "$DAEMON" + # Dockerd logs only to stdout/stderr, which is lost with + # --background. The wrapper script runs the given command + # (after "--") and forwards stdout/stderr to syslog. # 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 + --exec /usr/libexec/dockerd-syslog-wrapper.sh \ + -- "/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 diff --git a/package/docker-engine/docker-engine.mk b/package/docker-engine/docker-engine.mk index c7c51c5ef5..268b851520 100644 --- a/package/docker-engine/docker-engine.mk +++ b/package/docker-engine/docker-engine.mk @@ -72,6 +72,8 @@ endef define DOCKER_ENGINE_INSTALL_INIT_SYSV $(INSTALL) -D -m 755 package/docker-engine/S60dockerd \ $(TARGET_DIR)/etc/init.d/S60dockerd + $(INSTALL) -D -m 755 package/docker-engine/dockerd-syslog-wrapper.sh \ + $(TARGET_DIR)/usr/libexec/dockerd-syslog-wrapper.sh endef define DOCKER_ENGINE_USERS diff --git a/package/docker-engine/dockerd-syslog-wrapper.sh b/package/docker-engine/dockerd-syslog-wrapper.sh new file mode 100644 index 0000000000..3d6a80a739 --- /dev/null +++ b/package/docker-engine/dockerd-syslog-wrapper.sh @@ -0,0 +1,4 @@ +#!/bin/sh +"${@}" 2>&1 | while read -r LINE; do + logger -t "$(basename "${1}")" "$LINE"; +done -- 2.45.2 _______________________________________________ buildroot mailing list buildroot@buildroot.org https://lists.buildroot.org/mailman/listinfo/buildroot ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Buildroot] [PATCH 1/2] package/docker-engine: rewrite dockerd init script 2024-07-29 12:20 [Buildroot] [PATCH 1/2] package/docker-engine: rewrite dockerd init script Fiona Klute via buildroot 2024-07-29 12:20 ` [Buildroot] [RFC PATCH 2/2] package/docker-engine: add wrapper script for logging to syslog Fiona Klute via buildroot @ 2024-07-29 20:50 ` Thomas Petazzoni via buildroot 2024-07-30 15:33 ` Fiona Klute via buildroot 1 sibling, 1 reply; 6+ messages in thread From: Thomas Petazzoni via buildroot @ 2024-07-29 20:50 UTC (permalink / raw) To: Fiona Klute via buildroot; +Cc: Fiona Klute, Christian Stewart 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 -- Thomas Petazzoni, co-owner and CEO, Bootlin Embedded Linux and Kernel engineering and training https://bootlin.com _______________________________________________ buildroot mailing list buildroot@buildroot.org https://lists.buildroot.org/mailman/listinfo/buildroot ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Buildroot] [PATCH 1/2] package/docker-engine: rewrite dockerd init script 2024-07-29 20:50 ` [Buildroot] [PATCH 1/2] package/docker-engine: rewrite dockerd init script Thomas Petazzoni via buildroot @ 2024-07-30 15:33 ` Fiona Klute via buildroot 2024-07-30 15:50 ` Thomas Petazzoni via buildroot 0 siblings, 1 reply; 6+ messages in thread From: Fiona Klute via buildroot @ 2024-07-30 15:33 UTC (permalink / raw) To: Thomas Petazzoni, Fiona Klute via buildroot; +Cc: Christian Stewart 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 _______________________________________________ buildroot mailing list buildroot@buildroot.org https://lists.buildroot.org/mailman/listinfo/buildroot ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Buildroot] [PATCH 1/2] package/docker-engine: rewrite dockerd init script 2024-07-30 15:33 ` Fiona Klute via buildroot @ 2024-07-30 15:50 ` Thomas Petazzoni via buildroot 2024-07-31 17:51 ` Fiona Klute via buildroot 0 siblings, 1 reply; 6+ messages in thread From: Thomas Petazzoni via buildroot @ 2024-07-30 15:50 UTC (permalink / raw) To: Fiona Klute; +Cc: Christian Stewart, Fiona Klute via buildroot 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 -- Thomas Petazzoni, co-owner and CEO, Bootlin Embedded Linux and Kernel engineering and training https://bootlin.com _______________________________________________ buildroot mailing list buildroot@buildroot.org https://lists.buildroot.org/mailman/listinfo/buildroot ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Buildroot] [PATCH 1/2] package/docker-engine: rewrite dockerd init script 2024-07-30 15:50 ` Thomas Petazzoni via buildroot @ 2024-07-31 17:51 ` Fiona Klute via buildroot 0 siblings, 0 replies; 6+ messages in thread From: Fiona Klute via buildroot @ 2024-07-31 17:51 UTC (permalink / raw) To: Thomas Petazzoni; +Cc: Christian Stewart, Fiona Klute via buildroot 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 _______________________________________________ buildroot mailing list buildroot@buildroot.org https://lists.buildroot.org/mailman/listinfo/buildroot ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-07-31 17:51 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-07-29 12:20 [Buildroot] [PATCH 1/2] package/docker-engine: rewrite dockerd init script Fiona Klute via buildroot 2024-07-29 12:20 ` [Buildroot] [RFC PATCH 2/2] package/docker-engine: add wrapper script for logging to syslog Fiona Klute via buildroot 2024-07-29 20:50 ` [Buildroot] [PATCH 1/2] package/docker-engine: rewrite dockerd init script Thomas Petazzoni via buildroot 2024-07-30 15:33 ` Fiona Klute via buildroot 2024-07-30 15:50 ` Thomas Petazzoni via buildroot 2024-07-31 17:51 ` Fiona Klute via buildroot
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox