Skip to content

fix(harbor-dev): gate teardown on deletionPolicy, verify the right Kustomization (PLT-1216) - #423

Open
bdchatham wants to merge 18 commits into
mainfrom
feat/plt-1216-harbor-dev-teardown-v2
Open

fix(harbor-dev): gate teardown on deletionPolicy, verify the right Kustomization (PLT-1216)#423
bdchatham wants to merge 18 commits into
mainfrom
feat/plt-1216-harbor-dev-teardown-v2

Conversation

@bdchatham

@bdchatham bdchatham commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

The ticket premise is false. PLT-1216 says teardown leaks disks because a prune option is missing. Prune is already enabled — platform/clusters/harbor/engineers/base/sync.yaml:14, in the shared base that renders all nine engineer reconcilers. It works correctly for everything Flux owns. This PR changes no Flux YAML, no prune setting, and nothing in the platform repo.

What actually leaks the disks: SeiNetwork.spec.deletionPolicy defaults to Retain. On deletion the reconciler deliberately strips the owner reference from the child SeiNodes rather than deleting them (sei-k8s-controller internal/controller/seinetwork/controller.go:135-143). The orphaned validators keep running, each still holding its PVC and its EBS volume. Flux prune cannot reach them — the controller created them, so they were never in Flux's inventory — and garbage collection has no owner reference left to follow. Storage-class reclaim is irrelevant, because reclaim only fires on a PVC that actually gets deleted. gp3-10k-750 and gp3 are already reclaimPolicy: Delete; nothing needs changing there.

This PR does not stop the leak. It is docs/skills only. The load-bearing fix is benchmark SeiNetworks carrying deletionPolicy: Delete, tracked separately. What this PR does is stop the harbor-dev skill from reporting a successful teardown while disks are still running, and give the operator the procedure that prevents the orphaning in the first place.

What this fixes

1. Wrong reconcile target. Teardown verification reconciled flux-system and read its lastAppliedRevision. That root Kustomization tracks sei-protocol/platform at clusters/harbor — not harbor-engineering-workspace. So teardown verified a reconciler that never touched the engineer's change and reported success regardless. Workspace merges now target Flux Kustomization <alias> in namespace eng-<alias> (verified in-repo: references/onboarding-pr.md:110, references/cluster-inspection-recipes.md recipe #8). flux-system stays correct for platform-repo merges (onboarding), and the split is now a table rather than a single command.

2. No resource-disappearance check. A successful reconcile only means Flux issued the deletes. Added an explicit poll of the SeiNetwork/SeiNodes/PVCs on a bounded budget, with the caveat stated: deletion is asynchronous and sei.io/seinode-finalizer holds a SeiNode until the controller releases its PVC. An object still Terminating past the budget is a reportable finding — stripping the finalizer to make the check pass is exactly how a stuck teardown becomes a leaked disk.

3. The deletionPolicy: Retain trap was undocumented. Now a guardrail, a procedure step, a halt condition, and a section of the new reference. deletionPolicy is mutable (no CEL XValidation, no webhook — SeiNetworkSpec carries exactly three immutability rules: spec.genesis, spec.replicas, spec.dataVolume), so an operator can patch a live SeiNetwork to Delete. The ordering is the load-bearing part: the policy change must land in git and reconcile before the removal merges. A bare kubectl patch on a Flux-owned SeiNetwork is drift — the rendered manifest normally carries the server-defaulted Retain, so Flux owns the field and reverts the patch on its next pass, typically while the removal PR is in review. And once a Retain deletion has stripped the owner references and removed the parent, nothing restores the cascade at all.

4. No "destroy my namespace" procedure. Added, and honest about the boundary: a workspace PR removes the engineer's workloads only. The Namespace object, the three ServiceAccounts, the RBAC, and the Flux Kustomization itself are platform-owned and stay. So does anything created outside git — an escape-hatch apply, or an orphan from an earlier Retain teardown.

5. Teardown skipped the PR contract. Spinup goes through a PR; teardown now does too — branch, commit, gh pr create, merge, reconcile, verify.

6. No cleanup path for already-leaked resources. Added. Orphan signature: sei.io/role=validator and no ownerReferences and no live parent. Absence of owner references alone is not the signal — a follower from seictl node apply is a top-level object and legitimately has none. Cleanup is kubectl delete seinode on the confirmed orphan: its finalizer deletes the PVC and the Delete reclaim policy releases the volume. Disk signals are candidates, never verdicts — EC2 available means unattached, not unowned, and a volume backing a live Bound PVC reads available the moment its workload stops. Ownership resolves by walking volume ID → PV (spec.csi.volumeHandle) → claimRef → PVC → workload; any hop that is Forbidden, errors, or finds nothing escalates as UNRESOLVED, never as confirmed-safe. This skill never deletes an EBS volume.

