OCPBUGS-63720: orphan machines when managed identity is invalid on clus… - #8296
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
do-not-merge/needs-area
needs-rebase
📝 WalkthroughWalkthroughAdds Azure credential validation and orphaned-machine cleanup to the Azure platform code. Introduces ValidCredentials(hc *hyperv1.HostedCluster) bool to detect invalid Azure credentials from HostedCluster status conditions. Adds Azure.DeleteOrphanedMachines(ctx, c, hc, controlPlaneNamespace) error which returns early if managed identities are unset or credentials are valid; otherwise it lists capiav2 AzureMachine objects in the control plane namespace and, for machines with a non-nil DeletionTimestamp, clears Finalizers and updates the resource, aggregating per-machine errors. Unit tests for both functions were added. Sequence Diagram(s)sequenceDiagram
participant Controller as Azure.DeleteOrphanedMachines
participant HostedCluster as HostedCluster.Status
participant K8sAPI as Kubernetes API
participant AzureMachine as AzureMachine (resource)
Controller->>HostedCluster: call ValidCredentials(hc)
alt credentials valid
Controller-->>Controller: return (no-op)
else credentials invalid
Controller->>K8sAPI: List AzureMachine in controlPlaneNamespace
K8sAPI-->>Controller: list of AzureMachine objects
loop for each machine
Controller->>AzureMachine: inspect DeletionTimestamp
alt DeletionTimestamp set
Controller->>AzureMachine: clear Finalizers and Update()
AzureMachine-->>K8sAPI: Update request
K8sAPI-->>Controller: update result (success/failure)
else not pending deletion
Controller-->>AzureMachine: skip
end
end
Controller-->>Controller: aggregate update errors (if any) and return
end
🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @patilsuraj767. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
openshift-ci
Bot
added
area/hypershift-operator
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure_test.go (1)
652-663: Validation logic will skip assertions when DeletionTimestamp is zero.The validation condition
if !machine.DeletionTimestamp.IsZero()will befalsewhenDeletionTimestampis&metav1.Time{}(zero value), causing the assertion to never run. Combined with the test setup issue noted above, this results in no actual validation occurring for the pending deletion scenario.Consider also adding an explicit assertion that at least one machine was checked when
expectedFinalizersRemovedis true:Suggested validation improvement
// Validate finalizers were removed if expected if tc.expectedFinalizersRemoved { azureMachineList := &capiazure.AzureMachineList{} err := fakeClient.List(ctx, azureMachineList, client.InNamespace(controlPlaneNamespace)) g.Expect(err).ToNot(HaveOccurred()) + checkedCount := 0 for _, machine := range azureMachineList.Items { if !machine.DeletionTimestamp.IsZero() { + checkedCount++ g.Expect(machine.Finalizers).To(BeEmpty(), "Finalizers should be removed for machines with deletion timestamp") } } + g.Expect(checkedCount).To(BeNumerically(">", 0), "Expected at least one machine with deletion timestamp to validate") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure_test.go` around lines 652 - 663, The validation currently skips assertions when machine.DeletionTimestamp is the zero metav1.Time value, so update the check in the expectedFinalizersRemoved block to (1) treat a non-nil, non-zero DeletionTimestamp as the signal to assert Finalizers were removed (use machine.DeletionTimestamp != nil && !machine.DeletionTimestamp.IsZero()), (2) count how many machines you actually checked and add an explicit assertion that at least one machine was validated when expectedFinalizersRemoved is true (e.g., increment a checkedCount while iterating azureMachineList.Items and assert checkedCount > 0), and (3) keep the existing assertion that machine.Finalizers is empty for those checked machines; reference azureMachineList, fakeClient.List, controlPlaneNamespace, machine.DeletionTimestamp, and machine.Finalizers when making these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure_test.go`:
- Around line 583-595: The test uses a zero DeletionTimestamp (&metav1.Time{})
so IsZero() is true and the orphaning logic never runs; update the test's
azureMachines entry (capiazure.AzureMachine with Name "machine-1") to set a
non-zero DeletionTimestamp (e.g., metav1.NewTime(time.Now()) or
&metav1.Time{Time: time.Now()}) and likewise ensure the corresponding machine
used in the validation has a non-zero DeletionTimestamp so the finalizer-removal
branch executes; also add "time" to the imports.
In
`@hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure.go`:
- Around line 412-413: The log message logged via logger.Info for azureMachine
is misleading — it says "skipping cleanup of azuremachine" even though the
controller is removing finalizers/orphaning the machine locally; update the
message to clearly state that cloud-side cleanup is being skipped while the
AzureMachine is being orphaned/left with finalizers removed. Locate the
logger.Info call referencing client.ObjectKeyFromObject(azureMachine) and change
the text to something like "skipping cloud-side cleanup due to invalid Azure
managed identity; orphaning AzureMachine (removed finalizers)" so the action
(orphaning) and scope (cloud-side cleanup skipped) are explicit.
---
Nitpick comments:
In
`@hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure_test.go`:
- Around line 652-663: The validation currently skips assertions when
machine.DeletionTimestamp is the zero metav1.Time value, so update the check in
the expectedFinalizersRemoved block to (1) treat a non-nil, non-zero
DeletionTimestamp as the signal to assert Finalizers were removed (use
machine.DeletionTimestamp != nil && !machine.DeletionTimestamp.IsZero()), (2)
count how many machines you actually checked and add an explicit assertion that
at least one machine was validated when expectedFinalizersRemoved is true (e.g.,
increment a checkedCount while iterating azureMachineList.Items and assert
checkedCount > 0), and (3) keep the existing assertion that machine.Finalizers
is empty for those checked machines; reference azureMachineList,
fakeClient.List, controlPlaneNamespace, machine.DeletionTimestamp, and
machine.Finalizers when making these changes.
🪄 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: Pro Plus
Run ID: 182f5614-0b1a-40be-9431-17412835330e
📒 Files selected for processing (2)
hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure.gohypershift-operator/controllers/hostedcluster/internal/platform/azure/azure_test.go
92a46ca to
c7bde7f
Compare
openshift-ci
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure.go`:
- Around line 387-391: DeleteOrphanedMachines currently runs for all Azure
clusters when ValidCredentials(hc) is false and may orphan machines for
self-managed/workload-identity clusters; restrict this flow to only
managed-identity ARO clusters by adding an early return when the cluster is not
a managed-identity cluster. Concretely, update DeleteOrphanedMachines to call a
cluster-type helper (e.g., IsManagedIdentityCluster(hc) or inline check of
hc.Spec.Platform.Azure fields indicating managed identity) immediately after the
ValidCredentials check and return nil for non-managed-identity clusters; ensure
the helper inspects the HostedCluster Azure platform fields that differentiate
managed-identity vs workload/self-managed identity so the orphaning logic and
ARO cleanup identity path only run for managed-identity clusters.
🪄 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: Pro Plus
Run ID: 95c6feef-6d1a-4ae7-96ee-00e8bdfb0ddd
📒 Files selected for processing (2)
hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure.gohypershift-operator/controllers/hostedcluster/internal/platform/azure/azure_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure_test.go
c7bde7f to
fb9ab66
Compare
|
@patilsuraj767 This seems to handle the case of missing missing MI when deleting Azure machines. I am thinking that there could be other cases as well that'll fail deletion because of a missing MI (e.g KMS) or because of a missing user brought resources e.g KSM, Vnet, Subnet, or the overall RG etc Are you planning to handle that as well? |
@machi1990 We need to verify whether the HyperShift delete workflow fails when user-provided resources are missing during deletion. If the cluster deletion fails due to missing user-provided resources, that should be addressed in a separate PR. This change should be scoped only to missing/invalid managed identity handling. |
Yes it does and these are the cases we've already seen e.g the recent cspr orphaned are because of these issues. @patilsuraj767 okay if it needs to be a separate PR but let's make sure that this is addressed as part of your effort as its not only MI missing that can cause deletion stuck but other infra missing as well can cause deletion to be stuck and its critical we tackle it broadly |
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
fb9ab66 to
4d63699
Compare
|
/test e2e-aks e2e-aks-4-22 e2e-aws e2e-aws-4-22 e2e-aws-upgrade-hypershift-operator e2e-azure-self-managed e2e-kubevirt-aws-ovn-reduced e2e-v2-aws |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
/test e2e-aws-4-22 e2e-aks-4-22 |
|
/test e2e-azure-self-managed |
|
/test e2e-aws-4-22 |
| for i := range azureMachineList.Items { | ||
| azureMachine := &azureMachineList.Items[i] | ||
| if !azureMachine.DeletionTimestamp.IsZero() { | ||
| // Remove finalizers to orphan the machine |
There was a problem hiding this comment.
Can't we be specific about which finalizer(s) we intend to remove?
There was a problem hiding this comment.
I have updated the logic to remove only the specific finalizer instead of removing all finalizers indiscriminately.
| } | ||
|
|
||
| // If credentials are valid, nothing to do - normal deletion will work | ||
| if ValidCredentials(hc) { |
There was a problem hiding this comment.
Can we race whereby the CPO doesn't have time yet to post the invalid credential status, and we still start the deletion? Can we detect that case and still do the right thing?
There was a problem hiding this comment.
I think it can self-heal, If credentials become invalid around the same time deletion is triggered, ValidCredentials() can return true because CPO didn't completed yet, resulting in AzureMachine provider not able to delete the machine. The machine is stuck in deletion with its DeletionTimestamp set but finalizer never removed.
But HO will keep requeuing because the cluster still exists. Eventually the CPO posts the condition on the HCP, the HO copies it to the HC, and the next call to DeleteOrphanedMachines correctly orphans. So the race is transient.
|
Scheduling tests matching the |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
/test e2e-aws |
1 similar comment
|
/test e2e-aws |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
/hold Revision 185a7e3 was retested 3 times: holding |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/test e2e-aws |
1 similar comment
|
/test e2e-aws |
| // deletionFailedThreshold is the minimum duration a machine must have had a non-zero | ||
| // DeletionTimestamp before it is considered permanently stuck and eligible for orphaning. | ||
| // This guards against orphaning machines that hit transient failures (e.g. rate limiting). | ||
| const deletionFailedThreshold = 10 * time.Minute |
There was a problem hiding this comment.
Do we have a Kusto query showing our distribution of deletion times? If so, please leave it here so someone can re-run the analysis later, and specify what percentile 10 minutes is - p99? p99.99?
|
/test e2e-aws |
|
I now have all the evidence I need. Here is the complete analysis: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThis is a CI infrastructure failure, completely unrelated to the PR's code changes. The Root CauseThe CI infrastructure's pod-scaler component ( What happened step by step:
The PR's changes (orphan machine cleanup for invalid managed identity on Azure clusters) were never compiled into a test binary, never deployed, and never executed. This failure is purely a CI infrastructure issue. Recommendations
Evidence
|
|
/test e2e-aws |
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/hold cancel |
|
/ok-to-test |
|
@patilsuraj767: 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. |
|
@patilsuraj767: Jira Issue OCPBUGS-63720: All pull requests linked via external trackers have merged: This pull request has the 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. |
|
Fix included in release 5.0.0-0.nightly-2026-07-14-100335 |
…s during cluster deletion Replace the credential-level ValidAzureIdentityProvider signal with the machine-level DeletionFailed condition approach (aligned with PR openshift#8296). When an AzureMachine has been deleting for >10 minutes and CAPZ sets Ready=False with Reason=DeletionFailed, the finalizer is removed to unblock cluster teardown. Unlike openshift#8296 which only covers ARO HCP (ManagedIdentities != nil), this covers self-managed Azure clusters where credentials may expire or resource groups may be deleted out-of-band. Removes: ValidAzureIdentityProvider condition, CPO health check for Azure credentials, credential status types, and condition bubbling from HCP to HC. Keeps: cmd/cluster/azure/destroy.go 404 handling fix. Signed-off-by: Vimal Solanki <vsolanki@redhat.com>
…s during cluster deletion Replace the credential-level ValidAzureIdentityProvider signal with the machine-level DeletionFailed condition approach (aligned with PR openshift#8296). When an AzureMachine has been deleting for >10 minutes and CAPZ sets Ready=False with Reason=DeletionFailed, the finalizer is removed to unblock cluster teardown. Unlike openshift#8296 which only covers ARO HCP (ManagedIdentities != nil), this covers self-managed Azure clusters where credentials may expire or resource groups may be deleted out-of-band. Removes: ValidAzureIdentityProvider condition, CPO health check for Azure credentials, credential status types, and condition bubbling from HCP to HC. Keeps: cmd/cluster/azure/destroy.go 404 handling fix. Signed-off-by: Vimal Solanki <vsolanki@redhat.com>
…s during cluster deletion Replace the credential-level ValidAzureIdentityProvider signal with the machine-level DeletionFailed condition approach (aligned with PR openshift#8296). When an AzureMachine has been deleting for >10 minutes and CAPZ sets Ready=False with Reason=DeletionFailed, the finalizer is removed to unblock cluster teardown. Unlike openshift#8296 which only covers ARO HCP (ManagedIdentities != nil), this covers self-managed Azure clusters where credentials may expire or resource groups may be deleted out-of-band. Removes: ValidAzureIdentityProvider condition, CPO health check for Azure credentials, credential status types, and condition bubbling from HCP to HC. Keeps: cmd/cluster/azure/destroy.go 404 handling fix. Signed-off-by: Vimal Solanki <vsolanki@redhat.com>
13 participants
…ter deletion
Summary
When a customer deletes a managed identity required for cleaning up resources in the managed resource group, HyperShift will now skip deletion of those Azure machines and allow the ARO cluster service to handle cleanup using a special Azure 1P application identity.
Fixes: OCPBUGS-63720
Changes
How it works
Testing
Unit tests cover: