fix(spurctld,spur-cli): clear max-nodes on --max-nodes 0; pass proto request to update_partition

`update-partition --max-nodes 0` was a silent no-op: it arrives as
max_nodes_value=Some(0) with clear_max_nodes unset, and the handler
forwarded the raw request flag, so cluster.rs saw "no change". Collapse
both inputs into a single derived clear bool (via resolve_max_nodes_update)
and pass that through, so a literal 0 clears the limit as the proto
documents. Add a unit test covering the four intents.

Rework the client update_partition to take the proto UpdatePartitionRequest
directly; both callers assemble the struct, dropping the 24 positional
args, the redundant set_* bools, and the too_many_arguments allow. Fold
the duplicated split_csv closures into one shared helper.

Author: yansun1996

crates/spur-cli/src/scontrol.rs

@@ -442,49 +442,36 @@ pub async fn main_with_args(args: Vec<String>) -> Result<()> {
             priority_tier,
             preempt_mode,
         } => {
-            update_partition(
-                &args.controller,
-                &name,
+            let selector_map = match selector {
+                Some(ref s) => parse_selector(s)?,
+                None => HashMap::new(),
+            };
+            let req = spur_proto::proto::UpdatePartitionRequest {
+                name,
                 nodes,
-                selector,
-                clear_selector,
+                selector: selector_map,
+                set_selector: clear_selector || selector.is_some(),
                 state,
-                default,
+                is_default: default,
                 max_time,
                 default_time,
-                max_nodes,
+                max_nodes_value: max_nodes,
                 clear_max_nodes,
                 min_nodes,
-                if set_allow_accounts {
-                    Some(&allow_accounts)
-                } else {
-                    None
-                },
-                if set_allow_groups {
-                    Some(&allow_groups)
-                } else {
-                    None
-                },
+                allow_accounts: split_csv(&allow_accounts),
                 set_allow_accounts,
+                allow_groups: split_csv(&allow_groups),
                 set_allow_groups,
-                if set_deny_accounts {
-                    Some(&deny_accounts)
-                } else {
-                    None
-                },
-                if set_deny_qos { Some(&deny_qos) } else { None },
+                deny_accounts: split_csv(&deny_accounts),
                 set_deny_accounts,
+                deny_qos: split_csv(&deny_qos),
                 set_deny_qos,
-                if set_allow_qos {
-                    Some(&allow_qos)
-                } else {
-                    None
-                },
+                allow_qos: split_csv(&allow_qos),
                 set_allow_qos,
                 priority_tier,
                 preempt_mode,
-            )
-            .await
+            };
+            update_partition(&args.controller, req).await
         }
         ScontrolCommand::DeletePartition { name } => {
             delete_partition(&args.controller, &name).await
@@ -521,12 +508,6 @@ pub async fn main_with_args(args: Vec<String>) -> Result<()> {
             add_accounts,
             remove_accounts,
         } => {
-            let split_csv = |s: &str| -> Vec<String> {
-                s.split(',')
-                    .map(|s| s.trim().to_string())
-                    .filter(|s| !s.is_empty())
-                    .collect()
-            };
             let channel = spur_client::connect_channel(&args.controller)
                 .await
                 .context("failed to connect to spurctld")?;
@@ -1217,59 +1198,34 @@ async fn parse_and_update_partition(controller: &str, params: &[String]) -> Resu
         anyhow::bail!("scontrol update: PartitionName= is required");
     }
 
-    let set_allow_accounts = allow_accounts.is_some();
-    let set_allow_groups = allow_groups.is_some();
-    let set_deny_accounts = deny_accounts.is_some();
-    let set_deny_qos = deny_qos.is_some();
-    let set_allow_qos = allow_qos.is_some();
-
-    update_partition(
-        controller,
-        &name,
+    // An ACL is applied only when its key appeared; an empty value clears it.
+    let req = spur_proto::proto::UpdatePartitionRequest {
+        name,
         nodes,
-        None, // selector not supported in inline syntax
-        false,
+        selector: HashMap::new(), // selector not supported in inline syntax
+        set_selector: false,
         state,
         is_default,
         max_time,
         default_time,
-        max_nodes,
+        max_nodes_value: max_nodes,
         clear_max_nodes,
         min_nodes,
-        if set_allow_accounts {
-            allow_accounts.as_deref()
-        } else {
-            None
-        },
-        if set_allow_groups {
-            allow_groups.as_deref()
-        } else {
-            None
-        },
-        set_allow_accounts,
-        set_allow_groups,
-        if set_deny_accounts {
-            deny_accounts.as_deref()
-        } else {
-            None
-        },
-        if set_deny_qos {
-            deny_qos.as_deref()
-        } else {
-            None
-        },
-        set_deny_accounts,
-        set_deny_qos,
-        if set_allow_qos {
-            allow_qos.as_deref()
-        } else {
-            None
-        },
-        set_allow_qos,
+        set_allow_accounts: allow_accounts.is_some(),
+        allow_accounts: allow_accounts.as_deref().map(split_csv).unwrap_or_default(),
+        set_allow_groups: allow_groups.is_some(),
+        allow_groups: allow_groups.as_deref().map(split_csv).unwrap_or_default(),
+        set_deny_accounts: deny_accounts.is_some(),
+        deny_accounts: deny_accounts.as_deref().map(split_csv).unwrap_or_default(),
+        set_deny_qos: deny_qos.is_some(),
+        deny_qos: deny_qos.as_deref().map(split_csv).unwrap_or_default(),
+        set_allow_qos: allow_qos.is_some(),
+        allow_qos: allow_qos.as_deref().map(split_csv).unwrap_or_default(),
         priority_tier,
         preempt_mode,
-    )
-    .await
+    };
+
+    update_partition(controller, req).await
 }
 
 /// Update a node's state via the controller.
