diff --git a/src/engine/strat_engine/engine.rs b/src/engine/strat_engine/engine.rs index 8c4fb8c504..ed7d5f40b3 100644 --- a/src/engine/strat_engine/engine.rs +++ b/src/engine/strat_engine/engine.rs @@ -888,6 +888,8 @@ mod test { path::Path, }; + use retry::{delay::Fixed, retry}; + use devicemapper::Sectors; use crate::engine::{ @@ -944,10 +946,28 @@ mod test { cmd::udev_settle().unwrap(); - assert!(!Path::new(&format!("/dev/stratis/{name1}/{fs_name1}")).exists()); - assert!(!Path::new(&format!("/dev/stratis/{name1}/{fs_name2}")).exists()); - assert!(Path::new(&format!("/dev/stratis/{name2}/{fs_name1}")).exists()); - assert!(Path::new(&format!("/dev/stratis/{name2}/{fs_name2}")).exists()); + assert_matches!( + retry(Fixed::from_millis(1000).take(3), || { + for symlink in [ + format!("/dev/stratis/{name1}/{fs_name1}"), + format!("/dev/stratis/{name1}/{fs_name2}"), + ] { + if Path::new(&symlink).exists() { + return Err(StratisError::Msg(format!("{} still exists", symlink))); + } + } + for symlink in [ + format!("/dev/stratis/{name2}/{fs_name1}"), + format!("/dev/stratis/{name2}/{fs_name2}"), + ] { + if !Path::new(&symlink).exists() { + return Err(StratisError::Msg(format!("{} does not exist", symlink))); + } + } + Ok(()) + }), + Ok(()) + ); { let mut pool = test_async!(engine.get_mut_pool(PoolIdentifier::Uuid(uuid1))).unwrap(); diff --git a/src/engine/strat_engine/tests/util.rs b/src/engine/strat_engine/tests/util.rs index 789f5f1fd4..9f74d5a991 100644 --- a/src/engine/strat_engine/tests/util.rs +++ b/src/engine/strat_engine/tests/util.rs @@ -6,11 +6,10 @@ use std::{ fs::File, io::Read, path::{Path, PathBuf}, - thread::sleep, - time::Duration, }; use nix::mount::{umount2, MntFlags}; +use retry::{delay::Fixed, retry}; use devicemapper::{DevId, DmFlags, DmName, DmNameBuf, DmOptions, DM}; @@ -84,70 +83,53 @@ use self::cleanup_errors::{Error, Result}; /// FIXME: Current implementation complicated by https://bugzilla.redhat.com/show_bug.cgi?id=1506287 pub fn dm_stratis_devices_remove() -> Result<()> { /// One iteration of removing devicemapper devices - fn one_iteration() -> Result<(bool, Vec)> { - let mut progress_made = false; - let mut remain = get_dm() + fn one_iteration() -> Result> { + get_dm() .list_devices() .map_err(|e| { Error::Chained( "failed while listing DM devices, giving up".into(), Box::new(e.into()), ) - })? - .iter() - .map(|d| &d.0) - .filter_map(|n| { - if !n.to_string().starts_with("stratis-1") - && !n.to_string().starts_with("stratis_fail_device") - && !n.to_string().starts_with("stratis_test_device") - { - None - } else { - match get_dm().device_remove(&DevId::Name(n), DmOptions::default()) { - Ok(_) => { - progress_made = true; + }) + .map(|devices| { + devices + .iter() + .map(|d| &d.0) + .filter_map(|n| { + if !n.to_string().starts_with("stratis-1") + && !n.to_string().starts_with("stratis_fail_device") + && !n.to_string().starts_with("stratis_test_device") + { + None + } else if let Err(retry::Error { error, .. }) = + retry(Fixed::from_millis(1000).take(3), || { + get_dm().device_remove(&DevId::Name(n), DmOptions::default()) + }) + { + debug!("Failed to remove device {}: {}", n.to_string(), error); + Some(n.to_owned()) + } else { None } - Err(_) => Some(n.to_owned()), - } - } + }) + .collect::>() }) - .collect::>(); - - // Retries if no progress has been made. - if !remain.is_empty() && !progress_made { - remain.retain(|name| { - for _ in 0..3 { - match get_dm().device_remove(&DevId::Name(name), DmOptions::default()) { - Ok(_) => { - progress_made = true; - return false; - } - Err(e) => { - debug!( - "Failed to remove device {} on retry: {}", - name.to_string(), - e - ); - sleep(Duration::from_secs(1)); - } - } - } - true - }); - } - - Ok((progress_made, remain)) } /// Do one iteration of removals until progress stops. Return remaining /// dm devices. fn do_while_progress() -> Result> { - let mut result = one_iteration()?; - while result.0 { - result = one_iteration()?; + let mut remaining = one_iteration()?; + while !remaining.is_empty() { + let temp = one_iteration()?; + if temp.len() < remaining.len() { + remaining = temp; + } else { + break; + } } - Ok(result.1) + Ok(remaining) } || -> Result<()> {