Repository navigation
feat(admin,divide): draining admission lets instances stop receiving new work without deregistering (#8042) - #8101
Conversation
…new work without deregistering (apache#8042)
Aias00
left a comment
There was a problem hiding this comment.
This is a well-scoped first slice and the boundary it draws is the right one — "alive but not admitting new work" is genuinely different from the offline path, and keeping the row in the configured set so a later sync without the marker re-admits it is the correct semantic.
The partition in UpstreamCacheManager.doSubmit is sound: partitioningBy(isStatus && !isDraining) plus a separate filter(isDraining) and a filter(!isDraining) on the false bucket yields three disjoint, complete sets, so no upstream can be dropped or double-counted. updatePropsByUrl deliberately not touching status is a good call, since a heartbeat-driven status flip would otherwise wipe the marker.
Two things I would like your read on before this lands:
1. EventType.DRAINING and rolling upgrades. Adding an enum constant is source-compatible but not wire-compatible in the failing direction. During a rolling upgrade where a new client talks to an older admin, deserializing DRAINING into the old EventType will throw rather than degrade. If that path can be hit, it either needs a tolerant reader or the feature needs to be documented as requiring admin-first upgrade order. Worth confirming which register transports actually carry this value.
2. Timing of the draining removal. processDrainingUpstreams calls task.triggerRemoveOne(...) and then, a few lines later, doSubmit reads task.getHealthyUpstreamListBySelectorId(selectorId) to rebuild UPSTREAM_MAP. If triggerRemoveOne is asynchronous, that read can still observe the pre-removal snapshot and write a healthy list that contains the draining instance — which would keep it selectable until the next sync. processOfflineUpstreams has the same shape, so this may be pre-existing and benign, but for draining the "stop receiving new work" guarantee is the whole point, so it is worth stating explicitly whether the removal is synchronous or whether the subsequent read is guaranteed to happen after it.
Neither is a blocker from my side — flagging so the guarantee is documented rather than assumed.
|
Both answered — neither needs a code change:
|
Aias00
left a comment
There was a problem hiding this comment.
Approved. The draining admission feature is complete and well wired end to end: the client repository marks an instance draining via a new EventType.DRAINING, the admin merges a draining marker into the upstream's props (status is left intact so heartbeat-driven flips cannot cancel the drain, and only a fresh registration releases it), and the gateway reads UpstreamProps.isDraining() into Upstream.draining, which UpstreamCacheManager then excludes from new selections and pulls out of both health pools. Covering divide and websocket handlers plus tests across admin/loadbalancer/plugin makes the lifecycle consistent. The marker-in-props choice correctly survives heartbeat status churn. Good work.
First delivery slice of #8042: the draining admission boundary — a registered business instance can declare that it is alive but must stop receiving new work, and gateways stop selecting it while it keeps serving the work already admitted.
Problem
Today the only way to take a registered instance out of rotation is the offline path, which removes it from the configuration outright: in-flight work loses its target, and a restarting process must fully re-register. There is no intermediate state between "enabled" and "gone", and instance health (
instance_state) is display-only — it never influences upstream selection.Change
EventType.DRAINING(HTTP SDK:ShenyuClientRegisterRepository.draining, reusing the URI registration endpoint — no new admin endpoint).drainingmarker into the discovery upstream's free-formprops(markDrainingBySelectorIdAndUrl, idempotent, preserving existing keys such as warmup/gray/labels) and publishes the normalDISCOVER_UPSTREAMupdate, so every gateway replica converges through the existing sync channel.drainingfrom props (UpstreamProps), carry it on the loadbalancerUpstream(outside its identity), andUpstreamCacheManagerroutes draining instances out of both the healthy and unhealthy pools: no new selection returns them, no probe can restore them, and a later sync without the marker re-admits them through the regular paths.Tests
19/19.updateStatusByUrlonly — never a props write.DRAININGgrouped by namespace.Verification
All green; checkstyle clean on all eight touched modules.
Out of scope (later phases per the issue)
ready/not-ready state model, generation-scoped drain handshake, propagation deadlines/disconnect behavior, gRPC entity mapping, admin UI.