OSAC-1586: add E2E test for cluster deletion feedback - #227
openshift-merge-bot[bot] merged 5 commits into
Conversation
|
@vladikr: This pull request references OSAC-1586 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 bug 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. |
WalkthroughThe PR adds ClusterOrder and gRPC lifecycle polling helpers, introduces lightweight deletion-feedback coverage, and updates cluster creation tests to verify deletion states and perform best-effort cleanup. ChangesCluster deletion feedback
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Test
participant OsacCLI
participant Kubernetes
participant GRPCClient
Test->>OsacCLI: create cluster
OsacCLI->>Kubernetes: create ClusterOrder
Test->>Kubernetes: wait for Progressing
Test->>GRPCClient: verify cluster ID
Test->>OsacCLI: delete cluster
Test->>Kubernetes: wait for Deleting and removal
Test->>GRPCClient: verify deleting or archived state
Test->>GRPCClient: wait for cluster removal
Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 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 |
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/caas/test_cluster_delete_feedback.py`:
- Around line 30-47: Guarantee ClusterOrder cleanup by adding and using a
lifecycle context manager around the cluster operations in
tests/caas/test_cluster_delete_feedback.py lines 30-47 and
tests/caas/test_cluster_delete_feedback_light.py lines 30-47. In the manager’s
finally path, check k8s.is_present before attempting cli.delete_cluster,
tolerate deletion errors, and wait for ClusterOrder deletion; wrap the existing
readiness/progress and deletion feedback flows so cleanup runs when any wait or
assertion fails.
🪄 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: Enterprise
Run ID: 18940eac-381f-4cc1-bd9e-34904bdbf078
📒 Files selected for processing (3)
tests/caas/test_cluster_delete_feedback.pytests/caas/test_cluster_delete_feedback_light.pytests/core/helpers.py
|
/hold |
c8656bb to
e3a01b4
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/caas/test_cluster_delete_feedback.py`:
- Around line 30-37: Move ClusterOrder discovery into the cleanup scope
beginning immediately after cli.create_cluster in
tests/caas/test_cluster_delete_feedback.py#L30-L37, ensuring UUID-based deletion
runs even when wait_for_cluster_order_cr fails; apply the same lifecycle cleanup
adjustment in tests/caas/test_cluster_delete_feedback_light.py#L30-L37. Use the
existing cleanup mechanism or a context manager/yield fixture, preserving normal
cleanup after the ClusterOrder name is obtained.
- Around line 52-55: In the cleanup blocks surrounding cli.delete_cluster in
tests/caas/test_cluster_delete_feedback.py lines 52-55 and
tests/caas/test_cluster_delete_feedback_light.py lines 52-55, replace broad
Exception suppression with subprocess.CalledProcessError while preserving the
existing cleanup behavior.
In `@tests/core/helpers.py`:
- Around line 404-430: The _get_state function in wait_for_cluster_grpc_state
should only allow transient grpc.get_cluster call failures to be retried, while
malformed or missing response fields must surface immediately. Remove the broad
exception around response parsing and isolate retryable command/service errors
using the existing poll_until retry behavior with retry_on_error=True.
🪄 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: e95e9d89-78d6-43ec-a855-c4f9c08d7f7d
📒 Files selected for processing (3)
tests/caas/test_cluster_delete_feedback.pytests/caas/test_cluster_delete_feedback_light.pytests/core/helpers.py
|
/lgtm |
|
@vladikr Can you re-base on main so you can pickup the E2E CaaS job? |
oh, of course |
cb8e999 to
04260be
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/core/helpers.py (1)
414-420:⚠️ Potential issue | 🟡 MinorDo not turn malformed gRPC responses into a timeout.
_get_state()treats missingobject.status.statethe same as a transient command failure by returning"", so API/schema regressions are hidden until the poll times out. Let required fields raise, and letpoll_until(..., retry_on_error=True)handle onlyCalledProcessErrorretries while preserving the last error.🤖 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/core/helpers.py` around lines 414 - 420, Update _get_state in wait_for_cluster_grpc_state to let missing response fields, including object, status, or state, raise instead of defaulting to an empty string; retain the CalledProcessError handling so poll_until(..., retry_on_error=True) retries only transient command failures and preserves the final error.
🤖 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/caas/test_cluster_delete_feedback.py`:
- Around line 49-53: Complete teardown in the finally blocks of
tests/caas/test_cluster_delete_feedback.py lines 49-53 and
tests/caas/test_cluster_delete_feedback_light.py lines 49-53: after
cli.delete_cluster(uuid=uuid), check k8s_hub_client.is_present(...) and wait for
wait_for_cluster_deletion(...), while preserving handling of CalledProcessError.
Ensure both tests perform the asynchronous deletion wait even when the main flow
fails.
- Around line 50-53: The duplicated try/except suppression blocks should use
contextlib.suppress for subprocess.CalledProcessError. Update
tests/caas/test_cluster_delete_feedback.py lines 50-53 and
tests/caas/test_cluster_delete_feedback_light.py lines 50-53 identically, adding
the required contextlib import and wrapping each cli.delete_cluster call with
the suppression context manager.
---
Duplicate comments:
In `@tests/core/helpers.py`:
- Around line 414-420: Update _get_state in wait_for_cluster_grpc_state to let
missing response fields, including object, status, or state, raise instead of
defaulting to an empty string; retain the CalledProcessError handling so
poll_until(..., retry_on_error=True) retries only transient command failures and
preserves the final error.
🪄 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: 6a1a9b47-33b0-4e72-b015-76573da5ed3e
📒 Files selected for processing (3)
tests/caas/test_cluster_delete_feedback.pytests/caas/test_cluster_delete_feedback_light.pytests/core/helpers.py
| finally: | ||
| try: | ||
| cli.delete_cluster(uuid=uuid) | ||
| except subprocess.CalledProcessError: | ||
| pass |
There was a problem hiding this comment.
Complete teardown instead of issuing only a best-effort delete.
When the main flow fails, both tests skip the normal deletion waits, so asynchronous ClusterOrder cleanup may remain incomplete.
tests/caas/test_cluster_delete_feedback.py#L49-L53: checkk8s_hub_client.is_present(...)after deletion and wait forwait_for_cluster_deletion(...).tests/caas/test_cluster_delete_feedback_light.py#L49-L53: apply the same delete-and-wait cleanup.
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 50-53: Use contextlib.suppress(subprocess.CalledProcessError) instead of try-except-pass
Replace try-except-pass with with contextlib.suppress(subprocess.CalledProcessError): ...
(SIM105)
📍 Affects 2 files
tests/caas/test_cluster_delete_feedback.py#L49-L53(this comment)tests/caas/test_cluster_delete_feedback_light.py#L49-L53
🤖 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/caas/test_cluster_delete_feedback.py` around lines 49 - 53, Complete
teardown in the finally blocks of tests/caas/test_cluster_delete_feedback.py
lines 49-53 and tests/caas/test_cluster_delete_feedback_light.py lines 49-53:
after cli.delete_cluster(uuid=uuid), check k8s_hub_client.is_present(...) and
wait for wait_for_cluster_deletion(...), while preserving handling of
CalledProcessError. Ensure both tests perform the asynchronous deletion wait
even when the main flow fails.
Source: Learnings
| try: | ||
| cli.delete_cluster(uuid=uuid) | ||
| except subprocess.CalledProcessError: | ||
| pass |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Replace the duplicated suppression blocks with contextlib.suppress.
Ruff flags SIM105 in both files.
Proposed refactor
+import contextlib
- try:
+ with contextlib.suppress(subprocess.CalledProcessError):
cli.delete_cluster(uuid=uuid)
- except subprocess.CalledProcessError:
- passtests/caas/test_cluster_delete_feedback.py#L50-L53: apply the suppression context manager.tests/caas/test_cluster_delete_feedback_light.py#L50-L53: apply the same refactor.
📝 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.
| try: | |
| cli.delete_cluster(uuid=uuid) | |
| except subprocess.CalledProcessError: | |
| pass | |
| with contextlib.suppress(subprocess.CalledProcessError): | |
| cli.delete_cluster(uuid=uuid) |
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 50-53: Use contextlib.suppress(subprocess.CalledProcessError) instead of try-except-pass
Replace try-except-pass with with contextlib.suppress(subprocess.CalledProcessError): ...
(SIM105)
📍 Affects 2 files
tests/caas/test_cluster_delete_feedback.py#L50-L53(this comment)tests/caas/test_cluster_delete_feedback_light.py#L50-L53
🤖 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/caas/test_cluster_delete_feedback.py` around lines 50 - 53, The
duplicated try/except suppression blocks should use contextlib.suppress for
subprocess.CalledProcessError. Update tests/caas/test_cluster_delete_feedback.py
lines 50-53 and tests/caas/test_cluster_delete_feedback_light.py lines 50-53
identically, adding the required contextlib import and wrapping each
cli.delete_cluster call with the suppression context manager.
Source: Linters/SAST tools
| from tests.core.osac_cli import OsacCLI | ||
|
|
||
|
|
||
| def test_cluster_delete_reports_deleting_state( |
There was a problem hiding this comment.
It seems like this test is more or less the same as the existing test_cluster_create test with a few extra steps - https://github.com/osac-project/osac-test-infra/blob/main/tests/caas/test_cluster_create.py. Can these two tests be consolidated?
There was a problem hiding this comment.
yes, let me see how...
| cli.delete_cluster(uuid=uuid) | ||
|
|
||
| wait_for_cluster_deleting(k8s=k8s_hub_client, name=co_name) | ||
| wait_for_cluster_grpc_state(grpc=grpc, uuid=uuid, state="CLUSTER_STATE_DELETING") |
There was a problem hiding this comment.
Tracing through osac-operator#358's Reconcile for a deletion:
- handleDelete → syncPhaseDeleting sets CLUSTER_STATE_DELETING on the local cluster copy
- saveCluster → gRPC Update — state is now visible in the API ✓
- Immediately after: removes the feedback finalizer, then calls Signal → fulfillment-service archives the cluster
The CLUSTER_STATE_DELETING state is only visible in the gRPC API for the time between step 2 and the Signal response — one hubClient.Update round-trip, likely sub-second. wait_for_cluster_grpc_state polls every 5s, so it will almost always miss the window and then see get_cluster returning not-found (already archived), which the current error-swallowing returns as "" for all 150s.
There was a problem hiding this comment.
yes, that light test is strange... I think that archiving takes time as well, it's not 0
There was a problem hiding this comment.
ok, we'll just wait for the CR's status, and the grpc api will either be able to see the DELETING or was already archived
Verify that deleting a cluster transitions through DELETING state in both the K8s CR phase and the fulfillment-service gRPC API before being archived. Add wait_for_cluster_deleting and wait_for_cluster_grpc_state helpers to support polling for intermediate deletion states. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>
Add a kind-compatible variant of the cluster deletion feedback test that skips full provisioning (no HyperShift needed). Creates a cluster, waits for Progressing phase, deletes it, and verifies the DELETING state appears in both the K8s CR and the gRPC API. Add wait_for_cluster_progressing helper for polling the Progressing phase with short timeouts. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>
Wrap test bodies in try/finally to ensure ClusterOrder cleanup runs even if assertions or wait conditions fail mid-test. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>
- Move try/finally to cover cluster order CR discovery so cleanup runs even if wait_for_cluster_order_cr fails - Narrow exception handling to subprocess.CalledProcessError in cleanup and wait_for_cluster_grpc_state helper - Simplify cleanup to UUID-based delete (no CR name needed) Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>
Address review feedback: - Consolidate full deletion test into existing test_cluster_create.py instead of a separate test file (trewest) - Handle the DELETING→archived race: the CLUSTER_STATE_DELETING window can be sub-second when the feedback finalizer is the last one, so wait_for_cluster_grpc_deleting_or_archived accepts either state (trewest) - Use contextlib.suppress instead of try/except/pass (CodeRabbit) Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>
04260be to
2032230
Compare
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/caas/test_cluster_create.py`:
- Around line 47-49: Update the cleanup blocks in
tests/caas/test_cluster_create.py lines 47-49 and
tests/caas/test_cluster_delete_feedback_light.py lines 50-52 to use the shared
lifecycle cleanup context or fixture, conditionally delete while the
ClusterOrder exists, and wait for deletion to complete. Preserve cleanup on
failed assertions and ensure the wait_for_cluster_deletion force-cleanup path
can run.
In `@tests/core/helpers.py`:
- Around line 422-436: Update the _done helper to treat only an explicitly
confirmed not-found or archived response from grpc.get_cluster as success; do
not convert every subprocess.CalledProcessError into True. Preserve the
CLUSTER_STATE_DELETING check, while allowing transient transport,
authentication, and invocation failures to propagate or be retried by
poll_until.
🪄 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: bdd0d215-387f-447b-8594-fc558373e710
📒 Files selected for processing (3)
tests/caas/test_cluster_create.pytests/caas/test_cluster_delete_feedback_light.pytests/core/helpers.py
| finally: | ||
| with contextlib.suppress(subprocess.CalledProcessError): | ||
| cli.delete_cluster(uuid=uuid) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Make fallback cleanup wait for deletion.
A failed readiness/deletion wait enters finally, but the fallback only starts deletion and lets the test exit before the ClusterOrder is gone. That can leak resources and skip wait_for_cluster_deletion’s force-cleanup path.
tests/caas/test_cluster_create.py#L47-L49: use the shared lifecycle cleanup context/fixture; conditionally delete and wait while the ClusterOrder exists.tests/caas/test_cluster_delete_feedback_light.py#L50-L52: use the same cleanup pattern so failed assertions still complete teardown.
📍 Affects 2 files
tests/caas/test_cluster_create.py#L47-L49(this comment)tests/caas/test_cluster_delete_feedback_light.py#L50-L52
🤖 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/caas/test_cluster_create.py` around lines 47 - 49, Update the cleanup
blocks in tests/caas/test_cluster_create.py lines 47-49 and
tests/caas/test_cluster_delete_feedback_light.py lines 50-52 to use the shared
lifecycle cleanup context or fixture, conditionally delete while the
ClusterOrder exists, and wait for deletion to complete. Preserve cleanup on
failed assertions and ensure the wait_for_cluster_deletion force-cleanup path
can run.
Source: Learnings
| def _done() -> bool: | ||
| try: | ||
| cluster = grpc.get_cluster(cluster_id=uuid) | ||
| state = cluster.get("object", {}).get("status", {}).get("state", "") | ||
| return state == "CLUSTER_STATE_DELETING" | ||
| except subprocess.CalledProcessError: | ||
| return True | ||
|
|
||
| poll_until( | ||
| fn=_done, | ||
| until=lambda v: v is True, | ||
| retries=30, | ||
| delay=2, | ||
| description=f"{uuid} gRPC DELETING or already archived", | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not treat every gRPC command failure as archival.
CalledProcessError also covers transient transport, authentication, and invocation failures, so this helper can report success without observing either CLUSTER_STATE_DELETING or a confirmed not-found/archive response. Distinguish the expected not-found response from other failures; retry or surface the latter.
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 426-426: Consider moving this statement to an else block
(TRY300)
🤖 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/core/helpers.py` around lines 422 - 436, Update the _done helper to
treat only an explicitly confirmed not-found or archived response from
grpc.get_cluster as success; do not convert every subprocess.CalledProcessError
into True. Preserve the CLUSTER_STATE_DELETING check, while allowing transient
transport, authentication, and invocation failures to propagate or be retried by
poll_until.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: trewest, vladikr The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
57994c4
into
osac-project:main
5 participants
Summary
test_cluster_delete_reports_deleting_stateE2E test verifying that cluster deletion transitions throughDELETINGstate in both the K8s CR and the fulfillment-service gRPC API before archivalwait_for_cluster_deletingandwait_for_cluster_grpc_statehelpers for polling intermediate deletion statesTest plan
Depends on:
CLUSTER_STATE_DELETINGto proto)Assisted-by: Claude Code noreply@anthropic.com
Summary by CodeRabbit
Bug Fixes
Tests