refactor: extract dereference/validate pipeline from reconcile_hive by adwk67 · Pull Request #707 · stackabletech/hive-operator

adwk67 · 2026-05-12T14:58:04Z

Summary

Derive Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd on HiveRole so it can be used as a BTreeMap key and in validated structs
Extract external resource resolution (product image, S3 connection, metadata database, OPA config) into controller::dereference module with its own Snafu error enum
Extract product-config validation and config merging into validate_cluster(), producing a ValidatedHiveCluster struct that proves all validation succeeded before any Kubernetes resources are created
ValidatedHiveCluster owns the resolved product image and per-role/per-rolegroup merged configs; existing build functions are unchanged and receive parameters from the validated structs

Reviewer notes

dereference() and validate_cluster() contain no new logic — they are pure extractions of code that was previously inline in reconcile_hive()
The ValidatedHiveCluster struct intentionally has fewer fields than a full ownership model would (no name/namespace/uid/metadata validated types). This is a "construct but decompose" fail-fast gate: built early in reconcile to prove validation passes, then its fields feed the existing unchanged build functions
The controller/ directory coexists with controller.rs — Rust treats controller.rs as the module root and looks for submodules in controller/. Currently just dereference, with validate and further stages to follow in later PRs
The Dereference wrapper variant in the controller's Error enum replaces 3 individual error variants (ResolveProductImage, InvalidOpaConfig, InvalidMetadataDatabaseConnection) that moved into the new module's own error enum. ConfigureS3Connection and ObjectHasNoNamespace remain in the controller because they are still used by build functions
Mirrors the pattern established in the airflow-operator (refactor: extract dereference/validate pipeline from reconcile_airflow airflow-operator#795)

Test plan

Existing unit tests pass
Clean compile with no warnings
No behavioural changes — pure refactoring

🤖 Generated with Claude Code

Move external resource resolution (product image, S3 connection, metadata database, OPA config) into controller::dereference module with its own error enum. Extract config validation and merging into validate_cluster(), which produces a ValidatedHiveCluster proving all product-config validation succeeded before any Kubernetes resources are created. The validated struct owns the resolved product image and per-role/ per-rolegroup merged configs. Existing build functions are unchanged and receive their parameters from the validated structs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

adwk67 · 2026-05-13T08:53:30Z

Jenkins: 🟢 (one failure on NS teardown) https://testing.stackable.tech/view/02%20Operator%20Tests%20(custom)/job/hive-operator-it-custom/52/

Rename FailedToResolveResourceConfig to FailedToResolveConfig and fix OPA error display string to match the convention used across all three dereference/validate extraction PRs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Image resolution is a pure computation, not an I/O dereference, so it belongs in validate_cluster alongside the other config validation. This aligns with the pattern used by the trino and airflow operators. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

siegfriedweber · 2026-05-15T06:39:59Z

+    let metadata_database_connection_details = hive
+        .spec
+        .cluster_config
+        .metadata_database
+        .jdbc_connection_details("METADATA")
+        .context(InvalidMetadataDatabaseConnectionSnafu)?;


The database connection string is just validated. No resource is dereferenced. Therefore, I would move this code into the validation step.

siegfriedweber · 2026-05-15T06:52:04Z

+/// The validated cluster: proves that product-config validation and config merging
+/// succeeded for every role and role group before any resources are created.
+#[derive(Clone, Debug)]
+pub struct ValidatedHiveCluster {


I would not put the product name into the identifiers where it is not necessary. The context of this operator is Hive, therefore the validated cluster struct contains the validated configuration for Hive.

There are often tasks which must be applied to all operators. It is easier to apply them if the identifiers are equal and do not contain the product name.

Suggested change

pub struct ValidatedHiveCluster {

pub struct ValidatedCluster {

I'm in two minds about this: on the one hand it makes sense to have it generic (like I commented in the Trino PR), but on the other, I can see the downside of having generically-named structs that have very different fields.

(that's a very minor downside)

siegfriedweber · 2026-05-15T06:54:16Z

+/// The validated cluster: proves that product-config validation and config merging
+/// succeeded for every role and role group before any resources are created.
+#[derive(Clone, Debug)]
+pub struct ValidatedHiveCluster {


I would move all structs which are used by other steps, into the controller module, so that all steps depend on the parent module but not on other step modules. The ValidatedCluster struct will e.g. be used in the build step.

See 1758748. Although I would maybe have moved the struct over once dependencies are clear (i.e. when implementing subsequent steps).

siegfriedweber · 2026-05-15T07:03:43Z

+        .resolve(image_base_name, image_repository, pkg_version)
+        .context(ResolveProductImageSnafu)?;
+
+    let role = hive.spec.metastore.as_ref().context(NoMetaStoreRoleSnafu)?;


This is okay in this pull request, because we do not want to introduce CRD changes. But I wonder why this field is defined as optional in the CRD if it is actually mandatory. If we want to build a lean operator, we should not try to fix problems we never should have created.

I seem to remember that some of the Option-ality was to do with the fragment/merge mechanism, but not sure if that is relevant here.

siegfriedweber · 2026-05-15T07:19:15Z

+    pub role_groups: BTreeMap<HiveRole, BTreeMap<String, ValidatedRoleGroupConfig>>,
+    pub role_configs: BTreeMap<HiveRole, ValidatedRoleConfig>,


If there is only one role and we never intend to add another role, wouldn't it be better to skip the indirection with the singleton BTreeMap? Otherwise, all usages are prepended with an if let Some(role_config) = ....

Suggested change

pub role_groups: BTreeMap<HiveRole, BTreeMap<String, ValidatedRoleGroupConfig>>,

pub role_configs: BTreeMap<HiveRole, ValidatedRoleConfig>,

pub role_groups: BTreeMap<String, ValidatedRoleGroupConfig>,

pub role_configs: ValidatedRoleConfig,

We had new roles "appear" for Kafka (with KRaft), but, yes, I think that is very unlikely for Hive.

See 6db46c1. A single role is now assumed for the validated cluster which means the loops in the reconciler can also be simplified.

siegfriedweber · 2026-05-15T07:29:45Z

+pub fn validate_cluster(
+    hive: &v1alpha1::HiveCluster,
+    image_base_name: &str,
+    image_repository: &str,
+    pkg_version: &str,
+    product_config_manager: &ProductConfigManager,
+) -> Result<ValidatedHiveCluster, Error> {


Is it useful to have parameters for image_base_name and pkg_version which are always populated with constants?

Isn't pkg_version only determined at runtime?

See 9cdd1e1 for image base name.

siegfriedweber · 2026-05-15T07:44:36Z

+}
+
+/// External references resolved during the dereference step.
+pub struct DereferencedObjects {


The DereferencedObjects struct usually contains unvalidated resources. For instance, some resources are required with certain contents depending on the cluster specification. Therefore, the dereferenced objects are validated in the validation step and stored (sometimes converted) in the ValidatedCluster struct. The ValidatedCluster struct is then used in the following steps. The DereferencedObjectsshould only be necessary to hand over the resources to the validation step.

Are you requesting a change here?

siegfriedweber · 2026-05-15T08:04:20Z

The smoke test of the opensearch-operator has an assertion that covers all generated resources (https://github.com/stackabletech/opensearch-operator/blob/26.3.0/tests/templates/kuttl/smoke/10-assert.yaml.j2). The other integration tests only add assertions for the resources specific to the test case (https://github.com/stackabletech/opensearch-operator/blob/26.3.0/tests/templates/kuttl/security-config/11-assert.yaml#L29-L41). That leads to relatively high test coverage.

The pull requests for stackabletech/issues#850 should only perform internal changes. If we also add an assertion to the smoke test with the resources produced before this change, then we could be more confident that no unwanted changes are introduced.

Co-authored-by: Siegfried Weber <mail@siegfriedweber.net>

…move derives where they do not exist on the upstream type

…pendency for subsequent steps

adwk67 self-assigned this May 12, 2026

refactor: extract validate_cluster into controller::validate module

6da580c

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

This was referenced May 13, 2026

tracking: Smooth Operator Iteration 1: Dereferencing and cluster validation stackabletech/issues#850

Open

refactor: extract dereference/validate pipeline from reconcile_hbase stackabletech/hbase-operator#757

Open

adwk67 marked this pull request as ready for review May 13, 2026 10:12

siegfriedweber self-requested a review May 13, 2026 14:09

siegfriedweber added this to Stackable Engineering May 13, 2026

siegfriedweber moved this to Development: In Review in Stackable Engineering May 13, 2026

adwk67 and others added 3 commits May 13, 2026 16:58

changelog

213bac3

Merge branch 'main' into feature/validated-config-types-2

49e4a40

siegfriedweber requested changes May 15, 2026

View reviewed changes

siegfriedweber reviewed May 15, 2026

View reviewed changes

Comment thread rust/operator-binary/src/controller.rs Outdated

adwk67 and others added 5 commits May 15, 2026 13:24

Update rust/operator-binary/src/controller.rs

55ed469

Co-authored-by: Siegfried Weber <mail@siegfriedweber.net>

assume single role in ValidatedHiveCluster

6db46c1

move db connection details from dereferenced to the controller and re…

bd9b277

…move derives where they do not exist on the upstream type

move validated cluster to the controller which will be the central de…

1758748

…pendency for subsequent steps

replace parameter with constant

9cdd1e1

	pub struct ValidatedHiveCluster {
	pub struct ValidatedCluster {

		pub role_groups: BTreeMap<HiveRole, BTreeMap<String, ValidatedRoleGroupConfig>>,
		pub role_configs: BTreeMap<HiveRole, ValidatedRoleConfig>,

Uh oh!

Conversation

adwk67 commented May 12, 2026

Summary

Reviewer notes

Test plan

Uh oh!

adwk67 commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adwk67 May 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adwk67 May 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

adwk67 commented May 13, 2026 •

edited

Loading

adwk67 May 15, 2026 •

edited

Loading

adwk67 May 15, 2026 •

edited

Loading