From 303154b4d22db3bb098fe6ab59eaa003d9167b0c Mon Sep 17 00:00:00 2001 From: James Greenhill Date: Thu, 27 Aug 2026 21:37:11 +0000 Subject: [PATCH 1/2] Do not report a healthy Trino cell as unreachable 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. --- controlplane/admin/trino.go | 26 ++++++++++++-- controlplane/admin/trino_client.go | 32 ++++++++++++++--- controlplane/admin/trino_client_test.go | 20 +++++++++++ controlplane/admin/trino_test.go | 34 +++++++++++++++++++ controlplane/admin/ui/src/hooks/useApi.ts | 1 + controlplane/admin/ui/src/lib/trino.test.ts | 1 + .../admin/ui/src/pages/TrinoQueries.test.tsx | 1 + controlplane/admin/ui/src/types/api.ts | 5 +++ 8 files changed, 114 insertions(+), 6 deletions(-) diff --git a/controlplane/admin/trino.go b/controlplane/admin/trino.go index 201fb325..21d95d88 100644 --- a/controlplane/admin/trino.go +++ b/controlplane/admin/trino.go @@ -96,8 +96,12 @@ type TrinoStatus struct { // than one that is busy. BlockedQueries int `json:"blocked_queries"` - Nodes int `json:"nodes"` - FailedNodes int `json:"failed_nodes"` + // NodeStats reports whether Nodes and FailedNodes mean anything. A cell + // using Trino's default discovery.type=ANNOUNCE does not serve /v1/node, + // so the console must show "not reported here" rather than zero nodes. + NodeStats bool `json:"node_stats"` + Nodes int `json:"nodes"` + FailedNodes int `json:"failed_nodes"` // OrgsByState counts Trino-enabled orgs by provisioning state, so a // stuck tenant is visible without opening the org list. @@ -307,13 +311,20 @@ func (a *TrinoAPI) handleStatus(c *gin.Context) { } } + // Node stats are best-effort. A cell built with the default + // discovery.type=ANNOUNCE does not serve /v1/node at all, and a healthy + // cell must not be reported as down because an endpoint it never had is + // missing. Any other error still counts against the cell. if nodes, nodeErr := a.nodes.get(ctx, a.client.Nodes); nodeErr == nil { + status.NodeStats = true status.Nodes = len(nodes) for _, n := range nodes { if n.Failed { status.FailedNodes++ } } + } else if isTrinoEndpointUnavailable(nodeErr) { + status.NodeStats = false } else if status.Error == "" { status.Available = false status.Error = nodeErr.Error() @@ -477,6 +488,17 @@ func (a *TrinoAPI) handleKillQuery(c *gin.Context) { func (a *TrinoAPI) handleNodes(c *gin.Context) { nodes, err := a.nodes.get(c.Request.Context(), a.client.Nodes) + if isTrinoEndpointUnavailable(err) { + // Not a gateway failure: the coordinator answered, and it does not + // serve this route. 501 says the console asked for something this + // cell cannot provide, which is what an operator needs to know. + c.JSON(http.StatusNotImplemented, gin.H{ + "cell": a.cell, + "available": false, + "reason": "this cell does not serve /v1/node; Trino binds it only under discovery.type=AIRLIFT_DISCOVERY", + }) + return + } if err != nil { c.JSON(http.StatusBadGateway, gin.H{"error": err.Error(), "available": false}) return diff --git a/controlplane/admin/trino_client.go b/controlplane/admin/trino_client.go index f4d54f33..ef809dc5 100644 --- a/controlplane/admin/trino_client.go +++ b/controlplane/admin/trino_client.go @@ -50,6 +50,21 @@ var errTrinoNotFound = errors.New("trino: query not found") func isTrinoNotFound(err error) bool { return errors.Is(err, errTrinoNotFound) } +// errTrinoEndpointUnavailable is returned when the coordinator answers but +// does not serve the route at all. That is a statement about how the cell is +// built, not about its health, and it is not the same incident as a +// coordinator that never answered. +// +// /v1/node is the case this exists for. Trino binds NodeResource only under +// discovery.type=AIRLIFT_DISCOVERY; the default is ANNOUNCE, which these +// cells use, so the route is absent and the coordinator correctly answers +// 404. Reporting that as a dead cell hides the real state of a healthy one. +var errTrinoEndpointUnavailable = errors.New("trino: endpoint not served by this coordinator") + +func isTrinoEndpointUnavailable(err error) bool { + return errors.Is(err, errTrinoEndpointUnavailable) +} + // TrinoQuery is one query as the console shows it: identity, lifecycle and // the cost counters an operator triages on. // @@ -194,9 +209,16 @@ func NewTrinoCoordinatorClient(baseURL, tlsServerName string, creds TrinoCredent } // do issues one authenticated request and returns the body. Non-2xx is an -// error carrying the status, except 410 Gone, which maps to -// errTrinoNotFound so a stale query link reads as "gone" rather than as a -// broken cell. +// error carrying the status, except two cases that are not cell failures: +// +// 410 Gone the query is no longer held -> errTrinoNotFound, so a +// stale query link reads as "gone" rather than as a broken +// cell. QueryResource throws GoneException for this, and +// only for this. +// 404 Not Found the coordinator does not serve the route at all -> +// errTrinoEndpointUnavailable, so an endpoint this cell was +// never built with reads as unavailable rather than as +// silence from the coordinator. func (c *trinoCoordinatorHTTPClient) do(ctx context.Context, method, path string, body io.Reader) ([]byte, error) { req, err := http.NewRequestWithContext(ctx, method, c.baseURL+path, body) if err != nil { @@ -215,8 +237,10 @@ func (c *trinoCoordinatorHTTPClient) do(ctx context.Context, method, path string defer func() { _ = resp.Body.Close() }() raw, readErr := io.ReadAll(resp.Body) switch { - case resp.StatusCode == http.StatusGone, resp.StatusCode == http.StatusNotFound: + case resp.StatusCode == http.StatusGone: return nil, fmt.Errorf("%s %s: %w", method, path, errTrinoNotFound) + case resp.StatusCode == http.StatusNotFound: + return nil, fmt.Errorf("%s %s: %w", method, path, errTrinoEndpointUnavailable) case resp.StatusCode == http.StatusForbidden: // Worth its own message: this is what a missing observer grant or // an un-rolled-out OPA bundle looks like, and it is fixed in a diff --git a/controlplane/admin/trino_client_test.go b/controlplane/admin/trino_client_test.go index 657af6a8..c960289e 100644 --- a/controlplane/admin/trino_client_test.go +++ b/controlplane/admin/trino_client_test.go @@ -291,6 +291,26 @@ func TestCoordinatorClientGoneQueryIsNotFound(t *testing.T) { if !isTrinoNotFound(err) { t.Errorf("410 Gone should map to a not-found error, got %v", err) } + if isTrinoEndpointUnavailable(err) { + t.Error("410 Gone is a missing query, not a missing endpoint") + } +} + +// TestCoordinatorClientMissingRouteIsEndpointUnavailable: a coordinator +// built with Trino's default discovery.type=ANNOUNCE does not bind +// NodeResource, so /v1/node answers 404. The cell is healthy and must not be +// reported as one that never answered. +func TestCoordinatorClientMissingRouteIsEndpointUnavailable(t *testing.T) { + c, _ := newTrinoTestCoordinator(t, func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + }) + _, err := c.Nodes(context.Background()) + if !isTrinoEndpointUnavailable(err) { + t.Errorf("404 should map to an endpoint-unavailable error, got %v", err) + } + if isTrinoNotFound(err) { + t.Error("a missing route must not read as a missing query") + } } // TestCoordinatorClientForbiddenIsSurfaced: a 403 means the observer's OPA diff --git a/controlplane/admin/trino_test.go b/controlplane/admin/trino_test.go index 1c22e0d0..75481ca0 100644 --- a/controlplane/admin/trino_test.go +++ b/controlplane/admin/trino_test.go @@ -313,6 +313,40 @@ func TestStatusReportsProvisioningWhenTheCoordinatorIsDown(t *testing.T) { } } +// TestStatusStaysAvailableWhenTheCellDoesNotServeNodes is the bug this +// distinction exists for. Trino binds NodeResource only under +// discovery.type=AIRLIFT_DISCOVERY, and these cells run the default +// ANNOUNCE, so /v1/node answers 404 on a perfectly healthy coordinator. +// Before this, that 404 set available=false and the console reported a +// working cell as one that never answered. +func TestStatusStaysAvailableWhenTheCellDoesNotServeNodes(t *testing.T) { + coord := &fakeTrinoCoordinator{ + info: &TrinoServerInfo{Version: "484", Environment: "production"}, + queries: []TrinoQuery{{QueryID: "q1", State: "RUNNING", Principal: "db_a"}}, + nodesErr: fmt.Errorf("GET /v1/node: %w", errTrinoEndpointUnavailable), + } + r := trinoTestRouter(testTrinoAPI(t, coord, twoOrgTrinoStore()), RoleViewer) + + code, body := doTrinoJSON(t, r, http.MethodGet, "/api/v1/trino/status", "") + if code != http.StatusOK { + t.Fatalf("expected 200, got %d", code) + } + if body["available"] != true { + t.Errorf("a cell that does not serve /v1/node is still available, body: %v", body) + } + if s, _ := body["error"].(string); s != "" { + t.Errorf("a missing endpoint is not a cell error, got %q", s) + } + if body["node_stats"] != false { + t.Error("node_stats must be false so the console shows 'not reported' rather than zero nodes") + } + // The rest of the cell must still be reported. + states := body["queries_by_state"].(map[string]any) + if states["RUNNING"] != float64(1) { + t.Errorf("queries must still be counted, got %v", states) + } +} + func TestStatusCountsQueriesAndNodes(t *testing.T) { coord := &fakeTrinoCoordinator{ info: &TrinoServerInfo{Version: "484", Environment: "production", UptimeMS: 3600000}, diff --git a/controlplane/admin/ui/src/hooks/useApi.ts b/controlplane/admin/ui/src/hooks/useApi.ts index f73128de..3dccc07f 100644 --- a/controlplane/admin/ui/src/hooks/useApi.ts +++ b/controlplane/admin/ui/src/hooks/useApi.ts @@ -787,6 +787,7 @@ export function useTrinoStatus() { available: false, queries_by_state: {}, blocked_queries: 0, + node_stats: true, nodes: 0, failed_nodes: 0, orgs_by_state: {}, diff --git a/controlplane/admin/ui/src/lib/trino.test.ts b/controlplane/admin/ui/src/lib/trino.test.ts index 120f7eba..9d5dbeee 100644 --- a/controlplane/admin/ui/src/lib/trino.test.ts +++ b/controlplane/admin/ui/src/lib/trino.test.ts @@ -59,6 +59,7 @@ function status(over: Partial = {}): TrinoStatus { available: true, queries_by_state: {}, blocked_queries: 0, + node_stats: true, nodes: 0, failed_nodes: 0, orgs_by_state: {}, diff --git a/controlplane/admin/ui/src/pages/TrinoQueries.test.tsx b/controlplane/admin/ui/src/pages/TrinoQueries.test.tsx index 4d974a00..e3bb9a59 100644 --- a/controlplane/admin/ui/src/pages/TrinoQueries.test.tsx +++ b/controlplane/admin/ui/src/pages/TrinoQueries.test.tsx @@ -58,6 +58,7 @@ function status(over: Partial = {}): TrinoStatus { available: true, queries_by_state: {}, blocked_queries: 0, + node_stats: true, nodes: 2, failed_nodes: 0, orgs_by_state: {}, diff --git a/controlplane/admin/ui/src/types/api.ts b/controlplane/admin/ui/src/types/api.ts index 3d5deb4d..654dc135 100644 --- a/controlplane/admin/ui/src/types/api.ts +++ b/controlplane/admin/ui/src/types/api.ts @@ -836,6 +836,11 @@ export interface TrinoStatus { server?: TrinoServerInfo; queries_by_state: Record; blocked_queries: number; + // node_stats=false means this cell does not report nodes at all, so nodes + // and failed_nodes are both zero and mean nothing. Trino serves /v1/node + // only under discovery.type=AIRLIFT_DISCOVERY; these cells run the default + // ANNOUNCE. Render "not reported" rather than zero nodes. + node_stats: boolean; nodes: number; failed_nodes: number; orgs_by_state: Record; From b0ffa7a56c7af279f0525c17e9a191187c430bd2 Mon Sep 17 00:00:00 2001 From: James Greenhill Date: Thu, 27 Aug 2026 21:42:53 +0000 Subject: [PATCH 2/2] Keep a 404 on the per-query routes meaning a missing query MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- controlplane/admin/trino_client.go | 19 ++++++++++++++-- controlplane/admin/trino_client_test.go | 22 +++++++++++++++++++ .../admin/ui/src/pages/TrinoCluster.tsx | 8 +++++++ 3 files changed, 47 insertions(+), 2 deletions(-) diff --git a/controlplane/admin/trino_client.go b/controlplane/admin/trino_client.go index ef809dc5..282f6a0a 100644 --- a/controlplane/admin/trino_client.go +++ b/controlplane/admin/trino_client.go @@ -65,6 +65,21 @@ func isTrinoEndpointUnavailable(err error) bool { return errors.Is(err, errTrinoEndpointUnavailable) } +// queryRouteNotFound re-reads a 404 for the per-query routes as a missing +// query rather than a missing route. +// +// /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 the path parameter into a +// QueryId. Only the aged-out case answers 410, so without this an operator +// following a malformed link would be told the coordinator does not serve +// query lookups at all. +func queryRouteNotFound(err error) error { + if isTrinoEndpointUnavailable(err) { + return fmt.Errorf("%w", errTrinoNotFound) + } + return err +} + // TrinoQuery is one query as the console shows it: identity, lifecycle and // the cost counters an operator triages on. // @@ -376,7 +391,7 @@ func (c *trinoCoordinatorHTTPClient) Queries(ctx context.Context) ([]TrinoQuery, func (c *trinoCoordinatorHTTPClient) Query(ctx context.Context, queryID string) (*TrinoQuery, error) { raw, err := c.do(ctx, http.MethodGet, "/v1/query/"+url.PathEscape(queryID)+"?pruned=true", nil) if err != nil { - return nil, err + return nil, queryRouteNotFound(err) } var wire trinoBasicQueryInfo if err := json.Unmarshal(raw, &wire); err != nil { @@ -396,7 +411,7 @@ func (c *trinoCoordinatorHTTPClient) KillQuery(ctx context.Context, queryID, mes ctx, cancel := context.WithTimeout(ctx, trinoKillTimeout) defer cancel() _, err := c.do(ctx, http.MethodPut, "/v1/query/"+url.PathEscape(queryID)+"/killed", strings.NewReader(message)) - return err + return queryRouteNotFound(err) } type trinoNodeStats struct { diff --git a/controlplane/admin/trino_client_test.go b/controlplane/admin/trino_client_test.go index c960289e..3d4045a8 100644 --- a/controlplane/admin/trino_client_test.go +++ b/controlplane/admin/trino_client_test.go @@ -313,6 +313,28 @@ func TestCoordinatorClientMissingRouteIsEndpointUnavailable(t *testing.T) { } } +// TestCoordinatorClientQueryRoute404StaysNotFound guards the per-query +// routes. /v1/query/{id} is always served, so a 404 there is about the id — +// JAX-RS answers 404 when it cannot parse one into a QueryId. An operator +// following a malformed link must be told the query is missing, not that the +// coordinator does not serve query lookups. +func TestCoordinatorClientQueryRoute404StaysNotFound(t *testing.T) { + c, _ := newTrinoTestCoordinator(t, func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + }) + _, err := c.Query(context.Background(), "not a query id") + if !isTrinoNotFound(err) { + t.Errorf("404 on the query route should read as a missing query, got %v", err) + } + if isTrinoEndpointUnavailable(err) { + t.Error("the query route exists; a 404 there is not a missing endpoint") + } + + if killErr := c.KillQuery(context.Background(), "not a query id", "why"); !isTrinoNotFound(killErr) { + t.Errorf("404 on kill should read as a missing query, got %v", killErr) + } +} + // TestCoordinatorClientForbiddenIsSurfaced: a 403 means the observer's OPA // grant is missing or the bundle has not rolled out. That is a distinct, // actionable failure and must not be flattened into "no queries". diff --git a/controlplane/admin/ui/src/pages/TrinoCluster.tsx b/controlplane/admin/ui/src/pages/TrinoCluster.tsx index 75c3c9b4..94ba7b7b 100644 --- a/controlplane/admin/ui/src/pages/TrinoCluster.tsx +++ b/controlplane/admin/ui/src/pages/TrinoCluster.tsx @@ -181,6 +181,14 @@ export function TrinoCluster() { {nodes.isLoading ? ( + ) : status.data && !status.data.node_stats ? ( + // Not the same as an empty fleet. Trino serves /v1/node only + // under discovery.type=AIRLIFT_DISCOVERY, and a cell on the + // default ANNOUNCE has no such endpoint to answer with. + ) : (nodes.data?.nodes ?? []).length === 0 ? (