fix: Fix upgrade path 3.3.x -> 4.0.x (#539)

* fix: Fix upgrade path 3.3.x -> 4.0.x * changelog * changelog * reword warning * reword warning * quote bash args * noop Co-authored-by: Xenia Fischer <[email protected]> * Add udgrade test --------- Co-authored-by: Xenia Fischer <[email protected]>
stackabletech · Nov 15, 2024 · 523b904 · 523b904
1 parent e4d9e7f
commit 523b904
Show file tree

Hide file tree

Showing 15 changed files with 248 additions and 65 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -21,12 +21,14 @@ All notable changes to this project will be documented in this file.
 
 - BREAKING: The fields `connection` and `host` on `S3Connection` as well as `bucketName` on `S3Bucket`are now mandatory ([#518]).
 - An invalid `HiveCluster` doesn't cause the operator to stop functioning ([#523]).
+- Fix upgrade path from HMS `3.3.x` to `4.0.x`. Previously the schemaTool would try to re-create the database tables and would therefore fail. Starting with version `4.0.0` the schemaTool has the flag `-initOrUpgradeSchema`, which we use to resolve that problem ([#539]).
 
 [#505]: https://github.com/stackabletech/hive-operator/pull/505
 [#508]: https://github.com/stackabletech/hive-operator/pull/508
 [#518]: https://github.com/stackabletech/hive-operator/pull/518
 [#522]: https://github.com/stackabletech/hive-operator/pull/522
 [#523]: https://github.com/stackabletech/hive-operator/pull/523
+[#539]: https://github.com/stackabletech/hive-operator/pull/539
 
 ## [24.7.0] - 2024-07-24
 

diff --git a/rust/crd/src/lib.rs b/rust/crd/src/lib.rs
@@ -332,7 +332,6 @@ impl MetaStoreConfig {
     pub const CONNECTION_PASSWORD: &'static str = "javax.jdo.option.ConnectionPassword";
     pub const METASTORE_METRICS_ENABLED: &'static str = "hive.metastore.metrics.enabled";
     pub const METASTORE_WAREHOUSE_DIR: &'static str = "hive.metastore.warehouse.dir";
-    pub const DB_TYPE_CLI: &'static str = "dbType";
     // S3
     pub const S3_ENDPOINT: &'static str = "fs.s3a.endpoint";
     pub const S3_ACCESS_KEY: &'static str = "fs.s3a.access.key";
@@ -406,12 +405,6 @@ pub enum DbType {
     Mssql,
 }
 
-impl Default for DbType {
-    fn default() -> Self {
-        Self::Derby
-    }
-}
-
 impl DbType {
     pub fn get_jdbc_driver_class(&self) -> &str {
         match self {
@@ -425,7 +418,7 @@ impl DbType {
 }
 
 /// Database connection specification for the metadata database.
-#[derive(Clone, Debug, Default, Deserialize, Eq, Hash, JsonSchema, PartialEq, Serialize)]
+#[derive(Clone, Debug, Deserialize, Eq, Hash, JsonSchema, PartialEq, Serialize)]
 #[serde(rename_all = "camelCase")]
 pub struct DatabaseConnectionSpec {
     /// A connection string for the database. For example:
@@ -469,15 +462,10 @@ impl Configuration for MetaStoreConfigFragment {
 
     fn compute_cli(
         &self,
-        hive: &Self::Configurable,
+        _hive: &Self::Configurable,
         _role_name: &str,
     ) -> Result<BTreeMap<String, Option<String>>, product_config_utils::Error> {
-        let mut result = BTreeMap::new();
-        result.insert(
-            MetaStoreConfig::DB_TYPE_CLI.to_string(),
-            Some(hive.spec.cluster_config.database.db_type.to_string()),
-        );
-        Ok(result)
+        Ok(BTreeMap::new())
     }
 
     fn compute_files(
@@ -511,14 +499,7 @@ impl Configuration for MetaStoreConfigFragment {
                 );
                 result.insert(
                     MetaStoreConfig::CONNECTION_DRIVER_NAME.to_string(),
-                    Some(
-                        hive.spec
-                            .cluster_config
-                            .database
-                            .db_type
-                            .get_jdbc_driver_class()
-                            .to_string(),
-                    ),
+                    Some(hive.db_type().get_jdbc_driver_class().to_string()),
                 );
 
                 result.insert(
@@ -657,6 +638,10 @@ impl HiveCluster {
             .map(|k| k.secret_class.clone())
     }
 
+    pub fn db_type(&self) -> &DbType {
+        &self.spec.cluster_config.database.db_type
+    }
+
     /// Retrieve and merge resource configs for role and role groups
     pub fn merged_config(
         &self,

diff --git a/rust/operator-binary/src/controller.rs b/rust/operator-binary/src/controller.rs
@@ -3,7 +3,6 @@ use std::{
     borrow::Cow,
     collections::{BTreeMap, HashMap},
     hash::Hasher,
-    str::FromStr,
     sync::Arc,
 };
 
@@ -16,10 +15,10 @@ use product_config::{
 };
 use snafu::{OptionExt, ResultExt, Snafu};
 use stackable_hive_crd::{
-    Container, DbType, HiveCluster, HiveClusterStatus, HiveRole, MetaStoreConfig, APP_NAME,
-    CORE_SITE_XML, DB_PASSWORD_ENV, DB_USERNAME_ENV, HADOOP_HEAPSIZE, HIVE_ENV_SH, HIVE_PORT,
-    HIVE_PORT_NAME, HIVE_SITE_XML, JVM_HEAP_FACTOR, JVM_SECURITY_PROPERTIES_FILE, METRICS_PORT,
-    METRICS_PORT_NAME, STACKABLE_CONFIG_DIR, STACKABLE_CONFIG_DIR_NAME, STACKABLE_CONFIG_MOUNT_DIR,
+    Container, HiveCluster, HiveClusterStatus, HiveRole, MetaStoreConfig, APP_NAME, CORE_SITE_XML,
+    DB_PASSWORD_ENV, DB_USERNAME_ENV, HADOOP_HEAPSIZE, HIVE_ENV_SH, HIVE_PORT, HIVE_PORT_NAME,
+    HIVE_SITE_XML, JVM_HEAP_FACTOR, 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,
 };
@@ -188,12 +187,6 @@ pub enum Error {
         source: stackable_operator::client::Error,
     },
 
-    #[snafu(display("failed to parse db type {db_type}"))]
-    InvalidDbType {
-        source: strum::ParseError,
-        db_type: String,
-    },
-
     #[snafu(display("failed to configure S3 connection"))]
     ConfigureS3 { source: S3Error },
 
@@ -828,36 +821,25 @@ fn build_metastore_rolegroup_statefulset(
         .rolegroup(rolegroup_ref)
         .context(InternalOperatorSnafu)?;
 
-    let mut db_type: Option<DbType> = None;
     let mut container_builder =
         ContainerBuilder::new(APP_NAME).context(FailedToCreateHiveContainerSnafu {
             name: APP_NAME.to_string(),
         })?;
 
     for (property_name_kind, config) in metastore_config {
-        match property_name_kind {
-            PropertyNameKind::Env => {
-                // overrides
-                for (property_name, property_value) in config {
-                    if property_name.is_empty() {
-                        warn!("Received empty property_name for ENV... skipping");
-                        continue;
-                    }
-                    container_builder.add_env_var(property_name, property_value);
-                }
-            }
-            PropertyNameKind::Cli => {
-                for (property_name, property_value) in config {
-                    if property_name == MetaStoreConfig::DB_TYPE_CLI {
-                        db_type = Some(DbType::from_str(property_value).with_context(|_| {
-                            InvalidDbTypeSnafu {
-                                db_type: property_value.to_string(),
-                            }
-                        })?);
-                    }
+        if property_name_kind == &PropertyNameKind::Env {
+            // overrides
+            for (property_name, property_value) in config {
+                if property_name.is_empty() {
+                    warn!(
+                        property_name,
+                        property_value,
+                        "The env variable had an empty name, not adding it to the container"
+                    );
+                    continue;
                 }
+                container_builder.add_env_var(property_name, property_value);
             }
-            _ => {}
         }
     }
 
@@ -894,6 +876,26 @@ fn build_metastore_rolegroup_statefulset(
         }
     }
 
+    let db_type = hive.db_type();
+    let start_command = if resolved_product_image.product_version.starts_with("3.") {
+        // The schematool version in 3.1.x does *not* support the `-initOrUpgradeSchema` flag yet, so we can not use that.
+        // As we *only* support HMS 3.1.x (or newer) since SDP release 23.11, we can safely assume we are always coming
+        // from an existing 3.1.x installation. There is no need to upgrade the schema, we can just check if the schema
+        // is already there and create it if it isn't.
+        // The script `bin/start-metastore` is buggy (e.g. around version upgrades), but it's sufficient for that job :)
+        //
+        // TODO: Once we drop support for HMS 3.1.x we can remove this condition and very likely get rid of the
+        // "bin/start-metastore" script.
+        format!("bin/start-metastore --config {STACKABLE_CONFIG_DIR} --db-type {db_type} --hive-bin-dir bin &")
+    } else {
+        // schematool versions 4.0.x (and above) support the `-initOrUpgradeSchema`, which is exactly what we need :)
+        // Some docs for the schemaTool can be found here: https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=34835119
+        formatdoc! {"
+            bin/base --config \"{STACKABLE_CONFIG_DIR}\" --service schemaTool -dbType \"{db_type}\" -initOrUpgradeSchema
+            bin/base --config \"{STACKABLE_CONFIG_DIR}\" --service metastore &
+        "}
+    };
+
     let container_builder = container_builder
         .image_from_product_image(resolved_product_image)
         .command(vec![
@@ -905,23 +907,22 @@ fn build_metastore_rolegroup_statefulset(
         ])
         .args(build_container_command_args(
             hive,
-              formatdoc! {"
+            formatdoc! {"
             {kerberos_container_start_commands}
 
             {COMMON_BASH_TRAP_FUNCTIONS}
             {remove_vector_shutdown_file_command}
             prepare_signal_handlers
-            bin/start-metastore --config {STACKABLE_CONFIG_DIR} --db-type {db_type} --hive-bin-dir bin &
+            {start_command}
             wait_for_termination $!
             {create_vector_shutdown_file_command}
             ",
-            kerberos_container_start_commands = kerberos_container_start_commands(hive),
-            remove_vector_shutdown_file_command =
-                remove_vector_shutdown_file_command(STACKABLE_LOG_DIR),
-            create_vector_shutdown_file_command =
-                create_vector_shutdown_file_command(STACKABLE_LOG_DIR),
-                  db_type = &db_type.unwrap_or_default().to_string(),
-        },
+                kerberos_container_start_commands = kerberos_container_start_commands(hive),
+                remove_vector_shutdown_file_command =
+                    remove_vector_shutdown_file_command(STACKABLE_LOG_DIR),
+                create_vector_shutdown_file_command =
+                    create_vector_shutdown_file_command(STACKABLE_LOG_DIR),
+            },
             s3_connection,
         ))
         .add_volume_mount(STACKABLE_CONFIG_DIR_NAME, STACKABLE_CONFIG_DIR)

diff --git a/tests/templates/kuttl/upgrade/00-limit-range.yaml b/tests/templates/kuttl/upgrade/00-limit-range.yaml
@@ -0,0 +1,11 @@
+---
+apiVersion: v1
+kind: LimitRange
+metadata:
+  name: limit-request-ratio
+spec:
+  limits:
+    - type: "Container"
+      maxLimitRequestRatio:
+        cpu: 5
+        memory: 1
diff --git a/tests/templates/kuttl/upgrade/00-patch-ns.yaml.j2 b/tests/templates/kuttl/upgrade/00-patch-ns.yaml.j2
@@ -0,0 +1,9 @@
+{% if test_scenario['values']['openshift'] == 'true' %}
+# see https://github.com/stackabletech/issues/issues/566
+---
+apiVersion: kuttl.dev/v1beta1
+kind: TestStep
+commands:
+  - script: kubectl patch namespace $NAMESPACE -p '{"metadata":{"labels":{"pod-security.kubernetes.io/enforce":"privileged"}}}'
+    timeout: 120
+{% endif %}
diff --git a/tests/templates/kuttl/upgrade/10-assert.yaml.j2 b/tests/templates/kuttl/upgrade/10-assert.yaml.j2
@@ -0,0 +1,10 @@
+---
+apiVersion: kuttl.dev/v1beta1
+kind: TestAssert
+{% if lookup('env', 'VECTOR_AGGREGATOR') %}
+---
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: vector-aggregator-discovery
+{% endif %}
diff --git a/tests/templates/kuttl/upgrade/10-install-vector-aggregator-discovery-configmap.yaml.j2 b/tests/templates/kuttl/upgrade/10-install-vector-aggregator-discovery-configmap.yaml.j2
@@ -0,0 +1,9 @@
+{% if lookup('env', 'VECTOR_AGGREGATOR') %}
+---
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: vector-aggregator-discovery
+data:
+  ADDRESS: {{ lookup('env', 'VECTOR_AGGREGATOR') }}
+{% endif %}
diff --git a/tests/templates/kuttl/upgrade/20-assert.yaml b/tests/templates/kuttl/upgrade/20-assert.yaml
@@ -0,0 +1,19 @@
+---
+apiVersion: kuttl.dev/v1beta1
+kind: TestAssert
+timeout: 600
+---
+apiVersion: v1
+kind: Service
+metadata:
+  name: postgresql
+  labels:
+    app.kubernetes.io/name: postgresql
+---
+apiVersion: apps/v1
+kind: StatefulSet
+metadata:
+  name: postgresql
+status:
+  readyReplicas: 1
+  replicas: 1
diff --git a/tests/templates/kuttl/upgrade/20-install-postgres.yaml.j2 b/tests/templates/kuttl/upgrade/20-install-postgres.yaml.j2
@@ -0,0 +1,10 @@
+---
+apiVersion: kuttl.dev/v1beta1
+kind: TestStep
+commands:
+  - script: >-
+      helm install postgresql
+      --version={{ test_scenario['values']['postgres'] }}
+      --namespace $NAMESPACE
+      -f helm-bitnami-postgresql-values.yaml
+      --repo https://charts.bitnami.com/bitnami postgresql
diff --git a/tests/templates/kuttl/upgrade/30-assert.yaml.j2 b/tests/templates/kuttl/upgrade/30-assert.yaml.j2
@@ -0,0 +1,19 @@
+---
+apiVersion: kuttl.dev/v1beta1
+kind: TestAssert
+timeout: 600
+---
+apiVersion: apps/v1
+kind: StatefulSet
+metadata:
+  name: hive-metastore-default
+  labels:
+{% if test_scenario['values']['hive-old'].find(",") > 0 %}
+    # Yes, this *might* not work with custom images, I'm sorry!
+    app.kubernetes.io/version: "{{ test_scenario['values']['hive-old'].split(',')[0] }}-stackable0.0.0-dev"
+{% else %}
+    app.kubernetes.io/version: "{{ test_scenario['values']['hive-old'] }}-stackable0.0.0-dev"
+{% endif %}
+status:
+  readyReplicas: 1
+  replicas: 1
diff --git a/tests/templates/kuttl/upgrade/30-install-hive.yaml.j2 b/tests/templates/kuttl/upgrade/30-install-hive.yaml.j2
@@ -0,0 +1,35 @@
+---
+apiVersion: hive.stackable.tech/v1alpha1
+kind: HiveCluster
+metadata:
+  name: hive
+spec:
+  image:
+{% if test_scenario['values']['hive-old'].find(",") > 0 %}
+    custom: "{{ test_scenario['values']['hive-old'].split(',')[1] }}"
+    productVersion: "{{ test_scenario['values']['hive-old'].split(',')[0] }}"
+{% else %}
+    productVersion: "{{ test_scenario['values']['hive-old'] }}"
+{% endif %}
+    pullPolicy: IfNotPresent
+  clusterConfig:
+    database:
+      connString: jdbc:postgresql://postgresql:5432/hive
+      credentialsSecret: hive-credentials
+      dbType: postgres
+{% if lookup('env', 'VECTOR_AGGREGATOR') %}
+    vectorAggregatorConfigMapName: vector-aggregator-discovery
+{% endif %}
+  metastore:
+    roleGroups:
+      default:
+        replicas: 1
+---
+apiVersion: v1
+kind: Secret
+metadata:
+  name: hive-credentials
+type: Opaque
+stringData:
+  username: hive
+  password: hive
diff --git a/tests/templates/kuttl/upgrade/31-assert.yaml.j2 b/tests/templates/kuttl/upgrade/31-assert.yaml.j2
@@ -0,0 +1,19 @@
+---
+apiVersion: kuttl.dev/v1beta1
+kind: TestAssert
+timeout: 600
+---
+apiVersion: apps/v1
+kind: StatefulSet
+metadata:
+  name: hive-metastore-default
+  labels:
+{% if test_scenario['values']['hive-old'].find(",") > 0 %}
+    # Yes, this *might* not work with custom images, I'm sorry!
+    app.kubernetes.io/version: "{{ test_scenario['values']['hive-new'].split(',')[0] }}-stackable0.0.0-dev"
+{% else %}
+    app.kubernetes.io/version: "{{ test_scenario['values']['hive-new'] }}-stackable0.0.0-dev"
+{% endif %}
+status:
+  readyReplicas: 1
+  replicas: 1
diff --git a/tests/templates/kuttl/upgrade/31-upgrade-hive.yaml.j2 b/tests/templates/kuttl/upgrade/31-upgrade-hive.yaml.j2
@@ -0,0 +1,13 @@
+---
+apiVersion: hive.stackable.tech/v1alpha1
+kind: HiveCluster
+metadata:
+  name: hive
+spec:
+  image:
+{% if test_scenario['values']['hive-new'].find(",") > 0 %}
+    custom: "{{ test_scenario['values']['hive-new'].split(',')[1] }}"
+    productVersion: "{{ test_scenario['values']['hive-new'].split(',')[0] }}"
+{% else %}
+    productVersion: "{{ test_scenario['values']['hive-new'] }}"
+{% endif %}