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

[01xjw]

Summary

• Make spur node label, drain, and remove resolve ALL to every registered node.
• Match the existing scontrol behavior with case-insensitive ALL handling and a clear error for an empty cluster.
• Preserve normal hostlist expansion and add focused regression coverage.

Closes #566.

Validation

• cargo test -p spur-cli node::tests
• cargo fmt --all --check
• cargo clippy -p spur-cli --all-targets -- -D warnings
• Deterministic baseline failed before the change and passed afterward.

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 36.36364% with 28 lines in your changes missing coverage. Please review.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #568      +/-   ##
==========================================
- Coverage   76.03%   76.00%   -0.03%     
==========================================
  Files         166      166              
  Lines       63002    63043      +41     
==========================================
+ Hits        47898    47912      +14     
- Misses      15104    15131      +27     

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

[yansun1996]

This duplicates the resolve_node_names in scontrol.rs almost verbatim. Could we lift that one to pub(crate) and share it, to avoid the two copies drifting over time?

[yansun1996]

expand_node_pattern isn't changed by this PR, so this guards existing behavior rather than the new ALL path — it would pass even if the fix were reverted. The higher-value test would target the ALL branch of resolve_node_names (e.g. via the mock controller).

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