feat: reconcile retired Istio gateway leases post-upgrade - #322
feat: reconcile retired Istio gateway leases post-upgrade#322Trevor Williams (trevorwilliams2025) wants to merge 3 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: trevorwilliams2025 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 |
|
Hi Trevor Williams (@trevorwilliams2025). Thanks for your PR. I'm waiting for a Azure member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain 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. |
There was a problem hiding this comment.
Pull request overview
Adds post-upgrade cleanup for orphaned AKS Istio gateway leases once the mesh is stable.
Changes:
- Adds lease discovery, filtering, deletion, and
NotFoundhandling. - Integrates cleanup into upgrade and reconciliation flows.
- Adds tests and updates workspace checksums.
upgrade.goshould log ARM stability-read failures and skip cleanup rather than failing otherwise successful flows.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
tools/istio-upgrade/pkg/istio/upgrade.go |
Integrates stable-state lease reconciliation. |
tools/istio-upgrade/pkg/istio/upgrade_test.go |
Updates reconciliation test calls. |
tools/istio-upgrade/pkg/istio/leases.go |
Implements retired gateway lease cleanup. |
tools/istio-upgrade/pkg/istio/leases_test.go |
Tests lease cleanup and stability gating. |
go.work.sum |
Updates workspace dependency checksums. |
Suppressed comments (5)
tools/istio-upgrade/pkg/istio/leases_test.go:86
- The stability gate has four independent conditions, but this test exercises only
UpgradeInProgress. A regression in provisioning state, revision count, or the installed-revision/target match could then permit deletion without being caught; add cases for each remaining condition and assert the lease is preserved.
t.Run("skips while mesh is not stable", func(t *testing.T) {
ctx := logr.NewContext(context.Background(), testr.New(t))
client := fake.NewSimpleClientset(
&corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: istioSystemNamespace}},
gatewayLease("istio-gateway-deployment-asm-1-28"),
)
err := reconcileRetiredGatewayLeases(
ctx,
logr.FromContextOrDiscard(ctx),
&fakeAKSClient{
clusterInfo: &ClusterInfo{ProvisioningState: "Succeeded"},
meshProfile: &MeshProfile{Revisions: []string{"asm-1-29"}},
upgradeInfo: &MeshUpgradeInfo{UpgradeInProgress: true},
},
NewKubeClientFromInterface(client),
DefaultUpgradeOptions(),
"asm-1-29",
tools/istio-upgrade/pkg/istio/leases_test.go:67
- The tests cover only successful deletes, not the explicit idempotency and failure branches below. Add fake-client reactors for a delete that races and returns
NotFound, plus a non-NotFound delete/list error, so the benign handling and error propagation cannot regress.
func TestReconcileRetiredGatewayLeases(t *testing.T) {
t.Run("deletes only retired gateway lease formats", func(t *testing.T) {
client := fake.NewSimpleClientset(
&corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: istioSystemNamespace}},
gatewayLease("istio-gateway-deployment-asm-1-28"),
gatewayLease("istio-gateway-status-leader-asm-1-28"),
gatewayLease("istio-gateway-deployment-asm-1-29"),
gatewayLease("some-other-lease"),
)
err := ReconcileRetiredGatewayLeases(
context.Background(),
NewKubeClientFromInterface(client),
[]string{"asm-1-29"},
)
require.NoError(t, err)
_, err = client.CoordinationV1().Leases(istioSystemNamespace).Get(
context.Background(), "istio-gateway-deployment-asm-1-28", metav1.GetOptions{})
assert.True(t, apierrors.IsNotFound(err))
_, err = client.CoordinationV1().Leases(istioSystemNamespace).Get(
context.Background(), "istio-gateway-deployment-asm-1-29", metav1.GetOptions{})
require.NoError(t, err)
_, err = client.CoordinationV1().Leases(istioSystemNamespace).Get(
context.Background(), "some-other-lease", metav1.GetOptions{})
require.NoError(t, err)
})
tools/istio-upgrade/pkg/istio/leases_test.go:44
- The test creates the retired
status-leaderlease but never verifies that it is deleted; an implementation that failed to match this second supported format would still pass. Add anIsNotFoundassertion for that lease so the advertised support for both formats is actually covered.
gatewayLease("istio-gateway-status-leader-asm-1-28"),
tools/istio-upgrade/pkg/istio/upgrade.go:694
- This branch has the same non-fatality problem: a transient failure listing upgrade targets turns an otherwise completed upgrade into an error. Since cleanup is optional, log the failed stability check and return
nil(without deleting leases) instead of propagating it to the upgrade pipeline.
return fmt.Errorf("get Istio upgrade state before retired lease reconciliation: %w", err)
tools/istio-upgrade/pkg/istio/upgrade.go:702
- The new stability test exercises only
UpgradeInProgress; it does not cover the other three independent guards here (ProvisioningState, exactly one installed revision, and revision matchingtarget). These are safety-critical because a regression could delete leases during a provisioning transition or while the configured revision is not the installed one, so add table-driven cases that assert the retired lease remains for each condition.
if clusterInfo.ProvisioningState != "Succeeded" ||
upgradeInfo.UpgradeInProgress ||
len(meshProfile.Revisions) != 1 ||
meshProfile.Revisions[0] != target {
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| opts.ClusterName, | ||
| ) | ||
| if err != nil { | ||
| return fmt.Errorf("get mesh state before retired lease reconciliation: %w", err) |
There was a problem hiding this comment.
noted, will correct once local testing completes.
Summary
Add automatic cleanup of orphaned AKS-managed Istio gateway leader-election
leases after mesh upgrades complete and reach a stable state.
Addresses: Azure/AKS#5862
Problem
During Istio mesh upgrades, AKS leaves behind orphaned gateway leader-election
leases (
istio-gateway-deployment-asm-*,istio-gateway-status-leader-asm-*)for retired revisions. These leases are harmless but accumulate and clutter the
cluster. Until AKS adds proper garbage collection via owner references, we need
to clean them up ourselves.
Solution
Post-upgrade reconciliation that safely deletes retired gateway leases by:
ProvisioningState == "Succeeded")NotFounderrors as benign (idempotent operation)Changes
ReconcileRetiredGatewayLeases()functionrunReconcile()(no upgrade needed)runCanaryPostInstall()(after canary completes)runCleanupAndUpgrade()(after cleanup verification)Testing
Deployment