fix(spur-cli): support ALL in node subcommands

[01xjw]

Summary

• Make spur node label, drain, and remove resolve case-insensitive ALL to every registered node.
• Reuse scontrol's existing resolver while validating ordinary hostlists before connecting.
• Document the ALL scope and add focused controller-backed regression coverage.

Closes #566.

Behavior note

spur node remove ALL --force removes every registered node and may evict their jobs. This matches existing scontrol behavior and is documented in spur node remove --help.

Validation

• cargo fmt --all -- --check
• cargo clippy --workspace --exclude spur-ffi --all-targets --locked -- -D warnings
• cargo test --workspace --exclude spurd --locked (352 spur-cli tests and 626 workspace tests passed)

Disclosure

This change was prepared with assistance from the radeon-issue automation and independently checked by a separately configured validation model. Maintainer review is still required.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 93.40659% with 6 lines in your changes missing coverage. Please review.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #568      +/-   ##
==========================================
+ Coverage   76.70%   76.79%   +0.09%     
==========================================
  Files         168      168              
  Lines       68179    68266      +87     
==========================================
+ Hits        52295    52423     +128     
+ Misses      15884    15843      -41     

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[yansun1996]

Thanks for this — it lines up cleanly with the scontrol update NodeName=ALL behavior and the empty-cluster handling is a nice touch. A few suggestions before merge:

1. Shared resolver. resolve_node_names here is essentially identical to the one in scontrol.rs. Would it be worth making that one pub(crate) and calling it from both, so the two can't drift? (nodelist.rs looked like a candidate home but it's a sync/file-based resolver, so probably not the right fit.)
2. Help text. The node arg help for label/drain/remove still lists only comma-lists and hostlist ranges — could we mention ALL there so it's discoverable in --help? scontrol's docstring already calls it out.
3. Coverage of the new branch. The two added tests cover is_all_node_pattern and the (unchanged) expand_node_pattern path, but the actual new behavior — the ALL -> get_nodes -> empty-cluster bail! branch — isn't exercised yet. The in-process mock_controller could drive this deterministically, though get_nodes would need to be added to it first (it's currently unimplemented there). Worth a follow-up if not this PR.
4. remove ALL. Minor: remove ALL --force will deregister every node and evict all jobs with no confirmation. It matches scontrol so this may be intentional — might be worth a one-line note in the PR description either way.

[Copilot]

Pull request overview

This PR updates spur node subcommands to accept the Slurm-compatible ALL keyword (case-insensitive) by resolving it to the set of registered node names via the controller’s GetNodes RPC, aligning behavior with the existing scontrol path.

Changes:

• Switch spur node label/drain/remove from pure hostlist expansion to a new async resolver that expands hostlists and resolves ALL via GetNodes.
• Add an explicit error for the empty-cluster case when ALL is requested.
• Add unit tests covering ALL case-insensitivity and ensuring hostlist expansion behavior is preserved.

Suppressed comments (3)

crates/spur-cli/src/node.rs:152

• cmd_drain now connects to the controller before validating/expanding non-ALL hostlist patterns. This can mask hostlist parse errors behind connection failures and adds an unnecessary network dependency to argument validation.

async fn cmd_drain(controller: &str, node_pattern: String, reason: Option<String>) -> Result<()> {
    let mut client = spur_proto::controller_client(spur_client::connect_channel(controller).await?);
    let nodes = resolve_node_names(&mut client, &node_pattern).await?;

crates/spur-cli/src/node.rs:202

• cmd_remove now connects to the controller before validating/expanding non-ALL hostlist patterns, which can turn local hostlist errors into connection errors when the controller is unreachable and adds avoidable network work during argument validation.

async fn cmd_remove(
    controller: &str,
    node_pattern: String,
    force: bool,
    reason: Option<String>,
) -> Result<()> {
    let mut client = spur_proto::controller_client(spur_client::connect_channel(controller).await?);
    let nodes = resolve_node_names(&mut client, &node_pattern).await?;

crates/spur-cli/src/node.rs:274

• The new behavior that resolves case-insensitive ALL via GetNodes (including the empty-cluster error path) is not covered by tests here. The added unit tests only cover the string predicate and hostlist expansion, so regressions in the RPC-based ALL resolution would go unnoticed.

async fn resolve_node_names(
    client: &mut SlurmControllerClient<tonic::transport::Channel>,
    pattern: &str,
) -> Result<Vec<String>> {
    if is_all_node_pattern(pattern) {
        let response = client
            .get_nodes(GetNodesRequest {
                nodelist: String::new(),
                ..Default::default()
            })
            .await
            .context("failed to get nodes")?;
        let names: Vec<String> = response
            .into_inner()
            .nodes
            .into_iter()
            .map(|node| node.name)
            .collect();
        if names.is_empty() {
            bail!("no nodes registered in the cluster");
        }
        return Ok(names);

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[01xjw]

Thanks for the review — addressed in 38d3cfa:

• extracted one shared node_selection implementation for spur node and scontrol;
• parse malformed hostlists before connecting; only ALL calls GetNodes;
• expanded the mock with GetNodes, DrainNode, and DeregisterNode, covering case-insensitive ALL, empty clusters, drain ALL, remove ALL --force, normal hostlists, and validation order;
• documented ALL on all three commands and the destructive scope of remove ALL --force.

remove ALL --force intentionally remains non-interactive to match scontrol and preserve scripted use. Validation is green: fmt, workspace clippy, 352 spur-cli tests, and workspace tests excluding the pod-incompatible spurd cgroup tests.

[yansun1996]

Hi @01xjw in this PR could you please check #568 (comment) ? we need the test coverage on the newly added ALL subcommands.