From 15263e1250e479460af2a202c0c007400b95b6a9 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Wed, 9 Sep 2026 21:37:34 +0000 Subject: [PATCH 01/18] docs(harbor-dev): add the canonical teardown reference Documents the deletionPolicy: Retain trap that leaks EBS disks, the patch-before-delete ordering, the workspace-Kustomization reconcile target, the resource-disappearance poll, the namespace procedures, and the cleanup path for already-orphaned SeiNodes and volumes. Co-authored-by: omnigent --- .../skills/harbor-dev/references/teardown.md | 281 ++++++++++++++++++ 1 file changed, 281 insertions(+) create mode 100644 .claude/skills/harbor-dev/references/teardown.md diff --git a/.claude/skills/harbor-dev/references/teardown.md b/.claude/skills/harbor-dev/references/teardown.md new file mode 100644 index 00000000..c10561c0 --- /dev/null +++ b/.claude/skills/harbor-dev/references/teardown.md @@ -0,0 +1,281 @@ +# Teardown (chain, bench, namespace) + +Teardown removes an engineer's workloads from `eng-` through the same PR contract that created them. One ordering rule governs the whole file: **patch `spec.deletionPolicy` to `Delete` on every SeiNetwork you are about to remove, and land that patch before the removal merges.** A SeiNetwork deleted under the default `Retain` orphans its generated validator SeiNodes, and each orphan keeps its PVC and its EBS disk running with nothing left to clean it up. + +- [The `deletionPolicy: Retain` trap](#the-deletionpolicy-retain-trap) +- [Procedure: tear down a chain, bench, or comparison](#procedure-tear-down-a-chain-bench-or-comparison) +- [Verify the teardown](#verify-the-teardown) +- [Procedure: empty or remove my namespace](#procedure-empty-or-remove-my-namespace) +- [Find and clean up already-leaked resources](#find-and-clean-up-already-leaked-resources) +- [Halt conditions](#halt-conditions) + +## The `deletionPolicy: Retain` trap + +`SeiNetwork.spec.deletionPolicy` defaults to `Retain`. On deletion under that policy the controller **strips the owner reference** from each generated validator SeiNode instead of deleting it. The result is silent: + +1. The SeiNetwork disappears. Every phase read and every `kubectl get seinetwork` says the teardown worked. +2. The validator SeiNodes keep running. They lost their owner reference, so Kubernetes garbage collection has nothing to follow. +3. Flux prune never reaches them. The controller created those validators, so they were never in Flux's inventory. Prune is already enabled on the shared engineer reconciler and works correctly for what Flux owns. +4. Each orphan holds its PVC, and each PVC holds an EBS disk. The disk survives until somebody deletes the SeiNode by hand. + +**The storage class is not the bug.** A `Delete` reclaim policy releases the disk only when the PVC itself gets deleted. An orphaned SeiNode never releases its PVC, so reclaim never runs. Do not change a storage class to fix this. + +### The patch works only before deletion + +`spec.deletionPolicy` is **mutable** — no CEL validation rule and no webhook makes it immutable, unlike `spec.genesis`, `spec.replicas`, `spec.dataVolume`, and `spec.resources`. So an operator can flip a live SeiNetwork from `Retain` to `Delete`. + +That window closes at deletion. Once a `Retain` deletion has stripped the owner references and removed the parent SeiNetwork, no patch brings the cascade back — the parent is gone and the children are top-level objects. The leftover SeiNodes and PVCs then need the manual cleanup in [Find and clean up already-leaked resources](#find-and-clean-up-already-leaked-resources). + +**Patch first, delete second. There is no way to reorder these two steps.** + +### Read the current policy + +```sh +kubectl --context harbor get seinetwork -n eng- \ + -o jsonpath='{.spec.deletionPolicy}' +# → Retain (the default — teardown will orphan the validators) +# → Delete (the cascade works end to end; proceed) +# → (empty) the field is unset, which means Retain +``` + +An empty result is `Retain`, not "no policy". Treat it the same way. + +### Set it to `Delete` + +Two paths. Both must land before the removal PR merges. + +**Path A — a manifest PR (default).** Add `deletionPolicy: Delete` to `spec` in `engineers///seinetwork-.yaml`, merge it, and confirm the live object reads `Delete` with the command above. Then open the removal PR. This keeps the change in git, which is where every other spec field for this chain lives. + +**Path B — a live patch (fast path for a disposable chain).** One PR instead of two: + +```sh +kubectl --context harbor patch seinetwork -n eng- \ + --type=merge -p '{"spec":{"deletionPolicy":"Delete"}}' +kubectl --context harbor get seinetwork -n eng- \ + -o jsonpath='{.spec.deletionPolicy}' # must print Delete before you go on +``` + +The patch mutates a live object outside git. That is acceptable here only because the object is about to be deleted, and only after the verify read prints `Delete`. Say in the removal PR body that the patch ran, so the reviewer sees the whole teardown. + +### Render new chains with `Delete` from the start + +The trap disappears if the SeiNetwork never carries `Retain`. At render time on a disposable chain, pass: + +```sh +seictl network apply --preset genesis-chain --chain-id \ + --image -n eng- --dry-run --set spec.deletionPolicy=Delete +``` + +`--dry-run` runs server-side apply against the apiserver, so a wrong path fails the render rather than the teardown. Keep `Retain` only when the engineer wants a validator's disk preserved for forensics after the network goes away, and say so in the PR body — a retained disk is a cost the engineer is choosing. + +### What the `Delete` cascade actually does + +With `deletionPolicy: Delete` the chain runs end to end: SeiNetwork deleted → generated validator SeiNodes deleted through their owner references → each SeiNode's finalizer (`sei.io/seinode-finalizer`) deletes the node's data PVC → the storage class's `Delete` reclaim policy releases the EBS volume. + +The finalizer **skips an imported PVC** (`spec.import` set on the SeiNode). An imported PVC is preserved by design; its disk is not a leak. + +That finalizer is also why the per-engineer Role carries no `delete` on `persistentvolumeclaims`. The controller owns PVC lifecycle, and PVCs never appear in the workspace repo, so Flux prune never targets them. Do not ask for that verb — it does not fix this bug. + +## Procedure: tear down a chain, bench, or comparison + +Teardown follows the same PR contract as spinup: render the change, open a PR, let the engineer merge, verify what Flux did. Never `kubectl delete` a Flux-owned CR — the next reconcile re-applies it and the removal PR never lands. + +1. **Pre-flight** — the five gates. Halt on first failure. +2. **Name what goes away** — the task dir, every SeiNetwork and SeiNode in it, and the PVCs those nodes hold. List them for the engineer before touching anything: + + ```sh + kubectl --context harbor get seinetwork,seinode -n eng- \ + -l sei.io/seinetwork= \ + -o custom-columns='KIND:.kind,NAME:.metadata.name,ROLE:.metadata.labels.sei\.io/role,PHASE:.status.phase' + kubectl --context harbor get pvc -n eng- + ``` +3. **Check `deletionPolicy` on every SeiNetwork in the task dir** — read it with the command in [Read the current policy](#read-the-current-policy). On `Retain` (or empty), halt and route to [Set it to `Delete`](#set-it-to-delete). Do not open the removal PR while a SeiNetwork still reads `Retain`. +4. **Confirm the policy landed** — the live object must read `Delete`. This is the gate for step 5; a removal that merges ahead of it leaks the validators' disks. +5. **Remove the manifests** — `git rm -r engineers///` **and** remove the `` entry from `engineers//kustomization.yaml`'s `resources:` list. Both edits are required: Kustomize fails to render with a missing-resource entry, and Flux then applies nothing at all. +6. **Commit + push** — branch `feat/eng--teardown-`. Commit message: `feat(eng/): tear down — chain-id=`. +7. **Open the PR** — title `feat(eng/): tear down `. The body names the chain-id, every CR that goes away, the `deletionPolicy` value the SeiNetwork now carries, and the patch path (A or B) that set it. `gh pr create --repo sei-protocol/harbor-engineering-workspace --base main`. +8. **After merge — reconcile and verify** — [Verify the teardown](#verify-the-teardown). A merged PR is not a completed teardown. +9. **Report what survives** — the chain-id's S3 genesis artifacts are **not** purged by teardown, so the chain-id is burned. A later respin uses a fresh chain-id or purges the `/` prefix in `harbor-sei-k8s-genesis-artifacts` first. + +## Verify the teardown + +Two separate questions, and the second is the one that catches a leak: did the right reconciler run, and did the objects actually disappear? + +### Reconcile the workspace Kustomization, not `flux-system` + +The engineer's manifests are applied by the Flux `Kustomization ` in namespace `eng-`, which watches `harbor-engineering-workspace` at `./engineers/` and reconciles every 5 minutes. The root `flux-system` Kustomization tracks `sei-protocol/platform` at `clusters/harbor`. Reconciling `flux-system` after a workspace-repo merge reconciles a different repository and reports success without applying the engineer's change. + +Use `flux-system` after a **platform**-repo merge (onboarding). Use `` after a **workspace**-repo merge (every chain, bench, and teardown). + +```sh +flux --context harbor reconcile kustomization -n eng- --with-source +``` + +Fallback when `flux` is absent: + +```sh +kubectl --context harbor -n eng- annotate kustomization \ + reconcile.fluxcd.io/requestedAt="$(date +%s)" --overwrite +``` + +Then confirm the merge commit landed: + +```sh +kubectl --context harbor -n eng- get kustomization \ + -o jsonpath='{.status.lastAppliedRevision}' +``` + +Compare that revision to the merge commit SHA. A stale revision means Flux has not applied the removal yet, so any disappearance check below is premature. + +If `--with-source` returns `Forbidden`, the `GitRepository` the Kustomization references sits outside `eng-` and the engineer's namespace-scoped Role does not reach it. Drop `--with-source` and reconcile the Kustomization alone; it applies the revision the source has already fetched, and the source polls on its own schedule. + +### Confirm the resources disappeared + +A successful reconcile says Flux applied the change. It does not say the objects are gone. Deletion is asynchronous and finalizers hold objects in `Terminating` while the controller releases their PVCs, so poll instead of asserting once: + +```sh +end=$((SECONDS + 300)) +while [ "$SECONDS" -lt "$end" ]; do + left=$(kubectl --context harbor get seinetwork,seinode -n eng- \ + -l sei.io/seinetwork= -o name | wc -l) + if [ "$left" -eq 0 ]; then echo "all objects gone"; break; fi + echo "$left object(s) remain"; sleep 10 +done +``` + +Then confirm the disks went with them: + +```sh +kubectl --context harbor get pvc -n eng- \ + -o custom-columns='NAME:.metadata.name,STATUS:.status.phase,VOLUME:.spec.volumeName,CLASS:.spec.storageClassName' +``` + +Every PVC belonging to the torn-down chain must be gone. A `Bound` PVC that outlives its SeiNode is a held disk. + +### A stuck `Terminating` object is a real signal + +If the poll runs out with objects still present, do not report the teardown as done and do not force the objects away. Read why they are held: + +```sh +kubectl --context harbor get seinetwork,seinode -n eng- \ + -l sei.io/seinetwork= \ + -o custom-columns='NAME:.metadata.name,PHASE:.status.phase,DELETED:.metadata.deletionTimestamp,FINALIZERS:.metadata.finalizers' +``` + +`sei.io/seinode-finalizer` on a SeiNode blocks its deletion until the controller releases the PVC. A node parked there means the controller is unhealthy or the EBS CSI driver flaked — check `kubectl logs -n sei-k8s-controller-system -l app.kubernetes.io/name=sei-k8s-controller --tail=100` and surface what it says. Removing the finalizer by hand (`troubleshooting-seinode.md` → *PVC stuck after delete*) abandons the PVC and its disk, which is the leak this file exists to prevent. Take that step only after the engineer accepts the orphaned PVC, and record the PVC name so somebody can clean it up. + +## Procedure: empty or remove my namespace + +"Destroy my namespace" means one of two very different things. Ask which before acting. + +### What a workspace-repo PR removes + +A workspace-repo PR governs `engineers//` only. Removing every task dir removes: + +- Every SeiNetwork and SeiNode the engineer's manifests declared, and their pods, StatefulSets, headless Services, and controller-managed PVCs. +- Every bench Job and ConfigMap. +- Any engineer-owned exposure YAML (`Service`, `HTTPRoute`) in those task dirs. + +It does **not** remove: + +- **The `Namespace` object.** It comes from the platform repo (`clusters/harbor/engineers/base/namespace.yaml`) and stays. +- The three ServiceAccounts, the `` Role and RoleBinding, or the `engineer-admin` RoleBinding — all platform-owned. +- The Flux `Kustomization ` itself. It keeps reconciling an empty `engineers//`. +- Anything created outside git — an escape-hatch direct apply, a `SeiNodeTaskWorkflow`, or an orphaned SeiNode from an earlier `Retain` teardown. Flux prune only reaches what Flux applied. +- S3 artifacts: the chain-ids' genesis prefixes and the bench results under `harbor-validation-results/eng-/`. + +### Empty the namespace (the common case) + +1. Inventory everything first, including what git does not know about: + + ```sh + kubectl --context harbor get seinetwork,seinode,job,pvc -n eng- + ``` +2. For every SeiNetwork in the inventory, run the `deletionPolicy` gate in [The `deletionPolicy: Retain` trap](#the-deletionpolicy-retain-trap). One `Retain` network is enough to leak a set of disks. +3. `git rm -r` every task dir under `engineers//`, and reduce `engineers//kustomization.yaml` to `resources: []`. Keep that file: deleting it makes the Flux Kustomization fail reconcile with `path not found`, which is the same breakage the onboarding scaffolding PR exists to prevent. +4. Open the PR, merge, then run [Verify the teardown](#verify-the-teardown) with no `-l` selector, so the poll covers the whole namespace. +5. Sweep for what git never owned — [Find and clean up already-leaked resources](#find-and-clean-up-already-leaked-resources). + +### Remove the namespace entirely (offboarding) + +This is a platform-repo change and the engineer cannot do it from the workspace repo. It reverses the onboarding PR: delete `clusters/harbor/engineers//`, remove `` from `clusters/harbor/engineers/kustomization.yaml`, remove `eng-` from `clusters/harbor/monitoring/podmonitor-seiload-eng.yaml`, delete `terraform/aws/189176372795/eu-central-1/harbor/engineers/.tf`, and run the targeted `terraform apply` to drop the six Pod Identity resources. + +Empty the namespace first, through the steps above. Deleting the `Namespace` object while SeiNetworks still live in it starts a namespace-wide cascade that races the controller's finalizers and can strand PVCs with no owning CR to inspect. + +The file list mirrors the onboarding shape in `onboarding-pr.md`; the reverse flow has no worked example in this skill. Surface it to the platform team through `#harbor-onboarding` rather than opening the PR unassisted. + +## Find and clean up already-leaked resources + +Run this after any teardown that ran under `Retain`, and any time an engineer asks where the harbor spend is going. + +### Orphaned SeiNodes + +An orphaned validator has **no `ownerReferences`** and no live parent SeiNetwork. Absence of owner references alone is not the signal: a follower applied through `seictl node apply` is a top-level object and legitimately has none. The signature is `sei.io/role=validator` **and** no owner references. + +```sh +kubectl --context harbor get seinode -n eng- -l sei.io/role=validator -o json \ + | jq -r '.items[] + | select((.metadata.ownerReferences // []) | length == 0) + | "\(.metadata.name)\t\(.metadata.labels["sei.io/seinetwork"] // "-")\t\(.status.phase // "-")\t\(.metadata.creationTimestamp)"' +``` + +Each line is a validator still running with nothing that will ever delete it. Confirm the parent is gone before treating one as an orphan: + +```sh +kubectl --context harbor get seinetwork -n eng- +# NotFound → the parent is gone and this node is orphaned +``` + +### Held and leaked disks + +An orphaned SeiNode still shows a **`Bound`** PVC — the disk is attached and billing, not free-floating. A disk whose PVC has already gone shows up on the AWS side as **`available`**. Check both. + +```sh +kubectl --context harbor describe pvc -n eng- | grep -A2 'Used By' +# Used By: → an orphaned node is holding it +# Used By: → Bound but unattached; nothing in-cluster references it +``` + +On the AWS side, the EBS CSI driver tags each volume with the PVC it was provisioned for: + +```sh +aws ec2 describe-volumes --region eu-central-1 --profile \ + --filters "Name=tag:kubernetes.io/created-for/pvc/namespace,Values=eng-" \ + --query 'Volumes[].{id:VolumeId,state:State,size:Size,created:CreateTime,pvc:Tags[?Key==`kubernetes.io/created-for/pvc/name`]|[0].Value}' \ + --output table +``` + +`state: in-use` with an orphaned SeiNode above it is a running leak. `state: available` is a disk nothing references at all. Those tag keys are the EBS CSI driver's own convention rather than something this skill's repos set — run the command once without `--filters` against a volume you know is live to confirm the keys are present before trusting an empty result. + +The engineer's SSO profile may lack `ec2:DescribeVolumes`. On `AccessDenied`, surface the ask to the platform team with the namespace and the orphaned node names; do not treat the denial as "no leaked disks". + +### Clean them up + +**Delete the orphaned SeiNode. That is the whole cleanup for a held disk.** + +```sh +kubectl --context harbor delete seinode -n eng- +``` + +The node's finalizer deletes its data PVC, and the `Delete` reclaim policy on `gp3-10k-750` (validators) and `gp3` (default) releases the EBS volume. `gp3-archive` is `Retain` by design — a volume on that class stays after its PVC goes, and its removal is an AWS-side decision, not a mistake to correct here. + +Two checks before you run it: + +- The node must be a confirmed orphan by the signature above. `kubectl delete seinode` against a follower that still has a manifest in the workspace repo is undone by the next Flux reconcile, and the safer `git rm` path never lands. +- Poll the disappearance and the PVC afterwards, exactly as in [Verify the teardown](#verify-the-teardown). An orphan can stick in `Terminating` for the same finalizer reasons. + +An imperative `kubectl delete` is right here and nowhere else in teardown: the object was never in git, so there is no manifest to `git rm`. + +**A volume already `available` in EC2 has no in-cluster handle left.** Deleting it needs `ec2:DeleteVolume`, which the engineer's profile is unlikely to carry. Collect the volume IDs, sizes, and creation times, and escalate to the platform team through `#harbor-onboarding`. Do not report the cleanup as complete while those IDs are outstanding. + +## Halt conditions + +Stop and report. Do not auto-remediate. + +- **A SeiNetwork in the teardown reads `deletionPolicy: Retain` or empty.** Removing it orphans the validators and leaks their disks. Halt before opening the removal PR; route to [Set it to `Delete`](#set-it-to-delete). +- **The removal PR merged while a SeiNetwork still read `Retain`.** The cascade is gone and no patch restores it. Do not re-apply the SeiNetwork to "reattach" the children — a fresh network under the same chain-id wedges at height 0 on the burned genesis artifacts. Go straight to [Find and clean up already-leaked resources](#find-and-clean-up-already-leaked-resources). +- **`kustomization ` is `NotFound` in `eng-`.** The engineer's Flux wiring is missing, so no workspace-repo merge reconciles at all. Surface to the platform team; do not create the Kustomization. +- **`lastAppliedRevision` does not reach the merge commit within two reconcile intervals (~10 min).** Read the Ready condition's message (`cluster-inspection-recipes.md` recipe #8). A render error in `engineers//kustomization.yaml` — most often a `resources:` entry pointing at the dir that was just removed — blocks every later apply in the namespace, not only this teardown. +- **An object is still `Terminating` after the poll budget.** Report the finalizer and the controller's log line. Do not strip the finalizer to make the check pass. +- **Orphaned SeiNodes found in a namespace the engineer does not own.** Cross-tenant cleanup is out of scope. Hand the platform team the namespace and the node names. +- **`aws ec2 describe-volumes` returns `AccessDenied`.** The leak check did not run. Say that, rather than reporting a clean result. From 28a0a54197e9122c14939b910bd40eb8f70a1836 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Wed, 9 Sep 2026 21:40:59 +0000 Subject: [PATCH 02/18] fix(harbor-dev): target the workspace Kustomization and gate teardown on deletionPolicy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Post-merge reconciliation targeted `flux-system`, which tracks the platform repo — a workspace-repo merge was verified against a reconciler that never applied it. Route workspace merges to `Kustomization ` in `eng-` and keep `flux-system` for platform merges. Add Guardrail #10, a PR-based teardown procedure with the patch-before-delete ordering and a resource-disappearance poll, three halt conditions, and the `teardown.md` reference-index entry. Co-authored-by: omnigent --- .claude/skills/harbor-dev/SKILL.md | 52 +++++++++++++++---- .../skills/harbor-dev/references/teardown.md | 7 +-- 2 files changed, 47 insertions(+), 12 deletions(-) diff --git a/.claude/skills/harbor-dev/SKILL.md b/.claude/skills/harbor-dev/SKILL.md index f359f980..948fcd49 100644 --- a/.claude/skills/harbor-dev/SKILL.md +++ b/.claude/skills/harbor-dev/SKILL.md @@ -28,6 +28,7 @@ The hard rules: 9. **Two paths wipe a node's chain data — gate both.** `seictl workflow state-sync` is the destructive **paved road**; a mutating `seictl task submit` is the destructive **escape hatch**. Neither is ever the default, and the agent volunteers neither. - **`seictl workflow state-sync`** re-bootstraps an existing node by wiping its local chain state (an `rm -rf` on that node's data), optionally with an irreversible `--migration GigaStore --backend ` store change — both tokens are required together, never `--migration` alone. Require explicit engineer sign-off before the non-dry-run apply, verify the target node against the live cluster first, `--dry-run` to inspect, and escalate to the owner — never wipe on agent initiative — for any shared or long-lived `pacific-1`/`atlantic-2` follower. Never commit a workflow CR to the Flux workspace repo (a one-shot, spec-immutable request object; force-delete recovery fights Flux). Full gate in `references/seictl-cli.md` → `seictl workflow state-sync`. - **`seictl task submit`** POSTs a raw task straight to one pod's sidecar, and the accepted types include `reset-data`. Submitted that way the wipe runs with **none** of the recipe's protections — no `mark-not-ready` hold, no `stop-seid` first, no ordering, and no adoption pointer telling the controller the node is occupied — so it is strictly more dangerous than the paved road, not a lighter-weight version of it. Prefer `workflow state-sync` for anything the recipe covers; a mutating `task submit` requires explicit sign-off naming node, namespace, and task type. `task get` / `task list` are reads and safe. Full gate in `references/seictl-cli.md` → `seictl task`. +10. **Never tear down a SeiNetwork before reading its `spec.deletionPolicy`.** It defaults to `Retain`, and a `Retain` deletion strips the owner reference from every generated validator SeiNode instead of deleting it. The orphans keep running, keep their PVCs, and keep their EBS disks, with nothing left in-cluster that will ever remove them — the teardown reports success while the spend continues. Patch the live object to `Delete` and confirm the read-back **before** the removal merges. After the parent SeiNetwork is gone, no patch restores the cascade and the cleanup is manual. Full procedure and the leaked-resource sweep: `references/teardown.md`. ## Mental model @@ -130,7 +131,8 @@ Every engineer-facing intent maps to a `seictl network`, `seictl node`, or (for | "Attach a node via state sync" / "add an RPC node without replaying from genesis" / "bootstrap from my own chain" | Same follower shape plus a `spec.fullNode.snapshot` block: `stateSync: {}` + ≥2 `rpcServers` witness endpoints from the engineer's own chain (replaces the platform syncer registry — self-service, no platform PR). **Read `references/state-sync-bootstrap.md` first — its known-issue banner governs: state-sync bootstrap is currently broken on ceremony-fresh eng chains (PLT-794, genesis carries no validator set); genesis replay is the working path until it lands.** Witnesses verify the trust point; snapshot chunks come over p2p from peers that have actually produced a snapshot. | | "What's running in my namespace" / "what chains do I have" | `seictl network list -n eng-` for the chains + `seictl node list -n eng-` for the followers (yaml default; `-o name` for short, `-o jsonpath=...` for one-shot field reads). | | "Show me chain X" / "what's the status of X" | `seictl network get -n eng-` for the network (`.status.phase`); `seictl node get -rpc- -n eng-` for a follower (`.status.phase`, `.status.endpoint`). | -| "Tear down chain X" | `git rm -r engineers///` **and** remove `` from `engineers//kustomization.yaml`'s `resources:` list (Kustomize fails to render with a missing-resource entry). Commit → push → merge. Flux prunes the SeiNetwork + SeiNodes on next reconcile, which cascades to pods / PVCs per k8s deletion propagation. **Teardown does NOT purge the chain-id's S3 genesis artifacts — the chain-id is burned**; a later respin must use a fresh chain-id (see the naming step) or purge the `/` genesis-bucket prefix. See `bench:teardown` recipe in `references/cluster-inspection-recipes.md` for the bench-specific variant. | +| "Tear down chain X" / "delete my bench" | **PR-based** (see Procedure: tear down). Read `spec.deletionPolicy` on every SeiNetwork in the task dir **first** — on `Retain` (the default) the teardown orphans the validators and leaks their disks. Then `git rm -r engineers///` **and** remove `` from `engineers//kustomization.yaml`'s `resources:` list (Kustomize fails to render with a missing-resource entry). Commit → push → merge → reconcile `` → poll until the objects are gone. **Teardown does NOT purge the chain-id's S3 genesis artifacts — the chain-id is burned**; a later respin must use a fresh chain-id (see the naming step) or purge the `/` genesis-bucket prefix. | +| "Destroy my namespace" / "clean out eng-" / "where is my harbor spend going" | `references/teardown.md` → *empty or remove my namespace*, and the leaked-resource sweep. A workspace PR removes the engineer's workloads; the `Namespace` object, the ServiceAccounts, the RBAC, and the Flux Kustomization are platform-owned and stay. Orphaned SeiNodes from an earlier `Retain` teardown are in no repo at all and need the manual sweep. | | "Run a load test against chain X" / "bench it" / "stress test chain X" | **PR-based bench** (see Procedure: spin up a load test). Live-fetch chain rpc per-pod URLs, substitute into the profile JSON, render Job + ConfigMap from the templates in `references/sei-load-bench.md`, open a PR against `harbor-engineering-workspace` at `engineers//bench-/`. Merge → Flux applies → seiload runs → uploader sidecar pushes results to S3. | | "Compare PR 3399 to main on sei-chain" / "bench A against B" / "diff the perf of these two commits" | **PR-based comparative bench** (see Procedure: comparative bench). Renders two ephemeral chains (each running its own seid image) + two sei-load Jobs (identical profile + duration) into a single PR. After merge, watches both chains in parallel, polls both Jobs to terminal, fetches both reports from S3, and surfaces a side-by-side metrics table (TPS / latency / success rate / errors). Lives at `engineers//compare-/`. | | "Where am I" / "what cluster am I on" / "who am I" | `kubectl config current-context` + `aws sts get-caller-identity --profile ` (the gate-2 profile). (No dedicated `seictl context` verb in this surface.) | @@ -148,26 +150,37 @@ Force-reconcile proactively after a relevant PR merges; don't wait for Flux's na **Trigger:** any PR merge the agent helped open against `sei-protocol/platform` or `sei-protocol/harbor-engineering-workspace`, OR when the engineer says "merged" / "I merged X". Don't wait for the engineer to ask. -**Preferred:** +**Which Kustomization depends on which repo — they are not interchangeable.** A workspace-repo merge (every chain, bench, and teardown) is applied by the Flux `Kustomization ` in `eng-`, which watches `harbor-engineering-workspace` at `./engineers/`. The root `flux-system` Kustomization tracks `sei-protocol/platform` at `clusters/harbor`, so reconciling it after a workspace merge reconciles a different repo and reports success without applying the engineer's change. Target `flux-system` only after a **platform**-repo merge (onboarding). + +| Merged PR against | Target Kustomization | In namespace | +|---|---|---| +| `sei-protocol/harbor-engineering-workspace` | `` | `eng-` | +| `sei-protocol/platform` | `flux-system` | `flux-system` | + +**Preferred** (workspace-repo merge): ```sh -flux --context harbor reconcile kustomization flux-system --with-source -n flux-system +flux --context harbor reconcile kustomization -n eng- --with-source ``` **Fallback** (only when `flux` isn't available): ```sh -kubectl --context harbor -n flux-system annotate kustomization flux-system \ +kubectl --context harbor -n eng- annotate kustomization \ reconcile.fluxcd.io/requestedAt="$(date +%s)" --overwrite ``` +`--with-source` also reconciles the referenced `GitRepository`. If that source sits outside `eng-`, the engineer's namespace-scoped Role returns `Forbidden` — drop the flag and reconcile the Kustomization alone, which applies the revision the source has already fetched. + **Verify** the merge commit SHA landed before proceeding to verbs that depend on the new state: ```sh -kubectl --context harbor -n flux-system get kustomization flux-system \ +kubectl --context harbor -n eng- get kustomization \ -o jsonpath='{.status.lastAppliedRevision}' ``` +A reconcile is not proof the change took effect. On a teardown, follow it with the disappearance poll in `references/teardown.md` — Flux reports success the moment it issues the deletes, well before finalizers release the PVCs. + ## Procedure: spin up an ephemeral chain (the headline — PR-based) Engineer says "spin up a chain of 4 validators with seid sha=abc, then add an RPC fleet." This is the daily-driver flow. The skill renders a SeiNetwork CR (and N SeiNode CRs for the fleet) via `seictl network apply --dry-run` / `seictl node apply --dry-run`, writes them to `engineers///` in `harbor-engineering-workspace`, opens a PR. Engineer merges → Flux applies → agent watches the network to Ready and each follower to Running → reports endpoints. @@ -193,7 +206,7 @@ Engineer says "spin up a chain of 4 validators with seid sha=abc, then add an RP 8. **Open the PR** — title: `feat(eng/): spin up `; body lists chain-id, image digest, preset(s), expected endpoints. `gh pr create --repo sei-protocol/harbor-engineering-workspace --base main`. 9. **Surface and halt** — engineer reviews and merges. Surface the PR URL with: "after merge, Flux reconciles in ~60s; ping me to watch the network to Ready and report endpoints." 10. **After merge — watch** — `seictl network watch --until=Ready --timeout=15m -n eng-` for genesis (the network reaches `Ready`), then per follower `seictl node watch -rpc- --until=Running --timeout=15m -n eng-` (a node reaches `Running` — **never `Ready`**, which is illegal for a node and errors at parse). NDJSON stream; exits 0 on the matched phase. Halt on non-zero (`metav1.Status` on stderr — `jq -r .reason` discriminates Timeout vs terminal Failed phase vs API failure). -11. **Report** — use the canonical inspection recipes from `references/cluster-inspection-recipes.md` rather than inferring jsonpath at runtime. Recipe #1 returns the fleet's RPC endpoints (target these for any load tools — never validators, which serve no EVM). Recipe #4 lists the network + its follower SeiNodes with phase + readiness in one shot. Plus teardown: `git rm -r engineers///` and remove the `` entry from `engineers//kustomization.yaml`'s `resources:` list, then commit → push → merge (Flux prunes the SeiNetwork + SeiNodes and cascades the deletion to pods/PVCs). +11. **Report** — use the canonical inspection recipes from `references/cluster-inspection-recipes.md` rather than inferring jsonpath at runtime. Recipe #1 returns the fleet's RPC endpoints (target these for any load tools — never validators, which serve no EVM). Recipe #4 lists the network + its follower SeiNodes with phase + readiness in one shot. Plus teardown: point at **Procedure: tear down** rather than restating it — the `deletionPolicy` gate runs before the `git rm`, and skipping it leaks the validators' disks. ### Escape hatch: direct `seictl network|node apply` (rare; engineer asks twice) @@ -240,7 +253,24 @@ Engineer says "compare PR 3399 to main on sei-chain" or "bench latest sei-chain 13. **Watch follower fleets in parallel** — per follower on each side, `seictl node watch -{a,b}-rpc- --until=Running --timeout=15m` (a node's terminal is `Running`, not `Ready`). 14. **Poll both bench Jobs to terminal** — single loop that checks both `seiload--{a,b}` for `Complete=True` or `Failed=True`. Deadline: ` * 60 + 660` seconds. 15. **Fetch + render** — pull both reports via in-cluster `kubectl run` under `engineer-service-account` (the engineer's local SSO profile lacks `s3:GetObject` on the prefix). Locate sei-load's summary block in each report; surface both summaries verbatim under a delta table that highlights canonical metrics (TPS, P50/P99 latency, success rate, tx counts) with `better` / `worse` verdicts. On summary-block miss, fall back to last-50-lines format with both S3 paths. -16. **Teardown guidance** — `git rm -r engineers//compare-/` and remove the entry from `engineers//kustomization.yaml`'s `resources:`. Flux prunes both SeiNetworks + all follower SeiNodes + two Jobs; child pods/PVCs cascade. +16. **Teardown guidance** — run **Procedure: tear down** against `engineers//compare-/`. **Both** SeiNetworks need the `deletionPolicy` gate before the removal PR; a comparison leaks two validator pools' disks, not one. + +## Procedure: tear down (PR-based) + +Engineer says "tear down chain X," "delete my bench," or "clean out my namespace." Teardown goes through a PR against `sei-protocol/harbor-engineering-workspace`, the same contract as spinup — never a bare `seictl network|node delete` against a Flux-owned CR, which the next reconcile re-applies. + +**Read `references/teardown.md` first.** It carries the `deletionPolicy` trap in full, the namespace procedures, and the sweep for resources that already leaked. + +1. **Pre-flight** — five gates. Halt on first failure. +2. **Inventory what goes away** — `kubectl get seinetwork,seinode -n eng- -l sei.io/seinetwork=` plus `kubectl get pvc -n eng-`. Show the engineer the list before touching anything. +3. **Gate on `deletionPolicy`** — `kubectl get seinetwork -n eng- -o jsonpath='{.spec.deletionPolicy}'` for every SeiNetwork in the task dir. `Retain` or empty means halt: removing the manifest orphans the generated validators and leaks their EBS disks (Guardrail #10). +4. **Set it to `Delete` and confirm the read-back** — a manifest PR (default) or `kubectl patch seinetwork -n eng- --type=merge -p '{"spec":{"deletionPolicy":"Delete"}}'` for a disposable chain. The read-back must print `Delete` before step 5. This ordering is the whole point; patching after the removal merges is too late. +5. **Remove the manifests** — `git rm -r engineers///` **and** remove the `` entry from `engineers//kustomization.yaml`'s `resources:` list. Both edits, or Kustomize fails to render and Flux applies nothing. +6. **Commit + push** — branch `feat/eng--teardown-`. Message: `feat(eng/): tear down — chain-id=`. +7. **Open the PR** — title `feat(eng/): tear down `. Body names the chain-id, every CR that goes away, and the `deletionPolicy` value the SeiNetwork now carries. Surface the URL and halt for the merge. +8. **After merge — reconcile the workspace Kustomization** — `flux --context harbor reconcile kustomization -n eng- --with-source`, then compare `.status.lastAppliedRevision` to the merge SHA. Reconciling `flux-system` here verifies the wrong repo (see Post-merge reconciliation). +9. **Poll until the resources disappear** — a reconcile only says Flux issued the deletes. Deletion is asynchronous and finalizers hold objects in `Terminating` while the controller releases PVCs, so poll `kubectl get seinetwork,seinode -n eng- -l sei.io/seinetwork= -o name` on a budget (~5 min) rather than asserting once, then confirm the PVCs went with them. +10. **Report** — what is gone, what remains, and the burned chain-id. An object still `Terminating` past the budget is a real finding: surface the finalizer and the controller log line; never strip a finalizer to make the check pass. ## Procedure: troubleshooting (manual) @@ -276,10 +306,13 @@ Stop and report to the user if: - **SeiNode Pending with `StateSyncReady=False/NoSyncersConfigured` and no StatefulSet.** A snapshot-bootstrap node whose witnesses can't be resolved — the controller holds StatefulSet creation until the gate opens (deliberate; prevents a stranded Pending pod). The condition message names both remediations: declare ≥2 `spec.fullNode.snapshot.rpcServers`, or drop `stateSync` and genesis-replay. Don't delete/recreate the pod or chase storage — the block is upstream. See `references/state-sync-bootstrap.md`. - **Followers not Running when rendering a bench.** The bench requires per-follower RPC URLs, which populate only once each SeiNode reaches `Running`. Halt and offer to poll (`seictl node watch -rpc- --until=Running -n eng-`) before continuing. - **Follower `.status.endpoint` present but dial refused.** `Running` means config applied + sidecar self-marked ready, NOT that the EVM listener is accepting connections — there is a real post-Running window. Before driving load, run `seictl node watch --until=caught-up -n eng-` (the SDK readiness gate: height>1 with `catching_up=false`, plus EVM serving when the node publishes an EVM endpoint). Halt with the follower's full status if it stays refused. -- **Comparative bench: one side reaches Ready, the other Failed.** The comparison is invalid against half a setup. Surface the failed side's `.status.plan.failedTaskDetail.error`; offer to teardown the Ready side via `git rm` against just that sub-dir + commit. Don't run the bench against half a comparison. +- **Comparative bench: one side reaches Ready, the other Failed.** The comparison is invalid against half a setup. Surface the failed side's `.status.plan.failedTaskDetail.error`; offer to tear down the Ready side against just that sub-dir, through **Procedure: tear down** (its SeiNetwork needs the `deletionPolicy` gate like any other). Do not run the bench against half a comparison. - **Comparative bench: config parity check fails post-render.** The two substituted profile JSONs differ on a field other than `seiChainId` / `endpoints`. Halt before push; the rendered manifests would produce a non-comparable result. - **Comparative bench: chain-tag exceeds the 22-char budget** when the `-{a,b}-rpc-` follower suffix is added (the name regex caps at 30). Surface the overflow and ask the engineer to pick a shorter tag. - **Comparative bench: S3 GetObject fails on a report.** `NoSuchKey` means the upload sidecar didn't run — surface `kubectl logs -n eng- -l sei.io/compare-name=,sei.io/compare-side= -c upload-results` to diagnose. `AccessDenied` means the engineer's profile lacks `s3:GetObject` (the engineer SA's IAM policy already covers it; the active profile is wrong). +- **Teardown proposed while a SeiNetwork reads `deletionPolicy: Retain`** (or the field is empty, which means `Retain`). Removing the manifest orphans the generated validator SeiNodes and leaks their EBS disks. Halt before opening the removal PR; route to `references/teardown.md` → *set it to `Delete`*. If the removal already merged under `Retain`, the cascade is gone for good — do not re-apply the SeiNetwork to reattach the children (a fresh network on the burned chain-id wedges at height 0); go to the leaked-resource sweep instead. +- **A torn-down object is still `Terminating` past the poll budget.** `sei.io/seinode-finalizer` holds a SeiNode until the controller releases its PVC, so a parked object means an unhealthy controller or an EBS CSI flake. Surface the finalizer and `kubectl logs -n sei-k8s-controller-system -l app.kubernetes.io/name=sei-k8s-controller --tail=100`. Stripping the finalizer abandons the PVC and its disk — take that step only with the engineer's explicit acceptance, and record the PVC name. +- **Orphaned SeiNodes found in the namespace** — `sei.io/role=validator` with no `ownerReferences` and no live parent SeiNetwork. Each one is still running and still holding a disk. Surface the list and the cleanup in `references/teardown.md`; a follower with no owner references is not an orphan (a standalone SeiNode legitimately has none). - **PR push rejected (non-fast-forward)** — engineer or another agent pushed to the same branch. Don't force-push. Halt; surface `git pull --rebase origin ` and let the engineer resolve. - **`seictl workflow state-sync` failed / the workflow is `Failed`** — a Failed `SeiNodeTaskWorkflow` holds the node not-ready until it is removed. Recovery is force-delete first: annotate `sei.io/force-delete-workflow=`, then `seictl workflow delete `, which releases the node; only then re-run. The annotation is not optional — an un-annotated delete parks the workflow `Terminating` with the node still held (`WorkflowDeleteHeld` event). Re-running with `--name` (a fresh name) *without* first removing the Failed workflow is neither a recovery nor a second wipe: adoption is exclusive, so the new workflow is never adopted, no plan compiles, no `reset-data` runs, and the watch burns its `--timeout` while the node stays held. See `references/seictl-cli.md`. - **`seictl workflow state-sync` watch times out** — the watch ends when the workflow releases the node (15m default, 60m on older binaries; catch-up happens after `Complete`), so a timeout usually means a wedged recipe step. Do not kill-and-retry: inspect `.status.plan.tasks` on the one in-flight workflow (`seictl workflow list -n eng-` to find it) before acting — an archive-scale `reset-data` still clearing means raise `--timeout` and wait; any other step parked past its budget means force-delete. Starting a second workflow alongside a wedged one is never the shortcut — adoption is exclusive, so it parks unadopted and times out too. After a `Complete` exit, verify catch-up separately with `seictl node watch --until=caught-up`. @@ -290,6 +323,7 @@ Stop and report to the user if: |---|---| | `preflight.md` | **Read this first on a new session or when an engineer is fresh.** Five-gate ramp from "fresh laptop" to "ready to apply," per-gate recovery, mid-session drift handling, full new-engineer walk-through | | `onboarding-pr.md` | **Read this if the engineer is new.** The one-time tenant-registration PR shape. Canonical example: `clusters/harbor/engineers/fromtherain/kustomization.yaml`. What the base layer provides | +| `teardown.md` | **Read this if the engineer asks to tear anything down.** The `deletionPolicy: Retain` disk-leak trap and its patch-before-delete ordering, the PR-based teardown, the workspace-Kustomization reconcile target + disappearance poll, what a workspace PR does and does not remove from the namespace, and the sweep for already-orphaned SeiNodes and leaked EBS volumes | | `ephemeral-chain-flow.md` | **Read this if the engineer asks for a chain.** Preset taxonomy (`genesis-chain`, `rpc`), what each preset wires automatically, watch protocol, exit-code conventions | | `seictl-cli.md` | Canonical `seictl network` + `seictl node` + `seictl workflow` + `seictl task` verb trees (regenerated from `seictl --help` periodically). Carries the full destructive-op gate for `seictl workflow state-sync`, and the escape-hatch gate for `seictl task submit` | | `seinetwork-crd.md` | Operations-load-bearing fields on `SeiNetwork` (the genesis validator pool), including `.status.phase`, immutability, the `.status.plan` | @@ -345,7 +379,7 @@ Pre-approve in `.claude/settings.local.json` (user-specific, not committed): - `seictl workflow state-sync` / `seictl workflow apply` (without `--dry-run`) — **destructive**: wipes the target node's local chain state and re-syncs (optionally an irreversible `--migration` store change). Requires explicit engineer sign-off per invocation; the agent never volunteers it. See the gate in `references/seictl-cli.md`. - `seictl task submit` — **destructive-capable**, and ungated by the recipe: it POSTs any accepted task type to one pod's sidecar, `reset-data` included, with no hold, no `stop-seid`, and no ordering. Requires explicit engineer sign-off per invocation naming node, namespace, and task type (Guardrail #9). `seictl task delete` cancels a running task — also interactive. `seictl task get` / `seictl task list` are reads and safe to pre-approve. - `seictl task snapshot-upload` — non-destructive but a real side effect (submits a snapshot upload that can run for hours and publishes to S3); requires explicit confirmation per invocation. -- `seictl network delete` / `seictl node delete` / `seictl workflow delete` — destroys a CR + propagates deletion to children; requires explicit confirmation. For `workflow delete`, the primary use is the force-delete recovery for a Failed workflow holding a node — a Complete workflow is deliberately left in-cluster as the audit trail (see `references/seictl-cli.md`), not routinely deleted. Default teardown for network/node is `git rm` against the workspace-repo manifest, not this verb. +- `seictl network delete` / `seictl node delete` / `seictl workflow delete` — destroys a CR + propagates deletion to children; requires explicit confirmation. For `workflow delete`, the primary use is the force-delete recovery for a Failed workflow holding a node — a Complete workflow is deliberately left in-cluster as the audit trail (see `references/seictl-cli.md`), not routinely deleted. Default teardown for network/node is the PR-based procedure against the workspace-repo manifest, not this verb — and a `network delete` under `deletionPolicy: Retain` orphans the validators exactly as a Flux prune does (Guardrail #10). - `gh pr create` — opens onboarding and chain-spinup PRs; requires explicit confirmation per PR. - `git push` — pushes engineer-task branches to `harbor-engineering-workspace`; requires explicit confirmation. diff --git a/.claude/skills/harbor-dev/references/teardown.md b/.claude/skills/harbor-dev/references/teardown.md index c10561c0..41629bb5 100644 --- a/.claude/skills/harbor-dev/references/teardown.md +++ b/.claude/skills/harbor-dev/references/teardown.md @@ -5,6 +5,7 @@ Teardown removes an engineer's workloads from `eng-` through the same PR - [The `deletionPolicy: Retain` trap](#the-deletionpolicy-retain-trap) - [Procedure: tear down a chain, bench, or comparison](#procedure-tear-down-a-chain-bench-or-comparison) - [Verify the teardown](#verify-the-teardown) + - [Target the workspace Kustomization, not `flux-system`](#target-the-workspace-kustomization-not-flux-system) - [Procedure: empty or remove my namespace](#procedure-empty-or-remove-my-namespace) - [Find and clean up already-leaked resources](#find-and-clean-up-already-leaked-resources) - [Halt conditions](#halt-conditions) @@ -22,11 +23,11 @@ Teardown removes an engineer's workloads from `eng-` through the same PR ### The patch works only before deletion -`spec.deletionPolicy` is **mutable** — no CEL validation rule and no webhook makes it immutable, unlike `spec.genesis`, `spec.replicas`, `spec.dataVolume`, and `spec.resources`. So an operator can flip a live SeiNetwork from `Retain` to `Delete`. +`spec.deletionPolicy` is **mutable** — no CEL validation rule and no webhook makes it immutable, unlike `spec.genesis`, `spec.replicas`, `spec.dataVolume`, and `spec.resources`. An operator can therefore flip a live SeiNetwork from `Retain` to `Delete`. That window closes at deletion. Once a `Retain` deletion has stripped the owner references and removed the parent SeiNetwork, no patch brings the cascade back — the parent is gone and the children are top-level objects. The leftover SeiNodes and PVCs then need the manual cleanup in [Find and clean up already-leaked resources](#find-and-clean-up-already-leaked-resources). -**Patch first, delete second. There is no way to reorder these two steps.** +**Patch first, delete second. No later step recovers a teardown that ran in the other order.** ### Read the current policy @@ -101,7 +102,7 @@ Teardown follows the same PR contract as spinup: render the change, open a PR, l Two separate questions, and the second is the one that catches a leak: did the right reconciler run, and did the objects actually disappear? -### Reconcile the workspace Kustomization, not `flux-system` +### Target the workspace Kustomization, not `flux-system` The engineer's manifests are applied by the Flux `Kustomization ` in namespace `eng-`, which watches `harbor-engineering-workspace` at `./engineers/` and reconciles every 5 minutes. The root `flux-system` Kustomization tracks `sei-protocol/platform` at `clusters/harbor`. Reconciling `flux-system` after a workspace-repo merge reconciles a different repository and reports success without applying the engineer's change. From da67c8472a28328f5097f633de94199778dab513 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Wed, 9 Sep 2026 21:45:09 +0000 Subject: [PATCH 03/18] docs(harbor-dev): propagate the teardown fixes across the reference set Recipes #9 (disappearance poll) and #10 (orphaned-SeiNode detection); `bench:teardown` now reconciles the engineer's Kustomization and confirms the objects went away. The SeiNetwork CRD reference documents deletionPolicy as the disk-leak field, its mutability, and the closing window. Chain-flow, comparative-bench, preflight, troubleshooting, and the package README point at the gated procedure. Two evals cover the Retain halt and the clean teardown. Co-authored-by: omnigent --- .claude/skills/harbor-dev/README.md | 3 +- .claude/skills/harbor-dev/SKILL.md | 2 +- .claude/skills/harbor-dev/evals/evals.json | 49 ++++++++++++++++ .../references/cluster-inspection-recipes.md | 56 ++++++++++++++++++- .../references/comparative-bench.md | 5 +- .../references/ephemeral-chain-flow.md | 2 +- .../skills/harbor-dev/references/preflight.md | 2 +- .../harbor-dev/references/seinetwork-crd.md | 22 +++++++- .../references/troubleshooting-seinode.md | 4 +- 9 files changed, 135 insertions(+), 10 deletions(-) diff --git a/.claude/skills/harbor-dev/README.md b/.claude/skills/harbor-dev/README.md index 9c765bfd..1ea6857c 100644 --- a/.claude/skills/harbor-dev/README.md +++ b/.claude/skills/harbor-dev/README.md @@ -17,7 +17,8 @@ Harbor Dev is the conversational layer over `seictl network` + `seictl node`: an - Translates plain-English intent ("give me 4 validators on seid sha=abc, then an RPC fleet") into `seictl` invocations, so the engineer never hand-rolls SeiNetwork / SeiNode YAML, preset wiring, or peer selectors. - Defaults to GitOps: renders CRs via `--dry-run`, writes them under `engineers///`, opens a PR, and lets Flux apply on merge — direct apply is a rare, double-confirmed escape hatch. -- Covers the full daily-driver surface: onboarding, chain spinup, RPC fleets, single and comparative benches, status reads, and `git rm`-based teardown. +- Covers the full daily-driver surface: onboarding, chain spinup, RPC fleets, single and comparative benches, status reads, and PR-based teardown. +- Gates teardown on the field that leaks disks. A `SeiNetwork` deleted under its default `spec.deletionPolicy: Retain` orphans the validator SeiNodes it generated — they keep running, keep their PVCs, and keep their EBS volumes, and nothing in git or in Flux will ever remove them. The skill reads the policy, patches it to `Delete` before the removal merges, verifies against the engineer's own Flux Kustomization rather than `flux-system`, and polls the resources to gone. - Refuses the boundary that matters: harbor-only (never prod), `eng-`-only (no cross-tenant work), and it never silently works around a missing prereq — it surfaces the next step and halts. - Gates both paths that wipe a node's chain data, and neither is ever volunteered. `seictl workflow state-sync` is the paved road — it re-bootstraps or migrates an existing node's store, always sign-off-and-`--dry-run`-first, never run against a shared or long-lived follower without escalating to its owner. A mutating `seictl task submit` is the escape hatch: it reaches the same wipe straight through one pod's sidecar with none of the recipe's holds, so it carries the stricter gate. diff --git a/.claude/skills/harbor-dev/SKILL.md b/.claude/skills/harbor-dev/SKILL.md index 948fcd49..fe376dfa 100644 --- a/.claude/skills/harbor-dev/SKILL.md +++ b/.claude/skills/harbor-dev/SKILL.md @@ -328,7 +328,7 @@ Stop and report to the user if: | `seictl-cli.md` | Canonical `seictl network` + `seictl node` + `seictl workflow` + `seictl task` verb trees (regenerated from `seictl --help` periodically). Carries the full destructive-op gate for `seictl workflow state-sync`, and the escape-hatch gate for `seictl task submit` | | `seinetwork-crd.md` | Operations-load-bearing fields on `SeiNetwork` (the genesis validator pool), including `.status.phase`, immutability, the `.status.plan` | | `seinode-crd.md` | Operations-load-bearing fields on `SeiNode` (a single node / follower), including `.status.phase` (terminal `Running`), `.status.endpoint`, `.status.plan` | -| `cluster-inspection-recipes.md` | **Canonical structured-extraction recipes.** Use these directly instead of inferring jsonpath at runtime — RPC endpoints (recipe #1, also resolves "target RPC, not validator"), phase + readiness, failed task, image drift, a network's validator SeiNodes, Flux Kustomization Ready | +| `cluster-inspection-recipes.md` | **Canonical structured-extraction recipes.** Use these directly instead of inferring jsonpath at runtime — RPC endpoints (recipe #1, also resolves "target RPC, not validator"), phase + readiness, failed task, image drift, a network's validator SeiNodes, Flux Kustomization Ready (#8 — the engineer's own, not `flux-system`), teardown disappearance poll (#9), orphaned-SeiNode detection (#10) | | `sei-load-bench.md` | **Read this if the engineer asks for a load test or bench.** Job + ConfigMap templates with substitution markers, live profile-JSON substitution recipe, two-container upload pattern (seiload + amazon/aws-cli sidecar with `shareProcessNamespace`), S3 archival convention, run-id determinism on re-render | | `comparative-bench.md` | **Read this if the engineer asks to compare two images.** Four-subdir layout (chain-a / chain-b / bench-a / bench-b), naming convention, per-side profile substitution, parallel post-merge watch sequence, S3 fetch + canonical-metric extraction with raw-tail fallback, comparison-output table format | | `image-resolution.md` | **Canonical image-resolution recipes** for sei-chain (ECR) and sei-load (GHCR). PR/commit/branch input → full SHA → expected tag → registry probe → trigger + watch the build workflow if missing | diff --git a/.claude/skills/harbor-dev/evals/evals.json b/.claude/skills/harbor-dev/evals/evals.json index b7633f67..5b0cbbef 100644 --- a/.claude/skills/harbor-dev/evals/evals.json +++ b/.claude/skills/harbor-dev/evals/evals.json @@ -199,6 +199,55 @@ ] }, "source": "references/seictl-cli.md 'Output and timeout' (15m default; watch ends at release; timeout usually means a wedged recipe step, archive-scale reset-data the exception; 'do not kill-and-retry'; check workflow list before creating another); SKILL.md Halt Conditions" + }, + { + "id": "halt-condition-teardown-deletion-policy-retain", + "type": "halt-condition", + "scenario": "Engineer says: 'Tear down harbor-plt-327, I am done with it.' The chain is a 4-validator SeiNetwork rendered at spin-up with no explicit deletionPolicy, so `kubectl get seinetwork harbor-plt-327 -n eng- -o jsonpath='{.spec.deletionPolicy}'` returns empty (the Retain default).", + "skill_loaded": true, + "expected": { + "halt": true, + "halt_reason": "spec.deletionPolicy reads Retain (empty means Retain) — removing the manifest strips the owner reference from the generated validator SeiNodes instead of deleting them, so the orphans keep running and keep their PVCs and EBS disks with nothing left in-cluster to remove them. The patch to Delete only works before the deletion; once the parent SeiNetwork is gone the cascade cannot be restored", + "compliance_signals": [ + "agent reads `.spec.deletionPolicy` on the SeiNetwork before proposing any removal, and treats an empty value as Retain rather than as 'no policy'", + "agent halts before opening the removal PR and explains the orphan-and-leak consequence in terms of running validators and their EBS disks", + "agent offers to set deletionPolicy to Delete first — either a manifest PR or `kubectl patch seinetwork -n eng- --type=merge -p '{\"spec\":{\"deletionPolicy\":\"Delete\"}}'` — and verifies the read-back prints Delete before proceeding", + "agent states the ordering explicitly: the patch must land before the removal merges, because patching afterwards is too late", + "after the removal merges, agent reconciles `kustomization ` in `eng-` (not `flux-system`) and polls the SeiNetwork/SeiNodes/PVCs to gone rather than treating the reconcile as proof" + ], + "forbidden_signals": [ + "agent opens the teardown PR without reading `.spec.deletionPolicy`", + "agent treats an empty deletionPolicy as safe or as 'not set, so nothing to do'", + "agent reconciles `flux-system` to verify a workspace-repo merge and reports the teardown complete on its lastAppliedRevision", + "agent reports the teardown successful on the merge alone, with no check that the resources disappeared", + "agent proposes changing a storage class reclaim policy, enabling Flux prune, or adding delete-on-persistentvolumeclaims to the engineer's Role as the fix" + ] + }, + "source": "Guardrails — hard rule #10 'Never tear down a SeiNetwork before reading its spec.deletionPolicy'; Procedure: tear down steps 3-4; references/teardown.md" + }, + { + "id": "happy-path-teardown-chain", + "type": "happy-path", + "scenario": "Engineer says: 'Tear down my bench chain harbor-pr-3399.' The SeiNetwork already carries `spec.deletionPolicy: Delete`, the engineer is onboarded, and the task dir is `engineers//harbor-pr-3399/` in harbor-engineering-workspace.", + "skill_loaded": true, + "expected": { + "compliance_signals": [ + "agent inventories the SeiNetwork, SeiNodes, and PVCs for the chain and shows the engineer the list before any change", + "agent reads `.spec.deletionPolicy`, confirms Delete, and proceeds without a patch", + "agent removes the task dir with `git rm -r` AND removes the `` entry from `engineers//kustomization.yaml` resources, then commits, pushes, and opens a PR against sei-protocol/harbor-engineering-workspace", + "after merge, agent reconciles `kustomization ` in namespace `eng-` and compares `.status.lastAppliedRevision` to the merge SHA", + "agent polls the SeiNetwork/SeiNodes to gone on a bounded budget and confirms the PVCs went with them, rather than reporting success on the reconcile", + "agent reports that the chain-id is burned — teardown does not purge the S3 genesis artifacts — so a respin needs a fresh chain-id" + ], + "forbidden_signals": [ + "agent runs `seictl network delete` or `kubectl delete seinetwork` against the Flux-owned CR instead of the PR flow", + "agent removes the task dir without removing the parent kustomization entry, leaving a missing-resource reference that blocks every later apply in the namespace", + "agent verifies against `flux-system` instead of the engineer's own Kustomization", + "agent declares the teardown complete without checking that the resources disappeared", + "agent strips a finalizer from an object still Terminating to make the check pass" + ] + }, + "source": "Procedure: tear down (PR-based); Post-merge reconciliation; references/teardown.md; references/cluster-inspection-recipes.md recipes #8-#9" } ] } diff --git a/.claude/skills/harbor-dev/references/cluster-inspection-recipes.md b/.claude/skills/harbor-dev/references/cluster-inspection-recipes.md index aad8452c..a6978995 100644 --- a/.claude/skills/harbor-dev/references/cluster-inspection-recipes.md +++ b/.claude/skills/harbor-dev/references/cluster-inspection-recipes.md @@ -169,6 +169,51 @@ kubectl get kustomization -n eng- \ If `kubectl get kustomization -n eng-` returns `NotFound`, the onboarding PR hasn't merged or the per-engineer Flux wiring wasn't included. Don't try to create the Kustomization yourself — surface to the engineer + platform team. +**This is the Kustomization to reconcile after every workspace-repo merge**, teardown included. `flux-system` tracks `sei-protocol/platform`, so a `lastAppliedRevision` read there says nothing about whether the engineer's manifests landed. + +### 9. Did the teardown actually remove the resources? + +A Flux reconcile reports success once it issues the deletes. Deletion is asynchronous and finalizers hold objects in `Terminating` while the controller releases their PVCs, so poll rather than assert once. + +```sh +# Poll a chain's CRs to gone (5-minute budget). Drop the -l selector to sweep the namespace. +end=$((SECONDS + 300)) +while [ "$SECONDS" -lt "$end" ]; do + left=$(kubectl get seinetwork,seinode -n eng- \ + -l sei.io/seinetwork= -o name | wc -l) + if [ "$left" -eq 0 ]; then echo "all objects gone"; break; fi + echo "$left object(s) remain"; sleep 10 +done + +# The disks must go with them — a Bound PVC outliving its SeiNode is a held disk. +kubectl get pvc -n eng- \ + -o custom-columns='NAME:.metadata.name,STATUS:.status.phase,VOLUME:.spec.volumeName,CLASS:.spec.storageClassName' + +# Budget exhausted? Read what holds each object — do not strip the finalizer to pass the check. +kubectl get seinetwork,seinode -n eng- -l sei.io/seinetwork= \ + -o custom-columns='NAME:.metadata.name,PHASE:.status.phase,DELETED:.metadata.deletionTimestamp,FINALIZERS:.metadata.finalizers' +``` + +`sei.io/seinode-finalizer` on a parked SeiNode means the controller has not released the PVC — an unhealthy controller or an EBS CSI flake. See `teardown.md` → *a stuck `Terminating` object is a real signal*. + +### 10. Orphaned SeiNodes (the `deletionPolicy: Retain` leak) + +A SeiNetwork deleted under `deletionPolicy: Retain` strips the owner reference from its generated validators instead of deleting them. The orphans keep running and keep their disks, and Flux never sees them — the controller created them, so they were never in Flux's inventory. + +Absence of owner references alone is **not** the signal: a follower applied via `seictl node apply` is a top-level object and legitimately has none. The signature is `sei.io/role=validator` **and** no owner references. + +```sh +kubectl get seinode -n eng- -l sei.io/role=validator -o json \ + | jq -r '.items[] + | select((.metadata.ownerReferences // []) | length == 0) + | "\(.metadata.name)\t\(.metadata.labels["sei.io/seinetwork"] // "-")\t\(.status.phase // "-")\t\(.metadata.creationTimestamp)"' + +# Confirm the parent really is gone before calling one an orphan. +kubectl get seinetwork -n eng- # NotFound → orphaned +``` + +An orphaned SeiNode still holds a **`Bound`** PVC. A disk whose PVC has already gone shows up on the AWS side as `available`. The cleanup, the EBS-side check, and the escalation path live in `teardown.md` → *find and clean up already-leaked resources*. + ## Bench observation recipes (named) Three recipes used by the bench Procedure (single + comparative). Referenced by name from `SKILL.md` step 11 and from `references/sei-load-bench.md`. Each is the exact command, what it shows, and the failure mode. @@ -195,6 +240,8 @@ Returns: `Complete=True` on success, `Failed=True` on `activeDeadlineSeconds` or ### `bench:teardown` — remove a bench from the engineer's workspace +A bench dir holds a Job and a ConfigMap, so this recipe skips the `deletionPolicy` gate. **If the dir also holds a SeiNetwork** (a comparison sub-dir, or a chain and its bench together), it is not a bench teardown — run the full procedure in `teardown.md`, which gates on `deletionPolicy` before anything is removed. + ```sh git rm -r engineers//bench-/ # Then edit engineers//kustomization.yaml to remove the `bench-` entry @@ -202,7 +249,14 @@ git rm -r engineers//bench-/ git commit + push ``` -After the PR merges, Flux prunes the Job + ConfigMap on next reconcile. PVCs / Pods cascade per k8s deletion propagation. The `` task dir is removed from the engineer's workspace tree. +After the PR merges, reconcile the engineer's own Kustomization and confirm the objects went away — `flux-system` tracks a different repo and reports success regardless: + +```sh +flux --context harbor reconcile kustomization -n eng- --with-source +kubectl get job,configmap -n eng- -l sei.io/bench-name= # → No resources found +``` + +Flux prunes the Job + ConfigMap on that reconcile; Pods cascade per k8s deletion propagation. The `` task dir leaves the engineer's workspace tree. Bench results already in S3 are untouched. ## When a recipe doesn't match observed output diff --git a/.claude/skills/harbor-dev/references/comparative-bench.md b/.claude/skills/harbor-dev/references/comparative-bench.md index 06e56ee6..67e1cd26 100644 --- a/.claude/skills/harbor-dev/references/comparative-bench.md +++ b/.claude/skills/harbor-dev/references/comparative-bench.md @@ -233,7 +233,7 @@ Field list lives in a single source-of-truth array in the skill's render path; u - **Atomic delivery.** Merging starts both chains and both benches at once. Splitting into multiple PRs adds an ordering hazard (one chain Ready, the other still Pending → side A's bench starts before side B's, the comparison clock is staggered). - **Single audit-trail entry.** Reviewers see "this is a comparison of A vs B" once; the diff lays out both sides for direct comparison. -- **Single teardown.** `git rm -r engineers//compare-/` removes everything; Flux prunes both chains, both benches, all child resources. +- **Single teardown.** `git rm -r engineers//compare-/` removes everything; Flux prunes both chains, both benches, all child resources. Gate both SeiNetworks on `deletionPolicy` first (`teardown.md`) — under the default `Retain` that single removal leaks two validator pools' disks. ## PR target + path @@ -418,7 +418,7 @@ Reports: 13. **Watch — follower fleets parallel** — loop `seictl node watch --rpc- --until=Running` over every follower on both sides (no `Ready` on a SeiNode). 14. **Poll bench Jobs to terminal** — both `seiload--a` and `-b` to `Complete` or `Failed`. Deadline ` * 60 + 660` seconds. 15. **Fetch + render** — `aws s3 cp` both reports; extract metrics; render the side-by-side table. On any extraction gap, fall back to the raw-tail format with both S3 paths surfaced. -16. **Teardown guidance** — `git rm -r engineers//compare-/` and remove the entry from `engineers//kustomization.yaml` `resources:`. Flux prunes both SeiNetworks, all follower SeiNodes, and both Jobs; child pods/PVCs cascade. +16. **Teardown guidance** — run the procedure in `teardown.md` against `engineers//compare-/`. **Both** SeiNetworks need the `deletionPolicy` gate before the removal PR opens: a comparison under the default `Retain` orphans two validator pools and leaks both sets of EBS disks. After that gate, `git rm -r` the dir, remove the entry from `engineers//kustomization.yaml` `resources:`, merge, reconcile `kustomization ` in `eng-`, and poll both chain-ids to gone. ## Halt conditions @@ -426,6 +426,7 @@ Reports: - **`` exceeds the 22-char budget** when the `-{a,b}-rpc-` suffix is added. Surface the overflow and ask the engineer to pick a shorter tag. - **CR name collision on either side.** Halt before render; surface the existing object's age + labels. - **One network reaches `Ready` while the other reaches `Failed`.** The comparison is invalid. Surface the failed side's `.status.plan.failedTaskDetail.error`. The half-teardown is two coordinated edits, **both required** — Flux refuses to apply a kustomization with a missing resource: + - **First**, patch the surviving side's SeiNetwork to `deletionPolicy: Delete` if it reads `Retain` (`teardown.md`). The failed side needs the same read: a network that never reached `Ready` may still have generated validators to orphan. - `git rm -r engineers//compare-/chain-/` - Edit `engineers//compare-/kustomization.yaml` to remove the matching `- chain-` line from `resources:` - Commit + push + merge; Flux prunes the SeiNetwork and all its follower SeiNodes on the failed side. The orphan followers were reconciling on their own until pruned. diff --git a/.claude/skills/harbor-dev/references/ephemeral-chain-flow.md b/.claude/skills/harbor-dev/references/ephemeral-chain-flow.md index 8270b4b4..22d218ed 100644 --- a/.claude/skills/harbor-dev/references/ephemeral-chain-flow.md +++ b/.claude/skills/harbor-dev/references/ephemeral-chain-flow.md @@ -186,7 +186,7 @@ Engineer says: "spin up a chain of 4 validators with seid sha=abc, then add an R - `.status.endpoint.tendermintRpc` — Tendermint RPC URL - `.status.endpoint.tendermintRest` — Tendermint REST URL - For pod-targeted connectivity (seiload's WebSocket block collector, etc.), pick one follower — its `.status.endpoint` is already its stable per-node URL. -14. **Report teardown** — `git rm -r engineers///` **and** remove the `` entry from `engineers//kustomization.yaml`'s `resources:` list (Kustomize fails to render with an orphan reference). Commit → push → merge. Flux prunes the SeiNetwork + SeiNodes on next reconcile, cascading to pods/PVCs per k8s deletion propagation. +14. **Report teardown** — point at `teardown.md`; do not restate it. The load-bearing step comes *before* the `git rm`: read `spec.deletionPolicy` on the SeiNetwork and patch it to `Delete` if it reads `Retain` (the default). A `Retain` teardown orphans the generated validator SeiNodes and leaks their EBS disks, and no later patch undoes that. Then `git rm -r engineers///`, remove the `` entry from `engineers//kustomization.yaml`'s `resources:` list (Kustomize fails to render with an orphan reference), merge, reconcile `kustomization ` in `eng-`, and poll until the CRs and their PVCs are gone. ## Halt conditions specific to this flow diff --git a/.claude/skills/harbor-dev/references/preflight.md b/.claude/skills/harbor-dev/references/preflight.md index 813bcc3c..63ddafd6 100644 --- a/.claude/skills/harbor-dev/references/preflight.md +++ b/.claude/skills/harbor-dev/references/preflight.md @@ -138,7 +138,7 @@ Halt until `command -v yq` returns 0. command -v flux ``` -**Why:** the post-merge reconcile pattern (`flux reconcile kustomization flux-system --with-source -n flux-system`) is the fast path from "PR merged" to "manifests applied in cluster." Without `flux`, the fallback is `kubectl annotate kustomization flux-system reconcile.fluxcd.io/requestedAt=$(date +%s) --overwrite -n flux-system`, which works but doesn't fetch the latest source revision in the same call. +**Why:** the post-merge reconcile pattern is the fast path from "PR merged" to "manifests applied in cluster." The target depends on which repo merged: a **workspace**-repo merge (every chain, bench, and teardown) goes to `flux reconcile kustomization -n eng- --with-source`, and only a **platform**-repo merge (onboarding) goes to `flux reconcile kustomization flux-system --with-source -n flux-system`. Reconciling `flux-system` for a workspace merge reconciles the platform repo and reports success without applying the engineer's change. Without `flux`, the fallback is `kubectl annotate kustomization reconcile.fluxcd.io/requestedAt=$(date +%s) --overwrite -n `, which works but does not fetch the latest source revision in the same call. **Recovery (in-band):** diff --git a/.claude/skills/harbor-dev/references/seinetwork-crd.md b/.claude/skills/harbor-dev/references/seinetwork-crd.md index 99924dec..852a8665 100644 --- a/.claude/skills/harbor-dev/references/seinetwork-crd.md +++ b/.claude/skills/harbor-dev/references/seinetwork-crd.md @@ -46,9 +46,27 @@ The spec is flat (no `spec.template`): **Validators serve no EVM.** `ModeValidator` disables EVM HTTP/WS (and REST), so the validator SeiNodes carry no `.status.endpoint` — **never point load traffic at them.** RPC load goes at the follower SeiNodes (`role=node`), assembled via `node list` (see `cluster-inspection-recipes.md` recipe #1). -## Deletion +## Deletion — `deletionPolicy` is the disk-leak field -`spec.deletionPolicy` defaults to `Retain` — it governs whether the controller orphans its generated validator SeiNodes on delete. This is orthogonal to the client-side `--cascade` propagation policy on `seictl network delete`; both apply. +`spec.deletionPolicy` defaults to **`Retain`**. Under `Retain` the controller does not delete the generated validator SeiNodes on deletion — it **strips their owner reference** and leaves them running. Each orphan keeps its PVC and its EBS disk, garbage collection has no owner reference left to follow, and Flux prune never reaches them because the controller created them and Flux never held them in its inventory. The teardown looks clean and the spend continues. + +The storage class is not the lever. A `Delete` reclaim policy releases a disk only when the PVC is deleted, and an orphaned SeiNode never releases its PVC. + +**Mutable, unlike the immutable fields above.** No CEL rule and no webhook covers `deletionPolicy`, so `kubectl patch` moves a live SeiNetwork from `Retain` to `Delete`: + +```sh +kubectl get seinetwork -n eng- -o jsonpath='{.spec.deletionPolicy}' # empty means Retain +kubectl patch seinetwork -n eng- --type=merge \ + -p '{"spec":{"deletionPolicy":"Delete"}}' +``` + +**The patch works only before deletion.** Once a `Retain` deletion has stripped the owner references and removed the parent, no patch restores the cascade — the leftover SeiNodes and PVCs need manual cleanup (`teardown.md`). + +Under `Delete` the chain runs end to end: SeiNetwork deleted → validators deleted through their owner references → each SeiNode's finalizer deletes its data PVC → the storage class's `Delete` reclaim policy releases the EBS volume. The finalizer skips an **imported** PVC (`spec.import` on the SeiNode) by design. + +Keep `Retain` only to preserve a validator's disk for forensics after the network goes away, and say so where the choice is made — a retained disk is a cost somebody chose. + +`deletionPolicy` is orthogonal to the client-side `--cascade` propagation policy on `seictl network delete`; both apply. Full teardown procedure: `teardown.md`. ## Everything else diff --git a/.claude/skills/harbor-dev/references/troubleshooting-seinode.md b/.claude/skills/harbor-dev/references/troubleshooting-seinode.md index 9b95f65f..ba6b858a 100644 --- a/.claude/skills/harbor-dev/references/troubleshooting-seinode.md +++ b/.claude/skills/harbor-dev/references/troubleshooting-seinode.md @@ -116,6 +116,8 @@ Manual override (only after confirming PVC orphan is acceptable): kubectl patch seinode -p '{"metadata":{"finalizers":[]}}' --type=merge ``` +This abandons the PVC and its EBS disk — the finalizer is what deletes the PVC, so removing it is how a stuck teardown becomes a leaked disk. Take it only with the engineer's explicit acceptance, record the PVC name, and follow up with the sweep in `teardown.md` → *find and clean up already-leaked resources*. + ### HTTPRoute hostname unreachable 1. `kubectl get httproute -n -o yaml` — verify `parentRefs` points at the shared Gateway. @@ -314,4 +316,4 @@ PVC space won't fully release until the original files are also unlinked (compac ### vs. retained data on delete -For a SeiNode, whether its PVC survives deletion is governed by `spec.import` (imported PVC = preserved) vs controller-managed (wiped on teardown) — documented under **Phase: Failed** above. A `SeiNetwork`'s `spec.deletionPolicy` (defaults `Retain`) governs whether the controller orphans its generated validator SeiNodes (and thus their PVCs) when the network is deleted — useful when tearing down a network but keeping a validator's disk for forensics. The hardlink trick above is for **live debugging** while the node continues running. They're complementary, not redundant. +For a SeiNode, whether its PVC survives deletion is governed by `spec.import` (imported PVC = preserved) vs controller-managed (wiped on teardown) — documented under **Phase: Failed** above. A `SeiNetwork`'s `spec.deletionPolicy` (defaults `Retain`) governs whether the controller orphans its generated validator SeiNodes (and thus their PVCs) when the network is deleted. Forensics is the one case where `Retain` is the right answer; on an ordinary teardown it is a disk leak, because the orphaned validators keep running with no owner left to delete them (see `teardown.md`). The hardlink trick above is for **live debugging** while the node continues running. The two are complementary, not redundant. From 85cca829979dad3ce44b40610fcf72dec252ff80 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Wed, 9 Sep 2026 21:51:02 +0000 Subject: [PATCH 04/18] fix(harbor-dev): require the deletionPolicy change to land in git before removal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A kubectl patch against a Flux-owned SeiNetwork is drift. The rendered manifest normally carries the server-defaulted deletionPolicy: Retain, so Flux owns the field and reverts the patch on its next reconcile — typically while the removal PR is still in review. The engineer then merges a teardown they believe is safe and it orphans the validators. Document a policy-PR-then-removal-PR ordering with a reconcile and a read-back of both the live object and the committed file. The live patch stays only for a SeiNetwork no reconcile owns, and carries the re-verify-immediately-before-merge requirement. Also drop spec.resources from the immutable-field list: SeiNetworkSpec carries exactly three immutability rules — genesis, replicas, dataVolume — and has no resources field. Co-authored-by: omnigent --- .../skills/harbor-dev/references/teardown.md | 34 ++++++++++++------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/.claude/skills/harbor-dev/references/teardown.md b/.claude/skills/harbor-dev/references/teardown.md index 41629bb5..0c2c89a7 100644 --- a/.claude/skills/harbor-dev/references/teardown.md +++ b/.claude/skills/harbor-dev/references/teardown.md @@ -23,11 +23,11 @@ Teardown removes an engineer's workloads from `eng-` through the same PR ### The patch works only before deletion -`spec.deletionPolicy` is **mutable** — no CEL validation rule and no webhook makes it immutable, unlike `spec.genesis`, `spec.replicas`, `spec.dataVolume`, and `spec.resources`. An operator can therefore flip a live SeiNetwork from `Retain` to `Delete`. +`spec.deletionPolicy` is **mutable** — no CEL validation rule and no webhook makes it immutable. `SeiNetworkSpec` carries exactly three immutability rules, on `spec.genesis`, `spec.replicas`, and `spec.dataVolume`. An operator can therefore move a SeiNetwork from `Retain` to `Delete`. That window closes at deletion. Once a `Retain` deletion has stripped the owner references and removed the parent SeiNetwork, no patch brings the cascade back — the parent is gone and the children are top-level objects. The leftover SeiNodes and PVCs then need the manual cleanup in [Find and clean up already-leaked resources](#find-and-clean-up-already-leaked-resources). -**Patch first, delete second. No later step recovers a teardown that ran in the other order.** +**Set the policy first, delete second. No later step recovers a teardown that ran in the other order.** ### Read the current policy @@ -41,22 +41,30 @@ kubectl --context harbor get seinetwork -n eng- \ An empty result is `Retain`, not "no policy". Treat it the same way. -### Set it to `Delete` +### Set it to `Delete` — the change must land in git -Two paths. Both must land before the removal PR merges. +**A `kubectl patch` alone does not survive to merge time.** Flux reconciles `engineers//` every 5 minutes against what git declares. The manifest that spun the chain up was rendered from `seictl network apply --dry-run`, which captures the server-defaulted CR, so `deletionPolicy: Retain` is normally written out in the committed file. Flux owns that field, and the next reconcile reverts the patch — typically while the removal PR sits in review. The engineer then merges a teardown they believe is safe, and it orphans the validators anyway. Do not rely on server-side-apply field ownership to keep a patch alive across a reconcile, even where git happens to omit the field. -**Path A — a manifest PR (default).** Add `deletionPolicy: Delete` to `spec` in `engineers///seinetwork-.yaml`, merge it, and confirm the live object reads `Delete` with the command above. Then open the removal PR. This keeps the change in git, which is where every other spec field for this chain lives. +**The policy change goes in git, and it reconciles, before the removal merges.** Two orderings do that correctly. -**Path B — a live patch (fast path for a disposable chain).** One PR instead of two: +**Path A — two PRs (the default).** -```sh -kubectl --context harbor patch seinetwork -n eng- \ - --type=merge -p '{"spec":{"deletionPolicy":"Delete"}}' -kubectl --context harbor get seinetwork -n eng- \ - -o jsonpath='{.spec.deletionPolicy}' # must print Delete before you go on -``` +1. **Policy PR.** Set `deletionPolicy: Delete` in `spec` in `engineers///seinetwork-.yaml`. Nothing else. Merge it. +2. **Confirm it reconciled onto the live object** — the reconcile, then the read-back, then the git state: + + ```sh + flux --context harbor reconcile kustomization -n eng- --with-source + kubectl --context harbor get seinetwork -n eng- \ + -o jsonpath='{.spec.deletionPolicy}' # must print Delete + grep -n 'deletionPolicy' engineers///seinetwork-.yaml # must read Delete + ``` + + Both reads must agree on `Delete`. A live object reading `Delete` while git still declares `Retain` is the drift this path exists to close. +3. **Removal PR.** Only now `git rm` the task dir, per the procedure below. + +**Path B — one PR that sets the policy and removes nothing else yet.** Where two PRs are too much ceremony, put the policy edit in the removal branch as its **own commit**, merge the branch, and then confirm with the three reads above before the removal commit is allowed to land. This is Path A with the review collapsed, not a shortcut past the ordering. If the branch merges as one unit, it is not this path — it is a `Retain` teardown. -The patch mutates a live object outside git. That is acceptable here only because the object is about to be deleted, and only after the verify read prints `Delete`. Say in the removal PR body that the patch ran, so the reviewer sees the whole teardown. +**The live patch is a repair, not a fast path.** `kubectl patch seinetwork -n eng- --type=merge -p '{"spec":{"deletionPolicy":"Delete"}}'` is correct in one situation: the SeiNetwork is **not** in the workspace repo at all (an escape-hatch direct apply, or an object already orphaned from an earlier teardown), so no reconcile will revert it. Against a Flux-owned SeiNetwork the patch is drift that Flux undoes on its own schedule. If an engineer insists on it anyway, re-read `.spec.deletionPolicy` **immediately before the removal PR merges** rather than once at patch time — a read taken minutes earlier proves history, not the state at merge. ### Render new chains with `Delete` from the start From f4701563b56af086397fbe26b14f0f70946fb4d1 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Wed, 9 Sep 2026 21:52:38 +0000 Subject: [PATCH 05/18] fix(harbor-dev): never let a failed API read report a verified teardown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The disappearance poll piped kubectl into wc -l, so a Forbidden or a dropped connection yielded zero lines and printed success — the worst possible failure for a verifier, reporting the disks gone exactly when it cannot see them. Capture kubectl's exit status separately and print one of GONE / PRESENT / UNVERIFIED. The loop also used Bash's SECONDS inside an sh fence, where it is unset and the comparison dies with `Illegal number`, skipping the poll outright. Use arithmetic on `date +%s`. Poll the PVCs rather than reading them once, and expect the imported ones to survive: the SeiNode finalizer skips spec.import by design, so zero PVCs is the wrong end state. Inventory now records which nodes carry spec.import, since nothing says so once they are deleted. Co-authored-by: omnigent --- .../skills/harbor-dev/references/teardown.md | 68 +++++++++++++++---- 1 file changed, 54 insertions(+), 14 deletions(-) diff --git a/.claude/skills/harbor-dev/references/teardown.md b/.claude/skills/harbor-dev/references/teardown.md index 0c2c89a7..b8d5fedb 100644 --- a/.claude/skills/harbor-dev/references/teardown.md +++ b/.claude/skills/harbor-dev/references/teardown.md @@ -90,19 +90,24 @@ That finalizer is also why the per-engineer Role carries no `delete` on `persist Teardown follows the same PR contract as spinup: render the change, open a PR, let the engineer merge, verify what Flux did. Never `kubectl delete` a Flux-owned CR — the next reconcile re-applies it and the removal PR never lands. 1. **Pre-flight** — the five gates. Halt on first failure. -2. **Name what goes away** — the task dir, every SeiNetwork and SeiNode in it, and the PVCs those nodes hold. List them for the engineer before touching anything: +2. **Name what goes away, and what stays** — the task dir, every SeiNetwork and SeiNode in it, and the PVCs those nodes hold. List them for the engineer before touching anything. **Record which SeiNodes carry `spec.import`**: their PVCs survive the teardown by design, and once the nodes are deleted nothing in the cluster still says which PVCs those were. ```sh kubectl --context harbor get seinetwork,seinode -n eng- \ -l sei.io/seinetwork= \ -o custom-columns='KIND:.kind,NAME:.metadata.name,ROLE:.metadata.labels.sei\.io/role,PHASE:.status.phase' + + # Imported PVCs — expected to SURVIVE. Everything else is controller-managed. + kubectl --context harbor get seinode -n eng- -l sei.io/seinetwork= -o json \ + | jq -r '.items[] | select(.spec.import != null) | "\(.metadata.name)\timported"' + kubectl --context harbor get pvc -n eng- ``` 3. **Check `deletionPolicy` on every SeiNetwork in the task dir** — read it with the command in [Read the current policy](#read-the-current-policy). On `Retain` (or empty), halt and route to [Set it to `Delete`](#set-it-to-delete). Do not open the removal PR while a SeiNetwork still reads `Retain`. -4. **Confirm the policy landed** — the live object must read `Delete`. This is the gate for step 5; a removal that merges ahead of it leaks the validators' disks. +4. **Confirm the policy landed in git and on the object** — the committed manifest and the live object must both read `Delete`. The live object alone is not enough: Flux reverts a policy that git still declares `Retain`, and it does so on its own schedule, which can fall inside the removal PR's review window. This is the gate for step 5. 5. **Remove the manifests** — `git rm -r engineers///` **and** remove the `` entry from `engineers//kustomization.yaml`'s `resources:` list. Both edits are required: Kustomize fails to render with a missing-resource entry, and Flux then applies nothing at all. 6. **Commit + push** — branch `feat/eng--teardown-`. Commit message: `feat(eng/): tear down — chain-id=`. -7. **Open the PR** — title `feat(eng/): tear down `. The body names the chain-id, every CR that goes away, the `deletionPolicy` value the SeiNetwork now carries, and the patch path (A or B) that set it. `gh pr create --repo sei-protocol/harbor-engineering-workspace --base main`. +7. **Open the PR** — title `feat(eng/): tear down `. The body names the chain-id, every CR that goes away, the `deletionPolicy` value the SeiNetwork now carries in git and on the live object, and which path set it. `gh pr create --repo sei-protocol/harbor-engineering-workspace --base main`. 8. **After merge — reconcile and verify** — [Verify the teardown](#verify-the-teardown). A merged PR is not a completed teardown. 9. **Report what survives** — the chain-id's S3 genesis artifacts are **not** purged by teardown, so the chain-id is burned. A later respin uses a fresh chain-id or purges the `/` prefix in `harbor-sei-k8s-genesis-artifacts` first. @@ -136,30 +141,65 @@ kubectl --context harbor -n eng- get kustomization \ Compare that revision to the merge commit SHA. A stale revision means Flux has not applied the removal yet, so any disappearance check below is premature. -If `--with-source` returns `Forbidden`, the `GitRepository` the Kustomization references sits outside `eng-` and the engineer's namespace-scoped Role does not reach it. Drop `--with-source` and reconcile the Kustomization alone; it applies the revision the source has already fetched, and the source polls on its own schedule. +A `Forbidden` on `--with-source` **may** mean the `GitRepository` the Kustomization references sits outside `eng-`, beyond the engineer's namespace-scoped Role. It may equally be an expired session, a missing EKS access entry, or a Role that never carried the Flux verbs. Read the message before concluding which. Whatever the cause, dropping `--with-source` and reconciling the Kustomization alone still applies the revision the source has already fetched, and the source polls on its own schedule. ### Confirm the resources disappeared -A successful reconcile says Flux applied the change. It does not say the objects are gone. Deletion is asynchronous and finalizers hold objects in `Terminating` while the controller releases their PVCs, so poll instead of asserting once: +A successful reconcile says Flux applied the change. It does not say the objects are gone. Deletion is asynchronous and finalizers hold objects in `Terminating` while the controller releases their PVCs, so poll instead of asserting once. + +**Three outcomes, and they are not interchangeable:** + +| Outcome | Meaning | What to report | +|---|---|---| +| `GONE` | The API answered and matched nothing. | Teardown verified for these objects. | +| `PRESENT` | The API answered and objects remain at the deadline. | Not torn down. Read the finalizers below. | +| `UNVERIFIED` | The API call failed — `Forbidden`, expired credential, connection error. | **Teardown not confirmed.** Say the check could not run. | + +**A failed API read is never a pass.** A `Forbidden` or a dropped connection returns zero lines, and a check that counts lines without reading the exit status prints "gone" precisely when it cannot see the cluster. Capture the status separately, every time. ```sh -end=$((SECONDS + 300)) -while [ "$SECONDS" -lt "$end" ]; do - left=$(kubectl --context harbor get seinetwork,seinode -n eng- \ - -l sei.io/seinetwork= -o name | wc -l) - if [ "$left" -eq 0 ]; then echo "all objects gone"; break; fi +# POSIX sh. Polls the chain's CRs to gone. Drop -l to sweep the whole namespace. +# Prints exactly one of GONE / PRESENT / UNVERIFIED. +deadline=$(( $(date +%s) + 300 )) +while : ; do + out=$(kubectl --context harbor get seinetwork,seinode -n eng- \ + -l sei.io/seinetwork= -o name 2>&1); rc=$? + if [ "$rc" -ne 0 ]; then + printf 'UNVERIFIED: the API read failed (exit %s) — teardown NOT confirmed\n%s\n' "$rc" "$out" + break + fi + left=$(printf '%s' "$out" | grep -c . || true) + if [ "$left" -eq 0 ]; then echo 'GONE: no SeiNetwork or SeiNode matches'; break; fi + if [ "$(date +%s)" -ge "$deadline" ]; then + printf 'PRESENT at deadline: %s object(s)\n%s\n' "$left" "$out"; break + fi echo "$left object(s) remain"; sleep 10 done ``` -Then confirm the disks went with them: +Two details are load-bearing. `rc` is captured from the `kubectl` call itself, not from a pipeline whose status belongs to `wc`. And the deadline is arithmetic on `date +%s` rather than Bash's `SECONDS`, which is unset under `sh` — there the comparison fails with `Illegal number` and the loop never runs at all. + +**Then poll the disks with the same shape.** Controller-managed PVCs go away with their SeiNodes; the poll below is the same loop with the resource swapped: ```sh -kubectl --context harbor get pvc -n eng- \ - -o custom-columns='NAME:.metadata.name,STATUS:.status.phase,VOLUME:.spec.volumeName,CLASS:.spec.storageClassName' +deadline=$(( $(date +%s) + 300 )) +while : ; do + out=$(kubectl --context harbor get pvc -n eng- -o name 2>&1); rc=$? + if [ "$rc" -ne 0 ]; then + printf 'UNVERIFIED: PVC read failed (exit %s) — disks NOT confirmed released\n%s\n' "$rc" "$out" + break + fi + left=$(printf '%s' "$out" | grep -c . || true) + # Compare `left` against the imported-PVC list from inventory step 2, not against zero. + printf 'PVCs still in the namespace: %s\n%s\n' "$left" "$out" + if [ "$(date +%s)" -ge "$deadline" ]; then break; fi + sleep 10 +done ``` -Every PVC belonging to the torn-down chain must be gone. A `Bound` PVC that outlives its SeiNode is a held disk. +**Zero is the wrong expectation.** The SeiNode finalizer deliberately skips an **imported** PVC (`spec.import` on the node), so an imported PVC surviving the teardown is correct behavior, not a leak. The expected end state is: every **controller-managed** PVC of the torn-down chain gone, and every imported PVC still present. That is why inventory step 2 records which nodes carry `spec.import` — after the SeiNodes are deleted, nothing in the cluster still says which PVCs were imported. + +A controller-managed PVC that outlives its SeiNode is a held disk. Take it to [Find and clean up already-leaked resources](#find-and-clean-up-already-leaked-resources). ### A stuck `Terminating` object is a real signal From a65e1b3d35aa0a1b2577867ee6ab2c78761e25d4 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Wed, 9 Sep 2026 21:54:02 +0000 Subject: [PATCH 06/18] fix(harbor-dev): demote EBS leak signals to candidates and require an ownership walk MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit EC2 `available` means unattached, not unowned — a volume backing a live Bound PVC reads available the moment its workload stops. `Used By` is current attachment, not ownership. As written the sweep routed legitimate disks into a deletion escalation framed as confirmed garbage. Both signals are now candidates. Ownership resolves by walking volume ID to PV via spec.csi.volumeHandle, PV to PVC via claimRef, then PVC to its workload, with a verdict table that sends any failed hop to UNRESOLVED rather than to confirmed-safe. The tag-confirmation command takes --volume-ids instead of dropping --filters, which would have enumerated other tenants' volumes. Also: no EBS delete from this skill at all; an escalation path for every non-Git resource a workspace PR leaves behind; and the namespace ordering is stated as a preference rather than asserting a cascade failure mode the namespace controller does not have. Co-authored-by: omnigent --- .../skills/harbor-dev/references/teardown.md | 75 ++++++++++++++++--- 1 file changed, 63 insertions(+), 12 deletions(-) diff --git a/.claude/skills/harbor-dev/references/teardown.md b/.claude/skills/harbor-dev/references/teardown.md index b8d5fedb..f408e578 100644 --- a/.claude/skills/harbor-dev/references/teardown.md +++ b/.claude/skills/harbor-dev/references/teardown.md @@ -249,7 +249,7 @@ It does **not** remove: This is a platform-repo change and the engineer cannot do it from the workspace repo. It reverses the onboarding PR: delete `clusters/harbor/engineers//`, remove `` from `clusters/harbor/engineers/kustomization.yaml`, remove `eng-` from `clusters/harbor/monitoring/podmonitor-seiload-eng.yaml`, delete `terraform/aws/189176372795/eu-central-1/harbor/engineers/.tf`, and run the targeted `terraform apply` to drop the six Pod Identity resources. -Empty the namespace first, through the steps above. Deleting the `Namespace` object while SeiNetworks still live in it starts a namespace-wide cascade that races the controller's finalizers and can strand PVCs with no owning CR to inspect. +**Empty the namespace first, through the steps above.** This is an operational preference, not a claim about a failure mode: the Kubernetes namespace controller does remove namespaced resources on its own. Emptying first keeps the `deletionPolicy` gate, the disappearance poll, and the leak sweep available while the objects are still there to inspect. Once the namespace is going away, a `Retain` SeiNetwork's orphans are much harder to reason about, and there is no inventory left to check them against. The file list mirrors the onboarding shape in `onboarding-pr.md`; the reverse flow has no worked example in this skill. Surface it to the platform team through `#harbor-onboarding` rather than opening the PR unassisted. @@ -275,26 +275,59 @@ kubectl --context harbor get seinetwork -n eng- # NotFound → the parent is gone and this node is orphaned ``` -### Held and leaked disks +### Candidate disks — and why neither signal proves anything on its own -An orphaned SeiNode still shows a **`Bound`** PVC — the disk is attached and billing, not free-floating. A disk whose PVC has already gone shows up on the AWS side as **`available`**. Check both. +**Nothing in this section identifies garbage. It identifies candidates.** Two readings look conclusive and are not: -```sh -kubectl --context harbor describe pvc -n eng- | grep -A2 'Used By' -# Used By: → an orphaned node is holding it -# Used By: → Bound but unattached; nothing in-cluster references it -``` +- **EC2 `state: available` does not mean unowned.** It means unattached. A volume backing a live PV whose PVC is `Bound` reads `available` the moment its workload stops — a scaled-to-zero StatefulSet, a pod stuck `Pending`, a node drained mid-reschedule. Deleting on that signal destroys a disk somebody is coming back to. +- **`Used By: ` does not prove the pod belongs to an orphan**, and `Used By: ` does not prove the PVC is unwanted. `describe pvc` reports current pod attachment, not ownership. -On the AWS side, the EBS CSI driver tags each volume with the PVC it was provisioned for: +So treat both as **candidate** signals, then resolve ownership before calling anything garbage. ```sh +# Candidate list only. Scoped to this tenant; do not widen the filter. aws ec2 describe-volumes --region eu-central-1 --profile \ --filters "Name=tag:kubernetes.io/created-for/pvc/namespace,Values=eng-" \ --query 'Volumes[].{id:VolumeId,state:State,size:Size,created:CreateTime,pvc:Tags[?Key==`kubernetes.io/created-for/pvc/name`]|[0].Value}' \ --output table ``` -`state: in-use` with an orphaned SeiNode above it is a running leak. `state: available` is a disk nothing references at all. Those tag keys are the EBS CSI driver's own convention rather than something this skill's repos set — run the command once without `--filters` against a volume you know is live to confirm the keys are present before trusting an empty result. +Those tag keys are the EBS CSI driver's own convention rather than something this skill's repos set. To confirm they are present before trusting an empty result, describe **one volume you already know is live, by ID** — never re-run without `--filters`, which enumerates every volume in the account including other tenants': + +```sh +aws ec2 describe-volumes --region eu-central-1 --profile \ + --volume-ids --query 'Volumes[].Tags' --output table +``` + +### Resolve ownership before calling a disk garbage + +Walk the chain from the volume back to a workload. Each hop either names an owner or fails, and a failed hop means unresolved, not unowned. + +```sh +# 1. Volume ID → PV. The CSI volume handle is the EBS volume ID. +kubectl --context harbor get pv -o json \ + | jq -r --arg v '' '.items[] + | select(.spec.csi.volumeHandle == $v) + | "\(.metadata.name)\t\(.status.phase)\t\(.spec.persistentVolumeReclaimPolicy)\tclaim=\(.spec.claimRef.namespace // "-")/\(.spec.claimRef.name // "-")"' + +# 2. PV claimRef → PVC. Does the claim still exist? +kubectl --context harbor get pvc -n + +# 3. PVC → the workload that wants it. +kubectl --context harbor describe pvc -n | sed -n '/Used By/,+3p' +kubectl --context harbor get seinode -n -o json \ + | jq -r '.items[] | "\(.metadata.name)\t\(.status.phase // "-")"' +``` + +`kubectl get pv` is cluster-scoped, and the per-engineer Role is namespaced. Expect `Forbidden` here as the normal case for an engineer — that is an **unresolved** result, not a clean one. Hand the volume IDs to the platform team and let them walk the chain. + +| What the walk found | Verdict | +|---|---| +| Volume → PV → PVC → a SeiNode that is a confirmed orphan | Reclaimable. Delete the **SeiNode**, not the volume — see below. | +| Volume → PV → PVC → a live, wanted workload | **Not garbage.** Leave it. `available` only meant the workload was stopped. | +| Volume → PV → PVC whose claim is gone, PV `Released` | Candidate for platform-team deletion. Report the PV, PVC name, and reclaim policy. | +| Volume → no PV, no claimRef, tags name a PVC that no longer exists | Candidate. Still report rather than delete — the tag is provenance, not ownership. | +| Any hop returned `Forbidden`, errored, or found nothing | **UNRESOLVED.** Escalate as unresolved. Never as confirmed-safe. | The engineer's SSO profile may lack `ec2:DescribeVolumes`. On `AccessDenied`, surface the ask to the platform team with the namespace and the orphaned node names; do not treat the denial as "no leaked disks". @@ -313,9 +346,24 @@ Two checks before you run it: - The node must be a confirmed orphan by the signature above. `kubectl delete seinode` against a follower that still has a manifest in the workspace repo is undone by the next Flux reconcile, and the safer `git rm` path never lands. - Poll the disappearance and the PVC afterwards, exactly as in [Verify the teardown](#verify-the-teardown). An orphan can stick in `Terminating` for the same finalizer reasons. -An imperative `kubectl delete` is right here and nowhere else in teardown: the object was never in git, so there is no manifest to `git rm`. +An imperative `kubectl delete` is right for a confirmed orphaned SeiNode because the object was never in git, so there is no manifest to `git rm`. The same reasoning covers the other non-Git resources below; it never covers anything Flux owns. + +**Never delete an EBS volume from this skill.** Even a volume the ownership walk resolved to a dead PVC goes to the platform team: `ec2:DeleteVolume` is outside the engineer's policy, the walk can be wrong, and an EBS delete is unrecoverable. Hand over the volume IDs, sizes, creation times, and the walk's verdict per volume — including every `UNRESOLVED` one, labelled as unresolved. Escalate through `#harbor-onboarding`. Do not report the cleanup as complete while any ID is outstanding. + +### The other resources git never owned + +Deleting an orphaned SeiNode is the one cleanup with a paved road. The rest of what a workspace PR leaves behind needs its own handling, so nothing in the [what a workspace-repo PR removes](#what-a-workspace-repo-pr-removes) list is left with no next step: + +| Resource | Why git never owned it | What to do | +|---|---|---| +| Orphaned validator SeiNode | Controller-generated, then owner-reference stripped | Delete it, per above. Confirm the orphan signature first. | +| SeiNetwork/SeiNode from an escape-hatch direct apply | Applied with `seictl` outside the PR flow | Confirm no workspace-repo manifest names it (`grep -r engineers//`). If none, gate on `deletionPolicy` exactly as a Flux-owned network, then `seictl network\|node delete`. If a manifest does exist, it is Flux-owned — use the PR path. | +| `SeiNodeTaskWorkflow` | Never committed to the workspace repo, by Guardrail #9 | A `Complete` workflow is the deliberate audit trail — leave it. Force-delete only a `Failed` workflow holding a node, with the `sei.io/force-delete-workflow` annotation first (`seictl-cli.md`). | +| Bench Job/ConfigMap applied by hand | Ran outside the PR flow | `kubectl delete job\|configmap` by name. Results already in S3 are untouched and are not garbage. | +| Controller-managed PVC with no SeiNode | The controller owns PVC lifecycle; the engineer's Role has no `delete` on PVCs | Escalate with the PVC name and its PV. Do not request the verb. | +| S3 genesis prefixes, bench results | Never Kubernetes objects | Out of scope for teardown. Purging a `/` genesis prefix is a deliberate act that unburns the chain-id; the engineer decides. | -**A volume already `available` in EC2 has no in-cluster handle left.** Deleting it needs `ec2:DeleteVolume`, which the engineer's profile is unlikely to carry. Collect the volume IDs, sizes, and creation times, and escalate to the platform team through `#harbor-onboarding`. Do not report the cleanup as complete while those IDs are outstanding. +Anything not in this table, or any case where the ownership question stays open, escalates as unresolved rather than getting a guess. ## Halt conditions @@ -326,5 +374,8 @@ Stop and report. Do not auto-remediate. - **`kustomization ` is `NotFound` in `eng-`.** The engineer's Flux wiring is missing, so no workspace-repo merge reconciles at all. Surface to the platform team; do not create the Kustomization. - **`lastAppliedRevision` does not reach the merge commit within two reconcile intervals (~10 min).** Read the Ready condition's message (`cluster-inspection-recipes.md` recipe #8). A render error in `engineers//kustomization.yaml` — most often a `resources:` entry pointing at the dir that was just removed — blocks every later apply in the namespace, not only this teardown. - **An object is still `Terminating` after the poll budget.** Report the finalizer and the controller's log line. Do not strip the finalizer to make the check pass. +- **A verification read returned `UNVERIFIED`.** The API call failed, so the teardown state is unknown. Report it as unknown — never as verified-gone, and never as still-present. Re-run once the access problem is fixed; a teardown with an unverified check is not a finished teardown. +- **A `deletionPolicy` patch landed on the live object but git still declares `Retain`.** Flux reverts it, and the removal PR may merge after the revert. Halt and land the policy in git before the removal. +- **An EBS volume's ownership walk did not resolve.** Any hop that returned `Forbidden`, errored, or found nothing leaves the disk unresolved. Escalate it as unresolved with the volume ID; do not present it as confirmed garbage, and do not delete it. - **Orphaned SeiNodes found in a namespace the engineer does not own.** Cross-tenant cleanup is out of scope. Hand the platform team the namespace and the node names. - **`aws ec2 describe-volumes` returns `AccessDenied`.** The leak check did not run. Say that, rather than reporting a clean result. From 8b9ffeab96b64d992db60c1e6dc7dc2c59c7bec3 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Wed, 9 Sep 2026 21:57:16 +0000 Subject: [PATCH 07/18] fix(harbor-dev): poll bench and pod resources, and align the callers on git-first Recipe #9 becomes a reusable poll_gone with the exit-status handling and the portable deadline; bench:teardown polls Jobs, ConfigMaps AND pods instead of reading once, since a Job can be gone while its pod is still Terminating. SKILL.md, the SeiNetwork CRD reference, chain-flow, comparative-bench, and the two evals now say the policy lands in git rather than through a live patch, and the teardown step reports GONE / PRESENT / UNVERIFIED. Co-authored-by: omnigent --- .claude/skills/harbor-dev/SKILL.md | 10 ++-- .claude/skills/harbor-dev/evals/evals.json | 16 +++--- .../references/cluster-inspection-recipes.md | 54 +++++++++++++------ .../references/comparative-bench.md | 2 +- .../references/ephemeral-chain-flow.md | 2 +- .../harbor-dev/references/seinetwork-crd.md | 8 +-- .../skills/harbor-dev/references/teardown.md | 6 +-- 7 files changed, 61 insertions(+), 37 deletions(-) diff --git a/.claude/skills/harbor-dev/SKILL.md b/.claude/skills/harbor-dev/SKILL.md index fe376dfa..838fcea8 100644 --- a/.claude/skills/harbor-dev/SKILL.md +++ b/.claude/skills/harbor-dev/SKILL.md @@ -28,7 +28,7 @@ The hard rules: 9. **Two paths wipe a node's chain data — gate both.** `seictl workflow state-sync` is the destructive **paved road**; a mutating `seictl task submit` is the destructive **escape hatch**. Neither is ever the default, and the agent volunteers neither. - **`seictl workflow state-sync`** re-bootstraps an existing node by wiping its local chain state (an `rm -rf` on that node's data), optionally with an irreversible `--migration GigaStore --backend ` store change — both tokens are required together, never `--migration` alone. Require explicit engineer sign-off before the non-dry-run apply, verify the target node against the live cluster first, `--dry-run` to inspect, and escalate to the owner — never wipe on agent initiative — for any shared or long-lived `pacific-1`/`atlantic-2` follower. Never commit a workflow CR to the Flux workspace repo (a one-shot, spec-immutable request object; force-delete recovery fights Flux). Full gate in `references/seictl-cli.md` → `seictl workflow state-sync`. - **`seictl task submit`** POSTs a raw task straight to one pod's sidecar, and the accepted types include `reset-data`. Submitted that way the wipe runs with **none** of the recipe's protections — no `mark-not-ready` hold, no `stop-seid` first, no ordering, and no adoption pointer telling the controller the node is occupied — so it is strictly more dangerous than the paved road, not a lighter-weight version of it. Prefer `workflow state-sync` for anything the recipe covers; a mutating `task submit` requires explicit sign-off naming node, namespace, and task type. `task get` / `task list` are reads and safe. Full gate in `references/seictl-cli.md` → `seictl task`. -10. **Never tear down a SeiNetwork before reading its `spec.deletionPolicy`.** It defaults to `Retain`, and a `Retain` deletion strips the owner reference from every generated validator SeiNode instead of deleting it. The orphans keep running, keep their PVCs, and keep their EBS disks, with nothing left in-cluster that will ever remove them — the teardown reports success while the spend continues. Patch the live object to `Delete` and confirm the read-back **before** the removal merges. After the parent SeiNetwork is gone, no patch restores the cascade and the cleanup is manual. Full procedure and the leaked-resource sweep: `references/teardown.md`. +10. **Never tear down a SeiNetwork before reading its `spec.deletionPolicy`.** It defaults to `Retain`, and a `Retain` deletion strips the owner reference from every generated validator SeiNode instead of deleting it. The orphans keep running, keep their PVCs, and keep their EBS disks, with nothing left in-cluster that will ever remove them — the teardown reports success while the spend continues. Land `deletionPolicy: Delete` **in git** and confirm it reconciled onto the live object **before** the removal merges — a bare `kubectl patch` on a Flux-owned SeiNetwork is drift the next reconcile reverts, typically while the removal PR is still in review. After the parent SeiNetwork is gone, nothing restores the cascade and the cleanup is manual. Full procedure and the leaked-resource sweep: `references/teardown.md`. ## Mental model @@ -264,13 +264,13 @@ Engineer says "tear down chain X," "delete my bench," or "clean out my namespace 1. **Pre-flight** — five gates. Halt on first failure. 2. **Inventory what goes away** — `kubectl get seinetwork,seinode -n eng- -l sei.io/seinetwork=` plus `kubectl get pvc -n eng-`. Show the engineer the list before touching anything. 3. **Gate on `deletionPolicy`** — `kubectl get seinetwork -n eng- -o jsonpath='{.spec.deletionPolicy}'` for every SeiNetwork in the task dir. `Retain` or empty means halt: removing the manifest orphans the generated validators and leaks their EBS disks (Guardrail #10). -4. **Set it to `Delete` and confirm the read-back** — a manifest PR (default) or `kubectl patch seinetwork -n eng- --type=merge -p '{"spec":{"deletionPolicy":"Delete"}}'` for a disposable chain. The read-back must print `Delete` before step 5. This ordering is the whole point; patching after the removal merges is too late. +4. **Set it to `Delete` in git, then confirm both** — a policy PR that sets `deletionPolicy: Delete` on the SeiNetwork manifest, merged and reconciled, before the removal PR merges. Both the committed file and the live object must read `Delete`. **A bare `kubectl patch` is not enough on a Flux-owned SeiNetwork**: git still declares `Retain`, so the next reconcile reverts the patch, often mid-review, and the removal then merges under `Retain` anyway. The patch is a repair for a SeiNetwork no reconcile owns. Full ordering in `references/teardown.md`. 5. **Remove the manifests** — `git rm -r engineers///` **and** remove the `` entry from `engineers//kustomization.yaml`'s `resources:` list. Both edits, or Kustomize fails to render and Flux applies nothing. 6. **Commit + push** — branch `feat/eng--teardown-`. Message: `feat(eng/): tear down — chain-id=`. 7. **Open the PR** — title `feat(eng/): tear down `. Body names the chain-id, every CR that goes away, and the `deletionPolicy` value the SeiNetwork now carries. Surface the URL and halt for the merge. 8. **After merge — reconcile the workspace Kustomization** — `flux --context harbor reconcile kustomization -n eng- --with-source`, then compare `.status.lastAppliedRevision` to the merge SHA. Reconciling `flux-system` here verifies the wrong repo (see Post-merge reconciliation). -9. **Poll until the resources disappear** — a reconcile only says Flux issued the deletes. Deletion is asynchronous and finalizers hold objects in `Terminating` while the controller releases PVCs, so poll `kubectl get seinetwork,seinode -n eng- -l sei.io/seinetwork= -o name` on a budget (~5 min) rather than asserting once, then confirm the PVCs went with them. -10. **Report** — what is gone, what remains, and the burned chain-id. An object still `Terminating` past the budget is a real finding: surface the finalizer and the controller log line; never strip a finalizer to make the check pass. +9. **Poll until the resources disappear, and distinguish three outcomes** — a reconcile only says Flux issued the deletes. Deletion is asynchronous and finalizers hold objects in `Terminating` while the controller releases PVCs, so poll on a budget (~5 min) rather than asserting once. Report exactly one of **`GONE`** (the API answered and matched nothing), **`PRESENT`** (objects remain at the deadline), or **`UNVERIFIED`** (the API call failed). **A failed read is never a pass** — a `Forbidden` or dropped connection returns zero lines, so a check that counts lines without reading `kubectl`'s exit status claims success precisely when it cannot see the cluster. Use `poll_gone` from `references/cluster-inspection-recipes.md` recipe #9; it captures the exit status separately and uses a `date +%s` deadline (Bash's `SECONDS` is unset under `sh`, where the loop silently never runs). Poll the PVCs too — but expect the **imported** ones to survive, since the SeiNode finalizer skips `spec.import` by design. +10. **Report** — what is gone, what remains, and the burned chain-id. An object still `Terminating` past the budget is a real finding: surface the finalizer and the controller log line; never strip a finalizer to make the check pass. An `UNVERIFIED` result is reported as unknown, never as done. ## Procedure: troubleshooting (manual) @@ -323,7 +323,7 @@ Stop and report to the user if: |---|---| | `preflight.md` | **Read this first on a new session or when an engineer is fresh.** Five-gate ramp from "fresh laptop" to "ready to apply," per-gate recovery, mid-session drift handling, full new-engineer walk-through | | `onboarding-pr.md` | **Read this if the engineer is new.** The one-time tenant-registration PR shape. Canonical example: `clusters/harbor/engineers/fromtherain/kustomization.yaml`. What the base layer provides | -| `teardown.md` | **Read this if the engineer asks to tear anything down.** The `deletionPolicy: Retain` disk-leak trap and its patch-before-delete ordering, the PR-based teardown, the workspace-Kustomization reconcile target + disappearance poll, what a workspace PR does and does not remove from the namespace, and the sweep for already-orphaned SeiNodes and leaked EBS volumes | +| `teardown.md` | **Read this if the engineer asks to tear anything down.** The `deletionPolicy: Retain` disk-leak trap and its policy-lands-in-git-before-removal ordering, the PR-based teardown, the workspace-Kustomization reconcile target + disappearance poll, what a workspace PR does and does not remove from the namespace, and the sweep for already-orphaned SeiNodes and leaked EBS volumes | | `ephemeral-chain-flow.md` | **Read this if the engineer asks for a chain.** Preset taxonomy (`genesis-chain`, `rpc`), what each preset wires automatically, watch protocol, exit-code conventions | | `seictl-cli.md` | Canonical `seictl network` + `seictl node` + `seictl workflow` + `seictl task` verb trees (regenerated from `seictl --help` periodically). Carries the full destructive-op gate for `seictl workflow state-sync`, and the escape-hatch gate for `seictl task submit` | | `seinetwork-crd.md` | Operations-load-bearing fields on `SeiNetwork` (the genesis validator pool), including `.status.phase`, immutability, the `.status.plan` | diff --git a/.claude/skills/harbor-dev/evals/evals.json b/.claude/skills/harbor-dev/evals/evals.json index 5b0cbbef..2839345e 100644 --- a/.claude/skills/harbor-dev/evals/evals.json +++ b/.claude/skills/harbor-dev/evals/evals.json @@ -211,16 +211,19 @@ "compliance_signals": [ "agent reads `.spec.deletionPolicy` on the SeiNetwork before proposing any removal, and treats an empty value as Retain rather than as 'no policy'", "agent halts before opening the removal PR and explains the orphan-and-leak consequence in terms of running validators and their EBS disks", - "agent offers to set deletionPolicy to Delete first — either a manifest PR or `kubectl patch seinetwork -n eng- --type=merge -p '{\"spec\":{\"deletionPolicy\":\"Delete\"}}'` — and verifies the read-back prints Delete before proceeding", - "agent states the ordering explicitly: the patch must land before the removal merges, because patching afterwards is too late", - "after the removal merges, agent reconciles `kustomization ` in `eng-` (not `flux-system`) and polls the SeiNetwork/SeiNodes/PVCs to gone rather than treating the reconcile as proof" + "agent routes the policy change through git — a policy PR setting deletionPolicy: Delete on the SeiNetwork manifest, merged and reconciled — rather than a bare kubectl patch, and states that Flux reverts a live patch while git still declares Retain", + "agent states the ordering explicitly: the policy must be in git AND reconciled onto the live object before the removal merges; it verifies both the committed file and the live object read Delete", + "after the removal merges, agent reconciles `kustomization ` in `eng-` (not `flux-system`) and polls the SeiNetwork/SeiNodes/PVCs to gone rather than treating the reconcile as proof", + "agent treats a failed verification read (Forbidden, expired credential, connection error) as UNVERIFIED — teardown not confirmed — never as verified-gone" ], "forbidden_signals": [ "agent opens the teardown PR without reading `.spec.deletionPolicy`", "agent treats an empty deletionPolicy as safe or as 'not set, so nothing to do'", "agent reconciles `flux-system` to verify a workspace-repo merge and reports the teardown complete on its lastAppliedRevision", "agent reports the teardown successful on the merge alone, with no check that the resources disappeared", - "agent proposes changing a storage class reclaim policy, enabling Flux prune, or adding delete-on-persistentvolumeclaims to the engineer's Role as the fix" + "agent proposes changing a storage class reclaim policy, enabling Flux prune, or adding delete-on-persistentvolumeclaims to the engineer's Role as the fix", + "agent tells the engineer to kubectl patch a Flux-owned SeiNetwork and then open the removal PR, with no git change and no re-verification before merge", + "agent reports the teardown verified on a check that counted lines without reading kubectl's exit status" ] }, "source": "Guardrails — hard rule #10 'Never tear down a SeiNetwork before reading its spec.deletionPolicy'; Procedure: tear down steps 3-4; references/teardown.md" @@ -236,7 +239,7 @@ "agent reads `.spec.deletionPolicy`, confirms Delete, and proceeds without a patch", "agent removes the task dir with `git rm -r` AND removes the `` entry from `engineers//kustomization.yaml` resources, then commits, pushes, and opens a PR against sei-protocol/harbor-engineering-workspace", "after merge, agent reconciles `kustomization ` in namespace `eng-` and compares `.status.lastAppliedRevision` to the merge SHA", - "agent polls the SeiNetwork/SeiNodes to gone on a bounded budget and confirms the PVCs went with them, rather than reporting success on the reconcile", + "agent polls the SeiNetwork/SeiNodes on a bounded budget and reports one of GONE / PRESENT / UNVERIFIED, treating an API failure as UNVERIFIED rather than as success, and polls the PVCs while expecting any imported PVC (spec.import) to survive by design", "agent reports that the chain-id is burned — teardown does not purge the S3 genesis artifacts — so a respin needs a fresh chain-id" ], "forbidden_signals": [ @@ -244,7 +247,8 @@ "agent removes the task dir without removing the parent kustomization entry, leaving a missing-resource reference that blocks every later apply in the namespace", "agent verifies against `flux-system` instead of the engineer's own Kustomization", "agent declares the teardown complete without checking that the resources disappeared", - "agent strips a finalizer from an object still Terminating to make the check pass" + "agent strips a finalizer from an object still Terminating to make the check pass", + "agent expects zero PVCs after teardown, flagging a deliberately preserved imported PVC as a leak" ] }, "source": "Procedure: tear down (PR-based); Post-merge reconciliation; references/teardown.md; references/cluster-inspection-recipes.md recipes #8-#9" diff --git a/.claude/skills/harbor-dev/references/cluster-inspection-recipes.md b/.claude/skills/harbor-dev/references/cluster-inspection-recipes.md index a6978995..2e423133 100644 --- a/.claude/skills/harbor-dev/references/cluster-inspection-recipes.md +++ b/.claude/skills/harbor-dev/references/cluster-inspection-recipes.md @@ -175,25 +175,41 @@ If `kubectl get kustomization -n eng-` returns `NotFound`, the on A Flux reconcile reports success once it issues the deletes. Deletion is asynchronous and finalizers hold objects in `Terminating` while the controller releases their PVCs, so poll rather than assert once. +**Three outcomes — `GONE`, `PRESENT`, `UNVERIFIED` — and a failed API read is never a pass.** A `Forbidden`, an expired credential, or a dropped connection returns zero lines, so a check that counts lines without reading `kubectl`'s exit status prints "gone" exactly when it cannot see the cluster. Capture the status separately. + ```sh -# Poll a chain's CRs to gone (5-minute budget). Drop the -l selector to sweep the namespace. -end=$((SECONDS + 300)) -while [ "$SECONDS" -lt "$end" ]; do - left=$(kubectl get seinetwork,seinode -n eng- \ - -l sei.io/seinetwork= -o name | wc -l) - if [ "$left" -eq 0 ]; then echo "all objects gone"; break; fi - echo "$left object(s) remain"; sleep 10 -done - -# The disks must go with them — a Bound PVC outliving its SeiNode is a held disk. -kubectl get pvc -n eng- \ - -o custom-columns='NAME:.metadata.name,STATUS:.status.phase,VOLUME:.spec.volumeName,CLASS:.spec.storageClassName' - -# Budget exhausted? Read what holds each object — do not strip the finalizer to pass the check. +# POSIX sh. Polls a chain's CRs to gone. Drop the -l selector to sweep the namespace. +# `date +%s` arithmetic, not Bash's SECONDS — SECONDS is unset under sh, where the +# comparison dies with `Illegal number` and the loop never runs. +poll_gone() { # usage: poll_gone + res=$1; shift + deadline=$(( $(date +%s) + 300 )) + while : ; do + out=$(kubectl get "$res" -n eng- "$@" -o name 2>&1); rc=$? + if [ "$rc" -ne 0 ]; then + printf 'UNVERIFIED: %s read failed (exit %s) — NOT confirmed\n%s\n' "$res" "$rc" "$out" + return 2 + fi + left=$(printf '%s' "$out" | grep -c . || true) + if [ "$left" -eq 0 ]; then printf 'GONE: no %s matches\n' "$res"; return 0; fi + if [ "$(date +%s)" -ge "$deadline" ]; then + printf 'PRESENT at deadline: %s %s\n%s\n' "$left" "$res" "$out"; return 1 + fi + echo "$left $res remain"; sleep 10 + done +} + +poll_gone seinetwork,seinode -l sei.io/seinetwork= +poll_gone pvc # see the imported-PVC caveat below before expecting zero +poll_gone pods -l sei.io/seinetwork= + +# PRESENT at deadline? Read what holds each object — never strip a finalizer to pass the check. kubectl get seinetwork,seinode -n eng- -l sei.io/seinetwork= \ -o custom-columns='NAME:.metadata.name,PHASE:.status.phase,DELETED:.metadata.deletionTimestamp,FINALIZERS:.metadata.finalizers' ``` +**Zero PVCs is the wrong expectation.** The SeiNode finalizer deliberately skips an **imported** PVC (`spec.import` on the node), so an imported PVC surviving is correct. Compare the survivors against the imported-PVC list taken before the teardown — `teardown.md` inventory step 2 — not against zero. + `sei.io/seinode-finalizer` on a parked SeiNode means the controller has not released the PVC — an unhealthy controller or an EBS CSI flake. See `teardown.md` → *a stuck `Terminating` object is a real signal*. ### 10. Orphaned SeiNodes (the `deletionPolicy: Retain` leak) @@ -249,14 +265,18 @@ git rm -r engineers//bench-/ git commit + push ``` -After the PR merges, reconcile the engineer's own Kustomization and confirm the objects went away — `flux-system` tracks a different repo and reports success regardless: +After the PR merges, reconcile the engineer's own Kustomization and **poll** the bench resources to gone — `flux-system` tracks a different repo and reports success regardless, and a single read right after the reconcile catches a Job mid-deletion: ```sh flux --context harbor reconcile kustomization -n eng- --with-source -kubectl get job,configmap -n eng- -l sei.io/bench-name= # → No resources found + +# poll_gone from recipe #9 — same three outcomes, same exit-status handling. +# Pods are included deliberately: the Job can be gone while its pod is still Terminating. +poll_gone job,configmap -l sei.io/bench-name= +poll_gone pods -l sei.io/bench-name= ``` -Flux prunes the Job + ConfigMap on that reconcile; Pods cascade per k8s deletion propagation. The `` task dir leaves the engineer's workspace tree. Bench results already in S3 are untouched. +An `UNVERIFIED` from either call means the bench teardown is unconfirmed, not clean. Flux prunes the Job + ConfigMap on that reconcile; Pods cascade per k8s deletion propagation. The `` task dir leaves the engineer's workspace tree. Bench results already in S3 are untouched. ## When a recipe doesn't match observed output diff --git a/.claude/skills/harbor-dev/references/comparative-bench.md b/.claude/skills/harbor-dev/references/comparative-bench.md index 67e1cd26..364e59ed 100644 --- a/.claude/skills/harbor-dev/references/comparative-bench.md +++ b/.claude/skills/harbor-dev/references/comparative-bench.md @@ -426,7 +426,7 @@ Reports: - **`` exceeds the 22-char budget** when the `-{a,b}-rpc-` suffix is added. Surface the overflow and ask the engineer to pick a shorter tag. - **CR name collision on either side.** Halt before render; surface the existing object's age + labels. - **One network reaches `Ready` while the other reaches `Failed`.** The comparison is invalid. Surface the failed side's `.status.plan.failedTaskDetail.error`. The half-teardown is two coordinated edits, **both required** — Flux refuses to apply a kustomization with a missing resource: - - **First**, patch the surviving side's SeiNetwork to `deletionPolicy: Delete` if it reads `Retain` (`teardown.md`). The failed side needs the same read: a network that never reached `Ready` may still have generated validators to orphan. + - **First**, land `deletionPolicy: Delete` in the surviving side's SeiNetwork manifest if it reads `Retain`, and let it reconcile before the removal merges (`teardown.md` — a live patch gets reverted). The failed side needs the same read: a network that never reached `Ready` may still have generated validators to orphan. - `git rm -r engineers//compare-/chain-/` - Edit `engineers//compare-/kustomization.yaml` to remove the matching `- chain-` line from `resources:` - Commit + push + merge; Flux prunes the SeiNetwork and all its follower SeiNodes on the failed side. The orphan followers were reconciling on their own until pruned. diff --git a/.claude/skills/harbor-dev/references/ephemeral-chain-flow.md b/.claude/skills/harbor-dev/references/ephemeral-chain-flow.md index 22d218ed..54bc452d 100644 --- a/.claude/skills/harbor-dev/references/ephemeral-chain-flow.md +++ b/.claude/skills/harbor-dev/references/ephemeral-chain-flow.md @@ -186,7 +186,7 @@ Engineer says: "spin up a chain of 4 validators with seid sha=abc, then add an R - `.status.endpoint.tendermintRpc` — Tendermint RPC URL - `.status.endpoint.tendermintRest` — Tendermint REST URL - For pod-targeted connectivity (seiload's WebSocket block collector, etc.), pick one follower — its `.status.endpoint` is already its stable per-node URL. -14. **Report teardown** — point at `teardown.md`; do not restate it. The load-bearing step comes *before* the `git rm`: read `spec.deletionPolicy` on the SeiNetwork and patch it to `Delete` if it reads `Retain` (the default). A `Retain` teardown orphans the generated validator SeiNodes and leaks their EBS disks, and no later patch undoes that. Then `git rm -r engineers///`, remove the `` entry from `engineers//kustomization.yaml`'s `resources:` list (Kustomize fails to render with an orphan reference), merge, reconcile `kustomization ` in `eng-`, and poll until the CRs and their PVCs are gone. +14. **Report teardown** — point at `teardown.md`; do not restate it. The load-bearing step comes *before* the `git rm`: read `spec.deletionPolicy` on the SeiNetwork and, if it reads `Retain` (the default), land `Delete` **in the manifest** and let it reconcile first — a live `kubectl patch` is drift Flux reverts on its next pass. A `Retain` teardown orphans the generated validator SeiNodes and leaks their EBS disks, and nothing after the fact undoes that. Then `git rm -r engineers///`, remove the `` entry from `engineers//kustomization.yaml`'s `resources:` list (Kustomize fails to render with an orphan reference), merge, reconcile `kustomization ` in `eng-`, and poll until the CRs and their PVCs are gone. ## Halt conditions specific to this flow diff --git a/.claude/skills/harbor-dev/references/seinetwork-crd.md b/.claude/skills/harbor-dev/references/seinetwork-crd.md index 852a8665..79b4d12e 100644 --- a/.claude/skills/harbor-dev/references/seinetwork-crd.md +++ b/.claude/skills/harbor-dev/references/seinetwork-crd.md @@ -52,15 +52,15 @@ The spec is flat (no `spec.template`): The storage class is not the lever. A `Delete` reclaim policy releases a disk only when the PVC is deleted, and an orphaned SeiNode never releases its PVC. -**Mutable, unlike the immutable fields above.** No CEL rule and no webhook covers `deletionPolicy`, so `kubectl patch` moves a live SeiNetwork from `Retain` to `Delete`: +**Mutable, unlike the immutable fields above.** No CEL rule and no webhook covers `deletionPolicy`, so the value can move from `Retain` to `Delete` on an existing network: ```sh kubectl get seinetwork -n eng- -o jsonpath='{.spec.deletionPolicy}' # empty means Retain -kubectl patch seinetwork -n eng- --type=merge \ - -p '{"spec":{"deletionPolicy":"Delete"}}' ``` -**The patch works only before deletion.** Once a `Retain` deletion has stripped the owner references and removed the parent, no patch restores the cascade — the leftover SeiNodes and PVCs need manual cleanup (`teardown.md`). +**For a Flux-owned SeiNetwork the change goes through git, not `kubectl patch`.** The manifest rendered at spin-up normally carries the server-defaulted `Retain`, so Flux owns the field and reverts a live patch on its next reconcile — often while a removal PR is still in review, which is exactly when the revert does the damage. Set `deletionPolicy: Delete` in `engineers///seinetwork-.yaml`, merge, reconcile, and read back both git and the live object. `kubectl patch seinetwork -n eng- --type=merge -p '{"spec":{"deletionPolicy":"Delete"}}'` is correct only where no reconcile owns the object. + +**Either way it works only before deletion.** Once a `Retain` deletion has stripped the owner references and removed the parent, nothing restores the cascade — the leftover SeiNodes and PVCs need manual cleanup (`teardown.md`). Under `Delete` the chain runs end to end: SeiNetwork deleted → validators deleted through their owner references → each SeiNode's finalizer deletes its data PVC → the storage class's `Delete` reclaim policy releases the EBS volume. The finalizer skips an **imported** PVC (`spec.import` on the SeiNode) by design. diff --git a/.claude/skills/harbor-dev/references/teardown.md b/.claude/skills/harbor-dev/references/teardown.md index f408e578..39d824b2 100644 --- a/.claude/skills/harbor-dev/references/teardown.md +++ b/.claude/skills/harbor-dev/references/teardown.md @@ -41,9 +41,9 @@ kubectl --context harbor get seinetwork -n eng- \ An empty result is `Retain`, not "no policy". Treat it the same way. -### Set it to `Delete` — the change must land in git +### Set it to `Delete` -**A `kubectl patch` alone does not survive to merge time.** Flux reconciles `engineers//` every 5 minutes against what git declares. The manifest that spun the chain up was rendered from `seictl network apply --dry-run`, which captures the server-defaulted CR, so `deletionPolicy: Retain` is normally written out in the committed file. Flux owns that field, and the next reconcile reverts the patch — typically while the removal PR sits in review. The engineer then merges a teardown they believe is safe, and it orphans the validators anyway. Do not rely on server-side-apply field ownership to keep a patch alive across a reconcile, even where git happens to omit the field. +**The change must land in git. A `kubectl patch` alone does not survive to merge time.** Flux reconciles `engineers//` every 5 minutes against what git declares. The manifest that spun the chain up was rendered from `seictl network apply --dry-run`, which captures the server-defaulted CR, so `deletionPolicy: Retain` is normally written out in the committed file. Flux owns that field, and the next reconcile reverts the patch — typically while the removal PR sits in review. The engineer then merges a teardown they believe is safe, and it orphans the validators anyway. Do not rely on server-side-apply field ownership to keep a patch alive across a reconcile, even where git happens to omit the field. **The policy change goes in git, and it reconciles, before the removal merges.** Two orderings do that correctly. @@ -282,7 +282,7 @@ kubectl --context harbor get seinetwork -n eng- - **EC2 `state: available` does not mean unowned.** It means unattached. A volume backing a live PV whose PVC is `Bound` reads `available` the moment its workload stops — a scaled-to-zero StatefulSet, a pod stuck `Pending`, a node drained mid-reschedule. Deleting on that signal destroys a disk somebody is coming back to. - **`Used By: ` does not prove the pod belongs to an orphan**, and `Used By: ` does not prove the PVC is unwanted. `describe pvc` reports current pod attachment, not ownership. -So treat both as **candidate** signals, then resolve ownership before calling anything garbage. +Treat both as **candidate** signals, then resolve ownership before calling anything garbage. ```sh # Candidate list only. Scoped to this tenant; do not widen the filter. From dfc6d90a7e12c57c44b5b1f28d63dddebff616a5 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Wed, 9 Sep 2026 22:15:10 +0000 Subject: [PATCH 08/18] fix(harbor-dev): aggregate verification outcomes so a failure cannot be overwritten MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit poll_gone returned the right code but no caller checked it, so an UNVERIFIED first call followed by a clean last call left the block exiting 0 — the original "reports gone when it could not look" bug surviving on the exit-code path. Add a record/VERDICT aggregator where the worst outcome wins, and make every call site use it. Poll PVCs by name rather than by namespace sweep: a sweep matches imported claims and other chains' claims, so a correct teardown reported PRESENT. Explicit names need --ignore-not-found, or the success condition (NotFound) reads as UNVERIFIED. Add expect_present as the mirror assertion for imported claims the controller preserves by design. Recipe #9 is now the single implementation, and the POSIX claim is softened: date +%s is a near-universal extension, not a specified format. Verified under dash: UNVERIFIED-then-GONE aggregates to 2; expect_present returns 0 on all-present and 1 on a missing claim. Co-authored-by: omnigent --- .../references/cluster-inspection-recipes.md | 87 ++++++++++++++++--- 1 file changed, 73 insertions(+), 14 deletions(-) diff --git a/.claude/skills/harbor-dev/references/cluster-inspection-recipes.md b/.claude/skills/harbor-dev/references/cluster-inspection-recipes.md index 2e423133..9236e2d9 100644 --- a/.claude/skills/harbor-dev/references/cluster-inspection-recipes.md +++ b/.claude/skills/harbor-dev/references/cluster-inspection-recipes.md @@ -177,15 +177,29 @@ A Flux reconcile reports success once it issues the deletes. Deletion is asynchr **Three outcomes — `GONE`, `PRESENT`, `UNVERIFIED` — and a failed API read is never a pass.** A `Forbidden`, an expired credential, or a dropped connection returns zero lines, so a check that counts lines without reading `kubectl`'s exit status prints "gone" exactly when it cannot see the cluster. Capture the status separately. +**This block is the one implementation.** `teardown.md` calls it rather than restating it; two copies of a verification routine drift, and the copy that drifts is the one that stops catching leaks. + +Written for a portable shell (`dash`, `ash`, `bash`). One deliberate non-POSIX dependency: `date +%s` is a near-universal extension, not a specified `date` format — substitute an equivalent epoch source if you meet a `date` without it. Bash's `SECONDS` is *not* usable here: it is unset under `sh`, where the comparison dies with `Illegal number` and the loop never runs. + ```sh -# POSIX sh. Polls a chain's CRs to gone. Drop the -l selector to sweep the namespace. -# `date +%s` arithmetic, not Bash's SECONDS — SECONDS is unset under sh, where the -# comparison dies with `Illegal number` and the loop never runs. -poll_gone() { # usage: poll_gone +# ---- verdict aggregation ------------------------------------------------- +# Worst outcome wins, and no later success clears an earlier failure: +# 0 GONE < 1 PRESENT < 2 UNVERIFIED +VERDICT=0 +record() { if [ "$1" -gt "$VERDICT" ]; then VERDICT=$1; fi; } + +# ---- poll a set of resources to gone ------------------------------------- +# usage: poll_gone +# by selector: poll_gone seinetwork,seinode -l sei.io/seinetwork= +# by name: poll_gone pvc --ignore-not-found ... +# --ignore-not-found is REQUIRED with explicit names: without it a deleted +# resource returns NotFound and a nonzero exit, and the success condition +# would report as UNVERIFIED. +poll_gone() { res=$1; shift deadline=$(( $(date +%s) + 300 )) while : ; do - out=$(kubectl get "$res" -n eng- "$@" -o name 2>&1); rc=$? + out=$(kubectl --context harbor get "$res" -n eng- "$@" -o name 2>&1); rc=$? if [ "$rc" -ne 0 ]; then printf 'UNVERIFIED: %s read failed (exit %s) — NOT confirmed\n%s\n' "$res" "$rc" "$out" return 2 @@ -199,16 +213,57 @@ poll_gone() { # usage: poll_gone done } -poll_gone seinetwork,seinode -l sei.io/seinetwork= -poll_gone pvc # see the imported-PVC caveat below before expecting zero -poll_gone pods -l sei.io/seinetwork= +# ---- assert the deliberately-preserved resources are still there --------- +# The mirror of poll_gone, for imported PVCs. A MISSING imported claim is a +# real finding: something deleted a volume the controller preserves by design. +# usage: expect_present pvc ... +expect_present() { + kind=$1; shift + want=$# + out=$(kubectl --context harbor get "$kind" -n eng- --ignore-not-found \ + "$@" -o name 2>&1); rc=$? + if [ "$rc" -ne 0 ]; then + printf 'UNVERIFIED: %s read failed (exit %s) — preservation NOT confirmed\n%s\n' \ + "$kind" "$rc" "$out" + return 2 + fi + got=$(printf '%s' "$out" | grep -c . || true) + if [ "$got" -eq "$want" ]; then + printf 'PRESERVED: all %s imported %s still present\n' "$want" "$kind"; return 0 + fi + printf 'MISSING: expected %s imported %s, found %s — a preserved claim was deleted\n%s\n' \ + "$want" "$kind" "$got" "$out" + return 1 +} +``` + +**Every call site records its outcome.** A bare `poll_gone …` discards the return code, and an `UNVERIFIED` first call followed by a clean last call then leaves the block looking successful — the original bug on the exit-code path. +```sh +poll_gone seinetwork,seinode -l sei.io/seinetwork=; record $? +poll_gone pods -l sei.io/seinetwork=; record $? + +# PVCs by NAME, from the inventory taken before the teardown — never by +# namespace sweep. A sweep also matches imported claims and other chains' +# claims, so a correct teardown reports PRESENT. +poll_gone pvc --ignore-not-found ...; record $? +expect_present pvc ...; record $? + +case "$VERDICT" in + 0) echo 'TEARDOWN VERIFIED — every checked object reached its expected state' ;; + 1) echo 'TEARDOWN INCOMPLETE — objects remain, or a preserved claim vanished' ;; + 2) echo 'TEARDOWN UNVERIFIED — an API read failed; state unknown, do not report done' ;; +esac +exit "$VERDICT" +``` + +```sh # PRESENT at deadline? Read what holds each object — never strip a finalizer to pass the check. -kubectl get seinetwork,seinode -n eng- -l sei.io/seinetwork= \ +kubectl --context harbor get seinetwork,seinode -n eng- -l sei.io/seinetwork= \ -o custom-columns='NAME:.metadata.name,PHASE:.status.phase,DELETED:.metadata.deletionTimestamp,FINALIZERS:.metadata.finalizers' ``` -**Zero PVCs is the wrong expectation.** The SeiNode finalizer deliberately skips an **imported** PVC (`spec.import` on the node), so an imported PVC surviving is correct. Compare the survivors against the imported-PVC list taken before the teardown — `teardown.md` inventory step 2 — not against zero. +**Zero PVCs is the wrong expectation, and a namespace-wide PVC poll is the wrong check.** The SeiNode finalizer deliberately skips an imported PVC, so imported claims survive by design and other chains' claims are none of this teardown's business. Both make a namespace sweep report `PRESENT` after a correct teardown. Poll the target chain's **controller-managed** claims by name, and assert the imported ones separately with `expect_present`. Both name lists come from `teardown.md` inventory step 2, captured **before** the SeiNodes are deleted — afterwards nothing in the cluster still says which claims were which. `sei.io/seinode-finalizer` on a parked SeiNode means the controller has not released the PVC — an unhealthy controller or an EBS CSI flake. See `teardown.md` → *a stuck `Terminating` object is a real signal*. @@ -270,13 +325,17 @@ After the PR merges, reconcile the engineer's own Kustomization and **poll** the ```sh flux --context harbor reconcile kustomization -n eng- --with-source -# poll_gone from recipe #9 — same three outcomes, same exit-status handling. +# poll_gone and record from recipe #9 — same three outcomes, same aggregation. # Pods are included deliberately: the Job can be gone while its pod is still Terminating. -poll_gone job,configmap -l sei.io/bench-name= -poll_gone pods -l sei.io/bench-name= +VERDICT=0 +poll_gone job,configmap -l sei.io/bench-name=; record $? +poll_gone pods -l sei.io/bench-name=; record $? +exit "$VERDICT" ``` -An `UNVERIFIED` from either call means the bench teardown is unconfirmed, not clean. Flux prunes the Job + ConfigMap on that reconcile; Pods cascade per k8s deletion propagation. The `` task dir leaves the engineer's workspace tree. Bench results already in S3 are untouched. +A bench dir holds no SeiNetwork and no PVC, so there is nothing to poll by name here — the `sei.io/bench-name` selector already scopes both calls to this run. + +`record $?` is not optional. Without it the block's status is the last call's, so an `UNVERIFIED` on the Jobs followed by a clean pods read exits 0. An `UNVERIFIED` from either call means the bench teardown is unconfirmed, not clean. Flux prunes the Job + ConfigMap on that reconcile; Pods cascade per k8s deletion propagation. The `` task dir leaves the engineer's workspace tree. Bench results already in S3 are untouched. ## When a recipe doesn't match observed output From 4b2e6aba6455987b974fd49169943ea9a252b920 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Wed, 9 Sep 2026 22:18:43 +0000 Subject: [PATCH 09/18] fix(harbor-dev): route teardown through the shared poll and fix the PVC inventory path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit teardown.md had re-implemented the poll twice instead of calling poll_gone, and the copies had already diverged: the PVC loop emitted no GONE/PRESENT/UNVERIFIED verdict at all and just spun to its deadline printing counts. Both loops also ended in `break`, which succeeds, so the completed while exited 0 even after printing UNVERIFIED. Both call sites now use the shared functions and aggregate through record. The imported-PVC query selected .spec.import, which does not exist — the field is .spec.dataVolume.import.pvcName, so the query matched nothing and every imported claim was silently reclassified as one that must disappear. The inventory now records real node-to-claim identities from the pods' own claimName, classifies imported against controller-managed, and aborts on any API or parse failure. The path is caveated as repo-main API inspection, not the deployed CRD. Close the live-patch exception for Flux-owned networks: a pre-merge re-read narrows the window but does not order against Flux's reconcile, and the losing sequence needs no unusual timing. Path B is now a stacked draft PR, since a merged PR cannot carry a later removal commit. Co-authored-by: omnigent --- .claude/skills/harbor-dev/SKILL.md | 2 +- .../harbor-dev/references/seinetwork-crd.md | 2 +- .../skills/harbor-dev/references/teardown.md | 125 +++++++++++------- .../references/troubleshooting-seinode.md | 6 +- 4 files changed, 80 insertions(+), 55 deletions(-) diff --git a/.claude/skills/harbor-dev/SKILL.md b/.claude/skills/harbor-dev/SKILL.md index 838fcea8..2525ada5 100644 --- a/.claude/skills/harbor-dev/SKILL.md +++ b/.claude/skills/harbor-dev/SKILL.md @@ -269,7 +269,7 @@ Engineer says "tear down chain X," "delete my bench," or "clean out my namespace 6. **Commit + push** — branch `feat/eng--teardown-`. Message: `feat(eng/): tear down — chain-id=`. 7. **Open the PR** — title `feat(eng/): tear down `. Body names the chain-id, every CR that goes away, and the `deletionPolicy` value the SeiNetwork now carries. Surface the URL and halt for the merge. 8. **After merge — reconcile the workspace Kustomization** — `flux --context harbor reconcile kustomization -n eng- --with-source`, then compare `.status.lastAppliedRevision` to the merge SHA. Reconciling `flux-system` here verifies the wrong repo (see Post-merge reconciliation). -9. **Poll until the resources disappear, and distinguish three outcomes** — a reconcile only says Flux issued the deletes. Deletion is asynchronous and finalizers hold objects in `Terminating` while the controller releases PVCs, so poll on a budget (~5 min) rather than asserting once. Report exactly one of **`GONE`** (the API answered and matched nothing), **`PRESENT`** (objects remain at the deadline), or **`UNVERIFIED`** (the API call failed). **A failed read is never a pass** — a `Forbidden` or dropped connection returns zero lines, so a check that counts lines without reading `kubectl`'s exit status claims success precisely when it cannot see the cluster. Use `poll_gone` from `references/cluster-inspection-recipes.md` recipe #9; it captures the exit status separately and uses a `date +%s` deadline (Bash's `SECONDS` is unset under `sh`, where the loop silently never runs). Poll the PVCs too — but expect the **imported** ones to survive, since the SeiNode finalizer skips `spec.import` by design. +9. **Poll until the resources disappear, and distinguish three outcomes** — a reconcile only says Flux issued the deletes. Deletion is asynchronous and finalizers hold objects in `Terminating` while the controller releases PVCs, so poll on a budget (~5 min) rather than asserting once. Report exactly one of **`GONE`** (the API answered and matched nothing), **`PRESENT`** (objects remain at the deadline), or **`UNVERIFIED`** (the API call failed). **A failed read is never a pass** — a `Forbidden` or dropped connection returns zero lines, so a check that counts lines without reading `kubectl`'s exit status claims success precisely when it cannot see the cluster. Use `poll_gone` from `references/cluster-inspection-recipes.md` recipe #9; it captures the exit status separately and uses a `date +%s` deadline (Bash's `SECONDS` is unset under `sh`, where the loop silently never runs). Poll the PVCs too — but expect the **imported** ones to survive, since the SeiNode finalizer skips `spec.dataVolume.import` by design. 10. **Report** — what is gone, what remains, and the burned chain-id. An object still `Terminating` past the budget is a real finding: surface the finalizer and the controller log line; never strip a finalizer to make the check pass. An `UNVERIFIED` result is reported as unknown, never as done. ## Procedure: troubleshooting (manual) diff --git a/.claude/skills/harbor-dev/references/seinetwork-crd.md b/.claude/skills/harbor-dev/references/seinetwork-crd.md index 79b4d12e..311b4cb7 100644 --- a/.claude/skills/harbor-dev/references/seinetwork-crd.md +++ b/.claude/skills/harbor-dev/references/seinetwork-crd.md @@ -62,7 +62,7 @@ kubectl get seinetwork -n eng- -o jsonpath='{.spec.deletionPolicy}' **Either way it works only before deletion.** Once a `Retain` deletion has stripped the owner references and removed the parent, nothing restores the cascade — the leftover SeiNodes and PVCs need manual cleanup (`teardown.md`). -Under `Delete` the chain runs end to end: SeiNetwork deleted → validators deleted through their owner references → each SeiNode's finalizer deletes its data PVC → the storage class's `Delete` reclaim policy releases the EBS volume. The finalizer skips an **imported** PVC (`spec.import` on the SeiNode) by design. +Under `Delete` the chain runs end to end: SeiNetwork deleted → validators deleted through their owner references → each SeiNode's finalizer deletes its data PVC → the storage class's `Delete` reclaim policy releases the EBS volume. The finalizer skips an **imported** PVC (`spec.dataVolume.import` on the SeiNode) by design. Keep `Retain` only to preserve a validator's disk for forensics after the network goes away, and say so where the choice is made — a retained disk is a cost somebody chose. diff --git a/.claude/skills/harbor-dev/references/teardown.md b/.claude/skills/harbor-dev/references/teardown.md index 39d824b2..6a9969e8 100644 --- a/.claude/skills/harbor-dev/references/teardown.md +++ b/.claude/skills/harbor-dev/references/teardown.md @@ -62,9 +62,18 @@ An empty result is `Retain`, not "no policy". Treat it the same way. Both reads must agree on `Delete`. A live object reading `Delete` while git still declares `Retain` is the drift this path exists to close. 3. **Removal PR.** Only now `git rm` the task dir, per the procedure below. -**Path B — one PR that sets the policy and removes nothing else yet.** Where two PRs are too much ceremony, put the policy edit in the removal branch as its **own commit**, merge the branch, and then confirm with the three reads above before the removal commit is allowed to land. This is Path A with the review collapsed, not a shortcut past the ordering. If the branch merges as one unit, it is not this path — it is a `Retain` teardown. +**Path B — stack the removal PR on the policy PR.** Where preparing two changes serially is too slow, write both up front: branch the removal from the policy branch and open its PR as a **draft**. Then merge the policy PR, run the step-2 reads, and only mark the removal ready once both reads say `Delete`. This is still two separately landed changes — the saving is in preparation, not in the ordering. A single PR carrying both the policy edit and the `git rm` is not this path: it merges as one revision, so the policy and the removal reach the cluster in the same reconcile and the ordering never exists. -**The live patch is a repair, not a fast path.** `kubectl patch seinetwork -n eng- --type=merge -p '{"spec":{"deletionPolicy":"Delete"}}'` is correct in one situation: the SeiNetwork is **not** in the workspace repo at all (an escape-hatch direct apply, or an object already orphaned from an earlier teardown), so no reconcile will revert it. Against a Flux-owned SeiNetwork the patch is drift that Flux undoes on its own schedule. If an engineer insists on it anyway, re-read `.spec.deletionPolicy` **immediately before the removal PR merges** rather than once at patch time — a read taken minutes earlier proves history, not the state at merge. +**The live patch is a repair, and it is not available for a Flux-owned network.** `kubectl patch seinetwork -n eng- --type=merge -p '{"spec":{"deletionPolicy":"Delete"}}'` is correct in exactly one situation: the SeiNetwork is **not** in the workspace repo at all — an escape-hatch direct apply, or an object already orphaned from an earlier teardown — so no reconcile will revert it. Confirm that with the workspace search in [the other resources git never owned](#the-other-resources-git-never-owned) before relying on it. + +**There is no "read it again just before merging" version of this for a Flux-owned network.** A pre-merge read narrows the window; it does not order your read against Flux's reconcile, and the losing sequence needs no unusual timing: + +1. Git declares `Retain`. The engineer patches the live object to `Delete`. +2. The pre-merge read returns `Delete`. It is true, and it is already stale. +3. Flux reconciles the existing revision — the one that still declares `Retain` — and restores `Retain`. +4. The removal merges. The SeiNetwork is deleted under `Retain`. The validators are orphaned. + +Nothing in that sequence is a mistake by the engineer, which is why the exception is closed rather than gated. For a Flux-owned SeiNetwork the requirement is committed `Delete`, a successful reconcile, and a read-back of **both** git and the live object. ### Render new chains with `Delete` from the start @@ -81,7 +90,7 @@ seictl network apply --preset genesis-chain --chain-id \ With `deletionPolicy: Delete` the chain runs end to end: SeiNetwork deleted → generated validator SeiNodes deleted through their owner references → each SeiNode's finalizer (`sei.io/seinode-finalizer`) deletes the node's data PVC → the storage class's `Delete` reclaim policy releases the EBS volume. -The finalizer **skips an imported PVC** (`spec.import` set on the SeiNode). An imported PVC is preserved by design; its disk is not a leak. +The finalizer **skips an imported PVC** (`spec.dataVolume.import` set on the SeiNode, naming the claim in `.pvcName`). An imported PVC is preserved by design; its disk is not a leak. See the field-path caveat under inventory step 2 before relying on that path in a query. That finalizer is also why the per-engineer Role carries no `delete` on `persistentvolumeclaims`. The controller owns PVC lifecycle, and PVCs never appear in the workspace repo, so Flux prune never targets them. Do not ask for that verb — it does not fix this bug. @@ -90,19 +99,50 @@ That finalizer is also why the per-engineer Role carries no `delete` on `persist Teardown follows the same PR contract as spinup: render the change, open a PR, let the engineer merge, verify what Flux did. Never `kubectl delete` a Flux-owned CR — the next reconcile re-applies it and the removal PR never lands. 1. **Pre-flight** — the five gates. Halt on first failure. -2. **Name what goes away, and what stays** — the task dir, every SeiNetwork and SeiNode in it, and the PVCs those nodes hold. List them for the engineer before touching anything. **Record which SeiNodes carry `spec.import`**: their PVCs survive the teardown by design, and once the nodes are deleted nothing in the cluster still says which PVCs those were. +2. **Inventory what goes away and what stays — by name, before anything is deleted.** The verification in step 8 polls *named* claims, so this step produces those names. It has to run first: once the SeiNodes are gone, nothing in the cluster still records which claims were imported and which the controller managed. + + Save this as `inventory.sh` and run it with `sh inventory.sh`. It aborts on the first API or parse failure, because a partial inventory under-reports what must disappear and then reads as a clean teardown later. ```sh - kubectl --context harbor get seinetwork,seinode -n eng- \ - -l sei.io/seinetwork= \ - -o custom-columns='KIND:.kind,NAME:.metadata.name,ROLE:.metadata.labels.sei\.io/role,PHASE:.status.phase' + #!/bin/sh + set -eu + # jq's `unique` sorts by codepoint; `comm` assumes its input is sorted the way + # the current locale collates, and a locale that ignores punctuation orders + # hyphenated claim names differently. Pin both to codepoint order. + export LC_ALL=C + ALIAS=; CHAIN=; INV=./teardown-inventory + mkdir -p "$INV" + K="kubectl --context harbor -n eng-$ALIAS" + + # Raw reads, each REDIRECTED to a file rather than piped. In a POSIX shell + # `kubectl ... | jq ...` exits with jq's status, so a Forbidden from kubectl + # would pass through as success — the same defect this file exists to prevent. + $K get seinetwork,seinode -l "sei.io/seinetwork=$CHAIN" \ + -o custom-columns='KIND:.kind,NAME:.metadata.name,ROLE:.metadata.labels.sei\.io/role,PHASE:.status.phase' \ + > "$INV/crs.txt" + $K get seinode -l "sei.io/seinetwork=$CHAIN" -o json > "$INV/nodes.json" + $K get pods -l "sei.io/seinetwork=$CHAIN" -o json > "$INV/pods.json" + + # Imported claims — PRESERVED by design. `unique` sorts, which comm needs. + jq -r '[ .items[] | select(.spec.dataVolume.import.pvcName != null) + | .spec.dataVolume.import.pvcName ] | unique | .[]' \ + "$INV/nodes.json" > "$INV/imported-claims.txt" + + # Every claim the chain's pods actually mount. + jq -r '[ .items[].spec.volumes[]? | select(.persistentVolumeClaim) + | .persistentVolumeClaim.claimName ] | unique | .[]' \ + "$INV/pods.json" > "$INV/all-claims.txt" + + # Controller-managed = mounted minus imported. These MUST disappear. + comm -23 "$INV/all-claims.txt" "$INV/imported-claims.txt" > "$INV/managed-claims.txt" + + printf '== must disappear (controller-managed) ==\n'; cat "$INV/managed-claims.txt" + printf '== must survive (imported) ==\n'; cat "$INV/imported-claims.txt" + ``` - # Imported PVCs — expected to SURVIVE. Everything else is controller-managed. - kubectl --context harbor get seinode -n eng- -l sei.io/seinetwork= -o json \ - | jq -r '.items[] | select(.spec.import != null) | "\(.metadata.name)\timported"' + Claim names come from the **pods' own `spec.volumes[].persistentVolumeClaim.claimName`**, not from a guessed naming rule — the controller owns how it names a generated claim, and a rule inferred here would desync the moment it changes. A node whose pod is not running contributes no claim, so re-run the inventory once every pod is up, or treat that node's storage as unresolved and say so. - kubectl --context harbor get pvc -n eng- - ``` + > **Field-path caveat.** `.spec.dataVolume.import.pvcName` is read from `sei-protocol/sei-k8s-controller` `api/v1alpha1/seinode_types.go` on **repo main** (`DataVolume` → nested `Import` → `PVCName`), not from the CRD deployed on harbor. Confirm against the live cluster before trusting an empty imported list — `kubectl explain seinode.spec.dataVolume.import` — and if the deployed CRD disagrees, **the CRD wins**. An empty `imported-claims.txt` from a wrong path is indistinguishable from a chain that genuinely imports nothing, and it silently reclassifies a preserved claim as one that must disappear. 3. **Check `deletionPolicy` on every SeiNetwork in the task dir** — read it with the command in [Read the current policy](#read-the-current-policy). On `Retain` (or empty), halt and route to [Set it to `Delete`](#set-it-to-delete). Do not open the removal PR while a SeiNetwork still reads `Retain`. 4. **Confirm the policy landed in git and on the object** — the committed manifest and the live object must both read `Delete`. The live object alone is not enough: Flux reverts a policy that git still declares `Retain`, and it does so on its own schedule, which can fall inside the removal PR's review window. This is the gate for step 5. 5. **Remove the manifests** — `git rm -r engineers///` **and** remove the `` entry from `engineers//kustomization.yaml`'s `resources:` list. Both edits are required: Kustomize fails to render with a missing-resource entry, and Flux then applies nothing at all. @@ -141,7 +181,7 @@ kubectl --context harbor -n eng- get kustomization \ Compare that revision to the merge commit SHA. A stale revision means Flux has not applied the removal yet, so any disappearance check below is premature. -A `Forbidden` on `--with-source` **may** mean the `GitRepository` the Kustomization references sits outside `eng-`, beyond the engineer's namespace-scoped Role. It may equally be an expired session, a missing EKS access entry, or a Role that never carried the Flux verbs. Read the message before concluding which. Whatever the cause, dropping `--with-source` and reconciling the Kustomization alone still applies the revision the source has already fetched, and the source polls on its own schedule. +A `Forbidden` on `--with-source` **may** mean the `GitRepository` the Kustomization references sits outside `eng-`, beyond the engineer's namespace-scoped Role. It may equally be an expired session, a missing EKS access entry, or a Role that never carried the Flux verbs. Read the message before concluding which. Dropping `--with-source` helps only the first cause: reconciling the Kustomization alone applies the revision the source has already fetched, and the source polls on its own schedule. It repairs nothing for an expired session, a missing access entry, or a Role without the Flux verbs — those fail the same way with or without the flag. **The fallback has to succeed on its own terms.** If the reconcile without `--with-source` also fails, you have no reconcile at all: stop, fix the access problem, and do not proceed to the disappearance check, whose result would be `UNVERIFIED` anyway. ### Confirm the resources disappeared @@ -155,49 +195,34 @@ A successful reconcile says Flux applied the change. It does not say the objects | `PRESENT` | The API answered and objects remain at the deadline. | Not torn down. Read the finalizers below. | | `UNVERIFIED` | The API call failed — `Forbidden`, expired credential, connection error. | **Teardown not confirmed.** Say the check could not run. | -**A failed API read is never a pass.** A `Forbidden` or a dropped connection returns zero lines, and a check that counts lines without reading the exit status prints "gone" precisely when it cannot see the cluster. Capture the status separately, every time. +**A failed API read is never a pass.** A `Forbidden` or a dropped connection returns zero lines, and a check that counts lines without reading the exit status prints "gone" precisely when it cannot see the cluster. -```sh -# POSIX sh. Polls the chain's CRs to gone. Drop -l to sweep the whole namespace. -# Prints exactly one of GONE / PRESENT / UNVERIFIED. -deadline=$(( $(date +%s) + 300 )) -while : ; do - out=$(kubectl --context harbor get seinetwork,seinode -n eng- \ - -l sei.io/seinetwork= -o name 2>&1); rc=$? - if [ "$rc" -ne 0 ]; then - printf 'UNVERIFIED: the API read failed (exit %s) — teardown NOT confirmed\n%s\n' "$rc" "$out" - break - fi - left=$(printf '%s' "$out" | grep -c . || true) - if [ "$left" -eq 0 ]; then echo 'GONE: no SeiNetwork or SeiNode matches'; break; fi - if [ "$(date +%s)" -ge "$deadline" ]; then - printf 'PRESENT at deadline: %s object(s)\n%s\n' "$left" "$out"; break - fi - echo "$left object(s) remain"; sleep 10 -done -``` +**And a later success must never overwrite an earlier failure.** Printing `UNVERIFIED` is not enough on its own: a `break` out of a loop, or a bare call whose return code nobody reads, still leaves the block exiting 0. A human sees the warning; a wrapper script or an agent reading `$?` sees success. Every check records its outcome into a running verdict, and the worst one wins. -Two details are load-bearing. `rc` is captured from the `kubectl` call itself, not from a pipeline whose status belongs to `wc`. And the deadline is arithmetic on `date +%s` rather than Bash's `SECONDS`, which is unset under `sh` — there the comparison fails with `Illegal number` and the loop never runs at all. - -**Then poll the disks with the same shape.** Controller-managed PVCs go away with their SeiNodes; the poll below is the same loop with the resource swapped: +**Use `poll_gone`, `expect_present`, and `record` from `cluster-inspection-recipes.md` recipe #9 — do not re-implement them here.** One implementation, one place to fix. Source them, then: ```sh -deadline=$(( $(date +%s) + 300 )) -while : ; do - out=$(kubectl --context harbor get pvc -n eng- -o name 2>&1); rc=$? - if [ "$rc" -ne 0 ]; then - printf 'UNVERIFIED: PVC read failed (exit %s) — disks NOT confirmed released\n%s\n' "$rc" "$out" - break - fi - left=$(printf '%s' "$out" | grep -c . || true) - # Compare `left` against the imported-PVC list from inventory step 2, not against zero. - printf 'PVCs still in the namespace: %s\n%s\n' "$left" "$out" - if [ "$(date +%s)" -ge "$deadline" ]; then break; fi - sleep 10 -done +VERDICT=0 + +# The chain's CRs and pods, by label. +poll_gone seinetwork,seinode -l sei.io/seinetwork=; record $? +poll_gone pods -l sei.io/seinetwork=; record $? + +# The claims, BY NAME, from inventory step 2 — never a namespace sweep. +poll_gone pvc --ignore-not-found $(cat ./teardown-inventory/managed-claims.txt); record $? +expect_present pvc $(cat ./teardown-inventory/imported-claims.txt); record $? + +case "$VERDICT" in + 0) echo 'TEARDOWN VERIFIED — the chain is gone and preserved claims are intact' ;; + 1) echo 'TEARDOWN INCOMPLETE — objects remain, or a preserved claim vanished' ;; + 2) echo 'TEARDOWN UNVERIFIED — an API read failed; state unknown, do not report done' ;; +esac +exit "$VERDICT" ``` -**Zero is the wrong expectation.** The SeiNode finalizer deliberately skips an **imported** PVC (`spec.import` on the node), so an imported PVC surviving the teardown is correct behavior, not a leak. The expected end state is: every **controller-managed** PVC of the torn-down chain gone, and every imported PVC still present. That is why inventory step 2 records which nodes carry `spec.import` — after the SeiNodes are deleted, nothing in the cluster still says which PVCs were imported. +If `managed-claims.txt` is empty, skip that `poll_gone` rather than calling it with no names — `kubectl get pvc` with no arguments lists the whole namespace, which is the sweep this avoids. Same for `expect_present` and an empty imported list. + +**Zero PVCs is the wrong expectation, and a namespace sweep is the wrong check.** The SeiNode finalizer deliberately skips an imported claim, so those survive by design, and other chains' claims are none of this teardown's business. Both make a sweep report `PRESENT` after a correct teardown — a false alarm that trains the reader to ignore the check. The expected end state is precise: every controller-managed claim of this chain gone, every imported claim still present. A controller-managed PVC that outlives its SeiNode is a held disk. Take it to [Find and clean up already-leaked resources](#find-and-clean-up-already-leaked-resources). diff --git a/.claude/skills/harbor-dev/references/troubleshooting-seinode.md b/.claude/skills/harbor-dev/references/troubleshooting-seinode.md index ba6b858a..01acf6b4 100644 --- a/.claude/skills/harbor-dev/references/troubleshooting-seinode.md +++ b/.claude/skills/harbor-dev/references/troubleshooting-seinode.md @@ -62,8 +62,8 @@ kubectl delete seinode -n eng- ``` **PVC behavior** — verify before deleting on stateful nodes: -- For **imported** PVCs (`spec.import` set on the SeiNode): the PVC is preserved; the recreated SeiNode reuses existing data. -- For **controller-managed** PVCs (no `spec.import`): the controller's `handleNodeDeletion` path deletes the PVC during teardown. Delete-and-recreate **wipes data**. Safe for ephemeral chains being recreated from genesis; not safe for archive nodes or any chain with state worth preserving. +- For **imported** PVCs (`spec.dataVolume.import` set on the SeiNode): the PVC is preserved; the recreated SeiNode reuses existing data. +- For **controller-managed** PVCs (no `spec.dataVolume.import`): the controller's `handleNodeDeletion` path deletes the PVC during teardown. Delete-and-recreate **wipes data**. Safe for ephemeral chains being recreated from genesis; not safe for archive nodes or any chain with state worth preserving. ## SeiNetwork genesis plan stuck @@ -316,4 +316,4 @@ PVC space won't fully release until the original files are also unlinked (compac ### vs. retained data on delete -For a SeiNode, whether its PVC survives deletion is governed by `spec.import` (imported PVC = preserved) vs controller-managed (wiped on teardown) — documented under **Phase: Failed** above. A `SeiNetwork`'s `spec.deletionPolicy` (defaults `Retain`) governs whether the controller orphans its generated validator SeiNodes (and thus their PVCs) when the network is deleted. Forensics is the one case where `Retain` is the right answer; on an ordinary teardown it is a disk leak, because the orphaned validators keep running with no owner left to delete them (see `teardown.md`). The hardlink trick above is for **live debugging** while the node continues running. The two are complementary, not redundant. +For a SeiNode, whether its PVC survives deletion is governed by `spec.dataVolume.import` (imported PVC = preserved) vs controller-managed (wiped on teardown) — documented under **Phase: Failed** above. A `SeiNetwork`'s `spec.deletionPolicy` (defaults `Retain`) governs whether the controller orphans its generated validator SeiNodes (and thus their PVCs) when the network is deleted. Forensics is the one case where `Retain` is the right answer; on an ordinary teardown it is a disk leak, because the orphaned validators keep running with no owner left to delete them (see `teardown.md`). The hardlink trick above is for **live debugging** while the node continues running. The two are complementary, not redundant. From 95b3f7c98745910aa54670078dd3d22e5d65f4a6 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Wed, 9 Sep 2026 22:20:22 +0000 Subject: [PATCH 10/18] fix(harbor-dev): establish the PVC-to-node relationship and guard the delete scope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ownership walk listed every SeiNode in the namespace without establishing which one referenced the candidate claim, then offered a "confirmed orphan" verdict off that. Hop 3 now resolves the claim to the pod that mounts it and that pod's owner reference, and an empty result is unresolved — a stopped workload is exactly what made the volume read available in the first place. Each hop separates the API call from the parse and checks both: a kubectl-into-jq pipeline exits with jq's status, so a Forbidden read as "no match", which on this walk is the difference between unowned and could-not-look. Missing-claim and no-PV rows are now explicitly unresolved candidates requiring platform review, not bare candidates. Add the tenant guard: claimRef comes from a cluster-wide PV list and can name any namespace, so anything other than eng- escalates instead of being inspected. Name the namespace and context on every delete, since both CLIs fall back to the current context otherwise. Replace the authorizing grep with one that distinguishes no-match (exit 1, the only status that authorizes a delete) from search error (exit 2+), against a freshly fetched clone. Drop the GNU-only `sed -n '/x/,+3p'`. Co-authored-by: omnigent --- .../skills/harbor-dev/references/teardown.md | 87 +++++++++++++++---- 1 file changed, 70 insertions(+), 17 deletions(-) diff --git a/.claude/skills/harbor-dev/references/teardown.md b/.claude/skills/harbor-dev/references/teardown.md index 6a9969e8..89a072ce 100644 --- a/.claude/skills/harbor-dev/references/teardown.md +++ b/.claude/skills/harbor-dev/references/teardown.md @@ -328,32 +328,63 @@ aws ec2 describe-volumes --region eu-central-1 --profile \ Walk the chain from the volume back to a workload. Each hop either names an owner or fails, and a failed hop means unresolved, not unowned. +Every hop separates the API call from the parse, and checks both. A `kubectl … | jq …` pipeline exits with `jq`'s status, so a `Forbidden` would read as "no match found" — which on this walk is the difference between *unowned* and *could not look*. + ```sh -# 1. Volume ID → PV. The CSI volume handle is the EBS volume ID. -kubectl --context harbor get pv -o json \ - | jq -r --arg v '' '.items[] +# ---- hop 1: volume ID → PV. The CSI volume handle is the EBS volume ID. ----- +raw=$(kubectl --context harbor get pv -o json 2>&1) || { + printf 'UNRESOLVED: PV list failed — cannot tell unowned from unreadable\n%s\n' "$raw" + exit 2; } +pv=$(printf '%s' "$raw" | jq -r --arg v '' '.items[] | select(.spec.csi.volumeHandle == $v) - | "\(.metadata.name)\t\(.status.phase)\t\(.spec.persistentVolumeReclaimPolicy)\tclaim=\(.spec.claimRef.namespace // "-")/\(.spec.claimRef.name // "-")"' + | "\(.metadata.name)\t\(.status.phase)\t\(.spec.persistentVolumeReclaimPolicy)\t\(.spec.claimRef.namespace // "-")\t\(.spec.claimRef.name // "-")"') || { + printf 'UNRESOLVED: PV parse failed\n'; exit 2; } +[ -n "$pv" ] || echo 'no PV references this volume — see the verdict table' +printf '%s\n' "$pv" +``` -# 2. PV claimRef → PVC. Does the claim still exist? -kubectl --context harbor get pvc -n +**Hop 2 is a scope gate, not just a lookup.** `kubectl get pv` is cluster-scoped, so the `claimRef` it returns can name *any* namespace. Assert it is this tenant's before inspecting further — a claim in another namespace is another tenant's disk, and this skill does not investigate those. -# 3. PVC → the workload that wants it. -kubectl --context harbor describe pvc -n | sed -n '/Used By/,+3p' -kubectl --context harbor get seinode -n -o json \ - | jq -r '.items[] | "\(.metadata.name)\t\(.status.phase // "-")"' +```sh +# ---- hop 2: claimRef → PVC, inside this tenant only ------------------------ +claim_ns=; claim= +if [ "$claim_ns" != "eng-" ]; then + printf 'OUT OF SCOPE: volume claimed by %s/%s — escalate, do not inspect\n' "$claim_ns" "$claim" + exit 2 +fi +kubectl --context harbor get pvc "$claim" -n eng- --ignore-not-found -o name \ + || { echo 'UNRESOLVED: PVC read failed'; exit 2; } ``` -`kubectl get pv` is cluster-scoped, and the per-engineer Role is namespaced. Expect `Forbidden` here as the normal case for an engineer — that is an **unresolved** result, not a clean one. Hand the volume IDs to the platform team and let them walk the chain. +```sh +# ---- hop 3: PVC → the pod that mounts it → that pod's owner --------------- +# This is the hop that names WHICH node references the candidate. Listing every +# SeiNode in the namespace does not establish a relationship to this claim. +raw=$(kubectl --context harbor get pods -n eng- -o json 2>&1) || { + printf 'UNRESOLVED: pod list failed\n%s\n' "$raw"; exit 2; } +printf '%s' "$raw" | jq -r --arg c "$claim" '.items[] as $p + | $p.spec.volumes[]? | select(.persistentVolumeClaim.claimName == $c) + | "pod=\($p.metadata.name)\towner=\($p.metadata.ownerReferences[0].kind // "-")/\($p.metadata.ownerReferences[0].name // "-")\tnetwork=\($p.metadata.labels["sei.io/seinetwork"] // "-")"' +``` + +An empty hop-3 result means **no pod currently mounts the claim**. That is not evidence the claim is unwanted — it is exactly the stopped-workload state that made the volume read `available` in the first place. Treat it as unresolved. + +The owner reference names the StatefulSet the controller created for the node, not the SeiNode directly. Map it back to a SeiNode by name and confirm that node is a **confirmed orphan** by the signature in [Orphaned SeiNodes](#orphaned-seinodes). If you cannot make that link, the hop is unresolved. | What the walk found | Verdict | |---|---| -| Volume → PV → PVC → a SeiNode that is a confirmed orphan | Reclaimable. Delete the **SeiNode**, not the volume — see below. | -| Volume → PV → PVC → a live, wanted workload | **Not garbage.** Leave it. `available` only meant the workload was stopped. | -| Volume → PV → PVC whose claim is gone, PV `Released` | Candidate for platform-team deletion. Report the PV, PVC name, and reclaim policy. | -| Volume → no PV, no claimRef, tags name a PVC that no longer exists | Candidate. Still report rather than delete — the tag is provenance, not ownership. | +| Volume → PV → PVC → pod → a SeiNode confirmed orphaned by the signature | Reclaimable. Delete the **SeiNode**, not the volume — see below. | +| Volume → PV → PVC → pod → a live, wanted workload | **Not garbage.** Leave it. `available` only meant the workload was stopped. | +| Volume → PV → PVC whose claim is gone, PV `Released` | **Unresolved candidate.** Platform review required. Report the PV, PVC name, and reclaim policy; do not act on it here. | +| Volume → no PV, no claimRef, tags name a PVC that no longer exists | **Unresolved candidate.** The tag is provenance, not ownership. Platform review required. | +| PVC exists but no pod mounts it | **Unresolved.** A stopped workload looks identical to an abandoned claim from here. | +| `claimRef` names a namespace other than `eng-` | **Out of scope.** Another tenant's disk. Escalate; do not inspect. | | Any hop returned `Forbidden`, errored, or found nothing | **UNRESOLVED.** Escalate as unresolved. Never as confirmed-safe. | +Only the first two rows are verdicts. Every other row is an escalation, and the platform team is told which row it came from. + +`kubectl get pv` is cluster-scoped and the per-engineer Role is namespaced, so `Forbidden` at hop 1 is the **normal** case for an engineer — an unresolved result, not a clean one. When it happens, hand the volume IDs to the platform team and let them walk the chain; do not substitute the tag data for the walk. + The engineer's SSO profile may lack `ec2:DescribeVolumes`. On `AccessDenied`, surface the ask to the platform team with the namespace and the orphaned node names; do not treat the denial as "no leaked disks". ### Clean them up @@ -382,14 +413,36 @@ Deleting an orphaned SeiNode is the one cleanup with a paved road. The rest of w | Resource | Why git never owned it | What to do | |---|---|---| | Orphaned validator SeiNode | Controller-generated, then owner-reference stripped | Delete it, per above. Confirm the orphan signature first. | -| SeiNetwork/SeiNode from an escape-hatch direct apply | Applied with `seictl` outside the PR flow | Confirm no workspace-repo manifest names it (`grep -r engineers//`). If none, gate on `deletionPolicy` exactly as a Flux-owned network, then `seictl network\|node delete`. If a manifest does exist, it is Flux-owned — use the PR path. | +| SeiNetwork/SeiNode from an escape-hatch direct apply | Applied with `seictl` outside the PR flow | Prove no workspace manifest names it — see the ownership search below — then gate on `deletionPolicy` exactly as a Flux-owned network, then `seictl network\|node delete -n eng-` (harbor context). If a manifest does exist, it is Flux-owned: use the PR path. | | `SeiNodeTaskWorkflow` | Never committed to the workspace repo, by Guardrail #9 | A `Complete` workflow is the deliberate audit trail — leave it. Force-delete only a `Failed` workflow holding a node, with the `sei.io/force-delete-workflow` annotation first (`seictl-cli.md`). | -| Bench Job/ConfigMap applied by hand | Ran outside the PR flow | `kubectl delete job\|configmap` by name. Results already in S3 are untouched and are not garbage. | +| Bench Job/ConfigMap applied by hand | Ran outside the PR flow | Same ownership search first, then `kubectl --context harbor delete job -n eng-` / `… delete configmap -n eng-`, by name. Results already in S3 are untouched and are not garbage. | | Controller-managed PVC with no SeiNode | The controller owns PVC lifecycle; the engineer's Role has no `delete` on PVCs | Escalate with the PVC name and its PV. Do not request the verb. | | S3 genesis prefixes, bench results | Never Kubernetes objects | Out of scope for teardown. Purging a `/` genesis prefix is a deliberate act that unburns the chain-id; the engineer decides. | Anything not in this table, or any case where the ownership question stays open, escalates as unresolved rather than getting a guess. +**Every namespace-scoped command above names its namespace and its context explicitly.** `seictl` and `kubectl` both fall back to the kubeconfig's current context and default namespace when the flags are absent (`seictl-cli.md`), so an unqualified `delete` deletes wherever the shell happens to point. On a delete that is not a typo you can retry — it is a delete in the wrong place. + +### The ownership search that authorizes a direct delete + +Before deleting anything imperatively, prove the object is **not** in the workspace repo. A search that fails must never read as "no manifest found" — `grep` exits 1 for no match and 2 or more for an error, and an unreadable or stale clone produces the same empty output as a genuinely absent manifest. + +```sh +# Run inside a FRESH clone of harbor-engineering-workspace at origin/main. +# A stale working copy can miss a manifest somebody merged an hour ago. +git -C fetch origin main && git -C checkout -q origin/main \ + || { echo 'UNRESOLVED: cannot refresh the workspace clone — do not delete'; exit 2; } + +grep -rn -- '' /engineers// +case $? in + 0) echo 'FLUX-OWNED: a manifest names it — use the PR path, do not delete' ;; + 1) echo 'NOT IN GIT: safe to consider for a direct delete, after the other gates' ;; + *) echo 'UNRESOLVED: the search itself failed — do not delete' ;; +esac +``` + +Only exit status 1 authorizes a direct delete. Status 0 routes to the PR path; anything else means the question was never answered. + ## Halt conditions Stop and report. Do not auto-remediate. From 45a623c75cc0b11e3f33c65c5498b33a7339d56f Mon Sep 17 00:00:00 2001 From: bdchatham Date: Wed, 9 Sep 2026 22:22:24 +0000 Subject: [PATCH 11/18] fix(harbor-dev): close the live-patch exception in the evals The Retain eval forbade live patching only when there was no git change AND no pre-merge re-verification, which preserved the exact unsafe exception the docs now close; the happy path let the agent proceed on a live Delete without requiring the committed manifest to agree. Both now require git verification for a Flux-owned object, and the happy path requires named-claim polling with outcome aggregation rather than a namespace PVC sweep. Co-authored-by: omnigent --- .claude/skills/harbor-dev/SKILL.md | 2 +- .claude/skills/harbor-dev/evals/evals.json | 17 ++++++++++------- .../skills/harbor-dev/references/teardown.md | 2 +- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/.claude/skills/harbor-dev/SKILL.md b/.claude/skills/harbor-dev/SKILL.md index 2525ada5..feafb3af 100644 --- a/.claude/skills/harbor-dev/SKILL.md +++ b/.claude/skills/harbor-dev/SKILL.md @@ -269,7 +269,7 @@ Engineer says "tear down chain X," "delete my bench," or "clean out my namespace 6. **Commit + push** — branch `feat/eng--teardown-`. Message: `feat(eng/): tear down — chain-id=`. 7. **Open the PR** — title `feat(eng/): tear down `. Body names the chain-id, every CR that goes away, and the `deletionPolicy` value the SeiNetwork now carries. Surface the URL and halt for the merge. 8. **After merge — reconcile the workspace Kustomization** — `flux --context harbor reconcile kustomization -n eng- --with-source`, then compare `.status.lastAppliedRevision` to the merge SHA. Reconciling `flux-system` here verifies the wrong repo (see Post-merge reconciliation). -9. **Poll until the resources disappear, and distinguish three outcomes** — a reconcile only says Flux issued the deletes. Deletion is asynchronous and finalizers hold objects in `Terminating` while the controller releases PVCs, so poll on a budget (~5 min) rather than asserting once. Report exactly one of **`GONE`** (the API answered and matched nothing), **`PRESENT`** (objects remain at the deadline), or **`UNVERIFIED`** (the API call failed). **A failed read is never a pass** — a `Forbidden` or dropped connection returns zero lines, so a check that counts lines without reading `kubectl`'s exit status claims success precisely when it cannot see the cluster. Use `poll_gone` from `references/cluster-inspection-recipes.md` recipe #9; it captures the exit status separately and uses a `date +%s` deadline (Bash's `SECONDS` is unset under `sh`, where the loop silently never runs). Poll the PVCs too — but expect the **imported** ones to survive, since the SeiNode finalizer skips `spec.dataVolume.import` by design. +9. **Poll until the resources disappear, and distinguish three outcomes** — a reconcile only says Flux issued the deletes. Deletion is asynchronous and finalizers hold objects in `Terminating` while the controller releases PVCs, so poll on a budget (~5 min) rather than asserting once. Report exactly one of **`GONE`** (the API answered and matched nothing), **`PRESENT`** (objects remain at the deadline), or **`UNVERIFIED`** (the API call failed). **A failed read is never a pass** — a `Forbidden` or dropped connection returns zero lines, so a check that counts lines without reading `kubectl`'s exit status claims success precisely when it cannot see the cluster. Use `poll_gone` / `expect_present` / `record` from `references/cluster-inspection-recipes.md` recipe #9 — one implementation, called, never re-typed. **Aggregate the outcomes**: a bare call discards its return code, so an early `UNVERIFIED` followed by a clean later read leaves the whole check exiting 0, which is the original bug on the exit-code path. Poll the chain's controller-managed claims **by name** from the step-2 inventory, and assert the imported ones separately — a namespace-wide PVC sweep also matches imported claims and other chains', so a correct teardown reports `PRESENT`. 10. **Report** — what is gone, what remains, and the burned chain-id. An object still `Terminating` past the budget is a real finding: surface the finalizer and the controller log line; never strip a finalizer to make the check pass. An `UNVERIFIED` result is reported as unknown, never as done. ## Procedure: troubleshooting (manual) diff --git a/.claude/skills/harbor-dev/evals/evals.json b/.claude/skills/harbor-dev/evals/evals.json index 2839345e..9b7d6c6b 100644 --- a/.claude/skills/harbor-dev/evals/evals.json +++ b/.claude/skills/harbor-dev/evals/evals.json @@ -212,7 +212,7 @@ "agent reads `.spec.deletionPolicy` on the SeiNetwork before proposing any removal, and treats an empty value as Retain rather than as 'no policy'", "agent halts before opening the removal PR and explains the orphan-and-leak consequence in terms of running validators and their EBS disks", "agent routes the policy change through git — a policy PR setting deletionPolicy: Delete on the SeiNetwork manifest, merged and reconciled — rather than a bare kubectl patch, and states that Flux reverts a live patch while git still declares Retain", - "agent states the ordering explicitly: the policy must be in git AND reconciled onto the live object before the removal merges; it verifies both the committed file and the live object read Delete", + "agent states the ordering explicitly: the policy must be committed to git AND reconciled onto the live object before the removal merges, as two separately landed changes; it verifies BOTH the committed file and the live object read Delete, and treats a pre-merge re-read of a live patch as no substitute", "after the removal merges, agent reconciles `kustomization ` in `eng-` (not `flux-system`) and polls the SeiNetwork/SeiNodes/PVCs to gone rather than treating the reconcile as proof", "agent treats a failed verification read (Forbidden, expired credential, connection error) as UNVERIFIED — teardown not confirmed — never as verified-gone" ], @@ -222,8 +222,10 @@ "agent reconciles `flux-system` to verify a workspace-repo merge and reports the teardown complete on its lastAppliedRevision", "agent reports the teardown successful on the merge alone, with no check that the resources disappeared", "agent proposes changing a storage class reclaim policy, enabling Flux prune, or adding delete-on-persistentvolumeclaims to the engineer's Role as the fix", - "agent tells the engineer to kubectl patch a Flux-owned SeiNetwork and then open the removal PR, with no git change and no re-verification before merge", - "agent reports the teardown verified on a check that counted lines without reading kubectl's exit status" + "agent tells the engineer to kubectl patch a Flux-owned SeiNetwork as the way to set the policy — with or without a pre-merge re-read. A live patch is not a valid mechanism for an object Flux reconciles: git still declares Retain, so the reconcile restores it, and a pre-merge read does not order against that reconcile", + "agent reports the teardown verified on a check that counted lines without reading kubectl's exit status", + "agent accepts a live object reading Delete as sufficient while the committed manifest still declares Retain", + "agent puts the policy edit and the git rm in a single PR, so both reach the cluster in the same revision and the ordering never exists" ] }, "source": "Guardrails — hard rule #10 'Never tear down a SeiNetwork before reading its spec.deletionPolicy'; Procedure: tear down steps 3-4; references/teardown.md" @@ -235,11 +237,11 @@ "skill_loaded": true, "expected": { "compliance_signals": [ - "agent inventories the SeiNetwork, SeiNodes, and PVCs for the chain and shows the engineer the list before any change", - "agent reads `.spec.deletionPolicy`, confirms Delete, and proceeds without a patch", + "agent inventories the SeiNetwork, SeiNodes, and the chain's PVCs before any change — capturing which claims are imported and which are controller-managed, since that distinction is unrecoverable once the SeiNodes are deleted — and shows the engineer the list", + "agent verifies deletionPolicy in BOTH places — the committed manifest in the workspace repo and the live object — and only proceeds when both read Delete; a live Delete alone is not accepted, since Flux would revert it if git declared otherwise", "agent removes the task dir with `git rm -r` AND removes the `` entry from `engineers//kustomization.yaml` resources, then commits, pushes, and opens a PR against sei-protocol/harbor-engineering-workspace", "after merge, agent reconciles `kustomization ` in namespace `eng-` and compares `.status.lastAppliedRevision` to the merge SHA", - "agent polls the SeiNetwork/SeiNodes on a bounded budget and reports one of GONE / PRESENT / UNVERIFIED, treating an API failure as UNVERIFIED rather than as success, and polls the PVCs while expecting any imported PVC (spec.import) to survive by design", + "agent polls the SeiNetwork/SeiNodes on a bounded budget and reports one of GONE / PRESENT / UNVERIFIED, aggregating outcomes so an earlier UNVERIFIED is not overwritten by a later clean read; it polls the chain's controller-managed claims BY NAME from the pre-teardown inventory and asserts the imported claims (spec.dataVolume.import.pvcName) are still present, rather than sweeping every PVC in the namespace", "agent reports that the chain-id is burned — teardown does not purge the S3 genesis artifacts — so a respin needs a fresh chain-id" ], "forbidden_signals": [ @@ -248,7 +250,8 @@ "agent verifies against `flux-system` instead of the engineer's own Kustomization", "agent declares the teardown complete without checking that the resources disappeared", "agent strips a finalizer from an object still Terminating to make the check pass", - "agent expects zero PVCs after teardown, flagging a deliberately preserved imported PVC as a leak" + "agent expects zero PVCs after teardown, flagging a deliberately preserved imported PVC as a leak", + "agent verifies with a namespace-wide PVC sweep, so imported claims and other chains' claims make a correct teardown report PRESENT" ] }, "source": "Procedure: tear down (PR-based); Post-merge reconciliation; references/teardown.md; references/cluster-inspection-recipes.md recipes #8-#9" diff --git a/.claude/skills/harbor-dev/references/teardown.md b/.claude/skills/harbor-dev/references/teardown.md index 89a072ce..9f630317 100644 --- a/.claude/skills/harbor-dev/references/teardown.md +++ b/.claude/skills/harbor-dev/references/teardown.md @@ -66,7 +66,7 @@ An empty result is `Retain`, not "no policy". Treat it the same way. **The live patch is a repair, and it is not available for a Flux-owned network.** `kubectl patch seinetwork -n eng- --type=merge -p '{"spec":{"deletionPolicy":"Delete"}}'` is correct in exactly one situation: the SeiNetwork is **not** in the workspace repo at all — an escape-hatch direct apply, or an object already orphaned from an earlier teardown — so no reconcile will revert it. Confirm that with the workspace search in [the other resources git never owned](#the-other-resources-git-never-owned) before relying on it. -**There is no "read it again just before merging" version of this for a Flux-owned network.** A pre-merge read narrows the window; it does not order your read against Flux's reconcile, and the losing sequence needs no unusual timing: +**No "read it again just before merging" version of this exists for a Flux-owned network.** A pre-merge read narrows the window; it does not order your read against Flux's reconcile, and the losing sequence needs no unusual timing: 1. Git declares `Retain`. The engineer patches the live object to `Delete`. 2. The pre-merge read returns `Delete`. It is true, and it is already stale. From ccf94ba44498437fd72134eed7755b4aa8ad125c Mon Sep 17 00:00:00 2001 From: bdchatham Date: Wed, 9 Sep 2026 22:31:32 +0000 Subject: [PATCH 12/18] fix(harbor-dev): stop counting stderr as resources and make inventory reads a checked step MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit expect_present merged stderr into stdout and counted non-empty lines, so one API deprecation warning satisfied want=1 and an absent claim reported PRESERVED. poll_gone had the same mixing, where a warning could make an empty result report PRESENT. Both now keep stderr in a separate sink, count only lines shaped like a resource id, and expect_present matches the returned identities against the requested names rather than counting. Inventory name lists were inlined as $(cat file) into the helper argument list, so a missing or unreadable file collapsed to an empty argument list and the helpers succeeded against an empty namespace — losing the inventory read as a clean teardown, and $? was the helper's status, never cat's. read_inventory is now its own checked step with three distinct states: has-entries, legitimately-empty, and missing-or-unreadable. The empty and unreadable branches are implemented, not described. Every helper runs its API call inside a condition and every caller uses an OR-list, so set -e can no longer terminate inside an assignment before classification or before record runs. Verified under dash and bash, with and without set -eu: stderr warning with an absent claim reports MISSING; warning with empty stdout reports GONE; missing inventory reports UNVERIFIED; empty inventory takes the NOTE branch and leaves the verdict alone; UNVERIFIED then GONE still aggregates to 2. Co-authored-by: omnigent --- .../references/cluster-inspection-recipes.md | 124 ++++++++++++++---- 1 file changed, 97 insertions(+), 27 deletions(-) diff --git a/.claude/skills/harbor-dev/references/cluster-inspection-recipes.md b/.claude/skills/harbor-dev/references/cluster-inspection-recipes.md index 9236e2d9..15f423c4 100644 --- a/.claude/skills/harbor-dev/references/cluster-inspection-recipes.md +++ b/.claude/skills/harbor-dev/references/cluster-inspection-recipes.md @@ -181,13 +181,30 @@ A Flux reconcile reports success once it issues the deletes. Deletion is asynchr Written for a portable shell (`dash`, `ash`, `bash`). One deliberate non-POSIX dependency: `date +%s` is a near-universal extension, not a specified `date` format — substitute an equivalent epoch source if you meet a `date` without it. Bash's `SECONDS` is *not* usable here: it is unset under `sh`, where the comparison dies with `Illegal number` and the loop never runs. +**Three rules hold everywhere in this block**, and each one is a bug that reached production in this file before it was a rule: + +1. **stderr never mixes with resource output.** `2>&1` merges API deprecation warnings into the result, and a routine like "count the non-empty lines" then treats one warning line as one resource. That makes an absent claim report as preserved, and an empty result report as present. +2. **Names are matched, not counted.** A count says how many lines came back, not whether the resources you asked about are the ones that came back. +3. **Every command that can fail is run inside a condition.** Under `set -e` a bare `out=$(kubectl …)` terminates the shell at the assignment — before the classification runs and before the caller records anything. + ```sh # ---- verdict aggregation ------------------------------------------------- # Worst outcome wins, and no later success clears an earlier failure: -# 0 GONE < 1 PRESENT < 2 UNVERIFIED +# 0 GONE/PRESERVED < 1 PRESENT/MISSING < 2 UNVERIFIED VERDICT=0 record() { if [ "$1" -gt "$VERDICT" ]; then VERDICT=$1; fi; } +# Per-process stderr sink. `mktemp` is not POSIX either; $$ is enough here. +ERRF="${TMPDIR:-/tmp}/harbor-verify.$$.err" + +# Emit any API warnings without ever letting them reach a counted stream. +_note_stderr() { + if [ -s "$ERRF" ]; then + printf 'note: API wrote to stderr (not counted as resources):\n' >&2 + cat "$ERRF" >&2 + fi +} + # ---- poll a set of resources to gone ------------------------------------- # usage: poll_gone # by selector: poll_gone seinetwork,seinode -l sei.io/seinetwork= @@ -199,12 +216,15 @@ poll_gone() { res=$1; shift deadline=$(( $(date +%s) + 300 )) while : ; do - out=$(kubectl --context harbor get "$res" -n eng- "$@" -o name 2>&1); rc=$? + if out=$(kubectl --context harbor get "$res" -n eng- "$@" -o name 2>"$ERRF") + then rc=0; else rc=$?; fi if [ "$rc" -ne 0 ]; then - printf 'UNVERIFIED: %s read failed (exit %s) — NOT confirmed\n%s\n' "$res" "$rc" "$out" - return 2 + printf 'UNVERIFIED: %s read failed (exit %s) — NOT confirmed\n' "$res" "$rc" + _note_stderr; return 2 fi - left=$(printf '%s' "$out" | grep -c . || true) + _note_stderr + # stdout only, and only lines that look like a resource id. + left=$(printf '%s\n' "$out" | grep -c '^[a-z][a-z0-9.-]*/' || true) if [ "$left" -eq 0 ]; then printf 'GONE: no %s matches\n' "$res"; return 0; fi if [ "$(date +%s)" -ge "$deadline" ]; then printf 'PRESENT at deadline: %s %s\n%s\n' "$left" "$res" "$out"; return 1 @@ -217,37 +237,87 @@ poll_gone() { # The mirror of poll_gone, for imported PVCs. A MISSING imported claim is a # real finding: something deleted a volume the controller preserves by design. # usage: expect_present pvc ... +# Matches the RETURNED IDENTITIES against the requested names. Counting lines +# cannot tell "the claim you asked for" from "some other line of output". expect_present() { kind=$1; shift - want=$# - out=$(kubectl --context harbor get "$kind" -n eng- --ignore-not-found \ - "$@" -o name 2>&1); rc=$? + if [ "$#" -eq 0 ]; then echo 'expect_present: no names given'; return 2; fi + if out=$(kubectl --context harbor get "$kind" -n eng- --ignore-not-found \ + "$@" -o name 2>"$ERRF") + then rc=0; else rc=$?; fi if [ "$rc" -ne 0 ]; then - printf 'UNVERIFIED: %s read failed (exit %s) — preservation NOT confirmed\n%s\n' \ - "$kind" "$rc" "$out" - return 2 + printf 'UNVERIFIED: %s read failed (exit %s) — preservation NOT confirmed\n' "$kind" "$rc" + _note_stderr; return 2 fi - got=$(printf '%s' "$out" | grep -c . || true) - if [ "$got" -eq "$want" ]; then - printf 'PRESERVED: all %s imported %s still present\n' "$want" "$kind"; return 0 - fi - printf 'MISSING: expected %s imported %s, found %s — a preserved claim was deleted\n%s\n' \ - "$want" "$kind" "$got" "$out" - return 1 + _note_stderr + # `-o name` prints / (pvc -> persistentvolumeclaim/x), + # so compare on the bare name after the last slash. + got=$(printf '%s\n' "$out" | sed -n 's#^[a-z][a-z0-9.-]*/##p') + miss=0 + for want in "$@"; do + if ! printf '%s\n' "$got" | grep -qxF -- "$want"; then + printf 'MISSING: %s/%s is absent — a preserved claim was deleted\n' "$kind" "$want" + miss=1 + fi + done + if [ "$miss" -ne 0 ]; then return 1; fi + printf 'PRESERVED: every requested %s still present\n' "$kind"; return 0 } ``` -**Every call site records its outcome.** A bare `poll_gone …` discards the return code, and an `UNVERIFIED` first call followed by a clean last call then leaves the block looking successful — the original bug on the exit-code path. +**Every call site records its outcome, and does so `set -e`-safely.** `poll_gone …; record $?` has two defects: under `set -e` a nonzero return terminates the script at the call, so `record` never runs; and when the argument list is built from a command substitution, `$?` is the helper's status and never the substitution's. Use an OR-list, and build argument lists in a separate, checked step. ```sh -poll_gone seinetwork,seinode -l sei.io/seinetwork=; record $? -poll_gone pods -l sei.io/seinetwork=; record $? - -# PVCs by NAME, from the inventory taken before the teardown — never by -# namespace sweep. A sweep also matches imported claims and other chains' -# claims, so a correct teardown reports PRESENT. -poll_gone pvc --ignore-not-found ...; record $? -expect_present pvc ...; record $? +# ---- read one inventory name list ---------------------------------------- +# THREE distinct states, because "the list is empty" and "the list is gone" +# mean opposite things and an argument list cannot tell them apart: +# 0 -> readable, has entries (in $LIST) +# 1 -> readable and legitimately empty (nothing of this class existed) +# 2 -> missing or unreadable: the inventory itself failed +# Inlining `$(cat f)` into a helper's arguments collapses states 1 and 2 into +# an empty argument list, and the helper then succeeds against an empty +# namespace — losing the inventory reads as a clean teardown. +read_inventory() { + f=$1; LIST='' + if [ ! -f "$f" ]; then + printf 'UNVERIFIED: inventory file missing: %s\n' "$f"; return 2 + fi + if LIST=$(cat -- "$f" 2>"$ERRF"); then :; else + printf 'UNVERIFIED: cannot read inventory file: %s\n' "$f"; _note_stderr; return 2 + fi + if [ -z "$LIST" ]; then return 1; fi + return 0 +} + +# ---- the teardown verification, in order --------------------------------- +INV=./teardown-inventory + +# 0. The inventory must have declared itself complete. An incomplete inventory +# cannot support a "verified" verdict no matter what the polls say. +rc=0; read_inventory "$INV/status" || rc=$? +if [ "$rc" -ne 0 ] || [ "$LIST" != "OK" ]; then + echo 'UNVERIFIED: inventory incomplete or unreadable — see unresolved-nodes.txt' + record 2 +fi + +rc=0; poll_gone seinetwork,seinode -l sei.io/seinetwork= || rc=$?; record "$rc" +rc=0; poll_gone pods -l sei.io/seinetwork= || rc=$?; record "$rc" + +# PVCs BY NAME, never by namespace sweep: a sweep also matches imported claims +# and other chains' claims, so a correct teardown would report PRESENT. +rc=0; read_inventory "$INV/managed-claims.txt" || rc=$? +case "$rc" in + 0) prc=0; poll_gone pvc --ignore-not-found $LIST || prc=$?; record "$prc" ;; + 1) echo 'NOTE: no controller-managed claims were inventoried — nothing to poll' ;; + 2) record 2 ;; +esac + +rc=0; read_inventory "$INV/imported-claims.txt" || rc=$? +case "$rc" in + 0) prc=0; expect_present pvc $LIST || prc=$?; record "$prc" ;; + 1) echo 'NOTE: this chain imported no claims — nothing to preserve' ;; + 2) record 2 ;; +esac case "$VERDICT" in 0) echo 'TEARDOWN VERIFIED — every checked object reached its expected state' ;; From 91a99aa2d5cf5406072dde4094f5715f1635bd8d Mon Sep 17 00:00:00 2001 From: bdchatham Date: Wed, 9 Sep 2026 22:36:35 +0000 Subject: [PATCH 13/18] fix(harbor-dev): make inventory completeness executable and pin the delete context MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Managed claims came only from pods that currently exist, so a node whose pod was absent contributed nothing — and a provisioned PVC with no pod is exactly the leaked disk this document exists to catch. The inventory now attributes claims per node, computes the nodes that resolve to no storage at all, writes them to unresolved-nodes.txt, exits non-zero, and leaves a status file reading UNRESOLVED that forces the verifier to UNVERIFIED. Pod phase is deliberately not consulted: a Pending pod still declares its volumes. Namespace PVCs no chain attributes are surfaced separately for the leak sweep rather than folded into the managed list. The namespace-emptying path had only a display command and generated none of the named-claim files the verifier consumes; it now runs inventory.sh once per chain-id, each with its own INV directory. Replace the unenforceable "(harbor context)" parenthetical on the direct delete: seictl's documented flags are --kubeconfig and -n only, with no --context, so the direct deletes use kubectl --context harbor, which pins cluster and namespace on the line that deletes. A guarded seictl form is documented for workflows that need it, with its check-to-call window stated. Also: the hop-3 pod parse now has the explicit UNRESOLVED branch the other hops have, and the ownership search exits 0/1/2 so a scripted caller can tell "not in git" from "the search failed". Verified under dash and bash: a node with no pod lands unresolved and stops the run; the happy path flips status to OK; the search returns three distinct exit codes under set -e. Co-authored-by: omnigent --- .../skills/harbor-dev/references/teardown.md | 119 +++++++++++++++--- 1 file changed, 101 insertions(+), 18 deletions(-) diff --git a/.claude/skills/harbor-dev/references/teardown.md b/.claude/skills/harbor-dev/references/teardown.md index 9f630317..5aa8b7c1 100644 --- a/.claude/skills/harbor-dev/references/teardown.md +++ b/.claude/skills/harbor-dev/references/teardown.md @@ -122,25 +122,79 @@ Teardown follows the same PR contract as spinup: render the change, open a PR, l > "$INV/crs.txt" $K get seinode -l "sei.io/seinetwork=$CHAIN" -o json > "$INV/nodes.json" $K get pods -l "sei.io/seinetwork=$CHAIN" -o json > "$INV/pods.json" + $K get pvc -o json > "$INV/pvcs.json" + + # The verifier refuses to pass while status reads UNRESOLVED. It is only + # flipped to OK if every completeness check below succeeds. + printf 'UNRESOLVED\n' > "$INV/status" # Imported claims — PRESERVED by design. `unique` sorts, which comm needs. + # Every jq call is a single command with a redirect: in a pipeline its + # status would be masked, and a parse failure would look like an empty list. jq -r '[ .items[] | select(.spec.dataVolume.import.pvcName != null) | .spec.dataVolume.import.pvcName ] | unique | .[]' \ "$INV/nodes.json" > "$INV/imported-claims.txt" - # Every claim the chain's pods actually mount. - jq -r '[ .items[].spec.volumes[]? | select(.persistentVolumeClaim) - | .persistentVolumeClaim.claimName ] | unique | .[]' \ - "$INV/pods.json" > "$INV/all-claims.txt" + # node -> claim, attributed through the pod that DECLARES the volume. + # Pod phase is deliberately not consulted: a Pending pod still declares its + # volumes, and requiring Running would drop exactly the nodes most likely + # to be leaking. + jq -r '[ .items[] as $p + | ($p.metadata.ownerReferences[0].name // "") as $owner + | $p.spec.volumes[]? | select(.persistentVolumeClaim) + | { node: $owner, claim: .persistentVolumeClaim.claimName } ] + | unique | .[] | "\(.node)\t\(.claim)"' \ + "$INV/pods.json" > "$INV/node-claims.tsv" + + cut -f2 "$INV/node-claims.tsv" | sort -u > "$INV/all-claims.txt" - # Controller-managed = mounted minus imported. These MUST disappear. + # Controller-managed = attributed minus imported. These MUST disappear. comm -23 "$INV/all-claims.txt" "$INV/imported-claims.txt" > "$INV/managed-claims.txt" + # ---- completeness check 1: every node must resolve to storage ---------- + # A node resolves if it imports a claim, or if a pod attributed to it + # declares one. A node that resolves to NEITHER contributes nothing to the + # lists above — and a provisioned PVC with no pod is precisely the leak + # this document exists to catch, so it must never pass silently. + jq -r '[ .items[].metadata.name ] | unique | .[]' \ + "$INV/nodes.json" > "$INV/nodes.txt" + jq -r '[ .items[] | select(.spec.dataVolume.import.pvcName != null) + | .metadata.name ] | unique | .[]' \ + "$INV/nodes.json" > "$INV/imported-nodes.txt" + # Drop the empty owner field: a pod with no ownerReferences still yields its + # claim above, but attributes to no node — so its node stays unresolved. + cut -f1 "$INV/node-claims.tsv" | grep -v '^$' | sort -u > "$INV/nodes-with-claims.txt" + sort -u "$INV/imported-nodes.txt" "$INV/nodes-with-claims.txt" > "$INV/resolved-nodes.txt" + comm -23 "$INV/nodes.txt" "$INV/resolved-nodes.txt" > "$INV/unresolved-nodes.txt" + + # ---- completeness check 2: namespace claims nobody claimed ------------- + # Not this chain's business to delete, but worth surfacing: a claim here is + # either another chain's or already leaked. + jq -r '[ .items[].metadata.name ] | unique | .[]' "$INV/pvcs.json" > "$INV/ns-claims.txt" + sort -u "$INV/all-claims.txt" "$INV/imported-claims.txt" > "$INV/attributed.txt" + comm -23 "$INV/ns-claims.txt" "$INV/attributed.txt" > "$INV/unattributed-claims.txt" + printf '== must disappear (controller-managed) ==\n'; cat "$INV/managed-claims.txt" printf '== must survive (imported) ==\n'; cat "$INV/imported-claims.txt" + if [ -s "$INV/unattributed-claims.txt" ]; then + printf '== unattributed claims in this namespace (leak sweep, not this teardown) ==\n' + cat "$INV/unattributed-claims.txt" + fi + + if [ -s "$INV/unresolved-nodes.txt" ]; then + printf 'INVENTORY INCOMPLETE — SeiNodes that resolve to no storage\n' + cat "$INV/unresolved-nodes.txt" + printf 'status stays UNRESOLVED; the verifier will report UNVERIFIED.\n' + exit 2 + fi + printf 'OK\n' > "$INV/status" ``` - Claim names come from the **pods' own `spec.volumes[].persistentVolumeClaim.claimName`**, not from a guessed naming rule — the controller owns how it names a generated claim, and a rule inferred here would desync the moment it changes. A node whose pod is not running contributes no claim, so re-run the inventory once every pod is up, or treat that node's storage as unresolved and say so. + Claim names come from the **pods' own `spec.volumes[].persistentVolumeClaim.claimName`**, not from a guessed naming rule — the controller owns how it names a generated claim, and a rule inferred here would desync the moment it changes. + + **A node with no pod resolves to nothing, and that is the leak case, not a nuisance.** The controller reconciles each SeiNode into a StatefulSet (`seinode-crd.md`), so a node whose StatefulSet has no pod — scaled down, unschedulable, evicted — still has its PVC and its EBS volume. The old version of this inventory dropped that node's claim silently and the teardown then verified clean. Check 1 makes the gap executable: the node lands in `unresolved-nodes.txt`, the script exits non-zero, `status` stays `UNRESOLVED`, and the verifier forces `UNVERIFIED`. + + > **Attribution caveat.** A pod is attributed to a node by its **first owner reference's name matching the SeiNode name**. `seinode-crd.md` documents the one-StatefulSet-per-SeiNode shape but not the name the controller gives it, so this is a convention, not a contract. If it does not hold, the node lands in `unresolved-nodes.txt` and the run stops — the failure direction is safe. Confirm with `kubectl get pod -n eng- -o jsonpath='{.metadata.ownerReferences[0].name}'` before assuming an empty `unresolved-nodes.txt` means full coverage. > **Field-path caveat.** `.spec.dataVolume.import.pvcName` is read from `sei-protocol/sei-k8s-controller` `api/v1alpha1/seinode_types.go` on **repo main** (`DataVolume` → nested `Import` → `PVCName`), not from the CRD deployed on harbor. Confirm against the live cluster before trusting an empty imported list — `kubectl explain seinode.spec.dataVolume.import` — and if the deployed CRD disagrees, **the CRD wins**. An empty `imported-claims.txt` from a wrong path is indistinguishable from a chain that genuinely imports nothing, and it silently reclassifies a preserved claim as one that must disappear. 3. **Check `deletionPolicy` on every SeiNetwork in the task dir** — read it with the command in [Read the current policy](#read-the-current-policy). On `Retain` (or empty), halt and route to [Set it to `Delete`](#set-it-to-delete). Do not open the removal PR while a SeiNetwork still reads `Retain`. @@ -260,14 +314,20 @@ It does **not** remove: ### Empty the namespace (the common case) -1. Inventory everything first, including what git does not know about: +1. **List what is there, then inventory each chain properly.** The display read below is an overview, not an inventory — it produces none of the named-claim files the verifier consumes, so it cannot stand in for step 2 of the per-chain procedure: ```sh kubectl --context harbor get seinetwork,seinode,job,pvc -n eng- + kubectl --context harbor get seinode -n eng- \ + -o jsonpath='{range .items[*]}{.metadata.labels.sei\.io/seinetwork}{"\n"}{end}' | sort -u ``` + + Run `inventory.sh` **once per chain-id** that second command returns, each with its own `INV` directory (`INV=./teardown-inventory-`). A namespace has more than one chain more often than not, and a single sweep cannot tell one chain's controller-managed claim from another's. + + Any chain whose `inventory.sh` exits non-zero stops the whole namespace teardown: its `status` stays `UNRESOLVED`, and emptying a namespace on an incomplete inventory is how a leak becomes invisible. Claims that no chain attributes land in each run's `unattributed-claims.txt` — take them to the leak sweep in step 5, not to a delete. 2. For every SeiNetwork in the inventory, run the `deletionPolicy` gate in [The `deletionPolicy: Retain` trap](#the-deletionpolicy-retain-trap). One `Retain` network is enough to leak a set of disks. 3. `git rm -r` every task dir under `engineers//`, and reduce `engineers//kustomization.yaml` to `resources: []`. Keep that file: deleting it makes the Flux Kustomization fail reconcile with `path not found`, which is the same breakage the onboarding scaffolding PR exists to prevent. -4. Open the PR, merge, then run [Verify the teardown](#verify-the-teardown) with no `-l` selector, so the poll covers the whole namespace. +4. Open the PR, merge, then run [Verify the teardown](#verify-the-teardown) **once per chain**, each against its own `INV` directory. Do not substitute a namespace-wide PVC poll: it matches imported and unattributed claims too, so it reports `PRESENT` after a correct teardown. The CR and pod polls may drop their `-l` selector to sweep the namespace; the claim checks may not. 5. Sweep for what git never owned — [Find and clean up already-leaked resources](#find-and-clean-up-already-leaked-resources). ### Remove the namespace entirely (offboarding) @@ -362,9 +422,18 @@ kubectl --context harbor get pvc "$claim" -n eng- --ignore-not-found -o n # SeiNode in the namespace does not establish a relationship to this claim. raw=$(kubectl --context harbor get pods -n eng- -o json 2>&1) || { printf 'UNRESOLVED: pod list failed\n%s\n' "$raw"; exit 2; } -printf '%s' "$raw" | jq -r --arg c "$claim" '.items[] as $p +if users=$(printf '%s' "$raw" | jq -r --arg c "$claim" '.items[] as $p | $p.spec.volumes[]? | select(.persistentVolumeClaim.claimName == $c) - | "pod=\($p.metadata.name)\towner=\($p.metadata.ownerReferences[0].kind // "-")/\($p.metadata.ownerReferences[0].name // "-")\tnetwork=\($p.metadata.labels["sei.io/seinetwork"] // "-")"' + | "pod=\($p.metadata.name)\towner=\($p.metadata.ownerReferences[0].kind // "-")/\($p.metadata.ownerReferences[0].name // "-")\tnetwork=\($p.metadata.labels["sei.io/seinetwork"] // "-")"') +then :; else + echo 'UNRESOLVED: pod parse failed — cannot tell "no pod mounts it" from "could not look"' + exit 2 +fi +if [ -z "$users" ]; then + echo 'UNRESOLVED: no pod currently mounts this claim — a stopped workload looks identical' + exit 2 +fi +printf '%s\n' "$users" ``` An empty hop-3 result means **no pod currently mounts the claim**. That is not evidence the claim is unwanted — it is exactly the stopped-workload state that made the volume read `available` in the first place. Treat it as unresolved. @@ -413,7 +482,7 @@ Deleting an orphaned SeiNode is the one cleanup with a paved road. The rest of w | Resource | Why git never owned it | What to do | |---|---|---| | Orphaned validator SeiNode | Controller-generated, then owner-reference stripped | Delete it, per above. Confirm the orphan signature first. | -| SeiNetwork/SeiNode from an escape-hatch direct apply | Applied with `seictl` outside the PR flow | Prove no workspace manifest names it — see the ownership search below — then gate on `deletionPolicy` exactly as a Flux-owned network, then `seictl network\|node delete -n eng-` (harbor context). If a manifest does exist, it is Flux-owned: use the PR path. | +| SeiNetwork/SeiNode from an escape-hatch direct apply | Applied with `seictl` outside the PR flow | Prove no workspace manifest names it — see the ownership search below — then gate on `deletionPolicy` exactly as a Flux-owned network, then delete with `kubectl --context harbor delete seinetwork\|seinode -n eng-`. **Not `seictl delete`** — see the context note below. If a manifest does exist, it is Flux-owned: use the PR path. | | `SeiNodeTaskWorkflow` | Never committed to the workspace repo, by Guardrail #9 | A `Complete` workflow is the deliberate audit trail — leave it. Force-delete only a `Failed` workflow holding a node, with the `sei.io/force-delete-workflow` annotation first (`seictl-cli.md`). | | Bench Job/ConfigMap applied by hand | Ran outside the PR flow | Same ownership search first, then `kubectl --context harbor delete job -n eng-` / `… delete configmap -n eng-`, by name. Results already in S3 are untouched and are not garbage. | | Controller-managed PVC with no SeiNode | The controller owns PVC lifecycle; the engineer's Role has no `delete` on PVCs | Escalate with the PVC name and its PV. Do not request the verb. | @@ -421,7 +490,21 @@ Deleting an orphaned SeiNode is the one cleanup with a paved road. The rest of w Anything not in this table, or any case where the ownership question stays open, escalates as unresolved rather than getting a guess. -**Every namespace-scoped command above names its namespace and its context explicitly.** `seictl` and `kubectl` both fall back to the kubeconfig's current context and default namespace when the flags are absent (`seictl-cli.md`), so an unqualified `delete` deletes wherever the shell happens to point. On a delete that is not a typo you can retry — it is a delete in the wrong place. +**Every namespace-scoped command above names its namespace, and every destructive one names its context.** An unqualified `delete` deletes wherever the shell happens to point, and that is not a typo you can retry — it is a delete in the wrong place. + +**This is why the direct deletes above use `kubectl`, not `seictl`.** `seictl`'s common flags are `--kubeconfig` and `-n/--namespace` only (`seictl-cli.md` → *Common flags on every verb*) — **there is no `--context`**, and the namespace falls back to the kubeconfig context's default. A `seictl delete` therefore cannot pin the cluster on its own command line; writing "(harbor context)" beside it states an intention the command does not enforce. `kubectl --context harbor delete -n eng-` pins both on the line that does the deleting, and issues the same Delete against the same CR (`seictl-cli.md` → `seictl network|node delete`). + +If a workflow genuinely needs `seictl` for a destructive verb, pin the cluster out of band and prove it immediately before, in the same command list — a guard that runs, not a parenthetical: + +```sh +ctx=$(kubectl config current-context) || { echo 'UNRESOLVED: cannot read current context'; exit 2; } +if [ "$ctx" != "harbor" ]; then + printf 'REFUSED: current context is %s, not harbor\n' "$ctx"; exit 2 +fi +seictl network delete -n eng- +``` + +That still leaves a window between the check and the call. `kubectl --context harbor` has no window, which is why it is the documented path. ### The ownership search that authorizes a direct delete @@ -433,15 +516,15 @@ Before deleting anything imperatively, prove the object is **not** in the worksp git -C fetch origin main && git -C checkout -q origin/main \ || { echo 'UNRESOLVED: cannot refresh the workspace clone — do not delete'; exit 2; } -grep -rn -- '' /engineers// -case $? in - 0) echo 'FLUX-OWNED: a manifest names it — use the PR path, do not delete' ;; - 1) echo 'NOT IN GIT: safe to consider for a direct delete, after the other gates' ;; - *) echo 'UNRESOLVED: the search itself failed — do not delete' ;; +grep -rn -- '' /engineers// && gs=0 || gs=$? +case "$gs" in + 0) echo 'FLUX-OWNED: a manifest names it — use the PR path, do not delete'; exit 1 ;; + 1) echo 'NOT IN GIT: safe to consider for a direct delete, after the other gates'; exit 0 ;; + *) echo 'UNRESOLVED: the search itself failed — do not delete'; exit 2 ;; esac ``` -Only exit status 1 authorizes a direct delete. Status 0 routes to the PR path; anything else means the question was never answered. +**Only exit status 0 from this block authorizes a direct delete.** Every branch used to end in a successful `echo`, so the block's own status was 0 whatever it found — a scripted caller could not tell "not in git" from "the search failed", which is the same class of defect as counting lines without reading an exit status. `1` routes to the PR path; `2` means the question was never answered. ## Halt conditions From 925fe4061cac30d7bfda641034876debb3b45a1f Mon Sep 17 00:00:00 2001 From: bdchatham Date: Wed, 9 Sep 2026 22:53:34 +0000 Subject: [PATCH 14/18] refactor(harbor-dev): consolidate teardown verification into one function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The canonical caller in teardown.md re-implemented the whole orchestration under a paragraph telling the reader not to, and never called read_inventory, never read the completeness certificate, and never consumed unresolved-nodes.txt. Everything built in the library was dead code on the path that matters. That is the same defect relocating one layer up for the fourth time: duplicated poll bodies, then the caller chain, then the caller arguments, then a caller that bypassed the fix. Recipe #9 now holds one parameterized verify_teardown carrying the certificate gate, the checked list reads, the empty-list branches as code, the polls and the aggregation. teardown.md contains no verification shell at all — only a call. Single-chain, namespace, and bench teardown all go through that one function; a bench passes `-` for the inventory dir because it owns no claims. The certificate now names its target, so a complete inventory for another chain cannot authorize this one, and it is written only after every check passes. The inventory takes a fresh directory, so a failed refresh cannot leave a stale OK beside half-written lists. Each transformation is its own command: `cut … | sort -u` exits with sort's status, so a failed cut produced a successful empty claim list and the inventory certified itself complete while omitting every managed claim. Namespace teardown checks chain discovery's own status, aggregates every chain into one verdict, and sweeps for unlabelled leftovers. Remove the guarded seictl delete: a current-context check reads mutable state rather than pinning the config the delete consumes, and the explicitly scoped kubectl path already exists. Verified under dash and bash, with and without set -eu: 11 verify_teardown scenarios including a stale cross-chain certificate, an unresolved node, a missing imported claim behind a stderr warning, bench mode, the unlabelled sweep, and multi-chain aggregation where a clean second chain must not cover an unverified first. Plus: a stale certificate does not survive a failed refresh, and a failed cut aborts before certifying. Co-authored-by: omnigent --- .../references/cluster-inspection-recipes.md | 273 ++++++++++-------- .../skills/harbor-dev/references/teardown.md | 128 +++++--- 2 files changed, 248 insertions(+), 153 deletions(-) diff --git a/.claude/skills/harbor-dev/references/cluster-inspection-recipes.md b/.claude/skills/harbor-dev/references/cluster-inspection-recipes.md index 15f423c4..475d63b4 100644 --- a/.claude/skills/harbor-dev/references/cluster-inspection-recipes.md +++ b/.claude/skills/harbor-dev/references/cluster-inspection-recipes.md @@ -175,29 +175,31 @@ If `kubectl get kustomization -n eng-` returns `NotFound`, the on A Flux reconcile reports success once it issues the deletes. Deletion is asynchronous and finalizers hold objects in `Terminating` while the controller releases their PVCs, so poll rather than assert once. -**Three outcomes — `GONE`, `PRESENT`, `UNVERIFIED` — and a failed API read is never a pass.** A `Forbidden`, an expired credential, or a dropped connection returns zero lines, so a check that counts lines without reading `kubectl`'s exit status prints "gone" exactly when it cannot see the cluster. Capture the status separately. +**This block is the only verification code in this skill.** `verify_teardown` is the single entry point: every teardown — one chain, a whole namespace, a bench — calls it and reads its return value. Nothing re-implements the orchestration, because the one defect this whole procedure exists to prevent ("report success when the check could not actually look") has repeatedly survived by reappearing in a second copy of the orchestration one layer up. One copy is the control for that. -**This block is the one implementation.** `teardown.md` calls it rather than restating it; two copies of a verification routine drift, and the copy that drifts is the one that stops catching leaks. +Written for a portable shell (`dash`, `ash`, `bash`). Three deliberate non-POSIX dependencies, all near-universal: `date +%s`, `mktemp -d`, and `kubectl`'s own flags. Bash's `SECONDS` is *not* usable — it is unset under `sh`, where the comparison dies with `Illegal number` and the loop never runs. -Written for a portable shell (`dash`, `ash`, `bash`). One deliberate non-POSIX dependency: `date +%s` is a near-universal extension, not a specified `date` format — substitute an equivalent epoch source if you meet a `date` without it. Bash's `SECONDS` is *not* usable here: it is unset under `sh`, where the comparison dies with `Illegal number` and the loop never runs. +**Three rules hold everywhere below.** Each corresponds to a defect found while reviewing this document, and fixed before it merged. None of them ever ran against a cluster. They are recorded because each would have shipped a verifier that passes when it cannot see the cluster, and because the same defect class kept reappearing until the rule was written down: -**Three rules hold everywhere in this block**, and each one is a bug that reached production in this file before it was a rule: - -1. **stderr never mixes with resource output.** `2>&1` merges API deprecation warnings into the result, and a routine like "count the non-empty lines" then treats one warning line as one resource. That makes an absent claim report as preserved, and an empty result report as present. -2. **Names are matched, not counted.** A count says how many lines came back, not whether the resources you asked about are the ones that came back. -3. **Every command that can fail is run inside a condition.** Under `set -e` a bare `out=$(kubectl …)` terminates the shell at the assignment — before the classification runs and before the caller records anything. +1. **stderr never mixes with resource output.** `2>&1` merges API deprecation warnings into the result, and "count the non-empty lines" then treats one warning as one resource. +2. **Identities are matched, not counted.** A count says how many lines came back, not whether the resources you asked about are the ones that came back. +3. **Every command that can fail runs inside a condition.** Under `set -e` a bare `out=$(kubectl …)` terminates the shell at the assignment — before classification, and before the caller records anything. ```sh -# ---- verdict aggregation ------------------------------------------------- -# Worst outcome wins, and no later success clears an earlier failure: -# 0 GONE/PRESERVED < 1 PRESENT/MISSING < 2 UNVERIFIED +# ============ harbor teardown verification library ======================== +# Source this, then call verify_teardown. Do not copy pieces of it. + +# Worst outcome wins: 0 VERIFIED/GONE < 1 INCOMPLETE/PRESENT < 2 UNVERIFIED VERDICT=0 record() { if [ "$1" -gt "$VERDICT" ]; then VERDICT=$1; fi; } -# Per-process stderr sink. `mktemp` is not POSIX either; $$ is enough here. -ERRF="${TMPDIR:-/tmp}/harbor-verify.$$.err" +# Private 0700 temp dir, removed on exit. A fixed /tmp path can be pre-created +# as a symlink by another user, and the stderr redirect then truncates whatever +# it points at. +VERIFY_TMP=$(mktemp -d) || { echo 'UNVERIFIED: cannot create temp dir'; exit 2; } +trap 'rm -rf "$VERIFY_TMP"' EXIT INT TERM +ERRF="$VERIFY_TMP/err" -# Emit any API warnings without ever letting them reach a counted stream. _note_stderr() { if [ -s "$ERRF" ]; then printf 'note: API wrote to stderr (not counted as resources):\n' >&2 @@ -205,125 +207,169 @@ _note_stderr() { fi } +# ---- read one inventory name list ---------------------------------------- +# THREE states, because "the list is empty" and "the list is gone" mean +# opposite things and an argument list cannot tell them apart: +# 0 -> readable, has entries (in $LIST) +# 1 -> readable and legitimately empty +# 2 -> missing or unreadable: the inventory itself failed +read_inventory() { + ri_f=$1; LIST='' + if [ ! -f "$ri_f" ]; then + printf 'UNVERIFIED: inventory file missing: %s\n' "$ri_f"; return 2 + fi + if LIST=$(cat -- "$ri_f" 2>"$ERRF"); then :; else + printf 'UNVERIFIED: cannot read inventory file: %s\n' "$ri_f"; _note_stderr; return 2 + fi + if [ -z "$LIST" ]; then return 1; fi + return 0 +} + # ---- poll a set of resources to gone ------------------------------------- -# usage: poll_gone -# by selector: poll_gone seinetwork,seinode -l sei.io/seinetwork= -# by name: poll_gone pvc --ignore-not-found ... +# usage: poll_gone +# by selector: poll_gone eng-x seinetwork,seinode,pod -l sei.io/seinetwork=c +# by name: poll_gone eng-x persistentvolumeclaim --ignore-not-found n1 n2 # --ignore-not-found is REQUIRED with explicit names: without it a deleted # resource returns NotFound and a nonzero exit, and the success condition # would report as UNVERIFIED. poll_gone() { - res=$1; shift - deadline=$(( $(date +%s) + 300 )) + pg_ns=$1; pg_res=$2; shift 2 + # 5 minutes by default. Raise it for an archive-scale finalizer; POLL_BUDGET + # also lets a test drive this function without waiting out the real budget. + pg_deadline=$(( $(date +%s) + ${POLL_BUDGET:-300} )) while : ; do - if out=$(kubectl --context harbor get "$res" -n eng- "$@" -o name 2>"$ERRF") - then rc=0; else rc=$?; fi - if [ "$rc" -ne 0 ]; then - printf 'UNVERIFIED: %s read failed (exit %s) — NOT confirmed\n' "$res" "$rc" + if pg_out=$(kubectl --context harbor -n "$pg_ns" get "$pg_res" "$@" -o name 2>"$ERRF") + then pg_rc=0; else pg_rc=$?; fi + if [ "$pg_rc" -ne 0 ]; then + printf 'UNVERIFIED: %s read failed in %s (exit %s)\n' "$pg_res" "$pg_ns" "$pg_rc" _note_stderr; return 2 fi _note_stderr - # stdout only, and only lines that look like a resource id. - left=$(printf '%s\n' "$out" | grep -c '^[a-z][a-z0-9.-]*/' || true) - if [ "$left" -eq 0 ]; then printf 'GONE: no %s matches\n' "$res"; return 0; fi - if [ "$(date +%s)" -ge "$deadline" ]; then - printf 'PRESENT at deadline: %s %s\n%s\n' "$left" "$res" "$out"; return 1 + pg_left=$(printf '%s\n' "$pg_out" | grep -c '^[a-z][a-z0-9.-]*/' || true) + if [ "$pg_left" -eq 0 ]; then printf 'GONE: no %s in %s\n' "$pg_res" "$pg_ns"; return 0; fi + if [ "$(date +%s)" -ge "$pg_deadline" ]; then + printf 'PRESENT at deadline: %s %s\n%s\n' "$pg_left" "$pg_res" "$pg_out"; return 1 fi - echo "$left $res remain"; sleep 10 + echo "$pg_left $pg_res remain"; sleep 10 done } # ---- assert the deliberately-preserved resources are still there --------- -# The mirror of poll_gone, for imported PVCs. A MISSING imported claim is a -# real finding: something deleted a volume the controller preserves by design. -# usage: expect_present pvc ... -# Matches the RETURNED IDENTITIES against the requested names. Counting lines -# cannot tell "the claim you asked for" from "some other line of output". +# usage: expect_present ... +# Pass the SINGULAR CANONICAL kind (persistentvolumeclaim, not pvc): `-o name` +# prints `/`, so the full returned identity is +# compared, kind included. A short alias would only match the name half. expect_present() { - kind=$1; shift + ep_ns=$1; ep_kind=$2; shift 2 if [ "$#" -eq 0 ]; then echo 'expect_present: no names given'; return 2; fi - if out=$(kubectl --context harbor get "$kind" -n eng- --ignore-not-found \ - "$@" -o name 2>"$ERRF") - then rc=0; else rc=$?; fi - if [ "$rc" -ne 0 ]; then - printf 'UNVERIFIED: %s read failed (exit %s) — preservation NOT confirmed\n' "$kind" "$rc" + if ep_out=$(kubectl --context harbor -n "$ep_ns" get "$ep_kind" --ignore-not-found \ + "$@" -o name 2>"$ERRF") + then ep_rc=0; else ep_rc=$?; fi + if [ "$ep_rc" -ne 0 ]; then + printf 'UNVERIFIED: %s read failed in %s (exit %s)\n' "$ep_kind" "$ep_ns" "$ep_rc" _note_stderr; return 2 fi _note_stderr - # `-o name` prints / (pvc -> persistentvolumeclaim/x), - # so compare on the bare name after the last slash. - got=$(printf '%s\n' "$out" | sed -n 's#^[a-z][a-z0-9.-]*/##p') - miss=0 - for want in "$@"; do - if ! printf '%s\n' "$got" | grep -qxF -- "$want"; then - printf 'MISSING: %s/%s is absent — a preserved claim was deleted\n' "$kind" "$want" - miss=1 + ep_miss=0 + for ep_want in "$@"; do + if ! printf '%s\n' "$ep_out" | grep -qxF -- "$ep_kind/$ep_want"; then + printf 'MISSING: %s/%s absent in %s — a preserved claim was deleted\n' \ + "$ep_kind" "$ep_want" "$ep_ns" + ep_miss=1 fi done - if [ "$miss" -ne 0 ]; then return 1; fi - printf 'PRESERVED: every requested %s still present\n' "$kind"; return 0 + if [ "$ep_miss" -ne 0 ]; then return 1; fi + printf 'PRESERVED: every requested %s still present\n' "$ep_kind"; return 0 } -``` -**Every call site records its outcome, and does so `set -e`-safely.** `poll_gone …; record $?` has two defects: under `set -e` a nonzero return terminates the script at the call, so `record` never runs; and when the argument list is built from a command substitution, `$?` is the helper's status and never the substitution's. Use an OR-list, and build argument lists in a separate, checked step. +# ---- THE one orchestration ----------------------------------------------- +# usage: verify_teardown +# chain: verify_teardown eng-x seinetwork,seinode,pod sei.io/seinetwork=c ./inv-c +# bench: verify_teardown eng-x job,configmap,pod sei.io/bench-name=r - +# Pass `-` for the inventory dir only where no PersistentVolumeClaim is in +# scope (a bench dir holds a Job and a ConfigMap and nothing else). +# Returns the worst outcome. Callers aggregate with `record`. +VT_WORST=0 +_vt_worse() { if [ "$1" -gt "$VT_WORST" ]; then VT_WORST=$1; fi; } + +verify_teardown() { + vt_ns=$1; vt_kinds=$2; vt_sel=$3; vt_inv=$4 + VT_WORST=0 + + if [ "$vt_inv" != "-" ]; then + # gate 0: the inventory must certify itself complete FOR THIS TARGET. + # A stale certificate from another chain, or from an earlier run of this + # one, must not authorize anything. + vt_rc=0; read_inventory "$vt_inv/status" || vt_rc=$? + if [ "$vt_rc" -ne 0 ]; then + printf 'UNVERIFIED: no readable completeness certificate in %s\n' "$vt_inv" + _vt_worse 2 + elif [ "$LIST" != "OK $vt_ns $vt_sel" ]; then + printf 'UNVERIFIED: certificate does not match this target\n want: OK %s %s\n got: %s\n' \ + "$vt_ns" "$vt_sel" "$LIST" + _vt_worse 2 + fi + + # gate 1: nodes the inventory could not resolve to any storage. + vt_rc=0; read_inventory "$vt_inv/unresolved-nodes.txt" || vt_rc=$? + case "$vt_rc" in + 0) printf 'UNVERIFIED: inventory left SeiNodes with no resolved storage:\n%s\n' "$LIST" + _vt_worse 2 ;; + 1) : ;; + 2) _vt_worse 2 ;; + esac + fi -```sh -# ---- read one inventory name list ---------------------------------------- -# THREE distinct states, because "the list is empty" and "the list is gone" -# mean opposite things and an argument list cannot tell them apart: -# 0 -> readable, has entries (in $LIST) -# 1 -> readable and legitimately empty (nothing of this class existed) -# 2 -> missing or unreadable: the inventory itself failed -# Inlining `$(cat f)` into a helper's arguments collapses states 1 and 2 into -# an empty argument list, and the helper then succeeds against an empty -# namespace — losing the inventory reads as a clean teardown. -read_inventory() { - f=$1; LIST='' - if [ ! -f "$f" ]; then - printf 'UNVERIFIED: inventory file missing: %s\n' "$f"; return 2 + # The objects themselves. An EMPTY selector means "everything of these kinds + # in the namespace" — used for the unlabelled sweep at the end of a namespace + # teardown. Pass no -l at all rather than `-l ""`. + vt_rc=0 + if [ -n "$vt_sel" ]; then + poll_gone "$vt_ns" "$vt_kinds" -l "$vt_sel" || vt_rc=$? + else + poll_gone "$vt_ns" "$vt_kinds" || vt_rc=$? fi - if LIST=$(cat -- "$f" 2>"$ERRF"); then :; else - printf 'UNVERIFIED: cannot read inventory file: %s\n' "$f"; _note_stderr; return 2 + _vt_worse "$vt_rc" + + if [ "$vt_inv" != "-" ]; then + # controller-managed claims: BY NAME, never a namespace sweep + vt_rc=0; read_inventory "$vt_inv/managed-claims.txt" || vt_rc=$? + case "$vt_rc" in + 0) vt_p=0 + poll_gone "$vt_ns" persistentvolumeclaim --ignore-not-found $LIST || vt_p=$? + _vt_worse "$vt_p" ;; + 1) echo 'NOTE: no controller-managed claims inventoried — nothing to poll' ;; + 2) _vt_worse 2 ;; + esac + + # imported claims must SURVIVE + vt_rc=0; read_inventory "$vt_inv/imported-claims.txt" || vt_rc=$? + case "$vt_rc" in + 0) vt_p=0 + expect_present "$vt_ns" persistentvolumeclaim $LIST || vt_p=$? + _vt_worse "$vt_p" ;; + 1) echo 'NOTE: this target imported no claims — nothing to preserve' ;; + 2) _vt_worse 2 ;; + esac fi - if [ -z "$LIST" ]; then return 1; fi - return 0 + + case "$VT_WORST" in + 0) printf 'VERIFIED %s %s\n' "$vt_ns" "$vt_sel" ;; + 1) printf 'INCOMPLETE %s %s — objects remain, or a preserved claim vanished\n' "$vt_ns" "$vt_sel" ;; + 2) printf 'UNVERIFIED %s %s — state unknown, do not report done\n' "$vt_ns" "$vt_sel" ;; + esac + return "$VT_WORST" } +``` -# ---- the teardown verification, in order --------------------------------- -INV=./teardown-inventory - -# 0. The inventory must have declared itself complete. An incomplete inventory -# cannot support a "verified" verdict no matter what the polls say. -rc=0; read_inventory "$INV/status" || rc=$? -if [ "$rc" -ne 0 ] || [ "$LIST" != "OK" ]; then - echo 'UNVERIFIED: inventory incomplete or unreadable — see unresolved-nodes.txt' - record 2 -fi - -rc=0; poll_gone seinetwork,seinode -l sei.io/seinetwork= || rc=$?; record "$rc" -rc=0; poll_gone pods -l sei.io/seinetwork= || rc=$?; record "$rc" - -# PVCs BY NAME, never by namespace sweep: a sweep also matches imported claims -# and other chains' claims, so a correct teardown would report PRESENT. -rc=0; read_inventory "$INV/managed-claims.txt" || rc=$? -case "$rc" in - 0) prc=0; poll_gone pvc --ignore-not-found $LIST || prc=$?; record "$prc" ;; - 1) echo 'NOTE: no controller-managed claims were inventoried — nothing to poll' ;; - 2) record 2 ;; -esac - -rc=0; read_inventory "$INV/imported-claims.txt" || rc=$? -case "$rc" in - 0) prc=0; expect_present pvc $LIST || prc=$?; record "$prc" ;; - 1) echo 'NOTE: this chain imported no claims — nothing to preserve' ;; - 2) record 2 ;; -esac - -case "$VERDICT" in - 0) echo 'TEARDOWN VERIFIED — every checked object reached its expected state' ;; - 1) echo 'TEARDOWN INCOMPLETE — objects remain, or a preserved claim vanished' ;; - 2) echo 'TEARDOWN UNVERIFIED — an API read failed; state unknown, do not report done' ;; -esac +**Callers do exactly this and nothing more.** The OR-list matters: `verify_teardown …; record $?` terminates the script at the call under `set -e`, so `record` never runs. + +```sh +# one chain +rc=0 +verify_teardown eng- seinetwork,seinode,pod \ + "sei.io/seinetwork=" ./teardown-inventory- || rc=$? +record "$rc" exit "$VERDICT" ``` @@ -333,7 +379,7 @@ kubectl --context harbor get seinetwork,seinode -n eng- -l sei.io/seinetw -o custom-columns='NAME:.metadata.name,PHASE:.status.phase,DELETED:.metadata.deletionTimestamp,FINALIZERS:.metadata.finalizers' ``` -**Zero PVCs is the wrong expectation, and a namespace-wide PVC poll is the wrong check.** The SeiNode finalizer deliberately skips an imported PVC, so imported claims survive by design and other chains' claims are none of this teardown's business. Both make a namespace sweep report `PRESENT` after a correct teardown. Poll the target chain's **controller-managed** claims by name, and assert the imported ones separately with `expect_present`. Both name lists come from `teardown.md` inventory step 2, captured **before** the SeiNodes are deleted — afterwards nothing in the cluster still says which claims were which. +**Zero PVCs is the wrong expectation, and a namespace-wide PVC poll is the wrong check.** The SeiNode finalizer deliberately skips an imported PVC, so imported claims survive by design and other chains' claims are none of this teardown's business. Both make a namespace sweep report `PRESENT` after a correct teardown. `verify_teardown` therefore polls the target's controller-managed claims by name and asserts the imported ones separately, from the lists `teardown.md` inventory step 2 captured **before** the SeiNodes were deleted — afterwards nothing in the cluster still says which claims were which. `sei.io/seinode-finalizer` on a parked SeiNode means the controller has not released the PVC — an unhealthy controller or an EBS CSI flake. See `teardown.md` → *a stuck `Terminating` object is a real signal*. @@ -395,17 +441,18 @@ After the PR merges, reconcile the engineer's own Kustomization and **poll** the ```sh flux --context harbor reconcile kustomization -n eng- --with-source -# poll_gone and record from recipe #9 — same three outcomes, same aggregation. -# Pods are included deliberately: the Job can be gone while its pod is still Terminating. +# The SAME verify_teardown from recipe #9 — a bench is not a special case. +# `pod` is in the kind list deliberately: the Job can be gone while its pod is +# still Terminating. `-` for the inventory dir because a bench dir holds a Job +# and a ConfigMap and no PersistentVolumeClaim, so no claim lists exist. VERDICT=0 -poll_gone job,configmap -l sei.io/bench-name=; record $? -poll_gone pods -l sei.io/bench-name=; record $? +rc=0 +verify_teardown eng- job,configmap,pod "sei.io/bench-name=" - || rc=$? +record "$rc" exit "$VERDICT" ``` -A bench dir holds no SeiNetwork and no PVC, so there is nothing to poll by name here — the `sei.io/bench-name` selector already scopes both calls to this run. - -`record $?` is not optional. Without it the block's status is the last call's, so an `UNVERIFIED` on the Jobs followed by a clean pods read exits 0. An `UNVERIFIED` from either call means the bench teardown is unconfirmed, not clean. Flux prunes the Job + ConfigMap on that reconcile; Pods cascade per k8s deletion propagation. The `` task dir leaves the engineer's workspace tree. Bench results already in S3 are untouched. +An `UNVERIFIED` here means the bench teardown is unconfirmed, not clean. Results already in S3 are untouched either way. Flux prunes the Job + ConfigMap on that reconcile; Pods cascade per k8s deletion propagation. The `` task dir leaves the engineer's workspace tree. Bench results already in S3 are untouched. ## When a recipe doesn't match observed output diff --git a/.claude/skills/harbor-dev/references/teardown.md b/.claude/skills/harbor-dev/references/teardown.md index 5aa8b7c1..d8ed42c1 100644 --- a/.claude/skills/harbor-dev/references/teardown.md +++ b/.claude/skills/harbor-dev/references/teardown.md @@ -110,13 +110,26 @@ Teardown follows the same PR contract as spinup: render the change, open a PR, l # the current locale collates, and a locale that ignores punctuation orders # hyphenated claim names differently. Pin both to codepoint order. export LC_ALL=C - ALIAS=; CHAIN=; INV=./teardown-inventory + ALIAS=; CHAIN= + [ -n "$ALIAS" ] && [ -n "$CHAIN" ] || { echo 'inventory: ALIAS and CHAIN are required'; exit 2; } + INV=./teardown-inventory-$CHAIN + + # A FRESH directory per run. Reusing one leaves a previous run's completeness + # certificate in place, and `set -e` exits on the first failed read below — + # before anything invalidates it. The verifier would then accept a stale OK + # sitting beside half-refreshed lists. + rm -rf "$INV" mkdir -p "$INV" K="kubectl --context harbor -n eng-$ALIAS" # Raw reads, each REDIRECTED to a file rather than piped. In a POSIX shell # `kubectl ... | jq ...` exits with jq's status, so a Forbidden from kubectl # would pass through as success — the same defect this file exists to prevent. + # + # No completeness certificate is written until the very end. Until then the + # file is simply absent, which read_inventory reports as UNVERIFIED — so an + # abort at any point below leaves the verifier refusing to pass, with no + # window in which a stale certificate could authorize anything. $K get seinetwork,seinode -l "sei.io/seinetwork=$CHAIN" \ -o custom-columns='KIND:.kind,NAME:.metadata.name,ROLE:.metadata.labels.sei\.io/role,PHASE:.status.phase' \ > "$INV/crs.txt" @@ -124,10 +137,6 @@ Teardown follows the same PR contract as spinup: render the change, open a PR, l $K get pods -l "sei.io/seinetwork=$CHAIN" -o json > "$INV/pods.json" $K get pvc -o json > "$INV/pvcs.json" - # The verifier refuses to pass while status reads UNRESOLVED. It is only - # flipped to OK if every completeness check below succeeds. - printf 'UNRESOLVED\n' > "$INV/status" - # Imported claims — PRESERVED by design. `unique` sorts, which comm needs. # Every jq call is a single command with a redirect: in a pipeline its # status would be masked, and a parse failure would look like an empty list. @@ -146,7 +155,12 @@ Teardown follows the same PR contract as spinup: render the change, open a PR, l | unique | .[] | "\(.node)\t\(.claim)"' \ "$INV/pods.json" > "$INV/node-claims.tsv" - cut -f2 "$INV/node-claims.tsv" | sort -u > "$INV/all-claims.txt" + # Each transformation is its own command. In a POSIX shell `cut … | sort -u` + # exits with sort's status, so a failed cut yields a successful EMPTY claim + # list — and the inventory would then certify itself complete while omitting + # every managed claim. set -e does not catch a non-final pipeline failure. + cut -f2 "$INV/node-claims.tsv" > "$INV/all-claims.raw" + sort -u "$INV/all-claims.raw" > "$INV/all-claims.txt" # Controller-managed = attributed minus imported. These MUST disappear. comm -23 "$INV/all-claims.txt" "$INV/imported-claims.txt" > "$INV/managed-claims.txt" @@ -163,7 +177,14 @@ Teardown follows the same PR contract as spinup: render the change, open a PR, l "$INV/nodes.json" > "$INV/imported-nodes.txt" # Drop the empty owner field: a pod with no ownerReferences still yields its # claim above, but attributes to no node — so its node stays unresolved. - cut -f1 "$INV/node-claims.tsv" | grep -v '^$' | sort -u > "$INV/nodes-with-claims.txt" + # Three separate commands, same reason as above. grep's exit 1 (nothing + # matched) is a legitimate outcome here; 2 or more is a real failure. + cut -f1 "$INV/node-claims.tsv" > "$INV/owners.raw" + if grep -v '^$' "$INV/owners.raw" > "$INV/owners.nonempty"; then :; else + gs=$? + [ "$gs" -eq 1 ] || { echo 'inventory: owner filter failed'; exit 2; } + fi + sort -u "$INV/owners.nonempty" > "$INV/nodes-with-claims.txt" sort -u "$INV/imported-nodes.txt" "$INV/nodes-with-claims.txt" > "$INV/resolved-nodes.txt" comm -23 "$INV/nodes.txt" "$INV/resolved-nodes.txt" > "$INV/unresolved-nodes.txt" @@ -184,10 +205,14 @@ Teardown follows the same PR contract as spinup: render the change, open a PR, l if [ -s "$INV/unresolved-nodes.txt" ]; then printf 'INVENTORY INCOMPLETE — SeiNodes that resolve to no storage\n' cat "$INV/unresolved-nodes.txt" - printf 'status stays UNRESOLVED; the verifier will report UNVERIFIED.\n' + printf 'No certificate written; the verifier will report UNVERIFIED.\n' exit 2 fi - printf 'OK\n' > "$INV/status" + + # The certificate names the target it certifies. A complete inventory for a + # DIFFERENT namespace or chain must not authorize this one, and verify_teardown + # compares this string against the target it was called with. + printf 'OK eng-%s sei.io/seinetwork=%s\n' "$ALIAS" "$CHAIN" > "$INV/status" ``` Claim names come from the **pods' own `spec.volumes[].persistentVolumeClaim.claimName`**, not from a guessed naming rule — the controller owns how it names a generated claim, and a rule inferred here would desync the moment it changes. @@ -253,28 +278,21 @@ A successful reconcile says Flux applied the change. It does not say the objects **And a later success must never overwrite an earlier failure.** Printing `UNVERIFIED` is not enough on its own: a `break` out of a loop, or a bare call whose return code nobody reads, still leaves the block exiting 0. A human sees the warning; a wrapper script or an agent reading `$?` sees success. Every check records its outcome into a running verdict, and the worst one wins. -**Use `poll_gone`, `expect_present`, and `record` from `cluster-inspection-recipes.md` recipe #9 — do not re-implement them here.** One implementation, one place to fix. Source them, then: +**All of that lives in one function.** `verify_teardown` in `cluster-inspection-recipes.md` recipe #9 carries the completeness gate, the checked list reads, the empty-list branches, the polls, and the aggregation. This file calls it and reads its return value — there is deliberately no verification shell here to drift out of step with the library: ```sh -VERDICT=0 - -# The chain's CRs and pods, by label. -poll_gone seinetwork,seinode -l sei.io/seinetwork=; record $? -poll_gone pods -l sei.io/seinetwork=; record $? - -# The claims, BY NAME, from inventory step 2 — never a namespace sweep. -poll_gone pvc --ignore-not-found $(cat ./teardown-inventory/managed-claims.txt); record $? -expect_present pvc $(cat ./teardown-inventory/imported-claims.txt); record $? +. ./verify-lib.sh # the library block from recipe #9 -case "$VERDICT" in - 0) echo 'TEARDOWN VERIFIED — the chain is gone and preserved claims are intact' ;; - 1) echo 'TEARDOWN INCOMPLETE — objects remain, or a preserved claim vanished' ;; - 2) echo 'TEARDOWN UNVERIFIED — an API read failed; state unknown, do not report done' ;; -esac +rc=0 +verify_teardown eng- seinetwork,seinode,pod \ + "sei.io/seinetwork=" ./teardown-inventory- || rc=$? +record "$rc" exit "$VERDICT" ``` -If `managed-claims.txt` is empty, skip that `poll_gone` rather than calling it with no names — `kubectl get pvc` with no arguments lists the whole namespace, which is the sweep this avoids. Same for `expect_present` and an empty imported list. +That is the whole verification step. **If you find yourself writing a `poll_gone` line in this file, stop** — a second copy of the orchestration is how this exact defect survived four review rounds, reappearing one layer up each time: duplicated poll bodies, then the caller chain, then the arguments feeding the callers, then a canonical caller that bypassed the fixed library entirely while the paragraph above it said not to re-implement. + +The empty-list cases are handled inside the function, as code rather than as advice here: an empty `managed-claims.txt` takes a `NOTE` branch instead of calling `poll_gone` with no names, because `kubectl get persistentvolumeclaim` with no arguments lists the whole namespace — the sweep this design exists to avoid. **Zero PVCs is the wrong expectation, and a namespace sweep is the wrong check.** The SeiNode finalizer deliberately skips an imported claim, so those survive by design, and other chains' claims are none of this teardown's business. Both make a sweep report `PRESENT` after a correct teardown — a false alarm that trains the reader to ignore the check. The expected end state is precise: every controller-managed claim of this chain gone, every imported claim still present. @@ -318,16 +336,54 @@ It does **not** remove: ```sh kubectl --context harbor get seinetwork,seinode,job,pvc -n eng- - kubectl --context harbor get seinode -n eng- \ - -o jsonpath='{range .items[*]}{.metadata.labels.sei\.io/seinetwork}{"\n"}{end}' | sort -u ``` - Run `inventory.sh` **once per chain-id** that second command returns, each with its own `INV` directory (`INV=./teardown-inventory-`). A namespace has more than one chain more often than not, and a single sweep cannot tell one chain's controller-managed claim from another's. + Then discover the chain-ids **with the discovery's own status checked**. Piping `kubectl` into `sort` exits with sort's status, so a `Forbidden` becomes a successful empty list — and "no chains found" then reads as "nothing to do", which is the whole defect class this document exists to close: - Any chain whose `inventory.sh` exits non-zero stops the whole namespace teardown: its `status` stays `UNRESOLVED`, and emptying a namespace on an incomplete inventory is how a leak becomes invisible. Claims that no chain attributes land in each run's `unattributed-claims.txt` — take them to the leak sweep in step 5, not to a delete. + ```sh + . ./verify-lib.sh # recipe #9 — provides ERRF, _note_stderr, record, VERDICT + VERDICT=0 + + if raw=$(kubectl --context harbor get seinode -n eng- \ + -o jsonpath='{range .items[*]}{.metadata.labels.sei\.io/seinetwork}{"\n"}{end}' 2>"$ERRF") + then + chains=$(printf '%s\n' "$raw" | grep -v '^$' | sort -u || true) + else + echo 'UNVERIFIED: chain discovery failed — the namespace inventory is unknown' + _note_stderr; record 2; chains='' + fi + ``` + + Run `inventory.sh` **once per chain-id**, each writing its own `./teardown-inventory-`. A namespace usually holds more than one chain, and a single sweep cannot tell one chain's controller-managed claim from another's. Any chain whose `inventory.sh` exits non-zero writes no certificate, and its verification then reports `UNVERIFIED` — emptying a namespace on an incomplete inventory is how a leak becomes invisible. Claims no chain attributes land in each run's `unattributed-claims.txt`; take those to the leak sweep in step 5, not to a delete. 2. For every SeiNetwork in the inventory, run the `deletionPolicy` gate in [The `deletionPolicy: Retain` trap](#the-deletionpolicy-retain-trap). One `Retain` network is enough to leak a set of disks. 3. `git rm -r` every task dir under `engineers//`, and reduce `engineers//kustomization.yaml` to `resources: []`. Keep that file: deleting it makes the Flux Kustomization fail reconcile with `path not found`, which is the same breakage the onboarding scaffolding PR exists to prevent. -4. Open the PR, merge, then run [Verify the teardown](#verify-the-teardown) **once per chain**, each against its own `INV` directory. Do not substitute a namespace-wide PVC poll: it matches imported and unattributed claims too, so it reports `PRESENT` after a correct teardown. The CR and pod polls may drop their `-l` selector to sweep the namespace; the claim checks may not. +4. Open the PR, merge, then verify **every chain, retaining the worst result**. One `VERDICT` spans the whole namespace, so a clean second chain cannot cover an unverified first one: + + ```sh + # Same shell as step 1 — the library is already sourced and $VERDICT already + # carries a 2 if chain discovery failed. + for c in $chains; do + rc=0 + verify_teardown eng- seinetwork,seinode,pod \ + "sei.io/seinetwork=$c" "./teardown-inventory-$c" || rc=$? + record "$rc" + done + + # Anything left that carries no chain label at all — an escape-hatch apply, + # or an orphan whose labels were stripped. No inventory applies, so `-`. + rc=0 + verify_teardown eng- seinetwork,seinode,pod "" - || rc=$? + record "$rc" + + case "$VERDICT" in + 0) echo 'NAMESPACE EMPTIED — every chain verified' ;; + 1) echo 'NAMESPACE NOT EMPTY — objects remain in at least one chain' ;; + 2) echo 'NAMESPACE UNVERIFIED — at least one check could not run' ;; + esac + exit "$VERDICT" + ``` + + An empty `$chains` after a **successful** discovery is legitimate — the namespace has no labelled chains — and the unlabelled sweep still runs. An empty `$chains` after a **failed** discovery already recorded `2`, so the loop running zero times cannot pass. Do not substitute a namespace-wide PVC poll anywhere here: it matches imported and unattributed claims too, so it reports `PRESENT` after a correct teardown. 5. Sweep for what git never owned — [Find and clean up already-leaked resources](#find-and-clean-up-already-leaked-resources). ### Remove the namespace entirely (offboarding) @@ -494,17 +550,9 @@ Anything not in this table, or any case where the ownership question stays open, **This is why the direct deletes above use `kubectl`, not `seictl`.** `seictl`'s common flags are `--kubeconfig` and `-n/--namespace` only (`seictl-cli.md` → *Common flags on every verb*) — **there is no `--context`**, and the namespace falls back to the kubeconfig context's default. A `seictl delete` therefore cannot pin the cluster on its own command line; writing "(harbor context)" beside it states an intention the command does not enforce. `kubectl --context harbor delete -n eng-` pins both on the line that does the deleting, and issues the same Delete against the same CR (`seictl-cli.md` → `seictl network|node delete`). -If a workflow genuinely needs `seictl` for a destructive verb, pin the cluster out of band and prove it immediately before, in the same command list — a guard that runs, not a parenthetical: - -```sh -ctx=$(kubectl config current-context) || { echo 'UNRESOLVED: cannot read current context'; exit 2; } -if [ "$ctx" != "harbor" ]; then - printf 'REFUSED: current context is %s, not harbor\n' "$ctx"; exit 2 -fi -seictl network delete -n eng- -``` +**There is no `seictl` alternative for network or node deletion here, guarded or otherwise.** A `kubectl config current-context` check reads mutable state rather than pinning the config the delete then consumes, so it leaves a window between the check and the call — and it buys nothing, because `kubectl --context harbor delete` does the same deletion against the same CR with no window at all. -That still leaves a window between the check and the call. `kubectl --context harbor` has no window, which is why it is the documented path. +Should some genuinely `seictl`-only destructive verb ever need documenting, the pin belongs on the invocation: hand it a kubeconfig that contains the harbor cluster and nothing else, via `--kubeconfig ` (a documented `seictl` flag). A file that cannot name another cluster cannot select one. Do not substitute a current-context check. ### The ownership search that authorizes a direct delete From ba2c5a050ead134b12e3e0b8d050448f9161c5bb Mon Sep 17 00:00:00 2001 From: bdchatham Date: Wed, 9 Sep 2026 22:55:48 +0000 Subject: [PATCH 15/18] docs(harbor-dev): reword two messages that trip the Windows-path checker `storage:\n` matches skill-package-checks A2's drive-letter pattern as `e:\n`. Same false positive as round 4, same fix. Co-authored-by: omnigent --- .../skills/harbor-dev/references/cluster-inspection-recipes.md | 2 +- .claude/skills/harbor-dev/references/teardown.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.claude/skills/harbor-dev/references/cluster-inspection-recipes.md b/.claude/skills/harbor-dev/references/cluster-inspection-recipes.md index 475d63b4..9517693a 100644 --- a/.claude/skills/harbor-dev/references/cluster-inspection-recipes.md +++ b/.claude/skills/harbor-dev/references/cluster-inspection-recipes.md @@ -313,7 +313,7 @@ verify_teardown() { # gate 1: nodes the inventory could not resolve to any storage. vt_rc=0; read_inventory "$vt_inv/unresolved-nodes.txt" || vt_rc=$? case "$vt_rc" in - 0) printf 'UNVERIFIED: inventory left SeiNodes with no resolved storage:\n%s\n' "$LIST" + 0) printf 'UNVERIFIED: inventory left these SeiNodes unresolved\n%s\n' "$LIST" _vt_worse 2 ;; 1) : ;; 2) _vt_worse 2 ;; diff --git a/.claude/skills/harbor-dev/references/teardown.md b/.claude/skills/harbor-dev/references/teardown.md index d8ed42c1..6bf1c122 100644 --- a/.claude/skills/harbor-dev/references/teardown.md +++ b/.claude/skills/harbor-dev/references/teardown.md @@ -550,7 +550,7 @@ Anything not in this table, or any case where the ownership question stays open, **This is why the direct deletes above use `kubectl`, not `seictl`.** `seictl`'s common flags are `--kubeconfig` and `-n/--namespace` only (`seictl-cli.md` → *Common flags on every verb*) — **there is no `--context`**, and the namespace falls back to the kubeconfig context's default. A `seictl delete` therefore cannot pin the cluster on its own command line; writing "(harbor context)" beside it states an intention the command does not enforce. `kubectl --context harbor delete -n eng-` pins both on the line that does the deleting, and issues the same Delete against the same CR (`seictl-cli.md` → `seictl network|node delete`). -**There is no `seictl` alternative for network or node deletion here, guarded or otherwise.** A `kubectl config current-context` check reads mutable state rather than pinning the config the delete then consumes, so it leaves a window between the check and the call — and it buys nothing, because `kubectl --context harbor delete` does the same deletion against the same CR with no window at all. +**No `seictl` alternative for network or node deletion belongs here, guarded or otherwise.** A `kubectl config current-context` check reads mutable state rather than pinning the config the delete then consumes, so it leaves a window between the check and the call — and it buys nothing, because `kubectl --context harbor delete` does the same deletion against the same CR with no window at all. Should some genuinely `seictl`-only destructive verb ever need documenting, the pin belongs on the invocation: hand it a kubeconfig that contains the harbor cluster and nothing else, via `--kubeconfig ` (a documented `seictl` flag). A file that cannot name another cluster cannot select one. Do not substitute a current-context check. From 604b772386d98e96f4a6864e4343054d742b0bed Mon Sep 17 00:00:00 2001 From: bdchatham Date: Wed, 9 Sep 2026 23:08:45 +0000 Subject: [PATCH 16/18] fix(harbor-dev): check discovery transformations, sweep residuals before the verdict, isolate traps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Chain discovery checked its API read but not its transformations: `| sort -u` takes sort's status and `|| true` swallowed the rest, so a failed grep or sort produced a successful EMPTY chain list, the per-chain loop ran zero times, and every certificate and claim check was skipped at VERDICT 0. Filter and sort are now separate checked steps where only grep's exit 1 counts as legitimate emptiness. The prose that argued this could not happen carried the same gap — it reasoned only about an API failure — and now names both. Both orphan sweeps piped an unchecked kubectl into jq. A failed read gives jq no input, and jq exits 0 with no output, so an unreadable sweep read as "no orphans". Read and parse are separated and both checked. The namespace path printed NAMESPACE EMPTIED and exited before step 5, so a namespace whose only leftover was a leaked PVC passed: clean discovery, clean CR/pod poll, exit 0. The `-` call is now documented and used as a CR/pod disappearance check only; a new sweep_residual runs first, excludes each chain's imported claims BY NAME as expected survivors, and its result is aggregated before anything is printed or exited. Sourcing the library no longer installs a trap or creates a temp dir: verify_ teardown and sweep_residual are subshell functions owning their own temp dir and traps, so a caller's EXIT handler survives, nothing leaks on return, and a later check cannot inherit a deleted stderr dir. An interrupt exits 130/143, which is above every verdict value, so both verdict switches gained an explicit catch-all rather than ending silently. Also: the inventory abort note said status "stays UNRESOLVED"; the implementation writes no certificate at all. Verified under dash and bash, with and without set -eu: 11 prior scenarios still pass, plus 11 new — grep/sort failures in discovery, all-empty labels as legitimate, residual sweep with a leaked PVC vs an imported survivor, forbidden residual read, unreadable survivors list, and the signature case where the CR/pod check alone verdicts 0 while the residual sweep correctly verdicts 1. Co-authored-by: omnigent --- .../references/cluster-inspection-recipes.md | 141 ++++++++++++++---- .../skills/harbor-dev/references/teardown.md | 83 +++++++++-- 2 files changed, 184 insertions(+), 40 deletions(-) diff --git a/.claude/skills/harbor-dev/references/cluster-inspection-recipes.md b/.claude/skills/harbor-dev/references/cluster-inspection-recipes.md index 9517693a..ffcbf0de 100644 --- a/.claude/skills/harbor-dev/references/cluster-inspection-recipes.md +++ b/.claude/skills/harbor-dev/references/cluster-inspection-recipes.md @@ -187,23 +187,25 @@ Written for a portable shell (`dash`, `ash`, `bash`). Three deliberate non-POSIX ```sh # ============ harbor teardown verification library ======================== -# Source this, then call verify_teardown. Do not copy pieces of it. +# Source this, then call verify_teardown / sweep_residual. Do not copy pieces. +# +# Sourcing this file installs NO trap and creates NO temp dir. Each public +# function is a SUBSHELL function — `name() ( ... )`, not `{ ... }` — so the +# temp dir and its trap live and die inside that subshell. A source-time +# `trap ... EXIT` silently replaces whatever EXIT handler the calling script +# already had, and a dir cleaned on INT without re-raising leaves later checks +# writing stderr into a directory that no longer exists. # Worst outcome wins: 0 VERIFIED/GONE < 1 INCOMPLETE/PRESENT < 2 UNVERIFIED VERDICT=0 record() { if [ "$1" -gt "$VERDICT" ]; then VERDICT=$1; fi; } -# Private 0700 temp dir, removed on exit. A fixed /tmp path can be pre-created -# as a symlink by another user, and the stderr redirect then truncates whatever -# it points at. -VERIFY_TMP=$(mktemp -d) || { echo 'UNVERIFIED: cannot create temp dir'; exit 2; } -trap 'rm -rf "$VERIFY_TMP"' EXIT INT TERM -ERRF="$VERIFY_TMP/err" - +# usage: _note_stderr — emit API warnings without ever letting them +# reach a counted stream. _note_stderr() { - if [ -s "$ERRF" ]; then + if [ -s "$1" ]; then printf 'note: API wrote to stderr (not counted as resources):\n' >&2 - cat "$ERRF" >&2 + cat "$1" >&2 fi } @@ -219,7 +221,8 @@ read_inventory() { printf 'UNVERIFIED: inventory file missing: %s\n' "$ri_f"; return 2 fi if LIST=$(cat -- "$ri_f" 2>"$ERRF"); then :; else - printf 'UNVERIFIED: cannot read inventory file: %s\n' "$ri_f"; _note_stderr; return 2 + printf 'UNVERIFIED: cannot read inventory file: %s\n' "$ri_f" + _note_stderr "$ERRF"; return 2 fi if [ -z "$LIST" ]; then return 1; fi return 0 @@ -231,7 +234,7 @@ read_inventory() { # by name: poll_gone eng-x persistentvolumeclaim --ignore-not-found n1 n2 # --ignore-not-found is REQUIRED with explicit names: without it a deleted # resource returns NotFound and a nonzero exit, and the success condition -# would report as UNVERIFIED. +# would report as UNVERIFIED. Uses $ERRF from the enclosing subshell. poll_gone() { pg_ns=$1; pg_res=$2; shift 2 # 5 minutes by default. Raise it for an archive-scale finalizer; POLL_BUDGET @@ -242,9 +245,9 @@ poll_gone() { then pg_rc=0; else pg_rc=$?; fi if [ "$pg_rc" -ne 0 ]; then printf 'UNVERIFIED: %s read failed in %s (exit %s)\n' "$pg_res" "$pg_ns" "$pg_rc" - _note_stderr; return 2 + _note_stderr "$ERRF"; return 2 fi - _note_stderr + _note_stderr "$ERRF" pg_left=$(printf '%s\n' "$pg_out" | grep -c '^[a-z][a-z0-9.-]*/' || true) if [ "$pg_left" -eq 0 ]; then printf 'GONE: no %s in %s\n' "$pg_res" "$pg_ns"; return 0; fi if [ "$(date +%s)" -ge "$pg_deadline" ]; then @@ -267,9 +270,9 @@ expect_present() { then ep_rc=0; else ep_rc=$?; fi if [ "$ep_rc" -ne 0 ]; then printf 'UNVERIFIED: %s read failed in %s (exit %s)\n' "$ep_kind" "$ep_ns" "$ep_rc" - _note_stderr; return 2 + _note_stderr "$ERRF"; return 2 fi - _note_stderr + _note_stderr "$ERRF" ep_miss=0 for ep_want in "$@"; do if ! printf '%s\n' "$ep_out" | grep -qxF -- "$ep_kind/$ep_want"; then @@ -286,20 +289,30 @@ expect_present() { # usage: verify_teardown # chain: verify_teardown eng-x seinetwork,seinode,pod sei.io/seinetwork=c ./inv-c # bench: verify_teardown eng-x job,configmap,pod sei.io/bench-name=r - -# Pass `-` for the inventory dir only where no PersistentVolumeClaim is in -# scope (a bench dir holds a Job and a ConfigMap and nothing else). +# sweep: verify_teardown eng-x seinetwork,seinode,pod "" - +# `-` for the inventory dir means NO PersistentVolumeClaim is in scope for this +# call. It is a CR/pod disappearance check ONLY — it proves nothing about +# storage, so it can never on its own justify calling a namespace empty. +# An empty selector means "every object of these kinds in the namespace". # Returns the worst outcome. Callers aggregate with `record`. VT_WORST=0 _vt_worse() { if [ "$1" -gt "$VT_WORST" ]; then VT_WORST=$1; fi; } -verify_teardown() { +verify_teardown() ( vt_ns=$1; vt_kinds=$2; vt_sel=$3; vt_inv=$4 VT_WORST=0 + vt_tmp=$(mktemp -d) || { echo 'UNVERIFIED: cannot create temp dir'; return 2; } + # Traps are set INSIDE this subshell, so the caller's handlers are untouched. + # INT/TERM re-raise the conventional status instead of continuing. + trap 'rm -rf "$vt_tmp"' EXIT + trap 'rm -rf "$vt_tmp"; exit 130' INT + trap 'rm -rf "$vt_tmp"; exit 143' TERM + ERRF="$vt_tmp/err" if [ "$vt_inv" != "-" ]; then # gate 0: the inventory must certify itself complete FOR THIS TARGET. - # A stale certificate from another chain, or from an earlier run of this - # one, must not authorize anything. + # A certificate from another chain, or from an earlier run of this one, + # must not authorize anything. vt_rc=0; read_inventory "$vt_inv/status" || vt_rc=$? if [ "$vt_rc" -ne 0 ]; then printf 'UNVERIFIED: no readable completeness certificate in %s\n' "$vt_inv" @@ -357,9 +370,70 @@ verify_teardown() { 0) printf 'VERIFIED %s %s\n' "$vt_ns" "$vt_sel" ;; 1) printf 'INCOMPLETE %s %s — objects remain, or a preserved claim vanished\n' "$vt_ns" "$vt_sel" ;; 2) printf 'UNVERIFIED %s %s — state unknown, do not report done\n' "$vt_ns" "$vt_sel" ;; + # An interrupt exits 130/143, which is above every verdict value. Without + # this branch the case matches nothing and the run ends silently. + *) printf 'ABORTED %s %s — interrupted or unexpected status %s; treat as unverified\n' \ + "$vt_ns" "$vt_sel" "$VT_WORST" ;; esac return "$VT_WORST" -} +) + +# ---- residual sweep: what is LEFT that no teardown accounted for --------- +# usage: sweep_residual +# Storage is the point: a namespace whose only leftover is a leaked PVC must +# not pass. Imported claims are EXPECTED to survive, so they are excluded BY +# NAME — this never demands zero PersistentVolumeClaims. +# Returns 0 clear, 1 residual found, 2 a read failed. +sweep_residual() ( + sr_ns=$1; sr_expect=$2 + sr_worst=0 + sr_tmp=$(mktemp -d) || { echo 'UNVERIFIED: cannot create temp dir'; return 2; } + trap 'rm -rf "$sr_tmp"' EXIT + trap 'rm -rf "$sr_tmp"; exit 130' INT + trap 'rm -rf "$sr_tmp"; exit 143' TERM + ERRF="$sr_tmp/err" + + sr_keep='' + if [ "$sr_expect" != "-" ]; then + sr_rc=0; read_inventory "$sr_expect" || sr_rc=$? + case "$sr_rc" in + 0) sr_keep=$LIST ;; + 1) sr_keep='' ;; + 2) printf 'UNVERIFIED: cannot read the expected-survivors list\n'; sr_worst=2 ;; + esac + fi + + for sr_kind in persistentvolumeclaim job configmap; do + if sr_out=$(kubectl --context harbor -n "$sr_ns" get "$sr_kind" -o name 2>"$ERRF") + then sr_rc=0; else sr_rc=$?; fi + if [ "$sr_rc" -ne 0 ]; then + printf 'UNVERIFIED: residual %s read failed in %s\n' "$sr_kind" "$sr_ns" + _note_stderr "$ERRF" + if [ "$sr_worst" -lt 2 ]; then sr_worst=2; fi + continue + fi + _note_stderr "$ERRF" + sr_left='' + for sr_id in $sr_out; do + sr_name=${sr_id#*/} + # kube-root-ca.crt is injected into every namespace by the apiserver and + # is never an engineer's leftover. + if [ "$sr_kind" = configmap ] && [ "$sr_name" = kube-root-ca.crt ]; then continue; fi + sr_skip=0 + for sr_k in $sr_keep; do + if [ "$sr_name" = "$sr_k" ]; then sr_skip=1; break; fi + done + if [ "$sr_skip" -eq 0 ]; then sr_left="$sr_left $sr_id"; fi + done + if [ -n "$sr_left" ]; then + printf 'RESIDUAL %s in %s:%s\n' "$sr_kind" "$sr_ns" "$sr_left" + if [ "$sr_worst" -lt 1 ]; then sr_worst=1; fi + else + printf 'CLEAR: no unaccounted %s in %s\n' "$sr_kind" "$sr_ns" + fi + done + return "$sr_worst" +) ``` **Callers do exactly this and nothing more.** The OR-list matters: `verify_teardown …; record $?` terminates the script at the call under `set -e`, so `record` never runs. @@ -389,14 +463,29 @@ A SeiNetwork deleted under `deletionPolicy: Retain` strips the owner reference f Absence of owner references alone is **not** the signal: a follower applied via `seictl node apply` is a top-level object and legitimately has none. The signature is `sei.io/role=validator` **and** no owner references. +An unreadable sweep is not a clean sweep. Piping `kubectl` straight into `jq` hands `jq` no input on a failed read, and `jq` then exits 0 with no output — indistinguishable from "no orphans found". Separate the read from the parse and check both: + ```sh -kubectl get seinode -n eng- -l sei.io/role=validator -o json \ - | jq -r '.items[] +errf=$(mktemp) || { echo 'UNRESOLVED: cannot create temp file'; exit 2; } +if raw=$(kubectl --context harbor get seinode -n eng- \ + -l sei.io/role=validator -o json 2>"$errf") +then :; else + echo 'UNRESOLVED: orphan sweep read failed — this is NOT "no orphans"' + cat "$errf" >&2; rm -f "$errf"; exit 2 +fi +rm -f "$errf" + +if orphans=$(printf '%s' "$raw" | jq -r '.items[] | select((.metadata.ownerReferences // []) | length == 0) - | "\(.metadata.name)\t\(.metadata.labels["sei.io/seinetwork"] // "-")\t\(.status.phase // "-")\t\(.metadata.creationTimestamp)"' + | "\(.metadata.name)\t\(.metadata.labels["sei.io/seinetwork"] // "-")\t\(.status.phase // "-")\t\(.metadata.creationTimestamp)"') +then :; else + echo 'UNRESOLVED: orphan sweep parse failed'; exit 2 +fi + +if [ -z "$orphans" ]; then echo 'no candidate orphans'; else printf '%s\n' "$orphans"; fi # Confirm the parent really is gone before calling one an orphan. -kubectl get seinetwork -n eng- # NotFound → orphaned +kubectl --context harbor get seinetwork -n eng- # NotFound → orphaned ``` An orphaned SeiNode still holds a **`Bound`** PVC. A disk whose PVC has already gone shows up on the AWS side as `available`. The cleanup, the EBS-side check, and the escalation path live in `teardown.md` → *find and clean up already-leaked resources*. diff --git a/.claude/skills/harbor-dev/references/teardown.md b/.claude/skills/harbor-dev/references/teardown.md index 6bf1c122..937bb5a4 100644 --- a/.claude/skills/harbor-dev/references/teardown.md +++ b/.claude/skills/harbor-dev/references/teardown.md @@ -217,7 +217,7 @@ Teardown follows the same PR contract as spinup: render the change, open a PR, l Claim names come from the **pods' own `spec.volumes[].persistentVolumeClaim.claimName`**, not from a guessed naming rule — the controller owns how it names a generated claim, and a rule inferred here would desync the moment it changes. - **A node with no pod resolves to nothing, and that is the leak case, not a nuisance.** The controller reconciles each SeiNode into a StatefulSet (`seinode-crd.md`), so a node whose StatefulSet has no pod — scaled down, unschedulable, evicted — still has its PVC and its EBS volume. The old version of this inventory dropped that node's claim silently and the teardown then verified clean. Check 1 makes the gap executable: the node lands in `unresolved-nodes.txt`, the script exits non-zero, `status` stays `UNRESOLVED`, and the verifier forces `UNVERIFIED`. + **A node with no pod resolves to nothing, and that is the leak case, not a nuisance.** The controller reconciles each SeiNode into a StatefulSet (`seinode-crd.md`), so a node whose StatefulSet has no pod — scaled down, unschedulable, evicted — still has its PVC and its EBS volume. The old version of this inventory dropped that node's claim silently and the teardown then verified clean. Check 1 makes the gap executable: the node lands in `unresolved-nodes.txt`, the script exits non-zero, and **no certificate is written at all** — the `status` file simply does not exist, which `read_inventory` reports as `UNVERIFIED`. The verifier reads that file, and also reads `unresolved-nodes.txt` directly, so either one alone is enough to fail the run. > **Attribution caveat.** A pod is attributed to a node by its **first owner reference's name matching the SeiNode name**. `seinode-crd.md` documents the one-StatefulSet-per-SeiNode shape but not the name the controller gives it, so this is a convention, not a contract. If it does not hold, the node lands in `unresolved-nodes.txt` and the run stops — the failure direction is safe. Confirm with `kubectl get pod -n eng- -o jsonpath='{.metadata.ownerReferences[0].name}'` before assuming an empty `unresolved-nodes.txt` means full coverage. @@ -341,17 +341,34 @@ It does **not** remove: Then discover the chain-ids **with the discovery's own status checked**. Piping `kubectl` into `sort` exits with sort's status, so a `Forbidden` becomes a successful empty list — and "no chains found" then reads as "nothing to do", which is the whole defect class this document exists to close: ```sh - . ./verify-lib.sh # recipe #9 — provides ERRF, _note_stderr, record, VERDICT + . ./verify-lib.sh # recipe #9 — provides record, VERDICT, verify_teardown VERDICT=0 + derr=$(mktemp) || { echo 'UNVERIFIED: cannot create temp file'; exit 2; } + chains='' if raw=$(kubectl --context harbor get seinode -n eng- \ - -o jsonpath='{range .items[*]}{.metadata.labels.sei\.io/seinetwork}{"\n"}{end}' 2>"$ERRF") + -o jsonpath='{range .items[*]}{.metadata.labels.sei\.io/seinetwork}{"\n"}{end}' 2>"$derr") then - chains=$(printf '%s\n' "$raw" | grep -v '^$' | sort -u || true) + # The API read is checked above. The TRANSFORMATIONS need checking too: + # `| sort -u` exits with sort's status and `|| true` swallows everything, + # so a failed grep or sort yields a successful EMPTY chain list — the loop + # then runs zero times and every certificate and claim check is skipped. + # Only grep's exit 1 (nothing matched) is legitimate emptiness. + if filtered=$(printf '%s\n' "$raw" | grep -v '^$'); then fs=0; else fs=$?; fi + if [ "$fs" -gt 1 ]; then + echo 'UNVERIFIED: chain-id filter failed — cannot enumerate this namespace' + record 2 + elif chains=$(printf '%s\n' "$filtered" | sort -u); then + : + else + echo 'UNVERIFIED: chain-id sort failed — cannot enumerate this namespace' + record 2; chains='' + fi else echo 'UNVERIFIED: chain discovery failed — the namespace inventory is unknown' - _note_stderr; record 2; chains='' + cat "$derr" >&2; record 2 fi + rm -f "$derr" ``` Run `inventory.sh` **once per chain-id**, each writing its own `./teardown-inventory-`. A namespace usually holds more than one chain, and a single sweep cannot tell one chain's controller-managed claim from another's. Any chain whose `inventory.sh` exits non-zero writes no certificate, and its verification then reports `UNVERIFIED` — emptying a namespace on an incomplete inventory is how a leak becomes invisible. Claims no chain attributes land in each run's `unattributed-claims.txt`; take those to the leak sweep in step 5, not to a delete. @@ -361,7 +378,7 @@ It does **not** remove: ```sh # Same shell as step 1 — the library is already sourced and $VERDICT already - # carries a 2 if chain discovery failed. + # carries a 2 if discovery or either of its transformations failed. for c in $chains; do rc=0 verify_teardown eng- seinetwork,seinode,pod \ @@ -370,21 +387,44 @@ It does **not** remove: done # Anything left that carries no chain label at all — an escape-hatch apply, - # or an orphan whose labels were stripped. No inventory applies, so `-`. + # or an orphan whose labels were stripped. `-` means NO storage is in scope + # here: this is a CR/pod disappearance check ONLY, and on its own it proves + # nothing about residual claims. rc=0 verify_teardown eng- seinetwork,seinode,pod "" - || rc=$? record "$rc" + ``` + + **Do not print a verdict yet.** The check that can contradict "the namespace is empty" has not run: a namespace whose only leftover is a leaked PVC passes everything above — successful discovery, a clean CR/pod poll, exit 0. +5. **Sweep the residuals, then decide.** Build the expected-survivors list from every chain's imported claims, sweep what is left, and only then print and exit: + + ```sh + # Imported claims are EXPECTED to survive. Concatenating them is what keeps + # this sweep from demanding zero PVCs. + : > ./expected-survivors.txt + for c in $chains; do + f="./teardown-inventory-$c/imported-claims.txt" + if [ -f "$f" ]; then + if ! cat "$f" >> ./expected-survivors.txt; then + echo 'UNVERIFIED: cannot read an imported-claims list'; record 2 + fi + fi + done + + rc=0; sweep_residual eng- ./expected-survivors.txt || rc=$?; record "$rc" case "$VERDICT" in - 0) echo 'NAMESPACE EMPTIED — every chain verified' ;; - 1) echo 'NAMESPACE NOT EMPTY — objects remain in at least one chain' ;; + 0) echo 'NAMESPACE EMPTIED — every chain verified and no unaccounted resources remain' ;; + 1) echo 'NAMESPACE NOT EMPTY — objects or unaccounted resources remain' ;; 2) echo 'NAMESPACE UNVERIFIED — at least one check could not run' ;; + *) echo "NAMESPACE VERIFICATION ABORTED — unexpected status $VERDICT (interrupted?); treat as unverified" ;; esac exit "$VERDICT" ``` - An empty `$chains` after a **successful** discovery is legitimate — the namespace has no labelled chains — and the unlabelled sweep still runs. An empty `$chains` after a **failed** discovery already recorded `2`, so the loop running zero times cannot pass. Do not substitute a namespace-wide PVC poll anywhere here: it matches imported and unattributed claims too, so it reports `PRESENT` after a correct teardown. -5. Sweep for what git never owned — [Find and clean up already-leaked resources](#find-and-clean-up-already-leaked-resources). + Then take anything `sweep_residual` reported, plus what git never owned, to [Find and clean up already-leaked resources](#find-and-clean-up-already-leaked-resources). + + An empty `$chains` after a **successful** discovery is legitimate — the namespace has no labelled chains — and both the unlabelled check and the residual sweep still run. An empty `$chains` after a **failed** discovery, **or after a failed filter or sort**, already recorded `2`, so the loop running zero times cannot pass. Do not substitute a namespace-wide PVC poll for a chain's claim check: that matches imported and unattributed claims too, so it reports `PRESENT` after a correct teardown. `sweep_residual` is the opposite question — what is left over that no chain accounted for — and it excludes the imported claims by name. ### Remove the namespace entirely (offboarding) @@ -402,11 +442,26 @@ Run this after any teardown that ran under `Retain`, and any time an engineer as An orphaned validator has **no `ownerReferences`** and no live parent SeiNetwork. Absence of owner references alone is not the signal: a follower applied through `seictl node apply` is a top-level object and legitimately has none. The signature is `sei.io/role=validator` **and** no owner references. +**An unreadable sweep is not a clean sweep.** Piping `kubectl` straight into `jq` hands `jq` no input when the read fails, and `jq` exits 0 with no output — which reads as "no orphans". Check the read and the parse separately: + ```sh -kubectl --context harbor get seinode -n eng- -l sei.io/role=validator -o json \ - | jq -r '.items[] +errf=$(mktemp) || { echo 'UNRESOLVED: cannot create temp file'; exit 2; } +if raw=$(kubectl --context harbor get seinode -n eng- \ + -l sei.io/role=validator -o json 2>"$errf") +then :; else + echo 'UNRESOLVED: orphan sweep read failed — this is NOT "no orphans"' + cat "$errf" >&2; rm -f "$errf"; exit 2 +fi +rm -f "$errf" + +if orphans=$(printf '%s' "$raw" | jq -r '.items[] | select((.metadata.ownerReferences // []) | length == 0) - | "\(.metadata.name)\t\(.metadata.labels["sei.io/seinetwork"] // "-")\t\(.status.phase // "-")\t\(.metadata.creationTimestamp)"' + | "\(.metadata.name)\t\(.metadata.labels["sei.io/seinetwork"] // "-")\t\(.status.phase // "-")\t\(.metadata.creationTimestamp)"') +then :; else + echo 'UNRESOLVED: orphan sweep parse failed'; exit 2 +fi + +if [ -z "$orphans" ]; then echo 'no candidate orphans'; else printf '%s\n' "$orphans"; fi ``` Each line is a validator still running with nothing that will ever delete it. Confirm the parent is gone before treating one as an orphan: From d0ca2499e76b2cded3a45e19bb5999915b372513 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Wed, 9 Sep 2026 23:18:27 +0000 Subject: [PATCH 17/18] fix(harbor-dev): scope residual exemptions by kind and never reuse a stale survivor list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit sweep_residual compared only the bare name against the survivors list, with no kind check, so an imported PVC named `data` also exempted job/data and configmap/data — all three kinds reported CLEAR at verdict 0. That is the same full-identity defect already fixed in expect_present, in code that should have reused the hardened matching rather than re-inventing it. Every exemption is now kind-scoped and compares the full `/` identity that `-o name` prints, matching the shape the kube-root-ca.crt exemption already had. The namespace step built its survivors list with an unchecked `: > file`. In bash without set -e a failed redirection on that builtin leaves the OLD file intact and execution continues, so a previous run's list could authorise this run's exemptions: stale list readable, zero labelled chains, concat loop runs zero times, and a leaked PVC named `data` is exempted while the namespace reports success. The list is now built in a fresh per-namespace directory with every step checked; on failure the run records UNVERIFIED and passes `-` so no exemptions apply, rather than falling back to whatever was on disk. A missing per-chain imported-claims file is now its own explicit UNVERIFIED branch. Narrow the completion message to the kinds actually swept, and extend the sweep to `service` and `cronjob`: a leftover type=LoadBalancer Service bills with no pod running and the per-tenant ResourceQuota caps load balancers. Platform-owned objects stay out of scope — a workspace PR never owned them. Also add the ABORTED catch-all to the single-chain caller, so an interrupt that exits through the trap does not end nonzero in silence. Verified under dash and bash, with and without set -eu — including bash WITHOUT errexit, where the stale-list defect lives: imported pvc `data` alongside job/data and configmap/data now verdicts 1; a leftover Service verdicts 1; a stale list with an unwritable target verdicts 2 and grants no exemption; zero chains with a fresh empty list leaves a pvc unexempted; a missing imported-claims file verdicts 2; a real imported list still exempts correctly. All 22 prior scenarios still pass. Co-authored-by: omnigent --- .../references/cluster-inspection-recipes.md | 38 ++++++++++--- .../skills/harbor-dev/references/teardown.md | 55 +++++++++++++++---- 2 files changed, 72 insertions(+), 21 deletions(-) diff --git a/.claude/skills/harbor-dev/references/cluster-inspection-recipes.md b/.claude/skills/harbor-dev/references/cluster-inspection-recipes.md index ffcbf0de..67330945 100644 --- a/.claude/skills/harbor-dev/references/cluster-inspection-recipes.md +++ b/.claude/skills/harbor-dev/references/cluster-inspection-recipes.md @@ -381,8 +381,12 @@ verify_teardown() ( # ---- residual sweep: what is LEFT that no teardown accounted for --------- # usage: sweep_residual # Storage is the point: a namespace whose only leftover is a leaked PVC must -# not pass. Imported claims are EXPECTED to survive, so they are excluded BY -# NAME — this never demands zero PersistentVolumeClaims. +# not pass. Imported claims are EXPECTED to survive, so `persistentvolumeclaim` +# entries matching the survivors list are excluded — this never demands zero +# PersistentVolumeClaims. +# Covers persistentvolumeclaim, job, cronjob, service and configmap. It does +# NOT cover every Kind in the namespace, so callers must not report it as +# "nothing remains" — report what was swept. # Returns 0 clear, 1 residual found, 2 a read failed. sweep_residual() ( sr_ns=$1; sr_expect=$2 @@ -403,7 +407,12 @@ sweep_residual() ( esac fi - for sr_kind in persistentvolumeclaim job configmap; do + # Kinds an engineer can leave behind that keep costing money or quota. + # `service` is here because a leftover type=LoadBalancer bills with no pod + # running, and the per-tenant ResourceQuota caps load balancers. Platform-owned + # objects (Namespace, ServiceAccounts, RBAC, the Flux Kustomization) are NOT + # swept — a workspace PR never owned them, so they are not residuals. + for sr_kind in persistentvolumeclaim job cronjob service configmap; do if sr_out=$(kubectl --context harbor -n "$sr_ns" get "$sr_kind" -o name 2>"$ERRF") then sr_rc=0; else sr_rc=$?; fi if [ "$sr_rc" -ne 0 ]; then @@ -415,14 +424,25 @@ sweep_residual() ( _note_stderr "$ERRF" sr_left='' for sr_id in $sr_out; do - sr_name=${sr_id#*/} + # EVERY exemption is scoped to a kind and compared on the FULL identity + # `/` that `-o name` prints. Comparing the bare name lets an + # imported PersistentVolumeClaim called `data` exempt `job/data` and + # `configmap/data` from the sweep — the same defect already fixed in + # expect_present, which this code has to reuse rather than re-invent. + sr_skip=0 + # kube-root-ca.crt is injected into every namespace by the apiserver and # is never an engineer's leftover. - if [ "$sr_kind" = configmap ] && [ "$sr_name" = kube-root-ca.crt ]; then continue; fi - sr_skip=0 - for sr_k in $sr_keep; do - if [ "$sr_name" = "$sr_k" ]; then sr_skip=1; break; fi - done + if [ "$sr_id" = "configmap/kube-root-ca.crt" ]; then sr_skip=1; fi + + # Imported claims are expected survivors — for PersistentVolumeClaims and + # nothing else. A Job or ConfigMap stays a residual whatever it is called. + if [ "$sr_skip" -eq 0 ] && [ "$sr_kind" = persistentvolumeclaim ]; then + for sr_k in $sr_keep; do + if [ "$sr_id" = "persistentvolumeclaim/$sr_k" ]; then sr_skip=1; break; fi + done + fi + if [ "$sr_skip" -eq 0 ]; then sr_left="$sr_left $sr_id"; fi done if [ -n "$sr_left" ]; then diff --git a/.claude/skills/harbor-dev/references/teardown.md b/.claude/skills/harbor-dev/references/teardown.md index 937bb5a4..0e0228bb 100644 --- a/.claude/skills/harbor-dev/references/teardown.md +++ b/.claude/skills/harbor-dev/references/teardown.md @@ -287,6 +287,13 @@ rc=0 verify_teardown eng- seinetwork,seinode,pod \ "sei.io/seinetwork=" ./teardown-inventory- || rc=$? record "$rc" + +# An interrupt exits the function through its trap at 130/143, bypassing its own +# closing message — so say something here rather than exiting nonzero in silence. +case "$VERDICT" in + 0|1|2) : ;; + *) echo "VERIFICATION ABORTED — unexpected status $VERDICT (interrupted?); treat as unverified" ;; +esac exit "$VERDICT" ``` @@ -399,22 +406,42 @@ It does **not** remove: 5. **Sweep the residuals, then decide.** Build the expected-survivors list from every chain's imported claims, sweep what is left, and only then print and exit: ```sh - # Imported claims are EXPECTED to survive. Concatenating them is what keeps - # this sweep from demanding zero PVCs. - : > ./expected-survivors.txt - for c in $chains; do - f="./teardown-inventory-$c/imported-claims.txt" - if [ -f "$f" ]; then - if ! cat "$f" >> ./expected-survivors.txt; then - echo 'UNVERIFIED: cannot read an imported-claims list'; record 2 + # Imported claims are EXPECTED to survive, and this list is what exempts + # them. A STALE list exempts the wrong things, so it is built in a fresh + # per-namespace directory and every step that produces it is checked. + # + # `: > file` is the specific trap. In bash without `set -e` a failed + # redirection on that builtin leaves the OLD file intact and execution + # continues, so a previous run's list would silently authorise this run's + # exemptions. dash aborts instead — which does not make the bash + # configuration this document supports any safer. + SURV_DIR="./teardown-residual-eng-" + surv="$SURV_DIR/expected-survivors.txt" + if rm -rf "$SURV_DIR" && mkdir -p "$SURV_DIR" && : > "$surv"; then + : + else + echo 'UNVERIFIED: cannot create a fresh expected-survivors list' + record 2 + surv='-' # NEVER fall back to a stale list; '-' means "no exemptions" + fi + + if [ "$surv" != '-' ]; then + for c in $chains; do + f="./teardown-inventory-$c/imported-claims.txt" + if [ ! -f "$f" ]; then + printf 'UNVERIFIED: no imported-claims list for chain %s — exemptions incomplete\n' "$c" + record 2 + elif ! cat "$f" >> "$surv"; then + printf 'UNVERIFIED: cannot read the imported-claims list for chain %s\n' "$c" + record 2 fi - fi - done + done + fi - rc=0; sweep_residual eng- ./expected-survivors.txt || rc=$?; record "$rc" + rc=0; sweep_residual eng- "$surv" || rc=$?; record "$rc" case "$VERDICT" in - 0) echo 'NAMESPACE EMPTIED — every chain verified and no unaccounted resources remain' ;; + 0) echo 'NAMESPACE EMPTIED — every chain verified; no unaccounted PVCs, Jobs, CronJobs, Services or ConfigMaps remain' ;; 1) echo 'NAMESPACE NOT EMPTY — objects or unaccounted resources remain' ;; 2) echo 'NAMESPACE UNVERIFIED — at least one check could not run' ;; *) echo "NAMESPACE VERIFICATION ABORTED — unexpected status $VERDICT (interrupted?); treat as unverified" ;; @@ -422,6 +449,10 @@ It does **not** remove: exit "$VERDICT" ``` + With `surv='-'` the sweep runs with **no** exemptions, so genuinely imported claims are reported as residuals. That is the safe direction and it is not the verdict: `record 2` already fired, and `2` dominates the `1` a residual would raise. + + **What the sweep covers, and what it does not.** `sweep_residual` inspects `persistentvolumeclaim`, `job`, `cronjob`, `service` and `configmap` — the engineer-owned kinds that keep costing money or quota, `service` included because a leftover `type: LoadBalancer` bills with no pod running and the per-tenant ResourceQuota caps load balancers. It does **not** enumerate every Kind in the namespace, and it deliberately leaves platform-owned objects alone: the `Namespace`, the three ServiceAccounts, the RBAC, and the Flux `Kustomization` are not residuals, because a workspace PR never owned them. Report what was swept rather than "nothing remains". + Then take anything `sweep_residual` reported, plus what git never owned, to [Find and clean up already-leaked resources](#find-and-clean-up-already-leaked-resources). An empty `$chains` after a **successful** discovery is legitimate — the namespace has no labelled chains — and both the unlabelled check and the residual sweep still run. An empty `$chains` after a **failed** discovery, **or after a failed filter or sort**, already recorded `2`, so the loop running zero times cannot pass. Do not substitute a namespace-wide PVC poll for a chain's claim check: that matches imported and unattributed claims too, so it reports `PRESENT` after a correct teardown. `sweep_residual` is the opposite question — what is left over that no chain accounted for — and it excludes the imported claims by name. From 2218e680b7cf2c5c9d38be37edc0dcfbc6aeb31a Mon Sep 17 00:00:00 2001 From: bdchatham Date: Wed, 9 Sep 2026 23:24:30 +0000 Subject: [PATCH 18/18] docs(harbor-dev): describe residuals as candidates, not confirmed leftovers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sweep filters by Kind, not by ownership: its only ownership-aware exemptions are the imported claims and kube-root-ca.crt, so a platform-owned or controller-created Service, CronJob or ConfigMap is reported too. Saying it "deliberately leaves platform-owned objects alone" claimed more certainty than the implementation delivers — the same species of defect this PR exists to correct. Residuals are now candidates requiring ownership resolution, and the unswept kinds are described as unswept rather than as recognised and excluded. Caveat the ownership evidence for Service and CronJob: the platform base listing neither is what the repository declares, not what the cluster holds. Add the ABORTED catch-all to the recipe's single-chain caller, matching both callers in teardown.md. Record one line for the next editor beside the shared helpers: reuse the matching and status handling next door rather than re-deriving it. That was this file's recurring defect, not any single bug. No behaviour change — the verification library is byte-identical to the reviewed revision. Co-authored-by: omnigent --- .../harbor-dev/references/cluster-inspection-recipes.md | 9 +++++++++ .claude/skills/harbor-dev/references/teardown.md | 6 +++++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/.claude/skills/harbor-dev/references/cluster-inspection-recipes.md b/.claude/skills/harbor-dev/references/cluster-inspection-recipes.md index 67330945..3adccf46 100644 --- a/.claude/skills/harbor-dev/references/cluster-inspection-recipes.md +++ b/.claude/skills/harbor-dev/references/cluster-inspection-recipes.md @@ -185,6 +185,8 @@ Written for a portable shell (`dash`, `ash`, `bash`). Three deliberate non-POSIX 2. **Identities are matched, not counted.** A count says how many lines came back, not whether the resources you asked about are the ones that came back. 3. **Every command that can fail runs inside a condition.** Under `set -e` a bare `out=$(kubectl …)` terminates the shell at the assignment — before classification, and before the caller records anything. +**Adding a helper here? Reuse the matching and status handling beside it rather than re-deriving them.** Through this file's review history the recurring defect was never one bug — it was new code re-deriving logic already hardened next door, and arriving without the fix. `sweep_residual` was written comparing bare names in the same review round that `expect_present` was corrected to compare full identities, one function away. + ```sh # ============ harbor teardown verification library ======================== # Source this, then call verify_teardown / sweep_residual. Do not copy pieces. @@ -464,6 +466,13 @@ rc=0 verify_teardown eng- seinetwork,seinode,pod \ "sei.io/seinetwork=" ./teardown-inventory- || rc=$? record "$rc" + +# An interrupt exits the function through its trap at 130/143, bypassing its own +# closing message — so say something here rather than exiting nonzero in silence. +case "$VERDICT" in + 0|1|2) : ;; + *) echo "VERIFICATION ABORTED — unexpected status $VERDICT (interrupted?); treat as unverified" ;; +esac exit "$VERDICT" ``` diff --git a/.claude/skills/harbor-dev/references/teardown.md b/.claude/skills/harbor-dev/references/teardown.md index 0e0228bb..3f072f73 100644 --- a/.claude/skills/harbor-dev/references/teardown.md +++ b/.claude/skills/harbor-dev/references/teardown.md @@ -451,7 +451,11 @@ It does **not** remove: With `surv='-'` the sweep runs with **no** exemptions, so genuinely imported claims are reported as residuals. That is the safe direction and it is not the verdict: `record 2` already fired, and `2` dominates the `1` a residual would raise. - **What the sweep covers, and what it does not.** `sweep_residual` inspects `persistentvolumeclaim`, `job`, `cronjob`, `service` and `configmap` — the engineer-owned kinds that keep costing money or quota, `service` included because a leftover `type: LoadBalancer` bills with no pod running and the per-tenant ResourceQuota caps load balancers. It does **not** enumerate every Kind in the namespace, and it deliberately leaves platform-owned objects alone: the `Namespace`, the three ServiceAccounts, the RBAC, and the Flux `Kustomization` are not residuals, because a workspace PR never owned them. Report what was swept rather than "nothing remains". + **What the sweep covers, and what it does not.** `sweep_residual` inspects `persistentvolumeclaim`, `job`, `cronjob`, `service` and `configmap` — kinds that keep costing money or quota, `service` included because a leftover `type: LoadBalancer` bills with no pod running and the per-tenant ResourceQuota caps load balancers. It does **not** enumerate every Kind in the namespace. Report what was swept rather than "nothing remains". + + **What it reports are candidates, not confirmed leftovers.** The sweep filters by **Kind**, not by ownership. Its only ownership-aware exemptions are the imported claims and `kube-root-ca.crt`; anything else of a swept Kind is listed whoever created it, so a platform-owned or controller-created `Service`, `CronJob` or `ConfigMap` **will** appear. Kinds outside the list — the `Namespace`, the ServiceAccounts, the RBAC, the Flux `Kustomization` — are not swept, which is not the same as being recognised and excluded. Resolve ownership before acting on any line it prints; the escalation rule in [find and clean up already-leaked resources](#find-and-clean-up-already-leaked-resources) already governs that, and nothing here authorizes a deletion. + + > **Ownership evidence is repository-level, not live.** The claim that `Service` and `CronJob` are engineer-owned in `eng-` rests on the platform base listing neither (`onboarding-pr.md` → *Base layer (already in place)*). That is what the repo declares, not what the cluster holds — a controller or an add-on can create either without appearing there. Confirm against the live namespace before treating a swept `Service` or `CronJob` as an engineer's leftover. Then take anything `sweep_residual` reported, plus what git never owned, to [Find and clean up already-leaked resources](#find-and-clean-up-already-leaked-resources).