Skip to content

[10/n] [sled-agent] validate zone images as written in zone manifest #8190

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

35 changes: 32 additions & 3 deletions common/src/update/zone_manifest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// License, v. 2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at https://mozilla.org/MPL/2.0/.

use std::fmt;

use iddqd::{IdOrdItem, IdOrdMap, id_upcast};
use omicron_uuid_kinds::MupdateUuid;
use serde::{Deserialize, Serialize};
Expand All @@ -10,9 +12,8 @@ use tufaceous_artifact::ArtifactHash;
/// Describes the set of Omicron zones written out into an install dataset.
#[derive(Clone, Debug, Eq, PartialEq, Deserialize, Serialize)]
pub struct OmicronZoneManifest {
/// The UUID of the mupdate which created this manifest. Intended primarily
/// for checking equality.
pub mupdate_id: MupdateUuid,
/// The source of the manifest.
pub source: OmicronZoneManifestSource,

/// Omicron zone file names and hashes.
pub zones: IdOrdMap<OmicronZoneFileMetadata>,
Expand All @@ -23,6 +24,34 @@ impl OmicronZoneManifest {
pub const FILE_NAME: &str = "zones.json";
}

/// The source of truth for an Omicron zone manifest.
#[derive(Clone, Debug, Eq, PartialEq, Deserialize, Serialize)]
pub enum OmicronZoneManifestSource {
/// The manifest was written out by installinator and the mupdate process.
Installinator {
/// The UUID of the mupdate.
mupdate_id: MupdateUuid,
},

/// The zone manifest was not found during the install process. A synthetic
/// zone manifest was generated by Sled Agent by looking at all the
/// `.tar.gz` files in the install dataset.
SledAgent,
}

impl fmt::Display for OmicronZoneManifestSource {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
OmicronZoneManifestSource::Installinator { mupdate_id } => {
write!(f, "installinator (mupdate ID: {})", mupdate_id)
}
OmicronZoneManifestSource::SledAgent => {
write!(f, "sled-agent")
}
}
}
}

/// Information about an Omicron zone file written out to the install dataset.
///
/// Part of [`OmicronZoneManifest`].
Expand Down
12 changes: 9 additions & 3 deletions installinator/src/write.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ use omicron_common::{
disk::M2Slot,
update::{
MupdateOverrideInfo, OmicronZoneFileMetadata, OmicronZoneManifest,
OmicronZoneManifestSource,
},
};
use omicron_uuid_kinds::{MupdateOverrideUuid, MupdateUuid};
Expand Down Expand Up @@ -63,7 +64,8 @@ struct ArtifactDestination {

impl ArtifactDestination {
fn from_directory(dir: &Utf8Path) -> Result<Self> {
let control_plane_dir = dir.join("zones");
// The install dataset goes into a directory called "install".
let control_plane_dir = dir.join("install");
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this just wrong before? (Hopefully only used by tests??)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it was only used by tests. Actually hooking sled-agent's zone logic to installinator found the issue.

std::fs::create_dir_all(&control_plane_dir)
.with_context(|| format!("error creating directories at {dir}"))?;

Expand Down Expand Up @@ -827,8 +829,12 @@ impl ControlPlaneZoneWriteContext<'_> {
async fn omicron_zone_manifest_artifact(&self) -> BufList {
let zones = compute_zone_hashes(&self.zones).await;

let omicron_zone_manifest =
OmicronZoneManifest { mupdate_id: self.mupdate_id, zones };
let omicron_zone_manifest = OmicronZoneManifest {
source: OmicronZoneManifestSource::Installinator {
mupdate_id: self.mupdate_id,
},
zones,
};
let json_bytes = serde_json::to_vec(&omicron_zone_manifest)
.expect("this serialization is infallible");
BufList::from(json_bytes)
Expand Down
5 changes: 5 additions & 0 deletions sled-agent/zone-images/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,21 @@ illumos-utils.workspace = true
nexus-sled-agent-shared.workspace = true
omicron-common.workspace = true
omicron-workspace-hack.workspace = true
rayon.workspace = true
serde.workspace = true
serde_json.workspace = true
sha2.workspace = true
sled-agent-config-reconciler.workspace = true
sled-storage.workspace = true
slog.workspace = true
slog-error-chain.workspace = true
thiserror.workspace = true
tufaceous-artifact.workspace = true

[dev-dependencies]
camino-tempfile-ext.workspace = true
dropshot.workspace = true
expectorate.workspace = true
omicron-uuid-kinds.workspace = true
pretty_assertions.workspace = true
sled-agent-config-reconciler = { workspace = true, features = ["testing"] }
46 changes: 46 additions & 0 deletions sled-agent/zone-images/src/errors.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at https://mozilla.org/MPL/2.0/.

use std::{io, sync::Arc};
use thiserror::Error;

/// An `io::Error` wrapper that implements `Clone` and `PartialEq`.
#[derive(Clone, Debug, Error)]
#[error(transparent)]
pub struct ArcIoError(pub Arc<io::Error>);

impl ArcIoError {
pub(crate) fn new(error: io::Error) -> Self {
Self(Arc::new(error))
}
}

/// Testing aid.
impl PartialEq for ArcIoError {
fn eq(&self, other: &Self) -> bool {
// Simply comparing io::ErrorKind is good enough for tests.
self.0.kind() == other.0.kind()
}
}

/// A `serde_json::Error` that implements `Clone` and `PartialEq`.
#[derive(Clone, Debug, Error)]
#[error(transparent)]
pub struct ArcSerdeJsonError(pub Arc<serde_json::Error>);

impl ArcSerdeJsonError {
pub(crate) fn new(error: serde_json::Error) -> Self {
Self(Arc::new(error))
}
}

/// Testing aid.
impl PartialEq for ArcSerdeJsonError {
Comment on lines +38 to +39
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to put this behind #[cfg(test)]? Maybe doesn't matter, since I wouldn't expect real code to be comparing errors anyway...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be ok for now, but in an upcoming PR I move this into a more central location and it would become more annoying to manage.

fn eq(&self, other: &Self) -> bool {
// Simply comparing line/column/category is good enough for tests.
self.0.line() == other.0.line()
&& self.0.column() == other.0.column()
&& self.0.classify() == other.0.classify()
}
}
Loading
Loading