-
Notifications
You must be signed in to change notification settings - Fork 39
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
[reconfigurator] clickhouse-admin
SMF service with dropshot server
#6304
Conversation
clickhouse-admin
dropshot serverclickhouse-admin
dropshot server
clickhouse-admin
dropshot serverclickhouse-admin
SMF service with dropshot server
} | ||
|
||
#[derive(Debug)] | ||
pub struct Clickward { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realise not all tasks will interact with Clickward's libraries (some sill use the clickhouse CLIs), but I like the name and can't think of anything better :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great @karencfv! I'm super psyched to have this.
Run { | ||
/// Socket address for a running clickhouse server or keeper instance | ||
#[clap(long, short = 'a', action)] | ||
clickhouse_address: SocketAddrV6, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we're actually going to need this. The config will all come from nexus such that the only thing starting up will be clickhouse-admin
. We can't actually run the server without the rest of the config, so probably no need to pass this in.
However, I'm also fine keeping it for now as it's used to test the only end point. We can remove that and the address endpoint when the time comes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's meant for testing for now. I've added a comment so it's clear that we should remove it later
Self { clickhouse_address } | ||
} | ||
|
||
pub fn clickhouse_address( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll probably want to return the entire config struct instead, but this totally works for this PR.
@@ -153,6 +153,7 @@ source.type = "composite" | |||
source.packages = [ | |||
"clickhouse_svc.tar.gz", | |||
"internal-dns-cli.tar.gz", | |||
"omicron-clickhouse-admin.tar.gz", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we actually want to add it here. There's the new ClickhouseServer
omicron zone where it should probably live, although we don't yet deploy that. Also fine to leave this for now and take it out later to help testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above it was meant for testing, left a comment to specify this :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review @andrewjstone!
Run { | ||
/// Socket address for a running clickhouse server or keeper instance | ||
#[clap(long, short = 'a', action)] | ||
clickhouse_address: SocketAddrV6, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's meant for testing for now. I've added a comment so it's clear that we should remove it later
@@ -153,6 +153,7 @@ source.type = "composite" | |||
source.packages = [ | |||
"clickhouse_svc.tar.gz", | |||
"internal-dns-cli.tar.gz", | |||
"omicron-clickhouse-admin.tar.gz", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above it was meant for testing, left a comment to specify this :)
Overview
New SMF service in
clickhouse
andclickhouse_keeper
zones which runs a dropshot server. The API contains a single/node/address
endpoint to retrieve the node's listen address. Other endpoints will be added in future PRs.Purpose
This server will be used to manage ClickHouse server and Keeper nodes. For now it performs a single basic action to keep the size of this PR small, but this server will perform other actions like generating the XML config files, retrieving the state of the node etc.
Testing
I've deployed locally with the following results:
Related: #5999