fix(spur-k8s): give SPUR_PEER_NODES resolvable pod names - #807
Conversation
In Pod mode every Kubernetes node registers the one operator address as its agent address, so the peer list the controller dispatches held that address repeated once per node. SPUR_PEER_NODES therefore read "<operator-ip>:6818,<operator-ip>:6818,<operator-ip>:6818" and no distributed workload could reach a peer with it. A single node job showed the same value, so it was never usable. The Pods of a multi-node job already resolve each other: launch_job sets the Pod hostname to the target node and the subdomain to the job's headless Service. SPUR_PEER_NODES now carries those names, in nodelist order, so index N is the peer whose SPUR_NODE_RANK is N. A single node job has no headless Service and therefore no name to publish, so the variable is left unset instead of holding an address that resolves to nothing. Also correct the message the operator returns when no SpurJob carries the job id. It said "agent unreachable", although the operator is running and reachable; it now says that Pod mode launches only a SpurJob, and that a job from sbatch or spur submit has nothing to launch.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #807 +/- ##
==========================================
- Coverage 80.15% 80.15% -0.00%
==========================================
Files 184 184
Lines 87772 87809 +37
==========================================
+ Hits 70348 70376 +28
- Misses 17424 17433 +9 🚀 New features to boost your workflow:
|
shiv-tyagi
left a comment
shiv-tyagi
left a comment
There was a problem hiding this comment.
The DNS approach is right and the tests are solid. A few small things: one duplication nit around the service name, one missing test for the trim path, and a question about sanitize collisions.
| .iter() | ||
| .map(|n| { | ||
| format!( | ||
| "{}.spur-job-{}.{}.svc.cluster.local", |
There was a problem hiding this comment.
The spur-job-<id> name is spelled out in 7 spots now (176, 452, 578, 622, 716, 860, and here). The DNS built here only resolves as long as this matches the Service name at 860 and the subdomain at 452. That is easy to break later.
Pull the service name into one helper and build off it:
| "{}.spur-job-{}.{}.svc.cluster.local", | |
| "{}.{}.{}.svc.cluster.local", | |
| sanitize_k8s_name(n), | |
| job_service_name(job_id), | |
| namespace |
Where job_service_name(job_id) returns format!("spur-job-{job_id}"), reused at 452, 578, 860 too.
There was a problem hiding this comment.
Done in 3bc26d5: job_service_name builds the Service name, and the Pod subdomain, the Service create and delete, and the peer DNS all use it. Pod names stay literal, they are not the Service name.
| fn node_names_are_sanitized_the_same_way_as_the_pod_hostname() { | ||
| let out = headless_peer_dns("Node_A.example,node-b", 3, "ns"); | ||
| assert_eq!(out[0], "node-a-example.spur-job-3.ns.svc.cluster.local"); | ||
| } |
There was a problem hiding this comment.
None of these cover a trailing comma or padding. That path matters. "node-a," splits into two parts and the empty one only gets dropped by the filter. Without it you would emit a junk FQDN and still pass the len check.
Add a case that locks it in:
#[test]
fn segments_are_trimmed_and_empty_ones_dropped() {
let out = headless_peer_dns(" node-a , node-b, ", 7, "spur");
assert_eq!(
out,
vec![
"node-a.spur-job-7.spur.svc.cluster.local".to_string(),
"node-b.spur-job-7.spur.svc.cluster.local".to_string(),
]
);
}There was a problem hiding this comment.
Added as segments_are_trimmed_and_empty_ones_dropped in da90773.
| .map(|n| { | ||
| format!( | ||
| "{}.spur-job-{}.{}.svc.cluster.local", | ||
| sanitize_k8s_name(n), |
There was a problem hiding this comment.
Q: two node names that sanitize to the same string (say node_a and node-a) now collide here and on the pod name at 180. The pod one comes back as a 409 which launch_job counts as success, so a pod just never shows up and the peer DNS points at nothing. This PR leans on the sanitize for DNS now, so the collision surface is wider. Is that worth guarding, or are node names already constrained upstream?
There was a problem hiding this comment.
Not constrained upstream, register_agent accepts any hostname. K8s node names are DNS-1123 already, so only a dotted and a dashed variant (a.b, a-b) can collide. Since this PR widens the surface, 6accf17 refuses a nodelist in which two names sanitize alike, before any Pod is created, with an invalid_argument that names both. The 409-as-success path is a separate bug, filed as #873.
Two node names that sanitize to the same Kubernetes name, such as "node.a" and "node-a", give two Pods the same name and two peers the same DNS name. The second Pod create returns 409, which launch_job counts as success, so a peer is missing and the job hangs. Check the nodelist once before the Pod is created and refuse the whole launch with an invalid argument that names both nodes and the shared sanitized name. A partial launch is worse than a refused one. The check and headless_peer_dns read the nodelist with one shared helper so the two cannot disagree. A single node cannot collide, so that path is unchanged.
3 participants
Related:
Motivation
In Pod mode
SPUR_PEER_NODESis unusable, so no distributed workload can start.Every Kubernetes node registers the single operator address as its agent address.
The peer list the controller dispatches therefore holds that one address repeated
once per node, and the variable reads
"<operator-ip>:6818,<operator-ip>:6818,<operator-ip>:6818". An MPI or torchrunlauncher that reads it addresses the operator three times and never reaches a
peer. A single node job shows the same value, so it was never usable in Pod mode.
Technical Details
The Pods of a multi-node job already resolve each other.
launch_jobsets the Podhostname to the target node name and the subdomain to the job's headless Service,
so
<node>.<job-service>.<namespace>.svc.cluster.localis a working name for eachpeer.
SPUR_PEER_NODESnow carries those names, in nodelist order, so index N is thepeer whose
SPUR_NODE_RANKis N. A smallheadless_peer_dnshelper builds them.A single node job has no headless Service and therefore no name to publish. The
variable is left unset rather than holding an address that resolves to nothing,
so a launcher fails loudly instead of hanging on a dead address.
This also corrects the message the operator returns when no
SpurJobcarries therequested job id. It said "agent unreachable", although the operator is running and
answering. It now says that Pod mode launches only a
SpurJob, and that a job fromsbatchorspur submithas nothing for the operator to launch.Related:
Test Plan
SPUR_PEER_NODESinside every Pod.Test Result
Environment: three node RKE2 cluster on cloud VMs, 8 vCPU and 96 GiB each, no GPU.
order. Each name resolved from inside a Pod to the Pod IP of the matching node.
SPUR_PEER_NODESunset.cargo clippy --workspace --exclude spur-ffi --all-targets --lockedreportsnothing;
cargo test --lockedpasses 3528 tests, including three new ones forthe name builder.
Submission Checklist