Feat/remove product config#896
Draft
maltesander wants to merge 27 commits into
Draft
Conversation
Pulls in the v2 framework (role_utils::with_validated_config) needed for the product-config removal in subsequent commits. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a minimal `to_java_properties_string` under
`controller/build/properties/writer.rs`. Reproduces the escape rules
exercised by the kuttl ConfigMap snapshot (14-assert.yaml.j2):
- http\://opa-server...\:8081 (colons in URL values escaped)
- ${ENV\:INTERNAL_SECRET} (colon in env-var interpolation escaped)
No upstream v2 writer found in stackable-operator; vendored instead.
Follow-up: upstream the writer to operator-rs `v2` and delete the
vendored copy.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`stackable_operator::v2::role_utils::with_validated_config` requires `ConfigOverrides: Merge`. Adds a hand-written impl until `KeyValueConfigOverrides` derives `Merge` upstream. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds the typed ValidatedCluster state and a v2 validate function. Since upstream stackable_operator::v2::role_utils is not exported from operator-rs lib.rs on the smooth-operator branch, we vendor a minimal local version under src/framework/role_utils.rs (env_overrides kept as HashMap<String, String> for simplicity, no EnvVarSet). Notes: - JavaCommonConfig does not impl Merge upstream (its inner JvmArgumentOverrides::try_merge is fallible). The vendored with_validated_config drops the Merge bound on CommonConfig and lets product_specific_common_config carry the role-group level value through. Trino's JVM merging continues to flow through Role::get_merged_jvm_argument_overrides. - TrinoConfig::default_config is made pub so validate_v2 can call it. - TrinoRole derives Ord/PartialOrd so it can key BTreeMap. - TrinoOpaConfig, CatalogConfig, ResolvedClientProtocolConfig and ResolvedFaultTolerantExecutionConfig now derive Clone + Debug, so ValidatedCluster can derive them. The old validate function and the rest of the pipeline are untouched in this commit; subsequent commits switch reconcile over and remove the old paths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds controller/build/properties/log_properties.rs producing log.properties from defaults + container logging config + user overrides. The io.trino=INFO default is migrated from deploy/config-spec/properties.yaml. Tests are added in a later commit once the shared ValidatedCluster fixture exists. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The vendored Java-properties writer (Task 2), the log_properties builder (Task 5), and the get_log_property_map helper have no callers until build/config_map.rs is wired in at Task 12. Annotate each with #[allow(dead_code)] to keep `cargo check` clean during the intermediate commits. The annotations will be removed when callers land. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Migrates networkaddress.cache.ttl=30 and networkaddress.cache.negative.ttl=0 from deploy/config-spec/properties.yaml into the new builder. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Moves the node.environment derivation out of compute_files into a typed builder. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Lifts the OPA config injection out of build_rolegroup_config_map into a typed builder. TrinoOpaConfig::as_config() values that are None are dropped at this point (matches the old product-config write-time behavior); user overrides win. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Lifts the resolved-FTE exchange-manager properties out of build_rolegroup_config_map into a typed builder. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Lifts the resolved client-spooling protocol properties out of build_rolegroup_config_map into a typed builder. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Largest of the per-file builders. Reproduces today's config.properties content: defaults + compute_files contributions + dynamic_resolved_config block + user overrides. Migrates the node-scheduler.include-coordinator and query.max-memory defaults out of properties.yaml. Also adds a graceful_shutdown_config_properties_v2 helper taking &ValidatedCluster, used by the new builder. The legacy graceful_shutdown_config_properties stays until Task 15 cleanup. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirrors today's build_rolegroup_config_map but consumes the typed ValidatedCluster only — no &TrinoCluster or DereferencedObjects threaded in. Reconcile is switched over in a follow-up commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a shared validated_cluster_from_yaml fixture and one default_renders test per builder, pinning the migrated properties.yaml defaults (networkaddress.cache.ttl, networkaddress.cache.negative.ttl, io.trino, query.max-memory, node-scheduler.include-coordinator). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
reconcile_trino now consumes ValidatedCluster from validate (no longer
called validate_v2) and builds the rolegroup ConfigMap via
controller::build::config_map::build_rolegroup_config_map. The legacy
ValidatedInputs struct, the original validate function, and the
validated_product_config helper are deleted. Ctx no longer carries a
ProductConfigManager, and main.rs destructures product_config: _ from
RunArguments instead of loading deploy/config-spec/properties.yaml.
The legacy build_rolegroup_config_map in controller.rs and its
product-config-driven helpers remain in place (marked dead_code) until
Task 15's cleanup. build_rolegroup_statefulset now takes
env_overrides: &HashMap<String, String> directly from the new pipeline
instead of pulling it out of a PropertyNameKind-keyed map. Behavioral
parity is verified by the existing kuttl ConfigMap snapshot at
tests/templates/kuttl/smoke/14-assert.yaml.j2; no kuttl cluster was
available locally, so verification relies on CI.
The four legacy controller tests (test_config_overrides,
test_client_protocol_config_overrides, test_access_control_overrides,
test_env_overrides) were deleted because their helper depended on the
legacy ProductConfigManager API. Task 17 will rewrite them against the
new pipeline. Cargo test on stackable-trino-operator now reports 74
passing tests (down from 78).
The #[allow(dead_code)] annotations on the now-live builders have been
removed:
- controller/build/config_map.rs::build_rolegroup_config_map
- controller/build/properties/{access_control,config,exchange_manager,
log,node,security,spooling_manager}_properties::build
- controller/build/properties/writer.rs::{to_java_properties_string,Error}
- product_logging.rs::get_log_property_map
- operations/graceful_shutdown.rs::graceful_shutdown_config_properties_v2
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The legacy build_rolegroup_config_map ignored the product-config-validated map for LOG_PROPERTIES and emitted only get_log_properties output. The io.trino=INFO default in properties.yaml was dead — never reached the wire. The new builder must do the same to preserve byte-level parity with the kuttl smoke snapshot. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
HashMap iteration order is non-deterministic in Rust (RandomState seeded at process start). After Task 14's signature change, env_overrides flowed through HashMap from the framework helper into build_rolegroup_statefulset, which could shuffle env-var Vec order on subsequent reconciles and trigger unnecessary Pod restarts. The legacy product-config pipeline used a BTreeMap (sorted), so this restores byte-level determinism. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Drop product-config from workspace. - Delete `impl Configuration for TrinoConfigFragment` and `impl KeyValueOverridesProvider for TrinoConfigOverrides`. - Delete the legacy `build_rolegroup_config_map` and the helpers it exclusively relied on (replaced by `controller::build::config_map` in Task 14). Switch `build_rolegroup_catalog_config_map` to the new local writer. - Switch the authentication modules to the local `controller::build::properties::writer::to_java_properties_string`. - Delete legacy `get_log_properties` / legacy `graceful_shutdown_config_properties` helpers that no longer have callers, and rename `_v2` survivor back to its original name. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Deletes deploy/config-spec/properties.yaml and the regenerated chart copy at deploy/helm/trino-operator/configs/properties.yaml. Removes the config-spec volume + mount from deployment.yaml. The chart's ConfigMap template (templates/configmap.yaml) is preserved intentionally — it now renders an empty data block, which keeps the chart shape unchanged and lets any custom mounts users have added via overrides continue to work. The checksum/config annotation on the Deployment is harmless against an empty ConfigMap. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Rewrites the four tests that previously called ProductConfigManager::from_str(include_str!(...)) to use the new validate -> build_rolegroup_config_map pipeline directly. test_env_overrides now asserts against ValidatedCluster.role_group_configs[role][rg].env_overrides. Tests count: 74 -> 78. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The `deploy/config-spec/properties.yaml` file was deleted in commit 1854eba; this leftover Dockerfile COPY would have caused docker build to fail with "file not found". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Please add a description here. This will become the commit message of the merge request later.
Definition of Done Checklist
Author
Reviewer
Acceptance
type/deprecationlabel & add to the deprecation scheduletype/experimentallabel & add to the experimental features tracker