Skip to content

feat(admin,divide): draining admission lets instances stop receiving new work without deregistering (#8042) - #8101

Merged
Aias00 merged 1 commit into
apache:masterfrom
konlue:feat/draining-admission-phase1
Oct 26, 2026
Merged

Aias00 merged 1 commit into
apache:masterfrom
konlue:feat/draining-admission-phase1

Conversation

@konlue

@konlue konlue commented Oct 25, 2026

Copy link
Copy Markdown
Contributor

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

  • A client declares draining through the existing URI registry with a new EventType.DRAINING (HTTP SDK: ShenyuClientRegisterRepository.draining, reusing the URI registration endpoint — no new admin endpoint).
  • Admin merges a draining marker into the discovery upstream's free-form props (markDrainingBySelectorIdAndUrl, idempotent, preserving existing keys such as warmup/gray/labels) and publishes the normal DISCOVER_UPSTREAM update, so every gateway replica converges through the existing sync channel.
  • The marker deliberately does not use the enabled/status column: heartbeats and health probes flip that column back and forth. Because the heartbeat path only updates the status column, a draining instance that keeps beating is never re-admitted — the "heartbeats must not cancel a drain" requirement is structural, and locked by a test.
  • Gateways parse draining from props (UpstreamProps), carry it on the loadbalancer Upstream (outside its identity), and UpstreamCacheManager routes 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.
  • A fresh registration releases the drain — the registration path explicitly clears the marker, so a restarting replacement process claims its identity back, while heartbeats alone can never do so. The generation-scoped stale-drain handshake from the issue is left to a later phase together with ready/not-ready states and propagation deadlines.

Tests

  • Exclusion: Two upstreams with one draining → the candidate set contains only the sibling; the draining instance sits in neither health pool; a marker-free sync re-admits it. All-draining → no selectable upstream (the divide empty-pool 503 path already covers it).
  • Red/green: Removing the cache-manager routing makes exactly the two new exclusion tests fail; restoring it turns 19/19.
  • Heartbeat lock: The heartbeat path asserts updateStatusByUrl only — never a props write.
  • Admin: Props merge preserves existing keys and is idempotent; unknown handler/url are ignored; the subscriber dispatches DRAINING grouped by namespace.

Verification

UpstreamCacheManagerTest: 19
UpstreamPropsTest: 11
DivideUpstreamDataHandlerTest: 23
divide module: 125
shenyu-admin: 2427

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.

@Aias00 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@konlue

konlue commented Oct 25, 2026

Copy link
Copy Markdown
Contributor Author

Both answered — neither needs a code change:

  1. Compatibility: Only the HTTP registration transport carries DRAINING. On the degradation question I'd argue against a lenient reader: the Admin dispatcher treats a null event type as REGISTER, so a silently degraded DRAINING would re-register the instance — exactly the opposite of the declaration. A loud failure (request rejected, retried by the client's failback timer) plus an upgrade-order requirement is the safer contract.
  2. Timing: The removal is synchronous. triggerRemoveOne mutates both pool maps inline (no executor involved), and doSubmit runs inside task.withLock — the UPSTREAM_MAP rebuild and its getHealthyUpstreamListBySelectorId snapshot read happen in the same critical section, so removal happens-before the rebuild. This also covers the pre-existing processOfflineUpstreams pattern.

@Aias00 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Aias00
Aias00 merged commit 8a2fc92 into apache:master Oct 26, 2026
55 checks passed
@Aias00 Aias00 added this to the 2.7.2 milestone Nov 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants