Fix race conditions when starting dind containers #4841
Conversation
WalkthroughThe changes revise the Docker daemon startup in the container's entrypoint script. The script now begins with a strict error mode and incorporates a random delay based on the container’s hostname. A new function, Changes
Sequence Diagram(s)sequenceDiagram
participant E as entrypoint.sh
participant D as start_docker_daemon()
participant Da as Docker Daemon
E->>E: set -e and random delay
E->>D: Invoke start_docker_daemon()
D->>Da: Kill existing daemon processes & clean up sockets
D->>Da: Start new Docker daemon instance
loop Up to 5 Attempts (45s Timeout)
D->>Da: Check daemon readiness
alt Daemon Ready
Da-->>D: Return success
else Daemon Not Ready
D-->>D: Retry initialization
end
end
Da-->>D: Log daemon output
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docker/bacalhau-dind/entrypoint.sh (2)
3-4: Enforcing Immediate Exit on Error
The addition of “set -e” ensures that the script exits as soon as any command fails, which is vital for avoiding cascading failures during container startup. Consider, if your environment supports it, also using “set -eo pipefail” for even stricter error checking.
73-77: Graceful Failure After Exhausting Retries
If all retry attempts fail, the script prints the final logs and exits with an error. For improved clarity and to follow common error logging practices, consider directing the error messages and logs to stderr. For instance:- echo "ERROR: Failed to start Docker daemon after $MAX_RETRIES attempts" - echo "Final Docker daemon logs:" - cat /var/log/dockerd.log - exit 1 + >&2 echo "ERROR: Failed to start Docker daemon after $MAX_RETRIES attempts" + >&2 echo "Final Docker daemon logs:" + cat /var/log/dockerd.log >&2 + exit 1This change helps separate error output from standard output in container logs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docker/bacalhau-dind/entrypoint.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build / Build Binary (windows, amd64)
- GitHub Check: build / Build Binary (darwin, arm64)
- GitHub Check: build / Build Binary (darwin, amd64)
- GitHub Check: build / Build Binary (linux, armv6)
- GitHub Check: lint / lint
- GitHub Check: build / Build Binary (linux, armv7)
- GitHub Check: build / Build Binary (linux, arm64)
- GitHub Check: build / Build Binary (linux, amd64)
- GitHub Check: lint / go-lint (ubuntu-latest)
🔇 Additional comments (3)
docker/bacalhau-dind/entrypoint.sh (3)
12-17: Mitigating the Thundering Herd Problem with a Random Delay
Introducing an initial random delay based on the container’s hostname is a clever strategy to reduce race conditions. The use of awk to compute a delay in milliseconds and then converting it to seconds for sleep is inventive. Please verify in your target environments that fractional sleep values are supported and that the hostname reliably produces a sufficiently random seed.
18-21: Configuring Retry Parameters
Defining MAX_RETRIES, ATTEMPT, and TIMEOUT is clear and self-explanatory. However, please double-check that the TIMEOUT value is correctly interpreted later in the readiness loop, as the measurement unit should match the sleep increments.
60-71: Robust Retry Loop Implementation
The retry loop that repeatedly calls “start_docker_daemon” for up to MAX_RETRIES attempts is implemented correctly. A brief pause between attempts is provided, which can help avoid immediate reattempts in case of transient startup issues.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docker/bacalhau-dind/entrypoint.sh (1)
3-4: Enable strict error handling.
Adding "set -e" ensures that the script exits immediately when any command fails, which is essential for robust error detection in critical startup routines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docker/bacalhau-dind/entrypoint.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build / Build Binary (windows, amd64)
- GitHub Check: build / Build Binary (darwin, arm64)
- GitHub Check: build / Build Binary (darwin, amd64)
- GitHub Check: build / Build Binary (linux, armv6)
- GitHub Check: build / Build Binary (linux, armv7)
- GitHub Check: build / Build Binary (linux, arm64)
- GitHub Check: build / Build Binary (linux, amd64)
- GitHub Check: lint / lint
- GitHub Check: lint / go-lint (ubuntu-latest)
🔇 Additional comments (5)
docker/bacalhau-dind/entrypoint.sh (5)
12-17: Mitigate thundering herd with a randomized delay.
Introducing an initial random delay (0–2000 ms) based on the container’s hostname is an effective strategy to prevent simultaneous container startups. The use of awk to compute a fractional sleep value is acceptable assuming the sleep utility supports fractional seconds.
18-21: Clear initialization of retry parameters.
Setting MAX_RETRIES, ATTEMPT, and TIMEOUT explicitly aids in code readability and allows for easier future adjustments.
22-57: Robust Docker daemon startup via start_docker_daemon().
This function comprehensively handles the cleanup of any lingering Docker processes and socket files, then starts a fresh Docker daemon instance with proper background logging. The use of date-based arithmetic to calculate elapsed time accurately addresses previously noted timing discrepancies. Additionally, checking the process via "kill -0" ensures the daemon’s status is monitored effectively.Consider optionally verifying the presence and executability of "dockerd-entrypoint.sh" to preempt potential issues in environments where its location might vary.
59-70: Effective retry mechanism for Docker daemon startup.
The while-loop correctly implements a retry strategy by repeatedly invoking the start_docker_daemon() function. Incrementing the ATTEMPT counter and pausing briefly between retries adds resilience against transient failures.
72-77: Graceful termination upon persistent startup failure.
If the maximum retry count is exceeded, the script outputs an error along with the final Docker daemon logs before exiting. This ensures that any startup issues are clearly communicated for further debugging.
wdbaruni
deleted the
eng-611-fix-race-conditions-when-starting-dind-containers
branch
| ATTEMPT=1 | ||
| TIMEOUT=45 | ||
|
|
||
| start_docker_daemon() { |
There was a problem hiding this comment.
@wdbaruni stupid question - isn't the whole point of docker in docker is that you use the daemon that's already on the host?
There was a problem hiding this comment.
Actually Docker-in-Docker (using docker:dind) runs a separate Docker daemon inside the container, completely isolated from the host's daemon.
What you're thinking of is Docker-outside-of-Docker (DooD), where you mount the host's Docker socket (/var/run/docker.sock) to use the existing host daemon.
The docker:dind image specifically sets up a new, isolated daemon - that's why it needs the --privileged flag. If you want to use the host's daemon instead, you'd use the regular docker image and mount the socket.
Quick example:
- DinD (new daemon):
docker run --privileged docker:dind - DooD (host daemon):
docker run -v /var/run/docker.sock:/var/run/docker.sock docker
Fixes race conditions when multiple dind containers are started at the same time, which is usually the case with docker-compose
Summary by CodeRabbit