From 181048e902a8e288de3d89923fcb481ce531e113 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Fri, 11 Sep 2026 17:31:51 +0200 Subject: [PATCH 01/19] flatten role_configs into per-role fields --- .../src/controller/build/resource/pdb.rs | 7 +++++- rust/operator-binary/src/controller/mod.rs | 15 ++++++++++--- .../src/controller/validate.rs | 22 +++++++++++-------- 3 files changed, 31 insertions(+), 13 deletions(-) diff --git a/rust/operator-binary/src/controller/build/resource/pdb.rs b/rust/operator-binary/src/controller/build/resource/pdb.rs index 3823cd43..12136da2 100644 --- a/rust/operator-binary/src/controller/build/resource/pdb.rs +++ b/rust/operator-binary/src/controller/build/resource/pdb.rs @@ -13,7 +13,12 @@ use crate::{ /// Builds the [`PodDisruptionBudget`] for the given `role`, or `None` if the role /// has no validated config or PDBs are disabled. pub fn build_pdb(cluster: &ValidatedCluster, role: &HdfsNodeRole) -> Option { - let pdb = &cluster.role_configs.get(role)?.pdb; + let role_config = match role { + HdfsNodeRole::Name => cluster.namenode_config.as_ref(), + HdfsNodeRole::Data => cluster.datanode_config.as_ref(), + HdfsNodeRole::Journal => cluster.journalnode_config.as_ref(), + }?; + let pdb = &role_config.pdb; if !pdb.enabled { return None; } diff --git a/rust/operator-binary/src/controller/mod.rs b/rust/operator-binary/src/controller/mod.rs index afbbfedc..26b12b5a 100644 --- a/rust/operator-binary/src/controller/mod.rs +++ b/rust/operator-binary/src/controller/mod.rs @@ -100,7 +100,12 @@ pub struct ValidatedCluster { pub image: ResolvedProductImage, pub cluster_config: ValidatedClusterConfig, pub role_groups: BTreeMap>, - pub role_configs: BTreeMap, + /// The namenode role-level config (currently the PDB), or `None` if the role is absent. + pub namenode_config: Option, + /// The datanode role-level config (currently the PDB), or `None` if the role is absent. + pub datanode_config: Option, + /// The journalnode role-level config (currently the PDB), or `None` if the role is absent. + pub journalnode_config: Option, /// The namenode pod `Listener`s as currently stored in the cluster (see /// [`crate::controller::dereference::DereferencedObjects::namenode_listeners`]). pub namenode_listeners: Vec, @@ -121,7 +126,9 @@ impl ValidatedCluster { image: ResolvedProductImage, cluster_config: ValidatedClusterConfig, role_groups: BTreeMap>, - role_configs: BTreeMap, + namenode_config: Option, + datanode_config: Option, + journalnode_config: Option, namenode_listeners: Vec, discovery_config_map: Option, status: ValidatedClusterStatus, @@ -146,7 +153,9 @@ impl ValidatedCluster { product_version, cluster_config, role_groups, - role_configs, + namenode_config, + datanode_config, + journalnode_config, namenode_listeners, discovery_config_map, status, diff --git a/rust/operator-binary/src/controller/validate.rs b/rust/operator-binary/src/controller/validate.rs index 39223aeb..2cf4939f 100644 --- a/rust/operator-binary/src/controller/validate.rs +++ b/rust/operator-binary/src/controller/validate.rs @@ -88,18 +88,20 @@ pub fn validate_cluster( ) .context(ResolveProductImageSnafu)?; + let validated_role_config = |role: HdfsNodeRole| { + hdfs.role_config(&role).map( + |GenericRoleConfig { + pod_disruption_budget, + }| ValidatedRoleConfig { + pdb: pod_disruption_budget.clone(), + }, + ) + }; + let mut role_groups = BTreeMap::new(); - let mut role_configs = BTreeMap::new(); let cluster_name = get_cluster_name(hdfs).context(GetClusterNameSnafu)?; for hdfs_role in HdfsNodeRole::iter() { - if let Some(GenericRoleConfig { - pod_disruption_budget: pdb, - }) = hdfs.role_config(&hdfs_role) - { - role_configs.insert(hdfs_role, ValidatedRoleConfig { pdb: pdb.clone() }); - } - let group_configs = match hdfs_role { HdfsNodeRole::Name => validate_role_group_configs( hdfs.spec.name_nodes.as_ref(), @@ -142,7 +144,9 @@ pub fn validate_cluster( image, ValidatedClusterConfig::resolve(hdfs, hdfs_opa_config), role_groups, - role_configs, + validated_role_config(HdfsNodeRole::Name), + validated_role_config(HdfsNodeRole::Data), + validated_role_config(HdfsNodeRole::Journal), namenode_listeners, discovery_config_map, status, From f88031dbcfcadff0da0e22202726fba48f2b4952 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Fri, 11 Sep 2026 18:04:53 +0200 Subject: [PATCH 02/19] build_log4j_configs takes resolved log configs --- .../build/properties/product_logging/mod.rs | 44 +++++++------------ .../controller/build/resource/config_map.rs | 33 +++++++++++++- 2 files changed, 48 insertions(+), 29 deletions(-) diff --git a/rust/operator-binary/src/controller/build/properties/product_logging/mod.rs b/rust/operator-binary/src/controller/build/properties/product_logging/mod.rs index 6b956cd1..53dd1f70 100644 --- a/rust/operator-binary/src/controller/build/properties/product_logging/mod.rs +++ b/rust/operator-binary/src/controller/build/properties/product_logging/mod.rs @@ -1,8 +1,6 @@ //! Builders for the logging-related files in the rolegroup `ConfigMap`: the per-container //! `*.log4j.properties` configs and the (static) Vector agent config (`vector.yaml`). -use std::borrow::Cow; - use stackable_operator::{ memory::{BinaryMultiple, MemoryQuantity}, product_logging::{ @@ -12,12 +10,9 @@ use stackable_operator::{ v2::product_logging::framework::STACKABLE_LOG_DIR, }; -use crate::{ - controller::build::container::{ - FORMAT_NAMENODES_CONTAINER_NAME, FORMAT_ZOOKEEPER_CONTAINER_NAME, - WAIT_FOR_NAMENODES_CONTAINER_NAME, ZKFC_CONTAINER_NAME, - }, - crd::{AnyNodeConfig, DataNodeContainer, NameNodeContainer}, +use crate::controller::build::container::{ + FORMAT_NAMENODES_CONTAINER_NAME, FORMAT_ZOOKEEPER_CONTAINER_NAME, + WAIT_FOR_NAMENODES_CONTAINER_NAME, ZKFC_CONTAINER_NAME, }; // We have a maximum of 4 continuous logging files for Namenodes. Datanodes and Journalnodes @@ -79,12 +74,18 @@ pub fn vector_config_file_content() -> String { /// /// Returns `(filename, rendered content)` pairs; containers using a custom log ConfigMap are /// skipped, so the result is empty when none use automatic logging. -pub fn build_log4j_configs(merged_config: &AnyNodeConfig) -> Vec<(&'static str, String)> { +pub fn build_log4j_configs( + hdfs: Option<&ContainerLogConfig>, + zkfc: Option<&ContainerLogConfig>, + format_namenodes: Option<&ContainerLogConfig>, + format_zookeeper: Option<&ContainerLogConfig>, + wait_for_namenodes: Option<&ContainerLogConfig>, +) -> Vec<(&'static str, String)> { let mut configs = Vec::new(); add_log4j_config_if_automatic( &mut configs, - Some(merged_config.hdfs_logging()), + hdfs, HDFS_LOG4J_CONFIG_FILE, "hdfs", HDFS_LOG_FILE, @@ -92,9 +93,7 @@ pub fn build_log4j_configs(merged_config: &AnyNodeConfig) -> Vec<(&'static str, ); add_log4j_config_if_automatic( &mut configs, - merged_config - .as_namenode() - .map(|nn| nn.logging.for_container(&NameNodeContainer::Zkfc)), + zkfc, ZKFC_LOG4J_CONFIG_FILE, ZKFC_CONTAINER_NAME.as_ref(), ZKFC_LOG_FILE, @@ -102,10 +101,7 @@ pub fn build_log4j_configs(merged_config: &AnyNodeConfig) -> Vec<(&'static str, ); add_log4j_config_if_automatic( &mut configs, - merged_config.as_namenode().map(|nn| { - nn.logging - .for_container(&NameNodeContainer::FormatNameNodes) - }), + format_namenodes, FORMAT_NAMENODES_LOG4J_CONFIG_FILE, FORMAT_NAMENODES_CONTAINER_NAME.as_ref(), FORMAT_NAMENODES_LOG_FILE, @@ -113,10 +109,7 @@ pub fn build_log4j_configs(merged_config: &AnyNodeConfig) -> Vec<(&'static str, ); add_log4j_config_if_automatic( &mut configs, - merged_config.as_namenode().map(|nn| { - nn.logging - .for_container(&NameNodeContainer::FormatZooKeeper) - }), + format_zookeeper, FORMAT_ZOOKEEPER_LOG4J_CONFIG_FILE, FORMAT_ZOOKEEPER_CONTAINER_NAME.as_ref(), FORMAT_ZOOKEEPER_LOG_FILE, @@ -124,10 +117,7 @@ pub fn build_log4j_configs(merged_config: &AnyNodeConfig) -> Vec<(&'static str, ); add_log4j_config_if_automatic( &mut configs, - merged_config.as_datanode().map(|dn| { - dn.logging - .for_container(&DataNodeContainer::WaitForNameNodes) - }), + wait_for_namenodes, WAIT_FOR_NAMENODES_LOG4J_CONFIG_FILE, WAIT_FOR_NAMENODES_CONTAINER_NAME.as_ref(), WAIT_FOR_NAMENODES_LOG_FILE, @@ -139,7 +129,7 @@ pub fn build_log4j_configs(merged_config: &AnyNodeConfig) -> Vec<(&'static str, fn add_log4j_config_if_automatic( configs: &mut Vec<(&'static str, String)>, - log_config: Option>, + log_config: Option<&ContainerLogConfig>, log_config_file: &'static str, log_dir_name: &str, log_file: &str, @@ -147,7 +137,7 @@ fn add_log4j_config_if_automatic( ) { if let Some(ContainerLogConfig { choice: Some(ContainerLogConfigChoice::Automatic(log_config)), - }) = log_config.as_deref() + }) = log_config { configs.push(( log_config_file, diff --git a/rust/operator-binary/src/controller/build/resource/config_map.rs b/rust/operator-binary/src/controller/build/resource/config_map.rs index 154373cb..c098d44d 100644 --- a/rust/operator-binary/src/controller/build/resource/config_map.rs +++ b/rust/operator-binary/src/controller/build/resource/config_map.rs @@ -20,7 +20,7 @@ use crate::{ }, }, }, - crd::HdfsNodeRole, + crd::{DataNodeContainer, HdfsNodeRole, NameNodeContainer}, }; #[derive(Snafu, Debug)] @@ -108,7 +108,36 @@ pub fn build_rolegroup_config_map( )?, ); - for (log_config_file, log4j_config) in product_logging::build_log4j_configs(merged_config) { + let hdfs_logging = merged_config.hdfs_logging(); + let (zkfc_logging, format_namenodes_logging, format_zookeeper_logging) = + match merged_config.as_namenode() { + Some(namenode) => ( + Some(namenode.logging.for_container(&NameNodeContainer::Zkfc)), + Some( + namenode + .logging + .for_container(&NameNodeContainer::FormatNameNodes), + ), + Some( + namenode + .logging + .for_container(&NameNodeContainer::FormatZooKeeper), + ), + ), + None => (None, None, None), + }; + let wait_for_namenodes_logging = merged_config.as_datanode().map(|dn| { + dn.logging + .for_container(&DataNodeContainer::WaitForNameNodes) + }); + let log4j_configs = product_logging::build_log4j_configs( + Some(&*hdfs_logging), + zkfc_logging.as_deref(), + format_namenodes_logging.as_deref(), + format_zookeeper_logging.as_deref(), + wait_for_namenodes_logging.as_deref(), + ); + for (log_config_file, log4j_config) in log4j_configs { builder.add_data(log_config_file, log4j_config); } if merged_config.vector_logging_enabled() { From becf22e7749b0aa991bbeee05aa6a47801fb7650 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Fri, 11 Sep 2026 18:13:41 +0200 Subject: [PATCH 03/19] hdfs_site::build takes the datanode storage config --- .../controller/build/properties/hdfs_site.rs | 30 ++++--------------- .../controller/build/resource/config_map.rs | 4 ++- 2 files changed, 9 insertions(+), 25 deletions(-) diff --git a/rust/operator-binary/src/controller/build/properties/hdfs_site.rs b/rust/operator-binary/src/controller/build/properties/hdfs_site.rs index 3bf3b42c..e088e591 100644 --- a/rust/operator-binary/src/controller/build/properties/hdfs_site.rs +++ b/rust/operator-binary/src/controller/build/properties/hdfs_site.rs @@ -13,7 +13,7 @@ use stackable_operator::{ use crate::{ controller::{ValidatedCluster, build}, crd::{ - AnyNodeConfig, HdfsNodeRole, HdfsPodRef, + HdfsNodeRole, HdfsPodRef, constants::{ DEFAULT_JOURNAL_NODE_RPC_PORT, DEFAULT_NAME_NODE_HTTP_PORT, DEFAULT_NAME_NODE_HTTPS_PORT, DEFAULT_NAME_NODE_RPC_PORT, DFS_DATANODE_DATA_DIR, @@ -32,7 +32,7 @@ use crate::{ pub fn build( cluster: &ValidatedCluster, cluster_info: &KubernetesClusterInfo, - merged_config: &AnyNodeConfig, + datanode_storage: Option, overrides: KeyValueConfigOverrides, ) -> String { let cluster_config = &cluster.cluster_config; @@ -52,11 +52,7 @@ pub fn build( let mut hdfs_site = HdfsSiteConfigBuilder::new(cluster.name.as_ref().to_owned()); hdfs_site .dfs_namenode_name_dir() - .dfs_datanode_data_dir( - merged_config - .as_datanode() - .map(|node| node.resources.storage.clone()), - ) + .dfs_datanode_data_dir(datanode_storage) .dfs_journalnode_edits_dir() .dfs_replication(cluster_config.dfs_replication) .dfs_name_services() @@ -335,28 +331,15 @@ mod tests { use indoc::indoc; use super::*; - use crate::{ - controller::build::properties::test_support::{cluster_info, validated_cluster}, - crd::HdfsNodeRole, - test_support::{anynode_config, role_group_name}, - }; - - fn namenode_merged_config(validated_cluster: &ValidatedCluster) -> &AnyNodeConfig { - anynode_config( - validated_cluster, - &HdfsNodeRole::Name, - &role_group_name("default"), - ) - } + use crate::controller::build::properties::test_support::{cluster_info, validated_cluster}; #[test] fn renders_operator_defaults() { let validated_cluster = validated_cluster(); - let merged = namenode_merged_config(&validated_cluster); let xml = build( &validated_cluster, &cluster_info(), - merged, + None, KeyValueConfigOverrides::default(), ); assert!( @@ -376,11 +359,10 @@ mod tests { #[test] fn user_overrides_win_over_defaults() { let validated_cluster = validated_cluster(); - let merged = namenode_merged_config(&validated_cluster); let xml = build( &validated_cluster, &cluster_info(), - merged, + None, [("dfs.replication", "5")].into(), ); assert!( diff --git a/rust/operator-binary/src/controller/build/resource/config_map.rs b/rust/operator-binary/src/controller/build/resource/config_map.rs index c098d44d..2876b9a9 100644 --- a/rust/operator-binary/src/controller/build/resource/config_map.rs +++ b/rust/operator-binary/src/controller/build/resource/config_map.rs @@ -72,7 +72,9 @@ pub fn build_rolegroup_config_map( let hdfs_site_xml = hdfs_site::build( cluster, cluster_info, - merged_config, + merged_config + .as_datanode() + .map(|node| node.resources.storage.clone()), config_overrides.hdfs_site_xml.clone(), ); let core_site_xml = core_site::build( From 4f7929b2781be8be69160aeff3b9784a633036de Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Mon, 14 Sep 2026 08:49:07 +0200 Subject: [PATCH 04/19] container resources, PVCs and volumes take resolved values --- .../src/controller/build/container.rs | 197 ++++++++++-------- .../src/controller/build/jvm.rs | 5 +- .../controller/build/resource/statefulset.rs | 18 +- 3 files changed, 126 insertions(+), 94 deletions(-) diff --git a/rust/operator-binary/src/controller/build/container.rs b/rust/operator-binary/src/controller/build/container.rs index c3a5e298..6fcfe882 100644 --- a/rust/operator-binary/src/controller/build/container.rs +++ b/rust/operator-binary/src/controller/build/container.rs @@ -79,8 +79,8 @@ use crate::{ }, }, crd::{ - AnyNodeConfig, DataNodeContainer, HdfsNodeRole, HdfsPodRef, NameNodeContainer, - UpgradeState, + AnyNodeConfig, DataNodeConfig, DataNodeContainer, HdfsNodeRole, HdfsPodRef, + JournalNodeConfig, NameNodeConfig, NameNodeContainer, UpgradeState, constants::{ DATANODE_ROOT_DATA_DIR_PREFIX, DEFAULT_DATA_NODE_METRICS_PORT, DEFAULT_JOURNAL_NODE_METRICS_PORT, DEFAULT_NAME_NODE_METRICS_PORT, LISTENER_VOLUME_DIR, @@ -168,6 +168,20 @@ pub enum Error { }, } +/// Converts a role group's merged `resources` into Kubernetes resource requirements. +/// +/// This is temporary scaffolding: once the callers are typed per role, each of them converts its +/// own role group's `resources` directly and this function goes away. +pub(crate) fn role_group_resource_requirements( + merged_config: &AnyNodeConfig, +) -> ResourceRequirements { + match merged_config { + AnyNodeConfig::Name(config) => config.resources.clone().into(), + AnyNodeConfig::Data(config) => config.resources.clone().into(), + AnyNodeConfig::Journal(config) => config.resources.clone().into(), + } +} + /// ContainerConfig contains information to create all main, side and init containers for /// the HDFS cluster. #[derive(Display)] @@ -228,14 +242,31 @@ impl ContainerConfig { let object_name = resource_names.qualified_role_group_name().to_string(); let merged_config = &rolegroup_config.config; - pb.add_volumes(main_container_config.volumes(merged_config, &object_name, labels)?) + // Datanodes use ephemeral listener volumes while namenodes use persistent + // volume claim templates for stable per-pod identity. + let listener_volume = match merged_config { + AnyNodeConfig::Data(config) => Some( + VolumeBuilder::new(&*LISTENER_VOLUME_NAME) + .ephemeral( + ListenerOperatorVolumeSourceBuilder::new( + &ListenerReference::ListenerClass(config.listener_class.to_string()), + labels, + ) + .build_ephemeral() + .context(BuildListenerVolumeSnafu)?, + ) + .build(), + ), + AnyNodeConfig::Name(_) | AnyNodeConfig::Journal(_) => None, + }; + + pb.add_volumes(main_container_config.volumes(merged_config, listener_volume, &object_name)) .context(AddVolumeSnafu)?; pb.add_container(main_container_config.main_container( cluster, cluster_info, role, rolegroup_config, - labels, )?); // Vector sidecar container. @@ -335,27 +366,22 @@ impl ContainerConfig { HdfsNodeRole::Name => { // Zookeeper fail over container let zkfc_container_config = Self::Zkfc; - pb.add_volumes(zkfc_container_config.volumes( - merged_config, - &object_name, - labels, - )?) - .context(AddVolumeSnafu)?; + pb.add_volumes(zkfc_container_config.volumes(merged_config, None, &object_name)) + .context(AddVolumeSnafu)?; pb.add_container(zkfc_container_config.main_container( cluster, cluster_info, role, rolegroup_config, - labels, )?); // Format namenode init container let format_namenodes_container_config = Self::FormatNameNodes; pb.add_volumes(format_namenodes_container_config.volumes( merged_config, + None, &object_name, - labels, - )?) + )) .context(AddVolumeSnafu)?; pb.add_init_container(format_namenodes_container_config.init_container( cluster, @@ -363,16 +389,15 @@ impl ContainerConfig { role, rolegroup_config, &namenode_podrefs, - labels, )?); // Format ZooKeeper init container let format_zookeeper_container_config = Self::FormatZooKeeper; pb.add_volumes(format_zookeeper_container_config.volumes( merged_config, + None, &object_name, - labels, - )?) + )) .context(AddVolumeSnafu)?; pb.add_init_container(format_zookeeper_container_config.init_container( cluster, @@ -380,7 +405,6 @@ impl ContainerConfig { role, rolegroup_config, &namenode_podrefs, - labels, )?); } HdfsNodeRole::Data => { @@ -388,9 +412,9 @@ impl ContainerConfig { let wait_for_namenodes_container_config = Self::WaitForNameNodes; pb.add_volumes(wait_for_namenodes_container_config.volumes( merged_config, + None, &object_name, - labels, - )?) + )) .context(AddVolumeSnafu)?; pb.add_init_container(wait_for_namenodes_container_config.init_container( cluster, @@ -398,7 +422,6 @@ impl ContainerConfig { role, rolegroup_config, &namenode_podrefs, - labels, )?); } HdfsNodeRole::Journal => {} @@ -407,49 +430,54 @@ impl ContainerConfig { Ok(()) } - pub fn volume_claim_templates( - merged_config: &AnyNodeConfig, + /// The PVC templates for a namenode role group: one data PVC plus the listener PVC. + pub fn namenode_volume_claim_templates( + config: &NameNodeConfig, labels: &Labels, ) -> Result> { - match merged_config { - AnyNodeConfig::Name(node) => { - let listener = ListenerOperatorVolumeSourceBuilder::new( - &ListenerReference::ListenerClass(node.listener_class.to_string()), - labels, - ) - .build_ephemeral() - .context(BuildListenerVolumeSnafu)? - .volume_claim_template - .expect("The listener volume source builder always sets a volume claim template."); - - let pvcs = vec![ - node.resources.storage.data.build_pvc( - ContainerConfig::DATA_VOLUME_MOUNT_NAME, - Some(vec!["ReadWriteOnce"]), - ), - PersistentVolumeClaim { - metadata: ObjectMeta { - name: Some(LISTENER_VOLUME_NAME.to_string()), - ..listener.metadata.expect( - "The listener volume claim template always carries metadata.", - ) - }, - spec: Some(listener.spec), - ..Default::default() - }, - ]; + let listener = ListenerOperatorVolumeSourceBuilder::new( + &ListenerReference::ListenerClass(config.listener_class.to_string()), + labels, + ) + .build_ephemeral() + .context(BuildListenerVolumeSnafu)? + .volume_claim_template + .expect("The listener volume source builder always sets a volume claim template."); - Ok(pvcs) - } - AnyNodeConfig::Journal(node) => Ok(vec![node.resources.storage.data.build_pvc( + Ok(vec![ + config.resources.storage.data.build_pvc( ContainerConfig::DATA_VOLUME_MOUNT_NAME, Some(vec!["ReadWriteOnce"]), - )]), - AnyNodeConfig::Data(node) => Ok(DataNodeStorageConfig { - pvcs: node.resources.storage.clone(), - } - .build_pvcs()), + ), + PersistentVolumeClaim { + metadata: ObjectMeta { + name: Some(LISTENER_VOLUME_NAME.to_string()), + ..listener + .metadata + .expect("The listener volume claim template always carries metadata.") + }, + spec: Some(listener.spec), + ..Default::default() + }, + ]) + } + + /// The PVC template for a journalnode role group: one data PVC. + pub fn journalnode_volume_claim_templates( + config: &JournalNodeConfig, + ) -> Vec { + vec![config.resources.storage.data.build_pvc( + ContainerConfig::DATA_VOLUME_MOUNT_NAME, + Some(vec!["ReadWriteOnce"]), + )] + } + + /// The PVC templates for a datanode role group, one per configured data volume. + pub fn datanode_volume_claim_templates(config: &DataNodeConfig) -> Vec { + DataNodeStorageConfig { + pvcs: config.resources.storage.clone(), } + .build_pvcs() } /// Creates the main/side containers for: @@ -463,18 +491,17 @@ impl ContainerConfig { cluster_info: &KubernetesClusterInfo, role: &HdfsNodeRole, rolegroup_config: &ValidatedRoleGroupConfig, - labels: &Labels, ) -> Result { let merged_config = &rolegroup_config.config; let mut cb = new_container_builder(self.container_name()); - let resources = self.resources(merged_config); + let resources = self.resources(&role_group_resource_requirements(merged_config)); cb.image_from_product_image(&cluster.image) .command(Self::command()) .args(self.args(cluster, cluster_info, role, merged_config, &[])?) .add_env_vars(self.env(cluster, role, rolegroup_config, resources.as_ref())?) - .add_volume_mounts(self.volume_mounts(cluster, merged_config, labels)?) + .add_volume_mounts(self.volume_mounts(cluster, merged_config)?) .context(AddVolumeMountSnafu)? .add_container_ports(self.container_ports(cluster)); @@ -512,7 +539,6 @@ impl ContainerConfig { role: &HdfsNodeRole, rolegroup_config: &ValidatedRoleGroupConfig, namenode_podrefs: &[HdfsPodRef], - labels: &Labels, ) -> Result { let merged_config = &rolegroup_config.config; let mut cb = new_container_builder(self.container_name()); @@ -521,13 +547,13 @@ impl ContainerConfig { .command(Self::command()) .args(self.args(cluster, cluster_info, role, merged_config, namenode_podrefs)?) .add_env_vars(self.env(cluster, role, rolegroup_config, None)?) - .add_volume_mounts(self.volume_mounts(cluster, merged_config, labels)?) + .add_volume_mounts(self.volume_mounts(cluster, merged_config)?) .context(AddVolumeMountSnafu)?; // We use the main app container resources here in contrast to several operators (which use // hardcoded resources) due to the different code structure. // Going forward this should be replaced by calculating init container resources in the pod builder. - if let Some(resources) = self.resources(merged_config) { + if let Some(resources) = self.resources(&role_group_resource_requirements(merged_config)) { cb.resources(resources); } @@ -973,8 +999,13 @@ impl ContainerConfig { Ok(env.into_values().collect()) } - /// Returns the container resources. - pub fn resources(&self, merged_config: &AnyNodeConfig) -> Option { + /// Returns the container resources. `role_group_resources` is the role group's own + /// `resources`, already converted, and is used by the main and init containers; the ZKFC + /// sidecar has fixed requirements of its own. + pub fn resources( + &self, + role_group_resources: &ResourceRequirements, + ) -> Option { match self { // Namenode sidecar containers ContainerConfig::Zkfc => Some( @@ -989,11 +1020,7 @@ impl ContainerConfig { ContainerConfig::Hdfs { .. } | ContainerConfig::FormatNameNodes | ContainerConfig::FormatZooKeeper - | ContainerConfig::WaitForNameNodes => match merged_config { - AnyNodeConfig::Name(node) => Some(node.resources.clone().into()), - AnyNodeConfig::Data(node) => Some(node.resources.clone().into()), - AnyNodeConfig::Journal(node) => Some(node.resources.clone().into()), - }, + | ContainerConfig::WaitForNameNodes => Some(role_group_resources.clone()), } } @@ -1057,28 +1084,20 @@ impl ContainerConfig { } /// Return the container volumes. + /// + /// `listener_volume` is the role group's ephemeral listener volume, which only the main + /// container of the datanodes has. fn volumes( &self, merged_config: &AnyNodeConfig, + listener_volume: Option, object_name: &str, - labels: &Labels, - ) -> Result> { + ) -> Vec { let mut volumes = vec![]; if let ContainerConfig::Hdfs { .. } = self { - if let AnyNodeConfig::Data(node) = merged_config { - volumes.push( - VolumeBuilder::new(&*LISTENER_VOLUME_NAME) - .ephemeral( - ListenerOperatorVolumeSourceBuilder::new( - &ListenerReference::ListenerClass(node.listener_class.to_string()), - labels, - ) - .build_ephemeral() - .context(BuildListenerVolumeSnafu)?, - ) - .build(), - ); + if let Some(listener_volume) = listener_volume { + volumes.push(listener_volume); } volumes.push( @@ -1125,7 +1144,7 @@ impl ContainerConfig { volume_mount_dirs.log_mount_name(), )); - Ok(volumes) + volumes } /// Returns the container volume mounts. @@ -1133,7 +1152,6 @@ impl ContainerConfig { &self, cluster: &ValidatedCluster, merged_config: &AnyNodeConfig, - labels: &Labels, ) -> Result> { let volume_mount_dirs = self.volume_mount_dirs(); let mut volume_mounts = vec![ @@ -1192,7 +1210,10 @@ impl ContainerConfig { ); } HdfsNodeRole::Data => { - for pvc in Self::volume_claim_templates(merged_config, labels)? { + let config = merged_config.as_datanode().expect( + "A datanode container is only built for a datanode role group.", + ); + for pvc in Self::datanode_volume_claim_templates(config) { let pvc_name = pvc.name_any(); volume_mounts.push(VolumeMount { mount_path: format!("{DATANODE_ROOT_DATA_DIR_PREFIX}{pvc_name}"), diff --git a/rust/operator-binary/src/controller/build/jvm.rs b/rust/operator-binary/src/controller/build/jvm.rs index 54d9d0bd..f697deb1 100644 --- a/rust/operator-binary/src/controller/build/jvm.rs +++ b/rust/operator-binary/src/controller/build/jvm.rs @@ -96,7 +96,7 @@ mod tests { use super::*; use crate::{ - controller::build::container::ContainerConfig, + controller::build::container::{ContainerConfig, role_group_resource_requirements}, crd::constants::DEFAULT_NAME_NODE_METRICS_PORT, test_support::{deserialize_and_validate_cluster, role_group_config, role_group_name}, }; @@ -200,7 +200,8 @@ mod tests { let role_group_config = role_group_config(&validated_cluster, &role, &role_group_name("default")); - let resources = ContainerConfig::from(role).resources(&role_group_config.config); + let resources = ContainerConfig::from(role) + .resources(&role_group_resource_requirements(&role_group_config.config)); construct_role_specific_jvm_args( &role, diff --git a/rust/operator-binary/src/controller/build/resource/statefulset.rs b/rust/operator-binary/src/controller/build/resource/statefulset.rs index dfc8e54d..8ad421fa 100644 --- a/rust/operator-binary/src/controller/build/resource/statefulset.rs +++ b/rust/operator-binary/src/controller/build/resource/statefulset.rs @@ -23,7 +23,7 @@ use crate::{ graceful_shutdown::{self, add_graceful_shutdown_config}, }, }, - crd::HdfsNodeRole, + crd::{AnyNodeConfig, HdfsNodeRole}, }; #[derive(Snafu, Debug)] @@ -107,9 +107,19 @@ pub(crate) fn build_rolegroup_statefulset( let mut pod_template = pb.build_template(); pod_template.merge_from(rolegroup_config.pod_overrides.clone()); - // The same comment regarding labels is valid here as it is for the ContainerConfig::add_containers_and_volumes() call above. - let pvcs = ContainerConfig::volume_claim_templates(merged_config, &rolegroup_selector_labels) - .context(BuildRoleGroupVolumeClaimTemplatesSnafu)?; + // This match is temporary scaffolding: once this function is typed per role, each role + // computes its own PVC templates directly. + let pvcs = match merged_config { + AnyNodeConfig::Name(config) => { + // The same comment regarding labels is valid here as it is for the ContainerConfig::add_containers_and_volumes() call above. + ContainerConfig::namenode_volume_claim_templates(config, &rolegroup_selector_labels) + .context(BuildRoleGroupVolumeClaimTemplatesSnafu)? + } + AnyNodeConfig::Journal(config) => { + ContainerConfig::journalnode_volume_claim_templates(config) + } + AnyNodeConfig::Data(config) => ContainerConfig::datanode_volume_claim_templates(config), + }; let statefulset_spec = StatefulSetSpec { pod_management_policy: Some("OrderedReady".to_string()), From 343dafb5e42dac40e2a2e551d0f61fcdeefaae13 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Mon, 14 Sep 2026 09:00:06 +0200 Subject: [PATCH 05/19] resolve role group logging before the container layer --- .../src/controller/build/container.rs | 114 +++++++++--------- .../src/controller/build/mod.rs | 81 ++++++++++++- .../build/properties/product_logging/mod.rs | 27 ++--- .../controller/build/resource/config_map.rs | 36 +----- .../controller/build/resource/statefulset.rs | 3 + rust/operator-binary/src/crd/mod.rs | 1 + rust/operator-binary/src/hdfs_controller.rs | 3 +- 7 files changed, 158 insertions(+), 107 deletions(-) diff --git a/rust/operator-binary/src/controller/build/container.rs b/rust/operator-binary/src/controller/build/container.rs index 6fcfe882..aee390b0 100644 --- a/rust/operator-binary/src/controller/build/container.rs +++ b/rust/operator-binary/src/controller/build/container.rs @@ -66,7 +66,7 @@ use crate::{ controller::{ ValidatedCluster, ValidatedRoleGroupConfig, build::{ - self, + self, RoleGroupLogging, jvm::{self, construct_global_jvm_args, construct_role_specific_jvm_args}, kerberos::KERBEROS_CONTAINER_PATH, properties::product_logging::{ @@ -79,8 +79,8 @@ use crate::{ }, }, crd::{ - AnyNodeConfig, DataNodeConfig, DataNodeContainer, HdfsNodeRole, HdfsPodRef, - JournalNodeConfig, NameNodeConfig, NameNodeContainer, UpgradeState, + AnyNodeConfig, DataNodeConfig, HdfsNodeRole, HdfsPodRef, JournalNodeConfig, NameNodeConfig, + UpgradeState, constants::{ DATANODE_ROOT_DATA_DIR_PREFIX, DEFAULT_DATA_NODE_METRICS_PORT, DEFAULT_JOURNAL_NODE_METRICS_PORT, DEFAULT_NAME_NODE_METRICS_PORT, LISTENER_VOLUME_DIR, @@ -225,6 +225,9 @@ impl ContainerConfig { const ZKFC_LOG_VOLUME_MOUNT_NAME: &'static str = "zkfc-log-config"; /// Add all main, side and init containers as well as required volumes to the pod builder. + // The parameter list shrinks again once the callers are typed per role and this function + // no longer has to take both the merged config and the data resolved from it. + #[allow(clippy::too_many_arguments)] pub fn add_containers_and_volumes( pb: &mut PodBuilder, cluster: &ValidatedCluster, @@ -232,6 +235,7 @@ impl ContainerConfig { role: &HdfsNodeRole, role_group_name: &RoleGroupName, rolegroup_config: &ValidatedRoleGroupConfig, + logging: &RoleGroupLogging, labels: &Labels, ) -> Result<(), Error> { let namenode_podrefs = build::pod_refs(cluster, &HdfsNodeRole::Name); @@ -260,17 +264,18 @@ impl ContainerConfig { AnyNodeConfig::Name(_) | AnyNodeConfig::Journal(_) => None, }; - pb.add_volumes(main_container_config.volumes(merged_config, listener_volume, &object_name)) + pb.add_volumes(main_container_config.volumes(logging, listener_volume, &object_name)) .context(AddVolumeSnafu)?; pb.add_container(main_container_config.main_container( cluster, cluster_info, role, rolegroup_config, + logging, )?); // Vector sidecar container. - if merged_config.vector_logging_enabled() { + if let Some(vector_logging) = &logging.vector { let vector_aggregator_config_map_name = cluster .cluster_config .logging @@ -278,7 +283,7 @@ impl ContainerConfig { .clone() .context(VectorAggregatorConfigMapMissingSnafu)?; - let log_config = match &*merged_config.vector_logging() { + let log_config = match vector_logging { ContainerLogConfig { choice: Some(ContainerLogConfigChoice::Custom(CustomContainerLogConfig { @@ -366,19 +371,20 @@ impl ContainerConfig { HdfsNodeRole::Name => { // Zookeeper fail over container let zkfc_container_config = Self::Zkfc; - pb.add_volumes(zkfc_container_config.volumes(merged_config, None, &object_name)) + pb.add_volumes(zkfc_container_config.volumes(logging, None, &object_name)) .context(AddVolumeSnafu)?; pb.add_container(zkfc_container_config.main_container( cluster, cluster_info, role, rolegroup_config, + logging, )?); // Format namenode init container let format_namenodes_container_config = Self::FormatNameNodes; pb.add_volumes(format_namenodes_container_config.volumes( - merged_config, + logging, None, &object_name, )) @@ -388,13 +394,14 @@ impl ContainerConfig { cluster_info, role, rolegroup_config, + logging, &namenode_podrefs, )?); // Format ZooKeeper init container let format_zookeeper_container_config = Self::FormatZooKeeper; pb.add_volumes(format_zookeeper_container_config.volumes( - merged_config, + logging, None, &object_name, )) @@ -404,6 +411,7 @@ impl ContainerConfig { cluster_info, role, rolegroup_config, + logging, &namenode_podrefs, )?); } @@ -411,7 +419,7 @@ impl ContainerConfig { // Wait for namenode init container let wait_for_namenodes_container_config = Self::WaitForNameNodes; pb.add_volumes(wait_for_namenodes_container_config.volumes( - merged_config, + logging, None, &object_name, )) @@ -421,6 +429,7 @@ impl ContainerConfig { cluster_info, role, rolegroup_config, + logging, &namenode_podrefs, )?); } @@ -491,6 +500,7 @@ impl ContainerConfig { cluster_info: &KubernetesClusterInfo, role: &HdfsNodeRole, rolegroup_config: &ValidatedRoleGroupConfig, + logging: &RoleGroupLogging, ) -> Result { let merged_config = &rolegroup_config.config; let mut cb = new_container_builder(self.container_name()); @@ -499,9 +509,9 @@ impl ContainerConfig { cb.image_from_product_image(&cluster.image) .command(Self::command()) - .args(self.args(cluster, cluster_info, role, merged_config, &[])?) + .args(self.args(cluster, cluster_info, role, logging, &[])?) .add_env_vars(self.env(cluster, role, rolegroup_config, resources.as_ref())?) - .add_volume_mounts(self.volume_mounts(cluster, merged_config)?) + .add_volume_mounts(self.volume_mounts(cluster, merged_config)) .context(AddVolumeMountSnafu)? .add_container_ports(self.container_ports(cluster)); @@ -538,6 +548,7 @@ impl ContainerConfig { cluster_info: &KubernetesClusterInfo, role: &HdfsNodeRole, rolegroup_config: &ValidatedRoleGroupConfig, + logging: &RoleGroupLogging, namenode_podrefs: &[HdfsPodRef], ) -> Result { let merged_config = &rolegroup_config.config; @@ -545,9 +556,9 @@ impl ContainerConfig { cb.image_from_product_image(&cluster.image) .command(Self::command()) - .args(self.args(cluster, cluster_info, role, merged_config, namenode_podrefs)?) + .args(self.args(cluster, cluster_info, role, logging, namenode_podrefs)?) .add_env_vars(self.env(cluster, role, rolegroup_config, None)?) - .add_volume_mounts(self.volume_mounts(cluster, merged_config)?) + .add_volume_mounts(self.volume_mounts(cluster, merged_config)) .context(AddVolumeMountSnafu)?; // We use the main app container resources here in contrast to several operators (which use @@ -624,7 +635,7 @@ impl ContainerConfig { cluster: &ValidatedCluster, cluster_info: &KubernetesClusterInfo, role: &HdfsNodeRole, - merged_config: &AnyNodeConfig, + logging: &RoleGroupLogging, namenode_podrefs: &[HdfsPodRef], ) -> Result, Error> { let mut args = String::new(); @@ -646,10 +657,14 @@ impl ContainerConfig { match self { ContainerConfig::Hdfs { role, .. } => { - args.push_str(&self.copy_log4j_properties_cmd( - HDFS_LOG4J_CONFIG_FILE, - &merged_config.hdfs_logging(), - )); + if let Some(container_log_config) = &logging.hdfs { + args.push_str( + &self.copy_log4j_properties_cmd( + HDFS_LOG4J_CONFIG_FILE, + container_log_config, + ), + ); + } args.push_str(&formatdoc!( r#"\ @@ -676,12 +691,12 @@ impl ContainerConfig { )); } ContainerConfig::Zkfc => { - if let Some(container_config) = merged_config - .as_namenode() - .map(|node| node.logging.for_container(&NameNodeContainer::Zkfc)) - { + if let Some(container_log_config) = &logging.zkfc { args.push_str( - &self.copy_log4j_properties_cmd(ZKFC_LOG4J_CONFIG_FILE, &container_config), + &self.copy_log4j_properties_cmd( + ZKFC_LOG4J_CONFIG_FILE, + container_log_config, + ), ); } args.push_str(&format!( @@ -692,13 +707,10 @@ impl ContainerConfig { ContainerConfig::FormatNameNodes => { args.push_str(&bash_capture_shell_helper(self.container_name().as_ref())); - if let Some(container_config) = merged_config.as_namenode().map(|node| { - node.logging - .for_container(&NameNodeContainer::FormatNameNodes) - }) { + if let Some(container_log_config) = &logging.format_namenodes { args.push_str(&self.copy_log4j_properties_cmd( FORMAT_NAMENODES_LOG4J_CONFIG_FILE, - &container_config, + container_log_config, )); } // First step we check for active namenodes. This step should return an active namenode @@ -768,13 +780,10 @@ impl ContainerConfig { ContainerConfig::FormatZooKeeper => { args.push_str(&bash_capture_shell_helper(self.container_name().as_ref())); - if let Some(container_config) = merged_config.as_namenode().map(|node| { - node.logging - .for_container(&NameNodeContainer::FormatZooKeeper) - }) { + if let Some(container_log_config) = &logging.format_zookeeper { args.push_str(&self.copy_log4j_properties_cmd( FORMAT_ZOOKEEPER_LOG4J_CONFIG_FILE, - &container_config, + container_log_config, )); } args.push_str(&formatdoc!( @@ -800,13 +809,10 @@ impl ContainerConfig { ContainerConfig::WaitForNameNodes => { args.push_str(&bash_capture_shell_helper(self.container_name().as_ref())); - if let Some(container_config) = merged_config.as_datanode().map(|node| { - node.logging - .for_container(&DataNodeContainer::WaitForNameNodes) - }) { + if let Some(container_log_config) = &logging.wait_for_namenodes { args.push_str(&self.copy_log4j_properties_cmd( WAIT_FOR_NAMENODES_LOG4J_CONFIG_FILE, - &container_config, + container_log_config, )); } if cluster.has_kerberos_enabled() { @@ -1089,7 +1095,7 @@ impl ContainerConfig { /// container of the datanodes has. fn volumes( &self, - merged_config: &AnyNodeConfig, + logging: &RoleGroupLogging, listener_volume: Option, object_name: &str, ) -> Vec { @@ -1119,26 +1125,15 @@ impl ContainerConfig { } let container_log_config = match self { - ContainerConfig::Hdfs { .. } => Some(merged_config.hdfs_logging()), - ContainerConfig::Zkfc => merged_config - .as_namenode() - .map(|node| node.logging.for_container(&NameNodeContainer::Zkfc)), - ContainerConfig::FormatNameNodes => merged_config.as_namenode().map(|node| { - node.logging - .for_container(&NameNodeContainer::FormatNameNodes) - }), - ContainerConfig::FormatZooKeeper => merged_config.as_namenode().map(|node| { - node.logging - .for_container(&NameNodeContainer::FormatZooKeeper) - }), - ContainerConfig::WaitForNameNodes => merged_config.as_datanode().map(|node| { - node.logging - .for_container(&DataNodeContainer::WaitForNameNodes) - }), + ContainerConfig::Hdfs { .. } => logging.hdfs.as_ref(), + ContainerConfig::Zkfc => logging.zkfc.as_ref(), + ContainerConfig::FormatNameNodes => logging.format_namenodes.as_ref(), + ContainerConfig::FormatZooKeeper => logging.format_zookeeper.as_ref(), + ContainerConfig::WaitForNameNodes => logging.wait_for_namenodes.as_ref(), }; let volume_mount_dirs = self.volume_mount_dirs(); volumes.extend(Self::common_container_volumes( - container_log_config.as_deref(), + container_log_config, object_name, volume_mount_dirs.config_mount_name(), volume_mount_dirs.log_mount_name(), @@ -1152,7 +1147,7 @@ impl ContainerConfig { &self, cluster: &ValidatedCluster, merged_config: &AnyNodeConfig, - ) -> Result> { + ) -> Vec { let volume_mount_dirs = self.volume_mount_dirs(); let mut volume_mounts = vec![ VolumeMountBuilder::new(Self::STACKABLE_LOG_VOLUME_MOUNT_NAME, STACKABLE_LOG_DIR) @@ -1230,7 +1225,7 @@ impl ContainerConfig { | ContainerConfig::FormatZooKeeper => {} } - Ok(volume_mounts) + volume_mounts } /// Create a config directory for the respective container. @@ -1545,6 +1540,7 @@ mod tests { use strum::IntoEnumIterator; use super::*; + use crate::crd::{DataNodeContainer, NameNodeContainer}; #[test] fn test_constants() { diff --git a/rust/operator-binary/src/controller/build/mod.rs b/rust/operator-binary/src/controller/build/mod.rs index 7f4ff4fc..7a7e8d4f 100644 --- a/rust/operator-binary/src/controller/build/mod.rs +++ b/rust/operator-binary/src/controller/build/mod.rs @@ -4,6 +4,7 @@ use snafu::{ResultExt, Snafu}; use stackable_operator::{ builder::meta::ObjectMetaBuilder, kvp::{LabelError, Labels}, + product_logging::spec::ContainerLogConfig, utils::cluster_info::KubernetesClusterInfo, v2::{ builder::meta::ownerreference_from_resource, @@ -22,7 +23,7 @@ use crate::{ build::resource::rbac::{build_role_binding, build_service_account}, }, crd::{ - HdfsNodeRole, HdfsPodRef, + AnyNodeConfig, DataNodeContainer, HdfsNodeRole, HdfsPodRef, NameNodeContainer, constants::{ DEFAULT_DATA_NODE_DATA_PORT, DEFAULT_DATA_NODE_HTTP_PORT, DEFAULT_DATA_NODE_HTTPS_PORT, DEFAULT_DATA_NODE_IPC_PORT, DEFAULT_DATA_NODE_METRICS_PORT, @@ -76,6 +77,84 @@ pub enum Error { DiscoveryConfigMap { source: resource::discovery::Error }, } +/// The log configuration of every container in one role group, resolved during the build step +/// by code that knows the role, so the shared builders never see a role-specific +/// `Logging`. +#[derive(Clone, Debug, Default)] +pub struct RoleGroupLogging { + /// The main `hdfs` container. + pub hdfs: Option, + /// The Vector sidecar; `None` when the Vector agent is disabled for this role group. + pub vector: Option, + /// The namenode `zkfc` side container. + pub zkfc: Option, + /// The namenode `format-namenodes` init container. + pub format_namenodes: Option, + /// The namenode `format-zookeeper` init container. + pub format_zookeeper: Option, + /// The datanode `wait-for-namenodes` init container. + pub wait_for_namenodes: Option, +} + +/// Resolves a role group's merged `logging` into the role-agnostic [`RoleGroupLogging`] the +/// shared builders consume, filling in only the containers the role actually has. +/// +/// This is temporary scaffolding: once the callers are typed per role, each of them resolves its +/// own role group's containers directly and this function goes away. +pub(crate) fn role_group_logging(config: &AnyNodeConfig) -> RoleGroupLogging { + let hdfs = Some(config.hdfs_logging().into_owned()); + let vector = config + .vector_logging_enabled() + .then(|| config.vector_logging().into_owned()); + + match config { + AnyNodeConfig::Name(name_node) => RoleGroupLogging { + hdfs, + vector, + zkfc: Some( + name_node + .logging + .for_container(&NameNodeContainer::Zkfc) + .into_owned(), + ), + format_namenodes: Some( + name_node + .logging + .for_container(&NameNodeContainer::FormatNameNodes) + .into_owned(), + ), + format_zookeeper: Some( + name_node + .logging + .for_container(&NameNodeContainer::FormatZooKeeper) + .into_owned(), + ), + wait_for_namenodes: None, + }, + AnyNodeConfig::Data(data_node) => RoleGroupLogging { + hdfs, + vector, + zkfc: None, + format_namenodes: None, + format_zookeeper: None, + wait_for_namenodes: Some( + data_node + .logging + .for_container(&DataNodeContainer::WaitForNameNodes) + .into_owned(), + ), + }, + AnyNodeConfig::Journal(_) => RoleGroupLogging { + hdfs, + vector, + zkfc: None, + format_namenodes: None, + format_zookeeper: None, + wait_for_namenodes: None, + }, + } +} + /// Builds every Kubernetes resource for the given validated cluster. /// /// Does not need a Kubernetes client: every external reference is already dereferenced and diff --git a/rust/operator-binary/src/controller/build/properties/product_logging/mod.rs b/rust/operator-binary/src/controller/build/properties/product_logging/mod.rs index 53dd1f70..ad77747d 100644 --- a/rust/operator-binary/src/controller/build/properties/product_logging/mod.rs +++ b/rust/operator-binary/src/controller/build/properties/product_logging/mod.rs @@ -10,9 +10,12 @@ use stackable_operator::{ v2::product_logging::framework::STACKABLE_LOG_DIR, }; -use crate::controller::build::container::{ - FORMAT_NAMENODES_CONTAINER_NAME, FORMAT_ZOOKEEPER_CONTAINER_NAME, - WAIT_FOR_NAMENODES_CONTAINER_NAME, ZKFC_CONTAINER_NAME, +use crate::controller::build::{ + RoleGroupLogging, + container::{ + FORMAT_NAMENODES_CONTAINER_NAME, FORMAT_ZOOKEEPER_CONTAINER_NAME, + WAIT_FOR_NAMENODES_CONTAINER_NAME, ZKFC_CONTAINER_NAME, + }, }; // We have a maximum of 4 continuous logging files for Namenodes. Datanodes and Journalnodes @@ -74,18 +77,12 @@ pub fn vector_config_file_content() -> String { /// /// Returns `(filename, rendered content)` pairs; containers using a custom log ConfigMap are /// skipped, so the result is empty when none use automatic logging. -pub fn build_log4j_configs( - hdfs: Option<&ContainerLogConfig>, - zkfc: Option<&ContainerLogConfig>, - format_namenodes: Option<&ContainerLogConfig>, - format_zookeeper: Option<&ContainerLogConfig>, - wait_for_namenodes: Option<&ContainerLogConfig>, -) -> Vec<(&'static str, String)> { +pub fn build_log4j_configs(logging: &RoleGroupLogging) -> Vec<(&'static str, String)> { let mut configs = Vec::new(); add_log4j_config_if_automatic( &mut configs, - hdfs, + logging.hdfs.as_ref(), HDFS_LOG4J_CONFIG_FILE, "hdfs", HDFS_LOG_FILE, @@ -93,7 +90,7 @@ pub fn build_log4j_configs( ); add_log4j_config_if_automatic( &mut configs, - zkfc, + logging.zkfc.as_ref(), ZKFC_LOG4J_CONFIG_FILE, ZKFC_CONTAINER_NAME.as_ref(), ZKFC_LOG_FILE, @@ -101,7 +98,7 @@ pub fn build_log4j_configs( ); add_log4j_config_if_automatic( &mut configs, - format_namenodes, + logging.format_namenodes.as_ref(), FORMAT_NAMENODES_LOG4J_CONFIG_FILE, FORMAT_NAMENODES_CONTAINER_NAME.as_ref(), FORMAT_NAMENODES_LOG_FILE, @@ -109,7 +106,7 @@ pub fn build_log4j_configs( ); add_log4j_config_if_automatic( &mut configs, - format_zookeeper, + logging.format_zookeeper.as_ref(), FORMAT_ZOOKEEPER_LOG4J_CONFIG_FILE, FORMAT_ZOOKEEPER_CONTAINER_NAME.as_ref(), FORMAT_ZOOKEEPER_LOG_FILE, @@ -117,7 +114,7 @@ pub fn build_log4j_configs( ); add_log4j_config_if_automatic( &mut configs, - wait_for_namenodes, + logging.wait_for_namenodes.as_ref(), WAIT_FOR_NAMENODES_LOG4J_CONFIG_FILE, WAIT_FOR_NAMENODES_CONTAINER_NAME.as_ref(), WAIT_FOR_NAMENODES_LOG_FILE, diff --git a/rust/operator-binary/src/controller/build/resource/config_map.rs b/rust/operator-binary/src/controller/build/resource/config_map.rs index 2876b9a9..eee7b615 100644 --- a/rust/operator-binary/src/controller/build/resource/config_map.rs +++ b/rust/operator-binary/src/controller/build/resource/config_map.rs @@ -20,7 +20,7 @@ use crate::{ }, }, }, - crd::{DataNodeContainer, HdfsNodeRole, NameNodeContainer}, + crd::HdfsNodeRole, }; #[derive(Snafu, Debug)] @@ -110,39 +110,13 @@ pub fn build_rolegroup_config_map( )?, ); - let hdfs_logging = merged_config.hdfs_logging(); - let (zkfc_logging, format_namenodes_logging, format_zookeeper_logging) = - match merged_config.as_namenode() { - Some(namenode) => ( - Some(namenode.logging.for_container(&NameNodeContainer::Zkfc)), - Some( - namenode - .logging - .for_container(&NameNodeContainer::FormatNameNodes), - ), - Some( - namenode - .logging - .for_container(&NameNodeContainer::FormatZooKeeper), - ), - ), - None => (None, None, None), - }; - let wait_for_namenodes_logging = merged_config.as_datanode().map(|dn| { - dn.logging - .for_container(&DataNodeContainer::WaitForNameNodes) - }); - let log4j_configs = product_logging::build_log4j_configs( - Some(&*hdfs_logging), - zkfc_logging.as_deref(), - format_namenodes_logging.as_deref(), - format_zookeeper_logging.as_deref(), - wait_for_namenodes_logging.as_deref(), - ); + let logging = build::role_group_logging(merged_config); + + let log4j_configs = product_logging::build_log4j_configs(&logging); for (log_config_file, log4j_config) in log4j_configs { builder.add_data(log_config_file, log4j_config); } - if merged_config.vector_logging_enabled() { + if logging.vector.is_some() { builder.add_data( VECTOR_CONFIG_FILE, product_logging::vector_config_file_content(), diff --git a/rust/operator-binary/src/controller/build/resource/statefulset.rs b/rust/operator-binary/src/controller/build/resource/statefulset.rs index 8ad421fa..fc49165d 100644 --- a/rust/operator-binary/src/controller/build/resource/statefulset.rs +++ b/rust/operator-binary/src/controller/build/resource/statefulset.rs @@ -83,6 +83,8 @@ pub(crate) fn build_rolegroup_statefulset( .build(), ); + let logging = build::role_group_logging(merged_config); + // Adds all containers and volumes to the pod builder // We must use the selector labels ("rolegroup_selector_labels") and not the recommended labels // for the ephemeral listener volumes created by this function. @@ -96,6 +98,7 @@ pub(crate) fn build_rolegroup_statefulset( role, role_group_name, rolegroup_config, + &logging, &rolegroup_selector_labels, ) .context(FailedToCreateContainerAndVolumeConfigurationSnafu)?; diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs index 8c254786..4089bfdf 100644 --- a/rust/operator-binary/src/crd/mod.rs +++ b/rust/operator-binary/src/crd/mod.rs @@ -379,6 +379,7 @@ impl Deref for AnyNodeConfig { impl AnyNodeConfig { // Downcasting helpers for each variant + #[allow(unused)] pub fn as_namenode(&self) -> Option<&NameNodeConfig> { if let Self::Name(node) = self { Some(node) diff --git a/rust/operator-binary/src/hdfs_controller.rs b/rust/operator-binary/src/hdfs_controller.rs index 99a2eb07..8b3bfaad 100644 --- a/rust/operator-binary/src/hdfs_controller.rs +++ b/rust/operator-binary/src/hdfs_controller.rs @@ -179,7 +179,7 @@ mod test { use super::*; use crate::{ HDFS_FULL_CONTROLLER_NAME, - controller::build::container::ContainerConfig, + controller::build::{container::ContainerConfig, role_group_logging}, test_support::{deserialize_cluster, role_group_config, validate_cluster}, }; @@ -235,6 +235,7 @@ spec: &role, &role_group_name, role_group_config, + &role_group_logging(&role_group_config.config), &Labels::new(), ) .unwrap(); From fdb9c872e092f282f662cb4bf20ed4edabf15633 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Mon, 14 Sep 2026 10:14:19 +0200 Subject: [PATCH 06/19] statefulset and config map builders take resolved values --- .../src/controller/build/container.rs | 131 +++++++++--------- .../src/controller/build/jvm.rs | 10 +- .../src/controller/build/mod.rs | 91 +++++++++++- .../controller/build/resource/config_map.rs | 42 +++--- .../controller/build/resource/statefulset.rs | 72 +++++----- rust/operator-binary/src/crd/mod.rs | 1 + rust/operator-binary/src/hdfs_controller.rs | 12 +- 7 files changed, 229 insertions(+), 130 deletions(-) diff --git a/rust/operator-binary/src/controller/build/container.rs b/rust/operator-binary/src/controller/build/container.rs index aee390b0..b5b3e19b 100644 --- a/rust/operator-binary/src/controller/build/container.rs +++ b/rust/operator-binary/src/controller/build/container.rs @@ -53,6 +53,7 @@ use stackable_operator::{ STACKABLE_LOG_DIR, ValidatedContainerLogConfigChoice, VectorContainerLogConfig, vector_container, }, + role_utils::{JavaCommonConfig, RoleGroupConfig}, types::{ common::Port, kubernetes::{ConfigMapName, ContainerName, VolumeName}, @@ -64,7 +65,7 @@ use strum::{Display, EnumDiscriminants, IntoStaticStr}; use crate::{ controller::{ - ValidatedCluster, ValidatedRoleGroupConfig, + ValidatedCluster, build::{ self, RoleGroupLogging, jvm::{self, construct_global_jvm_args, construct_role_specific_jvm_args}, @@ -79,8 +80,8 @@ use crate::{ }, }, crd::{ - AnyNodeConfig, DataNodeConfig, HdfsNodeRole, HdfsPodRef, JournalNodeConfig, NameNodeConfig, - UpgradeState, + CommonNodeConfig, DataNodeConfig, HdfsNodeRole, HdfsPodRef, JournalNodeConfig, + NameNodeConfig, UpgradeState, constants::{ DATANODE_ROOT_DATA_DIR_PREFIX, DEFAULT_DATA_NODE_METRICS_PORT, DEFAULT_JOURNAL_NODE_METRICS_PORT, DEFAULT_NAME_NODE_METRICS_PORT, LISTENER_VOLUME_DIR, @@ -92,6 +93,7 @@ use crate::{ SERVICE_PORT_NAME_RPC, STACKABLE_ROOT_DATA_DIR, }, storage::DataNodeStorageConfig, + v1alpha1, }, }; @@ -168,20 +170,6 @@ pub enum Error { }, } -/// Converts a role group's merged `resources` into Kubernetes resource requirements. -/// -/// This is temporary scaffolding: once the callers are typed per role, each of them converts its -/// own role group's `resources` directly and this function goes away. -pub(crate) fn role_group_resource_requirements( - merged_config: &AnyNodeConfig, -) -> ResourceRequirements { - match merged_config { - AnyNodeConfig::Name(config) => config.resources.clone().into(), - AnyNodeConfig::Data(config) => config.resources.clone().into(), - AnyNodeConfig::Journal(config) => config.resources.clone().into(), - } -} - /// ContainerConfig contains information to create all main, side and init containers for /// the HDFS cluster. #[derive(Display)] @@ -225,18 +213,24 @@ impl ContainerConfig { const ZKFC_LOG_VOLUME_MOUNT_NAME: &'static str = "zkfc-log-config"; /// Add all main, side and init containers as well as required volumes to the pod builder. - // The parameter list shrinks again once the callers are typed per role and this function - // no longer has to take both the merged config and the data resolved from it. + /// + /// Every role-specific value is resolved by the caller: `common` is the role group's merged + /// common config, `resources` its container resource requirements, `volume_claim_templates` + /// its PVC templates (the datanode data volumes are mounted from them) and `listener_volume` + /// its ephemeral listener volume, which only datanodes have. #[allow(clippy::too_many_arguments)] - pub fn add_containers_and_volumes( + pub fn add_containers_and_volumes( pb: &mut PodBuilder, cluster: &ValidatedCluster, cluster_info: &KubernetesClusterInfo, role: &HdfsNodeRole, role_group_name: &RoleGroupName, - rolegroup_config: &ValidatedRoleGroupConfig, + rolegroup_config: &RoleGroupConfig, + common: &CommonNodeConfig, + resources: &ResourceRequirements, + volume_claim_templates: &[PersistentVolumeClaim], + listener_volume: Option, logging: &RoleGroupLogging, - labels: &Labels, ) -> Result<(), Error> { let namenode_podrefs = build::pod_refs(cluster, &HdfsNodeRole::Name); @@ -244,25 +238,6 @@ impl ContainerConfig { let main_container_config = Self::from(*role); let resource_names = cluster.role_group_resource_names(role, role_group_name); let object_name = resource_names.qualified_role_group_name().to_string(); - let merged_config = &rolegroup_config.config; - - // Datanodes use ephemeral listener volumes while namenodes use persistent - // volume claim templates for stable per-pod identity. - let listener_volume = match merged_config { - AnyNodeConfig::Data(config) => Some( - VolumeBuilder::new(&*LISTENER_VOLUME_NAME) - .ephemeral( - ListenerOperatorVolumeSourceBuilder::new( - &ListenerReference::ListenerClass(config.listener_class.to_string()), - labels, - ) - .build_ephemeral() - .context(BuildListenerVolumeSnafu)?, - ) - .build(), - ), - AnyNodeConfig::Name(_) | AnyNodeConfig::Journal(_) => None, - }; pb.add_volumes(main_container_config.volumes(logging, listener_volume, &object_name)) .context(AddVolumeSnafu)?; @@ -272,6 +247,8 @@ impl ContainerConfig { role, rolegroup_config, logging, + resources, + volume_claim_templates, )?); // Vector sidecar container. @@ -332,8 +309,8 @@ impl ContainerConfig { .with_format(SecretFormat::TlsPkcs12) .with_tls_pkcs12_password(TLS_STORE_PASSWORD) .with_auto_tls_cert_lifetime( - merged_config - .requested_secret_lifetime() + common + .requested_secret_lifetime .context(MissingSecretLifetimeSnafu)?, ) .build() @@ -379,6 +356,8 @@ impl ContainerConfig { role, rolegroup_config, logging, + resources, + volume_claim_templates, )?); // Format namenode init container @@ -395,6 +374,8 @@ impl ContainerConfig { role, rolegroup_config, logging, + resources, + volume_claim_templates, &namenode_podrefs, )?); @@ -412,6 +393,8 @@ impl ContainerConfig { role, rolegroup_config, logging, + resources, + volume_claim_templates, &namenode_podrefs, )?); } @@ -430,6 +413,8 @@ impl ContainerConfig { role, rolegroup_config, logging, + resources, + volume_claim_templates, &namenode_podrefs, )?); } @@ -489,29 +474,49 @@ impl ContainerConfig { .build_pvcs() } + /// The ephemeral listener volume of a datanode role group. + /// + /// Datanodes use an ephemeral listener volume, while namenodes use a persistent volume claim + /// template for stable per-pod identity (see [`Self::namenode_volume_claim_templates`]) and + /// journalnodes have no listener at all. + pub fn datanode_listener_volume(config: &DataNodeConfig, labels: &Labels) -> Result { + Ok(VolumeBuilder::new(&*LISTENER_VOLUME_NAME) + .ephemeral( + ListenerOperatorVolumeSourceBuilder::new( + &ListenerReference::ListenerClass(config.listener_class.to_string()), + labels, + ) + .build_ephemeral() + .context(BuildListenerVolumeSnafu)?, + ) + .build()) + } + /// Creates the main/side containers for: /// - Namenode main process /// - Namenode ZooKeeper fail over controller (ZKFC) /// - Datanode main process /// - Journalnode main process - fn main_container( + #[allow(clippy::too_many_arguments)] + fn main_container( &self, cluster: &ValidatedCluster, cluster_info: &KubernetesClusterInfo, role: &HdfsNodeRole, - rolegroup_config: &ValidatedRoleGroupConfig, + rolegroup_config: &RoleGroupConfig, logging: &RoleGroupLogging, + role_group_resources: &ResourceRequirements, + volume_claim_templates: &[PersistentVolumeClaim], ) -> Result { - let merged_config = &rolegroup_config.config; let mut cb = new_container_builder(self.container_name()); - let resources = self.resources(&role_group_resource_requirements(merged_config)); + let resources = self.resources(role_group_resources); cb.image_from_product_image(&cluster.image) .command(Self::command()) .args(self.args(cluster, cluster_info, role, logging, &[])?) .add_env_vars(self.env(cluster, role, rolegroup_config, resources.as_ref())?) - .add_volume_mounts(self.volume_mounts(cluster, merged_config)) + .add_volume_mounts(self.volume_mounts(cluster, volume_claim_templates)) .context(AddVolumeMountSnafu)? .add_container_ports(self.container_ports(cluster)); @@ -542,29 +547,31 @@ impl ContainerConfig { /// Creates respective init containers for: /// - Namenode (format-namenodes, format-zookeeper) /// - Datanode (wait-for-namenodes) - fn init_container( + #[allow(clippy::too_many_arguments)] + fn init_container( &self, cluster: &ValidatedCluster, cluster_info: &KubernetesClusterInfo, role: &HdfsNodeRole, - rolegroup_config: &ValidatedRoleGroupConfig, + rolegroup_config: &RoleGroupConfig, logging: &RoleGroupLogging, + role_group_resources: &ResourceRequirements, + volume_claim_templates: &[PersistentVolumeClaim], namenode_podrefs: &[HdfsPodRef], ) -> Result { - let merged_config = &rolegroup_config.config; let mut cb = new_container_builder(self.container_name()); cb.image_from_product_image(&cluster.image) .command(Self::command()) .args(self.args(cluster, cluster_info, role, logging, namenode_podrefs)?) .add_env_vars(self.env(cluster, role, rolegroup_config, None)?) - .add_volume_mounts(self.volume_mounts(cluster, merged_config)) + .add_volume_mounts(self.volume_mounts(cluster, volume_claim_templates)) .context(AddVolumeMountSnafu)?; // We use the main app container resources here in contrast to several operators (which use // hardcoded resources) due to the different code structure. // Going forward this should be replaced by calculating init container resources in the pod builder. - if let Some(resources) = self.resources(&role_group_resource_requirements(merged_config)) { + if let Some(resources) = self.resources(role_group_resources) { cb.resources(resources); } @@ -894,11 +901,11 @@ impl ContainerConfig { } /// Returns the container env variables. - fn env( + fn env( &self, cluster: &ValidatedCluster, role: &HdfsNodeRole, - rolegroup_config: &ValidatedRoleGroupConfig, + rolegroup_config: &RoleGroupConfig, resources: Option<&ResourceRequirements>, ) -> Result, Error> { // Maps env var name to env var object. This allows env_overrides to work @@ -1143,10 +1150,13 @@ impl ContainerConfig { } /// Returns the container volume mounts. + /// + /// `volume_claim_templates` are the role group's PVC templates; the datanode main container + /// mounts one data directory per data PVC, named after it. fn volume_mounts( &self, cluster: &ValidatedCluster, - merged_config: &AnyNodeConfig, + volume_claim_templates: &[PersistentVolumeClaim], ) -> Vec { let volume_mount_dirs = self.volume_mount_dirs(); let mut volume_mounts = vec![ @@ -1205,10 +1215,7 @@ impl ContainerConfig { ); } HdfsNodeRole::Data => { - let config = merged_config.as_datanode().expect( - "A datanode container is only built for a datanode role group.", - ); - for pvc in Self::datanode_volume_claim_templates(config) { + for pvc in volume_claim_templates { let pvc_name = pvc.name_any(); volume_mounts.push(VolumeMount { mount_path: format!("{DATANODE_ROOT_DATA_DIR_PREFIX}{pvc_name}"), @@ -1274,11 +1281,11 @@ impl ContainerConfig { } /// Build HADOOP_{*node}_OPTS for each namenode, datanodes and journalnodes. - fn build_hadoop_opts( + fn build_hadoop_opts( &self, cluster: &ValidatedCluster, resources: Option<&ResourceRequirements>, - rolegroup_config: &ValidatedRoleGroupConfig, + rolegroup_config: &RoleGroupConfig, ) -> Result { match self { ContainerConfig::Hdfs { diff --git a/rust/operator-binary/src/controller/build/jvm.rs b/rust/operator-binary/src/controller/build/jvm.rs index f697deb1..5b735e0e 100644 --- a/rust/operator-binary/src/controller/build/jvm.rs +++ b/rust/operator-binary/src/controller/build/jvm.rs @@ -96,7 +96,7 @@ mod tests { use super::*; use crate::{ - controller::build::container::{ContainerConfig, role_group_resource_requirements}, + controller::build::container::ContainerConfig, crd::constants::DEFAULT_NAME_NODE_METRICS_PORT, test_support::{deserialize_and_validate_cluster, role_group_config, role_group_name}, }; @@ -200,8 +200,12 @@ mod tests { let role_group_config = role_group_config(&validated_cluster, &role, &role_group_name("default")); - let resources = ContainerConfig::from(role) - .resources(&role_group_resource_requirements(&role_group_config.config)); + let namenode_config = role_group_config + .config + .as_namenode() + .expect("the namenode role group config should be a namenode config"); + let resources = + ContainerConfig::from(role).resources(&namenode_config.resources.clone().into()); construct_role_specific_jvm_args( &role, diff --git a/rust/operator-binary/src/controller/build/mod.rs b/rust/operator-binary/src/controller/build/mod.rs index 7a7e8d4f..0315f418 100644 --- a/rust/operator-binary/src/controller/build/mod.rs +++ b/rust/operator-binary/src/controller/build/mod.rs @@ -3,6 +3,7 @@ use std::{collections::HashMap, marker::PhantomData}; use snafu::{ResultExt, Snafu}; use stackable_operator::{ builder::meta::ObjectMetaBuilder, + k8s_openapi::api::core::v1::ResourceRequirements, kvp::{LabelError, Labels}, product_logging::spec::ContainerLogConfig, utils::cluster_info::KubernetesClusterInfo, @@ -20,10 +21,14 @@ use crate::{ controller::{ CONTROLLER_NAME, KubernetesResources, OPERATOR_NAME, PRODUCT_NAME, Prepared, ValidatedCluster, - build::resource::rbac::{build_role_binding, build_service_account}, + build::{ + container::ContainerConfig, + resource::rbac::{build_role_binding, build_service_account}, + }, }, crd::{ - AnyNodeConfig, DataNodeContainer, HdfsNodeRole, HdfsPodRef, NameNodeContainer, + AnyNodeConfig, CommonNodeConfig, DataNodeContainer, HdfsNodeRole, HdfsPodRef, + NameNodeContainer, constants::{ DEFAULT_DATA_NODE_DATA_PORT, DEFAULT_DATA_NODE_HTTP_PORT, DEFAULT_DATA_NODE_HTTPS_PORT, DEFAULT_DATA_NODE_IPC_PORT, DEFAULT_DATA_NODE_METRICS_PORT, @@ -75,6 +80,27 @@ pub enum Error { #[snafu(display("failed to build the discovery ConfigMap"))] DiscoveryConfigMap { source: resource::discovery::Error }, + + #[snafu(display("failed to build selector labels for role {role} role group {role_group}", role = role.as_ref()))] + RoleGroupSelectorLabels { + source: LabelError, + role: HdfsNodeRole, + role_group: RoleGroupName, + }, + + #[snafu(display("failed to build volume claim templates for role {role} role group {role_group}", role = role.as_ref()))] + VolumeClaimTemplates { + source: container::Error, + role: HdfsNodeRole, + role_group: RoleGroupName, + }, + + #[snafu(display("failed to build listener volume for role {role} role group {role_group}", role = role.as_ref()))] + ListenerVolume { + source: container::Error, + role: HdfsNodeRole, + role_group: RoleGroupName, + }, } /// The log configuration of every container in one role group, resolved during the build step @@ -192,12 +218,68 @@ pub fn build( role_group: role_group_name.clone(), })?, ); + // Everything the shared builders need that depends on the role is resolved here, so + // that they never see the `AnyNodeConfig` enum themselves. + // + // These matches are temporary scaffolding: once this loop is unrolled per role, each + // role resolves its own values from its own typed config. + let merged_config = &rg_config.config; + let common: &CommonNodeConfig = merged_config; + let logging = role_group_logging(merged_config); + let resources: ResourceRequirements = match merged_config { + AnyNodeConfig::Name(config) => config.resources.clone().into(), + AnyNodeConfig::Data(config) => config.resources.clone().into(), + AnyNodeConfig::Journal(config) => config.resources.clone().into(), + }; + + // We must use the selector labels and not the recommended labels for the listener + // volumes below. This is because the recommended set contains a "managed-by" label. + // That label triggers the cluster resources to "manage" listeners, which is wrong and + // leads to errors. The listeners are managed by the listener-operator. + let selector_labels = rolegroup_selector_labels(cluster, role, role_group_name) + .context(RoleGroupSelectorLabelsSnafu { + role: *role, + role_group: role_group_name.clone(), + })?; + + // Datanodes use an ephemeral listener volume while namenodes use a persistent volume + // claim template for stable per-pod identity. + let (volume_claim_templates, listener_volume) = match merged_config { + AnyNodeConfig::Name(config) => ( + ContainerConfig::namenode_volume_claim_templates(config, &selector_labels) + .context(VolumeClaimTemplatesSnafu { + role: *role, + role_group: role_group_name.clone(), + })?, + None, + ), + AnyNodeConfig::Data(config) => ( + ContainerConfig::datanode_volume_claim_templates(config), + Some( + ContainerConfig::datanode_listener_volume(config, &selector_labels) + .context(ListenerVolumeSnafu { + role: *role, + role_group: role_group_name.clone(), + })?, + ), + ), + AnyNodeConfig::Journal(config) => ( + ContainerConfig::journalnode_volume_claim_templates(config), + None, + ), + }; + config_maps.push( resource::config_map::build_rolegroup_config_map( cluster, cluster_info, role, role_group_name, + rg_config, + merged_config + .as_datanode() + .map(|config| config.resources.storage.clone()), + &logging, ) .context(ConfigMapSnafu { role: *role, @@ -211,6 +293,11 @@ pub fn build( role, role_group_name, rg_config, + common, + &resources, + volume_claim_templates, + listener_volume, + &logging, ) .context(StatefulSetSnafu { role: *role, diff --git a/rust/operator-binary/src/controller/build/resource/config_map.rs b/rust/operator-binary/src/controller/build/resource/config_map.rs index eee7b615..d353c6a5 100644 --- a/rust/operator-binary/src/controller/build/resource/config_map.rs +++ b/rust/operator-binary/src/controller/build/resource/config_map.rs @@ -1,33 +1,34 @@ //! Build the per-rolegroup `ConfigMap` for the HdfsCluster. -use snafu::{OptionExt, ResultExt, Snafu}; +use snafu::{ResultExt, Snafu}; use stackable_operator::{ builder::configmap::ConfigMapBuilder, k8s_openapi::api::core::v1::ConfigMap, product_logging::framework::VECTOR_CONFIG_FILE, utils::cluster_info::KubernetesClusterInfo, - v2::{config_file_writer::PropertiesWriterError, types::operator::RoleGroupName}, + v2::{ + config_file_writer::PropertiesWriterError, + role_utils::{JavaCommonConfig, RoleGroupConfig}, + types::operator::RoleGroupName, + }, }; use crate::{ controller::{ ValidatedCluster, build::{ - self, + self, RoleGroupLogging, properties::{ ConfigFileName, core_site, hadoop_policy, hdfs_site, product_logging, security_properties, ssl_client, ssl_server, }, }, }, - crd::HdfsNodeRole, + crd::{HdfsNodeRole, storage::DataNodeStorageConfigInnerType, v1alpha1}, }; #[derive(Snafu, Debug)] pub enum Error { - #[snafu(display("the validated cluster has no role group {role_group:?} for role {role:?}"))] - MissingRoleGroup { role: String, role_group: String }, - #[snafu(display("failed to serialize {} for {rolegroup}", ConfigFileName::Security))] JvmSecurityProperties { source: PropertiesWriterError, @@ -44,11 +45,19 @@ pub enum Error { type Result = std::result::Result; -pub fn build_rolegroup_config_map( +/// Builds the [`ConfigMap`] of one role group. +/// +/// Every role-specific value is resolved by the caller: `datanode_storage` is the datanode data +/// volume configuration (`None` for the other roles) and `logging` the log config of each of the +/// role group's containers. +pub fn build_rolegroup_config_map( cluster: &ValidatedCluster, cluster_info: &KubernetesClusterInfo, role: &HdfsNodeRole, role_group_name: &RoleGroupName, + rolegroup_config: &RoleGroupConfig, + datanode_storage: Option, + logging: &RoleGroupLogging, ) -> Result { tracing::info!( "Setting up ConfigMap for role {role} role group {role_group_name}", @@ -57,24 +66,13 @@ pub fn build_rolegroup_config_map( let metadata = build::rolegroup_metadata(cluster, role, role_group_name); - let rolegroup_config = cluster - .role_groups - .get(role) - .and_then(|role_groups| role_groups.get(role_group_name)) - .with_context(|| MissingRoleGroupSnafu { - role: role.to_string(), - role_group: role_group_name.to_string(), - })?; - let merged_config = &rolegroup_config.config; let config_overrides = &rolegroup_config.config_overrides; let cluster_config = &cluster.cluster_config; let hdfs_site_xml = hdfs_site::build( cluster, cluster_info, - merged_config - .as_datanode() - .map(|node| node.resources.storage.clone()), + datanode_storage, config_overrides.hdfs_site_xml.clone(), ); let core_site_xml = core_site::build( @@ -110,9 +108,7 @@ pub fn build_rolegroup_config_map( )?, ); - let logging = build::role_group_logging(merged_config); - - let log4j_configs = product_logging::build_log4j_configs(&logging); + let log4j_configs = product_logging::build_log4j_configs(logging); for (log_config_file, log4j_config) in log4j_configs { builder.add_data(log_config_file, log4j_config); } diff --git a/rust/operator-binary/src/controller/build/resource/statefulset.rs b/rust/operator-binary/src/controller/build/resource/statefulset.rs index fc49165d..61c1e6be 100644 --- a/rust/operator-binary/src/controller/build/resource/statefulset.rs +++ b/rust/operator-binary/src/controller/build/resource/statefulset.rs @@ -5,25 +5,31 @@ use stackable_operator::{ builder::pod::{PodBuilder, security::PodSecurityContextBuilder}, k8s_openapi::{ DeepMerge, - api::apps::v1::{StatefulSet, StatefulSetSpec}, + api::{ + apps::v1::{StatefulSet, StatefulSetSpec}, + core::v1::{PersistentVolumeClaim, ResourceRequirements, Volume}, + }, apimachinery::pkg::apis::meta::v1::LabelSelector, }, kube::api::ObjectMeta, kvp::{LabelError, Labels}, utils::cluster_info::KubernetesClusterInfo, - v2::types::operator::RoleGroupName, + v2::{ + role_utils::{JavaCommonConfig, RoleGroupConfig}, + types::operator::RoleGroupName, + }, }; use crate::{ controller::{ - ValidatedCluster, ValidatedRoleGroupConfig, + ValidatedCluster, build::{ - self, + self, RoleGroupLogging, container::{self, ContainerConfig}, graceful_shutdown::{self, add_graceful_shutdown_config}, }, }, - crd::{AnyNodeConfig, HdfsNodeRole}, + crd::{CommonNodeConfig, HdfsNodeRole, v1alpha1}, }; #[derive(Snafu, Debug)] @@ -36,17 +42,26 @@ pub enum Error { #[snafu(display("failed to configure graceful shutdown"))] GracefulShutdown { source: graceful_shutdown::Error }, - - #[snafu(display("failed to build role-group volume claim templates from config"))] - BuildRoleGroupVolumeClaimTemplates { source: container::Error }, } -pub(crate) fn build_rolegroup_statefulset( +/// Builds the [`StatefulSet`] of one role group. +/// +/// Every role-specific value is resolved by the caller: `common` is the role group's merged +/// common config, `resources` its container resource requirements, `volume_claim_templates` its +/// PVC templates, `listener_volume` its ephemeral listener volume (datanodes only) and `logging` +/// the log config of each of its containers. +#[allow(clippy::too_many_arguments)] +pub(crate) fn build_rolegroup_statefulset( validated: &ValidatedCluster, cluster_info: &KubernetesClusterInfo, role: &HdfsNodeRole, role_group_name: &RoleGroupName, - rolegroup_config: &ValidatedRoleGroupConfig, + rolegroup_config: &RoleGroupConfig, + common: &CommonNodeConfig, + resources: &ResourceRequirements, + volume_claim_templates: Vec, + listener_volume: Option, + logging: &RoleGroupLogging, ) -> Result { tracing::info!( "Setting up StatefulSet for role {role} role group {role_group_name}", @@ -54,7 +69,6 @@ pub(crate) fn build_rolegroup_statefulset( ); let image = &validated.image; - let merged_config = &rolegroup_config.config; // PodBuilder for StatefulSet Pod template. let mut pb = PodBuilder::new(); @@ -70,7 +84,7 @@ pub(crate) fn build_rolegroup_statefulset( pb.metadata(pb_metadata) .image_pull_secrets_from_product_image(image) - .affinity(&merged_config.affinity) + .affinity(&common.affinity) .service_account_name( validated .cluster_resource_names() @@ -83,14 +97,7 @@ pub(crate) fn build_rolegroup_statefulset( .build(), ); - let logging = build::role_group_logging(merged_config); - - // Adds all containers and volumes to the pod builder - // We must use the selector labels ("rolegroup_selector_labels") and not the recommended labels - // for the ephemeral listener volumes created by this function. - // This is because the recommended set contains a "managed-by" label. This label triggers - // the cluster resources to "manage" listeners which is wrong and leads to errors. - // The listeners are managed by the listener-operator. + // Adds all containers and volumes to the pod builder. ContainerConfig::add_containers_and_volumes( &mut pb, validated, @@ -98,32 +105,21 @@ pub(crate) fn build_rolegroup_statefulset( role, role_group_name, rolegroup_config, - &logging, - &rolegroup_selector_labels, + common, + resources, + &volume_claim_templates, + listener_volume, + logging, ) .context(FailedToCreateContainerAndVolumeConfigurationSnafu)?; - add_graceful_shutdown_config(merged_config, &mut pb).context(GracefulShutdownSnafu)?; + add_graceful_shutdown_config(common, &mut pb).context(GracefulShutdownSnafu)?; // The `podOverrides` were already merged (role <- role group) during validation // by the local-`framework` `with_validated_config`. let mut pod_template = pb.build_template(); pod_template.merge_from(rolegroup_config.pod_overrides.clone()); - // This match is temporary scaffolding: once this function is typed per role, each role - // computes its own PVC templates directly. - let pvcs = match merged_config { - AnyNodeConfig::Name(config) => { - // The same comment regarding labels is valid here as it is for the ContainerConfig::add_containers_and_volumes() call above. - ContainerConfig::namenode_volume_claim_templates(config, &rolegroup_selector_labels) - .context(BuildRoleGroupVolumeClaimTemplatesSnafu)? - } - AnyNodeConfig::Journal(config) => { - ContainerConfig::journalnode_volume_claim_templates(config) - } - AnyNodeConfig::Data(config) => ContainerConfig::datanode_volume_claim_templates(config), - }; - let statefulset_spec = StatefulSetSpec { pod_management_policy: Some("OrderedReady".to_string()), replicas: rolegroup_config.replicas.map(i32::from), @@ -138,7 +134,7 @@ pub(crate) fn build_rolegroup_statefulset( ), template: pod_template, - volume_claim_templates: Some(pvcs), + volume_claim_templates: Some(volume_claim_templates), ..StatefulSetSpec::default() }; diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs index 4089bfdf..e41a7525 100644 --- a/rust/operator-binary/src/crd/mod.rs +++ b/rust/operator-binary/src/crd/mod.rs @@ -433,6 +433,7 @@ impl AnyNodeConfig { } } + #[allow(unused)] pub fn requested_secret_lifetime(&self) -> Option { match self { AnyNodeConfig::Name(node) => node.common.requested_secret_lifetime, diff --git a/rust/operator-binary/src/hdfs_controller.rs b/rust/operator-binary/src/hdfs_controller.rs index 8b3bfaad..a8b19d04 100644 --- a/rust/operator-binary/src/hdfs_controller.rs +++ b/rust/operator-binary/src/hdfs_controller.rs @@ -180,7 +180,7 @@ mod test { use crate::{ HDFS_FULL_CONTROLLER_NAME, controller::build::{container::ContainerConfig, role_group_logging}, - test_support::{deserialize_cluster, role_group_config, validate_cluster}, + test_support::{datanode_config, deserialize_cluster, role_group_config, validate_cluster}, }; #[test] @@ -223,6 +223,8 @@ spec: let validated_cluster = validate_cluster(&hdfs); let role_group_name = RoleGroupName::from_str("default").unwrap(); let role_group_config = role_group_config(&validated_cluster, &role, &role_group_name); + let datanode_config = datanode_config(&validated_cluster, &role_group_name); + let labels = Labels::new(); let mut pb = PodBuilder::new(); pb.metadata(ObjectMeta::default()); @@ -235,8 +237,14 @@ spec: &role, &role_group_name, role_group_config, + &datanode_config.common, + &datanode_config.resources.clone().into(), + &ContainerConfig::datanode_volume_claim_templates(datanode_config), + Some( + ContainerConfig::datanode_listener_volume(datanode_config, &labels) + .expect("the datanode listener volume should build"), + ), &role_group_logging(&role_group_config.config), - &Labels::new(), ) .unwrap(); let containers = pb.build().unwrap().spec.unwrap().containers; From c552a33369e43f476a53800057f4a9557bcd238e Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Mon, 14 Sep 2026 10:49:36 +0200 Subject: [PATCH 07/19] flatten role group maps into per-role typed fields --- .../src/controller/build/container.rs | 111 ++-- .../src/controller/build/jvm.rs | 15 +- .../src/controller/build/mod.rs | 537 ++++++++++++------ .../build/properties/product_logging/mod.rs | 2 +- .../controller/build/resource/statefulset.rs | 45 +- rust/operator-binary/src/controller/mod.rs | 49 +- .../src/controller/validate.rs | 67 +-- rust/operator-binary/src/crd/affinity.rs | 4 +- rust/operator-binary/src/crd/mod.rs | 91 +-- rust/operator-binary/src/event.rs | 38 +- rust/operator-binary/src/hdfs_controller.rs | 56 +- rust/operator-binary/src/test_support.rs | 72 ++- 12 files changed, 623 insertions(+), 464 deletions(-) diff --git a/rust/operator-binary/src/controller/build/container.rs b/rust/operator-binary/src/controller/build/container.rs index b5b3e19b..6068ea5a 100644 --- a/rust/operator-binary/src/controller/build/container.rs +++ b/rust/operator-binary/src/controller/build/container.rs @@ -67,7 +67,7 @@ use crate::{ controller::{ ValidatedCluster, build::{ - self, RoleGroupLogging, + self, ResolvedRoleGroup, RoleGroupLogging, jvm::{self, construct_global_jvm_args, construct_role_specific_jvm_args}, kerberos::KERBEROS_CONTAINER_PATH, properties::product_logging::{ @@ -80,8 +80,7 @@ use crate::{ }, }, crd::{ - CommonNodeConfig, DataNodeConfig, HdfsNodeRole, HdfsPodRef, JournalNodeConfig, - NameNodeConfig, UpgradeState, + DataNodeConfig, HdfsNodeRole, HdfsPodRef, JournalNodeConfig, NameNodeConfig, UpgradeState, constants::{ DATANODE_ROOT_DATA_DIR_PREFIX, DEFAULT_DATA_NODE_METRICS_PORT, DEFAULT_JOURNAL_NODE_METRICS_PORT, DEFAULT_NAME_NODE_METRICS_PORT, LISTENER_VOLUME_DIR, @@ -214,11 +213,7 @@ impl ContainerConfig { /// Add all main, side and init containers as well as required volumes to the pod builder. /// - /// Every role-specific value is resolved by the caller: `common` is the role group's merged - /// common config, `resources` its container resource requirements, `volume_claim_templates` - /// its PVC templates (the datanode data volumes are mounted from them) and `listener_volume` - /// its ephemeral listener volume, which only datanodes have. - #[allow(clippy::too_many_arguments)] + /// Every role-specific value is resolved by the caller into `resolved`. pub fn add_containers_and_volumes( pb: &mut PodBuilder, cluster: &ValidatedCluster, @@ -226,11 +221,7 @@ impl ContainerConfig { role: &HdfsNodeRole, role_group_name: &RoleGroupName, rolegroup_config: &RoleGroupConfig, - common: &CommonNodeConfig, - resources: &ResourceRequirements, - volume_claim_templates: &[PersistentVolumeClaim], - listener_volume: Option, - logging: &RoleGroupLogging, + resolved: &ResolvedRoleGroup, ) -> Result<(), Error> { let namenode_podrefs = build::pod_refs(cluster, &HdfsNodeRole::Name); @@ -239,20 +230,22 @@ impl ContainerConfig { let resource_names = cluster.role_group_resource_names(role, role_group_name); let object_name = resource_names.qualified_role_group_name().to_string(); - pb.add_volumes(main_container_config.volumes(logging, listener_volume, &object_name)) - .context(AddVolumeSnafu)?; + pb.add_volumes(main_container_config.volumes( + &resolved.logging, + resolved.listener_volume.as_ref(), + &object_name, + )) + .context(AddVolumeSnafu)?; pb.add_container(main_container_config.main_container( cluster, cluster_info, role, rolegroup_config, - logging, - resources, - volume_claim_templates, + resolved, )?); // Vector sidecar container. - if let Some(vector_logging) = &logging.vector { + if let Some(vector_logging) = &resolved.logging.vector { let vector_aggregator_config_map_name = cluster .cluster_config .logging @@ -309,7 +302,8 @@ impl ContainerConfig { .with_format(SecretFormat::TlsPkcs12) .with_tls_pkcs12_password(TLS_STORE_PASSWORD) .with_auto_tls_cert_lifetime( - common + resolved + .common .requested_secret_lifetime .context(MissingSecretLifetimeSnafu)?, ) @@ -348,22 +342,24 @@ impl ContainerConfig { HdfsNodeRole::Name => { // Zookeeper fail over container let zkfc_container_config = Self::Zkfc; - pb.add_volumes(zkfc_container_config.volumes(logging, None, &object_name)) - .context(AddVolumeSnafu)?; + pb.add_volumes(zkfc_container_config.volumes( + &resolved.logging, + None, + &object_name, + )) + .context(AddVolumeSnafu)?; pb.add_container(zkfc_container_config.main_container( cluster, cluster_info, role, rolegroup_config, - logging, - resources, - volume_claim_templates, + resolved, )?); // Format namenode init container let format_namenodes_container_config = Self::FormatNameNodes; pb.add_volumes(format_namenodes_container_config.volumes( - logging, + &resolved.logging, None, &object_name, )) @@ -373,16 +369,14 @@ impl ContainerConfig { cluster_info, role, rolegroup_config, - logging, - resources, - volume_claim_templates, + resolved, &namenode_podrefs, )?); // Format ZooKeeper init container let format_zookeeper_container_config = Self::FormatZooKeeper; pb.add_volumes(format_zookeeper_container_config.volumes( - logging, + &resolved.logging, None, &object_name, )) @@ -392,9 +386,7 @@ impl ContainerConfig { cluster_info, role, rolegroup_config, - logging, - resources, - volume_claim_templates, + resolved, &namenode_podrefs, )?); } @@ -402,7 +394,7 @@ impl ContainerConfig { // Wait for namenode init container let wait_for_namenodes_container_config = Self::WaitForNameNodes; pb.add_volumes(wait_for_namenodes_container_config.volumes( - logging, + &resolved.logging, None, &object_name, )) @@ -412,9 +404,7 @@ impl ContainerConfig { cluster_info, role, rolegroup_config, - logging, - resources, - volume_claim_templates, + resolved, &namenode_podrefs, )?); } @@ -497,26 +487,23 @@ impl ContainerConfig { /// - Namenode ZooKeeper fail over controller (ZKFC) /// - Datanode main process /// - Journalnode main process - #[allow(clippy::too_many_arguments)] fn main_container( &self, cluster: &ValidatedCluster, cluster_info: &KubernetesClusterInfo, role: &HdfsNodeRole, rolegroup_config: &RoleGroupConfig, - logging: &RoleGroupLogging, - role_group_resources: &ResourceRequirements, - volume_claim_templates: &[PersistentVolumeClaim], + resolved: &ResolvedRoleGroup, ) -> Result { let mut cb = new_container_builder(self.container_name()); - let resources = self.resources(role_group_resources); + let resources = self.resources(&resolved.resources); cb.image_from_product_image(&cluster.image) .command(Self::command()) - .args(self.args(cluster, cluster_info, role, logging, &[])?) + .args(self.args(cluster, cluster_info, role, &resolved.logging, &[])?) .add_env_vars(self.env(cluster, role, rolegroup_config, resources.as_ref())?) - .add_volume_mounts(self.volume_mounts(cluster, volume_claim_templates)) + .add_volume_mounts(self.volume_mounts(cluster, &resolved.volume_claim_templates)) .context(AddVolumeMountSnafu)? .add_container_ports(self.container_ports(cluster)); @@ -547,31 +534,34 @@ impl ContainerConfig { /// Creates respective init containers for: /// - Namenode (format-namenodes, format-zookeeper) /// - Datanode (wait-for-namenodes) - #[allow(clippy::too_many_arguments)] fn init_container( &self, cluster: &ValidatedCluster, cluster_info: &KubernetesClusterInfo, role: &HdfsNodeRole, rolegroup_config: &RoleGroupConfig, - logging: &RoleGroupLogging, - role_group_resources: &ResourceRequirements, - volume_claim_templates: &[PersistentVolumeClaim], + resolved: &ResolvedRoleGroup, namenode_podrefs: &[HdfsPodRef], ) -> Result { let mut cb = new_container_builder(self.container_name()); cb.image_from_product_image(&cluster.image) .command(Self::command()) - .args(self.args(cluster, cluster_info, role, logging, namenode_podrefs)?) + .args(self.args( + cluster, + cluster_info, + role, + &resolved.logging, + namenode_podrefs, + )?) .add_env_vars(self.env(cluster, role, rolegroup_config, None)?) - .add_volume_mounts(self.volume_mounts(cluster, volume_claim_templates)) + .add_volume_mounts(self.volume_mounts(cluster, &resolved.volume_claim_templates)) .context(AddVolumeMountSnafu)?; // We use the main app container resources here in contrast to several operators (which use // hardcoded resources) due to the different code structure. // Going forward this should be replaced by calculating init container resources in the pod builder. - if let Some(resources) = self.resources(role_group_resources) { + if let Some(resources) = self.resources(&resolved.resources) { cb.resources(resources); } @@ -664,14 +654,9 @@ impl ContainerConfig { match self { ContainerConfig::Hdfs { role, .. } => { - if let Some(container_log_config) = &logging.hdfs { - args.push_str( - &self.copy_log4j_properties_cmd( - HDFS_LOG4J_CONFIG_FILE, - container_log_config, - ), - ); - } + args.push_str( + &self.copy_log4j_properties_cmd(HDFS_LOG4J_CONFIG_FILE, &logging.hdfs), + ); args.push_str(&formatdoc!( r#"\ @@ -1103,15 +1088,13 @@ impl ContainerConfig { fn volumes( &self, logging: &RoleGroupLogging, - listener_volume: Option, + listener_volume: Option<&Volume>, object_name: &str, ) -> Vec { let mut volumes = vec![]; if let ContainerConfig::Hdfs { .. } = self { - if let Some(listener_volume) = listener_volume { - volumes.push(listener_volume); - } + volumes.extend(listener_volume.cloned()); volumes.push( VolumeBuilder::new(ContainerConfig::STACKABLE_LOG_VOLUME_MOUNT_NAME) @@ -1132,7 +1115,7 @@ impl ContainerConfig { } let container_log_config = match self { - ContainerConfig::Hdfs { .. } => logging.hdfs.as_ref(), + ContainerConfig::Hdfs { .. } => Some(&logging.hdfs), ContainerConfig::Zkfc => logging.zkfc.as_ref(), ContainerConfig::FormatNameNodes => logging.format_namenodes.as_ref(), ContainerConfig::FormatZooKeeper => logging.format_zookeeper.as_ref(), diff --git a/rust/operator-binary/src/controller/build/jvm.rs b/rust/operator-binary/src/controller/build/jvm.rs index 5b735e0e..640ed4de 100644 --- a/rust/operator-binary/src/controller/build/jvm.rs +++ b/rust/operator-binary/src/controller/build/jvm.rs @@ -98,7 +98,10 @@ mod tests { use crate::{ controller::build::container::ContainerConfig, crd::constants::DEFAULT_NAME_NODE_METRICS_PORT, - test_support::{deserialize_and_validate_cluster, role_group_config, role_group_name}, + test_support::{ + deserialize_and_validate_cluster, namenode_config, namenode_role_group_config, + role_group_name, + }, }; #[test] @@ -197,13 +200,9 @@ mod tests { let role = HdfsNodeRole::Name; let validated_cluster = deserialize_and_validate_cluster(hdfs_cluster); - let role_group_config = - role_group_config(&validated_cluster, &role, &role_group_name("default")); - - let namenode_config = role_group_config - .config - .as_namenode() - .expect("the namenode role group config should be a namenode config"); + let role_group_name = role_group_name("default"); + let role_group_config = namenode_role_group_config(&validated_cluster, &role_group_name); + let namenode_config = namenode_config(&validated_cluster, &role_group_name); let resources = ContainerConfig::from(role).resources(&namenode_config.resources.clone().into()); diff --git a/rust/operator-binary/src/controller/build/mod.rs b/rust/operator-binary/src/controller/build/mod.rs index 0315f418..4661909c 100644 --- a/rust/operator-binary/src/controller/build/mod.rs +++ b/rust/operator-binary/src/controller/build/mod.rs @@ -1,15 +1,19 @@ -use std::{collections::HashMap, marker::PhantomData}; +use std::{ + collections::{BTreeMap, HashMap}, + marker::PhantomData, +}; use snafu::{ResultExt, Snafu}; use stackable_operator::{ builder::meta::ObjectMetaBuilder, - k8s_openapi::api::core::v1::ResourceRequirements, + k8s_openapi::api::core::v1::{PersistentVolumeClaim, ResourceRequirements, Service, Volume}, kvp::{LabelError, Labels}, product_logging::spec::ContainerLogConfig, utils::cluster_info::KubernetesClusterInfo, v2::{ builder::meta::ownerreference_from_resource, kvp::label, + role_utils::{JavaCommonConfig, RoleGroupConfig}, types::{ common::Port, operator::{RoleGroupName, RoleName}, @@ -27,7 +31,7 @@ use crate::{ }, }, crd::{ - AnyNodeConfig, CommonNodeConfig, DataNodeContainer, HdfsNodeRole, HdfsPodRef, + CommonNodeConfig, DataNodeContainer, HdfsNodeRole, HdfsPodRef, JournalNodeContainer, NameNodeContainer, constants::{ DEFAULT_DATA_NODE_DATA_PORT, DEFAULT_DATA_NODE_HTTP_PORT, DEFAULT_DATA_NODE_HTTPS_PORT, @@ -44,6 +48,7 @@ use crate::{ SERVICE_PORT_NAME_IPC, SERVICE_PORT_NAME_JMX_METRICS, SERVICE_PORT_NAME_METRICS, SERVICE_PORT_NAME_RPC, }, + v1alpha1, }, }; @@ -106,10 +111,10 @@ pub enum Error { /// The log configuration of every container in one role group, resolved during the build step /// by code that knows the role, so the shared builders never see a role-specific /// `Logging`. -#[derive(Clone, Debug, Default)] +#[derive(Clone, Debug)] pub struct RoleGroupLogging { - /// The main `hdfs` container. - pub hdfs: Option, + /// The main `hdfs` container, which every role has. + pub hdfs: ContainerLogConfig, /// The Vector sidecar; `None` when the Vector agent is disabled for this role group. pub vector: Option, /// The namenode `zkfc` side container. @@ -122,63 +127,32 @@ pub struct RoleGroupLogging { pub wait_for_namenodes: Option, } -/// Resolves a role group's merged `logging` into the role-agnostic [`RoleGroupLogging`] the -/// shared builders consume, filling in only the containers the role actually has. +/// Everything about one role group that the shared builders below cannot derive themselves: the +/// values resolved from its role-specific config, plus the selector labels, which the build loop +/// already needs for the listener volume and the PVC templates. /// -/// This is temporary scaffolding: once the callers are typed per role, each of them resolves its -/// own role group's containers directly and this function goes away. -pub(crate) fn role_group_logging(config: &AnyNodeConfig) -> RoleGroupLogging { - let hdfs = Some(config.hdfs_logging().into_owned()); - let vector = config - .vector_logging_enabled() - .then(|| config.vector_logging().into_owned()); - - match config { - AnyNodeConfig::Name(name_node) => RoleGroupLogging { - hdfs, - vector, - zkfc: Some( - name_node - .logging - .for_container(&NameNodeContainer::Zkfc) - .into_owned(), - ), - format_namenodes: Some( - name_node - .logging - .for_container(&NameNodeContainer::FormatNameNodes) - .into_owned(), - ), - format_zookeeper: Some( - name_node - .logging - .for_container(&NameNodeContainer::FormatZooKeeper) - .into_owned(), - ), - wait_for_namenodes: None, - }, - AnyNodeConfig::Data(data_node) => RoleGroupLogging { - hdfs, - vector, - zkfc: None, - format_namenodes: None, - format_zookeeper: None, - wait_for_namenodes: Some( - data_node - .logging - .for_container(&DataNodeContainer::WaitForNameNodes) - .into_owned(), - ), - }, - AnyNodeConfig::Journal(_) => RoleGroupLogging { - hdfs, - vector, - zkfc: None, - format_namenodes: None, - format_zookeeper: None, - wait_for_namenodes: None, - }, - } +/// Resolving these in the build loop, which knows the role, is what lets the builders be generic +/// over the role group's config type. +pub struct ResolvedRoleGroup { + /// The selector labels of the role group's pods, also used as the `StatefulSet` selector and + /// on its listener volume. + /// + /// We must use the selector labels and not the recommended labels for the listener volumes. + /// This is because the recommended set contains a "managed-by" label. That label triggers the + /// cluster resources to "manage" listeners, which is wrong and leads to errors. The listeners + /// are managed by the listener-operator. + pub selector_labels: Labels, + /// The role group's merged config that is common to every role. + pub common: CommonNodeConfig, + /// The resource requirements of the role group's containers. + pub resources: ResourceRequirements, + /// The `StatefulSet`'s persistent volume claim templates. + pub volume_claim_templates: Vec, + /// The ephemeral listener volume; only datanodes have one (namenodes get their listener from + /// a volume claim template and journalnodes have no listener at all). + pub listener_volume: Option, + /// The log config of each of the role group's containers. + pub logging: RoleGroupLogging, } /// Builds every Kubernetes resource for the given validated cluster. @@ -188,8 +162,10 @@ pub(crate) fn role_group_logging(config: &AnyNodeConfig) -> RoleGroupLogging { /// `cluster_info` carries static cluster information resolved at operator startup (e.g. the /// cluster domain used to build Kerberos principals), not a live client. /// -/// The resources are returned as flat, unordered collections. The reconcile step re-groups the -/// StatefulSets by role to preserve HDFS's ordered, rollout-gated deployment during upgrades. +/// The resources are returned as flat collections. `stateful_sets` is ordered by role — +/// journalnodes, then namenodes, then datanodes — because the apply step rolls them out in that +/// order during upgrades to preserve HDFS's rollout-gated deployment (see +/// [`crate::controller::apply::Applier::apply`]). /// The discovery `ConfigMap` is included when it can be built or re-emitted (see /// [`resource::discovery::build_discovery_config_map`]); it is only absent before its first /// successful build. @@ -202,113 +178,256 @@ pub fn build( let mut stateful_sets = vec![]; let mut pod_disruption_budgets = vec![]; - for (role, role_group_configs) in &cluster.role_groups { - for (role_group_name, rg_config) in role_group_configs { - services.push( - resource::service::rolegroup_headless_service(cluster, role, role_group_name) - .context(ServiceSnafu { - role: *role, - role_group: role_group_name.clone(), - })?, - ); - services.push( - resource::service::rolegroup_metrics_service(cluster, role, role_group_name) - .context(ServiceSnafu { - role: *role, - role_group: role_group_name.clone(), - })?, - ); - // Everything the shared builders need that depends on the role is resolved here, so - // that they never see the `AnyNodeConfig` enum themselves. - // - // These matches are temporary scaffolding: once this loop is unrolled per role, each - // role resolves its own values from its own typed config. - let merged_config = &rg_config.config; - let common: &CommonNodeConfig = merged_config; - let logging = role_group_logging(merged_config); - let resources: ResourceRequirements = match merged_config { - AnyNodeConfig::Name(config) => config.resources.clone().into(), - AnyNodeConfig::Data(config) => config.resources.clone().into(), - AnyNodeConfig::Journal(config) => config.resources.clone().into(), - }; - - // We must use the selector labels and not the recommended labels for the listener - // volumes below. This is because the recommended set contains a "managed-by" label. - // That label triggers the cluster resources to "manage" listeners, which is wrong and - // leads to errors. The listeners are managed by the listener-operator. - let selector_labels = rolegroup_selector_labels(cluster, role, role_group_name) - .context(RoleGroupSelectorLabelsSnafu { + // The roles are built in the order journalnode, namenode, datanode, and the resulting + // `stateful_sets` order is load-bearing: the apply step rolls the StatefulSets out in that + // order during upgrades, each role gated on the previous one (see + // [`crate::controller::apply::Applier::apply`]). + for (role_group_name, rg_config) in &cluster.journalnode_role_group_configs { + let role = &HdfsNodeRole::Journal; + let config = &rg_config.config; + + build_role_group_services(cluster, role, role_group_name, &mut services)?; + + let selector_labels = rolegroup_selector_labels(cluster, role, role_group_name).context( + RoleGroupSelectorLabelsSnafu { + role: *role, + role_group: role_group_name.clone(), + }, + )?; + let resolved = ResolvedRoleGroup { + selector_labels, + common: config.common.clone(), + resources: config.resources.clone().into(), + volume_claim_templates: ContainerConfig::journalnode_volume_claim_templates(config), + listener_volume: None, + logging: RoleGroupLogging { + hdfs: config + .logging + .for_container(&JournalNodeContainer::Hdfs) + .into_owned(), + vector: config.logging.enable_vector_agent.then(|| { + config + .logging + .for_container(&JournalNodeContainer::Vector) + .into_owned() + }), + zkfc: None, + format_namenodes: None, + format_zookeeper: None, + wait_for_namenodes: None, + }, + }; + + config_maps.push( + resource::config_map::build_rolegroup_config_map( + cluster, + cluster_info, + role, + role_group_name, + rg_config, + None, + &resolved.logging, + ) + .context(ConfigMapSnafu { + role: *role, + role_group: role_group_name.clone(), + })?, + ); + stateful_sets.push( + resource::statefulset::build_rolegroup_statefulset( + cluster, + cluster_info, + role, + role_group_name, + rg_config, + resolved, + ) + .context(StatefulSetSnafu { + role: *role, + role_group: role_group_name.clone(), + })?, + ); + } + if let Some(pdb) = resource::pdb::build_pdb(cluster, &HdfsNodeRole::Journal) { + pod_disruption_budgets.push(pdb); + } + + for (role_group_name, rg_config) in &cluster.namenode_role_group_configs { + let role = &HdfsNodeRole::Name; + let config = &rg_config.config; + + build_role_group_services(cluster, role, role_group_name, &mut services)?; + + let selector_labels = rolegroup_selector_labels(cluster, role, role_group_name).context( + RoleGroupSelectorLabelsSnafu { + role: *role, + role_group: role_group_name.clone(), + }, + )?; + // Namenodes get their listener from a persistent volume claim template, for stable + // per-pod identity, rather than from an ephemeral volume. + let volume_claim_templates = + ContainerConfig::namenode_volume_claim_templates(config, &selector_labels).context( + VolumeClaimTemplatesSnafu { role: *role, role_group: role_group_name.clone(), - })?; - - // Datanodes use an ephemeral listener volume while namenodes use a persistent volume - // claim template for stable per-pod identity. - let (volume_claim_templates, listener_volume) = match merged_config { - AnyNodeConfig::Name(config) => ( - ContainerConfig::namenode_volume_claim_templates(config, &selector_labels) - .context(VolumeClaimTemplatesSnafu { - role: *role, - role_group: role_group_name.clone(), - })?, - None, + }, + )?; + let resolved = ResolvedRoleGroup { + selector_labels, + common: config.common.clone(), + resources: config.resources.clone().into(), + volume_claim_templates, + listener_volume: None, + logging: RoleGroupLogging { + hdfs: config + .logging + .for_container(&NameNodeContainer::Hdfs) + .into_owned(), + vector: config.logging.enable_vector_agent.then(|| { + config + .logging + .for_container(&NameNodeContainer::Vector) + .into_owned() + }), + zkfc: Some( + config + .logging + .for_container(&NameNodeContainer::Zkfc) + .into_owned(), ), - AnyNodeConfig::Data(config) => ( - ContainerConfig::datanode_volume_claim_templates(config), - Some( - ContainerConfig::datanode_listener_volume(config, &selector_labels) - .context(ListenerVolumeSnafu { - role: *role, - role_group: role_group_name.clone(), - })?, - ), + format_namenodes: Some( + config + .logging + .for_container(&NameNodeContainer::FormatNameNodes) + .into_owned(), ), - AnyNodeConfig::Journal(config) => ( - ContainerConfig::journalnode_volume_claim_templates(config), - None, + format_zookeeper: Some( + config + .logging + .for_container(&NameNodeContainer::FormatZooKeeper) + .into_owned(), ), - }; - - config_maps.push( - resource::config_map::build_rolegroup_config_map( - cluster, - cluster_info, - role, - role_group_name, - rg_config, - merged_config - .as_datanode() - .map(|config| config.resources.storage.clone()), - &logging, - ) - .context(ConfigMapSnafu { - role: *role, - role_group: role_group_name.clone(), - })?, - ); - stateful_sets.push( - resource::statefulset::build_rolegroup_statefulset( - cluster, - cluster_info, - role, - role_group_name, - rg_config, - common, - &resources, - volume_claim_templates, - listener_volume, - &logging, - ) - .context(StatefulSetSnafu { + wait_for_namenodes: None, + }, + }; + + config_maps.push( + resource::config_map::build_rolegroup_config_map( + cluster, + cluster_info, + role, + role_group_name, + rg_config, + None, + &resolved.logging, + ) + .context(ConfigMapSnafu { + role: *role, + role_group: role_group_name.clone(), + })?, + ); + stateful_sets.push( + resource::statefulset::build_rolegroup_statefulset( + cluster, + cluster_info, + role, + role_group_name, + rg_config, + resolved, + ) + .context(StatefulSetSnafu { + role: *role, + role_group: role_group_name.clone(), + })?, + ); + } + if let Some(pdb) = resource::pdb::build_pdb(cluster, &HdfsNodeRole::Name) { + pod_disruption_budgets.push(pdb); + } + + for (role_group_name, rg_config) in &cluster.datanode_role_group_configs { + let role = &HdfsNodeRole::Data; + let config = &rg_config.config; + + build_role_group_services(cluster, role, role_group_name, &mut services)?; + + let selector_labels = rolegroup_selector_labels(cluster, role, role_group_name).context( + RoleGroupSelectorLabelsSnafu { + role: *role, + role_group: role_group_name.clone(), + }, + )?; + // Datanodes use an ephemeral listener volume, since they need no stable per-pod identity. + let listener_volume = Some( + ContainerConfig::datanode_listener_volume(config, &selector_labels).context( + ListenerVolumeSnafu { role: *role, role_group: role_group_name.clone(), - })?, - ); - } - - if let Some(pdb) = resource::pdb::build_pdb(cluster, role) { - pod_disruption_budgets.push(pdb); - } + }, + )?, + ); + let resolved = ResolvedRoleGroup { + selector_labels, + common: config.common.clone(), + resources: config.resources.clone().into(), + volume_claim_templates: ContainerConfig::datanode_volume_claim_templates(config), + listener_volume, + logging: RoleGroupLogging { + hdfs: config + .logging + .for_container(&DataNodeContainer::Hdfs) + .into_owned(), + vector: config.logging.enable_vector_agent.then(|| { + config + .logging + .for_container(&DataNodeContainer::Vector) + .into_owned() + }), + zkfc: None, + format_namenodes: None, + format_zookeeper: None, + wait_for_namenodes: Some( + config + .logging + .for_container(&DataNodeContainer::WaitForNameNodes) + .into_owned(), + ), + }, + }; + + config_maps.push( + resource::config_map::build_rolegroup_config_map( + cluster, + cluster_info, + role, + role_group_name, + rg_config, + Some(config.resources.storage.clone()), + &resolved.logging, + ) + .context(ConfigMapSnafu { + role: *role, + role_group: role_group_name.clone(), + })?, + ); + stateful_sets.push( + resource::statefulset::build_rolegroup_statefulset( + cluster, + cluster_info, + role, + role_group_name, + rg_config, + resolved, + ) + .context(StatefulSetSnafu { + role: *role, + role_group: role_group_name.clone(), + })?, + ); + } + if let Some(pdb) = resource::pdb::build_pdb(cluster, &HdfsNodeRole::Data) { + pod_disruption_budgets.push(pdb); } // The discovery ConfigMap is skipped only before its first successful build (no namenode @@ -332,6 +451,47 @@ pub fn build( }) } +/// Builds the two Services for one role group. Role-agnostic: it reads nothing from the role +/// config. +fn build_role_group_services( + cluster: &ValidatedCluster, + role: &HdfsNodeRole, + role_group_name: &RoleGroupName, + services: &mut Vec, +) -> Result<(), Error> { + services.push( + resource::service::rolegroup_headless_service(cluster, role, role_group_name).context( + ServiceSnafu { + role: *role, + role_group: role_group_name.clone(), + }, + )?, + ); + services.push( + resource::service::rolegroup_metrics_service(cluster, role, role_group_name).context( + ServiceSnafu { + role: *role, + role_group: role_group_name.clone(), + }, + )?, + ); + + Ok(()) +} + +/// The replica count of every role group in the map, defaulting to one where it is unset. +fn role_group_replicas( + role_group_configs: &BTreeMap< + RoleGroupName, + RoleGroupConfig, + >, +) -> Vec<(&RoleGroupName, u16)> { + role_group_configs + .iter() + .map(|(role_group_name, role_group)| (role_group_name, role_group.replicas.unwrap_or(1))) + .collect() +} + /// Builds the [`HdfsPodRef`]s expected for every pod of the given `role`, across all /// of its role groups. /// @@ -346,17 +506,20 @@ pub(crate) fn pod_refs(cluster: &ValidatedCluster, role: &HdfsNodeRole) -> Vec role_group_replicas(&cluster.namenode_role_group_configs), + HdfsNodeRole::Data => role_group_replicas(&cluster.datanode_role_group_configs), + HdfsNodeRole::Journal => role_group_replicas(&cluster.journalnode_role_group_configs), + }; + + replicas_per_role_group .into_iter() - .flatten() - .flat_map(|(role_group_name, role_group)| { + .flat_map(|(role_group_name, replicas)| { let service_name = cluster.governing_service_name(role, role_group_name); let object_name = service_name.to_string(); let namespace = cluster.namespace.clone(); let ports = ports.clone(); - (0..role_group.replicas.unwrap_or(1)).map(move |i| HdfsPodRef { + (0..replicas).map(move |i| HdfsPodRef { namespace: namespace.clone(), role_group_service_name: service_name.clone(), pod_name: format!("{object_name}-{i}"), @@ -422,11 +585,9 @@ pub(crate) fn rolegroup_selector_labels( /// The total number of datanode replicas across all datanode role groups. pub(crate) fn num_datanodes(cluster: &ValidatedCluster) -> u16 { cluster - .role_groups - .get(&HdfsNodeRole::Data) - .into_iter() - .flatten() - .map(|(_, role_group)| role_group.replicas.unwrap_or(1)) + .datanode_role_group_configs + .values() + .map(|role_group| role_group.replicas.unwrap_or(1)) .sum() } @@ -666,6 +827,32 @@ mod tests { assert_eq!(sorted_names(&resources.role_bindings), ["hdfs-rolebinding"]); } + /// The StatefulSets must come out in role order — journalnodes, then namenodes, then + /// datanodes — because the apply step rolls them out in exactly that order during upgrades, + /// each role gated on the previous one's rollout completing (see + /// [`crate::controller::apply::Applier::apply`]). The other tests here sort the names, which + /// would hide a reordering, so this one asserts on the order as built. + #[test] + fn stateful_sets_are_ordered_by_role() { + let cluster = validated_cluster(); + let resources = build(&cluster, &cluster_info()).expect("build succeeds"); + + let names: Vec = resources + .stateful_sets + .iter() + .filter_map(|stateful_set| stateful_set.meta().name.clone()) + .collect(); + + assert_eq!( + names, + [ + "hdfs-journalnode-default", + "hdfs-namenode-default", + "hdfs-datanode-default", + ] + ); + } + /// With every namenode Listener carrying an ingress address, the build step emits the /// discovery ConfigMap (named after the cluster) alongside the role-group ConfigMaps, so /// the apply step tracks it like any other resource. Without ready Listeners it is diff --git a/rust/operator-binary/src/controller/build/properties/product_logging/mod.rs b/rust/operator-binary/src/controller/build/properties/product_logging/mod.rs index ad77747d..cb7e62fa 100644 --- a/rust/operator-binary/src/controller/build/properties/product_logging/mod.rs +++ b/rust/operator-binary/src/controller/build/properties/product_logging/mod.rs @@ -82,7 +82,7 @@ pub fn build_log4j_configs(logging: &RoleGroupLogging) -> Vec<(&'static str, Str add_log4j_config_if_automatic( &mut configs, - logging.hdfs.as_ref(), + Some(&logging.hdfs), HDFS_LOG4J_CONFIG_FILE, "hdfs", HDFS_LOG_FILE, diff --git a/rust/operator-binary/src/controller/build/resource/statefulset.rs b/rust/operator-binary/src/controller/build/resource/statefulset.rs index 61c1e6be..bd9f1a5a 100644 --- a/rust/operator-binary/src/controller/build/resource/statefulset.rs +++ b/rust/operator-binary/src/controller/build/resource/statefulset.rs @@ -5,14 +5,10 @@ use stackable_operator::{ builder::pod::{PodBuilder, security::PodSecurityContextBuilder}, k8s_openapi::{ DeepMerge, - api::{ - apps::v1::{StatefulSet, StatefulSetSpec}, - core::v1::{PersistentVolumeClaim, ResourceRequirements, Volume}, - }, + api::apps::v1::{StatefulSet, StatefulSetSpec}, apimachinery::pkg::apis::meta::v1::LabelSelector, }, kube::api::ObjectMeta, - kvp::{LabelError, Labels}, utils::cluster_info::KubernetesClusterInfo, v2::{ role_utils::{JavaCommonConfig, RoleGroupConfig}, @@ -24,19 +20,16 @@ use crate::{ controller::{ ValidatedCluster, build::{ - self, RoleGroupLogging, + self, ResolvedRoleGroup, container::{self, ContainerConfig}, graceful_shutdown::{self, add_graceful_shutdown_config}, }, }, - crd::{CommonNodeConfig, HdfsNodeRole, v1alpha1}, + crd::{HdfsNodeRole, v1alpha1}, }; #[derive(Snafu, Debug)] pub enum Error { - #[snafu(display("failed to build roleGroup selector labels"))] - RoleGroupSelectorLabels { source: LabelError }, - #[snafu(display("failed to create container and volume configuration"))] FailedToCreateContainerAndVolumeConfiguration { source: container::Error }, @@ -46,22 +39,14 @@ pub enum Error { /// Builds the [`StatefulSet`] of one role group. /// -/// Every role-specific value is resolved by the caller: `common` is the role group's merged -/// common config, `resources` its container resource requirements, `volume_claim_templates` its -/// PVC templates, `listener_volume` its ephemeral listener volume (datanodes only) and `logging` -/// the log config of each of its containers. -#[allow(clippy::too_many_arguments)] +/// Every role-specific value is resolved by the caller into `resolved`. pub(crate) fn build_rolegroup_statefulset( validated: &ValidatedCluster, cluster_info: &KubernetesClusterInfo, role: &HdfsNodeRole, role_group_name: &RoleGroupName, rolegroup_config: &RoleGroupConfig, - common: &CommonNodeConfig, - resources: &ResourceRequirements, - volume_claim_templates: Vec, - listener_volume: Option, - logging: &RoleGroupLogging, + resolved: ResolvedRoleGroup, ) -> Result { tracing::info!( "Setting up StatefulSet for role {role} role group {role_group_name}", @@ -73,18 +58,14 @@ pub(crate) fn build_rolegroup_statefulset( // PodBuilder for StatefulSet Pod template. let mut pb = PodBuilder::new(); - let rolegroup_selector_labels: Labels = - build::rolegroup_selector_labels(validated, role, role_group_name) - .context(RoleGroupSelectorLabelsSnafu)?; - let pb_metadata = ObjectMeta { - labels: Some(rolegroup_selector_labels.clone().into()), + labels: Some(resolved.selector_labels.clone().into()), ..ObjectMeta::default() }; pb.metadata(pb_metadata) .image_pull_secrets_from_product_image(image) - .affinity(&common.affinity) + .affinity(&resolved.common.affinity) .service_account_name( validated .cluster_resource_names() @@ -105,15 +86,11 @@ pub(crate) fn build_rolegroup_statefulset( role, role_group_name, rolegroup_config, - common, - resources, - &volume_claim_templates, - listener_volume, - logging, + &resolved, ) .context(FailedToCreateContainerAndVolumeConfigurationSnafu)?; - add_graceful_shutdown_config(common, &mut pb).context(GracefulShutdownSnafu)?; + add_graceful_shutdown_config(&resolved.common, &mut pb).context(GracefulShutdownSnafu)?; // The `podOverrides` were already merged (role <- role group) during validation // by the local-`framework` `with_validated_config`. @@ -124,7 +101,7 @@ pub(crate) fn build_rolegroup_statefulset( pod_management_policy: Some("OrderedReady".to_string()), replicas: rolegroup_config.replicas.map(i32::from), selector: LabelSelector { - match_labels: Some(rolegroup_selector_labels.into()), + match_labels: Some(resolved.selector_labels.into()), ..LabelSelector::default() }, service_name: Some( @@ -134,7 +111,7 @@ pub(crate) fn build_rolegroup_statefulset( ), template: pod_template, - volume_claim_templates: Some(volume_claim_templates), + volume_claim_templates: Some(resolved.volume_claim_templates), ..StatefulSetSpec::default() }; diff --git a/rust/operator-binary/src/controller/mod.rs b/rust/operator-binary/src/controller/mod.rs index 26b12b5a..14765786 100644 --- a/rust/operator-binary/src/controller/mod.rs +++ b/rust/operator-binary/src/controller/mod.rs @@ -14,7 +14,7 @@ use stackable_operator::{ v2::{ HasName, HasUid, NameIsValidLabelValue, role_group_utils::{QualifiedRoleGroupName, ResourceNames}, - role_utils::{self, RoleGroupConfig}, + role_utils::{self, JavaCommonConfig, RoleGroupConfig}, types::{ kubernetes::{ConfigMapName, NamespaceName, ServiceName, Uid}, operator::{ @@ -29,8 +29,8 @@ use crate::{ HDFS_OPERATOR_NAME, controller::build::opa::HdfsOpaConfig, crd::{ - AnyNodeConfig, HdfsNodeRole, UpgradeState, constants::APP_NAME, - security::AuthenticationConfig, v1alpha1, + DataNodeConfig, HdfsNodeRole, JournalNodeConfig, NameNodeConfig, UpgradeState, + constants::APP_NAME, security::AuthenticationConfig, v1alpha1, }, hdfs_controller::HDFS_CONTROLLER_NAME, }; @@ -53,9 +53,10 @@ pub struct Applied; /// Every Kubernetes resource produced by the build step. /// -/// The resources are flat, unordered collections. The reconcile step re-groups the -/// StatefulSets by role to preserve HDFS's ordered, rollout-gated deployment during -/// upgrades. The discovery `ConfigMap` is part of `config_maps` whenever it can be built or +/// The resources are flat collections. `stateful_sets` is ordered by role — journalnodes, then +/// namenodes, then datanodes — because the apply step rolls them out in that order during +/// upgrades to preserve HDFS's rollout-gated deployment (see [`apply::Applier::apply`]). +/// The discovery `ConfigMap` is part of `config_maps` whenever it can be built or /// re-emitted; it is only absent before its first successful build (see /// [`build::resource::discovery::build_discovery_config_map`]). /// @@ -72,13 +73,17 @@ pub struct KubernetesResources { pub status: PhantomData, } -/// The [`RoleGroupConfig`] specialised for HDFS: the validated config is the -/// per-role [`AnyNodeConfig`], -pub type ValidatedRoleGroupConfig = RoleGroupConfig< - AnyNodeConfig, - stackable_operator::v2::role_utils::JavaCommonConfig, - v1alpha1::HdfsConfigOverrides, ->; +/// The [`RoleGroupConfig`] of one namenode role group. +pub type NameNodeRoleGroupConfig = + RoleGroupConfig; + +/// The [`RoleGroupConfig`] of one datanode role group. +pub type DataNodeRoleGroupConfig = + RoleGroupConfig; + +/// The [`RoleGroupConfig`] of one journalnode role group. +pub type JournalNodeRoleGroupConfig = + RoleGroupConfig; /// The validated cluster: proves that config merging and validation succeeded /// for every role and role group before any resources are created. Placed in the @@ -99,7 +104,15 @@ pub struct ValidatedCluster { pub product_version: ProductVersion, pub image: ResolvedProductImage, pub cluster_config: ValidatedClusterConfig, - pub role_groups: BTreeMap>, + /// The validated config of every namenode role group, keyed by role group name; empty if the + /// role is absent. + pub namenode_role_group_configs: BTreeMap, + /// The validated config of every datanode role group, keyed by role group name; empty if the + /// role is absent. + pub datanode_role_group_configs: BTreeMap, + /// The validated config of every journalnode role group, keyed by role group name; empty if + /// the role is absent. + pub journalnode_role_group_configs: BTreeMap, /// The namenode role-level config (currently the PDB), or `None` if the role is absent. pub namenode_config: Option, /// The datanode role-level config (currently the PDB), or `None` if the role is absent. @@ -125,7 +138,9 @@ impl ValidatedCluster { uid: Uid, image: ResolvedProductImage, cluster_config: ValidatedClusterConfig, - role_groups: BTreeMap>, + namenode_role_group_configs: BTreeMap, + datanode_role_group_configs: BTreeMap, + journalnode_role_group_configs: BTreeMap, namenode_config: Option, datanode_config: Option, journalnode_config: Option, @@ -152,7 +167,9 @@ impl ValidatedCluster { image, product_version, cluster_config, - role_groups, + namenode_role_group_configs, + datanode_role_group_configs, + journalnode_role_group_configs, namenode_config, datanode_config, journalnode_config, diff --git a/rust/operator-binary/src/controller/validate.rs b/rust/operator-binary/src/controller/validate.rs index 2cf4939f..ff0c4d7f 100644 --- a/rust/operator-binary/src/controller/validate.rs +++ b/rust/operator-binary/src/controller/validate.rs @@ -9,20 +9,19 @@ use stackable_operator::{ role_utils::GenericRoleConfig, v2::{ controller_utils::{get_cluster_name, get_namespace, get_uid}, - role_utils::{JavaCommonConfig, Role, with_validated_config}, + role_utils::{JavaCommonConfig, Role, RoleGroupConfig, with_validated_config}, types::operator::RoleGroupName, }, }; -use strum::IntoEnumIterator; use crate::{ controller::{ ValidatedCluster, ValidatedClusterConfig, ValidatedClusterStatus, ValidatedRoleConfig, - ValidatedRoleGroupConfig, dereference::DereferencedObjects, + dereference::DereferencedObjects, }, crd::{ - AnyNodeConfig, DataNodeConfigFragment, HdfsNodeRole, JournalNodeConfigFragment, - NameNodeConfigFragment, UpgradeStateError, v1alpha1, + DataNodeConfigFragment, HdfsNodeRole, JournalNodeConfigFragment, NameNodeConfigFragment, + UpgradeStateError, v1alpha1, }, }; @@ -98,30 +97,20 @@ pub fn validate_cluster( ) }; - let mut role_groups = BTreeMap::new(); let cluster_name = get_cluster_name(hdfs).context(GetClusterNameSnafu)?; - for hdfs_role in HdfsNodeRole::iter() { - let group_configs = match hdfs_role { - HdfsNodeRole::Name => validate_role_group_configs( - hdfs.spec.name_nodes.as_ref(), - NameNodeConfigFragment::default_config(cluster_name.as_ref(), &hdfs_role), - AnyNodeConfig::Name, - )?, - HdfsNodeRole::Data => validate_role_group_configs( - hdfs.spec.data_nodes.as_ref(), - DataNodeConfigFragment::default_config(cluster_name.as_ref(), &hdfs_role), - AnyNodeConfig::Data, - )?, - HdfsNodeRole::Journal => validate_role_group_configs( - hdfs.spec.journal_nodes.as_ref(), - JournalNodeConfigFragment::default_config(cluster_name.as_ref(), &hdfs_role), - AnyNodeConfig::Journal, - )?, - }; - - role_groups.insert(hdfs_role, group_configs); - } + let namenode_role_group_configs = validate_role_group_configs( + hdfs.spec.name_nodes.as_ref(), + NameNodeConfigFragment::default_config(cluster_name.as_ref(), &HdfsNodeRole::Name), + )?; + let datanode_role_group_configs = validate_role_group_configs( + hdfs.spec.data_nodes.as_ref(), + DataNodeConfigFragment::default_config(cluster_name.as_ref(), &HdfsNodeRole::Data), + )?; + let journalnode_role_group_configs = validate_role_group_configs( + hdfs.spec.journal_nodes.as_ref(), + JournalNodeConfigFragment::default_config(cluster_name.as_ref(), &HdfsNodeRole::Journal), + )?; let namespace = get_namespace(hdfs).context(GetClusterNamespaceSnafu)?; let uid = get_uid(hdfs).context(GetClusterUidSnafu)?; @@ -143,7 +132,9 @@ pub fn validate_cluster( uid, image, ValidatedClusterConfig::resolve(hdfs, hdfs_opa_config), - role_groups, + namenode_role_group_configs, + datanode_role_group_configs, + journalnode_role_group_configs, validated_role_config(HdfsNodeRole::Name), validated_role_config(HdfsNodeRole::Data), validated_role_config(HdfsNodeRole::Journal), @@ -159,15 +150,19 @@ pub fn validate_cluster( /// [`with_validated_config`], which folds the CRD config fragment (default <- /// role <- role group) plus the `configOverrides`, `envOverrides`, `cliOverrides` /// and `podOverrides` (role group wins) into a single -/// [`RoleGroupConfig`](stackable_operator::v2::role_utils::RoleGroupConfig). The -/// concrete per-role validated config is wrapped into [`AnyNodeConfig`] via `wrap`. +/// [`RoleGroupConfig`]. /// /// Returns an empty map if the role is not configured. fn validate_role_group_configs( role: Option<&Role>, default_config: Config, - wrap: fn(ValidatedConfig) -> AnyNodeConfig, -) -> Result, Error> +) -> Result< + BTreeMap< + RoleGroupName, + RoleGroupConfig, + >, + Error, +> where Config: Clone + Merge, ValidatedConfig: FromFragment, @@ -188,11 +183,11 @@ where >(role_group, role, &default_config) .context(ValidateRoleGroupConfigSnafu)?; - // Re-wrap the per-role validated config into the role-agnostic - // `AnyNodeConfig`; the merged overrides carry over unchanged. - let validated = ValidatedRoleGroupConfig { + // Flatten the nested config into a single `RoleGroupConfig`; the merged overrides + // carry over unchanged. + let validated = RoleGroupConfig { replicas: validated.replicas, - config: wrap(validated.config.config), + config: validated.config.config, config_overrides: validated.config.config_overrides, env_overrides: validated.config.env_overrides.into(), cli_overrides: validated.config.cli_overrides, diff --git a/rust/operator-binary/src/crd/affinity.rs b/rust/operator-binary/src/crd/affinity.rs index af13dd45..19f7098b 100644 --- a/rust/operator-binary/src/crd/affinity.rs +++ b/rust/operator-binary/src/crd/affinity.rs @@ -43,7 +43,7 @@ mod test { use crate::{ crd::HdfsNodeRole, - test_support::{anynode_config, deserialize_and_validate_cluster, role_group_name}, + test_support::{common_config, deserialize_and_validate_cluster, role_group_name}, }; #[rstest] @@ -78,7 +78,7 @@ spec: "#; let validated_cluster = deserialize_and_validate_cluster(input); - let merged_config = anynode_config(&validated_cluster, &role, &role_group_name("default")); + let merged_config = common_config(&validated_cluster, &role, &role_group_name("default")); assert_eq!( merged_config.affinity, diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs index e41a7525..419c5f56 100644 --- a/rust/operator-binary/src/crd/mod.rs +++ b/rust/operator-binary/src/crd/mod.rs @@ -25,10 +25,7 @@ use stackable_operator::{ deep_merger::ObjectOverrides, k8s_openapi::apimachinery::pkg::api::resource::Quantity, kube::CustomResource, - product_logging::{ - self, - spec::{ContainerLogConfig, Logging}, - }, + product_logging::{self, spec::Logging}, role_utils::GenericRoleConfig, schemars::{self, JsonSchema}, shared::time::Duration, @@ -357,92 +354,6 @@ pub struct CommonNodeConfig { pub requested_secret_lifetime: Option, } -/// Configuration for a rolegroup of an unknown type. -#[derive(Clone, Debug)] -pub enum AnyNodeConfig { - Name(NameNodeConfig), - Data(DataNodeConfig), - Journal(JournalNodeConfig), -} - -impl Deref for AnyNodeConfig { - type Target = CommonNodeConfig; - - fn deref(&self) -> &Self::Target { - match self { - AnyNodeConfig::Name(node) => &node.common, - AnyNodeConfig::Data(node) => &node.common, - AnyNodeConfig::Journal(node) => &node.common, - } - } -} - -impl AnyNodeConfig { - // Downcasting helpers for each variant - #[allow(unused)] - pub fn as_namenode(&self) -> Option<&NameNodeConfig> { - if let Self::Name(node) = self { - Some(node) - } else { - None - } - } - - pub fn as_datanode(&self) -> Option<&DataNodeConfig> { - if let Self::Data(node) = self { - Some(node) - } else { - None - } - } - - #[allow(unused)] - pub fn as_journalnode(&self) -> Option<&JournalNodeConfig> { - if let Self::Journal(node) = self { - Some(node) - } else { - None - } - } - - // Logging config is distinct between each role, due to the different enum types, - // so provide helpers for containers that are common between all roles. - pub fn hdfs_logging(&'_ self) -> Cow<'_, ContainerLogConfig> { - match self { - AnyNodeConfig::Name(node) => node.logging.for_container(&NameNodeContainer::Hdfs), - AnyNodeConfig::Data(node) => node.logging.for_container(&DataNodeContainer::Hdfs), - AnyNodeConfig::Journal(node) => node.logging.for_container(&JournalNodeContainer::Hdfs), - } - } - - pub fn vector_logging(&'_ self) -> Cow<'_, ContainerLogConfig> { - match &self { - AnyNodeConfig::Name(node) => node.logging.for_container(&NameNodeContainer::Vector), - AnyNodeConfig::Data(node) => node.logging.for_container(&DataNodeContainer::Vector), - AnyNodeConfig::Journal(node) => { - node.logging.for_container(&JournalNodeContainer::Vector) - } - } - } - - pub fn vector_logging_enabled(&self) -> bool { - match self { - AnyNodeConfig::Name(node) => node.logging.enable_vector_agent, - AnyNodeConfig::Data(node) => node.logging.enable_vector_agent, - AnyNodeConfig::Journal(node) => node.logging.enable_vector_agent, - } - } - - #[allow(unused)] - pub fn requested_secret_lifetime(&self) -> Option { - match self { - AnyNodeConfig::Name(node) => node.common.requested_secret_lifetime, - AnyNodeConfig::Data(node) => node.common.requested_secret_lifetime, - AnyNodeConfig::Journal(node) => node.common.requested_secret_lifetime, - } - } -} - constant!(JOURNALNODE_ROLE_NAME: RoleName = "journalnode"); constant!(NAMENODE_ROLE_NAME: RoleName = "namenode"); constant!(DATANODE_ROLE_NAME: RoleName = "datanode"); diff --git a/rust/operator-binary/src/event.rs b/rust/operator-binary/src/event.rs index 4e6520a8..8686db73 100644 --- a/rust/operator-binary/src/event.rs +++ b/rust/operator-binary/src/event.rs @@ -1,11 +1,21 @@ +use std::collections::BTreeMap; + use snafu::{ResultExt, Snafu}; use stackable_operator::{ k8s_openapi::api::core::v1::ObjectReference, kube::runtime::events::{Event, EventType}, + v2::{ + role_utils::{JavaCommonConfig, RoleGroupConfig}, + types::operator::RoleGroupName, + }, }; use strum::{EnumDiscriminants, IntoStaticStr}; -use crate::{controller::ValidatedCluster, crd::HdfsNodeRole, hdfs_controller::Ctx}; +use crate::{ + controller::ValidatedCluster, + crd::{HdfsNodeRole, v1alpha1}, + hdfs_controller::Ctx, +}; #[derive(Snafu, Debug, EnumDiscriminants)] #[strum_discriminants(derive(IntoStaticStr))] @@ -43,13 +53,11 @@ pub fn build_invalid_replica_message( validated_cluster: &ValidatedCluster, role: &HdfsNodeRole, ) -> Option { - let replicas: u16 = validated_cluster - .role_groups - .get(role) - .into_iter() - .flatten() - .map(|(_, role_group)| role_group.replicas.unwrap_or_default()) - .sum(); + let replicas = match role { + HdfsNodeRole::Name => total_replicas(&validated_cluster.namenode_role_group_configs), + HdfsNodeRole::Data => total_replicas(&validated_cluster.datanode_role_group_configs), + HdfsNodeRole::Journal => total_replicas(&validated_cluster.journalnode_role_group_configs), + }; let dfs_replication = validated_cluster.cluster_config.dfs_replication; let role_name = role.to_string(); @@ -71,3 +79,17 @@ pub fn build_invalid_replica_message( None } } + +/// The total number of replicas across the role groups of one role, counting a role group without +/// an explicit replica count as zero. +fn total_replicas( + role_group_configs: &BTreeMap< + RoleGroupName, + RoleGroupConfig, + >, +) -> u16 { + role_group_configs + .values() + .map(|role_group| role_group.replicas.unwrap_or_default()) + .sum() +} diff --git a/rust/operator-binary/src/hdfs_controller.rs b/rust/operator-binary/src/hdfs_controller.rs index a8b19d04..517d6cfc 100644 --- a/rust/operator-binary/src/hdfs_controller.rs +++ b/rust/operator-binary/src/hdfs_controller.rs @@ -110,10 +110,9 @@ pub async fn reconcile_hdfs( // Warn about invalid replica counts. This is validation feedback and independent of the // resource application below. + // Every role is checked, including one that is absent from the spec entirely: a cluster + // without journalnodes is exactly the case the warning is for. for role in HdfsNodeRole::iter() { - if !validated_cluster.role_groups.contains_key(&role) { - continue; - } if let Some(message) = build_invalid_replica_message(&validated_cluster, &role) { publish_warning_event( &ctx, @@ -179,8 +178,11 @@ mod test { use super::*; use crate::{ HDFS_FULL_CONTROLLER_NAME, - controller::build::{container::ContainerConfig, role_group_logging}, - test_support::{datanode_config, deserialize_cluster, role_group_config, validate_cluster}, + controller::build::{ResolvedRoleGroup, RoleGroupLogging, container::ContainerConfig}, + crd::DataNodeContainer, + test_support::{ + datanode_config, datanode_role_group_config, deserialize_cluster, validate_cluster, + }, }; #[test] @@ -222,9 +224,42 @@ spec: let hdfs = deserialize_cluster(cr); let validated_cluster = validate_cluster(&hdfs); let role_group_name = RoleGroupName::from_str("default").unwrap(); - let role_group_config = role_group_config(&validated_cluster, &role, &role_group_name); + let role_group_config = datanode_role_group_config(&validated_cluster, &role_group_name); let datanode_config = datanode_config(&validated_cluster, &role_group_name); let labels = Labels::new(); + let resolved = ResolvedRoleGroup { + selector_labels: labels.clone(), + common: datanode_config.common.clone(), + resources: datanode_config.resources.clone().into(), + volume_claim_templates: ContainerConfig::datanode_volume_claim_templates( + datanode_config, + ), + listener_volume: Some( + ContainerConfig::datanode_listener_volume(datanode_config, &labels) + .expect("the datanode listener volume should build"), + ), + logging: RoleGroupLogging { + hdfs: datanode_config + .logging + .for_container(&DataNodeContainer::Hdfs) + .into_owned(), + vector: datanode_config.logging.enable_vector_agent.then(|| { + datanode_config + .logging + .for_container(&DataNodeContainer::Vector) + .into_owned() + }), + zkfc: None, + format_namenodes: None, + format_zookeeper: None, + wait_for_namenodes: Some( + datanode_config + .logging + .for_container(&DataNodeContainer::WaitForNameNodes) + .into_owned(), + ), + }, + }; let mut pb = PodBuilder::new(); pb.metadata(ObjectMeta::default()); @@ -237,14 +272,7 @@ spec: &role, &role_group_name, role_group_config, - &datanode_config.common, - &datanode_config.resources.clone().into(), - &ContainerConfig::datanode_volume_claim_templates(datanode_config), - Some( - ContainerConfig::datanode_listener_volume(datanode_config, &labels) - .expect("the datanode listener volume should build"), - ), - &role_group_logging(&role_group_config.config), + &resolved, ) .unwrap(); let containers = pb.build().unwrap().spec.unwrap().containers; diff --git a/rust/operator-binary/src/test_support.rs b/rust/operator-binary/src/test_support.rs index afec9dfa..ef8c6e1f 100644 --- a/rust/operator-binary/src/test_support.rs +++ b/rust/operator-binary/src/test_support.rs @@ -3,8 +3,13 @@ use std::str::FromStr; use stackable_operator::v2::types::operator::RoleGroupName; use crate::{ - controller::{ValidatedCluster, ValidatedRoleGroupConfig, validate}, - crd::{AnyNodeConfig, DataNodeConfig, HdfsNodeRole, v1alpha1}, + controller::{ + DataNodeRoleGroupConfig, JournalNodeRoleGroupConfig, NameNodeRoleGroupConfig, + ValidatedCluster, validate, + }, + crd::{ + CommonNodeConfig, DataNodeConfig, HdfsNodeRole, JournalNodeConfig, NameNodeConfig, v1alpha1, + }, }; /// The expected `app.kubernetes.io/version` label value for the given product version. @@ -46,34 +51,69 @@ pub fn role_group_name(name: &str) -> RoleGroupName { RoleGroupName::from_str(name).expect("role group name should be valid") } -pub fn role_group_config<'a>( +pub fn namenode_role_group_config<'a>( validated_cluster: &'a ValidatedCluster, - role: &HdfsNodeRole, role_group_name: &RoleGroupName, -) -> &'a ValidatedRoleGroupConfig { +) -> &'a NameNodeRoleGroupConfig { validated_cluster - .role_groups - .get(role) - .expect("role should be defined") + .namenode_role_group_configs .get(role_group_name) - .expect("role group should be defined") + .expect("namenode role group should be defined") } -pub fn anynode_config<'a>( +pub fn datanode_role_group_config<'a>( + validated_cluster: &'a ValidatedCluster, + role_group_name: &RoleGroupName, +) -> &'a DataNodeRoleGroupConfig { + validated_cluster + .datanode_role_group_configs + .get(role_group_name) + .expect("datanode role group should be defined") +} + +pub fn journalnode_role_group_config<'a>( validated_cluster: &'a ValidatedCluster, - role: &HdfsNodeRole, role_group_name: &RoleGroupName, -) -> &'a AnyNodeConfig { - &role_group_config(validated_cluster, role, role_group_name).config +) -> &'a JournalNodeRoleGroupConfig { + validated_cluster + .journalnode_role_group_configs + .get(role_group_name) + .expect("journalnode role group should be defined") +} + +pub fn namenode_config<'a>( + validated_cluster: &'a ValidatedCluster, + role_group_name: &RoleGroupName, +) -> &'a NameNodeConfig { + &namenode_role_group_config(validated_cluster, role_group_name).config } pub fn datanode_config<'a>( validated_cluster: &'a ValidatedCluster, role_group_name: &RoleGroupName, ) -> &'a DataNodeConfig { - anynode_config(validated_cluster, &HdfsNodeRole::Data, role_group_name) - .as_datanode() - .expect("should be a DataNode") + &datanode_role_group_config(validated_cluster, role_group_name).config +} + +pub fn journalnode_config<'a>( + validated_cluster: &'a ValidatedCluster, + role_group_name: &RoleGroupName, +) -> &'a JournalNodeConfig { + &journalnode_role_group_config(validated_cluster, role_group_name).config +} + +/// The merged [`CommonNodeConfig`] of one role group, for tests that assert on the settings every +/// role shares. +pub fn common_config<'a>( + validated_cluster: &'a ValidatedCluster, + role: &HdfsNodeRole, + role_group_name: &RoleGroupName, +) -> &'a CommonNodeConfig { + match role { + HdfsNodeRole::Name => &namenode_config(validated_cluster, role_group_name).common, + HdfsNodeRole::Data => &datanode_config(validated_cluster, role_group_name).common, + HdfsNodeRole::Journal => &journalnode_config(validated_cluster, role_group_name).common, + } } /// A namenode pod `Listener` with a single ingress address, shaped as the dereference step From 22704dc83defc9376c7816fc8583dffbe3c75b41 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Mon, 14 Sep 2026 11:02:10 +0200 Subject: [PATCH 08/19] restore role validation order --- rust/operator-binary/src/controller/build/mod.rs | 3 ++- .../src/controller/build/resource/config_map.rs | 3 +-- rust/operator-binary/src/controller/validate.rs | 8 ++++---- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/rust/operator-binary/src/controller/build/mod.rs b/rust/operator-binary/src/controller/build/mod.rs index 4661909c..ff83f7a0 100644 --- a/rust/operator-binary/src/controller/build/mod.rs +++ b/rust/operator-binary/src/controller/build/mod.rs @@ -144,7 +144,8 @@ pub struct ResolvedRoleGroup { pub selector_labels: Labels, /// The role group's merged config that is common to every role. pub common: CommonNodeConfig, - /// The resource requirements of the role group's containers. + /// The resource requirements of the role group's main and init containers; the ZKFC sidecar + /// has fixed requirements of its own and ignores this. pub resources: ResourceRequirements, /// The `StatefulSet`'s persistent volume claim templates. pub volume_claim_templates: Vec, diff --git a/rust/operator-binary/src/controller/build/resource/config_map.rs b/rust/operator-binary/src/controller/build/resource/config_map.rs index d353c6a5..4cbb81a0 100644 --- a/rust/operator-binary/src/controller/build/resource/config_map.rs +++ b/rust/operator-binary/src/controller/build/resource/config_map.rs @@ -108,8 +108,7 @@ pub fn build_rolegroup_config_map( )?, ); - let log4j_configs = product_logging::build_log4j_configs(logging); - for (log_config_file, log4j_config) in log4j_configs { + for (log_config_file, log4j_config) in product_logging::build_log4j_configs(logging) { builder.add_data(log_config_file, log4j_config); } if logging.vector.is_some() { diff --git a/rust/operator-binary/src/controller/validate.rs b/rust/operator-binary/src/controller/validate.rs index ff0c4d7f..2c4b0cdf 100644 --- a/rust/operator-binary/src/controller/validate.rs +++ b/rust/operator-binary/src/controller/validate.rs @@ -99,6 +99,10 @@ pub fn validate_cluster( let cluster_name = get_cluster_name(hdfs).context(GetClusterNameSnafu)?; + let journalnode_role_group_configs = validate_role_group_configs( + hdfs.spec.journal_nodes.as_ref(), + JournalNodeConfigFragment::default_config(cluster_name.as_ref(), &HdfsNodeRole::Journal), + )?; let namenode_role_group_configs = validate_role_group_configs( hdfs.spec.name_nodes.as_ref(), NameNodeConfigFragment::default_config(cluster_name.as_ref(), &HdfsNodeRole::Name), @@ -107,10 +111,6 @@ pub fn validate_cluster( hdfs.spec.data_nodes.as_ref(), DataNodeConfigFragment::default_config(cluster_name.as_ref(), &HdfsNodeRole::Data), )?; - let journalnode_role_group_configs = validate_role_group_configs( - hdfs.spec.journal_nodes.as_ref(), - JournalNodeConfigFragment::default_config(cluster_name.as_ref(), &HdfsNodeRole::Journal), - )?; let namespace = get_namespace(hdfs).context(GetClusterNamespaceSnafu)?; let uid = get_uid(hdfs).context(GetClusterUidSnafu)?; From 423a46f77699ba359bc36274be695633853d241a Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Mon, 14 Sep 2026 13:41:55 +0200 Subject: [PATCH 09/19] put the role invariants in the types --- .../src/controller/build/container.rs | 18 +- .../src/controller/build/mod.rs | 685 ++++++++++++------ .../build/properties/product_logging/mod.rs | 8 +- .../controller/build/resource/config_map.rs | 26 +- rust/operator-binary/src/event.rs | 153 +++- rust/operator-binary/src/hdfs_controller.rs | 26 +- 6 files changed, 649 insertions(+), 267 deletions(-) diff --git a/rust/operator-binary/src/controller/build/container.rs b/rust/operator-binary/src/controller/build/container.rs index 6068ea5a..5fdf4452 100644 --- a/rust/operator-binary/src/controller/build/container.rs +++ b/rust/operator-binary/src/controller/build/container.rs @@ -232,7 +232,7 @@ impl ContainerConfig { pb.add_volumes(main_container_config.volumes( &resolved.logging, - resolved.listener_volume.as_ref(), + resolved.role.listener_volume(), &object_name, )) .context(AddVolumeSnafu)?; @@ -683,7 +683,7 @@ impl ContainerConfig { )); } ContainerConfig::Zkfc => { - if let Some(container_log_config) = &logging.zkfc { + if let Some(container_log_config) = logging.role.zkfc() { args.push_str( &self.copy_log4j_properties_cmd( ZKFC_LOG4J_CONFIG_FILE, @@ -699,7 +699,7 @@ impl ContainerConfig { ContainerConfig::FormatNameNodes => { args.push_str(&bash_capture_shell_helper(self.container_name().as_ref())); - if let Some(container_log_config) = &logging.format_namenodes { + if let Some(container_log_config) = logging.role.format_namenodes() { args.push_str(&self.copy_log4j_properties_cmd( FORMAT_NAMENODES_LOG4J_CONFIG_FILE, container_log_config, @@ -772,7 +772,7 @@ impl ContainerConfig { ContainerConfig::FormatZooKeeper => { args.push_str(&bash_capture_shell_helper(self.container_name().as_ref())); - if let Some(container_log_config) = &logging.format_zookeeper { + if let Some(container_log_config) = logging.role.format_zookeeper() { args.push_str(&self.copy_log4j_properties_cmd( FORMAT_ZOOKEEPER_LOG4J_CONFIG_FILE, container_log_config, @@ -801,7 +801,7 @@ impl ContainerConfig { ContainerConfig::WaitForNameNodes => { args.push_str(&bash_capture_shell_helper(self.container_name().as_ref())); - if let Some(container_log_config) = &logging.wait_for_namenodes { + if let Some(container_log_config) = logging.role.wait_for_namenodes() { args.push_str(&self.copy_log4j_properties_cmd( WAIT_FOR_NAMENODES_LOG4J_CONFIG_FILE, container_log_config, @@ -1116,10 +1116,10 @@ impl ContainerConfig { let container_log_config = match self { ContainerConfig::Hdfs { .. } => Some(&logging.hdfs), - ContainerConfig::Zkfc => logging.zkfc.as_ref(), - ContainerConfig::FormatNameNodes => logging.format_namenodes.as_ref(), - ContainerConfig::FormatZooKeeper => logging.format_zookeeper.as_ref(), - ContainerConfig::WaitForNameNodes => logging.wait_for_namenodes.as_ref(), + ContainerConfig::Zkfc => logging.role.zkfc(), + ContainerConfig::FormatNameNodes => logging.role.format_namenodes(), + ContainerConfig::FormatZooKeeper => logging.role.format_zookeeper(), + ContainerConfig::WaitForNameNodes => logging.role.wait_for_namenodes(), }; let volume_mount_dirs = self.volume_mount_dirs(); volumes.extend(Self::common_container_volumes( diff --git a/rust/operator-binary/src/controller/build/mod.rs b/rust/operator-binary/src/controller/build/mod.rs index ff83f7a0..d2cfef05 100644 --- a/rust/operator-binary/src/controller/build/mod.rs +++ b/rust/operator-binary/src/controller/build/mod.rs @@ -6,7 +6,11 @@ use std::{ use snafu::{ResultExt, Snafu}; use stackable_operator::{ builder::meta::ObjectMetaBuilder, - k8s_openapi::api::core::v1::{PersistentVolumeClaim, ResourceRequirements, Service, Volume}, + k8s_openapi::api::{ + apps::v1::StatefulSet, + core::v1::{ConfigMap, PersistentVolumeClaim, ResourceRequirements, Service, Volume}, + policy::v1::PodDisruptionBudget, + }, kvp::{LabelError, Labels}, product_logging::spec::ContainerLogConfig, utils::cluster_info::KubernetesClusterInfo, @@ -31,8 +35,8 @@ use crate::{ }, }, crd::{ - CommonNodeConfig, DataNodeContainer, HdfsNodeRole, HdfsPodRef, JournalNodeContainer, - NameNodeContainer, + CommonNodeConfig, DataNodeConfig, DataNodeContainer, HdfsNodeRole, HdfsPodRef, + JournalNodeConfig, JournalNodeContainer, NameNodeConfig, NameNodeContainer, constants::{ DEFAULT_DATA_NODE_DATA_PORT, DEFAULT_DATA_NODE_HTTP_PORT, DEFAULT_DATA_NODE_HTTPS_PORT, DEFAULT_DATA_NODE_IPC_PORT, DEFAULT_DATA_NODE_METRICS_PORT, @@ -48,6 +52,7 @@ use crate::{ SERVICE_PORT_NAME_IPC, SERVICE_PORT_NAME_JMX_METRICS, SERVICE_PORT_NAME_METRICS, SERVICE_PORT_NAME_RPC, }, + storage::DataNodeStorageConfigInnerType, v1alpha1, }, }; @@ -117,14 +122,69 @@ pub struct RoleGroupLogging { pub hdfs: ContainerLogConfig, /// The Vector sidecar; `None` when the Vector agent is disabled for this role group. pub vector: Option, - /// The namenode `zkfc` side container. - pub zkfc: Option, - /// The namenode `format-namenodes` init container. - pub format_namenodes: Option, - /// The namenode `format-zookeeper` init container. - pub format_zookeeper: Option, + /// The containers only one role has. + pub role: RoleContainerLogging, +} + +/// The log configuration of the side and init containers that only one role runs. +/// +/// These live in an enum rather than in `Option` fields so that "the `zkfc` log config exists +/// exactly when this is a namenode" is checked by the compiler at every construction site. A +/// missing log config is otherwise silent: the container's `log4j.properties` is left out of both +/// the `ConfigMap` and the `cp` in the container args, so it logs with Hadoop's built-in defaults +/// and Vector collects nothing for it. +#[derive(Clone, Debug)] +pub enum RoleContainerLogging { + /// Journalnodes run no role-specific container. + Journal, + /// The namenode `zkfc` side container and its two init containers. + Name { + zkfc: ContainerLogConfig, + format_namenodes: ContainerLogConfig, + format_zookeeper: ContainerLogConfig, + }, /// The datanode `wait-for-namenodes` init container. - pub wait_for_namenodes: Option, + Data { + wait_for_namenodes: ContainerLogConfig, + }, +} + +impl RoleContainerLogging { + /// The namenode `zkfc` side container's log config; `None` for the other roles. + pub fn zkfc(&self) -> Option<&ContainerLogConfig> { + match self { + Self::Name { zkfc, .. } => Some(zkfc), + Self::Journal | Self::Data { .. } => None, + } + } + + /// The namenode `format-namenodes` init container's log config; `None` for the other roles. + pub fn format_namenodes(&self) -> Option<&ContainerLogConfig> { + match self { + Self::Name { + format_namenodes, .. + } => Some(format_namenodes), + Self::Journal | Self::Data { .. } => None, + } + } + + /// The namenode `format-zookeeper` init container's log config; `None` for the other roles. + pub fn format_zookeeper(&self) -> Option<&ContainerLogConfig> { + match self { + Self::Name { + format_zookeeper, .. + } => Some(format_zookeeper), + Self::Journal | Self::Data { .. } => None, + } + } + + /// The datanode `wait-for-namenodes` init container's log config; `None` for the other roles. + pub fn wait_for_namenodes(&self) -> Option<&ContainerLogConfig> { + match self { + Self::Data { wait_for_namenodes } => Some(wait_for_namenodes), + Self::Journal | Self::Name { .. } => None, + } + } } /// Everything about one role group that the shared builders below cannot derive themselves: the @@ -149,270 +209,257 @@ pub struct ResolvedRoleGroup { pub resources: ResourceRequirements, /// The `StatefulSet`'s persistent volume claim templates. pub volume_claim_templates: Vec, - /// The ephemeral listener volume; only datanodes have one (namenodes get their listener from - /// a volume claim template and journalnodes have no listener at all). - pub listener_volume: Option, + /// The values that exist for one role only. + pub role: RoleSpecificResources, /// The log config of each of the role group's containers. pub logging: RoleGroupLogging, } -/// Builds every Kubernetes resource for the given validated cluster. +/// The role group values that exist for one role only. /// -/// Does not need a Kubernetes client: every external reference is already dereferenced and -/// validated by this point, so the errors returned here are resource-assembly failures only. -/// `cluster_info` carries static cluster information resolved at operator startup (e.g. the -/// cluster domain used to build Kerberos principals), not a live client. +/// These live in an enum rather than in `Option` fields so the compiler checks the pairing at +/// every construction site: a datanode without its storage configuration does not compile — that +/// would silently drop `dfs.datanode.data.dir` and send the datanodes' blocks to container-local +/// storage — and neither does a namenode with a pod-level listener volume, which would collide +/// with the identically named volume claim template and be rejected at apply time. +pub enum RoleSpecificResources { + /// Journalnodes have no listener and no role-specific storage configuration. + Journal, + /// Namenodes get their listener from a volume claim template in `volume_claim_templates`, for + /// stable per-pod identity, so they have no pod-level listener volume. + Name, + /// Datanodes need no stable per-pod identity, so their listener is an ephemeral pod volume. + /// They are also the only role that configures `dfs.datanode.data.dir`. + Data { + listener_volume: Volume, + storage: DataNodeStorageConfigInnerType, + }, +} + +impl RoleSpecificResources { + /// The role these values belong to. + pub fn node_role(&self) -> HdfsNodeRole { + match self { + Self::Journal => HdfsNodeRole::Journal, + Self::Name => HdfsNodeRole::Name, + Self::Data { .. } => HdfsNodeRole::Data, + } + } + + /// The role group's ephemeral listener volume; only datanodes have one. + pub fn listener_volume(&self) -> Option<&Volume> { + match self { + Self::Data { + listener_volume, .. + } => Some(listener_volume), + Self::Journal | Self::Name => None, + } + } + + /// The datanode data volume configuration, which drives `dfs.datanode.data.dir`; `None` for + /// the other roles. + pub fn datanode_storage(&self) -> Option { + match self { + Self::Data { storage, .. } => Some(storage.clone()), + Self::Journal | Self::Name => None, + } + } +} + +/// How to resolve one role group's role-specific values, implemented once per role config type. /// -/// The resources are returned as flat collections. `stateful_sets` is ordered by role — -/// journalnodes, then namenodes, then datanodes — because the apply step rolls them out in that -/// order during upgrades to preserve HDFS's rollout-gated deployment (see -/// [`crate::controller::apply::Applier::apply`]). -/// The discovery `ConfigMap` is included when it can be built or re-emitted (see -/// [`resource::discovery::build_discovery_config_map`]); it is only absent before its first -/// successful build. -pub fn build( - cluster: &ValidatedCluster, - cluster_info: &KubernetesClusterInfo, -) -> Result, Error> { - let mut services = vec![]; - let mut config_maps = vec![]; - let mut stateful_sets = vec![]; - let mut pod_disruption_budgets = vec![]; +/// This is what lets [`build_role`] be written once: the trait supplies the role and the single +/// role-dependent step, and everything else about building a role group is identical across the +/// three roles. +trait RoleGroupResolver { + /// The role whose config this is. + const ROLE: HdfsNodeRole; - // The roles are built in the order journalnode, namenode, datanode, and the resulting - // `stateful_sets` order is load-bearing: the apply step rolls the StatefulSets out in that - // order during upgrades, each role gated on the previous one (see - // [`crate::controller::apply::Applier::apply`]). - for (role_group_name, rg_config) in &cluster.journalnode_role_group_configs { - let role = &HdfsNodeRole::Journal; - let config = &rg_config.config; + /// Resolves everything the shared builders cannot derive themselves. Takes the selector + /// labels because two of the three roles need them to build their listener. + fn resolve( + &self, + role_group_name: &RoleGroupName, + selector_labels: Labels, + ) -> Result; +} - build_role_group_services(cluster, role, role_group_name, &mut services)?; +impl RoleGroupResolver for JournalNodeConfig { + const ROLE: HdfsNodeRole = HdfsNodeRole::Journal; - let selector_labels = rolegroup_selector_labels(cluster, role, role_group_name).context( - RoleGroupSelectorLabelsSnafu { - role: *role, - role_group: role_group_name.clone(), - }, - )?; - let resolved = ResolvedRoleGroup { + fn resolve( + &self, + _role_group_name: &RoleGroupName, + selector_labels: Labels, + ) -> Result { + Ok(ResolvedRoleGroup { selector_labels, - common: config.common.clone(), - resources: config.resources.clone().into(), - volume_claim_templates: ContainerConfig::journalnode_volume_claim_templates(config), - listener_volume: None, + common: self.common.clone(), + resources: self.resources.clone().into(), + volume_claim_templates: ContainerConfig::journalnode_volume_claim_templates(self), + role: RoleSpecificResources::Journal, logging: RoleGroupLogging { - hdfs: config + hdfs: self .logging .for_container(&JournalNodeContainer::Hdfs) .into_owned(), - vector: config.logging.enable_vector_agent.then(|| { - config - .logging + vector: self.logging.enable_vector_agent.then(|| { + self.logging .for_container(&JournalNodeContainer::Vector) .into_owned() }), - zkfc: None, - format_namenodes: None, - format_zookeeper: None, - wait_for_namenodes: None, + role: RoleContainerLogging::Journal, }, - }; - - config_maps.push( - resource::config_map::build_rolegroup_config_map( - cluster, - cluster_info, - role, - role_group_name, - rg_config, - None, - &resolved.logging, - ) - .context(ConfigMapSnafu { - role: *role, - role_group: role_group_name.clone(), - })?, - ); - stateful_sets.push( - resource::statefulset::build_rolegroup_statefulset( - cluster, - cluster_info, - role, - role_group_name, - rg_config, - resolved, - ) - .context(StatefulSetSnafu { - role: *role, - role_group: role_group_name.clone(), - })?, - ); - } - if let Some(pdb) = resource::pdb::build_pdb(cluster, &HdfsNodeRole::Journal) { - pod_disruption_budgets.push(pdb); + }) } +} - for (role_group_name, rg_config) in &cluster.namenode_role_group_configs { - let role = &HdfsNodeRole::Name; - let config = &rg_config.config; - - build_role_group_services(cluster, role, role_group_name, &mut services)?; +impl RoleGroupResolver for NameNodeConfig { + const ROLE: HdfsNodeRole = HdfsNodeRole::Name; - let selector_labels = rolegroup_selector_labels(cluster, role, role_group_name).context( - RoleGroupSelectorLabelsSnafu { - role: *role, - role_group: role_group_name.clone(), - }, - )?; + fn resolve( + &self, + role_group_name: &RoleGroupName, + selector_labels: Labels, + ) -> Result { // Namenodes get their listener from a persistent volume claim template, for stable // per-pod identity, rather than from an ephemeral volume. let volume_claim_templates = - ContainerConfig::namenode_volume_claim_templates(config, &selector_labels).context( + ContainerConfig::namenode_volume_claim_templates(self, &selector_labels).context( VolumeClaimTemplatesSnafu { - role: *role, + role: Self::ROLE, role_group: role_group_name.clone(), }, )?; - let resolved = ResolvedRoleGroup { + + Ok(ResolvedRoleGroup { selector_labels, - common: config.common.clone(), - resources: config.resources.clone().into(), + common: self.common.clone(), + resources: self.resources.clone().into(), volume_claim_templates, - listener_volume: None, + role: RoleSpecificResources::Name, logging: RoleGroupLogging { - hdfs: config + hdfs: self .logging .for_container(&NameNodeContainer::Hdfs) .into_owned(), - vector: config.logging.enable_vector_agent.then(|| { - config - .logging + vector: self.logging.enable_vector_agent.then(|| { + self.logging .for_container(&NameNodeContainer::Vector) .into_owned() }), - zkfc: Some( - config + role: RoleContainerLogging::Name { + zkfc: self .logging .for_container(&NameNodeContainer::Zkfc) .into_owned(), - ), - format_namenodes: Some( - config + format_namenodes: self .logging .for_container(&NameNodeContainer::FormatNameNodes) .into_owned(), - ), - format_zookeeper: Some( - config + format_zookeeper: self .logging .for_container(&NameNodeContainer::FormatZooKeeper) .into_owned(), - ), - wait_for_namenodes: None, + }, }, - }; - - config_maps.push( - resource::config_map::build_rolegroup_config_map( - cluster, - cluster_info, - role, - role_group_name, - rg_config, - None, - &resolved.logging, - ) - .context(ConfigMapSnafu { - role: *role, - role_group: role_group_name.clone(), - })?, - ); - stateful_sets.push( - resource::statefulset::build_rolegroup_statefulset( - cluster, - cluster_info, - role, - role_group_name, - rg_config, - resolved, - ) - .context(StatefulSetSnafu { - role: *role, - role_group: role_group_name.clone(), - })?, - ); - } - if let Some(pdb) = resource::pdb::build_pdb(cluster, &HdfsNodeRole::Name) { - pod_disruption_budgets.push(pdb); + }) } +} - for (role_group_name, rg_config) in &cluster.datanode_role_group_configs { - let role = &HdfsNodeRole::Data; - let config = &rg_config.config; - - build_role_group_services(cluster, role, role_group_name, &mut services)?; +impl RoleGroupResolver for DataNodeConfig { + const ROLE: HdfsNodeRole = HdfsNodeRole::Data; - let selector_labels = rolegroup_selector_labels(cluster, role, role_group_name).context( - RoleGroupSelectorLabelsSnafu { - role: *role, - role_group: role_group_name.clone(), - }, - )?; + fn resolve( + &self, + role_group_name: &RoleGroupName, + selector_labels: Labels, + ) -> Result { // Datanodes use an ephemeral listener volume, since they need no stable per-pod identity. - let listener_volume = Some( - ContainerConfig::datanode_listener_volume(config, &selector_labels).context( - ListenerVolumeSnafu { - role: *role, - role_group: role_group_name.clone(), - }, - )?, - ); - let resolved = ResolvedRoleGroup { + let listener_volume = ContainerConfig::datanode_listener_volume(self, &selector_labels) + .context(ListenerVolumeSnafu { + role: Self::ROLE, + role_group: role_group_name.clone(), + })?; + + Ok(ResolvedRoleGroup { selector_labels, - common: config.common.clone(), - resources: config.resources.clone().into(), - volume_claim_templates: ContainerConfig::datanode_volume_claim_templates(config), - listener_volume, + common: self.common.clone(), + resources: self.resources.clone().into(), + volume_claim_templates: ContainerConfig::datanode_volume_claim_templates(self), + role: RoleSpecificResources::Data { + listener_volume, + storage: self.resources.storage.clone(), + }, logging: RoleGroupLogging { - hdfs: config + hdfs: self .logging .for_container(&DataNodeContainer::Hdfs) .into_owned(), - vector: config.logging.enable_vector_agent.then(|| { - config - .logging + vector: self.logging.enable_vector_agent.then(|| { + self.logging .for_container(&DataNodeContainer::Vector) .into_owned() }), - zkfc: None, - format_namenodes: None, - format_zookeeper: None, - wait_for_namenodes: Some( - config + role: RoleContainerLogging::Data { + wait_for_namenodes: self .logging .for_container(&DataNodeContainer::WaitForNameNodes) .into_owned(), - ), + }, }, - }; + }) + } +} + +/// The resources built for the role groups of one role, accumulated across the roles by +/// [`build`]. +#[derive(Default)] +struct RoleGroupResources { + services: Vec, + config_maps: Vec, + stateful_sets: Vec, + pod_disruption_budgets: Vec, +} + +/// Builds every resource of every role group of one role, plus that role's PDB, appending them to +/// `out`. +fn build_role( + cluster: &ValidatedCluster, + cluster_info: &KubernetesClusterInfo, + role_group_configs: &BTreeMap< + RoleGroupName, + RoleGroupConfig, + >, + out: &mut RoleGroupResources, +) -> Result<(), Error> { + let role = &C::ROLE; + + for (role_group_name, rg_config) in role_group_configs { + build_role_group_services(cluster, role, role_group_name, &mut out.services)?; + + let selector_labels = rolegroup_selector_labels(cluster, role, role_group_name).context( + RoleGroupSelectorLabelsSnafu { + role: *role, + role_group: role_group_name.clone(), + }, + )?; + let resolved = rg_config.config.resolve(role_group_name, selector_labels)?; - config_maps.push( + out.config_maps.push( resource::config_map::build_rolegroup_config_map( cluster, cluster_info, - role, role_group_name, rg_config, - Some(config.resources.storage.clone()), - &resolved.logging, + &resolved, ) .context(ConfigMapSnafu { role: *role, role_group: role_group_name.clone(), })?, ); - stateful_sets.push( + out.stateful_sets.push( resource::statefulset::build_rolegroup_statefulset( cluster, cluster_info, @@ -427,10 +474,64 @@ pub fn build( })?, ); } - if let Some(pdb) = resource::pdb::build_pdb(cluster, &HdfsNodeRole::Data) { - pod_disruption_budgets.push(pdb); + + if let Some(pdb) = resource::pdb::build_pdb(cluster, role) { + out.pod_disruption_budgets.push(pdb); } + Ok(()) +} + +/// Builds every Kubernetes resource for the given validated cluster. +/// +/// Does not need a Kubernetes client: every external reference is already dereferenced and +/// validated by this point, so the errors returned here are resource-assembly failures only. +/// `cluster_info` carries static cluster information resolved at operator startup (e.g. the +/// cluster domain used to build Kerberos principals), not a live client. +/// +/// The resources are returned as flat collections. `stateful_sets` is ordered by role — +/// journalnodes, then namenodes, then datanodes — because the apply step rolls them out in that +/// order during upgrades to preserve HDFS's rollout-gated deployment (see +/// [`crate::controller::apply::Applier::apply`]). +/// The discovery `ConfigMap` is included when it can be built or re-emitted (see +/// [`resource::discovery::build_discovery_config_map`]); it is only absent before its first +/// successful build. +pub fn build( + cluster: &ValidatedCluster, + cluster_info: &KubernetesClusterInfo, +) -> Result, Error> { + let mut built = RoleGroupResources::default(); + + // The roles are built in the order journalnode, namenode, datanode, and the resulting + // `stateful_sets` order is load-bearing: the apply step rolls the StatefulSets out in that + // order during upgrades, each role gated on the previous one (see + // [`crate::controller::apply::Applier::apply`]). + build_role( + cluster, + cluster_info, + &cluster.journalnode_role_group_configs, + &mut built, + )?; + build_role( + cluster, + cluster_info, + &cluster.namenode_role_group_configs, + &mut built, + )?; + build_role( + cluster, + cluster_info, + &cluster.datanode_role_group_configs, + &mut built, + )?; + + let RoleGroupResources { + services, + mut config_maps, + stateful_sets, + pod_disruption_budgets, + } = built; + // The discovery ConfigMap is skipped only before its first successful build (no namenode // Listener addresses yet, nothing stored to re-emit); afterwards a stored ConfigMap is // re-emitted unchanged whenever it cannot be rebuilt, so it stays tracked. @@ -480,7 +581,12 @@ fn build_role_group_services( Ok(()) } -/// The replica count of every role group in the map, defaulting to one where it is unset. +/// The replica count a role group gets when it does not set one: Kubernetes runs a single pod for +/// a `StatefulSet` with `replicas: null`. +pub(crate) const DEFAULT_REPLICAS: u16 = 1; + +/// The replica count of every role group in the map, defaulting to [`DEFAULT_REPLICAS`] where it +/// is unset. fn role_group_replicas( role_group_configs: &BTreeMap< RoleGroupName, @@ -489,10 +595,29 @@ fn role_group_replicas( ) -> Vec<(&RoleGroupName, u16)> { role_group_configs .iter() - .map(|(role_group_name, role_group)| (role_group_name, role_group.replicas.unwrap_or(1))) + .map(|(role_group_name, role_group)| { + ( + role_group_name, + role_group.replicas.unwrap_or(DEFAULT_REPLICAS), + ) + }) .collect() } +/// The total number of replicas across the role groups of one role, counting a role group without +/// an explicit replica count as [`DEFAULT_REPLICAS`]. +pub(crate) fn total_replicas( + role_group_configs: &BTreeMap< + RoleGroupName, + RoleGroupConfig, + >, +) -> u16 { + role_group_configs + .values() + .map(|role_group| role_group.replicas.unwrap_or(DEFAULT_REPLICAS)) + .sum() +} + /// Builds the [`HdfsPodRef`]s expected for every pod of the given `role`, across all /// of its role groups. /// @@ -585,11 +710,7 @@ pub(crate) fn rolegroup_selector_labels( /// The total number of datanode replicas across all datanode role groups. pub(crate) fn num_datanodes(cluster: &ValidatedCluster) -> u16 { - cluster - .datanode_role_group_configs - .values() - .map(|role_group| role_group.replicas.unwrap_or(1)) - .sum() + total_replicas(&cluster.datanode_role_group_configs) } /// The ports exposed by the rolegroup headless service for the given `role`. @@ -760,7 +881,7 @@ pub(crate) fn role_group_selector( #[cfg(test)] mod tests { - use stackable_operator::kube::Resource; + use stackable_operator::{k8s_openapi::api::core::v1::ConfigMap, kube::Resource}; use super::build; use crate::{ @@ -902,4 +1023,150 @@ mod tests { ); } } + + /// The named role group `ConfigMap`. + fn config_map<'a>(config_maps: &'a [ConfigMap], name: &str) -> &'a ConfigMap { + config_maps + .iter() + .find(|config_map| config_map.meta().name.as_deref() == Some(name)) + .unwrap_or_else(|| panic!("the {name} ConfigMap is built")) + } + + /// The sorted data keys of the named role group `ConfigMap`. + fn config_map_keys(config_maps: &[ConfigMap], name: &str) -> Vec { + let mut keys: Vec = config_map(config_maps, name) + .data + .as_ref() + .unwrap_or_else(|| panic!("the {name} ConfigMap has data")) + .keys() + .cloned() + .collect(); + keys.sort(); + keys + } + + /// Each role gets the `log4j.properties` of exactly its own containers: the namenode's + /// `zkfc`, `format-namenodes` and `format-zookeeper`, the datanode's `wait-for-namenodes`, + /// and nothing role-specific for the journalnode. A missing file here is invisible at + /// runtime — the container just falls back to Hadoop's built-in logging and Vector collects + /// nothing for it — so it is pinned here. + #[test] + fn each_role_gets_only_its_own_log4j_configs() { + let cluster = validated_cluster(); + let resources = build(&cluster, &cluster_info()).expect("build succeeds"); + let config_maps = &resources.config_maps; + + assert_eq!( + config_map_keys(config_maps, "hdfs-journalnode-default"), + [ + "core-site.xml", + "hadoop-policy.xml", + "hdfs-site.xml", + "hdfs.log4j.properties", + "security.properties", + "ssl-client.xml", + "ssl-server.xml", + ] + ); + assert_eq!( + config_map_keys(config_maps, "hdfs-namenode-default"), + [ + "core-site.xml", + "format-namenodes.log4j.properties", + "format-zookeeper.log4j.properties", + "hadoop-policy.xml", + "hdfs-site.xml", + "hdfs.log4j.properties", + "security.properties", + "ssl-client.xml", + "ssl-server.xml", + "zkfc.log4j.properties", + ] + ); + assert_eq!( + config_map_keys(config_maps, "hdfs-datanode-default"), + [ + "core-site.xml", + "hadoop-policy.xml", + "hdfs-site.xml", + "hdfs.log4j.properties", + "security.properties", + "ssl-client.xml", + "ssl-server.xml", + "wait-for-namenodes.log4j.properties", + ] + ); + } + + /// `dfs.datanode.data.dir` must appear in the datanode's `hdfs-site.xml` and nowhere else. + /// Losing it is silent: the datanodes fall back to Hadoop's default directory, which is + /// container-local, so their blocks are gone on the next restart. + #[test] + fn only_the_datanode_config_map_sets_the_datanode_data_dir() { + let cluster = validated_cluster(); + let resources = build(&cluster, &cluster_info()).expect("build succeeds"); + + for (name, expected) in [ + ("hdfs-datanode-default", true), + ("hdfs-namenode-default", false), + ("hdfs-journalnode-default", false), + ] { + let hdfs_site = config_map(&resources.config_maps, name) + .data + .as_ref() + .and_then(|data| data.get("hdfs-site.xml")) + .unwrap_or_else(|| panic!("the {name} ConfigMap has an hdfs-site.xml")); + + assert_eq!( + hdfs_site.contains("dfs.datanode.data.dir"), + expected, + "{name}'s hdfs-site.xml should {}contain dfs.datanode.data.dir", + if expected { "" } else { "not " } + ); + } + } + + /// Datanodes get their listener from an ephemeral pod volume; namenodes get theirs from a + /// volume claim template, for stable per-pod identity; journalnodes have no listener at all. + /// Both at once for one role would mean a pod volume and a claim template of the same name, + /// which the API server rejects at apply time. + #[test] + fn the_listener_is_a_pod_volume_for_datanodes_and_a_claim_template_for_namenodes() { + let cluster = validated_cluster(); + let resources = build(&cluster, &cluster_info()).expect("build succeeds"); + + for (name, expect_pod_volume, expect_claim_template) in [ + ("hdfs-datanode-default", true, false), + ("hdfs-namenode-default", false, true), + ("hdfs-journalnode-default", false, false), + ] { + let spec = resources + .stateful_sets + .iter() + .find(|stateful_set| stateful_set.meta().name.as_deref() == Some(name)) + .and_then(|stateful_set| stateful_set.spec.as_ref()) + .unwrap_or_else(|| panic!("the {name} StatefulSet is built")); + + let has_pod_volume = spec + .template + .spec + .as_ref() + .and_then(|pod_spec| pod_spec.volumes.as_ref()) + .is_some_and(|volumes| volumes.iter().any(|volume| volume.name == "listener")); + let has_claim_template = spec.volume_claim_templates.as_ref().is_some_and(|claims| { + claims + .iter() + .any(|claim| claim.meta().name.as_deref() == Some("listener")) + }); + + assert_eq!( + has_pod_volume, expect_pod_volume, + "{name} listener pod volume" + ); + assert_eq!( + has_claim_template, expect_claim_template, + "{name} listener volume claim template" + ); + } + } } diff --git a/rust/operator-binary/src/controller/build/properties/product_logging/mod.rs b/rust/operator-binary/src/controller/build/properties/product_logging/mod.rs index cb7e62fa..9abbb3d9 100644 --- a/rust/operator-binary/src/controller/build/properties/product_logging/mod.rs +++ b/rust/operator-binary/src/controller/build/properties/product_logging/mod.rs @@ -90,7 +90,7 @@ pub fn build_log4j_configs(logging: &RoleGroupLogging) -> Vec<(&'static str, Str ); add_log4j_config_if_automatic( &mut configs, - logging.zkfc.as_ref(), + logging.role.zkfc(), ZKFC_LOG4J_CONFIG_FILE, ZKFC_CONTAINER_NAME.as_ref(), ZKFC_LOG_FILE, @@ -98,7 +98,7 @@ pub fn build_log4j_configs(logging: &RoleGroupLogging) -> Vec<(&'static str, Str ); add_log4j_config_if_automatic( &mut configs, - logging.format_namenodes.as_ref(), + logging.role.format_namenodes(), FORMAT_NAMENODES_LOG4J_CONFIG_FILE, FORMAT_NAMENODES_CONTAINER_NAME.as_ref(), FORMAT_NAMENODES_LOG_FILE, @@ -106,7 +106,7 @@ pub fn build_log4j_configs(logging: &RoleGroupLogging) -> Vec<(&'static str, Str ); add_log4j_config_if_automatic( &mut configs, - logging.format_zookeeper.as_ref(), + logging.role.format_zookeeper(), FORMAT_ZOOKEEPER_LOG4J_CONFIG_FILE, FORMAT_ZOOKEEPER_CONTAINER_NAME.as_ref(), FORMAT_ZOOKEEPER_LOG_FILE, @@ -114,7 +114,7 @@ pub fn build_log4j_configs(logging: &RoleGroupLogging) -> Vec<(&'static str, Str ); add_log4j_config_if_automatic( &mut configs, - logging.wait_for_namenodes.as_ref(), + logging.role.wait_for_namenodes(), WAIT_FOR_NAMENODES_LOG4J_CONFIG_FILE, WAIT_FOR_NAMENODES_CONTAINER_NAME.as_ref(), WAIT_FOR_NAMENODES_LOG_FILE, diff --git a/rust/operator-binary/src/controller/build/resource/config_map.rs b/rust/operator-binary/src/controller/build/resource/config_map.rs index 4cbb81a0..3ab90448 100644 --- a/rust/operator-binary/src/controller/build/resource/config_map.rs +++ b/rust/operator-binary/src/controller/build/resource/config_map.rs @@ -17,14 +17,14 @@ use crate::{ controller::{ ValidatedCluster, build::{ - self, RoleGroupLogging, + self, ResolvedRoleGroup, properties::{ ConfigFileName, core_site, hadoop_policy, hdfs_site, product_logging, security_properties, ssl_client, ssl_server, }, }, }, - crd::{HdfsNodeRole, storage::DataNodeStorageConfigInnerType, v1alpha1}, + crd::v1alpha1, }; #[derive(Snafu, Debug)] @@ -47,24 +47,24 @@ type Result = std::result::Result; /// Builds the [`ConfigMap`] of one role group. /// -/// Every role-specific value is resolved by the caller: `datanode_storage` is the datanode data -/// volume configuration (`None` for the other roles) and `logging` the log config of each of the -/// role group's containers. +/// Every role-specific value is resolved by the caller into `resolved`, the role itself included: +/// taking the role and the datanode storage configuration as two independent parameters would let +/// a caller pass a datanode without its storage, which silently drops `dfs.datanode.data.dir`. pub fn build_rolegroup_config_map( cluster: &ValidatedCluster, cluster_info: &KubernetesClusterInfo, - role: &HdfsNodeRole, role_group_name: &RoleGroupName, rolegroup_config: &RoleGroupConfig, - datanode_storage: Option, - logging: &RoleGroupLogging, + resolved: &ResolvedRoleGroup, ) -> Result { + let role = resolved.role.node_role(); + tracing::info!( "Setting up ConfigMap for role {role} role group {role_group_name}", role = role.as_ref() ); - let metadata = build::rolegroup_metadata(cluster, role, role_group_name); + let metadata = build::rolegroup_metadata(cluster, &role, role_group_name); let config_overrides = &rolegroup_config.config_overrides; let cluster_config = &cluster.cluster_config; @@ -72,12 +72,12 @@ pub fn build_rolegroup_config_map( let hdfs_site_xml = hdfs_site::build( cluster, cluster_info, - datanode_storage, + resolved.role.datanode_storage(), config_overrides.hdfs_site_xml.clone(), ); let core_site_xml = core_site::build( cluster, - *role, + role, cluster_info, config_overrides.core_site_xml.clone(), ); @@ -108,10 +108,10 @@ pub fn build_rolegroup_config_map( )?, ); - for (log_config_file, log4j_config) in product_logging::build_log4j_configs(logging) { + for (log_config_file, log4j_config) in product_logging::build_log4j_configs(&resolved.logging) { builder.add_data(log_config_file, log4j_config); } - if logging.vector.is_some() { + if resolved.logging.vector.is_some() { builder.add_data( VECTOR_CONFIG_FILE, product_logging::vector_config_file_content(), diff --git a/rust/operator-binary/src/event.rs b/rust/operator-binary/src/event.rs index 8686db73..0948f571 100644 --- a/rust/operator-binary/src/event.rs +++ b/rust/operator-binary/src/event.rs @@ -1,19 +1,13 @@ -use std::collections::BTreeMap; - use snafu::{ResultExt, Snafu}; use stackable_operator::{ k8s_openapi::api::core::v1::ObjectReference, kube::runtime::events::{Event, EventType}, - v2::{ - role_utils::{JavaCommonConfig, RoleGroupConfig}, - types::operator::RoleGroupName, - }, }; use strum::{EnumDiscriminants, IntoStaticStr}; use crate::{ - controller::ValidatedCluster, - crd::{HdfsNodeRole, v1alpha1}, + controller::{ValidatedCluster, build::total_replicas}, + crd::HdfsNodeRole, hdfs_controller::Ctx, }; @@ -71,7 +65,7 @@ pub fn build_invalid_replica_message( Some(format!( "{role_name}: currently has an even number of replicas [{replicas}], but should always have an odd number to ensure quorum" )) - } else if !role.replicas_can_be_even() && replicas < dfs_replication as u16 { + } else if role.check_valid_dfs_replication() && replicas < dfs_replication as u16 { Some(format!( "{role_name}: HDFS replication factor [{dfs_replication}] is configured greater than data node replicas [{replicas}]" )) @@ -80,16 +74,133 @@ pub fn build_invalid_replica_message( } } -/// The total number of replicas across the role groups of one role, counting a role group without -/// an explicit replica count as zero. -fn total_replicas( - role_group_configs: &BTreeMap< - RoleGroupName, - RoleGroupConfig, - >, -) -> u16 { - role_group_configs - .values() - .map(|role_group| role_group.replicas.unwrap_or_default()) - .sum() +#[cfg(test)] +mod tests { + use super::*; + use crate::test_support::deserialize_and_validate_cluster; + + /// A role group with no explicit `replicas` runs one pod — Kubernetes' default for a + /// `StatefulSet` with `replicas: null` — so it must not be counted as zero. Counting it as + /// zero produced a warning event telling the user to configure at least one datanode when + /// they already had one. + #[test] + fn an_unset_replica_count_counts_as_one_datanode() { + let cluster = deserialize_and_validate_cluster( + " +--- +apiVersion: hdfs.stackable.tech/v1alpha1 +kind: HdfsCluster +metadata: + name: hdfs + namespace: default + uid: c2c8c5c0-0b5a-4b1e-9f3e-1a2b3c4d5e6f +spec: + image: + productVersion: 3.4.0 + clusterConfig: + zookeeperConfigMapName: hdfs-zk + dfsReplication: 1 + nameNodes: + roleGroups: + default: + replicas: 2 + journalNodes: + roleGroups: + default: + replicas: 3 + dataNodes: + roleGroups: + default: {} +", + ); + + assert_eq!( + build_invalid_replica_message(&cluster, &HdfsNodeRole::Data), + None + ); + } + + /// A role with no role groups at all really does have zero replicas, and still warns. + #[test] + fn a_role_without_role_groups_still_warns() { + let cluster = deserialize_and_validate_cluster( + " +--- +apiVersion: hdfs.stackable.tech/v1alpha1 +kind: HdfsCluster +metadata: + name: hdfs + namespace: default + uid: c2c8c5c0-0b5a-4b1e-9f3e-1a2b3c4d5e6f +spec: + image: + productVersion: 3.4.0 + clusterConfig: + zookeeperConfigMapName: hdfs-zk + dfsReplication: 1 + nameNodes: + roleGroups: {} + journalNodes: + roleGroups: + default: + replicas: 3 + dataNodes: + roleGroups: + default: + replicas: 1 +", + ); + + assert_eq!( + build_invalid_replica_message(&cluster, &HdfsNodeRole::Name).as_deref(), + Some( + "namenode: only has 0 replicas configured, it is strongly recommended to use at \ + least [2]" + ) + ); + } + + /// A `dfsReplication` above the datanode count means HDFS cannot place every replica, so the + /// user is warned. The gate for this is [`HdfsNodeRole::check_valid_dfs_replication`], which + /// is true for datanodes only — the message is about datanodes. + #[test] + fn fewer_datanodes_than_the_replication_factor_warns() { + let cluster = deserialize_and_validate_cluster( + " +--- +apiVersion: hdfs.stackable.tech/v1alpha1 +kind: HdfsCluster +metadata: + name: hdfs + namespace: default + uid: c2c8c5c0-0b5a-4b1e-9f3e-1a2b3c4d5e6f +spec: + image: + productVersion: 3.4.0 + clusterConfig: + zookeeperConfigMapName: hdfs-zk + dfsReplication: 3 + nameNodes: + roleGroups: + default: + replicas: 2 + journalNodes: + roleGroups: + default: + replicas: 3 + dataNodes: + roleGroups: + default: + replicas: 2 +", + ); + + assert_eq!( + build_invalid_replica_message(&cluster, &HdfsNodeRole::Data).as_deref(), + Some( + "datanode: HDFS replication factor [3] is configured greater than data node \ + replicas [2]" + ) + ); + } } diff --git a/rust/operator-binary/src/hdfs_controller.rs b/rust/operator-binary/src/hdfs_controller.rs index 517d6cfc..666a3a69 100644 --- a/rust/operator-binary/src/hdfs_controller.rs +++ b/rust/operator-binary/src/hdfs_controller.rs @@ -178,7 +178,10 @@ mod test { use super::*; use crate::{ HDFS_FULL_CONTROLLER_NAME, - controller::build::{ResolvedRoleGroup, RoleGroupLogging, container::ContainerConfig}, + controller::build::{ + ResolvedRoleGroup, RoleContainerLogging, RoleGroupLogging, RoleSpecificResources, + container::ContainerConfig, + }, crd::DataNodeContainer, test_support::{ datanode_config, datanode_role_group_config, deserialize_cluster, validate_cluster, @@ -234,10 +237,14 @@ spec: volume_claim_templates: ContainerConfig::datanode_volume_claim_templates( datanode_config, ), - listener_volume: Some( - ContainerConfig::datanode_listener_volume(datanode_config, &labels) - .expect("the datanode listener volume should build"), - ), + role: RoleSpecificResources::Data { + listener_volume: ContainerConfig::datanode_listener_volume( + datanode_config, + &labels, + ) + .expect("the datanode listener volume should build"), + storage: datanode_config.resources.storage.clone(), + }, logging: RoleGroupLogging { hdfs: datanode_config .logging @@ -249,15 +256,12 @@ spec: .for_container(&DataNodeContainer::Vector) .into_owned() }), - zkfc: None, - format_namenodes: None, - format_zookeeper: None, - wait_for_namenodes: Some( - datanode_config + role: RoleContainerLogging::Data { + wait_for_namenodes: datanode_config .logging .for_container(&DataNodeContainer::WaitForNameNodes) .into_owned(), - ), + }, }, }; From d15253b9b24378bf1b37e99e19b0a483dd35efd2 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Mon, 14 Sep 2026 15:38:00 +0200 Subject: [PATCH 10/19] make the role a property of the type, not a parameter --- CHANGELOG.md | 10 ++ .../src/controller/build/container.rs | 10 +- .../src/controller/build/mod.rs | 126 +++++++++++------- .../controller/build/resource/config_map.rs | 2 +- .../src/controller/build/resource/pdb.rs | 6 +- .../controller/build/resource/statefulset.rs | 13 +- rust/operator-binary/src/controller/mod.rs | 67 +++++----- .../src/controller/validate.rs | 25 ++-- rust/operator-binary/src/crd/mod.rs | 7 + rust/operator-binary/src/event.rs | 6 +- rust/operator-binary/src/hdfs_controller.rs | 48 +------ rust/operator-binary/src/test_support.rs | 20 +-- 12 files changed, 177 insertions(+), 163 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 43228e48..7b4caf3b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,9 @@ All notable changes to this project will be documented in this file. `app.kubernetes.io/role-group: none` labels. StatefulSet selectors and volume claim templates are unchanged, so upgrading is non-breaking. - Make operations infallible where dependent on static inputs ([#824], [#829]). +- Internal operator refactoring: the validated cluster carries each role's configuration in typed + per-role fields instead of maps keyed by role, so role-specific values are resolved once and the + shared resource builders can no longer be handed another role's configuration ([#830]). ### Fixed @@ -32,6 +35,12 @@ All notable changes to this project will be documented in this file. See [our internal issue](https://github.com/stackabletech/hdfs-operator/issues/626) and [the fix](https://github.com/kube-rs/kube/pull/2042) for details ([#814]). - The operator now watches all resources that it creates and early-exits the reconcile action when the cluster is marked for deletion ([#821]). +- A role group that does not set `replicas` is now counted as one replica instead of zero when the + replica counts are checked, so such a role group no longer produces a warning event claiming it + has zero replicas configured ([#830]). +- The warning that `dfsReplication` is greater than the number of datanodes is now raised for + datanodes. It was previously raised for journalnodes, even though its text refers to datanode + replicas, and never for datanodes ([#830]). [#801]: https://github.com/stackabletech/hdfs-operator/pull/801 [#806]: https://github.com/stackabletech/hdfs-operator/pull/806 @@ -42,6 +51,7 @@ All notable changes to this project will be documented in this file. [#821]: https://github.com/stackabletech/hdfs-operator/pull/821 [#824]: https://github.com/stackabletech/hdfs-operator/pull/824 [#829]: https://github.com/stackabletech/hdfs-operator/pull/829 +[#830]: https://github.com/stackabletech/hdfs-operator/pull/830 ## [26.7.0] - 2026-07-21 diff --git a/rust/operator-binary/src/controller/build/container.rs b/rust/operator-binary/src/controller/build/container.rs index 5fdf4452..a1b300ba 100644 --- a/rust/operator-binary/src/controller/build/container.rs +++ b/rust/operator-binary/src/controller/build/container.rs @@ -67,7 +67,7 @@ use crate::{ controller::{ ValidatedCluster, build::{ - self, ResolvedRoleGroup, RoleGroupLogging, + self, ResolvedRoleGroup, RoleGroupLogging, RoleGroupResolver, jvm::{self, construct_global_jvm_args, construct_role_specific_jvm_args}, kerberos::KERBEROS_CONTAINER_PATH, properties::product_logging::{ @@ -213,16 +213,18 @@ impl ContainerConfig { /// Add all main, side and init containers as well as required volumes to the pod builder. /// - /// Every role-specific value is resolved by the caller into `resolved`. - pub fn add_containers_and_volumes( + /// Every role-specific value is resolved by the caller into `resolved`. The role comes from + /// the role group's config type, so it cannot disagree with `resolved`: pairing a role with + /// another role's resolved values would silently drop the containers' `log4j.properties`. + pub fn add_containers_and_volumes( pb: &mut PodBuilder, cluster: &ValidatedCluster, cluster_info: &KubernetesClusterInfo, - role: &HdfsNodeRole, role_group_name: &RoleGroupName, rolegroup_config: &RoleGroupConfig, resolved: &ResolvedRoleGroup, ) -> Result<(), Error> { + let role = &C::ROLE; let namenode_podrefs = build::pod_refs(cluster, &HdfsNodeRole::Name); // HDFS main container diff --git a/rust/operator-binary/src/controller/build/mod.rs b/rust/operator-binary/src/controller/build/mod.rs index d2cfef05..1456f25f 100644 --- a/rust/operator-binary/src/controller/build/mod.rs +++ b/rust/operator-binary/src/controller/build/mod.rs @@ -1,5 +1,6 @@ use std::{ collections::{BTreeMap, HashMap}, + fmt::Display, marker::PhantomData, }; @@ -12,7 +13,7 @@ use stackable_operator::{ policy::v1::PodDisruptionBudget, }, kvp::{LabelError, Labels}, - product_logging::spec::ContainerLogConfig, + product_logging::spec::{ContainerLogConfig, Logging}, utils::cluster_info::KubernetesClusterInfo, v2::{ builder::meta::ownerreference_from_resource, @@ -116,7 +117,7 @@ pub enum Error { /// The log configuration of every container in one role group, resolved during the build step /// by code that knows the role, so the shared builders never see a role-specific /// `Logging`. -#[derive(Clone, Debug)] +#[derive(Debug)] pub struct RoleGroupLogging { /// The main `hdfs` container, which every role has. pub hdfs: ContainerLogConfig, @@ -133,7 +134,7 @@ pub struct RoleGroupLogging { /// missing log config is otherwise silent: the container's `log4j.properties` is left out of both /// the `ConfigMap` and the `cp` in the container args, so it logs with Hadoop's built-in defaults /// and Vector collects nothing for it. -#[derive(Clone, Debug)] +#[derive(Debug)] pub enum RoleContainerLogging { /// Journalnodes run no role-specific container. Journal, @@ -258,20 +259,45 @@ impl RoleSpecificResources { /// The datanode data volume configuration, which drives `dfs.datanode.data.dir`; `None` for /// the other roles. - pub fn datanode_storage(&self) -> Option { + pub fn datanode_storage(&self) -> Option<&DataNodeStorageConfigInnerType> { match self { - Self::Data { storage, .. } => Some(storage.clone()), + Self::Data { storage, .. } => Some(storage), Self::Journal | Self::Name => None, } } } +/// The log config of the two containers every role has: the main `hdfs` container, and the Vector +/// sidecar, which is `None` when the Vector agent is disabled for the role group. +/// +/// Each role names these containers with its own enum, so this is generic over that enum rather +/// than repeated once per role. +fn common_container_logging( + logging: &Logging, + hdfs: T, + vector: T, +) -> (ContainerLogConfig, Option) +where + T: Clone + Display + Ord, +{ + ( + logging.for_container(&hdfs).into_owned(), + logging + .enable_vector_agent + .then(|| logging.for_container(&vector).into_owned()), + ) +} + /// How to resolve one role group's role-specific values, implemented once per role config type. /// /// This is what lets [`build_role`] be written once: the trait supplies the role and the single /// role-dependent step, and everything else about building a role group is identical across the /// three roles. -trait RoleGroupResolver { +/// +/// [`Self::ROLE`] is also the single source of truth for the role in the shared builders: they +/// take the role group's config type and read the role from it, rather than taking the role as a +/// second parameter that a caller could pair with the wrong config. +pub(crate) trait RoleGroupResolver { /// The role whose config this is. const ROLE: HdfsNodeRole; @@ -292,6 +318,12 @@ impl RoleGroupResolver for JournalNodeConfig { _role_group_name: &RoleGroupName, selector_labels: Labels, ) -> Result { + let (hdfs, vector) = common_container_logging( + &self.logging, + JournalNodeContainer::Hdfs, + JournalNodeContainer::Vector, + ); + Ok(ResolvedRoleGroup { selector_labels, common: self.common.clone(), @@ -299,15 +331,8 @@ impl RoleGroupResolver for JournalNodeConfig { volume_claim_templates: ContainerConfig::journalnode_volume_claim_templates(self), role: RoleSpecificResources::Journal, logging: RoleGroupLogging { - hdfs: self - .logging - .for_container(&JournalNodeContainer::Hdfs) - .into_owned(), - vector: self.logging.enable_vector_agent.then(|| { - self.logging - .for_container(&JournalNodeContainer::Vector) - .into_owned() - }), + hdfs, + vector, role: RoleContainerLogging::Journal, }, }) @@ -332,6 +357,12 @@ impl RoleGroupResolver for NameNodeConfig { }, )?; + let (hdfs, vector) = common_container_logging( + &self.logging, + NameNodeContainer::Hdfs, + NameNodeContainer::Vector, + ); + Ok(ResolvedRoleGroup { selector_labels, common: self.common.clone(), @@ -339,15 +370,8 @@ impl RoleGroupResolver for NameNodeConfig { volume_claim_templates, role: RoleSpecificResources::Name, logging: RoleGroupLogging { - hdfs: self - .logging - .for_container(&NameNodeContainer::Hdfs) - .into_owned(), - vector: self.logging.enable_vector_agent.then(|| { - self.logging - .for_container(&NameNodeContainer::Vector) - .into_owned() - }), + hdfs, + vector, role: RoleContainerLogging::Name { zkfc: self .logging @@ -382,6 +406,12 @@ impl RoleGroupResolver for DataNodeConfig { role_group: role_group_name.clone(), })?; + let (hdfs, vector) = common_container_logging( + &self.logging, + DataNodeContainer::Hdfs, + DataNodeContainer::Vector, + ); + Ok(ResolvedRoleGroup { selector_labels, common: self.common.clone(), @@ -392,15 +422,8 @@ impl RoleGroupResolver for DataNodeConfig { storage: self.resources.storage.clone(), }, logging: RoleGroupLogging { - hdfs: self - .logging - .for_container(&DataNodeContainer::Hdfs) - .into_owned(), - vector: self.logging.enable_vector_agent.then(|| { - self.logging - .for_container(&DataNodeContainer::Vector) - .into_owned() - }), + hdfs, + vector, role: RoleContainerLogging::Data { wait_for_namenodes: self .logging @@ -418,7 +441,10 @@ impl RoleGroupResolver for DataNodeConfig { struct RoleGroupResources { services: Vec, config_maps: Vec, - stateful_sets: Vec, + /// Keyed by role so that flattening the map yields the StatefulSets in rollout order, + /// whatever order [`build`] happens to call [`build_role`] in. See [`HdfsNodeRole`], whose + /// variant order defines that rollout order. + stateful_sets: BTreeMap>, pod_disruption_budgets: Vec, } @@ -459,11 +485,10 @@ fn build_role( role_group: role_group_name.clone(), })?, ); - out.stateful_sets.push( + out.stateful_sets.entry(C::ROLE).or_default().push( resource::statefulset::build_rolegroup_statefulset( cluster, cluster_info, - role, role_group_name, rg_config, resolved, @@ -492,7 +517,8 @@ fn build_role( /// The resources are returned as flat collections. `stateful_sets` is ordered by role — /// journalnodes, then namenodes, then datanodes — because the apply step rolls them out in that /// order during upgrades to preserve HDFS's rollout-gated deployment (see -/// [`crate::controller::apply::Applier::apply`]). +/// [`crate::controller::apply::Applier::apply`]). That ordering is structural: they are +/// accumulated in a [`BTreeMap`] keyed by [`HdfsNodeRole`] and flattened in key order. /// The discovery `ConfigMap` is included when it can be built or re-emitted (see /// [`resource::discovery::build_discovery_config_map`]); it is only absent before its first /// successful build. @@ -502,26 +528,27 @@ pub fn build( ) -> Result, Error> { let mut built = RoleGroupResources::default(); - // The roles are built in the order journalnode, namenode, datanode, and the resulting - // `stateful_sets` order is load-bearing: the apply step rolls the StatefulSets out in that - // order during upgrades, each role gated on the previous one (see - // [`crate::controller::apply::Applier::apply`]). + // The rollout order of the StatefulSets is load-bearing: the apply step rolls them out + // journalnodes first, then namenodes, then datanodes, each role gated on the previous one + // (see [`crate::controller::apply::Applier::apply`]). That order comes from the + // `HdfsNodeRole` key of `RoleGroupResources::stateful_sets`, not from the order of the calls + // below, which are free to be rearranged. build_role( cluster, cluster_info, - &cluster.journalnode_role_group_configs, + &cluster.journalnode.role_groups, &mut built, )?; build_role( cluster, cluster_info, - &cluster.namenode_role_group_configs, + &cluster.namenode.role_groups, &mut built, )?; build_role( cluster, cluster_info, - &cluster.datanode_role_group_configs, + &cluster.datanode.role_groups, &mut built, )?; @@ -546,7 +573,8 @@ pub fn build( services, config_maps, pod_disruption_budgets, - stateful_sets, + // `BTreeMap` iterates in key order, so this is the rollout order the apply step needs. + stateful_sets: stateful_sets.into_values().flatten().collect(), service_accounts: vec![build_service_account(cluster)], role_bindings: vec![build_role_binding(cluster)], status: PhantomData, @@ -633,9 +661,9 @@ pub(crate) fn pod_refs(cluster: &ValidatedCluster, role: &HdfsNodeRole) -> Vec role_group_replicas(&cluster.namenode_role_group_configs), - HdfsNodeRole::Data => role_group_replicas(&cluster.datanode_role_group_configs), - HdfsNodeRole::Journal => role_group_replicas(&cluster.journalnode_role_group_configs), + HdfsNodeRole::Name => role_group_replicas(&cluster.namenode.role_groups), + HdfsNodeRole::Data => role_group_replicas(&cluster.datanode.role_groups), + HdfsNodeRole::Journal => role_group_replicas(&cluster.journalnode.role_groups), }; replicas_per_role_group @@ -710,7 +738,7 @@ pub(crate) fn rolegroup_selector_labels( /// The total number of datanode replicas across all datanode role groups. pub(crate) fn num_datanodes(cluster: &ValidatedCluster) -> u16 { - total_replicas(&cluster.datanode_role_group_configs) + total_replicas(&cluster.datanode.role_groups) } /// The ports exposed by the rolegroup headless service for the given `role`. diff --git a/rust/operator-binary/src/controller/build/resource/config_map.rs b/rust/operator-binary/src/controller/build/resource/config_map.rs index 3ab90448..9d224cae 100644 --- a/rust/operator-binary/src/controller/build/resource/config_map.rs +++ b/rust/operator-binary/src/controller/build/resource/config_map.rs @@ -72,7 +72,7 @@ pub fn build_rolegroup_config_map( let hdfs_site_xml = hdfs_site::build( cluster, cluster_info, - resolved.role.datanode_storage(), + resolved.role.datanode_storage().cloned(), config_overrides.hdfs_site_xml.clone(), ); let core_site_xml = core_site::build( diff --git a/rust/operator-binary/src/controller/build/resource/pdb.rs b/rust/operator-binary/src/controller/build/resource/pdb.rs index 12136da2..f83ca934 100644 --- a/rust/operator-binary/src/controller/build/resource/pdb.rs +++ b/rust/operator-binary/src/controller/build/resource/pdb.rs @@ -14,9 +14,9 @@ use crate::{ /// has no validated config or PDBs are disabled. pub fn build_pdb(cluster: &ValidatedCluster, role: &HdfsNodeRole) -> Option { let role_config = match role { - HdfsNodeRole::Name => cluster.namenode_config.as_ref(), - HdfsNodeRole::Data => cluster.datanode_config.as_ref(), - HdfsNodeRole::Journal => cluster.journalnode_config.as_ref(), + HdfsNodeRole::Name => cluster.namenode.config.as_ref(), + HdfsNodeRole::Data => cluster.datanode.config.as_ref(), + HdfsNodeRole::Journal => cluster.journalnode.config.as_ref(), }?; let pdb = &role_config.pdb; if !pdb.enabled { diff --git a/rust/operator-binary/src/controller/build/resource/statefulset.rs b/rust/operator-binary/src/controller/build/resource/statefulset.rs index bd9f1a5a..8a94b2bb 100644 --- a/rust/operator-binary/src/controller/build/resource/statefulset.rs +++ b/rust/operator-binary/src/controller/build/resource/statefulset.rs @@ -20,12 +20,12 @@ use crate::{ controller::{ ValidatedCluster, build::{ - self, ResolvedRoleGroup, + self, ResolvedRoleGroup, RoleGroupResolver, container::{self, ContainerConfig}, graceful_shutdown::{self, add_graceful_shutdown_config}, }, }, - crd::{HdfsNodeRole, v1alpha1}, + crd::v1alpha1, }; #[derive(Snafu, Debug)] @@ -39,15 +39,17 @@ pub enum Error { /// Builds the [`StatefulSet`] of one role group. /// -/// Every role-specific value is resolved by the caller into `resolved`. -pub(crate) fn build_rolegroup_statefulset( +/// Every role-specific value is resolved by the caller into `resolved`. The role comes from the +/// role group's config type, so it cannot disagree with `resolved`. +pub(crate) fn build_rolegroup_statefulset( validated: &ValidatedCluster, cluster_info: &KubernetesClusterInfo, - role: &HdfsNodeRole, role_group_name: &RoleGroupName, rolegroup_config: &RoleGroupConfig, resolved: ResolvedRoleGroup, ) -> Result { + let role = &C::ROLE; + tracing::info!( "Setting up StatefulSet for role {role} role group {role_group_name}", role = role.as_ref() @@ -83,7 +85,6 @@ pub(crate) fn build_rolegroup_statefulset( &mut pb, validated, cluster_info, - role, role_group_name, rolegroup_config, &resolved, diff --git a/rust/operator-binary/src/controller/mod.rs b/rust/operator-binary/src/controller/mod.rs index 14765786..97fcccdd 100644 --- a/rust/operator-binary/src/controller/mod.rs +++ b/rust/operator-binary/src/controller/mod.rs @@ -73,17 +73,25 @@ pub struct KubernetesResources { pub status: PhantomData, } -/// The [`RoleGroupConfig`] of one namenode role group. -pub type NameNodeRoleGroupConfig = - RoleGroupConfig; +/// The [`RoleGroupConfig`] of one HDFS role group, specialised for the role's validated config +/// type `C` (one of [`NameNodeConfig`], [`DataNodeConfig`] or [`JournalNodeConfig`]). +pub type HdfsRoleGroupConfig = + RoleGroupConfig; -/// The [`RoleGroupConfig`] of one datanode role group. -pub type DataNodeRoleGroupConfig = - RoleGroupConfig; - -/// The [`RoleGroupConfig`] of one journalnode role group. -pub type JournalNodeRoleGroupConfig = - RoleGroupConfig; +/// One role's validated configuration: every role group of the role, plus the role-level config. +/// +/// The two are kept together so they cannot be paired with the wrong role. `C` is the role's own +/// validated config type, so a `ValidatedRole` does not compile where a +/// `ValidatedRole` is expected — passing one role's PodDisruptionBudget to +/// another role would otherwise be a silent swap of two same-typed values. +#[derive(Clone, Debug)] +pub struct ValidatedRole { + /// The validated config of every role group, keyed by role group name; empty if the role is + /// absent from the spec. + pub role_groups: BTreeMap>, + /// The role-level config (currently the PDB), or `None` if the role is absent. + pub config: Option, +} /// The validated cluster: proves that config merging and validation succeeded /// for every role and role group before any resources are created. Placed in the @@ -104,21 +112,12 @@ pub struct ValidatedCluster { pub product_version: ProductVersion, pub image: ResolvedProductImage, pub cluster_config: ValidatedClusterConfig, - /// The validated config of every namenode role group, keyed by role group name; empty if the - /// role is absent. - pub namenode_role_group_configs: BTreeMap, - /// The validated config of every datanode role group, keyed by role group name; empty if the - /// role is absent. - pub datanode_role_group_configs: BTreeMap, - /// The validated config of every journalnode role group, keyed by role group name; empty if - /// the role is absent. - pub journalnode_role_group_configs: BTreeMap, - /// The namenode role-level config (currently the PDB), or `None` if the role is absent. - pub namenode_config: Option, - /// The datanode role-level config (currently the PDB), or `None` if the role is absent. - pub datanode_config: Option, - /// The journalnode role-level config (currently the PDB), or `None` if the role is absent. - pub journalnode_config: Option, + /// The namenode role: its role groups and its role-level config. + pub namenode: ValidatedRole, + /// The datanode role: its role groups and its role-level config. + pub datanode: ValidatedRole, + /// The journalnode role: its role groups and its role-level config. + pub journalnode: ValidatedRole, /// The namenode pod `Listener`s as currently stored in the cluster (see /// [`crate::controller::dereference::DereferencedObjects::namenode_listeners`]). pub namenode_listeners: Vec, @@ -138,12 +137,9 @@ impl ValidatedCluster { uid: Uid, image: ResolvedProductImage, cluster_config: ValidatedClusterConfig, - namenode_role_group_configs: BTreeMap, - datanode_role_group_configs: BTreeMap, - journalnode_role_group_configs: BTreeMap, - namenode_config: Option, - datanode_config: Option, - journalnode_config: Option, + namenode: ValidatedRole, + datanode: ValidatedRole, + journalnode: ValidatedRole, namenode_listeners: Vec, discovery_config_map: Option, status: ValidatedClusterStatus, @@ -167,12 +163,9 @@ impl ValidatedCluster { image, product_version, cluster_config, - namenode_role_group_configs, - datanode_role_group_configs, - journalnode_role_group_configs, - namenode_config, - datanode_config, - journalnode_config, + namenode, + datanode, + journalnode, namenode_listeners, discovery_config_map, status, diff --git a/rust/operator-binary/src/controller/validate.rs b/rust/operator-binary/src/controller/validate.rs index 2c4b0cdf..fb02a9ff 100644 --- a/rust/operator-binary/src/controller/validate.rs +++ b/rust/operator-binary/src/controller/validate.rs @@ -16,8 +16,8 @@ use stackable_operator::{ use crate::{ controller::{ - ValidatedCluster, ValidatedClusterConfig, ValidatedClusterStatus, ValidatedRoleConfig, - dereference::DereferencedObjects, + ValidatedCluster, ValidatedClusterConfig, ValidatedClusterStatus, ValidatedRole, + ValidatedRoleConfig, dereference::DereferencedObjects, }, crd::{ DataNodeConfigFragment, HdfsNodeRole, JournalNodeConfigFragment, NameNodeConfigFragment, @@ -99,6 +99,9 @@ pub fn validate_cluster( let cluster_name = get_cluster_name(hdfs).context(GetClusterNameSnafu)?; + // Validated in `HdfsNodeRole` declaration order, because the first role that fails is the + // error the user sees: reordering these three statements changes which misconfiguration gets + // reported when more than one role is wrong. let journalnode_role_group_configs = validate_role_group_configs( hdfs.spec.journal_nodes.as_ref(), JournalNodeConfigFragment::default_config(cluster_name.as_ref(), &HdfsNodeRole::Journal), @@ -132,12 +135,18 @@ pub fn validate_cluster( uid, image, ValidatedClusterConfig::resolve(hdfs, hdfs_opa_config), - namenode_role_group_configs, - datanode_role_group_configs, - journalnode_role_group_configs, - validated_role_config(HdfsNodeRole::Name), - validated_role_config(HdfsNodeRole::Data), - validated_role_config(HdfsNodeRole::Journal), + ValidatedRole { + role_groups: namenode_role_group_configs, + config: validated_role_config(HdfsNodeRole::Name), + }, + ValidatedRole { + role_groups: datanode_role_group_configs, + config: validated_role_config(HdfsNodeRole::Data), + }, + ValidatedRole { + role_groups: journalnode_role_group_configs, + config: validated_role_config(HdfsNodeRole::Journal), + }, namenode_listeners, discovery_config_map, status, diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs index 419c5f56..13498519 100644 --- a/rust/operator-binary/src/crd/mod.rs +++ b/rust/operator-binary/src/crd/mod.rs @@ -358,6 +358,13 @@ constant!(JOURNALNODE_ROLE_NAME: RoleName = "journalnode"); constant!(NAMENODE_ROLE_NAME: RoleName = "namenode"); constant!(DATANODE_ROLE_NAME: RoleName = "datanode"); +/// The HDFS roles, declared in the order they must be rolled out during an upgrade: +/// journalnodes, then namenodes, then datanodes. +/// +/// The variant order is load-bearing, because the derived [`Ord`] is what orders the +/// StatefulSets the apply step rolls out (see +/// [`crate::controller::build::build`] and [`crate::controller::apply::Applier::apply`]). +/// Reordering the variants reorders an HDFS upgrade. #[derive( Clone, Copy, diff --git a/rust/operator-binary/src/event.rs b/rust/operator-binary/src/event.rs index 0948f571..dd83a375 100644 --- a/rust/operator-binary/src/event.rs +++ b/rust/operator-binary/src/event.rs @@ -48,9 +48,9 @@ pub fn build_invalid_replica_message( role: &HdfsNodeRole, ) -> Option { let replicas = match role { - HdfsNodeRole::Name => total_replicas(&validated_cluster.namenode_role_group_configs), - HdfsNodeRole::Data => total_replicas(&validated_cluster.datanode_role_group_configs), - HdfsNodeRole::Journal => total_replicas(&validated_cluster.journalnode_role_group_configs), + HdfsNodeRole::Name => total_replicas(&validated_cluster.namenode.role_groups), + HdfsNodeRole::Data => total_replicas(&validated_cluster.datanode.role_groups), + HdfsNodeRole::Journal => total_replicas(&validated_cluster.journalnode.role_groups), }; let dfs_replication = validated_cluster.cluster_config.dfs_replication; diff --git a/rust/operator-binary/src/hdfs_controller.rs b/rust/operator-binary/src/hdfs_controller.rs index 666a3a69..7734aa21 100644 --- a/rust/operator-binary/src/hdfs_controller.rs +++ b/rust/operator-binary/src/hdfs_controller.rs @@ -178,11 +178,7 @@ mod test { use super::*; use crate::{ HDFS_FULL_CONTROLLER_NAME, - controller::build::{ - ResolvedRoleGroup, RoleContainerLogging, RoleGroupLogging, RoleSpecificResources, - container::ContainerConfig, - }, - crd::DataNodeContainer, + controller::build::{RoleGroupResolver, container::ContainerConfig}, test_support::{ datanode_config, datanode_role_group_config, deserialize_cluster, validate_cluster, }, @@ -228,42 +224,11 @@ spec: let validated_cluster = validate_cluster(&hdfs); let role_group_name = RoleGroupName::from_str("default").unwrap(); let role_group_config = datanode_role_group_config(&validated_cluster, &role_group_name); - let datanode_config = datanode_config(&validated_cluster, &role_group_name); - let labels = Labels::new(); - let resolved = ResolvedRoleGroup { - selector_labels: labels.clone(), - common: datanode_config.common.clone(), - resources: datanode_config.resources.clone().into(), - volume_claim_templates: ContainerConfig::datanode_volume_claim_templates( - datanode_config, - ), - role: RoleSpecificResources::Data { - listener_volume: ContainerConfig::datanode_listener_volume( - datanode_config, - &labels, - ) - .expect("the datanode listener volume should build"), - storage: datanode_config.resources.storage.clone(), - }, - logging: RoleGroupLogging { - hdfs: datanode_config - .logging - .for_container(&DataNodeContainer::Hdfs) - .into_owned(), - vector: datanode_config.logging.enable_vector_agent.then(|| { - datanode_config - .logging - .for_container(&DataNodeContainer::Vector) - .into_owned() - }), - role: RoleContainerLogging::Data { - wait_for_namenodes: datanode_config - .logging - .for_container(&DataNodeContainer::WaitForNameNodes) - .into_owned(), - }, - }, - }; + // Resolved through the production path, so this test cannot drift from what the build + // step actually hands the container builder. + let resolved = datanode_config(&validated_cluster, &role_group_name) + .resolve(&role_group_name, Labels::new()) + .expect("the datanode role group should resolve"); let mut pb = PodBuilder::new(); pb.metadata(ObjectMeta::default()); @@ -273,7 +238,6 @@ spec: &KubernetesClusterInfo { cluster_domain: DomainName::try_from("cluster.local").unwrap(), }, - &role, &role_group_name, role_group_config, &resolved, diff --git a/rust/operator-binary/src/test_support.rs b/rust/operator-binary/src/test_support.rs index ef8c6e1f..fd72c4ae 100644 --- a/rust/operator-binary/src/test_support.rs +++ b/rust/operator-binary/src/test_support.rs @@ -3,10 +3,7 @@ use std::str::FromStr; use stackable_operator::v2::types::operator::RoleGroupName; use crate::{ - controller::{ - DataNodeRoleGroupConfig, JournalNodeRoleGroupConfig, NameNodeRoleGroupConfig, - ValidatedCluster, validate, - }, + controller::{HdfsRoleGroupConfig, ValidatedCluster, validate}, crd::{ CommonNodeConfig, DataNodeConfig, HdfsNodeRole, JournalNodeConfig, NameNodeConfig, v1alpha1, }, @@ -54,9 +51,10 @@ pub fn role_group_name(name: &str) -> RoleGroupName { pub fn namenode_role_group_config<'a>( validated_cluster: &'a ValidatedCluster, role_group_name: &RoleGroupName, -) -> &'a NameNodeRoleGroupConfig { +) -> &'a HdfsRoleGroupConfig { validated_cluster - .namenode_role_group_configs + .namenode + .role_groups .get(role_group_name) .expect("namenode role group should be defined") } @@ -64,9 +62,10 @@ pub fn namenode_role_group_config<'a>( pub fn datanode_role_group_config<'a>( validated_cluster: &'a ValidatedCluster, role_group_name: &RoleGroupName, -) -> &'a DataNodeRoleGroupConfig { +) -> &'a HdfsRoleGroupConfig { validated_cluster - .datanode_role_group_configs + .datanode + .role_groups .get(role_group_name) .expect("datanode role group should be defined") } @@ -74,9 +73,10 @@ pub fn datanode_role_group_config<'a>( pub fn journalnode_role_group_config<'a>( validated_cluster: &'a ValidatedCluster, role_group_name: &RoleGroupName, -) -> &'a JournalNodeRoleGroupConfig { +) -> &'a HdfsRoleGroupConfig { validated_cluster - .journalnode_role_group_configs + .journalnode + .role_groups .get(role_group_name) .expect("journalnode role group should be defined") } From 6791951d914fd9074267284e3793405969e29546 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Mon, 14 Sep 2026 17:30:43 +0200 Subject: [PATCH 11/19] split the roles back into the flat per-role fields the task specifies --- .../src/controller/build/mod.rs | 14 +-- .../src/controller/build/resource/pdb.rs | 6 +- rust/operator-binary/src/controller/mod.rs | 110 ++++++++---------- .../src/controller/validate.rs | 35 +++--- rust/operator-binary/src/event.rs | 6 +- rust/operator-binary/src/test_support.rs | 20 ++-- 6 files changed, 87 insertions(+), 104 deletions(-) diff --git a/rust/operator-binary/src/controller/build/mod.rs b/rust/operator-binary/src/controller/build/mod.rs index 1456f25f..9ff7c595 100644 --- a/rust/operator-binary/src/controller/build/mod.rs +++ b/rust/operator-binary/src/controller/build/mod.rs @@ -536,19 +536,19 @@ pub fn build( build_role( cluster, cluster_info, - &cluster.journalnode.role_groups, + &cluster.journalnode_role_group_configs, &mut built, )?; build_role( cluster, cluster_info, - &cluster.namenode.role_groups, + &cluster.namenode_role_group_configs, &mut built, )?; build_role( cluster, cluster_info, - &cluster.datanode.role_groups, + &cluster.datanode_role_group_configs, &mut built, )?; @@ -661,9 +661,9 @@ pub(crate) fn pod_refs(cluster: &ValidatedCluster, role: &HdfsNodeRole) -> Vec role_group_replicas(&cluster.namenode.role_groups), - HdfsNodeRole::Data => role_group_replicas(&cluster.datanode.role_groups), - HdfsNodeRole::Journal => role_group_replicas(&cluster.journalnode.role_groups), + HdfsNodeRole::Name => role_group_replicas(&cluster.namenode_role_group_configs), + HdfsNodeRole::Data => role_group_replicas(&cluster.datanode_role_group_configs), + HdfsNodeRole::Journal => role_group_replicas(&cluster.journalnode_role_group_configs), }; replicas_per_role_group @@ -738,7 +738,7 @@ pub(crate) fn rolegroup_selector_labels( /// The total number of datanode replicas across all datanode role groups. pub(crate) fn num_datanodes(cluster: &ValidatedCluster) -> u16 { - total_replicas(&cluster.datanode.role_groups) + total_replicas(&cluster.datanode_role_group_configs) } /// The ports exposed by the rolegroup headless service for the given `role`. diff --git a/rust/operator-binary/src/controller/build/resource/pdb.rs b/rust/operator-binary/src/controller/build/resource/pdb.rs index f83ca934..12136da2 100644 --- a/rust/operator-binary/src/controller/build/resource/pdb.rs +++ b/rust/operator-binary/src/controller/build/resource/pdb.rs @@ -14,9 +14,9 @@ use crate::{ /// has no validated config or PDBs are disabled. pub fn build_pdb(cluster: &ValidatedCluster, role: &HdfsNodeRole) -> Option { let role_config = match role { - HdfsNodeRole::Name => cluster.namenode.config.as_ref(), - HdfsNodeRole::Data => cluster.datanode.config.as_ref(), - HdfsNodeRole::Journal => cluster.journalnode.config.as_ref(), + HdfsNodeRole::Name => cluster.namenode_config.as_ref(), + HdfsNodeRole::Data => cluster.datanode_config.as_ref(), + HdfsNodeRole::Journal => cluster.journalnode_config.as_ref(), }?; let pdb = &role_config.pdb; if !pdb.enabled { diff --git a/rust/operator-binary/src/controller/mod.rs b/rust/operator-binary/src/controller/mod.rs index 97fcccdd..68f5a444 100644 --- a/rust/operator-binary/src/controller/mod.rs +++ b/rust/operator-binary/src/controller/mod.rs @@ -73,25 +73,17 @@ pub struct KubernetesResources { pub status: PhantomData, } -/// The [`RoleGroupConfig`] of one HDFS role group, specialised for the role's validated config -/// type `C` (one of [`NameNodeConfig`], [`DataNodeConfig`] or [`JournalNodeConfig`]). -pub type HdfsRoleGroupConfig = - RoleGroupConfig; +/// The [`RoleGroupConfig`] of one namenode role group. +pub type NameNodeRoleGroupConfig = + RoleGroupConfig; -/// One role's validated configuration: every role group of the role, plus the role-level config. -/// -/// The two are kept together so they cannot be paired with the wrong role. `C` is the role's own -/// validated config type, so a `ValidatedRole` does not compile where a -/// `ValidatedRole` is expected — passing one role's PodDisruptionBudget to -/// another role would otherwise be a silent swap of two same-typed values. -#[derive(Clone, Debug)] -pub struct ValidatedRole { - /// The validated config of every role group, keyed by role group name; empty if the role is - /// absent from the spec. - pub role_groups: BTreeMap>, - /// The role-level config (currently the PDB), or `None` if the role is absent. - pub config: Option, -} +/// The [`RoleGroupConfig`] of one datanode role group. +pub type DataNodeRoleGroupConfig = + RoleGroupConfig; + +/// The [`RoleGroupConfig`] of one journalnode role group. +pub type JournalNodeRoleGroupConfig = + RoleGroupConfig; /// The validated cluster: proves that config merging and validation succeeded /// for every role and role group before any resources are created. Placed in the @@ -112,12 +104,21 @@ pub struct ValidatedCluster { pub product_version: ProductVersion, pub image: ResolvedProductImage, pub cluster_config: ValidatedClusterConfig, - /// The namenode role: its role groups and its role-level config. - pub namenode: ValidatedRole, - /// The datanode role: its role groups and its role-level config. - pub datanode: ValidatedRole, - /// The journalnode role: its role groups and its role-level config. - pub journalnode: ValidatedRole, + /// The namenode role-level config (currently the PDB), or `None` if the role is absent. + pub namenode_config: Option, + /// The validated config of every namenode role group, keyed by role group name; empty if the + /// role is absent. + pub namenode_role_group_configs: BTreeMap, + /// The datanode role-level config (currently the PDB), or `None` if the role is absent. + pub datanode_config: Option, + /// The validated config of every datanode role group, keyed by role group name; empty if the + /// role is absent. + pub datanode_role_group_configs: BTreeMap, + /// The journalnode role-level config (currently the PDB), or `None` if the role is absent. + pub journalnode_config: Option, + /// The validated config of every journalnode role group, keyed by role group name; empty if + /// the role is absent. + pub journalnode_role_group_configs: BTreeMap, /// The namenode pod `Listener`s as currently stored in the cluster (see /// [`crate::controller::dereference::DereferencedObjects::namenode_listeners`]). pub namenode_listeners: Vec, @@ -130,48 +131,31 @@ pub struct ValidatedCluster { } impl ValidatedCluster { - #[allow(clippy::too_many_arguments)] - pub fn new( - name: ClusterName, - namespace: NamespaceName, - uid: Uid, - image: ResolvedProductImage, - cluster_config: ValidatedClusterConfig, - namenode: ValidatedRole, - datanode: ValidatedRole, - journalnode: ValidatedRole, - namenode_listeners: Vec, - discovery_config_map: Option, - status: ValidatedClusterStatus, - ) -> Self { - // `app_version_label_value` is constructed to be a valid label value, so it is also a valid - // `ProductVersion`. - let product_version = ProductVersion::from_str(&image.app_version_label_value) - .expect("the app version label value is a valid product version"); - Self { - metadata: ObjectMeta { - name: Some(name.to_string()), - namespace: Some(namespace.to_string()), - // The uid is required so this type can produce valid owner references - // (Kubernetes rejects owner references without a uid). - uid: Some(uid.to_string()), - ..ObjectMeta::default() - }, - name, - namespace, - uid, - image, - product_version, - cluster_config, - namenode, - datanode, - journalnode, - namenode_listeners, - discovery_config_map, - status, + /// The `ObjectMeta` a `ValidatedCluster` carries so it can own the objects built from it. + /// + /// The uid is required: Kubernetes rejects owner references without one. + pub(crate) fn object_meta( + name: &ClusterName, + namespace: &NamespaceName, + uid: &Uid, + ) -> ObjectMeta { + ObjectMeta { + name: Some(name.to_string()), + namespace: Some(namespace.to_string()), + uid: Some(uid.to_string()), + ..ObjectMeta::default() } } + /// The product version of the resolved image. + /// + /// `app_version_label_value` is constructed to be a valid label value, so it is also a valid + /// `ProductVersion`. + pub(crate) fn product_version(image: &ResolvedProductImage) -> ProductVersion { + ProductVersion::from_str(&image.app_version_label_value) + .expect("the app version label value is a valid product version") + } + /// Whether HTTPS is enabled, derived from the validated authentication settings. pub fn has_https_enabled(&self) -> bool { self.cluster_config.authentication.is_some() diff --git a/rust/operator-binary/src/controller/validate.rs b/rust/operator-binary/src/controller/validate.rs index fb02a9ff..9fafb430 100644 --- a/rust/operator-binary/src/controller/validate.rs +++ b/rust/operator-binary/src/controller/validate.rs @@ -16,8 +16,8 @@ use stackable_operator::{ use crate::{ controller::{ - ValidatedCluster, ValidatedClusterConfig, ValidatedClusterStatus, ValidatedRole, - ValidatedRoleConfig, dereference::DereferencedObjects, + ValidatedCluster, ValidatedClusterConfig, ValidatedClusterStatus, ValidatedRoleConfig, + dereference::DereferencedObjects, }, crd::{ DataNodeConfigFragment, HdfsNodeRole, JournalNodeConfigFragment, NameNodeConfigFragment, @@ -129,28 +129,27 @@ pub fn validate_cluster( .and_then(|status| status.upgrade_target_product_version.clone()), }; - Ok(ValidatedCluster::new( - cluster_name, + // Built as a struct literal rather than through a constructor: the three role-level configs + // are the same type, so as positional arguments two of them could be swapped silently, giving + // a role another role's PodDisruptionBudget. + Ok(ValidatedCluster { + metadata: ValidatedCluster::object_meta(&cluster_name, &namespace, &uid), + product_version: ValidatedCluster::product_version(&image), + name: cluster_name, namespace, uid, + cluster_config: ValidatedClusterConfig::resolve(hdfs, hdfs_opa_config), image, - ValidatedClusterConfig::resolve(hdfs, hdfs_opa_config), - ValidatedRole { - role_groups: namenode_role_group_configs, - config: validated_role_config(HdfsNodeRole::Name), - }, - ValidatedRole { - role_groups: datanode_role_group_configs, - config: validated_role_config(HdfsNodeRole::Data), - }, - ValidatedRole { - role_groups: journalnode_role_group_configs, - config: validated_role_config(HdfsNodeRole::Journal), - }, + namenode_config: validated_role_config(HdfsNodeRole::Name), + namenode_role_group_configs, + datanode_config: validated_role_config(HdfsNodeRole::Data), + datanode_role_group_configs, + journalnode_config: validated_role_config(HdfsNodeRole::Journal), + journalnode_role_group_configs, namenode_listeners, discovery_config_map, status, - )) + }) } /// Validates every role group of a role into a map keyed by role group name. diff --git a/rust/operator-binary/src/event.rs b/rust/operator-binary/src/event.rs index dd83a375..0948f571 100644 --- a/rust/operator-binary/src/event.rs +++ b/rust/operator-binary/src/event.rs @@ -48,9 +48,9 @@ pub fn build_invalid_replica_message( role: &HdfsNodeRole, ) -> Option { let replicas = match role { - HdfsNodeRole::Name => total_replicas(&validated_cluster.namenode.role_groups), - HdfsNodeRole::Data => total_replicas(&validated_cluster.datanode.role_groups), - HdfsNodeRole::Journal => total_replicas(&validated_cluster.journalnode.role_groups), + HdfsNodeRole::Name => total_replicas(&validated_cluster.namenode_role_group_configs), + HdfsNodeRole::Data => total_replicas(&validated_cluster.datanode_role_group_configs), + HdfsNodeRole::Journal => total_replicas(&validated_cluster.journalnode_role_group_configs), }; let dfs_replication = validated_cluster.cluster_config.dfs_replication; diff --git a/rust/operator-binary/src/test_support.rs b/rust/operator-binary/src/test_support.rs index fd72c4ae..ef8c6e1f 100644 --- a/rust/operator-binary/src/test_support.rs +++ b/rust/operator-binary/src/test_support.rs @@ -3,7 +3,10 @@ use std::str::FromStr; use stackable_operator::v2::types::operator::RoleGroupName; use crate::{ - controller::{HdfsRoleGroupConfig, ValidatedCluster, validate}, + controller::{ + DataNodeRoleGroupConfig, JournalNodeRoleGroupConfig, NameNodeRoleGroupConfig, + ValidatedCluster, validate, + }, crd::{ CommonNodeConfig, DataNodeConfig, HdfsNodeRole, JournalNodeConfig, NameNodeConfig, v1alpha1, }, @@ -51,10 +54,9 @@ pub fn role_group_name(name: &str) -> RoleGroupName { pub fn namenode_role_group_config<'a>( validated_cluster: &'a ValidatedCluster, role_group_name: &RoleGroupName, -) -> &'a HdfsRoleGroupConfig { +) -> &'a NameNodeRoleGroupConfig { validated_cluster - .namenode - .role_groups + .namenode_role_group_configs .get(role_group_name) .expect("namenode role group should be defined") } @@ -62,10 +64,9 @@ pub fn namenode_role_group_config<'a>( pub fn datanode_role_group_config<'a>( validated_cluster: &'a ValidatedCluster, role_group_name: &RoleGroupName, -) -> &'a HdfsRoleGroupConfig { +) -> &'a DataNodeRoleGroupConfig { validated_cluster - .datanode - .role_groups + .datanode_role_group_configs .get(role_group_name) .expect("datanode role group should be defined") } @@ -73,10 +74,9 @@ pub fn datanode_role_group_config<'a>( pub fn journalnode_role_group_config<'a>( validated_cluster: &'a ValidatedCluster, role_group_name: &RoleGroupName, -) -> &'a HdfsRoleGroupConfig { +) -> &'a JournalNodeRoleGroupConfig { validated_cluster - .journalnode - .role_groups + .journalnode_role_group_configs .get(role_group_name) .expect("journalnode role group should be defined") } From 83b01db7fd8380d954c34ae5a02fde5f59dc3464 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Mon, 14 Sep 2026 18:05:57 +0200 Subject: [PATCH 12/19] tighten/correct doc comments --- rust/operator-binary/src/controller/build/mod.rs | 2 +- rust/operator-binary/src/controller/mod.rs | 6 +++--- rust/operator-binary/src/controller/validate.rs | 6 +++--- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/rust/operator-binary/src/controller/build/mod.rs b/rust/operator-binary/src/controller/build/mod.rs index 9ff7c595..2e2d60d6 100644 --- a/rust/operator-binary/src/controller/build/mod.rs +++ b/rust/operator-binary/src/controller/build/mod.rs @@ -123,7 +123,7 @@ pub struct RoleGroupLogging { pub hdfs: ContainerLogConfig, /// The Vector sidecar; `None` when the Vector agent is disabled for this role group. pub vector: Option, - /// The containers only one role has. + /// The containers unique to this role group's own role; the variant identifies that role. pub role: RoleContainerLogging, } diff --git a/rust/operator-binary/src/controller/mod.rs b/rust/operator-binary/src/controller/mod.rs index 68f5a444..d3de284c 100644 --- a/rust/operator-binary/src/controller/mod.rs +++ b/rust/operator-binary/src/controller/mod.rs @@ -104,17 +104,17 @@ pub struct ValidatedCluster { pub product_version: ProductVersion, pub image: ResolvedProductImage, pub cluster_config: ValidatedClusterConfig, - /// The namenode role-level config (currently the PDB), or `None` if the role is absent. + /// The namenode role-level config, or `None` if the role is absent. pub namenode_config: Option, /// The validated config of every namenode role group, keyed by role group name; empty if the /// role is absent. pub namenode_role_group_configs: BTreeMap, - /// The datanode role-level config (currently the PDB), or `None` if the role is absent. + /// The datanode role-level config, or `None` if the role is absent. pub datanode_config: Option, /// The validated config of every datanode role group, keyed by role group name; empty if the /// role is absent. pub datanode_role_group_configs: BTreeMap, - /// The journalnode role-level config (currently the PDB), or `None` if the role is absent. + /// The journalnode role-level config, or `None` if the role is absent. pub journalnode_config: Option, /// The validated config of every journalnode role group, keyed by role group name; empty if /// the role is absent. diff --git a/rust/operator-binary/src/controller/validate.rs b/rust/operator-binary/src/controller/validate.rs index 9fafb430..20c69098 100644 --- a/rust/operator-binary/src/controller/validate.rs +++ b/rust/operator-binary/src/controller/validate.rs @@ -129,9 +129,9 @@ pub fn validate_cluster( .and_then(|status| status.upgrade_target_product_version.clone()), }; - // Built as a struct literal rather than through a constructor: the three role-level configs - // are the same type, so as positional arguments two of them could be swapped silently, giving - // a role another role's PodDisruptionBudget. + // The three role-level configs share one type, so each is named at the point it is set: as + // positional arguments two of them could be swapped silently, giving a role another role's + // PodDisruptionBudget. Ok(ValidatedCluster { metadata: ValidatedCluster::object_meta(&cluster_name, &namespace, &uid), product_version: ValidatedCluster::product_version(&image), From 4d8100d3522b93a9bc4533fca3443f1d828932c1 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Tue, 15 Sep 2026 12:32:54 +0200 Subject: [PATCH 13/19] move the role-specific log configs into RoleSpecificValues --- .../src/controller/build/container.rs | 133 +++---- .../src/controller/build/mod.rs | 365 +----------------- .../controller/build/properties/hdfs_site.rs | 31 +- .../build/properties/product_logging/mod.rs | 94 +++-- .../src/controller/build/resolve.rs | 290 ++++++++++++++ .../controller/build/resource/config_map.rs | 21 +- .../controller/build/resource/statefulset.rs | 13 +- .../src/controller/validate.rs | 40 +- rust/operator-binary/src/crd/mod.rs | 6 +- rust/operator-binary/src/event.rs | 45 ++- 10 files changed, 551 insertions(+), 487 deletions(-) create mode 100644 rust/operator-binary/src/controller/build/resolve.rs diff --git a/rust/operator-binary/src/controller/build/container.rs b/rust/operator-binary/src/controller/build/container.rs index a1b300ba..d6605b91 100644 --- a/rust/operator-binary/src/controller/build/container.rs +++ b/rust/operator-binary/src/controller/build/container.rs @@ -67,7 +67,7 @@ use crate::{ controller::{ ValidatedCluster, build::{ - self, ResolvedRoleGroup, RoleGroupLogging, RoleGroupResolver, + self, ResolvedRoleGroup, RoleGroupResolver, RoleSpecificValues, jvm::{self, construct_global_jvm_args, construct_role_specific_jvm_args}, kerberos::KERBEROS_CONTAINER_PATH, properties::product_logging::{ @@ -214,15 +214,17 @@ impl ContainerConfig { /// Add all main, side and init containers as well as required volumes to the pod builder. /// /// Every role-specific value is resolved by the caller into `resolved`. The role comes from - /// the role group's config type, so it cannot disagree with `resolved`: pairing a role with - /// another role's resolved values would silently drop the containers' `log4j.properties`. + /// `C::ROLE`, and `resolved` is [`ResolvedRoleGroup`](ResolvedRoleGroup), produced by that + /// same `C`'s [`RoleGroupResolver::resolve`], so it cannot disagree with `resolved`: pairing a + /// role with another role's resolved values would silently drop the containers' + /// `log4j.properties`. pub fn add_containers_and_volumes( pb: &mut PodBuilder, cluster: &ValidatedCluster, cluster_info: &KubernetesClusterInfo, role_group_name: &RoleGroupName, rolegroup_config: &RoleGroupConfig, - resolved: &ResolvedRoleGroup, + resolved: &ResolvedRoleGroup, ) -> Result<(), Error> { let role = &C::ROLE; let namenode_podrefs = build::pod_refs(cluster, &HdfsNodeRole::Name); @@ -233,7 +235,7 @@ impl ContainerConfig { let object_name = resource_names.qualified_role_group_name().to_string(); pb.add_volumes(main_container_config.volumes( - &resolved.logging, + &resolved.logging.hdfs, resolved.role.listener_volume(), &object_name, )) @@ -241,7 +243,7 @@ impl ContainerConfig { pb.add_container(main_container_config.main_container( cluster, cluster_info, - role, + &resolved.logging.hdfs, rolegroup_config, resolved, )?); @@ -339,21 +341,23 @@ impl ContainerConfig { .context(AddVolumeSnafu)?; } - // role specific pod settings configured here - match role { - HdfsNodeRole::Name => { + // The role-specific containers and their log configs come from one enum, so a container + // is never built without the log config that belongs to it. + match &resolved.role { + RoleSpecificValues::Journal => {} + RoleSpecificValues::Name { + zkfc, + format_namenodes, + format_zookeeper, + } => { // Zookeeper fail over container let zkfc_container_config = Self::Zkfc; - pb.add_volumes(zkfc_container_config.volumes( - &resolved.logging, - None, - &object_name, - )) - .context(AddVolumeSnafu)?; + pb.add_volumes(zkfc_container_config.volumes(zkfc, None, &object_name)) + .context(AddVolumeSnafu)?; pb.add_container(zkfc_container_config.main_container( cluster, cluster_info, - role, + zkfc, rolegroup_config, resolved, )?); @@ -361,7 +365,7 @@ impl ContainerConfig { // Format namenode init container let format_namenodes_container_config = Self::FormatNameNodes; pb.add_volumes(format_namenodes_container_config.volumes( - &resolved.logging, + format_namenodes, None, &object_name, )) @@ -369,7 +373,7 @@ impl ContainerConfig { pb.add_init_container(format_namenodes_container_config.init_container( cluster, cluster_info, - role, + format_namenodes, rolegroup_config, resolved, &namenode_podrefs, @@ -378,7 +382,7 @@ impl ContainerConfig { // Format ZooKeeper init container let format_zookeeper_container_config = Self::FormatZooKeeper; pb.add_volumes(format_zookeeper_container_config.volumes( - &resolved.logging, + format_zookeeper, None, &object_name, )) @@ -386,17 +390,19 @@ impl ContainerConfig { pb.add_init_container(format_zookeeper_container_config.init_container( cluster, cluster_info, - role, + format_zookeeper, rolegroup_config, resolved, &namenode_podrefs, )?); } - HdfsNodeRole::Data => { + RoleSpecificValues::Data { + wait_for_namenodes, .. + } => { // Wait for namenode init container let wait_for_namenodes_container_config = Self::WaitForNameNodes; pb.add_volumes(wait_for_namenodes_container_config.volumes( - &resolved.logging, + wait_for_namenodes, None, &object_name, )) @@ -404,13 +410,12 @@ impl ContainerConfig { pb.add_init_container(wait_for_namenodes_container_config.init_container( cluster, cluster_info, - role, + wait_for_namenodes, rolegroup_config, resolved, &namenode_podrefs, )?); } - HdfsNodeRole::Journal => {} } Ok(()) @@ -489,21 +494,22 @@ impl ContainerConfig { /// - Namenode ZooKeeper fail over controller (ZKFC) /// - Datanode main process /// - Journalnode main process - fn main_container( + fn main_container( &self, cluster: &ValidatedCluster, cluster_info: &KubernetesClusterInfo, - role: &HdfsNodeRole, + container_log_config: &ContainerLogConfig, rolegroup_config: &RoleGroupConfig, - resolved: &ResolvedRoleGroup, + resolved: &ResolvedRoleGroup, ) -> Result { + let role = &C::ROLE; let mut cb = new_container_builder(self.container_name()); let resources = self.resources(&resolved.resources); cb.image_from_product_image(&cluster.image) .command(Self::command()) - .args(self.args(cluster, cluster_info, role, &resolved.logging, &[])?) + .args(self.args(cluster, cluster_info, role, container_log_config, &[])?) .add_env_vars(self.env(cluster, role, rolegroup_config, resources.as_ref())?) .add_volume_mounts(self.volume_mounts(cluster, &resolved.volume_claim_templates)) .context(AddVolumeMountSnafu)? @@ -536,15 +542,16 @@ impl ContainerConfig { /// Creates respective init containers for: /// - Namenode (format-namenodes, format-zookeeper) /// - Datanode (wait-for-namenodes) - fn init_container( + fn init_container( &self, cluster: &ValidatedCluster, cluster_info: &KubernetesClusterInfo, - role: &HdfsNodeRole, + container_log_config: &ContainerLogConfig, rolegroup_config: &RoleGroupConfig, - resolved: &ResolvedRoleGroup, + resolved: &ResolvedRoleGroup, namenode_podrefs: &[HdfsPodRef], ) -> Result { + let role = &C::ROLE; let mut cb = new_container_builder(self.container_name()); cb.image_from_product_image(&cluster.image) @@ -553,7 +560,7 @@ impl ContainerConfig { cluster, cluster_info, role, - &resolved.logging, + container_log_config, namenode_podrefs, )?) .add_env_vars(self.env(cluster, role, rolegroup_config, None)?) @@ -634,7 +641,7 @@ impl ContainerConfig { cluster: &ValidatedCluster, cluster_info: &KubernetesClusterInfo, role: &HdfsNodeRole, - logging: &RoleGroupLogging, + container_log_config: &ContainerLogConfig, namenode_podrefs: &[HdfsPodRef], ) -> Result, Error> { let mut args = String::new(); @@ -657,7 +664,7 @@ impl ContainerConfig { match self { ContainerConfig::Hdfs { role, .. } => { args.push_str( - &self.copy_log4j_properties_cmd(HDFS_LOG4J_CONFIG_FILE, &logging.hdfs), + &self.copy_log4j_properties_cmd(HDFS_LOG4J_CONFIG_FILE, container_log_config), ); args.push_str(&formatdoc!( @@ -685,14 +692,9 @@ impl ContainerConfig { )); } ContainerConfig::Zkfc => { - if let Some(container_log_config) = logging.role.zkfc() { - args.push_str( - &self.copy_log4j_properties_cmd( - ZKFC_LOG4J_CONFIG_FILE, - container_log_config, - ), - ); - } + args.push_str( + &self.copy_log4j_properties_cmd(ZKFC_LOG4J_CONFIG_FILE, container_log_config), + ); args.push_str(&format!( "{hadoop_home}/bin/hdfs zkfc\n", hadoop_home = Self::HADOOP_HOME @@ -701,12 +703,10 @@ impl ContainerConfig { ContainerConfig::FormatNameNodes => { args.push_str(&bash_capture_shell_helper(self.container_name().as_ref())); - if let Some(container_log_config) = logging.role.format_namenodes() { - args.push_str(&self.copy_log4j_properties_cmd( - FORMAT_NAMENODES_LOG4J_CONFIG_FILE, - container_log_config, - )); - } + args.push_str(&self.copy_log4j_properties_cmd( + FORMAT_NAMENODES_LOG4J_CONFIG_FILE, + container_log_config, + )); // First step we check for active namenodes. This step should return an active namenode // for e.g. scaling. It may fail if the active namenode is restarted and the standby // namenode takes over. @@ -774,12 +774,10 @@ impl ContainerConfig { ContainerConfig::FormatZooKeeper => { args.push_str(&bash_capture_shell_helper(self.container_name().as_ref())); - if let Some(container_log_config) = logging.role.format_zookeeper() { - args.push_str(&self.copy_log4j_properties_cmd( - FORMAT_ZOOKEEPER_LOG4J_CONFIG_FILE, - container_log_config, - )); - } + args.push_str(&self.copy_log4j_properties_cmd( + FORMAT_ZOOKEEPER_LOG4J_CONFIG_FILE, + container_log_config, + )); args.push_str(&formatdoc!( r###" echo "Attempt to format ZooKeeper ZNode for $POD_NAME ..." @@ -803,12 +801,10 @@ impl ContainerConfig { ContainerConfig::WaitForNameNodes => { args.push_str(&bash_capture_shell_helper(self.container_name().as_ref())); - if let Some(container_log_config) = logging.role.wait_for_namenodes() { - args.push_str(&self.copy_log4j_properties_cmd( - WAIT_FOR_NAMENODES_LOG4J_CONFIG_FILE, - container_log_config, - )); - } + args.push_str(&self.copy_log4j_properties_cmd( + WAIT_FOR_NAMENODES_LOG4J_CONFIG_FILE, + container_log_config, + )); if cluster.has_kerberos_enabled() { args.push_str(&Self::get_kerberos_ticket(cluster, role, cluster_info)?); } @@ -1085,11 +1081,13 @@ impl ContainerConfig { /// Return the container volumes. /// - /// `listener_volume` is the role group's ephemeral listener volume, which only the main - /// container of the datanodes has. + /// `container_log_config` is this container's own log config, chosen by the caller from the + /// role group's `RoleGroupLogging` or [`RoleSpecificValues`]; a container is never built + /// without one. `listener_volume` is the role group's ephemeral listener volume, which only + /// the main container of the datanodes has. fn volumes( &self, - logging: &RoleGroupLogging, + container_log_config: &ContainerLogConfig, listener_volume: Option<&Volume>, object_name: &str, ) -> Vec { @@ -1116,16 +1114,9 @@ impl ContainerConfig { ); } - let container_log_config = match self { - ContainerConfig::Hdfs { .. } => Some(&logging.hdfs), - ContainerConfig::Zkfc => logging.role.zkfc(), - ContainerConfig::FormatNameNodes => logging.role.format_namenodes(), - ContainerConfig::FormatZooKeeper => logging.role.format_zookeeper(), - ContainerConfig::WaitForNameNodes => logging.role.wait_for_namenodes(), - }; let volume_mount_dirs = self.volume_mount_dirs(); volumes.extend(Self::common_container_volumes( - container_log_config, + Some(container_log_config), object_name, volume_mount_dirs.config_mount_name(), volume_mount_dirs.log_mount_name(), diff --git a/rust/operator-binary/src/controller/build/mod.rs b/rust/operator-binary/src/controller/build/mod.rs index 2e2d60d6..bd2e02b3 100644 --- a/rust/operator-binary/src/controller/build/mod.rs +++ b/rust/operator-binary/src/controller/build/mod.rs @@ -1,6 +1,5 @@ use std::{ collections::{BTreeMap, HashMap}, - fmt::Display, marker::PhantomData, }; @@ -9,11 +8,10 @@ use stackable_operator::{ builder::meta::ObjectMetaBuilder, k8s_openapi::api::{ apps::v1::StatefulSet, - core::v1::{ConfigMap, PersistentVolumeClaim, ResourceRequirements, Service, Volume}, + core::v1::{ConfigMap, Service}, policy::v1::PodDisruptionBudget, }, kvp::{LabelError, Labels}, - product_logging::spec::{ContainerLogConfig, Logging}, utils::cluster_info::KubernetesClusterInfo, v2::{ builder::meta::ownerreference_from_resource, @@ -30,14 +28,10 @@ use crate::{ controller::{ CONTROLLER_NAME, KubernetesResources, OPERATOR_NAME, PRODUCT_NAME, Prepared, ValidatedCluster, - build::{ - container::ContainerConfig, - resource::rbac::{build_role_binding, build_service_account}, - }, + build::resource::rbac::{build_role_binding, build_service_account}, }, crd::{ - CommonNodeConfig, DataNodeConfig, DataNodeContainer, HdfsNodeRole, HdfsPodRef, - JournalNodeConfig, JournalNodeContainer, NameNodeConfig, NameNodeContainer, + HdfsNodeRole, HdfsPodRef, constants::{ DEFAULT_DATA_NODE_DATA_PORT, DEFAULT_DATA_NODE_HTTP_PORT, DEFAULT_DATA_NODE_HTTPS_PORT, DEFAULT_DATA_NODE_IPC_PORT, DEFAULT_DATA_NODE_METRICS_PORT, @@ -53,7 +47,6 @@ use crate::{ SERVICE_PORT_NAME_IPC, SERVICE_PORT_NAME_JMX_METRICS, SERVICE_PORT_NAME_METRICS, SERVICE_PORT_NAME_RPC, }, - storage::DataNodeStorageConfigInnerType, v1alpha1, }, }; @@ -64,6 +57,7 @@ pub mod jvm; pub mod kerberos; pub mod opa; pub mod properties; +pub mod resolve; pub mod resource; #[derive(Snafu, Debug)] @@ -114,326 +108,8 @@ pub enum Error { }, } -/// The log configuration of every container in one role group, resolved during the build step -/// by code that knows the role, so the shared builders never see a role-specific -/// `Logging`. -#[derive(Debug)] -pub struct RoleGroupLogging { - /// The main `hdfs` container, which every role has. - pub hdfs: ContainerLogConfig, - /// The Vector sidecar; `None` when the Vector agent is disabled for this role group. - pub vector: Option, - /// The containers unique to this role group's own role; the variant identifies that role. - pub role: RoleContainerLogging, -} - -/// The log configuration of the side and init containers that only one role runs. -/// -/// These live in an enum rather than in `Option` fields so that "the `zkfc` log config exists -/// exactly when this is a namenode" is checked by the compiler at every construction site. A -/// missing log config is otherwise silent: the container's `log4j.properties` is left out of both -/// the `ConfigMap` and the `cp` in the container args, so it logs with Hadoop's built-in defaults -/// and Vector collects nothing for it. -#[derive(Debug)] -pub enum RoleContainerLogging { - /// Journalnodes run no role-specific container. - Journal, - /// The namenode `zkfc` side container and its two init containers. - Name { - zkfc: ContainerLogConfig, - format_namenodes: ContainerLogConfig, - format_zookeeper: ContainerLogConfig, - }, - /// The datanode `wait-for-namenodes` init container. - Data { - wait_for_namenodes: ContainerLogConfig, - }, -} - -impl RoleContainerLogging { - /// The namenode `zkfc` side container's log config; `None` for the other roles. - pub fn zkfc(&self) -> Option<&ContainerLogConfig> { - match self { - Self::Name { zkfc, .. } => Some(zkfc), - Self::Journal | Self::Data { .. } => None, - } - } - - /// The namenode `format-namenodes` init container's log config; `None` for the other roles. - pub fn format_namenodes(&self) -> Option<&ContainerLogConfig> { - match self { - Self::Name { - format_namenodes, .. - } => Some(format_namenodes), - Self::Journal | Self::Data { .. } => None, - } - } - - /// The namenode `format-zookeeper` init container's log config; `None` for the other roles. - pub fn format_zookeeper(&self) -> Option<&ContainerLogConfig> { - match self { - Self::Name { - format_zookeeper, .. - } => Some(format_zookeeper), - Self::Journal | Self::Data { .. } => None, - } - } - - /// The datanode `wait-for-namenodes` init container's log config; `None` for the other roles. - pub fn wait_for_namenodes(&self) -> Option<&ContainerLogConfig> { - match self { - Self::Data { wait_for_namenodes } => Some(wait_for_namenodes), - Self::Journal | Self::Name { .. } => None, - } - } -} - -/// Everything about one role group that the shared builders below cannot derive themselves: the -/// values resolved from its role-specific config, plus the selector labels, which the build loop -/// already needs for the listener volume and the PVC templates. -/// -/// Resolving these in the build loop, which knows the role, is what lets the builders be generic -/// over the role group's config type. -pub struct ResolvedRoleGroup { - /// The selector labels of the role group's pods, also used as the `StatefulSet` selector and - /// on its listener volume. - /// - /// We must use the selector labels and not the recommended labels for the listener volumes. - /// This is because the recommended set contains a "managed-by" label. That label triggers the - /// cluster resources to "manage" listeners, which is wrong and leads to errors. The listeners - /// are managed by the listener-operator. - pub selector_labels: Labels, - /// The role group's merged config that is common to every role. - pub common: CommonNodeConfig, - /// The resource requirements of the role group's main and init containers; the ZKFC sidecar - /// has fixed requirements of its own and ignores this. - pub resources: ResourceRequirements, - /// The `StatefulSet`'s persistent volume claim templates. - pub volume_claim_templates: Vec, - /// The values that exist for one role only. - pub role: RoleSpecificResources, - /// The log config of each of the role group's containers. - pub logging: RoleGroupLogging, -} - -/// The role group values that exist for one role only. -/// -/// These live in an enum rather than in `Option` fields so the compiler checks the pairing at -/// every construction site: a datanode without its storage configuration does not compile — that -/// would silently drop `dfs.datanode.data.dir` and send the datanodes' blocks to container-local -/// storage — and neither does a namenode with a pod-level listener volume, which would collide -/// with the identically named volume claim template and be rejected at apply time. -pub enum RoleSpecificResources { - /// Journalnodes have no listener and no role-specific storage configuration. - Journal, - /// Namenodes get their listener from a volume claim template in `volume_claim_templates`, for - /// stable per-pod identity, so they have no pod-level listener volume. - Name, - /// Datanodes need no stable per-pod identity, so their listener is an ephemeral pod volume. - /// They are also the only role that configures `dfs.datanode.data.dir`. - Data { - listener_volume: Volume, - storage: DataNodeStorageConfigInnerType, - }, -} - -impl RoleSpecificResources { - /// The role these values belong to. - pub fn node_role(&self) -> HdfsNodeRole { - match self { - Self::Journal => HdfsNodeRole::Journal, - Self::Name => HdfsNodeRole::Name, - Self::Data { .. } => HdfsNodeRole::Data, - } - } - - /// The role group's ephemeral listener volume; only datanodes have one. - pub fn listener_volume(&self) -> Option<&Volume> { - match self { - Self::Data { - listener_volume, .. - } => Some(listener_volume), - Self::Journal | Self::Name => None, - } - } - - /// The datanode data volume configuration, which drives `dfs.datanode.data.dir`; `None` for - /// the other roles. - pub fn datanode_storage(&self) -> Option<&DataNodeStorageConfigInnerType> { - match self { - Self::Data { storage, .. } => Some(storage), - Self::Journal | Self::Name => None, - } - } -} - -/// The log config of the two containers every role has: the main `hdfs` container, and the Vector -/// sidecar, which is `None` when the Vector agent is disabled for the role group. -/// -/// Each role names these containers with its own enum, so this is generic over that enum rather -/// than repeated once per role. -fn common_container_logging( - logging: &Logging, - hdfs: T, - vector: T, -) -> (ContainerLogConfig, Option) -where - T: Clone + Display + Ord, -{ - ( - logging.for_container(&hdfs).into_owned(), - logging - .enable_vector_agent - .then(|| logging.for_container(&vector).into_owned()), - ) -} - -/// How to resolve one role group's role-specific values, implemented once per role config type. -/// -/// This is what lets [`build_role`] be written once: the trait supplies the role and the single -/// role-dependent step, and everything else about building a role group is identical across the -/// three roles. -/// -/// [`Self::ROLE`] is also the single source of truth for the role in the shared builders: they -/// take the role group's config type and read the role from it, rather than taking the role as a -/// second parameter that a caller could pair with the wrong config. -pub(crate) trait RoleGroupResolver { - /// The role whose config this is. - const ROLE: HdfsNodeRole; - - /// Resolves everything the shared builders cannot derive themselves. Takes the selector - /// labels because two of the three roles need them to build their listener. - fn resolve( - &self, - role_group_name: &RoleGroupName, - selector_labels: Labels, - ) -> Result; -} - -impl RoleGroupResolver for JournalNodeConfig { - const ROLE: HdfsNodeRole = HdfsNodeRole::Journal; - - fn resolve( - &self, - _role_group_name: &RoleGroupName, - selector_labels: Labels, - ) -> Result { - let (hdfs, vector) = common_container_logging( - &self.logging, - JournalNodeContainer::Hdfs, - JournalNodeContainer::Vector, - ); - - Ok(ResolvedRoleGroup { - selector_labels, - common: self.common.clone(), - resources: self.resources.clone().into(), - volume_claim_templates: ContainerConfig::journalnode_volume_claim_templates(self), - role: RoleSpecificResources::Journal, - logging: RoleGroupLogging { - hdfs, - vector, - role: RoleContainerLogging::Journal, - }, - }) - } -} - -impl RoleGroupResolver for NameNodeConfig { - const ROLE: HdfsNodeRole = HdfsNodeRole::Name; - - fn resolve( - &self, - role_group_name: &RoleGroupName, - selector_labels: Labels, - ) -> Result { - // Namenodes get their listener from a persistent volume claim template, for stable - // per-pod identity, rather than from an ephemeral volume. - let volume_claim_templates = - ContainerConfig::namenode_volume_claim_templates(self, &selector_labels).context( - VolumeClaimTemplatesSnafu { - role: Self::ROLE, - role_group: role_group_name.clone(), - }, - )?; - - let (hdfs, vector) = common_container_logging( - &self.logging, - NameNodeContainer::Hdfs, - NameNodeContainer::Vector, - ); - - Ok(ResolvedRoleGroup { - selector_labels, - common: self.common.clone(), - resources: self.resources.clone().into(), - volume_claim_templates, - role: RoleSpecificResources::Name, - logging: RoleGroupLogging { - hdfs, - vector, - role: RoleContainerLogging::Name { - zkfc: self - .logging - .for_container(&NameNodeContainer::Zkfc) - .into_owned(), - format_namenodes: self - .logging - .for_container(&NameNodeContainer::FormatNameNodes) - .into_owned(), - format_zookeeper: self - .logging - .for_container(&NameNodeContainer::FormatZooKeeper) - .into_owned(), - }, - }, - }) - } -} - -impl RoleGroupResolver for DataNodeConfig { - const ROLE: HdfsNodeRole = HdfsNodeRole::Data; - - fn resolve( - &self, - role_group_name: &RoleGroupName, - selector_labels: Labels, - ) -> Result { - // Datanodes use an ephemeral listener volume, since they need no stable per-pod identity. - let listener_volume = ContainerConfig::datanode_listener_volume(self, &selector_labels) - .context(ListenerVolumeSnafu { - role: Self::ROLE, - role_group: role_group_name.clone(), - })?; - - let (hdfs, vector) = common_container_logging( - &self.logging, - DataNodeContainer::Hdfs, - DataNodeContainer::Vector, - ); - - Ok(ResolvedRoleGroup { - selector_labels, - common: self.common.clone(), - resources: self.resources.clone().into(), - volume_claim_templates: ContainerConfig::datanode_volume_claim_templates(self), - role: RoleSpecificResources::Data { - listener_volume, - storage: self.resources.storage.clone(), - }, - logging: RoleGroupLogging { - hdfs, - vector, - role: RoleContainerLogging::Data { - wait_for_namenodes: self - .logging - .for_container(&DataNodeContainer::WaitForNameNodes) - .into_owned(), - }, - }, - }) - } -} +pub(crate) use resolve::RoleGroupResolver; +pub use resolve::{ResolvedRoleGroup, RoleGroupLogging, RoleSpecificValues}; /// The resources built for the role groups of one role, accumulated across the roles by /// [`build`]. @@ -491,7 +167,7 @@ fn build_role( cluster_info, role_group_name, rg_config, - resolved, + &resolved, ) .context(StatefulSetSnafu { role: *role, @@ -532,7 +208,8 @@ pub fn build( // journalnodes first, then namenodes, then datanodes, each role gated on the previous one // (see [`crate::controller::apply::Applier::apply`]). That order comes from the // `HdfsNodeRole` key of `RoleGroupResources::stateful_sets`, not from the order of the calls - // below, which are free to be rearranged. + // below. The other three collections are plain `Vec`s appended in call order, which the apply + // step does not depend on: it applies each of them in bulk. build_role( cluster, cluster_info, @@ -614,13 +291,13 @@ fn build_role_group_services( pub(crate) const DEFAULT_REPLICAS: u16 = 1; /// The replica count of every role group in the map, defaulting to [`DEFAULT_REPLICAS`] where it -/// is unset. -fn role_group_replicas( - role_group_configs: &BTreeMap< +/// is unset. The single place that applies that default. +fn role_group_replicas<'a, C>( + role_group_configs: &'a BTreeMap< RoleGroupName, RoleGroupConfig, >, -) -> Vec<(&RoleGroupName, u16)> { +) -> impl Iterator + 'a { role_group_configs .iter() .map(|(role_group_name, role_group)| { @@ -629,7 +306,6 @@ fn role_group_replicas( role_group.replicas.unwrap_or(DEFAULT_REPLICAS), ) }) - .collect() } /// The total number of replicas across the role groups of one role, counting a role group without @@ -640,9 +316,8 @@ pub(crate) fn total_replicas( RoleGroupConfig, >, ) -> u16 { - role_group_configs - .values() - .map(|role_group| role_group.replicas.unwrap_or(DEFAULT_REPLICAS)) + role_group_replicas(role_group_configs) + .map(|(_, replicas)| replicas) .sum() } @@ -660,10 +335,12 @@ pub(crate) fn pod_refs(cluster: &ValidatedCluster, role: &HdfsNodeRole) -> Vec role_group_replicas(&cluster.namenode_role_group_configs), - HdfsNodeRole::Data => role_group_replicas(&cluster.datanode_role_group_configs), - HdfsNodeRole::Journal => role_group_replicas(&cluster.journalnode_role_group_configs), + let replicas_per_role_group: Vec<(&RoleGroupName, u16)> = match role { + HdfsNodeRole::Name => role_group_replicas(&cluster.namenode_role_group_configs).collect(), + HdfsNodeRole::Data => role_group_replicas(&cluster.datanode_role_group_configs).collect(), + HdfsNodeRole::Journal => { + role_group_replicas(&cluster.journalnode_role_group_configs).collect() + } }; replicas_per_role_group diff --git a/rust/operator-binary/src/controller/build/properties/hdfs_site.rs b/rust/operator-binary/src/controller/build/properties/hdfs_site.rs index e088e591..c121a115 100644 --- a/rust/operator-binary/src/controller/build/properties/hdfs_site.rs +++ b/rust/operator-binary/src/controller/build/properties/hdfs_site.rs @@ -331,7 +331,10 @@ mod tests { use indoc::indoc; use super::*; - use crate::controller::build::properties::test_support::{cluster_info, validated_cluster}; + use crate::{ + controller::build::properties::test_support::{cluster_info, validated_cluster}, + test_support::{datanode_config, role_group_name}, + }; #[test] fn renders_operator_defaults() { @@ -372,4 +375,30 @@ mod tests { "{xml}" ); } + + /// With a datanode's storage config, `dfs.datanode.data.dir` names one directory per PVC, + /// tagged with its HDFS storage type. Losing this property is silent: the datanodes fall back + /// to Hadoop's default directory, which is container-local, so their blocks are gone on the + /// next restart. + #[test] + fn datanode_storage_renders_the_data_dir() { + let validated_cluster = validated_cluster(); + let storage = datanode_config(&validated_cluster, &role_group_name("default")) + .resources + .storage + .clone(); + + let xml = build( + &validated_cluster, + &cluster_info(), + Some(storage), + KeyValueConfigOverrides::default(), + ); + + assert!( + xml.contains("dfs.datanode.data.dir") + && xml.contains("[DISK]/stackable/data/data/datanode"), + "rendered hdfs-site.xml:\n{xml}" + ); + } } diff --git a/rust/operator-binary/src/controller/build/properties/product_logging/mod.rs b/rust/operator-binary/src/controller/build/properties/product_logging/mod.rs index 9abbb3d9..f59e9b4f 100644 --- a/rust/operator-binary/src/controller/build/properties/product_logging/mod.rs +++ b/rust/operator-binary/src/controller/build/properties/product_logging/mod.rs @@ -11,7 +11,7 @@ use stackable_operator::{ }; use crate::controller::build::{ - RoleGroupLogging, + RoleGroupLogging, RoleSpecificValues, container::{ FORMAT_NAMENODES_CONTAINER_NAME, FORMAT_ZOOKEEPER_CONTAINER_NAME, WAIT_FOR_NAMENODES_CONTAINER_NAME, ZKFC_CONTAINER_NAME, @@ -77,64 +77,82 @@ pub fn vector_config_file_content() -> String { /// /// Returns `(filename, rendered content)` pairs; containers using a custom log ConfigMap are /// skipped, so the result is empty when none use automatic logging. -pub fn build_log4j_configs(logging: &RoleGroupLogging) -> Vec<(&'static str, String)> { +pub fn build_log4j_configs( + logging: &RoleGroupLogging, + role: &RoleSpecificValues, +) -> Vec<(&'static str, String)> { let mut configs = Vec::new(); add_log4j_config_if_automatic( &mut configs, - Some(&logging.hdfs), + &logging.hdfs, HDFS_LOG4J_CONFIG_FILE, "hdfs", HDFS_LOG_FILE, MAX_HDFS_LOG_FILE_SIZE, ); - add_log4j_config_if_automatic( - &mut configs, - logging.role.zkfc(), - ZKFC_LOG4J_CONFIG_FILE, - ZKFC_CONTAINER_NAME.as_ref(), - ZKFC_LOG_FILE, - MAX_ZKFC_LOG_FILE_SIZE, - ); - add_log4j_config_if_automatic( - &mut configs, - logging.role.format_namenodes(), - FORMAT_NAMENODES_LOG4J_CONFIG_FILE, - FORMAT_NAMENODES_CONTAINER_NAME.as_ref(), - FORMAT_NAMENODES_LOG_FILE, - MAX_FORMAT_NAMENODE_LOG_FILE_SIZE, - ); - add_log4j_config_if_automatic( - &mut configs, - logging.role.format_zookeeper(), - FORMAT_ZOOKEEPER_LOG4J_CONFIG_FILE, - FORMAT_ZOOKEEPER_CONTAINER_NAME.as_ref(), - FORMAT_ZOOKEEPER_LOG_FILE, - MAX_FORMAT_ZOOKEEPER_LOG_FILE_SIZE, - ); - add_log4j_config_if_automatic( - &mut configs, - logging.role.wait_for_namenodes(), - WAIT_FOR_NAMENODES_LOG4J_CONFIG_FILE, - WAIT_FOR_NAMENODES_CONTAINER_NAME.as_ref(), - WAIT_FOR_NAMENODES_LOG_FILE, - MAX_WAIT_NAMENODES_LOG_FILE_SIZE, - ); + + // Exhaustive, so a role's containers and their log4j configs cannot drift apart. + match role { + RoleSpecificValues::Journal => {} + RoleSpecificValues::Name { + zkfc, + format_namenodes, + format_zookeeper, + } => { + add_log4j_config_if_automatic( + &mut configs, + zkfc, + ZKFC_LOG4J_CONFIG_FILE, + ZKFC_CONTAINER_NAME.as_ref(), + ZKFC_LOG_FILE, + MAX_ZKFC_LOG_FILE_SIZE, + ); + add_log4j_config_if_automatic( + &mut configs, + format_namenodes, + FORMAT_NAMENODES_LOG4J_CONFIG_FILE, + FORMAT_NAMENODES_CONTAINER_NAME.as_ref(), + FORMAT_NAMENODES_LOG_FILE, + MAX_FORMAT_NAMENODE_LOG_FILE_SIZE, + ); + add_log4j_config_if_automatic( + &mut configs, + format_zookeeper, + FORMAT_ZOOKEEPER_LOG4J_CONFIG_FILE, + FORMAT_ZOOKEEPER_CONTAINER_NAME.as_ref(), + FORMAT_ZOOKEEPER_LOG_FILE, + MAX_FORMAT_ZOOKEEPER_LOG_FILE_SIZE, + ); + } + RoleSpecificValues::Data { + wait_for_namenodes, .. + } => { + add_log4j_config_if_automatic( + &mut configs, + wait_for_namenodes, + WAIT_FOR_NAMENODES_LOG4J_CONFIG_FILE, + WAIT_FOR_NAMENODES_CONTAINER_NAME.as_ref(), + WAIT_FOR_NAMENODES_LOG_FILE, + MAX_WAIT_NAMENODES_LOG_FILE_SIZE, + ); + } + } configs } fn add_log4j_config_if_automatic( configs: &mut Vec<(&'static str, String)>, - log_config: Option<&ContainerLogConfig>, + log_config: &ContainerLogConfig, log_config_file: &'static str, log_dir_name: &str, log_file: &str, max_log_file_size: MemoryQuantity, ) { - if let Some(ContainerLogConfig { + if let ContainerLogConfig { choice: Some(ContainerLogConfigChoice::Automatic(log_config)), - }) = log_config + } = log_config { configs.push(( log_config_file, diff --git a/rust/operator-binary/src/controller/build/resolve.rs b/rust/operator-binary/src/controller/build/resolve.rs new file mode 100644 index 00000000..104a3cf9 --- /dev/null +++ b/rust/operator-binary/src/controller/build/resolve.rs @@ -0,0 +1,290 @@ +//! Resolving one role group's role-specific values, once per role. +//! +//! These types live in their own module so that [`ResolvedRoleGroup`]'s private `_config` field is +//! private to *them*: the shared builders in [`super::container`] and [`super::resource`] are +//! siblings of this module rather than descendants, so [`RoleGroupResolver::resolve`] is the only +//! way any of them can obtain a bundle. + +use std::{fmt::Display, marker::PhantomData}; + +use snafu::ResultExt; +use stackable_operator::{ + k8s_openapi::api::core::v1::{PersistentVolumeClaim, ResourceRequirements, Volume}, + kvp::Labels, + product_logging::spec::{ContainerLogConfig, Logging}, + v2::types::operator::RoleGroupName, +}; + +use super::{Error, ListenerVolumeSnafu, VolumeClaimTemplatesSnafu, container::ContainerConfig}; +use crate::crd::{ + CommonNodeConfig, DataNodeConfig, DataNodeContainer, HdfsNodeRole, JournalNodeConfig, + JournalNodeContainer, NameNodeConfig, NameNodeContainer, + storage::DataNodeStorageConfigInnerType, +}; + +/// The log configuration of the two containers every role has, resolved during the build step by +/// code that knows the role, so the shared builders never see a role-specific `Logging`. +/// +/// The containers only one role runs carry their log config in [`RoleSpecificValues`], so that +/// "the `zkfc` log config exists exactly when this is a namenode" is one fact, not two that have +/// to agree. +#[derive(Debug)] +pub struct RoleGroupLogging { + /// The main `hdfs` container, which every role has. + pub hdfs: ContainerLogConfig, + /// The Vector sidecar; `None` when the Vector agent is disabled for this role group. + pub vector: Option, +} + +/// Everything about one role group that the shared builders below cannot derive themselves: the +/// values resolved from its role-specific config, plus the selector labels, which the build loop +/// already needs for the listener volume and the PVC templates. +/// +/// Resolving these in the build loop, which knows the role, is what lets the builders be generic +/// over the role group's config type. +/// +/// `C` is the role group's config type, the same one [`RoleGroupResolver`] is implemented on. +/// Every builder takes `RoleGroupConfig` and `ResolvedRoleGroup` together, so one role's +/// overrides and replica count cannot be paired with another role's resolved values: the two +/// parameters are the same `C` or they do not compile. The private `_config` field makes +/// [`RoleGroupResolver::resolve`] the only constructor outside this module — a struct literal +/// elsewhere is rejected with `E0451` — so a `C` can never disagree with the values filled in +/// beside it. +pub struct ResolvedRoleGroup { + /// The selector labels of the role group's pods, also used as the `StatefulSet` selector and + /// on its listener volume. + /// + /// We must use the selector labels and not the recommended labels for the listener volumes. + /// This is because the recommended set contains a "managed-by" label. That label triggers the + /// cluster resources to "manage" listeners, which is wrong and leads to errors. The listeners + /// are managed by the listener-operator. + pub selector_labels: Labels, + /// The role group's merged config that is common to every role. + pub common: CommonNodeConfig, + /// The resource requirements of the role group's main and init containers; the ZKFC sidecar + /// has fixed requirements of its own and ignores this. + pub resources: ResourceRequirements, + /// The `StatefulSet`'s persistent volume claim templates. + pub volume_claim_templates: Vec, + /// The values that exist for this role only. + pub role: RoleSpecificValues, + /// The log config of each of the role group's containers. + pub logging: RoleGroupLogging, + /// Ties this bundle to the role group's config type; see the struct documentation. The + /// `fn() -> C` spelling marks the relationship without claiming this struct owns a `C`. + _config: PhantomData C>, +} + +/// Everything that exists for one role only: the containers that role runs, their log configs, +/// and its storage and listener arrangements. +/// +/// One enum rather than several `Option` fields, so the compiler checks the pairing at every +/// construction site and every consumer is exhaustive. A datanode without its storage +/// configuration does not compile — that would silently drop `dfs.datanode.data.dir` and send the +/// datanodes' blocks to container-local storage. Neither does a namenode with a pod-level +/// listener volume, which would collide with the identically named volume claim template and be +/// rejected at apply time. And neither does a container without its log config, which would leave +/// `log4j.properties` out of both the `ConfigMap` and the `cp` in the container args, so the +/// container logs with Hadoop's built-in defaults and Vector collects nothing for it. +pub enum RoleSpecificValues { + /// Journalnodes run no role-specific container, have no listener and no role-specific + /// storage configuration. + Journal, + /// Namenodes run the `zkfc` side container and the `format-namenodes` and `format-zookeeper` + /// init containers. They get their listener from a volume claim template in + /// [`ResolvedRoleGroup::volume_claim_templates`], for stable per-pod identity, so they have + /// no pod-level listener volume. + Name { + zkfc: ContainerLogConfig, + format_namenodes: ContainerLogConfig, + format_zookeeper: ContainerLogConfig, + }, + /// Datanodes run the `wait-for-namenodes` init container. They need no stable per-pod + /// identity, so their listener is an ephemeral pod volume, and they are the only role that + /// configures `dfs.datanode.data.dir`. + Data { + listener_volume: Volume, + storage: DataNodeStorageConfigInnerType, + wait_for_namenodes: ContainerLogConfig, + }, +} + +impl RoleSpecificValues { + /// The role group's ephemeral listener volume; only datanodes have one. + pub fn listener_volume(&self) -> Option<&Volume> { + match self { + Self::Data { + listener_volume, .. + } => Some(listener_volume), + Self::Journal | Self::Name { .. } => None, + } + } + + /// The datanode data volume configuration, which drives `dfs.datanode.data.dir`; `None` for + /// the other roles. + pub fn datanode_storage(&self) -> Option<&DataNodeStorageConfigInnerType> { + match self { + Self::Data { storage, .. } => Some(storage), + Self::Journal | Self::Name { .. } => None, + } + } +} + +/// The log config of the two containers every role has: the main `hdfs` container, and the Vector +/// sidecar, which is `None` when the Vector agent is disabled for the role group. +/// +/// Each role names these containers with its own enum, so this is generic over that enum rather +/// than repeated once per role. +fn common_container_logging( + logging: &Logging, + hdfs: T, + vector: T, +) -> (ContainerLogConfig, Option) +where + T: Clone + Display + Ord, +{ + ( + logging.for_container(&hdfs).into_owned(), + logging + .enable_vector_agent + .then(|| logging.for_container(&vector).into_owned()), + ) +} + +/// How to resolve one role group's role-specific values, implemented once per role config type. +/// +/// This is what lets [`build_role`](super::build_role) be written once: the trait supplies the role and the single +/// role-dependent step, and everything else about building a role group is identical across the +/// three roles. +/// +/// [`Self::ROLE`] is the single source of truth for the role in the shared builders: they take the +/// role group's config type and read the role from it, rather than taking the role as a second +/// parameter a caller could pair with the wrong config. See [`ResolvedRoleGroup`] for what the +/// shared `C` guarantees. +pub(crate) trait RoleGroupResolver: Sized { + /// The role whose config this is. + const ROLE: HdfsNodeRole; + + /// Resolves everything the shared builders cannot derive themselves. Takes the selector + /// labels because two of the three roles need them to build their listener. + fn resolve( + &self, + role_group_name: &RoleGroupName, + selector_labels: Labels, + ) -> Result, Error>; +} + +impl RoleGroupResolver for JournalNodeConfig { + const ROLE: HdfsNodeRole = HdfsNodeRole::Journal; + + fn resolve( + &self, + _role_group_name: &RoleGroupName, + selector_labels: Labels, + ) -> Result, Error> { + let (hdfs, vector) = common_container_logging( + &self.logging, + JournalNodeContainer::Hdfs, + JournalNodeContainer::Vector, + ); + + Ok(ResolvedRoleGroup { + selector_labels, + common: self.common.clone(), + resources: self.resources.clone().into(), + volume_claim_templates: ContainerConfig::journalnode_volume_claim_templates(self), + role: RoleSpecificValues::Journal, + logging: RoleGroupLogging { hdfs, vector }, + _config: PhantomData, + }) + } +} + +impl RoleGroupResolver for NameNodeConfig { + const ROLE: HdfsNodeRole = HdfsNodeRole::Name; + + fn resolve( + &self, + role_group_name: &RoleGroupName, + selector_labels: Labels, + ) -> Result, Error> { + // Namenodes get their listener from a persistent volume claim template, for stable + // per-pod identity, rather than from an ephemeral volume. + let volume_claim_templates = + ContainerConfig::namenode_volume_claim_templates(self, &selector_labels).context( + VolumeClaimTemplatesSnafu { + role: Self::ROLE, + role_group: role_group_name.clone(), + }, + )?; + + let (hdfs, vector) = common_container_logging( + &self.logging, + NameNodeContainer::Hdfs, + NameNodeContainer::Vector, + ); + + Ok(ResolvedRoleGroup { + selector_labels, + common: self.common.clone(), + resources: self.resources.clone().into(), + volume_claim_templates, + role: RoleSpecificValues::Name { + zkfc: self + .logging + .for_container(&NameNodeContainer::Zkfc) + .into_owned(), + format_namenodes: self + .logging + .for_container(&NameNodeContainer::FormatNameNodes) + .into_owned(), + format_zookeeper: self + .logging + .for_container(&NameNodeContainer::FormatZooKeeper) + .into_owned(), + }, + logging: RoleGroupLogging { hdfs, vector }, + _config: PhantomData, + }) + } +} + +impl RoleGroupResolver for DataNodeConfig { + const ROLE: HdfsNodeRole = HdfsNodeRole::Data; + + fn resolve( + &self, + role_group_name: &RoleGroupName, + selector_labels: Labels, + ) -> Result, Error> { + // Datanodes use an ephemeral listener volume, since they need no stable per-pod identity. + let listener_volume = ContainerConfig::datanode_listener_volume(self, &selector_labels) + .context(ListenerVolumeSnafu { + role: Self::ROLE, + role_group: role_group_name.clone(), + })?; + + let (hdfs, vector) = common_container_logging( + &self.logging, + DataNodeContainer::Hdfs, + DataNodeContainer::Vector, + ); + + Ok(ResolvedRoleGroup { + selector_labels, + common: self.common.clone(), + resources: self.resources.clone().into(), + volume_claim_templates: ContainerConfig::datanode_volume_claim_templates(self), + role: RoleSpecificValues::Data { + listener_volume, + storage: self.resources.storage.clone(), + wait_for_namenodes: self + .logging + .for_container(&DataNodeContainer::WaitForNameNodes) + .into_owned(), + }, + logging: RoleGroupLogging { hdfs, vector }, + _config: PhantomData, + }) + } +} diff --git a/rust/operator-binary/src/controller/build/resource/config_map.rs b/rust/operator-binary/src/controller/build/resource/config_map.rs index 9d224cae..f77ebf9c 100644 --- a/rust/operator-binary/src/controller/build/resource/config_map.rs +++ b/rust/operator-binary/src/controller/build/resource/config_map.rs @@ -17,7 +17,7 @@ use crate::{ controller::{ ValidatedCluster, build::{ - self, ResolvedRoleGroup, + self, ResolvedRoleGroup, RoleGroupResolver, properties::{ ConfigFileName, core_site, hadoop_policy, hdfs_site, product_logging, security_properties, ssl_client, ssl_server, @@ -47,17 +47,20 @@ type Result = std::result::Result; /// Builds the [`ConfigMap`] of one role group. /// -/// Every role-specific value is resolved by the caller into `resolved`, the role itself included: -/// taking the role and the datanode storage configuration as two independent parameters would let -/// a caller pass a datanode without its storage, which silently drops `dfs.datanode.data.dir`. -pub fn build_rolegroup_config_map( +/// Every role-specific value is resolved by the caller into `resolved`. The role comes from +/// `C::ROLE`, and `C`'s [`RoleGroupResolver`] bound ties it to `resolved`, so this cannot read one +/// role's `HdfsNodeRole` alongside another role's resolved values. The datanode storage +/// configuration comes from `resolved` rather than a separate parameter: taking it independently +/// would let a caller pass a datanode without its storage, which silently drops +/// `dfs.datanode.data.dir`. +pub fn build_rolegroup_config_map( cluster: &ValidatedCluster, cluster_info: &KubernetesClusterInfo, role_group_name: &RoleGroupName, rolegroup_config: &RoleGroupConfig, - resolved: &ResolvedRoleGroup, + resolved: &ResolvedRoleGroup, ) -> Result { - let role = resolved.role.node_role(); + let role = C::ROLE; tracing::info!( "Setting up ConfigMap for role {role} role group {role_group_name}", @@ -108,7 +111,9 @@ pub fn build_rolegroup_config_map( )?, ); - for (log_config_file, log4j_config) in product_logging::build_log4j_configs(&resolved.logging) { + for (log_config_file, log4j_config) in + product_logging::build_log4j_configs(&resolved.logging, &resolved.role) + { builder.add_data(log_config_file, log4j_config); } if resolved.logging.vector.is_some() { diff --git a/rust/operator-binary/src/controller/build/resource/statefulset.rs b/rust/operator-binary/src/controller/build/resource/statefulset.rs index 8a94b2bb..a4988c69 100644 --- a/rust/operator-binary/src/controller/build/resource/statefulset.rs +++ b/rust/operator-binary/src/controller/build/resource/statefulset.rs @@ -39,14 +39,15 @@ pub enum Error { /// Builds the [`StatefulSet`] of one role group. /// -/// Every role-specific value is resolved by the caller into `resolved`. The role comes from the -/// role group's config type, so it cannot disagree with `resolved`. +/// Every role-specific value is resolved by the caller into `resolved`. The role comes from +/// `C::ROLE`, and `resolved` is [`ResolvedRoleGroup`](ResolvedRoleGroup), produced by that same +/// `C`'s [`RoleGroupResolver::resolve`], so it cannot disagree with `resolved`. pub(crate) fn build_rolegroup_statefulset( validated: &ValidatedCluster, cluster_info: &KubernetesClusterInfo, role_group_name: &RoleGroupName, rolegroup_config: &RoleGroupConfig, - resolved: ResolvedRoleGroup, + resolved: &ResolvedRoleGroup, ) -> Result { let role = &C::ROLE; @@ -87,7 +88,7 @@ pub(crate) fn build_rolegroup_statefulset( cluster_info, role_group_name, rolegroup_config, - &resolved, + resolved, ) .context(FailedToCreateContainerAndVolumeConfigurationSnafu)?; @@ -102,7 +103,7 @@ pub(crate) fn build_rolegroup_statefulset( pod_management_policy: Some("OrderedReady".to_string()), replicas: rolegroup_config.replicas.map(i32::from), selector: LabelSelector { - match_labels: Some(resolved.selector_labels.into()), + match_labels: Some(resolved.selector_labels.clone().into()), ..LabelSelector::default() }, service_name: Some( @@ -112,7 +113,7 @@ pub(crate) fn build_rolegroup_statefulset( ), template: pod_template, - volume_claim_templates: Some(resolved.volume_claim_templates), + volume_claim_templates: Some(resolved.volume_claim_templates.clone()), ..StatefulSetSpec::default() }; diff --git a/rust/operator-binary/src/controller/validate.rs b/rust/operator-binary/src/controller/validate.rs index 20c69098..1fa64d66 100644 --- a/rust/operator-binary/src/controller/validate.rs +++ b/rust/operator-binary/src/controller/validate.rs @@ -87,21 +87,12 @@ pub fn validate_cluster( ) .context(ResolveProductImageSnafu)?; - let validated_role_config = |role: HdfsNodeRole| { - hdfs.role_config(&role).map( - |GenericRoleConfig { - pod_disruption_budget, - }| ValidatedRoleConfig { - pdb: pod_disruption_budget.clone(), - }, - ) - }; - let cluster_name = get_cluster_name(hdfs).context(GetClusterNameSnafu)?; - // Validated in `HdfsNodeRole` declaration order, because the first role that fails is the - // error the user sees: reordering these three statements changes which misconfiguration gets - // reported when more than one role is wrong. + // The first failure propagates, so the order of these three statements decides which + // misconfiguration the user is told about when more than one role is wrong. It follows + // `HdfsNodeRole`'s declaration order to leave a reader one order to hold in mind rather than + // two; that declaration order is fixed by the upgrade rollout, for which see [`HdfsNodeRole`]. let journalnode_role_group_configs = validate_role_group_configs( hdfs.spec.journal_nodes.as_ref(), JournalNodeConfigFragment::default_config(cluster_name.as_ref(), &HdfsNodeRole::Journal), @@ -140,11 +131,11 @@ pub fn validate_cluster( uid, cluster_config: ValidatedClusterConfig::resolve(hdfs, hdfs_opa_config), image, - namenode_config: validated_role_config(HdfsNodeRole::Name), + namenode_config: validated_role_config(hdfs, HdfsNodeRole::Name), namenode_role_group_configs, - datanode_config: validated_role_config(HdfsNodeRole::Data), + datanode_config: validated_role_config(hdfs, HdfsNodeRole::Data), datanode_role_group_configs, - journalnode_config: validated_role_config(HdfsNodeRole::Journal), + journalnode_config: validated_role_config(hdfs, HdfsNodeRole::Journal), journalnode_role_group_configs, namenode_listeners, discovery_config_map, @@ -152,6 +143,23 @@ pub fn validate_cluster( }) } +/// The validated role-level config of one role, or `None` if the role is absent from the spec. +/// +/// [`GenericRoleConfig`] is destructured without `..`, so a field added to it upstream fails to +/// compile here instead of being silently left unvalidated. +fn validated_role_config( + hdfs: &v1alpha1::HdfsCluster, + role: HdfsNodeRole, +) -> Option { + hdfs.role_config(&role).map( + |GenericRoleConfig { + pod_disruption_budget, + }| ValidatedRoleConfig { + pdb: pod_disruption_budget.clone(), + }, + ) +} + /// Validates every role group of a role into a map keyed by role group name. /// /// Each role group is merged and validated via diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs index 13498519..8701670a 100644 --- a/rust/operator-binary/src/crd/mod.rs +++ b/rust/operator-binary/src/crd/mod.rs @@ -417,7 +417,11 @@ impl HdfsNodeRole { } } - pub fn check_valid_dfs_replication(&self) -> bool { + /// Whether this role's replica count is the one `dfs.replication` is compared against. + /// + /// Only datanodes hold block replicas, so only their count can fall short of the + /// replication factor. + pub fn replicas_must_cover_dfs_replication(&self) -> bool { match self { HdfsNodeRole::Name => false, HdfsNodeRole::Data => true, diff --git a/rust/operator-binary/src/event.rs b/rust/operator-binary/src/event.rs index 0948f571..289bbd12 100644 --- a/rust/operator-binary/src/event.rs +++ b/rust/operator-binary/src/event.rs @@ -65,7 +65,7 @@ pub fn build_invalid_replica_message( Some(format!( "{role_name}: currently has an even number of replicas [{replicas}], but should always have an odd number to ensure quorum" )) - } else if role.check_valid_dfs_replication() && replicas < dfs_replication as u16 { + } else if role.replicas_must_cover_dfs_replication() && replicas < dfs_replication as u16 { Some(format!( "{role_name}: HDFS replication factor [{dfs_replication}] is configured greater than data node replicas [{replicas}]" )) @@ -161,7 +161,7 @@ spec: } /// A `dfsReplication` above the datanode count means HDFS cannot place every replica, so the - /// user is warned. The gate for this is [`HdfsNodeRole::check_valid_dfs_replication`], which + /// user is warned. The gate for this is [`HdfsNodeRole::replicas_must_cover_dfs_replication`], which /// is true for datanodes only — the message is about datanodes. #[test] fn fewer_datanodes_than_the_replication_factor_warns() { @@ -203,4 +203,45 @@ spec: ) ); } + + /// The `dfsReplication` warning is worded in terms of datanode replicas, so only datanodes + /// are compared against it. A journalnode role group with fewer replicas than + /// `dfsReplication` is not a misconfiguration and must stay silent. + #[test] + fn journalnodes_are_not_compared_to_the_replication_factor() { + let cluster = deserialize_and_validate_cluster( + " +--- +apiVersion: hdfs.stackable.tech/v1alpha1 +kind: HdfsCluster +metadata: + name: hdfs + namespace: default + uid: c2c8c5c0-0b5a-4b1e-9f3e-1a2b3c4d5e6f +spec: + image: + productVersion: 3.4.0 + clusterConfig: + zookeeperConfigMapName: hdfs-zk + dfsReplication: 5 + nameNodes: + roleGroups: + default: + replicas: 2 + journalNodes: + roleGroups: + default: + replicas: 3 + dataNodes: + roleGroups: + default: + replicas: 5 +", + ); + + assert_eq!( + build_invalid_replica_message(&cluster, &HdfsNodeRole::Journal), + None + ); + } } From 001f76afa9df5da08a8f659190fbfcd8c983844e Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Tue, 15 Sep 2026 13:28:51 +0200 Subject: [PATCH 14/19] renamed parameter --- rust/operator-binary/src/controller/build/mod.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/rust/operator-binary/src/controller/build/mod.rs b/rust/operator-binary/src/controller/build/mod.rs index bd2e02b3..78cca113 100644 --- a/rust/operator-binary/src/controller/build/mod.rs +++ b/rust/operator-binary/src/controller/build/mod.rs @@ -125,7 +125,7 @@ struct RoleGroupResources { } /// Builds every resource of every role group of one role, plus that role's PDB, appending them to -/// `out`. +/// `rg_resources`. fn build_role( cluster: &ValidatedCluster, cluster_info: &KubernetesClusterInfo, @@ -133,12 +133,12 @@ fn build_role( RoleGroupName, RoleGroupConfig, >, - out: &mut RoleGroupResources, + rg_resources: &mut RoleGroupResources, ) -> Result<(), Error> { let role = &C::ROLE; for (role_group_name, rg_config) in role_group_configs { - build_role_group_services(cluster, role, role_group_name, &mut out.services)?; + build_role_group_services(cluster, role, role_group_name, &mut rg_resources.services)?; let selector_labels = rolegroup_selector_labels(cluster, role, role_group_name).context( RoleGroupSelectorLabelsSnafu { @@ -148,7 +148,7 @@ fn build_role( )?; let resolved = rg_config.config.resolve(role_group_name, selector_labels)?; - out.config_maps.push( + rg_resources.config_maps.push( resource::config_map::build_rolegroup_config_map( cluster, cluster_info, @@ -161,7 +161,7 @@ fn build_role( role_group: role_group_name.clone(), })?, ); - out.stateful_sets.entry(C::ROLE).or_default().push( + rg_resources.stateful_sets.entry(C::ROLE).or_default().push( resource::statefulset::build_rolegroup_statefulset( cluster, cluster_info, @@ -177,7 +177,7 @@ fn build_role( } if let Some(pdb) = resource::pdb::build_pdb(cluster, role) { - out.pod_disruption_budgets.push(pdb); + rg_resources.pod_disruption_budgets.push(pdb); } Ok(()) From 75e213989cd6f53970c7b1089f07fcfca73f1d7e Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Tue, 15 Sep 2026 14:34:36 +0200 Subject: [PATCH 15/19] improve comments --- .../src/controller/build/container.rs | 14 ++-- .../src/controller/build/mod.rs | 24 +++---- .../src/controller/build/resolve.rs | 64 ++++++------------- rust/operator-binary/src/controller/mod.rs | 5 +- .../src/controller/validate.rs | 14 ++-- rust/operator-binary/src/event.rs | 9 ++- 6 files changed, 45 insertions(+), 85 deletions(-) diff --git a/rust/operator-binary/src/controller/build/container.rs b/rust/operator-binary/src/controller/build/container.rs index d6605b91..45e93d35 100644 --- a/rust/operator-binary/src/controller/build/container.rs +++ b/rust/operator-binary/src/controller/build/container.rs @@ -213,11 +213,8 @@ impl ContainerConfig { /// Add all main, side and init containers as well as required volumes to the pod builder. /// - /// Every role-specific value is resolved by the caller into `resolved`. The role comes from - /// `C::ROLE`, and `resolved` is [`ResolvedRoleGroup`](ResolvedRoleGroup), produced by that - /// same `C`'s [`RoleGroupResolver::resolve`], so it cannot disagree with `resolved`: pairing a - /// role with another role's resolved values would silently drop the containers' - /// `log4j.properties`. + /// Every role-specific value is resolved by the caller into `resolved`; the role itself comes + /// from `C::ROLE`, the same `C` that produced it. pub fn add_containers_and_volumes( pb: &mut PodBuilder, cluster: &ValidatedCluster, @@ -1081,10 +1078,9 @@ impl ContainerConfig { /// Return the container volumes. /// - /// `container_log_config` is this container's own log config, chosen by the caller from the - /// role group's `RoleGroupLogging` or [`RoleSpecificValues`]; a container is never built - /// without one. `listener_volume` is the role group's ephemeral listener volume, which only - /// the main container of the datanodes has. + /// `container_log_config` is this container's own, chosen by the caller from + /// [`build::RoleGroupLogging`] or [`RoleSpecificValues`]. `listener_volume` is the role + /// group's ephemeral listener volume, which only the datanode main container has. fn volumes( &self, container_log_config: &ContainerLogConfig, diff --git a/rust/operator-binary/src/controller/build/mod.rs b/rust/operator-binary/src/controller/build/mod.rs index 78cca113..2eeb36d5 100644 --- a/rust/operator-binary/src/controller/build/mod.rs +++ b/rust/operator-binary/src/controller/build/mod.rs @@ -111,15 +111,13 @@ pub enum Error { pub(crate) use resolve::RoleGroupResolver; pub use resolve::{ResolvedRoleGroup, RoleGroupLogging, RoleSpecificValues}; -/// The resources built for the role groups of one role, accumulated across the roles by -/// [`build`]. +/// The resources of every role, accumulated one role at a time by [`build_role`]. #[derive(Default)] struct RoleGroupResources { services: Vec, config_maps: Vec, - /// Keyed by role so that flattening the map yields the StatefulSets in rollout order, - /// whatever order [`build`] happens to call [`build_role`] in. See [`HdfsNodeRole`], whose - /// variant order defines that rollout order. + /// Keyed by role, so flattening the map yields the StatefulSets in the rollout order + /// [`HdfsNodeRole`]'s variant order defines, whatever order the roles were built in. stateful_sets: BTreeMap>, pod_disruption_budgets: Vec, } @@ -190,11 +188,9 @@ fn build_role( /// `cluster_info` carries static cluster information resolved at operator startup (e.g. the /// cluster domain used to build Kerberos principals), not a live client. /// -/// The resources are returned as flat collections. `stateful_sets` is ordered by role — -/// journalnodes, then namenodes, then datanodes — because the apply step rolls them out in that -/// order during upgrades to preserve HDFS's rollout-gated deployment (see -/// [`crate::controller::apply::Applier::apply`]). That ordering is structural: they are -/// accumulated in a [`BTreeMap`] keyed by [`HdfsNodeRole`] and flattened in key order. +/// The resources are returned as flat collections. `stateful_sets` comes out in [`HdfsNodeRole`] +/// order, which the apply step depends on; that is structural, from a [`BTreeMap`] flattened in +/// key order, not from the order the roles are built in. /// The discovery `ConfigMap` is included when it can be built or re-emitted (see /// [`resource::discovery::build_discovery_config_map`]); it is only absent before its first /// successful build. @@ -204,12 +200,8 @@ pub fn build( ) -> Result, Error> { let mut built = RoleGroupResources::default(); - // The rollout order of the StatefulSets is load-bearing: the apply step rolls them out - // journalnodes first, then namenodes, then datanodes, each role gated on the previous one - // (see [`crate::controller::apply::Applier::apply`]). That order comes from the - // `HdfsNodeRole` key of `RoleGroupResources::stateful_sets`, not from the order of the calls - // below. The other three collections are plain `Vec`s appended in call order, which the apply - // step does not depend on: it applies each of them in bulk. + // These three calls are free to be reordered: the StatefulSets are keyed by role, and the + // apply step does not depend on the order of the other three collections. build_role( cluster, cluster_info, diff --git a/rust/operator-binary/src/controller/build/resolve.rs b/rust/operator-binary/src/controller/build/resolve.rs index 104a3cf9..2564d331 100644 --- a/rust/operator-binary/src/controller/build/resolve.rs +++ b/rust/operator-binary/src/controller/build/resolve.rs @@ -1,9 +1,6 @@ -//! Resolving one role group's role-specific values, once per role. +//! Resolving one role group into the values the shared builders cannot derive themselves. //! -//! These types live in their own module so that [`ResolvedRoleGroup`]'s private `_config` field is -//! private to *them*: the shared builders in [`super::container`] and [`super::resource`] are -//! siblings of this module rather than descendants, so [`RoleGroupResolver::resolve`] is the only -//! way any of them can obtain a bundle. +//! One [`RoleGroupResolver`] impl per role config type, so a role's resolution is written once. use std::{fmt::Display, marker::PhantomData}; @@ -22,12 +19,8 @@ use crate::crd::{ storage::DataNodeStorageConfigInnerType, }; -/// The log configuration of the two containers every role has, resolved during the build step by -/// code that knows the role, so the shared builders never see a role-specific `Logging`. -/// -/// The containers only one role runs carry their log config in [`RoleSpecificValues`], so that -/// "the `zkfc` log config exists exactly when this is a namenode" is one fact, not two that have -/// to agree. +/// The log config of the two containers every role has. Containers only one role runs carry theirs +/// in [`RoleSpecificValues`], which is the single place the role is decided. #[derive(Debug)] pub struct RoleGroupLogging { /// The main `hdfs` container, which every role has. @@ -36,20 +29,12 @@ pub struct RoleGroupLogging { pub vector: Option, } -/// Everything about one role group that the shared builders below cannot derive themselves: the -/// values resolved from its role-specific config, plus the selector labels, which the build loop -/// already needs for the listener volume and the PVC templates. -/// -/// Resolving these in the build loop, which knows the role, is what lets the builders be generic -/// over the role group's config type. +/// The values the shared builders cannot derive themselves, resolved by +/// [`RoleGroupResolver::resolve`], which knows the role. /// -/// `C` is the role group's config type, the same one [`RoleGroupResolver`] is implemented on. /// Every builder takes `RoleGroupConfig` and `ResolvedRoleGroup` together, so one role's -/// overrides and replica count cannot be paired with another role's resolved values: the two -/// parameters are the same `C` or they do not compile. The private `_config` field makes -/// [`RoleGroupResolver::resolve`] the only constructor outside this module — a struct literal -/// elsewhere is rejected with `E0451` — so a `C` can never disagree with the values filled in -/// beside it. +/// overrides and replica count cannot be paired with another role's resolved values: both are the +/// same `C` or they do not compile. pub struct ResolvedRoleGroup { /// The selector labels of the role group's pods, also used as the `StatefulSet` selector and /// on its listener volume. @@ -70,22 +55,20 @@ pub struct ResolvedRoleGroup { pub role: RoleSpecificValues, /// The log config of each of the role group's containers. pub logging: RoleGroupLogging, - /// Ties this bundle to the role group's config type; see the struct documentation. The - /// `fn() -> C` spelling marks the relationship without claiming this struct owns a `C`. + /// Ties the bundle to its config type. Private, so [`RoleGroupResolver::resolve`] is the only + /// constructor outside this module — a struct literal elsewhere is `E0451`. `fn() -> C` rather + /// than `C`, so the bundle does not read as owning one. _config: PhantomData C>, } -/// Everything that exists for one role only: the containers that role runs, their log configs, -/// and its storage and listener arrangements. +/// Everything that exists for one role only: the containers that role runs, their log configs, and +/// its storage and listener arrangements. /// -/// One enum rather than several `Option` fields, so the compiler checks the pairing at every -/// construction site and every consumer is exhaustive. A datanode without its storage -/// configuration does not compile — that would silently drop `dfs.datanode.data.dir` and send the -/// datanodes' blocks to container-local storage. Neither does a namenode with a pod-level -/// listener volume, which would collide with the identically named volume claim template and be -/// rejected at apply time. And neither does a container without its log config, which would leave -/// `log4j.properties` out of both the `ConfigMap` and the `cp` in the container args, so the -/// container logs with Hadoop's built-in defaults and Vector collects nothing for it. +/// An enum rather than `Option` fields, so consumers are exhaustive and three silent failures do +/// not compile: a datanode without its storage drops `dfs.datanode.data.dir` and sends its blocks +/// to container-local storage; a namenode with a pod-level listener volume collides with the +/// identically named claim template and is rejected at apply time; a container without its log +/// config falls back to Hadoop's built-in logging, uncollected by Vector. pub enum RoleSpecificValues { /// Journalnodes run no role-specific container, have no listener and no role-specific /// storage configuration. @@ -153,14 +136,9 @@ where /// How to resolve one role group's role-specific values, implemented once per role config type. /// -/// This is what lets [`build_role`](super::build_role) be written once: the trait supplies the role and the single -/// role-dependent step, and everything else about building a role group is identical across the -/// three roles. -/// -/// [`Self::ROLE`] is the single source of truth for the role in the shared builders: they take the -/// role group's config type and read the role from it, rather than taking the role as a second -/// parameter a caller could pair with the wrong config. See [`ResolvedRoleGroup`] for what the -/// shared `C` guarantees. +/// The trait supplies the role and the single role-dependent step, which is what lets +/// [`build_role`](super::build_role) be written once. The shared builders read [`Self::ROLE`] +/// instead of taking a role parameter a caller could pair with the wrong config. pub(crate) trait RoleGroupResolver: Sized { /// The role whose config this is. const ROLE: HdfsNodeRole; diff --git a/rust/operator-binary/src/controller/mod.rs b/rust/operator-binary/src/controller/mod.rs index d3de284c..432a7c57 100644 --- a/rust/operator-binary/src/controller/mod.rs +++ b/rust/operator-binary/src/controller/mod.rs @@ -53,9 +53,8 @@ pub struct Applied; /// Every Kubernetes resource produced by the build step. /// -/// The resources are flat collections. `stateful_sets` is ordered by role — journalnodes, then -/// namenodes, then datanodes — because the apply step rolls them out in that order during -/// upgrades to preserve HDFS's rollout-gated deployment (see [`apply::Applier::apply`]). +/// The resources are flat collections. `stateful_sets` is ordered by [`HdfsNodeRole`], which +/// [`apply::Applier::apply`] relies on when rolling out an upgrade. /// The discovery `ConfigMap` is part of `config_maps` whenever it can be built or /// re-emitted; it is only absent before its first successful build (see /// [`build::resource::discovery::build_discovery_config_map`]). diff --git a/rust/operator-binary/src/controller/validate.rs b/rust/operator-binary/src/controller/validate.rs index 1fa64d66..212b9ce6 100644 --- a/rust/operator-binary/src/controller/validate.rs +++ b/rust/operator-binary/src/controller/validate.rs @@ -89,10 +89,8 @@ pub fn validate_cluster( let cluster_name = get_cluster_name(hdfs).context(GetClusterNameSnafu)?; - // The first failure propagates, so the order of these three statements decides which - // misconfiguration the user is told about when more than one role is wrong. It follows - // `HdfsNodeRole`'s declaration order to leave a reader one order to hold in mind rather than - // two; that declaration order is fixed by the upgrade rollout, for which see [`HdfsNodeRole`]. + // The first failure propagates, so this order decides which misconfiguration the user is told + // about when more than one role is wrong. It is `HdfsNodeRole`'s variant order. let journalnode_role_group_configs = validate_role_group_configs( hdfs.spec.journal_nodes.as_ref(), JournalNodeConfigFragment::default_config(cluster_name.as_ref(), &HdfsNodeRole::Journal), @@ -120,9 +118,8 @@ pub fn validate_cluster( .and_then(|status| status.upgrade_target_product_version.clone()), }; - // The three role-level configs share one type, so each is named at the point it is set: as - // positional arguments two of them could be swapped silently, giving a role another role's - // PodDisruptionBudget. + // The three role-level configs share one type, so naming each field is what stops a role + // being given another role's PodDisruptionBudget. Ok(ValidatedCluster { metadata: ValidatedCluster::object_meta(&cluster_name, &namespace, &uid), product_version: ValidatedCluster::product_version(&image), @@ -199,8 +196,7 @@ where >(role_group, role, &default_config) .context(ValidateRoleGroupConfigSnafu)?; - // Flatten the nested config into a single `RoleGroupConfig`; the merged overrides - // carry over unchanged. + // The overrides carry over unchanged. let validated = RoleGroupConfig { replicas: validated.replicas, config: validated.config.config, diff --git a/rust/operator-binary/src/event.rs b/rust/operator-binary/src/event.rs index 289bbd12..7a891f27 100644 --- a/rust/operator-binary/src/event.rs +++ b/rust/operator-binary/src/event.rs @@ -80,9 +80,8 @@ mod tests { use crate::test_support::deserialize_and_validate_cluster; /// A role group with no explicit `replicas` runs one pod — Kubernetes' default for a - /// `StatefulSet` with `replicas: null` — so it must not be counted as zero. Counting it as - /// zero produced a warning event telling the user to configure at least one datanode when - /// they already had one. + /// `StatefulSet` with `replicas: null` — so counting it as zero would warn about a role group + /// that is fine. #[test] fn an_unset_replica_count_counts_as_one_datanode() { let cluster = deserialize_and_validate_cluster( @@ -161,8 +160,8 @@ spec: } /// A `dfsReplication` above the datanode count means HDFS cannot place every replica, so the - /// user is warned. The gate for this is [`HdfsNodeRole::replicas_must_cover_dfs_replication`], which - /// is true for datanodes only — the message is about datanodes. + /// user is warned. [`HdfsNodeRole::replicas_must_cover_dfs_replication`] gates it to datanodes, + /// which is what the message is about. #[test] fn fewer_datanodes_than_the_replication_factor_warns() { let cluster = deserialize_and_validate_cluster( From e3a7957ee9fea69f44972e18439cc98385aff704 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Tue, 15 Sep 2026 15:28:07 +0200 Subject: [PATCH 16/19] fix kerberos test and correct comment on smoke test --- .../kuttl/kerberos/20-install-hdfs.txt.j2 | 14 ++++++++++++++ .../templates/kuttl/smoke/30-install-hdfs.yaml.j2 | 14 ++++++++------ 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/tests/templates/kuttl/kerberos/20-install-hdfs.txt.j2 b/tests/templates/kuttl/kerberos/20-install-hdfs.txt.j2 index 455f9630..bc3558b6 100644 --- a/tests/templates/kuttl/kerberos/20-install-hdfs.txt.j2 +++ b/tests/templates/kuttl/kerberos/20-install-hdfs.txt.j2 @@ -27,6 +27,15 @@ spec: vectorAggregatorConfigMapName: vector-aggregator-discovery {% endif %} nameNodes: + configOverrides: + core-site.xml: + # The chaos monkey (31-unleash-the-chaosmonkey.yaml.j2) force-deletes every HDFS pod, so + # both namenodes go at once. `OrderedReady` then holds namenode-1 back until namenode-0 is + # ready, while namenode-0's format-namenodes init container probes namenode-1 with + # `hdfs haadmin -getServiceState` — a peer that cannot exist yet. At the default 45 retries + # x 20s that probe blocks for 15 min, outlasting the chaos monkey's 10 min `kubectl wait` + # and failing the step. + ipc.client.connect.max.retries.on.timeouts: "3" config: logging: enableVectorAgent: {{ lookup('env', 'VECTOR_AGGREGATOR') | length > 0 }} @@ -43,6 +52,11 @@ spec: default: replicas: 2 dataNodes: + configOverrides: + core-site.xml: + # wait-for-namenodes runs the same haadmin probe against the same unreachable peers, with + # the same retry budget. + ipc.client.connect.max.retries.on.timeouts: "3" config: requestedSecretLifetime: 2d logging: diff --git a/tests/templates/kuttl/smoke/30-install-hdfs.yaml.j2 b/tests/templates/kuttl/smoke/30-install-hdfs.yaml.j2 index fe2aac56..7c1abb1e 100644 --- a/tests/templates/kuttl/smoke/30-install-hdfs.yaml.j2 +++ b/tests/templates/kuttl/smoke/30-install-hdfs.yaml.j2 @@ -27,10 +27,12 @@ spec: nameNodes: configOverrides: core-site.xml: - # After the chaos monkey (60-unleash-the-chaosmonkey.yaml.j2) force-deletes all pods, - # the format-namenodes probe of the other namenode can resolve its name to the stale - # pre-delete pod IP (DNS cache) and silently retry a black-hole TCP connect for - # 45 x 20s = 15 min by default, exceeding the chaos-monkey step's 10 min `kubectl wait`. + # The chaos monkey (60-unleash-the-chaosmonkey.yaml.j2) force-deletes every HDFS pod, so + # both namenodes go at once. `OrderedReady` holds namenode-1 back until namenode-0 is + # ready, while namenode-0's format-namenodes init container probes namenode-1 with + # `hdfs haadmin -getServiceState`. That peer cannot exist yet, so its name does not + # resolve and the ipc client retries an `` address on the connect-timeout + # path: 45 x 20s = 15 min by default, outlasting the step's 10 min `kubectl wait`. # zkfc is unaffected: graceful fencing and health monitoring override their retries via # ha.failover-controller.graceful-fence.connection.retries and # ha.health-monitor.rpc.connect.max.retries respectively. @@ -63,8 +65,8 @@ spec: dataNodes: configOverrides: core-site.xml: - # See the identical nameNodes override: wait-for-namenodes runs the same haadmin - # probe against namenodes that may resolve to a stale pod IP after the chaos monkey. + # See the identical nameNodes override: wait-for-namenodes runs the same haadmin probe + # against namenodes that cannot exist yet, with the same retry budget. ipc.client.connect.max.retries.on.timeouts: "3" envOverrides: COMMON_VAR: role-value # overridden by role group below From a21c2d347a4c02b643294ae472b34f13a7017e28 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Tue, 15 Sep 2026 17:11:42 +0200 Subject: [PATCH 17/19] fix cluster-op test --- .../cluster-operation/20-install-hdfs.yaml.j2 | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/templates/kuttl/cluster-operation/20-install-hdfs.yaml.j2 b/tests/templates/kuttl/cluster-operation/20-install-hdfs.yaml.j2 index 34297d70..a2f68ae8 100644 --- a/tests/templates/kuttl/cluster-operation/20-install-hdfs.yaml.j2 +++ b/tests/templates/kuttl/cluster-operation/20-install-hdfs.yaml.j2 @@ -26,6 +26,15 @@ spec: stopped: false reconciliationPaused: false nameNodes: + configOverrides: + core-site.xml: + # 30-stop-hdfs.yaml.j2 scales every StatefulSet to zero, so both namenodes go at once. + # On restart, `OrderedReady` holds namenode-1 back until namenode-0 is ready, while + # namenode-0's format-namenodes init container probes namenode-1 with + # `hdfs haadmin -getServiceState`. That peer cannot exist yet, so its name does not + # resolve and the ipc client retries an `` address on the connect-timeout + # path: 45 x 20s = 15 min by default, outlasting this test's asserts. + ipc.client.connect.max.retries.on.timeouts: "3" config: logging: enableVectorAgent: {{ lookup('env', 'VECTOR_AGGREGATOR') | length > 0 }} @@ -33,6 +42,11 @@ spec: default: replicas: 2 dataNodes: + configOverrides: + core-site.xml: + # See the identical nameNodes override: wait-for-namenodes runs the same haadmin probe + # against namenodes that cannot exist yet, with the same retry budget. + ipc.client.connect.max.retries.on.timeouts: "3" config: logging: enableVectorAgent: {{ lookup('env', 'VECTOR_AGGREGATOR') | length > 0 }} From e989209d236aebf944cc008f2fcc998b042c9b98 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Tue, 15 Sep 2026 17:53:07 +0200 Subject: [PATCH 18/19] corrected test comments --- .../cluster-operation/20-install-hdfs.yaml.j2 | 14 ++++++-------- .../kuttl/kerberos/20-install-hdfs.txt.j2 | 14 ++++++-------- .../kuttl/smoke/30-install-hdfs.yaml.j2 | 17 ++++++----------- 3 files changed, 18 insertions(+), 27 deletions(-) diff --git a/tests/templates/kuttl/cluster-operation/20-install-hdfs.yaml.j2 b/tests/templates/kuttl/cluster-operation/20-install-hdfs.yaml.j2 index a2f68ae8..b069afe4 100644 --- a/tests/templates/kuttl/cluster-operation/20-install-hdfs.yaml.j2 +++ b/tests/templates/kuttl/cluster-operation/20-install-hdfs.yaml.j2 @@ -28,12 +28,11 @@ spec: nameNodes: configOverrides: core-site.xml: - # 30-stop-hdfs.yaml.j2 scales every StatefulSet to zero, so both namenodes go at once. - # On restart, `OrderedReady` holds namenode-1 back until namenode-0 is ready, while - # namenode-0's format-namenodes init container probes namenode-1 with - # `hdfs haadmin -getServiceState`. That peer cannot exist yet, so its name does not - # resolve and the ipc client retries an `` address on the connect-timeout - # path: 45 x 20s = 15 min by default, outlasting this test's asserts. + # After all namenodes go at once, a deleted pod's name keeps resolving to its dead IP for + # up to ~30s (CoreDNS cache racing the endpoint update). format-namenodes probes it with + # `hdfs haadmin -getServiceState`, the TCP connect black-holes, and the default 45 x 20s + # retry budget outlasts this test's asserts. Whether the probe lands in that window is + # environment-dependent: local kind loses the race far more often than CI. ipc.client.connect.max.retries.on.timeouts: "3" config: logging: @@ -44,8 +43,7 @@ spec: dataNodes: configOverrides: core-site.xml: - # See the identical nameNodes override: wait-for-namenodes runs the same haadmin probe - # against namenodes that cannot exist yet, with the same retry budget. + # See the identical nameNodes override: wait-for-namenodes runs the same haadmin probe. ipc.client.connect.max.retries.on.timeouts: "3" config: logging: diff --git a/tests/templates/kuttl/kerberos/20-install-hdfs.txt.j2 b/tests/templates/kuttl/kerberos/20-install-hdfs.txt.j2 index bc3558b6..ab1be56c 100644 --- a/tests/templates/kuttl/kerberos/20-install-hdfs.txt.j2 +++ b/tests/templates/kuttl/kerberos/20-install-hdfs.txt.j2 @@ -29,12 +29,11 @@ spec: nameNodes: configOverrides: core-site.xml: - # The chaos monkey (31-unleash-the-chaosmonkey.yaml.j2) force-deletes every HDFS pod, so - # both namenodes go at once. `OrderedReady` then holds namenode-1 back until namenode-0 is - # ready, while namenode-0's format-namenodes init container probes namenode-1 with - # `hdfs haadmin -getServiceState` — a peer that cannot exist yet. At the default 45 retries - # x 20s that probe blocks for 15 min, outlasting the chaos monkey's 10 min `kubectl wait` - # and failing the step. + # After all namenodes go at once, a deleted pod's name keeps resolving to its dead IP for + # up to ~30s (CoreDNS cache racing the endpoint update). format-namenodes probes it with + # `hdfs haadmin -getServiceState`, the TCP connect black-holes, and the default 45 x 20s + # retry budget outlasts this test's asserts. Whether the probe lands in that window is + # environment-dependent: local kind loses the race far more often than CI. ipc.client.connect.max.retries.on.timeouts: "3" config: logging: @@ -54,8 +53,7 @@ spec: dataNodes: configOverrides: core-site.xml: - # wait-for-namenodes runs the same haadmin probe against the same unreachable peers, with - # the same retry budget. + # See the identical nameNodes override: wait-for-namenodes runs the same haadmin probe. ipc.client.connect.max.retries.on.timeouts: "3" config: requestedSecretLifetime: 2d diff --git a/tests/templates/kuttl/smoke/30-install-hdfs.yaml.j2 b/tests/templates/kuttl/smoke/30-install-hdfs.yaml.j2 index 7c1abb1e..8f87c514 100644 --- a/tests/templates/kuttl/smoke/30-install-hdfs.yaml.j2 +++ b/tests/templates/kuttl/smoke/30-install-hdfs.yaml.j2 @@ -27,15 +27,11 @@ spec: nameNodes: configOverrides: core-site.xml: - # The chaos monkey (60-unleash-the-chaosmonkey.yaml.j2) force-deletes every HDFS pod, so - # both namenodes go at once. `OrderedReady` holds namenode-1 back until namenode-0 is - # ready, while namenode-0's format-namenodes init container probes namenode-1 with - # `hdfs haadmin -getServiceState`. That peer cannot exist yet, so its name does not - # resolve and the ipc client retries an `` address on the connect-timeout - # path: 45 x 20s = 15 min by default, outlasting the step's 10 min `kubectl wait`. - # zkfc is unaffected: graceful fencing and health monitoring override their retries via - # ha.failover-controller.graceful-fence.connection.retries and - # ha.health-monitor.rpc.connect.max.retries respectively. + # After all namenodes go at once, a deleted pod's name keeps resolving to its dead IP for + # up to ~30s (CoreDNS cache racing the endpoint update). format-namenodes probes it with + # `hdfs haadmin -getServiceState`, the TCP connect black-holes, and the default 45 x 20s + # retry budget outlasts this test's asserts. Whether the probe lands in that window is + # environment-dependent: local kind loses the race far more often than CI. ipc.client.connect.max.retries.on.timeouts: "3" envOverrides: COMMON_VAR: role-value # overridden by role group below @@ -65,8 +61,7 @@ spec: dataNodes: configOverrides: core-site.xml: - # See the identical nameNodes override: wait-for-namenodes runs the same haadmin probe - # against namenodes that cannot exist yet, with the same retry budget. + # See the identical nameNodes override: wait-for-namenodes runs the same haadmin probe. ipc.client.connect.max.retries.on.timeouts: "3" envOverrides: COMMON_VAR: role-value # overridden by role group below From d3bac7e17a8026f0dd75a3cadef8060a78c12df9 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Wed, 16 Sep 2026 09:57:54 +0200 Subject: [PATCH 19/19] minor change to C comment --- rust/operator-binary/src/controller/build/resolve.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/rust/operator-binary/src/controller/build/resolve.rs b/rust/operator-binary/src/controller/build/resolve.rs index 2564d331..66fcea75 100644 --- a/rust/operator-binary/src/controller/build/resolve.rs +++ b/rust/operator-binary/src/controller/build/resolve.rs @@ -55,10 +55,10 @@ pub struct ResolvedRoleGroup { pub role: RoleSpecificValues, /// The log config of each of the role group's containers. pub logging: RoleGroupLogging, - /// Ties the bundle to its config type. Private, so [`RoleGroupResolver::resolve`] is the only - /// constructor outside this module — a struct literal elsewhere is `E0451`. `fn() -> C` rather - /// than `C`, so the bundle does not read as owning one. - _config: PhantomData C>, + /// Ties the bundle to its config type. Needed because `C` appears in no other field, which on + /// its own does not compile (`E0392`). Private, so [`RoleGroupResolver::resolve`] is the only + /// constructor outside this module — a struct literal elsewhere is `E0451`. + _config: PhantomData, } /// Everything that exists for one role only: the containers that role runs, their log configs, and