Repository navigation
router: report no_capacity only for a ResourceExhausted cause - #1929
Jeff Luo (JeffLuoo) merged 3 commits into
Conversation
classifyOutcome sent every 503 with no gRPC cause to no_capacity, so a full parking lot, a denied egress request, and a failed policy or credential lookup all counted as fleet saturation. Only report no_capacity when the cause actually carries codes.ResourceExhausted; everything else falls back to unavailable, matching what the registry already says the value means. Also move the outcome strings into named constants in ateattr, next to the existing RouterResume ones, instead of scattering literals across the switch.
| // capacity problem, not a defect. Everything else that denies a request | ||
| // without a matching gRPC code, such as a full parking lot, a denied | ||
| // request, or a failed dependency lookup, is RouterOutcomeUnavailable or one | ||
| // of the other values below, never no_capacity. |
There was a problem hiding this comment.
This comment says "the fleet having no free worker". A worker can now hold more than one actor, so "free worker" is not correct any more. #1895 removes this wording from the registry. Can you use the same words as the no_capacity brief?
Suggestion -
// Values for RouterOutcomeKey. RouterOutcomeNoCapacity means that no worker
// that meets the constraints of the actor had room. scheduling.ErrNoCapacity
// reports the same condition. It is a capacity signal, not a defect. A 503
// without a gRPC cause, such as a full parking lot, a denied egress request,
// or a failed policy lookup, is RouterOutcomeUnavailable.
There was a problem hiding this comment.
Done, thanks. Used your wording.
Match the no_capacity brief.
Maya Wang (mayawang)
added
area/observability
kind/bug
|
Jeff Luo (@JeffLuoo) Tim Bai (@baizhenyu) PTAL when you get a chance. Thank you! |
Jeff Luo (JeffLuoo)
left a comment
Jeff Luo (JeffLuoo)
left a comment
There was a problem hiding this comment.
Thanks for the fix! Two requests:
- Please update the unavailable brief in https://github.com/agent-substrate/substrate/blob/main/docs/metrics/registry/metrics.yaml#L475. It now covers more than "a dependency was not available".
- #1896 also asks to change the "free worker" text in scheduling.go and workflow_resume. If that text and the parking lot change go in a follow-up, please change "Fixes #1896" to "Part of #1896".
| // The switch above already reports no_capacity for a wrapped | ||
| // ResourceExhausted cause, since status.Code unwraps through | ||
| // ReqError.Cause. A 503 that reaches here has no such cause: a | ||
| // full parking lot, a denied egress request, or a failed policy | ||
| // or credential lookup. None of these mean the fleet is full. | ||
| return ateattr.RouterOutcomeUnavailable |
There was a problem hiding this comment.
A request that the router sheds because the parking lot is full now always reports unavailable. But the router sheds a request only after its resume failed, often with ResourceExhausted (a full fleet). I think that request report no_capacity.
parkingFullErr needs to keep the resume error as its Cause. That change is in the ingress code, outside this PR, so I am OK with a follow-up PR. unavailable is a safer default than no_capacity until then.
I will update the parent issue for reflect it and can you please change "Fixes #1896" to "Part of #1896" in your PR description?
It also covers a 503 without a gRPC cause.
|
Thanks Jeff Luo (@JeffLuoo). Updated the unavailable brief and set the description to Part of #1896. PTAL when you get a chance. |
Jeff Luo (JeffLuoo)
changed the title
|
Thanks! I updated the title to make it clear on what we are changing in this PR. |
Part of #1896.
Only report no_capacity for a wrapped ResourceExhausted cause. Other 503s are unavailable.