Merge pull request #5182 from c-po/systemctl-stop-helper

T8831: smoketests: irregular PermissionError caused by systemctl stop

3 files changed, 56 insertions, 18 deletions

diff --git a/python/vyos/ifconfig/interface.py b/python/vyos/ifconfig/interface.py
index 136adb289..9cb76ee05 100644
--- a/python/vyos/ifconfig/interface.py
+++ b/python/vyos/ifconfig/interface.py
@@ -48,6 +48,7 @@ from vyos.utils.network import get_interface_namespace
 from vyos.utils.network import get_vrf_tableid
 from vyos.utils.network import is_netns_interface
 from vyos.utils.process import is_systemd_service_active
+from vyos.utils.process import stop_systemd_unit
 from vyos.utils.process import run
 from vyos.utils.file import read_file
 from vyos.utils.file import write_file
@@ -388,8 +389,8 @@ class Interface(Control):
         >>> i.remove()
         """
         # Stop WPA supplicant if EAPoL was in use
-        if is_systemd_service_active(f'wpa_supplicant-wired@{self.ifname}'):
-            self._cmd(f'systemctl stop wpa_supplicant-wired@{self.ifname}')
+        netns = self.config['netns'] if 'netns' in self.config else None
+        stop_systemd_unit(f'wpa_supplicant-wired@{self.ifname}', netns=netns)
 
         # remove all assigned IP addresses from interface - this is a bit redundant
         # as the kernel will remove all addresses on interface deletion, but we
@@ -1548,17 +1549,18 @@ class Interface(Control):
             # Reload systemd unit definitions as some options are dynamically generated
             self._cmd('systemctl daemon-reload')
 
+            netns = self.config['netns'] if 'netns' in self.config else None
             # When the DHCP client is restarted a brief outage will occur, as
             # the old lease is released a new one is acquired (T4203). We will
             # only restart DHCP client if it's option changed, or if it's not
             # running, but it should be running (e.g. on system startup)
             if (vrf_changed or
                 ('dhcp_options_changed' in self.config) or
-                (not is_systemd_service_active(systemd_service))):
+                (not is_systemd_service_active(systemd_service, netns=netns))):
                 return self._cmd(f'systemctl restart {systemd_service}')
         else:
-            if is_systemd_service_active(systemd_service):
-                self._cmd(f'systemctl stop {systemd_service}')
+            netns = self.config['netns'] if 'netns' in self.config else None
+            stop_systemd_unit(systemd_service, netns=netns)
 
             # Smoketests occasionally fail if the lease is not removed from the Kernel fast enough:
             # AssertionError: 2 unexpectedly found in {17: [{'addr': '52:54:00:00:00:00',
@@ -1607,15 +1609,16 @@ class Interface(Control):
             # Reload systemd unit definitions as some options are dynamically generated
             self._cmd('systemctl daemon-reload')
 
+            netns = self.config['netns'] if 'netns' in self.config else None
             # We must ignore any return codes. This is required to enable
             # DHCPv6-PD for interfaces which are yet not up and running.
             if (vrf_changed or
                 ('dhcpv6_options_changed' in self.config) or
-                (not is_systemd_service_active(systemd_service))):
+                (not is_systemd_service_active(systemd_service, netns=netns))):
                 return self._popen(f'systemctl restart {systemd_service}')
         else:
-            if is_systemd_service_active(systemd_service):
-                self._cmd(f'systemctl stop {systemd_service}')
+            netns = self.config['netns'] if 'netns' in self.config else None
+            stop_systemd_unit(systemd_service, netns=netns)
             if os.path.isfile(config_file):
                 os.remove(config_file)
             if os.path.isfile(script_file):
diff --git a/python/vyos/utils/process.py b/python/vyos/utils/process.py
index 94d6670dc..e78c0b969 100644
--- a/python/vyos/utils/process.py
+++ b/python/vyos/utils/process.py
@@ -15,13 +15,13 @@
 
 import os
 import shlex
+import time
 
 from subprocess import Popen
 from subprocess import PIPE
 from subprocess import STDOUT
 from subprocess import DEVNULL
 
-
 def get_wrapper(vrf, netns):
     wrapper = None
     if vrf:
@@ -90,9 +90,8 @@ def popen(command, flag='', shell=None, input=None, timeout=None, env=None,
     wrapper = get_wrapper(vrf, netns)
     if vrf or netns:
         if os.getuid() != 0:
-            raise OSError(
-                'Permission denied: cannot execute commands in VRF and netns contexts as an unprivileged user'
-            )
+            raise OSError('Permission denied: cannot execute commands in VRF ' \
+                          'and netns contexts as an unprivileged user')
 
         if use_shell:
             command = f'{shlex.join(wrapper)} {command}'
@@ -109,7 +108,7 @@ def popen(command, flag='', shell=None, input=None, timeout=None, env=None,
         input = input.encode() if type(input) is str else input
 
     text = None
-    bufsize = -1 # default: means the system default of io.DEFAULT_BUFFER_SIZE will be used
+    bufsize = -1 # default: system default of io.DEFAULT_BUFFER_SIZE is used
     if not buffered:
         text = True # Treat output as strings (not bytes)
         bufsize = 1 # Enable line buffering
@@ -279,7 +278,6 @@ def process_named_running(name: str, cmdline: str=None, timeout: int=0):
                 return p.info['pid']
         return None
     if timeout:
-        import time
         time_expire = time.time() + timeout
         while True:
             tmp = check_process(name, cmdline)
@@ -293,13 +291,49 @@ def process_named_running(name: str, cmdline: str=None, timeout: int=0):
         return check_process(name, cmdline)
     return None
 
-def is_systemd_service_active(service):
+def is_systemd_service_active(service: str, vrf=None, netns=None) -> bool:
     """ Test is a specified systemd service is activated.
     Returns True if service is active, false otherwise.
     Copied from: https://unix.stackexchange.com/a/435317 """
