Skip to content

AGENT-1352: revert ABI host unbinding when reset cluster#3280

Closed
ElayAharoni wants to merge 1 commit intoopenshift-assisted:masterfrom
ElayAharoni:revert-ABI--unbinding-on-reset-flow
Closed

AGENT-1352: revert ABI host unbinding when reset cluster#3280
ElayAharoni wants to merge 1 commit intoopenshift-assisted:masterfrom
ElayAharoni:revert-ABI--unbinding-on-reset-flow

Conversation

@ElayAharoni
Copy link
Copy Markdown
Contributor

@ElayAharoni ElayAharoni commented Nov 26, 2025

https://issues.redhat.com/browse/AGENT-1352

revert some of the logic regarding host unbinding

Summary by CodeRabbit

  • Refactor
    • Streamlined cluster removal workflow for improved efficiency

✏️ Tip: You can customize this high-level summary in your review settings.

Signed-off-by: Elay Aharoni <elayaha@gmail.com>
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Nov 26, 2025
@openshift-ci-robot
Copy link
Copy Markdown

openshift-ci-robot commented Nov 26, 2025

@ElayAharoni: This pull request references AGENT-1352 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 sub-task to target the "4.21.0" version, but no target version was set.

Details

In response to this:

https://issues.redhat.com/browse/AGENT-1352

revert some of the logic regarding host unbinding

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.

@openshift-ci openshift-ci bot requested review from ammont82 and celdrake November 26, 2025 14:37
@openshift-ci
Copy link
Copy Markdown

openshift-ci bot commented Nov 26, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ElayAharoni
Once this PR has been reviewed and has the lgtm label, please assign rawagner for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Nov 26, 2025

Walkthrough

The cluster reset flow in ResetSingleClusterModal is simplified by removing the host unbinding loop. The handleResetAsync function now directly calls ClustersService.remove() without iterating over cluster hosts to unbind them individually.

Changes

Cohort / File(s) Change Summary
Cluster Reset Flow Simplification
apps/assisted-disconnected-ui/src/components/ResetSingleClusterModal.tsx
Removed per-host unbinding loop from handleResetAsync. Cluster removal now directly calls ClustersService.remove(cluster.id) instead of iterating over cluster hosts to unbind each via HostsService.unbind. Simplifies error handling.

Sequence Diagram

sequenceDiagram
    participant User
    participant Modal as ResetSingleClusterModal
    participant Cluster as ClustersService
    participant Nav as Navigation

    rect rgb(240, 248, 255)
    Note over User,Nav: Previous Flow (with host unbinding)
    User->>Modal: Initiate reset
    Modal->>Modal: Get cluster hosts (loop)
    loop For each host
        Modal->>Cluster: unbind(host)
    end
    Modal->>Cluster: remove(cluster.id)
    Modal->>Nav: Navigate home
    end

    rect rgb(255, 250, 240)
    Note over User,Nav: New Flow (simplified)
    User->>Modal: Initiate reset
    Modal->>Cluster: remove(cluster.id)
    Modal->>Nav: Navigate home
    end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Verify that ClustersService.remove() handles host cleanup internally or that removal without explicit unbinding is intentional
  • Confirm no downstream dependencies rely on per-host unbinding side effects

Possibly related PRs

Suggested labels

lgtm, approved, size/M

Suggested reviewers

  • rawagner

Poem

🐰 A loop unwound, the path grows clear,
No host goodbyes, just cheer, cheer, cheer!
The cluster resets swift and clean,
Simplest flow we've ever seen! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'AGENT-1352: revert ABI host unbinding when reset cluster' is directly related to the main change—reverting the host unbinding logic during cluster reset—which matches the code change that removes the unbinding flow.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 41a3210 and 7883053.

📒 Files selected for processing (1)
  • apps/assisted-disconnected-ui/src/components/ResetSingleClusterModal.tsx (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T04:40:36.292Z
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx:55-63
Timestamp: 2025-10-21T04:40:36.292Z
Learning: In libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx, the network reordering logic in the useEffect (swapping clusterNetworks and serviceNetworks based on the primary machine network's IP family) is for UI consistency only. Validation of empty or invalid CIDRs is handled separately by validation schemas, not by the reordering logic.

Applied to files:

  • apps/assisted-disconnected-ui/src/components/ResetSingleClusterModal.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: tests
  • GitHub Check: unit-tests
  • GitHub Check: translation-files
  • GitHub Check: circular-deps
  • GitHub Check: lint
  • GitHub Check: format
🔇 Additional comments (2)
apps/assisted-disconnected-ui/src/components/ResetSingleClusterModal.tsx (2)

4-4: Import cleanup aligns with reverting host-unbinding from the UI

Dropping host-related services from this modal and keeping only useModalDialogsContext and ClustersService matches the intent to move host unbinding out of the UI and back to the backend/cluster lifecycle. No issues here.


30-47: Reset flow now relies solely on ClustersService.remove — verify backend semantics

handleResetAsync now just calls ClustersService.remove(cluster.id) and navigates home, which cleanly reverts the earlier per-host unbind loop and simplifies error handling. This looks good as long as the backend’s remove operation fully handles host unbinding/cleanup for the “reset cluster” scenario targeted by AGENT-1352.

Please double-check with backend/domain owners that:

  • ClustersService.remove is the correct operation for a “reset” in this flow (vs. a full delete with different semantics), and
  • It leaves no dangling/unbound hosts contrary to the JIRA’s intent.

If the semantics are confirmed, no further changes are needed.

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions:
    | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context.
    Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci openshift-ci bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 26, 2025
@openshift-ci-robot
Copy link
Copy Markdown

openshift-ci-robot commented Nov 26, 2025

@ElayAharoni: This pull request references AGENT-1352 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 sub-task to target the "4.21.0" version, but no target version was set.

Details

In response to this:

https://issues.redhat.com/browse/AGENT-1352

revert some of the logic regarding host unbinding

Summary by CodeRabbit

  • Refactor
  • Streamlined cluster removal workflow for improved efficiency

✏️ Tip: You can customize this high-level summary in your review settings.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants