Skip to content

Conversation

@arturshadnik
Copy link
Member

@arturshadnik arturshadnik commented Oct 29, 2025

Docs follow-up to #76, updating the sequence diagram to show taint steps, as well as moving the final agent cleanup to the very end.

Summary by CodeRabbit

  • Documentation
    • Updated cluster cleanup and reconciliation process documentation with revised hub and spoke cleanup phases
    • Enhanced force cluster drain handling with improved taint management
    • Refined manifest work cleanup sequencing across hub and spoke components

@coderabbitai
Copy link

coderabbitai bot commented Oct 29, 2025

Walkthrough

Documentation update to the 2-phase spoke reconciliation flow describing modifications to Hub and Spoke cleanup phases: adding ForceClusterDrain-triggered workload-cleanup taint, terminating taint application, and reordering AppliedManifestWork (FCC-agent) removal to occur after spoke resource deletion.

Changes

Cohort / File(s) Summary
Documentation Update
fleetconfig-controller/docs/2-phase-spoke-reconcile.md
Updated reconciliation flow diagram and description to include conditional workload-cleanup taint emission based on ForceClusterDrain flag, separate terminating taint application, removal of spoke-side AppliedManifestWork deletion step, and addition of hub-instructed cleanup after spoke resource deletion.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

  • Single documentation file with procedural/conceptual updates to existing flow diagrams
  • No code changes or logic alterations requiring validation
  • Verify alignment between documented sequence and related implementation (see possibly related PRs)

Possibly related PRs

Suggested reviewers

  • TylerGillson
  • ahmad-ibra

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "docs: update 2phase reconcile deletion diagram" directly and accurately describes the main change in this pull request. The PR modifies the documentation file "2-phase-spoke-reconcile.md" to update the sequence diagram with additional taint steps and reordered cleanup procedures, which aligns with the stated objective of updating the diagram. The title uses proper convention with a "docs:" prefix, is concise and specific enough for a teammate scanning history to understand this is a documentation update to a specific diagram, and avoids vague language or unnecessary detail. The implementation specifics of what changed in the diagram (taint steps, cleanup reordering) are appropriately omitted as they represent the "how" rather than the primary "what."
✨ Finishing touches
🧪 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 5a6ec6b and 3d1b281.

📒 Files selected for processing (1)
  • fleetconfig-controller/docs/2-phase-spoke-reconcile.md (2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#69
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:125-133
Timestamp: 2025-09-25T23:18:41.573Z
Learning: In the fleetconfig-controller spoke deletion flow, SpokeCleanupFinalizer is always removed before HubCleanupFinalizer. This means that checking for the existence of HubCleanupFinalizer in the deletion logic is sufficient regardless of cluster type, as any SpokeCleanupFinalizer would have already been removed by the time the hub cleanup runs.
📚 Learning: 2025-09-25T23:18:41.573Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#69
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:125-133
Timestamp: 2025-09-25T23:18:41.573Z
Learning: In the fleetconfig-controller spoke deletion flow, SpokeCleanupFinalizer is always removed before HubCleanupFinalizer. This means that checking for the existence of HubCleanupFinalizer in the deletion logic is sufficient regardless of cluster type, as any SpokeCleanupFinalizer would have already been removed by the time the hub cleanup runs.

Applied to files:

  • fleetconfig-controller/docs/2-phase-spoke-reconcile.md
⏰ 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). (4)
  • GitHub Check: e2e (fleetconfig-controller) / e2e
  • GitHub Check: test (fleetconfig-controller) / Ensure PR is reviewable & run tests
  • GitHub Check: e2e (fleetconfig-controller) / e2e
  • GitHub Check: test (fleetconfig-controller) / Ensure PR is reviewable & run tests
🔇 Additional comments (2)
fleetconfig-controller/docs/2-phase-spoke-reconcile.md (2)

166-173: Taint steps are clearly documented and well-positioned in the sequence.

The addition of the workload-cleanup taint (conditional on ForceClusterDrain) and the terminating taint are clear and logically placed within the Hub Pre-Flight Cleanup Phase. The ordering—check ManifestWorks, set terminating taint, then disable addons—is sensible and well-documented.


194-195: Clarify synchronization for final AppliedManifestWork removal.

The diagram shows the SpokeController removing AppliedManifestWork after the Hub cleanup completes and the Spoke resource is deleted, but it's unclear what triggers this action. Consider adding a synchronization mechanism or explanatory note to clarify:

  • Does the SpokeController wait for the HubCleanupFinalizer to be removed?
  • Is there a status condition or other signal that indicates when to proceed?
  • Should there be an explicit check/wait step similar to lines 178-181?

Adding this clarity would help readers understand the coordination between Hub and Spoke cleanup phases.


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
Copy link

openshift-ci bot commented Oct 29, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arturshadnik, TylerGillson

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

The pull request process is described here

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

@openshift-merge-bot openshift-merge-bot bot merged commit b5d7997 into open-cluster-management-io:main Oct 29, 2025
25 of 31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants