From 1541ce56cf2d8784f570903a0ac5ea871dc029f5 Mon Sep 17 00:00:00 2001
From: Valentin Rothberg <vrothberg@redhat.com>
Date: Fri, 10 Feb 2023 11:33:09 +0100
Subject: [PATCH] kube play: set service container as main PID when possible

Commit 4fa307f14923 fixed a number of issues in the sdnotify proxies.
Whenever a container runs with a custom sdnotify policy, the proxies
need to keep running which in turn required Podman to run and wait for
the service container to stop.  Improve on that behavior and set the
service container as the main PID (instead of Podman) when no container
needs sdnotify.

Fixes: #17345
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
---
 pkg/domain/infra/abi/play.go  | 35 +++++++++++++++++++++++++++++------
 test/system/250-systemd.bats  |  7 ++++---
 test/system/252-quadlet.bats  |  3 ++-
 test/system/260-sdnotify.bats | 10 ++++++++--
 4 files changed, 43 insertions(+), 12 deletions(-)

diff --git a/pkg/domain/infra/abi/play.go b/pkg/domain/infra/abi/play.go
index 1ec71f9b5c..b47ccba48b 100644
--- a/pkg/domain/infra/abi/play.go
+++ b/pkg/domain/infra/abi/play.go
@@ -314,14 +314,37 @@ func (ic *ContainerEngine) PlayKube(ctx context.Context, body io.Reader, options
 		return nil, fmt.Errorf("YAML document does not contain any supported kube kind")
 	}
 
+	// If we started containers along with a service container, we are
+	// running inside a systemd unit and need to set the main PID.
 	if options.ServiceContainer && ranContainers {
-		message := fmt.Sprintf("MAINPID=%d\n%s", os.Getpid(), daemon.SdNotifyReady)
-		if err := notifyproxy.SendMessage("", message); err != nil {
-			return nil, err
-		}
+		switch len(notifyProxies) {
+		case 0: // Optimization for containers/podman/issues/17345
+			// No container needs sdnotify, so we can mark the
+			// service container's conmon as the main PID and
+			// return early.
+			data, err := serviceContainer.Inspect(false)
+			if err != nil {
+				return nil, err
+			}
+			message := fmt.Sprintf("MAINPID=%d\n%s", data.State.ConmonPid, daemon.SdNotifyReady)
+			if err := notifyproxy.SendMessage("", message); err != nil {
+				return nil, err
+			}
+		default:
+			// At least one container has a custom sdnotify policy,
+			// so we need to let the sdnotify proxies run for the
+			// lifetime of the service container.  That means, we
+			// need to wait for the service container to stop.
+			// Podman will hence be marked as the main PID.  That
+			// comes at the cost of keeping Podman running.
+			message := fmt.Sprintf("MAINPID=%d\n%s", os.Getpid(), daemon.SdNotifyReady)
+			if err := notifyproxy.SendMessage("", message); err != nil {
+				return nil, err
+			}
 
-		if _, err := serviceContainer.Wait(ctx); err != nil {
-			return nil, fmt.Errorf("waiting for service container: %w", err)
+			if _, err := serviceContainer.Wait(ctx); err != nil {
+				return nil, fmt.Errorf("waiting for service container: %w", err)
+			}
 		}
 	}
 
diff --git a/test/system/250-systemd.bats b/test/system/250-systemd.bats
index d345765464..b3d8f82af0 100644
--- a/test/system/250-systemd.bats
+++ b/test/system/250-systemd.bats
@@ -399,7 +399,7 @@ EOF
 
     # Make sure that Podman is the service's MainPID
     run systemctl show --property=MainPID --value $service_name
-    is "$(</proc/$output/comm)" "podman" "podman is the service mainPID"
+    is "$(</proc/$output/comm)" "conmon" "podman is the service mainPID"
 
     # The name of the service container is predictable: the first 12 characters
     # of the hash of the YAML file followed by the "-service" suffix
@@ -433,12 +433,13 @@ EOF
     run_podman pod kill test_pod
     for i in {0..20}; do
         run systemctl is-active $service_name
-        if [[ $output == "inactive" ]]; then
+        if [[ $output == "failed" ]]; then
             break
         fi
         sleep 0.5
     done
-    is "$output" "inactive" "systemd service transitioned to 'inactive' state: $service_name"
+    # The service is marked as failed as the service container exits non-zero.
+    is "$output" "failed" "systemd service transitioned to 'inactive' state: $service_name"
 
     # Now stop and start the service again.
     systemctl stop $service_name
diff --git a/test/system/252-quadlet.bats b/test/system/252-quadlet.bats
index 0418f37567..a27f6732c3 100644
--- a/test/system/252-quadlet.bats
+++ b/test/system/252-quadlet.bats
@@ -420,7 +420,8 @@ EOF
     run_podman container inspect  --format "{{.State.Status}}" test_pod-test
     is "$output" "running" "container should be started by systemd and hence be running"
 
-    service_cleanup $QUADLET_SERVICE_NAME inactive
+    # The service is marked as failed as the service container exits non-zero.
+    service_cleanup $QUADLET_SERVICE_NAME failed
     run_podman rmi $(pause_image)
 }
 
diff --git a/test/system/260-sdnotify.bats b/test/system/260-sdnotify.bats
index 484bae4958..5f19f0a7d5 100644
--- a/test/system/260-sdnotify.bats
+++ b/test/system/260-sdnotify.bats
@@ -218,8 +218,14 @@ EOF
     _start_socat
     wait_for_file $_SOCAT_LOG
 
-    # Will run until all containers have stopped.
     run_podman play kube --service-container=true --log-driver journald $yaml_source
+
+    # The service container is the main PID since no container has a custom
+    # sdnotify policy.
+    run_podman container inspect $service_container --format "{{.State.ConmonPid}}"
+    main_pid="$output"
+
+    # Will run until all containers have stopped.
     run_podman container wait $service_container test_pod-test
 
     # Make sure the containers have the correct policy.
@@ -233,7 +239,7 @@ ignore"
     echo "$output"
 
     # The "with policies" test below checks the MAINPID.
-    is "$output" "MAINPID=.*
+    is "$output" "MAINPID=$main_pid
 READY=1" "sdnotify sent MAINPID and READY"
 
     _stop_socat