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)
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 |
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 45.74% 45.74% -0.01%
==========================================
Files 781 781
Lines 97858 97858
==========================================
- Hits 44765 44761 -4
- Misses 50024 50028 +4
Partials 3069 3069
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:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go (1)
452-481: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect mirror of AWS pattern; consider extracting shared helper.
The new Azure block is logically correct and mirrors the existing
ValidAWSIdentityProviderbubbling logic (lines 424-450) exactly, differing only in the condition type and platform check. This duplication will grow if more platforms adopt the same pattern.♻️ Suggested helper extraction
+func bubbleUpIdentityProviderCondition(hcluster *hyperv1.HostedCluster, hcp *hyperv1.HostedControlPlane, conditionType hyperv1.ConditionType) bool { + var validIdentityProviderCondition *metav1.Condition + if hcp != nil { + validIdentityProviderCondition = meta.FindStatusCondition(hcp.Status.Conditions, string(conditionType)) + } + if validIdentityProviderCondition == nil { + return meta.SetStatusCondition(&hcluster.Status.Conditions, metav1.Condition{ + Type: string(conditionType), + Status: metav1.ConditionUnknown, + Reason: hyperv1.StatusUnknownReason, + ObservedGeneration: hcluster.Generation, + }) + } + validIdentityProviderCondition.ObservedGeneration = hcluster.Generation + return meta.SetStatusCondition(&hcluster.Status.Conditions, *validIdentityProviderCondition) +} + if hcluster.Spec.Platform.Type == hyperv1.AWSPlatform { - updated := false - var validIdentityProviderCondition *metav1.Condition - if hcp != nil { - validIdentityProviderCondition = meta.FindStatusCondition(hcp.Status.Conditions, string(hyperv1.ValidAWSIdentityProvider)) - } - if validIdentityProviderCondition == nil { - updated = meta.SetStatusCondition(&hcluster.Status.Conditions, metav1.Condition{...}) - } else { - validIdentityProviderCondition.ObservedGeneration = hcluster.Generation - updated = meta.SetStatusCondition(&hcluster.Status.Conditions, *validIdentityProviderCondition) - } - if updated { + if bubbleUpIdentityProviderCondition(hcluster, hcp, hyperv1.ValidAWSIdentityProvider) { if err := r.Client.Status().Update(ctx, hcluster); err != nil { return ctrl.Result{}, fmt.Errorf("failed to update status: %w", err) } } } if hcluster.Spec.Platform.Type == hyperv1.AzurePlatform { - ... (same body as above with ValidAzureIdentityProvider) + if bubbleUpIdentityProviderCondition(hcluster, hcp, hyperv1.ValidAzureIdentityProvider) { + if err := r.Client.Status().Update(ctx, hcluster); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to update status: %w", 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 `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go` around lines 452 - 481, The Azure status-bubbling logic in hostedcluster_controller.go duplicates the existing AWS pattern in the same reconcile path, so extract the shared condition-propagation flow into a helper. Refactor the logic around ValidAWSIdentityProvider/ValidAzureIdentityProvider in HostedClusterReconciler so a single helper handles finding the HCP condition, setting ObservedGeneration, defaulting Unknown when missing, and persisting status updates, with platform/condition type passed in as parameters.hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure_test.go (2)
539-557: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest case names don't follow the mandated "When ... it should ..." format.
Names like
"When condition is True, return Valid"omit "it should". As per path instructions: "Always use 'When ... it should ...' format for describing test cases when creating unit tests."✏️ Suggested rename
- name: "When condition is True, return Valid", + name: "When condition is True, it should return Valid", ... - name: "When condition is False, return Invalid", + name: "When condition is False, it should return Invalid", ... - name: "When condition is Unknown, return Unknown", + name: "When condition is Unknown, it should return Unknown", ... - name: "When condition is missing, return Unknown", + name: "When condition is missing, it should return Unknown",🤖 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 `@hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure_test.go` around lines 539 - 557, The test case names in azure_test.go do not follow the required “When ... it should ...” pattern. Update the affected table-driven cases in the Azure credential status tests so each `name` uses the mandated format, keeping the existing intent but renaming them to include “it should” for the scenarios around `ValidAzureIdentityProviderConditionStatus` and `CredentialStatusValid`, `CredentialStatusInvalid`, and `CredentialStatusUnknown`.Source: Path instructions
532-577: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for
DeleteOrphanedMachines.This adds coverage only for
GetCredentialStatus. The newDeleteOrphanedMachinesfunction — the actual fix for the orphaned-machine deletion hang — has no unit test in this file (e.g. verifying finalizers are cleared only for machines with aDeletionTimestamp, thatValidcredentials skip cleanup, and that per-machine update errors are aggregated).As per path instructions, "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 `@hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure_test.go` around lines 532 - 577, The new coverage only exercises GetCredentialStatus, but the actual change in DeleteOrphanedMachines is untested. Add unit tests around DeleteOrphanedMachines in azure_test.go to verify it only clears finalizers for machines with a DeletionTimestamp, skips cleanup when GetCredentialStatus returns Valid, and aggregates per-machine update errors. Use the existing HostedCluster and machine-status setup in the Azure controller tests to locate the new behavior.Source: Path instructions
🤖 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
`@hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure.go`:
- Around line 400-411: In the AzureMachine cleanup loop, fix the misleading
status message and the reversed object identifier formatting. In the update path
inside azure.go’s Azure deletion handling, change the error construction in the
c.Update failure branch to use the conventional namespace/name order, and update
the logger message after a successful finalizer clear in the same loop so it
describes unblocking deletion rather than “skipping deletion.” Use the existing
azureMachine and logger references in this loop to keep the message accurate and
consistent with the rest of the file.
---
Nitpick comments:
In `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go`:
- Around line 452-481: The Azure status-bubbling logic in
hostedcluster_controller.go duplicates the existing AWS pattern in the same
reconcile path, so extract the shared condition-propagation flow into a helper.
Refactor the logic around ValidAWSIdentityProvider/ValidAzureIdentityProvider in
HostedClusterReconciler so a single helper handles finding the HCP condition,
setting ObservedGeneration, defaulting Unknown when missing, and persisting
status updates, with platform/condition type passed in as parameters.
In
`@hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure_test.go`:
- Around line 539-557: The test case names in azure_test.go do not follow the
required “When ... it should ...” pattern. Update the affected table-driven
cases in the Azure credential status tests so each `name` uses the mandated
format, keeping the existing intent but renaming them to include “it should” for
the scenarios around `ValidAzureIdentityProviderConditionStatus` and
`CredentialStatusValid`, `CredentialStatusInvalid`, and
`CredentialStatusUnknown`.
- Around line 532-577: The new coverage only exercises GetCredentialStatus, but
the actual change in DeleteOrphanedMachines is untested. Add unit tests around
DeleteOrphanedMachines in azure_test.go to verify it only clears finalizers for
machines with a DeletionTimestamp, skips cleanup when GetCredentialStatus
returns Valid, and aggregates per-machine update errors. Use the existing
HostedCluster and machine-status setup in the Azure controller tests to locate
the new 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: 409753bb-3406-4157-b968-095242beb2f3
⛔ Files ignored due to path filters (3)
docs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.mddocs/content/reference/api.mdis excluded by!docs/content/reference/api.mdvendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_conditions.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (10)
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.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.gohypershift-operator/controllers/hostedcluster/internal/platform/azure/azure.gohypershift-operator/controllers/hostedcluster/internal/platform/azure/azure_test.gohypershift-operator/controllers/hostedcluster/internal/platform/platform.go
✅ Files skipped from review due to trivial changes (1)
- hypershift-operator/controllers/hostedcluster/internal/platform/platform.go
🚧 Files skipped from review as they are similar to previous changes (6)
- control-plane-operator/controllers/healthcheck/azure_test.go
- cmd/cluster/azure/destroy.go
- control-plane-operator/controllers/healthcheck/healthcheck_controller.go
- api/hypershift/v1beta1/hostedcluster_conditions.go
- control-plane-operator/main.go
- control-plane-operator/controllers/healthcheck/azure.go
db44ac3 to
d5f9102
Compare
Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe codecov/patch check failed because the PR's new code achieves only 39.84% test coverage on the diff, falling short of the repository's 42.55% patch coverage target. The PR adds 438 lines across 13 files — including two significant new files ( Root CauseThe coverage gap is caused by insufficient unit test coverage across multiple new code paths introduced in this PR. Specifically:
The shortfall is approximately 7 lines (~3% of the 241 countable diff lines). The most impactful fix would be adding mock-based tests for the Azure API error branches in Recommendations
Addressing recommendation #1 alone would likely raise patch coverage above the 42.55% target. Evidence
|
d5f9102 to
da2d955
Compare
| @@ -729,3 +736,38 @@ func setupAzurePrivateControllers(ctx context.Context, mgr ctrl.Manager, hcp *hy | |||
| os.Exit(1) | |||
| } | |||
| } | |||
|
|
|||
| // resolveAzureHealthCheckCredentials returns Azure credentials for the identity | |||
| // provider health check. Returns nil if credentials cannot be obtained — the | |||
There was a problem hiding this comment.
this mentions identity provider, but in managed there's no idp involve right?
This func needs better doc and unit test for all use cases
There was a problem hiding this comment.
Done. Updated the docstring to clarify both credential paths (self-managed workload identity vs ARO HCP managed identity) and removed the "identity provider" wording. The function now documents what each path does and what happens when credentials are nil.
Unit testing resolveAzureHealthCheckCredentials directly is not practical since it creates real Azure SDK credential objects (DefaultAzureCredential / UserAssignedIdentityCredential), but the health check function that consumes its output (azureHealthCheckCredentials) is fully tested for all early-return paths (nil config, KAS unavailable, nil creds).
| ) | ||
|
|
||
| func azureHealthCheckIdentityProvider(ctx context.Context, hcp *hyperv1.HostedControlPlane, azureCreds azcore.TokenCredential) error { | ||
| if hcp.Spec.Platform.Azure == nil { |
There was a problem hiding this comment.
In managed we don't have idp hence this func naming and condition seems misleading
There was a problem hiding this comment.
Done. Renamed azureHealthCheckIdentityProvider → azureHealthCheckCredentials and setAzureIdentityCondition → setAzureCredentialCondition to avoid the misleading "identity provider" terminology for managed clusters. Also updated all condition messages to say "credentials" instead of "identity provider".
The API condition name ValidAzureIdentityProvider is kept for consistency with the AWS pattern (ValidAWSIdentityProvider), but the function godoc now explains this naming choice.
| return nil | ||
| } | ||
|
|
||
| _, err := azureutil.GetResourceGroupInfo(ctx, |
There was a problem hiding this comment.
We use this to decide if delete orphan machines, though we don't validate specific capZ perms in practice.
I know we have the same pattern/limitation in aws. Thoughts? @muraee @bryan-cox
There was a problem hiding this comment.
Correct — the Resource Group GET call validates that the credentials are functional and the infrastructure still exists, but doesn't verify specific CAPZ permissions (e.g. VM create/delete). This is the same limitation as AWS where ValidAWSIdentityProvider checks OIDC reachability but not specific IAM permissions for CAPA.
The primary goal here is detecting the "infrastructure already gone" case (RG returns 404) and "credentials completely broken" case (401/403), which are the scenarios that cause deletion to hang indefinitely. Validating fine-grained CAPZ permissions would require a more complex probe and is a separate concern.
AI-assisted response via Claude Code
da2d955 to
e855967
Compare
| // 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. | ||
| // A failure here may require external user intervention to resolve. |
There was a problem hiding this comment.
How often does this check run?
There was a problem hiding this comment.
Every 5 minutes on success, every 30 seconds on failure, same cadence as the existing AWS identity provider check. Both are driven by the shared HealthCheckUpdater ticker in healthcheck_controller.go.
There was a problem hiding this comment.
Have we seen any rate limiting issues on the AWS side from these frequent checks?
There was a problem hiding this comment.
Not that I'm aware of. The AWS check uses ec2.DescribeVpcEndpoints which is a read-only EC2 API — AWS rate limits for EC2 describe calls are generous (typically 100 req/s per account/region). At 1 call every 5 minutes per HCP, even a management cluster with hundreds of hosted clusters would be well within limits.
For Azure, the equivalent is a Resource Group GET call (resources.azure.com), which has similarly generous read limits (~250 reads/5min per subscription). Both are lightweight, read-only operations specifically chosen to avoid rate limiting concerns.
AI-assisted response via Claude Code
|
/approve for api |
|
This use case would be coverage by #8296 which relies on the machine delete condition |
e855967 to
05011f4
Compare
openshift-ci
Bot
added
the
needs-rebase
05011f4 to
6a1a329
Compare
openshift-ci
Bot
removed
the
needs-rebase
6a1a329 to
7a371d4
Compare
|
/rebase |
|
🤖 Rebasing PR onto main: workflow run |
openshift-ci
Bot
added
the
needs-rebase
|
|
…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>
7a371d4 to
ae634cf
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: JoelSpeed, vsolanki12 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 |
|
/cc @bryan-cox |
|
/test verify |
|
@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. |
4 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: