Skip to content

cpln-advisor 2.0.0 — deploy into an existing GVC - #523

Merged
jacobecox merged 5 commits into
mainfrom
claude/advisor-gvc
Aug 30, 2026
Merged

cpln-advisor 2.0.0 — deploy into an existing GVC#523
jacobecox merged 5 commits into
mainfrom
claude/advisor-gvc

Conversation

@jacobecox

Copy link
Copy Markdown
Contributor

Review diff: git diff da57950..HEAD -- cpln-advisor/versions/2.0.0/
(da57950 is the verbatim cp -R of 1.0.0.)

Scope: the GVC conversion and nothing else. This is a new template someone else owns, so everything the round found that is not GVC-related was deliberately reverted and is proposed separately — see the bottom.

Why this one matters more than the other ten

1.0.0's gvc.yaml renders name: {{ .Values.global.cpln.gvc }}it creates a GVC named after the one you install into. So a 1.0.0 release installed into an existing GVC has adopted it, and helm uninstall then deletes that GVC and every unrelated workload in it. That is what destroyed test-gvc on 2026-08-07, observed even with a helm.sh/resource-policy: keep annotation. The other ten created a separately-named GVC; this one takes over yours.

The gate passed. 1.0.0 installed into a throwaway chart-created GVC (never a sanctioned slot); the in-place upgrade onto 2.0.0 with 1.0.0's real values was refused at render, and the GVC survived with version 1 before / 1 after and a byte-identical spec — nothing was touched.

The conversion

gvc.yaml deleted, createsGvc: false, gvc.locations (a list) → top-level location (a name). Values no longer declare global.cpln.gvc (which also failed lint R9).

Three-layer defence, all three exercised on real infrastructure:

  1. Render-time fail on the legacy gvc key, verified against the real 1.0.0/values.yaml, not just --set.
  2. minScale/maxScale: 0 + complete localOptions on all five workloads. This layer carries more weight here than anywhere else: 1.0.0 enforced "exactly one location" by owning the GVC, and its own comment explains that a second location means a second scheduler firing every cron twice and a second writer on the database. Verified — all five tiers report This workload location is deactivated because maxScale is set to 0 in an added location.
  3. Boot GVC read against the real $CPLN_ENDPOINT with a proving 403 control. The backend image has no curl and no wget, so the read goes through python3's urllib, which speaks HTTP/1.1 — confirmed on the wire, because redis-ml shipped HTTP/1.0 and silently fail-opened behind istio-envoy's 426.

Functionally equivalent to 1.0.0 apart from placement and the boot guards; the postgres subchart shows 0 spec differences.

One knob change, and it is caused by the conversion

livenessProbe.failureThreshold 3 → 5 on the API. The boot GVC read is bounded at 22 s worst case (measured in this image against a blackholed address and an accept-never-respond server) and alembic upgrade head runs after it. At 3 the container is restarted 80 s after start, which the conversion itself can now exceed.

The firewall self-inclusion helper also stays: 1.0.0 had internalAccess, but its GVC was dedicated, so same-gvc meant "this chart's own workloads". Now the GVC is the user's and may hold anything, so that is a direct consequence of no longer owning it.

The maintainer's Postgres question, answered by measurement

The bundled postgres 3.4.1 has no location knob and renders no localOptions, so it cannot be pinned. Measured in a two-location GVC: the extra database is genuinely independent (8 tables vs 0) on its own billed EBS volume — but the service DNS is strictly location-local, 10/10 each way.

So a current install stays correct and merely pays for a dead database. The deferred risk is real though: changing location later silently repoints the app at the empty one. The chart's boot warning is accurate in every claim. Closing it properly needs a postgres release with a location knob.

Testing

16 PASS. Both guard arms behave as specified; placement confined; the boot read works with its 403 control; the backup restore was executed end to end (real 90 KB dump, 8 tables, database dropped, restored, application working on the restored data).

Found by this round, NOT fixed here — proposed separately

  • The advisor cannot scan any workload built with this conversion's idiom. It reads maxScale: 0 as "suspended" and ignores the localOptions entry supplying the real count, so all five of its own tiers are skipped with scan: skipping … — suspended while running and ready. Proved by changing one variable: patching a tier from maxScale: 0 to 1 flipped it from skipped to scored 83. That idiom is what all eleven conversions adopted, so the tool is currently blind to itself and to the converted catalog. This is an advisor-backend fix; no chart-side workaround exists that does not reopen the placement defect this PR closes.
  • render-vs-stored is 16, not 0. Seven are this chart's (missing readinessProbe.successThreshold and probe timeoutSeconds) and nine belong to the vendored postgres 3.4.1, which needs a subchart release. The chart-side seven were fixed and then reverted to keep this PR GVC-only.
  • The documented restore succeeds by luck. postgres:18's pg_dumpall emits \restrict, which older psql rejects — printing invalid command \restrict eleven times and exiting 0. It worked because those lines were not load-bearing. Wants -v ON_ERROR_STOP=1 and a psql ≥ 18 requirement.
  • postgres.internalAccess: same-gvc got wider — 1.0.0's GVC was dedicated, now it is the user's, so anything in it can open 5432. The README carries the workload-list recipe.
  • Lint's pre-existing findings are unchanged by design: 4× R7 WARN and 4× R13 FAIL, all present identically on 1.0.0.

