From 013df0aa0fcd5acc3088728c1dcec15cfcfd2898 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Karen=20C=C3=A1rcamo?= Date: Mon, 29 Jul 2024 09:59:12 +1200 Subject: [PATCH 1/4] [sled-agent] Remove non self-assembling zone code (#6162) This commit removes all code that is no longer necessary now that the self assembling zone work is finished. Additionally, the `smf_helper` module has been moved to `illumos_utils` where all the Illumos helper code lives. Closes: https://github.com/oxidecomputer/omicron/issues/1898 --- illumos-utils/src/lib.rs | 1 + illumos-utils/src/running_zone.rs | 55 +------------------ .../src/smf_helper.rs | 24 ++++---- sled-agent/src/lib.rs | 1 - sled-agent/src/params.rs | 9 --- sled-agent/src/services.rs | 10 +--- 6 files changed, 15 insertions(+), 85 deletions(-) rename {sled-agent => illumos-utils}/src/smf_helper.rs (90%) diff --git a/illumos-utils/src/lib.rs b/illumos-utils/src/lib.rs index 03b4bfb5a7..7140c62981 100644 --- a/illumos-utils/src/lib.rs +++ b/illumos-utils/src/lib.rs @@ -23,6 +23,7 @@ pub mod opte; pub mod route; pub mod running_zone; pub mod scf; +pub mod smf_helper; pub mod svc; pub mod svcadm; pub mod vmm_reservoir; diff --git a/illumos-utils/src/running_zone.rs b/illumos-utils/src/running_zone.rs index 6dfd692794..605809f019 100644 --- a/illumos-utils/src/running_zone.rs +++ b/illumos-utils/src/running_zone.rs @@ -12,7 +12,7 @@ use crate::dladm::Etherstub; use crate::link::{Link, VnicAllocator}; use crate::opte::{Port, PortTicket}; use crate::svc::wait_for_service; -use crate::zone::{AddressRequest, IPADM, ZONE_PREFIX}; +use crate::zone::{AddressRequest, ZONE_PREFIX}; use crate::zpool::{PathInPool, ZpoolName}; use camino::{Utf8Path, Utf8PathBuf}; use camino_tempfile::Utf8TempDir; @@ -487,65 +487,12 @@ impl RunningZone { zone: zone.name.to_string(), })?; - // TODO https://github.com/oxidecomputer/omicron/issues/1898: - // Remove all non-self assembling code - - // If the zone is self-assembling, then SMF service(s) inside the zone - // will be creating the listen address for the zone's service(s), - // setting the appropriate ifprop MTU, and so on. The idea behind - // self-assembling zones is that once they boot there should be *no* - // zlogin required. - - // Use the zone ID in order to check if /var/svc/profile/site.xml - // exists. let id = Zones::id(&zone.name) .await? .ok_or_else(|| BootError::NoZoneId { zone: zone.name.clone() })?; - let site_profile_xml_exists = - std::path::Path::new(&zone.site_profile_xml_path()).exists(); let running_zone = RunningZone { id: Some(id), inner: zone }; - if !site_profile_xml_exists { - // If the zone is not self-assembling, make sure the control vnic - // has an IP MTU of 9000 inside the zone. - const CONTROL_VNIC_MTU: usize = 9000; - let vnic = running_zone.inner.control_vnic.name().to_string(); - - let commands = vec![ - vec![ - IPADM.to_string(), - "create-if".to_string(), - "-t".to_string(), - vnic.clone(), - ], - vec![ - IPADM.to_string(), - "set-ifprop".to_string(), - "-t".to_string(), - "-p".to_string(), - format!("mtu={}", CONTROL_VNIC_MTU), - "-m".to_string(), - "ipv4".to_string(), - vnic.clone(), - ], - vec![ - IPADM.to_string(), - "set-ifprop".to_string(), - "-t".to_string(), - "-p".to_string(), - format!("mtu={}", CONTROL_VNIC_MTU), - "-m".to_string(), - "ipv6".to_string(), - vnic, - ], - ]; - - for args in &commands { - running_zone.run_cmd(args)?; - } - } - Ok(running_zone) } diff --git a/sled-agent/src/smf_helper.rs b/illumos-utils/src/smf_helper.rs similarity index 90% rename from sled-agent/src/smf_helper.rs rename to illumos-utils/src/smf_helper.rs index 230f146323..2c24ceaa4d 100644 --- a/sled-agent/src/smf_helper.rs +++ b/illumos-utils/src/smf_helper.rs @@ -2,7 +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 illumos_utils::running_zone::RunningZone; +use crate::running_zone::RunningZone; +use crate::zone::SVCCFG; #[derive(thiserror::Error, Debug)] pub enum Error { @@ -10,7 +11,7 @@ pub enum Error { ZoneCommand { intent: String, #[source] - err: illumos_utils::running_zone::RunCommandError, + err: crate::running_zone::RunCommandError, }, } @@ -44,7 +45,7 @@ impl<'t> SmfHelper<'t> { { self.running_zone .run_cmd(&[ - illumos_utils::zone::SVCCFG, + SVCCFG, "-s", &self.default_smf_name, "setprop", @@ -70,7 +71,7 @@ impl<'t> SmfHelper<'t> { { self.running_zone .run_cmd(&[ - illumos_utils::zone::SVCCFG, + SVCCFG, "-s", &self.smf_name, "addpropvalue", @@ -98,7 +99,7 @@ impl<'t> SmfHelper<'t> { { self.running_zone .run_cmd(&[ - illumos_utils::zone::SVCCFG, + SVCCFG, "-s", &self.default_smf_name, "addpropvalue", @@ -124,7 +125,7 @@ impl<'t> SmfHelper<'t> { { self.running_zone .run_cmd(&[ - illumos_utils::zone::SVCCFG, + SVCCFG, "-s", &self.smf_name, "addpg", @@ -148,7 +149,7 @@ impl<'t> SmfHelper<'t> { { self.running_zone .run_cmd(&[ - illumos_utils::zone::SVCCFG, + SVCCFG, "-s", &self.smf_name, "delpg", @@ -176,7 +177,7 @@ impl<'t> SmfHelper<'t> { match self .running_zone .run_cmd(&[ - illumos_utils::zone::SVCCFG, + SVCCFG, "-s", &self.default_smf_name, "delpropvalue", @@ -202,12 +203,7 @@ impl<'t> SmfHelper<'t> { pub fn refresh(&self) -> Result<(), Error> { self.running_zone - .run_cmd(&[ - illumos_utils::zone::SVCCFG, - "-s", - &self.default_smf_name, - "refresh", - ]) + .run_cmd(&[SVCCFG, "-s", &self.default_smf_name, "refresh"]) .map_err(|err| Error::ZoneCommand { intent: format!( "Refresh SMF manifest {}", diff --git a/sled-agent/src/lib.rs b/sled-agent/src/lib.rs index f7dc23e4d9..a2421528e2 100644 --- a/sled-agent/src/lib.rs +++ b/sled-agent/src/lib.rs @@ -34,7 +34,6 @@ pub mod rack_setup; pub mod server; pub mod services; mod sled_agent; -mod smf_helper; mod storage_monitor; mod swap_device; mod updates; diff --git a/sled-agent/src/params.rs b/sled-agent/src/params.rs index f98d5c82e5..ef24e47594 100644 --- a/sled-agent/src/params.rs +++ b/sled-agent/src/params.rs @@ -333,15 +333,6 @@ impl OmicronZoneTypeExt for OmicronZoneConfig { } } -impl crate::smf_helper::Service for OmicronZoneType { - fn service_name(&self) -> String { - self.kind().service_prefix().to_owned() - } - fn smf_name(&self) -> String { - format!("svc:/oxide/{}", self.service_name()) - } -} - #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema, PartialEq)] pub struct TimeSync { /// The synchronization state of the sled, true when the system clock diff --git a/sled-agent/src/services.rs b/sled-agent/src/services.rs index bc5ba370da..b62350ca93 100644 --- a/sled-agent/src/services.rs +++ b/sled-agent/src/services.rs @@ -35,7 +35,6 @@ use crate::params::{ ZoneBundleCause, ZoneBundleMetadata, }; use crate::profile::*; -use crate::smf_helper::SmfHelper; use crate::zone_bundle::BundleError; use crate::zone_bundle::ZoneBundler; use anyhow::anyhow; @@ -55,6 +54,7 @@ use illumos_utils::running_zone::{ EnsureAddressError, InstalledZone, RunCommandError, RunningZone, ZoneBuilderFactory, }; +use illumos_utils::smf_helper::SmfHelper; use illumos_utils::zfs::ZONE_ZFS_RAMDISK_DATASET_MOUNTPOINT; use illumos_utils::zone::AddressRequest; use illumos_utils::zpool::{PathInPool, ZpoolName}; @@ -157,7 +157,7 @@ pub enum Error { SledLocalZone(anyhow::Error), #[error("Failed to issue SMF command: {0}")] - SmfCommand(#[from] crate::smf_helper::Error), + SmfCommand(#[from] illumos_utils::smf_helper::Error), #[error("{}", display_zone_init_errors(.0))] ZoneInitialize(Vec<(String, Box)>), @@ -498,7 +498,7 @@ enum SwitchService { SpSim, } -impl crate::smf_helper::Service for SwitchService { +impl illumos_utils::smf_helper::Service for SwitchService { fn service_name(&self) -> String { match self { SwitchService::ManagementGatewayService => "mgs", @@ -1507,10 +1507,6 @@ impl ServiceManager { ServiceBuilder::new("network/dns/client") .add_instance(ServiceInstanceBuilder::new("default")); - // TODO(https://github.com/oxidecomputer/omicron/issues/1898): - // - // These zones are self-assembling -- after they boot, there should - // be no "zlogin" necessary to initialize. match &request { ZoneArgs::Omicron(OmicronZoneConfigLocal { zone: From 1bb75f28043e4e4d8c522b7dfaeb7df43a32cbd7 Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Mon, 29 Jul 2024 13:28:20 -0400 Subject: [PATCH 2/4] Regions can now be read-only (#6150) Up until this point, regions were only ever read-write, and region snapshots were read-only. In order to support snapshot replacement, Crucible recently gained support for read-only downstairs that performs a "clone" operation to copy blocks from another read-only downstairs. This commit adds a "read-only" flag to Region, and adds support for Nexus initializing a downstairs with this new clone option. --- nexus/db-model/src/region.rs | 8 ++ nexus/db-model/src/schema.rs | 2 + nexus/db-model/src/schema_versions.rs | 3 +- nexus/db-queries/src/db/datastore/region.rs | 10 +- nexus/db-queries/src/db/datastore/volume.rs | 1 + .../src/db/queries/region_allocation.rs | 74 ++++++++------- .../output/region_allocate_distinct_sleds.sql | 31 ++++--- .../output/region_allocate_random_sleds.sql | 31 ++++--- ..._allocate_with_snapshot_distinct_sleds.sql | 31 ++++--- ...on_allocate_with_snapshot_random_sleds.sql | 31 ++++--- .../execution/src/omicron_physical_disks.rs | 1 + .../tasks/decommissioned_disk_cleaner.rs | 1 + .../tasks/region_replacement_driver.rs | 6 ++ nexus/src/app/crucible.rs | 93 ++++++++++--------- .../app/sagas/region_replacement_finish.rs | 1 + .../src/app/sagas/region_replacement_start.rs | 11 +++ schema/crdb/dbinit.sql | 6 +- schema/crdb/region-read-only/up01.sql | 4 + schema/crdb/region-read-only/up02.sql | 2 + 19 files changed, 214 insertions(+), 133 deletions(-) create mode 100644 schema/crdb/region-read-only/up01.sql create mode 100644 schema/crdb/region-read-only/up02.sql diff --git a/nexus/db-model/src/region.rs b/nexus/db-model/src/region.rs index 666a7ef456..e38c5543ef 100644 --- a/nexus/db-model/src/region.rs +++ b/nexus/db-model/src/region.rs @@ -43,6 +43,9 @@ pub struct Region { // The port that was returned when the region was created. This field didn't // originally exist, so records may not have it filled in. port: Option, + + // A region may be read-only + read_only: bool, } impl Region { @@ -53,6 +56,7 @@ impl Region { blocks_per_extent: u64, extent_count: u64, port: u16, + read_only: bool, ) -> Self { Self { identity: RegionIdentity::new(Uuid::new_v4()), @@ -62,6 +66,7 @@ impl Region { blocks_per_extent: blocks_per_extent as i64, extent_count: extent_count as i64, port: Some(port.into()), + read_only, } } @@ -91,4 +96,7 @@ impl Region { pub fn port(&self) -> Option { self.port.map(|port| port.into()) } + pub fn read_only(&self) -> bool { + self.read_only + } } diff --git a/nexus/db-model/src/schema.rs b/nexus/db-model/src/schema.rs index dc57de9263..81c8712279 100644 --- a/nexus/db-model/src/schema.rs +++ b/nexus/db-model/src/schema.rs @@ -1046,6 +1046,8 @@ table! { extent_count -> Int8, port -> Nullable, + + read_only -> Bool, } } diff --git a/nexus/db-model/src/schema_versions.rs b/nexus/db-model/src/schema_versions.rs index cc34a3581c..97c933c987 100644 --- a/nexus/db-model/src/schema_versions.rs +++ b/nexus/db-model/src/schema_versions.rs @@ -17,7 +17,7 @@ use std::collections::BTreeMap; /// /// This must be updated when you change the database schema. Refer to /// schema/crdb/README.adoc in the root of this repository for details. -pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(83, 0, 0); +pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(84, 0, 0); /// List of all past database schema versions, in *reverse* order /// @@ -29,6 +29,7 @@ static KNOWN_VERSIONS: Lazy> = Lazy::new(|| { // | leaving the first copy as an example for the next person. // v // KnownVersion::new(next_int, "unique-dirname-with-the-sql-files"), + KnownVersion::new(84, "region-read-only"), KnownVersion::new(83, "dataset-address-optional"), KnownVersion::new(82, "region-port"), KnownVersion::new(81, "add-nullable-filesystem-pool"), diff --git a/nexus/db-queries/src/db/datastore/region.rs b/nexus/db-queries/src/db/datastore/region.rs index 3b1c20c1df..df733af5b5 100644 --- a/nexus/db-queries/src/db/datastore/region.rs +++ b/nexus/db-queries/src/db/datastore/region.rs @@ -19,6 +19,7 @@ use crate::db::model::Region; use crate::db::model::SqlU16; use crate::db::pagination::paginated; use crate::db::pagination::Paginator; +use crate::db::queries::region_allocation::RegionParameters; use crate::db::update_and_check::UpdateAndCheck; use crate::db::update_and_check::UpdateStatus; use crate::transaction_retry::OptionalError; @@ -259,9 +260,12 @@ impl DataStore { let query = crate::db::queries::region_allocation::allocation_query( volume_id, maybe_snapshot_id, - block_size, - blocks_per_extent, - extent_count, + RegionParameters { + block_size, + blocks_per_extent, + extent_count, + read_only: false, + }, allocation_strategy, num_regions_required, ); diff --git a/nexus/db-queries/src/db/datastore/volume.rs b/nexus/db-queries/src/db/datastore/volume.rs index b13006aa95..4ecba530b1 100644 --- a/nexus/db-queries/src/db/datastore/volume.rs +++ b/nexus/db-queries/src/db/datastore/volume.rs @@ -2349,6 +2349,7 @@ mod tests { 10, 10, 10001, + false, ); region_and_volume_ids[i].0 = region.id(); diff --git a/nexus/db-queries/src/db/queries/region_allocation.rs b/nexus/db-queries/src/db/queries/region_allocation.rs index cefca5914f..7cf378d53b 100644 --- a/nexus/db-queries/src/db/queries/region_allocation.rs +++ b/nexus/db-queries/src/db/queries/region_allocation.rs @@ -67,6 +67,18 @@ type SelectableSql = < >::SelectExpression as diesel::Expression >::SqlType; +/// Parameters for the region(s) being allocated +#[derive(Debug, Clone, Copy)] +pub struct RegionParameters { + pub block_size: u64, + pub blocks_per_extent: u64, + pub extent_count: u64, + + /// True if the region will be filled with a Clone operation and is meant to + /// be read-only. + pub read_only: bool, +} + /// For a given volume, idempotently allocate enough regions (according to some /// allocation strategy) to meet some redundancy level. This should only be used /// for the region set that is in the top level of the Volume (not the deeper @@ -75,9 +87,7 @@ type SelectableSql = < pub fn allocation_query( volume_id: uuid::Uuid, snapshot_id: Option, - block_size: u64, - blocks_per_extent: u64, - extent_count: u64, + params: RegionParameters, allocation_strategy: &RegionAllocationStrategy, redundancy: usize, ) -> TypedSqlQuery<(SelectableSql, SelectableSql)> { @@ -104,7 +114,8 @@ pub fn allocation_query( let seed = seed.to_le_bytes().to_vec(); - let size_delta = block_size * blocks_per_extent * extent_count; + let size_delta = + params.block_size * params.blocks_per_extent * params.extent_count; let redundancy: i64 = i64::try_from(redundancy).unwrap(); let builder = QueryBuilder::new().sql( @@ -243,7 +254,8 @@ pub fn allocation_query( ").param().sql(" AS block_size, ").param().sql(" AS blocks_per_extent, ").param().sql(" AS extent_count, - NULL AS port + NULL AS port, + ").param().sql(" AS read_only FROM shuffled_candidate_datasets") // Only select the *additional* number of candidate regions for the required // redundancy level @@ -253,9 +265,10 @@ pub fn allocation_query( )) ),") .bind::(volume_id) - .bind::(block_size as i64) - .bind::(blocks_per_extent as i64) - .bind::(extent_count as i64) + .bind::(params.block_size as i64) + .bind::(params.blocks_per_extent as i64) + .bind::(params.extent_count as i64) + .bind::(params.read_only) .bind::(redundancy) // A subquery which summarizes the changes we intend to make, showing: @@ -355,7 +368,7 @@ pub fn allocation_query( .sql(" inserted_regions AS ( INSERT INTO region - (id, time_created, time_modified, dataset_id, volume_id, block_size, blocks_per_extent, extent_count, port) + (id, time_created, time_modified, dataset_id, volume_id, block_size, blocks_per_extent, extent_count, port, read_only) SELECT ").sql(AllColumnsOfRegion::with_prefix("candidate_regions")).sql(" FROM candidate_regions WHERE @@ -405,9 +418,12 @@ mod test { #[tokio::test] async fn expectorate_query() { let volume_id = Uuid::nil(); - let block_size = 512; - let blocks_per_extent = 4; - let extent_count = 8; + let params = RegionParameters { + block_size: 512, + blocks_per_extent: 4, + extent_count: 8, + read_only: false, + }; // Start with snapshot_id = None @@ -418,14 +434,13 @@ mod test { let region_allocate = allocation_query( volume_id, snapshot_id, - block_size, - blocks_per_extent, - extent_count, + params, &RegionAllocationStrategy::RandomWithDistinctSleds { seed: Some(1), }, REGION_REDUNDANCY_THRESHOLD, ); + expectorate_query_contents( ®ion_allocate, "tests/output/region_allocate_distinct_sleds.sql", @@ -437,9 +452,7 @@ mod test { let region_allocate = allocation_query( volume_id, snapshot_id, - block_size, - blocks_per_extent, - extent_count, + params, &RegionAllocationStrategy::Random { seed: Some(1) }, REGION_REDUNDANCY_THRESHOLD, ); @@ -458,9 +471,7 @@ mod test { let region_allocate = allocation_query( volume_id, snapshot_id, - block_size, - blocks_per_extent, - extent_count, + params, &RegionAllocationStrategy::RandomWithDistinctSleds { seed: Some(1), }, @@ -477,9 +488,7 @@ mod test { let region_allocate = allocation_query( volume_id, snapshot_id, - block_size, - blocks_per_extent, - extent_count, + params, &RegionAllocationStrategy::Random { seed: Some(1) }, REGION_REDUNDANCY_THRESHOLD, ); @@ -502,18 +511,19 @@ mod test { let conn = pool.pool().get().await.unwrap(); let volume_id = Uuid::new_v4(); - let block_size = 512; - let blocks_per_extent = 4; - let extent_count = 8; + let params = RegionParameters { + block_size: 512, + blocks_per_extent: 4, + extent_count: 8, + read_only: false, + }; // First structure: Explain the query with "RandomWithDistinctSleds" let region_allocate = allocation_query( volume_id, None, - block_size, - blocks_per_extent, - extent_count, + params, &RegionAllocationStrategy::RandomWithDistinctSleds { seed: None }, REGION_REDUNDANCY_THRESHOLD, ); @@ -527,9 +537,7 @@ mod test { let region_allocate = allocation_query( volume_id, None, - block_size, - blocks_per_extent, - extent_count, + params, &RegionAllocationStrategy::Random { seed: None }, REGION_REDUNDANCY_THRESHOLD, ); diff --git a/nexus/db-queries/tests/output/region_allocate_distinct_sleds.sql b/nexus/db-queries/tests/output/region_allocate_distinct_sleds.sql index 624428c217..6331770ef5 100644 --- a/nexus/db-queries/tests/output/region_allocate_distinct_sleds.sql +++ b/nexus/db-queries/tests/output/region_allocate_distinct_sleds.sql @@ -10,7 +10,8 @@ WITH region.block_size, region.blocks_per_extent, region.extent_count, - region.port + region.port, + region.read_only FROM region WHERE @@ -99,11 +100,12 @@ WITH $8 AS block_size, $9 AS blocks_per_extent, $10 AS extent_count, - NULL AS port + NULL AS port, + $11 AS read_only FROM shuffled_candidate_datasets LIMIT - $11 - (SELECT count(*) FROM old_regions) + $12 - (SELECT count(*) FROM old_regions) ), proposed_dataset_changes AS ( @@ -122,7 +124,7 @@ WITH SELECT ( ( - (SELECT count(*) FROM old_regions LIMIT 1) < $12 + (SELECT count(*) FROM old_regions LIMIT 1) < $13 AND CAST( IF( ( @@ -132,7 +134,7 @@ WITH + (SELECT count(*) FROM existing_zpools LIMIT 1) ) ) - >= $13 + >= $14 ), 'TRUE', 'Not enough space' @@ -149,7 +151,7 @@ WITH + (SELECT count(*) FROM old_regions LIMIT 1) ) ) - >= $14 + >= $15 ), 'TRUE', 'Not enough datasets' @@ -185,7 +187,7 @@ WITH 1 ) ) - >= $15 + >= $16 ), 'TRUE', 'Not enough unique zpools selected' @@ -208,7 +210,8 @@ WITH block_size, blocks_per_extent, extent_count, - port + port, + read_only ) SELECT candidate_regions.id, @@ -219,7 +222,8 @@ WITH candidate_regions.block_size, candidate_regions.blocks_per_extent, candidate_regions.extent_count, - candidate_regions.port + candidate_regions.port, + candidate_regions.read_only FROM candidate_regions WHERE @@ -233,7 +237,8 @@ WITH region.block_size, region.blocks_per_extent, region.extent_count, - region.port + region.port, + region.read_only ), updated_datasets AS ( @@ -287,7 +292,8 @@ WITH old_regions.block_size, old_regions.blocks_per_extent, old_regions.extent_count, - old_regions.port + old_regions.port, + old_regions.read_only FROM old_regions INNER JOIN dataset ON old_regions.dataset_id = dataset.id ) @@ -312,7 +318,8 @@ UNION inserted_regions.block_size, inserted_regions.blocks_per_extent, inserted_regions.extent_count, - inserted_regions.port + inserted_regions.port, + inserted_regions.read_only FROM inserted_regions INNER JOIN updated_datasets ON inserted_regions.dataset_id = updated_datasets.id diff --git a/nexus/db-queries/tests/output/region_allocate_random_sleds.sql b/nexus/db-queries/tests/output/region_allocate_random_sleds.sql index 748e0d7b15..e713121d34 100644 --- a/nexus/db-queries/tests/output/region_allocate_random_sleds.sql +++ b/nexus/db-queries/tests/output/region_allocate_random_sleds.sql @@ -10,7 +10,8 @@ WITH region.block_size, region.blocks_per_extent, region.extent_count, - region.port + region.port, + region.read_only FROM region WHERE @@ -97,11 +98,12 @@ WITH $7 AS block_size, $8 AS blocks_per_extent, $9 AS extent_count, - NULL AS port + NULL AS port, + $10 AS read_only FROM shuffled_candidate_datasets LIMIT - $10 - (SELECT count(*) FROM old_regions) + $11 - (SELECT count(*) FROM old_regions) ), proposed_dataset_changes AS ( @@ -120,7 +122,7 @@ WITH SELECT ( ( - (SELECT count(*) FROM old_regions LIMIT 1) < $11 + (SELECT count(*) FROM old_regions LIMIT 1) < $12 AND CAST( IF( ( @@ -130,7 +132,7 @@ WITH + (SELECT count(*) FROM existing_zpools LIMIT 1) ) ) - >= $12 + >= $13 ), 'TRUE', 'Not enough space' @@ -147,7 +149,7 @@ WITH + (SELECT count(*) FROM old_regions LIMIT 1) ) ) - >= $13 + >= $14 ), 'TRUE', 'Not enough datasets' @@ -183,7 +185,7 @@ WITH 1 ) ) - >= $14 + >= $15 ), 'TRUE', 'Not enough unique zpools selected' @@ -206,7 +208,8 @@ WITH block_size, blocks_per_extent, extent_count, - port + port, + read_only ) SELECT candidate_regions.id, @@ -217,7 +220,8 @@ WITH candidate_regions.block_size, candidate_regions.blocks_per_extent, candidate_regions.extent_count, - candidate_regions.port + candidate_regions.port, + candidate_regions.read_only FROM candidate_regions WHERE @@ -231,7 +235,8 @@ WITH region.block_size, region.blocks_per_extent, region.extent_count, - region.port + region.port, + region.read_only ), updated_datasets AS ( @@ -285,7 +290,8 @@ WITH old_regions.block_size, old_regions.blocks_per_extent, old_regions.extent_count, - old_regions.port + old_regions.port, + old_regions.read_only FROM old_regions INNER JOIN dataset ON old_regions.dataset_id = dataset.id ) @@ -310,7 +316,8 @@ UNION inserted_regions.block_size, inserted_regions.blocks_per_extent, inserted_regions.extent_count, - inserted_regions.port + inserted_regions.port, + inserted_regions.read_only FROM inserted_regions INNER JOIN updated_datasets ON inserted_regions.dataset_id = updated_datasets.id diff --git a/nexus/db-queries/tests/output/region_allocate_with_snapshot_distinct_sleds.sql b/nexus/db-queries/tests/output/region_allocate_with_snapshot_distinct_sleds.sql index 8faca9ed4f..0b8dc4fca6 100644 --- a/nexus/db-queries/tests/output/region_allocate_with_snapshot_distinct_sleds.sql +++ b/nexus/db-queries/tests/output/region_allocate_with_snapshot_distinct_sleds.sql @@ -10,7 +10,8 @@ WITH region.block_size, region.blocks_per_extent, region.extent_count, - region.port + region.port, + region.read_only FROM region WHERE @@ -110,11 +111,12 @@ WITH $9 AS block_size, $10 AS blocks_per_extent, $11 AS extent_count, - NULL AS port + NULL AS port, + $12 AS read_only FROM shuffled_candidate_datasets LIMIT - $12 - (SELECT count(*) FROM old_regions) + $13 - (SELECT count(*) FROM old_regions) ), proposed_dataset_changes AS ( @@ -133,7 +135,7 @@ WITH SELECT ( ( - (SELECT count(*) FROM old_regions LIMIT 1) < $13 + (SELECT count(*) FROM old_regions LIMIT 1) < $14 AND CAST( IF( ( @@ -143,7 +145,7 @@ WITH + (SELECT count(*) FROM existing_zpools LIMIT 1) ) ) - >= $14 + >= $15 ), 'TRUE', 'Not enough space' @@ -160,7 +162,7 @@ WITH + (SELECT count(*) FROM old_regions LIMIT 1) ) ) - >= $15 + >= $16 ), 'TRUE', 'Not enough datasets' @@ -196,7 +198,7 @@ WITH 1 ) ) - >= $16 + >= $17 ), 'TRUE', 'Not enough unique zpools selected' @@ -219,7 +221,8 @@ WITH block_size, blocks_per_extent, extent_count, - port + port, + read_only ) SELECT candidate_regions.id, @@ -230,7 +233,8 @@ WITH candidate_regions.block_size, candidate_regions.blocks_per_extent, candidate_regions.extent_count, - candidate_regions.port + candidate_regions.port, + candidate_regions.read_only FROM candidate_regions WHERE @@ -244,7 +248,8 @@ WITH region.block_size, region.blocks_per_extent, region.extent_count, - region.port + region.port, + region.read_only ), updated_datasets AS ( @@ -298,7 +303,8 @@ WITH old_regions.block_size, old_regions.blocks_per_extent, old_regions.extent_count, - old_regions.port + old_regions.port, + old_regions.read_only FROM old_regions INNER JOIN dataset ON old_regions.dataset_id = dataset.id ) @@ -323,7 +329,8 @@ UNION inserted_regions.block_size, inserted_regions.blocks_per_extent, inserted_regions.extent_count, - inserted_regions.port + inserted_regions.port, + inserted_regions.read_only FROM inserted_regions INNER JOIN updated_datasets ON inserted_regions.dataset_id = updated_datasets.id diff --git a/nexus/db-queries/tests/output/region_allocate_with_snapshot_random_sleds.sql b/nexus/db-queries/tests/output/region_allocate_with_snapshot_random_sleds.sql index c1c03672a4..9ac945f71d 100644 --- a/nexus/db-queries/tests/output/region_allocate_with_snapshot_random_sleds.sql +++ b/nexus/db-queries/tests/output/region_allocate_with_snapshot_random_sleds.sql @@ -10,7 +10,8 @@ WITH region.block_size, region.blocks_per_extent, region.extent_count, - region.port + region.port, + region.read_only FROM region WHERE @@ -108,11 +109,12 @@ WITH $8 AS block_size, $9 AS blocks_per_extent, $10 AS extent_count, - NULL AS port + NULL AS port, + $11 AS read_only FROM shuffled_candidate_datasets LIMIT - $11 - (SELECT count(*) FROM old_regions) + $12 - (SELECT count(*) FROM old_regions) ), proposed_dataset_changes AS ( @@ -131,7 +133,7 @@ WITH SELECT ( ( - (SELECT count(*) FROM old_regions LIMIT 1) < $12 + (SELECT count(*) FROM old_regions LIMIT 1) < $13 AND CAST( IF( ( @@ -141,7 +143,7 @@ WITH + (SELECT count(*) FROM existing_zpools LIMIT 1) ) ) - >= $13 + >= $14 ), 'TRUE', 'Not enough space' @@ -158,7 +160,7 @@ WITH + (SELECT count(*) FROM old_regions LIMIT 1) ) ) - >= $14 + >= $15 ), 'TRUE', 'Not enough datasets' @@ -194,7 +196,7 @@ WITH 1 ) ) - >= $15 + >= $16 ), 'TRUE', 'Not enough unique zpools selected' @@ -217,7 +219,8 @@ WITH block_size, blocks_per_extent, extent_count, - port + port, + read_only ) SELECT candidate_regions.id, @@ -228,7 +231,8 @@ WITH candidate_regions.block_size, candidate_regions.blocks_per_extent, candidate_regions.extent_count, - candidate_regions.port + candidate_regions.port, + candidate_regions.read_only FROM candidate_regions WHERE @@ -242,7 +246,8 @@ WITH region.block_size, region.blocks_per_extent, region.extent_count, - region.port + region.port, + region.read_only ), updated_datasets AS ( @@ -296,7 +301,8 @@ WITH old_regions.block_size, old_regions.blocks_per_extent, old_regions.extent_count, - old_regions.port + old_regions.port, + old_regions.read_only FROM old_regions INNER JOIN dataset ON old_regions.dataset_id = dataset.id ) @@ -321,7 +327,8 @@ UNION inserted_regions.block_size, inserted_regions.blocks_per_extent, inserted_regions.extent_count, - inserted_regions.port + inserted_regions.port, + inserted_regions.read_only FROM inserted_regions INNER JOIN updated_datasets ON inserted_regions.dataset_id = updated_datasets.id diff --git a/nexus/reconfigurator/execution/src/omicron_physical_disks.rs b/nexus/reconfigurator/execution/src/omicron_physical_disks.rs index 9ae72d2049..7adc41213e 100644 --- a/nexus/reconfigurator/execution/src/omicron_physical_disks.rs +++ b/nexus/reconfigurator/execution/src/omicron_physical_disks.rs @@ -450,6 +450,7 @@ mod test { 10, 10, 1, + false, ) }; let conn = datastore.pool_connection_for_tests().await.unwrap(); diff --git a/nexus/src/app/background/tasks/decommissioned_disk_cleaner.rs b/nexus/src/app/background/tasks/decommissioned_disk_cleaner.rs index cb3ef9a569..602f3f85e8 100644 --- a/nexus/src/app/background/tasks/decommissioned_disk_cleaner.rs +++ b/nexus/src/app/background/tasks/decommissioned_disk_cleaner.rs @@ -261,6 +261,7 @@ mod tests { 10, 10, 1, + false, ) }; let region_id = region.id(); diff --git a/nexus/src/app/background/tasks/region_replacement_driver.rs b/nexus/src/app/background/tasks/region_replacement_driver.rs index c3af324f44..284ed2c368 100644 --- a/nexus/src/app/background/tasks/region_replacement_driver.rs +++ b/nexus/src/app/background/tasks/region_replacement_driver.rs @@ -351,6 +351,7 @@ mod test { 10, 10, 27015, + false, ) }; @@ -364,6 +365,7 @@ mod test { 10, 10, 27016, + false, ) }; @@ -448,6 +450,7 @@ mod test { 10, 10, 27015, + false, ) }; @@ -461,6 +464,7 @@ mod test { 10, 10, 27016, + false, ) }; @@ -595,6 +599,7 @@ mod test { 10, 10, 27015, + false, ) }; @@ -608,6 +613,7 @@ mod test { 10, 10, 27016, + false, ) }; diff --git a/nexus/src/app/crucible.rs b/nexus/src/app/crucible.rs index 72a5c80baf..fdd67da617 100644 --- a/nexus/src/app/crucible.rs +++ b/nexus/src/app/crucible.rs @@ -26,7 +26,8 @@ use slog::Logger; // within a disk at the same time. const MAX_CONCURRENT_REGION_REQUESTS: usize = 3; -/// Provides a way for (with BackoffError) Permanent errors to have a different error type than +/// Provides a way for (with BackoffError) Permanent errors to have a different +/// error type than /// Transient errors. #[derive(Debug, thiserror::Error)] enum WaitError { @@ -146,12 +147,14 @@ impl super::Nexus { } } - /// Call out to Crucible agent and perform region creation. - async fn ensure_region_in_dataset( + /// Call out to Crucible agent and perform region creation. Optionally, + /// supply a read-only source to invoke a clone. + pub async fn ensure_region_in_dataset( &self, log: &Logger, dataset: &db::model::Dataset, region: &db::model::Region, + source: Option, ) -> Result { let client = self.crucible_agent_client_for_dataset(dataset)?; let dataset_id = dataset.id(); @@ -173,7 +176,7 @@ impl super::Nexus { cert_pem: None, key_pem: None, root_pem: None, - source: None, + source, }; let create_region = || async { @@ -593,9 +596,10 @@ impl super::Nexus { .await .map_err(|e| match e { WaitError::Transient(e) => { - // The backoff crate can be configured with a maximum elapsed time - // before giving up, which means that Transient could be returned - // here. Our current policies do **not** set this though. + // The backoff crate can be configured with a maximum elapsed + // time before giving up, which means that Transient could be + // returned here. Our current policies do **not** set this + // though. Error::internal_error(&e.to_string()) } @@ -625,12 +629,10 @@ impl super::Nexus { backoff::retry_notify( backoff::retry_policy_internal_service_aggressive(), || async { - let response = match self.get_crucible_region_snapshots( - log, - dataset, - region_id, - ) - .await { + let response = match self + .get_crucible_region_snapshots(log, dataset, region_id) + .await + { Ok(v) => Ok(v), // Return Ok if the dataset's agent is gone, no @@ -645,7 +647,9 @@ impl super::Nexus { return Ok(()); } - Err(e) => Err(BackoffError::Permanent(WaitError::Permanent(e))), + Err(e) => { + Err(BackoffError::Permanent(WaitError::Permanent(e))) + } }?; match response.running_snapshots.get(&snapshot_id.to_string()) { @@ -662,27 +666,23 @@ impl super::Nexus { RegionState::Tombstoned => { Err(BackoffError::transient( WaitError::Transient(anyhow!( - "running_snapshot tombstoned, not deleted yet", - ) - ))) + "running_snapshot tombstoned, not \ + deleted yet", + )), + )) } RegionState::Destroyed => { - info!( - log, - "running_snapshot deleted", - ); + info!(log, "running_snapshot deleted",); Ok(()) } - _ => { - Err(BackoffError::transient( - WaitError::Transient(anyhow!( - "running_snapshot unexpected state", - ) - ))) - } + _ => Err(BackoffError::transient( + WaitError::Transient(anyhow!( + "running_snapshot unexpected state", + )), + )), } } @@ -710,20 +710,19 @@ impl super::Nexus { "region_id" => %region_id, "snapshot_id" => %snapshot_id, ); - } + }, ) .await .map_err(|e| match e { WaitError::Transient(e) => { - // The backoff crate can be configured with a maximum elapsed time - // before giving up, which means that Transient could be returned - // here. Our current policies do **not** set this though. + // The backoff crate can be configured with a maximum elapsed + // time before giving up, which means that Transient could be + // returned here. Our current policies do **not** set this + // though. Error::internal_error(&e.to_string()) } - WaitError::Permanent(e) => { - e - } + WaitError::Permanent(e) => e, }) } @@ -745,11 +744,12 @@ impl super::Nexus { region_id: Uuid, snapshot_id: Uuid, ) -> Result<(), Error> { - // Unlike other Crucible agent endpoints, this one is synchronous in that it - // is not only a request to the Crucible agent: `zfs destroy` is performed - // right away. However this is still a request to illumos that may not take - // effect right away. Wait until the snapshot no longer appears in the list - // of region snapshots, meaning it was not returned from `zfs list`. + // Unlike other Crucible agent endpoints, this one is synchronous in + // that it is not only a request to the Crucible agent: `zfs destroy` is + // performed right away. However this is still a request to illumos that + // may not take effect right away. Wait until the snapshot no longer + // appears in the list of region snapshots, meaning it was not returned + // from `zfs list`. let dataset_id = dataset.id(); @@ -838,9 +838,10 @@ impl super::Nexus { .await .map_err(|e| match e { WaitError::Transient(e) => { - // The backoff crate can be configured with a maximum elapsed time - // before giving up, which means that Transient could be returned - // here. Our current policies do **not** set this though. + // The backoff crate can be configured with a maximum elapsed + // time before giving up, which means that Transient could be + // returned here. Our current policies do **not** set this + // though. Error::internal_error(&e.to_string()) } @@ -860,13 +861,13 @@ impl super::Nexus { return Ok(vec![]); } - // Allocate regions, and additionally return the dataset that the region was - // allocated in. + // Allocate regions, and additionally return the dataset that the region + // was allocated in. let datasets_and_regions: Vec<(db::model::Dataset, Region)> = futures::stream::iter(datasets_and_regions) .map(|(dataset, region)| async move { match self - .ensure_region_in_dataset(log, &dataset, ®ion) + .ensure_region_in_dataset(log, &dataset, ®ion, None) .await { Ok(result) => Ok((dataset, result)), diff --git a/nexus/src/app/sagas/region_replacement_finish.rs b/nexus/src/app/sagas/region_replacement_finish.rs index a6964a8d06..e17f3405a0 100644 --- a/nexus/src/app/sagas/region_replacement_finish.rs +++ b/nexus/src/app/sagas/region_replacement_finish.rs @@ -249,6 +249,7 @@ pub(crate) mod test { 10, 10, 12345, + false, ) }; diff --git a/nexus/src/app/sagas/region_replacement_start.rs b/nexus/src/app/sagas/region_replacement_start.rs index 1297158b24..d4d455f927 100644 --- a/nexus/src/app/sagas/region_replacement_start.rs +++ b/nexus/src/app/sagas/region_replacement_start.rs @@ -221,6 +221,13 @@ async fn srrs_get_existing_datasets_and_regions( .await .map_err(ActionError::action_failed)?; + // XXX for now, bail out if requesting the replacement of a read-only region + if db_region.read_only() { + return Err(ActionError::action_failed(String::from( + "replacing read-only region currently unsupported", + ))); + } + // Find out the existing datasets and regions that back the volume let datasets_and_regions = osagactx .datastore() @@ -925,6 +932,7 @@ pub(crate) mod test { 10, 10, 1001, + false, ), Region::new( datasets[1].id(), @@ -933,6 +941,7 @@ pub(crate) mod test { 10, 10, 1002, + false, ), Region::new( datasets[2].id(), @@ -941,6 +950,7 @@ pub(crate) mod test { 10, 10, 1003, + false, ), Region::new( datasets[3].id(), @@ -949,6 +959,7 @@ pub(crate) mod test { 10, 10, 1004, + false, ), ]; diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 7fc83ad5d0..f6ded221cd 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -581,7 +581,9 @@ CREATE TABLE IF NOT EXISTS omicron.public.region ( blocks_per_extent INT NOT NULL, extent_count INT NOT NULL, - port INT4 + port INT4, + + read_only BOOL NOT NULL ); /* @@ -4145,7 +4147,7 @@ INSERT INTO omicron.public.db_metadata ( version, target_version ) VALUES - (TRUE, NOW(), NOW(), '83.0.0', NULL) + (TRUE, NOW(), NOW(), '84.0.0', NULL) ON CONFLICT DO NOTHING; COMMIT; diff --git a/schema/crdb/region-read-only/up01.sql b/schema/crdb/region-read-only/up01.sql new file mode 100644 index 0000000000..f69f585562 --- /dev/null +++ b/schema/crdb/region-read-only/up01.sql @@ -0,0 +1,4 @@ +ALTER TABLE omicron.public.region + ADD COLUMN IF NOT EXISTS read_only BOOLEAN NOT NULL + DEFAULT false; + diff --git a/schema/crdb/region-read-only/up02.sql b/schema/crdb/region-read-only/up02.sql new file mode 100644 index 0000000000..b8c0aff92a --- /dev/null +++ b/schema/crdb/region-read-only/up02.sql @@ -0,0 +1,2 @@ +ALTER TABLE omicron.public.region + ALTER COLUMN read_only DROP DEFAULT; From 24f7cc0144a9c57875b93dfcdd85ac7a2281039e Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Mon, 29 Jul 2024 10:59:41 -0700 Subject: [PATCH 3/4] Add faux-mgs binary to the switch zone (#6164) Closes #6153 --- Cargo.lock | 52 +++++++++++++++--------------- Cargo.toml | 12 +++++-- package-manifest.toml | 17 ++++++++++ package/src/bin/omicron-package.rs | 13 ++++---- workspace-hack/Cargo.toml | 8 ++--- 5 files changed, 64 insertions(+), 38 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 41c3bef36f..75952fd71c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -504,7 +504,7 @@ version = "0.69.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a00dc851838a2120612785d195287475a3ac45514741da670b735818822129a0" dependencies = [ - "bitflags 2.5.0", + "bitflags 2.6.0", "cexpr", "clang-sys", "itertools 0.12.1", @@ -561,9 +561,9 @@ checksum = "bef38d45163c2f1dde094a7dfd33ccf595c92905c8f8f4fdc18d06fb1037718a" [[package]] name = "bitflags" -version = "2.5.0" +version = "2.6.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "cf4b9d6a944f767f8e5e0db018570623c85f3d925ac718db4e06d0187adb21c1" +checksum = "b048fb63fd8b5923fc5aa7b340d8e156aec7ec02f0c78fa8a6ddc2613f6f71de" dependencies = [ "serde", ] @@ -1410,7 +1410,7 @@ version = "0.27.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f476fe445d41c9e991fd07515a6f463074b782242ccf4a5b7b1d1012e70824df" dependencies = [ - "bitflags 2.5.0", + "bitflags 2.6.0", "crossterm_winapi", "futures-core", "libc", @@ -1817,7 +1817,7 @@ version = "2.1.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ff236accb9a5069572099f0b350a92e9560e8e63a9b8d546162f4a5e03026bb2" dependencies = [ - "bitflags 2.5.0", + "bitflags 2.6.0", "byteorder", "chrono", "diesel_derives", @@ -2755,9 +2755,9 @@ dependencies = [ [[package]] name = "gateway-messages" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/management-gateway-service?rev=c85a4ca043aaa389df12aac5348d8a3feda28762#c85a4ca043aaa389df12aac5348d8a3feda28762" +source = "git+https://github.com/oxidecomputer/management-gateway-service?rev=319e7b92db69792ab8efa4c68554ad0cf83adf93#319e7b92db69792ab8efa4c68554ad0cf83adf93" dependencies = [ - "bitflags 2.5.0", + "bitflags 2.6.0", "hubpack", "serde", "serde_repr", @@ -2771,7 +2771,7 @@ dependencies = [ [[package]] name = "gateway-sp-comms" version = "0.1.1" -source = "git+https://github.com/oxidecomputer/management-gateway-service?rev=c85a4ca043aaa389df12aac5348d8a3feda28762#c85a4ca043aaa389df12aac5348d8a3feda28762" +source = "git+https://github.com/oxidecomputer/management-gateway-service?rev=319e7b92db69792ab8efa4c68554ad0cf83adf93#319e7b92db69792ab8efa4c68554ad0cf83adf93" dependencies = [ "async-trait", "backoff", @@ -4146,7 +4146,7 @@ version = "0.1.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c0ff37bd590ca25063e35af745c343cb7a0271906fb7b37e4813e8f79f00268d" dependencies = [ - "bitflags 2.5.0", + "bitflags 2.6.0", "libc", ] @@ -5124,7 +5124,7 @@ version = "0.27.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2eb04e9c688eff1c89d72b407f168cf79bb9e867a9d3323ed6c01519eb9cc053" dependencies = [ - "bitflags 2.5.0", + "bitflags 2.6.0", "cfg-if", "libc", "memoffset", @@ -5136,7 +5136,7 @@ version = "0.28.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ab2156c4fce2f8df6c499cc1c763e4394b7482525bf2a9701c9d79d215f519e4" dependencies = [ - "bitflags 2.5.0", + "bitflags 2.6.0", "cfg-if", "cfg_aliases", "libc", @@ -6029,7 +6029,7 @@ dependencies = [ "bit-set", "bit-vec", "bitflags 1.3.2", - "bitflags 2.5.0", + "bitflags 2.6.0", "bstr 0.2.17", "bstr 1.9.1", "byteorder", @@ -6244,7 +6244,7 @@ version = "0.10.66" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9529f4786b70a3e8c61e11179af17ab6188ad8d0ded78c5529441ed39d4bd9c1" dependencies = [ - "bitflags 2.5.0", + "bitflags 2.6.0", "cfg-if", "foreign-types 0.3.2", "libc", @@ -7418,7 +7418,7 @@ source = "git+https://github.com/oxidecomputer/propolis?rev=6dceb9ef69c217cb78a2 dependencies = [ "anyhow", "bhyve_api 0.0.0 (git+https://github.com/oxidecomputer/propolis?rev=6dceb9ef69c217cb78a2018bbedafbc19f6ec1af)", - "bitflags 2.5.0", + "bitflags 2.6.0", "bitstruct", "byteorder", "dladm", @@ -7551,7 +7551,7 @@ checksum = "31b476131c3c86cb68032fdc5cb6d5a1045e3e42d96b69fa599fd77701e1f5bf" dependencies = [ "bit-set", "bit-vec", - "bitflags 2.5.0", + "bitflags 2.6.0", "lazy_static", "num-traits", "rand 0.8.5", @@ -7736,7 +7736,7 @@ version = "0.27.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d16546c5b5962abf8ce6e2881e722b4e0ae3b6f1a08a26ae3573c55853ca68d3" dependencies = [ - "bitflags 2.5.0", + "bitflags 2.6.0", "cassowary", "compact_str", "crossterm", @@ -7849,7 +7849,7 @@ version = "0.5.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "469052894dcb553421e483e4209ee581a45100d31b4018de03e5a7ad86374a7e" dependencies = [ - "bitflags 2.5.0", + "bitflags 2.6.0", ] [[package]] @@ -8091,7 +8091,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b91f7eff05f748767f183df4320a63d6936e9c6107d97c9e6bdd9784f4289c94" dependencies = [ "base64 0.21.7", - "bitflags 2.5.0", + "bitflags 2.6.0", "serde", "serde_derive", ] @@ -8177,7 +8177,7 @@ dependencies = [ "aes", "aes-gcm", "async-trait", - "bitflags 2.5.0", + "bitflags 2.6.0", "byteorder", "chacha20", "ctr", @@ -8312,7 +8312,7 @@ version = "0.38.34" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "70dc5ec042f7a43c4a73241207cecc9873a06d45debb38b329f8541d85c2730f" dependencies = [ - "bitflags 2.5.0", + "bitflags 2.6.0", "errno", "libc", "linux-raw-sys", @@ -8446,7 +8446,7 @@ version = "14.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7803e8936da37efd9b6d4478277f4b2b9bb5cdb37a113e8d63222e58da647e63" dependencies = [ - "bitflags 2.5.0", + "bitflags 2.6.0", "cfg-if", "clipboard-win", "fd-lock", @@ -8614,7 +8614,7 @@ version = "2.11.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c627723fd09706bacdb5cf41499e95098555af3c3c29d014dc3c458ef6be11c0" dependencies = [ - "bitflags 2.5.0", + "bitflags 2.6.0", "core-foundation", "core-foundation-sys", "libc", @@ -9827,18 +9827,18 @@ dependencies = [ [[package]] name = "thiserror" -version = "1.0.62" +version = "1.0.63" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f2675633b1499176c2dff06b0856a27976a8f9d436737b4cf4f312d4d91d8bbb" +checksum = "c0342370b38b6a11b6cc11d6a805569958d54cfa061a29969c3b5ce2ea405724" dependencies = [ "thiserror-impl", ] [[package]] name = "thiserror-impl" -version = "1.0.62" +version = "1.0.63" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d20468752b09f49e909e55a5d338caa8bedf615594e9d80bc4c565d30faf798c" +checksum = "a4558b58466b9ad7ca0f102865eccc95938dca1a74a856f2b57b6629050da261" dependencies = [ "proc-macro2", "quote", diff --git a/Cargo.toml b/Cargo.toml index 578c4de5a1..12bea694f6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -321,8 +321,16 @@ fs-err = "2.11.0" futures = "0.3.30" gateway-api = { path = "gateway-api" } gateway-client = { path = "clients/gateway-client" } -gateway-messages = { git = "https://github.com/oxidecomputer/management-gateway-service", rev = "c85a4ca043aaa389df12aac5348d8a3feda28762", default-features = false, features = ["std"] } -gateway-sp-comms = { git = "https://github.com/oxidecomputer/management-gateway-service", rev = "c85a4ca043aaa389df12aac5348d8a3feda28762" } +# If you're updating the pinned revision of these MGS dependencies, you should +# also update the git commit revision for the `omicron-faux-mgs` package in +# `package-manifest.toml`. Failure to do so won't cause incorrect behavior, but +# does mean the `faux-mgs` shipped with the switch zone would be out of date +# relative to the MGS proper shipped in that same switch zone. (Generally this +# is "fine", because SP/MGS communication maintains forwards and backwards +# compatibility, but will mean that faux-mgs might be missing new +# functionality.) +gateway-messages = { git = "https://github.com/oxidecomputer/management-gateway-service", rev = "319e7b92db69792ab8efa4c68554ad0cf83adf93", default-features = false, features = ["std"] } +gateway-sp-comms = { git = "https://github.com/oxidecomputer/management-gateway-service", rev = "319e7b92db69792ab8efa4c68554ad0cf83adf93" } gateway-test-utils = { path = "gateway-test-utils" } gateway-types = { path = "gateway-types" } gethostname = "0.4.3" diff --git a/package-manifest.toml b/package-manifest.toml index 098e15d3b8..ffe571121c 100644 --- a/package-manifest.toml +++ b/package-manifest.toml @@ -489,6 +489,22 @@ output.type = "zone" # Packages not built within Omicron, but which must be imported. +[package.omicron-faux-mgs] +# This package bundles a `faux-mgs` binary into `/usr/bin` in the switch zone, +# allowing `pilot sp ...` to work without needing to manually scp a `faux-mgs` +# binary in during support operations. (On rare occasions a support operator may +# still need to do that to get a more recent faux-mgs.) +service_name = "faux_mgs" +only_for_targets.image = "standard" +source.type = "prebuilt" +source.repo = "management-gateway-service" +# In general, this commit should match the pinned revision of `gateway-sp-comms` +# in `Cargo.toml`. +source.commit = "319e7b92db69792ab8efa4c68554ad0cf83adf93" +source.sha256 = "f4cbc480c8cfc2605c13b319291e69cbf8c213bb9c625ff79d339f90a7124358" +output.type = "zone" +output.intermediate_only = true + # Refer to # https://github.com/oxidecomputer/crucible/blob/main/package/README.md # for instructions on building this manually. @@ -728,6 +744,7 @@ only_for_targets.switch = "asic" only_for_targets.image = "standard" source.type = "composite" source.packages = [ + "omicron-faux-mgs.tar.gz", "omicron-gateway-asic.tar.gz", "dendrite-asic.tar.gz", "lldp.tar.gz", diff --git a/package/src/bin/omicron-package.rs b/package/src/bin/omicron-package.rs index 6db168c9f8..b2b8703015 100644 --- a/package/src/bin/omicron-package.rs +++ b/package/src/bin/omicron-package.rs @@ -430,14 +430,15 @@ async fn download_prebuilt( } let digest = context.finish(); - if digest.as_ref() != expected_digest { - bail!( - "Digest mismatch downloading {package_name}: Saw {}, expected {}", + if digest.as_ref() == expected_digest { + Ok(()) + } else { + Err(anyhow!("Failed validating download of {url}").context(format!( + "Digest mismatch on {package_name}: Saw {}, expected {}", hex::encode(digest.as_ref()), hex::encode(expected_digest) - ); + ))) } - Ok(()) } // Ensures a package exists, either by creating it or downloading it. @@ -484,7 +485,7 @@ async fn ensure_package( let msg = format!("Failed to download prebuilt ({attempts_left} attempts remaining)"); progress.set_error_message(msg.into()); if attempts_left == 0 { - bail!("Failed to download package: {err}"); + return Err(err); } tokio::time::sleep(config.retry_duration).await; progress.reset(); diff --git a/workspace-hack/Cargo.toml b/workspace-hack/Cargo.toml index dbb4a51c43..94b169e26c 100644 --- a/workspace-hack/Cargo.toml +++ b/workspace-hack/Cargo.toml @@ -24,7 +24,7 @@ base16ct = { version = "0.2.0", default-features = false, features = ["alloc"] } bit-set = { version = "0.5.3" } bit-vec = { version = "0.6.3" } bitflags-dff4ba8e3ae991db = { package = "bitflags", version = "1.3.2" } -bitflags-f595c2ba2a3f28df = { package = "bitflags", version = "2.5.0", default-features = false, features = ["serde", "std"] } +bitflags-f595c2ba2a3f28df = { package = "bitflags", version = "2.6.0", default-features = false, features = ["serde", "std"] } bstr-6f8ce4dd05d13bba = { package = "bstr", version = "0.2.17" } bstr-dff4ba8e3ae991db = { package = "bstr", version = "1.9.1" } byteorder = { version = "1.5.0" } @@ -54,7 +54,7 @@ futures-io = { version = "0.3.30", default-features = false, features = ["std"] futures-sink = { version = "0.3.30" } futures-task = { version = "0.3.30", default-features = false, features = ["std"] } futures-util = { version = "0.3.30", features = ["channel", "io", "sink"] } -gateway-messages = { git = "https://github.com/oxidecomputer/management-gateway-service", rev = "c85a4ca043aaa389df12aac5348d8a3feda28762", features = ["std"] } +gateway-messages = { git = "https://github.com/oxidecomputer/management-gateway-service", rev = "319e7b92db69792ab8efa4c68554ad0cf83adf93", features = ["std"] } generic-array = { version = "0.14.7", default-features = false, features = ["more_lengths", "zeroize"] } getrandom = { version = "0.2.14", default-features = false, features = ["js", "rdrand", "std"] } group = { version = "0.13.0", default-features = false, features = ["alloc"] } @@ -129,7 +129,7 @@ base16ct = { version = "0.2.0", default-features = false, features = ["alloc"] } bit-set = { version = "0.5.3" } bit-vec = { version = "0.6.3" } bitflags-dff4ba8e3ae991db = { package = "bitflags", version = "1.3.2" } -bitflags-f595c2ba2a3f28df = { package = "bitflags", version = "2.5.0", default-features = false, features = ["serde", "std"] } +bitflags-f595c2ba2a3f28df = { package = "bitflags", version = "2.6.0", default-features = false, features = ["serde", "std"] } bstr-6f8ce4dd05d13bba = { package = "bstr", version = "0.2.17" } bstr-dff4ba8e3ae991db = { package = "bstr", version = "1.9.1" } byteorder = { version = "1.5.0" } @@ -159,7 +159,7 @@ futures-io = { version = "0.3.30", default-features = false, features = ["std"] futures-sink = { version = "0.3.30" } futures-task = { version = "0.3.30", default-features = false, features = ["std"] } futures-util = { version = "0.3.30", features = ["channel", "io", "sink"] } -gateway-messages = { git = "https://github.com/oxidecomputer/management-gateway-service", rev = "c85a4ca043aaa389df12aac5348d8a3feda28762", features = ["std"] } +gateway-messages = { git = "https://github.com/oxidecomputer/management-gateway-service", rev = "319e7b92db69792ab8efa4c68554ad0cf83adf93", features = ["std"] } generic-array = { version = "0.14.7", default-features = false, features = ["more_lengths", "zeroize"] } getrandom = { version = "0.2.14", default-features = false, features = ["js", "rdrand", "std"] } group = { version = "0.13.0", default-features = false, features = ["alloc"] } From 945e75fc724737b06fa541715a3d333e185cfb38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Karen=20C=C3=A1rcamo?= Date: Tue, 30 Jul 2024 09:05:51 +1200 Subject: [PATCH 4/4] [sled-agent] Rename switch zone functions to be more explicit (#6163) When working on the switch zone set up PRs, I was confused by some of these function names thinking they may apply to other zones apart from the switch zone. This very small PR changes a few names to help with readability of the code. --- sled-agent/src/services.rs | 108 +++++++++++++++++++------------------ 1 file changed, 57 insertions(+), 51 deletions(-) diff --git a/sled-agent/src/services.rs b/sled-agent/src/services.rs index b62350ca93..42466ce094 100644 --- a/sled-agent/src/services.rs +++ b/sled-agent/src/services.rs @@ -121,6 +121,9 @@ use illumos_utils::zone::MockZones as Zones; use illumos_utils::zone::Zones; const IPV6_UNSPECIFIED: IpAddr = IpAddr::V6(Ipv6Addr::UNSPECIFIED); + +const COCKROACH: &str = "/opt/oxide/cockroachdb/bin/cockroach"; + pub const SWITCH_ZONE_BASEBOARD_FILE: &str = "/opt/oxide/baseboard.json"; #[derive(thiserror::Error, Debug, slog_error_chain::SlogInlineError)] @@ -153,8 +156,8 @@ pub enum Error { #[error("No U.2 devices found with a {ZONE_DATASET} mountpoint")] U2NotFound, - #[error("Sled-local zone error: {0}")] - SledLocalZone(anyhow::Error), + #[error("Switch zone error: {0}")] + SwitchZone(anyhow::Error), #[error("Failed to issue SMF command: {0}")] SmfCommand(#[from] illumos_utils::smf_helper::Error), @@ -206,8 +209,8 @@ pub enum Error { #[error("Error contacting dpd: {0}")] DpdError(#[from] DpdError), - #[error("Failed to create Vnic in sled-local zone: {0}")] - SledLocalVnicCreation(illumos_utils::dladm::CreateVnicError), + #[error("Failed to create Vnic in the switch zone: {0}")] + SwitchZoneVnicCreation(illumos_utils::dladm::CreateVnicError), #[error("Failed to add GZ addresses: {message}: {err}")] GzAddress { @@ -544,9 +547,9 @@ impl<'a> ZoneArgs<'a> { } } - /// If this is a sled-local (switch) zone, iterate over the services it's + /// If this is a switch zone, iterate over the services it's /// supposed to be running - pub fn sled_local_services( + pub fn switch_zone_services( &self, ) -> Box + 'a> { match self { @@ -586,8 +589,8 @@ impl Task { } } -/// Describes the state of a sled-local zone. -enum SledLocalZone { +/// Describes the state of a switch zone. +enum SwitchZoneState { // The zone is not currently running. Disabled, // The zone is still initializing - it may be awaiting the initialization @@ -645,7 +648,7 @@ type ZoneMap = BTreeMap; pub struct ServiceManagerInner { log: Logger, global_zone_bootstrap_link_local_address: Ipv6Addr, - switch_zone: Mutex, + switch_zone: Mutex, sled_mode: SledMode, time_sync_config: TimeSyncConfig, time_synced: AtomicBool, @@ -727,7 +730,7 @@ impl ServiceManager { .global_zone_bootstrap_link_local_ip, // TODO(https://github.com/oxidecomputer/omicron/issues/725): // Load the switch zone if it already exists? - switch_zone: Mutex::new(SledLocalZone::Disabled), + switch_zone: Mutex::new(SwitchZoneState::Disabled), sled_mode, time_sync_config, time_synced: AtomicBool::new(false), @@ -943,7 +946,7 @@ impl ServiceManager { // physical devices need to be mapped into the zone when it is created. fn devices_needed(zone_args: &ZoneArgs<'_>) -> Result, Error> { let mut devices = vec![]; - for svc_details in zone_args.sled_local_services() { + for svc_details in zone_args.switch_zone_services() { match svc_details { SwitchService::Dendrite { asic: DendriteAsic::TofinoAsic, @@ -991,7 +994,7 @@ impl ServiceManager { .inner .bootstrap_vnic_allocator .new_bootstrap() - .map_err(Error::SledLocalVnicCreation)?; + .map_err(Error::SwitchZoneVnicCreation)?; Ok(Some((link, self.inner.switch_zone_bootstrap_address))) } else { Ok(None) @@ -1027,7 +1030,7 @@ impl ServiceManager { Error::Underlay(underlay::Error::SystemDetection(e)) })?; - for svc_details in zone_args.sled_local_services() { + for svc_details in zone_args.switch_zone_services() { match &svc_details { SwitchService::Tfport { pkt_source, asic: _ } => { // The tfport service requires a MAC device to/from which @@ -1251,7 +1254,7 @@ impl ServiceManager { "dtrace_user".to_string(), "dtrace_proc".to_string(), ]; - for svc_details in zone_args.sled_local_services() { + for svc_details in zone_args.switch_zone_services() { match svc_details { SwitchService::Tfport { .. } => { needed.push("sys_dl_config".to_string()); @@ -2526,7 +2529,7 @@ impl ServiceManager { &device_names[0].clone(), ); } else { - return Err(Error::SledLocalZone( + return Err(Error::SwitchZone( anyhow::anyhow!( "{dev_cnt} devices needed \ for tofino asic" @@ -3073,7 +3076,7 @@ impl ServiceManager { name: &str, ) -> Result { // Search for the named zone. - if let SledLocalZone::Running { zone, .. } = + if let SwitchZoneState::Running { zone, .. } = &*self.inner.switch_zone.lock().await { if zone.name() == name { @@ -3483,7 +3486,7 @@ impl ServiceManager { "Initializing CRDB Cluster - sending request to {host}" ); if let Err(err) = zone.runtime.run_cmd(&[ - "/opt/oxide/cockroachdb/bin/cockroach", + COCKROACH, "init", "--insecure", "--host", @@ -3499,7 +3502,7 @@ impl ServiceManager { info!(log, "Formatting CRDB"); zone.runtime .run_cmd(&[ - "/opt/oxide/cockroachdb/bin/cockroach", + COCKROACH, "sql", "--insecure", "--host", @@ -3510,7 +3513,7 @@ impl ServiceManager { .map_err(|err| Error::CockroachInit { err })?; zone.runtime .run_cmd(&[ - "/opt/oxide/cockroachdb/bin/cockroach", + COCKROACH, "sql", "--insecure", "--host", @@ -3666,7 +3669,7 @@ impl ServiceManager { // A pure gimlet sled should not be trying to activate a switch // zone. SledMode::Gimlet => { - return Err(Error::SledLocalZone(anyhow::anyhow!( + return Err(Error::SwitchZone(anyhow::anyhow!( "attempted to activate switch zone on non-scrimlet sled" ))) } @@ -3749,7 +3752,7 @@ impl ServiceManager { let request = SwitchZoneConfig { id: Uuid::new_v4(), addresses, services }; - self.ensure_zone( + self.ensure_switch_zone( // request= Some(request), // filesystems= @@ -3802,15 +3805,15 @@ impl ServiceManager { let mut switch_zone = self.inner.switch_zone.lock().await; let zone = match &mut *switch_zone { - SledLocalZone::Running { zone, .. } => zone, - SledLocalZone::Disabled => { - return Err(Error::SledLocalZone(anyhow!( + SwitchZoneState::Running { zone, .. } => zone, + SwitchZoneState::Disabled => { + return Err(Error::SwitchZone(anyhow!( "Cannot configure switch zone uplinks: \ switch zone disabled" ))); } - SledLocalZone::Initializing { .. } => { - return Err(Error::SledLocalZone(anyhow!( + SwitchZoneState::Initializing { .. } => { + return Err(Error::SwitchZone(anyhow!( "Cannot configure switch zone uplinks: \ switch zone still initializing" ))); @@ -3843,7 +3846,7 @@ impl ServiceManager { /// Ensures that no switch zone is active. pub async fn deactivate_switch(&self) -> Result<(), Error> { - self.ensure_zone( + self.ensure_switch_zone( // request= None, // filesystems= @@ -3854,32 +3857,32 @@ impl ServiceManager { .await } - // Forcefully initialize a sled-local zone. + // Forcefully initialize a sled-local switch zone. // - // This is a helper function for "ensure_zone". - fn start_zone( + // This is a helper function for "ensure_switch_zone". + fn start_switch_zone( self, - zone: &mut SledLocalZone, + zone: &mut SwitchZoneState, request: SwitchZoneConfig, filesystems: Vec, data_links: Vec, ) { let (exit_tx, exit_rx) = oneshot::channel(); - *zone = SledLocalZone::Initializing { + *zone = SwitchZoneState::Initializing { request, filesystems, data_links, worker: Some(Task { exit_tx, initializer: tokio::task::spawn(async move { - self.initialize_zone_loop(exit_rx).await + self.initialize_switch_zone_loop(exit_rx).await }), }), }; } // Moves the current state to align with the "request". - async fn ensure_zone( + async fn ensure_switch_zone( &self, request: Option, filesystems: Vec, @@ -3891,9 +3894,9 @@ impl ServiceManager { let zone_typestr = "switch"; match (&mut *sled_zone, request) { - (SledLocalZone::Disabled, Some(request)) => { + (SwitchZoneState::Disabled, Some(request)) => { info!(log, "Enabling {zone_typestr} zone (new)"); - self.clone().start_zone( + self.clone().start_switch_zone( &mut sled_zone, request, filesystems, @@ -3901,7 +3904,7 @@ impl ServiceManager { ); } ( - SledLocalZone::Initializing { request, .. }, + SwitchZoneState::Initializing { request, .. }, Some(new_request), ) => { info!(log, "Enabling {zone_typestr} zone (already underway)"); @@ -3909,7 +3912,7 @@ impl ServiceManager { // the next request with our new request. *request = new_request; } - (SledLocalZone::Running { request, zone }, Some(new_request)) + (SwitchZoneState::Running { request, zone }, Some(new_request)) if request.addresses != new_request.addresses => { // If the switch zone is running but we have new addresses, it @@ -4221,31 +4224,31 @@ impl ServiceManager { } } } - (SledLocalZone::Running { .. }, Some(_)) => { + (SwitchZoneState::Running { .. }, Some(_)) => { info!(log, "Enabling {zone_typestr} zone (already complete)"); } - (SledLocalZone::Disabled, None) => { + (SwitchZoneState::Disabled, None) => { info!(log, "Disabling {zone_typestr} zone (already complete)"); } - (SledLocalZone::Initializing { worker, .. }, None) => { + (SwitchZoneState::Initializing { worker, .. }, None) => { info!(log, "Disabling {zone_typestr} zone (was initializing)"); worker.take().unwrap().stop().await; - *sled_zone = SledLocalZone::Disabled; + *sled_zone = SwitchZoneState::Disabled; } - (SledLocalZone::Running { zone, .. }, None) => { + (SwitchZoneState::Running { zone, .. }, None) => { info!(log, "Disabling {zone_typestr} zone (was running)"); let _ = zone.stop().await; - *sled_zone = SledLocalZone::Disabled; + *sled_zone = SwitchZoneState::Disabled; } } Ok(()) } - async fn try_initialize_sled_local_zone( + async fn try_initialize_switch_zone( &self, - sled_zone: &mut SledLocalZone, + sled_zone: &mut SwitchZoneState, ) -> Result<(), Error> { - let SledLocalZone::Initializing { + let SwitchZoneState::Initializing { request, filesystems, data_links, @@ -4269,18 +4272,21 @@ impl ServiceManager { let zone = self .initialize_zone(zone_args, filesystems, data_links, None) .await?; - *sled_zone = SledLocalZone::Running { request: request.clone(), zone }; + *sled_zone = + SwitchZoneState::Running { request: request.clone(), zone }; Ok(()) } // Body of a tokio task responsible for running until the switch zone is // inititalized, or it has been told to stop. - async fn initialize_zone_loop(&self, mut exit_rx: oneshot::Receiver<()>) { + async fn initialize_switch_zone_loop( + &self, + mut exit_rx: oneshot::Receiver<()>, + ) { loop { { let mut sled_zone = self.inner.switch_zone.lock().await; - match self.try_initialize_sled_local_zone(&mut sled_zone).await - { + match self.try_initialize_switch_zone(&mut sled_zone).await { Ok(()) => return, Err(e) => warn!( self.inner.log,