diff --git a/src/agent/src/image.rs b/src/agent/src/image.rs index a06e1ad60d..c4fc21b57e 100644 --- a/src/agent/src/image.rs +++ b/src/agent/src/image.rs @@ -20,8 +20,6 @@ use tokio::sync::Mutex; use crate::rpc::CONTAINER_BASE; use crate::AGENT_CONFIG; -// A marker to merge container spec for images pulled inside guest. -const ANNO_K8S_IMAGE_NAME: &str = "io.kubernetes.cri.image-name"; const KATA_IMAGE_WORK_DIR: &str = "/run/kata-containers/image/"; const CONFIG_JSON: &str = "config.json"; const KATA_PAUSE_BUNDLE: &str = "/pause_bundle"; @@ -52,23 +50,17 @@ fn copy_if_not_exists(src: &Path, dst: &Path) -> Result<()> { pub struct ImageService { image_client: ImageClient, - images: HashMap, } impl ImageService { pub fn new() -> Self { Self { image_client: ImageClient::new(PathBuf::from(KATA_IMAGE_WORK_DIR)), - images: HashMap::new(), } } - async fn add_image(&mut self, image: String, cid: String) { - self.images.insert(image, cid); - } - /// pause image is packaged in rootfs - fn unpack_pause_image(cid: &str, target_subpath: &str) -> Result { + fn unpack_pause_image(cid: &str) -> Result { verify_id(cid).context("The guest pause image cid contains invalid characters.")?; let guest_pause_bundle = Path::new(KATA_PAUSE_BUNDLE); @@ -102,9 +94,7 @@ impl ImageService { bail!("The number of args should be greater than or equal to one! Please check the pause image."); } - let container_bundle = scoped_join(CONTAINER_BASE, cid)?; - fs::create_dir_all(&container_bundle)?; - let pause_bundle = scoped_join(&container_bundle, target_subpath)?; + let pause_bundle = scoped_join(CONTAINER_BASE, cid)?; fs::create_dir_all(&pause_bundle)?; let pause_rootfs = scoped_join(&pause_bundle, "rootfs")?; fs::create_dir_all(&pause_rootfs)?; @@ -125,7 +115,7 @@ impl ImageService { /// - `cid`: Container id /// - `image_metadata`: Annotations about the image (exp: "containerd.io/snapshot/cri.layer-digest": "sha256:24fb2886d6f6c5d16481dd7608b47e78a8e92a13d6e64d87d57cb16d5f766d63") /// # Returns - /// - The image rootfs bundle path. (exp. /run/kata-containers/cb0b47276ea66ee9f44cc53afa94d7980b57a52c3f306f68cb034e58d9fbd3c6/images/rootfs) + /// - The image rootfs bundle path. (exp. /run/kata-containers/cb0b47276ea66ee9f44cc53afa94d7980b57a52c3f306f68cb034e58d9fbd3c6/rootfs) pub async fn pull_image( &mut self, image: &str, @@ -146,16 +136,13 @@ impl ImageService { } if is_sandbox { - let mount_path = Self::unpack_pause_image(cid, "pause")?; - self.add_image(String::from(image), String::from(cid)).await; + let mount_path = Self::unpack_pause_image(cid)?; return Ok(mount_path); } // Image layers will store at KATA_IMAGE_WORK_DIR, generated bundles // with rootfs and config.json will store under CONTAINER_BASE/cid/images. - let bundle_base_dir = scoped_join(CONTAINER_BASE, cid)?; - fs::create_dir_all(&bundle_base_dir)?; - let bundle_path = scoped_join(&bundle_base_dir, "images")?; + let bundle_path = scoped_join(CONTAINER_BASE, cid)?; fs::create_dir_all(&bundle_path)?; info!(sl(), "pull image {image:?}, bundle path {bundle_path:?}"); @@ -179,35 +166,9 @@ impl ImageService { return Err(e); } }; - self.add_image(String::from(image), String::from(cid)).await; let image_bundle_path = scoped_join(&bundle_path, "rootfs")?; Ok(image_bundle_path.as_path().display().to_string()) } - - /// Partially merge an OCI process specification into another one. - fn merge_oci_process(&self, target: &mut oci::Process, source: &oci::Process) { - // Override the target args only when the target args is empty and source.args is not empty - if target.args.is_empty() && !source.args.is_empty() { - target.args.append(&mut source.args.clone()); - } - - // Override the target cwd only when the target cwd is blank and source.cwd is not blank - if target.cwd == "/" && source.cwd != "/" { - target.cwd = String::from(&source.cwd); - } - - for source_env in &source.env { - if let Some((variable_name, variable_value)) = source_env.split_once('=') { - debug!( - sl(), - "source spec environment variable: {variable_name:?} : {variable_value:?}" - ); - if !target.env.iter().any(|i| i.contains(variable_name)) { - target.env.push(source_env.to_string()); - } - } - } - } } /// Set proxy environment from AGENT_CONFIG @@ -237,55 +198,6 @@ pub async fn set_proxy_env_vars() { }; } -/// When being passed an image name through a container annotation, merge its -/// corresponding bundle OCI specification into the passed container creation one. -pub async fn merge_bundle_oci(container_oci: &mut oci::Spec) -> Result<()> { - let image_service = IMAGE_SERVICE.clone(); - let mut image_service = image_service.lock().await; - let image_service = image_service - .as_mut() - .expect("Image Service not initialized"); - if let Some(image_name) = container_oci.annotations.get(ANNO_K8S_IMAGE_NAME) { - if let Some(container_id) = image_service.images.get(image_name) { - let image_oci_config_path = Path::new(CONTAINER_BASE) - .join(container_id) - .join(CONFIG_JSON); - debug!( - sl(), - "Image bundle config path: {:?}", image_oci_config_path - ); - - let image_oci = oci::Spec::load(image_oci_config_path.to_str().ok_or_else(|| { - anyhow!( - "Invalid container image OCI config path {:?}", - image_oci_config_path - ) - })?) - .context("load image bundle")?; - - if let (Some(container_root), Some(image_root)) = - (container_oci.root.as_mut(), image_oci.root.as_ref()) - { - let root_path = Path::new(CONTAINER_BASE) - .join(container_id) - .join(image_root.path.clone()); - container_root.path = - String::from(root_path.to_str().ok_or_else(|| { - anyhow!("Invalid container image root path {:?}", root_path) - })?); - } - - if let (Some(container_process), Some(image_process)) = - (container_oci.process.as_mut(), image_oci.process.as_ref()) - { - image_service.merge_oci_process(container_process, image_process); - } - } - } - - Ok(()) -} - /// Init the image service pub async fn init_image_service() { let image_service = ImageService::new(); @@ -305,71 +217,3 @@ pub async fn pull_image( image_service.pull_image(image, cid, image_metadata).await } - -#[cfg(test)] -mod tests { - use super::ImageService; - use rstest::rstest; - - #[rstest] - // TODO - how can we tell the user didn't specifically set it to `/` vs not setting at all? Is that scenario valid? - #[case::image_cwd_should_override_blank_container_cwd("/", "/imageDir", "/imageDir")] - #[case::container_cwd_should_override_image_cwd("/containerDir", "/imageDir", "/containerDir")] - #[case::container_cwd_should_override_blank_image_cwd("/containerDir", "/", "/containerDir")] - async fn test_merge_cwd( - #[case] container_process_cwd: &str, - #[case] image_process_cwd: &str, - #[case] expected: &str, - ) { - let image_service = ImageService::new(); - let mut container_process = oci::Process { - cwd: container_process_cwd.to_string(), - ..Default::default() - }; - let image_process = oci::Process { - cwd: image_process_cwd.to_string(), - ..Default::default() - }; - image_service.merge_oci_process(&mut container_process, &image_process); - assert_eq!(expected, container_process.cwd); - } - - #[rstest] - #[case::pods_environment_overrides_images( - vec!["ISPRODUCTION=true".to_string()], - vec!["ISPRODUCTION=false".to_string()], - vec!["ISPRODUCTION=true".to_string()] - )] - #[case::multiple_environment_variables_can_be_overrided( - vec!["ISPRODUCTION=true".to_string(), "ISDEVELOPMENT=false".to_string()], - vec!["ISPRODUCTION=false".to_string(), "ISDEVELOPMENT=true".to_string()], - vec!["ISPRODUCTION=true".to_string(), "ISDEVELOPMENT=false".to_string()] - )] - #[case::not_override_them_when_none_of_variables_match( - vec!["ANOTHERENV=TEST".to_string()], - vec!["ISPRODUCTION=false".to_string(), "ISDEVELOPMENT=true".to_string()], - vec!["ANOTHERENV=TEST".to_string(), "ISPRODUCTION=false".to_string(), "ISDEVELOPMENT=true".to_string()] - )] - #[case::a_mix_of_both_overriding_and_not( - vec!["ANOTHERENV=TEST".to_string(), "ISPRODUCTION=true".to_string()], - vec!["ISPRODUCTION=false".to_string(), "ISDEVELOPMENT=true".to_string()], - vec!["ANOTHERENV=TEST".to_string(), "ISPRODUCTION=true".to_string(), "ISDEVELOPMENT=true".to_string()] - )] - async fn test_merge_env( - #[case] container_process_env: Vec, - #[case] image_process_env: Vec, - #[case] expected: Vec, - ) { - let image_service = ImageService::new(); - let mut container_process = oci::Process { - env: container_process_env, - ..Default::default() - }; - let image_process = oci::Process { - env: image_process_env, - ..Default::default() - }; - image_service.merge_oci_process(&mut container_process, &image_process); - assert_eq!(expected, container_process.env); - } -} diff --git a/src/agent/src/rpc.rs b/src/agent/src/rpc.rs index aa598bc7ca..cfafc5b21a 100644 --- a/src/agent/src/rpc.rs +++ b/src/agent/src/rpc.rs @@ -210,11 +210,6 @@ impl AgentService { "receive createcontainer, storages: {:?}", &req.storages ); - // In case of pulling image inside guest, we need to merge the image bundle OCI spec - // into the container creation request OCI spec. - #[cfg(feature = "guest-pull")] - image::merge_bundle_oci(&mut oci).await?; - // Some devices need some extra processing (the ones invoked with // --device for instance), and that's what this call is doing. It // updates the devices listed in the OCI spec, so that they actually @@ -1920,21 +1915,28 @@ pub fn setup_bundle(cid: &str, spec: &mut Spec) -> Result { return Err(anyhow!(nix::Error::EINVAL)); }; - let spec_root_path = Path::new(&spec_root.path); - let bundle_path = Path::new(CONTAINER_BASE).join(cid); let config_path = bundle_path.join("config.json"); let rootfs_path = bundle_path.join("rootfs"); + let spec_root_path = Path::new(&spec_root.path); - fs::create_dir_all(&rootfs_path)?; - baremount( - spec_root_path, - &rootfs_path, - "bind", - MsFlags::MS_BIND, - "", - &sl(), - )?; + let rootfs_exists = Path::new(&rootfs_path).exists(); + info!( + sl(), + "The rootfs_path is {:?} and exists: {}", rootfs_path, rootfs_exists + ); + + if !rootfs_exists { + fs::create_dir_all(&rootfs_path)?; + baremount( + spec_root_path, + &rootfs_path, + "bind", + MsFlags::MS_BIND, + "", + &sl(), + )?; + } let rootfs_path_name = rootfs_path .to_str() diff --git a/src/agent/src/storage/image_pull_handler.rs b/src/agent/src/storage/image_pull_handler.rs index 3cf546622c..c92f446cf4 100644 --- a/src/agent/src/storage/image_pull_handler.rs +++ b/src/agent/src/storage/image_pull_handler.rs @@ -3,6 +3,7 @@ // SPDX-License-Identifier: Apache-2.0 // +use super::new_device; use crate::image; use crate::storage::{StorageContext, StorageHandler}; use anyhow::{anyhow, Result}; @@ -12,8 +13,6 @@ use protocols::agent::Storage; use std::sync::Arc; use tracing::instrument; -use super::{common_storage_handler, new_device}; - #[derive(Debug)] pub struct ImagePullHandler {} @@ -36,7 +35,7 @@ impl StorageHandler for ImagePullHandler { #[instrument] async fn create_device( &self, - mut storage: Storage, + storage: Storage, ctx: &mut StorageContext, ) -> Result> { //Currently the image metadata is not used to pulling image in the guest. @@ -51,12 +50,7 @@ impl StorageHandler for ImagePullHandler { .ok_or_else(|| anyhow!("failed to get container id"))?; let bundle_path = image::pull_image(image_name, &cid, &image_pull_volume.metadata).await?; - storage.source = bundle_path; - storage.options = vec!["bind".to_string(), "ro".to_string()]; - - common_storage_handler(ctx.logger, &storage)?; - - new_device(storage.mount_point) + new_device(bundle_path) } } diff --git a/tests/integration/kubernetes/k8s-file-volume.bats b/tests/integration/kubernetes/k8s-file-volume.bats index 6b770a1993..1a5a4b98e8 100644 --- a/tests/integration/kubernetes/k8s-file-volume.bats +++ b/tests/integration/kubernetes/k8s-file-volume.bats @@ -12,8 +12,6 @@ TEST_INITRD="${TEST_INITRD:-no}" setup() { [ "${KATA_HYPERVISOR}" == "firecracker" ] && skip "test not working see: ${fc_limitations}" [ "${KATA_HYPERVISOR}" == "fc" ] && skip "test not working see: ${fc_limitations}" - [[ "${KATA_HYPERVISOR}" == "qemu-tdx" || "${KATA_HYPERVISOR}" == "qemu-coco-dev" ]] && \ - skip "See: https://github.com/kata-containers/kata-containers/issues/9667" pod_name="test-file-volume" container_name="busybox-file-volume-container" @@ -60,8 +58,6 @@ setup() { teardown() { [ "${KATA_HYPERVISOR}" == "firecracker" ] && skip "test not working see: ${fc_limitations}" [ "${KATA_HYPERVISOR}" == "fc" ] && skip "test not working see: ${fc_limitations}" - [[ "${KATA_HYPERVISOR}" == "qemu-tdx" || "${KATA_HYPERVISOR}" == "qemu-coco-dev" ]] && \ - skip "See: https://github.com/kata-containers/kata-containers/issues/9667" kubectl describe pod "$pod_name" diff --git a/tests/integration/kubernetes/k8s-kill-all-process-in-container.bats b/tests/integration/kubernetes/k8s-kill-all-process-in-container.bats index 794a4aa449..9174855c38 100644 --- a/tests/integration/kubernetes/k8s-kill-all-process-in-container.bats +++ b/tests/integration/kubernetes/k8s-kill-all-process-in-container.bats @@ -9,10 +9,6 @@ load "${BATS_TEST_DIRNAME}/../../common.bash" load "${BATS_TEST_DIRNAME}/tests_common.sh" setup() { - [[ "${KATA_HYPERVISOR}" = "qemu-tdx" || "${KATA_HYPERVISOR}" = "qemu-coco-dev" || \ - "${KATA_HYPERVISOR}" = "qemu-sev" || "${KATA_HYPERVISOR}" = "qemu-snp" ]] && \ - skip "See: https://github.com/kata-containers/kata-containers/issues/9664" - pod_name="busybox" first_container_name="first-test-container" @@ -43,10 +39,6 @@ setup() { } teardown() { - [[ "${KATA_HYPERVISOR}" = "qemu-tdx" || "${KATA_HYPERVISOR}" = "qemu-coco-dev" || \ - "${KATA_HYPERVISOR}" = "qemu-sev" || "${KATA_HYPERVISOR}" = "qemu-snp" ]] && \ - skip "See: https://github.com/kata-containers/kata-containers/issues/9664" - # Debugging information kubectl describe "pod/$pod_name" diff --git a/tests/integration/kubernetes/k8s-liveness-probes.bats b/tests/integration/kubernetes/k8s-liveness-probes.bats index 62c67f7af5..ea8d51c41e 100644 --- a/tests/integration/kubernetes/k8s-liveness-probes.bats +++ b/tests/integration/kubernetes/k8s-liveness-probes.bats @@ -9,8 +9,6 @@ load "${BATS_TEST_DIRNAME}/../../common.bash" load "${BATS_TEST_DIRNAME}/tests_common.sh" setup() { - [ "${KATA_HYPERVISOR}" = "qemu-tdx" ] && skip "See: https://github.com/kata-containers/kata-containers/issues/9665" - sleep_liveness=20 agnhost_name="${container_images_agnhost_name}" agnhost_version="${container_images_agnhost_version}" @@ -91,8 +89,6 @@ setup() { } teardown() { - [ "${KATA_HYPERVISOR}" = "qemu-tdx" ] && skip "See: https://github.com/kata-containers/kata-containers/issues/9665" - # Debugging information kubectl describe "pod/$pod_name" diff --git a/tests/integration/kubernetes/k8s-shared-volume.bats b/tests/integration/kubernetes/k8s-shared-volume.bats index f81eaf8449..98a0eabfda 100644 --- a/tests/integration/kubernetes/k8s-shared-volume.bats +++ b/tests/integration/kubernetes/k8s-shared-volume.bats @@ -13,8 +13,6 @@ setup() { } @test "Containers with shared volume" { - [ "${KATA_HYPERVISOR}" = "qemu-coco-dev" ] && \ - skip "See: https://github.com/kata-containers/kata-containers/issues/9668" pod_name="test-shared-volume" first_container_name="busybox-first-container" second_container_name="busybox-second-container" @@ -42,9 +40,6 @@ setup() { } @test "initContainer with shared volume" { - [[ "${KATA_HYPERVISOR}" = "qemu-tdx" || "${KATA_HYPERVISOR}" = "qemu-coco-dev" || \ - "${KATA_HYPERVISOR}" = "qemu-sev" || "${KATA_HYPERVISOR}" = "qemu-snp" ]] && \ - skip "See: https://github.com/kata-containers/kata-containers/issues/9668" pod_name="initcontainer-shared-volume" last_container="last" diff --git a/tests/integration/kubernetes/k8s-sysctls.bats b/tests/integration/kubernetes/k8s-sysctls.bats index 8987792e8d..9317aeaac1 100644 --- a/tests/integration/kubernetes/k8s-sysctls.bats +++ b/tests/integration/kubernetes/k8s-sysctls.bats @@ -9,9 +9,6 @@ load "${BATS_TEST_DIRNAME}/../../common.bash" load "${BATS_TEST_DIRNAME}/tests_common.sh" setup() { - [[ "${KATA_HYPERVISOR}" = "qemu-tdx" || "${KATA_HYPERVISOR}" = "qemu-coco-dev" || \ - "${KATA_HYPERVISOR}" = "qemu-sev" || "${KATA_HYPERVISOR}" = "qemu-snp" ]] && \ - skip "See: https://github.com/kata-containers/kata-containers/issues/9666" pod_name="sysctl-test" get_pod_config_dir @@ -34,9 +31,6 @@ setup() { } teardown() { - [[ "${KATA_HYPERVISOR}" = "qemu-tdx" || "${KATA_HYPERVISOR}" = "qemu-coco-dev" || \ - "${KATA_HYPERVISOR}" = "qemu-sev" || "${KATA_HYPERVISOR}" = "qemu-snp" ]] && \ - skip "See: https://github.com/kata-containers/kata-containers/issues/9666" # Debugging information kubectl describe "pod/$pod_name"