Fix issue with --rm and service in Quadlet (#982)

Fix #913

Signed-off-by: Sagi Shnaidman <sshnaidm@redhat.com>

plugins/module_utils/podman/quadlet.py

@@ -21,6 +21,7 @@ class Quadlet:
 
     def __init__(self, section: str, params: dict):
         self.section = section
+        self.service_section = {}
         self.custom_params = self.custom_prepare_params(params)
         self.dict_params = self.prepare_params()
 
@@ -31,6 +32,20 @@ class Quadlet:
         # This should be implemented in child classes if needed.
         return params
 
+    def _map_restart_policy(self, policy: str) -> str:
+        """
+        Map Podman restart policies to systemd Restart values.
+        """
+        # Podman restart policies: no, on-failure, always, unless-stopped
+        # systemd Restart values: no, on-success, on-failure, on-abnormal, on-watchdog, on-abort, always
+        policy_map = {
+            "no": "no",
+            "on-failure": "on-failure",
+            "always": "always",
+            "unless-stopped": "always",  # systemd doesn't have unless-stopped, use always
+        }
+        return policy_map.get(policy, policy)
+
     def prepare_params(self) -> dict:
         """
         Convert parameter values as per param_map.
@@ -55,10 +70,22 @@ class Quadlet:
         Construct the quadlet content as a string.
         """
         custom_user_options = self.custom_params.get("quadlet_options")
+
+        # Build main section
+        content = f"[{self.section}]\n" + "\n".join(f"{key}={value}" for key, value in self.dict_params)
+
+        # Add Service section if there are service parameters
+        # Check if user hasn't already added a [Service] section in quadlet_options
+        has_service_in_custom = False
+        if custom_user_options:
+            has_service_in_custom = any("[Service]" in opt for opt in custom_user_options)
+
+        if self.service_section and not has_service_in_custom:
+            service_text = "\n\n[Service]\n" + "\n".join(f"{key}={value}" for key, value in self.service_section.items())
+            content += service_text
+
         custom_text = "\n" + "\n".join(custom_user_options) if custom_user_options else ""
-        return (
-            f"[{self.section}]\n" + "\n".join(f"{key}={value}" for key, value in self.dict_params) + custom_text + "\n"
-        )
+        return content + custom_text + "\n"
 
     def write_to_file(self, path: str):
         """
@@ -341,8 +368,10 @@ class ContainerQuadlet(Quadlet):
             params["podman_args"].append(f"--rdt-class {params['rdt_class']}")
         if params["requires"]:
             params["podman_args"].append(f"--requires {','.join(params['requires'])}")
+        # restart_policy is handled in the [Service] section, not as a podman argument
+        # to avoid conflicts with --rm option in Quadlet files
         if params["restart_policy"]:
-            params["podman_args"].append(f"--restart {params['restart_policy']}")
+            self.service_section["Restart"] = self._map_restart_policy(params["restart_policy"])
         if params["retry"]:
             params["podman_args"].append(f"--retry {params['retry']}")
         if params["retry_delay"]:
@@ -530,8 +559,10 @@ class PodQuadlet(Quadlet):
             params["podman_args"].append(f"--pid {params['pid']}")
         if params["pod_id_file"]:
             params["podman_args"].append(f"--pod-id-file {params['pod_id_file']}")
+        # restart_policy is handled in the [Service] section, not as a podman argument
+        # to avoid conflicts with --rm option in Quadlet files
         if params["restart_policy"]:
-            params["podman_args"].append(f"--restart={params['restart_policy']}")
+            self.service_section["Restart"] = self._map_restart_policy(params["restart_policy"])
         if params["security_opt"]:
             for security_opt in params["security_opt"]:
                 params["podman_args"].append(f"--security-opt {security_opt}")

tests/integration/targets/podman_container/tasks/main.yml

@@ -1523,6 +1523,102 @@
           - quad3 is changed
           - "'127.0.0.45' in quad3.diff.after"
 
+    # Test for issue #913: restart_policy should go to [Service] section, not PodmanArgs
+    - name: Create a Quadlet for container with restart_policy
+      containers.podman.podman_container:
+        executable: "{{ test_executable | default('podman') }}"
+        name: container-restart-test
+        image: alpine
+        state: quadlet
+        quadlet_dir: /tmp
+        restart_policy: always
+      register: quad_restart
+
+    - name: Check if Quadlet file contains [Service] section with Restart
+      shell: cat /tmp/container-restart-test.container
+      register: quad_restart_content
+
+    - name: Verify restart_policy is in [Service] section and not in PodmanArgs
+      assert:
+        that:
+          - quad_restart is changed
+          - "'[Service]' in quad_restart_content.stdout"
+          - "'Restart=always' in quad_restart_content.stdout"
+          - "'--restart' not in quad_restart_content.stdout"
+        fail_msg: "restart_policy should be in [Service] section, not as --restart in PodmanArgs"
+
+    - name: Test different restart policies
+      containers.podman.podman_container:
+        executable: "{{ test_executable | default('podman') }}"
+        name: "container-restart-{{ item.name }}"
+        image: alpine
+        state: quadlet
+        quadlet_dir: /tmp
+        restart_policy: "{{ item.policy }}"
+      loop:
+        - { name: "on-failure", policy: "on-failure" }
+        - { name: "unless-stopped", policy: "unless-stopped" }
+      register: quad_restart_policies
+
+    - name: Verify all restart policies are correctly mapped
+      shell: cat /tmp/container-restart-{{ item.item.name }}.container
+      loop: "{{ quad_restart_policies.results }}"
+      register: quad_restart_files
+
+    - name: Check restart policy mapping
+      assert:
+        that:
+          - "'[Service]' in item.stdout"
+          - "'Restart=' in item.stdout"
+          - "'--restart' not in item.stdout"
+      loop: "{{ quad_restart_files.results }}"
+
+    # Test for issue #913: Verify no duplicate [Service] section when user provides custom one
+    - name: Create Quadlet with restart_policy and custom Service section in quadlet_options
+      containers.podman.podman_container:
+        executable: "{{ test_executable | default('podman') }}"
+        name: container-restart-custom-service
+        image: alpine
+        state: quadlet
+        quadlet_dir: /tmp
+        restart_policy: always
+        command: sleep infinity
+        quadlet_options:
+          - |
+            [Service]
+            TimeoutStopSec=70
+            Restart=on-failure
+      register: quad_custom_service
+
+    - name: Read Quadlet file with custom Service section
+      shell: cat /tmp/container-restart-custom-service.container
+      register: quad_custom_service_content
+
+    - name: Count [Service] sections in Quadlet file
+      shell: grep -c "^\[Service\]" /tmp/container-restart-custom-service.container
+      register: service_section_count
+      failed_when: false
+
+    - name: Verify only one [Service] section exists (user's custom one takes precedence)
+      assert:
+        that:
+          - quad_custom_service is changed
+          - service_section_count.stdout == "1"
+          - "'TimeoutStopSec=70' in quad_custom_service_content.stdout"
+          - "'Restart=on-failure' in quad_custom_service_content.stdout"
+          - "'--restart' not in quad_custom_service_content.stdout"
+        fail_msg: "Should have exactly one [Service] section when user provides custom one"
+
+    - name: Cleanup restart test Quadlet files
+      file:
+        path: "{{ item }}"
+        state: absent
+      loop:
+        - /tmp/container-restart-test.container
+        - /tmp/container-restart-on-failure.container
+        - /tmp/container-restart-unless-stopped.container
+        - /tmp/container-restart-custom-service.container
+
   always:
 
     - name: Remove container