Cleanup container shims, networks and mounts when removing k8s (#145)

* cleanup container shims, networks and mounts when removing k8s

* remove hook can cleanup containerd

* Add e2e test for node cleanup

* update strict patch to fix cleanup shims

* strict cannot delete netns

build-scripts/patches/strict/0001-Strict-patch.patch

@@ -1,13 +1,13 @@
-From 326a0d16dd7dd51742cc30eeba1cfca03af8f16b Mon Sep 17 00:00:00 2001
+From 37c1acf06d05404d89f42202aa19e18871495822 Mon Sep 17 00:00:00 2001
 From: Angelos Kolaitis <[email protected]>
 Date: Sun, 4 Feb 2024 17:39:41 +0200
 Subject: [PATCH] Strict patch
 
 ---
  build-scripts/print-patches-for.py |   2 +-
  k8s/hack/init.sh                   |   6 +-
- snap/snapcraft.yaml                | 168 ++++++++++++++++++++++++++++-
- 3 files changed, 173 insertions(+), 3 deletions(-)
+ snap/snapcraft.yaml                | 169 ++++++++++++++++++++++++++++-
+ 3 files changed, 174 insertions(+), 3 deletions(-)
 
 diff --git a/build-scripts/print-patches-for.py b/build-scripts/print-patches-for.py
 index 2c65083..13ea57c 100755
@@ -36,7 +36,7 @@ index a0b57c7..1507db4 100755
 +"${DIR}/connect-interfaces.sh"
 +"${DIR}/network-requirements.sh"
 diff --git a/snap/snapcraft.yaml b/snap/snapcraft.yaml
-index 2d157e2..0a1309d 100644
+index 2d157e2..ceb9e33 100644
 --- a/snap/snapcraft.yaml
 +++ b/snap/snapcraft.yaml
 @@ -7,7 +7,7 @@ description: |-
@@ -69,7 +69,7 @@ index 2d157e2..0a1309d 100644
    containerd:
      command: k8s/wrappers/services/containerd
      daemon: notify
-@@ -200,35 +214,187 @@ apps:
+@@ -200,35 +214,188 @@ apps:
      stop-mode: sigterm
      restart-condition: always
      start-timeout: 5m
@@ -255,7 +255,8 @@ index 2d157e2..0a1309d 100644
 +    plugs:
 +      - network
 +      - network-bind
++      - process-control
 +      - network-control
 +      - firewall-control
 --
-2.34.1
+2.25.1

k8s/lib.sh

@@ -71,6 +71,21 @@ k8s::remove::network() {
   done
 }
 
+# [DANGER] Cleanup containers and runtime state. Note that the order of operations below is crucial.
+k8s::remove::containers() {
+  k8s::common::setup_env
+
+  # kill all container shims and pause processes
+  k8s::cmd::k8s x-print-shim-pids | xargs -r -t kill -SIGKILL
+
+  # delete cni network namespaces
+  ip netns list | cut -f1 -d' ' | grep -- "^cni-" | xargs -n1 -r -t ip netns delete
+
+  # unmount volumes
+  cat /proc/mounts | grep /run/containerd/io.containerd. | cut -f2 -d' ' | xargs -r -t umount
+  cat /proc/mounts | grep /var/lib/kubelet/pods | cut -f2 -d' ' | xargs -r -t umount
+}
+
 # Run a ctr command against the local containerd socket
 # Example: 'k8s::cmd::ctr image ls -q'
 k8s::cmd::ctr() {
@@ -86,12 +101,6 @@ k8s::cmd::kubectl() {
   env KUBECONFIG="${KUBECONFIG:-/etc/kubernetes/admin.conf}" "${SNAP}/bin/kubectl" "${@}"
 }
 
-# Run snapctl
-# Example: 'k8s::cmd::snapctl start kube-apiserver'
-k8s::cmd::snapctl() {
-  snapctl "${@}"
-}
-
 # Run k8s CLI
 # Example: 'k8s::cmd::k8s status'
 k8s::cmd::k8s() {

k8s/profiles/containerd

@@ -37,4 +37,5 @@ profile cri-containerd.apparmor.d flags=(attach_disconnected,mediate_deleted) {
 
   signal (receive) peer=snap.k8s.containerd,
   signal (receive) peer=snap.k8s.kubelet,
+  signal (receive) peer=snap.k8s.hook.remove,
 }

k8s/wrappers/services/containerd

@@ -7,7 +7,18 @@ k8s::common::setup_env
 # Ensure XDG_RUNTIME_DIR exists, needed by runc for 'kubectl exec' to work
 [ ! -z "${XDG_RUNTIME_DIR:=}" ] && mkdir -p "${XDG_RUNTIME_DIR}"
 
-# Apply the containerd profile
-k8s::common::is_strict && apparmor_parser -r "$SNAP/k8s/profiles/containerd"
+# Apply apparmor profile for "cri-containerd.apparmor.d"
+if ! apparmor_parser -r "$SNAP/k8s/profiles/containerd"; then
+  echo "
+
+WARNING: Failed to configure the 'cri-containerd.apparmor.d' AppArmor profile.
+
+This might affect container shim management (e.g. pods failing to delete).
+You can try to apply the profile manually by running:
+
+    apparmor_parser -r '${SNAP}/k8s/profiles/containerd'
+
+"
+fi
 
 k8s::common::execute_service containerd

snap/hooks/remove

@@ -4,4 +4,6 @@
 
 k8s::common::setup_env
 
+k8s::remove::containers
+
 k8s::remove::network

src/k8s/cmd/k8s/k8s.go

@@ -59,6 +59,7 @@ func NewRootCmd() *cobra.Command {
 	// internal
 	rootCmd.AddCommand(newGenerateAuthTokenCmd())
 	rootCmd.AddCommand(newKubeConfigCmd())
+	rootCmd.AddCommand(xPrintShimPidsCmd)
 
 	// Those commands replace the executable - no need for error wrapping.
 	rootCmd.AddCommand(newHelmCmd())

src/k8s/cmd/k8s/k8s_x_print_shim_pids.go

@@ -0,0 +1,25 @@
+package k8s
+
+import (
+	"fmt"
+
+	"github.com/canonical/k8s/pkg/utils/shims"
+	"github.com/spf13/cobra"
+)
+
+var xPrintShimPidsCmd = &cobra.Command{
+	Use:    "x-print-shim-pids",
+	Short:  "Print list of PIDs started by the containerd shim and pause processes",
+	Hidden: true,
+	RunE: func(cmd *cobra.Command, args []string) error {
+		pids, err := shims.RunningContainerdShimPIDs(cmd.Context())
+		if err != nil {
+			panic(err)
+		}
+		for _, pid := range pids {
+			fmt.Println(pid)
+		}
+
+		return nil
+	},
+}

src/k8s/pkg/utils/shims/shims.go

@@ -0,0 +1,84 @@
+package shims
+
+import (
+	"context"
+	"fmt"
+	"os/exec"
+	"regexp"
+	"strings"
+)
+
+// reBinaryName is the regular expression used to match containerd shim and /pause processes.
+var reBinaryName = regexp.MustCompile(`(^/snap/k8s/.*/bin/containerd-shim-runc-v2|^/pause$)`)
+
+// RunningContainerdShimPIDs returns a list of all the pids on the system that have been started by a containerd shim.
+func RunningContainerdShimPIDs(ctx context.Context) ([]string, error) {
+	procs, err := listAllSystemProcesses(ctx)
+	if err != nil {
+		return nil, fmt.Errorf("failed to list running host processes: %w", err)
+	}
+
+	return findAllChildren(findShimPIDs(procs), makeShallowChildPIDs(procs)), nil
+}
+
+type processInfo struct {
+	command   string
+	parentPID string
+}
+
+// listAllSystemProcesses returns a map of all running processes on the host.
+// for each process, we store the ppid and the command line.
+func listAllSystemProcesses(ctx context.Context) (map[string]processInfo, error) {
+	// output is a list of lines in the following format, one line for each running process:
+	// [pid] [ppid] [arg1 arg2 arg3 ...]
+	// [pid] [ppid] [arg1 arg2 arg3 ...]
+	stdout, err := exec.CommandContext(ctx, "bash", "-c", `ps -e -o pid=,ppid=,args=`).CombinedOutput()
+	if err != nil {
+		return nil, fmt.Errorf("failed to execute ps command (output=%q): %w", stdout, err)
+	}
+
+	result := map[string]processInfo{}
+	for _, line := range strings.Split(string(stdout), "\n") {
+		parts := strings.Fields(line)
+		if len(parts) < 3 {
+			continue
+		}
+
+		result[parts[0]] = processInfo{
+			parentPID: parts[1],
+			command:   strings.Join(parts[2:], " "),
+		}
+	}
+
+	return result, nil
+}
+
+// makeShallowChildPIDs returns a shallow map of the direct children for each process.
+func makeShallowChildPIDs(procs map[string]processInfo) map[string][]string {
+	result := make(map[string][]string, len(procs))
+	for pid, info := range procs {
+		result[info.parentPID] = append(result[info.parentPID], pid)
+	}
+	return result
+}
+
+// findShimPIDs returns the list of PIDs of the parent shim and pause processes.
+func findShimPIDs(procs map[string]processInfo) []string {
+	var result []string
+	for pid, info := range procs {
+		if reBinaryName.MatchString(info.command) {
+			result = append(result, pid)
+		}
+	}
+	return result
+}
+
+// findAllChildren returns a list of all process IDs starting from a given set of parents.
+func findAllChildren(startPIDs []string, shallowChildPIDs map[string][]string) []string {
+	var result []string
+	for _, pid := range startPIDs {
+		result = append(result, pid)
+		result = append(result, findAllChildren(shallowChildPIDs[pid], shallowChildPIDs)...)
+	}
+	return result
+}

tests/e2e/tests/test_cleanup.py

@@ -0,0 +1,54 @@
+#
+# Copyright 2024 Canonical, Ltd.
+#
+import logging
+from typing import List
+
+import pytest
+from e2e_util import harness, util
+
+LOG = logging.getLogger(__name__)
+
+
+@pytest.mark.node_count(1)
+def test_node_cleanup(instances: List[harness.Instance]):
+    instance = instances[0]
+    util.setup_dns(instance)
+
+    LOG.info("Uninstall k8s...")
+    instance.exec(["snap", "remove", "k8s", "--purge"])
+
+    LOG.info("Waiting for shims to go away...")
+    util.stubbornly(retries=5, delay_s=5).on(instance).until(
+        lambda p: all(
+            x not in p.stdout.decode()
+            for x in ["containerd-shim", "cilium", "coredns", "/pause"]
+        )
+    ).exec(["ps", "-fea"])
+
+    LOG.info("Waiting for kubelet and containerd mounts to go away...")
+    util.stubbornly(retries=5, delay_s=5).on(instance).until(
+        lambda p: all(
+            x not in p.stdout.decode()
+            for x in ["/var/lib/kubelet/pods", "/run/containerd/io.containerd"]
+        )
+    ).exec(["mount"])
+
+    # NOTE(neoaggelos): Temporarily disable this as it fails on strict.
+    # For details, `snap changes` then `snap change $remove_k8s_snap_change`.
+    # Example output follows:
+    #
+    # 2024-02-23T14:10:42Z ERROR ignoring failure in hook "remove":
+    # -----
+    # ...
+    # ip netns delete cni-UUID1
+    # Cannot remove namespace file "/run/netns/cni-UUID1": Device or resource busy
+    # ip netns delete cni-UUID2
+    # Cannot remove namespace file "/run/netns/cni-UUID2": Device or resource busy
+    # ip netns delete cni-UUID3
+    # Cannot remove namespace file "/run/netns/cni-UUID3": Device or resource busy
+
+    # LOG.info("Waiting for CNI network namespaces to go away...")
+    # util.stubbornly(retries=5, delay_s=5).on(instance).until(
+    #     lambda p: "cni-" not in p.stdout.decode()
+    # ).exec(["ip", "netns", "list"])