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..282f6a0a 100644 --- a/controlplane/admin/trino_client.go +++ b/controlplane/admin/trino_client.go @@ -50,6 +50,36 @@ 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) +} + +// 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. // @@ -194,9 +224,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 +252,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 @@ -352,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 { @@ -372,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 657af6a8..3d4045a8 100644 --- a/controlplane/admin/trino_client_test.go +++ b/controlplane/admin/trino_client_test.go @@ -291,6 +291,48 @@ 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") + } +} + +// 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 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/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 ? ( = {}): 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;