Correct the provider blueprint after verification

A re-check against the code found four mismatches: the kvm connection is
opened at startup by the leader's first health check, so that isn't doc
drift; cmd/main.go, not the monitor, type-asserts Checker; QuotaExceeded
and PermanentError are reasons on Provisioned, not conditions; and the
latch line range. The checklist also now matches how the existing
providers do interface assertions and RBAC markers.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
2026-09-16 08:57:52 +00:00
co-authored by Claude Opus 5
parent d96e56f1a0
commit e08f23bcb3
2 changed files with 50 additions and 16 deletions
@@ -4,7 +4,7 @@ Plan: [docs/plans/2026-09-16-0841-provider-blueprint.md](../plans/2026-09-16-084
- [x] Step 0 — Save the plan; wait for the merge - [x] Step 0 — Save the plan; wait for the merge
- [x] Step 1 — Collect evidence (read-only, in parallel) - [x] Step 1 — Collect evidence (read-only, in parallel)
- [x] Step 2 — Write `docs/providers.md` - [x] Step 2 — Write `docs/providers.md`
- [ ] Step 3 — Verify, then open the MR - [x] Step 3 — Verify, then open the MR
## Step 0 — Save the plan; wait for the merge ## Step 0 — Save the plan; wait for the merge
@@ -61,10 +61,40 @@ Judgment calls:
planned. The contract rules in §2 cite the reconciler and GC lines each rule planned. The contract rules in §2 cite the reconciler and GC lines each rule
protects. protects.
- The gap list includes documentation drift (kvm-provider.md's ARP order and - The gap list includes documentation drift (kvm-provider.md's ARP order and
"from startup" dial, gcp-vm-validation.md's label value and its claim that log keys, gcp-vm-validation.md's label value and its claim that async errors
async errors reach status), as well as code gaps. Both would mislead the next reach status), as well as code gaps. Both would mislead the next
provider author in the same way. provider author in the same way.
- "Proposed features" stays a flat list with no design, as the user decided. - "Proposed features" stays a flat list with no design, as the user decided.
Worth noting: the line numbers in the doc are a snapshot. The doc says so and Worth noting: the line numbers in the doc are a snapshot. The doc says so and
asks readers to look for the named function when a line has moved. asks readers to look for the named function when a line has moved.
## Step 3 — Verify, then open the MR
An Explore agent re-checked the doc against the code: about 150 claims,
including every matrix cell. It found four mismatches, and all four were fixed:
- **Startup dial:** I had listed kvm-provider.md's "connection kept open from
startup" as doc drift. It isn't: the pool dials lazily, but the provider
health monitor runs a check as soon as the leader starts
(`monitor.go:147`, default interval 1m), and that check opens the connection.
The bullet was removed and the kvm notes now say this.
- **Checker wording:** §5 said the monitor type-asserts `Checker`. It is
`cmd/main.go:216`; the monitor only sees nil or not.
- **Reasons, not conditions:** §3 called `QuotaExceeded` and `PermanentError`
conditions. They are reasons on `Provisioned`.
- **Latch lines:** the line range is `:133-139`, not `:130-135`.
It also noted that none of the providers assert `var _ provider.Provider`, and
that the kubernetes provider's RBAC markers live in the controller package. The
§7 checklist now says both.
Link check (every relative `](…)` target and anchor in the new and edited docs
resolves), then the sanity build:
```bash
go build ./... && go test -short ./...
# all packages ok
```
Worth noting: no CHANGELOG entry yet. Per CLAUDE.md it goes in after the user
confirms the doc.
+17 -13
View File
@@ -200,9 +200,9 @@ results with the class (`metrics.go:61-74`, `tracing.go:67-80`).
| Class | Use it for | During normal reconciling | While deleting | | Class | Use it for | During normal reconciling | While deleting |
|---|---|---|---| |---|---|---|---|
| `ErrNotFound` | the instance doesn't exist | Get: recreate or advance (§2); other ops: returned as the error, so exponential backoff | Get: release the finalizer | | `ErrNotFound` | the instance doesn't exist | Get: recreate or advance (§2); other ops: returned as the error, so exponential backoff | Get: release the finalizer |
| `ErrQuotaExceeded` | quota or rate limit | condition `QuotaExceeded`, requeue after `QuotaRetry` (5m), not counted as an error (`:337-340`) | requeue after `QuotaRetry` | | `ErrQuotaExceeded` | quota or rate limit | `Provisioned=False` with reason `QuotaExceeded`, requeue after `QuotaRetry` (5m), not counted as an error (`:337-340`) | requeue after `QuotaRetry` |
| `ErrTransient` | network, 5xx, timeouts, cancellation, anything unknown | error returned, so workqueue exponential backoff | same | | `ErrTransient` | network, 5xx, timeouts, cancellation, anything unknown | error returned, so workqueue exponential backoff | same |
| `ErrPermanent` | bad config, bad argument, auth or permission denied, malformed ID | **latched**: condition `PermanentError`, no more provider calls until `metadata.generation` changes (`:130-135`, `:341-343`) | error returned and retried forever; Get Permanent also triggers `Delete` (`:292-302`) | | `ErrPermanent` | bad config, bad argument, auth or permission denied, malformed ID | **latched**: `Provisioned=False` with reason `PermanentError`, no more provider calls until `metadata.generation` changes (`:133-139`, `:341-343`) | error returned and retried forever; Get Permanent also triggers `Delete` (`:292-302`) |
Pitfalls: Pitfalls:
@@ -240,10 +240,10 @@ Pitfalls:
## 5. Optional interfaces: `Checker` ## 5. Optional interfaces: `Checker`
`provider.Checker` is `Check(ctx) []TargetStatus` (`check.go:37-46`). The `provider.Checker` is `Check(ctx) []TargetStatus` (`check.go:37-46`).
monitor type-asserts it on the **unwrapped** provider, before `cmd/main.go` adds `cmd/main.go` type-asserts it on the **unwrapped** provider (`cmd/main.go:216`)
the decorators (`cmd/main.go:211-222`). A provider without it shows up as before adding the decorators (`:219-222`). The monitor treats a nil Checker as
unsupported, not as failed. unsupported, not as failed (`monitor.go:212`).
- **MUST never fail as a whole.** Every failure is a `TargetStatus` with - **MUST never fail as a whole.** Every failure is a `TargetStatus` with
`Ready=false`. `Ready=false`.
@@ -358,16 +358,18 @@ the per-provider notes below the table.
failures. Nothing is Quota. failures. Nothing is Quota.
- **Check:** `check.go:41-139`. Runs concurrently per host through - **Check:** `check.go:41-139`. Runs concurrently per host through
`hostPool.probe`. Reasons are tagged by the dial stage. `hostPool.probe`. Reasons are tagged by the dial stage.
- **Connections:** `conn.go`. Dials lazily, one connection per host with a 15s - **Connections:** `conn.go`. One connection per host with a 15s keepalive.
keepalive. On cancel, `call` closes the shared connection; `probe` abandons The pool dials on first use; with provider checks on (the default), the
leader's first health check opens it at startup. On cancel, `call` closes the shared connection; `probe` abandons
the RPC and leaves the connection open. the RPC and leaves the connection open.
- **Full guide:** [kvm-provider.md](kvm-provider.md). - **Full guide:** [kvm-provider.md](kvm-provider.md).
## 7. Adding a provider: checklist ## 7. Adding a provider: checklist
1. **Package:** create `internal/provider/<type>/` with the provider type and a 1. **Package:** create `internal/provider/<type>/` with the provider type and a
compile-time `var _ provider.Provider = (*Provider)(nil)` (and compile-time interface assertions: `var _ provider.Checker = (*Provider)(nil)`
`provider.Checker`). as in `kubernetes/check.go:15`, and the same for `provider.Provider`, which
the existing providers don't assert yet.
- Put the backend SDK behind a small interface (gcp `instancesAPI`, kvm - Put the backend SDK behind a small interface (gcp `instancesAPI`, kvm
`libvirtAPI`) so a fake can stand in for it in tests. `libvirtAPI`) so a fake can stand in for it in tests.
- Build the logger with - Build the logger with
@@ -413,8 +415,11 @@ the per-provider notes below the table.
again, poll until Running, list, delete, delete again, and confirm it is gone again, poll until Running, list, delete, delete again, and confirm it is gone
and cleaned up. See `kvm/kvm_integration_test.go` and and cleaned up. See `kvm/kvm_integration_test.go` and
[testing.md](testing.md). [testing.md](testing.md).
10. **RBAC:** if the provider talks to the cluster, add package-level 10. **RBAC:** if the provider talks to the cluster, add `+kubebuilder:rbac`
`+kubebuilder:rbac` markers, then run `make manifests` and check markers in their own comment block, not inside a doc comment. The kubernetes
provider's Pod markers live next to the reconciler's
(`internal/controller/proxy_controller.go:86-88`); `controller-gen` scans
`./...`, so either place works. Then run `make manifests` and check
`git diff config/rbac/role.yaml`. `git diff config/rbac/role.yaml`.
11. **Docs:** 11. **Docs:**
- A `docs/<type>-provider.md` covering prerequisites, config reference, - A `docs/<type>-provider.md` covering prerequisites, config reference,
@@ -497,7 +502,6 @@ These are listed only. Nothing below is designed or implemented.
its backing image or size. its backing image or size.
- **Doc drift in kvm-provider.md:** - **Doc drift in kvm-provider.md:**
- It says ARP is the fallback; the code tries ARP first. - It says ARP is the fallback; the code tries ARP first.
- It describes a connection kept open "from startup"; the dial is lazy.
- It says `host` and `name` are on every log line; the check and list paths - It says `host` and `name` are on every log line; the check and list paths
lack them. lack them.