refactor: switch to v2 JavaCommonConfig

Author: maltesander

Cargo.lock

@@ -3,7 +3,7 @@
 use snafu::{OptionExt, ResultExt, Snafu};
 use stackable_operator::{
     memory::{BinaryMultiple, MemoryQuantity},
-    role_utils::{self, JvmArgumentOverrides},
+    v2::jvm_argument_overrides::JvmArgumentOverrides,
 };
 
 use crate::crd::{
@@ -36,18 +36,14 @@ pub enum Error {
         "Trino version {version} is not supported. Only specific versions are handled due to version specific JVM configuration generation"
     ))]
     TrinoVersionNotSupported { version: u16 },
-
-    #[snafu(display("failed to merge jvm argument overrides"))]
-    MergeJvmArgumentOverrides { source: role_utils::Error },
 }
 
 // Currently works for all supported versions (as of 2024-09-04) but maybe be changed
 // in the future depending on the role and version.
 pub fn jvm_config(
     product_version: u16,
     merged_config: &v1alpha1::TrinoConfig,
-    role_jvm_argument_overrides: &JvmArgumentOverrides,
-    role_group_jvm_argument_overrides: &JvmArgumentOverrides,
+    jvm_argument_overrides: &JvmArgumentOverrides,
 ) -> Result<String, Error> {
     let memory_unit = BinaryMultiple::Mebi;
     let heap_size = MemoryQuantity::try_from(
@@ -92,24 +88,10 @@ pub fn jvm_config(
 
     jvm_args.push("# Arguments from jvmArgumentOverrides".to_owned());
 
-    let operator_generated = JvmArgumentOverrides::new_with_only_additions(jvm_args);
-
-    // Merge order mirrors `Role::get_merged_jvm_argument_overrides`:
-    // 1. operator-generated args are layered on top of the role-level overrides,
-    // 2. the role-group-level overrides are applied last.
-    // Note that this is not a purely additive merge, hence the unusual order.
-    let mut from_role = role_jvm_argument_overrides.clone();
-    from_role
-        .try_merge(&operator_generated)
-        .context(MergeJvmArgumentOverridesSnafu)?;
-    let mut merged_jvm_argument_overrides = role_group_jvm_argument_overrides.clone();
-    merged_jvm_argument_overrides
-        .try_merge(&from_role)
-        .context(MergeJvmArgumentOverridesSnafu)?;
-
-    Ok(merged_jvm_argument_overrides
-        .effective_jvm_config_after_merging()
-        .join("\n"))
+    // `jvm_argument_overrides` already carries the merged role + role-group overrides (merged by
+    // `with_validated_config` in the validate step). Applying them to the operator-generated args
+    // layers the overrides on top, in the order: operator-generated <- role <- role group.
+    Ok(jvm_argument_overrides.apply_to(jvm_args).join("\n"))
 }
 
 /// For tests we don't actually look at the Trino version, and return a single "representative"
@@ -269,29 +251,22 @@ mod tests {
         let trino: v1alpha1::TrinoCluster =
             serde_yaml::from_str(trino_cluster).expect("illegal test input");
 
-        let role = TrinoRole::Coordinator;
-        let rolegroup_ref = role.rolegroup_ref(&trino, "default");
-        let merged_config = trino.merged_config(&role, &rolegroup_ref, &[]).unwrap();
-        let coordinators = trino.role(&role).unwrap();
-
-        let role_jvm_argument_overrides = coordinators
-            .config
-            .product_specific_common_config
-            .jvm_argument_overrides
-            .clone();
-        let role_group_jvm_argument_overrides = coordinators.role_groups["default"]
-            .config
-            .product_specific_common_config
-            .jvm_argument_overrides
-            .clone();
+        // Merge + validate via the shared production path; the role + role-group
+        // `jvmArgumentOverrides` end up merged in `product_specific_common_config`.
+        let rg = crate::controller::validate::merged_role_group_config(
+            &trino,
+            &TrinoRole::Coordinator,
+            "default",
+            &[],
+        );
 
-        let product_version = trino.spec.image.product_version();
+        let product_version =
+            u16::from_str(trino.spec.image.product_version()).expect("trino version as u16");
 
         jvm_config(
-            u16::from_str(product_version).expect("trino version as u16"),
-            &merged_config,
-            &role_jvm_argument_overrides,
-            &role_group_jvm_argument_overrides,
+            product_version,
+            &rg.config,
+            &rg.product_specific_common_config.jvm_argument_overrides,
         )
         .unwrap()
     }

Cargo.nix

@@ -54,9 +54,6 @@ pub enum Error {
         source: stackable_operator::builder::meta::Error,
     },
 
-    #[snafu(display("missing JVM argument overrides for role {role}"))]
-    MissingRoleJvmArgumentOverrides { role: String },
-
     #[snafu(display("failed to build jvm.config"))]
     BuildJvmConfig { source: crate::config::jvm::Error },
 }
@@ -169,18 +166,11 @@ pub fn build_rolegroup_config_map(
         );
     }
 
