Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Implemented clone fallback when clone3 returns ENOSYS #2121

Closed
wants to merge 21 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
125 changes: 41 additions & 84 deletions Cargo.lock

Large diffs are not rendered by default.

31 changes: 18 additions & 13 deletions crates/libcgroups/src/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -316,65 +316,70 @@ pub enum CreateCgroupSetupError {
Systemd(#[from] systemd::manager::SystemdManagerError),
}

pub fn create_cgroup_manager<P: Into<PathBuf>>(
cgroup_path: P,
systemd_cgroup: bool,
container_name: &str,
#[derive(Clone)]
pub struct CgroupConfig {
pub cgroup_path: PathBuf,
pub systemd_cgroup: bool,
pub container_name: String,
}

pub fn create_cgroup_manager(
config: &CgroupConfig,
) -> Result<AnyCgroupManager, CreateCgroupSetupError> {
let cgroup_setup = get_cgroup_setup().map_err(|err| match err {
GetCgroupSetupError::WrappedIo(err) => CreateCgroupSetupError::WrappedIo(err),
GetCgroupSetupError::NonDefault => CreateCgroupSetupError::NonDefault,
GetCgroupSetupError::FailedToDetect => CreateCgroupSetupError::FailedToDetect,
})?;
let cgroup_path = cgroup_path.into();
let cgroup_path = config.cgroup_path.as_path();

match cgroup_setup {
CgroupSetup::Legacy | CgroupSetup::Hybrid => {
Ok(create_v1_cgroup_manager(cgroup_path)?.any())
}
CgroupSetup::Unified => {
// ref https://github.com/opencontainers/runtime-spec/blob/main/config-linux.md#cgroups-path
if cgroup_path.is_absolute() || !systemd_cgroup {
if cgroup_path.is_absolute() || !config.systemd_cgroup {
return Ok(create_v2_cgroup_manager(cgroup_path)?.any());
}
Ok(create_systemd_cgroup_manager(cgroup_path, container_name)?.any())
Ok(create_systemd_cgroup_manager(cgroup_path, config.container_name.as_str())?.any())
}
}
}

#[cfg(feature = "v1")]
fn create_v1_cgroup_manager(
cgroup_path: PathBuf,
cgroup_path: &Path,
) -> Result<v1::manager::Manager, v1::manager::V1ManagerError> {
tracing::info!("cgroup manager V1 will be used");
v1::manager::Manager::new(cgroup_path)
}

#[cfg(not(feature = "v1"))]
fn create_v1_cgroup_manager(
_cgroup_path: PathBuf,
_cgroup_path: &Path,
) -> Result<v1::manager::Manager, v1::manager::V1ManagerError> {
Err(v1::manager::V1ManagerError::NotEnabled)
}

#[cfg(feature = "v2")]
fn create_v2_cgroup_manager(
cgroup_path: PathBuf,
cgroup_path: &Path,
) -> Result<v2::manager::Manager, v2::manager::V2ManagerError> {
tracing::info!("cgroup manager V2 will be used");
v2::manager::Manager::new(DEFAULT_CGROUP_ROOT.into(), cgroup_path)
}

#[cfg(not(feature = "v2"))]
fn create_v2_cgroup_manager(
_cgroup_path: PathBuf,
_cgroup_path: &Path,
) -> Result<v2::manager::Manager, v2::manager::V2ManagerError> {
Err(v2::manager::V2ManagerError::NotEnabled)
}

#[cfg(feature = "systemd")]
fn create_systemd_cgroup_manager(
cgroup_path: PathBuf,
cgroup_path: &Path,
container_name: &str,
) -> Result<systemd::manager::Manager, systemd::manager::SystemdManagerError> {
if !systemd::booted() {
Expand All @@ -399,7 +404,7 @@ fn create_systemd_cgroup_manager(

#[cfg(not(feature = "systemd"))]
fn create_systemd_cgroup_manager(
_cgroup_path: PathBuf,
_cgroup_path: &Path,
_container_name: &str,
) -> Result<systemd::manager::Manager, systemd::manager::SystemdManagerError> {
Err(systemd::manager::SystemdManagerError::NotEnabled)
Expand Down
6 changes: 3 additions & 3 deletions crates/libcgroups/src/systemd/manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -176,11 +176,11 @@ pub enum SystemdManagerError {
impl Manager {
pub fn new(
root_path: PathBuf,
cgroups_path: PathBuf,
cgroups_path: &Path,
container_name: String,
use_system: bool,
) -> Result<Self, SystemdManagerError> {
let mut destructured_path = cgroups_path.as_path().try_into()?;
let mut destructured_path = cgroups_path.try_into()?;
ensure_parent_unit(&mut destructured_path, use_system);

let client = match use_system {
Expand All @@ -191,7 +191,7 @@ impl Manager {
let (cgroups_path, delegation_boundary) =
Self::construct_cgroups_path(&destructured_path, &client)?;
let full_path = root_path.join_safely(&cgroups_path)?;
let fs_manager = FsManager::new(root_path.clone(), cgroups_path.clone())?;
let fs_manager = FsManager::new(root_path.clone(), &cgroups_path)?;

Ok(Manager {
root_path,
Expand Down
4 changes: 2 additions & 2 deletions crates/libcgroups/src/v1/manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,10 +82,10 @@ pub enum V1ManagerError {

impl Manager {
/// Constructs a new cgroup manager with cgroups_path being relative to the root of the subsystem
pub fn new(cgroup_path: PathBuf) -> Result<Self, V1ManagerError> {
pub fn new(cgroup_path: &Path) -> Result<Self, V1ManagerError> {
let mut subsystems = HashMap::<CtrlType, PathBuf>::new();
for subsystem in CONTROLLERS {
if let Ok(subsystem_path) = Self::get_subsystem_path(&cgroup_path, subsystem) {
if let Ok(subsystem_path) = Self::get_subsystem_path(cgroup_path, subsystem) {
subsystems.insert(*subsystem, subsystem_path);
} else {
tracing::warn!("cgroup {} not supported on this system", subsystem);
Expand Down
6 changes: 3 additions & 3 deletions crates/libcgroups/src/v2/manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,12 +84,12 @@ pub struct Manager {
impl Manager {
/// Constructs a new cgroup manager with root path being the mount point
/// of a cgroup v2 fs and cgroup path being a relative path from the root
pub fn new(root_path: PathBuf, cgroup_path: PathBuf) -> Result<Self, V2ManagerError> {
let full_path = root_path.join_safely(&cgroup_path)?;
pub fn new(root_path: PathBuf, cgroup_path: &Path) -> Result<Self, V2ManagerError> {
let full_path = root_path.join_safely(cgroup_path)?;

Ok(Self {
root_path,
cgroup_path,
cgroup_path: cgroup_path.to_owned(),
full_path,
})
}
Expand Down
1 change: 0 additions & 1 deletion crates/libcontainer/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ libseccomp = { version = "0.3.0", optional=true }
serde = { version = "1.0", features = ["derive"] }
serde_json = "1.0"
rust-criu = "0.4.0"
clone3 = "0.2.3"
regex = "1.9.0"
thiserror = "1.0.43"
tracing = { version = "0.1.37", features = ["attributes"]}
Expand Down
3 changes: 2 additions & 1 deletion crates/libcontainer/src/channel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,13 @@ pub enum ChannelError {
#[error("channel connection broken")]
BrokenChannel,
}

#[derive(Clone)]
pub struct Receiver<T> {
receiver: RawFd,
phantom: PhantomData<T>,
}

#[derive(Clone)]
pub struct Sender<T> {
sender: RawFd,
phantom: PhantomData<T>,
Comment on lines +21 to 30
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this contains RawFd, is it fine to allow clone on this? Also, should we impose Clone on T as well (even if we are not storing that) ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clone RawFd

This is just a c_int, so this is safe to do. It is like pass the RawFd by value with copy. When across the fork/clone boundary, the file descriptor table is duplicated so existing fds works exactly the same.

T for Clone

I don't think we have to impose Clone to T. In fact, it is possible that T does not have Clone trait. As long as T is serializable, we can use the channel to send and receive. So T can be serializable but not clone-able. The PhantomData does not store T as you pointed out and it is simply there to signal ownership of T.

Expand Down
Loading