diff --git a/CHANGELOG.md b/CHANGELOG.md index b1098b35..b0154b36 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,9 @@ All notable changes to this project will be documented in this file. delete each coordinator StatefulSet so that the operator immediately recreates it with the new labels ([#932]). - Make operations infallible where dependent on static inputs ([#939], [#943]). +- Internal operator refactoring: the validated cluster carries each role's configuration in its own + typed fields instead of maps keyed by role, and the coordinator's role config is no longer + converted to the worker's shape and its listener class recovered afterwards ([#945]). ### Fixed @@ -38,6 +41,10 @@ All notable changes to this project will be documented in this file. Trino's native S3 file system takes the transport from the endpoint scheme, so requiring TLS was never necessary ([#928]). - The operator now watches all resources that it creates and early-exits the reconcile action when the cluster is marked for deletion ([#934]). +- A coordinator role group that does not set `replicas` is now counted as one replica instead of zero + when predicting the coordinator pods. Kubernetes runs a single pod for a `StatefulSet` with + `replicas: null`, but counting it as zero left `discovery.uri` out of `config.properties` + altogether, so no pod could find the coordinator ([#945]). [#909]: https://github.com/stackabletech/trino-operator/pull/909 [#913]: https://github.com/stackabletech/trino-operator/pull/913 @@ -49,6 +56,7 @@ All notable changes to this project will be documented in this file. [#934]: https://github.com/stackabletech/trino-operator/pull/934 [#939]: https://github.com/stackabletech/trino-operator/pull/939 [#943]: https://github.com/stackabletech/trino-operator/pull/943 +[#945]: https://github.com/stackabletech/trino-operator/pull/945 ## [26.7.0] - 2026-07-21 diff --git a/rust/operator-binary/src/controller/build.rs b/rust/operator-binary/src/controller/build.rs index d6f8b1c5..3046ce33 100644 --- a/rust/operator-binary/src/controller/build.rs +++ b/rust/operator-binary/src/controller/build.rs @@ -29,6 +29,7 @@ use crate::{ statefulset, }, }, + crd::TrinoRole, trino_controller::{CONTROLLER_NAME, OPERATOR_NAME, PRODUCT_NAME}, }; @@ -68,7 +69,16 @@ pub fn build( let mut config_maps = vec![]; let mut pod_disruption_budgets = vec![]; - for (role, role_group_configs) in &cluster.role_group_configs { + // One entry per role, in `TrinoRole` declaration order. Each role's groups come from its own + // field, so the role and its groups cannot be paired up wrongly here. + for (role, role_group_configs) in [ + ( + TrinoRole::Coordinator, + &cluster.coordinator_role_group_configs, + ), + (TrinoRole::Worker, &cluster.worker_role_group_configs), + ] { + let role: &TrinoRole = &role; for (role_group_name, role_group_config) in role_group_configs { let selector = role_group_selector(cluster, role, role_group_name); @@ -115,22 +125,18 @@ pub fn build( ); } - let Some(role_config) = cluster.role_config(role) else { - continue; - }; - - if let Some(listener_class) = &role_config.listener_class - && let Some(listener_group_name) = group_listener_name(cluster, role) - { - listeners.push(build_group_listener( - cluster, - role, - listener_class, - &listener_group_name, - )); - } + pod_disruption_budgets.extend(build_pdb(cluster.pdb(role), cluster, role)); + } - pod_disruption_budgets.extend(build_pdb(&role_config.pdb, cluster, role)); + // Only the coordinator has a group listener, so it is built once here rather than inside the + // role loop. + if let Some(listener_group_name) = group_listener_name(cluster, &TrinoRole::Coordinator) { + listeners.push(build_group_listener( + cluster, + &TrinoRole::Coordinator, + &cluster.coordinator_config.listener_class, + &listener_group_name, + )); } Ok(KubernetesResources { diff --git a/rust/operator-binary/src/controller/build/graceful_shutdown.rs b/rust/operator-binary/src/controller/build/graceful_shutdown.rs index 3f000bd2..1619aa7e 100644 --- a/rust/operator-binary/src/controller/build/graceful_shutdown.rs +++ b/rust/operator-binary/src/controller/build/graceful_shutdown.rs @@ -62,16 +62,13 @@ pub fn graceful_shutdown_config_properties( } } -/// Returns the minimal `gracefulShutdownTimeout` across all worker role-groups, read from the -/// validated [`ValidatedCluster::role_group_configs`]. +/// Returns the minimal `gracefulShutdownTimeout` across all worker role-groups fn min_worker_graceful_shutdown_timeout( cluster: &ValidatedCluster, ) -> stackable_operator::shared::time::Duration { cluster - .role_group_configs - .get(&TrinoRole::Worker) - .into_iter() - .flat_map(|groups| groups.values()) + .worker_role_group_configs + .values() .filter_map(|rg| rg.config.graceful_shutdown_timeout) .min() .unwrap_or(DEFAULT_WORKER_GRACEFUL_SHUTDOWN_TIMEOUT) @@ -309,7 +306,8 @@ mod tests { #[test] fn worker_termination_grace_period_adds_overhead_and_sets_pre_stop() { let cluster = validated_cluster_from_yaml(MINIMAL_TRINO_YAML); - let merged = &cluster.role_group_configs[&TrinoRole::Worker] + let merged = &cluster + .worker_role_group_configs .values() .next() .expect("the fixture defines a worker role group") @@ -348,7 +346,8 @@ mod tests { #[test] fn coordinator_termination_grace_period_has_no_overhead_or_pre_stop() { let cluster = validated_cluster_from_yaml(MINIMAL_TRINO_YAML); - let merged = &cluster.role_group_configs[&TrinoRole::Coordinator] + let merged = &cluster + .coordinator_role_group_configs .values() .next() .expect("the fixture defines a coordinator role group") diff --git a/rust/operator-binary/src/controller/build/properties/access_control_properties.rs b/rust/operator-binary/src/controller/build/properties/access_control_properties.rs index 0c7e47b8..00d2d872 100644 --- a/rust/operator-binary/src/controller/build/properties/access_control_properties.rs +++ b/rust/operator-binary/src/controller/build/properties/access_control_properties.rs @@ -27,17 +27,15 @@ pub fn build(cluster: &ValidatedCluster, rg: &TrinoRoleGroupConfig) -> BTreeMap< #[cfg(test)] mod tests { use super::*; - use crate::{ - controller::build::properties::test_support::{ - MINIMAL_TRINO_YAML, validated_cluster_from_yaml, - }, - crd::TrinoRole, + use crate::controller::build::properties::test_support::{ + MINIMAL_TRINO_YAML, validated_cluster_from_yaml, }; #[test] fn default_renders_empty_when_no_opa() { let cluster = validated_cluster_from_yaml(MINIMAL_TRINO_YAML); - let rg = cluster.role_group_configs[&TrinoRole::Coordinator] + let rg = cluster + .coordinator_role_group_configs .values() .next() .unwrap() diff --git a/rust/operator-binary/src/controller/build/properties/config_properties.rs b/rust/operator-binary/src/controller/build/properties/config_properties.rs index c8754d7d..d47f432e 100644 --- a/rust/operator-binary/src/controller/build/properties/config_properties.rs +++ b/rust/operator-binary/src/controller/build/properties/config_properties.rs @@ -312,7 +312,8 @@ mod tests { } fn rg(cluster: &ValidatedCluster, role: &TrinoRole) -> TrinoRoleGroupConfig { - cluster.role_group_configs[role] + cluster + .role_group_configs(role) .values() .next() .expect("the fixture defines a role group") @@ -322,7 +323,8 @@ mod tests { #[test] fn default_renders_includes_coordinator_default_and_query_max_memory_default() { let cluster = validated_cluster_from_yaml(MINIMAL_TRINO_YAML); - let rg = cluster.role_group_configs[&TrinoRole::Coordinator] + let rg = cluster + .coordinator_role_group_configs .values() .next() .unwrap() @@ -347,6 +349,50 @@ mod tests { ); } + /// A coordinator role group with no explicit `replicas` runs one pod — Kubernetes' default + /// for a `StatefulSet` with `replicas: null` — so it must predict one pod ref, not none. + /// Predicting none leaves `discovery.uri` out of `config.properties` entirely, and no pod can + /// find the coordinator. + #[test] + fn a_coordinator_without_an_explicit_replica_count_still_sets_the_discovery_uri() { + const NO_REPLICAS_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: "481" + clusterConfig: + catalogLabelSelector: {} + coordinators: + roleGroups: + default: {} + workers: + roleGroups: + default: {} + "#; + + let cluster = validated_cluster_from_yaml(NO_REPLICAS_YAML); + assert_eq!(cluster.cluster_config.coordinator_pod_refs.len(), 1); + + let props = build( + &cluster, + TrinoRole::Coordinator, + &rg(&cluster, &TrinoRole::Coordinator), + &cluster_info(), + ) + .unwrap(); + assert_eq!( + props.get("discovery.uri").map(String::as_str), + Some( + "https://simple-trino-coordinator-default-0.simple-trino-coordinator-default-headless.default.svc.cluster.local:8443" + ) + ); + } + #[test] fn server_tls_only_uses_server_keystore_dir_and_http_discovery() { let cluster = validated_cluster_from_yaml(SERVER_TLS_ONLY_YAML); diff --git a/rust/operator-binary/src/controller/build/properties/exchange_manager_properties.rs b/rust/operator-binary/src/controller/build/properties/exchange_manager_properties.rs index 337e2175..bd4e8ada 100644 --- a/rust/operator-binary/src/controller/build/properties/exchange_manager_properties.rs +++ b/rust/operator-binary/src/controller/build/properties/exchange_manager_properties.rs @@ -27,17 +27,15 @@ pub fn build(cluster: &ValidatedCluster, rg: &TrinoRoleGroupConfig) -> BTreeMap< #[cfg(test)] mod tests { use super::*; - use crate::{ - controller::build::properties::test_support::{ - MINIMAL_TRINO_YAML, validated_cluster_from_yaml, - }, - crd::TrinoRole, + use crate::controller::build::properties::test_support::{ + MINIMAL_TRINO_YAML, validated_cluster_from_yaml, }; #[test] fn default_renders_empty_when_no_fte() { let cluster = validated_cluster_from_yaml(MINIMAL_TRINO_YAML); - let rg = cluster.role_group_configs[&TrinoRole::Coordinator] + let rg = cluster + .coordinator_role_group_configs .values() .next() .unwrap() diff --git a/rust/operator-binary/src/controller/build/properties/log_properties.rs b/rust/operator-binary/src/controller/build/properties/log_properties.rs index 6c5831b8..5241a7a5 100644 --- a/rust/operator-binary/src/controller/build/properties/log_properties.rs +++ b/rust/operator-binary/src/controller/build/properties/log_properties.rs @@ -29,17 +29,15 @@ pub fn build(rg: &TrinoRoleGroupConfig) -> BTreeMap { #[cfg(test)] mod tests { use super::*; - use crate::{ - controller::build::properties::test_support::{ - MINIMAL_TRINO_YAML, validated_cluster_from_yaml, - }, - crd::TrinoRole, + use crate::controller::build::properties::test_support::{ + MINIMAL_TRINO_YAML, validated_cluster_from_yaml, }; #[test] fn default_renders_root_logger_only() { let cluster = validated_cluster_from_yaml(MINIMAL_TRINO_YAML); - let rg = cluster.role_group_configs[&TrinoRole::Coordinator] + let rg = cluster + .coordinator_role_group_configs .values() .next() .unwrap() diff --git a/rust/operator-binary/src/controller/build/properties/node_properties.rs b/rust/operator-binary/src/controller/build/properties/node_properties.rs index 2046b56d..96d1d4ac 100644 --- a/rust/operator-binary/src/controller/build/properties/node_properties.rs +++ b/rust/operator-binary/src/controller/build/properties/node_properties.rs @@ -36,7 +36,8 @@ mod tests { #[test] fn default_renders_node_environment_from_cluster_name() { let cluster = validated_cluster_from_yaml(MINIMAL_TRINO_YAML); - let rg = cluster.role_group_configs[&crate::crd::TrinoRole::Coordinator] + let rg = cluster + .coordinator_role_group_configs .values() .next() .unwrap() diff --git a/rust/operator-binary/src/controller/build/properties/security_properties.rs b/rust/operator-binary/src/controller/build/properties/security_properties.rs index cdc01fcb..24e30e34 100644 --- a/rust/operator-binary/src/controller/build/properties/security_properties.rs +++ b/rust/operator-binary/src/controller/build/properties/security_properties.rs @@ -37,17 +37,15 @@ pub fn build(rg: &TrinoRoleGroupConfig) -> BTreeMap { #[cfg(test)] mod tests { use super::*; - use crate::{ - controller::build::properties::test_support::{ - MINIMAL_TRINO_YAML, validated_cluster_from_yaml, - }, - crd::TrinoRole, + use crate::controller::build::properties::test_support::{ + MINIMAL_TRINO_YAML, validated_cluster_from_yaml, }; fn coordinator_rg( cluster: &crate::controller::ValidatedCluster, ) -> crate::controller::TrinoRoleGroupConfig { - cluster.role_group_configs[&TrinoRole::Coordinator] + cluster + .coordinator_role_group_configs .values() .next() .unwrap() diff --git a/rust/operator-binary/src/controller/build/properties/spooling_manager_properties.rs b/rust/operator-binary/src/controller/build/properties/spooling_manager_properties.rs index 94a6d63a..941cd747 100644 --- a/rust/operator-binary/src/controller/build/properties/spooling_manager_properties.rs +++ b/rust/operator-binary/src/controller/build/properties/spooling_manager_properties.rs @@ -27,17 +27,15 @@ pub fn build(cluster: &ValidatedCluster, rg: &TrinoRoleGroupConfig) -> BTreeMap< #[cfg(test)] mod tests { use super::*; - use crate::{ - controller::build::properties::test_support::{ - MINIMAL_TRINO_YAML, validated_cluster_from_yaml, - }, - crd::TrinoRole, + use crate::controller::build::properties::test_support::{ + MINIMAL_TRINO_YAML, validated_cluster_from_yaml, }; #[test] fn default_renders_empty_when_no_spooling() { let cluster = validated_cluster_from_yaml(MINIMAL_TRINO_YAML); - let rg = cluster.role_group_configs[&TrinoRole::Coordinator] + let rg = cluster + .coordinator_role_group_configs .values() .next() .unwrap() 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 af80b8a6..1a2863a5 100644 --- a/rust/operator-binary/src/controller/build/resource/config_map.rs +++ b/rust/operator-binary/src/controller/build/resource/config_map.rs @@ -61,15 +61,8 @@ pub fn build_rolegroup_config_map( role_group_name: &RoleGroupName, cluster_info: &KubernetesClusterInfo, ) -> Result { - let role_group_configs = - cluster - .role_group_configs - .get(role) - .with_context(|| MissingRoleGroupSnafu { - role: role.to_string(), - role_group: role_group_name.to_string(), - })?; - let rg = role_group_configs + let rg = cluster + .role_group_configs(role) .get(role_group_name) .with_context(|| MissingRoleGroupSnafu { role: role.to_string(), diff --git a/rust/operator-binary/src/controller/build/resource/pdb.rs b/rust/operator-binary/src/controller/build/resource/pdb.rs index 68a9ffb7..1b744795 100644 --- a/rust/operator-binary/src/controller/build/resource/pdb.rs +++ b/rust/operator-binary/src/controller/build/resource/pdb.rs @@ -47,10 +47,8 @@ pub fn build_pdb( /// contribute nothing, as their size is not known at reconcile time. fn worker_count(cluster: &ValidatedCluster) -> u16 { cluster - .role_group_configs - .get(&TrinoRole::Worker) - .into_iter() - .flat_map(|groups| groups.values()) + .worker_role_group_configs + .values() .filter_map(|rg| rg.replicas) .sum() } diff --git a/rust/operator-binary/src/controller/build/resource/statefulset.rs b/rust/operator-binary/src/controller/build/resource/statefulset.rs index a7597d39..6cd36c48 100644 --- a/rust/operator-binary/src/controller/build/resource/statefulset.rs +++ b/rust/operator-binary/src/controller/build/resource/statefulset.rs @@ -807,8 +807,7 @@ mod tests { /// Builds the coordinator `default` role-group StatefulSet for the given cluster. fn build_coordinator_statefulset(cluster: &ValidatedCluster) -> Result { let role_group_name = RoleGroupName::from_str("default").expect("valid role group name"); - let role_group_config = - &cluster.role_group_configs[&TrinoRole::Coordinator][&role_group_name]; + let role_group_config = &cluster.coordinator_role_group_configs[&role_group_name]; build_rolegroup_statefulset( cluster, @@ -851,7 +850,7 @@ mod tests { let cluster = validated_cluster(); let role_group_name = RoleGroupName::from_str("default").expect("valid role group name"); let mut role_group_config = - cluster.role_group_configs[&TrinoRole::Coordinator][&role_group_name].clone(); + cluster.coordinator_role_group_configs[&role_group_name].clone(); role_group_config.env_overrides = EnvVarSet::new().with_value( &EnvVarName::from_str("CONTAINERDEBUG_LOG_DIRECTORY").expect("valid env var name"), "/custom/log/dir", diff --git a/rust/operator-binary/src/controller/mod.rs b/rust/operator-binary/src/controller/mod.rs index 65f171b0..bc16f15f 100644 --- a/rust/operator-binary/src/controller/mod.rs +++ b/rust/operator-binary/src/controller/mod.rs @@ -158,15 +158,26 @@ impl ValidatedTrinoConfig { } } -/// Per-role configuration extracted during validation. +/// The coordinator's validated role-level configuration. /// -/// Lets the reconciler and build steps consume this controller-owned type instead of re-reading -/// the raw [`v1alpha1::TrinoCluster`]. +/// Separate from the worker's because the two are different types in the CRD: +/// [`v1alpha1::TrinoCoordinatorRoleConfig`] carries a `listener_class` for which the worker's +/// `GenericRoleConfig` has no equivalent. One shared type would have to make that field an +/// `Option` — mandatory for one role, meaningless for the other — leaving every reader to work +/// out which role it is holding. #[derive(Clone, Debug)] -pub struct ValidatedRoleConfig { +pub struct ValidatedCoordinatorRoleConfig { + pub pdb: stackable_operator::commons::pdb::PdbConfig, + /// The listener class of the coordinator's group listener. Not optional: the CRD defaults it. + pub listener_class: ListenerClassName, +} + +/// The worker's validated role-level configuration. +/// +/// Workers have no group listener, so there is no listener class to carry. +#[derive(Clone, Debug)] +pub struct ValidatedWorkerRoleConfig { pub pdb: stackable_operator::commons::pdb::PdbConfig, - /// The listener class for the role's group listener, if it has one (coordinator only). - pub listener_class: Option, } /// The validated TrinoCluster. The output of the validate step. @@ -188,45 +199,62 @@ pub struct ValidatedCluster { /// parsed once from the resolved image's app version label value. pub product_version: ProductVersion, pub cluster_config: ValidatedClusterConfig, - pub role_configs: BTreeMap, - pub role_group_configs: BTreeMap>, + /// The coordinator's role-level config. + pub coordinator_config: ValidatedCoordinatorRoleConfig, + /// The validated config of every coordinator role group, keyed by role group name. + pub coordinator_role_group_configs: BTreeMap, + /// The worker's role-level config. + pub worker_config: ValidatedWorkerRoleConfig, + /// The validated config of every worker role group, keyed by role group name. + pub worker_role_group_configs: BTreeMap, } impl ValidatedCluster { - #[allow(clippy::too_many_arguments)] - pub fn new( - name: ClusterName, - namespace: NamespaceName, - uid: Uid, - image: ResolvedProductImage, - numeric_product_version: u16, - cluster_config: ValidatedClusterConfig, - role_configs: BTreeMap, - role_group_configs: BTreeMap>, - ) -> Self { - Self { - metadata: ObjectMeta { - name: Some(name.to_string()), - namespace: Some(namespace.to_string()), - uid: Some(uid.to_string()), - ..ObjectMeta::default() - }, - name, - namespace, - uid, - product_version: ProductVersion::from_str(&image.app_version_label_value) - .expect("the app version label value is a valid product version"), - image, - numeric_product_version, - cluster_config, - role_configs, - role_group_configs, + /// 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 validated per-role config for `role`, if the role is defined. - pub(crate) fn role_config(&self, role: &TrinoRole) -> Option<&ValidatedRoleConfig> { - self.role_configs.get(role) + /// 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") + } + + /// The role groups of `role`. + /// + /// A lookup, not a search: both roles always exist, because the CRD makes `coordinators` and + /// `workers` non-optional, so there is no `Option` to unwrap. + pub(crate) fn role_group_configs( + &self, + role: &TrinoRole, + ) -> &BTreeMap { + match role { + TrinoRole::Coordinator => &self.coordinator_role_group_configs, + TrinoRole::Worker => &self.worker_role_group_configs, + } + } + + /// The PodDisruptionBudget config of `role`. + pub(crate) fn pdb(&self, role: &TrinoRole) -> &stackable_operator::commons::pdb::PdbConfig { + match role { + TrinoRole::Coordinator => &self.coordinator_config.pdb, + TrinoRole::Worker => &self.worker_config.pdb, + } } /// Whether the (client-facing) server TLS is enabled. diff --git a/rust/operator-binary/src/controller/validate.rs b/rust/operator-binary/src/controller/validate.rs index 41580b96..f817467e 100644 --- a/rust/operator-binary/src/controller/validate.rs +++ b/rust/operator-binary/src/controller/validate.rs @@ -12,7 +12,6 @@ use stackable_operator::{ config::fragment, kube::ResourceExt as _, product_logging::spec::Logging, - role_utils::GenericRoleConfig, v2::{ controller_utils::{get_cluster_name, get_namespace, get_uid}, product_logging::framework::{ @@ -23,13 +22,13 @@ use stackable_operator::{ types::kubernetes::ConfigMapName, }, }; -use strum::{EnumDiscriminants, IntoEnumIterator, IntoStaticStr}; +use strum::{EnumDiscriminants, IntoStaticStr}; use crate::{ authentication::{self, TrinoAuthenticationConfig, TrinoAuthenticationTypes}, controller::{ - ValidatedCluster, ValidatedClusterConfig, ValidatedRoleConfig, ValidatedTls, - ValidatedTrinoConfig, dereference::DereferencedObjects, + ValidatedCluster, ValidatedClusterConfig, ValidatedCoordinatorRoleConfig, ValidatedTls, + ValidatedTrinoConfig, ValidatedWorkerRoleConfig, dereference::DereferencedObjects, }, crd::{Container, TrinoRole, catalog::TrinoCatalogName, v1alpha1}, }; @@ -208,56 +207,42 @@ pub fn validate( .vector_aggregator_config_map_name .clone(); - let mut role_configs: BTreeMap = BTreeMap::new(); - let mut role_group_configs: BTreeMap> = - BTreeMap::new(); - for trino_role in TrinoRole::iter() { - let role = trino.role(&trino_role); - - // Extract the per-role PDB and (optional) listener class up-front, so the reconciler and - // build steps consume the validated config instead of re-reading the raw cluster. - role_configs.insert( - trino_role.clone(), - ValidatedRoleConfig { - pdb: trino - .generic_role_config(&trino_role) - .pod_disruption_budget - .clone(), - listener_class: trino_role.listener_class_name(trino), - }, - ); - - let default_config = v1alpha1::TrinoConfig::default_config( - &trino.name_any(), - &trino_role, - &dereferenced_objects.catalog_definitions, - ); - let mut groups = BTreeMap::new(); - for (rg_name, rg) in &role.role_groups { - let role_group_name = - RoleGroupName::from_str(rg_name).with_context(|_| ParseRoleGroupNameSnafu { - role_group: rg_name.clone(), - })?; - // Merges and validates the role group config (default <- role <- role group). Because - // `JavaCommonConfig` implements `Merge`, the role and role-group `jvmArgumentOverrides` - // are merged here too and carried by `product_specific_common_config`. - let merged = with_validated_config::< - v1alpha1::TrinoConfig, - JavaCommonConfig, - v1alpha1::TrinoConfigFragment, - GenericRoleConfig, - v1alpha1::TrinoConfigOverrides, - >(rg, &role, &default_config) - .with_context(|_| FailedToResolveConfigSnafu { - role_group: role_group_name.clone(), - })?; - groups.insert( - role_group_name, - into_role_group_config(merged, &vector_aggregator_config_map_name)?, - ); - } - role_group_configs.insert(trino_role, groups); - } + // Each role's role groups are validated from that role's own CRD type: `validate_role_groups` + // is generic over the role config, so neither role has to be converted to the other's shape. + // + // Validated in `TrinoRole` declaration order: the first role that fails is the error the user + // sees, so swapping these two calls changes which misconfiguration gets reported when both + // roles are wrong. + let coordinator_role_group_configs = validate_role_groups( + &trino.spec.coordinators, + trino, + &TrinoRole::Coordinator, + dereferenced_objects, + &vector_aggregator_config_map_name, + )?; + let worker_role_group_configs = validate_role_groups( + &trino.spec.workers, + trino, + &TrinoRole::Worker, + dereferenced_objects, + &vector_aggregator_config_map_name, + )?; + + // Each role's config comes from its own role config in the spec, so the coordinator's + // `listener_class` stays mandatory and the worker has no such field at all. + let coordinator_config = ValidatedCoordinatorRoleConfig { + pdb: trino + .spec + .coordinators + .role_config + .common + .pod_disruption_budget + .clone(), + listener_class: trino.spec.coordinators.role_config.listener_class.clone(), + }; + let worker_config = ValidatedWorkerRoleConfig { + pdb: trino.spec.workers.role_config.pod_disruption_budget.clone(), + }; let mut catalogs = BTreeMap::new(); for catalog in &dereferenced_objects.catalogs { @@ -289,16 +274,77 @@ pub fn validate( let name = get_cluster_name(trino).context(GetClusterNameSnafu)?; let uid = get_uid(trino).context(GetClusterUidSnafu)?; - Ok(ValidatedCluster::new( + // The two role-group maps share one type, so each is named at the point it is set: as + // positional arguments they could be swapped silently, giving each role the other's role + // groups. + Ok(ValidatedCluster { + metadata: ValidatedCluster::object_meta(&name, &namespace, &uid), + product_version: ValidatedCluster::product_version(&image), name, namespace, uid, image, numeric_product_version, cluster_config, - role_configs, - role_group_configs, - )) + coordinator_config, + coordinator_role_group_configs, + worker_config, + worker_role_group_configs, + }) +} + +/// Validates every role group of one role, merging default <- role <- role group. +/// +/// Generic over the role's `RoleConfig` so that each role is read from its own CRD type: the +/// coordinator's [`v1alpha1::TrinoCoordinatorRoleConfig`] and the worker's `GenericRoleConfig` +/// are both accepted, neither converted to the other. +fn validate_role_groups( + role: &stackable_operator::v2::role_utils::Role< + v1alpha1::TrinoConfigFragment, + v1alpha1::TrinoConfigOverrides, + RoleConfig, + JavaCommonConfig, + >, + trino: &v1alpha1::TrinoCluster, + trino_role: &TrinoRole, + dereferenced_objects: &DereferencedObjects, + vector_aggregator_config_map_name: &Option, +) -> Result> +where + RoleConfig: Default + stackable_operator::schemars::JsonSchema + serde::Serialize, +{ + let default_config = v1alpha1::TrinoConfig::default_config( + &trino.name_any(), + trino_role, + &dereferenced_objects.catalog_definitions, + ); + + let mut role_groups = BTreeMap::new(); + for (rg_name, rg) in &role.role_groups { + let role_group_name = + RoleGroupName::from_str(rg_name).with_context(|_| ParseRoleGroupNameSnafu { + role_group: rg_name.clone(), + })?; + // Merges and validates the role group config (default <- role <- role group). Because + // `JavaCommonConfig` implements `Merge`, the role and role-group `jvmArgumentOverrides` + // are merged here too and carried by `product_specific_common_config`. + let merged = with_validated_config::< + v1alpha1::TrinoConfig, + JavaCommonConfig, + v1alpha1::TrinoConfigFragment, + RoleConfig, + v1alpha1::TrinoConfigOverrides, + >(rg, role, &default_config) + .with_context(|_| FailedToResolveConfigSnafu { + role_group: role_group_name.clone(), + })?; + role_groups.insert( + role_group_name, + into_role_group_config(merged, vector_aggregator_config_map_name)?, + ); + } + + Ok(role_groups) } /// Adapts the validated [`RoleGroup`] produced by [`with_validated_config`] into the flattened @@ -334,24 +380,30 @@ pub(crate) fn merged_role_group_config( role_group: &str, trino_catalogs: &[crate::crd::catalog::v1alpha1::TrinoCatalog], ) -> TrinoRoleGroupConfig { - let role = trino.role(trino_role); - let default_config = - v1alpha1::TrinoConfig::default_config(&trino.name_any(), trino_role, trino_catalogs); - let rg = role - .role_groups - .get(role_group) - .expect("role group should be defined"); - let merged = with_validated_config::< - v1alpha1::TrinoConfig, - JavaCommonConfig, - v1alpha1::TrinoConfigFragment, - GenericRoleConfig, - v1alpha1::TrinoConfigOverrides, - >(rg, &role, &default_config) - .expect("role group config should be valid"); // The shared test clusters do not enable the Vector agent, so no aggregator ConfigMap name is // required here. - into_role_group_config(merged, &None).expect("env overrides should be valid") + let derefs = DereferencedObjects { + catalog_definitions: trino_catalogs.to_vec(), + resolved_authentication_classes: Vec::new(), + catalogs: Vec::new(), + trino_opa_config: None, + resolved_fte_config: None, + resolved_client_protocol_config: None, + }; + let role_groups = match trino_role { + TrinoRole::Coordinator => { + validate_role_groups(&trino.spec.coordinators, trino, trino_role, &derefs, &None) + } + TrinoRole::Worker => { + validate_role_groups(&trino.spec.workers, trino, trino_role, &derefs, &None) + } + } + .expect("role group config should be valid"); + + role_groups + .get(&RoleGroupName::from_str(role_group).expect("valid role group name")) + .expect("role group should be defined") + .clone() } #[cfg(test)] @@ -479,29 +531,22 @@ mod tests { "simple-trino-coordinator-default-0" ); - // Per-role configs: default (enabled) PDBs; only the coordinator has a group listener. - let roles: Vec<_> = validated.role_configs.keys().collect(); - assert_eq!(roles, [&TrinoRole::Coordinator, &TrinoRole::Worker]); - for role_config in validated.role_configs.values() { - assert!(role_config.pdb.enabled); - assert_eq!(role_config.pdb.max_unavailable, None); + // Per-role configs: default (enabled) PDBs. Only the coordinator's config carries a + // listener class; the worker type has no such field. + for role in [TrinoRole::Coordinator, TrinoRole::Worker] { + let pdb = validated.pdb(&role); + assert!(pdb.enabled); + assert_eq!(pdb.max_unavailable, None); } assert_eq!( - validated.role_configs[&TrinoRole::Coordinator] - .listener_class - .as_ref() - .map(ToString::to_string), - Some("cluster-internal".to_string()) - ); - assert_eq!( - validated.role_configs[&TrinoRole::Worker].listener_class, - None + validated.coordinator_config.listener_class.to_string(), + "cluster-internal" ); // One `default` role group per role; the Vector agent is off. let default_rg = RoleGroupName::from_str("default").expect("valid role group name"); for role in [TrinoRole::Coordinator, TrinoRole::Worker] { - let role_group = &validated.role_group_configs[&role][&default_rg]; + let role_group = &validated.role_group_configs(&role)[&default_rg]; assert_eq!(role_group.replicas, Some(1)); assert!(!role_group.config.logging.enable_vector_agent); assert_eq!(role_group.config.logging.vector_container, None); diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs index b6529bd4..cfe57b1a 100644 --- a/rust/operator-binary/src/crd/mod.rs +++ b/rust/operator-binary/src/crd/mod.rs @@ -426,15 +426,6 @@ impl From<&TrinoRole> for RoleName { } } -impl TrinoRole { - pub fn listener_class_name(&self, trino: &v1alpha1::TrinoCluster) -> Option { - match self { - Self::Coordinator => Some(trino.spec.coordinators.role_config.listener_class.clone()), - Self::Worker => None, - } - } -} - #[derive( Clone, Debug, @@ -525,28 +516,19 @@ impl v1alpha1::TrinoConfig { } } -impl v1alpha1::TrinoCluster { - /// Returns the given role (both roles are required by the CRD). - pub fn role(&self, role_variant: &TrinoRole) -> TrinoRoleType { - match role_variant { - TrinoRole::Coordinator => { - extract_role_from_coordinator_config(self.spec.coordinators.to_owned()) - } - TrinoRole::Worker => self.spec.workers.to_owned(), - } - } - - pub fn generic_role_config(&self, role: &TrinoRole) -> &GenericRoleConfig { - match role { - TrinoRole::Coordinator => &self.spec.coordinators.role_config.common, - TrinoRole::Worker => &self.spec.workers.role_config, - } - } +/// 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 const DEFAULT_REPLICAS: u16 = 1; +impl v1alpha1::TrinoCluster { /// List all coordinator pods expected to form the cluster /// /// We try to predict the pods here rather than looking at the current cluster state in order to /// avoid instance churn. + /// + /// A role group without an explicit `replicas` counts as [`DEFAULT_REPLICAS`]. Counting it as + /// zero would yield no pod refs at all for a single-role-group cluster, and the first of these + /// is what sets `discovery.uri`. pub fn coordinator_pods( &self, namespace: &NamespaceName, @@ -571,7 +553,7 @@ impl v1alpha1::TrinoCluster { .expect("a role group name is a valid role group name"), }; let ns = ns.clone(); - (0..rolegroup.replicas.unwrap_or(0)).map(move |i| TrinoPodRef { + (0..rolegroup.replicas.unwrap_or(DEFAULT_REPLICAS)).map(move |i| TrinoPodRef { namespace: ns.clone(), role_group_service_name: resource_names.headless_service_name().to_string(), pod_name: format!("{}-{i}", resource_names.stateful_set_name()), @@ -598,14 +580,6 @@ impl v1alpha1::TrinoCluster { /// Converts the coordinator role (which carries the coordinator-specific `role_config`) into the /// generic [`TrinoRoleType`]. Only the `role_config` type parameter differs between the two; the /// `config` and `role_groups` carry over unchanged. -fn extract_role_from_coordinator_config(fragment: TrinoCoordinatorRoleType) -> TrinoRoleType { - Role { - config: fragment.config, - role_config: fragment.role_config.common, - role_groups: fragment.role_groups, - } -} - impl HasStatusCondition for v1alpha1::TrinoCluster { fn conditions(&self) -> Vec { match &self.status { diff --git a/rust/operator-binary/src/trino_controller.rs b/rust/operator-binary/src/trino_controller.rs index ca633904..9f8f92a7 100644 --- a/rust/operator-binary/src/trino_controller.rs +++ b/rust/operator-binary/src/trino_controller.rs @@ -485,7 +485,8 @@ mod tests { let validated_cluster = validate::validate(&trino, &derefs, &operator_env).expect("validate should succeed"); - let env = &validated_cluster.role_group_configs[&TrinoRole::Coordinator] + let env = &validated_cluster + .coordinator_role_group_configs .values() .next() .unwrap()