do not log error in the syscall crate (#1973)

* do not log error for mount in specific cases * clean up the logs in the syscall module * update toolchain to rust 1.70 --------- Signed-off-by: yihuaf <[email protected]>
youki-dev · Jun 5, 2023 · d09c984 · d09c984
1 parent 53ba8d4
commit d09c984
Show file tree

Hide file tree

Showing 12 changed files with 95 additions and 93 deletions.
diff --git a/crates/libcontainer/src/process/container_init_process.rs b/crates/libcontainer/src/process/container_init_process.rs
@@ -216,7 +216,6 @@ fn masked_path(path: &Path, mount_label: &Option<String>, syscall: &dyn Syscall)
         match err {
             SyscallError::Nix(nix::errno::Errno::ENOENT) => {
                 // ignore error if path is not exist.
-                tracing::warn!("masked path {:?} not exist", path);
             }
             SyscallError::Nix(nix::errno::Errno::ENOTDIR) => {
                 let label = match mount_label {

diff --git a/crates/libcontainer/src/process/container_intermediate_process.rs b/crates/libcontainer/src/process/container_intermediate_process.rs
@@ -96,7 +96,10 @@ pub fn container_intermediate_process(
     let proc = spec.process().as_ref().ok_or(MissingSpecError::Process)?;
     if let Some(rlimits) = proc.rlimits() {
         for rlimit in rlimits {
-            command.set_rlimit(rlimit)?;
+            command.set_rlimit(rlimit).map_err(|err| {
+                tracing::error!(?err, ?rlimit, "failed to set rlimit");
+                err
+            })?;
         }
     }
 

diff --git a/crates/libcontainer/src/rootfs/device.rs b/crates/libcontainer/src/rootfs/device.rs
@@ -102,9 +102,9 @@ impl Device {
             )
             .map_err(|err| {
                 tracing::error!(
-                    "failed to mount bind dev {:?}: {}",
-                    full_container_path,
-                    err
+                    ?err,
+                    path = ?full_container_path,
+                    "failed to mount bind dev",
                 );
                 err
             })?;
@@ -122,17 +122,41 @@ impl Device {
 
         let full_container_path = create_container_dev_path(rootfs, dev)?;
 
-        self.syscall.mknod(
-            &full_container_path,
-            to_sflag(dev.typ()),
-            Mode::from_bits_truncate(dev.file_mode().unwrap_or(0)),
-            makedev(dev.major(), dev.minor()),
-        )?;
-        self.syscall.chown(
-            &full_container_path,
-            dev.uid().map(Uid::from_raw),
-            dev.gid().map(Gid::from_raw),
-        )?;
+        self.syscall
+            .mknod(
+                &full_container_path,
+                to_sflag(dev.typ()),
+                Mode::from_bits_truncate(dev.file_mode().unwrap_or(0)),
+                makedev(dev.major(), dev.minor()),
+            )
+            .map_err(|err| {
+                tracing::error!(
+                    ?err,
+                    path = ?full_container_path,
+                    major = ?dev.major(),
+                    minor = ?dev.minor(),
+                    "failed to mknod device"
+                );
+
+                err
+            })?;
+        self.syscall
+            .chown(
+                &full_container_path,
+                dev.uid().map(Uid::from_raw),
+                dev.gid().map(Gid::from_raw),
+            )
+            .map_err(|err| {
+                tracing::error!(
+                    path = ?full_container_path,
+                    ?err,
+                    uid = ?dev.uid(),
+                    gid = ?dev.gid(),
+                    "failed to chown device"
+                );
+
+                err
+            })?;
 
         Ok(())
     }

diff --git a/crates/libcontainer/src/rootfs/rootfs.rs b/crates/libcontainer/src/rootfs/rootfs.rs
@@ -38,7 +38,7 @@ impl RootFS {
         bind_devices: bool,
         cgroup_ns: bool,
     ) -> Result<()> {
-        tracing::debug!("Prepare rootfs: {:?}", rootfs);
+        tracing::debug!(?rootfs, "prepare rootfs");
         let mut flags = MsFlags::MS_REC;
         let linux = spec.linux().as_ref().ok_or(MissingSpecError::Linux)?;
 
@@ -52,20 +52,34 @@ impl RootFS {
         }
 
         self.syscall
-            .mount(None, Path::new("/"), None, flags, None)?;
+            .mount(None, Path::new("/"), None, flags, None)
+            .map_err(|err| {
+                tracing::error!(
+                    ?err,
+                    ?flags,
+                    "failed to change the mount propagation type of the root"
+                );
+
+                err
+            })?;
 
         let mounter = Mount::new();
 
         mounter.make_parent_mount_private(rootfs)?;
 
         tracing::debug!("mount root fs {:?}", rootfs);
-        self.syscall.mount(
-            Some(rootfs),
-            rootfs,
-            None,
-            MsFlags::MS_BIND | MsFlags::MS_REC,
-            None,
-        )?;
+        self.syscall
+            .mount(
+                Some(rootfs),
+                rootfs,
+                None,
+                MsFlags::MS_BIND | MsFlags::MS_REC,
+                None,
+            )
+            .map_err(|err| {
+                tracing::error!(?rootfs, ?err, "failed to bind mount rootfs");
+                err
+            })?;
 
         let global_options = MountOptions {
             root: rootfs,
@@ -108,9 +122,16 @@ impl RootFS {
         };
 
         if let Some(flags) = flags {
-            tracing::debug!("make root mount {:?}", flags);
             self.syscall
-                .mount(None, Path::new("/"), None, flags, None)?;
+                .mount(None, Path::new("/"), None, flags, None)
+                .map_err(|err| {
+                    tracing::error!(
+                        ?err,
+                        ?flags,
+                        "failed to adjust the mount propagation type of the root"
+                    );
+                    err
+                })?;
         }
 
         Ok(())

diff --git a/crates/libcontainer/src/syscall/linux.rs b/crates/libcontainer/src/syscall/linux.rs
@@ -349,14 +349,10 @@ impl Syscall for LinuxSyscall {
     /// Disassociate parts of execution context
     // see https://man7.org/linux/man-pages/man2/unshare.2.html for more information
     fn unshare(&self, flags: CloneFlags) -> Result<()> {
-        unshare(flags).map_err(|err| {
-            tracing::error!(?err, ?flags, "failed to unshare");
-            err
-        })?;
+        unshare(flags)?;
 
         Ok(())
     }
-
     /// Set capabilities for container process
     fn set_capability(&self, cset: CapSet, value: &CapsHashSet) -> Result<()> {
         match cset {
@@ -382,10 +378,7 @@ impl Syscall for LinuxSyscall {
 
     /// Sets hostname for process
     fn set_hostname(&self, hostname: &str) -> Result<()> {
-        sethostname(hostname).map_err(|err| {
-            tracing::error!(?hostname, "failed to set hostname");
-            err
-        })?;
+        sethostname(hostname)?;
         Ok(())
     }
 
@@ -396,15 +389,9 @@ impl Syscall for LinuxSyscall {
         let len = domainname.len();
         match unsafe { setdomainname(ptr, len) } {
             0 => Ok(()),
-            -1 => {
-                tracing::error!(?domainname, "failed to set domainname");
-                Err(nix::Error::last())
-            }
+            -1 => Err(nix::Error::last()),
 
-            _ => {
-                tracing::error!(?domainname, "failed to set domainname for unknown reason");
-                Err(nix::Error::UnknownErrno)
-            }
+            _ => Err(nix::Error::UnknownErrno),
         }?;
 
         Ok(())
@@ -425,14 +412,8 @@ impl Syscall for LinuxSyscall {
 
         match res {
             0 => Ok(()),
-            -1 => {
-                tracing::error!(?res, rlimit = ?rlimit.typ(), "failed to set rlimit");
-                Err(SyscallError::Nix(nix::Error::last()))
-            }
-            _ => {
-                tracing::error!(?res, rlimit = ?rlimit.typ(), "failed to set rlimit for unknown reason");
-                Err(SyscallError::Nix(nix::Error::UnknownErrno))
-            }
+            -1 => Err(SyscallError::Nix(nix::Error::last())),
+            _ => Err(SyscallError::Nix(nix::Error::UnknownErrno)),
         }?;
 
         Ok(())
@@ -473,10 +454,7 @@ impl Syscall for LinuxSyscall {
     }
 
     fn chroot(&self, path: &Path) -> Result<()> {
-        unistd::chroot(path).map_err(|err| {
-            tracing::error!(?err, ?path, "failed to chroot");
-            err
-        })?;
+        unistd::chroot(path)?;
 
         Ok(())
     }
@@ -489,41 +467,24 @@ impl Syscall for LinuxSyscall {
         flags: MsFlags,
         data: Option<&str>,
     ) -> Result<()> {
-        mount(source, target, fstype, flags, data).map_err(|err| {
-            tracing::error!(
-                "failed to mount {source:?} to {target:?} with fstype {fstype:?}, flags {flags:?}, data {data:?}: {err}",
-            );
-            err
-        })?;
-
+        mount(source, target, fstype, flags, data)?;
         Ok(())
     }
 
     fn symlink(&self, original: &Path, link: &Path) -> Result<()> {
-        symlink(original, link).map_err(|err| {
-            tracing::error!("failed to create symlink from {original:?} to {link:?}: {err}");
-            err
-        })?;
+        symlink(original, link)?;
 
         Ok(())
     }
 
     fn mknod(&self, path: &Path, kind: SFlag, perm: Mode, dev: u64) -> Result<()> {
-        mknod(path, kind, perm, dev).map_err(|errno| {
-            tracing::error!(
-                "failed to mknod {path:?}, kind {kind:?}, perm {perm:?}, dev {dev:?}: {errno}"
-            );
-            errno
-        })?;
+        mknod(path, kind, perm, dev)?;
 
         Ok(())
     }
 
     fn chown(&self, path: &Path, owner: Option<Uid>, group: Option<Gid>) -> Result<()> {
-        chown(path, owner, group).map_err(|errno| {
-            tracing::error!("failed to chown {path:?} to {owner:?}:{group:?}: {errno}");
-            errno
-        })?;
+        chown(path, owner, group)?;
 
         Ok(())
     }
@@ -552,16 +513,10 @@ impl Syscall for LinuxSyscall {
                         // kernel 5.11. If the kernel is older we emulate close_range in userspace.
                         Self::emulate_close_range(preserve_fds)
                     }
-                    e => {
-                        tracing::error!(errno = ?e, "failed to close_range");
-                        Err(SyscallError::Nix(e))
-                    }
+                    e => Err(SyscallError::Nix(e)),
                 }
             }
-            _ => {
-                tracing::error!("failed to close_range unknown failure");
-                Err(SyscallError::Nix(nix::errno::Errno::UnknownErrno))
-            }
+            _ => Err(SyscallError::Nix(nix::errno::Errno::UnknownErrno)),
         }?;
 
         Ok(())

diff --git a/rust-toolchain.toml b/rust-toolchain.toml
@@ -1,3 +1,3 @@
 [toolchain]
 profile="default"
-channel="1.69.0"
+channel="1.70.0"
diff --git a/tests/rust-integration-tests/integration_test/src/utils/test_utils.rs b/tests/rust-integration-tests/integration_test/src/utils/test_utils.rs
@@ -1,5 +1,5 @@
-///! Contains utility functions for testing
-///! Similar to https://github.com/opencontainers/runtime-tools/blob/master/validation/util/test.go
+//! Contains utility functions for testing
+//! Similar to https://github.com/opencontainers/runtime-tools/blob/master/validation/util/test.go
 use super::{generate_uuid, prepare_bundle, set_config};
 use super::{get_runtime_path, get_runtimetest_path};
 use anyhow::{anyhow, bail, Context, Result};

diff --git a/tests/rust-integration-tests/test_framework/src/conditional_test.rs b/tests/rust-integration-tests/test_framework/src/conditional_test.rs
@@ -1,4 +1,4 @@
-///! Contains definition for a tests which should be conditionally run
+//! Contains definition for a tests which should be conditionally run
 use crate::testable::{TestResult, Testable};
 
 // type aliases for test function signature

diff --git a/tests/rust-integration-tests/test_framework/src/test.rs b/tests/rust-integration-tests/test_framework/src/test.rs
@@ -1,4 +1,4 @@
-///! Contains definition for a simple and commonly usable test structure
+//! Contains definition for a simple and commonly usable test structure
 use crate::testable::{TestResult, Testable};
 
 // type alias for the test function

diff --git a/tests/rust-integration-tests/test_framework/src/test_group.rs b/tests/rust-integration-tests/test_framework/src/test_group.rs
@@ -1,4 +1,4 @@
-///! Contains structure for a test group
+//! Contains structure for a test group
 use crate::testable::{TestResult, Testable, TestableGroup};
 use crossbeam::thread;
 use std::collections::BTreeMap;

diff --git a/tests/rust-integration-tests/test_framework/src/test_manager.rs b/tests/rust-integration-tests/test_framework/src/test_manager.rs
@@ -1,4 +1,4 @@
-///! This exposes the main control wrapper to control the tests
+//! This exposes the main control wrapper to control the tests
 use crate::testable::{TestResult, TestableGroup};
 use anyhow::Result;
 use crossbeam::thread;

diff --git a/tests/rust-integration-tests/test_framework/src/testable.rs b/tests/rust-integration-tests/test_framework/src/testable.rs
@@ -1,6 +1,6 @@
+//! Contains Basic setup for testing, testable trait and its result type
 use std::fmt::Debug;
 
-///! Contains Basic setup for testing, testable trait and its result type
 use anyhow::{bail, Error, Result};
 
 #[derive(Debug)]