Add rollback of Pool setup when starting a pool fails

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() {

src/engine/sim_engine/engine.rs

@@ -84,8 +84,9 @@ impl<'a> Into<Value> for &'a SimEngine {
                         unreachable!("json!() output is always JSON object");
                     }
                 })
-                .collect()
+                .collect(),
             ),
+            "partially_constructed_pools": Value::Array(Vec::new())
         })
     }
 }

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<Vec<StratBlockDev>> {
-        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::<HashMap<_, _>>()
     }
 
+    /// Drain the backstore devices into a set of all data and cache devices.
+    pub fn drain_bds(&mut self) -> Vec<StratBlockDev> {
+        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

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<StratBlockDev> {
+        self.block_devs.drain(..).collect::<Vec<_>>()
+    }
+
     /// Get a hashmap that maps UUIDs to Devices.
     pub fn uuid_to_devno(&self) -> HashMap<DevUuid, Device> {
         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<Vec<StratBlockDev>> {
-        let (oks, errs) = self
-            .block_devs
-            .drain(..)
-            .map(|mut bd| {
-                bd.teardown()?;
-                Ok(bd)
-            })
-            .partition::<Vec<_>, _>(|res| res.is_ok());
-        let (oks, errs) = (
-            oks.into_iter().filter_map(|ok| ok.ok()).collect::<Vec<_>>(),
-            errs.into_iter()
-                .filter_map(|err| match err {
-                    Ok(_) => None,
-                    Err(e) => Some(e),
-                })
-                .collect::<Vec<_>>(),
-        );
+    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))
         }

src/engine/strat_engine/backstore/crypt/initialize.rs

@@ -52,9 +52,10 @@ impl CryptInitializer {
         pool_uuid: PoolUuid,
         dev_uuid: DevUuid,
     ) -> CryptInitializer {
+        let dm_name = format_crypt_name(&dev_uuid).to_string();
         CryptInitializer {
             physical_path,
-            activation_name: format_crypt_name(&dev_uuid),
+            activation_name: dm_name,
             identifiers: StratisIdentifiers::new(pool_uuid, dev_uuid),
         }
     }

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<DmResult<DM>> = 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<bool> {
+    let mut did_something = false;
+    let devices = get_dm()
+        .list_devices()?
+        .into_iter()
+        .map(|(name, _, _)| name)
+        .collect::<Vec<_>>();
+    let dev_uuids = device_set
+        .iter()
+        .map(|(dev_uuid, _)| *dev_uuid)
+        .collect::<Vec<_>>();
+    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<DmNameBuf> {
+    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::<Vec<_>>();
+            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
+}

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<StopAction<PoolUuid>> {
         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 {

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<Vec<StratBlockDev>> for DeviceSet {
+    fn from(vec: Vec<StratBlockDev>) -> Self {
+        vec.into_iter()
+            .flat_map(|bd| {
+                let dev_uuid = bd.uuid();
+                Vec::<DeviceInfo>::from(bd)
+                    .into_iter()
+                    .map(move |info| (dev_uuid, LInfo::from(info)))
+            })
+            .collect::<DeviceSet>()
+    }
+}
+
 impl DeviceSet {
     /// Create a new, empty DeviceSet
     pub fn new() -> DeviceSet {