Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
28 changes: 27 additions & 1 deletion nexus/external-api/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ use nexus_types_versions::v2026_01_08_00;
use nexus_types_versions::v2026_01_15_00;
use nexus_types_versions::v2026_01_16_00;
use nexus_types_versions::v2026_01_16_01;
use nexus_types_versions::v2026_01_21_00;
use nexus_types_versions::v2026_01_22_00;
use nexus_types_versions::v2026_01_30_01;
use nexus_types_versions::v2026_01_31_00;
Expand Down Expand Up @@ -88,6 +89,7 @@ api_versions!([
// | date-based version should be at the top of the list.
// v
// (next_yyyy_mm_dd_nn, IDENT),
(2026_10_29_00, SLED_ADD_PRECHECK_OVERRIDES),
(2026_10_21_00, BYTE_WRAPPER_0_2_BUMP),
(2026_10_15_00, SYSTEM_CERTIFICATES),
(2026_10_13_00, ARTIFACT_HASH_SCHEMA),
Expand Down Expand Up @@ -7693,14 +7695,38 @@ pub trait NexusExternalApi {
method = POST,
path = "/v1/system/hardware/racks/{rack_id}/membership/add",
tags = ["experimental"],
versions = VERSION_TRUST_QUORUM_ADD_SLEDS_AND_GET_LATEST_CONFIG..
versions = VERSION_SLED_ADD_PRECHECK_OVERRIDES..
}]
async fn rack_membership_add_sleds(
rqctx: RequestContext<Self::Context>,
path_params: Path<latest::path_params::RackPath>,
req: TypedBody<latest::rack::RackMembershipAddSledsRequest>,
) -> Result<HttpResponseOk<latest::rack::RackMembershipStatus>, HttpError>;

/// Add new sleds to rack membership
#[endpoint {
operation_id = "rack_membership_add_sleds",
method = POST,
path = "/v1/system/hardware/racks/{rack_id}/membership/add",
tags = ["experimental"],
versions = VERSION_TRUST_QUORUM_ADD_SLEDS_AND_GET_LATEST_CONFIG..VERSION_SLED_ADD_PRECHECK_OVERRIDES,
}]
async fn rack_membership_add_sleds_v2026_01_21_00(
rqctx: RequestContext<Self::Context>,
path_params: Path<v2025_11_20_00::path_params::RackPath>,
req: TypedBody<v2026_01_21_00::rack::RackMembershipAddSledsRequest>,
) -> Result<
HttpResponseOk<v2026_01_21_00::rack::RackMembershipStatus>,
HttpError,
> {
Self::rack_membership_add_sleds(
rqctx,
path_params,
req.map(latest::rack::RackMembershipAddSledsRequest::from),
)
.await
}

