Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Modularize container creation #121

Merged
merged 12 commits into from
Jul 9, 2021
2 changes: 2 additions & 0 deletions .github/workflows/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ jobs:
run: ./build.sh
- name: Run tests
run: cargo test
- name: Run doc tests
run: cargo test --doc
Copy link
Member

Choose a reason for hiding this comment

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

👍

integration_tests:
runs-on: ubuntu-latest
steps:
Expand Down
19 changes: 12 additions & 7 deletions oci_spec/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
use nix::sys::stat::SFlag;
use std::collections::HashMap;
use std::fs::File;
use std::path::PathBuf;
use std::path::{Path, PathBuf};

use anyhow::{bail, Result};
use anyhow::{Context, Result, bail};
use serde::{Deserialize, Serialize};

#[derive(Serialize, Deserialize, Debug, Clone)]
Expand Down Expand Up @@ -607,13 +607,18 @@ pub struct Spec {
}

impl Spec {
pub fn load(path: &str) -> Result<Self> {
let file = File::open(path)?;
let mut spec: Spec = serde_json::from_reader(&file)?;
// FIME: It is fail if the caller isn't in the correct directory.
spec.root.path = std::fs::canonicalize(spec.root.path)?;
pub fn load<P: AsRef<Path>>(path: P) -> Result<Self> {
let path = path.as_ref();
let file = File::open(path).with_context(|| format!("failed to open {:?}", path))?;
let spec: Spec = serde_json::from_reader(&file)?;
Ok(spec)
}

pub fn canonicalize_rootfs(&mut self) -> Result<()> {
self.root.path = std::fs::canonicalize(&self.root.path)
.with_context(|| format!("failed to canonicalize {:?}", self.root.path))?;
Ok(())
}
}

#[cfg(feature = "proptests")]
Expand Down
20 changes: 10 additions & 10 deletions src/capabilities.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::command::Command;
use crate::command::Syscall;
use caps::*;

use anyhow::Result;
Expand All @@ -12,13 +12,13 @@ fn to_set(caps: &[LinuxCapabilityType]) -> CapsHashSet {
capabilities
}

pub fn reset_effective(command: &impl Command) -> Result<()> {
pub fn reset_effective(syscall: &impl Syscall) -> Result<()> {
log::debug!("reset all caps");
command.set_capability(CapSet::Effective, &caps::all())?;
syscall.set_capability(CapSet::Effective, &caps::all())?;
Ok(())
}

pub fn drop_privileges(cs: &LinuxCapabilities, command: &impl Command) -> Result<()> {
pub fn drop_privileges(cs: &LinuxCapabilities, syscall: &impl Syscall) -> Result<()> {
let all = caps::all();
log::debug!("dropping bounding capabilities to {:?}", cs.bounding);
for c in all.difference(&to_set(&cs.bounding)) {
Expand All @@ -31,11 +31,11 @@ pub fn drop_privileges(cs: &LinuxCapabilities, command: &impl Command) -> Result
}
}

command.set_capability(CapSet::Effective, &to_set(&cs.effective))?;
command.set_capability(CapSet::Permitted, &to_set(&cs.permitted))?;
command.set_capability(CapSet::Inheritable, &to_set(&cs.inheritable))?;
syscall.set_capability(CapSet::Effective, &to_set(&cs.effective))?;
syscall.set_capability(CapSet::Permitted, &to_set(&cs.permitted))?;
syscall.set_capability(CapSet::Inheritable, &to_set(&cs.inheritable))?;

if let Err(e) = command.set_capability(CapSet::Ambient, &to_set(&cs.ambient)) {
if let Err(e) = syscall.set_capability(CapSet::Ambient, &to_set(&cs.ambient)) {
log::error!("failed to set ambient capabilities: {}", e);
}
Ok(())
Expand All @@ -44,11 +44,11 @@ pub fn drop_privileges(cs: &LinuxCapabilities, command: &impl Command) -> Result
#[cfg(test)]
mod tests {
use super::*;
use crate::command::test::TestHelperCommand;
use crate::command::test::TestHelperSyscall;

#[test]
fn test_reset_effective() {
let test_command = TestHelperCommand::default();
let test_command = TestHelperSyscall::default();
assert!(reset_effective(&test_command).is_ok());
let set_capability_args: Vec<_> = test_command
.get_set_capability_args()
Expand Down
7 changes: 2 additions & 5 deletions src/cgroups/v1/pids.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::path::{Path};
use std::path::Path;

use anyhow::Result;

Expand All @@ -10,10 +10,7 @@ pub struct Pids {}
impl Controller for Pids {
type Resource = LinuxPids;

fn apply(
linux_resources: &LinuxResources,
cgroup_root: &Path,
) -> Result<()> {
fn apply(linux_resources: &LinuxResources, cgroup_root: &Path) -> Result<()> {
log::debug!("Apply pids cgroup config");

if let Some(pids) = &linux_resources.pids {
Expand Down
2 changes: 1 addition & 1 deletion src/cgroups/v1/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use std::{collections::HashMap, path::PathBuf};
use anyhow::{anyhow, Result};
use procfs::process::Process;

use super::{ControllerType, controller_type::CONTROLLERS};
use super::{controller_type::CONTROLLERS, ControllerType};

pub fn list_subsystem_mount_points() -> Result<HashMap<ControllerType, PathBuf>> {
let mut mount_paths = HashMap::with_capacity(CONTROLLERS.len());
Expand Down
14 changes: 8 additions & 6 deletions src/cgroups/v2/systemd_manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -261,9 +261,10 @@ mod tests {

#[test]
fn get_cgroups_path_works_with_a_complex_slice() -> Result<()> {
let cgroups_path =
SystemDCGroupManager::destructure_cgroups_path(PathBuf::from("test-a-b.slice:docker:foo"))
.expect("");
let cgroups_path = SystemDCGroupManager::destructure_cgroups_path(PathBuf::from(
"test-a-b.slice:docker:foo",
))
.expect("");

assert_eq!(
SystemDCGroupManager::construct_cgroups_path(cgroups_path)?,
Expand All @@ -275,9 +276,10 @@ mod tests {

#[test]
fn get_cgroups_path_works_with_a_simple_slice() -> Result<()> {
let cgroups_path =
SystemDCGroupManager::destructure_cgroups_path(PathBuf::from("machine.slice:libpod:foo"))
.expect("");
let cgroups_path = SystemDCGroupManager::destructure_cgroups_path(PathBuf::from(
"machine.slice:libpod:foo",
))
.expect("");

assert_eq!(
SystemDCGroupManager::construct_cgroups_path(cgroups_path)?,
Expand Down
8 changes: 4 additions & 4 deletions src/command/linux.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,14 @@ use nix::{sched::unshare, sys::stat::Mode};

use oci_spec::LinuxRlimit;

use super::Command;
use super::Syscall;
use crate::capabilities;

/// Empty structure to implement Command trait for
#[derive(Clone)]
pub struct LinuxCommand;
pub struct LinuxSyscall;

impl LinuxCommand {
impl LinuxSyscall {
unsafe fn from_raw_buf<'a, T>(p: *const c_char) -> T
where
T: From<&'a OsStr>,
Expand All @@ -46,7 +46,7 @@ impl LinuxCommand {
}
}

impl Command for LinuxCommand {
impl Syscall for LinuxSyscall {
/// To enable dynamic typing,
/// see https://doc.rust-lang.org/std/any/index.html for more information
fn as_any(&self) -> &dyn Any {
Expand Down
6 changes: 3 additions & 3 deletions src/command/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@
//! This provides a uniform interface for rest of Youki
//! to call syscalls required for container management

#[allow(clippy::module_inception)]
pub mod command;
pub mod linux;
#[allow(clippy::module_inception)]
pub mod syscall;
pub mod test;

pub use command::Command;
pub use syscall::Syscall;
10 changes: 5 additions & 5 deletions src/command/command.rs → src/command/syscall.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,11 @@ use nix::{

use oci_spec::LinuxRlimit;

use crate::command::{linux::LinuxCommand, test::TestHelperCommand};
use crate::command::{linux::LinuxSyscall, test::TestHelperSyscall};

/// This specifies various kernel/other functionalities required for
/// container management
pub trait Command {
pub trait Syscall {
fn as_any(&self) -> &dyn Any;
fn pivot_rootfs(&self, path: &Path) -> Result<()>;
fn set_ns(&self, rawfd: i32, nstype: CloneFlags) -> Result<()>;
Expand All @@ -28,10 +28,10 @@ pub trait Command {
fn get_pwuid(&self, uid: u32) -> Option<Arc<OsStr>>;
}

pub fn create_command() -> Box<dyn Command> {
pub fn create_syscall() -> Box<dyn Syscall> {
if cfg!(test) {
Box::new(TestHelperCommand::default())
Box::new(TestHelperSyscall::default())
} else {
Box::new(LinuxCommand)
Box::new(LinuxSyscall)
}
}
12 changes: 6 additions & 6 deletions src/command/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,26 +4,26 @@ use caps::{errors::CapsError, CapSet, CapsHashSet};
use nix::sched::CloneFlags;
use oci_spec::LinuxRlimit;

use super::Command;
use super::Syscall;

#[derive(Clone)]
pub struct TestHelperCommand {
pub struct TestHelperSyscall {
set_ns_args: RefCell<Vec<(i32, CloneFlags)>>,
unshare_args: RefCell<Vec<CloneFlags>>,
set_capability_args: RefCell<Vec<(CapSet, CapsHashSet)>>,
}

impl Default for TestHelperCommand {
impl Default for TestHelperSyscall {
fn default() -> Self {
TestHelperCommand {
TestHelperSyscall {
set_ns_args: RefCell::new(vec![]),
unshare_args: RefCell::new(vec![]),
set_capability_args: RefCell::new(vec![]),
}
}
}

impl Command for TestHelperCommand {
impl Syscall for TestHelperSyscall {
fn as_any(&self) -> &dyn Any {
self
}
Expand Down Expand Up @@ -66,7 +66,7 @@ impl Command for TestHelperCommand {
}
}

impl TestHelperCommand {
impl TestHelperSyscall {
pub fn get_setns_args(&self) -> Vec<(i32, CloneFlags)> {
self.set_ns_args.borrow_mut().clone()
}
Expand Down
130 changes: 130 additions & 0 deletions src/container/builder.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
use crate::command::linux::LinuxSyscall;
use std::path::PathBuf;

use super::{init_builder::InitContainerBuilder, tenant_builder::TenantContainerBuilder};
pub struct ContainerBuilder {
pub(super) container_id: String,

pub(super) root_path: PathBuf,

pub(super) syscall: LinuxSyscall,

pub(super) pid_file: Option<PathBuf>,

pub(super) console_socket: Option<PathBuf>,
}

/// Builder that can be used to configure the common properties of
/// either a init or a tenant container
///
/// # Example
///
/// ```no_run
/// use youki::container::builder::ContainerBuilder;
///
/// ContainerBuilder::new("74f1a4cb3801".to_owned())
/// .with_root_path("/run/containers/youki")
/// .with_pid_file("/var/run/docker.pid")
/// .with_console_socket("/var/run/docker/sock.tty")
/// .as_init("/var/run/docker/bundle")
/// .build();
/// ```
impl ContainerBuilder {
/// Generates the base configuration for a container which can be
/// transformed into either a init container or a tenant container
///
/// # Example
///
/// ```no_run
/// use youki::container::builder::ContainerBuilder;
///
/// let builder = ContainerBuilder::new("74f1a4cb3801".to_owned());
/// ```
pub fn new(container_id: String) -> Self {
let root_path = PathBuf::from("/run/youki");
Copy link
Member

Choose a reason for hiding this comment

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

It may be better to use Default Trait.

Copy link
Member

Choose a reason for hiding this comment

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

Also, it's better to use Default for other builders as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The default trait does not allow to specify arguments. I could do

ContainerBuilder::default()
.with_id(id)

instead of with new(id) and generate an id automatically if none is specified in with_id if you like. The TenantContainerBuilder and InitContainerBuilder are not intended to be instantiated directly but by calling as_tenant/is_tenant on the ContainerBuilder, so you do not really have a default value for them because you always have a builder that will be provided in new(...)

Copy link
Member

Choose a reason for hiding this comment

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

I was planning to use default() in new, and I thought default would be better to use in case there are more constructor functions of different types.
However, it's a small thing, so I think it's fine to prepare it when it's needed.
What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We only have one constructor function at the moment, so I think we should do a default implementation when we start to get multiple.


Self {
container_id,
root_path,
syscall: LinuxSyscall,
pid_file: None,
console_socket: None,
}
}

/// Transforms this builder into a tenant builder
/// # Example
///
/// ```no_run
/// # use youki::container::builder::ContainerBuilder;
///
/// ContainerBuilder::new("74f1a4cb3801".to_owned())
/// .as_tenant()
/// .with_container_command(vec!["sleep".to_owned(), "9001".to_owned()])
/// .build();
/// ```
#[allow(clippy::wrong_self_convention)]
pub fn as_tenant(self) -> TenantContainerBuilder {
TenantContainerBuilder::new(self)
}

/// Transforms this builder into an init builder
/// # Example
///
/// ```no_run
/// # use youki::container::builder::ContainerBuilder;
///
/// ContainerBuilder::new("74f1a4cb3801".to_owned())
/// .as_init("/var/run/docker/bundle")
/// .with_systemd(false)
/// .build();
/// ```
#[allow(clippy::wrong_self_convention)]
pub fn as_init<P: Into<PathBuf>>(self, bundle: P) -> InitContainerBuilder {
InitContainerBuilder::new(self, bundle.into())
}

/// Sets the root path which will be used to store the container state
/// # Example
///
/// ```no_run
/// # use youki::container::builder::ContainerBuilder;
///
/// ContainerBuilder::new("74f1a4cb3801".to_owned())
/// .with_root_path("/run/containers/youki");
/// ```
pub fn with_root_path<P: Into<PathBuf>>(mut self, path: P) -> Self {
self.root_path = path.into();
self
}

/// Sets the pid file which will be used to write the pid of the container
/// process
/// # Example
///
/// ```no_run
/// # use youki::container::builder::ContainerBuilder;
///
/// ContainerBuilder::new("74f1a4cb3801".to_owned())
/// .with_pid_file("/var/run/docker.pid");
/// ```
pub fn with_pid_file<P: Into<PathBuf>>(mut self, path: P) -> Self {
self.pid_file = Some(path.into());
self
}

/// Sets the console socket, which will be used to send the file descriptor
/// of the pseudoterminal
/// # Example
///
/// ```no_run
/// # use youki::container::builder::ContainerBuilder;
///
/// ContainerBuilder::new("74f1a4cb3801".to_owned())
/// .with_console_socket("/var/run/docker/sock.tty");
/// ```
pub fn with_console_socket<P: Into<PathBuf>>(mut self, path: P) -> Self {
self.console_socket = Some(path.into());
self
}
}
Loading