Skip to content

Do not report a healthy Trino cell as unreachable - #1132

Merged
fuziontech merged 2 commits into
mainfrom
trino-node-endpoint-unavailable
Aug 27, 2026
Merged

Do not report a healthy Trino cell as unreachable#1132
fuziontech merged 2 commits into
mainfrom
trino-node-endpoint-unavailable

Conversation

@fuziontech

Copy link
Copy Markdown
Member

Fixes the admin console reporting a healthy Trino cell as unreachable:

The Trino coordinator did not answer. Provisioning state below still reflects the config store.
GET /v1/node: trino: query not found

Both halves of that are wrong. The coordinator did answer — with a 404 — and
nothing about a query was involved.

Why /v1/node 404s

Trino binds NodeResource (and therefore /v1/node) in exactly one place:

// NodeManagerModule
case AIRLIFT_DISCOVERY -> install(new AirliftNodeInventoryModule(nodeVersion));  // binds NodeResource
case ANNOUNCE          -> install(new AnnounceNodeInventoryModule());            // does not
case DNS               -> install(new DnsNodeInventoryModule());                 // does not

NodeInventoryConfig defaults discovery.type to ANNOUNCE, and the prod-US
coordinator config sets discovery.uri but never discovery.type, so it takes
that default. The route is not registered and 404 is the correct answer. This
has been true since #1126 shipped; it is not a regression from any Trino image.

What changed

404 and 410 are no longer the same error. They mean different things:
QueryResource throws GoneException — and only GoneException — for a query
that has aged out, so 410 keeps meaning "that query is gone", while 404 now
means "this coordinator does not serve that route".

A missing endpoint no longer marks the cell unavailable. handleStatus let
any node error set available=false. Now an unavailable endpoint leaves
availability alone; every other node error still counts against the cell.

The console can tell the difference. status carries node_stats, so the
UI can say "not reported here" rather than drawing a cell with zero nodes.
/trino/nodes answers 501 rather than 502, because the coordinator answered
and the request was for something this cell cannot provide.

What this does not do

Worker-level visibility is not recoverable for the observer principal as things
stand, and I did not try to force it. The OPA bundle grants
__duckgres_observer query metadata and no catalog at all, deliberately —
that is the stated bargain in policy.rego, and it is why system.runtime.nodes
is denied. Getting the panel back needs either discovery.type=AIRLIFT_DISCOVERY
on the cells or a widened observer grant. Both are real trade-offs and neither
belongs in a bug fix.

Related: the policy comment stating "/v1/node + /v1/resourceGroupState are
MANAGEMENT_READ" is accurate about the annotation, but on these cells that
grant has never had a route to apply to.

Verification

  • go vet -tags kubernetes ./controlplane/admin/ clean
  • go test -tags kubernetes ./controlplane/admin/ — the 12 failures present are
    identical with and without this change (verified against a stashed baseline);
    all are max_hot_idle_workers schema drift in the local test Postgres,
    unrelated to this code
  • go test -tags kubernetes ./controlplane/provisioner/... passes
  • UI tsc -b --noEmit clean; vitest run 160/160 across 26 files

New tests cover both halves: a 404 maps to endpoint-unavailable and not to
not-found, a 410 still maps to not-found and not to endpoint-unavailable, and a
cell whose /v1/node is missing still reports available=true with its query
counts intact.

The admin console shows "The Trino coordinator did not answer" against a
cell that is answering, with the detail "GET /v1/node: trino: query not
found". Both halves are wrong.

Trino binds NodeResource, and therefore /v1/node, only under
discovery.type=AIRLIFT_DISCOVERY. NodeInventoryConfig defaults that setting
to ANNOUNCE and our cells take the default, so the route does not exist and
the coordinator correctly answers 404. The console then reported a working
cell as a dead one, which points an operator at the cluster when nothing is
wrong with it.

Two causes, fixed separately.

The client mapped 404 and 410 to the same error. They are different: Trino
throws GoneException, and only GoneException, for a query that has aged out
of the coordinator, so 410 keeps meaning "that query is gone" while 404 now
means "this coordinator does not serve that route". The second is a
statement about how a cell was built, not about its health.

