Skip to content

[CI] Extend withHealthyNode to blacklist k8s hosts #2446

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
leo-automation wants to merge 1 commit into
base: develop
Choose a base branch
from
+47 −8
Open

[CI] Extend withHealthyNode to blacklist k8s hosts #2446

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 47 additions & 8 deletions mlir/utils/jenkins/Jenkinsfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@ import java.util.concurrent.ConcurrentHashMap
// one instance for the whole run
@Field
ConcurrentHashMap<String,String> DOCKER_ARGS_BY_NODE = new ConcurrentHashMap<>()
// Physical Kubernetes nodes rejected by any withHealthyNode invocation in this build.
// Jenkins agent names are ephemeral pod names, so tracking only those can let a
// replacement pod land on the same unhealthy host.
@Field
ConcurrentHashMap<String,String> BLACKLISTED_K8S_NODES = new ConcurrentHashMap<>()
// Jenkins Git plugin defaults to 10 minutes per command. Use 2h for fetches and
// checkouts that can exceed that on a slow network.
@Field
Expand Down Expand Up @@ -1312,11 +1317,28 @@ void produceCoverageHtml(String cov, String cpath) {
"""
}

String currentKubernetesNode() {
// Jenkins Kubernetes templates expose this through a Pod Environment
// Variable whose field path is spec.nodeName.
return env.K8S_NODE_NAME?.trim()
}

void blacklistKubernetesNode(String k8sNode, String agentName) {
if (!k8sNode) return

String firstRejectedAgent = BLACKLISTED_K8S_NODES.putIfAbsent(k8sNode, agentName)
if (firstRejectedAgent) {
echo "[withHealthyNode] Kubernetes node ${k8sNode} was already blacklisted by ${firstRejectedAgent}."
} else {
echo "[withHealthyNode] Blacklisted Kubernetes node ${k8sNode} for the remainder of this build."
}
}

// Run the body on a node that passes the supplied healthChecks() block
// The health check is retried on fresh executors; the body is not retried.
// This function also retries the main 'body' if it fails due to a recoverable node-related issue (e.g., agent disconnect).
def withHealthyNode(String baseLabel, Closure<?> healthChecks, Closure<?> body, int maxAttempts = 3) {
def blacklist = [] // nodes and pods that already failed the check
def blacklist = [] // Jenkins nodes and pods that already failed the check
int attempt = 0
boolean done = false

Expand All @@ -1329,9 +1351,23 @@ def withHealthyNode(String baseLabel, Closure<?> healthChecks, Closure<?> body,

echo "[withHealthyNode] attempt #${attempt}: looking for '${expr}'"
node(expr.toString()) {
String agentName = env.NODE_NAME
String k8sNode = currentKubernetesNode()

if (agentName?.startsWith('k8s-') && !k8sNode) {

@rocmlir-pr-reviewer rocmlir-pr-reviewer Bot Aug 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kubernetes-agent detection hinges on an undocumented k8s- pod-name prefix. If a pod template in the frameworks-devops-k8s-cluster cloud is renamed, this warning silently stops firing and the loss of physical-node blacklisting becomes invisible — exactly the failure mode the warning exists to surface. Suggest hoisting the prefix into a named @Field constant next to BLACKLISTED_K8S_NODES (line 17) with a comment naming the Jenkins cloud whose templates it must track, so the coupling is discoverable from one place. Minor secondary point: agentName is assigned from env.NODE_NAME inside a node {} block, where it is always set, so the ?. safe-navigation here is dead defensiveness and can be a plain .startsWith(...).

echo "[withHealthyNode] WARNING: ${agentName} does not expose K8S_NODE_NAME; physical-node blacklisting is disabled for this agent."
}

String rejectedByAgent = k8sNode ? BLACKLISTED_K8S_NODES[k8sNode] : null
if (rejectedByAgent) {
echo "[withHealthyNode] Rejecting ${agentName}: Kubernetes node ${k8sNode} was blacklisted by ${rejectedByAgent}."
blacklist << agentName
return

@rocmlir-pr-reviewer rocmlir-pr-reviewer Bot Aug 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This early return exits the node {} block after attempt += 1 has already run at line 1346, so a pod rejected purely because it landed on an already-blacklisted physical host burns one of the three maxAttempts without ever executing a health check. Adding the ephemeral pod name to blacklist does not prevent the next pod from being scheduled onto the same host, so the collision can repeat: because the map is now shared across parallel matrix rows, a row that has failed nothing itself can consume all three attempts on scheduling collisions and then hit error "No healthy node found..." at line 1440 — escalating one unhealthy host into a whole-job re-kick (or a red PR build). Suggest not charging scheduling rejections to the health-check budget: track them in a separate counter with its own cap (e.g. rescheduleAttempts < 2 * maxAttempts) and decrement attempt on this path, so the loop still terminates but the health-check budget is preserved for actual health checks.

}

// Retry ONLY the health-check. We don't want to retry the actual stages
try {
stage("Health checks on ${env.NODE_NAME}") {
stage("Health checks on ${agentName}") {
echo 'Cleaning up old Docker images...'
def pruneStatus = sh(script: 'docker image prune -af --filter "until=720h"', returnStatus: true)
if (pruneStatus != 0) {
Expand All @@ -1341,14 +1377,16 @@ def withHealthyNode(String baseLabel, Closure<?> healthChecks, Closure<?> body,
gitHealthCheck()
}
} catch (Exception err) {
echo "[withHealthyNode] ❌ ${env.NODE_NAME} rejected: ${err}"
blacklist << env.NODE_NAME
echo "[withHealthyNode] ❌ ${agentName} rejected: ${err}"
blacklist << agentName
blacklistKubernetesNode(k8sNode, agentName)

@rocmlir-pr-reviewer rocmlir-pr-reviewer Bot Aug 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This records the physical host on any health-check exception, but the health-check block is not purely node-scoped: gitHealthCheck() (line 64) probes the SCM remote over the network and errors on auth/connectivity problems, and healthChecks() is caller-supplied. Previously such a failure only excluded one ephemeral agent from one row's retry loop; now it permanently removes the underlying physical node from the pool for every parallel row for the remainder of the build. During a transient GitHub or network blip every row's health check fails, and the shared map can blacklist the entire pool, converting a recoverable blip into "No healthy node found" for the whole job — the opposite of this PR's goal. Suggest either recording the physical node only for failures that are demonstrably node-scoped (e.g. reuse isPerServerTransient("${err}") to gate this call, as is already done on the body path at line 1411), or capping BLACKLISTED_K8S_NODES.size() and skipping further physical-node blacklisting once the cap is reached.

// return exits the node {} block here, not the whole function. Some groovy magic
return
}
stage("Node selected") {
// Health-check passed. Do real work
echo "[withHealthyNode] ✅ using ${env.NODE_NAME}"
String k8sNodeSuffix = k8sNode ? " on Kubernetes node ${k8sNode}" : ''
echo "[withHealthyNode] ✅ using ${agentName}${k8sNodeSuffix}"
}
// Per-row console log: shStrict mirrors output here so we can classify transient
// failures (e.g. GPU hang) that only appear in stdout, not in the thrown exception.
Expand All @@ -1374,13 +1412,14 @@ def withHealthyNode(String baseLabel, Closure<?> healthChecks, Closure<?> body,
// Group-1 transient on this node: blacklist it and retry the same arch on a
// fresh node. The while loop continues (done still false); if attempts run out
// this becomes "no healthy node found", which the post-block re-kicks whole-job.
echo "[withHealthyNode] Per-server transient on ${env.NODE_NAME}. Blacklisting the node and retrying.."
echo "[withHealthyNode] Per-server transient on ${agentName}. Blacklisting the node and retrying.."
echo "[withHealthyNode] Error was: ${err}"
blacklist << env.NODE_NAME
blacklist << agentName
blacklistKubernetesNode(k8sNode, agentName)
return
}
// Real failure (or a whole-job transient like no-healthy-node): fail immediately.
echo "[withHealthyNode] Execution failed with a non-recoverable error on ${env.NODE_NAME}"
echo "[withHealthyNode] Execution failed with a non-recoverable error on ${agentName}"
echo "[withHealthyNode] Error was: ${err}"
throw err
} finally {
Expand Down
Loading