Fix validation result status transitions

[Rui Gao (hippogr)]

Summary

• mark validation success and failure result alerts as error so the existing alert-parser query consumes them
• transition failed validation nodes from validating to triaged_unknown instead of cordoned
• add coverage for successful and failed validation state transitions

Background

The node recycler submits a SuperBench validation job and sets the node status to validating. After the job completes, job-status-change-notification parses the container log and emits one of these alerts:

• RecoverValidatedNodes when "diagnosis/accept": true is present
• CordonValidationFailedNodes when validation fails

Both alerts were emitted with severity: info, while alert-parser intentionally queries only severity: error records. As a result, alert-parser could not observe either validation result and nodes remained stuck in validating. This also prevented successful nodes from entering available_nodata, so cluster-local-storage could not copy local data and perform the final uncordon.

Design

For successful validation, the expected flow remains:

validating
  -> RecoverValidatedNodes
  -> available_nodata
  -> cluster-local-storage data sync
  -> uncordon
  -> available

This PR does not restore the old direct RecoverValidatedNodes -> uncordon Alertmanager route. The node remains cordoned until cluster-local-storage finishes copying data.

For failed validation, the node now transitions to triaged_unknown rather than cordoned:

validating
  -> CordonValidationFailedNodes
  -> triaged_unknown

The Kubernetes node is already cordoned during validation. Returning the Kusto state to cordoned would cause node-issue-classifier to classify the validation failure again. Hardware validation failures could then enter triaged_hardware and trigger another OFR cycle. triaged_unknown preserves the cordon, records the validation failure, and stops automatic reprocessing until manual investigation.

Validation

• alert-parser targeted tests: 6 passed
• notification JavaScript syntax check passed

[Copilot]

Pull request overview

This PR fixes the “validation result ingestion” pipeline so that alert-parser can observe validation success/failure alerts, and updates the resulting node status transitions to prevent reclassification loops after validation failures.

Changes:

• Emit RecoverValidatedNodes and CordonValidationFailedNodes alerts with severity: error so alert-parser’s existing query can consume them.
• Change failed validation state transition from validating -> cordoned to validating -> triaged_unknown.
• Extend alert-parser unit coverage for validation success (available_nodata) and failure (triaged_unknown) transitions.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/alert-manager/src/job-status-change-notification/controllers/alert.js	Changes validation result alerts to severity: error so alert-parser can ingest them.
src/alert-manager/src/alert-parser/node_alert_monitor.py	Updates validation-failure transition target state to triaged_unknown.
src/alert-manager/src/alert-parser/tests/test_alert_monitor.py	Adds coverage for validation success/failure transitions from validating.

Suppressed comments (1)

src/alert-manager/src/job-status-change-notification/controllers/alert.js:94

• The RecoverValidatedNodes alert summary says the node "will be uncordoned", but alert-parser now consumes this alert to move the node into available_nodata (still cordoned until data sync/un-cordon later). This message is misleading for operators and downstream consumers.

      severity: "error",
      node_name: node,
    },
    annotations: {
      summary: `The node ${node} has been validated and be uncordoned.`,

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Rui Gao (hippogr)]

Thanks for the fix — the core direction makes sense, especially the transition from validating to triaged_unknown and the matching test updates. I have a few follow-up suggestions:\n\n1) Severity semantics for RecoverValidatedNodes\nRecoverValidatedNodes represents a successful validation/uncordon path, but this PR changes its severity from info to error in alert.js. That may create noisy/error-classified alerts for success events.\n\nSuggestion: keep RecoverValidatedNodes as info, and make alert-parser robust to both info/error for this alertname (or filter by alertname instead of strict severity where appropriate).\n\n2) Timestamp consistency for state transition\nFor CordonValidationFailedNodes, transition time uses the alert event timestamp, but RecoverValidatedNodes still uses the polling timestamp. In delayed ingestion scenarios, this can cause status timeline skew.\n\nSuggestion: for RecoverValidatedNodes, also use the alert event timestamp (similar to validation failure branch) when calling update_status_action.\n\n3) Missing contract test around alert payload\nThe parser behavior depends on alert payload fields (alertname/severity/status), but there is no focused test around job-status-change-notification alert payload generation for these validation alerts.\n\nSuggestion: add a small unit test for alert.js to lock down alertname + severity + status for cordon/uncordon payloads and prevent regressions.\n\nOverall: logic fix looks good; with the above adjustments the change should be safer and easier to maintain.