From 485aeabb6b0965a0dc76814bb262ac0268741597 Mon Sep 17 00:00:00 2001 From: Braden Rayhorn Date: Mon, 28 Mar 2022 17:58:28 -0500 Subject: [PATCH 01/10] agent: add tests for do_write_stream function Add test coverage for do_write_stream function of AgentService in src/rpc.rs. Includes minor refactoring to make function more easily testable. Fixes #3984 Signed-off-by: Braden Rayhorn --- src/agent/src/rpc.rs | 186 ++++++++++++++++++++++++++++++++++++++- src/agent/src/sandbox.rs | 4 +- 2 files changed, 187 insertions(+), 3 deletions(-) diff --git a/src/agent/src/rpc.rs b/src/agent/src/rpc.rs index 77b18a32eb..50a11dfd96 100644 --- a/src/agent/src/rpc.rs +++ b/src/agent/src/rpc.rs @@ -85,6 +85,7 @@ use std::path::PathBuf; const CONTAINER_BASE: &str = "/run/kata-containers"; const MODPROBE_PATH: &str = "/sbin/modprobe"; +const ERR_CANNOT_GET_WRITER: &str = "Cannot get writer"; const ERR_INVALID_BLOCK_SIZE: &str = "Invalid block size"; // Convenience macro to obtain the scope logger @@ -577,7 +578,7 @@ impl AgentService { } }; - let writer = writer.ok_or_else(|| anyhow!("cannot get writer"))?; + let writer = writer.ok_or_else(|| anyhow!(ERR_CANNOT_GET_WRITER))?; writer.lock().await.write_all(req.data.as_slice()).await?; let mut resp = WriteStreamResponse::new(); @@ -1880,7 +1881,7 @@ mod tests { use super::*; use crate::protocols::agent_ttrpc::AgentService as _; use oci::{Hook, Hooks}; - use tempfile::tempdir; + use tempfile::{tempdir, TempDir}; use ttrpc::{r#async::TtrpcContext, MessageHeader}; // Parameters: @@ -1918,6 +1919,44 @@ mod tests { } } + fn create_dummy_opts() -> CreateOpts { + let root = Root { + path: String::from("/"), + ..Default::default() + }; + + let spec = Spec { + linux: Some(oci::Linux::default()), + root: Some(root), + ..Default::default() + }; + + CreateOpts { + cgroup_name: "".to_string(), + use_systemd_cgroup: false, + no_pivot_root: false, + no_new_keyring: false, + spec: Some(spec), + rootless_euid: false, + rootless_cgroup: false, + } + } + + fn create_linuxcontainer() -> (LinuxContainer, TempDir) { + let dir = tempdir().expect("failed to make tempdir"); + + ( + LinuxContainer::new( + "some_id", + dir.path().join("rootfs").to_str().unwrap(), + create_dummy_opts(), + &slog_scope::logger(), + ) + .unwrap(), + dir, + ) + } + #[test] fn test_load_kernel_module() { let mut m = protocols::agent::KernelModule { @@ -2010,6 +2049,149 @@ mod tests { assert!(result.is_err(), "expected add arp neighbors to fail"); } + #[tokio::test] + async fn test_do_write_stream() { + #[derive(Debug)] + struct TestData<'a> { + create_container: bool, + has_fd: bool, + has_tty: bool, + break_pipe: bool, + + container_id: &'a str, + exec_id: &'a str, + data: Vec, + result: Result, + } + + impl Default for TestData<'_> { + fn default() -> Self { + TestData { + create_container: true, + has_fd: true, + has_tty: true, + break_pipe: false, + + container_id: "1", + exec_id: "2", + data: vec![1, 2, 3], + result: Ok(WriteStreamResponse { + len: 3, + ..WriteStreamResponse::default() + }), + } + } + } + + let tests = &[ + TestData { + ..Default::default() + }, + TestData { + has_tty: false, + ..Default::default() + }, + TestData { + break_pipe: true, + result: Err(anyhow!(std::io::Error::from_raw_os_error(libc::EPIPE))), + ..Default::default() + }, + TestData { + create_container: false, + result: Err(anyhow!(crate::sandbox::ERR_INVALID_CONTAINER_ID)), + ..Default::default() + }, + TestData { + container_id: "8181", + result: Err(anyhow!(crate::sandbox::ERR_INVALID_CONTAINER_ID)), + ..Default::default() + }, + TestData { + data: vec![], + result: Ok(WriteStreamResponse { + len: 0, + ..WriteStreamResponse::default() + }), + ..Default::default() + }, + TestData { + has_fd: false, + result: Err(anyhow!(ERR_CANNOT_GET_WRITER)), + ..Default::default() + }, + ]; + + for (i, d) in tests.iter().enumerate() { + let msg = format!("test[{}]: {:?}", i, d); + + let logger = slog::Logger::root(slog::Discard, o!()); + let mut sandbox = Sandbox::new(&logger).unwrap(); + + let (rfd, wfd) = unistd::pipe().unwrap(); + if d.break_pipe { + unistd::close(rfd).unwrap(); + } + + if d.create_container { + let (mut linux_container, _root) = create_linuxcontainer(); + let exec_process_id = 2; + + linux_container.id = "1".to_string(); + + let mut exec_process = Process::new( + &logger, + &oci::Process::default(), + &exec_process_id.to_string(), + false, + 1, + ) + .unwrap(); + + let fd = { + if d.has_fd { + Some(wfd) + } else { + None + } + }; + + if d.has_tty { + exec_process.parent_stdin = None; + exec_process.term_master = fd; + } else { + exec_process.parent_stdin = fd; + exec_process.term_master = None; + } + linux_container + .processes + .insert(exec_process_id, exec_process); + + sandbox.add_container(linux_container); + } + + let agent_service = Box::new(AgentService { + sandbox: Arc::new(Mutex::new(sandbox)), + }); + + let result = agent_service + .do_write_stream(protocols::agent::WriteStreamRequest { + container_id: d.container_id.to_string(), + exec_id: d.exec_id.to_string(), + data: d.data.clone(), + ..Default::default() + }) + .await; + + if !d.break_pipe { + unistd::close(rfd).unwrap(); + } + unistd::close(wfd).unwrap(); + + let msg = format!("{}, result: {:?}", msg, result); + assert_result!(d.result, result, msg); + } + } + #[tokio::test] async fn test_get_memory_info() { #[derive(Debug)] diff --git a/src/agent/src/sandbox.rs b/src/agent/src/sandbox.rs index f1f60fbb91..3577f7f3fa 100644 --- a/src/agent/src/sandbox.rs +++ b/src/agent/src/sandbox.rs @@ -32,6 +32,8 @@ use tokio::sync::oneshot; use tokio::sync::Mutex; use tracing::instrument; +pub const ERR_INVALID_CONTAINER_ID: &str = "Invalid container id"; + type UeventWatcher = (Box, oneshot::Sender); #[derive(Debug)] @@ -232,7 +234,7 @@ impl Sandbox { pub fn find_container_process(&mut self, cid: &str, eid: &str) -> Result<&mut Process> { let ctr = self .get_container(cid) - .ok_or_else(|| anyhow!("Invalid container id"))?; + .ok_or_else(|| anyhow!(ERR_INVALID_CONTAINER_ID))?; if eid.is_empty() { return ctr From 962d05ec865320194b00a54b588e9d8ee8b059b4 Mon Sep 17 00:00:00 2001 From: bin Date: Thu, 31 Mar 2022 15:15:10 +0800 Subject: [PATCH 02/10] protocols: add src/csi.rs to .gitignore After running make in src/agent, the git working area will be changed: Untracked files: (use "git add ..." to include in what will be committed) src/libs/protocols/src/csi.rs The generated file by `build.rs` should be ignored in git. Fixes: #3959 Signed-off-by: bin --- src/libs/protocols/.gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libs/protocols/.gitignore b/src/libs/protocols/.gitignore index fe772076e3..ce4964c4f0 100644 --- a/src/libs/protocols/.gitignore +++ b/src/libs/protocols/.gitignore @@ -1,6 +1,7 @@ Cargo.lock src/agent.rs src/agent_ttrpc.rs +src/csi.rs src/empty.rs src/health.rs src/health_ttrpc.rs From eff7c7e0ff7aa963ae000442af59021400bf70a5 Mon Sep 17 00:00:00 2001 From: Manabu Sugimoto Date: Mon, 28 Mar 2022 21:54:08 +0900 Subject: [PATCH 03/10] agent: Allow the agent to be rebuilt with the change of Cargo features This allows the kata-agent to be rebuilt when Cargo "features" is changed. The Makefile for the agent do not need to specify the sources for prerequisites by having Cargo check for the sources changes. Fixes: #4052 Signed-off-by: Manabu Sugimoto --- src/agent/Makefile | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/agent/Makefile b/src/agent/Makefile index a0f330ea10..13f2ce618a 100644 --- a/src/agent/Makefile +++ b/src/agent/Makefile @@ -14,10 +14,6 @@ PROJECT_COMPONENT = kata-agent TARGET = $(PROJECT_COMPONENT) -SOURCES := \ - $(shell find . 2>&1 | grep -E '.*\.rs$$') \ - Cargo.toml - VERSION_FILE := ./VERSION VERSION := $(shell grep -v ^\# $(VERSION_FILE)) COMMIT_NO := $(shell git rev-parse HEAD 2>/dev/null || true) @@ -38,7 +34,7 @@ ifeq ($(SECCOMP),yes) endif ifneq ($(EXTRA_RUSTFEATURES),) - override EXTRA_RUSTFEATURES := --features $(EXTRA_RUSTFEATURES) + override EXTRA_RUSTFEATURES := --features "$(EXTRA_RUSTFEATURES)" endif include ../../utils.mk @@ -108,14 +104,14 @@ $(TARGET): $(GENERATED_CODE) logging-crate-tests $(TARGET_PATH) logging-crate-tests: make -C $(CWD)/../libs/logging -$(TARGET_PATH): $(SOURCES) | show-summary +$(TARGET_PATH): show-summary @RUSTFLAGS="$(EXTRA_RUSTFLAGS) --deny warnings" cargo build --target $(TRIPLE) --$(BUILD_TYPE) $(EXTRA_RUSTFEATURES) $(GENERATED_FILES): %: %.in @sed $(foreach r,$(GENERATED_REPLACEMENTS),-e 's|@$r@|$($r)|g') "$<" > "$@" ##TARGET optimize: optimized build -optimize: $(SOURCES) | show-summary show-header +optimize: show-summary show-header @RUSTFLAGS="-C link-arg=-s $(EXTRA_RUSTFLAGS) --deny warnings" cargo build --target $(TRIPLE) --$(BUILD_TYPE) $(EXTRA_RUSTFEATURES) ##TARGET install: install agent From 9d5b03a1b70bbd175237ec4b9f821d6ccee0a1f6 Mon Sep 17 00:00:00 2001 From: bin Date: Thu, 7 Apr 2022 17:44:04 +0800 Subject: [PATCH 04/10] runtime: delete debug option in virtiofsd virtiofsd's debug will be enabled if hypervisor's debug has been enabled, this will generate too many noisy logs from virtiofsd. Unbind the relationship of log level between virtiofsd and hypervisor, if users want to see debug log of virtiofsd, can set it by: virtio_fs_extra_args = ["-o", "log_level=debug"] Fixes: #3303 Signed-off-by: bin --- src/runtime/config/configuration-clh.toml.in | 3 ++- src/runtime/config/configuration-qemu.toml.in | 2 ++ src/runtime/virtcontainers/clh.go | 2 -- src/runtime/virtcontainers/qemu.go | 1 - src/runtime/virtcontainers/virtiofsd.go | 10 +--------- src/runtime/virtcontainers/virtiofsd_test.go | 3 --- 6 files changed, 5 insertions(+), 16 deletions(-) diff --git a/src/runtime/config/configuration-clh.toml.in b/src/runtime/config/configuration-clh.toml.in index e8b6b8c8b0..b3cfbd5836 100644 --- a/src/runtime/config/configuration-clh.toml.in +++ b/src/runtime/config/configuration-clh.toml.in @@ -125,7 +125,8 @@ virtio_fs_cache_size = @DEFVIRTIOFSCACHESIZE@ # # Format example: # ["-o", "arg1=xxx,arg2", "-o", "hello world", "--arg3=yyy"] -# +# Examples: +# Set virtiofsd log level to debug : ["-o", "log_level=debug"] or ["-d"] # see `virtiofsd -h` for possible options. virtio_fs_extra_args = @DEFVIRTIOFSEXTRAARGS@ diff --git a/src/runtime/config/configuration-qemu.toml.in b/src/runtime/config/configuration-qemu.toml.in index 422056f7fa..e0dd09cf6b 100644 --- a/src/runtime/config/configuration-qemu.toml.in +++ b/src/runtime/config/configuration-qemu.toml.in @@ -168,6 +168,8 @@ virtio_fs_cache_size = @DEFVIRTIOFSCACHESIZE@ # # Format example: # ["-o", "arg1=xxx,arg2", "-o", "hello world", "--arg3=yyy"] +# Examples: +# Set virtiofsd log level to debug : ["-o", "log_level=debug"] or ["-d"] # # see `virtiofsd -h` for possible options. virtio_fs_extra_args = @DEFVIRTIOFSEXTRAARGS@ diff --git a/src/runtime/virtcontainers/clh.go b/src/runtime/virtcontainers/clh.go index 95642dd396..d014150666 100644 --- a/src/runtime/virtcontainers/clh.go +++ b/src/runtime/virtcontainers/clh.go @@ -234,7 +234,6 @@ func (clh *cloudHypervisor) createVirtiofsDaemon(sharedPath string) (VirtiofsDae sourcePath: sharedPath, socketPath: virtiofsdSocketPath, extraArgs: clh.config.VirtioFSExtraArgs, - debug: clh.config.Debug, cache: clh.config.VirtioFSCache, }, nil } @@ -302,7 +301,6 @@ func (clh *cloudHypervisor) loadVirtiofsDaemon(sharedPath string) (VirtiofsDaemo return &virtiofsd{ PID: clh.state.VirtiofsDaemonPid, sourcePath: sharedPath, - debug: clh.config.Debug, socketPath: virtiofsdSocketPath, }, nil } diff --git a/src/runtime/virtcontainers/qemu.go b/src/runtime/virtcontainers/qemu.go index 724ad50081..d1da6d291e 100644 --- a/src/runtime/virtcontainers/qemu.go +++ b/src/runtime/virtcontainers/qemu.go @@ -499,7 +499,6 @@ func (q *qemu) createVirtiofsDaemon(sharedPath string) (VirtiofsDaemon, error) { sourcePath: sharedPath, socketPath: virtiofsdSocketPath, extraArgs: q.config.VirtioFSExtraArgs, - debug: q.config.Debug, cache: q.config.VirtioFSCache, }, nil } diff --git a/src/runtime/virtcontainers/virtiofsd.go b/src/runtime/virtcontainers/virtiofsd.go index 6ad0d01cfc..f9c567ce03 100644 --- a/src/runtime/virtcontainers/virtiofsd.go +++ b/src/runtime/virtcontainers/virtiofsd.go @@ -70,8 +70,6 @@ type virtiofsd struct { sourcePath string // extraArgs list of extra args to append to virtiofsd command extraArgs []string - // debug flag - debug bool // PID process ID of virtiosd process PID int } @@ -199,14 +197,8 @@ func (v *virtiofsd) args(FdSocketNumber uint) ([]string, error) { "-o", "source=" + v.sourcePath, // fd number of vhost-user socket fmt.Sprintf("--fd=%v", FdSocketNumber), - } - - if v.debug { - // enable debug output (implies -f) - args = append(args, "-d") - } else { // foreground operation - args = append(args, "-f") + "-f", } if len(v.extraArgs) != 0 { diff --git a/src/runtime/virtcontainers/virtiofsd_test.go b/src/runtime/virtcontainers/virtiofsd_test.go index c2ebc012c3..3705a559b9 100644 --- a/src/runtime/virtcontainers/virtiofsd_test.go +++ b/src/runtime/virtcontainers/virtiofsd_test.go @@ -22,7 +22,6 @@ func TestVirtiofsdStart(t *testing.T) { cache string extraArgs []string sourcePath string - debug bool PID int ctx context.Context } @@ -58,7 +57,6 @@ func TestVirtiofsdStart(t *testing.T) { cache: tt.fields.cache, extraArgs: tt.fields.extraArgs, sourcePath: tt.fields.sourcePath, - debug: tt.fields.debug, PID: tt.fields.PID, ctx: tt.fields.ctx, } @@ -86,7 +84,6 @@ func TestVirtiofsdArgs(t *testing.T) { assert.NoError(err) assert.Equal(expected, strings.Join(args, " ")) - v.debug = false expected = "--syslog -o cache=none -o no_posix_lock -o source=/run/kata-shared/foo --fd=456 -f" args, err = v.args(456) assert.NoError(err) From 1118a3d2da1b4f079ab54f77439e0b1d81873cc0 Mon Sep 17 00:00:00 2001 From: Braden Rayhorn Date: Thu, 7 Apr 2022 17:34:07 -0500 Subject: [PATCH 05/10] agent: add test coverage for parse_mount_flags_and_options function Add test coverage for the parse_mount_flags_and_options function in src/mount.rs. Fixes #4056 Signed-off-by: Braden Rayhorn --- src/agent/src/mount.rs | 51 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/src/agent/src/mount.rs b/src/agent/src/mount.rs index b944ea0936..794724ba4a 100644 --- a/src/agent/src/mount.rs +++ b/src/agent/src/mount.rs @@ -1552,4 +1552,55 @@ mod tests { } } } + + #[test] + fn test_parse_mount_flags_and_options() { + #[derive(Debug)] + struct TestData<'a> { + options_vec: Vec<&'a str>, + result: (MsFlags, &'a str), + } + + let tests = &[ + TestData { + options_vec: vec![], + result: (MsFlags::empty(), ""), + }, + TestData { + options_vec: vec!["ro"], + result: (MsFlags::MS_RDONLY, ""), + }, + TestData { + options_vec: vec!["rw"], + result: (MsFlags::empty(), ""), + }, + TestData { + options_vec: vec!["ro", "rw"], + result: (MsFlags::empty(), ""), + }, + TestData { + options_vec: vec!["ro", "nodev"], + result: (MsFlags::MS_RDONLY | MsFlags::MS_NODEV, ""), + }, + TestData { + options_vec: vec!["option1", "nodev", "option2"], + result: (MsFlags::MS_NODEV, "option1,option2"), + }, + TestData { + options_vec: vec!["rbind", "", "ro"], + result: (MsFlags::MS_BIND | MsFlags::MS_REC | MsFlags::MS_RDONLY, ""), + }, + ]; + + for (i, d) in tests.iter().enumerate() { + let msg = format!("test[{}]: {:?}", i, d); + + let result = parse_mount_flags_and_options(d.options_vec.clone()); + + let msg = format!("{}: result: {:?}", msg, result); + + let expected_result = (d.result.0, d.result.1.to_owned()); + assert_eq!(expected_result, result, "{}", msg); + } + } } From c31cd0e81a8188f53814e8febd10f36f1f397df7 Mon Sep 17 00:00:00 2001 From: garrettmahin Date: Thu, 7 Apr 2022 20:09:11 -0500 Subject: [PATCH 06/10] rustjail: add test coverage for process_grpc_to_oci function Add test coverage for the process_grpc_to_oci function in src/rustjail/lib.rs Fixes #4058 Signed-off-by: Garrett Mahin --- src/agent/rustjail/src/lib.rs | 159 ++++++++++++++++++++++++++++++++++ 1 file changed, 159 insertions(+) diff --git a/src/agent/rustjail/src/lib.rs b/src/agent/rustjail/src/lib.rs index 7535bf9901..5f740cc58c 100644 --- a/src/agent/rustjail/src/lib.rs +++ b/src/agent/rustjail/src/lib.rs @@ -513,6 +513,7 @@ pub fn grpc_to_oci(grpc: &grpc::Spec) -> oci::Spec { #[cfg(test)] mod tests { + use super::*; #[macro_export] macro_rules! skip_if_not_root { () => { @@ -522,4 +523,162 @@ mod tests { } }; } + + #[test] + fn test_process_grpc_to_oci() { + #[derive(Debug)] + struct TestData { + grpcproc: grpc::Process, + result: oci::Process, + } + + let tests = &[ + TestData { + // All fields specified + grpcproc: grpc::Process { + Terminal: true, + ConsoleSize: protobuf::SingularPtrField::::some(grpc::Box { + Height: 123, + Width: 456, + ..Default::default() + }), + User: protobuf::SingularPtrField::::some(grpc::User { + UID: 1234, + GID: 5678, + AdditionalGids: Vec::from([910, 1112]), + Username: String::from("username"), + ..Default::default() + }), + Args: protobuf::RepeatedField::from(Vec::from([ + String::from("arg1"), + String::from("arg2"), + ])), + Env: protobuf::RepeatedField::from(Vec::from([String::from("env")])), + Cwd: String::from("cwd"), + Capabilities: protobuf::SingularPtrField::some(grpc::LinuxCapabilities { + Bounding: protobuf::RepeatedField::from(Vec::from([String::from("bnd")])), + Effective: protobuf::RepeatedField::from(Vec::from([String::from("eff")])), + Inheritable: protobuf::RepeatedField::from(Vec::from([String::from( + "inher", + )])), + Permitted: protobuf::RepeatedField::from(Vec::from([String::from("perm")])), + Ambient: protobuf::RepeatedField::from(Vec::from([String::from("amb")])), + ..Default::default() + }), + Rlimits: protobuf::RepeatedField::from(Vec::from([ + grpc::POSIXRlimit { + Type: String::from("r#type"), + Hard: 123, + Soft: 456, + ..Default::default() + }, + grpc::POSIXRlimit { + Type: String::from("r#type2"), + Hard: 789, + Soft: 1011, + ..Default::default() + }, + ])), + NoNewPrivileges: true, + ApparmorProfile: String::from("apparmor profile"), + OOMScoreAdj: 123456, + SelinuxLabel: String::from("Selinux Label"), + ..Default::default() + }, + result: oci::Process { + terminal: true, + console_size: Some(oci::Box { + height: 123, + width: 456, + }), + user: oci::User { + uid: 1234, + gid: 5678, + additional_gids: Vec::from([910, 1112]), + username: String::from("username"), + }, + args: Vec::from([String::from("arg1"), String::from("arg2")]), + env: Vec::from([String::from("env")]), + cwd: String::from("cwd"), + capabilities: Some(oci::LinuxCapabilities { + bounding: Vec::from([String::from("bnd")]), + effective: Vec::from([String::from("eff")]), + inheritable: Vec::from([String::from("inher")]), + permitted: Vec::from([String::from("perm")]), + ambient: Vec::from([String::from("amb")]), + }), + rlimits: Vec::from([ + oci::PosixRlimit { + r#type: String::from("r#type"), + hard: 123, + soft: 456, + }, + oci::PosixRlimit { + r#type: String::from("r#type2"), + hard: 789, + soft: 1011, + }, + ]), + no_new_privileges: true, + apparmor_profile: String::from("apparmor profile"), + oom_score_adj: Some(123456), + selinux_label: String::from("Selinux Label"), + }, + }, + TestData { + // None ConsoleSize + grpcproc: grpc::Process { + ConsoleSize: protobuf::SingularPtrField::::none(), + OOMScoreAdj: 0, + ..Default::default() + }, + result: oci::Process { + console_size: None, + oom_score_adj: Some(0), + ..Default::default() + }, + }, + TestData { + // None User + grpcproc: grpc::Process { + User: protobuf::SingularPtrField::::none(), + OOMScoreAdj: 0, + ..Default::default() + }, + result: oci::Process { + user: oci::User { + uid: 0, + gid: 0, + additional_gids: vec![], + username: String::from(""), + }, + oom_score_adj: Some(0), + ..Default::default() + }, + }, + TestData { + // None Capabilities + grpcproc: grpc::Process { + Capabilities: protobuf::SingularPtrField::none(), + OOMScoreAdj: 0, + ..Default::default() + }, + result: oci::Process { + capabilities: None, + oom_score_adj: Some(0), + ..Default::default() + }, + }, + ]; + + for (i, d) in tests.iter().enumerate() { + let msg = format!("test[{}]: {:?}", i, d); + + let result = process_grpc_to_oci(&d.grpcproc); + + let msg = format!("{}, result: {:?}", msg, result); + + assert_eq!(d.result, result, "{}", msg); + } + } } From f8cc5d1ad8e1b03754242539c93cb22b4fce0a0d Mon Sep 17 00:00:00 2001 From: bin Date: Fri, 8 Apr 2022 16:32:29 +0800 Subject: [PATCH 07/10] kata-monitor: add some links when generating pages for browsers Add some links to rendered webpages for better user experience, let users can jump to pages only by clicking links in browsers. Fixes: #4061 Signed-off-by: bin --- src/runtime/cmd/kata-monitor/main.go | 35 +++++++++++++++++++++++-- src/runtime/pkg/kata-monitor/metrics.go | 15 +++++++++++ src/runtime/pkg/kata-monitor/monitor.go | 35 +++++++++++++++++++++++++ src/runtime/pkg/kata-monitor/pprof.go | 2 ++ 4 files changed, 85 insertions(+), 2 deletions(-) diff --git a/src/runtime/cmd/kata-monitor/main.go b/src/runtime/cmd/kata-monitor/main.go index 356316dcf2..86b5693d9b 100644 --- a/src/runtime/cmd/kata-monitor/main.go +++ b/src/runtime/cmd/kata-monitor/main.go @@ -175,6 +175,15 @@ func main() { } func indexPage(w http.ResponseWriter, r *http.Request) { + htmlResponse := kataMonitor.IfReturnHTMLResponse(w, r) + if htmlResponse { + indexPageHTML(w, r) + } else { + indexPageText(w, r) + } +} + +func indexPageText(w http.ResponseWriter, r *http.Request) { w.Write([]byte("Available HTTP endpoints:\n")) spacing := 0 @@ -184,13 +193,35 @@ func indexPage(w http.ResponseWriter, r *http.Request) { } } spacing = spacing + 3 + formatter := fmt.Sprintf("%%-%ds: %%s\n", spacing) - formattedString := fmt.Sprintf("%%-%ds: %%s\n", spacing) for _, endpoint := range endpoints { - w.Write([]byte(fmt.Sprintf(formattedString, endpoint.path, endpoint.desc))) + w.Write([]byte(fmt.Sprintf(formatter, endpoint.path, endpoint.desc))) } } +func indexPageHTML(w http.ResponseWriter, r *http.Request) { + + w.Write([]byte("

