Merge pull request #3738 from mulkieran/use-retry-in-test

Use retry in test

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

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<DmNameBuf>)> {
-        let mut progress_made = false;
-        let mut remain = get_dm()
+    fn one_iteration() -> Result<Vec<DmNameBuf>> {
+        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::<Vec<_>>()
             })
-            .collect::<Vec<_>>();
-
-        // 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<Vec<DmNameBuf>> {
-        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<()> {