-
Notifications
You must be signed in to change notification settings - Fork 45
[22/n] [reconfigurator-cli] specify sled by serial number #8489
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
[22/n] [reconfigurator-cli] specify sled by serial number #8489
Conversation
Created using spr 1.3.6-beta.1
blueprint-edit latest set-remove-mupdate-override b82ede02-399c-48c6-a1de-411df4fa49a7 00000000-0000-0000-0000-000000000000 | ||
blueprint-edit latest set-remove-mupdate-override d81c6a84-79b8-4958-ae41-ea46c9b19763 00000000-0000-0000-0000-000000000000 | ||
blueprint-edit latest set-remove-mupdate-override e96e226f-4ed9-4c01-91b9-69a9cd076c9e 00000000-0000-0000-0000-000000000000 | ||
blueprint-edit latest set-remove-mupdate-override serial2 00000000-0000-0000-0000-000000000000 |
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 like this change overall quite a lot; there are other places (e.g., omdb) I've wanted to refer to sleds by serial instead of ID too. This might be too bikesheddy, but I don't love the syntax where UUIDs get parsed as-is and serials have a special prefix. What would you think about either of these?
- Implicitly parse anything that doesn't look like a UUID as a serial number (so this value would just be
2
) - Make the kind explicit for both; either something like
id:SOME-UUID
/serial:SERIAL
or as more of a CLI-like argument like--id SOME-UUID
/--serial SERIAL
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 I agree, in that I like that you can specify a serial number, but I also want to be able to do this with data from real systems whose serials don't start with serial
. I don't think actual serials can overlap with uuids, so I like the idea of: if it parses as a uuid, make it a uuid. Otherwise, it's a serial number. @jgallagher was that what you meant as your first bullet?
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.
Yep! Although I misunderstood the serial
prefix here. I saw the parsing check for it as a prefix and assumed it stripped that prefix (e.g., the serial number of this simulated sled is actually 2
), but going back you're right that this only works if serials are literally serialSomething
. So my first bullet is off a little because of that misunderstanding.
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.
Good point re real sleds — thoughts on using the BRM prefix? Mostly I was concerned about a slightly typo'd UUID causing it to be treated as a serial, resulting in confusing error messages.
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.
Using BRM as a prefix for simulated sleds seems a little sketchy to me. A slightly typo'd UUID seems like it would give a pretty clear error message, right? "No sled with serial $ALMOST_A_UUID"?
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 I mean "serial" for simulated sleds and "BRM" for real sleds loaded in via JSON.
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.
Using BRM as a prefix for simulated sleds seems a little sketchy to me. A slightly typo'd UUID seems like it would give a pretty clear error message, right? "No sled with serial $ALMOST_A_UUID"?
I do think that's a bit more confusing than the error messages produced by explicitly checking for the serial
and BRM
prefixes, but maybe it's okay anyway.
edit: what I mean is changing this to:
- if it parses as a UUID, use that
- otherwise, if it has a
serial
prefix, treat it as a serial number - otherwise, if it has a
BRM
prefix, treat it as a serial number - otherwise, error out
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.
Ug, sorry for just remembering this, but there are least three other sources of serial numbers:
- a4x2 (IIRC, these are
g0
,g1
, ...?) - single-sled dev deployments (I have no idea)
- the Canada region (I have no idea, although @jmpesp could probably tell us)
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.
ahh, hmm. In that case I guess treating any non-UUID strings as serials makes the most sense.
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.
done -- the error messages are something like
〉sled-show a3a47d70-a5f4-42e2-b8fa-32a92795c88
error: sled not found with serial a3a47d70-a5f4-42e2-b8fa-32a92795c88 (known serials: serial0, serial1, serial2)
which seems fine.
blueprint 626487fa-7139-45ec-8416-902271fc730b created from latest blueprint (a5a8f242-ffa5-473c-8efd-2acf2dc0b736): set remove_mupdate_override to ffffffff-ffff-ffff-ffff-ffffffffffff | ||
|
||
> blueprint-edit latest debug force-sled-generation-bump d81c6a84-79b8-4958-ae41-ea46c9b19763 |
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.
Was this not sled 5 before?
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.
Before, this used lexicographic order by UUID, and now this uses serial number order -- so the output is perturbed a bit, but the test coverage is the same.
> sled-list
ID SERIAL NZPOOLS SUBNET
2b8f0cb3-0295-4b3c-bc58-4fe88b57112c serial1 0 fd00:1122:3344:102::/64
98e6b7c2-2efa-41ca-b20a-0a4d61102fe6 serial0 0 fd00:1122:3344:101::/64
9a867dc9-d505-427f-9eff-cdb1d4d9bd73 serial5 0 fd00:1122:3344:106::/64
aff6c093-197d-42c5-ad80-9f10ba051a34 serial3 0 fd00:1122:3344:104::/64
b82ede02-399c-48c6-a1de-411df4fa49a7 serial4 0 fd00:1122:3344:105::/64
d81c6a84-79b8-4958-ae41-ea46c9b19763 serial2 0 fd00:1122:3344:103::/64
e96e226f-4ed9-4c01-91b9-69a9cd076c9e serial6 0 fd00:1122:3344:107::/64
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.
Ahh, got it, that makes sense. 👍
Created using spr 1.3.6-beta.1
These serial numbers are somewhat easier to parse in many cases, and also synergize with the next commit in this stack (which allows setting a specific policy on each sled while loading the initial example).
Switch one of the tests over as an example -- I hope its intent becomes clearer.