Repository navigation
Conversation
| let role_group_name = RoleGroupName::from_str(role_group).unwrap(); | ||
| let rg = cluster | ||
| .role_group_configs | ||
| .get(&role) | ||
| .expect("role present") | ||
| .get(&role_group_name) | ||
| .expect("role group present") | ||
| .clone(); | ||
| build_rolegroup_config_map(&cluster, &role, &role_group_name, &rg).expect("config map") | ||
| let role_group_name = RoleGroupName::from_str(role_group).expect("valid role group name"); | ||
| let resources = build(&cluster).expect("build succeeds"); | ||
| role_group_config_map(&resources, &cluster, &role, &role_group_name).clone() |
There was a problem hiding this comment.
These tests used to call build_rolegroup_config_map directly. Now they build every resource in another module and look the ConfigMap back up by name, so it's only an assumption that this module produced it. Calling build_config_map directly would be less indirect and clearer about what's under test.
Same comment for the remaining tests using role_group_config_map and role_group_stateful_set.
| let druid = druid_from_yaml(&MINIMAL_DRUID_YAML.replace( | ||
| " brokers:\n", | ||
| " brokers:\n envOverrides:\n CONTAINERDEBUG_LOG_DIRECTORY: /custom/log/dir\n", | ||
| )); |
There was a problem hiding this comment.
String substitution on the fixture is fragile and obscures the test input.
There was a problem hiding this comment.
After the split this module holds nothing but the MiddleManager drain hook. Therefore I would move it into the role_group::middle_manager module.
| use crate::controller::validate::test_support::druid_from_yaml; | ||
| use crate::controller::{ | ||
| build::{build, test_support::role_group_config_map}, | ||
| validate::test_support::{druid_from_yaml, validated_cluster}, | ||
| }; |
There was a problem hiding this comment.
nit: Our imports are usually at the top of the module.
| pub vector_container: Option<VectorContainerLogConfig>, | ||
| pub enable_vector_agent: bool, |
There was a problem hiding this comment.
enable_vector_agent is exactly vector_container.is_some(), so enable_vector_agent is redundant and can be removed.
| /// A Coordinator role group's validated, merged config together with its validated logging. | ||
| #[derive(Clone, Debug, PartialEq)] | ||
| pub struct ValidatedCoordinatorConfig { | ||
| pub config: CoordinatorConfig, | ||
| /// Validated logging configuration (derived from `config.logging` during validation). | ||
| pub logging: ValidatedLogging, | ||
| } | ||
|
|
||
| /// A Broker role group's validated, merged config together with its validated logging. | ||
| #[derive(Clone, Debug, PartialEq)] | ||
| pub struct ValidatedBrokerConfig { | ||
| pub config: BrokerConfig, | ||
| /// Validated logging configuration (derived from `config.logging` during validation). | ||
| pub logging: ValidatedLogging, | ||
| } | ||
|
|
||
| /// A Historical role group's validated, merged config together with its validated logging. | ||
| #[derive(Clone, Debug, PartialEq)] | ||
| pub struct ValidatedHistoricalConfig { | ||
| pub config: HistoricalConfig, | ||
| /// Validated logging configuration (derived from `config.logging` during validation). | ||
| pub logging: ValidatedLogging, | ||
| } | ||
|
|
||
| /// A MiddleManager role group's validated, merged config together with its validated logging. | ||
| #[derive(Clone, Debug, PartialEq)] | ||
| pub struct ValidatedMiddleManagerConfig { | ||
| pub config: MiddleManagerConfig, | ||
| /// Validated logging configuration (derived from `config.logging` during validation). | ||
| pub logging: ValidatedLogging, | ||
| } | ||
|
|
||
| /// A Router role group's validated, merged config together with its validated logging. | ||
| #[derive(Clone, Debug, PartialEq)] | ||
| pub struct ValidatedRouterConfig { | ||
| pub config: RouterConfig, | ||
| /// Validated logging configuration (derived from `config.logging` during validation). | ||
| pub logging: ValidatedLogging, | ||
| } |
There was a problem hiding this comment.
These wrappers keep the merged config verbatim and add the validated logging beside it, which has three consequences: The raw logging inside config is dead once validate_logging has run, but it is still reachable as rg_config.config.config.logging, so every role module has to discard it explicitly with logging: _. requested_secret_lifetime stays Option, so it is unwrapped five times in the build step instead of once during validation. And graceful_shutdown_timeout stays Option too, where it then fails silently. ValidatedDruidConfig on main was a transform rather than a wrapper and listed exactly the five fields the build step needs, with requested_secret_lifetime: Duration and no second logging. Could the wrappers go back to that shape?
There was a problem hiding this comment.
Done: 35eefe5
I took "go back to that shape" to mean the same set of fields as ValidatedDruidConfig on main. Each role now has its own struct with exactly those five fields (resources, logging, affinity, graceful_shutdown_timeout, requested_secret_lifetime), built from the merged config in validation instead of wrapping it:
- Only the validated logging remains, so the logging: _ lines and the config.config nesting are gone
- requested_secret_lifetime is unwrapped once per role group during validation
- graceful_shutdown_timeout is now a Duration too (it was still an Option on main)
I kept one type per role rather than one shared type, so that resources stays typed (Historicals use HistoricalStorage, the others DruidStorage).
| - Bump stackable-operator to 0.119.0 ([#880]). | ||
| - test: Bump vector-aggregator to 0.58.0 ([#885]). | ||
| - Internal operator refactoring: build each role group through per-role modules (brokers, | ||
| coordinators, historicals, middle managers, routers) and a shared role group builder, with | ||
| each role's configuration held in typed fields of the validated cluster ([#887]). |
There was a problem hiding this comment.
The entries for stackable-operator and vector-aggregator are leftovers from the merge with main and must be removed.
| - Bump stackable-operator to 0.119.0 ([#880]). | |
| - test: Bump vector-aggregator to 0.58.0 ([#885]). | |
| - Internal operator refactoring: build each role group through per-role modules (brokers, | |
| coordinators, historicals, middle managers, routers) and a shared role group builder, with | |
| each role's configuration held in typed fields of the validated cluster ([#887]). | |
| - Internal operator refactoring: build each role group through per-role modules (brokers, | |
| coordinators, historicals, middle managers, routers) and a shared role group builder, with | |
| each role's configuration held in typed fields of the validated cluster ([#887]). |
Description
Part of https://github.com/stackabletech/internal-issues/issues/167
Overview of logic flow
%%{init: {"flowchart": {"wrappingWidth": 900, "htmlLabels": true}}}%% flowchart TB START["`**build()** mod.rs:160`"] START --> LCO START --> LBR START --> LHI START --> LMM START --> LRO subgraph coordinator_track["Coordinator"] direction TB LCO["`**build_pdb()** :166 · **build_group_listener()** :169 **loop** over coordinator_role_group_configs :175`"] RCO["`**coordinator::build()** role_group/coordinator.rs:30 **heap_memory()** :45 · no direct memory **RoleGroupInputs { .. }** :47 listener: Some :58 · role_jvm_args: Derby log :64 · oidc_enabled: true :66`"] LCO --> RCO end subgraph broker_track["Broker"] direction TB LBR["`**build_pdb()** :184 · **build_group_listener()** :187 **loop** over broker_role_group_configs :193`"] RBR["`**broker::build()** role_group/broker.rs:22 **heap_memory()** :38 · 400 MiB direct memory :37 **RoleGroupInputs { .. }** :40 listener: Some :51 · oidc_enabled: true :58`"] LBR --> RBR end subgraph historical_track["Historical"] direction TB LHI["`**build_pdb()** :202 · no listener **loop** over historical_role_group_configs :205`"] RHI["`**historical::build()** role_group/historical.rs:29 **HistoricalDerivedSettings::try_from()** :44 · heap + direct from memory limit **segment_cache_locations()** :50 · resource_runtime_properties **RoleGroupInputs { .. }** :54 listener: None · role_pod_volumes: segment cache :66 · role_druid_mounts :67 · oidc_enabled: true :72`"] LHI --> RHI end subgraph middle_manager_track["MiddleManager"] direction TB LMM["`**build_pdb()** :214 · no listener **loop** over middle_manager_role_group_configs :221`"] RMM["`**middle_manager::build()** role_group/middle_manager.rs:56 **heap_memory()** :71 · no direct memory **RoleGroupInputs { .. }** :73 listener: None · **indexer_java_opts()** :90 · **drain_pre_stop_hook()** :98 · oidc_enabled: false :106`"] LMM --> RMM end subgraph router_track["Router"] direction TB LRO["`**build_pdb()** :230 · **build_group_listener()** :233 **loop** over router_role_group_configs :239`"] RRO["`**router::build()** role_group/router.rs:22 **heap_memory()** :38 · 128 MiB direct memory :37 **RoleGroupInputs { .. }** :40 listener: Some :51 · oidc_enabled: true :58`"] LRO --> RRO end RCO --> NEW RBR --> NEW RHI --> NEW RMM --> NEW RRO --> NEW NEW["`**RoleGroupBuilder::new(inputs)** role_group/mod.rs:163 · resolve, same for every role **config_map_data()** resource/config_map.rs:118 · runtime.properties layers: resource_runtime_properties :141, TLS :262, auth with oidc_enabled :266, indexer_java_opts :278, role_runtime_defaults :281, configOverrides :282 · jvm.config: **construct_jvm_args()** :297 with role_jvm_args :301 · security.properties: indexer_java_opts :314 **containers_and_volumes()** container.rs:107 · prepare :117, druid :118 with druid_pre_stop :121, role_druid_mounts :179, role_pod_volumes :181, listener PVC + mount if listener :186, Vector :333`"] NEW --> ADD["`**RoleGroupResources::add()** mod.rs:284 · emit **build_services()** role_group/mod.rs:190 · headless + metrics Service **build_config_map()** resource/config_map.rs:343 · wraps builder.config_map_data **build_statefulset()** resource/statefulset.rs:41 · affinity :48, grace period :51, pod_volumes :59, init_containers :83, containers :87, pod overrides :92, volume_claim_templates :113`"] ADD --> OUT["`**KubernetesResources { .. }** mod.rs:260 PDBs and Listeners from the role level above · **build_discovery_configmap()** :255 · **build_service_account()** :266 · **build_role_binding()** :267`"] OUT --> APPLY["`**apply()** controller.rs:155 · outside build/`"] classDef coordinator fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a classDef broker fill:#fde68a,stroke:#b45309,color:#78350f classDef historical fill:#bbf7d0,stroke:#15803d,color:#14532d classDef middle_manager fill:#fbcfe8,stroke:#be185d,color:#831843 classDef router fill:#ddd6fe,stroke:#6d28d9,color:#4c1d95 classDef shared fill:#e5e7eb,stroke:#4b5563,color:#111827 classDef edge fill:#ffffff,stroke:#6b7280,color:#111827,stroke-dasharray:4 2 class LCO,RCO coordinator class LBR,RBR broker class LHI,RHI historical class LMM,RMM middle_manager class LRO,RRO router class NEW,ADD,OUT shared class START,APPLY edge style coordinator_track fill:#eff6ff,stroke:#93c5fd style broker_track fill:#fffbeb,stroke:#fcd34d style historical_track fill:#f0fdf4,stroke:#86efac style middle_manager_track fill:#fdf2f8,stroke:#f9a8d4 style router_track fill:#f5f3ff,stroke:#c4b5fdTests (one a little flakey which passed on the second attempt):
Definition of Done Checklist
Author
Reviewer
Acceptance
type/deprecationlabel & add to the deprecation scheduletype/experimentallabel & add to the experimental features tracker