Available HTTP endpoints:

\n")) + + var formattedString string + needLinkPaths := []string{"/metrics", "/sandboxes"} + + w.Write([]byte("
    ")) + for _, endpoint := range endpoints { + formattedString = fmt.Sprintf("%s: %s\n", endpoint.path, endpoint.desc) + for _, linkPath := range needLinkPaths { + if linkPath == endpoint.path { + formattedString = fmt.Sprintf("%s: %s\n", endpoint.path, endpoint.path, endpoint.desc) + break + } + } + formattedString = fmt.Sprintf("
  • %s
    • ", formattedString) + w.Write([]byte(formattedString)) + } + w.Write([]byte("
")) +} + // initLog setup logger func initLog() { kataMonitorLog := logrus.WithFields(logrus.Fields{ diff --git a/src/runtime/pkg/kata-monitor/metrics.go b/src/runtime/pkg/kata-monitor/metrics.go index 7249906cea..e5d9477670 100644 --- a/src/runtime/pkg/kata-monitor/metrics.go +++ b/src/runtime/pkg/kata-monitor/metrics.go @@ -78,6 +78,21 @@ func (km *KataMonitor) ProcessMetricsRequest(w http.ResponseWriter, r *http.Requ scrapeDurationsHistogram.Observe(float64(time.Since(start).Nanoseconds() / int64(time.Millisecond))) }() + // this is likely the same as `kata-runtime metrics `. + sandboxID, err := getSandboxIDFromReq(r) + if err == nil && sandboxID != "" { + metrics, err := GetSandboxMetrics(sandboxID) + if err != nil { + w.WriteHeader(http.StatusInternalServerError) + w.Write([]byte(err.Error())) + return + } + w.Write([]byte(metrics)) + return + } + + // if no sandbox provided, will get all sandbox's metrics. + // prepare writer for writing response. contentType := expfmt.Negotiate(r.Header) diff --git a/src/runtime/pkg/kata-monitor/monitor.go b/src/runtime/pkg/kata-monitor/monitor.go index ed3ea5c089..1e60a50b7b 100644 --- a/src/runtime/pkg/kata-monitor/monitor.go +++ b/src/runtime/pkg/kata-monitor/monitor.go @@ -27,6 +27,7 @@ const ( RuntimeCRIO = "cri-o" fsMonitorRetryDelaySeconds = 60 podCacheRefreshDelaySeconds = 5 + contentTypeHtml = "text/html" ) // SetLogger sets the logger for katamonitor package. @@ -194,7 +195,41 @@ func (km *KataMonitor) GetAgentURL(w http.ResponseWriter, r *http.Request) { // ListSandboxes list all sandboxes running in Kata func (km *KataMonitor) ListSandboxes(w http.ResponseWriter, r *http.Request) { sandboxes := km.sandboxCache.getSandboxList() + htmlResponse := IfReturnHTMLResponse(w, r) + if htmlResponse { + listSandboxesHtml(sandboxes, w) + } else { + listSandboxesText(sandboxes, w) + } +} + +func listSandboxesText(sandboxes []string, w http.ResponseWriter) { for _, s := range sandboxes { w.Write([]byte(fmt.Sprintf("%s\n", s))) } } +func listSandboxesHtml(sandboxes []string, w http.ResponseWriter) { + w.Write([]byte("

