remove second metrics service, consolidate ports

maltesander · maltesander · commit 7cc2e2ff2229 · 2025-10-22T15:17:53.000+02:00
diff --git a/rust/operator-binary/src/container.rs b/rust/operator-binary/src/container.rs
@@ -488,9 +488,7 @@ impl ContainerConfig {
             )?)
             .add_volume_mounts(self.volume_mounts(hdfs, merged_config, labels)?)
             .context(AddVolumeMountSnafu)?
-            .add_container_ports(self.container_ports(hdfs))
-            // TODO: This currently adds the metrics port also to the zkfc containers, not needed there?
-            .add_container_port(SERVICE_PORT_NAME_METRICS, hdfs.metrics_port(role).into());
+            .add_container_ports(self.container_ports(hdfs));
 
         if let Some(resources) = resources {
             cb.resources(resources);
@@ -1251,16 +1249,18 @@ wait_for_termination $!
     /// Container ports for the main containers namenode, datanode and journalnode.
     fn container_ports(&self, hdfs: &v1alpha1::HdfsCluster) -> Vec<ContainerPort> {
         match self {
-            ContainerConfig::Hdfs { role, .. } => hdfs
-                .ports(role)
-                .into_iter()
-                .map(|(name, value)| ContainerPort {
-                    name: Some(name),
-                    container_port: i32::from(value),
-                    protocol: Some("TCP".to_string()),
-                    ..ContainerPort::default()
-                })
-                .collect(),
+            ContainerConfig::Hdfs { role, .. } => {
+                // data ports
+                hdfs.hdfs_main_container_ports(role)
+                    .into_iter()
+                    .map(|(name, value)| ContainerPort {
+                        name: Some(name),
+                        container_port: i32::from(value),
+                        protocol: Some("TCP".to_string()),
+                        ..ContainerPort::default()
+                    })
+                    .collect()
+            }
             _ => {
                 vec![]
             }
diff --git a/rust/operator-binary/src/crd/constants.rs b/rust/operator-binary/src/crd/constants.rs
@@ -20,6 +20,7 @@ pub const SERVICE_PORT_NAME_HTTP: &str = "http";
 pub const SERVICE_PORT_NAME_HTTPS: &str = "https";
 pub const SERVICE_PORT_NAME_DATA: &str = "data";
 pub const SERVICE_PORT_NAME_METRICS: &str = "metrics";
+pub const SERVICE_PORT_NAME_JMX_METRICS: &str = "jmx-metrics";
 
 pub const DEFAULT_LISTENER_CLASS: &str = "cluster-internal";
 
diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs
@@ -65,7 +65,8 @@ use crate::crd::{
         DEFAULT_NAME_NODE_RPC_PORT, DFS_REPLICATION, HADOOP_POLICY_XML, HDFS_SITE_XML,
         JVM_SECURITY_PROPERTIES_FILE, LISTENER_VOLUME_NAME, SERVICE_PORT_NAME_DATA,
         SERVICE_PORT_NAME_HTTP, SERVICE_PORT_NAME_HTTPS, SERVICE_PORT_NAME_IPC,
-        SERVICE_PORT_NAME_METRICS, SERVICE_PORT_NAME_RPC, SSL_CLIENT_XML, SSL_SERVER_XML,
+        SERVICE_PORT_NAME_JMX_METRICS, SERVICE_PORT_NAME_METRICS, SERVICE_PORT_NAME_RPC,
+        SSL_CLIENT_XML, SSL_SERVER_XML,
     },
     security::{AuthenticationConfig, KerberosConfig},
     storage::{
@@ -390,10 +391,10 @@ impl v1alpha1::HdfsCluster {
                 let ns = ns.clone();
                 (0..*replicas).map(move |i| HdfsPodRef {
                     namespace: ns.clone(),
-                    role_group_service_name: rolegroup_ref.object_name(),
+                    role_group_service_name: rolegroup_ref.rolegroup_headless_service_name(),
                     pod_name: format!("{}-{}", rolegroup_ref.object_name(), i),
                     ports: self
-                        .ports(role)
+                        .data_ports(role)
                         .iter()
                         .map(|(n, p)| (n.clone(), *p))
                         .collect(),
@@ -671,7 +672,7 @@ impl v1alpha1::HdfsCluster {
     }
 
     /// Returns required port name and port number tuples depending on the role.
-    pub fn ports(&self, role: &HdfsNodeRole) -> Vec<(String, u16)> {
+    pub fn data_ports(&self, role: &HdfsNodeRole) -> Vec<(String, u16)> {
         match role {
             HdfsNodeRole::Name => vec![
                 (
@@ -731,26 +732,26 @@ impl v1alpha1::HdfsCluster {
         }
     }
 
-    /// Returns required metrics port name and metrics port number tuples depending on the role.
-    pub fn metrics_ports(&self, role: &HdfsNodeRole) -> Vec<(String, u16)> {
+    /// Deprecated required JMX metrics port name and metrics port number tuples depending on the role.
+    pub fn jmx_metrics_ports(&self, role: &HdfsNodeRole) -> Vec<(String, u16)> {
         match role {
             HdfsNodeRole::Name => vec![(
-                String::from(SERVICE_PORT_NAME_METRICS),
+                String::from(SERVICE_PORT_NAME_JMX_METRICS),
                 DEFAULT_NAME_NODE_METRICS_PORT,
             )],
             HdfsNodeRole::Data => vec![(
-                String::from(SERVICE_PORT_NAME_METRICS),
+                String::from(SERVICE_PORT_NAME_JMX_METRICS),
                 DEFAULT_DATA_NODE_METRICS_PORT,
             )],
             HdfsNodeRole::Journal => vec![(
-                String::from(SERVICE_PORT_NAME_METRICS),
+                String::from(SERVICE_PORT_NAME_JMX_METRICS),
                 DEFAULT_JOURNAL_NODE_METRICS_PORT,
             )],
         }
     }
 
-    /// Returns required metrics port name and native metrics port number tuples depending on the role.
-    pub fn native_metrics_ports(&self, role: &HdfsNodeRole) -> Vec<(String, u16)> {
+    /// Returns required metrics port name and metrics port number tuples depending on the role and security settings.
+    pub fn metrics_ports(&self, role: &HdfsNodeRole) -> Vec<(String, u16)> {
         match role {
             HdfsNodeRole::Name => vec![if self.has_https_enabled() {
                 (
@@ -821,6 +822,28 @@ impl v1alpha1::HdfsCluster {
             }
         }
     }
+
+    pub fn metrics_service_ports(&self, role: &HdfsNodeRole) -> Vec<(String, u16)> {
+        let mut metrics_service_ports = vec![];
+        // "native" ports
+        metrics_service_ports.extend(self.metrics_ports(role));
+        metrics_service_ports.extend(self.jmx_metrics_ports(role));
+        metrics_service_ports
+    }
+
+    pub fn headless_service_ports(&self, role: &HdfsNodeRole) -> Vec<(String, u16)> {
+        let mut headless_service_ports = vec![];
+        headless_service_ports.extend(self.data_ports(role));
+        headless_service_ports
+    }
+
+    pub fn hdfs_main_container_ports(&self, role: &HdfsNodeRole) -> Vec<(String, u16)> {
+        let mut main_container_ports = vec![];
+        main_container_ports.extend(self.data_ports(role));
+        // TODO: This will be exposed in the listener if added to container ports?
+        // main_container_ports.extend(self.jmx_metrics_ports(role));
+        main_container_ports
+    }
 }
 
 #[derive(Clone, Debug, Deserialize, Eq, Hash, JsonSchema, PartialEq, Serialize)]
diff --git a/rust/operator-binary/src/hdfs_controller.rs b/rust/operator-binary/src/hdfs_controller.rs
@@ -69,10 +69,7 @@ use crate::{
     },
     product_logging::extend_role_group_config_map,
     security::{self, kerberos, opa::HdfsOpaConfig},
-    service::{
-        self, rolegroup_headless_service, rolegroup_metrics_service,
-        rolegroup_native_metrics_service,
-    },
+    service::{self, rolegroup_headless_service, rolegroup_metrics_service},
 };
 
 pub const RESOURCE_MANAGER_HDFS_CONTROLLER: &str = "hdfs-operator-hdfs-controller";
@@ -399,13 +396,6 @@ pub async fn reconcile_hdfs(
             let rg_metrics_service =
                 rolegroup_metrics_service(hdfs, &role, &rolegroup_ref, &resolved_product_image)
                     .context(BuildServiceSnafu)?;
-            let rg_native_metrics_service = rolegroup_native_metrics_service(
-                hdfs,
-                &role,
-                &rolegroup_ref,
-                &resolved_product_image,
-            )
-            .context(BuildServiceSnafu)?;
 
             // We need to split the creation and the usage of the "metadata" variable in two statements.
             // to avoid the compiler error "E0716 (temporary value dropped while borrowed)".
@@ -453,7 +443,6 @@ pub async fn reconcile_hdfs(
 
             let rg_service_name = rg_service.name_any();
             let rg_metrics_service_name = rg_metrics_service.name_any();
-            let rg_native_metrics_service_name = rg_native_metrics_service.name_any();
 
             cluster_resources
                 .add(client, rg_service)
@@ -467,12 +456,6 @@ pub async fn reconcile_hdfs(
                 .with_context(|_| ApplyRoleGroupServiceSnafu {
                     name: rg_metrics_service_name,
                 })?;
-            cluster_resources
-                .add(client, rg_native_metrics_service)
-                .await
-                .with_context(|_| ApplyRoleGroupServiceSnafu {
-                    name: rg_native_metrics_service_name,
-                })?;
             let rg_configmap_name = rg_configmap.name_any();
             cluster_resources
                 .add(client, rg_configmap.clone())
diff --git a/rust/operator-binary/src/service.rs b/rust/operator-binary/src/service.rs
@@ -67,7 +67,7 @@ pub(crate) fn rolegroup_headless_service(
         type_: Some("ClusterIP".to_string()),
         cluster_ip: Some("None".to_string()),
         ports: Some(
-            hdfs.ports(role)
+            hdfs.headless_service_ports(role)
                 .into_iter()
                 .map(|(name, value)| ServicePort {
                     name: Some(name),
@@ -106,7 +106,7 @@ pub(crate) fn rolegroup_metrics_service(
         type_: Some("ClusterIP".to_string()),
         cluster_ip: Some("None".to_string()),
         ports: Some(
-            hdfs.metrics_ports(role)
+            hdfs.metrics_service_ports(role)
                 .into_iter()
                 .map(|(name, value)| ServicePort {
                     name: Some(name),
@@ -145,79 +145,6 @@ pub(crate) fn rolegroup_metrics_service(
                 Label::try_from(("prometheus.io/scrape", "true"))
                     .context(BuildPrometheusLabelSnafu)?,
             )
-            .with_annotations(
-                Annotations::try_from([
-                    ("prometheus.io/path".to_owned(), "/metrics".to_owned()),
-                    (
-                        "prometheus.io/port".to_owned(),
-                        hdfs.metrics_port(role).to_string(),
-                    ),
-                    ("prometheus.io/scheme".to_owned(), "http".to_owned()),
-                    ("prometheus.io/scrape".to_owned(), "true".to_owned()),
-                ])
-                .expect("should be valid annotations"),
-            )
-            .build(),
-        spec: Some(service_spec),
-        status: None,
-    })
-}
-
-pub(crate) fn rolegroup_native_metrics_service(
-    hdfs: &v1alpha1::HdfsCluster,
-    role: &HdfsNodeRole,
-    rolegroup_ref: &RoleGroupRef<v1alpha1::HdfsCluster>,
-    resolved_product_image: &ResolvedProductImage,
-) -> Result<Service, Error> {
-    tracing::info!("Setting up native metrics Service for {:?}", rolegroup_ref);
-
-    let service_spec = ServiceSpec {
-        // Internal communication does not need to be exposed
-        type_: Some("ClusterIP".to_string()),
-        cluster_ip: Some("None".to_string()),
-        ports: Some(
-            hdfs.native_metrics_ports(role)
-                .into_iter()
-                .map(|(name, value)| ServicePort {
-                    name: Some(name),
-                    port: i32::from(value),
-                    protocol: Some("TCP".to_string()),
-                    ..ServicePort::default()
-                })
-                .collect(),
-        ),
-        selector: Some(
-            hdfs.rolegroup_selector_labels(rolegroup_ref)
-                .context(RoleGroupSelectorLabelsSnafu)?
-                .into(),
-        ),
-        publish_not_ready_addresses: Some(true),
-        ..ServiceSpec::default()
-    };
-
-    Ok(Service {
-        metadata: ObjectMetaBuilder::new()
-            .name_and_namespace(hdfs)
-            .name(format!(
-                "{name}-native-metrics",
-                name = rolegroup_ref.object_name()
-            ))
-            .ownerreference_from_resource(hdfs, None, Some(true))
-            .with_context(|_| ObjectMissingMetadataForOwnerRefSnafu {
-                obj_ref: ObjectRef::from_obj(hdfs),
-            })?
-            .with_recommended_labels(build_recommended_labels(
-                hdfs,
-                RESOURCE_MANAGER_HDFS_CONTROLLER,
-                &resolved_product_image.app_version_label_value,
-                &rolegroup_ref.role,
-                &rolegroup_ref.role_group,
-            ))
-            .context(ObjectMetaSnafu)?
-            .with_label(
-                Label::try_from(("prometheus.io/scrape", "true"))
-                    .context(BuildPrometheusLabelSnafu)?,
-            )
             .with_annotations(
                 Annotations::try_from([
                     ("prometheus.io/path".to_owned(), "/prom".to_owned()),