-    tmp = cmd(f'systemctl show --value -p ActiveState {service}')
+    tmp = cmd(f'systemctl show --value -p ActiveState {service}',
+              vrf=vrf, netns=netns)
     return bool((tmp == 'active'))
 
+def stop_systemd_unit(service: str, retries: int=3, delay_s: float=0.250,
+                      raise_on_failure: bool=True, vrf=None, netns=None) -> None:
+    """
+    Stop systemd unit used during interface teardown (e.g. DHCP clients).
+
+    Retries transient "systemctl stop| failures and verifies ActiveState is no
+    longer "active". Escalate to systemctl kill if stop attempts fail while
+    unit remains active.
+    """
+
+    if not is_systemd_service_active(service, vrf=vrf, netns=netns):
+        return None
+
+    for _ in range(retries):
+        rc_cmd(f'systemctl stop {service}', vrf=vrf, netns=netns)
+        if not is_systemd_service_active(service, vrf=vrf, netns=netns):
+            # Service properly stopped - return early, this should be the default
+            return None
+        time.sleep(delay_s)
+
+    rc_cmd(f'systemctl kill {service}', vrf=vrf, netns=netns)
+    time.sleep(delay_s)
+    if not is_systemd_service_active(service, vrf=vrf, netns=netns):
+        return None
+
+    # This should not happen
+    if raise_on_failure:
+        code, out = rc_cmd(f'systemctl show --value -p ActiveState {service}',
+                           vrf=vrf, netns=netns)
+        disp_state = out.strip() if code == 0 and out.strip() else 'unknown'
+
+        raise RuntimeError(f'systemd unit {service} still has ActiveState={disp_state} ' \
+                           f'after {retries} stop attempts and systemctl kill')
+    return None
+
 def is_systemd_service_running(service):
     """ Test is a specified systemd service is actually running.
     Returns True if service is running, false otherwise.
diff --git a/src/services/vyos-netlinkd b/src/services/vyos-netlinkd
index c759ebd3d..7bf6b8cce 100755
--- a/src/services/vyos-netlinkd
+++ b/src/services/vyos-netlinkd
@@ -32,6 +32,7 @@ from vyos.utils.commit import commit_in_progress
 from vyos.utils.dict import dict_search
 from vyos.utils.process import cmd
 from vyos.utils.process import is_systemd_service_active
+from vyos.utils.process import stop_systemd_unit
 
 running = True
 
@@ -62,10 +63,10 @@ def _handle_dhcp_events(operstate: Optional[str], ifname: str) -> None:
         # Interface moved state to down
         if is_systemd_service_active(systemdV4_service):
             syslog.syslog(syslog.LOG_DEBUG, f'Stopping {systemdV4_service}...')
-            cmd(f'systemctl stop {systemdV4_service}')
+            stop_systemd_unit(systemdV4_service, raise_on_failure=False)
         if is_systemd_service_active(systemdV6_service):
             syslog.syslog(syslog.LOG_DEBUG, f'Stopping {systemdV6_service}...')
-            cmd(f'systemctl stop {systemdV6_service}')
+            stop_systemd_unit(systemdV6_service, raise_on_failure=False)
 
     elif operstate == 'UP':
         v6_restart = False