/// Abort the latest rack membership change
///
/// This operation is synchronous. Upon returning from the API call, a
Expand Down
11 changes: 9 additions & 2 deletions nexus/src/app/rack.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ use nexus_types::external_api::certificate;
use nexus_types::external_api::hardware;
use nexus_types::external_api::networking;
use nexus_types::external_api::policy;
use nexus_types::external_api::rack::SledAddPrecheckOverrides;
use nexus_types::external_api::silo;
use nexus_types::inventory::SpType;
use nexus_types::silo::silo_dns_name;
Expand Down Expand Up @@ -832,8 +833,14 @@ impl super::Nexus {
.await?;

// Check the sled's software before allowing it to be added to the
// system.
self.sled_check_versions(opctx, [&baseboard_id]).await?;
// system. This endpoint was removed from the API before overrides
// were added, so its callers cannot skip any checks.
self.sled_check_versions(
opctx,
[&baseboard_id],
&SledAddPrecheckOverrides::default(),

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.

I think I picked this up from you a long time ago - should we use a more descriptive name than default here? SledAddPrecheckOverrides::none() or SledAddPrecheckOverrides::no_overrides() or something like that?

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.

holy smokes: @davepacheco using "default"!?!?!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I wasn't super excited about this. I can get behind default when it's super clear what the semantics are (like maybe for i32), or when the default can't reasonably be computed by the caller (like the default path to store user configuration, which varies from system to system), or when it's very unusual to need to change it (like in this case, hopefully). I like the idea of not impl'ing Default on the type and using something like no_overrides(), but it seemed wrong at the API level to make people write out that they're not overriding anything. The natural way to let them leave that out is to impl Default on this type. But I could as well use the serde annotation to get the default from a helper, though. I don't have a strong feeling either way here.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I went down this path, but I think using #[serde(default = ...)] instead of #[serde(default)] might have the side effect that users of generated clients have to provide these fields instead of getting a default? That seems meaningfully worse to me. What do you think @jgallagher @ahl?

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.

That... seems terrible and not at all what I'd expect. Is that because the default only applies to deserialization and doesn't make it to the OpenAPI spec?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I didn't poke too hard at it, to be honest. 7142d62 is the commit in which I undid this. In the "before", it was using #[serde(default = ...)]. Nothing about the default value for this top-level field makes it into the OpenAPI document. That's also true in the "after" version with #[serde(default)], but the difference is that the individual boolean fields themselves have default values and those do make it into the OpenAPI document. Since these are plain objects, I imagine a client can infer that they can construct the whole struct from the defaults, but I'm not sure if that happens in practice or if that's technically implied by the spec.

)
.await?;

let subnet = self.db_datastore.rack_subnet(opctx, self.rack_id).await?;
let rack_subnet =
Expand Down
111 changes: 107 additions & 4 deletions nexus/src/app/sled_add_checks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,25 +13,34 @@ use nexus_reconfigurator_planning::mgs_updates::DeployedSledVersionsError;
use nexus_types::deployment::Blueprint;
use nexus_types::deployment::BlueprintArtifactVersion;
use nexus_types::deployment::MgsUpdateComponent;
use nexus_types::external_api::rack::SledAddPrecheckOverrides;
use nexus_types::inventory::NotInInventory;
use nexus_types::inventory::SpType;
use nexus_types::tuf_repo::TufRepoDescription;
use omicron_common::api::external::Error;
use omicron_generation_kinds::TargetReleaseGeneration;
use omicron_uuid_kinds::SledUuid;
use sled_hardware_types::BaseboardId;
use slog::Logger;
use slog::info;
use slog::warn;
use slog_error_chain::InlineErrorChain;
use std::fmt::Display;

impl super::Nexus {
/// Checks that both the system and the sleds are ready for these sleds to
/// be added.
///
/// `overrides` describes checks that the operator asked to skip. We still
/// run those checks when we can, but we log any problem they find instead
/// of returning it.
pub(super) async fn sled_check_versions(
&self,
opctx: &OpContext,
sleds: impl IntoIterator<Item = &BaseboardId>,
overrides: &SledAddPrecheckOverrides,
) -> Result<(), Error> {
let log = opctx.log.new(o!("overrides" => format!("{overrides:?}")));
let target_release =
self.datastore().target_release_get_current(opctx).await?;
let release_source = target_release.release_source().map_err(|e| {
Expand All @@ -48,7 +57,7 @@ impl super::Nexus {
// adopted self-service update. In these cases, it's not
// obvious what versions of software we would check against.
info!(
opctx.log,
&log,
"skipping software checks for sled add: \
no target release has been set"
);
Expand All @@ -64,12 +73,35 @@ impl super::Nexus {
.into();
let (_, blueprint) =
self.datastore().blueprint_target_get_current_full(opctx).await?;
check_system_settled(&blueprint, target_release.generation(), &repo)
.map_err(SledAddPrecheckError::from)?;

// Each call to `apply_override` names the single field of `overrides`
// that applies to every error the check can return.
let SledAddPrecheckOverrides {
allow_without_settled_target_release,
allow_unverified_sled_software,
} = *overrides;

apply_override(
&log,
allow_without_settled_target_release,
check_system_settled(
&blueprint,
target_release.generation(),
&repo,
),
)?;

let Some(inventory) =
self.datastore().inventory_get_latest_collection(opctx).await?
else {
if allow_unverified_sled_software {
warn!(
log,
"skipping software checks for sled add: no inventory \
collection found, and the operator overrode these checks"
);
return Ok(());
}
return Err(Error::internal_error("no inventory collection found"));
};

Expand All @@ -84,19 +116,45 @@ impl super::Nexus {
})
}
Err(DeployedSledVersionsError::NotASled(sp_type)) => {
// This is not overridable. Return immediately.
return Err(SledAddPrecheckError::NotASled {
board: sled.clone(),
sp_type,
}
.into());
}
};
report.map_err(SledAddPrecheckError::from)?;
apply_override(&log, allow_unverified_sled_software, report)?;
}
Ok(())
}
}

/// Returns the error from a failed check unless `allowed` is set, in which
/// case the error is logged instead
fn apply_override<E>(
log: &Logger,
allowed: bool,
result: Result<(), E>,
) -> Result<(), SledAddPrecheckError>
where
E: std::error::Error + Into<SledAddPrecheckError>,
{
let Err(error) = result else {
return Ok(());
};
if !allowed {
return Err(error.into());
}
warn!(
log,
"sled add precheck failed, but proceeding because the operator \
overrode it";
InlineErrorChain::new(&error),
);
Ok(())
}

/// Checks that the system has settled on the target release described by
/// `repo`
///
Expand Down Expand Up @@ -170,6 +228,9 @@ enum SledAddPrecheckError {
}

/// Reasons that the system has not settled on its target release
///
/// The operator can override all of these with
/// `allow_without_settled_target_release`.
#[derive(Debug, thiserror::Error)]
enum SystemNotSettledError {
#[error(
Expand Down Expand Up @@ -199,6 +260,9 @@ enum SystemNotSettledError {

/// Reasons that a sled's software could not be confirmed to match the target
/// release
///
/// The operator can override all of these with
/// `allow_unverified_sled_software`.
#[derive(Debug, thiserror::Error)]
enum SledSoftwareError {
#[error("the software on sled {sled} could not be checked")]
Expand Down Expand Up @@ -314,6 +378,7 @@ mod tests {
use nexus_reconfigurator_planning::mgs_updates::RotCheckError;
use nexus_types::inventory::CabooseVersionError;
use nexus_types::inventory::CabooseWhich;
use omicron_test_utils::dev::test_setup_log;
use sled_hardware_types::GIMLET_SLED_MODEL;
use std::collections::BTreeMap;
use tufaceous_artifact::ArtifactHash;
Expand Down Expand Up @@ -458,4 +523,42 @@ mod tests {
}]
);
}

#[test]
fn test_apply_override() {
let logctx = test_setup_log("test_apply_override");
let log = &logctx.log;
let mismatch = || SledSoftwareError::Mismatch {
sled: test_sled(),
target_release_version: "17.0.0".parse().expect("valid version"),
problems: vec![SledSoftwareProblem::WrongImage {
component: MgsUpdateComponent::Sp,
found: "1.0.0".to_string(),
expected: "2.0.0".to_string(),
}],
};

// A check that passed is never an error, whether or not it was
// overridden.
apply_override::<SledSoftwareError>(log, false, Ok(()))
.expect("check passed");
apply_override::<SledSoftwareError>(log, true, Ok(()))
.expect("check passed");

// A check that failed is an error unless it was overridden.
let error = apply_override(log, false, Err(mismatch()))
.expect_err("not overridden");
assert!(
matches!(
error,
SledAddPrecheckError::SledSoftware(
SledSoftwareError::Mismatch { .. }
)
),
"unexpected error: {error:?}"
);
apply_override(log, true, Err(mismatch())).expect("overridden");

logctx.cleanup_successful();
}
}
4 changes: 3 additions & 1 deletion nexus/src/app/trust_quorum.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

use nexus_auth::authz;
use nexus_auth::context::OpContext;
use nexus_types::external_api::rack::SledAddPrecheckOverrides;
use nexus_types::trust_quorum::{
IsLrtqUpgrade, ProposedTrustQuorumConfig, TrustQuorumConfig,
};
Expand All @@ -28,6 +29,7 @@ impl super::Nexus {
opctx: &OpContext,
authz_tq: authz::TrustQuorumConfig,
new_sleds: BTreeSet<BaseboardId>,
override_prechecks: &SledAddPrecheckOverrides,
) -> Result<TrustQuorumConfig, Error> {
let rack_id = authz_tq.rack().id();
let (latest_committed_config, latest_epoch) = self
Expand All @@ -36,7 +38,7 @@ impl super::Nexus {

// Check the new sleds' software before allowing them to be added to
// the system.
self.sled_check_versions(opctx, &new_sleds).await?;
self.sled_check_versions(opctx, &new_sleds, override_prechecks).await?;

let new_epoch = latest_epoch.next();
let proposed = self
Expand Down
10 changes: 7 additions & 3 deletions nexus/src/external_api/http_entrypoints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6616,13 +6616,17 @@ impl NexusExternalApi for NexusExternalApiImpl {
req: TypedBody<rack::RackMembershipAddSledsRequest>,
) -> Result<HttpResponseOk<RackMembershipStatus>, HttpError> {
audit_and_time(&rqctx, |opctx, nexus| async move {
let req = req.into_inner();
let rack::RackMembershipAddSledsRequest {
sled_ids,
override_prechecks,
} = req.into_inner();
let rack_id = path_params.into_inner().rack_id;
let authz_tq = authz::TrustQuorumConfig::for_rack_id(
RackUuid::from_untyped_uuid(rack_id),
);
let status =
nexus.tq_add_sleds(&opctx, authz_tq, req.sled_ids).await?;
let status = nexus
.tq_add_sleds(&opctx, authz_tq, sled_ids, &override_prechecks)
.await?;
Ok(HttpResponseOk(status.into()))
})
.await
Expand Down
1 change: 1 addition & 0 deletions nexus/tests/integration_tests/endpoints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ pub static DEMO_RACK_ADD_SLEDS_REQUEST: LazyLock<
serial_number: "demo-serial".to_string(),
part_number: "demo-part".to_string(),
}]),
override_prechecks: rack::SledAddPrecheckOverrides::default(),
});

