Conversation
davepacheco
left a comment
davepacheco
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround! I have just a few suggestions, none of them blockers.
| zone::MockZones, | ||
| }; | ||
|
|
||
| use nexus_sled_agent_shared::inventory::OmicronZoneImageSource; |
There was a problem hiding this comment.
I'm all for having this PR not try to also fix sled agent to honor this field at the same time, but what do you think about having it reject any use of OmicronZoneImageSource::Artifact in the meantime?
There was a problem hiding this comment.
I think this might make it more difficult to test the Nexus side of it if omicron_zones_put is rejecting anything using the Artifact variant, and I'm not sure I want to figure out how to word and wire up a new error variant to rip out in an upcoming PR.
| pub enum OmicronZoneImageSource { | ||
| InstallDataset, | ||
| Artifact { hash: ArtifactHash }, | ||
| } |
There was a problem hiding this comment.
This might be more of a question about the repo depot API - can sled-agent check that the hash provided here matches the zone type? E.g., I'd expect if I PUT /omicron-zones with a nonsense hash I'd get back an error that sled-agent has no such artifact, but ideally I'd also expect sled-agent to return an error if I tell it to run a Nexus zone with an artifact hash that points to an Oximeter zone image.
There was a problem hiding this comment.
Sled Agent doesn't currently know what type of artifact is what but I think it probably makes sense to either tell Sled Agent what the artifact types are (this could be added to the artifact config introduced in #7519) or have Sled Agent quickly inspect the artifact to make sure it looks like a zone (tar.gz, oxide.json is present with the correct type and perhaps even correct zone name).
|
Hmm. Does it make more sense to update the representative blueprint files (e.g. |
I could go either way on this. In the limit, we'll need something better than those files to achieve what they're doing because we want to be able to change the blueprint incompatibly without having to support older versions. But I'm not sure you want to build a replacement for whatever that right now. |
I think this isn't limited to those tests, right? sled-agent has ledgered |
Yes, at load-time, we should fill those in. It'd be nice if we could drop that in the next release. Maybe we could have sled agent rewrite this ledger after loading it? |
|
If we keep the standard release procedure of "generate a new blueprint and execute it after upgrade", we might get them filled in for free if we make any other changes that will necessitate a bumped |
sunshowers
left a comment
sunshowers
left a comment
There was a problem hiding this comment.
This is lovely, thank you!
Does this suggest we should keep at least one json fixture around that's representative of what customer systems would have as of the current release? |
That's a good idea. Maybe we could grab the ledgers from each sled after updates to the colo rack? |
That sounds good to me -- in general this kind of coverage across versions is quite difficult, so having an easy way to do this (having json fixtures) is valuable. |
4 participants
This adds the necessary bit of plumbing to be able to work on #7281 from both the Reconfigurator and Sled Agent sides simultaneously.