From d5db65562f2679469f2cfeb98dd2d95dae141e86 Mon Sep 17 00:00:00 2001
From: Yatin Karel <ykarel@redhat.com>
Date: Fri, 15 Mar 2024 03:46:30 -0400
Subject: [PATCH] Fix preStop Hooks for ovn-controller pod containers

preStop Hooks currently fails with:-
ovs-ctl: exactly one non-option argument required
becaues ";sleep 2" is passed to the stop command which
it do not expect.

To run additional commands we could use "bash -c" but
considering the extra sleep do not help to avoid the
"FailedPreStopHook" event just dropping the extra sleep as
part of this patch.

"FailedPreStopHook" event gets raised even with successful
graceful shutdown because container get's killed due to
these stop commands.

To avoid "FailedPreStopHook" one option is to use wrapper
startup script and handle SIGTERM signals, something like
revert of [1], but leaving that for later.

Also add kuttl test to validate graceful shutdown of
ovn-controller by checking SB DB for the chassis.

[1] https://github.com/openstack-k8s-operators/ovs-operator/pull/53

Resolves: OSPRH-4553
---
 pkg/ovncontroller/daemonset.go                      |  9 +++------
 tests/kuttl/common/assert_sample_deployment.yaml    | 12 ++++++++++++
 tests/kuttl/tests/ovn_config/03-assert.yaml         | 13 +++++++++++++
 .../tests/ovn_config/03-remove-ovncontroller.yaml   |  5 +++++
 4 files changed, 33 insertions(+), 6 deletions(-)
 create mode 100644 tests/kuttl/tests/ovn_config/03-assert.yaml
 create mode 100644 tests/kuttl/tests/ovn_config/03-remove-ovncontroller.yaml

diff --git a/pkg/ovncontroller/daemonset.go b/pkg/ovncontroller/daemonset.go
index d000d9c2..e324175d 100644
--- a/pkg/ovncontroller/daemonset.go
+++ b/pkg/ovncontroller/daemonset.go
@@ -108,9 +108,8 @@ func DaemonSet(
 	ovsDbArgs = []string{
 		"--single-child", "--", "/usr/local/bin/container-scripts/start-ovsdb-server.sh",
 	}
-	// sleep is required as workaround for https://github.com/kubernetes/kubernetes/issues/39170
 	ovsDbPreStopCmd = []string{
-		"/usr/share/openvswitch/scripts/ovs-ctl", "stop", "--no-ovs-vswitchd", ";", "sleep", "2",
+		"/usr/share/openvswitch/scripts/ovs-ctl", "stop", "--no-ovs-vswitchd",
 	}
 
 	ovsVswitchdLivenessProbe.Exec = &corev1.ExecAction{
@@ -125,9 +124,8 @@ func DaemonSet(
 	ovsVswitchdArgs = []string{
 		"--pidfile", "--mlockall",
 	}
-	// sleep is required as workaround for https://github.com/kubernetes/kubernetes/issues/39170
 	ovsVswitchdPreStopCmd = []string{
-		"/usr/share/openvswitch/scripts/ovs-ctl", "stop", "--no-ovsdb-server", ";", "sleep", "2",
+		"/usr/share/openvswitch/scripts/ovs-ctl", "stop", "--no-ovsdb-server",
 	}
 
 	ovnControllerCmd = []string{
@@ -142,9 +140,8 @@ func DaemonSet(
 			" ",
 		),
 	}
-	// sleep is required as workaround for https://github.com/kubernetes/kubernetes/issues/39170
 	ovnControllerPreStopCmd = []string{
-		"/usr/share/ovn/scripts/ovn-ctl", "stop_controller", ";", "sleep", "2",
+		"/usr/share/ovn/scripts/ovn-ctl", "stop_controller",
 	}
 
 	envVars := map[string]env.Setter{}
diff --git a/tests/kuttl/common/assert_sample_deployment.yaml b/tests/kuttl/common/assert_sample_deployment.yaml
index c1e4ce88..b8873d63 100644
--- a/tests/kuttl/common/assert_sample_deployment.yaml
+++ b/tests/kuttl/common/assert_sample_deployment.yaml
@@ -330,3 +330,15 @@ commands:
         done
 
         exit 0
+---
+# check that the chassis is registered in sb db
+apiVersion: kuttl.dev/v1beta1
+kind: TestAssert
+commands:
+    - script: |
+        controller_pod=$(oc get pod -n $NAMESPACE -l service=ovn-controller -o name|head -1)
+        host=$(oc rsh -n $NAMESPACE ${controller_pod} ovs-vsctl get open . external_ids:hostname)
+
+        sb_pod=$(oc get pod -n $NAMESPACE -l service=ovsdbserver-sb -o name|head -1)
+        oc rsh -n $NAMESPACE ${sb_pod} ovn-sbctl list chassis | grep -q ${host} || exit 1
+        exit 0
diff --git a/tests/kuttl/tests/ovn_config/03-assert.yaml b/tests/kuttl/tests/ovn_config/03-assert.yaml
new file mode 100644
index 00000000..92abb5a6
--- /dev/null
+++ b/tests/kuttl/tests/ovn_config/03-assert.yaml
@@ -0,0 +1,13 @@
+#
+# Check for:
+#
+# - chassis unregistered in sb db
+
+apiVersion: kuttl.dev/v1beta1
+kind: TestAssert
+timeout: 30
+commands:
+    - script: |
+        sb_pod=$(oc get pod -n $NAMESPACE -l service=ovsdbserver-sb -o name|head -1)
+        oc rsh -n $NAMESPACE ${sb_pod} ovn-sbctl list chassis | grep -q hostname && exit 1
+        exit 0
diff --git a/tests/kuttl/tests/ovn_config/03-remove-ovncontroller.yaml b/tests/kuttl/tests/ovn_config/03-remove-ovncontroller.yaml
new file mode 100644
index 00000000..ef300220
--- /dev/null
+++ b/tests/kuttl/tests/ovn_config/03-remove-ovncontroller.yaml
@@ -0,0 +1,5 @@
+apiVersion: kuttl.dev/v1beta1
+kind: TestStep
+commands:
+  - script: |
+      oc patch OVNController -n $NAMESPACE ovncontroller-sample --type='json' -p='[{"op": "replace", "path": "/spec/nodeSelector", "value":{"node": "non-existing-node-name"}}]'