pub static DEMO_SLED_PROVISION_POLICY: LazyLock<
Expand Down
4 changes: 3 additions & 1 deletion nexus/types/versions/src/latest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -517,12 +517,14 @@ pub mod physical_disk {
pub mod rack {
pub use crate::v2025_11_20_00::rack::Rack;

pub use crate::v2026_01_21_00::rack::RackMembershipAddSledsRequest;
pub use crate::v2026_01_21_00::rack::RackMembershipChangeState;
pub use crate::v2026_01_21_00::rack::RackMembershipConfigPathParams;
pub use crate::v2026_01_21_00::rack::RackMembershipStatus;
pub use crate::v2026_01_21_00::rack::RackMembershipVersion;
pub use crate::v2026_01_21_00::rack::RackMembershipVersionParam;

pub use crate::v2026_10_29_00::rack::RackMembershipAddSledsRequest;
pub use crate::v2026_10_29_00::rack::SledAddPrecheckOverrides;
}

pub mod subnet_pool {
Expand Down
2 changes: 2 additions & 0 deletions nexus/types/versions/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,3 +111,5 @@ pub mod v2026_09_11_00;
pub mod v2026_09_15_00;
#[path = "artifact_hash_schema/mod.rs"]
pub mod v2026_10_13_00;
#[path = "sled_add_precheck_overrides/mod.rs"]
pub mod v2026_10_29_00;
9 changes: 9 additions & 0 deletions nexus/types/versions/src/sled_add_precheck_overrides/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// 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/.

//! Version `SLED_ADD_PRECHECK_OVERRIDES` of the Nexus external API.
//!
//! Adds `override_prechecks` to the request to add sleds to a rack.

pub mod rack;
Loading
Loading