Fix validation result status transitions

[Rui Gao (hippogr)]

Summary

• emit failed validation results as error while keeping successful validation results as info
• explicitly query RecoverValidatedNodes by alert name for nodes in validating state
• transition successful validation to available_nodata for cluster-local-storage data synchronization
• transition failed validation to triaged_unknown to avoid repeated classification and OFR cycles
• record transitions using the relevant validation alert details and event timestamp

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

Alert-parser normally queries only severity: error records. Validation success is an informational event, so this PR keeps RecoverValidatedNodes at severity: info and performs a second, narrowly scoped query by alert name only for nodes currently in validating. This avoids scanning unrelated informational alerts while preserving correct severity semantics and compatibility with historical recovery alerts.

State transitions

Successful validation follows the existing data-safety workflow:

validating
  -> RecoverValidatedNodes(info)
  -> 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.

Failed validation follows:

validating
  -> CordonValidationFailedNodes(error)
  -> 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.

Review follow-ups

• validation transition reasons/details are derived only from the matching validation alert, not unrelated alerts in the same time window
• success and failure transitions use the validation alert event timestamp rather than the parser polling time
• the successful validation message now states that the node is ready for data synchronization, not that it has already been uncordoned

Validation

• alert-parser targeted tests: 11 passed
• notification JavaScript syntax check passed
• Kusto and PostgreSQL alert clients both support the forwarded alertname filter

[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.

[Rui Gao (hippogr)]

Implemented the review feedback in 715c864.

The final design keeps the alert severity semantics intact:

• CordonValidationFailedNodes remains error.
• RecoverValidatedNodes remains info because it represents a successful validation result.

Alert-parser keeps its existing error query and performs one additional, narrowly scoped query for RecoverValidatedNodes only when the node is currently validating. The query is still bounded by node and by the time since the node entered its current state, so it does not scan or return unrelated info alerts. The two result sets are merged and deduplicated. Both Kusto and PostgreSQL alert clients already support the alertname filter, so no SDK changes were required.

The transition handling was also tightened:

• reason/detail are generated only from the matching validation alert
• success and failure use the alert event timestamp instead of the parser polling timestamp
• the success message now says the node is ready for data synchronization rather than already uncordoned

The intended workflow remains:

success: validating -> available_nodata -> data sync -> uncordon -> available
failure: validating -> triaged_unknown (remains cordoned, avoids repeated classifier/OFR cycles)

Container-based targeted tests pass: 11 passed.

[Rui Gao (hippogr)]

Thanks for the update — this revision addresses my prior feedback well.

What I verified in this round:

• RecoverValidatedNodes is back to informational severity, while CordonValidationFailedNodes remains error for parser ingestion.
• Alert parser now explicitly fetches recovery alerts by alertname when the node is in validating, so success signals are consumed without forcing error severity.
• Transition timestamps now use the relevant alert event time for both validation failure and recovery branches.
• Added tests cover filtered reason/detail inputs, recovery/failure transition timestamps, merged alert fetching behavior, and alertname forwarding in alert util.

No further issues from my side in this revision.