Explicitly not done

  • No Flux YAML, no prune setting, no platform-repo change. Prune is correct.
  • No delete on persistentvolumeclaims added to the per-engineer Role. That restriction is deliberate: the controller's finalizer owns PVC lifecycle, PVCs are not in the workspace repo so prune never targets them, and widening the Role would be a security regression that does not fix this bug. The new reference says so in place, so the next reader does not re-propose it.
  • No controller Go changes. No storage-class changes.

Files

File Change
references/teardown.md New. The canonical teardown reference: the Retain trap and patch-before-delete ordering, the PR-based procedure, the reconcile target + disappearance poll, the namespace procedures, the leaked-resource sweep, and seven halt conditions.
SKILL.md Post-merge reconciliation retargeted with a repo→Kustomization table; Guardrail #10; Procedure: tear down (PR-based); three halt conditions; teardown + namespace intent rows; reference-index entry; step-11/step-16 pointers.
references/cluster-inspection-recipes.md Recipes #9 (disappearance poll, stuck-Terminating read) and #10 (orphan detection); recipe #8 states it is the target for workspace merges; bench:teardown reconciles <alias> and confirms the objects went away.
references/seinetwork-crd.md deletionPolicy documented as the disk-leak field: the orphan mechanism, mutability, the closing window, and what the Delete cascade does end to end.
references/ephemeral-chain-flow.md Step 14 gates on deletionPolicy before the git rm.
references/comparative-bench.md Step 16, the single-teardown bullet, and the half-comparison halt: both SeiNetworks need the gate.
references/preflight.md Gate 2b's rationale names the right target per repo.
references/troubleshooting-seinode.md The finalizer-strip override says what it costs; the Retain note distinguishes forensics from a leak.
README.md Teardown is PR-based, and the skill gates on the field that leaks disks.
evals/evals.json Two evals: the Retain halt (empty ≠ safe; forbids the prune/reclaim/Role "fixes") and a clean teardown happy path.

Verification

Docs/skills only — no build, no module compile.

  • The teardown verifier distinguishes API failure from verified absence. It reports exactly one of GONE / PRESENT / UNVERIFIED, capturing kubectl's exit status separately rather than counting lines from a pipeline. A Forbidden or dropped connection is UNVERIFIED, never a pass. The poll deadline is arithmetic on date +%s; Bash's SECONDS is unset under sh, where the comparison fails with Illegal number and the loop never runs. Both behaviours were reproduced under this host's /bin/sh before and after the fix.
  • writing/scripts/lint.sh (Vale 3.17.1, after vale sync): 0 errors on every added line (530 added lines across 9 changed .md files). Warnings remain (sentence length, passive voice) at the same density as the surrounding file; CI's fail_level is error.
  • xreview/scripts/skill-package-checks.sh --skill-dir .claude/skills/harbor-dev: no block-severity failure. Same two pre-existing non-block failures as main (T2 info: missing state/.gitkeep; A1 warn: time-sensitive content in SKILL.md/preflight.md/seictl-cli.md). B1 passes — SKILL.md is 396 lines against the 500 limit.
  • writing/scripts/check-{verifiers,artifact-length,coverage,contract-anchors,anchor-authorities,generated-rules,admission,consumer-scoping,template-deltas}.sh: all pass.
  • scripts/verify-references.sh: no error-class finding. scripts/sync-skills.sh --verify: catalog coverage OK. scripts/tests/skill-package-checks.test.sh: 47 passed, 0 failed.
  • evals.json parses; the diff is 49 insertions, 0 deletions.

No command in this PR was run against a live cluster — none was available. Every documented invocation is derived from the repo's existing conventions.

