From f30f9a6a448ebc8099f047394a6801e535b68604 Mon Sep 17 00:00:00 2001 From: John Baublitz Date: Wed, 15 Mar 2023 16:16:00 -0400 Subject: [PATCH 1/4] Add rollback of Pool setup when starting a pool fails --- src/dbus_api/api/prop_conv.rs | 1 + src/engine/sim_engine/engine.rs | 3 +- .../strat_engine/backstore/backstore.rs | 23 +++- .../strat_engine/backstore/blockdevmgr.rs | 32 +++-- .../backstore/crypt/initialize.rs | 2 +- src/engine/strat_engine/dm.rs | 101 +++++++++++++++- src/engine/strat_engine/engine.rs | 45 ++++---- .../strat_engine/liminal/device_info.rs | 14 +++ src/engine/strat_engine/liminal/identify.rs | 45 +++++--- src/engine/strat_engine/liminal/liminal.rs | 109 ++++++++++++++---- src/engine/strat_engine/names.rs | 7 +- src/engine/strat_engine/pool.rs | 36 +++--- src/engine/strat_engine/thinpool/thinpool.rs | 17 ++- src/engine/types/mod.rs | 1 + src/jsonrpc/server/pool.rs | 21 ++-- 15 files changed, 340 insertions(+), 117 deletions(-) diff --git a/src/dbus_api/api/prop_conv.rs b/src/dbus_api/api/prop_conv.rs index 3d2332ede7..e53051fac1 100644 --- a/src/dbus_api/api/prop_conv.rs +++ b/src/dbus_api/api/prop_conv.rs @@ -70,6 +70,7 @@ pub fn stopped_pools_to_prop(pools: &StoppedPoolsInfo) -> StoppedOrLockedPools { pools .stopped .iter() + .chain(pools.partially_constructed.iter()) .map(|(u, stopped)| { let mut map = HashMap::new(); if let Some(enc_info) = stopped.info.as_ref() { diff --git a/src/engine/sim_engine/engine.rs b/src/engine/sim_engine/engine.rs index 5c84b56cf5..4426d2c201 100644 --- a/src/engine/sim_engine/engine.rs +++ b/src/engine/sim_engine/engine.rs @@ -84,8 +84,9 @@ impl<'a> Into for &'a SimEngine { unreachable!("json!() output is always JSON object"); } }) - .collect() + .collect(), ), + "partially_constructed_pools": Value::Array(Vec::new()) }) } } diff --git a/src/engine/strat_engine/backstore/backstore.rs b/src/engine/strat_engine/backstore/backstore.rs index d0cff6c763..6e873499a0 100644 --- a/src/engine/strat_engine/backstore/backstore.rs +++ b/src/engine/strat_engine/backstore/backstore.rs @@ -569,25 +569,24 @@ impl Backstore { } /// Teardown the DM devices in the backstore. - pub fn teardown(&mut self) -> StratisResult> { - let mut devs = match self + pub fn teardown(&mut self) -> StratisResult<()> { + match self .cache .as_mut() .and_then(|c| self.cache_tier.as_mut().map(|ct| (c, ct))) { Some((cache, cache_tier)) => { cache.teardown(get_dm())?; - cache_tier.block_mgr.teardown()? + cache_tier.block_mgr.teardown()?; } None => { if let Some(ref mut linear) = self.linear { linear.teardown(get_dm())?; } - Vec::new() } }; - devs.extend(self.data_tier.block_mgr.teardown()?); - Ok(devs) + self.data_tier.block_mgr.teardown()?; + Ok(()) } /// Consume the backstore and convert it into a set of BDAs representing @@ -605,6 +604,18 @@ impl Backstore { .collect::>() } + /// Drain the backstore devices into a set of all data and cache devices. + pub fn drain_bds(&mut self) -> Vec { + let mut bds = self.data_tier.block_mgr.drain_bds(); + bds.extend( + self.cache_tier + .as_mut() + .map(|ct| ct.block_mgr.drain_bds()) + .unwrap_or_default(), + ); + bds + } + /// Return the device that this tier is currently using. /// This changes, depending on whether the backstore is supporting a cache /// or not. There may be no device if no data has yet been allocated from diff --git a/src/engine/strat_engine/backstore/blockdevmgr.rs b/src/engine/strat_engine/backstore/blockdevmgr.rs index c2b48a6bc0..3146e37bbe 100644 --- a/src/engine/strat_engine/backstore/blockdevmgr.rs +++ b/src/engine/strat_engine/backstore/blockdevmgr.rs @@ -81,6 +81,11 @@ impl BlockDevMgr { bds_to_bdas(self.block_devs) } + /// Drain the BlockDevMgr block devices into a collection of block devices. + pub fn drain_bds(&mut self) -> Vec { + self.block_devs.drain(..).collect::>() + } + /// Get a hashmap that maps UUIDs to Devices. pub fn uuid_to_devno(&self) -> HashMap { self.block_devs @@ -399,27 +404,16 @@ impl BlockDevMgr { } /// Tear down devicemapper devices for the block devices in this BlockDevMgr. - pub fn teardown(&mut self) -> StratisResult> { - let (oks, errs) = self - .block_devs - .drain(..) - .map(|mut bd| { - bd.teardown()?; - Ok(bd) - }) - .partition::, _>(|res| res.is_ok()); - let (oks, errs) = ( - oks.into_iter().filter_map(|ok| ok.ok()).collect::>(), - errs.into_iter() - .filter_map(|err| match err { - Ok(_) => None, - Err(e) => Some(e), - }) - .collect::>(), - ); + pub fn teardown(&mut self) -> StratisResult<()> { + let errs = self.block_devs.iter_mut().fold(Vec::new(), |mut errs, bd| { + if let Err(e) = bd.teardown() { + errs.push(e); + } + errs + }); if errs.is_empty() { - Ok(oks) + Ok(()) } else { Err(StratisError::BestEffortError("Failed to remove devicemapper devices for some or all physical devices in the pool".to_string(), errs)) } diff --git a/src/engine/strat_engine/backstore/crypt/initialize.rs b/src/engine/strat_engine/backstore/crypt/initialize.rs index 138c8fe525..2808ab1526 100644 --- a/src/engine/strat_engine/backstore/crypt/initialize.rs +++ b/src/engine/strat_engine/backstore/crypt/initialize.rs @@ -54,7 +54,7 @@ impl CryptInitializer { ) -> CryptInitializer { CryptInitializer { physical_path, - activation_name: format_crypt_name(&dev_uuid), + activation_name: format_crypt_name(&dev_uuid).to_string(), identifiers: StratisIdentifiers::new(pool_uuid, dev_uuid), } } diff --git a/src/engine/strat_engine/dm.rs b/src/engine/strat_engine/dm.rs index 0c7135e627..6358db1064 100644 --- a/src/engine/strat_engine/dm.rs +++ b/src/engine/strat_engine/dm.rs @@ -4,11 +4,23 @@ // Get ability to instantiate a devicemapper context. -use std::sync::Once; +use std::{path::Path, sync::Once}; -use devicemapper::{DmResult, DM}; +use devicemapper::{DevId, DmNameBuf, DmOptions, DmResult, DM}; -use crate::stratis::{StratisError, StratisResult}; +use crate::{ + engine::{ + strat_engine::{ + liminal::DeviceSet, + names::{ + format_backstore_ids, format_crypt_name, format_flex_ids, format_thinpool_ids, + CacheRole, FlexRole, ThinPoolRole, + }, + }, + types::{DevUuid, PoolUuid}, + }, + stratis::{StratisError, StratisResult}, +}; static INIT: Once = Once::new(); static mut DM_CONTEXT: Option> = None; @@ -35,3 +47,86 @@ pub fn get_dm() -> &'static DM { "the engine has already called get_dm_init() and exited if get_dm_init() returned an error", ) } + +pub fn stop_partially_constructed_pool( + pool_uuid: PoolUuid, + device_set: &DeviceSet, +) -> StratisResult { + let mut did_something = false; + let devices = get_dm() + .list_devices()? + .into_iter() + .map(|(name, _, _)| name) + .collect::>(); + let dev_uuids = device_set + .iter() + .map(|(dev_uuid, _)| *dev_uuid) + .collect::>(); + for device in list_of_devices(pool_uuid, &dev_uuids) { + if devices.contains(&device) { + did_something = true; + get_dm().device_remove(&DevId::Name(&device), DmOptions::default())?; + } + } + Ok(did_something) +} + +/// List of device names for removal on partially constructed pool stop. Does not have +/// filesystem names because partially constructed pools are guaranteed not to have any +/// active filesystems. +fn list_of_devices(pool_uuid: PoolUuid, dev_uuids: &[DevUuid]) -> Vec { + let mut devs = Vec::new(); + let (thin_pool, _) = format_thinpool_ids(pool_uuid, ThinPoolRole::Pool); + devs.push(thin_pool); + let (thin_data, _) = format_flex_ids(pool_uuid, FlexRole::ThinData); + devs.push(thin_data); + let (thin_meta, _) = format_flex_ids(pool_uuid, FlexRole::ThinMeta); + devs.push(thin_meta); + let (thin_meta_spare, _) = format_flex_ids(pool_uuid, FlexRole::ThinMetaSpare); + devs.push(thin_meta_spare); + let (thin_mdv, _) = format_flex_ids(pool_uuid, FlexRole::MetadataVolume); + devs.push(thin_mdv); + let (cache, _) = format_backstore_ids(pool_uuid, CacheRole::Cache); + devs.push(cache); + let (cache_sub, _) = format_backstore_ids(pool_uuid, CacheRole::CacheSub); + devs.push(cache_sub); + let (cache_meta, _) = format_backstore_ids(pool_uuid, CacheRole::MetaSub); + devs.push(cache_meta); + let (origin, _) = format_backstore_ids(pool_uuid, CacheRole::OriginSub); + devs.push(origin); + + for dev_uuid in dev_uuids.iter() { + let crypt = format_crypt_name(dev_uuid); + devs.push(crypt); + } + + devs +} + +/// Check whether there are any leftover devicemapper devices from the pool. +pub fn has_leftover_devices(pool_uuid: PoolUuid, dev_uuids: &[DevUuid]) -> bool { + let mut has_leftover = false; + let devices = list_of_devices(pool_uuid, dev_uuids); + match get_dm().list_devices() { + Ok(l) => { + let listed_devices = l + .into_iter() + .map(|(dm_name, _, _)| dm_name) + .collect::>(); + for device in devices { + if listed_devices.contains(&device) { + has_leftover |= true; + } + } + } + Err(_) => { + for device in devices { + if Path::new(&format!("/dev/mapper/{}", &*device)).exists() { + has_leftover |= true; + } + } + } + } + + has_leftover +} diff --git a/src/engine/strat_engine/engine.rs b/src/engine/strat_engine/engine.rs index 8de6a6fc65..859d306932 100644 --- a/src/engine/strat_engine/engine.rs +++ b/src/engine/strat_engine/engine.rs @@ -27,7 +27,7 @@ use crate::{ cmd::verify_executables, dm::get_dm, keys::StratKeyActions, - liminal::{find_all, LiminalDevices}, + liminal::{find_all, DeviceSet, LiminalDevices}, ns::MemoryFilesystem, pool::StratPool, }, @@ -453,10 +453,21 @@ impl Engine for StratEngine { .remove_by_uuid(uuid) .expect("Must succeed since self.pools.get_by_uuid() returned a value"); - let (res, pool) = spawn_blocking!((pool.destroy(), pool))?; - if let Err(err) = res { + let (res, mut pool) = spawn_blocking!((pool.destroy(), pool))?; + if let Err((err, true)) = res { guard.insert(pool_name, uuid, pool); Err(err) + } else if let Err((err, false)) = res { + // We use blkid to scan for existing devices with this pool UUID and device UUIDs + // because some of the block devices could have been destroyed above. Using the + // cached data structures alone could result in phantom devices that have already + // been destroyed but are still recorded in the stopped pool. + let device_set = DeviceSet::from(pool.drain_bds()); + self.liminal_devices + .write() + .await + .handle_stopped_pool(uuid, device_set); + Err(err) } else { Ok(DeleteAction::Deleted(uuid)) } @@ -638,30 +649,22 @@ impl Engine for StratEngine { async fn stop_pool(&self, pool_uuid: PoolUuid) -> StratisResult> { let mut pools = self.pools.write_all().await; - if let Some((name, mut pool)) = pools.remove_by_uuid(pool_uuid) { - if let Err(e) = self - .liminal_devices + if let Some((name, pool)) = pools.remove_by_uuid(pool_uuid) { + self.liminal_devices .write() .await - .stop_pool(&name, pool_uuid, &mut pool) - { - pools.insert(name, pool_uuid, pool); - return Err(e); - } else { - return Ok(StopAction::Stopped(pool_uuid)); - } + .stop_pool(&mut pools, name, pool_uuid, pool)?; + return Ok(StopAction::Stopped(pool_uuid)); } drop(pools); - if self - .liminal_devices - .read() - .await - .stopped_pools() - .stopped - .get(&pool_uuid) - .is_some() + let stopped_pools = self.liminal_devices.read().await.stopped_pools(); + if stopped_pools.stopped.get(&pool_uuid).is_some() + || stopped_pools + .partially_constructed + .get(&pool_uuid) + .is_some() { Ok(StopAction::Identity) } else { diff --git a/src/engine/strat_engine/liminal/device_info.rs b/src/engine/strat_engine/liminal/device_info.rs index ef73ddbc67..d8df7340c9 100644 --- a/src/engine/strat_engine/liminal/device_info.rs +++ b/src/engine/strat_engine/liminal/device_info.rs @@ -18,6 +18,7 @@ use crate::{ engine::{ shared::{gather_encryption_info, gather_pool_name}, strat_engine::{ + backstore::StratBlockDev, liminal::{ identify::{DeviceInfo, LuksInfo, StratisDevInfo, StratisInfo}, setup::get_name, @@ -501,6 +502,19 @@ impl FromIterator<(DevUuid, LInfo)> for DeviceSet { } } +impl From> for DeviceSet { + fn from(vec: Vec) -> Self { + vec.into_iter() + .flat_map(|bd| { + let dev_uuid = bd.uuid(); + Vec::::from(bd) + .into_iter() + .map(move |info| (dev_uuid, LInfo::from(info))) + }) + .collect::() + } +} + impl DeviceSet { /// Create a new, empty DeviceSet pub fn new() -> DeviceSet { diff --git a/src/engine/strat_engine/liminal/identify.rs b/src/engine/strat_engine/liminal/identify.rs index 5dc2532450..6eed0baba9 100644 --- a/src/engine/strat_engine/liminal/identify.rs +++ b/src/engine/strat_engine/liminal/identify.rs @@ -171,30 +171,45 @@ impl DeviceInfo { } } -impl From for DeviceInfo { +impl From for Vec { fn from(bd: StratBlockDev) -> Self { + let mut device_infos = Vec::new(); match (bd.encryption_info(), bd.pool_name(), bd.luks_device()) { - (Some(ei), Some(pname), Some(dev)) => DeviceInfo::Luks(LuksInfo { - encryption_info: ei.clone(), - dev_info: StratisDevInfo { - device_number: *dev, - devnode: bd.physical_path().to_owned(), - }, - identifiers: StratisIdentifiers { - pool_uuid: bd.pool_uuid(), - device_uuid: bd.uuid(), - }, - pool_name: pname.cloned(), - }), - (None, None, None) => DeviceInfo::Stratis(StratisInfo { + (Some(ei), Some(pname), Some(dev)) => { + if bd.physical_path().exists() { + device_infos.push(DeviceInfo::Luks(LuksInfo { + encryption_info: ei.clone(), + dev_info: StratisDevInfo { + device_number: *dev, + devnode: bd.physical_path().to_owned(), + }, + identifiers: StratisIdentifiers { + pool_uuid: bd.pool_uuid(), + device_uuid: bd.uuid(), + }, + pool_name: pname.cloned(), + })); + if bd.metadata_path().exists() { + device_infos.push(DeviceInfo::Stratis(StratisInfo { + dev_info: StratisDevInfo { + device_number: *bd.device(), + devnode: bd.metadata_path().to_owned(), + }, + bda: bd.bda, + })); + } + } + } + (None, None, None) => device_infos.push(DeviceInfo::Stratis(StratisInfo { dev_info: StratisDevInfo { device_number: *bd.device(), devnode: bd.physical_path().to_owned(), }, bda: bd.bda, - }), + })), (_, _, _) => unreachable!("If bd.is_encrypted(), all are Some(_)"), } + device_infos } } diff --git a/src/engine/strat_engine/liminal/liminal.rs b/src/engine/strat_engine/liminal/liminal.rs index 8b5b18e60a..7664cd5d59 100644 --- a/src/engine/strat_engine/liminal/liminal.rs +++ b/src/engine/strat_engine/liminal/liminal.rs @@ -20,6 +20,7 @@ use crate::{ backstore::{ find_stratis_devs_by_uuid, CryptActivationHandle, CryptHandle, StratBlockDev, }, + dm::{has_leftover_devices, stop_partially_constructed_pool}, liminal::{ device_info::{ reconstruct_stratis_infos, split_stratis_infos, stratis_infos_ref, DeviceSet, @@ -57,6 +58,9 @@ pub struct LiminalDevices { uuid_lookup: HashMap, /// Devices that have not yet been set up or have been stopped. stopped_pools: HashMap, + /// Devices that have been left in a partially constructed state either during start + /// or stop. + partially_constructed_pools: HashMap, /// Lookup data structure for name to UUID mapping for starting pools by name. name_to_uuid: HashMap, } @@ -105,7 +109,11 @@ impl LiminalDevices { } } - let unlocked = match self.stopped_pools.get(&pool_uuid) { + let unlocked = match self + .stopped_pools + .get(&pool_uuid) + .or_else(|| self.partially_constructed_pools.get(&pool_uuid)) + { Some(map) => { let encryption_info = map.encryption_info(); if let Ok(None) = encryption_info { @@ -180,12 +188,16 @@ impl LiminalDevices { .ok_or_else(|| StratisError::Msg(format!("Could not find a pool with name {n}"))) .and_then(|uc| uc.to_result())?, }; + // Here we take a reference to entries in stopped pools because the call to unlock_pool + // below requires the pool being unlocked to still have its entry in stopped_pools. + // Removing it here would cause an error. let encryption_info = self .stopped_pools .get(&pool_uuid) + .or_else(|| self.partially_constructed_pools.get(&pool_uuid)) .ok_or_else(|| { StratisError::Msg(format!( - "Requested pool with UUID {pool_uuid} was not found in stopped pools" + "Requested pool with UUID {pool_uuid} was not found in stopped or partially constructed pools" )) })? .encryption_info(); @@ -217,6 +229,7 @@ impl LiminalDevices { let mut stopped_pool = self .stopped_pools .remove(&pool_uuid) + .or_else(|| self.partially_constructed_pools.remove(&pool_uuid)) .expect("Checked above"); match find_stratis_devs_by_uuid(pool_uuid, uuids) { Ok(infos) => infos.into_iter().for_each(|(dev_uuid, (path, devno))| { @@ -240,8 +253,8 @@ impl LiminalDevices { }), Err(e) => { warn!("Failed to scan for newly unlocked Stratis devices: {}", e); - self.stopped_pools.insert(pool_uuid, stopped_pool); - return Err(handle_unlock_rollback(e, handles)); + let err = handle_unlock_rollback(e, handles); + return Err(err); } }; @@ -255,11 +268,22 @@ impl LiminalDevices { /// in an internal data structure for later starting. pub fn stop_pool( &mut self, - pool_name: &Name, + pools: &mut Table, + pool_name: Name, pool_uuid: PoolUuid, - pool: &mut StratPool, + mut pool: StratPool, ) -> StratisResult<()> { - let devices = pool.stop(pool_name)?; + let (devices, err) = match pool.stop(&pool_name) { + Ok(devs) => (devs, None), + Err((e, true)) => { + pools.insert(pool_name, pool_uuid, pool); + return Err(e); + } + Err((e, false)) => { + warn!("Failed to stop pool; placing in partially constructed pools"); + (DeviceSet::from(pool.drain_bds()), Some(e)) + } + }; for (_, device) in devices.iter() { match device { LInfo::Luks(l) => { @@ -279,8 +303,12 @@ impl LiminalDevices { } } } - self.stopped_pools.insert(pool_uuid, devices); - if let Some(maybe_conflict) = self.name_to_uuid.get_mut(pool_name) { + if err.is_some() { + self.partially_constructed_pools.insert(pool_uuid, devices); + } else { + self.stopped_pools.insert(pool_uuid, devices); + } + if let Some(maybe_conflict) = self.name_to_uuid.get_mut(&pool_name) { maybe_conflict.add(pool_uuid); if let UuidOrConflict::Conflict(set) = maybe_conflict { warn!("Found conflicting names for stopped pools; UUID will be required to start pools with UUIDs {}", set.iter().map(|u| u.to_string()).collect::>().join(", ")); @@ -289,7 +317,11 @@ impl LiminalDevices { self.name_to_uuid .insert(pool_name.clone(), UuidOrConflict::Uuid(pool_uuid)); } - Ok(()) + if let Some(e) = err { + Err(e) + } else { + Ok(()) + } } /// Get a mapping of pool UUIDs from all of the LUKS2 devices that are currently @@ -356,6 +388,13 @@ impl LiminalDevices { map.stopped_pool_info().map(|info| (*pool_uuid, info)) }) .collect(), + partially_constructed: self + .partially_constructed_pools + .iter() + .filter_map(|(pool_uuid, map)| { + map.stopped_pool_info().map(|info| (*pool_uuid, info)) + }) + .collect(), } } @@ -530,9 +569,7 @@ impl LiminalDevices { "Some of the devices in pool with UUID {pool_uuid} are unopened" )); info!("Attempt to set up pool failed, but it may be possible to set up the pool later, if the situation changes: {}", err); - if !ds.is_empty() { - self.stopped_pools.insert(pool_uuid, ds); - } + self.handle_stopped_pool(pool_uuid, ds); return Err(err); } }; @@ -566,9 +603,7 @@ impl LiminalDevices { Err((err, bdas)) => { info!("Attempt to set up pool failed, but it may be possible to set up the pool later, if the situation changes: {}", err); let device_set = reconstruct_stratis_infos(infos, bdas); - if !device_set.is_empty() { - self.stopped_pools.insert(pool_uuid, device_set); - } + self.handle_stopped_pool(pool_uuid, device_set); Err(err) } } @@ -620,9 +655,7 @@ impl LiminalDevices { "Some of the devices in pool with UUID {pool_uuid} are unopened" )); info!("Attempt to set up pool failed, but it may be possible to set up the pool later, if the situation changes: {}", err); - if !ds.is_empty() { - self.stopped_pools.insert(pool_uuid, ds); - } + self.handle_stopped_pool(pool_uuid, ds); return None; } }; @@ -655,9 +688,7 @@ impl LiminalDevices { } Ok(Either::Right(bdas)) => { let device_set = reconstruct_stratis_infos(infos, bdas); - if !device_set.is_empty() { - self.stopped_pools.insert(pool_uuid, device_set); - } + self.handle_stopped_pool(pool_uuid, device_set); None } Err((err, bdas)) => { @@ -842,6 +873,29 @@ impl LiminalDevices { None } } + + pub fn handle_stopped_pool(&mut self, pool_uuid: PoolUuid, device_set: DeviceSet) { + if !device_set.is_empty() { + let device_uuids = device_set + .iter() + .map(|(dev_uuid, _)| *dev_uuid) + .collect::>(); + if has_leftover_devices(pool_uuid, &device_uuids) { + match stop_partially_constructed_pool(pool_uuid, &device_set) { + Ok(_) => { + assert!(!has_leftover_devices(pool_uuid, &device_uuids,)); + self.stopped_pools.insert(pool_uuid, device_set); + } + Err(_) => { + self.partially_constructed_pools + .insert(pool_uuid, device_set); + } + } + } else { + self.stopped_pools.insert(pool_uuid, device_set); + } + } + } } impl<'a> Into for &'a LiminalDevices { @@ -858,6 +912,17 @@ impl<'a> Into for &'a LiminalDevices { }) .collect() ), + "partially_constructed_pools": Value::Array( + self.partially_constructed_pools + .iter() + .map(|(uuid, set)| { + json!({ + "pool_uuid": uuid.to_string(), + "devices": <&DeviceSet as Into>::into(set), + }) + }) + .collect() + ), "path_to_ids_map": Value::Object( self.uuid_lookup .iter() diff --git a/src/engine/strat_engine/names.rs b/src/engine/strat_engine/names.rs index 553c966a4e..18ea42de67 100644 --- a/src/engine/strat_engine/names.rs +++ b/src/engine/strat_engine/names.rs @@ -61,12 +61,13 @@ impl KeyDescription { /// < 128 /// /// which is equivalent to len(format!("{}", FORMAT_VERSION) < 73 -pub fn format_crypt_name(dev_uuid: &DevUuid) -> String { - format!( +pub fn format_crypt_name(dev_uuid: &DevUuid) -> DmNameBuf { + let value = format!( "stratis-{}-private-{}-crypt", FORMAT_VERSION, uuid_to_string!(dev_uuid) - ) + ); + DmNameBuf::new(value).expect("FORMAT_VERSION display length < 73") } #[derive(Clone, Copy)] diff --git a/src/engine/strat_engine/pool.rs b/src/engine/strat_engine/pool.rs index b802f55833..8a44ffb419 100644 --- a/src/engine/strat_engine/pool.rs +++ b/src/engine/strat_engine/pool.rs @@ -19,8 +19,8 @@ use crate::{ }, strat_engine::{ backstore::{Backstore, ProcessedPathInfos, StratBlockDev, UnownedDevices}, - liminal::{DeviceInfo, DeviceSet, LInfo}, - metadata::MDADataSize, + liminal::DeviceSet, + metadata::{MDADataSize, BDA}, serde_structs::{FlexDevsSave, PoolSave, Recordable}, shared::tiers_to_bdas, thinpool::{StratFilesystem, ThinPool, ThinPoolSizeParams, DATA_BLOCK_SIZE}, @@ -318,7 +318,7 @@ impl StratPool { /// Teardown a pool. #[cfg(test)] pub fn teardown(&mut self) -> StratisResult<()> { - self.thin_pool.teardown()?; + self.thin_pool.teardown().map_err(|(e, _)| e)?; self.backstore.teardown()?; Ok(()) } @@ -390,9 +390,9 @@ impl StratPool { /// /// This method is not a mutating action as the pool should be allowed /// to be destroyed even if the metadata is inconsistent. - pub fn destroy(&mut self) -> StratisResult<()> { + pub fn destroy(&mut self) -> Result<(), (StratisError, bool)> { self.thin_pool.teardown()?; - self.backstore.destroy()?; + self.backstore.destroy().map_err(|e| (e, false))?; Ok(()) } @@ -408,17 +408,27 @@ impl StratPool { /// Stop a pool, consuming it and converting it into a set of devices to be /// set up again later. - pub fn stop(&mut self, pool_name: &Name) -> StratisResult { + pub fn stop(&mut self, pool_name: &Name) -> Result { self.thin_pool.teardown()?; let mut data = self.record(pool_name); data.started = Some(false); - let json = serde_json::to_string(&data)?; - self.backstore.save_state(json.as_bytes())?; - let bds = self.backstore.teardown()?; - Ok(bds - .into_iter() - .map(|bd| (bd.uuid(), LInfo::from(DeviceInfo::from(bd)))) - .collect::()) + let json = serde_json::to_string(&data).map_err(|e| (StratisError::from(e), false))?; + self.backstore + .save_state(json.as_bytes()) + .map_err(|e| (e, false))?; + self.backstore.teardown().map_err(|e| (e, false))?; + let bds = self.backstore.drain_bds(); + Ok(DeviceSet::from(bds)) + } + + /// Convert a pool into a record of BDAs for the given block devices in the pool. + pub fn into_bdas(self) -> HashMap { + self.backstore.into_bdas() + } + + /// Drain pool block devices into a record of block devices in the pool. + pub fn drain_bds(&mut self) -> Vec { + self.backstore.drain_bds() } #[cfg(test)] diff --git a/src/engine/strat_engine/thinpool/thinpool.rs b/src/engine/strat_engine/thinpool/thinpool.rs index dc010d5f9d..5ebdc26528 100644 --- a/src/engine/strat_engine/thinpool/thinpool.rs +++ b/src/engine/strat_engine/thinpool/thinpool.rs @@ -810,16 +810,25 @@ impl ThinPool { /// Tear down the components managed here: filesystems, the MDV, /// and the actual thinpool device itself. - pub fn teardown(&mut self) -> StratisResult<()> { + /// + /// Err(_) contains a tuple with a bool as the second element indicating whether or not + /// there are filesystems that were unable to be torn down. This distinction exists because + /// if filesystems remain, the pool could receive IO and should remain in set up pool data + /// structures. However if all filesystems were torn down, the pool can be moved to + /// the designation of partially constructed pools as no IO can be received on the pool + /// and it has been partially torn down. + pub fn teardown(&mut self) -> Result<(), (StratisError, bool)> { // Must succeed in tearing down all filesystems before the // thinpool.. for (_, _, ref mut fs) in &mut self.filesystems { - fs.teardown()?; + fs.teardown().map_err(|e| (e, true))?; } - self.thin_pool.teardown(get_dm())?; + self.thin_pool + .teardown(get_dm()) + .map_err(|e| (StratisError::from(e), false))?; // ..but MDV has no DM dependencies with the above - self.mdv.teardown()?; + self.mdv.teardown().map_err(|e| (e, false))?; Ok(()) } diff --git a/src/engine/types/mod.rs b/src/engine/types/mod.rs index 8accf010cf..23e5031816 100644 --- a/src/engine/types/mod.rs +++ b/src/engine/types/mod.rs @@ -249,6 +249,7 @@ pub struct StoppedPoolInfo { #[derive(Default, Debug, Eq, PartialEq)] pub struct StoppedPoolsInfo { pub stopped: HashMap, + pub partially_constructed: HashMap, pub name_to_uuid: HashMap, pub uuid_to_name: HashMap, } diff --git a/src/jsonrpc/server/pool.rs b/src/jsonrpc/server/pool.rs index 6cda09c568..c697fe1a39 100644 --- a/src/jsonrpc/server/pool.rs +++ b/src/jsonrpc/server/pool.rs @@ -232,16 +232,19 @@ where { let stopped = engine.stopped_pools().await; if engine.get_pool(id.clone()).await.is_some() { - Ok(false) - } else if stopped + return Ok(false); + } + let pool_uuid = match id { + PoolIdentifier::Uuid(ref u) => u, + PoolIdentifier::Name(ref n) => stopped + .name_to_uuid + .get(n) + .ok_or_else(|| StratisError::Msg(format!("Could not find pool with name {n}")))?, + }; + if stopped .stopped - .get(match id { - PoolIdentifier::Uuid(ref u) => u, - PoolIdentifier::Name(ref n) => stopped - .name_to_uuid - .get(n) - .ok_or_else(|| StratisError::Msg(format!("Could not find pool with name {n}")))?, - }) + .get(pool_uuid) + .or_else(|| stopped.partially_constructed.get(pool_uuid)) .is_some() { Ok(true) From d04f8f3741403b0597b35bab2794fd6fb568236a Mon Sep 17 00:00:00 2001 From: John Baublitz Date: Fri, 17 Mar 2023 15:40:01 -0400 Subject: [PATCH 2/4] Change activation names to DmNameBuf --- .../strat_engine/backstore/crypt/handle.rs | 14 +- .../backstore/crypt/initialize.rs | 14 +- .../strat_engine/backstore/crypt/macros.rs | 41 -- .../backstore/crypt/metadata_handle.rs | 8 +- .../strat_engine/backstore/crypt/mod.rs | 4 +- .../strat_engine/backstore/crypt/shared.rs | 363 ++++++++++-------- src/engine/strat_engine/cmd.rs | 6 +- 7 files changed, 229 insertions(+), 221 deletions(-) diff --git a/src/engine/strat_engine/backstore/crypt/handle.rs b/src/engine/strat_engine/backstore/crypt/handle.rs index f5bcbc84da..58f9b57074 100644 --- a/src/engine/strat_engine/backstore/crypt/handle.rs +++ b/src/engine/strat_engine/backstore/crypt/handle.rs @@ -11,7 +11,7 @@ use std::{ use either::Either; use serde_json::Value; -use devicemapper::{Device, Sectors}; +use devicemapper::{Device, DmName, DmNameBuf, Sectors}; use libcryptsetup_rs::{ c_uint, consts::{flags::CryptActivate, vals::EncryptionFormat}, @@ -64,7 +64,7 @@ impl CryptHandle { physical_path: DevicePath, identifiers: StratisIdentifiers, encryption_info: EncryptionInfo, - activation_name: String, + activation_name: DmNameBuf, pool_name: Option, ) -> StratisResult { let device = get_devno_from_path(&physical_path)?; @@ -82,8 +82,8 @@ impl CryptHandle { metadata_handle: CryptMetadataHandle, ) -> StratisResult { let activated_path = DevicePath::new( - &once(DEVICEMAPPER_PATH) - .chain(once(metadata_handle.activation_name())) + &once(DEVICEMAPPER_PATH.to_string()) + .chain(once(metadata_handle.activation_name().to_string())) .collect::(), )?; Ok(CryptHandle { @@ -138,7 +138,7 @@ impl CryptHandle { } /// Return the name of the activated devicemapper device. - pub fn activation_name(&self) -> &str { + pub fn activation_name(&self) -> &DmName { self.metadata_handle.activation_name() } @@ -408,7 +408,7 @@ impl CryptHandle { let name = self.activation_name().to_owned(); let active_device = log_on_failure!( self.acquire_crypt_device()? - .runtime_handle(&name) + .runtime_handle(&name.to_string()) .get_active_device(), "Failed to get device size for encrypted logical device" ); @@ -449,7 +449,7 @@ impl CryptHandle { )?; crypt .context_handle() - .resize(self.activation_name(), processed_size) + .resize(&self.activation_name().to_string(), processed_size) .map_err(StratisError::Crypt) } } diff --git a/src/engine/strat_engine/backstore/crypt/initialize.rs b/src/engine/strat_engine/backstore/crypt/initialize.rs index 2808ab1526..7f220330a6 100644 --- a/src/engine/strat_engine/backstore/crypt/initialize.rs +++ b/src/engine/strat_engine/backstore/crypt/initialize.rs @@ -5,8 +5,9 @@ use std::path::Path; use either::Either; -use serde_json::Value; +use serde_json::{to_value, Value}; +use devicemapper::{DmName, DmNameBuf}; use libcryptsetup_rs::{ consts::{ flags::CryptVolumeKey, @@ -43,7 +44,7 @@ use crate::{ pub struct CryptInitializer { physical_path: DevicePath, identifiers: StratisIdentifiers, - activation_name: String, + activation_name: DmNameBuf, } impl CryptInitializer { @@ -54,7 +55,7 @@ impl CryptInitializer { ) -> CryptInitializer { CryptInitializer { physical_path, - activation_name: format_crypt_name(&dev_uuid).to_string(), + activation_name: format_crypt_name(&dev_uuid), identifiers: StratisIdentifiers::new(pool_uuid, dev_uuid), } } @@ -236,12 +237,11 @@ impl CryptInitializer { log_on_failure!( device.token_handle().json_set(TokenInput::ReplaceToken( STRATIS_TOKEN_ID, - &StratisLuks2Token { + &to_value(&StratisLuks2Token { devname: self.activation_name.clone(), identifiers: self.identifiers, pool_name: Some(pool_name.clone()), - } - .into(), + })?, )), "Failed to create the Stratis token" ); @@ -259,7 +259,7 @@ impl CryptInitializer { pub fn rollback( device: &mut CryptDevice, physical_path: &Path, - name: &str, + name: &DmName, ) -> StratisResult<()> { ensure_wiped(device, physical_path, name) } diff --git a/src/engine/strat_engine/backstore/crypt/macros.rs b/src/engine/strat_engine/backstore/crypt/macros.rs index 68d583ad5e..465149ef42 100644 --- a/src/engine/strat_engine/backstore/crypt/macros.rs +++ b/src/engine/strat_engine/backstore/crypt/macros.rs @@ -15,44 +15,3 @@ macro_rules! log_on_failure { result? }} } - -macro_rules! check_key { - ($condition:expr, $key:tt, $value:tt) => { - if $condition { - return Err($crate::stratis::StratisError::Msg(format!( - "Stratis token key '{}' requires a value of '{}'", - $key, $value, - ))); - } - }; -} - -macro_rules! check_and_get_key { - ($get:expr, $key:tt) => { - if let Some(v) = $get { - v - } else { - return Err($crate::stratis::StratisError::Msg(format!( - "Stratis token is missing key '{}' or the value is of the wrong type", - $key - ))); - } - }; - ($get:expr, $func:expr, $key:tt, $ty:ty) => { - if let Some(ref v) = $get { - $func(v).map_err(|e| { - $crate::stratis::StratisError::Msg(format!( - "Failed to convert value for key '{}' to type {}: {}", - $key, - stringify!($ty), - e - )) - })? - } else { - return Err($crate::stratis::StratisError::Msg(format!( - "Stratis token is missing key '{}' or the value is of the wrong type", - $key - ))); - } - }; -} diff --git a/src/engine/strat_engine/backstore/crypt/metadata_handle.rs b/src/engine/strat_engine/backstore/crypt/metadata_handle.rs index 9fdc1851f6..9da6d6abc4 100644 --- a/src/engine/strat_engine/backstore/crypt/metadata_handle.rs +++ b/src/engine/strat_engine/backstore/crypt/metadata_handle.rs @@ -4,7 +4,7 @@ use std::path::Path; -use devicemapper::Device; +use devicemapper::{Device, DmName, DmNameBuf}; use crate::{ engine::{ @@ -23,7 +23,7 @@ pub struct CryptMetadataHandle { pub(super) physical_path: DevicePath, pub(super) identifiers: StratisIdentifiers, pub(super) encryption_info: EncryptionInfo, - pub(super) activation_name: String, + pub(super) activation_name: DmNameBuf, pub(super) pool_name: Option, pub(super) device: Device, } @@ -33,7 +33,7 @@ impl CryptMetadataHandle { physical_path: DevicePath, identifiers: StratisIdentifiers, encryption_info: EncryptionInfo, - activation_name: String, + activation_name: DmNameBuf, pool_name: Option, device: Device, ) -> Self { @@ -72,7 +72,7 @@ impl CryptMetadataHandle { } /// Get the name of the activated device when it is activated. - pub fn activation_name(&self) -> &str { + pub fn activation_name(&self) -> &DmName { &self.activation_name } diff --git a/src/engine/strat_engine/backstore/crypt/mod.rs b/src/engine/strat_engine/backstore/crypt/mod.rs index 89bf86b8d3..29c2f169b0 100644 --- a/src/engine/strat_engine/backstore/crypt/mod.rs +++ b/src/engine/strat_engine/backstore/crypt/mod.rs @@ -274,11 +274,11 @@ mod tests { libc::close(fd); }; - let device_name = handle.activation_name().to_owned(); + let device_name = handle.activation_name(); loop { match libcryptsetup_rs::status( Some(&mut handle.acquire_crypt_device().unwrap()), - &device_name, + &device_name.to_string(), ) { Ok(CryptStatusInfo::Busy) => (), Ok(CryptStatusInfo::Active) => break, diff --git a/src/engine/strat_engine/backstore/crypt/shared.rs b/src/engine/strat_engine/backstore/crypt/shared.rs index 9810f99696..c786cda831 100644 --- a/src/engine/strat_engine/backstore/crypt/shared.rs +++ b/src/engine/strat_engine/backstore/crypt/shared.rs @@ -3,6 +3,7 @@ // file, You can obtain one at http://mozilla.org/MPL/2.0/. use std::{ + fmt::{self, Formatter}, fs::OpenOptions, io::Write, path::{Path, PathBuf}, @@ -10,12 +11,17 @@ use std::{ use data_encoding::BASE64URL_NOPAD; use either::Either; -use retry::{delay::Fixed, retry_with_index, Error}; -use serde_json::{Map, Value}; +use retry::{delay::Fixed, retry_with_index, Error as RetryError}; +use serde::{ + de::{Error, MapAccess, Visitor}, + ser::SerializeMap, + Deserialize, Deserializer, Serialize, Serializer, +}; +use serde_json::{from_value, to_value, Map, Value}; use sha2::{Digest, Sha256}; use tempfile::TempDir; -use devicemapper::Bytes; +use devicemapper::{Bytes, DmName, DmNameBuf}; use libcryptsetup_rs::{ c_uint, consts::{ @@ -24,7 +30,7 @@ use libcryptsetup_rs::{ CryptDebugLevel, CryptLogLevel, CryptStatusInfo, CryptWipePattern, EncryptionFormat, }, }, - set_debug_level, set_log_callback, CryptDevice, CryptInit, LibcryptErr, TokenInput, + set_debug_level, set_log_callback, CryptDevice, CryptInit, TokenInput, }; use crate::{ @@ -75,83 +81,181 @@ pub fn set_up_crypt_logging() { } pub struct StratisLuks2Token { - pub devname: String, + pub devname: DmNameBuf, pub identifiers: StratisIdentifiers, pub pool_name: Option, } -impl Into for StratisLuks2Token { - fn into(self) -> Value { - let mut object = json!({ - TOKEN_TYPE_KEY: STRATIS_TOKEN_TYPE, - TOKEN_KEYSLOTS_KEY: [], - STRATIS_TOKEN_DEVNAME_KEY: self.devname, - STRATIS_TOKEN_POOL_UUID_KEY: self.identifiers.pool_uuid.to_string(), - STRATIS_TOKEN_DEV_UUID_KEY: self.identifiers.device_uuid.to_string(), - }); - if let Some(o) = object.as_object_mut() { - if let Some(n) = self.pool_name { - o.insert( - STRATIS_TOKEN_POOLNAME_KEY.to_string(), - Value::from(n.to_string()), - ); - } +impl Serialize for StratisLuks2Token { + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + let mut map_serializer = serializer.serialize_map(None)?; + map_serializer.serialize_entry(TOKEN_TYPE_KEY, STRATIS_TOKEN_TYPE)?; + map_serializer.serialize_entry::<_, [u32; 0]>(TOKEN_KEYSLOTS_KEY, &[])?; + map_serializer.serialize_entry(STRATIS_TOKEN_DEVNAME_KEY, &self.devname.to_string())?; + map_serializer.serialize_entry( + STRATIS_TOKEN_POOL_UUID_KEY, + &self.identifiers.pool_uuid.to_string(), + )?; + map_serializer.serialize_entry( + STRATIS_TOKEN_DEV_UUID_KEY, + &self.identifiers.device_uuid.to_string(), + )?; + if let Some(ref pn) = self.pool_name { + map_serializer.serialize_entry(STRATIS_TOKEN_POOLNAME_KEY, pn)?; } - object + map_serializer.end() } } -impl<'a> TryFrom<&'a Value> for StratisLuks2Token { - type Error = StratisError; +impl<'de> Deserialize<'de> for StratisLuks2Token { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + struct StratisTokenVisitor; - fn try_from(v: &Value) -> StratisResult { - let map = if let Value::Object(m) = v { - m - } else { - return Err(StratisError::Crypt(LibcryptErr::InvalidConversion)); - }; + impl<'de> Visitor<'de> for StratisTokenVisitor { + type Value = StratisLuks2Token; - check_key!( - map.get(TOKEN_TYPE_KEY).and_then(|v| v.as_str()) != Some(STRATIS_TOKEN_TYPE), - "type", - STRATIS_TOKEN_TYPE - ); - check_key!( - map.get(TOKEN_KEYSLOTS_KEY).and_then(|v| v.as_array()) != Some(&Vec::new()), - "keyslots", - "[]" - ); - let devname = check_and_get_key!( - map.get(STRATIS_TOKEN_DEVNAME_KEY) - .and_then(|s| s.as_str()) - .map(|s| s.to_string()), - STRATIS_TOKEN_DEVNAME_KEY - ); - let pool_uuid = check_and_get_key!( - map.get(STRATIS_TOKEN_POOL_UUID_KEY) - .and_then(|s| s.as_str()) - .map(|s| s.to_string()), - PoolUuid::parse_str, - STRATIS_TOKEN_POOL_UUID_KEY, - PoolUuid - ); - let dev_uuid = check_and_get_key!( - map.get(STRATIS_TOKEN_DEV_UUID_KEY) - .and_then(|s| s.as_str()) - .map(|s| s.to_string()), - DevUuid::parse_str, - STRATIS_TOKEN_DEV_UUID_KEY, - DevUuid - ); - let pool_name = map - .get(STRATIS_TOKEN_POOLNAME_KEY) - .and_then(|s| s.as_str()) - .map(|s| Name::new(s.to_string())); - Ok(StratisLuks2Token { - devname, - identifiers: StratisIdentifiers::new(pool_uuid, dev_uuid), - pool_name, - }) + fn expecting(&self, f: &mut Formatter<'_>) -> fmt::Result { + write!(f, "a Stratis LUKS2 token") + } + + fn visit_map(self, mut map: A) -> Result + where + A: MapAccess<'de>, + { + let mut token_type = None; + let mut token_keyslots = None; + let mut d_name = None; + let mut p_uuid = None; + let mut d_uuid = None; + let mut p_name = None; + + while let Some((k, v)) = map.next_entry::()? { + match k.as_str() { + TOKEN_TYPE_KEY => { + token_type = Some(v); + } + TOKEN_KEYSLOTS_KEY => { + token_keyslots = Some(v); + } + STRATIS_TOKEN_DEVNAME_KEY => { + d_name = Some(v); + } + STRATIS_TOKEN_POOL_UUID_KEY => { + p_uuid = Some(v); + } + STRATIS_TOKEN_DEV_UUID_KEY => { + d_uuid = Some(v); + } + STRATIS_TOKEN_POOLNAME_KEY => { + p_name = Some(v); + } + st => { + return Err(A::Error::custom(format!("Found unrecognized key {st}"))); + } + } + } + + token_type + .ok_or_else(|| A::Error::custom(format!("Missing field {TOKEN_TYPE_KEY}"))) + .and_then(|ty| match ty { + Value::String(s) => { + if s == STRATIS_TOKEN_TYPE { + Ok(()) + } else { + Err(A::Error::custom(format!( + "Incorrect value for {TOKEN_TYPE_KEY}: {s}" + ))) + } + } + _ => Err(A::Error::custom(format!( + "Unrecognized value type for {TOKEN_TYPE_KEY}" + ))), + }) + .and_then(|_| { + let value = token_keyslots.ok_or_else(|| { + A::Error::custom(format!("Missing field {TOKEN_KEYSLOTS_KEY}")) + })?; + match value { + Value::Array(a) => { + if a.is_empty() { + Ok(()) + } else { + Err(A::Error::custom(format!( + "Found non-empty array for {TOKEN_KEYSLOTS_KEY}" + ))) + } + } + _ => Err(A::Error::custom(format!( + "Unrecognized value type for {TOKEN_TYPE_KEY}" + ))), + } + }) + .and_then(|_| { + let value = d_name.ok_or_else(|| { + A::Error::custom(format!("Missing field {STRATIS_TOKEN_DEVNAME_KEY}")) + })?; + match value { + Value::String(s) => DmNameBuf::new(s).map_err(A::Error::custom), + _ => Err(A::Error::custom(format!( + "Unrecognized value type for {STRATIS_TOKEN_DEVNAME_KEY}" + ))), + } + }) + .and_then(|dev_name| { + let value = p_uuid.ok_or_else(|| { + A::Error::custom(format!("Missing field {STRATIS_TOKEN_POOL_UUID_KEY}")) + })?; + match value { + Value::String(s) => PoolUuid::parse_str(&s) + .map(|uuid| (dev_name, uuid)) + .map_err(A::Error::custom), + _ => Err(A::Error::custom(format!( + "Unrecognized value type for {STRATIS_TOKEN_POOL_UUID_KEY}" + ))), + } + }) + .and_then(|(dev_name, pool_uuid)| { + let value = d_uuid.ok_or_else(|| { + A::Error::custom(format!("Missing field {STRATIS_TOKEN_DEV_UUID_KEY}")) + })?; + match value { + Value::String(s) => DevUuid::parse_str(&s) + .map(|uuid| (dev_name, pool_uuid, uuid)) + .map_err(A::Error::custom), + _ => Err(A::Error::custom(format!( + "Unrecognized value type for {STRATIS_TOKEN_DEV_UUID_KEY}" + ))), + } + }) + .and_then(|(devname, pool_uuid, device_uuid)| { + let pool_name = match p_name { + Some(Value::String(s)) => Some(Name::new(s)), + Some(_) => { + return Err(A::Error::custom(format!( + "Unrecognized value type for {STRATIS_TOKEN_POOLNAME_KEY}" + ))) + } + None => None, + }; + Ok(StratisLuks2Token { + devname, + identifiers: StratisIdentifiers { + pool_uuid, + device_uuid, + }, + pool_name, + }) + }) + } + } + + deserializer.deserialize_map(StratisTokenVisitor) } } @@ -339,7 +443,7 @@ pub fn setup_crypt_handle( } Some(UnlockMethod::Clevis) => activate(Either::Right(physical_path), &name)?, None => { - if let Err(_) | Ok(CryptStatusInfo::Inactive | CryptStatusInfo::Invalid) = libcryptsetup_rs::status(Some(device), &name) { + if let Err(_) | Ok(CryptStatusInfo::Inactive | CryptStatusInfo::Invalid) = libcryptsetup_rs::status(Some(device), &name.to_string()) { return Err(StratisError::Msg( "Found a crypt device but it is not activated and no unlock method was provided".to_string(), )); @@ -588,7 +692,7 @@ fn is_encrypted_stratis_device(device: &mut CryptDevice) -> bool { return Err(StratisError::Msg("LUKS2 token is invalid".to_string())); } } - if let Some(ref st) = stratis_token { + if let Some(st) = stratis_token { if !stratis_token_is_valid(st) { return Err(StratisError::Msg("Stratis token is invalid".to_string())); } @@ -608,8 +712,8 @@ fn is_encrypted_stratis_device(device: &mut CryptDevice) -> bool { .unwrap_or(false) } -fn device_is_active(device: Option<&mut CryptDevice>, device_name: &str) -> StratisResult<()> { - match libcryptsetup_rs::status(device, device_name) { +fn device_is_active(device: Option<&mut CryptDevice>, device_name: &DmName) -> StratisResult<()> { + match libcryptsetup_rs::status(device, &device_name.to_string()) { Ok(CryptStatusInfo::Active) => Ok(()), Ok(CryptStatusInfo::Busy) => { info!( @@ -649,11 +753,11 @@ fn device_is_active(device: Option<&mut CryptDevice>, device_name: &str) -> Stra /// /// Precondition: The key description has been verified to be present in the keyring /// if matches!(unlock_method, UnlockMethod::Keyring). -fn activate_with_keyring(crypt_device: &mut CryptDevice, name: &str) -> StratisResult<()> { +fn activate_with_keyring(crypt_device: &mut CryptDevice, name: &DmName) -> StratisResult<()> { // Activate by token log_on_failure!( crypt_device.token_handle().activate_by_token::<()>( - Some(name), + Some(&name.to_string()), Some(LUKS2_TOKEN_ID), None, CryptActivate::empty(), @@ -668,7 +772,7 @@ fn activate_with_keyring(crypt_device: &mut CryptDevice, name: &str) -> StratisR /// Stratis token. pub fn activate( unlock_param: Either<(&mut CryptDevice, &KeyDescription), &Path>, - name: &str, + name: &DmName, ) -> StratisResult<()> { let crypt_device = match unlock_param { Either::Left((device, kd)) => { @@ -752,9 +856,9 @@ pub fn get_keyslot_number( /// a destructive action. `name` should be the name of the device as registered /// with devicemapper and cryptsetup. This method is idempotent and leaves /// the state as inactive. -pub fn ensure_inactive(device: &mut CryptDevice, name: &str) -> StratisResult<()> { +pub fn ensure_inactive(device: &mut CryptDevice, name: &DmName) -> StratisResult<()> { let status = log_on_failure!( - libcryptsetup_rs::status(Some(device), name), + libcryptsetup_rs::status(Some(device), &name.to_string()), "Failed to determine status of device with name {}", name ); @@ -763,7 +867,7 @@ pub fn ensure_inactive(device: &mut CryptDevice, name: &str) -> StratisResult<() log_on_failure!( device .activate_handle() - .deactivate(name, CryptDeactivate::empty()), + .deactivate(&name.to_string(), CryptDeactivate::empty()), "Failed to deactivate the crypt device with name {}", name ); @@ -773,16 +877,16 @@ pub fn ensure_inactive(device: &mut CryptDevice, name: &str) -> StratisResult<() trace!("Crypt device deactivate attempt {}", i); device .activate_handle() - .deactivate(name, CryptDeactivate::empty()) + .deactivate(&name.to_string(), CryptDeactivate::empty()) .map_err(StratisError::Crypt) }) .map_err(|e| match e { - Error::Internal(s) => StratisError::Chained( + RetryError::Internal(s) => StratisError::Chained( "Retries for crypt device deactivation failed with an internal error" .to_string(), Box::new(StratisError::Msg(s)), ), - Error::Operation { error, .. } => error, + RetryError::Operation { error, .. } => error, })?; } _ => (), @@ -836,7 +940,7 @@ pub fn wipe_fallback(path: &Path, causal_error: StratisError) -> StratisError { pub fn ensure_wiped( device: &mut CryptDevice, physical_path: &Path, - name: &str, + name: &DmName, ) -> StratisResult<()> { ensure_inactive(device, name)?; let keyslot_number = get_keyslot_number(device, LUKS2_TOKEN_ID); @@ -928,10 +1032,10 @@ fn luks2_token_type_is_valid(json: &Value) -> bool { } /// Validate that the Stratis token is present and valid -fn stratis_token_is_valid(json: &Value) -> bool { +fn stratis_token_is_valid(json: Value) -> bool { debug!("Stratis LUKS2 token: {}", json); - let result = StratisLuks2Token::try_from(json); + let result = from_value::(json); if let Err(ref e) = result { debug!( "LUKS2 token in the Stratis token slot does not appear \ @@ -963,30 +1067,8 @@ pub fn read_key(key_description: &KeyDescription) -> StratisResult StratisResult { - let json = log_on_failure!( - device.token_handle().json_get(STRATIS_TOKEN_ID), - "Failed to get Stratis JSON token from LUKS2 metadata" - ); - let name = log_on_failure!( - json.get(STRATIS_TOKEN_DEVNAME_KEY) - .ok_or_else(|| { - StratisError::Msg(format!( - "Missing JSON value for {STRATIS_TOKEN_DEVNAME_KEY}" - )) - }) - .and_then(|type_val| { - type_val.as_str().ok_or_else(|| { - StratisError::Msg(format!( - "Malformed JSON value for {STRATIS_TOKEN_DEVNAME_KEY}" - )) - }) - }) - .map(|type_str| type_str.to_string()), - "Could not get value for key {} from Stratis JSON token", - STRATIS_TOKEN_DEVNAME_KEY - ); - Ok(name) +fn activation_name_from_metadata(device: &mut CryptDevice) -> StratisResult { + Ok(from_value::(device.token_handle().json_get(STRATIS_TOKEN_ID)?)?.devname) } /// Query the Stratis metadata for the key description used to unlock the @@ -997,63 +1079,30 @@ pub fn key_desc_from_metadata(device: &mut CryptDevice) -> Option { /// Query the Stratis metadata for the pool name. pub fn pool_name_from_metadata(device: &mut CryptDevice) -> StratisResult> { - Ok(StratisLuks2Token::try_from(&device.token_handle().json_get(STRATIS_TOKEN_ID)?)?.pool_name) + Ok( + from_value::(device.token_handle().json_get(STRATIS_TOKEN_ID)?)? + .pool_name, + ) } /// Replace the old pool name in the Stratis LUKS2 token. pub fn replace_pool_name(device: &mut CryptDevice, new_name: Name) -> StratisResult<()> { let mut token = - StratisLuks2Token::try_from(&device.token_handle().json_get(STRATIS_TOKEN_ID)?)?; + from_value::(device.token_handle().json_get(STRATIS_TOKEN_ID)?)?; token.pool_name = Some(new_name); - device - .token_handle() - .json_set(TokenInput::ReplaceToken(STRATIS_TOKEN_ID, &token.into()))?; + device.token_handle().json_set(TokenInput::ReplaceToken( + STRATIS_TOKEN_ID, + &to_value(token)?, + ))?; Ok(()) } /// Query the Stratis metadata for the device identifiers. fn identifiers_from_metadata(device: &mut CryptDevice) -> StratisResult { - let json = log_on_failure!( - device.token_handle().json_get(STRATIS_TOKEN_ID), - "Failed to get Stratis JSON token from LUKS2 metadata" - ); - let pool_uuid = log_on_failure!( - json.get(STRATIS_TOKEN_POOL_UUID_KEY) - .ok_or_else(|| { - StratisError::Msg(format!( - "Missing JSON value for {STRATIS_TOKEN_POOL_UUID_KEY}" - )) - }) - .and_then(|type_val| { - type_val.as_str().ok_or_else(|| { - StratisError::Msg(format!( - "Malformed JSON value for {STRATIS_TOKEN_POOL_UUID_KEY}" - )) - }) - }) - .and_then(PoolUuid::parse_str), - "Could not get value for key {} from Stratis JSON token", - STRATIS_TOKEN_POOL_UUID_KEY - ); - let dev_uuid = log_on_failure!( - json.get(STRATIS_TOKEN_DEV_UUID_KEY) - .ok_or_else(|| { - StratisError::Msg(format!( - "Missing JSON value for {STRATIS_TOKEN_DEV_UUID_KEY}" - )) - }) - .and_then(|type_val| { - type_val.as_str().ok_or_else(|| { - StratisError::Msg(format!( - "Malformed JSON value for {STRATIS_TOKEN_DEV_UUID_KEY}" - )) - }) - }) - .and_then(|type_str| DevUuid::parse_str(type_str).map_err(StratisError::from)), - "Could not get value for key {} from Stratis JSON token", - STRATIS_TOKEN_DEV_UUID_KEY - ); - Ok(StratisIdentifiers::new(pool_uuid, dev_uuid)) + Ok( + from_value::(device.token_handle().json_get(STRATIS_TOKEN_ID)?)? + .identifiers, + ) } // Bytes occupied by crypt metadata diff --git a/src/engine/strat_engine/cmd.rs b/src/engine/strat_engine/cmd.rs index 689ccbf6f2..4877ed8121 100644 --- a/src/engine/strat_engine/cmd.rs +++ b/src/engine/strat_engine/cmd.rs @@ -26,7 +26,7 @@ use libc::c_uint; use libcryptsetup_rs::SafeMemHandle; use serde_json::Value; -use devicemapper::{MetaBlocks, Sectors}; +use devicemapper::{DmName, MetaBlocks, Sectors}; use crate::{ engine::{ @@ -339,7 +339,7 @@ pub fn clevis_luks_unbind(dev_path: &Path, keyslot: libc::c_uint) -> StratisResu } /// Unlock a device using the clevis CLI. -pub fn clevis_luks_unlock(dev_path: &Path, dm_name: &str) -> StratisResult<()> { +pub fn clevis_luks_unlock(dev_path: &Path, dm_name: &DmName) -> StratisResult<()> { execute_cmd( Command::new(get_clevis_executable(CLEVIS)?) .arg("luks") @@ -347,7 +347,7 @@ pub fn clevis_luks_unlock(dev_path: &Path, dm_name: &str) -> StratisResult<()> { .arg("-d") .arg(dev_path.display().to_string()) .arg("-n") - .arg(dm_name), + .arg(dm_name.to_string()), ) } From 91a997aea79f4c425441fa153ca445fdf0eef6ca Mon Sep 17 00:00:00 2001 From: John Baublitz Date: Mon, 20 Mar 2023 12:08:18 -0400 Subject: [PATCH 3/4] Replace devicemapper-rs high level teardown with name-based teardown --- .../strat_engine/backstore/backstore.rs | 52 ++++--------- .../strat_engine/backstore/crypt/shared.rs | 33 ++------ src/engine/strat_engine/dm.rs | 78 +++++++++++++------ src/engine/strat_engine/engine.rs | 4 +- .../strat_engine/liminal/device_info.rs | 18 ++++- src/engine/strat_engine/liminal/liminal.rs | 34 +++++++- src/engine/strat_engine/pool.rs | 34 ++++---- .../strat_engine/thinpool/filesystem.rs | 9 ++- src/engine/strat_engine/thinpool/mdv.rs | 11 ++- src/engine/strat_engine/thinpool/thinpool.rs | 19 +++-- 10 files changed, 166 insertions(+), 126 deletions(-) diff --git a/src/engine/strat_engine/backstore/backstore.rs b/src/engine/strat_engine/backstore/backstore.rs index 6e873499a0..ad88046d59 100644 --- a/src/engine/strat_engine/backstore/backstore.rs +++ b/src/engine/strat_engine/backstore/backstore.rs @@ -26,7 +26,7 @@ use crate::{ shared::BlockSizeSummary, transaction::RequestTransaction, }, - dm::get_dm, + dm::{get_dm, list_of_backstore_devices, remove_optional_devices}, metadata::{MDADataSize, BDA}, names::{format_backstore_ids, CacheRole}, serde_structs::{BackstoreSave, CapSave, Recordable}, @@ -550,43 +550,23 @@ impl Backstore { } /// Destroy the entire store. - pub fn destroy(&mut self) -> StratisResult<()> { - match self.cache { - Some(ref mut cache) => { - cache.teardown(get_dm())?; - self.cache_tier - .as_mut() - .expect("if dm_device is cache, cache tier exists") - .destroy()?; - } - None => { - if let Some(ref mut linear) = self.linear { - linear.teardown(get_dm())?; - } - } - }; + pub fn destroy(&mut self, pool_uuid: PoolUuid) -> StratisResult<()> { + let devs = list_of_backstore_devices(pool_uuid); + remove_optional_devices(devs)?; + if let Some(ref mut cache_tier) = self.cache_tier { + cache_tier.destroy()?; + } self.data_tier.destroy() } /// Teardown the DM devices in the backstore. - pub fn teardown(&mut self) -> StratisResult<()> { - match self - .cache - .as_mut() - .and_then(|c| self.cache_tier.as_mut().map(|ct| (c, ct))) - { - Some((cache, cache_tier)) => { - cache.teardown(get_dm())?; - cache_tier.block_mgr.teardown()?; - } - None => { - if let Some(ref mut linear) = self.linear { - linear.teardown(get_dm())?; - } - } - }; - self.data_tier.block_mgr.teardown()?; - Ok(()) + pub fn teardown(&mut self, pool_uuid: PoolUuid) -> StratisResult<()> { + let devs = list_of_backstore_devices(pool_uuid); + remove_optional_devices(devs)?; + if let Some(ref mut cache_tier) = self.cache_tier { + cache_tier.block_mgr.teardown()?; + } + self.data_tier.block_mgr.teardown() } /// Consume the backstore and convert it into a set of BDAs representing @@ -1252,7 +1232,7 @@ mod tests { CacheDevStatus::Fail => panic!("cache is in a failed state"), } - backstore.destroy().unwrap(); + backstore.destroy(pool_uuid).unwrap(); } #[test] @@ -1333,7 +1313,7 @@ mod tests { assert_ne!(backstore.device(), old_device); - backstore.destroy().unwrap(); + backstore.destroy(pool_uuid).unwrap(); } #[test] diff --git a/src/engine/strat_engine/backstore/crypt/shared.rs b/src/engine/strat_engine/backstore/crypt/shared.rs index c786cda831..87d87fa7bc 100644 --- a/src/engine/strat_engine/backstore/crypt/shared.rs +++ b/src/engine/strat_engine/backstore/crypt/shared.rs @@ -11,7 +11,6 @@ use std::{ use data_encoding::BASE64URL_NOPAD; use either::Either; -use retry::{delay::Fixed, retry_with_index, Error as RetryError}; use serde::{ de::{Error, MapAccess, Visitor}, ser::SerializeMap, @@ -21,11 +20,11 @@ use serde_json::{from_value, to_value, Map, Value}; use sha2::{Digest, Sha256}; use tempfile::TempDir; -use devicemapper::{Bytes, DmName, DmNameBuf}; +use devicemapper::{Bytes, DevId, DmName, DmNameBuf, DmOptions}; use libcryptsetup_rs::{ c_uint, consts::{ - flags::{CryptActivate, CryptDeactivate, CryptVolumeKey, CryptWipe}, + flags::{CryptActivate, CryptVolumeKey, CryptWipe}, vals::{ CryptDebugLevel, CryptLogLevel, CryptStatusInfo, CryptWipePattern, EncryptionFormat, }, @@ -51,6 +50,7 @@ use crate::{ devices::get_devno_from_path, }, cmd::clevis_luks_unlock, + dm::get_dm, keys, metadata::StratisIdentifiers, }, @@ -863,31 +863,8 @@ pub fn ensure_inactive(device: &mut CryptDevice, name: &DmName) -> StratisResult name ); match status { - CryptStatusInfo::Active => { - log_on_failure!( - device - .activate_handle() - .deactivate(&name.to_string(), CryptDeactivate::empty()), - "Failed to deactivate the crypt device with name {}", - name - ); - } - CryptStatusInfo::Busy => { - retry_with_index(Fixed::from_millis(100).take(2), |i| { - trace!("Crypt device deactivate attempt {}", i); - device - .activate_handle() - .deactivate(&name.to_string(), CryptDeactivate::empty()) - .map_err(StratisError::Crypt) - }) - .map_err(|e| match e { - RetryError::Internal(s) => StratisError::Chained( - "Retries for crypt device deactivation failed with an internal error" - .to_string(), - Box::new(StratisError::Msg(s)), - ), - RetryError::Operation { error, .. } => error, - })?; + CryptStatusInfo::Active | CryptStatusInfo::Busy => { + get_dm().device_remove(&DevId::Name(name), DmOptions::default())?; } _ => (), } diff --git a/src/engine/strat_engine/dm.rs b/src/engine/strat_engine/dm.rs index 6358db1064..960e978f56 100644 --- a/src/engine/strat_engine/dm.rs +++ b/src/engine/strat_engine/dm.rs @@ -10,14 +10,11 @@ use devicemapper::{DevId, DmNameBuf, DmOptions, DmResult, DM}; use crate::{ engine::{ - strat_engine::{ - liminal::DeviceSet, - names::{ - format_backstore_ids, format_crypt_name, format_flex_ids, format_thinpool_ids, - CacheRole, FlexRole, ThinPoolRole, - }, + strat_engine::names::{ + format_backstore_ids, format_crypt_name, format_flex_ids, format_thin_ids, + format_thinpool_ids, CacheRole, FlexRole, ThinPoolRole, ThinRole, }, - types::{DevUuid, PoolUuid}, + types::{DevUuid, FilesystemUuid, PoolUuid}, }, stratis::{StratisError, StratisResult}, }; @@ -48,21 +45,14 @@ pub fn get_dm() -> &'static DM { ) } -pub fn stop_partially_constructed_pool( - pool_uuid: PoolUuid, - device_set: &DeviceSet, -) -> StratisResult { +pub fn remove_optional_devices(devs: Vec) -> StratisResult { let mut did_something = false; let devices = get_dm() .list_devices()? .into_iter() .map(|(name, _, _)| name) .collect::>(); - let dev_uuids = device_set - .iter() - .map(|(dev_uuid, _)| *dev_uuid) - .collect::>(); - for device in list_of_devices(pool_uuid, &dev_uuids) { + for device in devs { if devices.contains(&device) { did_something = true; get_dm().device_remove(&DevId::Name(&device), DmOptions::default())?; @@ -71,11 +61,22 @@ pub fn stop_partially_constructed_pool( Ok(did_something) } -/// List of device names for removal on partially constructed pool stop. Does not have -/// filesystem names because partially constructed pools are guaranteed not to have any -/// active filesystems. -fn list_of_devices(pool_uuid: PoolUuid, dev_uuids: &[DevUuid]) -> Vec { +pub fn stop_partially_constructed_pool( + pool_uuid: PoolUuid, + dev_uuids: &[DevUuid], +) -> StratisResult { + let devs = list_of_partial_pool_devices(pool_uuid, dev_uuids); + remove_optional_devices(devs) +} + +pub fn thin_device(pool_uuid: PoolUuid, fs_uuid: FilesystemUuid) -> DmNameBuf { + let (dm_name, _) = format_thin_ids(pool_uuid, ThinRole::Filesystem(fs_uuid)); + dm_name +} + +pub fn list_of_thin_pool_devices(pool_uuid: PoolUuid) -> Vec { let mut devs = Vec::new(); + let (thin_pool, _) = format_thinpool_ids(pool_uuid, ThinPoolRole::Pool); devs.push(thin_pool); let (thin_data, _) = format_flex_ids(pool_uuid, FlexRole::ThinData); @@ -84,8 +85,18 @@ fn list_of_devices(pool_uuid: PoolUuid, dev_uuids: &[DevUuid]) -> Vec devs.push(thin_meta); let (thin_meta_spare, _) = format_flex_ids(pool_uuid, FlexRole::ThinMetaSpare); devs.push(thin_meta_spare); + + devs +} + +pub fn mdv_device(pool_uuid: PoolUuid) -> DmNameBuf { let (thin_mdv, _) = format_flex_ids(pool_uuid, FlexRole::MetadataVolume); - devs.push(thin_mdv); + thin_mdv +} + +pub fn list_of_backstore_devices(pool_uuid: PoolUuid) -> Vec { + let mut devs = Vec::new(); + let (cache, _) = format_backstore_ids(pool_uuid, CacheRole::Cache); devs.push(cache); let (cache_sub, _) = format_backstore_ids(pool_uuid, CacheRole::CacheSub); @@ -95,6 +106,12 @@ fn list_of_devices(pool_uuid: PoolUuid, dev_uuids: &[DevUuid]) -> Vec let (origin, _) = format_backstore_ids(pool_uuid, CacheRole::OriginSub); devs.push(origin); + devs +} + +pub fn list_of_crypt_devices(dev_uuids: &[DevUuid]) -> Vec { + let mut devs = Vec::new(); + for dev_uuid in dev_uuids.iter() { let crypt = format_crypt_name(dev_uuid); devs.push(crypt); @@ -103,10 +120,27 @@ fn list_of_devices(pool_uuid: PoolUuid, dev_uuids: &[DevUuid]) -> Vec devs } +/// List of device names for removal on partially constructed pool stop. Does not have +/// filesystem names because partially constructed pools are guaranteed not to have any +/// active filesystems. +fn list_of_partial_pool_devices(pool_uuid: PoolUuid, dev_uuids: &[DevUuid]) -> Vec { + let mut devs = Vec::new(); + + devs.extend(list_of_thin_pool_devices(pool_uuid)); + + devs.push(mdv_device(pool_uuid)); + + devs.extend(list_of_backstore_devices(pool_uuid)); + + devs.extend(list_of_crypt_devices(dev_uuids)); + + devs +} + /// Check whether there are any leftover devicemapper devices from the pool. pub fn has_leftover_devices(pool_uuid: PoolUuid, dev_uuids: &[DevUuid]) -> bool { let mut has_leftover = false; - let devices = list_of_devices(pool_uuid, dev_uuids); + let devices = list_of_partial_pool_devices(pool_uuid, dev_uuids); match get_dm().list_devices() { Ok(l) => { let listed_devices = l diff --git a/src/engine/strat_engine/engine.rs b/src/engine/strat_engine/engine.rs index 859d306932..bc83ed3189 100644 --- a/src/engine/strat_engine/engine.rs +++ b/src/engine/strat_engine/engine.rs @@ -233,7 +233,7 @@ impl StratEngine { let mut untorndown_pools = Vec::new(); let mut write_all = block_on(self.pools.write_all()); for (_, uuid, pool) in write_all.iter_mut() { - pool.teardown() + pool.teardown(*uuid) .unwrap_or_else(|_| untorndown_pools.push(uuid)); } if untorndown_pools.is_empty() { @@ -453,7 +453,7 @@ impl Engine for StratEngine { .remove_by_uuid(uuid) .expect("Must succeed since self.pools.get_by_uuid() returned a value"); - let (res, mut pool) = spawn_blocking!((pool.destroy(), pool))?; + let (res, mut pool) = spawn_blocking!((pool.destroy(uuid), pool))?; if let Err((err, true)) = res { guard.insert(pool_name, uuid, pool); Err(err) diff --git a/src/engine/strat_engine/liminal/device_info.rs b/src/engine/strat_engine/liminal/device_info.rs index d8df7340c9..1b65aa22e2 100644 --- a/src/engine/strat_engine/liminal/device_info.rs +++ b/src/engine/strat_engine/liminal/device_info.rs @@ -47,8 +47,13 @@ impl fmt::Display for LLuksInfo { write!( f, "{}, {}, {}", - self.dev_info, self.identifiers, self.encryption_info - ) + self.dev_info, self.identifiers, self.encryption_info, + )?; + if let Some(ref pn) = self.pool_name { + write!(f, ", {}", pn) + } else { + Ok(()) + } } } @@ -502,6 +507,15 @@ impl FromIterator<(DevUuid, LInfo)> for DeviceSet { } } +impl IntoIterator for DeviceSet { + type Item = as IntoIterator>::Item; + type IntoIter = as IntoIterator>::IntoIter; + + fn into_iter(self) -> Self::IntoIter { + self.internal.into_iter() + } +} + impl From> for DeviceSet { fn from(vec: Vec) -> Self { vec.into_iter() diff --git a/src/engine/strat_engine/liminal/liminal.rs b/src/engine/strat_engine/liminal/liminal.rs index 7664cd5d59..4b78bc5e21 100644 --- a/src/engine/strat_engine/liminal/liminal.rs +++ b/src/engine/strat_engine/liminal/liminal.rs @@ -273,7 +273,7 @@ impl LiminalDevices { pool_uuid: PoolUuid, mut pool: StratPool, ) -> StratisResult<()> { - let (devices, err) = match pool.stop(&pool_name) { + let (devices, err) = match pool.stop(&pool_name, pool_uuid) { Ok(devs) => (devs, None), Err((e, true)) => { pools.insert(pool_name, pool_uuid, pool); @@ -881,9 +881,37 @@ impl LiminalDevices { .map(|(dev_uuid, _)| *dev_uuid) .collect::>(); if has_leftover_devices(pool_uuid, &device_uuids) { - match stop_partially_constructed_pool(pool_uuid, &device_set) { + let dev_uuids = device_set + .iter() + .map(|(dev_uuid, _)| *dev_uuid) + .collect::>(); + let res = stop_partially_constructed_pool(pool_uuid, &dev_uuids); + let device_set = device_set + .into_iter() + .map(|(dev_uuid, info)| { + ( + dev_uuid, + match info { + LInfo::Luks(l) => LInfo::Luks(l), + LInfo::Stratis(mut s) => { + if let Some(l) = s.luks { + if !s.dev_info.devnode.exists() { + LInfo::Luks(l) + } else { + s.luks = Some(l); + LInfo::Stratis(s) + } + } else { + LInfo::Stratis(s) + } + } + }, + ) + }) + .collect::(); + match res { Ok(_) => { - assert!(!has_leftover_devices(pool_uuid, &device_uuids,)); + assert!(!has_leftover_devices(pool_uuid, &device_uuids)); self.stopped_pools.insert(pool_uuid, device_set); } Err(_) => { diff --git a/src/engine/strat_engine/pool.rs b/src/engine/strat_engine/pool.rs index 8a44ffb419..862560da09 100644 --- a/src/engine/strat_engine/pool.rs +++ b/src/engine/strat_engine/pool.rs @@ -187,7 +187,7 @@ impl StratPool { match ThinPoolSizeParams::new(backstore.datatier_usable_size()) { Ok(ref params) => params, Err(causal_error) => { - if let Err(cleanup_err) = backstore.destroy() { + if let Err(cleanup_err) = backstore.destroy(pool_uuid) { warn!("Failed to clean up Stratis metadata for incompletely set up pool with UUID {}: {}.", pool_uuid, cleanup_err); return Err(StratisError::NoActionRollbackError { causal_error: Box::new(causal_error), @@ -204,7 +204,7 @@ impl StratPool { let thinpool = match thinpool { Ok(thinpool) => thinpool, Err(causal_error) => { - if let Err(cleanup_err) = backstore.destroy() { + if let Err(cleanup_err) = backstore.destroy(pool_uuid) { warn!("Failed to clean up Stratis metadata for incompletely set up pool with UUID {}: {}.", pool_uuid, cleanup_err); return Err(StratisError::NoActionRollbackError { causal_error: Box::new(causal_error), @@ -317,9 +317,9 @@ impl StratPool { /// Teardown a pool. #[cfg(test)] - pub fn teardown(&mut self) -> StratisResult<()> { - self.thin_pool.teardown().map_err(|(e, _)| e)?; - self.backstore.teardown()?; + pub fn teardown(&mut self, pool_uuid: PoolUuid) -> StratisResult<()> { + self.thin_pool.teardown(pool_uuid).map_err(|(e, _)| e)?; + self.backstore.teardown(pool_uuid)?; Ok(()) } @@ -390,9 +390,9 @@ impl StratPool { /// /// This method is not a mutating action as the pool should be allowed /// to be destroyed even if the metadata is inconsistent. - pub fn destroy(&mut self) -> Result<(), (StratisError, bool)> { - self.thin_pool.teardown()?; - self.backstore.destroy().map_err(|e| (e, false))?; + pub fn destroy(&mut self, pool_uuid: PoolUuid) -> Result<(), (StratisError, bool)> { + self.thin_pool.teardown(pool_uuid)?; + self.backstore.destroy(pool_uuid).map_err(|e| (e, false))?; Ok(()) } @@ -408,15 +408,19 @@ impl StratPool { /// Stop a pool, consuming it and converting it into a set of devices to be /// set up again later. - pub fn stop(&mut self, pool_name: &Name) -> Result { - self.thin_pool.teardown()?; + pub fn stop( + &mut self, + pool_name: &Name, + pool_uuid: PoolUuid, + ) -> Result { + self.thin_pool.teardown(pool_uuid)?; let mut data = self.record(pool_name); data.started = Some(false); let json = serde_json::to_string(&data).map_err(|e| (StratisError::from(e), false))?; self.backstore .save_state(json.as_bytes()) .map_err(|e| (e, false))?; - self.backstore.teardown().map_err(|e| (e, false))?; + self.backstore.teardown(pool_uuid).map_err(|e| (e, false))?; let bds = self.backstore.drain_bds(); Ok(DeviceSet::from(bds)) } @@ -1304,7 +1308,7 @@ mod tests { } assert_eq!(&buf, bytestring); umount(tmp_dir.path()).unwrap(); - pool.teardown().unwrap(); + pool.teardown(uuid).unwrap(); } #[test] @@ -1344,7 +1348,7 @@ mod tests { pool.add_blockdevs(uuid, name, data_paths, BlockDevTier::Data) .unwrap(); - pool.teardown().unwrap(); + pool.teardown(uuid).unwrap(); } #[test] @@ -1459,14 +1463,14 @@ mod tests { let (stratis_devices, unowned_devices) = devices.unpack(); stratis_devices.error_on_not_empty().unwrap(); - let (_, mut pool) = StratPool::initialize(name, unowned_devices, None).unwrap(); + let (uuid, mut pool) = StratPool::initialize(name, unowned_devices, None).unwrap(); invariant(&pool, name); assert_eq!(pool.action_avail, ActionAvailability::Full); assert!(pool.return_rollback_failure().is_err()); assert_eq!(pool.action_avail, ActionAvailability::NoRequests); - pool.destroy().unwrap(); + pool.destroy(uuid).unwrap(); udev_settle().unwrap(); let name = "stratis-test-pool"; diff --git a/src/engine/strat_engine/thinpool/filesystem.rs b/src/engine/strat_engine/thinpool/filesystem.rs index bb84e297cc..ac85aec876 100644 --- a/src/engine/strat_engine/thinpool/filesystem.rs +++ b/src/engine/strat_engine/thinpool/filesystem.rs @@ -15,7 +15,7 @@ use retry::{delay::Fixed, retry_with_index}; use serde_json::{Map, Value}; use devicemapper::{ - Bytes, DmDevice, DmName, DmOptions, DmUuid, Sectors, ThinDev, ThinDevId, ThinPoolDev, + Bytes, DevId, DmDevice, DmName, DmOptions, DmUuid, Sectors, ThinDev, ThinDevId, ThinPoolDev, ThinStatus, }; @@ -31,7 +31,7 @@ use crate::{ strat_engine::{ cmd::{create_fs, set_uuid, xfs_growfs}, devlinks, - dm::get_dm, + dm::{get_dm, thin_device}, names::{format_thin_ids, ThinRole}, serde_structs::FilesystemSave, }, @@ -322,8 +322,9 @@ impl StratFilesystem { } /// Tear down the filesystem. - pub fn teardown(&mut self) -> StratisResult<()> { - self.thin_dev.teardown(get_dm())?; + pub fn teardown(pool_uuid: PoolUuid, fs_uuid: FilesystemUuid) -> StratisResult<()> { + let device = thin_device(pool_uuid, fs_uuid); + get_dm().device_remove(&DevId::Name(&device), DmOptions::default())?; Ok(()) } diff --git a/src/engine/strat_engine/thinpool/mdv.rs b/src/engine/strat_engine/thinpool/mdv.rs index 0bdf0cbb3a..444b763eda 100644 --- a/src/engine/strat_engine/thinpool/mdv.rs +++ b/src/engine/strat_engine/thinpool/mdv.rs @@ -19,13 +19,15 @@ use nix::{ }; use retry::{delay::Fixed, retry_with_index}; -use devicemapper::{DmDevice, DmOptions, LinearDev, LinearDevTargetParams, Sectors, TargetLine}; +use devicemapper::{ + DevId, DmDevice, DmOptions, LinearDev, LinearDevTargetParams, Sectors, TargetLine, +}; use crate::{ engine::{ strat_engine::{ cmd::create_fs, - dm::get_dm, + dm::{get_dm, mdv_device}, ns::NS_TMPFS_LOCATION, serde_structs::FilesystemSave, thinpool::filesystem::{fs_usage, StratFilesystem}, @@ -188,7 +190,7 @@ impl MetadataVol { } /// Tear down a Metadata Volume. - pub fn teardown(&mut self) -> StratisResult<()> { + pub fn teardown(&mut self, pool_uuid: PoolUuid) -> StratisResult<()> { if let Err(e) = retry_with_index(Fixed::from_millis(100).take(2), |i| { trace!("MDV unmount attempt {}", i); umount(&self.mount_pt) @@ -206,7 +208,8 @@ impl MetadataVol { warn!("Could not remove MDV mount point: {}", err); } - self.dev.teardown(get_dm())?; + let dev = mdv_device(pool_uuid); + get_dm().device_remove(&DevId::Name(&dev), DmOptions::default())?; Ok(()) } diff --git a/src/engine/strat_engine/thinpool/thinpool.rs b/src/engine/strat_engine/thinpool/thinpool.rs index 5ebdc26528..68743cabbe 100644 --- a/src/engine/strat_engine/thinpool/thinpool.rs +++ b/src/engine/strat_engine/thinpool/thinpool.rs @@ -26,7 +26,7 @@ use crate::{ strat_engine::{ backstore::Backstore, cmd::{thin_check, thin_metadata_size, thin_repair}, - dm::get_dm, + dm::{get_dm, list_of_thin_pool_devices, remove_optional_devices}, names::{ format_flex_ids, format_thin_ids, format_thinpool_ids, FlexRole, ThinPoolRole, ThinRole, @@ -817,18 +817,17 @@ impl ThinPool { /// structures. However if all filesystems were torn down, the pool can be moved to /// the designation of partially constructed pools as no IO can be received on the pool /// and it has been partially torn down. - pub fn teardown(&mut self) -> Result<(), (StratisError, bool)> { + pub fn teardown(&mut self, pool_uuid: PoolUuid) -> Result<(), (StratisError, bool)> { // Must succeed in tearing down all filesystems before the // thinpool.. - for (_, _, ref mut fs) in &mut self.filesystems { - fs.teardown().map_err(|e| (e, true))?; + for (_, fs_uuid, _) in &mut self.filesystems { + StratFilesystem::teardown(pool_uuid, *fs_uuid).map_err(|e| (e, true))?; } - self.thin_pool - .teardown(get_dm()) - .map_err(|e| (StratisError::from(e), false))?; + let devs = list_of_thin_pool_devices(pool_uuid); + remove_optional_devices(devs).map_err(|e| (e, false))?; // ..but MDV has no DM dependencies with the above - self.mdv.teardown().map_err(|e| (e, false))?; + self.mdv.teardown(pool_uuid).map_err(|e| (e, false))?; Ok(()) } @@ -2143,7 +2142,7 @@ mod tests { let flexdevs: FlexDevsSave = pool.record(); let thinpoolsave: ThinPoolDevSave = pool.record(); - retry_operation!(pool.teardown()); + retry_operation!(pool.teardown(pool_uuid)); let pool = ThinPool::setup(pool_name, pool_uuid, &thinpoolsave, &flexdevs, &backstore).unwrap(); @@ -2275,7 +2274,7 @@ mod tests { retry_operation!(pool.destroy_filesystem(pool_name, fs_uuid)); let flexdevs: FlexDevsSave = pool.record(); let thinpooldevsave: ThinPoolDevSave = pool.record(); - pool.teardown().unwrap(); + pool.teardown(pool_uuid).unwrap(); // Check that destroyed fs is not present in MDV. If the record // had been left on the MDV that didn't match a thin_id in the From 83660a100f05f651b5898fc29cbfd0f54b6bb361 Mon Sep 17 00:00:00 2001 From: John Baublitz Date: Wed, 29 Mar 2023 13:56:15 -0400 Subject: [PATCH 4/4] Remove filesystem record after teardown --- src/engine/strat_engine/thinpool/thinpool.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/engine/strat_engine/thinpool/thinpool.rs b/src/engine/strat_engine/thinpool/thinpool.rs index 68743cabbe..97427d969f 100644 --- a/src/engine/strat_engine/thinpool/thinpool.rs +++ b/src/engine/strat_engine/thinpool/thinpool.rs @@ -818,10 +818,17 @@ impl ThinPool { /// the designation of partially constructed pools as no IO can be received on the pool /// and it has been partially torn down. pub fn teardown(&mut self, pool_uuid: PoolUuid) -> Result<(), (StratisError, bool)> { + let fs_uuids = self + .filesystems + .iter() + .map(|(_, fs_uuid, _)| *fs_uuid) + .collect::>(); + // Must succeed in tearing down all filesystems before the // thinpool.. - for (_, fs_uuid, _) in &mut self.filesystems { - StratFilesystem::teardown(pool_uuid, *fs_uuid).map_err(|e| (e, true))?; + for fs_uuid in fs_uuids { + StratFilesystem::teardown(pool_uuid, fs_uuid).map_err(|e| (e, true))?; + self.filesystems.remove_by_uuid(fs_uuid); } let devs = list_of_thin_pool_devices(pool_uuid); remove_optional_devices(devs).map_err(|e| (e, false))?;