Add Minimal Ignition and Change InitContainer to Checks Only#689
Merged
Add Minimal Ignition and Change InitContainer to Checks Only#689
Conversation
The necessary tools, users and groups shall be provided by image and/or ignition.
Also do only checks in the Init container and rely on Ignition for user and group configuration, etc...
github-actions
bot
added
size/L
chore
documentation
lukasfrank
previously approved these changes
Feb 25, 2026
WalkthroughRenamed manager init container and environment variables, added libvirt package dependencies, replaced in-init-container user/group creation with explicit validations (including directory ownership/permissions), changed provider volume type to Directory, added an Ignition doc to provision host users/dirs, and updated site navigation. Changes
Sequence Diagram(s)sequenceDiagram
participant Ignition as Ignition (host)
participant Postinstall as postinstall.service
participant PackageMgr as Package Manager
participant HostFS as Host FS (/var/lib/libvirt-provider)
participant ManagerInit as Manager init (check-prerequisites)
participant Provider as libvirt-provider
Ignition->>Postinstall: install & enable /opt/postinstall.sh
Postinstall->>PackageMgr: install libvirt packages (libvirt-daemon-system, libvirt-clients, ovmf, ceph-common)
Postinstall->>HostFS: create /var/lib/libvirt-provider (uid/gid 65532)
Postinstall->>HostFS: create & make executable /opt/postinstall.sh
Postinstall-->>Ignition: complete
Provider->>ManagerInit: start init (check-prerequisites)
ManagerInit->>HostFS: verify directory exists, owner, group, mode (0755)
ManagerInit->>ManagerInit: verify users/groups exist and memberships (LIBVIRT_GROUP, LIBVIRT_QEMU_GROUP, LIBVIRT_QEMU_USER, USER)
alt validations pass
ManagerInit->>Provider: allow startup
Provider->>Provider: run normally
else validation fail
ManagerInit->>Provider: exit with error (no auto-modification)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/manager/manager.yaml`:
- Around line 82-84: The current existence-only check for
$(LIBVIRT_PROVIDER_DIR) can pass when kubelet created the directory with default
root:root 0755; update the validation to assert both ownership and permission
bits match the expected values (e.g., correct uid:gid and mode that Ignition
should have set) rather than only testing -d. Locate the block checking
"$(LIBVIRT_PROVIDER_DIR)" and replace/augment it with a stat-based check (use
stat -c to read uid:gids and mode) and compare against the expected UID:GID and
octal mode; if they don’t match, log a clear error including the actual
owner/permissions and exit non-zero. Ensure the error message references
$(LIBVIRT_PROVIDER_DIR) and the actual values to aid troubleshooting.
- Around line 58-85: The validation block incorrectly uses command-substitution
syntax like $(LIBVIRT_GROUP) which attempts to run a command; replace all such
occurrences with POSIX parameter expansion ${LIBVIRT_GROUP},
${LIBVIRT_QEMU_GROUP}, ${USER}, ${USER_GROUP}, ${LIBVIRT_QEMU_USER}, and
${LIBVIRT_PROVIDER_DIR} in the getent, id -u, id -nG, grep checks to correctly
expand environment variables, and add a missing environment definition for
LIBVIRT_QEMU_GROUP in the env section so the getent check for
${LIBVIRT_QEMU_GROUP} can succeed.
- Around line 45-46: The manifest defines LIBVIRT_QEMU_USER but later references
LIBVIRT_QEMU_GROUP, causing a deterministic failure; fix by either adding a new
env entry named LIBVIRT_QEMU_GROUP with the appropriate value (e.g., the group
string expected by the check) or update the check to reference LIBVIRT_QEMU_USER
instead—modify the env block to include LIBVIRT_QEMU_GROUP or change the code
that reads LIBVIRT_QEMU_GROUP to use LIBVIRT_QEMU_USER so the variable
referenced actually exists.
In `@docs/config/ignition.md`:
- Around line 6-7: The doc currently declares "variant: fcos" but still shows
apt-get usage; update the FCOS example to use rpm-ostree layering by replacing
the apt-get commands (the apt-get install lines referenced) with a systemd
oneshot unit that runs "rpm-ostree install <packages>" on first boot and
triggers a reboot so the layered packages apply; keep the "variant: fcos"
context and show the recommended pattern (systemd oneshot service + rpm-ostree
install + reboot) instead of apt-get commands.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
config/manager/manager.yaml (2)
27-27: Container name is misleading after refactoring.The container is named
create-libvirt-userbut now only performs validation checks (no longer creates users or modifies groups). Consider renaming to reflect its actual purpose.- - name: create-libvirt-user + - name: validate-libvirt-config🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/manager/manager.yaml` at line 27, The container named create-libvirt-user is misleading because it now only runs validation; rename the container to something like libvirt-validation (or validate-libvirt-config) in the manifest where the container name is defined (replace create-libvirt-user with the new name), and update any related identifiers that reference it (container name references in probes, volumeMounts, env, CI scripts, RBAC/mount policies, and any docs/log messages) so they stay consistent. Ensure the new name appears wherever the old name was used (startup scripts, health checks, Pod templates, and deployment/daemonset references) and run a quick deploy/test to confirm there are no missing references.
60-87: The$(VAR)syntax is correct for Kubernetes manifests.Contrary to a past review comment, the
$(VAR)syntax used here is the correct Kubernetes variable substitution syntax—not shell command substitution. Kubernetes performs this substitution before passing the script to the container, so bash receives the literal values (e.g.,getent group libvirt).The validation logic is sound and covers all required checks for groups, users, memberships, and directory existence.
Optional improvement: Suppress stdout from
getentandid -uto reduce log noise:♻️ Suggested cleanup for quieter logs
- if ! getent group $(LIBVIRT_GROUP); then + if ! getent group $(LIBVIRT_GROUP) >/dev/null; then echo "Error: $(LIBVIRT_GROUP) group does not exist." exit 1 fi - if ! getent group $(LIBVIRT_QEMU_GROUP); then + if ! getent group $(LIBVIRT_QEMU_GROUP) >/dev/null; then echo "Error: $(LIBVIRT_QEMU_GROUP) group does not exist." exit 1 fi - if ! id -u $(USER); then + if ! id -u $(USER) >/dev/null 2>&1; then echo "Error: $(USER) user does not exist." exit 1 fi - if ! getent group $(USER_GROUP); then + if ! getent group $(USER_GROUP) >/dev/null; then echo "Error: $(USER_GROUP) group does not exist." exit 1 fi,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/manager/manager.yaml` around lines 60 - 87, Quiet the validation checks by suppressing command stdout/stderr: redirect getent and id -u calls to /dev/null (e.g., change checks using getent group $(LIBVIRT_GROUP) and getent group $(LIBVIRT_QEMU_GROUP) and id -u $(USER) to run with >/dev/null 2>&1) so they only return exit status, and ensure other membership checks (id -nG $(USER) | grep -qw $(LIBVIRT_GROUP) and id -nG $(LIBVIRT_QEMU_USER) | grep -qw $(USER_GROUP)) keep their current pipeline behavior so output remains quiet due to grep -q.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@config/manager/manager.yaml`:
- Line 27: The container named create-libvirt-user is misleading because it now
only runs validation; rename the container to something like libvirt-validation
(or validate-libvirt-config) in the manifest where the container name is defined
(replace create-libvirt-user with the new name), and update any related
identifiers that reference it (container name references in probes,
volumeMounts, env, CI scripts, RBAC/mount policies, and any docs/log messages)
so they stay consistent. Ensure the new name appears wherever the old name was
used (startup scripts, health checks, Pod templates, and deployment/daemonset
references) and run a quick deploy/test to confirm there are no missing
references.
- Around line 60-87: Quiet the validation checks by suppressing command
stdout/stderr: redirect getent and id -u calls to /dev/null (e.g., change checks
using getent group $(LIBVIRT_GROUP) and getent group $(LIBVIRT_QEMU_GROUP) and
id -u $(USER) to run with >/dev/null 2>&1) so they only return exit status, and
ensure other membership checks (id -nG $(USER) | grep -qw $(LIBVIRT_GROUP) and
id -nG $(LIBVIRT_QEMU_USER) | grep -qw $(USER_GROUP)) keep their current
pipeline behavior so output remains quiet due to grep -q.
lukasfrank
reviewed
Feb 27, 2026
friegger
changed the title
There was a problem hiding this comment.
♻️ Duplicate comments (1)
config/manager/manager.yaml (1)
68-70:⚠️ Potential issue | 🟡 Minor
id -ustill leaks stderr noise on failure.At Line 68, only stdout is redirected. Missing users still emit
id:errors on stderr before your custom message, which adds avoidable log noise.Suggested patch
- if ! id -u $(USER) > /dev/null; then + if ! id -u $(USER) >/dev/null 2>&1; then echo "Error: $(USER) user does not exist." exit 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/manager/manager.yaml` around lines 68 - 70, The id check currently only redirects stdout so failing lookups still print id's stderr; update the conditional that uses id -u $(USER) > /dev/null to also discard stderr (redirect it to /dev/null) and quote the $(USER) expansion to be robust; leave the existing echo/exit behavior unchanged (i.e., change only the redirection and quoting where id -u is invoked).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@config/manager/manager.yaml`:
- Around line 68-70: The id check currently only redirects stdout so failing
lookups still print id's stderr; update the conditional that uses id -u $(USER)
> /dev/null to also discard stderr (redirect it to /dev/null) and quote the
$(USER) expansion to be robust; leave the existing echo/exit behavior unchanged
(i.e., change only the redirection and quoting where id -u is invoked).
lukasfrank
approved these changes
Feb 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Also do only checks in the Init container and rely on Ignition
for user and group configuration, etc...
Fixes #688
Summary by CodeRabbit
Documentation
Configuration