feat: add pool autoscaling policies (min/max, demand-based) - #84
richardcase wants to merge 1 commit into
Conversation
Adds an optional AutoscalingPolicy on PoolSpec that dynamically adjusts a pool's target size within [min_size, max_size] based on observed claim rate, layered on top of the existing replenishment strategies rather than replacing them. - New AutoscalingPolicy proto message, PoolStatus fields (last_scaled_at, observed_claims_per_sec), and POOL_SCALED_UP/DOWN events. - New Autoscaler component: tracks a per-pool claim-rate sliding window and decides scale up/down/none with cooldown and min/max clamping. - Reconciler evaluates the autoscaler each tick, persists a changed size, and emits the corresponding event; existing replenishment strategies keep provisioning toward whatever the current size is. - Store persists autoscaling_policy alongside the rest of PoolSpec. - PoolAdminServer surfaces live autoscaler state (claim rate, last scaled time) in PoolStatus via a narrow PoolLifecycle.AutoscalerSnapshot method. Pools without an autoscaling_policy are unaffected. Closes #57
richardcase
left a comment
richardcase
left a comment
There was a problem hiding this comment.
Reviewed the implementation and its integration with persistence, pool lifecycle, and replenishment strategies. The existing tests for reconciler, store, API, pool manager, and CLI pass. Temporary targeted regression checks reproduced the database-upgrade failure, unbounded history for disabled policies, ignored target growth in two strategies, and acceptance of invalid policies. The inline comments also describe a concurrent operator-update overwrite found by tracing the write ordering.
| hook_failure_policy INTEGER NOT NULL, | ||
| heartbeat_interval_ns INTEGER NOT NULL, | ||
| heartbeat_expiry_threshold_ns INTEGER NOT NULL, | ||
| autoscaling_policy TEXT NOT NULL DEFAULT '', |
There was a problem hiding this comment.
[P1] Migrate existing databases before querying the new column
Open only runs CREATE TABLE IF NOT EXISTS, so adding this column to the table declaration does not add it to an existing database. I reproduced opening a database created with the previous schema: Open succeeds, then ListPools fails with no such column: autoscaling_policy. Since poolmgrd calls Manager.Seed on startup, upgrading with an existing database prevents the daemon from starting, even for pools without an autoscaling policy. Please add an idempotent migration and an upgrade test.
| eventType = poolmgrv1alpha1.EventType_POOL_SCALED_DOWN | ||
| } | ||
|
|
||
| r.pool.Size = newSize |
There was a problem hiding this comment.
[P1] Reconcile actual VM capacity after changing the target
Changing PoolSpec.Size does not make the existing strategies converge to the new target. Both IMMEDIATE_ON_LEASE and REPLACE_ON_DELETE ignore size entirely: their tick hook returns zero and their event hook always creates one replacement. A targeted check grew the target from 2 to 3 with two available VMs, but both strategies requested zero additional VMs. There is also no path to remove surplus available VMs when the target decreases, so an idle pool can report POOL_SCALED_DOWN without releasing any capacity. Please implement target convergence for enabled autoscaling policies, including safe removal of surplus available VMs, and test actual VM counts for the supported strategies.
| } | ||
|
|
||
| r.pool.Size = newSize | ||
| if err := r.store.UpdatePool(ctx, r.pool); err != nil { |
There was a problem hiding this comment.
[P1] Prevent autoscaling from overwriting concurrent operator updates
This writes the reconciler's entire cached spec back to the store without participating in PoolAdminServer.lockPool. UpdatePool persists the operator's new spec before stopping the old reconciler, so an autoscaling tick in that interval can replace the new hosts, template, hooks, and policy with the old values. The RPC can then return success and start a reconciler with the new spec while the database contains the old one. Please serialize scaling with the pool lifecycle update or use a conditional size-only update tied to the spec revision, so a stale reconciler cannot clobber an accepted update.
| // blocks: if a notification is already pending, this is a no-op, since | ||
| // Run's next pass will observe the same underlying state change either way. | ||
| func (r *Reconciler) NotifyVMClaimed() { | ||
| r.autoscaler.RecordClaim(time.Now()) |
There was a problem hiding this comment.
[P2] Avoid collecting claim history when autoscaling is disabled
Every successful claim now appends a timestamp, including pools with no policy or enabled=false. Both evaluateAutoscaler and AutoscalerSnapshot return before pruning for those pools, so their claim history grows for the entire lifetime of the reconciler. A targeted check retained all 10,000 timestamps after evaluation and a snapshot on a pool without a policy. This introduces unbounded memory growth for existing non-autoscaling pools; gate recording on an enabled policy or otherwise bound/prune the history.
| } | ||
|
|
||
| a.pruneLocked(now, policy.GetClaimRateWindow().AsDuration()) | ||
| rate := float64(len(a.claimTimes)) / policy.GetClaimRateWindow().AsDuration().Seconds() |
There was a problem hiding this comment.
[P2] Validate autoscaling parameters before accepting the pool spec
Neither validatePoolSpec nor New validates the new policy. I confirmed CreatePool accepts an enabled policy with inverted min/max bounds, a negative step, and no claim-rate window. With a missing/zero window this division normally becomes 0/0, so neither threshold comparison fires and scaling silently stops; a negative step can reverse scaling direction and violate the opposite bound. Please reject invalid enabled policies with InvalidArgument in both create and update (positive window/step, consistent nonnegative bounds, valid durations and finite ordered thresholds), and add validation tests.
Summary
Adds autoscaling policies on top of the existing per-pool reconciler, as requested in #57: min/max bounds on pool size, and demand-based scaling driven by claim rate.
AutoscalingPolicyproto message onPoolSpec(min/max size, scale step, scale-up/down claim-rate thresholds, claim-rate window, cooldown). Optional and backward compatible — pools without it behave exactly as before.Autoscalercomponent (internal/reconciler/autoscaler.go): tracks a per-pool claim-rate sliding window and decides scale up/down/none, with cooldown and min/max clamping.Reconcilerevaluates the autoscaler each tick, persists a changed size via the store, and emits newPOOL_SCALED_UP/POOL_SCALED_DOWNevents. Existing replenishment strategies are unchanged — they keep provisioning toward whatever the current size is.autoscaling_policyalongside the rest ofPoolSpec.PoolStatusgainslast_scaled_at/observed_claims_per_sec, surfaced from the live per-poolAutoscalerstate through a narrowPoolLifecycle.AutoscalerSnapshotmethod (Reconciler→poolmanager.Manager→PoolAdminServer).poolmgrctlcode changes needed:create/update --spec-fileand JSON output already round-trip the wholePoolSpecvia protojson, andevents tailalready printsEventTypegenerically.Test plan
internal/reconciler/autoscaler_test.go: unit tests for claim-rate window pruning, scale-up/down threshold crossing, min/max clamping, cooldown enforcement, dead zone.internal/reconciler/reconciler_test.go: tick-driven scale-up integration test against a real store, and a regression test confirming size never changes for pools without a policy.internal/store/sqlite_test.go: round-trip tests forautoscaling_policy(absent and set).internal/poolmanager/manager_test.go/internal/api/pooladmin_test.go:AutoscalerSnapshotforwarding throughManagerand intoPoolStatus.go build ./...,go vet ./...,gofmt,golangci-lint run ./..., and the fullgo test ./...suite all pass.Closes #57