Sandbox list

\n")) + w.Write([]byte("
    \n")) + for _, s := range sandboxes { + w.Write([]byte(fmt.Sprintf("
  • %s: pprof, metrics, agent-url
    • \n", s, s, s, s))) + } + w.Write([]byte("
\n")) +} + +// IfReturnHTMLResponse returns true if request accepts html response +// NOTE: IfReturnHTMLResponse will also set response header to `text/html` +func IfReturnHTMLResponse(w http.ResponseWriter, r *http.Request) bool { + accepts := r.Header["Accept"] + for _, accept := range accepts { + fields := strings.Split(accept, ",") + for _, field := range fields { + if field == contentTypeHtml { + w.Header().Set("Content-Type", contentTypeHtml) + return true + } + } + } + + return false +} diff --git a/src/runtime/pkg/kata-monitor/pprof.go b/src/runtime/pkg/kata-monitor/pprof.go index 5dac2da03c..a2ff76bc30 100644 --- a/src/runtime/pkg/kata-monitor/pprof.go +++ b/src/runtime/pkg/kata-monitor/pprof.go @@ -55,8 +55,10 @@ func (km *KataMonitor) proxyRequest(w http.ResponseWriter, r *http.Request) { } uri := fmt.Sprintf("http://shim%s", r.URL.String()) + monitorLog.Debugf("proxyRequest to: %s, uri: %s", socketAddress, uri) resp, err := client.Get(uri) if err != nil { + serveError(w, http.StatusInternalServerError, fmt.Sprintf("failed to request %s through %s", uri, socketAddress)) return } From 9d5e7ee0d4aeaa74aa2b21acbff2346e67b7bea5 Mon Sep 17 00:00:00 2001 From: Braden Rayhorn Date: Sun, 10 Apr 2022 21:39:46 -0500 Subject: [PATCH 08/10] agent: add tests for mount_storage Add test coverage for mount_storage function in src/mount.rs. Fixes: #4068 Signed-off-by: Braden Rayhorn --- src/agent/src/mount.rs | 124 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 124 insertions(+) diff --git a/src/agent/src/mount.rs b/src/agent/src/mount.rs index 794724ba4a..da46ddae66 100644 --- a/src/agent/src/mount.rs +++ b/src/agent/src/mount.rs @@ -1497,6 +1497,130 @@ mod tests { assert!(testfile.is_file()); } + #[test] + fn test_mount_storage() { + #[derive(Debug)] + struct TestData<'a> { + test_user: TestUserType, + storage: Storage, + error_contains: &'a str, + + make_source_dir: bool, + make_mount_dir: bool, + deny_mount_permission: bool, + } + + impl Default for TestData<'_> { + fn default() -> Self { + TestData { + test_user: TestUserType::Any, + storage: Storage { + mount_point: "mnt".to_string(), + source: "src".to_string(), + fstype: "tmpfs".to_string(), + ..Default::default() + }, + make_source_dir: true, + make_mount_dir: false, + deny_mount_permission: false, + error_contains: "", + } + } + } + + let tests = &[ + TestData { + test_user: TestUserType::NonRootOnly, + error_contains: "EPERM: Operation not permitted", + ..Default::default() + }, + TestData { + test_user: TestUserType::RootOnly, + ..Default::default() + }, + TestData { + storage: Storage { + mount_point: "mnt".to_string(), + source: "src".to_string(), + fstype: "bind".to_string(), + ..Default::default() + }, + make_source_dir: false, + make_mount_dir: true, + error_contains: "Could not create mountpoint", + ..Default::default() + }, + TestData { + test_user: TestUserType::NonRootOnly, + deny_mount_permission: true, + error_contains: "Could not create mountpoint", + ..Default::default() + }, + ]; + + for (i, d) in tests.iter().enumerate() { + let msg = format!("test[{}]: {:?}", i, d); + if d.test_user == TestUserType::RootOnly { + skip_loop_if_not_root!(msg); + } else if d.test_user == TestUserType::NonRootOnly { + skip_loop_if_root!(msg); + } + + let drain = slog::Discard; + let logger = slog::Logger::root(drain, o!()); + + let tempdir = tempdir().unwrap(); + + let source = tempdir.path().join(&d.storage.source); + let mount_point = tempdir.path().join(&d.storage.mount_point); + + let storage = Storage { + source: source.to_str().unwrap().to_string(), + mount_point: mount_point.to_str().unwrap().to_string(), + ..d.storage.clone() + }; + + if d.make_source_dir { + fs::create_dir_all(&storage.source).unwrap(); + } + if d.make_mount_dir { + fs::create_dir_all(&storage.mount_point).unwrap(); + } + + if d.deny_mount_permission { + fs::set_permissions( + mount_point.parent().unwrap(), + fs::Permissions::from_mode(0o000), + ) + .unwrap(); + } + + let result = mount_storage(&logger, &storage); + + // restore permissions so tempdir can be cleaned up + if d.deny_mount_permission { + fs::set_permissions( + mount_point.parent().unwrap(), + fs::Permissions::from_mode(0o755), + ) + .unwrap(); + } + + if result.is_ok() { + nix::mount::umount(&mount_point).unwrap(); + } + + let msg = format!("{}: result: {:?}", msg, result); + if d.error_contains.is_empty() { + assert!(result.is_ok(), "{}", msg); + } else { + assert!(result.is_err(), "{}", msg); + let error_msg = format!("{}", result.unwrap_err()); + assert!(error_msg.contains(d.error_contains), "{}", msg); + } + } + } + #[test] fn test_get_pagesize_and_size_from_option() { let expected_pagesize = 2048; From 6e9e4e8ce538225bdca2b30e59cc9d7fbbaa602f Mon Sep 17 00:00:00 2001 From: Gabriela Cervantes Date: Mon, 11 Apr 2022 15:49:57 +0000 Subject: [PATCH 09/10] docs: Update link to contributions guide This PR updates the url link to the contributions guide at the Limitations document. Fixes #4070 Signed-off-by: Gabriela Cervantes --- docs/Limitations.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/Limitations.md b/docs/Limitations.md index d7b73776c5..1c4cfcb7ba 100644 --- a/docs/Limitations.md +++ b/docs/Limitations.md @@ -46,7 +46,7 @@ The following link shows the latest list of limitations: # Contributing If you would like to work on resolving a limitation, please refer to the -[contributors guide](https://github.com/kata-containers/community/blob/master/CONTRIBUTING.md). +[contributors guide](https://github.com/kata-containers/community/blob/main/CONTRIBUTING.md). If you wish to raise an issue for a new limitation, either [raise an issue directly on the runtime](https://github.com/kata-containers/kata-containers/issues/new) or see the From 78f30c33c6643a9f1c7a8d6f877f3910ee38a3e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= Date: Tue, 12 Apr 2022 10:46:10 +0200 Subject: [PATCH 10/10] agent: Avoid agent panic when reading empty stats MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This was seen in an issue report, where we'd try to unwrap a None value, leading to a panic. Fixes: #4077 Related: #4043 Full backtrace: ``` "thread 'tokio-runtime-worker' panicked at 'called `Option::unwrap()` on a `None` value', rustjail/src/cgroups/fs/mod.rs:593:31" "stack backtrace:" " 0: 0x7f0390edcc3a - std::backtrace_rs::backtrace::libunwind::trace::hd5eff4de16dbdd15" " at /rustc/db9d1b20bba1968c1ec1fc49616d4742c1725b4b/library/std/src/../../backtrace/src/backtrace/libunwind.rs:93:5" " 1: 0x7f0390edcc3a - std::backtrace_rs::backtrace::trace_unsynchronized::h04a775b4c6ab90d6" " at /rustc/db9d1b20bba1968c1ec1fc49616d4742c1725b4b/library/std/src/../../backtrace/src/backtrace/mod.rs:66:5" " 2: 0x7f0390edcc3a - std::sys_common::backtrace::_print_fmt::h3253c3db9f17d826" " at /rustc/db9d1b20bba1968c1ec1fc49616d4742c1725b4b/library/std/src/sys_common/backtrace.rs:67:5" " 3: 0x7f0390edcc3a - ::fmt::h02bfc712fc868664" " at /rustc/db9d1b20bba1968c1ec1fc49616d4742c1725b4b/library/std/src/sys_common/backtrace.rs:46:22" " 4: 0x7f0390a91fbc - core::fmt::write::hfd5090d1132106d8" " at /rustc/db9d1b20bba1968c1ec1fc49616d4742c1725b4b/library/core/src/fmt/mod.rs:1149:17" " 5: 0x7f0390edb804 - std::io::Write::write_fmt::h34acb699c6d6f5a9" " at /rustc/db9d1b20bba1968c1ec1fc49616d4742c1725b4b/library/std/src/io/mod.rs:1697:15" " 6: 0x7f0390edbee0 - std::sys_common::backtrace::_print::hfca761479e3d91ed" " at /rustc/db9d1b20bba1968c1ec1fc49616d4742c1725b4b/library/std/src/sys_common/backtrace.rs:49:5" " 7: 0x7f0390edbee0 - std::sys_common::backtrace::print::hf666af0b87d2b5ba" " at /rustc/db9d1b20bba1968c1ec1fc49616d4742c1725b4b/library/std/src/sys_common/backtrace.rs:36:9" " 8: 0x7f0390edbee0 - std::panicking::default_hook::{{closure}}::hb4617bd1d4a09097" " at /rustc/db9d1b20bba1968c1ec1fc49616d4742c1725b4b/library/std/src/panicking.rs:211:50" " 9: 0x7f0390edb2da - std::panicking::default_hook::h84f684d9eff1eede" " at /rustc/db9d1b20bba1968c1ec1fc49616d4742c1725b4b/library/std/src/panicking.rs:228:9" " 10: 0x7f0390edb2da - std::panicking::rust_panic_with_hook::h8e784f5c39f46346" " at /rustc/db9d1b20bba1968c1ec1fc49616d4742c1725b4b/library/std/src/panicking.rs:606:17" " 11: 0x7f0390f0c416 - std::panicking::begin_panic_handler::{{closure}}::hef496869aa926670" " at /rustc/db9d1b20bba1968c1ec1fc49616d4742c1725b4b/library/std/src/panicking.rs:500:13" " 12: 0x7f0390f0c3b6 - std::sys_common::backtrace::__rust_end_short_backtrace::h8e9b039b8ed3e70f" " at /rustc/db9d1b20bba1968c1ec1fc49616d4742c1725b4b/library/std/src/sys_common/backtrace.rs:139:18" " 13: 0x7f0390f0c372 - rust_begin_unwind" " at /rustc/db9d1b20bba1968c1ec1fc49616d4742c1725b4b/library/std/src/panicking.rs:498:5" " 14: 0x7f03909062c0 - core::panicking::panic_fmt::h568976b83a33ae59" " at /rustc/db9d1b20bba1968c1ec1fc49616d4742c1725b4b/library/core/src/panicking.rs:107:14" " 15: 0x7f039090641c - core::panicking::panic::he2e71cfa6548cc2c" " at /rustc/db9d1b20bba1968c1ec1fc49616d4742c1725b4b/library/core/src/panicking.rs:48:5" " 16: 0x7f0390eb443f - ::get_stats::h85031fc1c59c53d9" " 17: 0x7f03909c0138 - as core::future::future::Future>::poll::hfa6e6cd7516f8d11" " 18: 0x7f0390d697e5 - as core::future::future::Future>::poll::hffbaa534cfa97d44" " 19: 0x7f039099c0b3 - as core::future::future::Future>::poll::hae3ab083a06d0b4b" " 20: 0x7f0390af9e1e - std::panic::catch_unwind::h1fdd25c8ebba32e1" " 21: 0x7f0390b7c4e6 - tokio::runtime::task::raw::poll::hd3ebbd0717dac808" " 22: 0x7f0390f49f3f - tokio::runtime::thread_pool::worker::Context::run_task::hfdd63cd1e0b17abf" " 23: 0x7f0390f3a599 - tokio::runtime::task::raw::poll::h62954f6369b1d210" " 24: 0x7f0390f37863 - std::sys_common::backtrace::__rust_begin_short_backtrace::h1c58f232c078bfe9" " 25: 0x7f0390f4f3dd - core::ops::function::FnOnce::call_once{{vtable.shim}}::h2d329a84c0feed57" " 26: 0x7f0390f0e535 - as core::ops::function::FnOnce>::call_once::h137e5243c6233a3b" " at /rustc/db9d1b20bba1968c1ec1fc49616d4742c1725b4b/library/alloc/src/boxed.rs:1694:9" " 27: 0x7f0390f0e535 - as core::ops::function::FnOnce>::call_once::h7331c46863d912b7" " at /rustc/db9d1b20bba1968c1ec1fc49616d4742c1725b4b/library/alloc/src/boxed.rs:1694:9" " 28: 0x7f0390f0e535 - std::sys::unix::thread::Thread::new::thread_start::h1fb20b966cb927ab" " at /rustc/db9d1b20bba1968c1ec1fc49616d4742c1725b4b/library/std/src/sys/unix/thread.rs:106:17" ``` Signed-off-by: Fabiano Fidêncio --- src/agent/rustjail/src/cgroups/fs/mod.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/agent/rustjail/src/cgroups/fs/mod.rs b/src/agent/rustjail/src/cgroups/fs/mod.rs index b264e5e9d6..035bb8a6b4 100644 --- a/src/agent/rustjail/src/cgroups/fs/mod.rs +++ b/src/agent/rustjail/src/cgroups/fs/mod.rs @@ -590,9 +590,9 @@ fn get_cpuacct_stats(cg: &cgroups::Cgroup) -> SingularPtrField { let h = lines_to_map(&cpuacct.stat); let usage_in_usermode = - (((*h.get("user").unwrap() * NANO_PER_SECOND) as f64) / *CLOCK_TICKS) as u64; + (((*h.get("user").unwrap_or(&0) * NANO_PER_SECOND) as f64) / *CLOCK_TICKS) as u64; let usage_in_kernelmode = - (((*h.get("system").unwrap() * NANO_PER_SECOND) as f64) / *CLOCK_TICKS) as u64; + (((*h.get("system").unwrap_or(&0) * NANO_PER_SECOND) as f64) / *CLOCK_TICKS) as u64; let total_usage = cpuacct.usage; @@ -623,9 +623,9 @@ fn get_cpuacct_stats(cg: &cgroups::Cgroup) -> SingularPtrField { let cpu_controller: &CpuController = get_controller_or_return_singular_none!(cg); let stat = cpu_controller.cpu().stat; let h = lines_to_map(&stat); - let usage_in_usermode = *h.get("user_usec").unwrap(); - let usage_in_kernelmode = *h.get("system_usec").unwrap(); - let total_usage = *h.get("usage_usec").unwrap(); + let usage_in_usermode = *h.get("user_usec").unwrap_or(&0); + let usage_in_kernelmode = *h.get("system_usec").unwrap_or(&0); + let total_usage = *h.get("usage_usec").unwrap_or(&0); let percpu_usage = vec![]; SingularPtrField::some(CpuUsage {