The status handler then let any node error set available=false. A cell that
never served /v1/node must not fail that way, so an unavailable endpoint now
leaves availability alone and reports node stats as absent. Every other node
error still counts against the cell.

status carries node_stats so the console can say "not reported here" instead
of drawing zero nodes, and /trino/nodes answers 501 rather than 502, because
the coordinator did answer and the request was for something this cell
cannot provide.

Worker-level visibility is not recoverable for the observer principal as
things stand. The OPA bundle grants it query metadata and no catalog at all,
by design, so system.runtime.nodes is denied and rightly so. Restoring the
panel needs either discovery.type=AIRLIFT_DISCOVERY on the cells or a
widened observer grant. Both are deliberate trades and neither belongs in
this fix.

The policy comment claiming "/v1/node + /v1/resourceGroupState are
MANAGEMENT_READ" is accurate about the annotation but that grant has never
had a route to apply to on these cells.
@fuziontech
fuziontech requested a review from a team August 27, 2026 21:37
@github-actions

github-actions Bot commented Aug 27, 2026

Copy link
Copy Markdown

Test Impact Plan

Deterministic summary of how this PR changes tests, CI runners, and coverage-risk signals.

Summary

Area Added Changed Deleted
Test files 0 2 0
E2E/journey files 0 0 0
Workflow files 0 0 0

Signals

  • Test cases: +3 / -0
  • Assertions: +14 / -0
  • Skips or known failures added: 0
  • Workflow continue-on-error added: 0
  • Workflow path filters added: 0
  • Test commands removed from justfile: 0
  • E2E/journey retry lines added: 0

Coverage risk: neutral or increased

No coverage-reduction warnings detected.

Splitting 404 away from 410 was too broad. /v1/query/{id} is always served,
so a 404 there is about the id and not the route: JAX-RS answers 404 when it
cannot convert a path parameter into a QueryId. Only the aged-out case
answers 410.

Without this, an operator following a malformed query link was told the
coordinator does not serve query lookups at all, and handleQueryDetail and
handleKillQuery lost the clean "query not found" they had before. The
per-query routes now re-read that 404 as a missing query.

Also render the distinction the previous commit only made available. The
cluster page drew "No nodes reported — the failure detector has not reported
any peers" for a cell that has no /v1/node to report from, which reads as an
empty fleet rather than as an endpoint this cell was never built with.
@fuziontech

Copy link
Copy Markdown
Member Author

Self-review before merge found two problems with the first commit; both are fixed in b0ffa7a.

1. Splitting 404 from 410 was too broad, and regressed the query routes.

/v1/query/{id} is always served, so a 404 there is about the id, not the route — JAX-RS answers 404 when it cannot convert a path parameter into a QueryId. Only the aged-out case answers 410.

The first commit sent that 404 to errTrinoEndpointUnavailable, so handleQueryDetail and handleKillQuery lost the clean "query not found" they had before and fell through to 502 "endpoint not served by this coordinator". An operator following a malformed query link would have been told the coordinator does not serve query lookups at all.

Notably the existing comment in handleQueryDetail"a stale link is an expected 404, not a cell problem" — shows the original author deliberately covered this case, and I narrowed it. The per-query routes now re-read a 404 as a missing query, with a test asserting both directions.

2. The UI change was claimed but not made.

The first commit added node_stats to the type and the API, and the PR text said the console "can say not reported here" — but nothing consumed the field. TrinoCluster would still have drawn "No nodes reported — the coordinator's failure detector has not reported any peers", which reads as an empty fleet rather than as an endpoint the cell was never built with. That page already has status, so it now renders the accurate message.

Verification after the fixes: go vet clean; the full admin suite shows the same 12 pre-existing failures as a stashed baseline and none new (all max_hot_idle_workers schema drift in the local test Postgres); UI tsc -b --noEmit clean; vitest run 160/160 across 26 files.

