install: configure docker to NOT use containerd-snapshotter#3951
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Writing a new /etc/docker/daemon.json unconditionally will overwrite any existing Docker daemon configuration; consider merging with an existing file or appending this feature flag in a safer, non-destructive way.
- The comment mentions two ways the containerd image store breaks BlueOS but only describes one; either clarify the second issue or adjust the comment so it accurately reflects the behavior being documented.
- You may want to guard this configuration by Docker version (e.g., only when Docker >= 29) to avoid setting an unsupported or unnecessary feature flag on older Docker installations.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Writing a new /etc/docker/daemon.json unconditionally will overwrite any existing Docker daemon configuration; consider merging with an existing file or appending this feature flag in a safer, non-destructive way.
- The comment mentions two ways the containerd image store breaks BlueOS but only describes one; either clarify the second issue or adjust the comment so it accurately reflects the behavior being documented.
- You may want to guard this configuration by Docker version (e.g., only when Docker >= 29) to avoid setting an unsupported or unnecessary feature flag on older Docker installations.
## Individual Comments
### Comment 1
<location path="install/install.sh" line_range="170-177" />
<code_context>
+# That store breaks BlueOS in two ways: the booted docker.service talks to the
+# system containerd (a different data-root), so images baked into the image during
+# the build are invisible at runtime;
+mkdir -p /etc/docker
+cat <<'EOF' > /etc/docker/daemon.json
+{
+ "features": {
+ "containerd-snapshotter": false
+ }
+}
+EOF
+
if [ $RUNNING_IN_CI -eq 1 ]
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid blindly overwriting an existing /etc/docker/daemon.json
Unconditionally rewriting `daemon.json` will drop any existing Docker configuration (registry mirrors, log options, cgroup driver, etc.), potentially breaking customized setups. Please check whether `/etc/docker/daemon.json` exists and either merge in `features.containerd-snapshotter=false` (e.g., via `jq`) or only create the file when it doesn’t exist, instead of always overwriting it.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Automated PR Review0. Summary
Writes 1. Correctness & Implementation Bugs
8. Documentation
Generated by PR Review Bot. This is advisory, a human reviewer must still approve. |
That caused incompatibilities with DinD and increased disk usage
bcf3dd7 to
e366890
Compare
this is not a regular system. this is running within pimod in a chroot. Docker only starts after running the DinD script. |
|
I'm going to release a new version of BlueOS to also test the new image generation. |
That caused incompatibilities with DinD and increased disk usage
fix #3917
image for testing is available at https://github.com/Williangalvani/BlueOS/actions/runs/27427008535
Summary by Sourcery
Build:
Summary by Sourcery
Build: