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

Single node clickhouse init #6903

Merged
merged 9 commits into from
Nov 19, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 23 additions & 6 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ members = [
"clients/bootstrap-agent-client",
"clients/clickhouse-admin-keeper-client",
"clients/clickhouse-admin-server-client",
"clients/clickhouse-admin-single-client",
"clients/cockroach-admin-client",
"clients/ddm-admin-client",
"clients/dns-service-client",
Expand Down Expand Up @@ -137,6 +138,7 @@ default-members = [
"clients/bootstrap-agent-client",
"clients/clickhouse-admin-keeper-client",
"clients/clickhouse-admin-server-client",
"clients/clickhouse-admin-single-client",
"clients/cockroach-admin-client",
"clients/ddm-admin-client",
"clients/dns-service-client",
Expand Down Expand Up @@ -329,6 +331,7 @@ clap = { version = "4.5", features = ["cargo", "derive", "env", "wrap_help"] }
clickhouse-admin-api = { path = "clickhouse-admin/api" }
clickhouse-admin-keeper-client = { path = "clients/clickhouse-admin-keeper-client" }
clickhouse-admin-server-client = { path = "clients/clickhouse-admin-server-client" }
clickhouse-admin-single-client = { path = "clients/clickhouse-admin-single-client" }
clickhouse-admin-types = { path = "clickhouse-admin/types" }
clickhouse-admin-test-utils = { path = "clickhouse-admin/test-utils" }
clickward = { git = "https://github.com/oxidecomputer/clickward", rev = "a1b342c2558e835d09e6e39a40d3de798a29c2f" }
Expand Down
2 changes: 1 addition & 1 deletion clickhouse-admin/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ http.workspace = true
illumos-utils.workspace = true
omicron-common.workspace = true
omicron-uuid-kinds.workspace = true
oximeter-db.workspace = true
schemars.workspace = true
slog.workspace = true
slog-async.workspace = true
Expand All @@ -35,7 +36,6 @@ clickhouse-admin-test-utils.workspace = true
dropshot.workspace = true
expectorate.workspace = true
omicron-test-utils.workspace = true
oximeter-db.workspace = true
oximeter-test-utils.workspace = true
openapi-lint.workspace = true
openapiv3.workspace = true
Expand Down
24 changes: 23 additions & 1 deletion clickhouse-admin/api/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ use clickhouse_admin_types::{
ServerConfigurableSettings,
};
use dropshot::{
HttpError, HttpResponseCreated, HttpResponseOk, RequestContext, TypedBody,
HttpError, HttpResponseCreated, HttpResponseOk,
HttpResponseUpdatedNoContent, RequestContext, TypedBody,
};

/// API interface for our clickhouse-admin-keeper server
Expand Down Expand Up @@ -116,3 +117,24 @@ pub trait ClickhouseAdminServerApi {
rqctx: RequestContext<Self::Context>,
) -> Result<HttpResponseOk<Vec<DistributedDdlQueue>>, HttpError>;
}

/// API interface for our clickhouse-admin-single server
///
/// The single-node server is distinct from the both the multi-node servers
/// and its keepers. The sole purpose of this API is to serialize database
/// initialization requests from reconfigurator execution. Multi-node clusters
/// must eventually implement a similar interface, but the implementation will
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I'd probably avoid speculating here about implementation time and detail and only state that multi-node clusters must implement a similar interface through clickhouse-admin-server

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, fixed in 2ddcbd9.

/// obviously be more complex.
#[dropshot::api_description]
pub trait ClickhouseAdminSingleApi {
type Context;

/// Idempotently initialize a single-node ClickHouse database.
#[endpoint {
method = PUT,
path = "/init"
}]
async fn init_db(
rqctx: RequestContext<Self::Context>,
) -> Result<HttpResponseUpdatedNoContent, HttpError>;
}
72 changes: 72 additions & 0 deletions clickhouse-admin/src/bin/clickhouse-admin-single.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at https://mozilla.org/MPL/2.0/.

//! Single-node ClickHouse database admin binary.

