Repository navigation
Conversation
ModelPool activation serialized swaps through a single in-process mutex, so the operator pinned a pooled router to one proxy replica and a second replica would have raced the shared GPU slot. spec.proxy.replicas is now honored: swap decisions are coordinated by a per-pool Lease (defilantech#1477). The router-proxy claims the pool's lease before scaling a member; a replica that does not own it defers by waiting for the member the owner brings up, and never deactivates it. Member writes use an optimistic lock so a concurrent update conflicts (ErrMemberWriteConflict) rather than silently last-write-winning. The activation Role gains the lease verbs, and the proxy reads its namespace from a POD_NAMESPACE fieldRef. Signed-off-by: Christopher Maher <chris@mahercode.io>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Acquire set RenewTime once and nothing refreshed it, so any swap longer than the lease TTL let a second replica take the lease over and drive its own activation: the double-write defilantech#1477 exists to close, reached by the ordinary slow-load path. The default ModelPool swapBudget is 300s against a 60s TTL, so this was the common case, not an edge case. Acquire now returns a release closure and renews RenewTime at a third of the duration for as long as the hold is outstanding, so a hold survives any cold load and two consecutive failed renewals are still needed before another replica can take over. The TTL's only job is bounding how fast a dead owner frees the pool; it is raised to 120s for that. The release is idempotent and clears the hold outright, so the next swap need not wait out the duration. Signed-off-by: Christopher Maher <chris@mahercode.io>
…ters are not Failed The runtime activation Role grants the router-proxy coordination.k8s.io leases, but the operator's ClusterRole granted them nowhere, so RBAC escalation prevention rejected the Role create with a 403 in every namespace a kustomize install serves (and everywhere but the release namespace under Helm, whose leader-election Role grants leases there). The blast radius was the whole router, not just the lease: the activation RBAC step runs before the Deployment, so a pooled ModelRouter was reported Failed with reason ReconcileFailed and never reconciled further. Grant the operator the same lease verbs cluster-wide via a kubebuilder marker (regenerating config/rbac/role.yaml) and add the rule to the chart's ClusterRole by hand, since the chart is hand-maintained and the RBAC sync check would otherwise be satisfied by the namespaced leader-election Role. A pooled router outside the operator's namespace is now provisioned normally, guarded by TestClusterRoleGrantsActivationLease. Signed-off-by: Christopher Maher <chris@mahercode.io>
The RBAC sync check unioned every Role and ClusterRole the chart renders, so the llmkube chart's namespaced leader-election Role could satisfy a marker for a permission the operator needs cluster-wide. That blind spot is how a lease grant read as present while the operator's ClusterRole had none. Restrict the coverage set to ClusterRole documents. A namespaced Role is a grant in one namespace and cannot cover a cluster-wide cache requirement, which is the failure this check exists to catch (defilantech#376, defilantech#1593). Verified the tightening neither weakens nor breaks the existing markers: both operators' control-plane markers are already covered by their ClusterRoles alone. Signed-off-by: Christopher Maher <chris@mahercode.io>
…s shared A per-pool Lease makes the member write single-writer across proxy replicas, but the activator's residency belief and in-flight accounting stay per-replica: a second replica can start a swap and dispatch on a stale fast-path belief until its next member-phase re-read. The controller still owns the drain and gates it on the member's own /slots idleness, so no in-flight request is dropped, but the stale-dispatch window is real and sharing that state is the prerequisite for lifting the pin. Restore the pooled replicas=1 override and say why in the CRD field doc. The Lease, the renewal and the optimistic-locked member writes stay as defense in depth: a rollout overlaps the old and new proxy pod, so two coordinators for one pool are normal during an upgrade even at one replica. Signed-off-by: Christopher Maher <chris@mahercode.io>
Guard the existing owned-gate on the swap counters: a replica that only waits for another replica's member write must not inflate ModelPoolSwapsTotal or the hold/duration histograms, because that is the rate an operator reads to confirm the thrash is gone (defilantech#1477). The control case asserts the owning replica's swap is still counted, so the assertion cannot pass merely because the counter never moves. Signed-off-by: Christopher Maher <chris@mahercode.io>
A lease API failure failed the swap and surfaced through the generic backend-failure path, so a control-plane blip was indistinguishable from a serving outage in the proxy log and status. Wrap lease read/write failures in ErrActivationLeaseUnavailable, keep the sentinel through the activator's swap error, and map it in the proxy to 503 + Retry-After with its own audit reason (pool_lease_unavailable), the same transient class as a busy incumbent or an exhausted hold budget. The swap is still not driven: without the lease this replica cannot know it owns it. Also log once when activation runs without POD_NAMESPACE, since pooled swap coordination silently degrades to in-process serialization there. Signed-off-by: Christopher Maher <chris@mahercode.io>
Unit and envtest suites cannot catch this class of bug: the envtest fake client does not enforce RBAC, and no other e2e context creates a pooled router, so a missing coordination.k8s.io grant on the operator's ClusterRole shipped green while every pooled ModelRouter was reported Failed (defilantech#1477). Apply a real pooled router (member InferenceService + ModelPool + router referencing it) in its own namespace through the API server, then assert the activation Role exists and grants swap leases, and that the router reaches Ready without an ActivationRBAC reason. Runs in the existing kind e2e job, which installs the operator via kustomize and so exercises config/rbac/role.yaml, the artifact the escalation check reads. Signed-off-by: Christopher Maher <chris@mahercode.io>
Resolve the regenerated modelrouters CRD against the endpoint-resolution field and keep both new builder tests plus the lease-branch error mapping in the proxy handler. Signed-off-by: Christopher Maher <chris@mahercode.io>
…ent builder The env block still carried a comment from the pre-pin design asserting that a pooled router runs one or more replicas and needs no pin, which is the opposite of the pooled replicas=1 override 30 lines above it and of the behavior. Comment and code agreeing is the point; delete the stale one. Refs defilantech#1477 Signed-off-by: Christopher Maher <chris@mahercode.io>
Defilan
left a comment
Defilan
left a comment
There was a problem hiding this comment.
This is close and the core is right, but I want two things settled before it merges.
First, the post-failure scale-down is gone whenever a coordinator is set, and that is every pooled router now that POD_NAMESPACE is injected. deactivate := pr.leaseHeld || a.coord == nil is false on the path that matters: the swap goroutine has already run releaseSwapLease, so leaseHeld is cleared before the last waiter's deferred func decides, and a failed Activate leaves the member up. I ran it both ways against the fake controller: with a coordinator the deactivate count after a failed swap is 0, without one it is 1. The "another replica might own it" reading only applies at replicas >= 2, and the pin holds pooled routers at exactly one, so on the only shipping configuration it is a leak. The comment on that line says the deactivate is what stops a member being "left scaled up for nobody", which is exactly what this reaches.
Second, the description and the code disagree. It says spec.proxy.replicas is honored for pooled routers and that the pin is removed, but router_deployment_builder.go still sets replicas = 1 and logs the override, and the new CRD doc says the pin stays. That means #1477's acceptance item 3, "the replicas=1 pin is removed", is unmet, so this should be Refs #1477 rather than Fixes, or the issue closes with the pin still in place.
Everything else held up. I reverted the optimistic lock and the conflict test failed; I let a live holder be taken over and the serialization test failed; and I removed the lease rule from the chart ClusterRole and check-helm-rbac.sh exited 1, so the new RBAC guard covers what it claims. One small thing: the lease default is 120s in the code while the open question says 60s. It also needs a rebase, activator.go conflicts with main.
|
Closing this rather than rebasing it. Main is 192 commits ahead and the branch conflicts in #1477 stays open, and main still pins a pooled router to one proxy replica, so nothing is unsafe in the meantime. The one piece that stands on its own, the |
What
ModelPool activation is now single-writer across router-proxy replicas.
spec.proxy.replicasis honored for pooled routers, swap decisions are coordinated by a per-pool Lease, and member writes use an optimistic lock.Why
Activation serialized swaps through a single in-process mutex, so the operator pinned a pooled router to one replica; at two or more replicas two proxies receiving requests for different members would each drive a swap and the later write silently won. The controller-side exclusive-slot invariant still held, so this was thrash (a full drain plus a multi-gigabyte reload) rather than corruption, with nothing in status to indicate it.
Fixes #1477
How
LeaseCoordinatorclaims acoordination.k8s.ioLease per pool before a swap. A replica that does not own it defers by waiting for the member the owner brings up and never deactivates it; an expired lease is taken over so a crashed owner does not wedge the pool.KubeMemberControllerwrites withclient.MergeFromWithOptimisticLock, returningErrMemberWriteConflicton a resourceVersion race instead of last-write-winning.POD_NAMESPACEfieldRef.spec.proxy.replicaspin is removed and its CRD doc updated.Checklist
make testpasses locallymake lintpasses locallygit commit -s) per DCOOpen questions