Inferred rather than verified

  • --with-source on the workspace Kustomization. The GitRepository it references comes from the platform-repo base (clusters/harbor/engineers/base/sync.yaml), which is not in this repo, so its namespace is unverified. If it sits outside eng-<alias>, the engineer's namespace-scoped Role returns Forbidden. The docs state that failure mode and the fallback (reconcile the Kustomization alone) rather than asserting the flag always works.
  • EBS volume tag keys. kubernetes.io/created-for/pvc/namespace and .../pvc/name are the EBS CSI driver's own convention, not something these repos set. The doc tells the operator to confirm the keys against a known-live volume before trusting an empty filter result.
  • The offboarding file list. Derived by mirroring the verified onboarding four-file shape in references/onboarding-pr.md; no worked offboarding PR exists to check it against. Marked as such in place, and routed to the platform team rather than presented as a self-service runbook.

The Kustomization name for fix 1 was verified, not inferred<alias> in eng-<alias>, from references/onboarding-pr.md:110 and recipe #8.

🤖 Generated with Claude Code


Fix round — cross-review findings addressed

All three P1s, the P2, and the three non-blocking items are fixed on this branch. Commits 85cca82, f470156, a65e1b3, 8b9ffea.

# Finding Fix
P1-1 A failed API read reported success — kubectl | wc -l takes wc's exit status, so a Forbidden yielded zero lines and printed "all objects gone" The poll captures kubectl's status separately and reports one of GONE / PRESENT / UNVERIFIED, with distinct exit codes 0/1/2. UNVERIFIED is a halt condition. Reproduced the false pass under /bin/sh, then reproduced all three outcomes correctly after the fix.
P1-2 The live-patch fast path is revertible — Flux reconciles Retain from git back over the patch, typically mid-review Git-first is now the documented default: a policy PR, merged and reconciled, verified on both the committed file and the live object before the removal PR merges. The live patch survives only as a repair for a SeiNetwork no reconcile owns, and carries the re-verify-immediately-before-merge requirement. Propagated to SKILL.md, seinetwork-crd.md, ephemeral-chain-flow.md, comparative-bench.md, and both evals.
P1-3 available / Used By: <none> treated as proof of garbage, routing live disks into a deletion escalation Both demoted to candidate signals. Added an ownership walk (volume ID → PV spec.csi.volumeHandleclaimRef → PVC → workload) and a verdict table whose every failed hop lands on UNRESOLVED. kubectl get pv is cluster-scoped, so Forbidden is called out as the normal engineer case and an unresolved result. This skill now never deletes an EBS volume.
P2-4 Bash SECONDS in an sh fence skips the poll; PVC and bench checks were single reads; PVC expectation contradicted the imported-PVC exception Portable date +%s deadline. poll_gone in recipe #9 is reused for CRs, PVCs, pods, and bench Jobs/ConfigMaps — bench now polls pods too, since a Job can be gone while its pod is Terminating. The PVC expectation is no longer zero: controller-managed PVCs must go, imported PVCs must survive, and inventory step 2 now records which nodes carry spec.import because nothing says so after they are deleted.
NB Unsupported namespace-cascade claim Restated as an operational preference. The namespace controller does remove namespaced resources; emptying first is about keeping the gate, the poll, and the sweep usable while the objects still exist.
NB Non-Git resources acknowledged but given no cleanup path Added a table covering direct-applied CRs, SeiNodeTaskWorkflow, hand-applied bench Jobs, controller-managed PVCs, and S3 artifacts — each with an escalation or a guarded path. Anything not in it escalates as unresolved.
NB Forbidden on --with-source stated as diagnostic Now "may indicate" the cross-namespace GitRepository, alongside expired session, missing access entry, or a Role without the Flux verbs. Read the message first.
NB "run without --filters" would enumerate other tenants' volumes Replaced with --volume-ids <known-live-id>, scoped to one volume the operator already knows.

Also corrected, and it was wrong in the original body as well as the doc: spec.resources was listed among the immutable SeiNetworkSpec fields. There are exactly three immutability rules — spec.genesis, spec.replicas, spec.dataVolume — and SeiNetworkSpec has no resources field. Removed from both.

Untouched by design, per the review: the reconcile target (A), PR parity (E), the orphan-detection triple, the legitimate flux-system command in preflight.md, and the three inference caveats.

Gates after the fix round

  • Vale: 0 errors on all 530 added lines across 9 changed .md files.
  • skill-package-checks.sh: 20 pass / 2 fail / 4 skipped — the same two non-block failures as main (T2 info, A1 warn), no block failure. B1 passes at 396/500 lines.
  • All 9 writing/scripts/check-*.sh: pass. verify-references.sh, sync-skills.sh --verify, skill-package-checks.test.sh (47/0): pass.
  • evals.json parses; internal anchors resolve.

Round 3 — cross-review findings addressed

