fix(harbor-dev): gate teardown on deletionPolicy, verify the right Kustomization (PLT-1216) - #423
Open
bdchatham wants to merge 18 commits into
Open
fix(harbor-dev): gate teardown on deletionPolicy, verify the right Kustomization (PLT-1216)#423bdchatham wants to merge 18 commits into
bdchatham wants to merge 18 commits into
Conversation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The ticket premise is false. PLT-1216 says teardown leaks disks because a
pruneoption 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, noprunesetting, and nothing in the platform repo.What actually leaks the disks:
SeiNetwork.spec.deletionPolicydefaults toRetain. On deletion the reconciler deliberately strips the owner reference from the child SeiNodes rather than deleting them (sei-k8s-controllerinternal/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-750andgp3are alreadyreclaimPolicy: 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-systemand read itslastAppliedRevision. That root Kustomization trackssei-protocol/platformatclusters/harbor— notharbor-engineering-workspace. So teardown verified a reconciler that never touched the engineer's change and reported success regardless. Workspace merges now target FluxKustomization <alias>in namespaceeng-<alias>(verified in-repo:references/onboarding-pr.md:110,references/cluster-inspection-recipes.mdrecipe #8).flux-systemstays 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-finalizerholds a SeiNode until the controller releases its PVC. An object stillTerminatingpast 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: Retaintrap was undocumented. Now a guardrail, a procedure step, a halt condition, and a section of the new reference.deletionPolicyis mutable (no CELXValidation, no webhook —SeiNetworkSpeccarries exactly three immutability rules:spec.genesis,spec.replicas,spec.dataVolume), so an operator can patch a live SeiNetwork toDelete. The ordering is the load-bearing part: the policy change must land in git and reconcile before the removal merges. A barekubectl patchon a Flux-owned SeiNetwork is drift — the rendered manifest normally carries the server-defaultedRetain, so Flux owns the field and reverts the patch on its next pass, typically while the removal PR is in review. And once aRetaindeletion 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
Namespaceobject, 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 earlierRetainteardown.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=validatorand noownerReferencesand no live parent. Absence of owner references alone is not the signal — a follower fromseictl node applyis a top-level object and legitimately has none. Cleanup iskubectl delete seinodeon the confirmed orphan: its finalizer deletes the PVC and theDeletereclaim policy releases the volume. Disk signals are candidates, never verdicts — EC2availablemeans unattached, not unowned, and a volume backing a liveBoundPVC readsavailablethe moment its workload stops. Ownership resolves by walking volume ID → PV (spec.csi.volumeHandle) →claimRef→ PVC → workload; any hop that isForbidden, errors, or finds nothing escalates as UNRESOLVED, never as confirmed-safe. This skill never deletes an EBS volume.Explicitly not done
prunesetting, no platform-repo change. Prune is correct.deleteonpersistentvolumeclaimsadded 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.Files
references/teardown.mdRetaintrap 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.mdProcedure: tear down (PR-based); three halt conditions; teardown + namespace intent rows; reference-index entry; step-11/step-16 pointers.references/cluster-inspection-recipes.mdTerminatingread) and #10 (orphan detection); recipe #8 states it is the target for workspace merges;bench:teardownreconciles<alias>and confirms the objects went away.references/seinetwork-crd.mddeletionPolicydocumented as the disk-leak field: the orphan mechanism, mutability, the closing window, and what theDeletecascade does end to end.references/ephemeral-chain-flow.mddeletionPolicybefore thegit rm.references/comparative-bench.mdreferences/preflight.mdreferences/troubleshooting-seinode.mdRetainnote distinguishes forensics from a leak.README.mdevals/evals.jsonRetainhalt (empty ≠ safe; forbids the prune/reclaim/Role "fixes") and a clean teardown happy path.Verification
Docs/skills only — no build, no module compile.
GONE/PRESENT/UNVERIFIED, capturingkubectl's exit status separately rather than counting lines from a pipeline. AForbiddenor dropped connection isUNVERIFIED, never a pass. The poll deadline is arithmetic ondate +%s; Bash'sSECONDSis unset undersh, where the comparison fails withIllegal numberand the loop never runs. Both behaviours were reproduced under this host's/bin/shbefore and after the fix.writing/scripts/lint.sh(Vale 3.17.1, aftervale sync): 0 errors on every added line (530 added lines across 9 changed.mdfiles). Warnings remain (sentence length, passive voice) at the same density as the surrounding file; CI'sfail_leveliserror.xreview/scripts/skill-package-checks.sh --skill-dir .claude/skills/harbor-dev: no block-severity failure. Same two pre-existing non-block failures asmain(T2info: missingstate/.gitkeep;A1warn: time-sensitive content inSKILL.md/preflight.md/seictl-cli.md).B1passes —SKILL.mdis 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.jsonparses; 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-sourceon the workspace Kustomization. TheGitRepositoryit 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 outsideeng-<alias>, the engineer's namespace-scoped Role returnsForbidden. The docs state that failure mode and the fallback (reconcile the Kustomization alone) rather than asserting the flag always works.kubernetes.io/created-for/pvc/namespaceand.../pvc/nameare 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.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>ineng-<alias>, fromreferences/onboarding-pr.md:110and 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.kubectl | wc -ltakeswc's exit status, so aForbiddenyielded zero lines and printed "all objects gone"kubectl's status separately and reports one ofGONE/PRESENT/UNVERIFIED, with distinct exit codes 0/1/2.UNVERIFIEDis a halt condition. Reproduced the false pass under/bin/sh, then reproduced all three outcomes correctly after the fix.Retainfrom git back over the patch, typically mid-reviewSKILL.md,seinetwork-crd.md,ephemeral-chain-flow.md,comparative-bench.md, and both evals.available/Used By: <none>treated as proof of garbage, routing live disks into a deletion escalationspec.csi.volumeHandle→claimRef→ PVC → workload) and a verdict table whose every failed hop lands onUNRESOLVED.kubectl get pvis cluster-scoped, soForbiddenis called out as the normal engineer case and an unresolved result. This skill now never deletes an EBS volume.SECONDSin anshfence skips the poll; PVC and bench checks were single reads; PVC expectation contradicted the imported-PVC exceptiondate +%sdeadline.poll_gonein 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 isTerminating. The PVC expectation is no longer zero: controller-managed PVCs must go, imported PVCs must survive, and inventory step 2 now records which nodes carryspec.importbecause nothing says so after they are deleted.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.Forbiddenon--with-sourcestated as diagnosticGitRepository, alongside expired session, missing access entry, or a Role without the Flux verbs. Read the message first.--filters" would enumerate other tenants' volumes--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.resourceswas listed among the immutableSeiNetworkSpecfields. There are exactly three immutability rules —spec.genesis,spec.replicas,spec.dataVolume— andSeiNetworkSpechas noresourcesfield. Removed from both.Untouched by design, per the review: the reconcile target (A), PR parity (E), the orphan-detection triple, the legitimate
flux-systemcommand inpreflight.md, and the three inference caveats.Gates after the fix round
.mdfiles.skill-package-checks.sh: 20 pass / 2 fail / 4 skipped — the same two non-block failures asmain(T2info,A1warn), no block failure.B1passes at 396/500 lines.writing/scripts/check-*.sh: pass.verify-references.sh,sync-skills.sh --verify,skill-package-checks.test.sh(47/0): pass.evals.jsonparses; 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.teardown.mdre-implemented the poll twice instead of calling the sharedpoll_gone, and the copies had already diverged — the PVC loop emitted no verdict at all, just counts until its deadlineUNVERIFIEDstill exited 0 —breaksucceeds, so the completedwhilereturned 0, and no caller checkedpoll_gone's return code eitherrecord/VERDICTaggregation where the worst outcome wins (0 GONE < 1 PRESENT < 2 UNVERIFIED) and every call site records. Confirmed underdash:break-after-UNVERIFIEDdoes exit 0; after the fix,UNVERIFIEDthenGONEaggregates to 2..spec.importdoes not exist — the field is.spec.dataVolume.import.pvcName, so the query matched nothing and every imported claim was reclassified as one that must disappearclaimName, imported vs controller-managed classification, abort on any API or parse failure. Poll named managed claims; assert imported ones withexpect_present. Field path caveated as repo-main API inspection, not the deployed CRD. Also corrected the three pre-existingspec.importprose mentions elsewhere in the skill — see "scope call" below.claimReffollowed without a namespace check; deletes without namespace/context; a failedgrepread as "no manifest"claim_ns != eng-<alias>escalation gate; every delete names--context harborand-n eng-<alias>; the authorizing search runs against a freshly fetched clone and branches ongrep's exit status — only 1 (no match) authorizes a delete, 2+ is unresolved. Verified the three exit codes.sed -n '/Used By/,+3p'is a GNU extension; blanket POSIX claimdash,ash,bash)" and namesdate +%sas a deliberate near-universal extension rather than POSIX.DeletealoneDeletefor a Flux-owned object; the live-patch forbidden signal no longer has an escape clause; the happy path requires named-claim polling with aggregation.--with-sourcefallback claimed to help "whatever the cause"One extra defect found while fixing 4.
jq'suniquesorts by codepoint;commassumes its input is sorted the way the locale collates, and a locale that ignores punctuation orders hyphenated claim names differently. The inventory script now pinsLC_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 introubleshooting-seinode.mdand one inseinetwork-crd.md. Leaving them would have made the skill contradict itself on an API path, so they were corrected tospec.dataVolume.import— prose only, no query. Revert if you would rather keep that out of this PR.Gates after round 3
.mdfiles (onewrite-good.ThereIsintroduced and fixed mid-round).skill-package-checks.sh: 20 pass / 2 fail / 4 skipped — the same two non-block failures asmain(T2info,A1warn).B1passes at 396/500.writing/scripts/check-*.shpass.verify-references.sh,sync-skills.sh --verify,skill-package-checks.test.sh(47/0) pass.evals.jsonparses; internal anchors resolve.jqfilters. This host has nojq, and installing one was out of budget — they are reviewed by inspection only. The shell control flow around them (aggregation,expect_present,set -euabort on a redirected failure,grepexit codes,comm) was executed under/bin/sh.