fix: Remove the hive-env.sh

sbernauer · sbernauer · commit b3a4e161e741 · 2025-02-19T11:01:09.000+01:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -14,6 +14,15 @@ All notable changes to this project will be documented in this file.
 
 - Default to OCI for image metadata and product image selection ([#561]).
 
+### Fixed
+
+- BREAKING: Remove the `hive-env.sh` config file, as e.g. setting `HADOOP_OPTS` in there had absolutely no effect.
+  This is considered a fix, as users expected the envs to be used, but they haven't.
+  Users should use `envOverrides` instead, which are actually working ([#572]).
+- BREAKING: The env variable `HADOOP_HEAPSIZE` was previously put in `hive-env.sh` and very likely had no effect.
+  It is now passed as env variable, thus working.
+  This might impact your stacklet as the heap size setting now actually has an effect ([#572]).
+
 [#554]: https://github.com/stackabletech/hive-operator/pull/554
 [#560]: https://github.com/stackabletech/hive-operator/pull/560
 [#561]: https://github.com/stackabletech/hive-operator/pull/561
diff --git a/rust/operator-binary/src/controller.rs b/rust/operator-binary/src/controller.rs
@@ -37,9 +37,8 @@ use stackable_operator::{
         api::{
             apps::v1::{StatefulSet, StatefulSetSpec},
             core::v1::{
-                ConfigMap, ConfigMapVolumeSource, EmptyDirVolumeSource, EnvVar, EnvVarSource,
-                Probe, SecretKeySelector, Service, ServicePort, ServiceSpec, TCPSocketAction,
-                Volume,
+                ConfigMap, ConfigMapVolumeSource, EmptyDirVolumeSource, Probe, Service,
+                ServicePort, ServiceSpec, TCPSocketAction, Volume,
             },
         },
         apimachinery::pkg::{
@@ -66,7 +65,7 @@ use stackable_operator::{
             CustomContainerLogConfig,
         },
     },
-    role_utils::{GenericRoleConfig, JavaCommonConfig, Role, RoleGroupRef},
+    role_utils::{GenericRoleConfig, RoleGroupRef},
     status::condition::{
         compute_conditions, operations::ClusterOperationsConditionBuilder,
         statefulset::StatefulSetConditionBuilder,
@@ -81,11 +80,10 @@ use crate::{
     command::build_container_command_args,
     config::jvm::{construct_hadoop_heapsize_env, construct_non_heap_jvm_args},
     crd::{
-        v1alpha1, Container, HiveClusterStatus, HiveRole, MetaStoreConfig, MetaStoreConfigFragment,
-        APP_NAME, CORE_SITE_XML, DB_PASSWORD_ENV, DB_USERNAME_ENV, HIVE_ENV_SH, HIVE_PORT,
-        HIVE_PORT_NAME, HIVE_SITE_XML, JVM_SECURITY_PROPERTIES_FILE, METRICS_PORT,
-        METRICS_PORT_NAME, STACKABLE_CONFIG_DIR, STACKABLE_CONFIG_DIR_NAME,
-        STACKABLE_CONFIG_MOUNT_DIR, STACKABLE_CONFIG_MOUNT_DIR_NAME,
+        v1alpha1, Container, HiveClusterStatus, HiveRole, MetaStoreConfig, APP_NAME, CORE_SITE_XML,
+        DB_PASSWORD_ENV, DB_USERNAME_ENV, HIVE_PORT, HIVE_PORT_NAME, HIVE_SITE_XML,
+        JVM_SECURITY_PROPERTIES_FILE, METRICS_PORT, METRICS_PORT_NAME, STACKABLE_CONFIG_DIR,
+        STACKABLE_CONFIG_DIR_NAME, STACKABLE_CONFIG_MOUNT_DIR, STACKABLE_CONFIG_MOUNT_DIR_NAME,
         STACKABLE_LOG_CONFIG_MOUNT_DIR, STACKABLE_LOG_CONFIG_MOUNT_DIR_NAME, STACKABLE_LOG_DIR,
         STACKABLE_LOG_DIR_NAME,
     },
@@ -387,7 +385,6 @@ pub async fn reconcile_hive(
                         PropertyNameKind::Env,
                         PropertyNameKind::Cli,
                         PropertyNameKind::File(HIVE_SITE_XML.to_string()),
-                        PropertyNameKind::File(HIVE_ENV_SH.to_string()),
                         PropertyNameKind::File(JVM_SECURITY_PROPERTIES_FILE.to_string()),
                     ],
                     role.clone(),
@@ -460,7 +457,6 @@ pub async fn reconcile_hive(
             hive,
             &hive_namespace,
             &resolved_product_image,
-            role,
             &rolegroup,
             rolegroup_config,
             s3_connection_spec.as_ref(),
@@ -604,7 +600,6 @@ fn build_metastore_rolegroup_config_map(
     hive: &v1alpha1::HiveCluster,
     hive_namespace: &str,
     resolved_product_image: &ResolvedProductImage,
-    role: &Role<MetaStoreConfigFragment, GenericRoleConfig, JavaCommonConfig>,
     rolegroup: &RoleGroupRef<v1alpha1::HiveCluster>,
     role_group_config: &HashMap<PropertyNameKind, BTreeMap<String, String>>,
     s3_connection_spec: Option<&S3ConnectionSpec>,
@@ -613,35 +608,9 @@ fn build_metastore_rolegroup_config_map(
     cluster_info: &KubernetesClusterInfo,
 ) -> Result<ConfigMap> {
     let mut hive_site_data = String::new();
-    let mut hive_env_data = String::new();
 
     for (property_name_kind, config) in role_group_config {
         match property_name_kind {
-            PropertyNameKind::File(file_name) if file_name == HIVE_ENV_SH => {
-                let mut data = BTreeMap::from([
-                    (
-                        "HADOOP_HEAPSIZE".to_string(),
-                        construct_hadoop_heapsize_env(merged_config)
-                            .context(ConstructJvmArgumentsSnafu)?,
-                    ),
-                    (
-                        "HADOOP_OPTS".to_string(),
-                        construct_non_heap_jvm_args(hive, role, &rolegroup.role_group)
-                            .context(ConstructJvmArgumentsSnafu)?,
-                    ),
-                ]);
-
-                // other properties /  overrides
-                for (property_name, property_value) in config {
-                    data.insert(property_name.to_string(), property_value.to_string());
-                }
-
-                hive_env_data = data
-                    .into_iter()
-                    .map(|(key, value)| format!("export {key}=\"{value}\""))
-                    .collect::<Vec<String>>()
-                    .join("\n");
-            }
             PropertyNameKind::File(file_name) if file_name == HIVE_SITE_XML => {
                 let mut data = BTreeMap::new();
 
@@ -724,7 +693,6 @@ fn build_metastore_rolegroup_config_map(
                 .build(),
         )
         .add_data(HIVE_SITE_XML, hive_site_data)
-        .add_data(HIVE_ENV_SH, hive_env_data)
         .add_data(
             JVM_SECURITY_PROPERTIES_FILE,
             to_java_properties_string(jvm_sec_props.iter()).with_context(|_| {
@@ -827,6 +795,27 @@ fn build_metastore_rolegroup_statefulset(
             name: APP_NAME.to_string(),
         })?;
 
+    let credentials_secret_name = hive.spec.cluster_config.database.credentials_secret.clone();
+
+    container_builder
+        // load database credentials to environment variables: these will be used to replace
+        // the placeholders in hive-site.xml so that the operator does not "touch" the secret.
+        .add_env_var_from_secret(DB_USERNAME_ENV, &credentials_secret_name, "username")
+        .add_env_var_from_secret(DB_PASSWORD_ENV, &credentials_secret_name, "password")
+        .add_env_var(
+            "HADOOP_HEAPSIZE",
+            construct_hadoop_heapsize_env(merged_config).context(ConstructJvmArgumentsSnafu)?,
+        )
+        .add_env_var(
+            "HADOOP_OPTS",
+            construct_non_heap_jvm_args(hive, role, &rolegroup_ref.role_group)
+                .context(ConstructJvmArgumentsSnafu)?,
+        )
+        .add_env_var(
+            "CONTAINERDEBUG_LOG_DIRECTORY",
+            format!("{STACKABLE_LOG_DIR}/containerdebug"),
+        );
+
     for (property_name_kind, config) in metastore_config {
         if property_name_kind == &PropertyNameKind::Env {
             // overrides
@@ -844,21 +833,6 @@ fn build_metastore_rolegroup_statefulset(
         }
     }
 
-    // load database credentials to environment variables: these will be used to replace
-    // the placeholders in hive-site.xml so that the operator does not "touch" the secret.
-    let credentials_secret_name = hive.spec.cluster_config.database.credentials_secret.clone();
-
-    container_builder.add_env_vars(vec![
-        env_var_from_secret(DB_USERNAME_ENV, &credentials_secret_name, "username"),
-        env_var_from_secret(DB_PASSWORD_ENV, &credentials_secret_name, "password"),
-        // Needed for the `containerdebug` process to log it's tracing information to.
-        EnvVar {
-            name: "CONTAINERDEBUG_LOG_DIRECTORY".to_string(),
-            value: Some(format!("{STACKABLE_LOG_DIR}/containerdebug")),
-            value_from: None,
-        },
-    ]);
-
     let mut pod_builder = PodBuilder::new();
 
     if let Some(hdfs) = &hive.spec.cluster_config.hdfs {
@@ -1127,21 +1101,6 @@ fn build_metastore_rolegroup_statefulset(
     })
 }
 
-fn env_var_from_secret(var_name: &str, secret: &str, secret_key: &str) -> EnvVar {
-    EnvVar {
-        name: String::from(var_name),
-        value_from: Some(EnvVarSource {
-            secret_key_ref: Some(SecretKeySelector {
-                name: String::from(secret),
-                key: String::from(secret_key),
-                ..Default::default()
-            }),
-            ..Default::default()
-        }),
-        ..Default::default()
-    }
-}
-
 pub fn error_policy(
     _obj: Arc<DeserializeGuard<v1alpha1::HiveCluster>>,
     error: &Error,
diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs
@@ -51,7 +51,6 @@ pub const STACKABLE_LOG_CONFIG_MOUNT_DIR_NAME: &str = "log-config-mount";
 // Config file names
 pub const CORE_SITE_XML: &str = "core-site.xml";
 pub const HIVE_SITE_XML: &str = "hive-site.xml";
-pub const HIVE_ENV_SH: &str = "hive-env.sh";
 pub const HIVE_METASTORE_LOG4J2_PROPERTIES: &str = "metastore-log4j2.properties";
 pub const JVM_SECURITY_PROPERTIES_FILE: &str = "security.properties";
 
@@ -590,39 +589,35 @@ impl Configuration for MetaStoreConfigFragment {
     ) -> Result<BTreeMap<String, Option<String>>, product_config_utils::Error> {
         let mut result = BTreeMap::new();
 
-        match file {
-            HIVE_SITE_XML => {
-                if let Some(warehouse_dir) = &self.warehouse_dir {
-                    result.insert(
-                        MetaStoreConfig::METASTORE_WAREHOUSE_DIR.to_string(),
-                        Some(warehouse_dir.to_string()),
-                    );
-                }
+        if file == HIVE_SITE_XML {
+            if let Some(warehouse_dir) = &self.warehouse_dir {
                 result.insert(
-                    MetaStoreConfig::CONNECTION_URL.to_string(),
-                    Some(hive.spec.cluster_config.database.conn_string.clone()),
-                );
-                // use a placeholder that will be replaced in the start command (also for the password)
-                result.insert(
-                    MetaStoreConfig::CONNECTION_USER_NAME.to_string(),
-                    Some(DB_USERNAME_PLACEHOLDER.into()),
-                );
-                result.insert(
-                    MetaStoreConfig::CONNECTION_PASSWORD.to_string(),
-                    Some(DB_PASSWORD_PLACEHOLDER.into()),
-                );
-                result.insert(
-                    MetaStoreConfig::CONNECTION_DRIVER_NAME.to_string(),
-                    Some(hive.db_type().get_jdbc_driver_class().to_string()),
-                );
-
-                result.insert(
-                    MetaStoreConfig::METASTORE_METRICS_ENABLED.to_string(),
-                    Some("true".to_string()),
+                    MetaStoreConfig::METASTORE_WAREHOUSE_DIR.to_string(),
+                    Some(warehouse_dir.to_string()),
                 );
             }
-            HIVE_ENV_SH => {}
-            _ => {}
+            result.insert(
+                MetaStoreConfig::CONNECTION_URL.to_string(),
+                Some(hive.spec.cluster_config.database.conn_string.clone()),
+            );
+            // use a placeholder that will be replaced in the start command (also for the password)
+            result.insert(
+                MetaStoreConfig::CONNECTION_USER_NAME.to_string(),
+                Some(DB_USERNAME_PLACEHOLDER.into()),
+            );
+            result.insert(
+                MetaStoreConfig::CONNECTION_PASSWORD.to_string(),
+                Some(DB_PASSWORD_PLACEHOLDER.into()),
+            );
+            result.insert(
+                MetaStoreConfig::CONNECTION_DRIVER_NAME.to_string(),
+                Some(hive.db_type().get_jdbc_driver_class().to_string()),
+            );
+
+            result.insert(
+                MetaStoreConfig::METASTORE_METRICS_ENABLED.to_string(),
+                Some("true".to_string()),
+            );
         }
 
         Ok(result)
diff --git a/tests/templates/kuttl/smoke/60-install-hive.yaml.j2 b/tests/templates/kuttl/smoke/60-install-hive.yaml.j2
@@ -30,21 +30,20 @@ spec:
       COMMON_VAR: role-value # overridden by role group below
       ROLE_VAR: role-value   # only defined here at role level
     configOverrides:
-      hive-env.sh:
-        HADOOP_HEAPSIZE: "512" # also set by the operator
-        COMMON_VAR: role-value # overridden by role group below
-        ROLE_VAR: role-value   # only defined here at role level
+      hive-site.xml:
+        hive.metastore.warehouse.dir: "/stackable/warehouse/override" # Also set by the operator
+        common-var: role-value # Overridden by role group below
+        role-var: role-value # Only defined here at role level
     roleGroups:
       default:
         replicas: 1
         envOverrides:
           COMMON_VAR: group-value # overrides role value
           GROUP_VAR: group-value # only defined here at group level
         configOverrides:
-          hive-env.sh:
-            COMMON_VAR: group-value # overridden by role group below
-            GROUP_VAR: group-value # only defined here at group level
-
+          hive-site.xml:
+            common-var: group-value
+            group-var: group-value
 ---
 apiVersion: s3.stackable.tech/v1alpha1
 kind: S3Connection
diff --git a/tests/templates/kuttl/smoke/61-assert.yaml b/tests/templates/kuttl/smoke/61-assert.yaml
@@ -14,7 +14,7 @@ commands:
   # Test configOverrides
   #
   - script: |
-      kubectl -n $NAMESPACE get cm hive-metastore-default -o yaml | yq -e '.data."hive-env.sh"' | grep "export ROLE_VAR=role-value"
-      kubectl -n $NAMESPACE get cm hive-metastore-default -o yaml | yq -e '.data."hive-env.sh"' | grep "export GROUP_VAR=group-value"
-      kubectl -n $NAMESPACE get cm hive-metastore-default -o yaml | yq -e '.data."hive-env.sh"' | grep "export COMMON_VAR=group-value"
-      kubectl -n $NAMESPACE get cm hive-metastore-default -o yaml | yq -e '.data."hive-env.sh"' | grep "export HADOOP_HEAPSIZE=512"
+      kubectl -n $NAMESPACE get cm hive-metastore-default -o yaml | yq -e '.data."hive-site.xml"' | yq -p=xml '.configuration.property[] | select(.name == "hive.metastore.warehouse.dir") | .value' | grep -qx "/stackable/warehouse/override"
+      kubectl -n $NAMESPACE get cm hive-metastore-default -o yaml | yq -e '.data."hive-site.xml"' | yq -p=xml '.configuration.property[] | select(.name == "role-var") | .value' | grep -qx "role-value"
+      kubectl -n $NAMESPACE get cm hive-metastore-default -o yaml | yq -e '.data."hive-site.xml"' | yq -p=xml '.configuration.property[] | select(.name == "group-var") | .value' | grep -qx "group-value"
+      kubectl -n $NAMESPACE get cm hive-metastore-default -o yaml | yq -e '.data."hive-site.xml"' | yq -p=xml '.configuration.property[] | select(.name == "common-var") | .value' | grep -qx "group-value"
