diff --git a/docs/plans-executions/2026-09-16-0841-provider-blueprint.md b/docs/plans-executions/2026-09-16-0841-provider-blueprint.md index b5c3589..b092194 100644 --- a/docs/plans-executions/2026-09-16-0841-provider-blueprint.md +++ b/docs/plans-executions/2026-09-16-0841-provider-blueprint.md @@ -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 1 — Collect evidence (read-only, in parallel) - [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 @@ -61,10 +61,40 @@ Judgment calls: planned. The contract rules in §2 cite the reconciler and GC lines each rule protects. - 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 - async errors reach status), as well as code gaps. Both would mislead the next + log keys, gcp-vm-validation.md's label value and its claim that async errors + reach status), as well as code gaps. Both would mislead the next provider author in the same way. - "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 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. diff --git a/docs/providers.md b/docs/providers.md index 49f7c68..3821318 100644 --- a/docs/providers.md +++ b/docs/providers.md @@ -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 | |---|---|---|---| | `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 | -| `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: @@ -240,10 +240,10 @@ Pitfalls: ## 5. Optional interfaces: `Checker` -`provider.Checker` is `Check(ctx) []TargetStatus` (`check.go:37-46`). The -monitor type-asserts it on the **unwrapped** provider, before `cmd/main.go` adds -the decorators (`cmd/main.go:211-222`). A provider without it shows up as -unsupported, not as failed. +`provider.Checker` is `Check(ctx) []TargetStatus` (`check.go:37-46`). +`cmd/main.go` type-asserts it on the **unwrapped** provider (`cmd/main.go:216`) +before adding the decorators (`:219-222`). The monitor treats a nil Checker as +unsupported, not as failed (`monitor.go:212`). - **MUST never fail as a whole.** Every failure is a `TargetStatus` with `Ready=false`. @@ -358,16 +358,18 @@ the per-provider notes below the table. failures. Nothing is Quota. - **Check:** `check.go:41-139`. Runs concurrently per host through `hostPool.probe`. Reasons are tagged by the dial stage. -- **Connections:** `conn.go`. Dials lazily, one connection per host with a 15s - keepalive. On cancel, `call` closes the shared connection; `probe` abandons +- **Connections:** `conn.go`. One connection per host with a 15s keepalive. + 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. - **Full guide:** [kvm-provider.md](kvm-provider.md). ## 7. Adding a provider: checklist 1. **Package:** create `internal/provider//` with the provider type and a - compile-time `var _ provider.Provider = (*Provider)(nil)` (and - `provider.Checker`). + compile-time interface assertions: `var _ provider.Checker = (*Provider)(nil)` + 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 `libvirtAPI`) so a fake can stand in for it in tests. - 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 and cleaned up. See `kvm/kvm_integration_test.go` and [testing.md](testing.md). -10. **RBAC:** if the provider talks to the cluster, add package-level - `+kubebuilder:rbac` markers, then run `make manifests` and check +10. **RBAC:** if the provider talks to the cluster, add `+kubebuilder:rbac` + 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`. 11. **Docs:** - A `docs/-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. - **Doc drift in kvm-provider.md:** - 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 lack them.