Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add rollback of Pool setup when starting a pool fails #3285

Merged
merged 4 commits into from
Apr 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/dbus_api/api/prop_conv.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
3 changes: 2 additions & 1 deletion src/engine/sim_engine/engine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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())
})
}
}
Expand Down
65 changes: 28 additions & 37 deletions src/engine/strat_engine/backstore/backstore.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -550,44 +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<Vec<StratBlockDev>> {
let mut devs = 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())?;
}
Vec::new()
}
};
devs.extend(self.data_tier.block_mgr.teardown()?);
Ok(devs)
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
Expand All @@ -605,6 +584,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
Expand Down Expand Up @@ -1241,7 +1232,7 @@ mod tests {
CacheDevStatus::Fail => panic!("cache is in a failed state"),
}

backstore.destroy().unwrap();
backstore.destroy(pool_uuid).unwrap();
}

#[test]
Expand Down Expand Up @@ -1322,7 +1313,7 @@ mod tests {

assert_ne!(backstore.device(), old_device);

backstore.destroy().unwrap();
backstore.destroy(pool_uuid).unwrap();
}

#[test]
Expand Down
32 changes: 13 additions & 19 deletions src/engine/strat_engine/backstore/blockdevmgr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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))
}
Expand Down
14 changes: 7 additions & 7 deletions src/engine/strat_engine/backstore/crypt/handle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -64,7 +64,7 @@ impl CryptHandle {
physical_path: DevicePath,
identifiers: StratisIdentifiers,
encryption_info: EncryptionInfo,
activation_name: String,
activation_name: DmNameBuf,
pool_name: Option<Name>,
) -> StratisResult<CryptHandle> {
let device = get_devno_from_path(&physical_path)?;
Expand All @@ -82,8 +82,8 @@ impl CryptHandle {
metadata_handle: CryptMetadataHandle,
) -> StratisResult<CryptHandle> {
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::<PathBuf>(),
)?;
Ok(CryptHandle {
Expand Down Expand Up @@ -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()
}

Expand Down Expand Up @@ -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"
);
Expand Down Expand Up @@ -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)
}
}
12 changes: 6 additions & 6 deletions src/engine/strat_engine/backstore/crypt/initialize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -43,7 +44,7 @@ use crate::{
pub struct CryptInitializer {
physical_path: DevicePath,
identifiers: StratisIdentifiers,
activation_name: String,
activation_name: DmNameBuf,
}

impl CryptInitializer {
Expand Down Expand Up @@ -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"
);
Expand All @@ -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)
}
Expand Down
41 changes: 0 additions & 41 deletions src/engine/strat_engine/backstore/crypt/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
)));
}
};
}
8 changes: 4 additions & 4 deletions src/engine/strat_engine/backstore/crypt/metadata_handle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

use std::path::Path;

use devicemapper::Device;
use devicemapper::{Device, DmName, DmNameBuf};

use crate::{
engine::{
Expand All @@ -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<Name>,
pub(super) device: Device,
}
Expand All @@ -33,7 +33,7 @@ impl CryptMetadataHandle {
physical_path: DevicePath,
identifiers: StratisIdentifiers,
encryption_info: EncryptionInfo,
activation_name: String,
activation_name: DmNameBuf,
pool_name: Option<Name>,
device: Device,
) -> Self {
Expand Down Expand Up @@ -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
}

Expand Down
4 changes: 2 additions & 2 deletions src/engine/strat_engine/backstore/crypt/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading