-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Start Supervisor container with its cidfile mounted to /run/cid #4276
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Use the --cidfile Docker CLI argument when starting the container and bind-mount the generated file containing full ID of the container to the container itself. Using --mount instead of --volume is needed, as --volume is racy and creates empty directory volume at the destination path instead. This is prerequisite for home-assistant/supervisor#6006 but can come handy for other cases too.
📝 WalkthroughWalkthroughAdds a SUPERVISOR_CIDFILE variable, removes any existing CID file before container creation, and updates the Docker create command to write the container ID to the CID file and bind-mount it into the container at /run/cid (read-only). Minor formatting changes included. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Init as System Init
participant Script as hassos-supervisor (script)
participant Docker as Docker Engine
participant Ctn as Supervisor Container
Init->>Script: Start supervisor
Script->>Script: Define SUPERVISOR_CIDFILE
alt CID file exists
Script->>Script: Remove existing CID file
end
Script->>Docker: docker create --cidfile CIDFILE<br/>--mount src=CIDFILE,dst=/run/cid,ro …
Docker-->>Script: Container ID written to CIDFILE
Docker-->>Ctn: Start container
Ctn-->>Ctn: Access /run/cid (read-only)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
buildroot-external/rootfs-overlay/usr/sbin/hassos-supervisor (1)
14-14: Introduce a separate temp CID path to avoid bind-mount vs --cidfile conflictDefining only SUPERVISOR_CIDFILE is fine, but we’ll need a second path for --cidfile to avoid colliding with the bind-mount’s “source must exist” rule (see detailed fix below). Also confirm the directory exists before first use.
Apply:
-SUPERVISOR_CIDFILE="${SUPERVISOR_DATA}/hassio_supervisor.cid" +SUPERVISOR_CIDFILE="${SUPERVISOR_DATA}/hassio_supervisor.cid" +# Separate tmp path for --cidfile to avoid conflict with bind mount existence checks. +SUPERVISOR_CIDFILE_TMP="${SUPERVISOR_CIDFILE}.tmp"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
buildroot-external/rootfs-overlay/usr/sbin/hassos-supervisor(3 hunks)
🔇 Additional comments (1)
buildroot-external/rootfs-overlay/usr/sbin/hassos-supervisor (1)
62-64: Comment tweak LGTMNo functional changes; message reads clearly.
| # We need to remove the CID file here, Docker will refuse to start if the | ||
| # file is present. Single instance is ensured by other code paths. | ||
| [ -f "${SUPERVISOR_CIDFILE}" ] && rm -f "${SUPERVISOR_CIDFILE}" | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Creation will fail: bind-mounting a non-existent file while using --cidfile to create it
Docker requires the bind source to exist at create time; meanwhile, --cidfile refuses to write if the file already exists. Removing the file (Lines 91–94) and then attempting to bind it (Lines 101–102) in the same docker container create leads to “invalid mount config for type 'bind': bind source path does not exist.”
Fix by:
- Pre-creating the file used as the bind source.
- Using a separate temp file for
--cidfile. - After create, move/copy the temp file’s content into the mounted file before
start.
Apply within this block:
- # We need to remove the CID file here, Docker will refuse to start if the
- # file is present. Single instance is ensured by other code paths.
- [ -f "${SUPERVISOR_CIDFILE}" ] && rm -f "${SUPERVISOR_CIDFILE}"
+ # Ensure bind source exists; keep a separate tmp for --cidfile.
+ mkdir -p "$(dirname "${SUPERVISOR_CIDFILE}")"
+ : > "${SUPERVISOR_CIDFILE}"
+ chmod 0644 "${SUPERVISOR_CIDFILE}" || true
+ [ -n "${SUPERVISOR_CIDFILE_TMP}" ] && rm -f "${SUPERVISOR_CIDFILE_TMP}"- --cidfile "${SUPERVISOR_CIDFILE}" \
+ --cidfile "${SUPERVISOR_CIDFILE_TMP}" \
--mount type=bind,src="${SUPERVISOR_CIDFILE}",dst=/run/cid,readonly \And insert right after the docker container create ... "${SUPERVISOR_IMAGE}:latest" line (before touching SUPERVISOR_STARTSCRIPT_VERSION):
+ # Populate the mounted CID file with the created container ID
+ if [ -s "${SUPERVISOR_CIDFILE_TMP}" ]; then
+ mv -f "${SUPERVISOR_CIDFILE_TMP}" "${SUPERVISOR_CIDFILE}"
+ else
+ echo "[ERROR] CID file was not written by Docker."
+ exit 1
+ fiOptional: if you prefer not to replace the inode for the mounted file, use cat "${SUPERVISOR_CIDFILE_TMP}" > "${SUPERVISOR_CIDFILE}" && rm -f "${SUPERVISOR_CIDFILE_TMP}".
Also applies to: 101-102
🤖 Prompt for AI Agents
In buildroot-external/rootfs-overlay/usr/sbin/hassos-supervisor around lines
91–94, removing SUPERVISOR_CIDFILE before creating the container causes Docker
to fail because bind sources must exist at create time; instead pre-create the
mount target file and use a separate temporary file for --cidfile during
container create (e.g. SUPERVISOR_CIDFILE_TMP), then after the container is
created copy/move the temp file content into the mounted SUPERVISOR_CIDFILE
before starting the container (or use cat SUPERVISOR_CIDFILE_TMP >
SUPERVISOR_CIDFILE && rm -f SUPERVISOR_CIDFILE_TMP to avoid replacing the
inode).
…un/cid There is no standard way to get the container ID in the container itself, which can be needed for instance for #6006. The usual pattern is to use the --cidfile argument of Docker CLI and mount the generated file to the container. However, this is feature of Docker CLI and we can't use it when creating the containers via API. To get container ID to implement native logging in e.g. Core as well, we need the help of the Supervisor. This change implements similar feature fully in Supervisor's DockerAPI class that orchestrates lifetime of all containers managed by Supervisor. The files are created in the SUPERVISOR_DATA directory, as it needs to be persisted between reboots, just as the instances of Docker containers are. Supervisor's cidfile must be created when starting the Supervisor itself, for that see home-assistant/operating-system#4276.
…un/cid There is no standard way to get the container ID in the container itself, which can be needed for instance for #6006. The usual pattern is to use the --cidfile argument of Docker CLI and mount the generated file to the container. However, this is feature of Docker CLI and we can't use it when creating the containers via API. To get container ID to implement native logging in e.g. Core as well, we need the help of the Supervisor. This change implements similar feature fully in Supervisor's DockerAPI class that orchestrates lifetime of all containers managed by Supervisor. The files are created in the SUPERVISOR_DATA directory, as it needs to be persisted between reboots, just as the instances of Docker containers are. Supervisor's cidfile must be created when starting the Supervisor itself, for that see home-assistant/operating-system#4276.
…un/cid (#6154) * Write cidfiles of Docker containers and mount them individually to /run/cid There is no standard way to get the container ID in the container itself, which can be needed for instance for #6006. The usual pattern is to use the --cidfile argument of Docker CLI and mount the generated file to the container. However, this is feature of Docker CLI and we can't use it when creating the containers via API. To get container ID to implement native logging in e.g. Core as well, we need the help of the Supervisor. This change implements similar feature fully in Supervisor's DockerAPI class that orchestrates lifetime of all containers managed by Supervisor. The files are created in the SUPERVISOR_DATA directory, as it needs to be persisted between reboots, just as the instances of Docker containers are. Supervisor's cidfile must be created when starting the Supervisor itself, for that see home-assistant/operating-system#4276. * Address review comments, fix mounting of the cidfile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Use the --cidfile Docker CLI argument when starting the container and bind-mount the generated file containing full ID of the container to the container itself.
Using --mount instead of --volume is needed, as --volume is racy and creates empty directory volume at the destination path instead.
This is prerequisite for home-assistant/supervisor#6006 but can come handy for other cases too.
Summary by CodeRabbit