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

wicket cli: add "inventory configured-bootstrap-sleds" #6218

Merged
merged 11 commits into from
Aug 9, 2024
9 changes: 8 additions & 1 deletion wicket/src/cli/command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use anyhow::Result;
use clap::{Args, ColorChoice, Parser, Subcommand};

use super::{
preflight::PreflightArgs, rack_setup::SetupArgs,
inventory::InventoryArgs, preflight::PreflightArgs, rack_setup::SetupArgs,
rack_update::RackUpdateArgs, upload::UploadArgs,
};

Expand Down Expand Up @@ -49,6 +49,9 @@ impl ShellApp {
args.exec(log, wicketd_addr, self.global_opts).await
}
ShellCommand::Preflight(args) => args.exec(log, wicketd_addr).await,
ShellCommand::Inventory(args) => {
args.exec(log, wicketd_addr, output).await
}
}
}
}
Expand Down Expand Up @@ -100,4 +103,8 @@ enum ShellCommand {
/// Run checks prior to setting up the rack.
#[command(subcommand)]
Preflight(PreflightArgs),

/// Enumerate rack components
#[command(subcommand)]
Inventory(InventoryArgs),
}
110 changes: 110 additions & 0 deletions wicket/src/cli/inventory.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
// 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/.

//! Support for inventory checks via wicketd.

use crate::cli::CommandOutput;
use crate::wicketd::create_wicketd_client;
use anyhow::Context;
use anyhow::Result;
use clap::Subcommand;
use owo_colors::OwoColorize;
use sled_hardware_types::Baseboard;
use slog::Logger;
use std::net::SocketAddrV6;
use std::time::Duration;
use wicket_common::rack_setup::BootstrapSledDescription;

const WICKETD_TIMEOUT: Duration = Duration::from_secs(5);

#[derive(Debug, Subcommand)]
pub(crate) enum InventoryArgs {
/// List state of all bootstrap sleds, as configured with rack-setup
ConfiguredBootstrapSleds {
/// Print output as json
#[clap(long)]
json: bool,
Copy link
Member

Choose a reason for hiding this comment

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

suuuper nitpicky, doesn't actually matter: elsewhere, when writing CLI commands with multiple output formats, i like to define an output format enum and have a --output plain or --output text in addition to --output json/--output csv or what-have-you, rather than having a boolean flag for a specific format. that has two very small advantages IMO:

  1. if adding an additional output format, it makes it a bit clearer in the CLI that the output formats are mutually exclusive,
  2. when programmatically invoking the command, it's a bit nicer to just have to change the value of an arg based on the desired output format, rather than conditionally add or not-add an arg. e.g., a script invoking this can just always add an --output flag and pass plain or json, rather than adding/removing the --json.

but, OTTOH, this is a bit more code as it requires adding an enum and stuff...and, if other wicket commands have a --json already (AFAIK they don't, but i may have missed something), let's keep it consistent.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed about this! I'd use --format json and --format human.

},
}

impl InventoryArgs {
pub(crate) async fn exec(
self,
log: Logger,
wicketd_addr: SocketAddrV6,
mut output: CommandOutput<'_>,
) -> Result<()> {
let client = create_wicketd_client(&log, wicketd_addr, WICKETD_TIMEOUT);

match self {
InventoryArgs::ConfiguredBootstrapSleds { json } => {
// We don't use the /bootstrap-sleds endpoint, because that
// gets all sleds visible on the bootstrap network. We want
// something subtly different here.
// - We want the status of only sleds we've configured wicket
// to use for setup. /bootstrap-sleds will give us sleds
// we don't want
// - We want the status even if they aren't visible on the
// bootstrap network yet.
//
// In other words, we want the sled information displayed at the
// bottom of the rack setup screen in the TUI, and we get it the
// same way it does.
let conf = client
.get_rss_config()
.await
.context("failed to get rss config")?;

let bootstrap_sleds = &conf.insensitive.bootstrap_sleds;
if json {
let json_str = serde_json::to_string(bootstrap_sleds)
Copy link
Member

Choose a reason for hiding this comment

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

nitpicky, take it or leave it: maybe nicer to use

Suggested change
let json_str = serde_json::to_string(bootstrap_sleds)
let json_str = serde_json::to_string_pretty(bootstrap_sleds)

here, since there isn't a ton of JSON and we're not trying to send it over the network? that way, a human user gets more readable output by default, which is maybe nice if the thing consuming the JSON chokes on it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, love the idea of using to_string_pretty.

.context("serializing sled data failed")?;
writeln!(output.stdout, "{}", json_str)
.expect("writing to stdout failed");
} else {
for sled in bootstrap_sleds {
print_bootstrap_sled_data(sled, &mut output);
}
}

Ok(())
}
}
}
}

fn print_bootstrap_sled_data(
desc: &BootstrapSledDescription,
output: &mut CommandOutput<'_>,
) {
let slot = desc.id.slot;

let identifier = match &desc.baseboard {
Baseboard::Gimlet { identifier, .. } => identifier.clone(),
Baseboard::Pc { identifier, .. } => identifier.clone(),
Baseboard::Unknown => "unknown".to_string(),
};

let address = desc.bootstrap_ip;

// Create status indicators
let status = match address {
None => format!("{}", '⚠'.red()),
Some(_) => format!("{}", '✔'.green()),
Comment on lines +116 to +117
Copy link
Member

Choose a reason for hiding this comment

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

IIUC, the colored crate's .red() and .green() return a ColoredString, which is the same type either way, so i don't think we need to format! these?

Suggested change
None => format!("{}", '⚠'.red()),
Some(_) => format!("{}", '✔'.green()),
None => '⚠'.red(),
Some(_) => '✔'.green(),

although maybe i'm overlooking something...

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I think we use owo-colors here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah i think red() and green() here return Display or something like that.

};

let addr_fmt = match address {
None => "(not available)".to_string(),
Copy link
Member

Choose a reason for hiding this comment

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

nitpicky: looks like there's no whitespace in this case and it's smushed up against the identifier field? should there maybe be a tab or space here?

Copy link
Contributor Author

@faithanalog faithanalog Aug 9, 2024

Choose a reason for hiding this comment

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

yeah. artifact from my first pass where i didnt print anything when there wasnt an address.

Some(addr) => format!("\t{}", addr),
};

// Print out this entry. We say "Cubby" rather than "Slot" here purely
// because the TUI also says "Cubby".
writeln!(
output.stdout,
"{status} Cubby {:02}\t{identifier}{addr_fmt}",
slot
)
.expect("writing to stdout failed");
}
1 change: 1 addition & 0 deletions wicket/src/cli/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
//! support for that.

mod command;
mod inventory;
mod preflight;
mod rack_setup;
mod rack_update;
Expand Down
60 changes: 60 additions & 0 deletions wicketd/tests/integration_tests/inventory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ use std::time::Duration;
use super::setup::WicketdTestContext;
use gateway_messages::SpPort;
use gateway_test_utils::setup as gateway_setup;
use sled_hardware_types::Baseboard;
use wicket::OutputKind;
use wicket_common::inventory::{SpIdentifier, SpType};
use wicket_common::rack_setup::BootstrapSledDescription;
use wicketd_client::types::{GetInventoryParams, GetInventoryResponse};

#[tokio::test]
Expand Down Expand Up @@ -45,5 +49,61 @@ async fn test_inventory() {
// 4 SPs attached to the inventory.
assert_eq!(inventory.sps.len(), 4);

// Test CLI command
{
let args = vec!["inventory", "configured-bootstrap-sleds", "--json"];
let mut stdout = Vec::new();
let mut stderr = Vec::new();
let output = OutputKind::Captured {
log: wicketd_testctx.log().clone(),
stdout: &mut stdout,
stderr: &mut stderr,
};

wicket::exec_with_args(wicketd_testctx.wicketd_addr, args, output)
.await
.expect("wicket inventory configured-bootstrap-sleds failed");

// stdout should contain a JSON object.
let response: Vec<BootstrapSledDescription> =
serde_json::from_slice(&stdout).expect("stdout is valid JSON");

// This only tests the case that we get sleds back with no current
// bootstrap IP. This does provide svalue: it check that the command
// exists, accesses data within wicket, and returns it in the schema we
// expect. But it does not test the case where a sled does have a
// bootstrap IP.
//
// Unfortunately, that's a difficult thing to test today. Wicket gets
// that information by enumerating the IPs on the bootstrap network and
// reaching out to the bootstrap_agent on them directly to ask them who
// they are. Our testing setup does not have a way to provide such an
// IP, or run a bootstrap_agent on an IP to respond. We should update
// this test when we do have that capabilitiy.
assert_eq!(
response,
vec![
BootstrapSledDescription {
id: SpIdentifier { type_: SpType::Sled, slot: 0 },
baseboard: Baseboard::Gimlet {
identifier: "SimGimlet00".to_string(),
model: "i86pc".to_string(),
revision: 0
},
bootstrap_ip: None
},
BootstrapSledDescription {
id: SpIdentifier { type_: SpType::Sled, slot: 1 },
baseboard: Baseboard::Gimlet {
identifier: "SimGimlet01".to_string(),
model: "i86pc".to_string(),
revision: 0
},
bootstrap_ip: None
},
]
);
}

wicketd_testctx.teardown().await;
}
Loading