Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add clickhouse-cluster-config to omdb blueprint output #6968

Merged
merged 10 commits into from
Nov 12, 2024

Conversation

andrewjstone
Copy link
Contributor

@andrewjstone andrewjstone commented Oct 31, 2024

Print clickhouse cluster config related tables for blueprint show and
blueprint diff omdb commands.

A bunch of the complexity and duplication here arises from the fact
that we are diffing not only between blueprints that have identical
structures, but between blueprints and collections that have
drastically different contents. This is useful, but we probably should
consider separating the two types of diffs and reworking all the
blueprint diff logic to use some sort of semantic diff between types
such as https://github.com/distil/diffus as recommended by @sunshowers.

In order to make the cluster_secret UUID generation deterministic for
tests I had use an rng seed in the BlueprintBuilder. This required moving
creation of the initial ClickhouseClusterConfig and it's wrapping
ClickhouseAllocator from BlueprintBuilder::new_based_on to
BlueprintBuilder::build.

Fixes #6941

@andrewjstone andrewjstone force-pushed the clickhouse-config-in-blueprint-display branch from de8cef1 to caa075b Compare November 7, 2024 00:58
@andrewjstone andrewjstone marked this pull request as ready for review November 7, 2024 00:58
@andrewjstone andrewjstone changed the title WIP: Add clickhouse-cluster-config to omdb blueprint output Add clickhouse-cluster-config to omdb blueprint output Nov 7, 2024
Print clickhouse cluster config related tables for `blueprint show` and
`blueprint diff` omdb commands.

A bunch of the complexity and duplication here arises from the fact
that  we are diffing not only between blueprints that have identical
structures, but between a blueprints and collections that have
drastically different contents. This is useful, but we probably should
consider a separating the two types of diffs and reworking all the
blueprint diff logic to use some sort of semantic diff between types
such as https://github.com/distil/diffus as recommended by @sunshowers.

This still needs some manual testing, and potentially some expectorate
based testing depending upon what those tests currently do.
@andrewjstone andrewjstone force-pushed the clickhouse-config-in-blueprint-display branch from caa075b to d147f4c Compare November 7, 2024 00:59
@andrewjstone
Copy link
Contributor Author

Fixes ##6941

@andrewjstone
Copy link
Contributor Author

Expectorate tests added. Ready for review.

Copy link
Contributor

@jgallagher jgallagher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the long wait on this one! LGTM

Comment on lines 513 to 515
let Some(config) = &self.blueprint.clickhouse_cluster_config else {
return None;
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tiny nit - I think this could be

Suggested change
let Some(config) = &self.blueprint.clickhouse_cluster_config else {
return None;
};
let config = self.blueprint.clickhouse_cluster_config.as_ref()?;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Silly me. Thanks! Fixed in 81e85f5

diff_table.keepers,
// Safety: The call to
// `ClickhouseClusterConfigDiffTables::single_blueprint_table`
// always returns all tables.
Copy link
Contributor

@jgallagher jgallagher Nov 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't looked at ClickhouseClusterConfigDiffTables yet, but leaving a note mostly for myself for when I get to it: could it return a different type so we don't have to unwrap here?

Edit: followed up in #6968 (comment)

@@ -176,6 +192,9 @@ pub trait BpSledSubtableData {
}

/// A table specific to a sled resource, such as a zone or disk.
//
// TODO: Rename to `BpTable` since it is also used for blueprint wide tables
// like those relating to `ClickhouseClusterConfig`?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd vote yes on this, I think? Nothing in this table is sled-specific, AFAICT; it's more like BpGenerationTable or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd vote yes on this, I think? Nothing in this table is sled-specific, AFAICT; it's more like BpGenerationTable or something.

Done in f0ae7db

I thought this was going to be a larger change. I liked BpGenerationTable, but went with the shorter BpTable because of the ripple of changes to other type names.

..
},
None,
) => Some(ClickhouseClusterConfigDiffTables::removed_from_collection(before)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

W.r.t. my question earlier about the unwrap; I think the situation is:

  • The only method that doesn't fill in ClickhouseClusterConfigDiffTables::servers is removed_from_collection()
  • make_clickhouse_cluster_config_diff_tables() can therefore also return a servers value of None

I guess we could make a second diff table type where servers is not optional, return it from all the methods except this one and removed_from_collection(), and add a From impl to convert it into ClickhouseClusterConfigDiffTables? Doesn't seem terrible, but does seem like a fair bit of busywork to avoid a single .unwrap(). I'd probably do it, but if you'd rather not that's fine with me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion @jgallagher. I was unhappy with this unwrap as well, especially since the SAFETY message would become invalid if someone changed a method far away from that message.

Fix made in 81e85f5

@andrewjstone andrewjstone enabled auto-merge (squash) November 12, 2024 20:22
@andrewjstone andrewjstone merged commit 45f5f1c into main Nov 12, 2024
16 checks passed
@andrewjstone andrewjstone deleted the clickhouse-config-in-blueprint-display branch November 12, 2024 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

omdb: Add clickhouse-config to blueprint output
2 participants