-
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] Retrieve keeper lgif information #6549
Merged
karencfv
merged 20 commits into
oxidecomputer:main
from
karencfv:keeper-cluster-membership
Sep 19, 2024
Merged
Changes from all commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
e3a40bf
Basic lgif endpoint
karencfv 78520f5
Return Lgif
karencfv 40ae8f2
generate opeapi spec
karencfv 9959fd7
improve the parsing a bit
karencfv bc1705a
better...
karencfv bb3a272
Improvement...
karencfv 3c8ada0
clean up
karencfv 83c7869
fmt
karencfv 481aad8
openapi spec
karencfv 0866bbe
Add some tests
karencfv 6c442d0
Add some docs
karencfv c8a1f89
Add some logging
karencfv 380139f
Clean up and more tests
karencfv 3b5bb45
address comments
karencfv 7a6619b
clean up
karencfv 8d1104d
address comments
karencfv 6371954
generate openapi spec
karencfv 55f59bd
add integration test
karencfv 5cbbae2
actually add the new integration test
karencfv bd3465d
Goodbye macro
karencfv File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,130 @@ | ||
// 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/. | ||
|
||
use anyhow::Result; | ||
use camino::Utf8PathBuf; | ||
use clickhouse_admin_types::Lgif; | ||
use dropshot::HttpError; | ||
use illumos_utils::{output_to_exec_error, ExecutionError}; | ||
use slog::Logger; | ||
use slog_error_chain::{InlineErrorChain, SlogInlineError}; | ||
use std::ffi::OsStr; | ||
use std::io; | ||
use std::net::SocketAddrV6; | ||
use tokio::process::Command; | ||
|
||
#[derive(Debug, thiserror::Error, SlogInlineError)] | ||
pub enum ClickhouseCliError { | ||
#[error("failed to run `clickhouse {subcommand}`")] | ||
Run { | ||
description: &'static str, | ||
subcommand: String, | ||
#[source] | ||
err: io::Error, | ||
}, | ||
#[error(transparent)] | ||
ExecutionError(#[from] ExecutionError), | ||
#[error("failed to parse command output")] | ||
Parse { | ||
description: &'static str, | ||
stdout: String, | ||
stderr: String, | ||
#[source] | ||
err: anyhow::Error, | ||
}, | ||
} | ||
|
||
impl From<ClickhouseCliError> for HttpError { | ||
fn from(err: ClickhouseCliError) -> Self { | ||
match err { | ||
ClickhouseCliError::Run { .. } | ||
| ClickhouseCliError::Parse { .. } | ||
| ClickhouseCliError::ExecutionError(_) => { | ||
let message = InlineErrorChain::new(&err).to_string(); | ||
HttpError { | ||
status_code: http::StatusCode::INTERNAL_SERVER_ERROR, | ||
error_code: Some(String::from("Internal")), | ||
external_message: message.clone(), | ||
internal_message: message, | ||
} | ||
} | ||
} | ||
} | ||
} | ||
|
||
#[derive(Debug)] | ||
pub struct ClickhouseCli { | ||
/// Path to where the clickhouse binary is located | ||
pub binary_path: Utf8PathBuf, | ||
/// Address on where the clickhouse keeper is listening on | ||
pub listen_address: SocketAddrV6, | ||
pub log: Option<Logger>, | ||
} | ||
|
||
impl ClickhouseCli { | ||
pub fn new(binary_path: Utf8PathBuf, listen_address: SocketAddrV6) -> Self { | ||
Self { binary_path, listen_address, log: None } | ||
} | ||
|
||
pub fn with_log(mut self, log: Logger) -> Self { | ||
self.log = Some(log); | ||
self | ||
} | ||
|
||
pub async fn lgif(&self) -> Result<Lgif, ClickhouseCliError> { | ||
self.keeper_client_non_interactive( | ||
"lgif", | ||
"Retrieve logically grouped information file", | ||
Lgif::parse, | ||
self.log.clone().unwrap(), | ||
) | ||
.await | ||
} | ||
|
||
async fn keeper_client_non_interactive<F, T>( | ||
&self, | ||
query: &str, | ||
subcommand_description: &'static str, | ||
parse: F, | ||
log: Logger, | ||
) -> Result<T, ClickhouseCliError> | ||
where | ||
F: FnOnce(&Logger, &[u8]) -> Result<T>, | ||
{ | ||
let mut command = Command::new(&self.binary_path); | ||
command | ||
.arg("keeper-client") | ||
.arg("--host") | ||
.arg(&format!("[{}]", self.listen_address.ip())) | ||
.arg("--port") | ||
.arg(&format!("{}", self.listen_address.port())) | ||
.arg("--query") | ||
.arg(query); | ||
|
||
let output = command.output().await.map_err(|err| { | ||
let err_args: Vec<&OsStr> = command.as_std().get_args().collect(); | ||
let err_args_parsed: Vec<String> = err_args | ||
.iter() | ||
.map(|&os_str| os_str.to_string_lossy().into_owned()) | ||
karencfv marked this conversation as resolved.
Show resolved
Hide resolved
|
||
.collect(); | ||
let err_args_str = err_args_parsed.join(" "); | ||
ClickhouseCliError::Run { | ||
description: subcommand_description, | ||
subcommand: err_args_str, | ||
err, | ||
} | ||
})?; | ||
|
||
if !output.status.success() { | ||
return Err(output_to_exec_error(command.as_std(), &output).into()); | ||
} | ||
|
||
parse(&log, &output.stdout).map_err(|err| ClickhouseCliError::Parse { | ||
description: subcommand_description, | ||
stdout: String::from_utf8_lossy(&output.stdout).to_string(), | ||
stderr: String::from_utf8_lossy(&output.stdout).to_string(), | ||
err, | ||
}) | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 think this command may be good for debugging/introspection aside from the inventory we need for reconfiguration and we should keep it.
I just want to point out that for inventory we'll likely not use this API command directly, but rather use the underlying keeper request along with another request to
/keeper/config
to get the current committed configuration. That will eliminate an extra network round trip.Unfortunately, keeper doesn't seem to provide a way to get the configuration and its log index together in one call. If it did that we could use that for inventory. I think that would be a good patch to make to keeper and the keeper cli, but until then we'll need to make two separate calls. And because of the inherent race condition of two calls, we'll likely have to call at least one endpoint twice to see that the config hasn't changed while polling. We would want to do that local to the sled-agent without a round trip from nexus.
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.
Ah, yeah definitely! I realise there would be another endpoint to collect all the information we need for inventory itself. My reasoning was that the lgif command is useful for many things, and in the spirit of keeping PRs as small and easily digestible as possible, I was going to make a separate PR with that endpoint. On hindsight I should have written that on the PRs description. Sorry about that!