Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ffeca63463
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a6ab5b69e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
This change is already incorporated on |
The placement error taxonomy classifies a no-candidate placement as FailedToPlaceSandboxError, but the error only listed the requirements (machine, labels, features), not why the fleet failed to meet them. That return path in placeSandbox was also the only one with no structured log line, so a 'no compatible node' failure left no trace in Loki. Count how many nodes each filter stage in sample() rejected (excluded, not-accepting, cpu-incompatible, feature-incompatible, label-filtered), thread the tally into FailedToPlaceSandboxError, and append it to the error message. Add a logger.L().Error at the no-eligible-node site with sandbox/template/build IDs and the full breakdown. The breakdown stays internal: placementAPIError returns a fixed 'no compatible node for this template's requirements' for this class and never calls err.Error(), so node counts and CPU details are not exposed to callers.
1468cfe to
884b23d
Compare
Fixes #3553
Rebased onto the placement error taxonomy (
placement/errors.go+orchestrator/placement_errors.go) that has since landed onmain. That taxonomy already classifies placement failures (timeout / no-capacity / no-compatible-node / unsupported-feature / create-failed), maps each to a semanticerror_codeand HTTP status, and forwards a curated, safe client message. This PR fills the two observability gaps it left, without touching what the client sees.What
FailedToPlaceSandboxErrornow says why the fleet had no candidate. Previously it only listed the requirements (machine, labels, features).sample()now counts how many nodes each filter stage discarded —excluded,not-accepting,cpu-incompatible,feature-incompatible,label-filtered— and the error appends that breakdown.placeSandboxnow logs. It was the only failure return with nologger.L()call (the create-failed and ResourceExhausted paths already log), so asandbox_no_compatible_nodefailure left no trace in structured logs. Added alogger.L().Errorwith sandbox/template/build IDs and the full breakdown.Why this does not re-open the topology-exposure concern
placementAPIErrorreturns a fixed"Failed to place sandbox: no compatible node for this template's requirements"for this class and never callserr.Error(). The rejection counts and CPU details live only in the internalErrfield, the OTel span, and the new log line — visible to operators, never to callers.Example log / internal error
Changes
placement/placement_best_of_K.go— addnodeRejectionCounts, tally per stage insample(), thread intoFailedToPlaceSandboxError, append breakdown inError()placement/placement.go— log at the no-eligible-node siteplacement/placement_best_of_K_test.go— cover the breakdown inError(), addsample()rejection-count test and end-to-endchooseNodepropagation testTesting
go test ./internal/orchestrator/placement/...passes (39 tests).go vetclean. Client-facingTestPlacementAPIErrorstill passes, confirming the breakdown does not leak intoClientMsg.