test: test reboot and unlock with separate nbde_server machine#266
test: test reboot and unlock with separate nbde_server machine#266richm wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR adds multi-host test support for the nbde_client role: a qemu drive size setting in provisioning config, conditional multi-host setup logic for encrypted test storage, and a new multi-host playbook that deploys the server, runs the client flow, captures diagnostics, and cleans up. ChangesMulti-host testing support
Related PRs: None identified. Suggested labels: tests, enhancement Suggested reviewers: None identified. 🐰 A drive of ten gigs tucked in place, 🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 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 (4)
tests/tests_simple_bind_multihost.yml (3)
9-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOnly the success/bind scenario is tested.
Path instructions state tests should verify both success and failure scenarios. This playbook only covers the successful bind + reboot + unlock path. Consider whether a companion failure scenario (e.g., wrong password/servers, unreachable tang server) is planned elsewhere in the stack.
As per path instructions: "Tests should verify both success and failure scenarios."
🤖 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_simple_bind_multihost.yml` around lines 9 - 67, The tests in tests_simple_bind_multihost.yml only cover the successful bind, reboot, and unlock flow, but the path instructions require both success and failure coverage. Extend the playbook or add a companion test case around nbde_client_bindings and the existing nbde-client task block to exercise a failing bind/unlock path, such as an invalid password or unreachable server, while keeping the current success scenario intact.Source: Path instructions
56-63: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse
assertinstead of acopy+failed_whentrick to verify post-reboot state.The path instructions call for using the
assertmodule to verify expected state after role execution. The current approach relies oncopyreportingchangedto infer content mismatch, which works but is indirect and departs from the documented pattern (e.g. slurp + assert).As per path instructions: "Use assert module to verify expected state after role execution."
♻️ Suggested alternative using assert
- - name: Verify that the test file is present with the unencrypted contents - copy: - content: "{{ nbde_client_test_file_contents }}" - dest: "{{ nbde_client_test_file }}" - mode: "0644" - register: __register_copy_test_file - failed_when: __register_copy_test_file is failed or __register_copy_test_file is changed + - name: Slurp the test file contents + ansible.builtin.slurp: + src: "{{ nbde_client_test_file }}" + register: __register_test_file_slurp + + - name: Verify that the test file is present with the unencrypted contents + ansible.builtin.assert: + that: + - (__register_test_file_slurp.content | b64decode) == nbde_client_test_file_contents + fail_msg: "Test file contents do not match expected unencrypted contents"🤖 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_simple_bind_multihost.yml` around lines 56 - 63, Replace the current post-reboot verification step in the test task with an explicit assert-based check instead of using copy plus failed_when. The issue is in the verification task that uses the registered result from the copy operation; update the test flow to read the file state and assert the expected unencrypted contents using the existing test variables such as nbde_client_test_file and nbde_client_test_file_contents, following the same pattern used elsewhere for state validation.Source: Path instructions
35-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win"Debug0" task looks like leftover debugging scaffolding.
This unconditional shell dump (df, mount, dmesg, journalctl, lsblk, etc.) doesn't assert anything and appears to be a debug artifact left in the test. Consider removing it or gating it behind a failure handler / debug tag so it only runs on failure.
🤖 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_simple_bind_multihost.yml` around lines 35 - 54, The Debug0 task in the test playbook is leftover debugging scaffolding and should not run unconditionally. Remove the Debug0 shell dump from the normal test flow, or move it behind a failure-only/debug path so it only executes when troubleshooting is needed. Use the Debug0 task name and the surrounding test task block in tests_simple_bind_multihost.yml to locate and clean it up.tests/tasks/setup_test.yml (1)
58-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate hostvars-length condition.
The single-host and multi-host gates repeat the same
hostvars.keys() | list | lengthexpression. Extracting it to a shared fact/var would reduce duplication and centralize the (currently questionable) detection logic.Also applies to: 93-93
🤖 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/tasks/setup_test.yml` at line 58, The host-count gate logic repeats the same hostvars-length check in multiple places, so factor that shared expression out into a single variable or fact and have both the single-host and multi-host conditions use it. Update the gating around the playbook conditions in the setup task flow so the host-count detection is centralized instead of duplicating `hostvars.keys() | list | length` in each `when` clause.
🤖 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/tasks/setup_test.yml`:
- Around line 95-122: The test setup is using only the first unused disk for
nbde_client_test_device, but the storage role is being given the full
__register_unused_disk.disks list, which can create a mismatched volume layout.
Update the setup around the set_fact tasks and the include_role for
fedora.linux_system_roles.storage so the same single disk is used consistently
for nbde_client_test_device and storage_volumes[0].disks, or otherwise ensure
the later validation logic matches the exact disks passed into the storage role.
In `@tests/tests_simple_bind_multihost.yml`:
- Around line 2-7: The `tests_simple_bind_multihost.yml` playbook is invoking
`fedora.linux_system_roles.nbde_server` directly with `include_role`, which
conflicts with the `tests/tests_*.yml` wrapper convention. Update the `Setup
nbde_server` task to use the centrally managed role wrapper instead of direct
role invocation, and keep the role call routed through the same wrapper pattern
used elsewhere in these test playbooks. If this file is meant to allow only
`nbde_client` to use the wrapper, then narrow or clarify the instruction scope
accordingly; otherwise, apply the wrapper consistently to `nbde_server` here as
well.
---
Nitpick comments:
In `@tests/tasks/setup_test.yml`:
- Line 58: The host-count gate logic repeats the same hostvars-length check in
multiple places, so factor that shared expression out into a single variable or
fact and have both the single-host and multi-host conditions use it. Update the
gating around the playbook conditions in the setup task flow so the host-count
detection is centralized instead of duplicating `hostvars.keys() | list |
length` in each `when` clause.
In `@tests/tests_simple_bind_multihost.yml`:
- Around line 9-67: The tests in tests_simple_bind_multihost.yml only cover the
successful bind, reboot, and unlock flow, but the path instructions require both
success and failure coverage. Extend the playbook or add a companion test case
around nbde_client_bindings and the existing nbde-client task block to exercise
a failing bind/unlock path, such as an invalid password or unreachable server,
while keeping the current success scenario intact.
- Around line 56-63: Replace the current post-reboot verification step in the
test task with an explicit assert-based check instead of using copy plus
failed_when. The issue is in the verification task that uses the registered
result from the copy operation; update the test flow to read the file state and
assert the expected unencrypted contents using the existing test variables such
as nbde_client_test_file and nbde_client_test_file_contents, following the same
pattern used elsewhere for state validation.
- Around line 35-54: The Debug0 task in the test playbook is leftover debugging
scaffolding and should not run unconditionally. Remove the Debug0 shell dump
from the normal test flow, or move it behind a failure-only/debug path so it
only executes when troubleshooting is needed. Use the Debug0 task name and the
surrounding test task block in tests_simple_bind_multihost.yml to locate and
clean it up.
🪄 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: fe9fc8e0-013b-4252-96df-0c43fd4c3328
📒 Files selected for processing (3)
tests/provision.fmftests/tasks/setup_test.ymltests/tests_simple_bind_multihost.yml
This relies on linux-system-roles/tox-lsr#244 or some other system by which you can create two machines with hostnames nbde-server and nbde-client that can network with each other. Run it like this: ```bash tox -e libvirt-ansible-core-2-X -- --image-name centos-Y --log-level debug --hostnames nbde-client --hostnames nbde-server -- \ tests/tests_simple_bind_multihost.yml 2>&1 | tee output ``` Signed-off-by: Rich Megginson <rmeggins@redhat.com>
|
[citest] |
1 participant
This relies on linux-system-roles/tox-lsr#244
or some other system by which you can create two machines with hostnames
nbde-server and nbde-client that can network with each other.
Run it like this:
Signed-off-by: Rich Megginson rmeggins@redhat.com
Summary by CodeRabbit
standard-inventory-qcow2target.