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 omdb migrations list command #6160

Merged
merged 9 commits into from
Aug 5, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
254 changes: 249 additions & 5 deletions dev-tools/omdb/src/bin/omdb/db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,9 @@ enum DbCommands {
Instances(InstancesOptions),
/// Print information about the network
Network(NetworkArgs),
/// Print information about migrations
#[clap(alias = "migration")]
Migrations(MigrationsArgs),
/// Print information about snapshots
Snapshots(SnapshotArgs),
/// Validate the contents of the database
Expand Down Expand Up @@ -544,6 +547,75 @@ enum NetworkCommands {
ListVnics,
}

#[derive(Debug, Args)]
struct MigrationsArgs {
#[command(subcommand)]
command: MigrationsCommands,
}

#[derive(Debug, Subcommand)]
enum MigrationsCommands {
/// List migrations
#[clap(alias = "ls")]
List(MigrationsListArgs),
// N.B. I'm making this a subcommand not because there are currently any
// other subcommands, but to reserve the optionality to add things other
// than `list`...
}

#[derive(Debug, Args)]
struct MigrationsListArgs {
/// Include only migrations where at least one side reports the migration
/// is in progress.
///
/// By default, migrations in all states are included. This can be combined
/// with the `--pending`, `--completed`, and `--failed` arguments to include
/// migrations in multiple states.
#[arg(short = 'r', long, action = ArgAction::SetTrue)]
in_progress: bool,

/// Include only migrations where at least one side is still pending (the
/// sled-agent hasn't reported in yet).
///
/// By default, migrations in all states are included. This can be combined
/// with the `--in-progress`, `--completed` and `--failed` arguments to
/// include migrations in multiple states.
#[arg(short = 'p', long, action = ArgAction::SetTrue)]
pending: bool,

/// Include only migrations where at least one side reports the migration
/// has completed.
///
/// By default, migrations in all states are included. This can be combined
/// with the `--in-progress`, `--pending`, and `--failed` arguments to
/// include migrations in multiple states.
#[arg(short = 'c', long, action = ArgAction::SetTrue)]
completed: bool,

/// Include only migrations where at least one side reports the migration
/// has failed.
///
/// By default, migrations in all states are included. This can be combined
/// with the `--pending`, `--in-progress`, and --completed` arguments to
/// include migrations in multiple states.
#[arg(short = 'f', long, action = ArgAction::SetTrue)]
failed: bool,

/// Show only migrations for this instance ID.
///
/// By default, all instances are shown. This argument be repeated to select
/// other instances.
#[arg(short = 'i', long = "instance-id")]
instance_ids: Vec<Uuid>,

/// Output all data from the migration record.
///
/// This includes update and deletion timestamps, the source and target
/// generation numbers.
#[arg(short, long, action = ArgAction::SetTrue)]
verbose: bool,
}

#[derive(Debug, Args)]
struct SnapshotArgs {
#[command(subcommand)]
Expand Down Expand Up @@ -730,6 +802,11 @@ impl DbArgs {
)
.await
}
DbCommands::Migrations(MigrationsArgs {
command: MigrationsCommands::List(args),
}) => {
cmd_db_migrations_list(&datastore, &self.fetch_opts, args).await
}
DbCommands::Snapshots(SnapshotArgs {
command: SnapshotCommands::Info(uuid),
}) => cmd_db_snapshot_info(&opctx, &datastore, uuid).await,
Expand Down Expand Up @@ -2705,11 +2782,6 @@ async fn cmd_db_eips(
owner_disposition: Option<BlueprintZoneDisposition>,
}

// Display an empty cell for an Option<T> if it's None.
fn display_option_blank<T: Display>(opt: &Option<T>) -> String {
opt.as_ref().map(|x| x.to_string()).unwrap_or_else(|| "".to_string())
}

if verbose {
for ip in &ips {
if verbose {
Expand Down Expand Up @@ -4094,3 +4166,175 @@ async fn cmd_db_reconfigurator_save(
eprintln!("wrote {}", output_path);
Ok(())
}

// Migrations

async fn cmd_db_migrations_list(
datastore: &DataStore,
fetch_opts: &DbFetchOptions,
args: &MigrationsListArgs,
) -> Result<(), anyhow::Error> {
use db::model::Migration;
use db::model::MigrationState;
use db::schema::migration::dsl;
use omicron_common::api::internal::nexus;

let mut state_filters = Vec::new();
if args.completed {
state_filters.push(MigrationState(nexus::MigrationState::Completed));
}
if args.failed {
state_filters.push(MigrationState(nexus::MigrationState::Failed));
}
if args.in_progress {
state_filters.push(MigrationState(nexus::MigrationState::InProgress));
}
if args.pending {
state_filters.push(MigrationState(nexus::MigrationState::Pending));
}

let mut query = dsl::migration.into_boxed();

if !fetch_opts.include_deleted {
query = query.filter(dsl::time_deleted.is_null());
}

if !state_filters.is_empty() {
query = query.filter(
dsl::source_state
.eq_any(state_filters.clone())
.or(dsl::target_state.eq_any(state_filters)),
);
}

if !args.instance_ids.is_empty() {
query =
query.filter(dsl::instance_id.eq_any(args.instance_ids.clone()));
}

let migrations = query
.limit(i64::from(u32::from(fetch_opts.fetch_limit)))
.order_by(dsl::time_created)
// This is just to prove to CRDB that it can use the
// migrations-by-time-created index, it doesn't actually do anything.
.filter(dsl::time_created.gt(chrono::DateTime::UNIX_EPOCH))
.select(Migration::as_select())
.load_async(&*datastore.pool_connection_for_tests().await?)
.await
.context("listing migrations")?;
Comment on lines +4215 to +4224
Copy link
Collaborator

Choose a reason for hiding this comment

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

This all seems good as far as using the new index. Does it make sense to let the caller / CLI provide timestamps themselves? E.g., they could look at migrations in the last hour, if they wanted, or the code could use the UNIX epoch if nothing was provided.

Part of why I ask is we have given control of the limit in the CLI, but not the starting point.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like to add that as well, but I was thinking about doing it in a subsequent
PR. I could do it now, though!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, if you want to leave it for a follow-up, that's fine too! I think it'd be nice to have both in the limit, but not crucial they land in the same commit. Up to you :)


check_limit(&migrations, fetch_opts.fetch_limit, || "listing migrations");

#[derive(Tabled)]
#[tabled(rename_all = "SCREAMING_SNAKE_CASE")]
struct Vmms {
src_state: MigrationState,
tgt_state: MigrationState,
src_vmm: Uuid,
tgt_vmm: Uuid,
}

impl From<&'_ Migration> for Vmms {
fn from(
&Migration {
source_propolis_id,
target_propolis_id,
source_state,
target_state,
..
}: &Migration,
) -> Self {
Self {
src_state: source_state,
tgt_state: target_state,
src_vmm: source_propolis_id,
tgt_vmm: target_propolis_id,
}
}
}

let table = if args.verbose {
// If verbose mode is enabled, include the migration's ID as well as the
// source and target updated timestamps.
#[derive(Tabled)]
#[tabled(rename_all = "SCREAMING_SNAKE_CASE")]
struct VerboseMigrationRow {
created: chrono::DateTime<Utc>,
id: Uuid,
instance: Uuid,
#[tabled(inline)]
vmms: Vmms,
#[tabled(display_with = "display_option_blank")]
src_updated: Option<chrono::DateTime<Utc>>,
#[tabled(display_with = "display_option_blank")]
tgt_updated: Option<chrono::DateTime<Utc>>,
#[tabled(display_with = "display_option_blank")]
deleted: Option<chrono::DateTime<Utc>>,
}

let rows = migrations.into_iter().map(|m| VerboseMigrationRow {
id: m.id,
instance: m.instance_id,
vmms: Vmms::from(&m),
src_updated: m.time_source_updated,
tgt_updated: m.time_target_updated,
created: m.time_created,
deleted: m.time_deleted,
});

tabled::Table::new(rows)
.with(tabled::settings::Style::empty())
.with(tabled::settings::Padding::new(0, 1, 0, 0))
.to_string()
} else if args.instance_ids.len() == 1 {
// If only the migrations for a single instance are shown, we omit the
// instance ID row for conciseness sake.
#[derive(Tabled)]
#[tabled(rename_all = "SCREAMING_SNAKE_CASE")]
struct SingleInstanceMigrationRow {
created: chrono::DateTime<Utc>,
#[tabled(inline)]
vmms: Vmms,
}
let rows = migrations.into_iter().map(|m| SingleInstanceMigrationRow {
created: m.time_created,
vmms: Vmms::from(&m),
});

tabled::Table::new(rows)
.with(tabled::settings::Style::empty())
.with(tabled::settings::Padding::new(0, 1, 0, 0))
.to_string()
} else {
// Otherwise, the default format includes the instance ID, but omits
// most of the timestamps for brevity.
#[derive(Tabled)]
#[tabled(rename_all = "SCREAMING_SNAKE_CASE")]
struct MigrationRow {
created: chrono::DateTime<Utc>,
instance: Uuid,
#[tabled(inline)]
vmms: Vmms,
}

let rows = migrations.into_iter().map(|m| MigrationRow {
created: m.time_created,
instance: m.instance_id,
vmms: Vmms::from(&m),
});

tabled::Table::new(rows)
.with(tabled::settings::Style::empty())
.with(tabled::settings::Padding::new(0, 1, 0, 0))
.to_string()
};

println!("{table}");

Ok(())
}

// Display an empty cell for an Option<T> if it's None.
fn display_option_blank<T: Display>(opt: &Option<T>) -> String {
opt.as_ref().map(|x| x.to_string()).unwrap_or_else(|| "".to_string())
}
2 changes: 2 additions & 0 deletions dev-tools/omdb/tests/usage_errors.out
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ Commands:
sleds Print information about sleds
instances Print information about customer instances
network Print information about the network
migrations Print information about migrations
snapshots Print information about snapshots
validate Validate the contents of the database
volumes Print information about volumes
Expand Down Expand Up @@ -159,6 +160,7 @@ Commands:
sleds Print information about sleds
instances Print information about customer instances
network Print information about the network
migrations Print information about migrations
snapshots Print information about snapshots
validate Validate the contents of the database
volumes Print information about volumes
Expand Down
3 changes: 2 additions & 1 deletion nexus/db-model/src/schema_versions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use std::collections::BTreeMap;
///
/// This must be updated when you change the database schema. Refer to
/// schema/crdb/README.adoc in the root of this repository for details.
pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(84, 0, 0);
pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(85, 0, 0);

/// List of all past database schema versions, in *reverse* order
///
Expand All @@ -29,6 +29,7 @@ static KNOWN_VERSIONS: Lazy<Vec<KnownVersion>> = Lazy::new(|| {
// | leaving the first copy as an example for the next person.
// v
// KnownVersion::new(next_int, "unique-dirname-with-the-sql-files"),
KnownVersion::new(85, "add-migrations-by-time-created-index"),
KnownVersion::new(84, "region-read-only"),
KnownVersion::new(83, "dataset-address-optional"),
KnownVersion::new(82, "region-port"),
Expand Down
9 changes: 9 additions & 0 deletions schema/crdb/add-migrations-by-time-created-index/up.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@

/* Migrations by time created.
*
* Currently, this is only used by OMDB for ordering the `omdb migration list`
* output, but it may be used by other UIs in the future...
*/
CREATE INDEX IF NOT EXISTS migrations_by_time_created ON omicron.public.migration (
time_created
);
11 changes: 10 additions & 1 deletion schema/crdb/dbinit.sql
Original file line number Diff line number Diff line change
Expand Up @@ -4131,6 +4131,15 @@ CREATE INDEX IF NOT EXISTS lookup_migrations_by_instance_id ON omicron.public.mi
instance_id
);

/* Migrations by time created.
*
* Currently, this is only used by OMDB for ordering the `omdb migration list`
* output, but it may be used by other UIs in the future...
*/
CREATE INDEX IF NOT EXISTS migrations_by_time_created ON omicron.public.migration (
time_created
);

/* Lookup region snapshot by snapshot id */
CREATE INDEX IF NOT EXISTS lookup_region_snapshot_by_snapshot_id on omicron.public.region_snapshot (
snapshot_id
Expand All @@ -4147,7 +4156,7 @@ INSERT INTO omicron.public.db_metadata (
version,
target_version
) VALUES
(TRUE, NOW(), NOW(), '84.0.0', NULL)
(TRUE, NOW(), NOW(), '85.0.0', NULL)
ON CONFLICT DO NOTHING;

COMMIT;
Loading