| Message ID | 20220707200309.3276163-1-andrew.zaborowski@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v2,1/4] test-runner: Support running hostapd in namespaces | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| prestwoj/iwd-alpine-ci-fetch | success | Fetch PR |
| prestwoj/iwd-ci-gitlint | fail | [v2,4/4] test-runner: Mark source directory as safe for git 9: B3 Line contains hard tab characters (\t): " git config --global --add safe.directory /home/balrog/repos/iwd" |
| prestwoj/iwd-ci-fetch | success | Fetch PR |
| prestwoj/iwd-ci-makedistcheck | success | Make Distcheck |
| prestwoj/iwd-alpine-ci-makedistcheck | success | Make Distcheck |
| prestwoj/iwd-ci-build | success | Build - Configure |
| prestwoj/iwd-alpine-ci-build | success | Build - Configure |
| prestwoj/iwd-ci-clang | success | clang PASS |
| prestwoj/iwd-ci-makecheckvalgrind | success | Make Check w/Valgrind |
| prestwoj/iwd-ci-makecheck | success | Make Check |
| prestwoj/iwd-alpine-ci-makecheckvalgrind | success | Make Check w/Valgrind |
| prestwoj/iwd-alpine-ci-makecheck | success | Make Check |
| prestwoj/iwd-ci-incremental_build | success | Incremental Build with patches |
| prestwoj/iwd-alpine-ci-incremental_build | success | Incremental Build with patches |
| prestwoj/iwd-ci-testrunner | success | test-runner PASS |
Hi Andrew, On Thu, 2022-07-07 at 22:03 +0200, Andrew Zaborowski wrote: > The kernel will not let us test some scenarios of communication > between > two hwsim radios (e.g. STA and AP) if they're in the same net > namespace. > For example, when connected, you can't add normal IPv4 subnet routes > for > the same subnet on two different interfaces in one namespace (you'd > either get an EEXIST or you'd replace the other route), you can set > different metrics on the routes but that won't fix IP routing. For > testNetconfig the result is that communication works for DHCP before > we > get the inital lease but renewals won't work because they're unicast. > Allow hostapd to run on a radio that has been moved to a different > namespace in hw.conf so we don't have to work around these issues. > --- > changes in v2: > - ensure at least one hostapd session starts if the [HOSTAPD] > section > is present to mimic original behaviour and fix testEAD > > tools/run-tests | 82 ++++++++++++++++++++++++++++++++--------------- > -- > tools/utils.py | 2 +- > 2 files changed, 54 insertions(+), 30 deletions(-) > > diff --git a/tools/run-tests b/tools/run-tests > index 565847df..c0f230cb 100755 > --- a/tools/run-tests > +++ b/tools/run-tests > @@ -58,16 +58,19 @@ def exit_vm(): > runner.stop() > > class Interface: > - def __init__(self, name, config): > + def __init__(self, name, config, ns=None): > self.name = name > self.ctrl_interface = '/var/run/hostapd/' + name > self.config = config > + self.ns = ns > > def __del__(self): > - Process(['iw', 'dev', self.name, 'del']).wait() > + Process(['iw', 'dev', self.name, 'del'], > + namespace=self.ns.name if self.ns > else None).wait() > > def set_interface_state(self, state): > - Process(['ip', 'link', 'set', self.name, > state]).wait() > + Process(['ip', 'link', 'set', self.name, state], > + namespace=self.ns.name if self.ns > else None).wait() > So rather than this "self.ns.name if self.ns else None" thing, you could either store the namespace name directly (since self.ns is only used for the name), or store the namespace object like you do and replace Process(...) with self.ns.start_process(...). > class Radio: > def __init__(self, name): > @@ -75,11 +78,16 @@ class Radio: > # hostapd will reset this if this radio is used by it > self.use = 'iwd' > self.interface = None > + self.ns = None > > def __del__(self): > print("Removing radio %s" % self.name) > self.interface = None > > + def set_namespace(self, ns): > + self.ns = ns > + Process(['iw', 'phy', self.name, 'set', 'netns', > 'name', ns.name]).wait() > + > def create_interface(self, config, use): > global intf_id > > @@ -87,19 +95,21 @@ class Radio: > > intf_id += 1 > > - self.interface = Interface(ifname, config) > + self.interface = Interface(ifname, config, > ns=self.ns) > self.use = use > > Process(['iw', 'phy', self.name, 'interface', 'add', > ifname, > - 'type', 'managed']).wait() > + 'type', 'managed'], > namespace=self.ns.name if self.ns else None).wait() > > return self.interface Since create_interface() is the only function that needs the namespace couldn't we remove set_namespace entirely and just pass the namespace name into create_interface? > > def __str__(self): > ret = self.name + ':\n' > - ret += '\tUsed By: %s ' % self.use > + ret += '\tUsed By: %s' % self.use > if self.interface: > - ret += '(%s)' % self.interface.name > + ret += ' (%s)' % self.interface.name > + if self.ns is not None: > + ret += ' (ns=%s)' % self.ns.name > > ret += '\n' > > @@ -188,7 +198,7 @@ class Hostapd: > A set of running hostapd instances. This is really > just a single > process since hostapd can be started with multiple > config files. > ''' > - def __init__(self, radios, configs, radius): > + def __init__(self, ns, radios, configs, radius): > if len(configs) != len(radios): > raise Exception("Config (%d) and radio (%d) > list length not equal" % \ > (len(configs), > len(radios))) > @@ -198,8 +208,8 @@ class Hostapd: > Process(['ip', 'link', 'set', 'eth0', 'up']).wait() > Process(['ip', 'link', 'set', 'eth1', 'up']).wait() > > - self.global_ctrl_iface = '/var/run/hostapd/ctrl' > - > + self.ns = ns > + self.global_ctrl_iface = '/var/run/hostapd/ctrl' + > (str(ns.name) if ns.name else 'main') > self.instances = [HostapdInstance(c, r) for c, r in > zip(configs, radios)] > > ifaces = [rad.interface.name for rad in radios] > @@ -227,7 +237,7 @@ class Hostapd: > if Process.is_verbose('hostapd'): > args.append('-d') > > - self.process = Process(args) > + self.process = Process(args, namespace=ns.name) > > self.process.wait_for_socket(self.global_ctrl_iface, > 30) > > @@ -397,32 +407,44 @@ class TestContext(Namespace): > # tests. In this case you would not > care what > # was using each radio, just that > there was > # enough to run all tests. > - nradios = 0 > - for k, _ in settings.items(): > - if k == 'radius_server': > - continue > - nradios += 1 > - > - hapd_radios = self.radios[:nradios] > - > + hapd_configs = [conf for rad, conf in > settings.items() if rad != 'radius_server'] > + hapd_processes = [(self, > self.radios[:len(hapd_configs)], hapd_configs)] > else: > - hapd_radios = [rad for rad in self.radios if > rad.name in settings] > - > - hapd_configs = [conf for rad, conf in > settings.items() if rad != 'radius_server'] > + hapd_processes = [] > + for ns in [self] + self.namespaces: > + ns_radios = [rad for rad in ns.radios > if rad.name in settings] > + if len(ns_radios): > + ns_configs = > [settings[rad.name] for rad in ns_radios] > + hapd_processes.append((ns, > ns_radios, ns_configs)) > + if not hapd_processes: > + hapd_processes.append((self, [], [])) > > radius_config = settings.get('radius_server', None) > > - self.hostapd = Hostapd(hapd_radios, hapd_configs, > radius_config) > - self.hostapd.attach_cli() > + self.hostapd = [Hostapd(ns, radios, configs, > radius_config) > + for ns, radios, configs in > hapd_processes] > + > + for hapd in self.hostapd: > + hapd.attach_cli() > > def get_frequencies(self): > frequencies = [] > > - for hapd in self.hostapd.instances: > - frequencies.append(hapd.cli.frequency) > + for hapd in self.hostapd: > + frequencies += [instance.cli.frequency for > instance in hapd.instances] > > return frequencies > > + def get_hapd_instance(self, config=None): > + instances = [i for hapd in self.hostapd for i in > hapd.instances] > + > + if config is None: > + return instances[0] > + > + for hapd in instances: > + if hapd.config == config: > + return hapd > + Looks like this got carried over from that dropped patch? > def start_wpas_interfaces(self): > if 'WPA_SUPPLICANT' not in self.hw_config: > return > @@ -543,11 +565,13 @@ class TestContext(Namespace): > for arg in vars(self.args): > ret += '\t --%s %s\n' % (arg, > str(getattr(self.args, arg))) > > - ret += 'Hostapd:\n' > if self.hostapd: > - for h in self.hostapd.instances: > - ret += '\t%s\n' % str(h) > + for hapd in self.hostapd: > + ret += 'Hostapd (ns=%s):\n' % > (hapd.ns.name,) > + for h in hapd.instances: > + ret += '\t%s\n' % (str(h),) > else: > + ret += 'Hostapd:\n' > ret += '\tNo Hostapd instances\n' > > info = self.meminfo_to_dict() > diff --git a/tools/utils.py b/tools/utils.py > index f3e12a85..bc030230 100644 > --- a/tools/utils.py > +++ b/tools/utils.py > @@ -324,7 +324,7 @@ class Namespace: > > Process(['ip', 'netns', 'add', name]).wait() > for r in radios: > - Process(['iw', 'phy', r.name, 'set', 'netns', > 'name', name]).wait() > + r.set_namespace(self) > > self.start_dbus() >
Hi James, On Fri, 8 Jul 2022 at 00:37, James Prestwood <prestwoj@gmail.com> wrote: > On Thu, 2022-07-07 at 22:03 +0200, Andrew Zaborowski wrote: > > class Interface: > > - def __init__(self, name, config): > > + def __init__(self, name, config, ns=None): > > self.name = name > > self.ctrl_interface = '/var/run/hostapd/' + name > > self.config = config > > + self.ns = ns > > > > def __del__(self): > > - Process(['iw', 'dev', self.name, 'del']).wait() > > + Process(['iw', 'dev', self.name, 'del'], > > + namespace=self.ns.name if self.ns > > else None).wait() > > > > def set_interface_state(self, state): > > - Process(['ip', 'link', 'set', self.name, > > state]).wait() > > + Process(['ip', 'link', 'set', self.name, state], > > + namespace=self.ns.name if self.ns > > else None).wait() > > > > So rather than this "self.ns.name if self.ns else None" thing, you > could either store the namespace name directly (since self.ns is only > used for the name), or store the namespace object like you do and > replace Process(...) with self.ns.start_process(...). Ok, either of those should work, or maybe make sure Radios and Interfaces are initalized with "ctx" as the namespace so self.ns is never None. > > > class Radio: > > def __init__(self, name): > > @@ -75,11 +78,16 @@ class Radio: > > # hostapd will reset this if this radio is used by it > > self.use = 'iwd' > > self.interface = None > > + self.ns = None > > > > def __del__(self): > > print("Removing radio %s" % self.name) > > self.interface = None > > > > + def set_namespace(self, ns): > > + self.ns = ns > > + Process(['iw', 'phy', self.name, 'set', 'netns', > > 'name', ns.name]).wait() > > + > > def create_interface(self, config, use): > > global intf_id > > > > @@ -87,19 +95,21 @@ class Radio: > > > > intf_id += 1 > > > > - self.interface = Interface(ifname, config) > > + self.interface = Interface(ifname, config, > > ns=self.ns) > > self.use = use > > > > Process(['iw', 'phy', self.name, 'interface', 'add', > > ifname, > > - 'type', 'managed']).wait() > > + 'type', 'managed'], > > namespace=self.ns.name if self.ns else None).wait() > > > > return self.interface > > Since create_interface() is the only function that needs the namespace > couldn't we remove set_namespace entirely and just pass the namespace > name into create_interface? So set_namespace() also moves the radio to the right namespace. We need this for radios used by both hostapd and IWD, where we don't use create_interface(). I also think it's nice if you can obtain the namespace reference from `radio.ns` or `interface.ns`, say, in autotests/util/*.py code or elsewhere. > > > + def get_hapd_instance(self, config=None): > > + instances = [i for hapd in self.hostapd for i in > > hapd.instances] > > + > > + if config is None: > > + return instances[0] > > + > > + for hapd in instances: > > + if hapd.config == config: > > + return hapd > > + > > Looks like this got carried over from that dropped patch? I use this in autotests/util/hostapd.py (patch 2/4) since ctx.hostapd is now a list and to override the [] operator you'd need to subclass the list and overall it's simpler this way. Thanks for the review, Best regards
Hi Andrew, > > > class Radio: > > > def __init__(self, name): > > > @@ -75,11 +78,16 @@ class Radio: > > > # hostapd will reset this if this radio is used > > > by it > > > self.use = 'iwd' > > > self.interface = None > > > + self.ns = None > > > > > > def __del__(self): > > > print("Removing radio %s" % self.name) > > > self.interface = None > > > > > > + def set_namespace(self, ns): > > > + self.ns = ns > > > + Process(['iw', 'phy', self.name, 'set', 'netns', > > > 'name', ns.name]).wait() > > > + > > > def create_interface(self, config, use): > > > global intf_id > > > > > > @@ -87,19 +95,21 @@ class Radio: > > > > > > intf_id += 1 > > > > > > - self.interface = Interface(ifname, config) > > > + self.interface = Interface(ifname, config, > > > ns=self.ns) > > > self.use = use > > > > > > Process(['iw', 'phy', self.name, 'interface', > > > 'add', > > > ifname, > > > - 'type', 'managed']).wait() > > > + 'type', 'managed'], > > > namespace=self.ns.name if self.ns else None).wait() > > > > > > return self.interface > > > > Since create_interface() is the only function that needs the > > namespace > > couldn't we remove set_namespace entirely and just pass the > > namespace > > name into create_interface? > > So set_namespace() also moves the radio to the right namespace. We > need this for radios used by both hostapd and IWD, where we don't use > create_interface(). > > I also think it's nice if you can obtain the namespace reference from > `radio.ns` or `interface.ns`, say, in autotests/util/*.py code or > elsewhere. Are there situations where hostapd and IWD share a radio? But I agree, its nice knowing what namespace a radio/interface belongs to. > > > > > > + def get_hapd_instance(self, config=None): > > > + instances = [i for hapd in self.hostapd for i in > > > hapd.instances] > > > + > > > + if config is None: > > > + return instances[0] > > > + > > > + for hapd in instances: > > > + if hapd.config == config: > > > + return hapd > > > + > > > > Looks like this got carried over from that dropped patch? > > I use this in autotests/util/hostapd.py (patch 2/4) since ctx.hostapd > is now a list and to override the [] operator you'd need to subclass > the list and overall it's simpler this way. Sorry, I was confusing this with the HostapdCLI stuff. Nevermind. > > Thanks for the review, > Best regards
On Fri, 8 Jul 2022 at 01:19, James Prestwood <prestwoj@gmail.com> wrote: > > > Since create_interface() is the only function that needs the > > > namespace > > > couldn't we remove set_namespace entirely and just pass the > > > namespace > > > name into create_interface? > > > > So set_namespace() also moves the radio to the right namespace. We > > need this for radios used by both hostapd and IWD, where we don't use > > create_interface(). > > > > I also think it's nice if you can obtain the namespace reference from > > `radio.ns` or `interface.ns`, say, in autotests/util/*.py code or > > elsewhere. > > > Are there situations where hostapd and IWD share a radio? Sorry, I meant set_namespace() may be used by tools/utils.py whether this is an IWD radio or a hostapd radio. create_interface() on the other hand is only needed for hostapd so we can't replace one with the other. > > But I agree, its nice knowing what namespace a radio/interface belongs > to. > > > > > > > > > > + def get_hapd_instance(self, config=None): > > > > + instances = [i for hapd in self.hostapd for i in > > > > hapd.instances] > > > > + > > > > + if config is None: > > > > + return instances[0] > > > > + > > > > + for hapd in instances: > > > > + if hapd.config == config: > > > > + return hapd > > > > + > > > > > > Looks like this got carried over from that dropped patch? > > > > I use this in autotests/util/hostapd.py (patch 2/4) since ctx.hostapd > > is now a list and to override the [] operator you'd need to subclass > > the list and overall it's simpler this way. > > > Sorry, I was confusing this with the HostapdCLI stuff. Nevermind. (That code also used this method, you weren't wrong) Best regards
diff --git a/tools/run-tests b/tools/run-tests index 565847df..c0f230cb 100755 --- a/tools/run-tests +++ b/tools/run-tests @@ -58,16 +58,19 @@ def exit_vm(): runner.stop() class Interface: - def __init__(self, name, config): + def __init__(self, name, config, ns=None): self.name = name self.ctrl_interface = '/var/run/hostapd/' + name self.config = config + self.ns = ns def __del__(self): - Process(['iw', 'dev', self.name, 'del']).wait() + Process(['iw', 'dev', self.name, 'del'], + namespace=self.ns.name if self.ns else None).wait() def set_interface_state(self, state): - Process(['ip', 'link', 'set', self.name, state]).wait() + Process(['ip', 'link', 'set', self.name, state], + namespace=self.ns.name if self.ns else None).wait() class Radio: def __init__(self, name): @@ -75,11 +78,16 @@ class Radio: # hostapd will reset this if this radio is used by it self.use = 'iwd' self.interface = None + self.ns = None def __del__(self): print("Removing radio %s" % self.name) self.interface = None + def set_namespace(self, ns): + self.ns = ns + Process(['iw', 'phy', self.name, 'set', 'netns', 'name', ns.name]).wait() + def create_interface(self, config, use): global intf_id @@ -87,19 +95,21 @@ class Radio: intf_id += 1 - self.interface = Interface(ifname, config) + self.interface = Interface(ifname, config, ns=self.ns) self.use = use Process(['iw', 'phy', self.name, 'interface', 'add', ifname, - 'type', 'managed']).wait() + 'type', 'managed'], namespace=self.ns.name if self.ns else None).wait() return self.interface def __str__(self): ret = self.name + ':\n' - ret += '\tUsed By: %s ' % self.use + ret += '\tUsed By: %s' % self.use if self.interface: - ret += '(%s)' % self.interface.name + ret += ' (%s)' % self.interface.name + if self.ns is not None: + ret += ' (ns=%s)' % self.ns.name ret += '\n' @@ -188,7 +198,7 @@ class Hostapd: A set of running hostapd instances. This is really just a single process since hostapd can be started with multiple config files. ''' - def __init__(self, radios, configs, radius): + def __init__(self, ns, radios, configs, radius): if len(configs) != len(radios): raise Exception("Config (%d) and radio (%d) list length not equal" % \ (len(configs), len(radios))) @@ -198,8 +208,8 @@ class Hostapd: Process(['ip', 'link', 'set', 'eth0', 'up']).wait() Process(['ip', 'link', 'set', 'eth1', 'up']).wait() - self.global_ctrl_iface = '/var/run/hostapd/ctrl' - + self.ns = ns + self.global_ctrl_iface = '/var/run/hostapd/ctrl' + (str(ns.name) if ns.name else 'main') self.instances = [HostapdInstance(c, r) for c, r in zip(configs, radios)] ifaces = [rad.interface.name for rad in radios] @@ -227,7 +237,7 @@ class Hostapd: if Process.is_verbose('hostapd'): args.append('-d') - self.process = Process(args) + self.process = Process(args, namespace=ns.name) self.process.wait_for_socket(self.global_ctrl_iface, 30) @@ -397,32 +407,44 @@ class TestContext(Namespace): # tests. In this case you would not care what # was using each radio, just that there was # enough to run all tests. - nradios = 0 - for k, _ in settings.items(): - if k == 'radius_server': - continue - nradios += 1 - - hapd_radios = self.radios[:nradios] - + hapd_configs = [conf for rad, conf in settings.items() if rad != 'radius_server'] + hapd_processes = [(self, self.radios[:len(hapd_configs)], hapd_configs)] else: - hapd_radios = [rad for rad in self.radios if rad.name in settings] - - hapd_configs = [conf for rad, conf in settings.items() if rad != 'radius_server'] + hapd_processes = [] + for ns in [self] + self.namespaces: + ns_radios = [rad for rad in ns.radios if rad.name in settings] + if len(ns_radios): + ns_configs = [settings[rad.name] for rad in ns_radios] + hapd_processes.append((ns, ns_radios, ns_configs)) + if not hapd_processes: + hapd_processes.append((self, [], [])) radius_config = settings.get('radius_server', None) - self.hostapd = Hostapd(hapd_radios, hapd_configs, radius_config) - self.hostapd.attach_cli() + self.hostapd = [Hostapd(ns, radios, configs, radius_config) + for ns, radios, configs in hapd_processes] + + for hapd in self.hostapd: + hapd.attach_cli() def get_frequencies(self): frequencies = [] - for hapd in self.hostapd.instances: - frequencies.append(hapd.cli.frequency) + for hapd in self.hostapd: + frequencies += [instance.cli.frequency for instance in hapd.instances] return frequencies + def get_hapd_instance(self, config=None): + instances = [i for hapd in self.hostapd for i in hapd.instances] + + if config is None: + return instances[0] + + for hapd in instances: + if hapd.config == config: + return hapd + def start_wpas_interfaces(self): if 'WPA_SUPPLICANT' not in self.hw_config: return @@ -543,11 +565,13 @@ class TestContext(Namespace): for arg in vars(self.args): ret += '\t --%s %s\n' % (arg, str(getattr(self.args, arg))) - ret += 'Hostapd:\n' if self.hostapd: - for h in self.hostapd.instances: - ret += '\t%s\n' % str(h) + for hapd in self.hostapd: + ret += 'Hostapd (ns=%s):\n' % (hapd.ns.name,) + for h in hapd.instances: + ret += '\t%s\n' % (str(h),) else: + ret += 'Hostapd:\n' ret += '\tNo Hostapd instances\n' info = self.meminfo_to_dict() diff --git a/tools/utils.py b/tools/utils.py index f3e12a85..bc030230 100644 --- a/tools/utils.py +++ b/tools/utils.py @@ -324,7 +324,7 @@ class Namespace: Process(['ip', 'netns', 'add', name]).wait() for r in radios: - Process(['iw', 'phy', r.name, 'set', 'netns', 'name', name]).wait() + r.set_namespace(self) self.start_dbus()