fix(epp): tolerate transient EndpointSlice port absence during startup - #13534
fix(epp): tolerate transient EndpointSlice port absence during startup#13534panpan0000 wants to merge 5 commits into
Conversation
The EPP resolves its replica-agg peer port from the Service EndpointSlices at startup and treated "a slice does not expose the named port" as fatal. The EndpointSlice controller rewrites slices while pods churn, so restarting all pods at once makes every new pod LIST a slice mid-update that momentarily lacks the port: each replica then crashes once and is restarted by Kubernetes (observed as `EndpointSlice dynamo-epp-... does not expose named port "replica-agg"`). The Service is the single source of truth for the port list, so a momentarily-incomplete slice is a transient race, not a misconfiguration. Skip slices that lack the named port and error only when no slice exposes it or values conflict; an explicit non-TCP protocol is still rejected. Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
|
👋 Hi panpan0000! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
github-actions
Bot
added
the
external-contribution
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. Walkthrough
ChangesReplica aggregate port discovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🔵 Low · up to The change is localized and addresses a narrow startup race, but the repository-required full formatting and workspace Clippy checks still need to be completed or explicitly accepted by an owner before merge. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Comment |
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
…pdates Skipping a transiently-incomplete slice still leaves the single-slice case failing once: when the only slice is mid-update, resolution has nothing to resolve and the EPP crashes at startup, then Kubernetes restarts it after the slice settles. The transient window is short (hundreds of ms), so re-LIST with a bounded doubling backoff (5 attempts, 100ms doubling) before giving up. A genuine misconfiguration still fails with the same clear error after retries exhaust. Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
Add missing docstrings to raise docstring coverage on the touched file. Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
|
Addressed both pre-merge check warnings:
The two commits implementing the fix itself remain |
|
/ok to test 3069089 |
|
I think the intended startup flow should be:
An EndpointSlice with endpoints but without the required named port is not a recovery source. That may be brief controller convergence, an older/incompatible peer, or a configuration problem, but it should not crash or block EPP startup. We should exclude it from the initial recovery set, log/metric it, and let normal EndpointSlice discovery incorporate it if a later update makes it usable. This avoids making startup depend on a retry window while retaining the Service as the source of truth for the required port contract. |
Resolve the replica-sync port from the peer Service's stable spec.ports instead of LISTing EndpointSlices. Pod restarts rewrite EndpointSlices while the Service spec never changes, so a momentarily-incomplete slice can no longer fail EPP startup: the race this PR originally worked around with skip-and-retry is gone at the source, and the bounded retry loop is removed. The Service stays the single source of truth for the port list: missing, duplicated, non-TCP, or non-positive ports still fail startup as a genuine misconfiguration. EndpointSlices remain the discovery source for which peers exist (spawn), never for the port number. Rewrites the port-resolution tests against Service fixtures, including the key assertion that resolution consults only the stable Service object, so transient slice state can never fail startup. Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
|
Implemented, following your intended startup flow — the port contract is now validated from the peer Service's stable
One note: this reads the Service |
tmonty12
left a comment
tmonty12
left a comment
There was a problem hiding this comment.
Two P1 issues in the new Service-based port resolution:
|
/ok to test 270dec6 |
Signed-off-by: Peter Pan <Peter.Pan@daocloud.io>
github-actions
Bot
added
the
documentation
2 participants
this is an issue when I developing #13451
Summary
The standalone EPP resolves its
replica-aggpeer port at startup. Resolution previously LISTed the Service's EndpointSlices and treated "an EndpointSlice does not expose the named port" as a fatal startup error. When all EPP pods restart at once, the EndpointSlice controller rewrites the slice while pods churn, so a new pod can LIST a slice mid-update that momentarily lacks the port — every replica then crashes once and is restarted by Kubernetes.This is a pre-existing weakness in the replica-sync port resolution (surfaced while validating #13451, but independent of that feature).
Observed error (all EPP pods restarted simultaneously)
Each pod crashed once and recovered on the next start once the slice settled (
RESTARTS: 1, then1/1 Running).Root fix
The Service spec — not its EndpointSlices — is the source of truth for the port list, and it never changes during pod restarts. Resolution now GETs the peer Service and validates exactly one TCP
replica-aggport fromspec.ports; EndpointSlices are consulted only for endpoint membership (the existing watch inspawn). A momentarily-incomplete slice can therefore never fail EPP startup — the race is gone at the source, and the earlier skip-and-retry workaround is removed.Genuine misconfiguration still fails fast with a clear error: missing, duplicated, non-TCP, or non-positive
replica-aggports.Changes
270dec60: resolve thereplica-aggport from the Service contract —resolve_replica_sync_portnow GETs the Service and validatesspec.ports; the bounded retry loop and skip-transient-slice logic are removed.56dd847c,f55d92aa) document the journey to the fix; the final state is the Service-contract resolution above.Reviewer starting point
resolve_replica_sync_portindeploy/inference-gateway/ext-proc/src/peer_discovery.rs.Related Issues
No dedicated issue; this was surfaced while validating #13451 and is an independent pre-existing fix.
Validation
cargo fmt -p dynamo-ext-proc -- --checkcargo test -p dynamo-ext-proc peer_discovery(15 passed, including the newresolves_from_service_contract_regardless_of_slice_state)cargo clippy -p dynamo-ext-proc --no-deps --all-targets -- -D warnings