fix: mount retry and logging

Signed-off-by: Kaita Nakamura <[email protected]>

Author: z63d

crates/libcontainer/src/rootfs/mount.rs

@@ -2,6 +2,7 @@ use std::fs::{canonicalize, create_dir_all, OpenOptions};
 use std::mem;
 use std::os::unix::io::AsRawFd;
 use std::path::{Path, PathBuf};
+use std::time::Duration;
 #[cfg(feature = "v1")]
 use std::{borrow::Cow, collections::HashMap};
 
@@ -24,7 +25,16 @@ use super::symlink::SymlinkError;
 use super::utils::{parse_mount, MountOptionConfig};
 use crate::syscall::syscall::create_syscall;
 use crate::syscall::{linux, Syscall, SyscallError};
-use crate::utils::PathBufExt;
+use crate::utils::{retry, PathBufExt};
+
+const MAX_EBUSY_MOUNT_ATTEMPTS: u32 = 3;
+// runc has a retry interval of 100ms. We are following this.
+// https://github.com/opencontainers/runc/blob/v1.3.0/libcontainer/rootfs_linux.go#L1235
+#[cfg(not(test))]
+const MOUNT_RETRY_DELAY_MS: u64 = 100;
+// In tests, there is no need to delay, so set it to 0ms.
+#[cfg(test)]
+const MOUNT_RETRY_DELAY_MS: u64 = 0;
 
 #[derive(Debug, thiserror::Error)]
 pub enum MountError {
@@ -551,24 +561,35 @@ impl Mount {
                 .mount(Some(&*src), dest, typ, mount_option_config.flags, Some(&*d))
         {
             if let SyscallError::Nix(errno) = err {
-                if !matches!(errno, Errno::EINVAL) {
-                    tracing::error!("mount of {:?} failed. {}", m.destination(), errno);
+                if matches!(errno, Errno::EINVAL) {
+                    self.syscall.mount(
+                        Some(&*src),
+                        dest,
+                        typ,
+                        mount_option_config.flags,
+                        Some(&mount_option_config.data),
+                    )?;
+                } else if matches!(errno, Errno::EBUSY) {
+                    let mount_op = || -> std::result::Result<(), SyscallError> {
+                        self.syscall.mount(
+                            Some(&*src),
+                            dest,
+                            typ,
+                            mount_option_config.flags,
+                            Some(&*d),
+                        )
+                    };
+                    let delay = Duration::from_millis(MOUNT_RETRY_DELAY_MS);
+                    let retry_policy = |err: &SyscallError| -> bool {
+                        matches!(err, SyscallError::Nix(Errno::EBUSY))
+                    };
+                    retry(mount_op, MAX_EBUSY_MOUNT_ATTEMPTS - 1, delay, retry_policy)?;
+                } else {
                     return Err(err.into());
                 }
+            } else {
+                return Err(err.into());
             }
-
-            self.syscall
-                .mount(
-                    Some(&*src),
-                    dest,
-                    typ,
-                    mount_option_config.flags,
-                    Some(&mount_option_config.data),
-                )
-                .map_err(|err| {
-                    tracing::error!("failed to mount {src:?} to {dest:?}");
-                    err
-                })?;
         }
 
         if typ == Some("bind")
@@ -635,7 +656,7 @@ mod tests {
     use anyhow::{Context, Ok, Result};
 
     use super::*;
-    use crate::syscall::test::{MountArgs, TestHelperSyscall};
+    use crate::syscall::test::{ArgName, MountArgs, TestHelperSyscall};
 
     #[test]
     fn test_mount_into_container() -> Result<()> {
@@ -729,6 +750,102 @@ mod tests {
             assert_eq!(want, *got);
             assert_eq!(got.len(), 2);
         }
+        {
+            let m = Mount::new();
+            let mount = &SpecMountBuilder::default()
+                .destination(PathBuf::from("/tmp/retry"))
+                .typ("tmpfs")
+                .source(PathBuf::from("tmpfs"))
+                .build()?;
+            let mount_option_config = parse_mount(mount)?;
+
+            let syscall = m
+                .syscall
+                .as_any()
+                .downcast_ref::<TestHelperSyscall>()
+                .unwrap();
+            syscall.set_ret_err(ArgName::Mount, || {
+                Err(crate::syscall::SyscallError::Nix(nix::errno::Errno::EINVAL))
+            });
+            syscall.set_ret_err_times(ArgName::Mount, 1);
+
+            assert!(m
+                .mount_into_container(mount, tmp_dir.path(), &mount_option_config, None)
+                .is_ok());
+            assert_eq!(syscall.get_mount_args().len(), 1);
+        }
+        {
+            let m = Mount::new();
+            let mount = &SpecMountBuilder::default()
+                .destination(PathBuf::from("/tmp/retry"))
+                .typ("tmpfs")
+                .source(PathBuf::from("tmpfs"))
+                .build()?;
+            let mount_option_config = parse_mount(mount)?;
+
+            let syscall = m
+                .syscall
+                .as_any()
+                .downcast_ref::<TestHelperSyscall>()
+                .unwrap();
+            syscall.set_ret_err(ArgName::Mount, || {
+                Err(crate::syscall::SyscallError::Nix(nix::errno::Errno::EINVAL))
+            });
+            syscall.set_ret_err_times(ArgName::Mount, 2);
+
+            assert!(m
+                .mount_into_container(mount, tmp_dir.path(), &mount_option_config, None)
+                .is_err());
+            assert_eq!(syscall.get_mount_args().len(), 0);
+        }
+        {
+            let m = Mount::new();
+            let mount = &SpecMountBuilder::default()
+                .destination(PathBuf::from("/tmp/retry"))
+                .typ("tmpfs")
+                .source(PathBuf::from("tmpfs"))
+                .build()?;
+            let mount_option_config = parse_mount(mount)?;
+
+            let syscall = m
+                .syscall
+                .as_any()
+                .downcast_ref::<TestHelperSyscall>()
+                .unwrap();
+            syscall.set_ret_err(ArgName::Mount, || {
+                Err(crate::syscall::SyscallError::Nix(nix::errno::Errno::EBUSY))
+            });
+            syscall.set_ret_err_times(ArgName::Mount, MAX_EBUSY_MOUNT_ATTEMPTS as usize - 1);
+
+            assert!(m
+                .mount_into_container(mount, tmp_dir.path(), &mount_option_config, None)
+                .is_ok());
+            assert_eq!(syscall.get_mount_args().len(), 1);
+        }
+        {
+            let m = Mount::new();
+            let mount = &SpecMountBuilder::default()
+                .destination(PathBuf::from("/tmp/retry"))
+                .typ("tmpfs")
+                .source(PathBuf::from("tmpfs"))
+                .build()?;
+            let mount_option_config = parse_mount(mount)?;
+
+            let syscall = m
+                .syscall
+                .as_any()
+                .downcast_ref::<TestHelperSyscall>()
+                .unwrap();
+            syscall.set_ret_err(ArgName::Mount, || {
+                Err(crate::syscall::SyscallError::Nix(nix::errno::Errno::EBUSY))
+            });
+            syscall.set_ret_err_times(ArgName::Mount, MAX_EBUSY_MOUNT_ATTEMPTS as usize);
+
+            assert!(m
+                .mount_into_container(mount, tmp_dir.path(), &mount_option_config, None)
+                .is_err());
+            assert_eq!(syscall.get_mount_args().len(), 0);
+        }
 
         Ok(())
     }

crates/libcontainer/src/utils.rs

@@ -5,6 +5,7 @@ use std::fs::{self, DirBuilder, File};
 use std::os::linux::fs::MetadataExt;
 use std::os::unix::fs::DirBuilderExt;
 use std::path::{Component, Path, PathBuf};
+use std::time::Duration;
 
 use nix::sys::stat::Mode;
 use nix::sys::statfs;
@@ -282,6 +283,33 @@ pub fn validate_spec_for_new_user_ns(spec: &Spec) -> Result<(), LibcontainerErro
     Ok(())
 }
 
+// Generic retry function with delay and policy.
+// Retries the operation `op` up to `attempts` times if it fails.
+// Waits for `delay` duration between retries.
+// Only retries if the error satisfies the `policy` function.
+pub fn retry<F, T, E, P>(mut op: F, attempts: u32, delay: Duration, policy: P) -> Result<T, E>
+where
+    F: FnMut() -> Result<T, E>,
+    P: Fn(&E) -> bool,
+{
+    if attempts == 0 {
+        panic!("retry called with 0 attempts. Minimum attempts is 1.");
+    }
+    for attempt in 0..attempts {
+        match op() {
+            Ok(res) => return Ok(res),
+            Err(err) => {
+                if attempt + 1 < attempts && policy(&err) {
+                    std::thread::sleep(delay);
+                } else {
+                    return Err(err);
+                }
+            }
+        }
+    }
+    unreachable!("retry loop completed without returning a result.");
+}
+
 #[cfg(test)]
 mod tests {
     use anyhow::{bail, Result};