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 internal API and omdb command to expunge a sled #5234

Merged
merged 12 commits into from
Mar 13, 2024
1 change: 1 addition & 0 deletions Cargo.lock

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

1 change: 1 addition & 0 deletions dev-tools/omdb/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ oximeter-client.workspace = true
# See omicron-rpaths for more about the "pq-sys" dependency.
pq-sys = "*"
ratatui.workspace = true
reedline.workspace = true
serde.workspace = true
serde_json.workspace = true
sled-agent-client.workspace = true
Expand Down
112 changes: 68 additions & 44 deletions dev-tools/omdb/src/bin/omdb/db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,9 +148,8 @@ impl Display for MaybeSledId {

#[derive(Debug, Args)]
pub struct DbArgs {
/// URL of the database SQL interface
#[clap(long, env("OMDB_DB_URL"))]
db_url: Option<PostgresConfigWithUrl>,
#[clap(flatten)]
db_url_opts: DbUrlOptions,

#[clap(flatten)]
fetch_opts: DbFetchOptions,
Expand All @@ -159,6 +158,71 @@ pub struct DbArgs {
command: DbCommands,
}

#[derive(Debug, Args)]
pub struct DbUrlOptions {
/// URL of the database SQL interface
#[clap(long, env("OMDB_DB_URL"))]
db_url: Option<PostgresConfigWithUrl>,
jgallagher marked this conversation as resolved.
Show resolved Hide resolved
}

impl DbUrlOptions {
async fn resolve_pg_url(
&self,
omdb: &Omdb,
log: &slog::Logger,
) -> anyhow::Result<PostgresConfigWithUrl> {
match &self.db_url {
Some(cli_or_env_url) => Ok(cli_or_env_url.clone()),
None => {
eprintln!(
"note: database URL not specified. Will search DNS."
);
eprintln!("note: (override with --db-url or OMDB_DB_URL)");
let addrs = omdb
.dns_lookup_all(
log.clone(),
internal_dns::ServiceName::Cockroach,
)
.await?;

format!(
"postgresql://root@{}/omicron?sslmode=disable",
addrs
.into_iter()
.map(|a| a.to_string())
.collect::<Vec<_>>()
.join(",")
)
.parse()
.context("failed to parse constructed postgres URL")
}
}
}

pub async fn connect(
&self,
omdb: &Omdb,
log: &slog::Logger,
) -> anyhow::Result<Arc<DataStore>> {
let db_url = self.resolve_pg_url(omdb, log).await?;
eprintln!("note: using database URL {}", &db_url);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this use the logger?

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 don't think it's supposed to, but I'm not positive. Probably a question for @davepacheco; I think the logger is only passed into all the omicron types that need one, and output from omdb itself goes to stdout or stderr.

Copy link
Contributor

@sunshowers sunshowers Mar 11, 2024

Choose a reason for hiding this comment

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

Interesting. I think one way to handle this would be to have some kind of tag passed in as a kv to indicate that the message is user-facing, and in the log handler do filtering based on that. (You can also show such messages in a different manner, e.g. "warning: <text>" instead of [timestamp WARN] text.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is one of very many things that get emitted directly to stdout/stderr. That's basically how omdb works today. There are definitely tradeoffs to doing it that way but I don't think it makes sense to change just this one and rethinking this approach seems a lot broader than this PR.


let db_config = db::Config { url: db_url.clone() };
let pool = Arc::new(db::Pool::new(&log.clone(), &db_config));

// Being a dev tool, we want to try this operation even if the schema
// doesn't match what we expect. So we use `DataStore::new_unchecked()`
// here. We will then check the schema version explicitly and warn the
// user if it doesn't match.
let datastore = Arc::new(
DataStore::new_unchecked(log.clone(), pool)
.map_err(|e| anyhow!(e).context("creating datastore"))?,
);
check_schema_version(&datastore).await;
Ok(datastore)
}
}

#[derive(Debug, Args)]
pub struct DbFetchOptions {
/// limit to apply to queries that fetch rows
Expand Down Expand Up @@ -405,47 +469,7 @@ impl DbArgs {
omdb: &Omdb,
log: &slog::Logger,
) -> Result<(), anyhow::Error> {
let db_url = match &self.db_url {
Some(cli_or_env_url) => cli_or_env_url.clone(),
None => {
eprintln!(
"note: database URL not specified. Will search DNS."
);
eprintln!("note: (override with --db-url or OMDB_DB_URL)");
let addrs = omdb
.dns_lookup_all(
log.clone(),
internal_dns::ServiceName::Cockroach,
)
.await?;

format!(
"postgresql://root@{}/omicron?sslmode=disable",
addrs
.into_iter()
.map(|a| a.to_string())
.collect::<Vec<_>>()
.join(",")
)
.parse()
.context("failed to parse constructed postgres URL")?
}
};
eprintln!("note: using database URL {}", &db_url);

let db_config = db::Config { url: db_url.clone() };
let pool = Arc::new(db::Pool::new(&log.clone(), &db_config));

// Being a dev tool, we want to try this operation even if the schema
// doesn't match what we expect. So we use `DataStore::new_unchecked()`
// here. We will then check the schema version explicitly and warn the
// user if it doesn't match.
let datastore = Arc::new(
DataStore::new_unchecked(log.clone(), pool)
.map_err(|e| anyhow!(e).context("creating datastore"))?,
);
check_schema_version(&datastore).await;

let datastore = self.db_url_opts.connect(omdb, log).await?;
let opctx = OpContext::for_tests(log.clone(), datastore.clone());
match &self.command {
DbCommands::Rack(RackArgs { command: RackCommands::List }) => {
Expand Down
26 changes: 19 additions & 7 deletions dev-tools/omdb/src/bin/omdb/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,16 +92,28 @@ struct Omdb {
command: OmdbCommands,
}

impl Omdb {
fn check_allow_destructive(&self) -> anyhow::Result<()> {
anyhow::ensure!(
self.allow_destructive,
"This command is potentially destructive. \
mod check_allow_destructive {
/// Zero-size type that potentially-destructive functions can accept to
/// ensure `Omdb::check_allow_destructive` has been called.
// This is tucked away inside a module to prevent it from being constructed
// by anything other than `Omdb::check_allow_destructive`.
pub(crate) struct DestructiveOperationToken(());
jgallagher marked this conversation as resolved.
Show resolved Hide resolved

impl super::Omdb {
pub(crate) fn check_allow_destructive(
&self,
) -> anyhow::Result<DestructiveOperationToken> {
anyhow::ensure!(
self.allow_destructive,
"This command is potentially destructive. \
Pass the `-w` / `--destructive` flag to allow it."
);
Ok(())
);
Ok(DestructiveOperationToken(()))
}
}
}

impl Omdb {
async fn dns_lookup_all(
&self,
log: slog::Logger,
Expand Down
Loading