Fix Helm cleanup ordering for AKS destroy - #19243
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 19243Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 19243" |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Adds AKS destroy ordering so Kubernetes cleanup acquires credentials before Helm/cert-manager teardown and Azure deletion runs last.
Changes:
- Tags and orders Kubernetes cleanup steps per AKS environment.
- Adds destroy-specific credential acquisition.
- Expands dependency-graph and deployment E2E coverage.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
AzureKubernetesInfrastructureTests.cs |
Tests destroy dependency graphs. |
AksWithHelmChartDeploymentTests.cs |
Tests destroy with an empty kubeconfig. |
KubernetesHelmChartExtensions.cs |
Tags external-chart cleanup. |
HelmDeploymentEngine.cs |
Defines destroy tags for Helm cleanup. |
CertManagerExtensions.cs |
Tags issuer cleanup. |
AzureKubernetesEnvironmentResource.cs |
Orders credentials, cleanup, and Azure deletion. |
AzureKubernetesEnvironmentResource.AksPipeline.cs |
Adds destroy credential acquisition. |
Suppressed comments (2)
src/Aspire.Hosting.Azure.Kubernetes/AzureKubernetesEnvironmentResource.AksPipeline.cs:312
- This call cannot complete in a destroy-only graph. Pipeline step creation initializes this resource's
ProvisioningTaskCompletionSource, but the new credential step deliberately does not depend on the provisioning step;GetAksCredentialsAsyncthen callsNameOutputReference.GetValueAsync, which waits on that never-completed task. Read/hydrate the saved AKSnameoutput directly for destroy (without awaiting provisioning) and pass it into the credential routine.
await GetAksCredentialsAsync(context).ConfigureAwait(false);
src/Aspire.Hosting.Azure.Kubernetes/AzureKubernetesEnvironmentResource.AksPipeline.cs:304
- Azure state can still exist after the resource-group delete has started because that step uses
WaitUntil.Started, and the original Helm failure leaves overall state uncleared. On a retry after AKS has disappeared, this check enters credential acquisition and fails before the idempotent Azure destroy step can report that the group is already gone. Treat a missing AKS cluster/resource group as a successful no-op in the destroy credential step so partial-teardown retries can finish.
if (string.IsNullOrEmpty(resourceGroupName) || string.IsNullOrEmpty(subscriptionId))
PR Testing ReportPR Information
Artifact Version Verification
The CLI was installed from the PR's Changes AnalyzedFiles Changed
Change Categories
Test Scenarios ExecutedScenario 1: PR artifact identity and fresh AKS AppHostObjective: Verify that testing uses the artifact containing the PR changes and that a fresh AppHost can consume the PR AKS package. Coverage Type: Artifact and build validation Status: Passed Steps:
Evidence:
Observations:
Scenario 2: Safe AKS destroy without deployment stateObjective: Verify that a never-deployed AKS AppHost can be destroyed safely without ambient Kubernetes credentials or Azure deployment state. Coverage Type: Unhappy path Status: Passed Steps:
Evidence:
Expected Unhappy-Path Outcome: Credential acquisition, Helm teardown, and Azure deletion should each recognize missing persisted state and complete safely. Observations:
Scenario 3: AKS cluster-cleanup dependency graphObjective: Verify the dogfood artifact exposes the intended destroy topology for the main release, an opted-in external Helm chart, and cert-manager resources. Coverage Type: Boundary and topology validation Status: Passed Steps:
Evidence:
Observations:
Scenario 4: Focused source regression suitesObjective: Verify the PR source passes the affected Azure Kubernetes and Kubernetes hosting test projects. Coverage Type: Automated regression Status: Passed Steps:
Evidence:
Observations:
Scenario Not ExecutedLive AKS deploy and destroy with empty kubeconfigStatus: Not run This scenario provisions billable Azure resources and requires explicit approval. No user was available to approve cloud provisioning, so testing stopped short of the live deployment. The updated deployment E2E test remains the appropriate end-to-end validation for this path. Summary
Overall ResultPARTIALLY VERIFIED All non-provisioning artifact and source scenarios passed. The live AKS deployment scenario is still required to verify the original failure against a real cluster with an empty ambient kubeconfig. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a5fbe5fe-6726-431d-af32-692b54aa1c2b
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a5fbe5fe-6726-431d-af32-692b54aa1c2b
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/Aspire.Hosting.Azure.Kubernetes/AzureKubernetesEnvironmentResource.AksPipeline.cs:344
- When persisted AKS state is incomplete, returning here leaves
KubernetesEnvironment.KubeConfigPathunset, but all tagged cleanup steps still run. External chart uninstall falls back to its configured release name, and cert-manager issuer deletion always invokeskubectl, so both will use the caller's ambient kubeconfig and can delete resources from an unrelated cluster—the behavior this change is intended to eliminate. Fail this prerequisite (or otherwise prevent the downstream cleanup steps from executing) instead of continuing without an isolated kubeconfig.
context.Logger.LogInformation(
"No complete Azure deployment state found for AKS environment '{EnvironmentName}'. Skipping credential acquisition.",
Name);
return;
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
James Newton-King (JamesNK)
left a comment
James Newton-King (JamesNK)
left a comment
There was a problem hiding this comment.
Two destroy-path correctness issues remain at 3bb5670509: incomplete persisted state can send cleanup to the ambient Kubernetes context, and adopted AKS resources ignore their persisted resource-specific Azure scope. The existing --clear-cache E2E coverage issue is already tracked in a separate thread and was not duplicated.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a5fbe5fe-6726-431d-af32-692b54aa1c2b
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a5fbe5fe-6726-431d-af32-692b54aa1c2b
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
src/Aspire.Hosting.Azure.Kubernetes/AzureKubernetesEnvironmentResource.AksPipeline.cs:345
- The destroy path always uses the app-wide Azure subscription/resource group, but AKS can target an existing cluster in a different scope via
AsExistingInResourceGrouporScope. In that supported scenario, credential acquisition now targets the app resource group (or skips when global Azure state is absent) instead of the cluster scope, so Helm cleanup still fails or reaches the wrong cluster. Apply the same explicit-scope precedence used by the deploy credential path, while using persisted state only as its fallback.
var resourceGroupName = stateSection.Data["ResourceGroup"]?.ToString();
var subscriptionId = stateSection.Data["SubscriptionId"]?.ToString();
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a5fbe5fe-6726-431d-af32-692b54aa1c2b
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/Aspire.Hosting.Azure.Kubernetes/AzureKubernetesEnvironmentResource.AksPipeline.cs:428
- A malformed persisted
Scopeaborts the credential prerequisite, which also prevents the dependentdestroy-azure-*step from deleting the resource group.Scopeis optional cached state elsewhere, and the already-validatedclusterResourceIdcontains the exact subscription and resource group, so use it as the safe fallback instead of blocking aggregate destroy.
catch (Exception ex) when (ex is not OperationCanceledException)
{
throw new InvalidOperationException(
$"The Azure deployment state for AKS environment '{Name}' contains an invalid scope.",
ex);
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Mitch Denny (mitchdenny)
left a comment
Mitch Denny (mitchdenny)
left a comment
There was a problem hiding this comment.
Looks good — the destroy ordering fix is sound: tagging cluster-scoped cleanup steps and making destroy-azure-* depend on them is the right shape, and the destroy-only credential step correctly avoids pulling provisioning into the destroy graph.
Three non-blocking follow-ups:
AzureKubernetesEnvironmentResource.csuses.Single()for theAzureEnvironmentResourceand thedestroy-azure-*step. Every other call site in the repo usesSingleOrDefault/FirstOrDefaultwith a guard — safe in practice sinceAddAzureProvisioningalways adds it, but a mis-shaped model would surface an opaque "Sequence contains no elements".AksResourceExistsAsyncthrows on any nonzeroaz resource listexit. If the resource group itself was deleted out-of-band, that'sResourceGroupNotFound, which fails the credential step and blocks the entire destroy including Azure cleanup. Probably worth treating a missing RG the same as a missing cluster.- The E2E test dropped the
helm list -n podinfoassertion, so destroy is now only verified by pipeline success. Understandable given the cluster is gone by then, but it's lost coverage on theWithDestroy()contract.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a5fbe5fe-6726-431d-af32-692b54aa1c2b
|
Addressed the three follow-ups from review 5108517925 in c303691:
The Azure Kubernetes suite passes 110/110, and the deployment E2E project builds with no warnings or errors. |
Tests selector5 / 99 PR test projects · 2 PR jobs · 2 advisory-only targets, from 14 changed files. Selected PR test projects (5 / 99)
Selected PR jobs (2)
Advisory workflow impact (2)
How these were chosen — grouped by what changed🔧 🔧 🔧 🔧 🔧 🔧 🧪 🧪 🧪 🧪 🧪 🧪 🧪 Job reasons
Selection computed for commit |
There was a problem hiding this comment.
🟢 Approval recommended
The destroy graph, persisted-state handling, retry behavior, and real deployment scenario are comprehensively covered.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Documents the new destroy-specific AKS credential acquisition from microsoft/aspire#19243, which lets 'aspire destroy' tear down AKS deployments without relying on the caller's ambient kubectl context. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Pull request created: #1620
|
|
📝 Documentation has been drafted in microsoft/aspire.dev#1620 targeting Added a "Clean up resources" section to the AKS deployment doc covering
Note This draft PR needs human review before merging. |
|
The CI build failed due to test failure(s) that appear unrelated to the PR changes. These may be flaky tests. Suspected flaky test(s):
Suggested actions:
You can re-run the failed jobs from the workflow run page. |
Description
aspire destroycould fail for AKS deployments when the caller did not already have the cluster selected in their ambient kubeconfig. Helm cleanup ran without first acquiring AKS credentials, while Azure resource-group deletion could start concurrently and race the cluster teardown.This change adds a destroy-specific AKS credential step that stays separate from the provisioning-dependent deploy credential step. Kubernetes cleanup steps are tagged per environment so the pipeline can order the main Helm release, opted-in external charts, and cert-manager resources after credential acquisition. Azure resource-group deletion now waits for all cluster cleanup to finish. Credential acquisition uses persisted Azure state so retries after partial teardown continue to target the correct cluster.
User-facing usage
AKS deployments can now be destroyed without relying on the caller's current Kubernetes context:
KUBECONFIG="$(mktemp)" aspire destroy \ --apphost MyApp.AppHost/MyApp.AppHost.csproj \ --environment my-environment \ --non-interactive \ --yesThe regression coverage verifies the complete destroy dependency graph, including multiple AKS environments, and the deployment scenario replaces its ambient kubeconfig before running destroy.
Fixes: #19206
Checklist
<remarks />and<code />elements on your triple slash comments?