[RFD 621 3/n] switch to Tufaceous v2 in Nexus and Sled Agent#10774
[RFD 621 3/n] switch to Tufaceous v2 in Nexus and Sled Agent#10774iliana wants to merge 10 commits into
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.
| Ok(Some(rv)) | ||
| oxide_tokio_rt::run(async { | ||
| let output_path = args.output.unwrap_or_else(|| { | ||
| // This is relative to the current directory. |
There was a problem hiding this comment.
It'd be nice to have this comment as part of the flag's documentation above
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!
There was a problem hiding this comment.
Do we know if the new repo-depot-standalone still works with other dev tools that use it like reconfigurator-sp-updater?
| /// as part of the process, which `From` doesn't necessarily communicate | ||
| /// and can be surprising. | ||
| /// [`nexus_types::tuf_repo::TufRepoDescription`]. | ||
| pub fn from_external( |
There was a problem hiding this comment.
should this still be called from_external if we're no longer converting from external::TufRepoDescription?
There was a problem hiding this comment.
I don't really know what to name it. When I had that in omicron_common in an earlier draft it was renamed to from_common but there's not really an obvious choice here...
|
|
||
| impl TufArtifactDescription { | ||
| pub fn from_artifact( | ||
| artifact: Artifact, |
There was a problem hiding this comment.
It can be a little confusing to differentiate between TufArtifact and Artifact. Can you add some documentation above the method?
| #[diesel(table_name = tuf_artifact_tag)] | ||
| pub struct TufArtifactTag { | ||
| pub tuf_artifact_id: DbTypedUuid<TufArtifactKind>, | ||
| pub key: String, |
There was a problem hiding this comment.
These tag keys are not freeform right? We already know what they are. It'd be nice to have the keys be an enum for correctness.
There was a problem hiding this comment.
They are by definition freeform, an old Nexus cannot necessarily know about future tags. (See RFD 621 section 2.1.1)
| // We don't bother paginating in this method because each repo has a | ||
| // limited number of artifacts (currently <100 as of February 2026) and tags | ||
| // (currently <250). |
There was a problem hiding this comment.
It seems like if the number of tags blew up for some reason, we could forget to modify this method to paginate. What's the downside of paginating now?
There was a problem hiding this comment.
The downside is making the APIs much more difficult to use even though all of the callers need a complete listing of artifacts + tags in a repository regardless. The key point here is not that the number of entries is "currently" small, more that it is a consistent size that users are not likely to intentionally increase to cause a problem.
| // We don't bother paginating in this method because each repo has a | ||
| // limited number of artifacts (currently <100 as of February 2026). |
There was a problem hiding this comment.
Same as above. Is there a downside to pagination?
| // We don't enforce that two artifacts with the same SHA-256 checksum | ||
| // have the same tags; they are entered as separate artifacts. The |
There was a problem hiding this comment.
For my own understanding: Why not?
There was a problem hiding this comment.
We don't have this situation today but we want to allow for it in the future. Consider a future where we need two different kinds of host phase 2 images depending on what board we're booting. The cosmo and gimlet images could still be the same, so they should still only be in the repository once, but we can have multiple sets of tags that refer to the same content so that the application logic of "which image do I pick" is not inconsistent. Additionally we would need to support the old tag definitions, so we would have the same cosmo/gimlet image be described as {"kind":"os_phase_2","os_variant":"host"} as well as a new set of tags (e.g. {"kind":"os_phase_2_v2","os_variant":"host","os_board":"gimlet"}).
Also, it is not really possible to write a database constraint that limits this.
| let mut filter_dsl = tuf_artifact_dsl::tuf_artifact.into_boxed(); | ||
| for artifact in &desc.artifacts { | ||
| filter_dsl = filter_dsl.or_filter( | ||
| tuf_artifact_dsl::sha256.eq(artifact.artifact.sha256), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Take this with a grain of salt. I seem to remember ANY being more efficient than OR. Additionaly, the code is simpler to read.
let shas: Vec<_> = desc.artifacts.iter().map(|a| a.artifact.sha256).collect();
let filter_dsl = tuf_artifact_dsl::tuf_artifact
.filter(tuf_artifact_dsl::sha256.eq_any(shas));| let mut filter_dsl = | ||
| tuf_artifact_tag_dsl::tuf_artifact_tag.into_boxed(); | ||
| for artifact in &results { | ||
| filter_dsl = filter_dsl.or_filter( |
There was a problem hiding this comment.
Smae comment about using eq_any() over or_filter()
| @@ -567,7 +659,7 @@ impl DataStore { | |||
| async fn insert_impl( | |||
There was a problem hiding this comment.
This method is pretty gnarly, is there some way to add a test for this?
There was a problem hiding this comment.
insert_impl is only split out here because the 80-character line limit is stifling. This is covered by tests that use tuf_repo_insert.
2 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.