@@ -1312,6 +1268,14 @@ async fn update_node(
     Ok(())
 }
 
+/// Split a comma-separated list into trimmed, non-empty entries.
+fn split_csv(s: &str) -> Vec<String> {
+    s.split(',')
+        .map(|s| s.trim().to_string())
+        .filter(|s| !s.is_empty())
+        .collect()
+}
+
 /// Parse "KEY=VALUE,KEY2=VALUE2" into a HashMap.
 fn parse_selector(s: &str) -> Result<HashMap<String, String>> {
     let mut map = HashMap::new();
@@ -1351,13 +1315,6 @@ async fn create_partition(
         .context("failed to connect to spurctld")?;
     let mut client = spur_proto::controller_client(channel);
 
-    let split_csv = |s: &str| -> Vec<String> {
-        s.split(',')
-            .map(|s| s.trim().to_string())
-            .filter(|s| !s.is_empty())
-            .collect()
-    };
-
     client
         .create_partition(spur_proto::proto::CreatePartitionRequest {
             name: name.to_string(),
@@ -1384,78 +1341,21 @@ async fn create_partition(
     Ok(())
 }
 
-/// Update a partition via the controller.
-#[allow(clippy::too_many_arguments)]
+/// Update a partition via the controller. The request is already the proto
+/// struct, so callers assemble it directly rather than threading a long
+/// positional field list through this sender.
 async fn update_partition(
     controller: &str,
-    name: &str,
-    nodes: Option<String>,
-    selector: Option<String>,
-    clear_selector: bool,
-    state: Option<String>,
-    is_default: Option<bool>,
-    max_time: Option<String>,
-    default_time: Option<String>,
-    max_nodes: Option<u32>,
-    clear_max_nodes: bool,
-    min_nodes: Option<u32>,
-    allow_accounts: Option<&str>,
-    allow_groups: Option<&str>,
-    set_allow_accounts: bool,
-    set_allow_groups: bool,
-    deny_accounts: Option<&str>,
-    deny_qos: Option<&str>,
-    set_deny_accounts: bool,
-    set_deny_qos: bool,
-    allow_qos: Option<&str>,
-    set_allow_qos: bool,
-    priority_tier: Option<u32>,
-    preempt_mode: Option<String>,
+    req: spur_proto::proto::UpdatePartitionRequest,
 ) -> Result<()> {
     let channel = spur_client::connect_channel(controller)
         .await
         .context("failed to connect to spurctld")?;
     let mut client = spur_proto::controller_client(channel);
 
-    let split_csv = |s: &str| -> Vec<String> {
-        s.split(',')
-            .map(|s| s.trim().to_string())
-            .filter(|s| !s.is_empty())
-            .collect()
-    };
-
-    let selector_map = if let Some(ref s) = selector {
-        parse_selector(s)?
-    } else {
-        HashMap::new()
-    };
-
+    let name = req.name.clone();
     client
-        .update_partition(spur_proto::proto::UpdatePartitionRequest {
-            name: name.to_string(),
-            nodes,
-            selector: selector_map,
-            set_selector: clear_selector || selector.is_some(),
-            state,
-            is_default,
-            max_time,
-            default_time,
-            max_nodes_value: max_nodes,
-            clear_max_nodes,
-            min_nodes,
-            allow_accounts: allow_accounts.map(split_csv).unwrap_or_default(),
-            set_allow_accounts,
-            allow_groups: allow_groups.map(split_csv).unwrap_or_default(),
-            set_allow_groups,
-            deny_accounts: deny_accounts.map(split_csv).unwrap_or_default(),
-            set_deny_accounts,
-            deny_qos: deny_qos.map(split_csv).unwrap_or_default(),
-            set_deny_qos,
-            allow_qos: allow_qos.map(split_csv).unwrap_or_default(),
-            set_allow_qos,
-            priority_tier,
-            preempt_mode,
-        })
+        .update_partition(req)
         .await
         .context("failed to update partition")?;
 

crates/spurctld/src/server.rs

@@ -1419,13 +1419,8 @@ impl SlurmController for ControllerService {
         } else {
             None
         };
-        // `clear_max_nodes` and a literal 0 both mean "no limit" (0 documented
-        // as "clear limit" in the proto); neither can express a real 0-node cap.
-        let max_nodes = if req.clear_max_nodes || req.max_nodes_value == Some(0) {
-            None
-        } else {
-            req.max_nodes_value
-        };
+        let (max_nodes, clear_max_nodes) =
+            resolve_max_nodes_update(req.max_nodes_value, req.clear_max_nodes);
 
         let selector = if req.set_selector || !req.selector.is_empty() {
             Some(req.selector.into_iter().collect())
@@ -1445,7 +1440,7 @@ impl SlurmController for ControllerService {
                 max_time,
                 req.default_time,
                 max_nodes,
-                req.clear_max_nodes,
+                clear_max_nodes,
                 min_nodes,
                 allow_accounts,
                 allow_groups,
@@ -2591,6 +2586,20 @@ fn partition_rpc_status(err: PartitionError) -> Status {
     }
 }
 
+/// Resolve an `UpdatePartitionRequest`'s max-nodes intent into the
+/// `(max_nodes, clear)` pair `ClusterManager::update_partition` expects.
+///
+/// `clear_max_nodes` and a literal `max_nodes_value == 0` both mean "no limit"
+/// (0 is documented as "clear limit" in the proto); neither can express a real
+/// 0-node cap. The two inputs must be collapsed into a single `clear` bool that
+/// is passed through — forwarding the raw request flag would drop a `--max-nodes
+/// 0` clear, since that arrives as `Some(0)` with the flag unset.
+fn resolve_max_nodes_update(max_nodes_value: Option<u32>, clear_flag: bool) -> (Option<u32>, bool) {
+    let clear = clear_flag || max_nodes_value == Some(0);
+    let max_nodes = if clear { None } else { max_nodes_value };
+    (max_nodes, clear)
+}
+
 fn cluster_err_to_status(err: anyhow::Error) -> Status {
     if err.downcast_ref::<spur_core::auth::AuthError>().is_some() {
         return Status::permission_denied(err.to_string());
@@ -2716,6 +2725,20 @@ mod tests {
         assert_eq!(status.code(), Code::Internal);
     }
 
+    #[test]
+    fn resolve_max_nodes_update_maps_intents() {
+        // `--max-nodes 0` (Some(0), flag unset) must resolve to a clear, not a
+        // silent no-op: cluster.rs only clears when the passed bool is true.
+        assert_eq!(resolve_max_nodes_update(Some(0), false), (None, true));
+        // Explicit clear flag, regardless of value.
+        assert_eq!(resolve_max_nodes_update(None, true), (None, true));
+        assert_eq!(resolve_max_nodes_update(Some(4), true), (None, true));
+        // A real positive cap is preserved and does not clear.
+        assert_eq!(resolve_max_nodes_update(Some(4), false), (Some(4), false));
+        // No value and no flag means "leave unchanged".
+        assert_eq!(resolve_max_nodes_update(None, false), (None, false));
+    }
+
     fn make_node_info(name: &str) -> NodeInfo {
         NodeInfo {
             name: name.into(),