kvm2: apply the milestone review
Shorter start wait with immediate verdicts for dead or paused domains, ListByTag without state or address RPCs, Timeout for an expired check round, no adoption of unmarked domains, reference-counted keyed locks, dead options removed, HEAD fallback on any non-2xx. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
Claude Fable 5.1
@@ -172,7 +172,8 @@ closes the SSH connection to unblock it.
|
||||
|
||||
kvm2 provider (internal/provider/kvm2/) - the same transport, a second type built from the contract
|
||||
prov.Create ──► placement checked against the catalogue (Permanent before any dial)
|
||||
DomainLookupByName: exists → uid guard (foreign uid = Permanent), ─┐ every step
|
||||
DomainLookupByName: exists → mark+uid guard (no mark or foreign ─┐ every step
|
||||
uid = Permanent; paused = Permanent), │
|
||||
start if needed, wait for running, done │ idempotent,
|
||||
else: StoragePoolGetInfo precheck: image (if uncached) + diskSizeGb │ under a
|
||||
+ 16 MiB must fit, else ErrQuotaExceeded (reconciler │ per-name lock
|
||||
@@ -184,7 +185,7 @@ prov.Create ──► placement checked against the catalogue (Permanent before
|
||||
seed ISO (always rebuilt) │
|
||||
DomainDefineXML (<metadata> in kvm2's own namespace: │
|
||||
managed, uid, createdAt) + DomainCreate, poll until │
|
||||
running (2m cap) ─┘
|
||||
running (15s cap; a domain that dies at once = Transient) ─┘
|
||||
prov.Get ──► DomainLookupByName + DomainGetMetadata + DomainGetState
|
||||
+ DomainInterfaceAddresses (guest agent, then ARP; first routable IPv4)
|
||||
running without IP = Provisioning; nostate = Provisioning;
|
||||
|
||||
+52
-24
@@ -41,9 +41,12 @@ ID.
|
||||
**Answer:** `name-is-key`. libvirt addresses domains by name
|
||||
(`DomainLookupByName`); `req.Name` is used verbatim as the domain name and as
|
||||
the base of the per-VM volume names. Create looks the name up first and adopts
|
||||
an existing domain only if its metadata `uid` is empty or equals `req.UID`; a
|
||||
different non-empty `uid` returns `ErrPermanent` naming both UIDs. This closes
|
||||
the contract's "adoption is not verified" limit for this provider.
|
||||
an existing domain only if it carries the kvm2 mark (`managed` true, entry 8)
|
||||
and its metadata `uid` is empty or equals `req.UID`. A domain without the
|
||||
mark returns `ErrPermanent` saying it was not created by this provider: kvm2
|
||||
never starts or takes over a VM someone else defined under that name. A
|
||||
different non-empty `uid` returns `ErrPermanent` naming both UIDs. This
|
||||
closes the contract's "adoption is not verified" limit for this provider.
|
||||
|
||||
**Evidence:** `DomainLookupByName(Name string) (Domain, error)` in the
|
||||
go-libvirt module; contract §2.1 idempotency and §13.
|
||||
@@ -201,15 +204,24 @@ libvirt has no locks or tasks; every RPC is synchronous.
|
||||
**Answer:** `create-return`: `blocking`. `async-ops`: `none`. Create returns
|
||||
when the domain is running, holding one reconcile worker for the duration,
|
||||
including the first image download on a host (minutes on a cold pool; the
|
||||
guide says so). The wait after `DomainCreate` is capped at 2 minutes; every
|
||||
RPC has its own deadline (entry 18).
|
||||
guide says so). The wait after `DomainCreate` is capped at 15 seconds:
|
||||
`DomainCreate` is synchronous in libvirt, so the first `DomainGetState`
|
||||
normally answers running and the cap only covers state settling. Two rules
|
||||
keep the wait from pinning a worker: a state after the start that is neither
|
||||
active nor `nostate` (shutoff, crashed, paused, pmsuspended, shutdown) is
|
||||
`ErrTransient` at once, since polling will not bring that guest up; and a
|
||||
domain found paused or pmsuspended before the start is `ErrPermanent`
|
||||
(`cannot be started by the operator; resume or destroy it on the host`), an
|
||||
admin's doing that `DomainCreate` would refuse and the reconciler must not
|
||||
retry. Every RPC has its own deadline (entry 18).
|
||||
|
||||
Steps, each idempotent, in order, all under the per-name lock (entry 19):
|
||||
|
||||
1. Validate placement (entry 3). No backend call yet.
|
||||
2. `DomainLookupByName`. Found: adoption guard (entry 2), then start it if it
|
||||
is not running (`DomainCreate`; `ErrOperationInvalid` means already
|
||||
running), return the ID.
|
||||
is not running (paused or pmsuspended is `ErrPermanent`; `DomainCreate`;
|
||||
`ErrOperationInvalid` means already running), poll as in step 9, return
|
||||
the ID.
|
||||
3. Pool space precheck (entry 13, quota).
|
||||
4. Ensure the base image (entry 4): marker present and matching means done.
|
||||
5. Overlay volume `<name>.qcow2`, qcow2 backed by the base file with
|
||||
@@ -220,7 +232,9 @@ Steps, each idempotent, in order, all under the per-name lock (entry 19):
|
||||
7. `StorageVolGetPath` for the overlay and the seed.
|
||||
8. `DomainDefineXML` with file-type disks (entry 15's AppArmor note), the
|
||||
metadata element, firmware per image.
|
||||
9. `DomainCreate`, then poll `DomainGetState` until running.
|
||||
9. `DomainCreate`, then poll `DomainGetState` until running: `nostate` is
|
||||
polled again, any other inactive state is `ErrTransient` at once, and the
|
||||
15-second cap is `ErrTransient` too.
|
||||
10. Return `hosts/<host>/domains/<name>`.
|
||||
|
||||
A retry after a crash finds whatever the previous attempt got to: an existing
|
||||
@@ -267,13 +281,16 @@ contract §2.3.
|
||||
**Answer:** `fail-whole`, per host, with single unreadable domains skipped.
|
||||
Scope: every host in `hosts`, in config order, `ConnectListAllDomains` with
|
||||
active and inactive domains. For each domain, read the metadata element; keep
|
||||
those with `managed` true, then read state and addresses for the `Instance`.
|
||||
A domain whose metadata or state cannot be read (mid teardown, corrupt
|
||||
element) is skipped with a V(1) line; a host that cannot be dialled or whose
|
||||
listing fails fails the whole call, so GC skips this provider for the sweep
|
||||
rather than act on a partial view. Filtering is client-side; libvirt has no
|
||||
label index. Cost: one `DomainGetMetadata` per domain per sweep, about half a
|
||||
millisecond each over the tunnel.
|
||||
those with `managed` true and build the `Instance` from the domain name and
|
||||
the element alone: `ID`, `UID` and `CreatedAt`, which is all GC reads
|
||||
(contract §2.4). `State` and `IP` are left empty; no state or address RPC is
|
||||
made. A domain whose metadata cannot be read (mid teardown, corrupt element)
|
||||
is skipped with a V(1) line, unless ctx ended, which fails the call; a host
|
||||
that cannot be dialled or whose listing fails fails the whole call, so GC
|
||||
skips this provider for the sweep rather than act on a partial view.
|
||||
Filtering is client-side; libvirt has no label index. Cost: exactly one
|
||||
`DomainGetMetadata` per domain per sweep, about half a millisecond each over
|
||||
the tunnel.
|
||||
|
||||
**Evidence:** execution log run 6 (per-domain metadata cost); contract §2.4
|
||||
and §4.7.
|
||||
@@ -390,9 +407,11 @@ pooled connection, so a check that runs out of time closes only its own
|
||||
connection and cannot disrupt a reconcile. Cost: one SSH handshake and four
|
||||
cheap RPCs per host per round.
|
||||
|
||||
Reasons: `Reachable`; `Timeout` (ctx ended before the check finished);
|
||||
`Unreachable` (TCP connect or the SSH transport failed, or the link died
|
||||
mid-check); `Unauthorized` (SSH authentication refused, host key unknown or
|
||||
Reasons: `Reachable`; `Timeout` (the round's ctx ended before the check
|
||||
finished, whichever stage was running, the dial included, or an RPC's own
|
||||
budget ran out once the connection was up); `Unreachable` (TCP connect or the
|
||||
SSH transport failed, including on the dial's own 10 s budget while the round
|
||||
still had time, or the link died mid-check); `Unauthorized` (SSH authentication refused, host key unknown or
|
||||
changed); `Forbidden` (`ErrAccessDenied` or `ErrAuthFailed` from libvirt: the
|
||||
socket refused the user); `APIError` (any other libvirt error);
|
||||
`CredentialsUnavailable` (the key or known_hosts file is missing or
|
||||
@@ -458,7 +477,7 @@ goroutine and waits on the result, `ctx.Done()` and a timer:
|
||||
| `DomainCreate` | 90 s |
|
||||
| `StorageVolUpload` (seed) | 60 s |
|
||||
| `StorageVolUpload` (image), HTTP download | ctx, capped at 20 m |
|
||||
| wait for running after `DomainCreate` | 2 m |
|
||||
| wait for running after `DomainCreate` | 15 s (entry 10) |
|
||||
| HTTP client for images | no client timeout; the per-request ctx carries the cap |
|
||||
|
||||
When a call is abandoned (ctx cancelled or budget exceeded), the adapter
|
||||
@@ -479,10 +498,14 @@ every 15 s) marks the connection dead on failure.
|
||||
**Answer:** shared state is the host connection table (one pooled connection
|
||||
per host, dialled lazily, under a mutex with a per-host dial mutex so
|
||||
concurrent callers dial once and share the result), the per-`(host, volume)`
|
||||
download locks, and a keyed mutex per instance name. Create and Delete for the
|
||||
same name run under that name's lock, so two Deletes (GC and the reconciler)
|
||||
never race on destroy and undefine, and a Delete cannot interleave with a
|
||||
Create's steps. Get and ListByTag take no name lock. go-libvirt multiplexes
|
||||
download locks, and a lock per instance name. Both lock sets are one instance
|
||||
each of `keyedLocks`: a one-slot semaphore per key, so a waiter gives up when
|
||||
its ctx ends, reference-counted so an entry is removed from the map once no
|
||||
caller holds or waits for it and the map does not grow with every name ever
|
||||
seen, while two waiters on one key always share one semaphore. Create and
|
||||
Delete for the same name run under that name's lock, so two Deletes (GC and
|
||||
the reconciler) never race on destroy and undefine, and a Delete cannot
|
||||
interleave with a Create's steps. Get and ListByTag take no name lock. go-libvirt multiplexes
|
||||
RPCs on one connection, so calls for different names proceed concurrently
|
||||
on the shared connection. Three reconciles, a GC sweep and a check round at
|
||||
once are safe; the check uses its own connections.
|
||||
@@ -598,7 +621,7 @@ entry after the user confirms.
|
||||
|
||||
## 24. Known limits accepted at design time
|
||||
|
||||
**Answer:** eight limits, each with its workaround:
|
||||
**Answer:** nine limits, each with its workaround:
|
||||
|
||||
- **Create blocks**, including the first image download per host, holding one
|
||||
of three reconcile workers. Workaround: warm the cache with one Proxy per
|
||||
@@ -615,6 +638,11 @@ entry after the user confirms.
|
||||
isolated on its own connection.
|
||||
- **`Content-Length` is required** for image downloads; a chunked mirror is
|
||||
rejected as Permanent.
|
||||
- **A paused or pmsuspended domain is not started.** A repeat `Create` for
|
||||
a domain in that state is `ErrPermanent` (entry 10), so the Proxy latches
|
||||
as Failed until its spec changes, rather than retrying a `DomainCreate`
|
||||
that libvirt refuses. Resume the domain on the host (`virsh resume`) and
|
||||
touch the spec, or delete the Proxy.
|
||||
- **No v1 log-format compatibility**: the `libvirt call` lines have the same
|
||||
keys where the same facts exist, but dashboards keyed on `kvm` logger names
|
||||
need `kvm2`.
|
||||
|
||||
+27
-20
@@ -235,10 +235,12 @@ Its `<metadata>` carries one element in kvm2's own namespace,
|
||||
|
||||
That is how orphan GC recognises its own VMs, since libvirt has no
|
||||
labels, and how a repeat `Create` knows whether an existing domain of the
|
||||
same name belongs to this Proxy: a different non-empty `uid` is a
|
||||
permanent error naming both UIDs; an empty one is adopted. The namespace
|
||||
is kvm2's own, so the `kvm` provider's domains are invisible to it (see
|
||||
*Migrating from kvm*).
|
||||
same name belongs to this Proxy: a domain without the element is a
|
||||
permanent error (`carries no kvm2 mark; it was not created by this
|
||||
provider`), a different non-empty `uid` is a permanent error naming both
|
||||
UIDs, and a marked domain with a matching or empty `uid` is adopted. The
|
||||
namespace is kvm2's own, so the `kvm` provider's domains are invisible to
|
||||
it (see *Migrating from kvm*).
|
||||
|
||||
The seed (`buildSeedISO` in `seed.go`) is an ISO 9660 image labelled
|
||||
`cidata` with three files. `user-data` is the Proxy's `cloudInit`, **byte
|
||||
@@ -321,8 +323,10 @@ Before creating any volume, `Create` reads `StoragePoolGetInfo` and
|
||||
compares the pool's free space with what it is about to commit
|
||||
(`checkPoolSpace` and `neededBytes` in `image.go`): the image's
|
||||
`Content-Length` when the base is not cached yet (found with a `HEAD`
|
||||
request, or a `GET` whose body is closed at once when the server refuses
|
||||
`HEAD`), plus `diskSizeGb` in bytes for the overlay, plus 16 MiB for the
|
||||
request, or a `GET` whose body is closed at once when `HEAD` answers
|
||||
anything but 2xx, since mirrors refuse it with 405, 403 or 501 alike; the
|
||||
`GET`'s status is then what counts), plus `diskSizeGb` in bytes for the
|
||||
overlay, plus 16 MiB for the
|
||||
seed. Below that it returns `ErrQuotaExceeded` with a message naming the
|
||||
pool, the bytes available and the bytes needed. The reconciler answers
|
||||
quota with a wait, not a failure: the Proxy shows `Provisioned=False`
|
||||
@@ -347,9 +351,12 @@ by changing `type: kvm` to `type: kvm2` and `kvm:` to `kvm2:`. The
|
||||
|
||||
What does not port is the VMs. kvm2 marks its domains in its own metadata
|
||||
namespace and neither lists nor adopts domains the `kvm` provider made:
|
||||
`ListByTag` skips them, so orphan GC never sees them, and a Proxy's
|
||||
`spec.provider` is immutable, so an existing Proxy cannot be moved to the
|
||||
new entry anyway. Do it in this order:
|
||||
`ListByTag` skips them, so orphan GC never sees them, and `Create` refuses
|
||||
a domain of the same name that lacks the kvm2 mark with a permanent error
|
||||
rather than start it (a Proxy recreated under the new entry gets the same
|
||||
deterministic name, so this is what happens when step 1 below is skipped).
|
||||
A Proxy's `spec.provider` is immutable, so an existing Proxy cannot be
|
||||
moved to the new entry anyway. Do it in this order:
|
||||
|
||||
1. Delete the Proxies on the `kvm` entry and wait for them to go; the
|
||||
`kvm` provider removes their domains and volumes.
|
||||
@@ -437,7 +444,7 @@ applied by `call`):
|
||||
| `DomainCreate` | 90 s |
|
||||
| `StorageVolUpload` of at most 16 MiB (the seed, the marker) | 60 s |
|
||||
| `StorageVolUpload` of an image, `StorageVolDownload`, and the whole image fetch from HTTP request to marker | 20 m; the caller's ctx may end it earlier |
|
||||
| wait for running after `DomainCreate` | 2 m, polled every 500 ms (`defaultRunningWait` and `defaultRunningPoll` in `create.go`) |
|
||||
| wait for running after `DomainCreate` | 15 s, polled every 500 ms (`defaultRunningWait` and `defaultRunningPoll` in `create.go`); `DomainCreate` is synchronous, so the first poll normally answers running, and a domain that is shut off, crashed or paused right after the start fails at once rather than waiting out the cap |
|
||||
| the `HEAD` request of the pool-space precheck | 30 s |
|
||||
| the courtesy `ConnectClose` when a connection is closed | 5 s |
|
||||
|
||||
@@ -499,10 +506,10 @@ A failing target says where the check broke (`checkReason` in `check.go`):
|
||||
|---|---|---|
|
||||
| `Reachable` | ready | |
|
||||
| `CredentialsUnavailable` | the key or known_hosts file is missing or does not parse | the `kvm-ssh` Secret (see *Cluster-side setup*); fixed in the cluster, not on the host |
|
||||
| `Unreachable` | the TCP connect failed or ran out of the 10 s dial budget, the SSH transport failed for a reason other than authentication, the forwarded libvirt socket was refused, or the link died mid-check | DNS, Tailscale or a firewall between the cluster and `address`; `libvirtd` running and the `socket` path |
|
||||
| `Unreachable` | the TCP connect failed or ran out of the 10 s dial budget while the round still had time, the SSH transport failed for a reason other than authentication, the forwarded libvirt socket was refused, or the link died mid-check | DNS, Tailscale or a firewall between the cluster and `address`; `libvirtd` running and the `socket` path |
|
||||
| `Unauthorized` | the SSH server rejected the key, or the host key is missing from known_hosts or has changed | `user`'s `authorized_keys`; the known_hosts entry (re-run `ssh-keyscan` and update the Secret) |
|
||||
| `Forbidden` | SSH worked, but libvirt refused the user (`ErrAccessDenied` or `ErrAuthFailed`) | `user` in the `libvirt` group |
|
||||
| `Timeout` | the connection was up, but the monitor's `--provider-check-timeout` (default 15s) or an RPC's budget ran out before the check finished | a hung `libvirtd` or a saturated link |
|
||||
| `Timeout` | the monitor's `--provider-check-timeout` (default 15s) ran out before the check finished, whichever stage was running, the dial included; or the connection was up and an RPC's own budget ran out | a hung `libvirtd`, a saturated link, or a round timeout shorter than the dial takes |
|
||||
| `PoolNotFound` | there is no pool named `pool` | `virsh pool-list --all` |
|
||||
| `PoolNotActive` | the pool exists but is not running | `virsh pool-start <pool>` and `virsh pool-autostart <pool>` |
|
||||
| `APIError` | any other libvirt error | the message carries libvirt's own text |
|
||||
@@ -518,10 +525,10 @@ waits 5 minutes on `ErrQuotaExceeded`, recreates on `ErrNotFound` from
|
||||
|
||||
| Class | What lands there |
|
||||
|---|---|
|
||||
| `ErrPermanent` | placement values not in the catalogue; a malformed providerID; a host no longer in the config (from `Get`; `Delete` treats it as done); libvirt `ErrXMLError`, `ErrNoSupport`, `ErrInvalidArg`, `ErrAuthFailed`, `ErrAccessDenied` and `ErrNoStoragePool`; a pool that is not running; SSH authentication refused (`unable to authenticate`); a host key missing from known_hosts or changed (`knownhosts.KeyError`); a key or known_hosts file that exists but does not parse; a domain of the same name owned by another UID; corrupt metadata on a domain; an image URL answering 4xx, a response without `Content-Length` or with length 0, a sha256 that does not match the configured one |
|
||||
| `ErrPermanent` | placement values not in the catalogue; a malformed providerID; a host no longer in the config (from `Get`; `Delete` treats it as done); libvirt `ErrXMLError`, `ErrNoSupport`, `ErrInvalidArg`, `ErrAuthFailed`, `ErrAccessDenied` and `ErrNoStoragePool`; a pool that is not running; SSH authentication refused (`unable to authenticate`); a host key missing from known_hosts or changed (`knownhosts.KeyError`); a key or known_hosts file that exists but does not parse; a domain of the same name owned by another UID or carrying no kvm2 mark; a domain of the same name that is paused or pmsuspended; corrupt metadata on a domain; an image URL answering 4xx, a response without `Content-Length` or with length 0, a sha256 that does not match the configured one |
|
||||
| `ErrQuotaExceeded` | the pool-space precheck |
|
||||
| `ErrNotFound` | libvirt `ErrNoDomain` or `ErrNoStorageVol` from `Get`, `Delete` or `ListByTag`; inside `Create` the same codes are `ErrTransient`, because the contract forbids NotFound from Create |
|
||||
| `ErrTransient` | everything else: a missing key or known_hosts file (the Secret has not landed yet), TCP and network errors, context cancellation, an abandoned or budget-exceeded call, a connection killed by another caller, libvirt `ErrOperationInvalid` and every unrecognised libvirt code, an image URL answering 429 or 5xx or ending the body early, the wait for running expiring |
|
||||
| `ErrTransient` | everything else: a missing key or known_hosts file (the Secret has not landed yet), TCP and network errors, context cancellation, an abandoned or budget-exceeded call, a connection killed by another caller, libvirt `ErrOperationInvalid` and every unrecognised libvirt code, an image URL answering 429 or 5xx or ending the body early, the wait for running expiring or the domain going shut off or crashed right after its start |
|
||||
|
||||
Error strings name paths, hosts, volumes and libvirt's own text. They
|
||||
never contain key bytes, the known_hosts content or cloud-init.
|
||||
@@ -556,12 +563,12 @@ never contain key bytes, the known_hosts content or cloud-init.
|
||||
- **`Content-Length` is required** for image downloads; a mirror that
|
||||
only chunks is rejected permanently. Point the catalogue at one that
|
||||
sends the length.
|
||||
- **A paused or pmsuspended domain cannot be adopted.** A repeat `Create`
|
||||
for a domain in that state calls `DomainCreate`, which libvirt refuses
|
||||
as "already running"; the provider then waits for the domain to report
|
||||
running, the 2-minute wait expires, and the call fails transiently on
|
||||
every retry. Resume the domain on the host (`virsh resume <name>`) or
|
||||
delete the Proxy.
|
||||
- **A paused or pmsuspended domain is not started.** A repeat `Create`
|
||||
for a domain in that state fails permanently (`is DomainPaused and
|
||||
cannot be started by the operator; resume or destroy it on the host`),
|
||||
so the Proxy shows `Failed` until its spec changes; the operator never
|
||||
resumes a VM an admin paused. Resume it on the host
|
||||
(`virsh resume <name>`) and touch the spec, or delete the Proxy.
|
||||
- **Parser text in credential errors.** A key or known_hosts file that
|
||||
does not parse fails with the parser's own message, which can quote a
|
||||
PEM header type or a malformed known_hosts token. Key bytes are never
|
||||
|
||||
@@ -12,7 +12,7 @@ Plan: [docs/plans/2026-09-16-1333-kvm2-provider.md](../plans/2026-09-16-1333-kvm
|
||||
- [x] Step 7 — Checker and the CRD comment
|
||||
- [x] Step 8 — Deployment files, Makefile, integration test
|
||||
- [x] Step 9 — Documentation
|
||||
- [ ] Step 10 — Milestone review
|
||||
- [x] Step 10 — Milestone review
|
||||
- [ ] Step 11 — Real-host validation, changelog, MR
|
||||
|
||||
## Step 0 — Plan file and branch
|
||||
@@ -338,3 +338,45 @@ resolves. The one added em-dash is the `Leader-elected` cell of the new
|
||||
components-table row, where the table's own convention wins.
|
||||
|
||||
Attestation: "files read outside the allowed set: none".
|
||||
|
||||
## Step 10 — Milestone review
|
||||
|
||||
2026-09-16 21:19. The `go-operator-reviewer` agent reviewed the package
|
||||
against the contract and the spec (v1 code off limits to it as well) and
|
||||
returned seven findings, all accepted and applied by a fresh fix agent in
|
||||
one commit with regression tests:
|
||||
|
||||
1. `waitRunning`'s two-minute cap only ever elapsed in the bad cases and
|
||||
pinned a reconcile worker; now 15 s, a domain that dies right after
|
||||
start is Transient at once, and a paused or pmsuspended domain is
|
||||
Permanent before `DomainCreate` (an admin's doing).
|
||||
2. `ListByTag` read state and addresses that GC never uses, one agent RPC
|
||||
per active domain per sweep; it now builds instances from the name and
|
||||
the metadata only (`TestListByTag_doesNotReadStateOrAddresses`).
|
||||
3. A health round that ran out of time was blamed on the host
|
||||
(`Unreachable`); `checkHost` now reports `Timeout` whenever the round's
|
||||
ctx ended, whichever dial stage was running, and the promptness test
|
||||
uses the production error shape.
|
||||
4. `Create` adopted an unmarked domain and never wrote the mark, so GC could
|
||||
not see it; an unmarked domain under the name is now Permanent
|
||||
(`TestCreate_adoptsOnlyMarkedDomains`).
|
||||
5. `hostTable.closeAll` had no production caller and `withBudgets` could not
|
||||
work (the dialer captured the defaults first); both removed.
|
||||
6. The per-name and per-image lock maps grew without bound; replaced by a
|
||||
reference-counted `keyedLocks` whose entries vanish when uncontended.
|
||||
7. `imageSizeIfUncached` falls back to GET on any non-2xx HEAD, and two
|
||||
Delete no-op lines moved from V(0) to V(1).
|
||||
|
||||
The spec (entries 2, 10, 12, 16, 18, 19, 24), the guide, the appendix and
|
||||
the architecture flow were updated in the same commit. The reviewer also
|
||||
listed what it checked and found correct: the error boundary, the state
|
||||
rules, Delete's idempotency and the replacement window, ListByTag's
|
||||
partial-failure policy, the abandon policy and double-close guards,
|
||||
resource handling on every path, and secret hygiene in the format strings.
|
||||
|
||||
Lint: `bin/golangci-lint` still panics on this toolchain (Go 1.27 against
|
||||
a staticcheck built for 1.26), reproducible on untouched packages, so
|
||||
`make lint` could not gate this step; `go vet` with every build tag is
|
||||
clean. Tests: `go test -race -count=3 ./internal/provider/kvm2/` (168
|
||||
top-level tests) and `go test -short ./...` pass; `make test` with envtest
|
||||
runs next.
|
||||
|
||||
+33
-25
@@ -30,12 +30,12 @@ and the symbol that shows it.
|
||||
| Axis | kubernetes | gcp | kvm | kvm2 |
|
||||
|---|---|---|---|---|
|
||||
| `readiness` | immediate: a Pod is scheduled within seconds (`stateFromPod`) | provisioning-window: minutes of boot (`mapState`) | provisioning-window: boot plus, on first use, an image download (`imageCache.ensure`) | provisioning-window: boot plus, on first use, an image download (`ensureImage`) |
|
||||
| `create-return` | submit: returns after `client.Create` | submit: `Insert` is never awaited, `Operation.Wait` is not called | **blocking**: waits through `DomainCreate` (`ensureRunning`), and the first image fetch runs inside (`imageCache.fetch`) | **blocking**: waits through `DomainCreate` and a 2-minute poll for running (`ensureRunning`, `waitRunning`), and the first image fetch runs inside (`fetchImage`) |
|
||||
| `create-return` | submit: returns after `client.Create` | submit: `Insert` is never awaited, `Operation.Wait` is not called | **blocking**: waits through `DomainCreate` (`ensureRunning`), and the first image fetch runs inside (`imageCache.fetch`) | **blocking**: waits through `DomainCreate` and a 15-second poll for running (`ensureRunning`, `waitRunning`), and the first image fetch runs inside (`fetchImage`) |
|
||||
| `delete-return` | returns-early: `client.Delete`, Pod terminates on its own | returns-early: `Delete` submits an operation each time | waits: destroy, undefine and volume removal complete before return (`removeDomain`, `removeVolumes`) | waits: destroy, undefine and volume removal complete before return (`removeDomain`, `removeVolumes`) |
|
||||
| `ip-source` | assigned-at-create: `PodIP` (`instanceFromPod`) | assigned-at-create: the external NAT IP of the first NIC (`toInstance`) | discovered: ARP table first, then the guest agent, first routable IPv4 (`address`, `firstIPv4`) | discovered: the guest agent first, then the ARP table, first routable IPv4 (`addressSources`, `address`, `firstIPv4`) |
|
||||
| `gc-mark` | native-labels: Pod labels (`buildPod`) | native-labels: instance labels (`buildInsertRequest`) | written-metadata: a namespaced `<metadata>` element written in `DomainDefineXML` (`metadata.go`) | written-metadata: an element in kvm2's own namespace written in `DomainDefineXML` (`metadataURI`, `newInstanceMetadata`) |
|
||||
| `created-at` | backend-timestamp: `CreationTimestamp` | backend-timestamp: `CreationTimestamp`, parse error gives zero | written-at-create: `createdAt` in the metadata element (`newInstanceMetadata`) | written-at-create: `createdAt` in the metadata element, parse error gives zero (`newInstanceMetadata`, `createdAt`) |
|
||||
| `list-partial` | n/a: one call, all namespaces | omit-unreachable: `ReturnPartialSuccess` on `AggregatedList` | fail-whole per host, one unreadable domain skipped (`ListByTag`, `managedInstance`) | fail-whole per host, one unreadable domain skipped unless ctx ended (`ListByTag`) |
|
||||
| `list-partial` | n/a: one call, all namespaces | omit-unreachable: `ReturnPartialSuccess` on `AggregatedList` | fail-whole per host, one unreadable domain skipped (`ListByTag`, `managedInstance`) | fail-whole per host, one unreadable domain skipped unless ctx ended; `ID`, `UID` and `CreatedAt` from the name and the mark only, no state or address RPC (`ListByTag`) |
|
||||
| `quota` | absent: a quota 403 is Permanent (`classify`) | detected: 429, and 403 with reason `quotaExceeded` or `rateLimitExceeded` (`classify`, `hasReason`) | absent (`classify`) | detected: a pool-space precheck before any volume is created (`checkPoolSpace`, `neededBytes`) |
|
||||
| `not-found` | status-404: `apierrors.IsNotFound` | status-404: `isNotFound` | error-code: `ErrNoDomain`, `ErrNoStorageVol` (`hasCode`) | error-code: `ErrNoDomain`, `ErrNoStorageVol`, Transient inside Create (`classifyLibvirt`) |
|
||||
| `primary-key` | name-is-key: `namespace/name` | name-is-key: `zones/<zone>/instances/<name>` | name-is-key: `hosts/<host>/domains/<name>` | name-is-key: `hosts/<host>/domains/<name>`, with a UID guard on adoption (`create`) |
|
||||
@@ -56,7 +56,7 @@ against.
|
||||
|
||||
| Feature | kubernetes | gcp | kvm | kvm2 |
|
||||
|---|---|---|---|---|
|
||||
| Create verifies the adopted instance's UID | no | no | no | **yes**: a different non-empty UID in the metadata is Permanent, an empty one is adopted (`create`) |
|
||||
| Create verifies the adopted instance's UID | no | no | no | **yes**: a domain without the kvm2 mark or with a different non-empty UID is Permanent; a marked one with a matching or empty UID is adopted (`create`) |
|
||||
| Placement used | none | Zone, MachineType, Image | Zone (host), MachineType, Image | Zone (host), MachineType, Image |
|
||||
| CloudInit delivered | **no**: ignored | yes: `user-data` metadata | yes: NoCloud seed ISO built in-process (`buildSeedISO`) | yes: NoCloud seed ISO built in-process (`buildSeedISO`) |
|
||||
| Port used | yes: Squid config and containerPort (`squidConf`) | no: firewall is manual | no | no |
|
||||
@@ -372,9 +372,10 @@ same libvirt.
|
||||
logs the resolved parameters with `cloudInitBytes` at V(2). Then, under
|
||||
the per-name lock (`lockName`) and on the pooled connection
|
||||
(`hostTable.get`), `create`: `DomainLookupByName`; found means the
|
||||
adoption guard (a non-empty metadata UID that differs from `req.UID` is
|
||||
Permanent) and `ensureRunning`. Otherwise `StoragePoolLookupByName`,
|
||||
`imageSizeIfUncached` (marker lookup, then `HEAD` or a closed `GET`) and
|
||||
adoption guard (no kvm2 mark, or a non-empty metadata UID that differs
|
||||
from `req.UID`, is Permanent) and `ensureRunning`. Otherwise
|
||||
`StoragePoolLookupByName`, `imageSizeIfUncached` (marker lookup, then
|
||||
`HEAD`, or a closed `GET` when `HEAD` is not 2xx) and
|
||||
`checkPoolSpace` with `neededBytes` (image if uncached, plus
|
||||
`diskSizeGb`, plus 16 MiB), then `ensureImage` (`cachedImage` on the
|
||||
pooled connection, `fetchImage` streaming into `uploadImage` on a
|
||||
@@ -382,10 +383,11 @@ same libvirt.
|
||||
`ensureOverlay` (`ErrStorageVolExist` means reuse), `writeSeed` (delete,
|
||||
build, upload; always rebuilt), two `StorageVolGetPath`, `buildDomainXML`
|
||||
with file-type disks and the metadata element, `DomainDefineXML`,
|
||||
`ensureRunning` (`DomainGetState`; `DomainCreate` with
|
||||
`ErrOperationInvalid` as already running; `waitRunning` polls every
|
||||
500 ms for up to 2 minutes, Transient on expiry). `Create` returns only
|
||||
when the domain is running.
|
||||
`ensureRunning` (`DomainGetState`, where paused or pmsuspended is
|
||||
Permanent; `DomainCreate` with `ErrOperationInvalid` as already running;
|
||||
`waitRunning` polls every 500 ms for up to 15 seconds, Transient on
|
||||
expiry and at once for any inactive state other than `nostate`).
|
||||
`Create` returns only when the domain is running.
|
||||
- **Get and state.** `mapState(state, ip)`:
|
||||
|
||||
| libvirt state | IP | Provider state |
|
||||
@@ -398,8 +400,8 @@ same libvirt.
|
||||
`get`: `DomainLookupByName` (only `ErrNoDomain` reaches NotFound),
|
||||
`readMetadata` (`DomainGetMetadata` with `metadataURI`;
|
||||
`ErrNoDomainMetadata` means unmanaged with an empty UID; an element that
|
||||
does not parse is Permanent), `inspect` (`DomainGetState`, then for an
|
||||
active domain `address`). `addressSources` asks the guest agent first,
|
||||
does not parse is Permanent), then `DomainGetState` and, for an active
|
||||
domain, `address`. `addressSources` asks the guest agent first,
|
||||
then ARP; a source that fails is skipped at V(2), only ctx ending is an
|
||||
error, so a cancelled Get is Transient and not a false Provisioning.
|
||||
`firstIPv4` drops loopback, link-local and unspecified addresses and an
|
||||
@@ -413,9 +415,12 @@ same libvirt.
|
||||
when nil is returned.
|
||||
- **ListByTag.** `ConnectListAllDomains` (active and inactive) on every
|
||||
host in config order; `readMetadata` and `managed` keep kvm2's own
|
||||
domains; `inspect` fills state and address. A domain whose metadata or
|
||||
state cannot be read is skipped at V(1), unless ctx ended, which fails
|
||||
the call. A host that cannot be dialled or listed fails the whole call.
|
||||
domains, and the `Instance` is `ID`, `UID` and `CreatedAt` from the name
|
||||
and the element, with `State` and `IP` empty: GC reads nothing else
|
||||
(contract §2.4), so no state or address RPC is made. A domain whose
|
||||
metadata cannot be read is skipped at V(1), unless ctx ended, which
|
||||
fails the call. A host that cannot be dialled or listed fails the whole
|
||||
call.
|
||||
- **Errors.** `classify(op, err)`: an error already carrying a class keeps
|
||||
it; libvirt codes through `classifyLibvirt` (`ErrNoDomain` and
|
||||
`ErrNoStorageVol` NotFound, Transient when `op == "create"`;
|
||||
@@ -436,14 +441,16 @@ same libvirt.
|
||||
`ConnectGetHostname`, `StoragePoolLookupByName`, `StoragePoolGetInfo`;
|
||||
a pool that is not running is `poolNotActiveError`. Details are filled
|
||||
as facts arrive (`user` and `pool` always; `libvirtVersion`, `hostname`,
|
||||
`poolState`, `poolCapacityGiB`, `poolAvailableGiB`). `checkReason`: a
|
||||
`dialError` is judged by its stage (`credentials` gives
|
||||
`CredentialsUnavailable`; `ssh` with a `knownhosts.KeyError` or the auth
|
||||
text gives `Unauthorized`; any other stage, including a dial that ran
|
||||
out of time, gives `Unreachable`); then `context.DeadlineExceeded` or
|
||||
`Canceled` `Timeout`, `ErrAccessDenied` or `ErrAuthFailed` `Forbidden`,
|
||||
`ErrNoStoragePool` `PoolNotFound`, any other libvirt code `APIError`,
|
||||
anything else `Unreachable`; `PoolNotActive` from the pool state.
|
||||
`poolState`, `poolCapacityGiB`, `poolAvailableGiB`). `checkHost` answers
|
||||
`Timeout` whenever the round's ctx ended, whatever stage was running and
|
||||
whatever shape the error has; otherwise `checkReason`: a `dialError` is
|
||||
judged by its stage (`credentials` gives `CredentialsUnavailable`; `ssh`
|
||||
with a `knownhosts.KeyError` or the auth text gives `Unauthorized`; any
|
||||
other stage, including a dial that ran out of its own budget, gives
|
||||
`Unreachable`); then `context.DeadlineExceeded` or `Canceled` `Timeout`,
|
||||
`ErrAccessDenied` or `ErrAuthFailed` `Forbidden`, `ErrNoStoragePool`
|
||||
`PoolNotFound`, any other libvirt code `APIError`, anything else
|
||||
`Unreachable`; `PoolNotActive` from the pool state.
|
||||
- **Connections.** `hostTable` keeps one pooled connection per host,
|
||||
dialled lazily under a per-host one-slot semaphore so waiting ends with
|
||||
the caller's ctx; a dead backend (`liveness`) is closed and redialled on
|
||||
@@ -510,8 +517,9 @@ same libvirt.
|
||||
- Cancelling one operation cuts off the others on that host once: a
|
||||
cancelled `call` kills the pooled connection.
|
||||
- `Content-Length` is required for image downloads.
|
||||
- A paused or pmsuspended domain cannot be adopted by a repeat `Create`:
|
||||
`DomainCreate` refuses, `waitRunning` expires, Transient on every retry.
|
||||
- A paused or pmsuspended domain is not started by a repeat `Create`:
|
||||
`ensureRunning` returns Permanent, so the Proxy latches until its spec
|
||||
changes; the admin resumes the VM on the host.
|
||||
- Credential parse errors carry the parser's own text, which can quote a
|
||||
PEM header type or a malformed known_hosts token (never key bytes).
|
||||
- No reused-overlay check: an existing overlay is reused without checking
|
||||
|
||||
Reference in New Issue
Block a user