From bf2c5b1db6b31f837eb2dfa321e755ead907d5c2 Mon Sep 17 00:00:00 2001 From: Charalampos Mainas Date: Tue, 11 Nov 2025 14:25:46 +0000 Subject: [PATCH 1/3] fix(kill): entering container namespace When a new container is created the container's config instructs us on joining or creating anew namespace. Using the same information, we need to enter the correct network namespace and rmeove the tap device that we previously created. THis commit properly handles the above scenario and joins the correct namespace before killing the monitor process and cleaning up urunc's created tap devices. PR: https://github.com/urunc-dev/urunc/pull/337 Signed-off-by: Charalampos Mainas Reviewed-by: Anastassios Nanos Approved-by: Anastassios Nanos --- pkg/unikontainers/unikontainers.go | 71 ++++++++++++++---------------- pkg/unikontainers/utils.go | 12 +++-- 2 files changed, 42 insertions(+), 41 deletions(-) diff --git a/pkg/unikontainers/unikontainers.go b/pkg/unikontainers/unikontainers.go index 194d83bb..f40bff91 100644 --- a/pkg/unikontainers/unikontainers.go +++ b/pkg/unikontainers/unikontainers.go @@ -53,6 +53,7 @@ var uniklog = logrus.WithField("subsystem", "unikontainers") var ErrQueueProxy = errors.New("this a queue proxy container") var ErrNotUnikernel = errors.New("this is not a unikernel container") +var ErrNotExistingNS = errors.New("the namespace does not exist") // Unikontainer holds the data necessary to create, manage and delete unikernel containers type Unikontainer struct { @@ -437,7 +438,11 @@ func (u *Unikontainer) Exec(metrics m.Writer) error { vmmArgs.Command = unikernelCmd // pivot - withPivot := containsNS(u.Spec.Linux.Namespaces, specs.MountNamespace) + _, err = findNS(u.Spec.Linux.Namespaces, specs.MountNamespace) + // We just want to check if a mount namespace was define din the list + // Therefore, if there was no error and the mount namespace was found + // we can pivot. + withPivot := err != nil err = changeRoot(rootfsParams.MonRootfs, withPivot) if err != nil { return err @@ -492,12 +497,27 @@ func setupUser(user specs.User) error { // Kill stops the VMM process, first by asking the VMM struct to stop // and consequently by killing the process described in u.State.Pid func (u *Unikontainer) Kill() error { + // Try to join the Network namespace of the monitor befor ekilling it. + // If we kill it there might be no process inside the namespace and hence + // the namespace gets destroyed. + err := u.joinSandboxNetNs() + if err != nil { + if errors.Is(err, ErrNotExistingNS) { + // There is no network namespace to join. + // Most probably the sandbox is dead and the namespace + // has been destroyed. + uniklog.Infof("could not find sandbox's network namespace: %v", err) + return nil + } + return fmt.Errorf("failed to join sandbox netns: %v", err) + } vmmType := u.State.Annotations[annotHypervisor] // get a new vmm vmm, err := hypervisors.NewVMM(hypervisors.VmmType(vmmType), u.UruncCfg.Hypervisors) if err != nil { return err } + // TODO: Properly implement this and let that code handle the shutdown err = vmm.Stop(u.State.ID) if err != nil { return err @@ -525,14 +545,6 @@ func (u *Unikontainer) Kill() error { time.Sleep(100 * time.Millisecond) } - // If PID is running we need to kill the process - // Once the process is dead, we need to enter the network namespace - // and delete the TC rules and TAP device - err = u.joinSandboxNetNs() - if err != nil { - uniklog.Errorf("failed to join sandbox netns: %v", err) - return nil - } // TODO: tap0_urunc should not be hardcoded err = network.Cleanup("tap0_urunc") if err != nil { @@ -606,39 +618,24 @@ func (u *Unikontainer) Delete() error { return os.RemoveAll(u.BaseDir) } -// joinSandboxNetns joins the network namespace of the sandbox (pause container). +// joinSandboxNetns joins the network namespace of the sandbox // This function should be called only from a locked thread // (i.e. runtime. LockOSThread()) func (u Unikontainer) joinSandboxNetNs() error { - var netNsPath string - // We want enter the network namespace of the container. - // There are two possibilities: - // 1. The unikernel was running inside a Pod and hence we need to join - // the namespace of the pause container - // 2. The unikernel was running in its own network namespace (typical - // in docker, nerdctl etc.). If that is the case, then when the - // unikernel dies/exits the namespace will also die, since there will - // not be any process in that namespace. Therefore, the cleanup will - // happen automatically and we do not need to care about that. - // Therefore, focus only in the first case above. - for _, ns := range u.Spec.Linux.Namespaces { - if ns.Type == specs.NetworkNamespace { - if ns.Path == "" { - // We had to create the network namespace, when - // creating the container. Therefore, the namespace - // will die along with the unikernel. - return nil - } - err := checkValidNsPath(ns.Path) - if err == nil { - netNsPath = ns.Path - } else { - return err - } - break + netNsPath, err := findNS(u.Spec.Linux.Namespaces, specs.NetworkNamespace) + if err != nil && !errors.Is(err, ErrNotExistingNS) { + return err + } + // In case no path was specified for the network namespace it means, + // that we had to create a new one and therefore we can join it by + // using the pid of the monitor process. + if netNsPath == "" { + netNsPath = fmt.Sprintf("/proc/%d/ns/net", u.State.Pid) + err := checkValidNsPath(netNsPath) + if err != nil { + return err } } - uniklog.WithFields(logrus.Fields{ "path": netNsPath, }).Debug("Joining network namespace") diff --git a/pkg/unikontainers/utils.go b/pkg/unikontainers/utils.go index 2bb5de4a..2f61cfd7 100644 --- a/pkg/unikontainers/utils.go +++ b/pkg/unikontainers/utils.go @@ -193,7 +193,7 @@ func remove(s []string, i int) []string { func checkValidNsPath(path string) error { // only set to join this namespace if it exists if _, err := os.Lstat(path); err != nil { - return fmt.Errorf("namespace path: %w", err) + return ErrNotExistingNS } // do not allow namespace path with comma as we use it to separate // the namespace paths @@ -279,14 +279,18 @@ func fileExists(fpath string) bool { } // containsNS checks of the container's configuration contains a specific namespace -func containsNS(namespaces []specs.LinuxNamespace, nsType specs.LinuxNamespaceType) bool { +func findNS(namespaces []specs.LinuxNamespace, nsType specs.LinuxNamespaceType) (string, error) { for _, ns := range namespaces { if ns.Type == nsType { - return true + err := checkValidNsPath(ns.Path) + if err != nil { + return "", err + } + return ns.Path, nil } } - return false + return "", fmt.Errorf("Namespace %s was not found", string(nsType)) } // findQemuDataDir tries to find the location of data and BIOS files for Qemu. From ece2eff6b3ce5da38d995ff382035961c0c9adfb Mon Sep 17 00:00:00 2001 From: Charalampos Mainas Date: Wed, 12 Nov 2025 16:48:12 +0000 Subject: [PATCH 2/3] refactor(kill): Move code to the Stop function of monitors Move the code that actually kills the monitor process in the Stop method of monitors to make it cleaner. PR: https://github.com/urunc-dev/urunc/pull/337 Signed-off-by: Charalampos Mainas Reviewed-by: Anastassios Nanos Approved-by: Anastassios Nanos --- cmd/urunc/delete.go | 17 +++++++++++ pkg/unikontainers/hypervisors/firecracker.go | 4 +-- pkg/unikontainers/hypervisors/hedge.go | 2 +- pkg/unikontainers/hypervisors/hvt.go | 7 ++--- pkg/unikontainers/hypervisors/qemu.go | 4 +-- pkg/unikontainers/hypervisors/spt.go | 7 ++--- pkg/unikontainers/hypervisors/utils.go | 30 ++++++++++++++++++++ pkg/unikontainers/types/types.go | 2 +- pkg/unikontainers/unikontainers.go | 27 ++---------------- 9 files changed, 61 insertions(+), 39 deletions(-) diff --git a/cmd/urunc/delete.go b/cmd/urunc/delete.go index 1154403a..c9f0da01 100644 --- a/cmd/urunc/delete.go +++ b/cmd/urunc/delete.go @@ -16,7 +16,9 @@ package main import ( "context" + "errors" "os" + "path/filepath" "runtime" "github.com/sirupsen/logrus" @@ -54,6 +56,21 @@ status of "ubuntu01" as "stopped" the following will delete resources held for // get Unikontainer data from state.json unikontainer, err := getUnikontainer(cmd) if err != nil { + if errors.Is(err, os.ErrNotExist) { + containerID := cmd.Args().First() + if containerID == "" { + return ErrEmptyContainerID + } + rootDir := cmd.String("root") + containerDir := filepath.Join(rootDir, containerID) + e := os.RemoveAll(containerDir) + if e != nil { + logrus.Errorf("remove %s: %v", containerDir, e) + } + if cmd.Bool("force") { + return nil + } + } return err } if cmd.Bool("force") { diff --git a/pkg/unikontainers/hypervisors/firecracker.go b/pkg/unikontainers/hypervisors/firecracker.go index b47c2527..d950428e 100644 --- a/pkg/unikontainers/hypervisors/firecracker.go +++ b/pkg/unikontainers/hypervisors/firecracker.go @@ -69,8 +69,8 @@ type FirecrackerConfig struct { NetIfs []FirecrackerNet `json:"network-interfaces"` } -func (fc *Firecracker) Stop(_ string) error { - return nil +func (fc *Firecracker) Stop(pid int) error { + return killProcess(pid) } func (fc *Firecracker) Ok() error { diff --git a/pkg/unikontainers/hypervisors/hedge.go b/pkg/unikontainers/hypervisors/hedge.go index bbca6d26..b58b4801 100644 --- a/pkg/unikontainers/hypervisors/hedge.go +++ b/pkg/unikontainers/hypervisors/hedge.go @@ -33,7 +33,7 @@ func (h *Hedge) Ok() error { return fmt.Errorf("hedge not implemented yet") } -func (h *Hedge) Stop(_ string) error { +func (h *Hedge) Stop(_ int) error { return fmt.Errorf("hedge not implemented yet") } diff --git a/pkg/unikontainers/hypervisors/hvt.go b/pkg/unikontainers/hypervisors/hvt.go index 12bedf71..e758fa69 100644 --- a/pkg/unikontainers/hypervisors/hvt.go +++ b/pkg/unikontainers/hypervisors/hvt.go @@ -120,10 +120,9 @@ func applySeccompFilter() error { return nil } -// Stop is an empty function to satisfy VMM interface compatibility requirements. -// It does not perform any actions and always returns nil. -func (h *HVT) Stop(_ string) error { - return nil +// Stop kills the hvt process +func (h *HVT) Stop(pid int) error { + return killProcess(pid) } // UsesKVM returns a bool value depending on if the monitor uses KVM diff --git a/pkg/unikontainers/hypervisors/qemu.go b/pkg/unikontainers/hypervisors/qemu.go index bad9b5c4..814eac2f 100644 --- a/pkg/unikontainers/hypervisors/qemu.go +++ b/pkg/unikontainers/hypervisors/qemu.go @@ -33,8 +33,8 @@ type Qemu struct { binary string } -func (q *Qemu) Stop(_ string) error { - return nil +func (q *Qemu) Stop(pid int) error { + return killProcess(pid) } func (q *Qemu) Ok() error { diff --git a/pkg/unikontainers/hypervisors/spt.go b/pkg/unikontainers/hypervisors/spt.go index ca210067..c2e9eaec 100644 --- a/pkg/unikontainers/hypervisors/spt.go +++ b/pkg/unikontainers/hypervisors/spt.go @@ -32,10 +32,9 @@ type SPT struct { binary string } -// Stop is an empty function to satisfy VMM interface compatibility requirements. -// It does not perform any actions and always returns nil. -func (s *SPT) Stop(_ string) error { - return nil +// Stop kills the spt process +func (s *SPT) Stop(pid int) error { + return killProcess(pid) } // UsesKVM returns a bool value depending on if the monitor uses KVM diff --git a/pkg/unikontainers/hypervisors/utils.go b/pkg/unikontainers/hypervisors/utils.go index 6e5872d4..5b78c17f 100644 --- a/pkg/unikontainers/hypervisors/utils.go +++ b/pkg/unikontainers/hypervisors/utils.go @@ -15,8 +15,14 @@ package hypervisors import ( + "errors" + "fmt" "runtime" "strconv" + "syscall" + "time" + + "golang.org/x/sys/unix" ) func cpuArch() string { @@ -60,3 +66,27 @@ func BytesToStringMB(argMem uint64) string { return stringMem } + +func killProcess(pid int) error { + const timeout = 2 * time.Second + err := syscall.Kill(pid, unix.SIGKILL) + if err != nil { + return err + } + deadline := time.Now().Add(timeout) + for { + if err := syscall.Kill(pid, 0); err != nil { + if errors.Is(err, syscall.ESRCH) { + // process is dead + break + } + return fmt.Errorf("error checking if process with pid %d is alive: %w", pid, err) + } + if time.Now().After(deadline) { + return fmt.Errorf("timeout waiting for pid %d to die", pid) + } + time.Sleep(100 * time.Millisecond) + } + + return nil +} diff --git a/pkg/unikontainers/types/types.go b/pkg/unikontainers/types/types.go index a6bc38f5..0c1915ae 100644 --- a/pkg/unikontainers/types/types.go +++ b/pkg/unikontainers/types/types.go @@ -26,7 +26,7 @@ type Unikernel interface { type VMM interface { Execve(args ExecArgs, ukernel Unikernel) error - Stop(t string) error + Stop(int) error Path() string UsesKVM() bool SupportsSharedfs(string) bool diff --git a/pkg/unikontainers/unikontainers.go b/pkg/unikontainers/unikontainers.go index f40bff91..3bd5e4fc 100644 --- a/pkg/unikontainers/unikontainers.go +++ b/pkg/unikontainers/unikontainers.go @@ -29,7 +29,6 @@ import ( "strings" "sync" "syscall" - "time" "github.com/urunc-dev/urunc/pkg/network" "github.com/urunc-dev/urunc/pkg/unikontainers/hypervisors" @@ -517,39 +516,17 @@ func (u *Unikontainer) Kill() error { if err != nil { return err } - // TODO: Properly implement this and let that code handle the shutdown - err = vmm.Stop(u.State.ID) + err = vmm.Stop(u.State.Pid) if err != nil { return err } - // Check if pid is running - if syscall.Kill(u.State.Pid, syscall.Signal(0)) == nil { - err = syscall.Kill(u.State.Pid, unix.SIGKILL) - if err != nil { - return err - } - } - const timeout = 2 * time.Second - deadline := time.Now().Add(timeout) - for { - if err := syscall.Kill(u.State.Pid, 0); err != nil { - if errors.Is(err, syscall.ESRCH) { - break // process is dead - } - return fmt.Errorf("error checking pid %d: %w", u.State.Pid, err) - } - if time.Now().After(deadline) { - return fmt.Errorf("timeout waiting for pid %d to die", u.State.Pid) - } - time.Sleep(100 * time.Millisecond) - } - // TODO: tap0_urunc should not be hardcoded err = network.Cleanup("tap0_urunc") if err != nil { uniklog.Errorf("failed to delete tap0_urunc: %v", err) } + return nil } From 7e2409432d9c548a01b7067a21576fd7ed9bd2f0 Mon Sep 17 00:00:00 2001 From: Charalampos Mainas Date: Mon, 17 Nov 2025 13:15:21 +0000 Subject: [PATCH 3/3] refactor(delete): Simplify directories removal Refactor the cod eof thr Delete function in unikontainers to simplify the process of rmeoving the directories that urunc has created. Furthermore, remove the path to the monitor binary in the new container's rootfs, covering also the case that the binary is not placed under one of the removed directories. PR: https://github.com/urunc-dev/urunc/pull/337 Signed-off-by: Charalampos Mainas Reviewed-by: Anastassios Nanos Approved-by: Anastassios Nanos --- pkg/unikontainers/unikontainers.go | 84 +++++++++++++++--------------- pkg/unikontainers/utils.go | 11 ++++ tests/e2e/e2e_test.go | 2 +- 3 files changed, 53 insertions(+), 44 deletions(-) diff --git a/pkg/unikontainers/unikontainers.go b/pkg/unikontainers/unikontainers.go index 3bd5e4fc..1b4d7adc 100644 --- a/pkg/unikontainers/unikontainers.go +++ b/pkg/unikontainers/unikontainers.go @@ -25,7 +25,6 @@ import ( "os/exec" "path/filepath" "runtime" - "strconv" "strings" "sync" "syscall" @@ -496,7 +495,7 @@ func setupUser(user specs.User) error { // Kill stops the VMM process, first by asking the VMM struct to stop // and consequently by killing the process described in u.State.Pid func (u *Unikontainer) Kill() error { - // Try to join the Network namespace of the monitor befor ekilling it. + // Try to join the Network namespace of the monitor before killing it. // If we kill it there might be no process inside the namespace and hence // the namespace gets destroyed. err := u.joinSandboxNetNs() @@ -510,8 +509,9 @@ func (u *Unikontainer) Kill() error { } return fmt.Errorf("failed to join sandbox netns: %v", err) } - vmmType := u.State.Annotations[annotHypervisor] + // get a new vmm + vmmType := u.State.Annotations[annotHypervisor] vmm, err := hypervisors.NewVMM(hypervisors.VmmType(vmmType), u.UruncCfg.Hypervisors) if err != nil { return err @@ -532,66 +532,64 @@ func (u *Unikontainer) Kill() error { // Delete removes the containers base directory and its contents func (u *Unikontainer) Delete() error { + var dirs []string + var prefPath string + if u.isRunning() { - return fmt.Errorf("cannot delete running unikernel: %s", u.State.ID) + return fmt.Errorf("cannot delete running container: %s", u.State.ID) } + + // get a monitor instance of the running monitor + vmmType := u.State.Annotations[annotHypervisor] + vmm, err := hypervisors.NewVMM(hypervisors.VmmType(vmmType), u.UruncCfg.Hypervisors) + if err != nil { + return err + } + // Make sure paths are clean bundleDir := filepath.Clean(u.State.Bundle) rootfsDir := filepath.Clean(u.Spec.Root.Path) if !filepath.IsAbs(rootfsDir) { rootfsDir = filepath.Join(bundleDir, rootfsDir) } + monRootfs := filepath.Join(bundleDir, monitorRootfsDirName) - // Check if we used a different directory for monitor's rootfs than the - // container's one. - withRootfsMount := false - withRootfsMount, err := strconv.ParseBool(u.State.Annotations[annotMountRootfs]) - if err != nil { - withRootfsMount = false - } - annotBlock := u.State.Annotations[annotBlock] - // TODO: We might not need to remove all these directories. - if annotBlock == "" && withRootfsMount { + // TODO: We might not need to remove any of the directories and let + // the kernel cleanup the mounts and shim to remove directories. + // However, just to be on the safe side, we remove all the newly + // created directories from urunc. In order to check if we used the + // rootfs under the bundle directory or we create anew one, we can check + // if the monitorRootfsDirName directory exists under the bundle. + _, err = os.Stat(monRootfs) + if !os.IsNotExist(err) { // Since there was no no block defined for the unikernel // and we created a new rootfs for the monitor, we need to // clean it up. - monRootfs := filepath.Join(bundleDir, monitorRootfsDirName) - err = os.RemoveAll(monRootfs) - if err != nil { - return fmt.Errorf("cannot remove %s: %v", monRootfs, err) - } + dirs = append(dirs, monitorRootfsDirName) + prefPath = bundleDir } else { // Otherwise remove the enw directories we created inside the // container's rootfs. - cntrDev := filepath.Join(rootfsDir, "/dev") - err = os.RemoveAll(cntrDev) - if err != nil { - return fmt.Errorf("cannot remove /dev: %v", err) - } - cntrTmp := filepath.Join(rootfsDir, "/tmp") - err = os.RemoveAll(cntrTmp) - if err != nil { - return fmt.Errorf("cannot remove /tmp: %v", err) - } - cntrLib := filepath.Join(rootfsDir, "/lib") - err = os.RemoveAll(cntrLib) - if err != nil { - return fmt.Errorf("cannot remove /lib: %v", err) - } - cntrLib64 := filepath.Join(rootfsDir, "/lib64") - err = os.RemoveAll(cntrLib64) - if err != nil { - return fmt.Errorf("cannot remove /lib64: %v", err) - } // We do not need to unmount anything here, since we rely on Linux // to do the cleanup for us. This will happen automatically, // when the mount namespace gets destroyed - cntrUsr := filepath.Join(rootfsDir, "/usr") - err = os.RemoveAll(cntrUsr) - if err != nil { - return fmt.Errorf("cannot remove /usr: %v", err) + dirs = []string{ + "/lib", + "/lib64", + "/usr", + "/proc", + "/dev", + "/tmp", } + dirs = append(dirs, vmm.Path()) + prefPath = rootfsDir } + + err = rmMultipleDirs(prefPath, dirs) + if err != nil { + return err + } + return os.RemoveAll(u.BaseDir) } diff --git a/pkg/unikontainers/utils.go b/pkg/unikontainers/utils.go index 2f61cfd7..9316f460 100644 --- a/pkg/unikontainers/utils.go +++ b/pkg/unikontainers/utils.go @@ -323,3 +323,14 @@ func findQemuDataDir(basename string) (string, error) { return qdPath, nil } + +func rmMultipleDirs(prefixPath string, dirs []string) error { + for _, d := range dirs { + path := filepath.Join(prefixPath, d) + if err := os.RemoveAll(path); err != nil { + return fmt.Errorf("cannot remove %s: %w", d, err) + } + } + + return nil +} diff --git a/tests/e2e/e2e_test.go b/tests/e2e/e2e_test.go index 4f16f33f..c5763181 100644 --- a/tests/e2e/e2e_test.go +++ b/tests/e2e/e2e_test.go @@ -843,7 +843,7 @@ func TestCrictl(t *testing.T) { TestFunc: pingTest, }, { - Image: "harbor.nbfc.io/nubificus/urunc/nginx-firecracker-unikraft:latest", + Image: "harbor.nbfc.io/nubificus/urunc/nginx-firecracker-unikraft-initrd:latest", Name: "Firecracker-unikraft-nginx", Devmapper: false, Seccomp: true,