Untested

The values-less upgrade hole (destructive by construction; confirmed at render that 2.0.0 emits no kind: gvc), Slack and LLM integrations (no keys), autopilot apply/revert, appUrl, session.rememberDays, logLevel: DEBUG, web.replicas.min: 0 scale-to-zero, and the gcp/minio backup providers.

🤖 Generated with Claude Code

jacobecox and others added 5 commits August 30, 2026 14:11
1.0.0 rendered `kind: gvc` with `name: {{ .Values.global.cpln.gvc }}` — the GVC
it was installed INTO — so Helm ADOPTED the user's GVC. An in-place upgrade onto
a version that stops declaring it makes Helm prune it, taking every workload,
volumeset and identity inside. 2.0.0 removes the GVC resource and ships the
three-layer defence:

  1. A render-time `fail` on the legacy `gvc` values key (tested against the real
     1.0.0 values file, not only --set).
  2. defaultOptions.minScale/maxScale 0 plus a complete localOptions entry on all
     five workloads, so an undeclared GVC location starts nothing. `gvc.locations`
     became the single top-level `location`.
  3. A boot-time GVC read in the API's startup script, scoped by a new
     `-gvc-policy` (view on that one GVC), plus an inline FATAL location guard on
     api/worker/scheduler/redis. The GVC-derived check only warns — every tier is
     stateless, so there is no fresh-vs-initialised discriminator; severity splits
     by cost as grafana-multi-location 2.0.0 does.

The bundled `postgres` 3.4.1 is the one tier that cannot be pinned: it has no
`location` knob and a subchart's values cannot be templated. Documented in the
README, values and briefing; the API warns about extra GVC locations at boot.

Also: internal firewall links now come from one helper instead of hand-written
strings; worker/redis `resources` renamed to bare `cpu`/`memory` (lint R13);
`scheme: HTTP` and liveness `successThreshold: 1` declared (R7); complete
options/firewall blocks so a default install does not drift from creation.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…command

Two test-round findings.

**Seven render-vs-stored differences were this chart's.** It already declared
`scheme` and the liveness `successThreshold` with the comment "API backfill,
declared so rendered == stored" -- and missed `readinessProbe.successThreshold`
and `timeoutSeconds` on every probe. Now every probe on every tier carries all
of them; verified against the render, 0 incomplete.

(The round's other NINE differences belong to the vendored postgres 3.4.1 and
need a subchart release. Not fixable here.)

**The documented restore succeeded by luck.** It was executed end to end -- real
90 KB dump with 8 tables, database dropped, restored, application working on the
restored data -- but `postgres:18`'s `pg_dumpall` emits `\restrict` directives
that older clients reject, and `psql` printed `invalid command \restrict` eleven
times and **exited 0**. It worked only because those lines were not load-bearing.
The same silent-error path would hide a real failure just as completely.

The command now carries `-v ON_ERROR_STOP=1` and the README requires psql >= 18,
explaining why. The "has not been executed against this template" caveat is
replaced with what was actually measured.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Maintainer ruling: cpln-advisor is a brand-new template someone else owns, so
2.0.0 should carry the GVC conversion and nothing else. The probe backfill
declarations and the psql >= 18 / ON_ERROR_STOP restore correction are real
findings and both are proposed separately -- they are just not this change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Maintainer ruling: this is a brand-new template someone else owns, so 2.0.0
carries the GVC conversion and nothing else, to ship quickly.

Reverted as out of scope:
- `worker.resources` / `redis.resources` renamed from maxCpu/maxMemory to bare
  cpu/memory (lint R13). A values-key rename is breaking for anyone already on
  1.0.0 and has nothing to do with the GVC.
- `scheme: HTTP` and `livenessProbe.successThreshold: 1` declarations (lint R7).
  Drift fixes, and pre-existing -- 1.0.0 carries the identical findings.

KEPT, because the conversion causes it: `livenessProbe.failureThreshold` 3 -> 5
on the API. The boot-time GVC read is bounded at 22s worst case and `alembic
upgrade head` runs after it, so at 3 the container is restarted 80s after start,
which the conversion itself can now exceed.

The firewall helper also stays. 1.0.0 already had `internalAccess` and an
inboundAllowWorkload list, but its GVC was DEDICATED -- `same-gvc` meant "this
chart's own workloads". After the conversion the GVC is the user's and may hold
anything, so the self-inclusion helper is a direct consequence of no longer
owning the GVC rather than an unrelated cleanup.

Lint now reports 1.0.0's pre-existing findings unchanged (4x R7 WARN, 4x R13
FAIL), which is the intended result of a scope-limited change; R9 and R16 stay
fixed because both are GVC-specific. All of it is proposed separately.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@jacobecox
jacobecox merged commit f215b85 into main Aug 30, 2026
6 checks passed
@jacobecox
jacobecox deleted the claude/advisor-gvc branch August 30, 2026 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant