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

faithanalog · 2024-08-03T02:51:44Z

This PR

My main motivation for adding: I am working on automating control plane deployment on london and madrid. Before we can start rack init, we need to know if all the sleds we are going to initialize are actually available. This information is something you can see from the rack init TUI, but there was no way to get it from the CLI.

I have added an inventory command with a configured-bootstrap-sleds subcommand, which presents exactly the same information accessible in the TUI's rack init screen.

(Note: there is no way to start rack init for the CLI yet, but I will add that in a future PR.)

In theory inventory could be expanded to provide other information.

Note, I am absolutely not attached to the command interface or data format. I only care about making this information accessible so I can use it in my scripts. Meaning, if any of this interface should be different from how I've done it in this initial PR, I will make those changes as asked.

Example Usage

artemis@jeeves ~ $ ssh londonwicket inventory configured-bootstrap-sleds
⚠ Cubby 14      BRM42220036     not_available
✔ Cubby 15      BRM42220062     fdb0:a840:2504:312::1
✔ Cubby 16      BRM42220030     fdb0:a840:2504:257::1
✔ Cubby 17      BRM44220007     fdb0:a840:2504:492::1

artemis@jeeves ~ $ ssh londonwicket -- inventory configured-bootstrap-sleds --json | jq
[
  {
    "id": {
      "slot": 14,
      "type": "sled"
    },
    "baseboard": {
      "type": "gimlet",
      "identifier": "BRM42220036",
      "model": "913-0000019",
      "revision": 6
    },
    "bootstrap_ip": "fdb0:a840:2504:212::1"
  },
  {
    "id": {
      "slot": 15,
      "type": "sled"
    },
    "baseboard": {
      "type": "gimlet",
      "identifier": "BRM42220062",
      "model": "913-0000019",
      "revision": 6
    },
    "bootstrap_ip": "fdb0:a840:2504:312::1"
  },
  {
    "id": {
      "slot": 16,
      "type": "sled"
    },
    "baseboard": {
      "type": "gimlet",
      "identifier": "BRM42220030",
      "model": "913-0000019",
      "revision": 6
    },
    "bootstrap_ip": "fdb0:a840:2504:257::1"
  },
  {
    "id": {
      "slot": 17,
      "type": "sled"
    },
    "baseboard": {
      "type": "gimlet",
      "identifier": "BRM44220007",
      "model": "913-0000019",
      "revision": 6
    },
    "bootstrap_ip": "fdb0:a840:2504:492::1"
  }
]

sunshowers

Looks fine to me! This isn't a stable interface I presume so we can iterate on it.

How hard would it be to add an integration test for this? There are a few in wicketd that spin up mgs, sp-sim, wicketd, and wicket, and ensure that it's sound end-to-end.

sunshowers · 2024-08-03T22:23:50Z

wicket/src/cli/inventory.rs

+
+                if json {
+                    let json_str = serde_json::to_string(&sled_data)
+                        .context("serializing sled data")?;


Maybe "error serializing sled data"?

sunshowers · 2024-08-03T22:24:33Z

wicket/src/cli/inventory.rs

+
+#[derive(Serialize)]
+struct ConfiguredBootstrapSledData {
+    cubby: u32,


Can we call this slot?

sunshowers · 2024-08-03T22:26:44Z

wicket/src/cli/inventory.rs

+    };
+
+    let addr_fmt = match address {
+        None => "not_available".to_string(),


Can this be (not available)?

I'll admit I made it not_available to make it one field in awk even if you are using all whitespace as your field separator. but i think that's probably a bad reason given we have the json output and the data is tab-separated so yeah I'll change that

faithanalog · 2024-08-05T18:37:04Z

Looks fine to me! This isn't a stable interface I presume so we can iterate on it.

How hard would it be to add an integration test for this? There are a few in wicketd that spin up mgs, sp-sim, wicketd, and wicket, and ensure that it's sound end-to-end.

yeah I will take a look, I was wanting to write a test for it but didn't know we had a framework in place for doing that.

iximeow · 2024-08-06T23:09:39Z

wicket/src/cli/inventory.rs

+    };
+
+    // The rest of the data
+    println!("{status} Cubby {:02}\t{identifier}{addr_fmt}", slot);


@sunshowers i saw in the other comment above you asked for the field to be changed from slot to cubby, so:

is there a distinction between what "slot" vs "cubby" describes? i've seen "cubby" elsewhere and i'm kinda wondering if it's an (implicit?) word choice change or something else.

should this be {status} Slot {:02}? 😁

yeah i am also not sure about that. This says Cubby because the wicket TUI also says Cubby today and I am just matching what that does.

is there a distinction between what "slot" vs "cubby" describes? i've seen "cubby" elsewhere and i'm kinda wondering if it's an (implicit?) word choice change or something else.

I think we've used the two terms interchangeably, not aware of specific differences between the two -- I know that the data structures internally mostly use "slot" which is why I was hoping at least the JSON could use "slot".

faithanalog · 2024-08-07T01:28:51Z

I changed the json interface to use the type I was extracting data out of. It's a bit more verbose but it's got all the data I care about, and some extras someone else might find handy. And more importantly, it's a type used in other places of the codebase, so it'll be more clear that this command (and its test) might need adjustment if it changes.

sunshowers

Thank you!

sunshowers · 2024-08-07T03:23:41Z

wicketd/tests/integration_tests/inventory.rs

+        // This is the data we have today but I want it to be different from
+        // this to test having an address and such


Is this comment left over from an earlier iteration?

oh yeah sure is

sunshowers · 2024-08-07T22:59:45Z

The test failure is a flaky one that should be resolved if you rebase on main FYI

hawkw

I left some very minor little nits, but overall, this looks good to me!

hawkw · 2024-08-08T16:55:49Z

wicket/src/cli/inventory.rs

+        /// Print output as json
+        #[clap(long)]
+        json: bool,


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:

if adding an additional output format, it makes it a bit clearer in the CLI that the output formats are mutually exclusive,

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.

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

hawkw · 2024-08-08T16:57:16Z

wicket/src/cli/inventory.rs

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


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?

Yeah, love the idea of using to_string_pretty.

hawkw · 2024-08-08T16:59:13Z

wicket/src/cli/inventory.rs

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


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...

Ah I think we use owo-colors here.

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

hawkw · 2024-08-08T17:00:00Z

wicket/src/cli/inventory.rs

+    };
+
+    let addr_fmt = match address {
+        None => "(not available)".to_string(),


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?

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

faithanalog · 2024-08-09T00:58:19Z

oh dear i've been nitted! I will add --format json|table and make the json pretty so you dont need to jq it

faithanalog · 2024-08-09T21:44:10Z

this worked fine on london so i am going to do the merge

faithanalog requested a review from sunshowers August 3, 2024 02:51

sunshowers reviewed Aug 3, 2024

View reviewed changes

iximeow reviewed Aug 6, 2024

View reviewed changes

sunshowers approved these changes Aug 7, 2024

View reviewed changes

hawkw approved these changes Aug 8, 2024

View reviewed changes

faithanalog added 9 commits August 8, 2024 17:44

add wicket inventory command

ca0b62a

add field for no ipv6, switch emoji to ascii

1108222

0pad the cubby

f806bb8

i guess this is the standard though

6806620

oops had these swapped

684ec65

stylistic changes

a147d8c

a basic test

fbba682

fix up the test, add comments about it

a0b89d8

remove leftover comment

83ee04f

faithanalog force-pushed the artemis/wicket-inventory-bootstrap-sleds branch from c259882 to 83ee04f Compare August 9, 2024 00:44

faithanalog added 2 commits August 8, 2024 18:25

polish it up

0302895

ugh i did defaults wrong

fce6fa8

faithanalog merged commit d391e5c into main Aug 9, 2024
22 checks passed

faithanalog deleted the artemis/wicket-inventory-bootstrap-sleds branch August 9, 2024 21:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

faithanalog commented Aug 3, 2024 •

edited

Loading

sunshowers left a comment

sunshowers Aug 3, 2024

sunshowers Aug 3, 2024

sunshowers Aug 3, 2024

faithanalog Aug 5, 2024

faithanalog commented Aug 5, 2024

iximeow Aug 6, 2024

faithanalog Aug 6, 2024

sunshowers Aug 7, 2024

faithanalog commented Aug 7, 2024

sunshowers left a comment

sunshowers Aug 7, 2024

faithanalog Aug 7, 2024

sunshowers commented Aug 7, 2024

hawkw left a comment

hawkw Aug 8, 2024

sunshowers Aug 8, 2024

hawkw Aug 8, 2024

sunshowers Aug 8, 2024

hawkw Aug 8, 2024

sunshowers Aug 8, 2024

faithanalog Aug 9, 2024

hawkw Aug 8, 2024

faithanalog Aug 9, 2024 •

edited

Loading

faithanalog commented Aug 9, 2024

faithanalog commented Aug 9, 2024

		// This is the data we have today but I want it to be different from
		// this to test having an address and such

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

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

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

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

Conversation

faithanalog commented Aug 3, 2024 • edited Loading

This PR

Example Usage

sunshowers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faithanalog commented Aug 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faithanalog commented Aug 7, 2024

sunshowers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunshowers commented Aug 7, 2024

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faithanalog Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

faithanalog commented Aug 9, 2024

faithanalog commented Aug 9, 2024

faithanalog commented Aug 3, 2024 •

edited

Loading

faithanalog Aug 9, 2024 •

edited

Loading