Merge pull request #3845 from vyos/mergify/bp/sagitta/pr-3833

wireless: T6597: improve hostapd startup and corresponding smoketests (backport #3833)

4 files changed, 54 insertions, 31 deletions

diff --git a/python/vyos/utils/process.py b/python/vyos/utils/process.py
index bd0644bc0..a2398edf7 100644
--- a/python/vyos/utils/process.py
+++ b/python/vyos/utils/process.py
@@ -225,7 +225,7 @@ def process_named_running(name: str, cmdline: str=None, timeout: int=0):
             if not tmp:
                 if time.time() > time_expire:
                     break
-                time.sleep(0.100) # wait 250ms
+                time.sleep(0.100) # wait 100ms
                 continue
             return tmp
     else:
diff --git a/smoketest/scripts/cli/base_vyostest_shim.py b/smoketest/scripts/cli/base_vyostest_shim.py
index efaa74fe0..112c58c09 100644
--- a/smoketest/scripts/cli/base_vyostest_shim.py
+++ b/smoketest/scripts/cli/base_vyostest_shim.py
@@ -75,6 +75,8 @@ class VyOSUnitTestSHIM:
             self._session.delete(config)
 
         def cli_commit(self):
+            if self.debug:
+                print('commit')
             self._session.commit()
             # during a commit there is a process opening commit_lock, and run() returns 0
             while run(f'sudo lsof -nP {commit_lock}') == 0:
diff --git a/smoketest/scripts/system/test_kernel_options.py b/smoketest/scripts/system/test_kernel_options.py
index 4666e98e7..700e4cec7 100755
--- a/smoketest/scripts/system/test_kernel_options.py
+++ b/smoketest/scripts/system/test_kernel_options.py
@@ -19,8 +19,9 @@ import os
 import platform
 import unittest
 
-kernel = platform.release()
+from vyos.utils.kernel import check_kmod
 
+kernel = platform.release()
 class TestKernelModules(unittest.TestCase):
     """ VyOS makes use of a lot of Kernel drivers, modules and features. The
     required modules which are essential for VyOS should be tested that they are
@@ -35,9 +36,8 @@ class TestKernelModules(unittest.TestCase):
 
         super(TestKernelModules, cls).setUpClass()
         CONFIG = '/proc/config.gz'
-
         if not os.path.isfile(CONFIG):
-            call('sudo modprobe configs')
+            check_kmod('configs')
 
         with gzip.open(CONFIG, 'rt') as f:
             cls._config_data = f.read()
diff --git a/src/conf_mode/interfaces_wireless.py b/src/conf_mode/interfaces_wireless.py
index 3f01612a2..ff38c979c 100755
--- a/src/conf_mode/interfaces_wireless.py
+++ b/src/conf_mode/interfaces_wireless.py
@@ -19,6 +19,7 @@ import os
 from sys import exit
 from re import findall
 from netaddr import EUI, mac_unix_expanded
+from time import sleep
 
 from vyos.config import Config
 from vyos.configdict import get_interface_dict
@@ -34,6 +35,9 @@ from vyos.template import render
 from vyos.utils.dict import dict_search
 from vyos.utils.kernel import check_kmod
 from vyos.utils.process import call
+from vyos.utils.process import is_systemd_service_active
+from vyos.utils.process import is_systemd_service_running
+from vyos.utils.network import interface_exists
 from vyos import ConfigError
 from vyos import airbag
 airbag.enable()
@@ -87,6 +91,11 @@ def get_config(config=None):
         if wifi.from_defaults(['security', 'wpa']): # if not set by user
             del wifi['security']['wpa']
 
+    # XXX: Jinja2 can not operate on a dictionary key when it starts of with a number
+    if '40mhz_incapable' in (dict_search('capabilities.ht', wifi) or []):
+        wifi['capabilities']['ht']['fourtymhz_incapable'] = wifi['capabilities']['ht']['40mhz_incapable']
+        del wifi['capabilities']['ht']['40mhz_incapable']
+
     if dict_search('security.wpa', wifi) != None:
         wpa_cipher = wifi['security']['wpa'].get('cipher')
         wpa_mode = wifi['security']['wpa'].get('mode')
@@ -114,7 +123,7 @@ def get_config(config=None):
     tmp = find_other_stations(conf, base, wifi['ifname'])
     if tmp: wifi['station_interfaces'] = tmp
 
-    # used in hostapt.conf.j2
+    # used in hostapd.conf.j2
     wifi['hostapd_accept_station_conf'] = hostapd_accept_station_conf.format(**wifi)
     wifi['hostapd_deny_station_conf'] = hostapd_deny_station_conf.format(**wifi)
 
@@ -218,11 +227,6 @@ def verify(wifi):
 def generate(wifi):
     interface = wifi['ifname']
 
-    # always stop hostapd service first before reconfiguring it
-    call(f'systemctl stop hostapd@{interface}.service')
-    # always stop wpa_supplicant service first before reconfiguring it
-    call(f'systemctl stop wpa_supplicant@{interface}.service')
-
     # Delete config files if interface is removed
     if 'deleted' in wifi:
         if os.path.isfile(hostapd_conf.format(**wifi)):
@@ -258,11 +262,6 @@ def generate(wifi):
             mac.dialect = mac_unix_expanded
             wifi['mac'] = str(mac)
 
-    # XXX: Jinja2 can not operate on a dictionary key when it starts of with a number
-    if '40mhz_incapable' in (dict_search('capabilities.ht', wifi) or []):
-        wifi['capabilities']['ht']['fourtymhz_incapable'] = wifi['capabilities']['ht']['40mhz_incapable']
-        del wifi['capabilities']['ht']['40mhz_incapable']
-
     # render appropriate new config files depending on access-point or station mode
     if wifi['type'] == 'access-point':
         render(hostapd_conf.format(**wifi), 'wifi/hostapd.conf.j2', wifi)
@@ -276,23 +275,45 @@ def generate(wifi):
 
 def apply(wifi):
     interface = wifi['ifname']
+    # From systemd source code:
+    # If there's a stop job queued before we enter the DEAD state, we shouldn't act on Restart=,
+    # in order to not undo what has already been enqueued. */
+    #
+    # It was found that calling restart on hostapd will (4 out of 10 cases) deactivate
+    # the service instead of restarting it, when it was not yet properly stopped
+    # systemd[1]: hostapd@wlan1.service: Deactivated successfully.
+    # Thus kill all WIFI service and start them again after it's ensured nothing lives
+    call(f'systemctl stop hostapd@{interface}.service')
+    call(f'systemctl stop wpa_supplicant@{interface}.service')
+
     if 'deleted' in wifi:
-        WiFiIf(interface).remove()
-    else:
-        # Finally create the new interface
-        w = WiFiIf(**wifi)
-        w.update(wifi)
-
-        # Enable/Disable interface - interface is always placed in
-        # administrative down state in WiFiIf class
-        if 'disable' not in wifi:
-            # Physical interface is now configured. Proceed by starting hostapd or
-            # wpa_supplicant daemon. When type is monitor we can just skip this.
-            if wifi['type'] == 'access-point':
-                call(f'systemctl start hostapd@{interface}.service')
-
-            elif wifi['type'] == 'station':
-                call(f'systemctl start wpa_supplicant@{interface}.service')
+        WiFiIf(**wifi).remove()
+        return None
+
+    while (is_systemd_service_running(f'hostapd@{interface}.service') or \
+           is_systemd_service_active(f'hostapd@{interface}.service')):
+        sleep(0.250) # wait 250ms
+
+    # Finally create the new interface
+    w = WiFiIf(**wifi)
+    w.update(wifi)
+
+    # Enable/Disable interface - interface is always placed in
+    # administrative down state in WiFiIf class
+    if 'disable' not in wifi:
+        # Wait until interface was properly added to the Kernel
+        ii = 0
+        while not (interface_exists(interface) and ii < 20):
+            sleep(0.250) # wait 250ms
+            ii += 1
+
+        # Physical interface is now configured. Proceed by starting hostapd or
+        # wpa_supplicant daemon. When type is monitor we can just skip this.
+        if wifi['type'] == 'access-point':
+            call(f'systemctl start hostapd@{interface}.service')
+
+        elif wifi['type'] == 'station':
+            call(f'systemctl start wpa_supplicant@{interface}.service')
 
     return None