fix(proxy): stop deregistering apps that are idle at min_replicas 0 - #184
Draft
nilsmechtel wants to merge 1 commit into
Draft
nilsmechtel wants to merge 1 commit into
nilsmechtel wants to merge 1 commit into
Conversation
check_health deregistered the Hypha service whenever a sibling deployment that had been seen ready dropped to zero RUNNING replicas. That is right for a crash and wrong for autoscaling: under min_replicas: 0 zero replicas is the correct idle state, and deregistering there is unrecoverable by construction — waking the deployment needs a request, and a request needs the registration just removed. Measured at KTH on smart-microscopy-assistant: the app vanished from list_services, get_service raised KeyError, and get_app_status still read RUNNING with empty replica_states. The readiness gate had the same assumption one level up: it required all(running > 0) before registering at all. initial_replicas defaults to None and the autoscaler's lower bound is then min_replicas, so an app *deployed* at min_replicas: 0 starts at zero replicas and never registered in the first place — a second, separate path to the same symptom. Both gates now read Serve's own status alongside the replica count, which is what separates the two cases: Serve keeps a scaled-to-zero deployment HEALTHY and a crashed one is not. UPSCALING and DOWNSCALING count as serviceable too — UPSCALING is precisely the wake-up this enables, and deregistering there would remove the service mid-wake. Wake-on-request then needs no new machinery; it falls out of the Serve autoscaler once the registration stops being removed in front of it. Refs: svamp #59
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Turning on idle scaling (
min_replicas: 0) currently makes an app disappear. The scaling-down half works — the replica goes away and the GPU is genuinely released — but the app is then removed from Hypha's registry entirely, callers get a hard "service not found" rather than a slow first request, and nothing ever brings it back. Meanwhile the worker keeps reporting the app asRUNNING, so an operator sees a green app and a freed GPU with no indication it has stopped being reachable.This is not a missing feature. It is a deliberate deregistration in the proxy's own health check that predates autoscaling support, and it is non-recoverable by construction: it deregisters "until it recovers", but recovery needs traffic and traffic needs the registration it just removed.
Fixes the framework half of svamp #59. Found and measured at KTH by
cold-ruff; root-caused by them to the exact block.What was wrong
check_healthderegistered whenever a sibling deployment that had been seen ready dropped to zero RUNNING replicas. Correct for a crash — a sibling crashing to zero genuinely does mean the app cannot serve — but it fires identically for a deployment autoscaled tomin_replicas: 0, where zero replicas is the correct idle state.Measured at KTH on
smart-microscopy-assistant, reverted after ~7 minutes:downscale_delay_s, rayused_gpu2.0 → 1.0, exactly as askedlist_servicesno longer contained the app at allget_service(...)raisedKeyError: Service not foundget_app_status()still readstatus: "RUNNING"withreplica_states: {}andrunning_version: NoneThere is a second, separate path to the same symptom that the KTH reproduction could not reach. The readiness gate one level up required
all(running > 0)before registering at all.AutoscalingConfig.initial_replicasdefaults toNone, andautoscaling_state.get_num_replicas_lower_bound()then returnsmin_replicas— so an app deployed atmin_replicas: 0starts with zero replicas, never satisfies that gate, and never registers in the first place. The KTH run only saw the autoscale-down path because it applied scaling to an already-running app. A fix touching only the deregistration branch would have left new deployments silently unregistered.The fix
Both gates now read Serve's own per-deployment status alongside the replica count. That is the discriminator, and it is already in the object being read —
DeploymentStatusOverviewcarriesstatusnext toreplica_states. Serve keeps a scaled-to-zero deploymentHEALTHY; a crashed one is not. The KTH measurement grounds this: app-levelRUNNINGrequires every deploymentHEALTHY, so Serve had already classified that scaled-to-zero deployment as healthy while the proxy was treating it as an outage.UPSCALINGandDOWNSCALINGcount as serviceable alongsideHEALTHY, each for a reason:UPSCALINGat zero replicas is the wake-up this change exists to allow. Deregistering there would remove the service during the very request that is bringing it back.DOWNSCALINGat zero is the transient between the last replica going away and the status settling; deregistering and re-registering across it would flap.UPDATINGis deliberately not serviceable — it is also the cold-start state, and treating it as serviceable would register normal apps before they can serve.Wake-on-request needs no new machinery. It falls out of the Serve autoscaler once the registration stops being removed in front of it, which is why this does not need
min_replicas: 0rejected atdeploy_apptime — kth-k8s #12 wants that feature, since KTH's two GPUs are both held and idle-scaling the VLM assistant is the natural way to share them.Tests
tests/apps/test_proxy_entry_saturation_tolerance.py, 5 tests → 10. The four pre-existing gate tests are kept (their doubles are updated to the new(running, status)shape);test_sibling_drop_deregistersis renamed totest_sibling_crash_deregistersand now pins the crash case explicitly asUNHEALTHYat zero, which is what it was always meant to assert.New:
test_scaled_to_zero_does_not_deregister— the Feat/save endpoint improvements #59 regression itselftest_wake_up_window_does_not_deregister—UPSCALINGat zerotest_downscaling_to_zero_does_not_deregister— the settle transienttest_cold_start_at_min_replicas_zero_registers— the second path above, which no live reproduction has yet exercisedtest_serviceability_separates_idle_from_crashed— the discriminator as a table, so it cannot drift silentlyFull suite in the worker image (
0.16.6-dev3), same scope both sides:origin/main209 passed / 4 failed / 24 skipped, this branch 214 passed / 4 failed / 24 skipped. The 4 failures are pre-existing onmain(tests/apps/cellpose/test_metadata_and_glob.py,_FakeArtifact.ls()keyword mismatch in app code) and are untouched by this change.On the strength of the positive control, stated plainly: the new tests are written against the new
_sibling_statesAPI and so cannot run againstmainat all — that is a weak control and I am not presenting it as more. The behavioural control ismain's owntest_sibling_drop_deregisters, which passes onmainand asserts that a sibling at zero replicas gets deregistered with no way to express why it is at zero. That test passing onmainis the bug.Validation status — NOT yet validated live
cold-ruffis running a before/after on a throwaway app at KTH (disable_gpu: true; no production app touched, no GPU spent). Both paths, against current code first so the cold-start half stops being a code reading:min_replicas: 0from the start; expect it never registers, with⏳ Waiting for app ... deployments to run (pending: [...])repeating in the proxy replica logs as the positive marker that the readiness gate is what holds it. Absence of the service without that log line would mean the diagnosis is wrong and this needs rescoping.downscale_delay_s, then call it; expectKeyErroron current code and a blocking upscale under this branch.Marking ready is blocked on those results plus the version bump.