test: add unit test for region mover & znode

Author: maltesander

rust/operator-binary/src/controller/build/region_mover.rs

@@ -66,3 +66,146 @@ impl AnyServiceConfig {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use indoc::indoc;
+
+    use crate::{crd::HbaseRole, test_utils};
+
+    /// Renders the region-mover args for the default region-server role group of a cluster whose
+    /// `regionServers.config` is built from `config_lines`. Each entry is one YAML line nested
+    /// under `config:`; indentation relative to `config:` is written as leading spaces in the entry
+    /// (e.g. `"  runBeforeShutdown: true"` for a key under `regionMover:`).
+    fn region_mover_args_for(config_lines: &[&str]) -> String {
+        // `regionServers.config` is placed last so the generated config lines simply extend the
+        // document, avoiding any trailing-key indentation juggling.
+        const HEADER: &str = indoc! {r#"
+            apiVersion: hbase.stackable.tech/v1alpha1
+            kind: HbaseCluster
+            metadata:
+              name: test-hbase
+              namespace: default
+              uid: 12345678-1234-1234-1234-123456789012
+            spec:
+              image:
+                productVersion: 2.6.4
+              clusterConfig:
+                hdfsConfigMapName: test-hdfs
+                zookeeperConfigMapName: test-znode
+              masters:
+                roleGroups:
+                  default:
+                    replicas: 1
+              restServers:
+                roleGroups:
+                  default:
+                    replicas: 1
+              regionServers:
+                roleGroups:
+                  default:
+                    replicas: 1
+                config:
+        "#};
+
+        let config = config_lines
+            .iter()
+            .map(|line| format!("      {line}"))
+            .collect::<Vec<_>>()
+            .join("\n");
+        let yaml = format!("{HEADER}{config}\n");
+
+        let hbase = test_utils::hbase_from_yaml(&yaml);
+        let validated = test_utils::validated_cluster_from(&hbase);
+        test_utils::merged_config_for(&validated, &HbaseRole::RegionServer, "default")
+            .region_mover_args()
+    }
+
+    #[test]
+    fn empty_when_disabled() {
+        assert_eq!(
+            region_mover_args_for(&["regionMover:", "  runBeforeShutdown: false"]),
+            ""
+        );
+    }
+
+    #[test]
+    fn empty_for_non_region_server_role() {
+        // The region mover only applies to region servers; every other role returns no args.
+        let validated = test_utils::validated_cluster();
+        assert_eq!(
+            test_utils::merged_config(&validated, &HbaseRole::Master).region_mover_args(),
+            ""
+        );
+    }
+
+    #[test]
+    fn uses_default_graceful_shutdown_timeout_minus_delta() {
+        // Default region-server graceful shutdown timeout is 60m (3600s); the region mover reserves
+        // a 1m (60s) delta, leaving 3540s.
+        assert_eq!(
+            region_mover_args_for(&["regionMover:", "  runBeforeShutdown: true"]),
+            "--maxthreads 1 --timeout 3540"
+        );
+    }
+
+    #[test]
+    fn subtracts_delta_when_timeout_above_delta() {
+        // 5m (300s) - 1m delta = 240s.
+        assert_eq!(
+            region_mover_args_for(&[
+                "gracefulShutdownTimeout: 5m",
+                "regionMover:",
+                "  runBeforeShutdown: true",
+            ]),
+            "--maxthreads 1 --timeout 240"
+        );
+    }
+
+    #[test]
+    fn keeps_timeout_when_exactly_at_delta() {
+        // 1m (60s) is not greater than the 60s delta, so it is used verbatim (no underflow).
+        assert_eq!(
+            region_mover_args_for(&[
+                "gracefulShutdownTimeout: 1m",
+                "regionMover:",
+                "  runBeforeShutdown: true",
+            ]),
+            "--maxthreads 1 --timeout 60"
+        );
+    }
+
+    #[test]
+    fn keeps_timeout_when_below_delta() {
+        // 30s is below the 60s delta, so it is used verbatim rather than underflowing.
+        assert_eq!(
+            region_mover_args_for(&[
+                "gracefulShutdownTimeout: 30s",
+                "regionMover:",
+                "  runBeforeShutdown: true",
+            ]),
+            "--maxthreads 1 --timeout 30"
+        );
+    }
+
+    #[test]
+    fn appends_noack_when_ack_disabled() {
+        assert_eq!(
+            region_mover_args_for(&["regionMover:", "  runBeforeShutdown: true", "  ack: false",]),
+            "--maxthreads 1 --timeout 3540 --noack"
+        );
+    }
+
+    #[test]
+    fn appends_shell_escaped_extra_options() {
+        // Extra options are passed through verbatim except for shell escaping of unsafe values.
+        assert_eq!(
+            region_mover_args_for(&[
+                "regionMover:",
+                "  runBeforeShutdown: true",
+                r#"  additionalMoverOptions: ["--foo", "a b"]"#,
+            ]),
+            "--maxthreads 1 --timeout 3540 --foo 'a b'"
+        );
+    }
+}

rust/operator-binary/src/controller/zookeeper.rs

@@ -17,6 +17,10 @@ const HBASE_ZOOKEEPER_QUORUM: &str = "hbase.zookeeper.quorum";
 const HBASE_ZOOKEEPER_PROPERTY_CLIENT_PORT: &str = "hbase.zookeeper.property.clientPort";
 const ZOOKEEPER_ZNODE_PARENT: &str = "zookeeper.znode.parent";
 
+/// The znode sub-path HBase stores its data under, appended to the chroot read from the discovery
+/// `ConfigMap` (see [`chroot_with_hbase_suffix`]).
+const HBASE_ZNODE_SUFFIX: &str = "hbase";
+
 #[derive(Snafu, Debug, EnumDiscriminants)]
 #[strum_discriminants(derive(IntoStaticStr))]
 pub enum Error {
@@ -91,7 +95,7 @@ impl ZookeeperConnectionInformation {
                 cm_name: zk_discovery_cm_name.to_string(),
                 entry: ZOOKEEPER_DISCOVERY_CM_HOSTS_ENTRY,
             })?;
-        let mut chroot = zk_discovery_cm
+        let chroot = zk_discovery_cm
             .data
             .as_mut()
             .and_then(|data| data.remove(ZOOKEEPER_DISCOVERY_CM_CHROOT_ENTRY))
@@ -113,26 +117,9 @@ impl ZookeeperConnectionInformation {
                 entry: ZOOKEEPER_DISCOVERY_CM_CLIENT_PORT_ENTRY,
             })?;
 
