diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index 74ed3242a..4956c257b 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -17,6 +17,23 @@ ## Purpose +PBS node-status collection must reject HTTP-success responses whose `data` +is omitted or null (including a null response envelope). Absent status is +unavailable telemetry, not measured zero usage: the poller retains independently +established connectivity, marks node metrics unavailable, and must not resolve +an active metric incident from that response. This does not add per-field +validation of populated status objects. + +Verification: `TestClient_GetNodeStatus_MissingData` in +`pkg/pbs/client_http_test.go` covers the absent envelopes. +`TestPBSPartialMetricsWebhookLifecycle` in +`internal/monitoring/monitor_pbs_webhook_test.go` exercises repeated null status +through the real poller, alert manager, notification queue and local webhook. +It requires online-but-unavailable projection, unchanged incident identity, +no false recovery history or delivery, then one identity-preserving recovery +after valid low-memory samples. These are local synthetic checks, not +installed-artifact or off-host destination qualification. + Direct PBS backup polling classifies typed API and authentication failures by the response status exposed by `pbs.HTTPStatus`, including wrapped errors. A 5xx gateway or server response remains transient even when its body quotes diff --git a/internal/monitoring/monitor_pbs_health_authority_test.go b/internal/monitoring/monitor_pbs_health_authority_test.go index dc603bafd..117be78f4 100644 --- a/internal/monitoring/monitor_pbs_health_authority_test.go +++ b/internal/monitoring/monitor_pbs_health_authority_test.go @@ -26,6 +26,7 @@ const ( pbsHealthTestNodeGatewayFailure pbsHealthTestUnavailable pbsHealthTestLowMemory + pbsHealthTestNullNodeStatus ) type pbsHealthTestServer struct { @@ -66,6 +67,10 @@ func newPBSHealthTestServer(t *testing.T) *pbsHealthTestServer { "data": map[string]any{"version": "3.4.2"}, }) case "/api2/json/nodes/localhost/status": + if mode == pbsHealthTestNullNodeStatus { + _, _ = w.Write([]byte(`{"data":null}`)) + return + } if mode == pbsHealthTestNodeDenied { http.Error(w, "permission denied", http.StatusForbidden) return diff --git a/internal/monitoring/monitor_pbs_webhook_test.go b/internal/monitoring/monitor_pbs_webhook_test.go index cb15df277..098ae51a8 100644 --- a/internal/monitoring/monitor_pbs_webhook_test.go +++ b/internal/monitoring/monitor_pbs_webhook_test.go @@ -98,11 +98,15 @@ func TestPBSPartialMetricsWebhookLifecycle(t *testing.T) { } incident := firing.Alerts[0] waitSent(1) - for _, mode := range []pbsHealthTestMode{pbsHealthTestNodeDenied, pbsHealthTestNodeGatewayFailure} { + for _, mode := range []pbsHealthTestMode{pbsHealthTestNodeDenied, pbsHealthTestNodeGatewayFailure, pbsHealthTestNullNodeStatus} { fixture.setMode(mode) for range 5 { poll() } + projection := pbsInstanceByName(t, monitor.state.GetSnapshot(), instance.Name) + if projection.Status != "online" || !projection.NodeMetricsUnavailable { + t.Fatalf("partial failure must retain connectivity without claiming metrics: %+v", projection) + } active := manager.GetActiveAlerts() if len(active) != 1 || active[0].ID != incident.ID || !active[0].StartTime.Equal(incident.StartTime) { t.Fatal("partial failure changed the active incident") diff --git a/pkg/pbs/client.go b/pkg/pbs/client.go index b45f705b3..ef9aba61e 100644 --- a/pkg/pbs/client.go +++ b/pkg/pbs/client.go @@ -771,14 +771,20 @@ func (c *Client) GetNodeStatus(ctx context.Context) (*NodeStatus, error) { log.Debug().Str("response", string(body)).Msg("PBS node status response") var statusResult struct { - Data NodeStatus `json:"data"` + Data *NodeStatus `json:"data"` } if err := json.Unmarshal(body, &statusResult); err != nil { return nil, fmt.Errorf("failed to decode status response: %w", err) } - return &statusResult.Data, nil + // A successful HTTP response is not evidence of available metrics. + // Missing/null data must not become zero usage and resolve active alerts. + if statusResult.Data == nil { + return nil, fmt.Errorf("node status response contains no data") + } + + return statusResult.Data, nil } // GetDatastores returns all datastores with their status. diff --git a/pkg/pbs/client_http_test.go b/pkg/pbs/client_http_test.go index c711befff..68eba0500 100644 --- a/pkg/pbs/client_http_test.go +++ b/pkg/pbs/client_http_test.go @@ -817,3 +817,18 @@ func TestClient_GetNodeName_ConcurrentTransientFailureIsSingleFlight(t *testing. t.Fatalf("/nodes hit %d times after recovery and cache read, want 2", got) } } +func TestClient_GetNodeStatus_MissingData(t *testing.T) { + for _, body := range []string{`{"data":null}`, `{}`, `null`} { + t.Run(body, func(t *testing.T) { + client, server := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(body)) + }) + defer server.Close() + status, err := client.GetNodeStatus(context.Background()) + if status != nil || err == nil { + t.Fatalf("absent metrics = (%+v, %v), want nil status and error", status, err) + } + }) + } +}