fix(cleanup): protect identity pools from expiry (AROSLSRE-1916) - #6706
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the cleanup-sweeper rg-ordered discovery policy to avoid selecting long-lived slot-managed MSI container resource groups for deletion under the generic persist=true 15-day rule, and adds a regression test plus documentation to prevent reintroducing the issue.
Changes:
- Add an ordered
skiprule to exclude resource groups whose names start witharo-hcp-msi-container-(case-insensitive) before the genericpersist=trueexpiry rule. - Add a regression test that loads the repository policy file and asserts the skip behavior and case-insensitive matching.
- Document the rationale for excluding these long-lived identity-container pools in cleanup CI docs.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tooling/cleanup-sweeper/resourcegroups.policy.yaml | Adds a new first-match skip rule to exclude identity-container pools by name regex before persist=true expiration rules. |
| tooling/cleanup-sweeper/pkg/policy/policy_test.go | Adds a regression test that loads/validates the repo policy and verifies identity-container pools are skipped while unrelated persist=true RGs still expire. |
| docs/ci/cleanup.md | Documents why identity-container pools are excluded from rg-ordered deletion candidates. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Gerd Oberlechner (@geoberle) Steve Kuznetsov (@stevekuznetsov) The |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tooling/cleanup-sweeper/pkg/policy/policy_test.go:110
- Subtest is marked t.Parallel() inside a range loop but captures the loop variable
tcby reference. This can make the assertions run with the wrong test case (flaky / incorrect) once the loop advances.
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
|
Copilot Reviewed the suppressed loop-variable note: no change is needed. This repository targets Go 1.25.7 ( |
|
/retest |
|
/test image-updater-images |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: raelga, stevekuznetsov The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test e2e-parallel Retesting: the KMS-key-rotation cluster provisioning failed with: InternalServerError: [hypershiftHostedCluster] hosted cluster has no installed version; hosted cluster degraded: UnavailableReplicas: router deployment has 1 unavailable replicas This is a guest-cluster data-plane/router provisioning issue, not related to this PR's identity-pool cleanup policy diff. The identical signature also hit #6708 (unrelated ARM-retry PR) in the same time window, confirming a shared-CI infra episode rather than a PR-specific problem. |
|
/retest |
|
/hold Revision 812783e was retested 3 times: holding |
|
/unhold |
|
Rael Garcia (@raelga): The following test failed, say
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. |
Active incident: cleanup sweeper deleted the DEV identity poolsAzure Activity Log confirms that the scheduled cleanup identity (
The deleting job is The complete DEV validation before recovery found:
This directly caused E2E run 2093225833677197312 to finish with 55 failed, 5 passed, and 40 skipped tests. At least 54 distinct shard3 resource groups were reported as missing; this is environmental fallout from the cleanup policy, not a failure caused by this three-file policy/docs/test change. Containment and recovery are in progress:
After reconciliation completes, I will run the full catalog validator, restore the cleanup bot's role assignment, remove temporary locks, and retest E2E. Merging this PR is the durable containment required before the next aged pool can be selected again. |
Recovery validation completeThe complete catalog-driven DEV validation now passes for every managed pool:
There are no missing or unexpected resource groups or identities. Every recovered group has a fresh Temporary incident containment has been fully rolled back:
The current #6706 E2E run remains in progress and has not reported any identity-pool |
|
Force-merge justification This is an exceptional green-but-blocked infrastructure fix needed to break a circular dependency: the required E2E check cannot complete reliably because the cleanup policy being fixed is actively deleting the identity pools that E2E requires. It meets all four force-merge criteria: 1. The merge queue is blocked by the problem. The four-hour This creates a circular gate: #6706 cannot merge without E2E, while E2E cannot run reliably until #6706 prevents the sweeper from deleting its prerequisites. 2. High confidence this change fixes the problem. Azure Activity Log identifies This PR adds a narrow, first-match, case-insensitive exclusion for resource-group names beginning with 3. The change builds and passes all fast testing. All non-E2E required checks are green: unit tests, integration, lint, verify, mega-linter, images, E2E images, config-change detection, GitHub analysis, and CLA. Tide is merge automation rather than a CI check. Local cleanup-sweeper tests and YAML verification also pass. 4. The change is clean and authored by the original PR author. This is a focused three-file change by Rael Garcia (@raelga): one policy exclusion, regression coverage, and the corresponding cleanup documentation. It contains no unrelated refactoring or production-service changes and has already received Temporary RBAC containment and deletion locks are currently required while the three damaged subscriptions are reconciled. Force-merging #6706 removes the deletion source, allows those temporary controls to be removed, and restores a stable environment for the required E2E retest. |
AROSLSRE-1916
What
Excludes slot-managed identity-container resource groups from
cleanup-sweeperdiscovery before the generic 15-daypersist=trueexpiryrule.
Adds regression coverage against the repository policy and documents why these
long-lived E2E pools are protected.
Why
The restored DEV shard0 pool has 300 resource groups with
persist=true, anewly stamped
createdAt, and nomanagedByparent. Without this exclusion,the four-hour cleanup sweeper would select the entire pool after 15 days and
reintroduce the
ResourceGroupNotFoundfailures recovered under AROSLSRE-1895.Testing
cd tooling/cleanup-sweeper && go test ./...GOTOOLCHAIN=go1.25.7 make verify-yamlfmtresourcegroups.policy.yamland verifies:persist=truegroup still expires after 15 daysSpecial notes for your reviewer
This changes the policy consumed by the periodic
sweeper-rg-orderedCI job.The exclusion is intentionally narrow to resource-group names beginning with
aro-hcp-msi-container-.PR Checklist
If E2E tests are included:
demonstrate that the test is able to detect a defect/error and fail with
proper error message and logs which communicates nature of the problem. - not applicable