use anyhow::anyhow;
use camino::Utf8PathBuf;
use clap::Parser;
use omicron_clickhouse_admin::{ClickhouseCli, Config};
use omicron_common::cmd::fatal;
use omicron_common::cmd::CmdError;
use std::net::{SocketAddr, SocketAddrV6};

#[derive(Debug, Parser)]
#[clap(
name = "clickhouse-admin-single",
about = "Single-node ClickHouse admin server"
)]
enum Args {
/// Start the single-node ClickHouse admin server
Run {
/// Address on which this server should run
#[clap(long, short = 'a', action)]
http_address: SocketAddrV6,

/// Path to the server configuration file
#[clap(long, short, action)]
config: Utf8PathBuf,
karencfv marked this conversation as resolved.
Show resolved Hide resolved

/// Address of the ClickHouse single-node database server
#[clap(long, short = 'l', action)]
listen_address: SocketAddrV6,

/// Path to the clickhouse binary
#[clap(long, short, action)]
binary_path: Utf8PathBuf,
},
}

#[tokio::main]
async fn main() {
if let Err(err) = main_impl().await {
fatal(err);
}
}

async fn main_impl() -> Result<(), CmdError> {
let args = Args::parse();

match args {
Args::Run { http_address, config, listen_address, binary_path } => {
let mut config = Config::from_file(&config)
.map_err(|err| CmdError::Failure(anyhow!(err)))?;
config.dropshot.bind_address = SocketAddr::V6(http_address);
let clickhouse_cli =
ClickhouseCli::new(binary_path, listen_address);

let server = omicron_clickhouse_admin::start_single_admin_server(
clickhouse_cli,
config,
)
.await
.map_err(|err| CmdError::Failure(anyhow!(err)))?;
server.await.map_err(|err| {
CmdError::Failure(anyhow!(
"server failed after starting: {err}"
))
})
}
}
}
21 changes: 21 additions & 0 deletions clickhouse-admin/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

use crate::{ClickhouseCli, Clickward};
use slog::Logger;
use std::sync::Arc;
use tokio::sync::Mutex;

pub struct ServerContext {
clickward: Clickward,
Expand All @@ -28,3 +30,22 @@ impl ServerContext {
&self.clickhouse_cli
}
}

pub struct SingleServerContext {
clickhouse_cli: ClickhouseCli,
db_initialized: Arc<Mutex<bool>>,
}

impl SingleServerContext {
pub fn new(clickhouse_cli: ClickhouseCli) -> Self {
Self { clickhouse_cli, db_initialized: Arc::new(Mutex::new(false)) }
}

pub fn clickhouse_cli(&self) -> &ClickhouseCli {
&self.clickhouse_cli
}

pub fn db_initialized(&self) -> Arc<Mutex<bool>> {
self.db_initialized.clone()
}
}
67 changes: 61 additions & 6 deletions clickhouse-admin/src/http_entrypoints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,31 +2,40 @@
// License, v. 2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at https://mozilla.org/MPL/2.0/.

use crate::context::ServerContext;
use crate::context::{ServerContext, SingleServerContext};
use clickhouse_admin_api::*;
use clickhouse_admin_types::{
ClickhouseKeeperClusterMembership, DistributedDdlQueue, KeeperConf,
KeeperConfig, KeeperConfigurableSettings, Lgif, RaftConfig, ReplicaConfig,
ServerConfigurableSettings,
};
use dropshot::{
HttpError, HttpResponseCreated, HttpResponseOk, RequestContext, TypedBody,
ApiDescription, HttpError, HttpResponseCreated, HttpResponseOk,
HttpResponseUpdatedNoContent, RequestContext, TypedBody,
};
use illumos_utils::svcadm::Svcadm;
use omicron_common::address::CLICKHOUSE_TCP_PORT;
use oximeter_db::{Client as OximeterClient, OXIMETER_VERSION};
use slog::debug;
use std::net::SocketAddrV6;
use std::sync::Arc;

type ClickhouseApiDescription = dropshot::ApiDescription<Arc<ServerContext>>;

