[RFD 621 3/n] switch to Tufaceous v2 in Nexus and Sled Agent - #10774
Conversation
these are in their own commit for reviewability
Current status (will be edited)This (along with #10755) has been tested on a racklette and performs as expected with both v1 and v2 repos. |
7c5fbb6 to
5a34123
Compare
karencfv
left a comment
karencfv
left a comment
There was a problem hiding this comment.
Sorry again for taking so long to have a look! I haven't finished reviewing, but I have some initial questions. A lot of this is just me wanting to understand what's going on. I'll continue with the review tomorrow.
karencfv
left a comment
karencfv
left a comment
There was a problem hiding this comment.
Alright, time to log off now, I've added a few more comments. Will continue with the review tomorrow!
karencfv
left a comment
karencfv
left a comment
There was a problem hiding this comment.
Still not finished, but leaving a trail of comments 😄
|
|
||
| // There's no difference between Gimlet and Cosmo from an RoT perspective | ||
| // so both of them still use the same `ArtifactKind` everywhere. This is | ||
| // why we need the extra `sled_type` argument here | ||
| fn test_artifact_for_artifact_kind( | ||
| kind: ArtifactKind, | ||
| sled_type: Option<OxideSled>, | ||
| ) -> ArtifactHash { | ||
| if kind == ArtifactKind::GIMLET_ROT_IMAGE_A | ||
| && sled_type == Some(OxideSled::Gimlet) | ||
| { | ||
| ARTIFACT_HASH_ROT_GIMLET_A | ||
| } else if kind == ArtifactKind::GIMLET_ROT_IMAGE_B | ||
| && sled_type == Some(OxideSled::Gimlet) | ||
| { | ||
| ARTIFACT_HASH_ROT_GIMLET_B | ||
| } else if kind == ArtifactKind::GIMLET_ROT_IMAGE_A | ||
| && sled_type == Some(OxideSled::Cosmo) | ||
| { | ||
| ARTIFACT_HASH_ROT_COSMO_A | ||
| } else if kind == ArtifactKind::GIMLET_ROT_IMAGE_B | ||
| && sled_type == Some(OxideSled::Cosmo) | ||
| { | ||
| ARTIFACT_HASH_ROT_COSMO_B | ||
| } else if kind == ArtifactKind::PSC_ROT_IMAGE_A { | ||
| ARTIFACT_HASH_ROT_PSC_A | ||
| } else if kind == ArtifactKind::PSC_ROT_IMAGE_B { | ||
| ARTIFACT_HASH_ROT_PSC_B | ||
| } else if kind == ArtifactKind::SWITCH_ROT_IMAGE_A { | ||
| ARTIFACT_HASH_ROT_SWITCH_A | ||
| } else if kind == ArtifactKind::SWITCH_ROT_IMAGE_B { | ||
| ARTIFACT_HASH_ROT_SWITCH_B | ||
| } else if kind == ArtifactKind::GIMLET_ROT_STAGE0 | ||
| && sled_type == Some(OxideSled::Gimlet) | ||
| { | ||
| ARTIFACT_HASH_ROT_BOOTLOADER_GIMLET | ||
| } else if kind == ArtifactKind::GIMLET_ROT_STAGE0 | ||
| && sled_type == Some(OxideSled::Cosmo) | ||
| { | ||
| ARTIFACT_HASH_ROT_BOOTLOADER_COSMO | ||
| } else if kind == ArtifactKind::PSC_ROT_STAGE0 { | ||
| ARTIFACT_HASH_ROT_BOOTLOADER_PSC | ||
| } else if kind == ArtifactKind::SWITCH_ROT_STAGE0 { | ||
| ARTIFACT_HASH_ROT_BOOTLOADER_SWITCH | ||
| } else { | ||
| panic!("test bug: no artifact for artifact kind {kind:?}") | ||
| } | ||
| } |
karencfv
left a comment
karencfv
left a comment
There was a problem hiding this comment.
Last batch of review comments for now. Will have to answer the other comments separately because gh is annoying. Sorry for the barrage of notifications to come.
| SELECT DISTINCT ON (sha256) sha256, version, artifact_size | ||
| FROM omicron.public.tuf_artifact | ||
| ORDER BY sha256 ASC, version ASC, artifact_size ASC |
There was a problem hiding this comment.
This statement keeps exactly one (version, artifact_size) per sha256 and silently drops the rest. Just want to double check this is the behaviour we want. Is there a possibility that a rack could have more than one row with the same hash but different versions? If this were the case, the migration would relabel the artifact to the lower version. What are the consequences of this behaviour?
Do we want to fail loudly if any sha256 is associated with more than one (version, artifact_size)?
There was a problem hiding this comment.
This is intended, and we believe that we've never shipped artifacts that have the same sha256 but different versions/sizes. This is one of the intents of having the giant CSV file of every artifact that's been uploaded to the dogfood rack as part of a schema upgrade test (the schema upgrade itself does not loudly fail, but we do check that the number of rows is consistent).
I think it's okay to silently drop rows if our expectations are wrong. The data here is relatively low-risk: version is used for filling in the versions of artifacts when fetching blueprints from the database, and version and artifact_size are used for validating that a re-uploaded repository matches what we've already recorded in the database. These aren't use cases that I would hold up the world and bring in support, then engineering, for if we hit this in the field during an upgrade.
There was a problem hiding this comment.
It still makes me uneasy that during a migration we'd keep the lowest version (lowest from TEXT field which would mean inconsistency) if such a collision existed. At the very least we'd want to keep the most recent version, no?
Perhaps @jgallagher or @davepacheco have opinions on this.
| .await? | ||
| { | ||
| if let Some(mut artifact) = | ||
| results.get_mut(&tag.tuf_artifact_id) |
There was a problem hiding this comment.
This get_mut must always return Some(_) based on the query above, right? Should we either assert that's true, or bail out if it's not, to catch any future change that breaks this?
| sled-update-host-phase2 serial1 --boot-disk B --slot-b d944ae205b61ccf4322448f7d0311a819c53d9844769de066c5307c1682abb47 | ||
| sled-update-host-phase1 serial1 --active B --slot-b b99d5273ba1418bebb19d74b701d716896409566d41de76ada71bded4c9b166b | ||
| sled-update-host-phase2 serial1 --boot-disk B --slot-b c2a8af75d6907ef138d033dc3be1fcbf7d7a78a87aee04796caf283a6bb0952d | ||
| sled-update-host-phase1 serial1 --active B --slot-b 2902cb5932fa703ed62fc6a0a330898fe6fd4bde68c74f6398a6aa916cb7e79c |
There was a problem hiding this comment.
Where did these new hashes come from? The old hashes were in the output of set-target-release, but I don't see the new ones in the output file except in these commands.
There was a problem hiding this comment.
These are the hashes of the fake artifacts as generated by Tufaceous v2.
jgallagher
left a comment
jgallagher
left a comment
There was a problem hiding this comment.
LGTM, just one note about a place we may need more tests.
3 participants
This replaces Tufaceous v1 with Tufaceous v2 in Nexus, Sled Agent, and a couple of other crates. Closes #10734.
This PR is independent of (but somewhat useless without) #10676.