OCPBUGS-98492: Fix CAPI MachineSet deletion when MAPI MachineSet has already been deleted - #626
OCPBUGS-98492: Fix CAPI MachineSet deletion when MAPI MachineSet has already been deleted#626Kaustubh-pande wants to merge 2 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@Kaustubh-pande: This pull request references Jira Issue OCPBUGS-98492, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
openshift-ci-robot
added
the
jira/invalid-bug
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
WalkthroughThe MachineSet sync reconciler now handles CAPI MachineSet deletion when the MAPI MachineSet is absent, including error and requeue handling. A regression test verifies that the sync finalizer is removed and the CAPI resource is deleted. ChangesMachineSet deletion reconciliation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
/jira refresh |
|
@Kaustubh-pande: This pull request references Jira Issue OCPBUGS-98492, which is invalid:
Comment 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. |
![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 `@pkg/controllers/machinesetsync/machineset_sync_controller.go`:
- Around line 175-179: Remove the redundant capiMachineSet nil-check and its
return/logging block from the reconciliation logic, since the preceding
mapiMachineSet and capiMachineSet nil-check already handles that case. Preserve
the remaining machine-set synchronization flow unchanged.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 5fb49de9-608a-41c6-8cd6-ee74a5d1067d
📒 Files selected for processing (1)
pkg/controllers/machinesetsync/machineset_sync_controller.go
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 `@pkg/controllers/machinesetsync/machineset_sync_controller_test.go`:
- Around line 1177-1194: Add the appropriate timeout and a meaningful failure
message to the `Eventually(k.Get(capiMachineSet), timeout)` assertion in the
“should remove the sync finalizer and allow the CAPI machine set to be deleted”
test, and ensure the nearby `Eventually` calls in this scenario follow the same
timeout-and-message convention without changing their behavior.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f4868842-9698-4b8a-b832-9a19dc7ec013
📒 Files selected for processing (2)
pkg/controllers/machinesetsync/machineset_sync_controller.gopkg/controllers/machinesetsync/machineset_sync_controller_test.go
💤 Files with no reviewable changes (1)
- pkg/controllers/machinesetsync/machineset_sync_controller.go
| Eventually(k.Update(capiMachineSet, func() { | ||
| capiMachineSet.SetFinalizers([]string{machinesync.SyncFinalizer}) | ||
| })).Should(Succeed()) | ||
|
|
||
| By("Waiting for the manager's informer cache to observe the finalizer") | ||
| eventuallyManagerInformerCache(capiMachineSet).Should( | ||
| HaveField("ObjectMeta.Finalizers", ContainElement(machinesync.SyncFinalizer)), | ||
| ) | ||
|
|
||
| By("Deleting the CAPI machine set") | ||
| Eventually(kDelete(ctx, capiMachineSet)).Should(Succeed()) | ||
| }) | ||
|
|
||
| It("should remove the sync finalizer and allow the CAPI machine set to be deleted", func() { | ||
| Eventually(k.Get(capiMachineSet), timeout).Should( | ||
| WithTransform(apierrors.IsNotFound, BeTrue()), | ||
| ) | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add timeouts and failure messages to Ginkgo assertions.
As per coding guidelines, operations interacting with the cluster must include appropriate timeouts for Eventually, and assertions must include meaningful failure messages.
🛠️ Proposed fix
- Eventually(k.Update(capiMachineSet, func() {
- capiMachineSet.SetFinalizers([]string{machinesync.SyncFinalizer})
- })).Should(Succeed())
+ Eventually(k.Update(capiMachineSet, func() {
+ capiMachineSet.SetFinalizers([]string{machinesync.SyncFinalizer})
+ }), timeout).Should(Succeed(), "should be able to add the sync finalizer to the CAPI machine set")
By("Waiting for the manager's informer cache to observe the finalizer")
- eventuallyManagerInformerCache(capiMachineSet).Should(
- HaveField("ObjectMeta.Finalizers", ContainElement(machinesync.SyncFinalizer)),
- )
+ eventuallyManagerInformerCache(capiMachineSet).Should(
+ HaveField("ObjectMeta.Finalizers", ContainElement(machinesync.SyncFinalizer)),
+ "manager cache should observe the sync finalizer on the CAPI machine set",
+ )
By("Deleting the CAPI machine set")
- Eventually(kDelete(ctx, capiMachineSet)).Should(Succeed())
+ Eventually(kDelete(ctx, capiMachineSet), timeout).Should(Succeed(), "should be able to delete the CAPI machine set")
})
It("should remove the sync finalizer and allow the CAPI machine set to be deleted", func() {
- Eventually(k.Get(capiMachineSet), timeout).Should(
- WithTransform(apierrors.IsNotFound, BeTrue()),
- )
+ Eventually(k.Get(capiMachineSet), timeout).Should(
+ WithTransform(apierrors.IsNotFound, BeTrue()),
+ "eventually the CAPI machine set should be deleted and not found",
+ )📝 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.
| Eventually(k.Update(capiMachineSet, func() { | |
| capiMachineSet.SetFinalizers([]string{machinesync.SyncFinalizer}) | |
| })).Should(Succeed()) | |
| By("Waiting for the manager's informer cache to observe the finalizer") | |
| eventuallyManagerInformerCache(capiMachineSet).Should( | |
| HaveField("ObjectMeta.Finalizers", ContainElement(machinesync.SyncFinalizer)), | |
| ) | |
| By("Deleting the CAPI machine set") | |
| Eventually(kDelete(ctx, capiMachineSet)).Should(Succeed()) | |
| }) | |
| It("should remove the sync finalizer and allow the CAPI machine set to be deleted", func() { | |
| Eventually(k.Get(capiMachineSet), timeout).Should( | |
| WithTransform(apierrors.IsNotFound, BeTrue()), | |
| ) | |
| }) | |
| Eventually(k.Update(capiMachineSet, func() { | |
| capiMachineSet.SetFinalizers([]string{machinesync.SyncFinalizer}) | |
| }), timeout).Should(Succeed(), "should be able to add the sync finalizer to the CAPI machine set") | |
| By("Waiting for the manager's informer cache to observe the finalizer") | |
| eventuallyManagerInformerCache(capiMachineSet).Should( | |
| HaveField("ObjectMeta.Finalizers", ContainElement(machinesync.SyncFinalizer)), | |
| "manager cache should observe the sync finalizer on the CAPI machine set", | |
| ) | |
| By("Deleting the CAPI machine set") | |
| Eventually(kDelete(ctx, capiMachineSet), timeout).Should(Succeed(), "should be able to delete the CAPI machine set") | |
| }) | |
| It("should remove the sync finalizer and allow the CAPI machine set to be deleted", func() { | |
| Eventually(k.Get(capiMachineSet), timeout).Should( | |
| WithTransform(apierrors.IsNotFound, BeTrue()), | |
| "eventually the CAPI machine set should be deleted and not found", | |
| ) | |
| }) |
🤖 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 `@pkg/controllers/machinesetsync/machineset_sync_controller_test.go` around
lines 1177 - 1194, Add the appropriate timeout and a meaningful failure message
to the `Eventually(k.Get(capiMachineSet), timeout)` assertion in the “should
remove the sync finalizer and allow the CAPI machine set to be deleted” test,
and ensure the nearby `Eventually` calls in this scenario follow the same
timeout-and-message convention without changing their behavior.
Source: Coding guidelines
92f8b2a to
7f7f713
Compare
|
/test unit |
|
/pipeline auto |
|
Pipeline controller notification The |
|
/test unit |
|
Scheduling tests matching the |
|
/retest-required |
|
@Kaustubh-pande: The following tests 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. |
|
/test e2e-metal3-capi-techpreview |
3 participants
Summary
When a CAPI MachineSet is reconciled after its corresponding MAPI MachineSet has already been removed, the existing deletion reconciliation is not executed, leaving the sync.machine.openshift.io/finalizer on the CAPI MachineSet and preventing it from completing deletion.
This change updates the reconciliation flow to invoke the existing CAPI deletion reconciliation when the MAPI MachineSet is absent, allowing the controller to remove the sync finalizer and complete CAPI MachineSet deletion.
This PR also adds a regression integration test covering the scenario where the MAPI MachineSet is absent and the CAPI MachineSet is deleted while still carrying the sync finalizer. The test failed prior to this change and now passes.
Additionally, this PR removes an unreachable
capiMachineSet == nilcheck from Reconcile(), since that case is already handled by the earliermapiMachineSet == nil && capiMachineSet == nilcheck.Testing
✅ Added a regression integration test for OCPBUGS-98492
✅ Verified the regression test fails without the fix and passes with the fix
✅ Ran the full MachineSetSync test suite (123/123 passing)
Fixes: OCPBUGS-98492
Summary by CodeRabbit