From f2be28ff123a45f826718bf7760417e76bfef4ce Mon Sep 17 00:00:00 2001 From: Dennis Ramdass Date: Mon, 26 Oct 2026 11:40:17 -0700 Subject: [PATCH] Hold an engine Deployment unready until its rollout finishes A replica reported Ready while its engine's Deployment was partway through a rolling update. The Deployment's readiness query required Available and a ready pod per copy, but readyReplicas also counts pods from the previous ReplicaSet. With the default maxSurge, a one-copy engine kept its old pod ready while the new one started. The query held, so the replica, its endpoint and its deployment all reported Ready though no copy ran the current spec. The query could also pass on the status provider-kubernetes reads back when it applies a spec change, before the Deployment controller has processed that change. The Deployment's query now also requires status.observedGeneration to match metadata.generation, updatedReplicas to reach spec.replicas, and status.replicas to equal updatedReplicas. These are the checks kubectl rollout status makes. During a rollout the replica keeps serving from its old copies and reports ModelReady False with reason RollingOut. Its deployment reports ReplicasServing True and Ready False until the rollout finishes. The LeaderWorkerSet query doesn't change. lws sets Available only once every group is ready and updated, and sets it False while it replaces a group. lws doesn't publish an observedGeneration, so the query can't reject a status read back before the lws controller processes a template change. The shared Available query now checks that status exists, as the other functions' queries do. Without that check cel-go fails with "no such key: status" on a workload that has no status yet, where the query should return false. Fixes #497. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Dennis Ramdass --- docs/content/models/model-deployment.md | 11 ++++---- .../function/backends/base.py | 28 ++++++++++++++----- .../function/backends/native.py | 2 +- .../compose-model-replica/function/fn.py | 4 +-- .../tests/test_backends.py | 13 +++++++-- .../compose-model-replica/tests/test_fn.py | 10 +++++-- 6 files changed, 48 insertions(+), 20 deletions(-) diff --git a/docs/content/models/model-deployment.md b/docs/content/models/model-deployment.md index e851d998f..0b7543934 100644 --- a/docs/content/models/model-deployment.md +++ b/docs/content/models/model-deployment.md @@ -261,8 +261,8 @@ whether the model answers requests. | `ReplicasReady` | Every replica in `spec.replicas` is at full capacity. | | `Ready` | `ReplicasReady` is True and the referenced `ModelCache` exists. | -A deployment with `ReplicasServing=True` and `Ready=False` is degraded. It -serves from the replicas and copies that are up. `kubectl get` shows +A deployment with `ReplicasServing=True` and `Ready=False` is degraded, or is +rolling out an update. It serves from the replicas and copies that are up. `kubectl get` shows `ReplicasServing` in the `SERVING` column, and a replica's `ModelServing` in its own. @@ -274,9 +274,10 @@ waiting on: - `ModelServing`: at least one copy of every engine is ready, and the endpoint picker and routes in front of them are in place. The deployment routes to a replica only while it's serving. -- `ModelReady`: every copy is ready. Its message counts the ready copies, and - its reason is `Degraded` while the replica serves below full capacity. The - replica's `Ready` mirrors it. +- `ModelReady`: every copy is ready and runs the current spec. Its message + counts the ready copies. Its reason is `Degraded` while the replica serves + below full capacity, and `RollingOut` while an update replaces copies that + still serve. The replica's `Ready` mirrors it. To see which replica a deployment is waiting on: diff --git a/functions/compose-model-replica/function/backends/base.py b/functions/compose-model-replica/function/backends/base.py index b08f67519..354db6da8 100644 --- a/functions/compose-model-replica/function/backends/base.py +++ b/functions/compose-model-replica/function/backends/base.py @@ -491,20 +491,34 @@ def workload_keys(replica: v1alpha1.ModelReplica) -> list[str]: # CEL readiness query matching a Deployment's or LeaderWorkerSet's Available=True # condition. Both publish this condition but neither publishes a Ready # condition, so provider-kubernetes' DeriveFromObject policy (which only checks a -# Ready condition) can never mark them ready. The has() guard keeps the query -# false (not erroring) before the workload first writes status.conditions. +# Ready condition) can never mark them ready. The has() guards keep the query +# false (not erroring) before the workload first writes its status. AVAILABLE_CEL = ( - 'has(object.status.conditions) && object.status.conditions.exists(c, c.type == "Available" && c.status == "True")' + "has(object.status) && has(object.status.conditions) && " + 'object.status.conditions.exists(c, c.type == "Available" && c.status == "True")' ) -# CEL readiness query for an engine's Deployment or LeaderWorkerSet with every -# copy ready. Available alone isn't enough: a Deployment stays Available with up -# to its maxUnavailable (25% by default) of copies down. So the query also -# requires a ready pod, or ready group, per copy. +# CEL readiness query for an engine's LeaderWorkerSet with every copy ready. +# lws sets Available only once every group is ready and runs the current +# template, and the query also requires a ready group per copy. ALL_COPIES_READY_CEL = ( AVAILABLE_CEL + " && has(object.status.readyReplicas) && object.status.readyReplicas >= object.spec.replicas" ) +# CEL readiness query for an engine's Deployment with every copy ready and +# running the current spec. Available alone isn't enough: a Deployment stays +# Available with up to its maxUnavailable (25% by default) of copies down. A +# ready pod per copy isn't enough either, because readyReplicas also counts +# the previous ReplicaSet's pods while a rollout replaces them. So the query +# also requires the Deployment controller to have processed the current spec, +# every copy to be updated, and no pod from an older ReplicaSet to remain. +DEPLOYMENT_ALL_COPIES_READY_CEL = ( + ALL_COPIES_READY_CEL + " && has(object.status.observedGeneration)" + " && object.status.observedGeneration == object.metadata.generation" + " && has(object.status.updatedReplicas) && object.status.updatedReplicas >= object.spec.replicas" + " && has(object.status.replicas) && object.status.replicas == object.status.updatedReplicas" +) + # Grove publishes no Ready or Available condition on a PodCliqueSet, so # readiness comes from its replica counters: a replica counts as available once # every clique and scaling group is at or above minAvailable. observedGeneration diff --git a/functions/compose-model-replica/function/backends/native.py b/functions/compose-model-replica/function/backends/native.py index 5c8a3133a..3481a3654 100644 --- a/functions/compose-model-replica/function/backends/native.py +++ b/functions/compose-model-replica/function/backends/native.py @@ -119,7 +119,7 @@ def build( composed = { base.workload_key(engine): base.wrap_object( - provider_config, deployment, cel_query=base.ALL_COPIES_READY_CEL, watch=True + provider_config, deployment, cel_query=base.DEPLOYMENT_ALL_COPIES_READY_CEL, watch=True ), } # Gated like the container's resources above, and for the same reason. diff --git a/functions/compose-model-replica/function/fn.py b/functions/compose-model-replica/function/fn.py index 56be2da1d..c429ecd92 100644 --- a/functions/compose-model-replica/function/fn.py +++ b/functions/compose-model-replica/function/fn.py @@ -267,8 +267,8 @@ def ready(key: str) -> bool: if serving and sum(ready_copies.values()) < sum(copies.values()): waiting = CONDITION_REASON_DEGRADED elif serving: - # Every copy counts as ready but a workload doesn't yet, as during - # a LeaderWorkerSet's rolling update. + # Each copy counts as ready but a workload doesn't yet, as while a + # rolling update replaces copies that still serve. waiting = CONDITION_REASON_ROLLING_OUT elif accepted and copy_each: waiting = CONDITION_REASON_WAITING_FOR_ROUTING diff --git a/functions/compose-model-replica/tests/test_backends.py b/functions/compose-model-replica/tests/test_backends.py index 3bc81c5e3..d4a0c0b7c 100644 --- a/functions/compose-model-replica/tests/test_backends.py +++ b/functions/compose-model-replica/tests/test_backends.py @@ -273,7 +273,8 @@ def _epp( "readiness": { "policy": "DeriveFromCelQuery", "celQuery": ( - 'has(object.status.conditions) && object.status.conditions.exists(c, c.type == "Available" && c.status == "True")' + "has(object.status) && has(object.status.conditions) && " + 'object.status.conditions.exists(c, c.type == "Available" && c.status == "True")' ' && !object.status.conditions.exists(c, c.type == "Progressing" && c.status == "False")' ), }, @@ -457,8 +458,13 @@ def _deployment( "readiness": { "policy": "DeriveFromCelQuery", "celQuery": ( - 'has(object.status.conditions) && object.status.conditions.exists(c, c.type == "Available" && c.status == "True")' + "has(object.status) && has(object.status.conditions) && " + 'object.status.conditions.exists(c, c.type == "Available" && c.status == "True")' " && has(object.status.readyReplicas) && object.status.readyReplicas >= object.spec.replicas" + " && has(object.status.observedGeneration)" + " && object.status.observedGeneration == object.metadata.generation" + " && has(object.status.updatedReplicas) && object.status.updatedReplicas >= object.spec.replicas" + " && has(object.status.replicas) && object.status.replicas == object.status.updatedReplicas" ), }, "watch": True, @@ -516,7 +522,8 @@ def _leader_worker_set( "readiness": { "policy": "DeriveFromCelQuery", "celQuery": ( - 'has(object.status.conditions) && object.status.conditions.exists(c, c.type == "Available" && c.status == "True")' + "has(object.status) && has(object.status.conditions) && " + 'object.status.conditions.exists(c, c.type == "Available" && c.status == "True")' " && has(object.status.readyReplicas) && object.status.readyReplicas >= object.spec.replicas" ), }, diff --git a/functions/compose-model-replica/tests/test_fn.py b/functions/compose-model-replica/tests/test_fn.py index 599432ec4..4ee370073 100644 --- a/functions/compose-model-replica/tests/test_fn.py +++ b/functions/compose-model-replica/tests/test_fn.py @@ -237,8 +237,13 @@ def _deployment(*, ready: fnv1.Ready, name: str, claim_template_name: str, label "readiness": { "policy": "DeriveFromCelQuery", "celQuery": ( - 'has(object.status.conditions) && object.status.conditions.exists(c, c.type == "Available" && c.status == "True")' + "has(object.status) && has(object.status.conditions) && " + 'object.status.conditions.exists(c, c.type == "Available" && c.status == "True")' " && has(object.status.readyReplicas) && object.status.readyReplicas >= object.spec.replicas" + " && has(object.status.observedGeneration)" + " && object.status.observedGeneration == object.metadata.generation" + " && has(object.status.updatedReplicas) && object.status.updatedReplicas >= object.spec.replicas" + " && has(object.status.replicas) && object.status.replicas == object.status.updatedReplicas" ), }, "watch": True, @@ -457,7 +462,8 @@ def _epp(*, ready: fnv1.Ready, config_checksum: str) -> fnv1.Resource: "readiness": { "policy": "DeriveFromCelQuery", "celQuery": ( - 'has(object.status.conditions) && object.status.conditions.exists(c, c.type == "Available" && c.status == "True")' + "has(object.status) && has(object.status.conditions) && " + 'object.status.conditions.exists(c, c.type == "Available" && c.status == "True")' ' && !object.status.conditions.exists(c, c.type == "Progressing" && c.status == "False")' ), },