All nine fixed. Commits dfc6d90, 4b2e6ab, 95b3f7c, 45a623c. No finding was disputed; findings 1, 2 and 4 were reproduced or re-derived locally before fixing.

# Finding Fix
1 (block) teardown.md re-implemented the poll twice instead of calling the shared poll_gone, and the copies had already diverged — the PVC loop emitted no verdict at all, just counts until its deadline Both call sites now invoke the shared functions. Recipe #9 is the single implementation and says so.
2 (block) UNVERIFIED still exited 0 — break succeeds, so the completed while returned 0, and no caller checked poll_gone's return code either Added record/VERDICT aggregation where the worst outcome wins (0 GONE < 1 PRESENT < 2 UNVERIFIED) and every call site records. Confirmed under dash: break-after-UNVERIFIED does exit 0; after the fix, UNVERIFIED then GONE aggregates to 2.
3 (block) The "engineer insists" exception let a pre-merge re-read stand in for a committed policy Exception deleted for Flux-owned networks, with the four-step losing sequence written out — a pre-merge read narrows the window but does not order against Flux's reconcile. Path B is now a stacked draft PR; a merged PR cannot carry a later removal commit.
4 (block) .spec.import does not exist — the field is .spec.dataVolume.import.pvcName, so the query matched nothing and every imported claim was reclassified as one that must disappear Inventory rewritten: real node→claim identities from the pods' own claimName, imported vs controller-managed classification, abort on any API or parse failure. Poll named managed claims; assert imported ones with expect_present. Field path caveated as repo-main API inspection, not the deployed CRD. Also corrected the three pre-existing spec.import prose mentions elsewhere in the skill — see "scope call" below.
5 EBS walk: no PVC→node relationship; kubectl-into-jq hid API failures; missing-claim rows read as plain candidates Hop 3 resolves the claim to the mounting pod and its owner reference; an empty result is unresolved, not unowned. Every hop separates the API call from the parse and checks both. Missing-claim and no-PV rows are now explicitly unresolved candidates requiring platform review.
6 Scope guards: cluster-wide claimRef followed without a namespace check; deletes without namespace/context; a failed grep read as "no manifest" Added the claim_ns != eng-<alias> escalation gate; every delete names --context harbor and -n eng-<alias>; the authorizing search runs against a freshly fetched clone and branches on grep's exit status — only 1 (no match) authorizes a delete, 2+ is unresolved. Verified the three exit codes.
7 sed -n '/Used By/,+3p' is a GNU extension; blanket POSIX claim That parse is gone (replaced by the hop-3 jq). The claim now reads "portable shell (dash, ash, bash)" and names date +%s as a deliberate near-universal extension rather than POSIX.
8 Evals preserved the unsafe exception and let the happy path proceed on live Delete alone Both scenarios now require committed Delete for a Flux-owned object; the live-patch forbidden signal no longer has an escape clause; the happy path requires named-claim polling with aggregation.
9 --with-source fallback claimed to help "whatever the cause" Now: it helps only the cross-namespace-source case, repairs nothing for expired credentials or a missing permission, and the fallback itself must succeed or you have no reconcile at all.

One extra defect found while fixing 4. jq's unique sorts by codepoint; comm assumes its input is sorted the way the locale collates, and a locale that ignores punctuation orders hyphenated claim names differently. The inventory script now pins LC_ALL=C.

Scope call to flag. Finding 4 was scoped to teardown.md, but the same wrong path appeared in three pre-existing prose lines in troubleshooting-seinode.md and one in seinetwork-crd.md. Leaving them would have made the skill contradict itself on an API path, so they were corrected to spec.dataVolume.import — prose only, no query. Revert if you would rather keep that out of this PR.

Gates after round 3

  • Vale: 0 errors on all 669 added lines across 9 changed .md files (one write-good.ThereIs introduced and fixed mid-round).
  • skill-package-checks.sh: 20 pass / 2 fail / 4 skipped — the same two non-block failures as main (T2 info, A1 warn). B1 passes at 396/500.
  • All 9 writing/scripts/check-*.sh pass. verify-references.sh, sync-skills.sh --verify, skill-package-checks.test.sh (47/0) pass. evals.json parses; internal anchors resolve.
  • Not executed: the jq filters. This host has no jq, and installing one was out of budget — they are reviewed by inspection only. The shell control flow around them (aggregation, expect_present, set -eu abort on a redirected failure, grep exit codes, comm) was executed under /bin/sh.

Loading
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