OCPBUGS-86690: fix Azure cluster deletion hanging when resource groups are already deleted - #8682
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
jira/valid-reference
|
@vsolanki12: This pull request references Jira Issue OCPBUGS-86690, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
|
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:
📝 WalkthroughWalkthroughThis PR adds Azure identity-provider validation to the control-plane healthcheck path, passes Azure credentials into that check, and propagates the resulting condition to HostedCluster status. It also adds Azure orphaned-machine cleanup logic in the hostedcluster platform code, a new HostedCluster condition type, and a destroy-path change that continues when an explicitly provided Azure resource group returns 404. Sequence Diagram(s)sequenceDiagram
participant Main as control-plane-operator/main.go
participant HCU as HealthCheckUpdater
participant HC as azureHealthCheckIdentityProvider
participant AzureUtil as azureutil.GetResourceGroupInfo
participant HCP as HostedControlPlane
Main->>Main: resolveAzureHealthCheckCredentials
Main->>HCU: set AzureCreds
HCU->>HC: run Azure identity check
HC->>HCP: read platform and status
HC->>AzureUtil: validate Azure resource group info
AzureUtil-->>HC: success or Azure error
HC-->>HCU: update ValidAzureIdentityProvider
Suggested reviewers
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/cli
|
@vsolanki12: This pull request references Jira Issue OCPBUGS-86690, which is valid. 3 validation(s) were run on this bug
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. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8682 +/- ##
==========================================
+ Coverage 46.62% 46.70% +0.07%
==========================================
Files 784 784
Lines 98849 98897 +48
==========================================
+ Hits 46092 46192 +100
+ Misses 49628 49574 -54
- Partials 3129 3131 +2
... and 3 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
79425af to
ba7737f
Compare
|
I have tried to reproduce and tested the fix in my test cluster as below: Environment: Azure HCP cluster vsolanki-86690, CP version 4.22.0 Custom hypershift-operator image deployed: Test scenario: Created two AzureMachines with Stuck AzureMachines (before HC deletion): After fix triggered HC deletion, AzureMachines cleaned up: Operator logs confirm finalizer removal: Both AzureMachines had their finalizers removed (stuck for 37 minutes, well past the 5-minute threshold). HC deletion cascade proceeded without hanging. |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
| const orphanedMachineDeletionThreshold = 5 * time.Minute | ||
|
|
||
| func (Azure) DeleteOrphanedMachines(ctx context.Context, c client.Client, hc *hyperv1.HostedCluster, controlPlaneNamespace string) error { | ||
| azureMachineList := capiazure.AzureMachineList{} |
There was a problem hiding this comment.
Seems we are missing a semantic for orphan machines, similar to what we have in aws via GetCredentialStatus. That would be the criteria to let deletion move forward
There was a problem hiding this comment.
Thanks you for guidance @enxebre, as discussed, we need to implement AWS like conditions (ValidOIDCConfiguration and ValidAWSIdentityProvider) for Azure as well, so GetCredentialStatus can read those instead of needing a timeout.
This would touch API, CPO, and HO. Should I expand this PR or close it and open a fresh one with the broader scope or Do I need another JIRA with broader change and later touch this PR?
ba7737f to
e677429
Compare
openshift-ci
Bot
added
area/api
vsolanki12
changed the title
|
@vsolanki12: This pull request references Jira Issue OCPBUGS-86690, which is valid. 3 validation(s) were run on this bug
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. |
e677429 to
5c7524b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
api/hypershift/v1beta1/hostedcluster_conditions.go (2)
167-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix garbled doc comment for
ValidAzureIdentityProvider.The sentence "ValidAzureIdentityProvider indicates if the Identity Provider is used by the Azure Platform credentials is" is grammatically broken ("is used by ... is healthy"). Since this is an exported API type's godoc comment, it's worth tightening for clarity.
✏️ Proposed wording fix
- // ValidAzureIdentityProvider indicates if the Identity Provider is used by the Azure Platform credentials is - // healthy. The check makes a harmless Resource Group GET call to verify credentials work and infrastructure is available. + // ValidAzureIdentityProvider indicates whether the Identity Provider used by the Azure Platform credentials + // is healthy. The check makes a harmless Resource Group GET call to verify credentials work and infrastructure is available. // A failure here may require external user intervention to resolve. ValidAzureIdentityProvider ConditionType = "ValidAzureIdentityProvider"🤖 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 `@api/hypershift/v1beta1/hostedcluster_conditions.go` around lines 167 - 171, The godoc for ValidAzureIdentityProvider is grammatically broken and should be rewritten for clarity on the exported ConditionType in hostedcluster_conditions.go. Update the comment so it clearly states that the condition indicates whether the Azure platform identity provider credentials are healthy, and keep the existing explanation about the harmless Resource Group GET check and possible user intervention.
268-278: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
EtcdDataEncryptionUpToDatemisplaced under "PublicEndpointExposed" reasons block.Line 268's comment scopes this
constblock to reasons forPublicEndpointExposed, butEtcdDataEncryptionUpToDate(aConditionType, not a reason string for that condition) is declared inside it at Line 278. This groups an unrelated condition type under a misleading heading.♻️ Suggested regrouping
// Reasons for PublicEndpointExposed condition. const ( PublicEndpointSharedIngressConfiguredReason = "SharedIngressConfigured" PublicEndpointTopologyPrivateReason = "TopologyPrivate" PublicEndpointConvergenceInProgressReason = "ConvergenceInProgress" - // EtcdDataEncryptionUpToDate indicates whether all etcd data is encrypted with the - // currently active encryption key. - // True: all data confirmed encrypted with the active key. - // False: re-encryption is in progress or has failed. - // Absent: encryption is not configured. - EtcdDataEncryptionUpToDate ConditionType = "EtcdDataEncryptionUpToDate" +) + +// EtcdDataEncryptionUpToDate indicates whether all etcd data is encrypted with the +// currently active encryption key. +// True: all data confirmed encrypted with the active key. +// False: re-encryption is in progress or has failed. +// Absent: encryption is not configured. +const EtcdDataEncryptionUpToDate ConditionType = "EtcdDataEncryptionUpToDate"🤖 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 `@api/hypershift/v1beta1/hostedcluster_conditions.go` around lines 268 - 278, The EtcdDataEncryptionUpToDate constant is grouped under the PublicEndpointExposed reason constants, but it represents a separate ConditionType and should not live in that block. Move EtcdDataEncryptionUpToDate out of the “Reasons for PublicEndpointExposed condition” const group in hostedcluster_conditions.go and place it with the other Etcd-related condition type declarations, keeping the PublicEndpointSharedIngressConfiguredReason/PublicEndpointTopologyPrivateReason/PublicEndpointConvergenceInProgressReason block focused only on PublicEndpointExposed reasons.
🤖 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 `@api/hypershift/v1beta1/hostedcluster_conditions.go`:
- Around line 167-171: The godoc for ValidAzureIdentityProvider is grammatically
broken and should be rewritten for clarity on the exported ConditionType in
hostedcluster_conditions.go. Update the comment so it clearly states that the
condition indicates whether the Azure platform identity provider credentials are
healthy, and keep the existing explanation about the harmless Resource Group GET
check and possible user intervention.
- Around line 268-278: The EtcdDataEncryptionUpToDate constant is grouped under
the PublicEndpointExposed reason constants, but it represents a separate
ConditionType and should not live in that block. Move EtcdDataEncryptionUpToDate
out of the “Reasons for PublicEndpointExposed condition” const group in
hostedcluster_conditions.go and place it with the other Etcd-related condition
type declarations, keeping the
PublicEndpointSharedIngressConfiguredReason/PublicEndpointTopologyPrivateReason/PublicEndpointConvergenceInProgressReason
block focused only on PublicEndpointExposed reasons.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: d4cd767e-3318-46bf-9eea-aeb9138c4080
⛔ Files ignored due to path filters (2)
docs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.mddocs/content/reference/api.mdis excluded by!docs/content/reference/api.md
📒 Files selected for processing (6)
api/hypershift/v1beta1/hostedcluster_conditions.gocmd/cluster/azure/destroy.gocontrol-plane-operator/controllers/healthcheck/azure.gocontrol-plane-operator/controllers/healthcheck/azure_test.gocontrol-plane-operator/controllers/healthcheck/healthcheck_controller.gocontrol-plane-operator/main.go
💤 Files with no reviewable changes (5)
- control-plane-operator/controllers/healthcheck/azure_test.go
- cmd/cluster/azure/destroy.go
- control-plane-operator/controllers/healthcheck/azure.go
- control-plane-operator/main.go
- control-plane-operator/controllers/healthcheck/healthcheck_controller.go
|
I have tested this in test cluster. Before fix:
After Fix:
|
|
/rebase |
|
🤖 Rebasing PR onto main: workflow run |
…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>
c021040 to
5e7b1fb
Compare
openshift-ci-robot
removed
the
verified
openshift-ci
Bot
removed
lgtm
|
/verified by @georgelipceanu Reference here: #8682 (comment) |
openshift-ci-robot
added
the
verified
|
@vsolanki12: 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. |
|
/lgtm |
|
Scheduling tests matching the |
|
/hold Revision 5e7b1fb was retested 3 times: holding |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/retest |
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/retest |
|
/override ci/prow/okd-scos-images |
|
@csrwng: Overrode contexts on behalf of csrwng: ci/prow/okd-scos-images 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 kubernetes-sigs/prow repository. |
|
@vsolanki12: 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. |
|
@vsolanki12: Jira Issue Verification Checks: Jira Issue OCPBUGS-86690 Jira Issue OCPBUGS-86690 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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.1.0-0.nightly-2026-09-04-051944 |
9 participants
What this PR does / why we need it:
When an Azure self-managed HCP cluster's resource groups are deleted out-of-band (e.g. via Azure portal or expired credentials),
hypershift destroy cluster azurehangs indefinitely. AzureMachines get stuck in deletion because CAPZ cannot communicate with the deleted Azure infrastructure, and their finalizers are never cleared.This PR uses the same DeletionFailed condition approach as #8296 (ARO HCP) but extends coverage to self-managed Azure clusters. PR #8296 guards with
ManagedIdentities == nilearly return, so self-managed clusters (which don't use managed identities) are not covered.How it works:
Machine-level signal: When CAPZ cannot delete an Azure VM (expired credentials, deleted resource group, etc.), it sets
Ready=FalsewithReason=DeletionFailedon the AzureMachine.Orphaned machine cleanup (
DeleteOrphanedMachines): For each AzureMachine that has been deleting for >10 minutes (deletionFailedThreshold) AND has theDeletionFailedcondition, theMachineFinalizeris removed — unblocking cluster teardown.CLI fix (
cmd/cluster/azure/destroy.go): Handles 404 on resource group duringhypershift destroy cluster azureso it continues instead of failing hard when infrastructure is already gone.Difference from #8296:
ManagedIdentities != nil)DeletionFailedconditionManagedIdentities == nilearly returnThe
DeletionFailedcondition is set by CAPZ regardless of credential type, so this approach works for both managed identity and service principal clusters.Which issue(s) this PR fixes:
Fixes OCPBUGS-86690
Special notes for your reviewer:
ValidAzureIdentityProvidercondition) has been fully removed per feedback from @enxebre. All CPO health check code, condition bubbling, and credential status types are gone.deletionFailedThreshold+hasDeletionFailedCondition) without theManagedIdentitiesguard.cmd/cluster/azure/destroy.go404 handling fix is independent and still needed.Checklist: