Merge pull request #5310 from vyos/T9048-smoketest-dhclient-races

smoketest: T9048: harden DHCP client process checks against CI timing races

1 files changed, 79 insertions, 21 deletions

diff --git a/smoketest/scripts/cli/base_interfaces_test.py b/smoketest/scripts/cli/base_interfaces_test.py
index 3917cca71..e0b26f525 100644
--- a/smoketest/scripts/cli/base_interfaces_test.py
+++ b/smoketest/scripts/cli/base_interfaces_test.py
@@ -16,6 +16,8 @@ import re
 import jmespath
 
 from json import loads
+from time import sleep
+from time import time
 from netifaces import ifaddresses # pylint: disable = no-name-in-module
 from socket import AF_INET
 from socket import AF_INET6
@@ -47,6 +49,12 @@ dhclient_process_name = 'dhclient'
 dhcp6c_base_dir = directories['dhcp6_client_dir']
 dhcp6c_process_name = 'dhcp6c'
 
+# Daemon startup/shutdown on a loaded CI runner can exceed 10 seconds. The
+# polls in process_named_running() and wait_for_result() complete as soon as
+# the expected state is reached, so a longer window only delays the failure
+# path.
+PROCESS_WAIT_TIMEOUT = 60
+
 MSG_TESTCASE_UNSUPPORTED = 'unsupported on interface family'
 
 server_ca_root_cert_data = """
@@ -208,19 +216,51 @@ class BasicInterfaceTest:
                 for map_entry in ct_map:
                      self.assertNotEqual(intf, map_entry['interface'])
 
-            # No daemon started during tests should remain running
+            # No daemon started during tests should remain running. A client
+            # daemon may still be shutting down after its config was removed,
+            # so grant a grace period before declaring a leak.
             for daemon in ['dhcp6c', 'dhclient']:
                 # if _interface list is populated do a more fine grained search
                 # by also checking the cmd arguments passed to the daemon
                 if self._interfaces:
                     for tmp in self._interfaces:
-                        self.assertFalse(process_named_running(daemon, tmp))
+                        _, pid = self.wait_for_result(
+                            lambda d=daemon, i=tmp: process_named_running(d, i), None,
+                            timeout=PROCESS_WAIT_TIMEOUT)
+                        self.assertFalse(pid)
                 else:
-                    self.assertFalse(process_named_running(daemon))
+                    _, pid = self.wait_for_result(
+                        lambda d=daemon: process_named_running(d), None,
+                        timeout=PROCESS_WAIT_TIMEOUT)
+                    self.assertFalse(pid)
 
             # always forward to base class
             super().tearDown()
 
+        def get_process_cmdline(self, process_name, interface, pid):
+            # dhclient re-executes itself while daemonizing - the PID found
+            # right after commit may already be gone when /proc is read.
+            # Poll PID discovery and the /proc read together until the
+            # deadline instead of failing on a transient PID. Returns the
+            # (pid, cmdline) pair that was actually validated so callers
+            # can assert against the same process.
+            # NB: read_file() re-raises on failure unless defaultonfailure
+            # is set to a non-None value.
+            time_expire = time() + PROCESS_WAIT_TIMEOUT
+            cmdline = read_file(f'/proc/{pid}/cmdline', defaultonfailure='')
+            while not cmdline:
+                if time() > time_expire:
+                    break
+                sleep(0.250)
+                tmp = process_named_running(process_name, cmdline=interface)
+                if not tmp:
+                    continue
+                pid = tmp
+                cmdline = read_file(f'/proc/{pid}/cmdline', defaultonfailure='')
+            self.assertTrue(cmdline,
+                f'{process_name} cmdline unreadable for interface {interface}')
+            return pid, cmdline
+
         def test_dhcp_disable_interface(self):
             if not self._test_dhcp:
                 self.skipTest(MSG_TESTCASE_UNSUPPORTED)
@@ -269,7 +309,8 @@ class BasicInterfaceTest:
 
             for interface in self._interfaces:
                 # Check if dhclient process runs
-                dhclient_pid = process_named_running(dhclient_process_name, cmdline=interface, timeout=10)
+                dhclient_pid = process_named_running(dhclient_process_name, cmdline=interface,
+                                                     timeout=PROCESS_WAIT_TIMEOUT)
                 self.assertTrue(dhclient_pid)
 
                 dhclient_config = read_file(f'{dhclient_base_dir}/dhclient_{interface}.conf')
@@ -281,7 +322,8 @@ class BasicInterfaceTest:
                 self.assertIn(f'send user-class "{user_class}";', dhclient_config)
 
                 # and the commandline has the appropriate options
-                cmdline = read_file(f'/proc/{dhclient_pid}/cmdline')
+                dhclient_pid, cmdline = self.get_process_cmdline(
+                    dhclient_process_name, interface, dhclient_pid)
                 self.assertIn(f'-e\x00IF_METRIC={distance}', cmdline)
 
         def test_dhcp_vrf(self):
@@ -309,14 +351,19 @@ class BasicInterfaceTest:
                 self.assertEqual(tmp, vrf_name)
 
                 # Check if dhclient process runs
-                dhclient_pid = process_named_running(dhclient_process_name, cmdline=interface, timeout=10)
+                dhclient_pid = process_named_running(dhclient_process_name, cmdline=interface,
+                                                     timeout=PROCESS_WAIT_TIMEOUT)
                 self.assertTrue(dhclient_pid)
-                # .. inside the appropriate VRF instance
+                # The commandline must carry the appropriate options ..
+                # (get_process_cmdline() may re-resolve the PID, so it runs
+                # first and the VRF check below targets the same process)
+                dhclient_pid, cmdline = self.get_process_cmdline(
+                    dhclient_process_name, interface, dhclient_pid)
+                self.assertIn(f'-e\x00IF_METRIC={cli_default_metric}', cmdline)
+                # .. and the process must run inside the appropriate VRF
+                # instance
                 vrf_pids = cmd(f'ip vrf pids {vrf_name}')
                 self.assertIn(str(dhclient_pid), vrf_pids)
-                # and the commandline has the appropriate options
-                cmdline = read_file(f'/proc/{dhclient_pid}/cmdline')
-                self.assertIn(f'-e\x00IF_METRIC={cli_default_metric}', cmdline)
 
             # T5103: remove interface from VRF instance and move DHCP client
             # back to default VRF. This must restart the DHCP client process
@@ -330,14 +377,19 @@ class BasicInterfaceTest:
                 tmp = get_interface_vrf(interface)
                 self.assertEqual(tmp, 'default')
                 # Check if dhclient process runs
-                dhclient_pid = process_named_running(dhclient_process_name, cmdline=interface, timeout=10)
+                dhclient_pid = process_named_running(dhclient_process_name, cmdline=interface,
+                                                     timeout=PROCESS_WAIT_TIMEOUT)
                 self.assertTrue(dhclient_pid)
-                # .. inside the appropriate VRF instance
+                # The commandline must carry the appropriate options ..
+                # (get_process_cmdline() may re-resolve the PID, so it runs
+                # first and the VRF check below targets the same process)
+                dhclient_pid, cmdline = self.get_process_cmdline(
+                    dhclient_process_name, interface, dhclient_pid)
+                self.assertIn(f'-e\x00IF_METRIC={cli_default_metric}', cmdline)
+                # .. and the process must no longer run inside the VRF
+                # instance
                 vrf_pids = cmd(f'ip vrf pids {vrf_name}')
                 self.assertNotIn(str(dhclient_pid), vrf_pids)
-                # and the commandline has the appropriate options
-                cmdline = read_file(f'/proc/{dhclient_pid}/cmdline')
-                self.assertIn(f'-e\x00IF_METRIC={cli_default_metric}', cmdline)
 
             self.cli_delete(['vrf', 'name', vrf_name])
 
@@ -365,7 +417,8 @@ class BasicInterfaceTest:
                 self.assertEqual(tmp, vrf_name)
 
                 # Check if dhclient process runs
-                tmp = process_named_running(dhcp6c_process_name, cmdline=interface, timeout=10)
+                tmp = process_named_running(dhcp6c_process_name, cmdline=interface,
+                                            timeout=PROCESS_WAIT_TIMEOUT)
                 self.assertTrue(tmp)
                 # .. inside the appropriate VRF instance
                 vrf_pids = cmd(f'ip vrf pids {vrf_name}')
@@ -384,7 +437,8 @@ class BasicInterfaceTest:
                 self.assertEqual(tmp, 'default')
 
                 # Check if dhclient process runs
-                tmp = process_named_running(dhcp6c_process_name, cmdline=interface, timeout=10)
+                tmp = process_named_running(dhcp6c_process_name, cmdline=interface,
+                                            timeout=PROCESS_WAIT_TIMEOUT)
                 self.assertTrue(tmp)
                 # .. inside the appropriate VRF instance
                 vrf_pids = cmd(f'ip vrf pids {vrf_name}')
@@ -1195,11 +1249,13 @@ class BasicInterfaceTest:
                     self.assertNotIn('syntax error', entry.get('MESSAGE', ''))
 
                 # Better ask the process about it's commandline in the future
-                pid = process_named_running(dhcp6c_process_name, cmdline=interface, timeout=10)
+                pid = process_named_running(dhcp6c_process_name, cmdline=interface,
+                                            timeout=PROCESS_WAIT_TIMEOUT)
                 self.assertTrue(pid)
 
                 # DHCPv6 option "no-release" requires "-n" daemon startup option
-                dhcp6c_options = read_file(f'/proc/{pid}/cmdline')
+                pid, dhcp6c_options = self.get_process_cmdline(
+                    dhcp6c_process_name, interface, pid)
                 self.assertIn('-n', dhcp6c_options)
 
         def test_dhcpv6pd_auto_sla_id(self):
@@ -1254,7 +1310,8 @@ class BasicInterfaceTest:
                     address = str(int(address) + 1)
 
                 # Check for running process
-                self.assertTrue(process_named_running(dhcp6c_process_name, cmdline=interface, timeout=10))
+                self.assertTrue(process_named_running(dhcp6c_process_name, cmdline=interface,
+                                                      timeout=PROCESS_WAIT_TIMEOUT))
 
             for delegatee in delegatees:
                 # we can already cleanup the test delegatee interface here
@@ -1319,7 +1376,8 @@ class BasicInterfaceTest:
                     address = str(int(address) + 1)
 
                 # Check for running process
-                self.assertTrue(process_named_running(dhcp6c_process_name, cmdline=interface, timeout=10))
+                self.assertTrue(process_named_running(dhcp6c_process_name, cmdline=interface,
+                                                      timeout=PROCESS_WAIT_TIMEOUT))
 
             for delegatee in delegatees:
                 # we can already cleanup the test delegatee interface here