Add finalizer-guarded deletion for VCFM to prevent interrupted migrat… - #84
Add finalizer-guarded deletion for VCFM to prevent interrupted migrat…#84jcpowermac wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe migration API adds finalizer and force-delete constants. The controller manages the finalizer, blocks deletion during active migrations, reports blocked status, emits a warning event, and permits forced or completed deletion. Tests cover each deletion state. ChangesMigration deletion protection
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Reconciler
participant KubernetesAPI
participant EventRecorder
Reconciler->>KubernetesAPI: Check and add finalizer
Reconciler->>KubernetesAPI: Check migration Ready status
alt Migration not Ready
Reconciler->>KubernetesAPI: Update Ready=False condition
Reconciler->>EventRecorder: Emit deletion warning
Reconciler->>Reconciler: Requeue for periodic check
else Migration Ready or force-deleted
Reconciler->>KubernetesAPI: Remove finalizer
end
Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jcpowermac 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 |
openshift-ci
Bot
added
the
approved
![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
🧹 Nitpick comments (5)
internal/controller/vmwarecloudfoundationmigration_controller_test.go (3)
330-359: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGive this cleanup
Eventuallyan explicit timeout.It relies on Gomega's 1s default while performing several sequential envtest API round-trips per attempt (Get, Update, Delete, Get,
handleFinalizer→ Update). That is a realistic flake source for suite cleanup.As per coding guidelines: "timeouts on cluster operations (Eventually/Consistently calls)".
♻️ Suggested change
- }).Should(BeTrue()) + }, "30s", "200ms").Should(BeTrue())🤖 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 `@internal/controller/vmwarecloudfoundationmigration_controller_test.go` around lines 330 - 359, Update the cleanup Eventually block around handleFinalizer to pass an explicit timeout appropriate for the sequential envtest API operations, rather than relying on Gomega’s default timeout; preserve the existing polling behavior and success condition.Source: Coding guidelines
372-385: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSetup contradicts the test name and makes the assertion incidental.
newResource()setsState: Running, so "never started" only holds becausehandleFinalizerreturns early after adding the finalizer and never reaches theStartTimeassignment. Create the resource withMigrationStatePending(or explicitly assertStatus.StartTimeis nil before deleting) so the test pins the intended precondition rather than an implementation detail.🤖 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 `@internal/controller/vmwarecloudfoundationmigration_controller_test.go` around lines 372 - 385, Update the “deletes immediately when the migration never started” test setup to create the resource with MigrationStatePending instead of relying on newResource()’s Running state and finalizer behavior. Keep the deletion and not-found assertions unchanged so the test explicitly covers a migration that has not started.
387-447: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider splitting this
Itinto per-behavior specs.It currently asserts blocking, the warning event, event idempotence on a second pass, and force-delete unblocking. A failure in the first half hides the force-delete coverage entirely. Three
Itblocks over a sharedBeforeEach(create → start → delete) would isolate the failures.As per coding guidelines: "Review Ginkgo test code for single responsibility (each It block tests one specific behavior)".
🤖 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 `@internal/controller/vmwarecloudfoundationmigration_controller_test.go` around lines 387 - 447, Split the single test around handleFinalizer into separate It specs for deletion blocking, warning-event idempotence on a second pass, and force-delete unblocking. Move the shared create/start/delete setup into BeforeEach, while retaining each behavior’s existing assertions and using the appropriate reconciler and recorder setup within each isolated spec.Source: Coding guidelines
internal/controller/vmwarecloudfoundationmigration_controller.go (2)
126-129: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
erris silently dropped whendoneis false.Today every error path in
handleFinalizerreturnsdone=true, so nothing is lost — but the call site makes that invariant implicit and easy to break later. Consider checkingerrindependently.♻️ Suggested hardening
- if result, done, err := r.handleFinalizer(ctx, migration); done { - return result, err - } + result, done, err := r.handleFinalizer(ctx, migration) + if err != nil || done { + return result, err + }🤖 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 `@internal/controller/vmwarecloudfoundationmigration_controller.go` around lines 126 - 129, Update the handleFinalizer call in the reconciliation flow to check err independently of done, returning the error whenever it is non-nil even if done is false; preserve the existing result return for completed finalizer handling.
213-213: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStrict
"true"match on the force-delete annotation.
"True"/"1"/" true"are silently ignored, which is a confusing UX for an escape hatch used under pressure.strconv.ParseBool(strings.TrimSpace(...))is more forgiving.🤖 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 `@internal/controller/vmwarecloudfoundationmigration_controller.go` at line 213, Update the force-delete detection near the forced variable in the migration reconciliation flow to trim whitespace and parse the annotation with strconv.ParseBool, accepting standard boolean representations such as “true”, “True”, and “1”. Preserve false behavior for missing, invalid, or explicitly false annotation values.
🤖 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 `@internal/controller/vmwarecloudfoundationmigration_controller.go`:
- Around line 222-237: Ensure the blocked-deletion path requeues reconciliation
while returning done=false, rather than returning an empty Result that is
discarded. Update the call site that invokes this path to propagate its returned
Result whenever reconciliation short-circuits, including the
Spec.State-not-Running branch, so paused or unchanged migrations are
re-evaluated. Preserve the existing status and event behavior guarded by
alreadyRecorded.
---
Nitpick comments:
In `@internal/controller/vmwarecloudfoundationmigration_controller_test.go`:
- Around line 330-359: Update the cleanup Eventually block around
handleFinalizer to pass an explicit timeout appropriate for the sequential
envtest API operations, rather than relying on Gomega’s default timeout;
preserve the existing polling behavior and success condition.
- Around line 372-385: Update the “deletes immediately when the migration never
started” test setup to create the resource with MigrationStatePending instead of
relying on newResource()’s Running state and finalizer behavior. Keep the
deletion and not-found assertions unchanged so the test explicitly covers a
migration that has not started.
- Around line 387-447: Split the single test around handleFinalizer into
separate It specs for deletion blocking, warning-event idempotence on a second
pass, and force-delete unblocking. Move the shared create/start/delete setup
into BeforeEach, while retaining each behavior’s existing assertions and using
the appropriate reconciler and recorder setup within each isolated spec.
In `@internal/controller/vmwarecloudfoundationmigration_controller.go`:
- Around line 126-129: Update the handleFinalizer call in the reconciliation
flow to check err independently of done, returning the error whenever it is
non-nil even if done is false; preserve the existing result return for completed
finalizer handling.
- Line 213: Update the force-delete detection near the forced variable in the
migration reconciliation flow to trim whitespace and parse the annotation with
strconv.ParseBool, accepting standard boolean representations such as “true”,
“True”, and “1”. Preserve false behavior for missing, invalid, or explicitly
false annotation values.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9bd0accb-ff77-4f09-bd0d-bb0f87674f35
📒 Files selected for processing (3)
api/v1alpha1/vmwarecloudfoundationmigration_types.gointernal/controller/vmwarecloudfoundationmigration_controller.gointernal/controller/vmwarecloudfoundationmigration_controller_test.go
…ions Deleting a VCFM CR mid-migration left the cluster split across vCenters with CPMS partially updated and no way to resume or track the migration. This adds a protection finalizer that blocks deletion while a migration is in progress (StartTime set, Ready not True), with an annotation-based force-delete override for deliberate abandonment. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
3bdbc72 to
8fc17e9
Compare
|
@jcpowermac: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
…ions
Deleting a VCFM CR mid-migration left the cluster split across vCenters with CPMS partially updated and no way to resume or track the migration. This adds a protection finalizer that blocks deletion while a migration is in progress (StartTime set, Ready not True), with an annotation-based force-delete override for deliberate abandonment.
Summary by CodeRabbit