From 93fa1053d10756e5b361fbfc972ab1177533e54e Mon Sep 17 00:00:00 2001
From: Martin Kalcok <martin.kalcok@canonical.com>
Date: Thu, 2 Nov 2023 15:43:32 +0100
Subject: [PATCH] Cleanup OVS resource on snap removal.

This change modifies `microovn.switch` service to stop OVS vswitchd
service with `--cleanup` flag. This ensures that when snap is removed,
datapath resources like ports and bridges are cleaned up.

This new behavior is overriden in pre-refresh hook to avoid dataplane
outages during the snap refresh.

(cherry picked from commit 8be9f0b80e4c38dd054cc5a41858e1b829d90b14)
Signed-off-by: Martin Kalcok <martin.kalcok@canonical.com>
---
 snap/hooks/pre-refresh                |  9 ++++
 snap/snapcraft.yaml                   |  2 +-
 tests/lifecycle.bats                  |  1 +
 tests/test_helper/bats/lifecycle.bats | 63 +++++++++++++++++++++++++++
 4 files changed, 74 insertions(+), 1 deletion(-)
 create mode 100755 snap/hooks/pre-refresh
 create mode 120000 tests/lifecycle.bats
 create mode 100644 tests/test_helper/bats/lifecycle.bats

diff --git a/snap/hooks/pre-refresh b/snap/hooks/pre-refresh
new file mode 100755
index 00000000..c63bf705
--- /dev/null
+++ b/snap/hooks/pre-refresh
@@ -0,0 +1,9 @@
+#!/bin/sh
+
+# Note (mkalcok): `microovn.switch` service, by default, stops OVS
+# vswitch daemon with `--cleanup` flag that releases datapath
+# resources like ports and bridges. This hook prevents such behavior
+# by stopping the daemon without `--cleanup` flag during the snap
+# refresh.
+
+${SNAP}/commands/ovs-appctl exit || true
diff --git a/snap/snapcraft.yaml b/snap/snapcraft.yaml
index dd631f13..bda589f3 100644
--- a/snap/snapcraft.yaml
+++ b/snap/snapcraft.yaml
@@ -74,7 +74,7 @@ apps:
     command: commands/switch.start
     daemon: simple
     install-mode: disable
-    refresh-mode: endure
+    stop-command: commands/ovs-appctl exit --cleanup
     plugs:
       - firewall-control
       - hardware-observe
diff --git a/tests/lifecycle.bats b/tests/lifecycle.bats
new file mode 120000
index 00000000..eecf31b4
--- /dev/null
+++ b/tests/lifecycle.bats
@@ -0,0 +1 @@
+test_helper/bats/lifecycle.bats
\ No newline at end of file
diff --git a/tests/test_helper/bats/lifecycle.bats b/tests/test_helper/bats/lifecycle.bats
new file mode 100644
index 00000000..a03de9e5
--- /dev/null
+++ b/tests/test_helper/bats/lifecycle.bats
@@ -0,0 +1,63 @@
+# This is a bash shell fragment -*- bash -*-
+
+setup_file() {
+    load test_helper/common.bash
+    load test_helper/lxd.bash
+    load test_helper/microovn.bash
+
+
+    TEST_CONTAINERS=$(container_names "$BATS_TEST_FILENAME" 1)
+    export TEST_CONTAINERS
+    launch_containers jammy $TEST_CONTAINERS
+    wait_containers_ready $TEST_CONTAINERS
+}
+
+teardown_file() {
+    delete_containers $TEST_CONTAINERS
+}
+
+setup() {
+    load test_helper/common.bash
+    load test_helper/lxd.bash
+    load test_helper/microovn.bash
+    load ${ABS_TOP_TEST_DIRNAME}../.bats/bats-support/load.bash
+    load ${ABS_TOP_TEST_DIRNAME}../.bats/bats-assert/load.bash
+
+    # Ensure TEST_CONTAINERS is populated, otherwise the tests below will
+    # provide false positive results.
+    assert [ -n "$TEST_CONTAINERS" ]
+
+    # Trim trailing whitespace from a variable with only single container
+    TEST_CONTAINER="$(echo -e "${TEST_CONTAINERS}" | sed -e 's/[[:space:]]*$//')"
+    export TEST_CONTAINER
+}
+
+teardown() {
+    lxc_exec "$TEST_CONTAINER" "snap remove microovn" || true
+}
+
+@test "Cleanup OVS datapaths on snap removal" {
+    # Verify that removal of MicroOVN snap cleans up DP resources
+
+    # The tests will need external `ovs-dpctl` command to check DPs after
+    # microovn removal.
+    lxc_exec "$TEST_CONTAINER" "DEBIAN_FRONTEND=noninteractive apt install -yqq openvswitch-switch"
+
+    echo "Checking datapaths on container '$TEST_CONTAINER' before MicroOVN installation."
+    run lxc_exec "$TEST_CONTAINER" "ovs-dpctl dump-dps | wc -l"
+    assert_output "0"
+
+    install_microovn "$MICROOVN_SNAP_PATH" "$TEST_CONTAINER"
+    bootstrap_cluster "$TEST_CONTAINER"
+
+    echo "Checking datapaths on container '$TEST_CONTAINER' after MicroOVN bootstrap."
+    run lxc_exec "$TEST_CONTAINER" "ovs-dpctl dump-dps | wc -l"
+    assert_output "1"
+
+    echo "Removing MicroOVN snap."
+    run lxc_exec "$TEST_CONTAINER" "snap remove microovn"
+
+    echo "Checking datapaths on container '$TEST_CONTAINER' after MicroOVN removal."
+    run lxc_exec "$TEST_CONTAINER" "ovs-dpctl dump-dps | wc -l"
+    assert_output "0"
+}