feat(kubernetes): add ready-on-first-replica option#1011
Conversation
Resolves the TODO in DeploymentInspect: add an option to consider a Deployment or StatefulSet ready as soon as at least one replica is ready, instead of requiring readyReplicas == spec.replicas. With the default all-replicas rule, a single restarting pod of a multi-replica workload flips the instance back to 'starting' - and its sessions to the waiting page - even though the workload still serves traffic through its remaining ready replicas. It also delays wake-up of large environments until every replica of every workload is up. The option is opt-in (--provider.kubernetes.ready-on-first-replica, default false) so existing behavior is unchanged.
There was a problem hiding this comment.
Pull request overview
Adds an opt-in Kubernetes provider option to consider a Deployment/StatefulSet “ready” when at least one replica is ready, to prevent multi-replica workloads from flipping back to starting during partial restarts.
Changes:
- Added
provider.kubernetes.ready-on-first-replicaconfig/CLI/env option and plumbed it into the Kubernetes provider. - Updated Deployment/StatefulSet readiness inspection logic to support “ready on first replica”.
- Updated docs and config-precedence test fixtures; added a KinD-based test covering the new behavior.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/sabliercmd/testdata/config.yml | Adds YAML fixture for the new kubernetes option. |
| pkg/sabliercmd/testdata/config.legacy.env | Adds legacy env var fixture for the new option. |
| pkg/sabliercmd/testdata/config.env | Adds SABLIER_ env var fixture for the new option. |
| pkg/sabliercmd/testdata/config_yaml_wanted.json | Updates expected config JSON for YAML precedence test. |
| pkg/sabliercmd/testdata/config_env_wanted.json | Updates expected config JSON for env precedence test. |
| pkg/sabliercmd/testdata/config_default.json | Updates expected default config JSON with the new field. |
| pkg/sabliercmd/testdata/config_cli_wanted.json | Updates expected config JSON for CLI precedence test. |
| pkg/sabliercmd/root.go | Registers/binds the new CLI flag. |
| pkg/sabliercmd/cmd_test.go | Ensures CLI precedence test covers the new flag. |
| pkg/provider/kubernetes/kubernetes.go | Stores the new option on the provider for downstream logic. |
| pkg/provider/kubernetes/deployment_inspect.go | Implements “ready on first replica” for deployments. |
| pkg/provider/kubernetes/statefulset_inspect.go | Implements “ready on first replica” for statefulsets. |
| pkg/provider/kubernetes/deployment_inspect_test.go | Adds a KinD-based test for the new readiness behavior. |
| pkg/config/provider.go | Adds the new config field with docs/env/CLI metadata. |
| docs/content/reference/cli.md | Documents the new CLI option (docgen output). |
Comments suppressed due to low confidence (1)
pkg/provider/kubernetes/deployment_inspect.go:29
- With
ready-on-first-replicaenabled,readyis currently based only ond.Status.ReadyReplicas > 0. This can incorrectly report a Deployment asreadyeven when it is scaled to 0 (transiently during scale-down,spec.replicasmay already be 0 whilestatus.readyReplicasstill reflects terminating pods). Also, in the ready branchCurrentReplicasis set toconfig.Replicas, which becomes incorrect with this option (e.g. 1/2 ready will be reported as CurrentReplicas=2).
ready := *d.Spec.Replicas != 0 && *d.Spec.Replicas == d.Status.ReadyReplicas
if p.readyOnFirstReplica {
ready = d.Status.ReadyReplicas > 0
}
if ready {
info = sablier.InstanceInfo{
Name: config.Original,
CurrentReplicas: config.Replicas,
DesiredReplicas: config.Replicas,
Status: sablier.InstanceStatusReady,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Please address github copilot review comments. Then we should be good to go :) |
…first-replica
Address review feedback:
- ready-on-first-replica still requires spec.replicas != 0, so a workload
being scaled down to zero is reported as stopped even while terminating
pods transiently keep status.readyReplicas above zero.
- The deployment ready branch now reports status.readyReplicas as
CurrentReplicas (matching the StatefulSet inspect), instead of claiming
all desired replicas are up when only one is ready.
- CreateMimicDeployment/CreateMimicStatefulSet now actually wire
MimicOptions.Healthcheck into the pod spec as a readiness probe, and
callers pass a real /mimic healthcheck exec probe instead of an empty
Probe{} that was silently ignored. This makes the 1/2-ready assertion
in the new test (and the pre-existing 0/1-ready tests) deterministic
instead of relying on pod startup latency.
- The new test also covers scale-to-zero reporting stopped with the
option enabled.
|
@acouvreur Copilot's comments are addressed in d803952:
Locally: |
What
Adds an opt-in Kubernetes provider option
--provider.kubernetes.ready-on-first-replica(envSABLIER_PROVIDER_KUBERNETES_READY_ON_FIRST_REPLICA, defaultfalse) that reports a Deployment or StatefulSet as ready as soon as at least one replica is ready, instead of requiringstatus.readyReplicas == spec.replicas.This resolves the TODO in
DeploymentInspect:Why
With the all-replicas rule, a single restarting pod of a multi-replica workload flips the instance back to
starting— and every session that includes it back to the waiting page — even though the workload is still online and serving through its remaining ready replicas. We hit this in a cluster where sablier manages ~320 deployments across several environments: one crash-looping pod of one 2-replica deployment repeatedly blanked a whole environment behind the loading page.As a side effect, large environments also become reachable during wake-up as soon as the first replica of each workload is up, instead of waiting for full scale-out.
This is intentionally distinct from
sablier.ready-on-start, which skips readiness entirely: here at least one replica must actually be ready.Changes
pkg/config/provider.go: newKubernetes.ReadyOnFirstReplicafield (documented,Since: NEXT_RELEASE)pkg/sabliercmd/root.go: flag registration + viper bindingpkg/provider/kubernetes/kubernetes.go: plumb the option into the providerpkg/provider/kubernetes/deployment_inspect.go,statefulset_inspect.go: apply it (default path unchanged)pkg/provider/kubernetes/deployment_inspect_test.go: KinD test that observes a deterministic 1/2-ready deployment (via-healthy-afterand a scale-up) and assertsstartingwith the default config andreadywith the option enableddocs/content/reference/cli.md: regenerated viacmd/docgenTesting
go build ./...,go vetcleango test -short ./pkg/config/... ./pkg/provider/kubernetes/...passes