Repository navigation
OSAC-3402: Add E2E test for bare metal instance restart - #277
mennyaboush wants to merge 10 commits into
Conversation
Add a dedicated test for BMI restart (power cycle) that verifies the full restart_trigger flow: increment spec.restart_trigger via Update RPC, observe RESTART_IN_PROGRESS condition, wait for status.restart_trigger echo, confirm host comes back online, and clean up. Also adds update_baremetal_instance_restart_trigger() helper to GRPCClient for field-masked restart_trigger updates. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: MENNY ABOUSH <maboush@maboush-thinkpadt14gen5.raanaii.csb>
|
@mennyaboush: This pull request references OSAC-3402 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mennyaboush The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a gRPC method for updating a bare metal instance restart trigger. Adds lifecycle diagnostics and an end-to-end test for restart, recovery, deletion, and failure cleanup. ChangesBare Metal Instance Restart
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RestartTest as pytest restart test
participant GRPCClient
participant K8sHub as k8s_hub_client
RestartTest->>GRPCClient: Update spec.restart_trigger
GRPCClient->>GRPCClient: Call BareMetalInstances/Update
RestartTest->>GRPCClient: Poll restart conditions and status.restartTrigger
RestartTest->>K8sHub: Poll BMH power state
RestartTest->>GRPCClient: Verify Running state and delete instance
Possibly related PRs
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (9 passed)
✨ 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: 2
🤖 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/bmaas/test_baremetal_instance_restart.py`:
- Around line 77-83: Update the restart verification around poll_until to assert
a restart-specific power-cycle transition rather than only the final powered-on
state; observe the host becoming powered off before confirming it returns to
"true", or use an equivalent restart-specific signal exposed by the test client.
- Around line 112-113: Update the broad exception handler in the cleanup flow of
the bare-metal instance restart test to retain the best-effort cleanup while
replacing the silent pass with logger.exception(...). Log the cleanup failure
and traceback before continuing, so cleanup errors are visible without changing
test flow.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 938418a0-bc60-4566-b459-731f1cd60273
📒 Files selected for processing (2)
tests/bmaas/test_baremetal_instance_restart.pytests/core/grpc_client.py
Address CodeRabbit review feedback: replace bare `except: pass` with `logger.exception()` so cleanup failures are visible for same-session debugging. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: MENNY ABOUSH <maboush@maboush-thinkpadt14gen5.raanaii.csb>
The RESTART_IN_PROGRESS condition can flip TRUE→FALSE between polling intervals when the restart completes quickly, causing a false timeout. Instead, wait for the durable end state (status.restart_trigger echo) and verify RESTART_IN_PROGRESS is cleared afterward. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: MENNY ABOUSH <maboush@maboush-thinkpadt14gen5.raanaii.csb>
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: MENNY ABOUSH <maboush@maboush-thinkpadt14gen5.raanaii.csb>
|
/retest |
|
Re-triggered failed runs:
|
Poll every 2s for the condition to appear with any status (TRUE or FALSE), rather than requiring the exact TRUE state which is transient and can be missed between polling intervals when the restart completes quickly. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: MENNY ABOUSH <maboush@maboush-thinkpadt14gen5.raanaii.csb>
Log BMH inventory (name, provisioning state, online, powered, consumer) at test start and after BMH assignment to diagnose why the restart test consistently fails to provision while the lifecycle test succeeds on the same CI run. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: MENNY ABOUSH <maboush@maboush-thinkpadt14gen5.raanaii.csb>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/bmaas/test_baremetal_instance_restart.py (2)
90-96: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequire a true restart-in-progress condition.
The predicate
v != ""also acceptsCONDITION_STATUS_FALSE. A pre-existing false condition can satisfy this poll before the restart starts.Use
CONDITION_STATUS_TRUEor the matching project enum.Suggested fix
- until=lambda v: v != "", + until=lambda v: v == "CONDITION_STATUS_TRUE",🤖 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/bmaas/test_baremetal_instance_restart.py` around lines 90 - 96, Update the poll predicate in the restart test around _get_restart_condition_status so it only succeeds when the RESTART_IN_PROGRESS condition equals CONDITION_STATUS_TRUE, or the project’s equivalent enum value, rather than accepting any non-empty status; preserve the existing polling parameters and description.
134-142: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not skip cleanup when the CR name is empty.
The
if bmi_cr:guard prevents deletion when failure occurs beforewait_for_bmi_crobserves the CR. The BMI can remain in the backend and retain a BMH allocation.Always call
cli.delete_baremetal_instanceandwait_for_bmi_grpc_removalusingbmi_id. Make onlywait_for_bmi_deletionconditional onbmi_cr. Keep cleanup steps independently guarded so one failed cleanup operation does not skip the remaining steps.🤖 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/bmaas/test_baremetal_instance_restart.py` around lines 134 - 142, The cleanup block in the BaseException handler must not be gated by the CR name returned from get_baremetal_instance_name. In the cleanup logic, always call cli.delete_baremetal_instance and wait_for_bmi_grpc_removal with bmi_id; invoke wait_for_bmi_deletion only when bmi_cr is present, and keep each cleanup operation independently guarded so a failure does not prevent subsequent cleanup.
🤖 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/bmaas/test_baremetal_instance_lifecycle.py`:
- Around line 50-68: Update the BMI lifecycle flow after wait_for_bmi_cr and
before the assignment log so it polls
k8s_hub_client.get_baremetal_instance_external_host_id for a non-empty
external_host_id. Only after the ID is available should it log the BMH
assignment and call _log_bmh_inventory; keep the subsequent wait_for_bmi_running
flow unchanged.
- Around line 16-37: Update `_log_bmh_inventory` to remain best-effort by
catching `TimeoutExpired` and `FileNotFoundError` from `run_unchecked`, logging
a warning, and returning without propagating the error. Configure the inventory
`kubectl` call with a short diagnostic timeout instead of relying on the
300-second default, while preserving the existing non-zero return handling.
In `@tests/bmaas/test_baremetal_instance_restart.py`:
- Around line 74-78: Update the restart test flow around
get_baremetal_instance_external_host_id and wait_for_bmi_running to poll until
spec.externalHostID is non-empty before proceeding. After the ID becomes
available, validate its namespace and name, then retain the existing inventory
logging and running-state wait.
---
Outside diff comments:
In `@tests/bmaas/test_baremetal_instance_restart.py`:
- Around line 90-96: Update the poll predicate in the restart test around
_get_restart_condition_status so it only succeeds when the RESTART_IN_PROGRESS
condition equals CONDITION_STATUS_TRUE, or the project’s equivalent enum value,
rather than accepting any non-empty status; preserve the existing polling
parameters and description.
- Around line 134-142: The cleanup block in the BaseException handler must not
be gated by the CR name returned from get_baremetal_instance_name. In the
cleanup logic, always call cli.delete_baremetal_instance and
wait_for_bmi_grpc_removal with bmi_id; invoke wait_for_bmi_deletion only when
bmi_cr is present, and keep each cleanup operation independently guarded so a
failure does not prevent subsequent cleanup.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 71232b7f-d9e8-4e7f-9fd8-447d3f05c8cc
📒 Files selected for processing (2)
tests/bmaas/test_baremetal_instance_lifecycle.pytests/bmaas/test_baremetal_instance_restart.py
| external_host_id: str = k8s_hub_client.get_baremetal_instance_external_host_id(name=bmi_cr_name) | ||
| logger.info("BMI %s assigned to BMH: %s", bmi_id, external_host_id) | ||
| _log_bmh_inventory(bmh_namespace) | ||
|
|
||
| wait_for_bmi_running(grpc=grpc, bmi_id=bmi_id) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Wait for spec.externalHostID before using it.
Line 71 waits only for CR creation. Line 74 performs one direct lookup of spec.externalHostID. The test can fail before wait_for_bmi_running if BMH assignment is still pending.
Poll until the external-host ID is non-empty, then validate its namespace and name.
Suggested fix
- external_host_id: str = k8s_hub_client.get_baremetal_instance_external_host_id(name=bmi_cr_name)
+ external_host_id: str = poll_until(
+ fn=lambda: k8s_hub_client.get_baremetal_instance_external_host_id(name=bmi_cr_name),
+ until=lambda v: v != "",
+ retries=60,
+ delay=5,
+ description=f"{bmi_id} external host assignment",
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| external_host_id: str = k8s_hub_client.get_baremetal_instance_external_host_id(name=bmi_cr_name) | |
| logger.info("BMI %s assigned to BMH: %s", bmi_id, external_host_id) | |
| _log_bmh_inventory(bmh_namespace) | |
| wait_for_bmi_running(grpc=grpc, bmi_id=bmi_id) | |
| external_host_id: str = poll_until( | |
| fn=lambda: k8s_hub_client.get_baremetal_instance_external_host_id(name=bmi_cr_name), | |
| until=lambda v: v != "", | |
| retries=60, | |
| delay=5, | |
| description=f"{bmi_id} external host assignment", | |
| ) | |
| logger.info("BMI %s assigned to BMH: %s", bmi_id, external_host_id) | |
| _log_bmh_inventory(bmh_namespace) | |
| wait_for_bmi_running(grpc=grpc, bmi_id=bmi_id) |
🤖 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/bmaas/test_baremetal_instance_restart.py` around lines 74 - 78, Update
the restart test flow around get_baremetal_instance_external_host_id and
wait_for_bmi_running to poll until spec.externalHostID is non-empty before
proceeding. After the ID becomes available, validate its namespace and name,
then retain the existing inventory logging and running-state wait.
Log BMI gRPC state, conditions, restart_trigger values, BMH provisioning state, online/powered/error fields, and BMI CR phase/conditions on every poll iteration while the BMI is stuck in STARTING after restart. This will pinpoint whether the issue is in the BMH (Ironic/sushy-tools), the BMI CR (operator), or the fulfillment-service controller. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: MENNY ABOUSH <maboush@maboush-thinkpadt14gen5.raanaii.csb>
Log gRPC state and CR PowerSynced condition after the stop/start power cycle to compare with the restart test. If PowerSynced=False appears here too, the bug affects all power operations, not just restart_trigger. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: MENNY ABOUSH <maboush@maboush-thinkpadt14gen5.raanaii.csb>
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 `@tests/bmaas/test_baremetal_instance_lifecycle.py`:
- Around line 110-126: Update the kubectl diagnostic flow in the lifecycle test
to unpack run_unchecked’s result into separate cr_output and cr_exit_code
values, then log cr_output as the CR data. When cr_exit_code is non-zero, emit a
warning containing the command failure context and exit code instead of treating
the result as a valid phase.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 86deed97-27fc-4f05-bb19-9bd721845ba0
📒 Files selected for processing (1)
tests/bmaas/test_baremetal_instance_lifecycle.py
- Extract log_bmh_inventory to conftest.py (was duplicated in both tests) - Restart test: replace inline poll with shared wait_for_bmi_running (adds FAILED-state fail-fast guard, matches lifecycle pattern) - Lifecycle test: add wait_for_bmi_running after power-on to verify whether the PowerSynced bug affects stop/start too (not just restart) - Both tests: consistent use of shared helpers, diagnostic logging on failure via _log_bmi_state Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: MENNY ABOUSH <maboush@maboush-thinkpadt14gen5.raanaii.csb>
…_running The BMI CR's externalHostID is empty until the operator assigns a BMH during provisioning. Reading it before wait_for_bmi_running returns an empty string. Move the read after the BMI reaches RUNNING state. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: MENNY ABOUSH <maboush@maboush-thinkpadt14gen5.raanaii.csb>
|
Superseded by a clean PR — this branch contains investigation diagnostics for OSAC-3548. Keeping the branch for reference. |
This branch had an error being deployed
2 participants
Summary
tests/bmaas/test_baremetal_instance_restart.pyupdate_baremetal_instance_restart_trigger()helper toGRPCClientfor field-maskedspec.restart_triggerupdatesTest Coverage
The test exercises the full restart_trigger flow:
spec.restart_triggerviaBareMetalInstances/Updatewith field maskRESTART_IN_PROGRESScondition to appearstatus.restart_triggerechoes the value set in specstatus.poweredOn=true, BMI back to Running)RESTART_IN_PROGRESScondition clears andRESTART_FAILEDis absentJira
OSAC-3402
Test plan
ruff checkpasses (verified locally)ruff format --checkpasses (verified locally)make test-bmaaspasses in CI (e2e-bmaas-full-install workflow)test_baremetal_instance_lifecycleAssisted-by: Claude Code noreply@anthropic.com
Summary by CodeRabbit