Fix #459 - domain name cleanup

build-dist/rpm-assets/sonar.cfg

@@ -56,7 +56,6 @@ role = node
 # Cluster info (from Slurm) is normally only done when Sonar is in the `master` role.
 # [cluster]
 # cadence =
-# domain =
 # on-startup = true
 
 # Helper programs that Sonar sometimes needs to use.

doc/CHANGELOG.md

@@ -11,6 +11,15 @@ output format can most easily be be seen by diffing this file against a desired
 
 ## Changes in v0.19.0 (on `main`)
 
+* Bug 516 - **IMPORTANT FUNCTIONALITY.** Introduce `global.hostname-only` as a better way of managing
+  how node names are reported, default true (differs from v0.18)
+* Bug 459 - **REMOVED FUNCTIONALITY.** Remove the half-implemented (and poorly-implemented) `cluster.domain`
+  setting.
+
+## Changes in v0.18.1 (on `release_0_18`)
+
+* Bug 516 - **IMPORTANT FUNCTIONALITY.** Introduce `global.hostname-only` as a better way of managing
+  how node names are reported, default false.
 
 ## Changes in v0.18.0 (on `release_0_18`)
 
@@ -20,8 +29,6 @@ output format can most easily be be seen by diffing this file against a desired
 * Bug 502 - User manual, design doc, developer doc, and general doc cleanup
 * Bug 503 - Use newer power API on AMD GPUs
 * Bug 505 - Log errors to the journal, not to stderr
-* Bug 516 - **IMPORTANT FUNCTIONALITY.** Introduce `global.hostname-only` as a better way of managing
-  how node names are reported.
 * No bug - introduced `sonar sample` as the canonical way to say `sonar ps` (older)
 * No bug - introduced `sonar jobs` as the canonical way to say `sonar slurm` (older)
 * Misc tweaks and cleanup as usual

doc/HOWTO-DAEMON.md

@@ -43,7 +43,7 @@ cluster = <canonical cluster name>
 role = node | master
 lock-directory = <string>                       # default none
 topic-prefix = <string>                         # default none
-hostname-only = <bool>                          # default false
+hostname-only = <bool>                          # default true
 ```
 
 The `cluster` option is required, eg `fox.educloud.no`.
@@ -172,15 +172,9 @@ may become so large that they cause transmission issues, notably by default Kafk
 
 ```
 cadence = <duration value>
-domain = <string>                               # default none
 on-startup = <bool>                             # default true
 ```
 
-If there is a `domain` then it must have the form `.x.y.z` with at least one element.  It will be
-appended to all slurm prefix names in every NodeRange value to form full node names.  (Bug in v0.18:
-it is not actually appended in nodelists in jobs.)  The `domain` setting is disallowed if the
-`global.hostname-only` setting is true.
-
 If `on-startup` is `true` then a cluster operation will be executed every time the daemon is
 started, in addition to according to the cadence.
 

doc/MANUAL.md

@@ -50,17 +50,16 @@ name can be given to the cluster; usually it takes the form of a domain name, bu
 The `role` setting is either `node` for a compute node or `master` for a cluster master node.  The
 setting is required.
 
-The `hostname-only` setting is false by default but it is recommended to set it to true.  When set,
-it causes Sonar to strip all domain information from the node names; node names may otherwise be
-reported either with a domain or without, depending on the source of the information, and this must
-then be dealt with in the back-end.
+The `hostname-only` setting is true by default.  When set, it causes Sonar to strip all domain
+information from the node names; node names may otherwise be reported either with a domain or
+without, depending on the source of the information, and this must then be dealt with in the
+back-end.
 
 Example:
 ```
 [global]
 cluster = saga.sigma2.no
 role = node
