Pool image/version rollout: rolling replacement of warm VMs - #85
richardcase wants to merge 7 commits into
Conversation
Add PoolSpec.template_hash (12) and rollout_policy (13, new RolloutPolicy message), VMRecord.template_hash (9), PoolStatus.stale_count (5), and EventType values VM_DELETED_FOR_ROLLOUT (11), POOL_ROLLOUT_STARTED (12), POOL_ROLLOUT_COMPLETED (13). Regenerated via buf generate. Fields are additive and unused; no other Go code changes.
CreatePool/UpdatePool now compute PoolSpec.template_hash server-side from microvm_template (protojson.Marshal is used instead of proto.Marshal because the template contains map fields, and protojson sorts map keys for deterministic output). UpdatePool ignores any client-supplied template_hash and emits POOL_ROLLOUT_STARTED when the hash changes and the resulting stale VM count is nonzero. GetPool/ListPools report PoolStatus.stale_count by comparing each VMRecord's template_hash against the pool's current one, and Provisioner.Provision copies the pool's current template_hash onto newly created VMRecords. Also add the template_hash column to the pools/vms tables and thread it through the sqlite store's convert/query code, since neither existed before this change and the feature can't round-trip without it.
Add store.UpdateVMPhaseIfCurrent and reconciler.EnsureVMDeletedIfPhase so RolloutController's deletion path is guarded against a VM being concurrently claimed between its ListVMsByPool snapshot and the delete call, mirroring ClaimAvailableVM's own guarded-UPDATE pattern. Also clamp resolveBatchSize to the pool's size, honor an explicit rollout_policy.count of 0 as an intentional pause instead of coercing it to 1, and document the known event-mislabeling limitation when Sweeper.retryPendingDeletions finishes a previously-failed rollout deletion.
…I plumbing pool get/list table output now includes a STALE column sourced from PoolStatus.stale_count (JSON output already covered it automatically). rollout_policy needed no new CLI plumbing: pool create/update already pass an arbitrary PoolSpec loaded from --spec-file straight through, and RolloutPolicy is just another field on that generated message.
CreatePool/UpdatePool passed RolloutPolicy through in-memory, but the
SQLite schema/row mapping (extended in a prior change for
template_hash) never gained a column for it, so GetPool/ListPools and
any poolmgrd restart reloading pools from the store silently dropped
it, reverting to the reconciler's default batch size of 1 with no
error.
Store it as protojson in a nullable TEXT column so an unset policy
round-trips as nil rather than an empty RolloutPolicy{}, matching the
reconciler's unset-vs-explicit-zero distinction in resolveBatchSize.
Count current-hash VMs still PROVISIONING/CREATE_HOOK_RUNNING (a rollout's own in-progress replacements) as unavailable load alongside in-flight DELETING VMs when computing RolloutController's per-tick budget, so max_unavailable actually bounds pool unavailability instead of only accounting for VMs still mid-deletion. Add rollout test coverage for the notifier-driven backfill path (the "replacement" half of rolling replacement), and add a newRolloutController override with a fake in poolmanager tests, mirroring newReconciler/withFakeReconciler, so tests that pass a nil store no longer risk a real RolloutController.Tick panicking on it.
richardcase
left a comment
richardcase
left a comment
There was a problem hiding this comment.
Found three correctness issues in the rollout and upgrade paths. The existing tests in internal/{reconciler,store,api,poolmanager,poolmgrctl} pass; an additional two-tick budget reproduction fails, and applying the new schema over the main-branch schema reproduces the missing-column error.
| hook_failure_policy INTEGER NOT NULL, | ||
| heartbeat_interval_ns INTEGER NOT NULL, | ||
| heartbeat_expiry_threshold_ns INTEGER NOT NULL, | ||
| template_hash TEXT NOT NULL DEFAULT '', |
There was a problem hiding this comment.
[P1] Migrate existing databases before querying the new columns
store.Open only executes this schema, and CREATE TABLE IF NOT EXISTS does not add columns to existing tables. Opening a database created by the current main branch therefore leaves both tables unchanged, while all pool/VM queries now require the new columns. Applying the two schemas in sequence reproduces no such column: template_hash; daemon startup's pool loading and API operations consequently fail after an upgrade. Please add an idempotent migration for pools.template_hash, pools.rollout_policy, and vms.template_hash, with an upgrade test using the old schema.
| if err != nil { | ||
| continue // left DELETING; retried by Sweeper.retryPendingDeletions or a later Tick | ||
| } | ||
| FinishVMDeletion(ctx, c.store, c.pool, vm, c.notifier, c.metrics, poolmgrv1alpha1.EventType_VM_DELETED_FOR_ROLLOUT) |
There was a problem hiding this comment.
[P1] Provision rollout replacements independently of the lease strategy
This notification does not guarantee a replacement: immediateOnLease.OnVMDeleted and DesiredNewVMs both return zero. Updating the template of a warm IMMEDIATE_ON_LEASE pool therefore deletes its available VMs over successive ticks without provisioning any new ones, eventually leaving an empty pool that cannot receive claims to trigger replenishment. MIN_SIZE_THRESHOLD also defers replacements until availability falls below its threshold. Please give rollout deletions a replacement path that works for every supported strategy and add integration coverage with the real reconciler.
| c.rollingOut = true | ||
| } | ||
|
|
||
| budget := resolveBatchSize(c.pool.GetRolloutPolicy(), c.pool.GetSize()) - inFlight - unavailableProvisioning |
There was a problem hiding this comment.
[P1] Keep missing replacements charged against the availability budget
Once a stale VM's row is deleted, its budget slot is forgotten until a replacement's PROVISIONING row appears. Provision persists that row only after CreateMicroVM returns, so a slow/failed create (or a replacement later quarantined/deleted) allows the next rollout tick to delete another healthy VM despite the previous replacement never becoming available. A two-tick reproduction with size=3, default max_unavailable=1, and the first replacement still pending leaves only one VM. Please account for missing/failed replacements or the actual availability deficit, rather than only DELETING and current-hash provisioning rows.
Summary
Closes #74. When a pool's
microvm_template(image/kernel/rootfs spec) changes viaUpdatePool, this adds an automatic, budget-controlled rolling-replacement mechanism: stale warm VMs are proactively swapped out for VMs built from the new template, without ever touching a leased VM, and without dropping the pool below its configured availability budget.PoolSpec.TemplateHash(server-computed),PoolSpec.RolloutPolicy(max_unavailable as count or percent),VMRecord.TemplateHash,PoolStatus.StaleCount, new rollout-relatedEventTypevalues.internal/api: server-side hash computation (never trusts client input),stale_countreporting,POOL_ROLLOUT_STARTEDemission.internal/reconciler: newRolloutController, one per pool, deletes budgeted stale VMs; backfill is free via the existingStrategy/reconciler wiring. Includes a fix for a TOCTOU race that could otherwise delete a freshly-leased VM, and a fix so the configured budget genuinely bounds unavailability (accounting for in-flight replacements, not just in-flight deletions).internal/store: persiststemplate_hashandrollout_policyacross restarts.internal/poolmgrctl: surfacesstale_countinpool get/pool list;rollout_policyis settable via the existing spec-file mechanism.Test plan
go build ./...go vet ./...go test ./...(full module, including race-detector coverage on the reconciler/store packages)