-
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 endpoints to generate ClickHouse config files #6476
Conversation
Heya @andrewjstone 👋 Could I get a review when you get a chance? |
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.
Generally looks good @karencfv . Great progress!
clickhouse-admin/api/src/lib.rs
Outdated
/// directory. | ||
#[endpoint { | ||
method = POST, | ||
path = "/node/keeper/generate-config/{generation}", |
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.
A few notes on the API structure here:
- We should be using REST style APIs with nouns, not verbs, similar to the rest of our APIs. I suggest
/keeper/config
. - These APIs are also idempotent and should be PUTs.
- The generation number should be part of the payload, and not a query parameter. We typically do this by wrapping the inner type in a container type with generation number.
- The
/node
prefix seems somewhat superfluous. If we need a sharable endpoint for something likestats
we can add it and name it precisely without a prefix.
) -> Result<HttpResponseCreated<KeeperConfig>, HttpError> { | ||
let ctx = rqctx.context(); | ||
let keeper_settings = body.into_inner(); | ||
let output = ctx.clickward().generate_keeper_config(keeper_settings)?; |
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.
What does the returned HttpError
look like on conversion here?
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.
$ curl -X put http://[::1]:8888/keeper/config -H "Content-Type: application/json" -d '{"generation": 3, "settings": {"node_id": 1, "raft_servers": [{"id": 1, "host": {"ipv6": "ff::01"}}, {"id": 2, "host":{"ipv4": "127.0.0.1"}}, {"id": 3, "host":{"domain_name": "hi.there"}}], "config_dir": "./bob/bob", "datastore_path": "./", "listen_addr": "::1" }}' | jq
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 439 100 175 100 264 67281 99k --:--:-- --:--:-- --:--:-- 214k
{
"request_id": "f79d5187-6ed8-4b55-ab1f-fb1b5aa010c6",
"error_code": "Internal",
"message": "clickward XML generation failure: No such file or directory (os error 2)"
}
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.
Hmm. That looks pretty good. I'm assuming Internal
means 500, which is probably correct here. I'm not an expert in http response codes 😄.
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.
clickhouse-admin/types/src/lib.rs
Outdated
pub id: KeeperId, | ||
pub raft_servers: Vec<RaftServerConfig>, | ||
/// Unique ID of the keeper | ||
pub node_id: KeeperId, |
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.
keeper_id
?
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.
How about just id
? It feels a bit redundant otherwise?
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.
Works for me.
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 taking a look @andrewjstone ! I think I've addressed all your comments. Please let me know if you think there's anything missing :)
) -> Result<HttpResponseCreated<KeeperConfig>, HttpError> { | ||
let ctx = rqctx.context(); | ||
let keeper_settings = body.into_inner(); | ||
let output = ctx.clickward().generate_keeper_config(keeper_settings)?; |
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.
$ curl -X put http://[::1]:8888/keeper/config -H "Content-Type: application/json" -d '{"generation": 3, "settings": {"node_id": 1, "raft_servers": [{"id": 1, "host": {"ipv6": "ff::01"}}, {"id": 2, "host":{"ipv4": "127.0.0.1"}}, {"id": 3, "host":{"domain_name": "hi.there"}}], "config_dir": "./bob/bob", "datastore_path": "./", "listen_addr": "::1" }}' | jq
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 439 100 175 100 264 67281 99k --:--:-- --:--:-- --:--:-- 214k
{
"request_id": "f79d5187-6ed8-4b55-ab1f-fb1b5aa010c6",
"error_code": "Internal",
"message": "clickward XML generation failure: No such file or directory (os error 2)"
}
clickhouse-admin/types/src/lib.rs
Outdated
pub id: KeeperId, | ||
pub raft_servers: Vec<RaftServerConfig>, | ||
/// Unique ID of the keeper | ||
pub node_id: KeeperId, |
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.
How about just id
? It feels a bit redundant otherwise?
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! Thanks for the fixes @karencfv.
Overview
This commit introduces two new endpoints to the
clickhouse-admin
API. One to generate a ClickHouse server XML configuration file and the other to generate a ClickHouse keeper XML configuration file.Purpose
"Reconfigurator" will need to call these endpoints to generate the necessary files when bringing up new
clickhouse_server
andclickhouse_keeper
zones. They will also be necessary to update existing zones with the keeper and server information from new zones.API structure
Moving forward the API endpoints will follow the following structure:
/keeper/
: For endpoints that will perform actions on keeper nodes solely./server/
: For endpoints that will perform actions on server nodes solely.Usage
To generate a server XML config file:
To generate a keeper XML config file:
Caveats
For the time being the generation number isn't used for anything. In a follow up PR we will use it to keep track of the configuration version to make sure we're not generating an incorrect XML configuration file.
Testing
Dropshot server:
Generate server XML file:
Generate keeper XML file:
Related: #5999 , #3824