Skip to content

Fix cleanup deletion ordering - #228

Open
almaslennikov wants to merge 1 commit into
mainfrom
dev/clean-delete-all-first
Open

Fix cleanup deletion ordering#228
almaslennikov wants to merge 1 commit into
mainfrom
dev/clean-delete-all-first

Conversation

@almaslennikov

Copy link
Copy Markdown
Collaborator

Summary

  • combine namespaced and cluster-scoped cleanup targets into one deletion set
  • send every deletion request before polling any CR for finalizer completion
  • preserve the same delete-all-then-wait ordering during cleanup re-sweeps
  • document the cleanup ordering in the user guide, CLI reference, README, and bundled skill

Why

The previous flow deleted and waited for namespaced CRs before signaling cluster-scoped CRs. A finalizer on an early CR could wait for a later CR that had not entered deletion, leaving l8k clean stuck indefinitely.

The regression test models that dependency: the first CR remains blocked until a cluster-scoped CR receives its deletion request.

Verification

  • go test -race -count=1 ./...
  • go build ./...
  • golangci-lint v2.11.0 run ./...
  • mkdocs build --strict --clean
  • cleanup skill validation

@greptile-apps

greptile-apps Bot commented Aug 19, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes cleanup ordering so all namespaced and cluster-scoped custom resources receive deletion requests before finalizer polling begins.

  • Combines both resource scopes into a single deletion set.
  • Preserves delete-all-then-wait ordering during re-sweeps.
  • Adds regression coverage for cross-resource finalizer dependencies.
  • Aligns user, CLI, README, and agent-skill documentation with the new behavior.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
pkg/networkoperatorplugin/clean.go Combines namespaced and cluster-scoped resources so every deletion request is issued before polling, including during re-sweeps.
pkg/networkoperatorplugin/clean_test.go Adds regression coverage proving a namespaced finalizer can depend on a cluster-scoped resource receiving its deletion request.
docs/user/cleanup.md Documents the combined discovery, delete-all, wait, and re-sweep sequence.
skills/k8s-launch-kit-clean/SKILL.md Updates agent-facing cleanup instructions to reflect delete-all-then-wait ordering.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Discover namespaced CRs] --> C[Combine deletion set]
  B[Discover cluster-scoped CRs] --> C
  C --> D[Send every deletion request]
  D --> E[Poll resources for finalizer completion]
  E --> F{Any resources discovered on re-sweep?}
  F -->|Yes| D
  F -->|No| G[Uninstall Helm release]
Loading

Reviews (2): Last reviewed commit: "Delete all CRs before cleanup waits" | Re-trigger Greptile

Comment thread pkg/networkoperatorplugin/clean.go Outdated
Combine namespaced and cluster-scoped cleanup targets into one deletion
pass before polling for finalizer completion. This prevents a CR from
blocking cleanup while it waits for another CR that has not yet received
a deletion request.

Apply the same ordering to cleanup re-sweeps and document the
behavior.

Signed-off-by: Alexander Maslennikov <amaslennikov@nvidia.com>
@almaslennikov
almaslennikov force-pushed the dev/clean-delete-all-first branch from 5ca9f76 to b68ed69 Compare August 19, 2026 16:14
@almaslennikov

Copy link
Copy Markdown
Collaborator Author

Addressed the Greptile finding (force-pushed b68ed69):

  1. Later reads abort active cleanup — restored the original conservative polling behavior, which stops the current poll as soon as it finds a CR still present. This avoids exposing a transient Get error from a later CR while preserving the new invariant that the complete combined deletion set is signaled before polling begins.

Verification: go test -race -count=1 ./... and golangci-lint v2.11.0 run ./... pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant