From d09c984567a478a02115117f721b8db9241434f4 Mon Sep 17 00:00:00 2001 From: Eric Fang Date: Sun, 4 Jun 2023 23:19:51 -0700 Subject: [PATCH] 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 --- .../src/process/container_init_process.rs | 1 - .../process/container_intermediate_process.rs | 5 +- crates/libcontainer/src/rootfs/device.rs | 52 ++++++++++---- crates/libcontainer/src/rootfs/rootfs.rs | 43 ++++++++--- crates/libcontainer/src/syscall/linux.rs | 71 ++++--------------- rust-toolchain.toml | 2 +- .../integration_test/src/utils/test_utils.rs | 4 +- .../test_framework/src/conditional_test.rs | 2 +- .../test_framework/src/test.rs | 2 +- .../test_framework/src/test_group.rs | 2 +- .../test_framework/src/test_manager.rs | 2 +- .../test_framework/src/testable.rs | 2 +- 12 files changed, 95 insertions(+), 93 deletions(-) diff --git a/crates/libcontainer/src/process/container_init_process.rs b/crates/libcontainer/src/process/container_init_process.rs index 9a6c3598..c807d646 100644 --- 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, 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 index 0d8ac0de..a9fc5d35 100644 --- 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 index dcd10711..31b0db9a 100644 --- 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 index c15aa725..fd3b92dd 100644 --- 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 index c425a63e..a338a561 100644 --- 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, group: Option) -> 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 index 8ee05a4c..8f98f44e 100644 --- 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 index b0c501e8..207a9f0d 100644 --- 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 index 0732bdbc..67c5a375 100644 --- 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 index 1aeaa65d..35dc93af 100644 --- 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 index 4b3f0eee..e61cd975 100644 --- 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 index ec188f86..f2c87ec9 100644 --- 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 index fd3f1746..2c90986b 100644 --- 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)]