-    // 8. jvm.config.
-    let role_jvm_argument_overrides =
-        cluster
-            .role_jvm_argument_overrides
-            .get(role)
-            .with_context(|| MissingRoleJvmArgumentOverridesSnafu {
-                role: role.to_string(),
-            })?;
+    // 8. jvm.config. The role + role-group `jvmArgumentOverrides` were already merged in the
+    // validate step and are carried by `product_specific_common_config`.
     let jvm_config = jvm::jvm_config(
         cluster.product_version,
         &rg.config,
-        role_jvm_argument_overrides,
         &rg.product_specific_common_config.jvm_argument_overrides,
     )
     .context(BuildJvmConfigSnafu)?;

rust/operator-binary/src/config/jvm.rs

@@ -6,7 +6,6 @@ use std::collections::BTreeMap;
 use stackable_operator::{
     commons::product_image_selection::ResolvedProductImage,
     kube::{Resource, api::ObjectMeta},
-    role_utils::JvmArgumentOverrides,
     v2::types::{
         kubernetes::{NamespaceName, Uid},
         operator::ClusterName,
@@ -65,10 +64,6 @@ pub struct ValidatedCluster {
     pub product_version: u16,
     pub cluster_config: ValidatedClusterConfig,
     pub role_group_configs: BTreeMap<TrinoRole, BTreeMap<RoleGroupName, TrinoRoleGroupConfig>>,
-    /// Role-level JVM argument overrides per role. Required to render `jvm.config` in the build
-    /// step without access to the raw [`v1alpha1::TrinoCluster`] (the role-group level overrides
-    /// are carried by [`TrinoRoleGroupConfig::product_specific_common_config`]).
-    pub role_jvm_argument_overrides: BTreeMap<TrinoRole, JvmArgumentOverrides>,
 }
 
 impl ValidatedCluster {
@@ -81,7 +76,6 @@ impl ValidatedCluster {
         product_version: u16,
         cluster_config: ValidatedClusterConfig,
         role_group_configs: BTreeMap<TrinoRole, BTreeMap<RoleGroupName, TrinoRoleGroupConfig>>,
-        role_jvm_argument_overrides: BTreeMap<TrinoRole, JvmArgumentOverrides>,
     ) -> Self {
         Self {
             metadata: ObjectMeta {
@@ -97,7 +91,6 @@ impl ValidatedCluster {
             product_version,
             cluster_config,
             role_group_configs,
-            role_jvm_argument_overrides,
         }
     }
 }
@@ -130,3 +123,58 @@ impl Resource for ValidatedCluster {
         &mut self.metadata
     }
 }
+
+/// A minimal, valid TrinoCluster spec shared across unit tests.
+#[cfg(test)]
+pub(crate) const MINIMAL_TRINO_YAML: &str = r#"
+    apiVersion: trino.stackable.tech/v1alpha1
+    kind: TrinoCluster
+    metadata:
+      name: simple-trino
+      namespace: default
+      uid: "e6ac237d-a6d4-43a1-8135-f36506110912"
+    spec:
+      image:
+        productVersion: "479"
+      clusterConfig:
+        catalogLabelSelector: {}
+      coordinators:
+        roleGroups:
+          default:
+            replicas: 1
+      workers:
+        roleGroups:
+          default:
+            replicas: 1
+    "#;
+
+/// Parses [`MINIMAL_TRINO_YAML`] into a [`v1alpha1::TrinoCluster`].
+#[cfg(test)]
+pub(crate) fn minimal_trino() -> v1alpha1::TrinoCluster {
+    serde_yaml::from_str(MINIMAL_TRINO_YAML).expect("invalid test TrinoCluster YAML")
+}
+
+/// The validated [`MINIMAL_TRINO_YAML`] cluster with empty dereferenced inputs. The common test
+/// fixture for build-step unit tests.
+#[cfg(test)]
+pub(crate) fn validated_cluster() -> ValidatedCluster {
+    use stackable_operator::cli::OperatorEnvironmentOptions;
+
+    use crate::controller::dereference::DereferencedObjects;
+
+    let derefs = DereferencedObjects {
+        resolved_authentication_classes: Vec::new(),
+        catalog_definitions: Vec::new(),
+        catalogs: Vec::new(),
+        trino_opa_config: None,
+        resolved_fte_config: None,
+        resolved_client_protocol_config: None,
+    };
+    let operator_env = OperatorEnvironmentOptions {
+        operator_namespace: "stackable-operators".to_string(),
+        operator_service_name: "trino-operator".to_string(),
+        image_repository: "oci.example.org".to_string(),
+    };
+
+    validate::validate(&minimal_trino(), &derefs, &operator_env).expect("validate should succeed")
+}