CNTRLPLANE-3978: Fix CPO finalizer race leaving orphaned Azure Private Endpoint resources - #9194
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@Nirshal: This pull request references CNTRLPLANE-3978 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. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReconciliation resolves the owning Sequence Diagram(s)sequenceDiagram
participant AzurePrivateLinkServiceReconciler
participant KubernetesAPI
participant Azure
AzurePrivateLinkServiceReconciler->>KubernetesAPI: Resolve HostedControlPlane
AzurePrivateLinkServiceReconciler->>KubernetesAPI: List sibling AzurePrivateLinkService resources
AzurePrivateLinkServiceReconciler->>Azure: Clean up Azure dependencies
AzurePrivateLinkServiceReconciler->>KubernetesAPI: Remove per-resource finalizers
AzurePrivateLinkServiceReconciler->>KubernetesAPI: Remove shared HostedControlPlane finalizer
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/control-plane-operator
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
control-plane-operator/controllers/azureprivatelinkservice/controller.go (1)
424-495: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider splitting
reconcileHCPDeletioninto smaller helpers.
reconcileHCPDeletionperforms four distinct jobs in one function: the finalizer guard check, Azure cleanup across all sibling CRs, per-CR finalizer removal across all sibling CRs, and shared HCP finalizer removal. Extract the cleanup loop and the finalizer-removal loop into two small helper methods (for examplecleanupSiblingAzureResourcesandremoveSiblingFinalizers), each returning an aggregated error. This keepsreconcileHCPDeletionfocused on orchestration and makes each step independently testable.As per coding guidelines, "Keep functions small and focused."
♻️ Suggested extraction
func (r *AzurePrivateLinkServiceReconciler) reconcileHCPDeletion(ctx context.Context, azPLS *hyperv1.AzurePrivateLinkService, hcp *hyperv1.HostedControlPlane, log logr.Logger) (ctrl.Result, error) { if !controllerutil.ContainsFinalizer(hcp, hcpAzurePLSFinalizerName) { return ctrl.Result{}, nil } log.Info("HCP is being deleted, cleaning up Azure resources before removing HCP finalizer") var allPLS hyperv1.AzurePrivateLinkServiceList if err := r.List(ctx, &allPLS, client.InNamespace(azPLS.Namespace)); err != nil { return ctrl.Result{}, fmt.Errorf("failed to list AzurePrivateLinkService resources: %w", err) } - var errs []error - for i := range allPLS.Items { - pls := &allPLS.Items[i] - log.Info("Cleaning up Azure resources for AzurePrivateLinkService", "name", pls.Name) - if err := r.reconcileDelete(ctx, pls, log); err != nil { - errs = append(errs, fmt.Errorf("failed to clean up %s: %w", pls.Name, err)) - } - } - - if err := utilerrors.NewAggregate(errs); err != nil { + if err := r.cleanupSiblingAzureResources(ctx, allPLS.Items, log); err != nil { return ctrl.Result{}, fmt.Errorf("failed to clean up Azure resources during HCP deletion: %w", err) } - for i := range allPLS.Items { - pls := &allPLS.Items[i] - if !controllerutil.ContainsFinalizer(pls, azurePrivateLinkServiceFinalizer) { - continue - } - log.Info("Removing per-CR finalizer from AzurePrivateLinkService", "name", pls.Name) - controllerutil.RemoveFinalizer(pls, azurePrivateLinkServiceFinalizer) - if err := r.Update(ctx, pls); err != nil { - errs = append(errs, fmt.Errorf("failed to remove per-CR finalizer from %s: %w", pls.Name, err)) - } - } - - if err := utilerrors.NewAggregate(errs); err != nil { + if err := r.removeSiblingFinalizers(ctx, allPLS.Items, log); err != nil { return ctrl.Result{}, fmt.Errorf("failed to remove per-CR finalizers during HCP deletion: %w", err) } // Remove the HCP finalizer to unblock HCP deletion log.Info("Azure resource cleanup complete for all AzurePrivateLinkService CRs, removing HCP finalizer") originalHCP := hcp.DeepCopy() controllerutil.RemoveFinalizer(hcp, hcpAzurePLSFinalizerName) if err := r.Patch(ctx, hcp, client.MergeFromWithOptions(originalHCP, client.MergeFromWithOptimisticLock{})); err != nil { if apierrors.IsConflict(err) { return ctrl.Result{RequeueAfter: time.Second}, nil } return ctrl.Result{}, fmt.Errorf("failed to remove HCP finalizer: %w", err) } return ctrl.Result{}, nil } + +func (r *AzurePrivateLinkServiceReconciler) cleanupSiblingAzureResources(ctx context.Context, items []hyperv1.AzurePrivateLinkService, log logr.Logger) error { + var errs []error + for i := range items { + pls := &items[i] + log.Info("Cleaning up Azure resources for AzurePrivateLinkService", "name", pls.Name) + if err := r.reconcileDelete(ctx, pls, log); err != nil { + errs = append(errs, fmt.Errorf("failed to clean up %s: %w", pls.Name, err)) + } + } + return utilerrors.NewAggregate(errs) +} + +func (r *AzurePrivateLinkServiceReconciler) removeSiblingFinalizers(ctx context.Context, items []hyperv1.AzurePrivateLinkService, log logr.Logger) error { + var errs []error + for i := range items { + pls := &items[i] + if !controllerutil.ContainsFinalizer(pls, azurePrivateLinkServiceFinalizer) { + continue + } + log.Info("Removing per-CR finalizer from AzurePrivateLinkService", "name", pls.Name) + controllerutil.RemoveFinalizer(pls, azurePrivateLinkServiceFinalizer) + if err := r.Update(ctx, pls); err != nil { + errs = append(errs, fmt.Errorf("failed to remove per-CR finalizer from %s: %w", pls.Name, err)) + } + } + return utilerrors.NewAggregate(errs) +}🤖 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 `@control-plane-operator/controllers/azureprivatelinkservice/controller.go` around lines 424 - 495, Refactor reconcileHCPDeletion so it remains focused on orchestration: retain the finalizer guard, sibling-list retrieval, helper calls, and shared HCP finalizer removal. Extract the Azure cleanup iteration into a helper such as cleanupSiblingAzureResources and the per-CR finalizer update iteration into removeSiblingFinalizers; each helper should accept the sibling resources and relevant context, aggregate all item errors, and return a single error while preserving the current error messages and behavior.control-plane-operator/controllers/azureprivatelinkservice/controller_test.go (1)
1380-1428: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen
TestReconcileHCPDeletion_WhenMultipleCRsExist_ItShouldRemoveAllPerCRFinalizerscoverage.This test verifies only that per-CR finalizers are removed. It does not assert that the shared HCP finalizer (
hcpAzurePLSFinalizerName) is removed fromhcp, which isreconcileHCPDeletion's final and primary outcome. Add an assertion that fetches or checkshcp.Finalizersafter the call.Also add a companion test for the partial-failure path: when Azure cleanup or the per-CR finalizer update fails for one sibling CR, verify that no per-CR finalizers and the HCP finalizer are removed. This is the core new error-aggregation behavior in
reconcileHCPDeletionand currently has no direct test coverage.As per coding guidelines, "Unit test any code changes and additions."
🤖 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 `@control-plane-operator/controllers/azureprivatelinkservice/controller_test.go` around lines 1380 - 1428, Strengthen TestReconcileHCPDeletion_WhenMultipleCRsExist_ItShouldRemoveAllPerCRFinalizers by asserting that the fetched HCP no longer contains hcpAzurePLSFinalizerName after successful reconciliation. Add a companion partial-failure test that makes Azure cleanup or a sibling per-CR finalizer update fail, then verify reconciliation returns the error and preserves every AzurePrivateLinkService finalizer and the HCP finalizer.
🤖 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.
Nitpick comments:
In
`@control-plane-operator/controllers/azureprivatelinkservice/controller_test.go`:
- Around line 1380-1428: Strengthen
TestReconcileHCPDeletion_WhenMultipleCRsExist_ItShouldRemoveAllPerCRFinalizers
by asserting that the fetched HCP no longer contains hcpAzurePLSFinalizerName
after successful reconciliation. Add a companion partial-failure test that makes
Azure cleanup or a sibling per-CR finalizer update fail, then verify
reconciliation returns the error and preserves every AzurePrivateLinkService
finalizer and the HCP finalizer.
In `@control-plane-operator/controllers/azureprivatelinkservice/controller.go`:
- Around line 424-495: Refactor reconcileHCPDeletion so it remains focused on
orchestration: retain the finalizer guard, sibling-list retrieval, helper calls,
and shared HCP finalizer removal. Extract the Azure cleanup iteration into a
helper such as cleanupSiblingAzureResources and the per-CR finalizer update
iteration into removeSiblingFinalizers; each helper should accept the sibling
resources and relevant context, aggregate all item errors, and return a single
error while preserving the current error messages and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 17503a62-168b-4db0-899f-cecffbf827bd
📒 Files selected for processing (2)
control-plane-operator/controllers/azureprivatelinkservice/controller.gocontrol-plane-operator/controllers/azureprivatelinkservice/controller_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9194 +/- ##
==========================================
+ Coverage 44.96% 44.98% +0.01%
==========================================
Files 778 778
Lines 97452 97510 +58
==========================================
+ Hits 43820 43863 +43
- Misses 50607 50617 +10
- Partials 3025 3030 +5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/area control-plane-operator |
|
/area platform/azure |
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
`@control-plane-operator/controllers/azureprivatelinkservice/controller_test.go`:
- Around line 1417-1425: Extend the HCP deletion reconciliation test after the
existing updated1 and updated2 assertions to fetch the HCP resource and verify
that its Finalizers no longer contain hcpAzurePLSFinalizerName. Keep the
existing per-resource finalizer checks 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: 957ca4da-1c24-49b5-a882-797269fde664
📒 Files selected for processing (1)
control-plane-operator/controllers/azureprivatelinkservice/controller_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
control-plane-operator/controllers/azureprivatelinkservice/controller_test.go (1)
1442-1497: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a partial-failure test case to verify independent per-resource finalizer removal.
This test only covers the case where Azure cleanup fails for both siblings (
RecordSets.deleteErrtriggers on every call). The PR objective states that per-resource finalizers are removed independently "to prevent namespace cleanup from being blocked after Azure resources are deleted," which implies that if cleanup succeeds for one sibling but fails for another, the succeeding sibling's finalizer should be removed while the failing sibling's finalizer stays. No test in this segment exercises that partial-success case.Add a case where cleanup succeeds for
azPLS1but fails forazPLS2(or vice versa), and assert that only the failing sibling's finalizer remains, and the shared HCP finalizer also remains, since one sibling did not complete cleanup.// Example additional scenario to add alongside the existing failure test func TestReconcileHCPDeletion_WhenOneSiblingSucceedsAndOneFails_ItShouldRemoveOnlySucceedingFinalizer(t *testing.T) { // azPLS1 cleanup succeeds, azPLS2 cleanup fails (e.g., a mock keyed by CR name) // Assert: azPLS1 finalizer removed, azPLS2 finalizer preserved, HCP finalizer preserved. }🤖 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 `@control-plane-operator/controllers/azureprivatelinkservice/controller_test.go` around lines 1442 - 1497, Add a partial-success test alongside TestReconcileHCPDeletion_WhenSiblingCleanupFails_ItShouldPreserveAllFinalizers, configuring cleanup to succeed for one sibling and fail for the other. Assert the successful sibling’s azurePrivateLinkServiceFinalizer is removed, the failing sibling’s remains, and the HCP’s hcpAzurePLSFinalizerName remains; also verify reconciliation returns an 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.
Nitpick comments:
In
`@control-plane-operator/controllers/azureprivatelinkservice/controller_test.go`:
- Around line 1442-1497: Add a partial-success test alongside
TestReconcileHCPDeletion_WhenSiblingCleanupFails_ItShouldPreserveAllFinalizers,
configuring cleanup to succeed for one sibling and fail for the other. Assert
the successful sibling’s azurePrivateLinkServiceFinalizer is removed, the
failing sibling’s remains, and the HCP’s hcpAzurePLSFinalizerName remains; also
verify reconciliation returns an error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e80609ed-cde7-4216-b7f6-23c18a87cdfb
📒 Files selected for processing (2)
control-plane-operator/controllers/azureprivatelinkservice/controller.gocontrol-plane-operator/controllers/azureprivatelinkservice/controller_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- control-plane-operator/controllers/azureprivatelinkservice/controller.go
|
Re: review comment suggesting a partial-success test where one sibling's cleanup succeeds and the other fails: This doesn't match the actual code flow. When The existing |
81515ee to
12be13c
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
control-plane-operator/controllers/azureprivatelinkservice/controller_test.go (2)
1442-1496: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the propagated error to confirm the intended failure path.
Line 1475 only checks that an error occurred. It does not check the error content. Add an assertion that the returned error contains the injected message. This confirms the test exercises the sibling-cleanup failure path, not an unrelated error.
💡 Proposed test strengthening
_, err := r.reconcileHCPDeletion(t.Context(), azPLS1, hcp, testr.New(t)) g.Expect(err).To(HaveOccurred(), "expected error when sibling Azure cleanup fails") + g.Expect(err.Error()).To(ContainSubstring("simulated Azure API failure"), + "error should originate from the injected Azure cleanup failure")🤖 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 `@control-plane-operator/controllers/azureprivatelinkservice/controller_test.go` around lines 1442 - 1496, Strengthen the error assertion in TestReconcileHCPDeletion_WhenSiblingCleanupFails_ItShouldPreserveAllFinalizers by verifying that err contains the injected “simulated Azure API failure” message, while retaining the existing assertion that an error occurred.
1380-1577: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting shared reconciler/fake-client setup.
The four new tests in this range repeat the same
AzurePrivateLinkServiceReconcilerconstruction with mock APIs. Extract a small helper, for examplenewTestReconciler(fakeClient client.Client) *AzurePrivateLinkServiceReconciler, to reduce duplication across these tests.As per coding guidelines, "Use table-driven tests where possible" for
**/*_test.go; a shared constructor helper is a smaller step toward that goal without forcing these differently-shaped scenarios into one table.🤖 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 `@control-plane-operator/controllers/azureprivatelinkservice/controller_test.go` around lines 1380 - 1577, Extract the repeated AzurePrivateLinkServiceReconciler construction from the four tests into a shared helper such as newTestReconciler(client.Client), initializing all mock API dependencies consistently. Replace each inline reconciler literal in TestReconcileHCPDeletion_WhenMultipleCRsExist_ItShouldRemoveAllPerCRFinalizers, TestReconcileHCPDeletion_WhenSiblingCleanupFails_ItShouldPreserveAllFinalizers, TestReconcile_WhenHCPIsGone_ItShouldRemoveOrphanedPerCRFinalizer, and TestReconcile_WhenHCPIsBeingDeleted_ItShouldNotReAddPerCRFinalizer with the helper, preserving the custom RecordSets delete error in the failure case.Source: Coding guidelines
🤖 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.
Nitpick comments:
In
`@control-plane-operator/controllers/azureprivatelinkservice/controller_test.go`:
- Around line 1442-1496: Strengthen the error assertion in
TestReconcileHCPDeletion_WhenSiblingCleanupFails_ItShouldPreserveAllFinalizers
by verifying that err contains the injected “simulated Azure API failure”
message, while retaining the existing assertion that an error occurred.
- Around line 1380-1577: Extract the repeated AzurePrivateLinkServiceReconciler
construction from the four tests into a shared helper such as
newTestReconciler(client.Client), initializing all mock API dependencies
consistently. Replace each inline reconciler literal in
TestReconcileHCPDeletion_WhenMultipleCRsExist_ItShouldRemoveAllPerCRFinalizers,
TestReconcileHCPDeletion_WhenSiblingCleanupFails_ItShouldPreserveAllFinalizers,
TestReconcile_WhenHCPIsGone_ItShouldRemoveOrphanedPerCRFinalizer, and
TestReconcile_WhenHCPIsBeingDeleted_ItShouldNotReAddPerCRFinalizer with the
helper, preserving the custom RecordSets delete error in the failure case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 71c93aa6-a5f2-4fb4-b302-46d6032a28ec
📒 Files selected for processing (2)
control-plane-operator/controllers/azureprivatelinkservice/controller.gocontrol-plane-operator/controllers/azureprivatelinkservice/controller_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- control-plane-operator/controllers/azureprivatelinkservice/controller.go
|
/test e2e-v2-azure-self-managed |
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
Addressing the two nitpick comments from this review:
Incoming push. |
|
Re: review suggesting partial-success test (one sibling succeeds, the other fails): Already addressed in this comment. The code returns early from |
|
/test all |
95b7520 to
b9e1ffc
Compare
|
/test e2e-v2-azure-self-managed |
e2e-v2-azure-self-managed failure analysis (build 2084917155651915776)Failed test1 Failed | 425 Passed | 540 Skipped - failure is in the What the test doesThe test ( Why this is unrelated to this PRThis PR modifies
No code path is shared between the AzurePrivateLinkService reconciler and the HCCO webhook deletion controller. This PR does not modify HCCO, does not touch webhook handling, and does not affect the guest cluster API. Cross-check with other PRsChecked
The failure is sporadic and not correlated with any specific code change. It appears to be a timing-sensitive flaky in the HCCO webhook cleanup reconciliation (60s timeout may be too tight under load). |
CPO finalizer fix validated by downstream e2ePR #8584 (CNTRLPLANE-3277) is rebased on top of this PR and includes all 5 CPO commits in its base. Its This serves as additional e2e validation for this PR's changes, beyond the unit tests included here. |
| return ctrl.Result{}, nil | ||
| } | ||
|
|
||
| func (r *AzurePrivateLinkServiceReconciler) cleanupSiblingAzureResources(ctx context.Context, items []hyperv1.AzurePrivateLinkService, log logr.Logger) error { |
There was a problem hiding this comment.
Nit: The "sibling" framing assumes there's always more than one CR, but the common case is a single private-router CR — the second oauth-openshift CR only exists with oauthPublishingStrategy=LoadBalancer. When there's one CR, "sibling" reads oddly.
Consider cleanupAllAzureResources / removeAllCRFinalizers — count-neutral and still accurate.
There was a problem hiding this comment.
Good catch, renamed to cleanupAllAzureResources and removeAllCRFinalizers. The "sibling" framing was indeed misleading since the function operates on all CRs unconditionally.
| } | ||
|
|
||
| if err := r.cleanupSiblingAzureResources(ctx, allPLS.Items, log); err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("failed to clean up Azure resources during HCP deletion: %w", err) |
There was a problem hiding this comment.
When cleanupSiblingAzureResources calls reconcileDelete for each CR, deleteBaseDomainResources calls hasSiblingCR to decide whether to delete the base domain zone. During HCP deletion, neither CR has a DeletionTimestamp set (only the HCP does), so:
- private-router cleanup: sees oauth-openshift as an active sibling → skips zone deletion
- oauth-openshift cleanup: sees private-router as an active sibling → skips zone deletion
After both cleanups, per-CR finalizers are removed, CRs are garbage-collected without their own finalizer logic running, and the base domain DNS zone is never deleted.
This is the same class of bug this PR is fixing — just for the base domain zone instead of the PE. Could you either skip the hasSiblingCR check when called from the HCP deletion path, or add a dedicated base domain zone cleanup pass after all CRs are processed?
If resource group deletion is expected to clean this up, a comment documenting that assumption would be helpful.
There was a problem hiding this comment.
You're right, this is a real bug. During HCP deletion neither CR has DeletionTimestamp set, so hasSiblingCR returns true for both and the base domain zone is never deleted.
Before fixing, I verified that resource group deletion wouldn't clean this up implicitly. The guest resource group (azPLS.Spec.ResourceGroupName) is only deleted by the CLI path (hypershift destroy cluster azure). During normal HCP-driven teardown, the HO controller does not delete the resource group: AzureCluster is annotated managed-by: external so CAPZ doesn't manage it either, and the HO's delete() function only removes individual resources via finalizers. So the zone would remain orphaned in the customer's subscription.
I considered adding a flag parameter to reconcileDelete (e.g. skipSiblingCheck bool) to bypass hasSiblingCR when called from the HCP deletion path, but that would mean threading a boolean through two levels of calls (reconcileDelete -> deleteBaseDomainResources). As a fan of Uncle Bob Martin's Clean Code, I'd rather avoid flag arguments that change a function's behavior based on a boolean - it's a sign the function is doing two things.
Instead I went with a dedicated deleteBaseDomainDNSZone pass in reconcileHCPDeletion, called after cleanupAllAzureResources. At that point all per-CR A records and VNet links are already cleaned up, so the zone is empty and safe to delete. The call is idempotent: in the single-CR case, deleteBaseDomainResources already deleted the zone (no siblings), and deleteBaseDomainDNSZone gets a NotFound which is handled gracefully.
Added tests for: multi-CR deletion (zone gets deleted), single-CR idempotency, zone deletion failure preserving the HCP finalizer, and a no-BaseDomain assertion in the existing multi-CR test.
| // is gone (NotFound), it removes any orphaned per-CR finalizer so the CR can be | ||
| // garbage-collected with the namespace. Returns (nil, nil) when the HCP is gone and | ||
| // cleanup succeeded; the caller should return immediately. | ||
| func (r *AzurePrivateLinkServiceReconciler) getHCPOrCleanupOrphan(ctx context.Context, azPLS *hyperv1.AzurePrivateLinkService) (*hyperv1.HostedControlPlane, error) { |
There was a problem hiding this comment.
This function removes a finalizer via r.Update (a mutating side effect) but doesn't log that it did so. Every other helper that performs side effects (cleanupSiblingAzureResources, removeSiblingFinalizers, reconcileHCPDeletion) accepts a logr.Logger and logs its actions. Consider adding a logger parameter and a log.Info("HCP is gone, removed orphaned per-CR finalizer") inside the finalizer-removal branch for observability.
There was a problem hiding this comment.
Added a logr.Logger parameter and a log.Info("HostedControlPlane not found, removing orphaned per-CR finalizer") call before the finalizer removal. The call site already has log in scope so it's a clean pass-through.
| log.Info("Removing per-CR finalizer from AzurePrivateLinkService", "name", pls.Name) | ||
| controllerutil.RemoveFinalizer(pls, azurePrivateLinkServiceFinalizer) | ||
| if err := r.Update(ctx, pls); err != nil { | ||
| errs = append(errs, fmt.Errorf("failed to remove per-CR finalizer from %s: %w", pls.Name, err)) |
There was a problem hiding this comment.
Minor: This Update uses the object from the List snapshot at the top of reconcileHCPDeletion. If the CR is modified between the List and this Update, it's a last-writer-wins with no optimistic lock. The HCP finalizer removal at line 464 uses MergeFromWithOptimisticLock — consider the same pattern here for consistency. With MaxConcurrentReconciles: 1 and this being a deletion path the risk is low, but it would be more defensive.
There was a problem hiding this comment.
Switched from r.Update to r.Patch with MergeFromWithOptimisticLock in removeAllCRFinalizers, consistent with how ensureHCPFinalizer and the HCP finalizer removal in reconcileHCPDeletion already work.
|
/test e2e-v2-azure-self-managed |
|
/restruture-commits |
When a hosted cluster with endpointAccess=Private and oauthPublishingStrategy=LoadBalancer is deleted, two AzurePrivateLinkService CRs exist (private-router and oauth-openshift). The shared HCP finalizer was removed after cleaning up only the first CR to reconcile, orphaning the second CR's Azure resources (PE, DNS zone, VNet link). Replace single-CR cleanup with an all-CR pass in reconcileHCPDeletion: list all AzurePrivateLinkService CRs, call reconcileDelete for each, remove per-CR finalizers so namespace deletion is not blocked, then remove the shared HCP finalizer. Additionally fix the base domain DNS zone cleanup: during HCP deletion neither CR has DeletionTimestamp set, so hasSiblingCR returns true for both and neither deletes the shared zone. Add an explicit deleteBaseDomainDNSZone pass after all per-CR resources are cleaned up. The guest resource group is not deleted by the HO controller during normal teardown, so without this fix the zone remains orphaned in the customer's subscription. Other improvements from review feedback: - Rename cleanupSiblingAzureResources/removeSiblingFinalizers to cleanupAllAzureResources/removeAllCRFinalizers - Add logging to getHCPOrCleanupOrphan for orphaned finalizer removal - Use MergeFromWithOptimisticLock in removeAllCRFinalizers Signed-off-by: Alessandro Rossi <alesross@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
49a2a30 to
30d6ce5
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, Nirshal 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
|
/retest |
|
/lgtm |
|
Scheduling tests matching the |
|
/verified by e2e |
openshift-ci-robot
added
the
verified
|
@Nirshal: This PR has been marked as verified by 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. |
|
@Nirshal: all tests passed! 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. |
5 participants
What this PR does / why we need it:
When a HostedCluster with
endpointAccess=PrivateandoauthPublishingStrategy=LoadBalanceris deleted, two AzurePrivateLinkService CRsexist in the HCP namespace (one for
private-router, one foroauth-openshift).Both share a single HCP finalizer (
hypershift.openshift.io/azure-pls-endpoint-cleanup),but
reconcileHCPDeletiononly cleans up the CR that reconciles first, then removesthe shared finalizer. The second CR's Azure resources (Private Endpoint, DNS zone,
VNet link) are orphaned, and its per-CR finalizer blocks namespace deletion.
The orphaned Private Endpoint then blocks management cluster resource group deletion
with Azure 409:
PrivateLinkServiceWithPrivateEndpointConnectionsCannotBeDeleted.This fix makes
reconcileHCPDeletionlist and clean up ALL AzurePrivateLinkServiceCRs in a single pass before removing the shared HCP finalizer. Per-CR finalizers are
also removed during HCP deletion so they do not block namespace cleanup after Azure
resources are already gone.
Which issue(s) this PR fixes:
Fixes https://redhat.atlassian.net/browse/CNTRLPLANE-3978
Special notes for your reviewer:
reconcileDeleteis idempotent (checksIsAzureNotFoundError), so processingall CRs in a single pass is safe.
Reconcilewhen CRs aregarbage-collected during namespace cleanup. Removing them during HCP deletion
avoids the race where CPO is torn down before the second CR reconciles.
destroy-guestscompletes in ~12m47s vs theprevious 40-minute timeout.
production scenario combined
Privateendpoint access withLoadBalancerOAuth publishing strategy until CNTRLPLANE-3277.
Checklist:
Summary by CodeRabbit
Bug Fixes
Tests