From 071898c96bcfb482730290293dc591e010ec7595 Mon Sep 17 00:00:00 2001 From: Richard Fussenegger Date: Thu, 23 Jun 2022 02:37:01 +0200 Subject: [PATCH] Overhauled `startup.sh` Script (#1454) This overhaul turns it into a shellcheck valid script with explicit error handling for all possible situations I could think of. This change takes https://github.com/actions-runner-controller/actions-runner-controller/pull/1409 into account and things can be merged in any order. There are a few important changes here to the logic: - The wait logic for checking if docker comes up was fundamentally flawed because it checks for the PID. Docker will always come up and thus become visible in the process list, just to immediately die when it encounters an issue, after which supervisor starts it again. This means that our check so far is flaky due to the `sleep 1` it might encounter a PID, or it might not, and the existence of the PID does not mean anything. The `docker ps` check we have in the `entrypoint.sh` script does not suffer from this as it checks for a feature of docker and not a PID. I thus entirely removed the PID check, and instead I am handing things over to our `entrypoint.sh` script by setting the environment variables correctly. - This change has an influence on the `docker0` interface MTU configuration, because the interface might or might not exist after we started docker. Hence, I changed this to a time boxed loop that tries for one minute to set up the interface's MTU. In case the command fails we log an error and continue with the run. - I changed the entire MTU handling by validating its value before configuring it, logging an error and continuing without if it is set incorrectly. This ensures that we are not going to send our users on a bug hunt. - The way we started supervisord did not make much sense to me. It sends itself into the background automatically, there is no need for us to do so with Bash. The decision to not fail on errors but continue is a deliberate choice, because I believe that running a build is more important than having a perfectly configured system. However, this strategy might also hide issues for all users who are not properly checking their logs. It also makes testing harder. Hence, we could change all error conditions from graceful to panicking. We should then align the exit codes across `startup.sh` and `entrypoint.sh` to ensure that every possible error condition has its own unique error code for easy debugging. --- runner/startup.sh | 119 ++++++++++++++++++++++++---------------------- 1 file changed, 62 insertions(+), 57 deletions(-) diff --git a/runner/startup.sh b/runner/startup.sh index 6352a0d5..dd287f8a 100755 --- a/runner/startup.sh +++ b/runner/startup.sh @@ -1,70 +1,75 @@ -#!/bin/bash -source logger.bash +#!/usr/bin/env bash +set -Eeuo pipefail -function wait_for_process () { - local max_time_wait=30 - local process_name="$1" - local waited_sec=0 - while ! pgrep "$process_name" >/dev/null && ((waited_sec < max_time_wait)); do - log.debug "Process $process_name is not running yet. Retrying in 1 seconds" - log.debug "Waited $waited_sec seconds of $max_time_wait seconds" - sleep 1 - ((waited_sec=waited_sec+1)) - if ((waited_sec >= max_time_wait)); then - return 1 - fi - done - return 0 -} +# This path can only be customized through arguments passed to the dockerd cli +# at startup. Parsing the supervisor docker.conf file is cumbersome, and there +# is little reason to allow users to customize this. Hence, we hardcode the path +# here. +# +# See: https://docs.docker.com/engine/reference/commandline/dockerd/ +readonly dockerd_config_path=/etc/docker/daemon.json +readonly dockerd_supervisor_config_path=/etc/supervisor/conf.d/dockerd.conf -sudo /bin/bash <