feat: Support extra dracut settings for static IP#268
Conversation
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughChangesExtra dracut settings
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
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: 2
🧹 Nitpick comments (1)
tests/tests_extra_dracut_settings.yml (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove AI-generation artifact.
Consider removing this auto-generated comment before merging, as it does not add value to the codebase.
♻️ Proposed fix
-# Generated by: Cursor using models Cursor Grok 4.5, Composer 2.5, Sonnet 5🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/tests_extra_dracut_settings.yml` at line 1, Remove the auto-generated “Generated by” comment from tests/tests_extra_dracut_settings.yml, leaving the file content otherwise unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/tests_extra_dracut_settings.yml`:
- Around line 11-13: Extend the test task block in
tests_extra_dracut_settings.yml with a failure scenario for
nbde_client_extra_dracut_settings, passing an invalid value such as a dictionary
instead of a list of strings and asserting that role execution fails. Preserve
the existing success and idempotency checks.
- Around line 8-18: Update tests/tests_extra_dracut_settings.yml at lines 8-18
and 49-53: use ansible.builtin.include_tasks for both wrapper invocations, and
move nbde_client_extra_dracut_settings from the play-level vars into each
invocation’s vars block. Do not alter the other tasks unless required for these
corrections.
---
Nitpick comments:
In `@tests/tests_extra_dracut_settings.yml`:
- Line 1: Remove the auto-generated “Generated by” comment from
tests/tests_extra_dracut_settings.yml, leaving the file content otherwise
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 568d5884-403a-4e72-ab2c-a1cabc7d5491
📒 Files selected for processing (5)
README.mddefaults/main.ymltemplates/nbde_client.conftests/tests_default_vars.ymltests/tests_extra_dracut_settings.yml
|
[citest] |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ansible-lint.yml:
- Line 47: Update the tox-lsr dependency in the workflow’s pip3 install command
to reference a reviewed immutable commit SHA or trusted immutable tag instead of
the mutable tox-4.58-fixes branch, preserving the existing repository source.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c7d41000-7fab-4a1f-9223-ff66d823de85
📒 Files selected for processing (1)
.github/workflows/ansible-lint.yml
sergio-correia
left a comment
sergio-correia
left a comment
There was a problem hiding this comment.
A couple of issues to address beyond what CodeRabbit flagged.
Initramfs not rebuilt when only extra_dracut_settings changes
The template task at tasks/main-clevis.yml:18-24 has no notify: Handle nbde_client update initramfs. The only task that notifies the handler is "Perform clevis operations" (line 68), which only fires when bindings change. So if a user adds nbde_client_extra_dracut_settings on a host whose bindings are already in place, the conf file is updated but dracut -fv --regenerate-all never runs. The initramfs stays stale. Please add notify: Handle nbde_client update initramfs to the template task. This is safe because the template module only reports changed when the file content actually differs, and __nbde_client_update_initramfs is already true in vars/main.yml.
extra_dracut_settings silently dropped when early_boot is false
The template task is gated on when: nbde_client_early_boot | bool. If a user sets nbde_client_early_boot: false and provides nbde_client_extra_dracut_settings, the settings are silently ignored with no warning. Would it be worth adding a validation task that fails when both are set, something like:
- name: Fail when extra dracut settings are set but early boot is disabled
fail:
msg: >-
nbde_client_extra_dracut_settings has
{{ nbde_client_extra_dracut_settings | length }} entries, but
nbde_client_early_boot is false so they will not be written.
when:
- not (nbde_client_early_boot | bool)
- nbde_client_extra_dracut_settings | length > 0
5d98f3b to
f1dcc64
Compare
|
@sergio-correia Thanks! Done |
|
[citest] |
f1dcc64 to
9e3b0a1
Compare
|
@sergio-correia I reviewed the docs again - I had to add leading and trailing spaces to the kernel_cmdline like this:
|
|
[citest] |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tasks/main.yml`:
- Line 4: Update the module reference in the task to use the fully qualified
builtin name ansible.builtin.fail instead of fail, preserving the task’s
existing behavior and configuration.
- Around line 10-12: Update the early-boot condition in the task’s when clause
to evaluate nbde_client_early_boot through the bool filter, using the normalized
negated value before checking nbde_client_extra_dracut_settings.
In `@tests/tests_extra_dracut_settings.yml`:
- Around line 83-104: Add an always section to the failure-path task block
containing the required tagged cleanup for this test. Keep the existing rescue
assertions unchanged, and ensure cleanup runs after both expected and unexpected
failures.
- Around line 67-72: Update the “Clean up generated dracut config” task to
notify the existing “Handle nbde_client update initramfs” handler when the file
is removed, ensuring cleanup rebuilds initramfs and leaves subsequent test runs
isolated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cb0761bf-82ed-4de4-81fd-743f59437eda
📒 Files selected for processing (7)
README.mddefaults/main.ymltasks/main-clevis.ymltasks/main.ymltemplates/nbde_client.conftests/tests_default_vars.ymltests/tests_extra_dracut_settings.yml
🚧 Files skipped from review as they are similar to previous changes (4)
- templates/nbde_client.conf
- defaults/main.yml
- tests/tests_default_vars.yml
- README.md
Feature: Introduce the nbde_client_extra_dracut_settings variable. This is a list of strings written to /etc/dracut.conf.d/nbde_client.conf in addition to the implicit platform-specific __nbde_client_dracut_settings. Reason: NBDE clients with static IP addressing need custom early-boot network settings in dracut (for example kernel_cmdline with a static ip= and nameserver). Previously the role only wrote fixed platform settings, so users had to disable early boot or manage dracut config outside the role. Result: Users can pass extra dracut configuration lines via nbde_client_extra_dracut_settings (documented in README.md) while keeping the role's implicit settings. A new integration test verifies the static IP kernel_cmdline example is written to the conf file. Assisted-by: Cursor using models Cursor Grok 4.5, Composer 2.5, Sonnet 5 Signed-off-by: Rich Megginson <rmeggins@redhat.com>
9e3b0a1 to
45abc5a
Compare
|
[citest] |
|
the centos-9 failure is due to a python problem which looks like it has now been fixed in Testing Farm images but not in qcow2 images |
2 participants
Feature: Introduce the nbde_client_extra_dracut_settings variable.
This is a list of strings written to
/etc/dracut.conf.d/nbde_client.conf in addition to the implicit
platform-specific __nbde_client_dracut_settings.
Reason: NBDE clients with static IP addressing need custom early-boot
network settings in dracut (for example kernel_cmdline with a static
ip= and nameserver). Previously the role only wrote fixed platform
settings, so users had to disable early boot or manage dracut config
outside the role.
Result: Users can pass extra dracut configuration lines via
nbde_client_extra_dracut_settings (documented in README.md) while
keeping the role's implicit settings. A new integration test verifies
the static IP kernel_cmdline example is written to the conf file.
Assisted-by: Cursor using models Cursor Grok 4.5, Composer 2.5, Sonnet 5
Signed-off-by: Rich Megginson rmeggins@redhat.com
Summary by CodeRabbit