@fuziontech
fuziontech merged commit aa2ab61 into main Aug 27, 2026
28 checks passed
@fuziontech
fuziontech deleted the trino-node-endpoint-unavailable branch August 27, 2026 21:59
fuziontech added a commit that referenced this pull request Aug 27, 2026
…tually serves (#1133)

* List the fleet from the inventory the cell actually serves

The Trino cluster page reports "Nodes are not reported by this cell" against
prod-us. That copy is accurate — #1132 made it so — but it is not the end of
the story: the cell can name its workers, the console was just asking for
them at the one route this cell does not bind.

Trino binds exactly one node-listing route, chosen by discovery.type:

  AIRLIFT_DISCOVERY  /v1/node        heartbeat health, + /v1/node/failed
  ANNOUNCE (default) /v1/announce    the set of announced node URIs
  DNS                /v1/announce    same

AnnounceNodeInventoryModule and DnsNodeInventoryModule both bind
AnnounceNodeResource, so between the two routes every cell can list its
fleet; only the detail differs. Try /v1/node, fall back to /v1/announce on
the endpoint-unavailable error #1132 introduced.

Both routes are declared @ResourceSecurity(MANAGEMENT_READ), which maps to
checkCanReadSystemInformation, which __duckgres_observer already holds via
the ReadSystemInformation grant. No policy.rego change: the tenant-isolation
boundary is untouched, and the observer gains no catalog access.

The announce inventory carries membership and nothing else, so the source
travels with the data rather than being guessed downstream. Every heartbeat
field on an announced node is a zero meaning "not measured", which is not the
same as measured-and-zero: summarizing it would have the console report a
0.0 failure ratio and an "all healthy" fleet on the strength of numbers the
coordinator never sent. TrinoNodeInventory.HasHealth gates that, the status
payload carries node_source, and the page renders a membership-only table
plus a note on what is missing and why, instead of zero-filled health
columns. The Nodes stat card reads "membership only" rather than "all
healthy".

The remaining option, if per-node health on these cells is ever worth it, is
system.runtime.nodes — node_id, http_uri, node_version, coordinator, state,
so it would also close the version-skew gap. It costs the observer a scoped
catalog grant and a resource-group selector that admits it, both changes to
the isolation boundary, so it is deliberately not bundled here.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* Read the fleet from system.runtime.nodes, including worker version

The announce fallback names the workers but reports nothing about them, and
it cannot report the one thing a rollout needs: which version each node is
running. system.runtime.nodes carries node_id, http_uri, node_version,
coordinator and state, and the system connector is served by every cell
regardless of discovery.type. So the order is now

  /v1/node  ->  system.runtime.nodes  ->  /v1/announce

with the SQL path preferred over bare membership, and membership kept as the
last resort so a cell whose grant has not rolled out yet still lists a fleet.

This widens the observer, so the grant is written as narrowly as the OPA
operations allow: AccessCatalog on `system`, and SelectFromColumns pinned to
that one table. AccessCatalog opens nothing by itself -- every read still has
to pass SelectFromColumns -- so what stays denied is the part that matters:

  system.runtime.queries   tenant SQL text, unfiltered by query-owner
                           visibility (the REST path it already has is both
                           redacted and bounded by FilterViewQueryOwnedBy)
  system.metadata.*        enumerates every tenant catalog/schema/table/column
  system.jdbc.*            the same enumeration by another route
  org_<tenant>             unchanged; readable_catalog still excludes the
                           observer group, and this rule does not touch it

TestObserverSystemGrantIsPinnedToTheNodesTable pins each of those denials,
including a nodes table in another catalog and metadata browsing on `system`;
TestSystemNodesGrantIsObserverOnly pins that no tenant and not the admin gets
it, and that claiming the observer group without the observer username still
grants nothing. system.runtime.nodes itself contains no tenant identifier, so
the grant exposes no customer data even read in full.

The observer also needed its own resource-group lane. The last selector is
user `(?<org>.*)`, which matches anything, so without one the console's query
would have been admitted as a tenant into root.tenants.free.__duckgres_observer
-- and those leaves are JmxExport=true, so it would have shown up as a
phantom tenant in the per-tenant metrics added in #1124. Both operational
principals now select into the unexported admin tier ahead of the tenant
selectors.

The page gains Role / Version / State columns, and the Nodes card leads with
version skew when more than one version is present, since a stalled rollout
is the case worth seeing first. HasHealth stays false for this source: it
carries lifecycle state, not the failure detector's heartbeat ratios, and
conflating them would put a health badge on a number nothing measured.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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.

1 participant