Skip to content

nexus: split out control plane zones as separate artifacts#7452

Merged
iliana merged 9 commits intomainfrom
iliana/control-plane-artifact-split
Feb 1, 2025
Merged

nexus: split out control plane zones as separate artifacts#7452
iliana merged 9 commits intomainfrom
iliana/control-plane-artifact-split

Conversation

@iliana
Copy link
Copy Markdown
Contributor

@iliana iliana commented Jan 30, 2025

The primary change here is adding a control knob to ArtifactsWithPlan::from_stream that either splits out the artifacts or combines all the zones into a single tarball.

ControlPlaneZonesMode::Composite is the current behavior and is used in Wicket and existing unit tests. For the moment this doesn't support handling repositories with artifacts already split out, but the semantics of this setting are that if/when we do that, it will combine them into a single artifact for Wicket.

ControlPlaneZonesMode::Split enables the new behavior and is used in Nexus. This makes the individual zone images available in the TUF Repo Depot and therefore immediately usable by Sled Agent when creating zones.

Additionally, we add a check to assembling a TUF repo to ensure that no individual zone images have the same hash; since the fake zones created during tests had the same hashes, we learned such a repo would not be accepted by Nexus due to this check:

let by_hash_slot = match self.by_hash.entry(artifact_hash_id) {
Entry::Occupied(slot) => {
return Err(RepositoryError::DuplicateHashEntry(
slot.key().clone(),
));
}
Entry::Vacant(slot) => slot,
};

Closes #4411. (If we want to split out the control plane zones in actual repos we can open a new issue for that.)

@iliana
Copy link
Copy Markdown
Contributor Author

iliana commented Jan 30, 2025

Note to self: this doesn't set the artifact name correctly (it uses zone1.tar.gz instead of zone1). Will fix...

Copy link
Copy Markdown
Contributor

@jgallagher jgallagher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a couple nits to take or leave.

#[derive(thiserror::Error, Debug)]
pub enum Error {
#[error("I/O Error: {message}: {err}")]
#[error("I/O Error: while accessing {path}: {err}")]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit - we shouldn't be including the #[source] error in the string (https://github.com/oxidecomputer/slog-error-chain?tab=readme-ov-file#aside-embedding-source-error-strings). I realize this was here before, but since we're patching up the message, can we drop the : {err} here and in ZoneVersion?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe not right now -- the one place we use this error we're just running err.to_string() on it, and apparently thiserror's Display implementation doesn't support including the error chain with the alternate format like anyhow does.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we replace that with InlineErrorChain::new(&err).to_string()?

@@ -70,8 +70,10 @@ async fn test_updates() {
.into_inner();

// We should have an artifact for every known artifact kind...
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add (except Zone) like the other comments? I'm not sure how valuable that really is, but it does seem like the comment as written is now incorrect.

cp_builder
.append_zone(
name,
CompositeEntry { data, mtime_source: MtimeSource::Now },
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The unit test failure seems legit - does data need to be a tarball containing an oxide.json?

@iliana iliana merged commit 1a0bb74 into main Feb 1, 2025
@iliana iliana deleted the iliana/control-plane-artifact-split branch February 1, 2025 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TUF repo control plane artifact should be split into individual zone artifacts

2 participants