From bab82b0f2475509ed36281b27a70b349c624ef19 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Wed, 5 Aug 2026 11:12:03 -0400 Subject: [PATCH 1/4] expand TargetReleaseChangeError to detect mixed-version mupdate --- nexus/src/app/deployment.rs | 238 ++++++++++++++++++++++++++---------- nexus/src/app/update.rs | 17 ++- 2 files changed, 186 insertions(+), 69 deletions(-) diff --git a/nexus/src/app/deployment.rs b/nexus/src/app/deployment.rs index f529484befb..0adfac353ed 100644 --- a/nexus/src/app/deployment.rs +++ b/nexus/src/app/deployment.rs @@ -38,6 +38,7 @@ use slog::Logger; use slog::warn; use slog_error_chain::InlineErrorChain; use std::collections::BTreeMap; +use std::collections::BTreeSet; use std::sync::Arc; use uuid::Uuid; @@ -429,14 +430,23 @@ impl super::Nexus { enum TargetReleaseChangeError { #[error("no evidence a mupdate has occurred - recovery not needed")] NoMupdateRecoveryNeeded, + #[error( + "mupdate recovery required, but found other versions in addititon to \ + {proposed_new_version} (this is an invalid state that requires \ + support intervention): {}", + display_versions_found(versions_found) + )] + MupdateRecoveryMixedVersions { + versions_found: BTreeMap>, + proposed_new_version: semver::Version, + }, #[error( "mupdate recovery required, but specified version \ - {proposed_new_version} does not match the version of \ - components deployed on sled {sled_id} ({version_found})" + {proposed_new_version} was not found; {}", + display_versions_found(versions_found) )] - MupdateRecoveryToWrongVersion { - sled_id: SledUuid, - version_found: BlueprintArtifactVersion, + MupdateRecoveryVersionNotFound { + versions_found: BTreeMap>, proposed_new_version: semver::Version, }, #[error( @@ -460,6 +470,34 @@ enum TargetReleaseChangeError { is older than current target release version {current}" )] CannotDowngrade { current: semver::Version, proposed: semver::Version }, + #[error( + "cannot determine whether setting the target release is possible: \ + no sleds found in current blueprint (this is unexpected!)" + )] + NoSledsFound, +} + +// Helper for the `#[error(..)]` strings on `TargetReleaseChangeError`. +fn display_versions_found( + versions_found: &BTreeMap>, +) -> String { + match versions_found.len() { + 0 => "found no versions (this is unexpected!)".to_string(), + 1 => { + let (version, sleds) = versions_found.iter().next().unwrap(); + format!("found version {version} on {} sleds", sleds.len()) + } + n => { + let versions = versions_found + .iter() + .map(|(version, sleds)| { + let plural = if sleds.len() > 1 { "s" } else { "" }; + format!("{version} ({} sled{plural})", sleds.len()) + }) + .collect::>(); + format!("found {n} versions: {}", versions.join(", ")) + } + } } // Check whether we should allow an operator to change the current target @@ -502,7 +540,7 @@ fn validate_can_set_target_release_for_mupdate_recovery( current_blueprint, proposed_new_version, ) { - BlueprintTargetReleaseStatus::AllComponentsMatchTargetRelease => { + BlueprintTargetReleaseStatus::AllComponentsMatch => { if min_target_release_gen_is_ahead_of_actual_target_release_gen { // All components are on the proposed new version, but we need // to allow recovery to catch up to the min target release @@ -542,32 +580,70 @@ fn validate_can_set_target_release_for_mupdate_recovery( Ok(()) } BlueprintTargetReleaseStatus::FoundDifferentVersion { - sled_id, - version_found, + found_version_to_check, + different_versions_found, } => { - // There are two obvious ways to get here: + // If we get here, that means the operator has requested a mupdate + // recovery to version N, but we found at least one component + // running some other version M. There are several possibilities + // here: + // + // 1. The operator called this endpoint erroneously: there was not a + // mupdate at all, and version N doesn't match all the software + // currently deployed on the rack, either because the rack is on + // some other version M or because the rack is currently in the + // middle of a live update between M and N (either direction). + // 2. The operator called this endpoint incorrectly: there was a + // mupdate, but to version N, not version M. + // 3. The rack was mupdated in an invalid way. The only valid + // mupdates are: + // + // * The entire rack is mupdated together to some new version. + // * If the entire rack is running some version X, any sled(s) + // can be mupdated to version X. (This is part of the "sled + // add process, where we mupdate new sleds to a matching + // version before adding them to the control plane.) + // + // A couple invalid mupdates that could land us in this branch + // are: // - // 1. No mupdate has happened, and the operator has called this - // endpoint erroneously - // 2. A mupdate to the current target release has happened, but the - // operator has called this endpoint with the wrong version + // * The rack is currently undergoing a live update, and a + // single sled was mupdated. + // * The rack as a whole was running version M, but some strict + // subset of sleds were mupdated to version N // - // We'll key off of - // `min_target_release_gen_is_ahead_of_actual_target_release_gen` to - // try to guess which case we're in: if it does look like a mupdate - // has happened that needs to be recovered from, we'll return an - // error noting that we think we're in case 2. Otherwise, it looks - // like we're in case 1 and no mupdate recovery is needed. + // In either of these cases, we're going to reject the operator's + // request to recover, and there's nothing they can do about it + // without getting support involved. But that's critical here: the + // rack is in some invalid, unsupported state, and we don't know + // whether it's safe to recover here without a person + // investigating and (probably) correcting the situation. if min_target_release_gen_is_ahead_of_actual_target_release_gen { - Err(TargetReleaseChangeError::MupdateRecoveryToWrongVersion { - sled_id, - version_found, - proposed_new_version: proposed_new_version.clone(), - }) + // Min target release gen ahead of actual means we did detect a + // mupdate: we're in either case 2 or 3 above. If we found the + // version the operator requested, it we're in case 3 (a mupdate + // has occurred and some components are on version N, but not + // all); otherwise, we're in case 2 (a mupdate has occurred, but + // not to version N). + if found_version_to_check { + Err(TargetReleaseChangeError::MupdateRecoveryMixedVersions { + versions_found: different_versions_found, + proposed_new_version: proposed_new_version.clone(), + }) + } else { + Err(TargetReleaseChangeError::MupdateRecoveryVersionNotFound { + versions_found: different_versions_found, + proposed_new_version: proposed_new_version.clone(), + }) + } } else { + // We're in case 1: we haven't detected a mupdate at all. Err(TargetReleaseChangeError::NoMupdateRecoveryNeeded) } } + BlueprintTargetReleaseStatus::NoSledsFound => { + Err(TargetReleaseChangeError::NoSledsFound) + } } } @@ -633,19 +709,31 @@ fn validate_update_version_number_ordering( pub(super) enum BlueprintTargetReleaseStatus { /// All sled and zone configs match the specified target release version; no /// evidence of a mupdate. - AllComponentsMatchTargetRelease, + AllComponentsMatch, + /// At least one sled or zone shows evidence of a mupdate that must be /// cleared. WaitingForMupdateToBeCleared { how: SledMupdateDetectedHow, sled_id: SledUuid, }, + /// At least one sled or zone is not on the specified target release /// version (and no mupdate evidence was found). FoundDifferentVersion { - sled_id: SledUuid, - version_found: BlueprintArtifactVersion, + /// Did we find the version we're looking for on at least one component? + found_version_to_check: bool, + + /// Guaranteed non-empty map of other versions we found and which sleds + /// we found them on. + different_versions_found: + BTreeMap>, }, + + /// No sleds were found in the current blueprint. + /// + /// This should be impossible. + NoSledsFound, } impl BlueprintTargetReleaseStatus { @@ -657,7 +745,7 @@ impl BlueprintTargetReleaseStatus { // `WaitingForMupdateToBeCleared { .. }` will be returned // 2. Otherwise, if we find any components at a version other than // `version_to_check`, `FoundDifferentVersion { .. }` will be returned - // 3. Otherwise, `AllComponentsMatchTargetRelease` will be returned. + // 3. Otherwise, `AllComponentsMatch` will be returned. // // We don't attempt to check Hubris components: // @@ -670,7 +758,11 @@ impl BlueprintTargetReleaseStatus { version_to_check: &semver::Version, ) -> Self { let mut found_mupdate = None; - let mut found_different_version = None; + let mut found_version_to_check = false; + let mut different_versions_found: BTreeMap< + BlueprintArtifactVersion, + BTreeSet, + > = BTreeMap::new(); // Blueprint artifact versions are stored as strings, not // `semver::Version`s. Here we're only looking at zone and OS versions, @@ -685,11 +777,14 @@ impl BlueprintTargetReleaseStatus { found_mupdate.get_or_insert((how, sled_id)); } SledUpdateStatus::FoundDifferentVersion { os_version } => { - found_different_version - .get_or_insert((sled_id, os_version)); + different_versions_found + .entry(os_version) + .or_default() + .insert(sled_id); } SledUpdateStatus::VersionMatches => { // This sled is okay; move on to the next. + found_version_to_check = true; } } } @@ -711,10 +806,13 @@ impl BlueprintTargetReleaseStatus { BlueprintZoneImageSource::Artifact { version, .. } => { match version { BlueprintArtifactVersion::Available { version: v } => { - if v.as_str() != version_to_check { - found_different_version.get_or_insert_with( - || (sled_id, version.clone()), - ); + if v.as_str() == version_to_check { + found_version_to_check = true; + } else { + different_versions_found + .entry(version.clone()) + .or_default() + .insert(sled_id); } } // This shouldn't happen; it means we have an artifact @@ -726,9 +824,10 @@ impl BlueprintTargetReleaseStatus { // For now, record this as "not the version we're // checking for". BlueprintArtifactVersion::Unknown => { - found_different_version.get_or_insert_with(|| { - (sled_id, version.clone()) - }); + different_versions_found + .entry(version.clone()) + .or_default() + .insert(sled_id); } } } @@ -736,22 +835,25 @@ impl BlueprintTargetReleaseStatus { } // Prioritize "found a mupdate" > "found a wrong version" > "ok" - match (found_mupdate, found_different_version) { - (Some((how, sled_id)), _) => { - BlueprintTargetReleaseStatus::WaitingForMupdateToBeCleared { - how, - sled_id, - } + if let Some((how, sled_id)) = found_mupdate { + BlueprintTargetReleaseStatus::WaitingForMupdateToBeCleared { + how, + sled_id, } - (None, Some((sled_id, version_found))) => { - BlueprintTargetReleaseStatus::FoundDifferentVersion { - sled_id, - version_found, - } - } - (None, None) => { - BlueprintTargetReleaseStatus::AllComponentsMatchTargetRelease + } else if !different_versions_found.is_empty() { + BlueprintTargetReleaseStatus::FoundDifferentVersion { + found_version_to_check, + different_versions_found, } + } else if found_version_to_check { + BlueprintTargetReleaseStatus::AllComponentsMatch + } else { + // The loops above over the blueprint's active sleds / current zones + // always set one of the three previous values. The only way to land + // in this branch is if the loops didn't iterate at all, which means + // we have no active sleds in the blueprint. This should be + // impossible; we'll turn this into a 500 error on the way out. + BlueprintTargetReleaseStatus::NoSledsFound } } } @@ -794,7 +896,7 @@ fn validate_can_set_target_release_for_update( ) { // When all components are on the current target release it means no // mupdate is detected - BlueprintTargetReleaseStatus::AllComponentsMatchTargetRelease => Ok(()), + BlueprintTargetReleaseStatus::AllComponentsMatch => Ok(()), BlueprintTargetReleaseStatus::WaitingForMupdateToBeCleared { how, sled_id, @@ -808,17 +910,20 @@ fn validate_can_set_target_release_for_update( Err(TargetReleaseChangeError::WaitingForMupdateToBeCleared) } BlueprintTargetReleaseStatus::FoundDifferentVersion { - sled_id, - version_found, + found_version_to_check, + different_versions_found, } => { warn!( log, "cannot start update: previous update not complete"; - "sled_id" => %sled_id, - "version_found" => %version_found, + "found_current_target_version" => found_version_to_check, + "other_versions_found" => ?different_versions_found, ); Err(TargetReleaseChangeError::PreviousUpdateInProgress) } + BlueprintTargetReleaseStatus::NoSledsFound => { + Err(TargetReleaseChangeError::NoSledsFound) + } } } @@ -1463,15 +1568,20 @@ mod tests { // showing evidence of a mupdate, so we only allow mupdate recovery to a // version that matches all configured component sources. let expected_err = - TargetReleaseChangeError::MupdateRecoveryToWrongVersion { - // Our checks always return the first sled with a problem, which - // in this case just means "the first sled". - sled_id: blueprint.active_sled_configs().next().unwrap().0, + TargetReleaseChangeError::MupdateRecoveryVersionNotFound { proposed_new_version: different_version.clone(), - version_found: BlueprintArtifactVersion::Available { - version: ArtifactVersion::new(current_version.to_string()) + versions_found: BTreeMap::from([( + BlueprintArtifactVersion::Available { + version: ArtifactVersion::new( + current_version.to_string(), + ) .unwrap(), - }, + }, + blueprint + .active_sled_configs() + .map(|(sled_id, _)| sled_id) + .collect(), + )]), }; assert_eq!( validate_can_set_target_release_for_mupdate_recovery( diff --git a/nexus/src/app/update.rs b/nexus/src/app/update.rs index 71a630340f6..19bff33b9a6 100644 --- a/nexus/src/app/update.rs +++ b/nexus/src/app/update.rs @@ -375,15 +375,22 @@ fn is_update_in_progress( // components, the check below will sitll be sufficient. let blueprint_in_progress = match current_target_version { Some(v) => match BlueprintTargetReleaseStatus::new(blueprint, v) { + // Common cases: an update is or is not in progress. BlueprintTargetReleaseStatus::FoundDifferentVersion { .. } => true, - // We don't consider a Mupdate as an "update in-progress" because - // recofigurator is not driving this update. + BlueprintTargetReleaseStatus::AllComponentsMatch => false, + + // Less common cases: + // + // 1. We don't consider a Mupdate as an "update in-progress" because + // recofigurator is not driving this update. + // 2. We don't consider "no sleds" an "update in-progress" because + // it's a state that should only exist in pathological tests - if + // the blueprint has no sleds, there's no reasonable way for us + // to be executing this check anyway. BlueprintTargetReleaseStatus::WaitingForMupdateToBeCleared { .. } - | BlueprintTargetReleaseStatus::AllComponentsMatchTargetRelease => { - false - } + | BlueprintTargetReleaseStatus::NoSledsFound => false, }, // When `current_target_version` is `None` no target release has ever // been set. We can safely assume no update is in progress. From 24ad34556ee50c3ddd1d28a18b715c1dde8e41bd Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Wed, 5 Aug 2026 11:18:19 -0400 Subject: [PATCH 2/4] add MupdateRecoveryMixedVersions case to unit tests --- nexus/src/app/deployment.rs | 41 +++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/nexus/src/app/deployment.rs b/nexus/src/app/deployment.rs index 0adfac353ed..03cf0f580fd 100644 --- a/nexus/src/app/deployment.rs +++ b/nexus/src/app/deployment.rs @@ -1627,6 +1627,47 @@ mod tests { Err(TargetReleaseChangeError::NoMupdateRecoveryNeeded) ); + // Modify the blueprint to emulate an illegal mupdate: set the + // components on _one_ sled to `different_version`, but leave the rest + // at `current_version`. The error we get back should describe this. + { + let (_, sled_config) = blueprint.sleds.iter_mut().next().unwrap(); + sled_config.host_phase_2 = BlueprintHostPhase2DesiredSlots { + slot_a: make_os_artifact(&different_version), + slot_b: make_os_artifact(&different_version), + }; + for mut zone_config in sled_config.zones.iter_mut() { + zone_config.image_source = + make_zone_artifact(&different_version); + } + } + let expected_err = + TargetReleaseChangeError::MupdateRecoveryMixedVersions { + proposed_new_version: different_version.clone(), + versions_found: BTreeMap::from([( + BlueprintArtifactVersion::Available { + version: ArtifactVersion::new( + current_version.to_string(), + ) + .unwrap(), + }, + blueprint + .active_sled_configs() + .skip(1) // skip the one we "mupdated" + .map(|(sled_id, _)| sled_id) + .collect(), + )]), + }; + assert_eq!( + validate_can_set_target_release_for_mupdate_recovery( + &blueprint, + initial_target_release_generation, + &different_version, + log, + ), + Err(expected_err), + ); + logctx.cleanup_successful(); } } From fb7346b276ca10b50adfb1f034909ca64d05316d Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Wed, 5 Aug 2026 14:24:52 -0400 Subject: [PATCH 3/4] more accurate error conversion (NoSleds => internal error) --- nexus/src/app/deployment.rs | 49 ++++++++++++++++++++++++++++--------- 1 file changed, 37 insertions(+), 12 deletions(-) diff --git a/nexus/src/app/deployment.rs b/nexus/src/app/deployment.rs index 03cf0f580fd..77e6c09b758 100644 --- a/nexus/src/app/deployment.rs +++ b/nexus/src/app/deployment.rs @@ -375,7 +375,7 @@ impl super::Nexus { // because the system is in an illegal state that required // support intervention in the first place (mupdating a single // sled in the middle of a live update). - let validation_result = match intent { + match intent { SetTargetReleaseIntent::Update => { let current_version = self .datastore() @@ -386,7 +386,7 @@ impl super::Nexus { ¤t_version, &new_system_version, &self.log, - ) + )?; } SetTargetReleaseIntent::RecoverFromMupdate => { validate_can_set_target_release_for_mupdate_recovery( @@ -394,17 +394,9 @@ impl super::Nexus { *current_target_release.generation, &new_system_version, &self.log, - ) + )?; } - }; - - // Unpack the result and convert the error, if any. - let () = validation_result.map_err(|err| { - Error::invalid_request(format!( - "Target release cannot be changed: {}", - InlineErrorChain::new(&err), - )) - })?; + } } } @@ -500,6 +492,39 @@ fn display_versions_found( } } +impl From for Error { + fn from(err: TargetReleaseChangeError) -> Self { + match err { + // Each of these variants indicates that the system is not currently + // in a state consistent with the request to change the target + // release in the requested way. + TargetReleaseChangeError::NoMupdateRecoveryNeeded + | TargetReleaseChangeError::MupdateRecoveryMixedVersions { + .. + } + | TargetReleaseChangeError::MupdateRecoveryVersionNotFound { + .. + } + | TargetReleaseChangeError::WaitingForMupdateToBeCleared + | TargetReleaseChangeError::PreviousUpdateInProgress + | TargetReleaseChangeError::UpdateToIdenticalVersion(_) + | TargetReleaseChangeError::CannotSkipScheduledRelease { .. } + | TargetReleaseChangeError::CannotDowngrade { .. } => { + Error::invalid_request(format!( + "Target release cannot be changed: {}", + InlineErrorChain::new(&err) + )) + } + + // This should never happen, and if it does, it's not because the + // request was invalid in some way. + TargetReleaseChangeError::NoSledsFound => { + Error::internal_error(InlineErrorChain::new(&err).to_string()) + } + } + } +} + // Check whether we should allow an operator to change the current target // release to recover from a mupdate. // From 1f4f17af57c3bcb81c3adf18d5d5eb114e166b4d Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Wed, 5 Aug 2026 15:24:04 -0400 Subject: [PATCH 4/4] fix up integration test and error message typos --- nexus/src/app/deployment.rs | 3 ++- nexus/tests/integration_tests/target_release.rs | 5 ++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/nexus/src/app/deployment.rs b/nexus/src/app/deployment.rs index 77e6c09b758..e7fd932ebef 100644 --- a/nexus/src/app/deployment.rs +++ b/nexus/src/app/deployment.rs @@ -477,7 +477,8 @@ fn display_versions_found( 0 => "found no versions (this is unexpected!)".to_string(), 1 => { let (version, sleds) = versions_found.iter().next().unwrap(); - format!("found version {version} on {} sleds", sleds.len()) + let plural = if sleds.len() > 1 { "s" } else { "" }; + format!("found {version} on {} sled{plural}", sleds.len()) } n => { let versions = versions_found diff --git a/nexus/tests/integration_tests/target_release.rs b/nexus/tests/integration_tests/target_release.rs index f4a058a31a0..f13c2f52cd6 100644 --- a/nexus/tests/integration_tests/target_release.rs +++ b/nexus/tests/integration_tests/target_release.rs @@ -262,9 +262,8 @@ async fn mupdate_recovery_after_noop_conversion() -> Result<()> { let err = response.parsed_body::().unwrap(); for needle in [ "mupdate recovery required", - "components deployed on sled", - "2.0.0", - "1.0.0", + "version 2.0.0 was not found", + "found version 1.0.0 on 1 sled", ] { assert!( err.message.contains(needle),