pub fn clickhouse_admin_server_api() -> ClickhouseApiDescription {
pub fn clickhouse_admin_server_api() -> ApiDescription<Arc<ServerContext>> {
clickhouse_admin_server_api_mod::api_description::<ClickhouseAdminServerImpl>()
.expect("registered entrypoints")
}

pub fn clickhouse_admin_keeper_api() -> ClickhouseApiDescription {
pub fn clickhouse_admin_keeper_api() -> ApiDescription<Arc<ServerContext>> {
clickhouse_admin_keeper_api_mod::api_description::<ClickhouseAdminKeeperImpl>()
.expect("registered entrypoints")
}

pub fn clickhouse_admin_single_api() -> ApiDescription<Arc<SingleServerContext>>
{
clickhouse_admin_single_api_mod::api_description::<ClickhouseAdminSingleImpl>()
.expect("registered entrypoints")
}

enum ClickhouseAdminServerImpl {}

impl ClickhouseAdminServerApi for ClickhouseAdminServerImpl {
Expand Down Expand Up @@ -110,3 +119,49 @@ impl ClickhouseAdminKeeperApi for ClickhouseAdminKeeperImpl {
Ok(HttpResponseOk(output))
}
}

enum ClickhouseAdminSingleImpl {}

impl ClickhouseAdminSingleApi for ClickhouseAdminSingleImpl {
type Context = Arc<SingleServerContext>;

async fn init_db(
rqctx: RequestContext<Self::Context>,
) -> Result<HttpResponseUpdatedNoContent, HttpError> {
let log = &rqctx.log;
let ctx = rqctx.context();
let initialized = ctx.db_initialized();
let mut initialized = initialized.lock().await;
if !*initialized {
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if we want to update the schema? Is there any harm in just letting initialization run through every time? It's supposed to be idempotent, right?

CC @bnaecker

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's supposed to be idempotent, right?

Yeah, schema updates are idempotent, and also a no-op if the version in the database is at least as high as that on the client side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We talked a little about this in an update meeting a few weeks ago; my understanding was that letting initialization run each time and therefore update the schema would represent a change to our current policy of not performing automatic schema updates. This was one of the reasons for preferring a server-side approach rather than initializing the database on every client connection. I'm fine either way; it's a trivial change to the code, but a potentially less trivial change in policy.

Copy link
Contributor

Choose a reason for hiding this comment

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

We talked a little about this in an update meeting a few weeks ago; my understanding was that letting initialization run each time and therefore update the schema would represent a change to our current policy of not performing automatic schema updates. This was one of the reasons for preferring a server-side approach rather than initializing the database on every client connection. I'm fine either way; it's a trivial change to the code, but a potentially less trivial change in policy.

Thanks for reminding me of that discussion Alex. However:

  • Currently the schema version is being issued based on a constant in the code, so it won't be updated at all unless the code changes.
  • If the admin server crashes or the sled is rebooted, the in-memory state will be lost and the schema will be re-initialized anyway.

I think what we'll want to do in a follow up is to make schema updates part of the planner, and then only execute them when the plan changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, and the API call should then take the schema version to update from the executor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, those are excellent points; you've convinced me. 012b6a1 does unconditional (but still serialized) database initialization. We'll have to take explicit versioning as a follow-up.

let http_address = ctx.clickhouse_cli().listen_address;
let native_address = SocketAddrV6::new(
*http_address.ip(),
CLICKHOUSE_TCP_PORT,
0,
0,
);

let client = OximeterClient::new(
http_address.into(),
native_address.into(),
log,
);
debug!(
log,
"initializing single-node ClickHouse \
at {http_address} to version {OXIMETER_VERSION}"
);
client
.initialize_db_with_version(false, OXIMETER_VERSION)
.await
.map_err(|e| {
HttpError::for_internal_error(format!(
"can't initialize single-node ClickHouse \
at {http_address} to version {OXIMETER_VERSION}: {e}",
))
})?;
*initialized = true;
}
Ok(HttpResponseUpdatedNoContent())
}
}
Loading
Loading