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

Remove unnecessary chdir #2780

Merged
merged 7 commits into from
May 15, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
3 changes: 1 addition & 2 deletions Vagrantfile.containerd2youki
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,7 @@ Vagrant.configure("2") do |config|
./script/setup/install-cni
./script/setup/install-critools
rm -rf /bin/runc /sbin/runc /usr/sbin/runc /usr/bin/runc

cp /vagrant/youki/youki /usr/bin/runc
ln -s /vagrant/youki/youki /usr/bin/runc
Copy link
Member Author

Choose a reason for hiding this comment

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

It'd be more beneficial because we don't need to copy again when we rebuild.

SHELL
end

Expand Down
7 changes: 1 addition & 6 deletions crates/libcontainer/src/container/container_start.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use crate::{
};

use super::{Container, ContainerStatus};
use nix::{sys::signal, unistd};
use nix::sys::signal;

impl Container {
/// Starts a previously created container
Expand Down Expand Up @@ -59,11 +59,6 @@ impl Container {
})?;
}

unistd::chdir(self.root.as_os_str()).map_err(|err| {
tracing::error!("failed to change directory to container root: {}", err);
LibcontainerError::OtherSyscall(err)
})?;

YJDoc2 marked this conversation as resolved.
Show resolved Hide resolved
let mut notify_socket = NotifySocket::new(self.root.join(NOTIFY_FILE));
notify_socket.notify_container_start()?;
self.set_status(ContainerStatus::Running)
Expand Down
9 changes: 0 additions & 9 deletions crates/libcontainer/src/container/init_builder.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use nix::unistd;
use oci_spec::runtime::Spec;
use std::{
fs,
Expand Down Expand Up @@ -61,14 +60,6 @@ impl InitContainerBuilder {
.set_systemd(self.use_systemd)
.set_annotations(spec.annotations().clone());

unistd::chdir(&container_dir).map_err(|err| {
tracing::error!(
?container_dir,
?err,
"failed to chdir into the container directory"
);
LibcontainerError::OtherSyscall(err)
})?;
let notify_path = container_dir.join(NOTIFY_FILE);
// convert path of root file system of the container to absolute path
let rootfs = fs::canonicalize(spec.root().as_ref().ok_or(MissingSpecError::Root)?.path())
Expand Down
3 changes: 1 addition & 2 deletions crates/libcontainer/src/container/tenant_builder.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use caps::Capability;
use nix::fcntl::OFlag;
use nix::unistd::{self, close, pipe2, read, Pid};
use nix::unistd::{close, pipe2, read, Pid};
use oci_spec::runtime::{
Capabilities as SpecCapabilities, Capability as SpecCapability, LinuxBuilder,
LinuxCapabilities, LinuxCapabilitiesBuilder, LinuxNamespace, LinuxNamespaceBuilder,
Expand Down Expand Up @@ -108,7 +108,6 @@ impl TenantContainerBuilder {

tracing::debug!("{:#?}", spec);

unistd::chdir(&container_dir).map_err(LibcontainerError::OtherSyscall)?;
let notify_path = Self::setup_notify_listener(&container_dir)?;
// convert path of root file system of the container to absolute path
let rootfs = fs::canonicalize(spec.root().as_ref().ok_or(MissingSpecError::Root)?.path())
Expand Down
82 changes: 35 additions & 47 deletions crates/libcontainer/src/tty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ pub fn setup_console_socket(
);
let csocketfd = match socket::connect(
csocketfd.as_raw_fd(),
&socket::UnixAddr::new(socket_name).map_err(|err| TTYError::InvalidSocketName {
&socket::UnixAddr::new(linked.as_path()).map_err(|err| TTYError::InvalidSocketName {
source: err,
socket_name: socket_name.to_string(),
})?,
Expand Down Expand Up @@ -165,84 +165,72 @@ fn connect_stdio(stdin: &RawFd, stdout: &RawFd, stderr: &RawFd) -> Result<()> {
mod tests {
use super::*;

use anyhow::Result;
use anyhow::{Ok, Result};
use serial_test::serial;
use std::env;
use std::fs::{self, File};
use std::fs::File;
use std::os::unix::net::UnixListener;
use std::path::PathBuf;

const CONSOLE_SOCKET: &str = "console-socket";

fn setup() -> Result<(tempfile::TempDir, PathBuf, PathBuf)> {
let testdir = tempfile::tempdir()?;
let rundir_path = Path::join(testdir.path(), "run");
fs::create_dir(&rundir_path)?;
let socket_path = Path::new(&rundir_path).join("socket");
let _ = File::create(&socket_path);
env::set_current_dir(&testdir)?;
Ok((testdir, rundir_path, socket_path))
}

YJDoc2 marked this conversation as resolved.
Show resolved Hide resolved
#[test]
#[serial]
fn test_setup_console_socket() {
let init = setup();
assert!(init.is_ok());
let (testdir, rundir_path, socket_path) = init.unwrap();
let lis = UnixListener::bind(Path::join(testdir.path(), "console-socket"));
fn test_setup_console_socket() -> Result<()> {
let testdir = tempfile::tempdir()?;
let socket_path = Path::join(testdir.path(), "test-socket");
let lis = UnixListener::bind(&socket_path);
assert!(lis.is_ok());
let fd = setup_console_socket(&rundir_path, &socket_path, CONSOLE_SOCKET);
assert!(fd.is_ok());
assert_ne!(fd.unwrap().as_raw_fd(), -1);
let fd = setup_console_socket(testdir.path(), &socket_path, CONSOLE_SOCKET)?;
assert_ne!(fd.as_raw_fd(), -1);
Ok(())
}

#[test]
#[serial]
fn test_setup_console_socket_empty() {
let init = setup();
assert!(init.is_ok());
let (_testdir, rundir_path, socket_path) = init.unwrap();
let fd = setup_console_socket(&rundir_path, &socket_path, CONSOLE_SOCKET);
assert!(fd.is_ok());
assert_eq!(fd.unwrap().as_raw_fd(), -1);
fn test_setup_console_socket_empty() -> Result<()> {
let testdir = tempfile::tempdir()?;
let socket_path = Path::join(testdir.path(), "test-socket");
let fd = setup_console_socket(testdir.path(), &socket_path, CONSOLE_SOCKET)?;
assert_eq!(fd.as_raw_fd(), -1);
Ok(())
}

#[test]
#[serial]
fn test_setup_console_socket_invalid() {
let init = setup();
assert!(init.is_ok());
let (testdir, rundir_path, socket_path) = init.unwrap();
fn test_setup_console_socket_invalid() -> Result<()> {
let testdir = tempfile::tempdir()?;
let socket_path = Path::join(testdir.path(), "test-socket");
let _socket = File::create(Path::join(testdir.path(), "console-socket"));
assert!(_socket.is_ok());
let fd = setup_console_socket(&rundir_path, &socket_path, CONSOLE_SOCKET);
let fd = setup_console_socket(testdir.path(), &socket_path, CONSOLE_SOCKET);
assert!(fd.is_err());

Ok(())
}

#[test]
#[serial]
fn test_setup_console() {
let init = setup();
fn test_setup_console() -> Result<()> {
let testdir = tempfile::tempdir()?;
let socket_path = Path::join(testdir.path(), "test-socket");
// duplicate the existing std* fds
// we need to restore them later, and we cannot simply store them
// as they themselves get modified in setup_console
let old_stdin: RawFd = nix::unistd::dup(StdIO::Stdin.into()).unwrap();
let old_stdout: RawFd = nix::unistd::dup(StdIO::Stdout.into()).unwrap();
let old_stderr: RawFd = nix::unistd::dup(StdIO::Stderr.into()).unwrap();
let old_stdin: RawFd = nix::unistd::dup(StdIO::Stdin.into())?;
let old_stdout: RawFd = nix::unistd::dup(StdIO::Stdout.into())?;
let old_stderr: RawFd = nix::unistd::dup(StdIO::Stderr.into())?;

assert!(init.is_ok());
let (testdir, rundir_path, socket_path) = init.unwrap();
let lis = UnixListener::bind(Path::join(testdir.path(), "console-socket"));
let lis = UnixListener::bind(&socket_path);
assert!(lis.is_ok());
let fd = setup_console_socket(&rundir_path, &socket_path, CONSOLE_SOCKET);
let fd = setup_console_socket(testdir.path(), &socket_path, CONSOLE_SOCKET);
let status = setup_console(&fd.unwrap());

// restore the original std* before doing final assert
dup2(old_stdin, StdIO::Stdin.into()).unwrap();
dup2(old_stdout, StdIO::Stdout.into()).unwrap();
dup2(old_stderr, StdIO::Stderr.into()).unwrap();
dup2(old_stdin, StdIO::Stdin.into())?;
dup2(old_stdout, StdIO::Stdout.into())?;
dup2(old_stderr, StdIO::Stderr.into())?;

assert!(status.is_ok());

Ok(())
}
}
4 changes: 4 additions & 0 deletions justfile
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,10 @@ containerd-test: youki-dev
VAGRANT_VAGRANTFILE=Vagrantfile.containerd2youki vagrant up
VAGRANT_VAGRANTFILE=Vagrantfile.containerd2youki vagrant provision --provision-with test

# run containerd integration tests
clean-containerd-test:
VAGRANT_VAGRANTFILE=Vagrantfile.containerd2youki vagrant destroy

[private]
kind-cluster: bin-kind
#!/usr/bin/env bash
Expand Down
Loading