Tufaceous v2: project rewrite / RFD 621 implementation#45
Conversation
the primary purpose of this change is `ArtifactsExt::fake`, which is guaranteed to return the same set of `Artifacts` that you would get from generating a repository, but without requiring async. this also replaced the struct variants of `KnownArtifactTags` with named structs (RFD 643).
|
I think I've now covered all review feedback. |
sunshowers
left a comment
There was a problem hiding this comment.
other than the spawn_blocking panic safety things we discussed...
| // Collect all the sha256 hashes and lengths for each source. For file | ||
| // and fake sources, we want to calculate the hashes in parallel, so | ||
| // we spawn their calculation tasks on a JoinSet. Sources from borrowed | ||
| // repositories can't be moved into a task, but we already know their | ||
| // hash. | ||
| let mut all_targets = Vec::new(); | ||
| let mut tasks = JoinSet::new(); | ||
| for (target_name, sources) in self.targets { |
There was a problem hiding this comment.
do we want to bound the number of tasks running in parallel here? probably doesn't matter much.
There was a problem hiding this comment.
I pushed an implementation which adds a semaphore -- it doesn't bound the number of tasks but it does limit the number of actively-running tasks. Seems like a reasonable thing to do.
|
|
||
| fn from_str(s: &str) -> std::result::Result<Self, Self::Err> { | ||
| let mut out = [0u8; 32]; | ||
| hex::decode_to_slice(s, &mut out)?; |
There was a problem hiding this comment.
This looks like infinite recursion?
| let mut stream = transport | ||
| .fetch(root_url.clone()) | ||
| .await | ||
| .map_err(ErrorKind::Fetch)?; |
There was a problem hiding this comment.
does this enforce a size limit? should it? probably not a huge deal.
There was a problem hiding this comment.
No but it absolutely should, good catch.
| let mut file = File::open(&phase_2_path).await.ok()?; | ||
| // Read the header block from the image and guess whether it's a | ||
| // recovery image based on the image name. | ||
| let mut buf = [0; 4096]; | ||
| file.read_exact(&mut buf).await.ok()?; |
There was a problem hiding this comment.
is the ok()? on both of these calls required?
There was a problem hiding this comment.
Yes but it could probably stand to be more specific.
| let Ok(archive) = input.source.read_hubris_archive().await else { | ||
| return Ok(ControlFlow::Continue(input)); | ||
| }; | ||
| let Ok(caboose) = archive.read_caboose() else { | ||
| return Ok(ControlFlow::Continue(input)); | ||
| }; |
There was a problem hiding this comment.
should these handle errors more specifically?
There was a problem hiding this comment.
In the first one, sure, we could return errors related to reading the file, but we deliberately want to not error on if it happens to be a ZIP file that isn't a valid Hubris archive.
| pub fn remove_target(mut self, target_name: impl AsRef<str>) -> Self { | ||
| let target_name = target_name.as_ref(); | ||
| self.targets.remove(target_name); | ||
| self.artifacts.remove(target_name); | ||
| self | ||
| } |
There was a problem hiding this comment.
should this error out if the target wasn't found?
There was a problem hiding this comment.
If this one does, should remove_artifacts error if the target isn't found either?
I kind of would like to punt on this question because I'm genuinely not sure.
There was a problem hiding this comment.
Yeah, I'm worried about a typo or something leading to a confusing experience.
| return Some(Err(ErrorKind::ReadFile { | ||
| source, | ||
| path: Some(path.to_owned()), | ||
| } |
There was a problem hiding this comment.
do you mean to use phase_2_path here?
| let phase_2_path = path.join(PHASE_2_PATH); | ||
| let mut file = File::open(&phase_2_path).await.ok()?; |
There was a problem hiding this comment.
Sorry, I apologize for not fully understanding this, but why is ok()? fine here? Shouldn't we handle not-found vs other errors differently?
There was a problem hiding this comment.
I apologize for not fully understanding this
At this point I think you understand it better than I do! :)
sunshowers
left a comment
There was a problem hiding this comment.
Thank you for all your hard work on this!
|
Thank you all for the extremely helpful reviews! This is going to be great to integrate. |
I don't think this is 100% to where I want it to be yet but I have to cut a PR at some point.
This is essentially a complete redo of Tufaceous. I have been developing it in parallel with an Omicron changeset that integrates it (which you can see the WIP tree here: https://github.com/oxidecomputer/omicron/tree/iliana/rfd621).
Here are the main ideas:
All-in-one API
Everything a consumer needs for creating and reading repositories is in Tufaceous; there's no need to directly use APIs from tough. (The one exception to this currently is using non-ed25519 signing keys; when I get more of the signing tooling done I will probably add this.)
Artifact tags
RFD 621 goes into detail here, but the name-version-kind triple really doesn't do a good job of differentiating artifacts in practice. This system was developed in order to future-proof Nexus against future kinds of artifacts, but when we started this system we didn't fully understand what types of artifacts we need in the repo.
All artifacts that can go in the repo are described with
KnownArtifactTags, an enum type that serializes to a mapping of string keys to string values. It is not possible to use the library to create a repository with "unknown" tags. When an older Nexus, using an older Tufaceous v2 library, interacts with a repo from the future with unknown tags, it will still store them in the database but ignore them for planning purposes.ArtifactSetThis is basically a
BTreeSet<Artifact>which has an index based on an artifact's known tags. This structure replaces most of the code inupdate-commoncurrently in Omicron (apart from reading ZIP archives, below).Builder API for creating / editing archives
In v1, repos were generated from manifests, but you couldn't automatically generate a manifest from a repo. This meant that patching a repo to add/change artifacts was extremely difficult, and it was usually easier to just use our releng tooling.
Instead we use a builder API to create and edit archives using the
RepositoryEditor. You can start with an empty repository, or a fake repository with one of every artifact in it, or an already-loaded repository in order to modify it.A significant amount of the code (and related abstractions) in the
editmodule is used for guessing artifact tags. This was done to make the CLI tooling bearable. I've added a note to the new README that indicates that future artifacts should be "guessable" by this tooling.ZIP archive implementation
Tufaceous now has a ZIP transport implementation for tough, so Omicron no longer needs to unpack ZIP archives to disk in order to load the repository. There is a lengthy comment at the start of
lib/src/zip_transport.rson some of the pitfalls of using ZIP archives and the mitigations we've taken.v1 compatibility
The v2 repository format is not compatible with the v1 format.
RepositoryLoadercan read v1 repos, unpacking composite artifacts along the way. This library is not intended for creating v1 repos, and we expect therelengtool will carry a dependency on both Tufaceous v1 and v2 until we stop producing v1 repos. After that happens, we can start to remove v1 compatibility code from v2.The
tufaceous verifysubcommand has v1 compatibility enabled and can be used to check for problems in the conversion (in fact, writing the problem checker and using it on our v1 repos made me find problems in the compatibility code!).Currently this code cannot import a v1 repository into a [
RepositoryEditor], and thus cannot directly convert a v1 archive into a v2 archive. I would like to add this functionality at some point (there is some additional complexity around handling the composite artifacts).