From bb3309afe3e54e28dff621b373af4543cb4a90d5 Mon Sep 17 00:00:00 2001 From: Alberto Faria Date: Fri, 12 Apr 2024 21:23:29 +0100 Subject: [PATCH 1/8] create: Always require absolute paths for custom options Signed-off-by: Alberto Faria --- docs/2-podman-docker.md | 15 ++---- src/commands/create/custom_opts.rs | 80 ++++++++++++------------------ tests/run.rs | 31 ++---------- 3 files changed, 39 insertions(+), 87 deletions(-) diff --git a/docs/2-podman-docker.md b/docs/2-podman-docker.md index 228946f..33edc6f 100644 --- a/docs/2-podman-docker.md +++ b/docs/2-podman-docker.md @@ -106,9 +106,6 @@ this and do other first-boot customization, you can provide a [cloud-init] NoCloud configuration to the VM by passing in the non-standard option `--cloud-init` *after* the image specification: -> For this command to work with Docker, you must provide an absolute path to -> `--cloud-init`. - ```console $ ls examples/cloud-init/config/ meta-data user-data vendor-data @@ -117,7 +114,7 @@ $ podman run \ --runtime crun-vm \ -it --rm \ quay.io/containerdisks/fedora:39 \ - --cloud-init examples/cloud-init/config + --cloud-init ~/examples/cloud-init/config # path must be absolute ``` You should now be able to log in with the default `fedora` username and password @@ -139,15 +136,12 @@ $ podman run \ Similarly, you can provide an [Ignition] configuration to the VM by passing in the `--ignition` option: -> For this command to work with Docker, you must provide an absolute path to -> `--ignition`. - ```console $ podman run \ --runtime crun-vm \ -it --rm \ quay.io/crun-vm/example-fedora-coreos:39 \ - --ignition examples/ignition/config.ign + --ignition ~/examples/ignition/config.ign # path must be absolute ``` You should now be able to log in with the default `core` username and password @@ -312,16 +306,13 @@ option also allows you specify a regular file as the source, and the source may be in any disk format known to QEMU (*e.g.*, raw, qcow2; when using `--device`, raw format is assumed): -> For this command to work with Docker, you must provide absolute paths to -> `--blockdev`. - ```console $ podman run \ --runtime crun-vm \ -it --rm \ quay.io/containerdisks/fedora:39 \ --password pass \ - --blockdev source=my-disk.qcow2,target=/home/fedora/my-disk,format=qcow2 + --blockdev source=~/my-disk.qcow2,target=/home/fedora/my-disk,format=qcow2 # paths must be absolute ``` ## Advanced options diff --git a/src/commands/create/custom_opts.rs b/src/commands/create/custom_opts.rs index 3c43c1e..79bc455 100644 --- a/src/commands/create/custom_opts.rs +++ b/src/commands/create/custom_opts.rs @@ -110,57 +110,39 @@ impl CustomOptions { Ok(path_in_host) } - match env { - RuntimeEnv::Docker => { - // Docker doesn't run the runtime with the same working directory as the process - // that launched `docker-run`, so we require custom option paths to be absolute. - // - // TODO: There must be a better way... - ensure!( - all_are_absolute(options.blockdev.iter().flat_map(|b| [&b.source, &b.target])) - && all_are_absolute(&options.cloud_init) - && all_are_absolute(&options.ignition) - && all_are_absolute(&options.merge_libvirt_xml), - concat!( - "paths specified using --blockdev, --cloud-init, --ignition, or", - " --merge-libvirt-xml must be absolute when using crun-vm as a Docker", - " runtime", - ), - ); + // Docker doesn't run the runtime with the same working directory as the process + // that launched `docker-run`. Similarly, custom option paths in Kubernetes refer to + // paths in the container/VM, and there isn't a reasonable notion of what the + // current directory is. We thus simply always require custom option paths to be + // absolute. + ensure!( + all_are_absolute(options.blockdev.iter().flat_map(|b| [&b.source, &b.target])) + && all_are_absolute(&options.cloud_init) + && all_are_absolute(&options.ignition) + && all_are_absolute(&options.merge_libvirt_xml), + concat!( + "paths specified using --blockdev, --cloud-init, --ignition, or", + " --merge-libvirt-xml must be absolute", + ), + ); + + if env == RuntimeEnv::Kubernetes { + for blockdev in &mut options.blockdev { + blockdev.source = path_in_container_into_path_in_host(spec, &blockdev.source)?; + blockdev.target = path_in_container_into_path_in_host(spec, &blockdev.target)?; + } + + if let Some(path) = &mut options.cloud_init { + *path = path_in_container_into_path_in_host(spec, &path)?; } - RuntimeEnv::Kubernetes => { - // Custom option paths in Kubernetes refer to paths in the container/VM, and there - // isn't a reasonable notion of what the current directory is. - ensure!( - all_are_absolute(options.blockdev.iter().flat_map(|b| [&b.source, &b.target])) - && all_are_absolute(&options.cloud_init) - && all_are_absolute(&options.ignition) - && all_are_absolute(&options.merge_libvirt_xml), - concat!( - "paths specified using --blockdev, --cloud-init, --ignition, or", - " --merge-libvirt-xml must be absolute when using crun-vm as a", - " Kubernetes runtime", - ), - ); - - for blockdev in &mut options.blockdev { - blockdev.source = path_in_container_into_path_in_host(spec, &blockdev.source)?; - blockdev.target = path_in_container_into_path_in_host(spec, &blockdev.target)?; - } - - if let Some(path) = &mut options.cloud_init { - *path = path_in_container_into_path_in_host(spec, &path)?; - } - - if let Some(path) = &mut options.ignition { - *path = path_in_container_into_path_in_host(spec, &path)?; - } - - for path in &mut options.merge_libvirt_xml { - *path = path_in_container_into_path_in_host(spec, &path)?; - } + + if let Some(path) = &mut options.ignition { + *path = path_in_container_into_path_in_host(spec, &path)?; + } + + for path in &mut options.merge_libvirt_xml { + *path = path_in_container_into_path_in_host(spec, &path)?; } - RuntimeEnv::Other => {} } Ok(options) diff --git a/tests/run.rs b/tests/run.rs index 0b284cf..0a39278 100644 --- a/tests/run.rs +++ b/tests/run.rs @@ -19,16 +19,7 @@ fn simple_test_case(image: &str, home_dir: &str) -> TestCase { } } -fn complex_test_case( - image: &str, - home_dir: &str, - cloud_init_and_ignition_prefix: &str, -) -> TestCase { - let cloud_init_and_ignition_prefix = match cloud_init_and_ignition_prefix { - "" => "".to_string(), - prefix => format!("{prefix}/"), - }; - +fn complex_test_case(image: &str, home_dir: &str) -> TestCase { TestCase { run_args: vec![ "-h=my-test-vm".to_string(), @@ -36,8 +27,8 @@ fn complex_test_case( format!("-v=./README.md:{home_dir}/README.md:z,ro"), // "ro" is so qemu uses shared lock format!("--mount=type=tmpfs,dst={home_dir}/tmp"), image.to_string(), - format!("--cloud-init={cloud_init_and_ignition_prefix}examples/cloud-init/config"), - format!("--ignition={cloud_init_and_ignition_prefix}examples/ignition/config.ign"), + format!("--cloud-init={REPO_PATH}/examples/cloud-init/config"), + format!("--ignition={REPO_PATH}/examples/ignition/config.ign"), ], exec_user: Utf8Path::new(home_dir).file_name().unwrap().to_string(), test_script: format!( @@ -63,20 +54,8 @@ fn complex_test_case( simple_test_case("quay.io/containerdisks/fedora:39", "/home/fedora"), simple_test_case("quay.io/crun-vm/example-fedora-coreos:39", "/var/home/core"), - complex_test_case("quay.io/containerdisks/fedora:39", "/home/fedora", REPO_PATH), - complex_test_case("quay.io/crun-vm/example-fedora-coreos:39", "/var/home/core", REPO_PATH), - ] -)] -#[test_matrix( - // engines - [ - Engine::Podman, - ], - - // cases - [ - complex_test_case("quay.io/containerdisks/fedora:39", "/home/fedora", ""), - complex_test_case("quay.io/crun-vm/example-fedora-coreos:39", "/var/home/core", ""), + complex_test_case("quay.io/containerdisks/fedora:39", "/home/fedora"), + complex_test_case("quay.io/crun-vm/example-fedora-coreos:39", "/var/home/core"), ] )] fn test_run(engine: Engine, case: TestCase) { From 407bd2f41588217032dc24cdbb80f396fea250cf Mon Sep 17 00:00:00 2001 From: Alberto Faria Date: Sat, 13 Apr 2024 13:55:40 +0100 Subject: [PATCH 2/8] create: Use CPU mode host-passthrough It can sometimes be more performant than host-model. Signed-off-by: Alberto Faria --- src/commands/create/domain.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/commands/create/domain.rs b/src/commands/create/domain.rs index 3a21446..fbe2887 100644 --- a/src/commands/create/domain.rs +++ b/src/commands/create/domain.rs @@ -38,7 +38,7 @@ fn generate( s(&mut w, "domain", &[("type", "kvm")], |w| { st(w, "name", &[], "domain")?; - se(w, "cpu", &[("mode", "host-model")])?; + se(w, "cpu", &[("mode", "host-passthrough")])?; let vcpus = get_vcpu_count(spec).to_string(); if let Some(cpu_set) = get_cpu_set(spec) { st(w, "vcpu", &[("cpuset", cpu_set.as_str())], vcpus.as_str())?; From fd17070d5d6e99c65dcb3cc8b622bdd2d72414e1 Mon Sep 17 00:00:00 2001 From: Alberto Faria Date: Sat, 13 Apr 2024 18:35:29 +0100 Subject: [PATCH 3/8] create: Make overlayfs/bind mounts work on all engines after a container restart Signed-off-by: Alberto Faria --- src/commands/create/mod.rs | 156 ++++++++++++++++++++++++------------- src/util.rs | 97 ++++++++++++++++++----- 2 files changed, 182 insertions(+), 71 deletions(-) diff --git a/src/commands/create/mod.rs b/src/commands/create/mod.rs index 5504a65..83e2c00 100644 --- a/src/commands/create/mod.rs +++ b/src/commands/create/mod.rs @@ -5,7 +5,8 @@ mod domain; mod first_boot; mod runtime_env; -use std::fs::{self, Permissions}; +use std::fs::{self, File, Permissions}; +use std::io::ErrorKind; use std::os::unix::fs::{FileTypeExt, MetadataExt, PermissionsExt}; use std::path::Path; use std::process::Command; @@ -21,8 +22,9 @@ use crate::commands::create::first_boot::FirstBootConfig; use crate::commands::create::runtime_env::RuntimeEnv; use crate::crun::crun_create; use crate::util::{ - bind_mount_dir_with_different_context, bind_mount_file, create_overlay_vm_image, - find_single_file_in_dirs, set_file_context, SpecExt, VmImageInfo, + bind_mount_dir_read_only_with_different_context, bind_mount_dir_with_different_context, + bind_mount_file, create_overlay_vm_image, find_single_file_in_dirs, is_mountpoint, + set_file_context, SpecExt, VmImageInfo, }; pub fn create(global_args: &liboci_cli::GlobalOpts, args: &liboci_cli::Create) -> Result<()> { @@ -30,14 +32,29 @@ pub fn create(global_args: &liboci_cli::GlobalOpts, args: &liboci_cli::Create) - let config_path = bundle_path.join("config.json"); let mut spec = oci_spec::runtime::Spec::load(&config_path)?; + + // We include container_id in the path to ensure no overlap with the user container's contents. let original_root_path = spec.root_path()?.to_path_buf(); + let new_root_path = original_root_path.join(format!("crun-vm-root-{}", args.container_id)); + fs::create_dir_all(&new_root_path)?; + + let private_root_path = original_root_path.join(format!("crun-vm-priv-{}", args.container_id)); + fs::create_dir_all(&private_root_path)?; + let runtime_env = RuntimeEnv::current(&spec, &original_root_path)?; let custom_options = CustomOptions::from_spec(&spec, runtime_env)?; - set_up_container_root(&mut spec, bundle_path, &custom_options)?; - let base_vm_image_info = - set_up_vm_image(&spec, bundle_path, &original_root_path, &custom_options)?; + set_up_container_root(&mut spec, &new_root_path, &custom_options)?; + let is_first_create = is_first_create(&spec)?; + + let base_vm_image_info = set_up_vm_image( + &spec, + &original_root_path, + &private_root_path, + &custom_options, + is_first_create, + )?; let mut mounts = Mounts::default(); set_up_mounts(&mut spec, &mut mounts)?; @@ -47,8 +64,10 @@ pub fn create(global_args: &liboci_cli::GlobalOpts, args: &liboci_cli::Create) - set_up_extra_container_mounts_and_devices(&mut spec)?; set_up_security(&mut spec); - set_up_first_boot_config(&spec, &mounts, &custom_options, runtime_env)?; - set_up_libvirt_domain_xml(&spec, &base_vm_image_info, &mounts, &custom_options)?; + if is_first_create { + set_up_first_boot_config(&spec, &mounts, &custom_options, runtime_env)?; + set_up_libvirt_domain_xml(&spec, &base_vm_image_info, &mounts, &custom_options)?; + } adjust_container_rlimits_and_resources(&mut spec); @@ -60,28 +79,43 @@ pub fn create(global_args: &liboci_cli::GlobalOpts, args: &liboci_cli::Create) - Ok(()) } +fn is_first_create(spec: &oci_spec::runtime::Spec) -> Result { + let path = spec.root_path()?.join("crun-vm/create-ran"); + + let error = File::options() + .read(true) + .write(true) + .create_new(true) + .open(path) + .err(); + + match error { + Some(e) if e.kind() == ErrorKind::AlreadyExists => Ok(false), + Some(e) => Err(e.into()), + None => Ok(true), + } +} + fn set_up_container_root( spec: &mut oci_spec::runtime::Spec, - bundle_path: &Utf8Path, + new_root_path: &Utf8Path, custom_options: &CustomOptions, ) -> Result<()> { // create root directory spec.set_root(Some( oci_spec::runtime::RootBuilder::default() - .path(bundle_path.join("crun-vm-root")) + .path(new_root_path) .readonly(false) .build() .unwrap(), )); - fs::create_dir_all(spec.root_path()?)?; - if let Some(context) = spec.mount_label() { // the directory we're using as the root for the container is not the one that podman // prepared for us, so we need to set its context ourselves to prevent SELinux from getting // angry at us - set_file_context(spec.root_path()?, context)?; + set_file_context(new_root_path, context)?; } // set up container scripts @@ -91,7 +125,7 @@ fn set_up_container_root( struct Scripts; for path in Scripts::iter() { - let path_in_host = spec.root_path()?.join("crun-vm").join(path.as_ref()); + let path_in_host = new_root_path.join("crun-vm").join(path.as_ref()); fs::create_dir_all(path_in_host.parent().unwrap())?; let file = Scripts::get(&path).unwrap(); @@ -119,9 +153,10 @@ fn set_up_container_root( fn set_up_vm_image( spec: &oci_spec::runtime::Spec, - bundle_path: &Utf8Path, original_root_path: &Utf8Path, + private_root_path: &Utf8Path, custom_options: &CustomOptions, + is_first_create: bool, ) -> Result { // where inside the container to look for the VM image const VM_IMAGE_SEARCH_PATHS: [&str; 2] = ["./", "disk/"]; @@ -136,54 +171,63 @@ fn set_up_vm_image( // mount user-provided VM image file into container + // TODO: Can we assume the container engine will always clean up all our mounts, since they're + // under the container's root? + let mirror_vm_image_path_in_container = Utf8Path::new("crun-vm/image").join(vm_image_path_in_host.file_name().unwrap()); let mirror_vm_image_path_in_host = spec.root_path()?.join(&mirror_vm_image_path_in_container); let mirror_vm_image_path_in_container = Utf8Path::new("/").join(mirror_vm_image_path_in_container); - let private_dir = if custom_options.persistent { + if custom_options.persistent { let vm_image_dir_path = vm_image_path_in_host.parent().unwrap(); let vm_image_dir_name = vm_image_dir_path.file_name().unwrap(); - let overlay_private_dir_name = format!(".crun-vm.{}.tmp", vm_image_dir_name); - let overlay_private_dir_path = vm_image_dir_path - .parent() - .unwrap() - .join(overlay_private_dir_name); - - overlay_private_dir_path - } else { - bundle_path.join("crun-vm-vm-image-overlayfs") - }; - - // We may need to change the VM image context to actually be able to access it, but we don't - // want to change the user's original image file and also don't want to do a full data copy, so - // we use an overlayfs mount, which allows us to expose the same file with a different context. - // - // TODO: Clean up `private_dir` when VM is terminated (would be best-effort, but better than - // nothing). - bind_mount_dir_with_different_context( - vm_image_path_in_host.parent().unwrap(), - mirror_vm_image_path_in_host.parent().unwrap(), - spec.mount_label(), - custom_options.persistent, - private_dir, - )?; - - let mut vm_image_info = VmImageInfo::of(&mirror_vm_image_path_in_host)?; + // Mount overlayfs to expose the user's VM image file with a different SELinux context so we + // can always access it, using the file's parent as the upperdir so that writes still + // propagate to it. + + if !is_mountpoint(mirror_vm_image_path_in_host.parent().unwrap())? { + let scratch_dir_path = vm_image_dir_path + .parent() + .unwrap() + .join(format!(".crun-vm.{}.tmp", vm_image_dir_name)); + + bind_mount_dir_with_different_context( + vm_image_path_in_host.parent().unwrap(), + mirror_vm_image_path_in_host.parent().unwrap(), + spec.mount_label(), + true, + scratch_dir_path, + )?; + } - if custom_options.persistent { - // We want to propagate writes but not removal, so that the user's file isn't deleted by - // Podman on cleanup, so we bind mount it on top of itself. + // Prevent the container engine from deleting the user's actual VM image file by mounting it + // on top of itself under our overlayfs mount. bind_mount_file(&mirror_vm_image_path_in_host, &mirror_vm_image_path_in_host)?; + let mut vm_image_info = VmImageInfo::of(&mirror_vm_image_path_in_host)?; vm_image_info.path = mirror_vm_image_path_in_container; + + Ok(vm_image_info) } else { - // The overlayfs mount already isolates the user's original image file from writes, but to - // ensure that we get copy-on-write and page cache sharing even when the underlying file - // system doesn't support reflinks, we create a qcow2 overlay and use that as the image. + // Mount overlayfs to expose the user's VM image file with a different SELinux context so we + // can always access it. + + if !is_mountpoint(mirror_vm_image_path_in_host.parent().unwrap())? { + bind_mount_dir_read_only_with_different_context( + vm_image_path_in_host.parent().unwrap(), + mirror_vm_image_path_in_host.parent().unwrap(), + spec.mount_label(), + private_root_path.join("scratch"), + )?; + } + + // The overlayfs mount forbids writes to the VM image file, and also we want to get + // copy-on-write and page cache sharing even when the underlying file system doesn't support + // reflinks, so we create a qcow2 overlay image. let overlay_vm_image_path_in_container = Utf8PathBuf::from("crun-vm/image-overlay.qcow2"); let overlay_vm_image_path_in_host = @@ -191,13 +235,19 @@ fn set_up_vm_image( let overlay_vm_image_path_in_container = Utf8Path::new("/").join(overlay_vm_image_path_in_container); - vm_image_info.path = mirror_vm_image_path_in_container; - create_overlay_vm_image(&overlay_vm_image_path_in_host, &vm_image_info)?; + let mut base_vm_image_info = VmImageInfo::of(&mirror_vm_image_path_in_host)?; + base_vm_image_info.path = mirror_vm_image_path_in_container; - vm_image_info.path = overlay_vm_image_path_in_container; - } + if is_first_create { + create_overlay_vm_image(&overlay_vm_image_path_in_host, &base_vm_image_info)?; + } - Ok(vm_image_info) + Ok(VmImageInfo { + path: Utf8Path::new("/").join(overlay_vm_image_path_in_container), + size: base_vm_image_info.size, + format: "qcow2".to_string(), + }) + } } #[derive(Default)] diff --git a/src/util.rs b/src/util.rs index b13984a..49757d0 100644 --- a/src/util.rs +++ b/src/util.rs @@ -2,9 +2,9 @@ use std::ffi::{c_char, CString}; use std::fs::{self, OpenOptions, Permissions}; -use std::io; +use std::io::{self, ErrorKind}; use std::os::unix::ffi::OsStrExt; -use std::os::unix::fs::PermissionsExt; +use std::os::unix::fs::{MetadataExt, PermissionsExt}; use std::process::{Command, Stdio}; use anyhow::{anyhow, bail, ensure, Result}; @@ -27,6 +27,23 @@ pub fn set_file_context(path: impl AsRef, context: &str) -> Result<()> Ok(()) } +pub fn is_mountpoint(path: impl AsRef) -> Result { + let parent = path + .as_ref() + .parent() + .ok_or_else(|| anyhow!("path does not have a parent"))?; + + let path_dev = match fs::symlink_metadata(path.as_ref()) { + Ok(meta) => meta.dev(), + Err(e) if e.kind() == ErrorKind::NotFound => return Ok(false), + Err(e) => return Err(e.into()), + }; + + let parent_dev = fs::symlink_metadata(parent)?.dev(); + + Ok(path_dev != parent_dev) +} + pub fn bind_mount_file(from: impl AsRef, to: impl AsRef) -> Result<()> { // ensure target exists @@ -59,11 +76,24 @@ pub fn bind_mount_file(from: impl AsRef, to: impl AsRef) -> Ok(()) } +fn escape_path(mount_option: impl AsRef) -> String { + mount_option + .as_ref() + .as_str() + .replace('\\', "\\\\") + .replace(',', "\\,") +} + +fn escape_context(mount_option: &str) -> String { + assert!(!mount_option.contains('"')); + format!("\"{}\"", mount_option) +} + /// Expose directory `from` at `to` with the given SELinux `context`, if any, recursively applied. /// /// This does *not* modify the SELinux context of `from` nor of files under `from`. /// -/// If `propagate_changes` is true, `private_dir` must belong to the same file system as `from` and +/// If `propagate_changes` is true, `scratch_dir` must belong to the same file system as `from` and /// be a separate subtree. /// /// TODO: Is this a neat relabeling trick or simply a bad hack? @@ -72,27 +102,15 @@ pub fn bind_mount_dir_with_different_context( to: impl AsRef, context: Option<&str>, propagate_changes: bool, - private_dir: impl AsRef, + scratch_dir: impl AsRef, ) -> Result<()> { - let layer_dir = private_dir.as_ref().join("layer"); - let work_dir = private_dir.as_ref().join("work"); + let layer_dir = scratch_dir.as_ref().join("layer"); + let work_dir = scratch_dir.as_ref().join("work"); fs::create_dir_all(&layer_dir)?; fs::create_dir_all(&work_dir)?; fs::create_dir_all(to.as_ref())?; - fn escape_path(mount_option: &Utf8Path) -> String { - mount_option - .as_str() - .replace('\\', "\\\\") - .replace(',', "\\,") - } - - fn escape_context(mount_option: &str) -> String { - assert!(!mount_option.contains('"')); - format!("\"{}\"", mount_option) - } - let (lower_dir, upper_dir) = match propagate_changes { true => (layer_dir.as_path(), from.as_ref()), false => (from.as_ref(), layer_dir.as_path()), @@ -131,6 +149,49 @@ pub fn bind_mount_dir_with_different_context( Ok(()) } +/// Expose directory `from` read-only at `to` with the given SELinux `context`, if any, recursively +/// applied. +/// +/// This does *not* modify the SELinux context of `from` nor of files under `from`. +/// +/// TODO: Is this a neat relabeling trick or simply a bad hack? +pub fn bind_mount_dir_read_only_with_different_context( + from: impl AsRef, + to: impl AsRef, + context: Option<&str>, + scratch_dir: impl AsRef, +) -> Result<()> { + fs::create_dir_all(scratch_dir.as_ref())?; + fs::create_dir_all(to.as_ref())?; + + let mut options = format!( + "lowerdir={}:{}", + escape_path(scratch_dir), + escape_path(from) + ); + + if let Some(context) = context { + options = format!("{},context={}", options, escape_context(context)); + } + + if let Err(e) = nix::mount::mount( + Some("overlay"), + to.as_ref().as_std_path(), + Some("overlay"), + MsFlags::empty(), + Some(options.as_str()), + ) { + bail!( + "mount(\"overlay\", {:?}, \"overlay\", 0, {:?}) failed: {}", + to.as_ref(), + options, + e, + ); + } + + Ok(()) +} + pub trait SpecExt { fn root_path(&self) -> Result<&Utf8Path>; fn mount_label(&self) -> Option<&str>; From adb8e2c3022c181794a6f06a7d3306841068e7db Mon Sep 17 00:00:00 2001 From: Alberto Faria Date: Sat, 13 Apr 2024 13:51:30 +0100 Subject: [PATCH 4/8] exec: Allow customizing timeout Signed-off-by: Alberto Faria --- scripts/exec.sh | 22 +++++++++++++++++----- src/commands/exec.rs | 39 ++++++++++++++++++++++++++++++--------- 2 files changed, 47 insertions(+), 14 deletions(-) diff --git a/scripts/exec.sh b/scripts/exec.sh index 5bc014b..d741d50 100644 --- a/scripts/exec.sh +++ b/scripts/exec.sh @@ -3,23 +3,35 @@ set -e +timeout=$1 +user=$2 +command=( "${@:3}" ) + __ssh() { ssh \ -o LogLevel=ERROR \ -o StrictHostKeyChecking=no \ - -l "$1" \ + -l "$user" \ localhost \ - "${@:2}" + "$@" } if [[ ! -e /crun-vm/ssh-successful ]]; then # retry ssh for some time, ignoring some common errors - for (( i = 0; i < 60; ++i )); do + start_time=$( date +%s ) + end_time=$(( start_time + timeout )) + + while true; do + + if (( timeout > 0 && $( date +%s ) >= end_time )); then + >&2 echo "exec timed out while attempting ssh" + exit 255 + fi set +e - output=$( __ssh "$1" -o BatchMode=yes &1 ) + output=$( __ssh -o BatchMode=yes &1 ) exit_code=$? set -e @@ -43,4 +55,4 @@ if [[ ! -e /crun-vm/ssh-successful ]]; then fi -__ssh "$1" -- "${@:2}" +__ssh -- "${command[@]}" diff --git a/src/commands/exec.rs b/src/commands/exec.rs index e18ecac..4229dd2 100644 --- a/src/commands/exec.rs +++ b/src/commands/exec.rs @@ -1,11 +1,12 @@ // SPDX-License-Identifier: GPL-2.0-or-later use std::{ + env, fs::File, io::{BufReader, BufWriter}, }; -use anyhow::Result; +use anyhow::{bail, Result}; use clap::Parser; use crate::crun::crun_exec; @@ -19,7 +20,7 @@ pub fn exec(global_args: &liboci_cli::GlobalOpts, args: &liboci_cli::Exec) -> Re let command = process.args().as_ref().expect("command specified"); - let new_command = build_command(command); + let new_command = build_command(command)?; process.set_args(Some(new_command)); serde_json::to_writer( @@ -41,27 +42,47 @@ struct ExecArgs { #[clap(long, conflicts_with = "user")] container: bool, + #[clap(long = "timeout")] + timeout: Option, + #[arg(trailing_var_arg = true, allow_hyphen_values = true)] command: Vec, } -fn build_command(original_command: &Vec) -> Vec { +fn build_command(original_command: &Vec) -> Result> { let mut args: ExecArgs = ExecArgs::parse_from(original_command); + let timeout = if let Some(t) = args.timeout { + t + } else if let Some(t) = env::var_os("CRUN_VM_EXEC_TIMEOUT") { + match t.to_str().and_then(|t| t.parse().ok()) { + Some(t) => t, + None => bail!("env var CRUN_VM_EXEC_TIMEOUT has invalid value"), + } + } else { + 60 + }; + if args.command.starts_with(&["".to_string()]) { args.command.remove(0); } - if args.container { + let command = if args.container { if args.command.is_empty() { vec!["/bin/bash".to_string()] } else { args.command } } else { - ["/crun-vm/exec.sh".to_string(), args.user] - .into_iter() - .chain(args.command) - .collect() - } + [ + "/crun-vm/exec.sh".to_string(), + timeout.to_string(), + args.user, + ] + .into_iter() + .chain(args.command) + .collect() + }; + + Ok(command) } From 4115843e8f2d1e24145ce2e230bf1db4b0b2d672 Mon Sep 17 00:00:00 2001 From: Alberto Faria Date: Sat, 13 Apr 2024 14:28:32 +0100 Subject: [PATCH 5/8] exec: Fix ssh readiness check after reboot Signed-off-by: Alberto Faria --- scripts/entrypoint.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/scripts/entrypoint.sh b/scripts/entrypoint.sh index 9060b4e..f134295 100644 --- a/scripts/entrypoint.sh +++ b/scripts/entrypoint.sh @@ -50,6 +50,9 @@ virsh --connect "qemu+unix:///session?socket=$socket" "\$@" EOF chmod +x /crun-vm/virsh +# remove if present from previous boot +rm -f /crun-vm/ssh-successful + # launch VM function __bg_ensure_tty() { From f17c1fe88b68f2752d995a5e85a436df0a8a4021 Mon Sep 17 00:00:00 2001 From: Alberto Faria Date: Sat, 13 Apr 2024 14:29:34 +0100 Subject: [PATCH 6/8] exec: Avoid "remote host identification has changed" warning Signed-off-by: Alberto Faria --- scripts/exec.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts/exec.sh b/scripts/exec.sh index d741d50..41f9c63 100644 --- a/scripts/exec.sh +++ b/scripts/exec.sh @@ -11,6 +11,7 @@ __ssh() { ssh \ -o LogLevel=ERROR \ -o StrictHostKeyChecking=no \ + -o UserKnownHostsFile=/dev/null \ -l "$user" \ localhost \ "$@" From 7c32dd9bcbea6676365926e053ae40107694f0d3 Mon Sep 17 00:00:00 2001 From: Alberto Faria Date: Fri, 12 Apr 2024 22:35:31 +0100 Subject: [PATCH 7/8] util/package-vm-image.sh: Build KubeVirt containerdisk-compatible images Signed-off-by: Alberto Faria --- util/package-vm-image.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util/package-vm-image.sh b/util/package-vm-image.sh index 1caedea..792c074 100755 --- a/util/package-vm-image.sh +++ b/util/package-vm-image.sh @@ -12,7 +12,7 @@ fi vm_image_file=$1 container_image_tag=$2 -image_path_in_container=/$( basename "${vm_image_file}" ) +image_path_in_container=/disk/$( basename "${vm_image_file}" ) podman image build --file=- --tag="${container_image_tag}" . < Date: Fri, 12 Apr 2024 22:50:11 +0100 Subject: [PATCH 8/8] util/package-vm-image.sh: Make it work with files not under the current dir Signed-off-by: Alberto Faria --- util/package-vm-image.sh | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/util/package-vm-image.sh b/util/package-vm-image.sh index 792c074..b614410 100755 --- a/util/package-vm-image.sh +++ b/util/package-vm-image.sh @@ -12,9 +12,10 @@ fi vm_image_file=$1 container_image_tag=$2 +abs_vm_image_file=$( readlink -e "${vm_image_file}" ) image_path_in_container=/disk/$( basename "${vm_image_file}" ) -podman image build --file=- --tag="${container_image_tag}" . <