wireless: T6597: improve hostapd startup and corresponding smoketests

This was found during smoketesting as thoase started to repeadingly fail in the last weeks

  File "/usr/libexec/vyos/tests/smoke/cli/test_interfaces_wireless.py", line 534, in test_wireless_security_station_address
      self.assertTrue(process_named_running('hostapd'))
  AssertionError: None is not true

Digging into this revealed that this is NOT related to the smoketest coding but
to hostapd/systemd instead. With a configured WIFI interface and calling:
"sudo systemctl reload-or-restart hostapd@wlan1" multiple times in a short
period caused systemd to report:
"Jul 18 16:15:32 systemd[1]: hostapd@wlan1.service: Deactivated successfully."

According to the internal systemd logic used in our version this is explained by:

  /* 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. */
  if (unit_stop_pending(UNIT(s)))
          allow_restart = false;

  if (s->result == SERVICE_SUCCESS)
          s->result = f;

  if (s->result == SERVICE_SUCCESS) {
          unit_log_success(UNIT(s));
          end_state = SERVICE_DEAD;`

Where unit_log_success() generates the log message in question.

Improve the restart login in the wireless interface script and an upgrade to
hostapd solved the issue.

2 files changed, 79 insertions, 48 deletions

diff --git a/smoketest/scripts/cli/test_interfaces_wireless.py b/smoketest/scripts/cli/test_interfaces_wireless.py
index 58aef0001..7bfe0d221 100755
--- a/smoketest/scripts/cli/test_interfaces_wireless.py
+++ b/smoketest/scripts/cli/test_interfaces_wireless.py
@@ -22,9 +22,11 @@ from base_interfaces_test import BasicInterfaceTest
 from glob import glob
 
 from vyos.configsession import ConfigSessionError
-from vyos.utils.process import process_named_running
-from vyos.utils.kernel import check_kmod
 from vyos.utils.file import read_file
+from vyos.utils.kernel import check_kmod
+from vyos.utils.network import interface_exists
+from vyos.utils.process import process_named_running
+from vyos.utils.process import call
 from vyos.xml_ref import default_value
 
 def get_config_value(interface, key):
@@ -33,7 +35,7 @@ def get_config_value(interface, key):
     return tmp[0]
 
 wifi_cc_path = ['system', 'wireless', 'country-code']
-
+country = 'se'
 class WirelessInterfaceTest(BasicInterfaceTest.TestCase):
     @classmethod
     def setUpClass(cls):
@@ -66,7 +68,8 @@ class WirelessInterfaceTest(BasicInterfaceTest.TestCase):
         cls._test_ipv6 = False
         cls._test_vlan = False
 
-        cls.cli_set(cls, wifi_cc_path + ['se'])
+        cls.cli_set(cls, wifi_cc_path + [country])
+
 
     def test_wireless_add_single_ip_address(self):
         # derived method to check if member interfaces are enslaved properly
@@ -84,7 +87,7 @@ class WirelessInterfaceTest(BasicInterfaceTest.TestCase):
 
     def test_wireless_hostapd_config(self):
         # Only set the hostapd (access-point) options
-        interface = 'wlan1'
+        interface = self._interfaces[1] # wlan1
         ssid = 'ssid'
 
         self.cli_set(self._base_path + [interface, 'ssid', ssid])
@@ -161,7 +164,7 @@ class WirelessInterfaceTest(BasicInterfaceTest.TestCase):
 
     def test_wireless_hostapd_vht_mu_beamformer_config(self):
         # Multi-User-Beamformer
-        interface = 'wlan1'
+        interface = self._interfaces[1] # wlan1
         ssid = 'vht_mu-beamformer'
         antennas = '3'
 
@@ -230,7 +233,7 @@ class WirelessInterfaceTest(BasicInterfaceTest.TestCase):
 
     def test_wireless_hostapd_vht_su_beamformer_config(self):
         # Single-User-Beamformer
-        interface = 'wlan1'
+        interface = self._interfaces[1] # wlan1
         ssid = 'vht_su-beamformer'
         antennas = '3'
 
@@ -299,16 +302,14 @@ class WirelessInterfaceTest(BasicInterfaceTest.TestCase):
 
     def test_wireless_hostapd_he_config(self):
         # Only set the hostapd (access-point) options - HE mode for 802.11ax at 6GHz
-        interface = 'wlan1'
+        interface = self._interfaces[1] # wlan1
         ssid = 'ssid'
         channel = '1'
         sae_pw = 'VyOSVyOSVyOS'
-        country = 'de'
         bss_color = '37'
         channel_set_width = '134'
         center_channel_freq_1 = '15'
 
-        self.cli_set(wifi_cc_path + [country])
         self.cli_set(self._base_path + [interface, 'ssid', ssid])
         self.cli_set(self._base_path + [interface, 'type', 'access-point'])
         self.cli_set(self._base_path + [interface, 'channel', channel])
@@ -353,10 +354,6 @@ class WirelessInterfaceTest(BasicInterfaceTest.TestCase):
         tmp = get_config_value(interface, 'he_oper_centr_freq_seg0_idx')
         self.assertEqual(center_channel_freq_1, tmp)
 
-        # Country code
-        tmp = get_config_value(interface, 'country_code')
-        self.assertEqual(country.upper(), tmp)
-
         # BSS coloring
         tmp = get_config_value(interface, 'he_bss_color')
         self.assertEqual(bss_color, tmp)
@@ -386,15 +383,12 @@ class WirelessInterfaceTest(BasicInterfaceTest.TestCase):
 
     def test_wireless_hostapd_wpa_config(self):
         # Only set the hostapd (access-point) options
-        interface = 'wlan1'
-        phy = 'phy0'
+        interface = self._interfaces[1] # wlan1
         ssid = 'VyOS-SMOKETEST'
         channel = '1'
         wpa_key = 'VyOSVyOSVyOS'
         mode = 'n'
-        country = 'de'
 
-        self.cli_set(self._base_path + [interface, 'physical-device', phy])
         self.cli_set(self._base_path + [interface, 'type', 'access-point'])
         self.cli_set(self._base_path + [interface, 'mode', mode])
 
@@ -454,7 +448,7 @@ class WirelessInterfaceTest(BasicInterfaceTest.TestCase):
         self.assertTrue(process_named_running('hostapd'))
 
     def test_wireless_access_point_bridge(self):
-        interface = 'wlan1'
+        interface = self._interfaces[1] # wlan1
         ssid = 'VyOS-Test'
         bridge = 'br42477'
 
@@ -491,7 +485,7 @@ class WirelessInterfaceTest(BasicInterfaceTest.TestCase):
         self.cli_delete(bridge_path)
 
     def test_wireless_security_station_address(self):
-        interface = 'wlan1'
+        interface = self._interfaces[1] # wlan1
         ssid = 'VyOS-ACL'
 
         hostapd_accept_station_conf = f'/run/hostapd/{interface}_station_accept.conf'
@@ -511,6 +505,12 @@ class WirelessInterfaceTest(BasicInterfaceTest.TestCase):
 
         self.cli_commit()
 
+        self.assertTrue(interface_exists(interface))
+        self.assertTrue(os.path.isfile(f'/run/hostapd/{interface}_station_accept.conf'))
+        self.assertTrue(os.path.isfile(f'/run/hostapd/{interface}_station_deny.conf'))
+
+        self.assertTrue(process_named_running('hostapd'))
+
         # in accept mode all addresses are allowed unless specified in the deny list
         tmp = get_config_value(interface, 'macaddr_acl')
         self.assertEqual(tmp, '0')
@@ -526,6 +526,11 @@ class WirelessInterfaceTest(BasicInterfaceTest.TestCase):
         #  Switch mode accept -> deny
         self.cli_set(self._base_path + [interface, 'security', 'station-address', 'mode', 'deny'])
         self.cli_commit()
+
+        self.assertTrue(interface_exists(interface))
+        self.assertTrue(os.path.isfile(f'/run/hostapd/{interface}_station_accept.conf'))
+        self.assertTrue(os.path.isfile(f'/run/hostapd/{interface}_station_deny.conf'))
+
         # In deny mode all addresses are denied unless specified in the allow list
         tmp = get_config_value(interface, 'macaddr_acl')
         self.assertEqual(tmp, '1')
@@ -535,4 +540,9 @@ class WirelessInterfaceTest(BasicInterfaceTest.TestCase):
 
 if __name__ == '__main__':
     check_kmod('mac80211_hwsim')
-    unittest.main(verbosity=2, failfast=True)
+    # loading the module created two WIFI Interfaces in the background (wlan0 and wlan1)
+    # remove them to have a clean test start
+    for interface in ['wlan0', 'wlan1']:
+        if interface_exists(interface):
+            call(f'sudo iw dev {interface} del')
+    unittest.main(verbosity=2)
diff --git a/src/conf_mode/interfaces_wireless.py b/src/conf_mode/interfaces_wireless.py
index 5fd7ab6e9..f35a250cb 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()
@@ -93,6 +97,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')
@@ -120,7 +129,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)
 
@@ -232,11 +241,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)):
@@ -272,11 +276,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)
@@ -290,23 +289,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