From f528bc01031a84e997447c61eec30ef08082876c Mon Sep 17 00:00:00 2001 From: Yibo Zhuang Date: Fri, 20 May 2022 14:55:00 -0700 Subject: [PATCH 1/4] runtime: fix incorrect Action function for direct-volume stats The action function expects a function that returns error but the current direct-volume stats Action returns (string, error) which is invalid. This change fixes the format and print out the stats from the command instead. Fixes: #4293 Signed-off-by: Yibo Zhuang --- src/runtime/cmd/kata-runtime/kata-volume.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/runtime/cmd/kata-runtime/kata-volume.go b/src/runtime/cmd/kata-runtime/kata-volume.go index e08e9482fa..a94d35d72e 100644 --- a/src/runtime/cmd/kata-runtime/kata-volume.go +++ b/src/runtime/cmd/kata-runtime/kata-volume.go @@ -7,10 +7,11 @@ package main import ( "encoding/json" + "fmt" "net/url" containerdshim "github.com/kata-containers/kata-containers/src/runtime/pkg/containerd-shim-v2" - "github.com/kata-containers/kata-containers/src/runtime/pkg/direct-volume" + volume "github.com/kata-containers/kata-containers/src/runtime/pkg/direct-volume" "github.com/kata-containers/kata-containers/src/runtime/pkg/utils/shimclient" "github.com/urfave/cli" @@ -89,12 +90,14 @@ var statsCommand = cli.Command{ Destination: &volumePath, }, }, - Action: func(c *cli.Context) (string, error) { + Action: func(c *cli.Context) error { stats, err := Stats(volumePath) if err != nil { - return "", cli.NewExitError(err.Error(), 1) + return cli.NewExitError(err.Error(), 1) } - return string(stats), nil + + fmt.Println(string(stats)) + return nil }, } From 338c9f2b0baa40062ed5946265dce51e82d8d3ec Mon Sep 17 00:00:00 2001 From: Yibo Zhuang Date: Fri, 20 May 2022 18:41:51 -0700 Subject: [PATCH 2/4] runtime: direct-volume stats update to use GET parameter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The go default http mux AFAIK doesn’t support pattern routing so right now client is padding the url for direct-volume stats with a subpath of the volume path and this will always result in 404 not found returned by the shim. This change will update the shim to take the volume path as a GET query parameter instead of a subpath. If the parameter is missing or empty, then return 400 BadRequest to the client. Fixes: #4297 Signed-off-by: Yibo Zhuang --- .../pkg/containerd-shim-v2/shim_management.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/runtime/pkg/containerd-shim-v2/shim_management.go b/src/runtime/pkg/containerd-shim-v2/shim_management.go index b5ad03eed2..e109222507 100644 --- a/src/runtime/pkg/containerd-shim-v2/shim_management.go +++ b/src/runtime/pkg/containerd-shim-v2/shim_management.go @@ -32,6 +32,8 @@ import ( ) const ( + DirectVolumePathKey = "path" + DirectVolumeStatUrl = "/direct-volume/stats" DirectVolumeResizeUrl = "/direct-volume/resize" ) @@ -139,7 +141,16 @@ func decodeAgentMetrics(body string) []*dto.MetricFamily { } func (s *service) serveVolumeStats(w http.ResponseWriter, r *http.Request) { - volumePath, err := url.PathUnescape(strings.TrimPrefix(r.URL.Path, DirectVolumeStatUrl)) + val := r.URL.Query().Get(DirectVolumePathKey) + if val == "" { + msg := fmt.Sprintf("Required parameter %s not found", DirectVolumePathKey) + shimMgtLog.Info(msg) + w.WriteHeader(http.StatusBadRequest) + w.Write([]byte(msg)) + return + } + + volumePath, err := url.PathUnescape(val) if err != nil { shimMgtLog.WithError(err).Error("failed to unescape the volume stat url path") w.WriteHeader(http.StatusInternalServerError) From d1848523d37f2a2b5ebeac92b25fecc28bbfa81c Mon Sep 17 00:00:00 2001 From: Yibo Zhuang Date: Fri, 20 May 2022 18:42:47 -0700 Subject: [PATCH 3/4] runtime: direct-volume stats use correct name Today the shim does a translation when doing direct-volume stats where it takes the source and returns the mount path within the guest. The source for a direct-assigned volume is actually the device path on the host and not the publish volume path. This change will perform a lookup of the mount info during direct-volume stats to ensure that the device path is provided to the shim for querying the volume stats. Fixes: #4297 Signed-off-by: Yibo Zhuang --- src/runtime/cmd/kata-runtime/kata-volume.go | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/runtime/cmd/kata-runtime/kata-volume.go b/src/runtime/cmd/kata-runtime/kata-volume.go index a94d35d72e..55274f7d80 100644 --- a/src/runtime/cmd/kata-runtime/kata-volume.go +++ b/src/runtime/cmd/kata-runtime/kata-volume.go @@ -130,8 +130,14 @@ func Stats(volumePath string) ([]byte, error) { if err != nil { return nil, err } - urlSafeDevicePath := url.PathEscape(volumePath) - body, err := shimclient.DoGet(sandboxId, defaultTimeout, containerdshim.DirectVolumeStatUrl+"/"+urlSafeDevicePath) + volumeMountInfo, err := volume.VolumeMountInfo(volumePath) + if err != nil { + return nil, err + } + + urlSafeDevicePath := url.PathEscape(volumeMountInfo.Device) + body, err := shimclient.DoGet(sandboxId, defaultTimeout, + fmt.Sprintf("%s?%s=%s", containerdshim.DirectVolumeStatUrl, containerdshim.DirectVolumePathKey, urlSafeDevicePath)) if err != nil { return nil, err } @@ -144,8 +150,13 @@ func Resize(volumePath string, size uint64) error { if err != nil { return err } + volumeMountInfo, err := volume.VolumeMountInfo(volumePath) + if err != nil { + return err + } + resizeReq := containerdshim.ResizeRequest{ - VolumePath: volumePath, + VolumePath: volumeMountInfo.Device, Size: size, } encoded, err := json.Marshal(resizeReq) From 29c9391da16c714c71157cf318e87f34692269c1 Mon Sep 17 00:00:00 2001 From: Yibo Zhuang Date: Fri, 20 May 2022 18:43:27 -0700 Subject: [PATCH 4/4] agent: fix direct-assigned volume stats The current implementation of walking the disks to match with the requested volume path in agent doesn't work because the volume path provided by the shim to the agent is the mount path within the guest and not the device name. The current logic is trying to match the device name to the volume path which will never match. This change will simplify the get_volume_capacity_stats and get_volume_inode_stats to just call statfs and get the bytes and inodes usage of the volume path directly. Fixes: #4297 Signed-off-by: Yibo Zhuang --- src/agent/src/rpc.rs | 109 ++++++++++++++++++++++++++++++------------- 1 file changed, 76 insertions(+), 33 deletions(-) diff --git a/src/agent/src/rpc.rs b/src/agent/src/rpc.rs index 5fce4fb4ee..765a3a6bf1 100644 --- a/src/agent/src/rpc.rs +++ b/src/agent/src/rpc.rs @@ -40,13 +40,11 @@ use rustjail::specconv::CreateOpts; use nix::errno::Errno; use nix::mount::MsFlags; -use nix::sys::stat; +use nix::sys::{stat, statfs}; use nix::unistd::{self, Pid}; use rustjail::cgroups::Manager; use rustjail::process::ProcessOperations; -use sysinfo::{DiskExt, System, SystemExt}; - use crate::device::{ add_devices, get_virtio_blk_pci_device_name, update_device_cgroup, update_env_pci, }; @@ -71,7 +69,6 @@ use tracing::instrument; use libc::{self, c_char, c_ushort, pid_t, winsize, TIOCSWINSZ}; use std::fs; -use std::os::unix::fs::MetadataExt; use std::os::unix::prelude::PermissionsExt; use std::process::{Command, Stdio}; use std::time::Duration; @@ -1452,20 +1449,12 @@ fn get_memory_info(block_size: bool, hotplug: bool) -> Result<(u64, bool)> { fn get_volume_capacity_stats(path: &str) -> Result { let mut usage = VolumeUsage::new(); - let s = System::new(); - for disk in s.disks() { - if let Some(v) = disk.name().to_str() { - if v.to_string().eq(path) { - usage.available = disk.available_space(); - usage.total = disk.total_space(); - usage.used = usage.total - usage.available; - usage.unit = VolumeUsage_Unit::BYTES; // bytes - break; - } - } else { - return Err(anyhow!(nix::Error::EINVAL)); - } - } + let stat = statfs::statfs(path)?; + let block_size = stat.block_size() as u64; + usage.total = stat.blocks() * block_size; + usage.available = stat.blocks_free() * block_size; + usage.used = usage.total - usage.available; + usage.unit = VolumeUsage_Unit::BYTES; Ok(usage) } @@ -1473,20 +1462,11 @@ fn get_volume_capacity_stats(path: &str) -> Result { fn get_volume_inode_stats(path: &str) -> Result { let mut usage = VolumeUsage::new(); - let s = System::new(); - for disk in s.disks() { - if let Some(v) = disk.name().to_str() { - if v.to_string().eq(path) { - let meta = fs::metadata(disk.mount_point())?; - let inode = meta.ino(); - usage.used = inode; - usage.unit = VolumeUsage_Unit::INODES; - break; - } - } else { - return Err(anyhow!(nix::Error::EINVAL)); - } - } + let stat = statfs::statfs(path)?; + usage.total = stat.files(); + usage.available = stat.files_free(); + usage.used = usage.total - usage.available; + usage.unit = VolumeUsage_Unit::INODES; Ok(usage) } @@ -1866,7 +1846,8 @@ fn load_kernel_module(module: &protocols::agent::KernelModule) -> Result<()> { #[cfg(test)] mod tests { use super::*; - use crate::protocols::agent_ttrpc::AgentService as _; + use crate::{protocols::agent_ttrpc::AgentService as _, skip_if_not_root}; + use nix::mount; use oci::{Hook, Hooks}; use ttrpc::{r#async::TtrpcContext, MessageHeader}; @@ -2197,4 +2178,66 @@ mod tests { } } } + + #[tokio::test] + async fn test_volume_capacity_stats() { + skip_if_not_root!(); + + // Verify error if path does not exist + assert!(get_volume_capacity_stats("/does-not-exist").is_err()); + + // Create a new tmpfs mount, and verify the initial values + let mount_dir = tempfile::tempdir().unwrap(); + mount::mount( + Some("tmpfs"), + mount_dir.path().to_str().unwrap(), + Some("tmpfs"), + mount::MsFlags::empty(), + None::<&str>, + ) + .unwrap(); + let mut stats = get_volume_capacity_stats(mount_dir.path().to_str().unwrap()).unwrap(); + assert_eq!(stats.used, 0); + assert_ne!(stats.available, 0); + let available = stats.available; + + // Verify that writing a file will result in increased utilization + fs::write(mount_dir.path().join("file.dat"), "foobar").unwrap(); + stats = get_volume_capacity_stats(mount_dir.path().to_str().unwrap()).unwrap(); + + assert_eq!(stats.used, 4 * 1024); + assert_eq!(stats.available, available - 4 * 1024); + } + + #[tokio::test] + async fn test_get_volume_inode_stats() { + skip_if_not_root!(); + + // Verify error if path does not exist + assert!(get_volume_inode_stats("/does-not-exist").is_err()); + + // Create a new tmpfs mount, and verify the initial values + let mount_dir = tempfile::tempdir().unwrap(); + mount::mount( + Some("tmpfs"), + mount_dir.path().to_str().unwrap(), + Some("tmpfs"), + mount::MsFlags::empty(), + None::<&str>, + ) + .unwrap(); + let mut stats = get_volume_inode_stats(mount_dir.path().to_str().unwrap()).unwrap(); + assert_eq!(stats.used, 1); + assert_ne!(stats.available, 0); + let available = stats.available; + + // Verify that creating a directory and writing a file will result in increased utilization + let dir = mount_dir.path().join("foobar"); + fs::create_dir_all(&dir).unwrap(); + fs::write(dir.as_path().join("file.dat"), "foobar").unwrap(); + stats = get_volume_inode_stats(mount_dir.path().to_str().unwrap()).unwrap(); + + assert_eq!(stats.used, 3); + assert_eq!(stats.available, available - 2); + } }