-        // IMPORTANT!
-        // Before https://github.com/stackabletech/hbase-operator/issues/354, hbase automatically added a `/hbase` suffix, ending up with a chroot of e.g. `/znode-fe51edff-8df9-43a8-ac5f-4781b071ae5f/hbase`.
-        // Because the chroot we read from the ZNode discovery CM is only `/znode-fe51edff-8df9-43a8-ac5f-4781b071ae5f`, we need to prepend the `/hbase` suffix ourselves.
-        // If we don't do so, hbase clusters created before #354 would need to be migrated to the different znode path!
-
-        // Check if a user points to a discovery CM of a HBaseCluster rather than a ZNode.
-        if chroot == "/" {
-            warn!(
-                "It is recommended to let the HBase cluster point to a discovery ConfigMap of a ZNode rather than a ZookeeperCluster. \
-            This prevents accidental reuse of the same Zookeeper path for multiple product instances. \
-            See https://docs.stackable.tech/home/stable/zookeeper/getting_started/first_steps for details"
-            );
-            chroot = "/hbase".to_string();
-        } else {
-            chroot = format!("{chroot}/hbase");
-        }
-
         Ok(Self {
             hosts,
-            chroot,
+            chroot: chroot_with_hbase_suffix(&chroot),
             port,
         })
     }
@@ -155,3 +142,43 @@ impl ZookeeperConnectionInformation {
         ])
     }
 }
+
+/// Appends the [`HBASE_ZNODE_SUFFIX`] to the chroot read from the discovery `ConfigMap`.
+///
+/// IMPORTANT!
+/// Before <https://github.com/stackabletech/hbase-operator/issues/354>, hbase automatically added a
+/// `/hbase` suffix, ending up with a chroot of e.g. `/znode-fe51edff-.../hbase`. Because the chroot
+/// we read from the ZNode discovery `ConfigMap` is only `/znode-fe51edff-...`, we need to append the
+/// `/hbase` suffix ourselves. If we don't, hbase clusters created before #354 would need to be
+/// migrated to the different znode path!
+fn chroot_with_hbase_suffix(chroot: &str) -> String {
+    // Check if a user points to a discovery CM of a HBaseCluster rather than a ZNode.
+    if chroot == "/" {
+        warn!(
+            "It is recommended to let the HBase cluster point to a discovery ConfigMap of a ZNode rather than a ZookeeperCluster. \
+            This prevents accidental reuse of the same Zookeeper path for multiple product instances. \
+            See https://docs.stackable.tech/home/stable/zookeeper/getting_started/first_steps for details"
+        );
+        format!("/{HBASE_ZNODE_SUFFIX}")
+    } else {
+        format!("{chroot}/{HBASE_ZNODE_SUFFIX}")
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn appends_suffix_to_znode_chroot() {
+        // A ZNode discovery CM exposes e.g. `/znode-test`; HBase stores its data under `.../hbase`.
+        assert_eq!(chroot_with_hbase_suffix("/znode-test"), "/znode-test/hbase");
+    }
+
+    #[test]
+    fn replaces_root_chroot_with_suffix() {
+        // Pointing directly at a ZookeeperCluster discovery CM yields a `/` chroot, which becomes
+        // `/hbase` (rather than `//hbase`).
+        assert_eq!(chroot_with_hbase_suffix("/"), "/hbase");
+    }
+}