-hostname-only = true
 ```
 
 #### Programs section

src/cluster.rs

@@ -60,18 +60,15 @@ fn do_show_cluster(
         p.push_s(CLUSTER_PARTITION_NAME, name);
         p.push_a(
             CLUSTER_PARTITION_NODES,
-            nodelist::parse_and_render(system, &nodelist)?,
+            nodelist::parse_nodelist(&nodelist)?,
         );
         partitions.push_o(p);
     }
 
     let mut nodes = output::Array::new();
     for (nodelist, statelist) in system.compute_cluster_nodes()? {
         let mut p = output::Object::new();
-        p.push_a(
-            CLUSTER_NODES_NAMES,
-            nodelist::parse_and_render(system, &nodelist)?,
-        );
+        p.push_a(CLUSTER_NODES_NAMES, nodelist::parse_nodelist(&nodelist)?);
         let mut states = output::Array::new();
         for s in statelist.split('+') {
             states.push_s(s.to_ascii_uppercase());

src/daemon.rs

@@ -98,7 +98,6 @@ pub struct JobsIni {
 pub struct ClusterIni {
     pub on_startup: bool,
     pub cadence: Option<Dur>,
-    pub domain: Option<Vec<String>>,
 }
 
 pub struct ProgramsIni {
@@ -232,9 +231,6 @@ pub fn daemon_mode(
         system = system.with_jobmanager(Box::new(jobsapi::AnyJobManager::new(force_slurm)));
     }
 
-    if let Some(ref p) = ini.cluster.domain {
-        system = system.with_node_domain(p);
-    }
     if ini.global.hostname_only {
         system = system.with_hostname_only();
     }
@@ -710,7 +706,7 @@ fn parse_config(config_file: &str) -> Result<Ini, String> {
             role: "".to_string(),
             lockdir: None,
             topic_prefix: None,
-            hostname_only: false,
+            hostname_only: true,
         },
         #[cfg(feature = "kafka")]
         kafka: KafkaIni {
@@ -761,7 +757,6 @@ fn parse_config(config_file: &str) -> Result<Ini, String> {
         cluster: ClusterIni {
             on_startup: true,
             cadence: None,
-            domain: None,
         },
     };
 
@@ -1006,20 +1001,7 @@ fn parse_config(config_file: &str) -> Result<Ini, String> {
                     ini.cluster.cadence = Some(parse_duration("cluster.cadence", &value, false)?);
                 }
                 "domain" => {
-                    // FIXME: Bug #459 / #516: This setting should not be attached to cluster.
-                    let mut xs = value
-                        .split(".")
-                        .map(|x| x.to_string())
-                        .collect::<Vec<String>>();
-                    if xs.len() < 2 || xs[0] != "" || xs[1..].iter().any(|x| x == "") {
-                        return Err(format!(
-                            "Invalid global.domain value `{value}` - form .x.y.z required"
-                        ));
-                    }
-                    // Drop initial, empty element
-                    xs.rotate_left(1);
-                    xs.pop();
-                    ini.cluster.domain = Some(xs);
+                    // No longer does anything.
                 }
                 _ => return Err(format!("Invalid [cluster] setting name `{name}`")),
             },
@@ -1065,9 +1047,6 @@ fn parse_config(config_file: &str) -> Result<Ini, String> {
     if ini.global.role == "" {
         return Err("Missing global.role setting".to_string());
     }
-    if ini.global.hostname_only && ini.cluster.domain.is_some() {
-        return Err("Can't have both global.hostname-only and cluster.domain".to_string());
-    }
 
     let mut sinks = 0;
     if have_directory {
@@ -1341,7 +1320,6 @@ pub fn test_parser() {
 
     assert!(ini.cluster.cadence == Some(Dur::Minutes(15)));
     assert!(!ini.cluster.on_startup);
-    assert!(ini.cluster.domain == Some(vec!["fox".to_string(), "nux".to_string()]));
 
     let ini = parse_config("src/testdata/daemon-stdio-config2.txt").unwrap();
 

src/linux/mocksystem.rs

@@ -25,7 +25,6 @@ pub struct Builder {
     timestamp: Option<String>,
     hostname: Option<String>,
     cluster: Option<String>,
-    node_domain: Option<Vec<String>>,
     version: Option<String>,
     os_name: Option<String>,
     os_release: Option<String>,
@@ -108,13 +107,6 @@ impl Builder {
         }
     }
 
-    pub fn with_node_domain(self, domain: &[String]) -> Builder {
-        Builder {
-            node_domain: Some(domain.iter().map(|x| x.clone()).collect::<Vec<String>>()),
-            ..self
-        }
-    }
-
     pub fn with_hostname_only(self) -> Builder {
         Builder {
             hostname_only: true,
@@ -192,7 +184,6 @@ impl Builder {
             } else {
                 "no.cluster.com".to_string()
             },
-            node_domain: self.node_domain,
             hostname_only: self.hostname_only,
             os_name: if let Some(x) = self.os_name {
                 x
@@ -244,7 +235,7 @@ pub struct MockSystem {
     jm: Box<dyn jobsapi::JobManager>,
     hostname: String,
     cluster: String,
-    node_domain: Option<Vec<String>>,
+    #[allow(unused)]
     hostname_only: bool,
     os_name: String,
     os_release: String,
@@ -284,10 +275,6 @@ impl systemapi::SystemAPI for MockSystem {
         self.cluster.clone()
     }
 
-    fn get_node_domain(&self) -> &Option<Vec<String>> {
-        &self.node_domain
-    }
-
     fn get_hostname_only(&self) -> bool {
         self.hostname_only
     }

src/linux/system.rs

@@ -43,7 +43,6 @@ const SINFO_TIMEOUT_S: u64 = 10;
 pub struct Builder {
     jm: Option<Box<dyn jobsapi::JobManager>>,
     cluster: String,
-    node_domain: Option<Vec<String>>,
     hostname_only: bool,
     sacct: String,
     scontrol: String,
@@ -57,7 +56,6 @@ impl Builder {
         Builder {
             jm: None,
             cluster: "".to_string(),
-            node_domain: None,
             hostname_only: false,
             sacct: "sacct".to_string(),
             scontrol: "scontrol".to_string(),
@@ -67,14 +65,6 @@ impl Builder {
         }
     }
 
-    #[allow(dead_code)]
-    pub fn with_node_domain(self, domain: &[String]) -> Builder {
-        Builder {
-            node_domain: Some(domain.to_vec()),
-            ..self
-        }
-    }
-
     #[allow(dead_code)]
     pub fn with_hostname_only(self) -> Builder {
         Builder {
@@ -155,7 +145,6 @@ impl Builder {
         };
         Ok(System {
             hostname: hostname.clone(),
-            node_domain: self.node_domain,
             hostname_only: self.hostname_only,
             cluster: self.cluster,
             jm: if let Some(x) = self.jm {
@@ -179,55 +168,6 @@ impl Builder {
     }
 }
 
-// The entire suffix of hostname must match a prefix of domain, and in that case we attach the rest
-// of domain, otherwise we attach the entire domain to hostname.
-#[allow(dead_code)]
-fn expand_domain(hostname: String, domain: &[String]) -> String {
-    let mut full = hostname
-        .split('.')
-        .map(|x| x.to_string())
-        .collect::<Vec<String>>();
-    let mut f = 1;
-    let mut d = 0;
-    let mut matched = true;
-    while f < full.len() && d < domain.len() && matched {
-        if full[f] != domain[d] {
-            matched = false;
-            break;
-        }
-        f += 1;
-        d += 1;
-    }
-    if matched && f == full.len() {
-        for de in domain[d..].iter() {
-            full.push(de.clone())
-        }
-    } else {
-        for de in domain {
-            full.push(de.clone());
-        }
-    }
-    full.join(".")
-}
-
-#[test]
-fn test_expand_domain() {
-    assert!(expand_domain("a".to_string(), &[]) == "a");
-    assert!(expand_domain("a.b.c".to_string(), &[]) == "a.b.c");
-    assert!(expand_domain("a.b".to_string(), &["c".to_string()]) == "a.b.c");
-    assert!(expand_domain("a.b".to_string(), &["b".to_string(), "c".to_string()]) == "a.b.c");
-    assert!(expand_domain("a.b.c".to_string(), &["b".to_string(), "c".to_string()]) == "a.b.c");
-    assert!(
-        expand_domain("a.b.c.d".to_string(), &["b".to_string(), "c".to_string()]) == "a.b.c.d.b.c"
-    );
-    assert!(
-        expand_domain(
-            "a.b".to_string(),
-            &["c".to_string(), "d".to_string(), "e".to_string()]
-        ) == "a.b.c.d.e"
-    );
-}
-
 #[cfg(target_arch = "x86_64")]
 const ARCHITECTURE: &str = "x86_64";
 
@@ -239,7 +179,7 @@ const ARCHITECTURE: &'static str = "aarch64";
 
 pub struct System {
     hostname: String,
-    node_domain: Option<Vec<String>>,
+    #[allow(unused)]
     hostname_only: bool,
     cluster: String,
     fs: RealProcFS,
@@ -281,10 +221,6 @@ impl systemapi::SystemAPI for System {
         self.cluster.clone()
     }
 
-    fn get_node_domain(&self) -> &Option<Vec<String>> {
-        &self.node_domain
-    }
-
     fn get_hostname_only(&self) -> bool {
         self.hostname_only
     }

src/nodelist.rs

@@ -1,22 +1,11 @@
 use crate::output;
-use crate::systemapi::SystemAPI;
 
 // Parse a nodelist and render it into an output object as an array of strings.
 
-pub fn parse_and_render(system: &dyn SystemAPI, xs: &str) -> Result<output::Array, String> {
+pub fn parse_nodelist(xs: &str) -> Result<output::Array, String> {
     let mut a = output::Array::new();
-    let suffix = if system.get_hostname_only() {
-        "".to_string()
-    } else if let Some(xs) = system.get_node_domain() {
-        ".".to_string() + &xs.join(".")
-    } else {
-        "".to_string()
-    };
     for v in parse(xs)? {
-        // One could argue that if system.get_hostname_only() then the parsed value should be
-        // stripped here.  In practice, I've never seen slurm report node names as anything other
-        // than leaf names, so it should not matter.
-        a.push_s(v + &suffix);
+        a.push_s(v);
     }
     Ok(a)
 }

src/slurmjobs.rs

@@ -383,10 +383,6 @@ fn parse_sacct_jobs_newfmt(
                 }
                 "NodeList" => {
                     if fieldvals[i] != "" {
-                        // FIXME: Bug #459 / #516: we really must use parse_and_render() here or in
-                        // the final rendering to attach a domain if it is set, but that will change
-                        // the output format and it would also depend on the cluster.domain setting,
-                        // which is suprising for the jobs output.
                         if let Ok(nodes) = nodelist::parse(&fieldvals[i]) {
                             output_line.nodes = nodes;
                         }

src/systemapi.rs

@@ -18,7 +18,7 @@ pub trait SystemAPI {
     fn get_version(&self) -> String;
     fn get_timestamp(&self) -> String;
     fn get_cluster(&self) -> String;
-    fn get_node_domain(&self) -> &Option<Vec<String>>;
+    #[allow(dead_code)]
     fn get_hostname_only(&self) -> bool;
     // The hostname produced here has been stripped of its domain iff get_hostname_only().
     fn get_hostname(&self) -> String;