From 5f3819b060db7e0fec38c82320d61406abc23572 Mon Sep 17 00:00:00 2001 From: Florent Carli Date: Mon, 7 Sep 2026 19:06:35 +0200 Subject: [PATCH 1/6] check: make the duplicate NIC guard span the configuration dpdk_interfaces and system_interfaces were locals of _check_port_configuration, which runs once per port. Both lists were therefore rebuilt empty on every call and the two "already used in another port" guards could never fire: the same NIC could be claimed by any number of ports without the check saying anything. Hoist the two lists into configuration_check and pass them down, so they accumulate across every port of every bridge. A NIC is claimed by a single port anywhere in the configuration, so the guard deliberately spans bridges and not just the current one. The defect dates back to 6c83aa0, "init repo from meta-seapath folder". It was covered by an xfail(strict=True) marker added in #22, now removed, and the tests cover the two-ports, two-bridges and system-interface cases. Signed-off-by: Florent Carli --- setup_ovs/check.py | 19 ++++++++++--- tests/test_check.py | 67 +++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 76 insertions(+), 10 deletions(-) diff --git a/setup_ovs/check.py b/setup_ovs/check.py index e2de505..a358f8b 100644 --- a/setup_ovs/check.py +++ b/setup_ovs/check.py @@ -32,6 +32,8 @@ def configuration_check(config): raise SetupOVSConfigException( "The configuration should be a dictionary" ) + dpdk_interfaces = [] + system_interfaces = [] if "bridges" in config: if not isinstance(config["bridges"], list): raise SetupOVSConfigException( @@ -51,7 +53,12 @@ def configuration_check(config): raise SetupOVSConfigException( "A port must be a dictionary" ) - _check_port_configuration(bridge["name"], port) + _check_port_configuration( + bridge["name"], + port, + dpdk_interfaces, + system_interfaces, + ) if "other_config" in bridge: attribute_value = ( [bridge["other_config"]] @@ -155,15 +162,19 @@ def _attribute_is_a_mac( ) -def _check_port_configuration(bridge_name, port): +def _check_port_configuration( + bridge_name, port, dpdk_interfaces, system_interfaces +): """ Helper method for _configuration_check which checks the port configuration :param bridge_name: the bridge name in which the port take from :param port: the port configuration + :param dpdk_interfaces: the DPDK NICs already claimed by another port, + appended to by this call + :param system_interfaces: the system NICs already claimed by another + port, appended to by this call """ - dpdk_interfaces = [] - system_interfaces = [] if "name" not in port: raise SetupOVSConfigException( "Bridge {}: Port without name attribute".format(bridge_name) diff --git a/tests/test_check.py b/tests/test_check.py index 8de7af2..ebbe032 100644 --- a/tests/test_check.py +++ b/tests/test_check.py @@ -517,12 +517,6 @@ def test_mac_rejection_message_names_the_mac_attribute(self): class TestDuplicateInterfaces: - @pytest.mark.xfail( - strict=True, - reason="dpdk_interfaces/system_interfaces are locals of " - "_check_port_configuration, which runs once per port, so the " - "duplicate-NIC guard can never fire", - ) def test_rejects_the_same_dpdk_nic_on_two_ports(self, run_command): config = { "bridges": [ @@ -545,3 +539,64 @@ def test_rejects_the_same_dpdk_nic_on_two_ports(self, run_command): } assert_rejects(config, "already used") + + def test_rejects_the_same_dpdk_nic_on_two_bridges(self, run_command): + # A NIC is claimed by a single port anywhere in the configuration, + # so the guard spans every bridge and not only the current one. + port = {"name": "p0", "type": "dpdk", "interface": "0000:3b:00.0"} + config = { + "bridges": [ + {"name": "br0", "ports": [dict(port, name="p0")]}, + {"name": "br1", "ports": [dict(port, name="p1")]}, + ] + } + + assert_rejects(config, "already used") + + def test_rejects_the_same_system_nic_on_two_ports( + self, existing_interfaces + ): + port = {"name": "p0", "type": "system", "interface": "eth0"} + config = { + "bridges": [ + { + "name": "br0", + "ports": [dict(port, name="p0"), dict(port, name="p1")], + } + ] + } + + assert_rejects(config, "already used") + + def test_accepts_distinct_nics(self, run_command, existing_interfaces): + check.configuration_check( + { + "bridges": [ + { + "name": "br0", + "ports": [ + { + "name": "p0", + "type": "dpdk", + "interface": "0000:3b:00.0", + }, + { + "name": "p1", + "type": "dpdk", + "interface": "0000:3b:00.1", + }, + { + "name": "p2", + "type": "system", + "interface": "eth0", + }, + { + "name": "p3", + "type": "system", + "interface": "eth1", + }, + ], + } + ] + } + ) From 78228f479d06c8b8c244aadd8e5d83077bf9b659 Mon Sep 17 00:00:00 2001 From: Florent Carli Date: Mon, 7 Sep 2026 19:06:56 +0200 Subject: [PATCH 2/6] check: fix three defects in the port attribute validation The three come from 6c83aa0, "init repo from meta-seapath folder", and each was covered by an xfail(strict=True) marker added in #22. They sit in the same forty lines of _check_port_configuration and share the same range checkers, so they are fixed together. The tag range was never checked. The guard read "vlan" in port while the attribute ovs._create_bridges actually consumes is "tag", so any tag value went through untouched, and a configuration carrying "vlan" without "tag" raised KeyError on port["tag"] instead. Validate "tag". "vlan" is consumed nowhere and is now simply ignored. The "must be set if type is vxlan" branch was unreachable. It sat inside "if attribute in port" and then tested "attribute not in port", which cannot both hold. A vxlan port declaring neither key nor remote_ip passed the check and blew up later in ovs.py on port["remote_ip"]. The requirement is now tested on the port type instead of on the attribute, and the "ignored when type is not vxlan" warning moves to the matching else. The IANA VXLAN port 4789 was rejected. _attribute_is_a_port is documented as a TCP/UDP port check but enforced the VLAN tag range, 0 to 4,095, so no configuration could use the default VXLAN destination port. Split the two ranges: _attribute_is_a_port accepts 0 to 65,535, and the new _attribute_is_a_vlan_tag keeps 0 to 4,095 for tag and trunks. Both delegate to _attribute_is_in_range, so the integer check is written once. Requiring key and remote_ip on vxlan ports is the one behaviour change that can reject a configuration accepted before. Such a configuration never worked: it crashed in _create_bridges with a KeyError. It now fails during the check with a message naming the missing attribute. Also drop a third argument passed to the mac error message, which has only two placeholders. Signed-off-by: Florent Carli --- setup_ovs/check.py | 70 ++++++++++++++++++++--------- tests/test_check.py | 107 +++++++++++++++++++++++++++++++------------- 2 files changed, 127 insertions(+), 50 deletions(-) diff --git a/setup_ovs/check.py b/setup_ovs/check.py index a358f8b..0ebd762 100644 --- a/setup_ovs/check.py +++ b/setup_ovs/check.py @@ -100,28 +100,59 @@ def configuration_check(config): logging.info("Configuration check: OK") -def _attribute_is_a_port( - attribute_name, attribute_value, bridge_name, port_name +def _attribute_is_in_range( + attribute_name, attribute_value, bridge_name, port_name, maximum ): """ - Check if an attribute is a TCP or UDP port + Check if an attribute is an integer between 0 and maximum :param attribute_name: The attribute name :param attribute_value: The attribute value :param bridge_name: The attribute bridge name :param port_name: The attribute port name + :param maximum: The highest accepted value, included """ if not isinstance(attribute_value, int): raise SetupOVSConfigException( "Bridge {} Port {}: attribute {} must be an " "integer".format(bridge_name, port_name, attribute_name) ) - if attribute_value < 0 or attribute_value > 4095: + if attribute_value < 0 or attribute_value > maximum: raise SetupOVSConfigException( "Bridge {} Port {}: attribute {} must be in range 0 to " - "4,095".format(bridge_name, port_name, attribute_name) + "{:,}".format(bridge_name, port_name, attribute_name, maximum) ) +def _attribute_is_a_port( + attribute_name, attribute_value, bridge_name, port_name +): + """ + Check if an attribute is a TCP or UDP port + :param attribute_name: The attribute name + :param attribute_value: The attribute value + :param bridge_name: The attribute bridge name + :param port_name: The attribute port name + """ + _attribute_is_in_range( + attribute_name, attribute_value, bridge_name, port_name, 65535 + ) + + +def _attribute_is_a_vlan_tag( + attribute_name, attribute_value, bridge_name, port_name +): + """ + Check if an attribute is a 802.1Q VLAN identifier + :param attribute_name: The attribute name + :param attribute_value: The attribute value + :param bridge_name: The attribute bridge name + :param port_name: The attribute port name + """ + _attribute_is_in_range( + attribute_name, attribute_value, bridge_name, port_name, 4095 + ) + + def _attribute_is_an_ipv4( attribute_name, attribute_value, bridge_name, port_name ): @@ -275,23 +306,24 @@ def _check_port_configuration( "Bridge {} Port {}: attribute interface is ignored when " " type is not system nor dpdk".format(bridge_name, port_name) ) - for attribute in ("key", "remote_ip", "remote_port"): - if attribute in port: - if port["type"] == "vxlan": - if attribute != "remote_port" and attribute not in port: - raise SetupOVSConfigException( - "Bridge {} Port {}: {} must be set if type is " - "vxlan".format(bridge_name, port_name, attribute) - ) - else: + if port_type == "vxlan": + for attribute in ("key", "remote_ip"): + if attribute not in port: + raise SetupOVSConfigException( + "Bridge {} Port {}: {} must be set if type is " + "vxlan".format(bridge_name, port_name, attribute) + ) + else: + for attribute in ("key", "remote_ip", "remote_port"): + if attribute in port: logging.warning( "Bridge {} Port {}: attribute {} is ignored" " when type is not vxlan".format( bridge_name, port_name, attribute ) ) - if "vlan" in port: - _attribute_is_a_port("tag", port["tag"], bridge_name, port_name) + if "tag" in port: + _attribute_is_a_vlan_tag("tag", port["tag"], bridge_name, port_name) if "trunks" in port: trunks = ( [port["trunks"]] if isinstance(port["trunks"], int) else port["trunks"] @@ -302,7 +334,7 @@ def _check_port_configuration( " an integer list".format(bridge_name, port_name) ) for trunk in trunks: - _attribute_is_a_port("trunks", trunk, bridge_name, port_name) + _attribute_is_a_vlan_tag("trunks", trunk, bridge_name, port_name) if "vlan_mode" in port and port["vlan_mode"] not in ( "access", "native-tagged", @@ -378,7 +410,5 @@ def _check_port_configuration( raise SetupOVSConfigException( "Bridge {} Port {}: attribute mac only works if" " interface is tap or" - " dpdkvhostuserclient".format( - bridge_name, port_name, attribute - ) + " dpdkvhostuserclient".format(bridge_name, port_name) ) diff --git a/tests/test_check.py b/tests/test_check.py index ebbe032..25040ea 100644 --- a/tests/test_check.py +++ b/tests/test_check.py @@ -165,13 +165,25 @@ def test_rejects_unknown_type(self): @pytest.mark.parametrize( "port_type", - ["internal", "tap", "dpdkvhostuserclient", "vxlan"], + ["internal", "tap", "dpdkvhostuserclient"], ) def test_accepts_types_needing_no_interface(self, port_type): check.configuration_check( bridge_with_port({"name": "p0", "type": port_type}) ) + def test_accepts_a_complete_vxlan_port(self): + check.configuration_check( + bridge_with_port( + { + "name": "p0", + "type": "vxlan", + "key": "42", + "remote_ip": "10.0.0.1", + } + ) + ) + def test_warns_when_interface_is_ignored(self, caplog): check.configuration_check( bridge_with_port( @@ -321,16 +333,29 @@ def test_rejects_out_of_range_trunk(self, value): assert_rejects(config, "range 0 to") - @pytest.mark.xfail( - strict=True, - reason="the tag range check is guarded by 'vlan' in port while the " - "attribute consumed by ovs._create_bridges is 'tag', so tag is never " - "validated", - ) - def test_rejects_out_of_range_tag(self): - config = bridge_with_port({"name": "p0", "type": "tap", "tag": 9999}) + @pytest.mark.parametrize("value", [-1, 4096, 9999]) + def test_rejects_out_of_range_tag(self, value): + config = bridge_with_port({"name": "p0", "type": "tap", "tag": value}) - assert_rejects(config, "range 0 to") + assert_rejects(config, "range 0 to 4,095") + + def test_rejects_non_integer_tag(self): + config = bridge_with_port({"name": "p0", "type": "tap", "tag": "10"}) + + assert_rejects(config, "must be an integer") + + @pytest.mark.parametrize("value", [0, 10, 4095]) + def test_accepts_in_range_tag(self, value): + check.configuration_check( + bridge_with_port({"name": "p0", "type": "tap", "tag": value}) + ) + + def test_vlan_without_tag_is_not_a_crash(self): + # "vlan" is not consumed by ovs._create_bridges. It used to gate the + # tag check and raised KeyError when tag was absent. + check.configuration_check( + bridge_with_port({"name": "p0", "type": "tap", "vlan": 10}) + ) class TestPolicingAndVxlan: @@ -367,24 +392,31 @@ def test_accepts_complete_vxlan_port(self): def test_rejects_non_integer_remote_port(self): config = bridge_with_port( - {"name": "p0", "type": "vxlan", "remote_port": "4789"} + { + "name": "p0", + "type": "vxlan", + "key": "42", + "remote_ip": "10.0.0.1", + "remote_port": "4789", + } ) assert_rejects(config, "must be an integer") - def test_rejects_negative_remote_port(self): + @pytest.mark.parametrize("value", [-1, 65536]) + def test_rejects_out_of_range_remote_port(self, value): config = bridge_with_port( - {"name": "p0", "type": "vxlan", "remote_port": -1} + { + "name": "p0", + "type": "vxlan", + "key": "42", + "remote_ip": "10.0.0.1", + "remote_port": value, + } ) - assert_rejects(config, "range 0 to") + assert_rejects(config, "range 0 to 65,535") - @pytest.mark.xfail( - strict=True, - reason="_attribute_is_a_port enforces the VLAN tag range 0-4095 on " - "remote_port too, so the IANA VXLAN port 4789 is refused. A TCP/UDP " - "port goes up to 65535", - ) def test_accepts_the_iana_vxlan_port(self): check.configuration_check( bridge_with_port( @@ -399,13 +431,20 @@ def test_accepts_the_iana_vxlan_port(self): ) def test_rejects_non_string_key(self): - config = bridge_with_port({"name": "p0", "type": "vxlan", "key": 42}) + config = bridge_with_port( + { + "name": "p0", + "type": "vxlan", + "key": 42, + "remote_ip": "10.0.0.1", + } + ) assert_rejects(config, "must be a string") def test_rejects_malformed_remote_ip(self): config = bridge_with_port( - {"name": "p0", "type": "vxlan", "remote_ip": "10.0.0"} + {"name": "p0", "type": "vxlan", "key": "42", "remote_ip": "10.0.0"} ) assert_rejects(config, "IPv4 address") @@ -424,17 +463,25 @@ def test_rejects_non_string_hook_file(self): assert_rejects(config, "must be a string") - @pytest.mark.xfail( - strict=True, - reason="the 'must be set if type is vxlan' branch is unreachable: it " - "sits inside 'if attribute in port' and then tests " - "'attribute not in port'. ovs._create_bridges later raises KeyError " - "on such a config", - ) def test_rejects_vxlan_port_without_key_nor_remote_ip(self): config = bridge_with_port({"name": "p0", "type": "vxlan"}) - assert_rejects(config, "vxlan") + assert_rejects(config, "must be set if type is vxlan") + + @pytest.mark.parametrize("missing", ["key", "remote_ip"]) + def test_rejects_vxlan_port_missing_one_attribute(self, missing): + port = { + "name": "p0", + "type": "vxlan", + "key": "42", + "remote_ip": "10.0.0.1", + } + del port[missing] + + assert_rejects( + bridge_with_port(port), + "{} must be set if type is vxlan".format(missing), + ) class TestIpAndMac: From 1a2239fe1df79dd1d155555acd6a8ce079a930c9 Mon Sep 17 00:00:00 2001 From: Florent Carli Date: Mon, 7 Sep 2026 19:07:11 +0200 Subject: [PATCH 3/6] helpers: run the command when the caller passes check run_command silently did nothing whenever a caller passed check explicitly. The "return subprocess.run(...)" line sat inside the "if 'check' not in kargs" branch, so the only path that reached subprocess was the one that also set the default. Passing check=False, the very argument the docstring says the helper accepts, returned None without running anything. Dedent the call, and the stdout suppression with it: silencing stdout outside DEBUG has nothing to do with how check was obtained, so it now applies on both paths as the docstring describes. No caller passes check today, which is why it went unnoticed since 6c83aa0, "init repo from meta-seapath folder". It was a trap for the next one. The xfail(strict=True) marker added in #22 is removed. Signed-off-by: Florent Carli --- setup_ovs/helpers.py | 20 ++++++++++---------- tests/test_helpers.py | 19 +++++++++++++------ 2 files changed, 23 insertions(+), 16 deletions(-) diff --git a/setup_ovs/helpers.py b/setup_ovs/helpers.py index 2d39215..3ecc7c6 100644 --- a/setup_ovs/helpers.py +++ b/setup_ovs/helpers.py @@ -48,13 +48,13 @@ def run_command(*cmd_args, **kargs): if not dry_run: if "check" not in kargs: kargs["check"] = True - if ( - logging.getLogger().getEffectiveLevel() != logging.DEBUG - and "stdout" not in kargs - and ( - "capture_output" not in kargs - or not kargs["capture_output"] - ) - ): - kargs["stdout"] = subprocess.DEVNULL - return subprocess.run(cmd_args, **kargs) + if ( + logging.getLogger().getEffectiveLevel() != logging.DEBUG + and "stdout" not in kargs + and ( + "capture_output" not in kargs + or not kargs["capture_output"] + ) + ): + kargs["stdout"] = subprocess.DEVNULL + return subprocess.run(cmd_args, **kargs) diff --git a/tests/test_helpers.py b/tests/test_helpers.py index f821214..72d64fe 100644 --- a/tests/test_helpers.py +++ b/tests/test_helpers.py @@ -177,12 +177,6 @@ def fake_run(cmd_args, **kwargs): with pytest.raises(subprocess.CalledProcessError): helpers.run_command("/bin/false") - @pytest.mark.xfail( - strict=True, - reason="run_command returns without running anything when the caller " - "passes check explicitly: the subprocess.run call sits inside the " - "'if \"check\" not in kargs' branch", - ) def test_explicit_check_still_runs_the_command(self, monkeypatch): recorded = {} monkeypatch.setattr( @@ -194,6 +188,19 @@ def test_explicit_check_still_runs_the_command(self, monkeypatch): helpers.run_command("/bin/false", check=False) assert recorded["cmd_args"] == ("/bin/false",) + assert recorded["kwargs"]["check"] is False + + def test_explicit_check_still_silences_stdout(self, monkeypatch): + recorded = {} + monkeypatch.setattr( + subprocess, + "run", + lambda a, **k: recorded.update(cmd_args=a, kwargs=k), + ) + + helpers.run_command("/bin/true", check=False) + + assert recorded["kwargs"]["stdout"] == subprocess.DEVNULL class TestMatchers: From 34be10ffbfae6554cb73c2797bdf9db10cdb32f6 Mon Sep 17 00:00:00 2001 From: Florent Carli Date: Mon, 7 Sep 2026 19:07:18 +0200 Subject: [PATCH 4/6] ovs: build trunks and remote_port from their validated types Two places in _create_bridges disagreed with what configuration_check accepts, so a configuration that passed the check crashed while being applied. remote_port is validated as an integer, and _create_bridges built its argument with "options:remote_port=" + port["remote_port"]. Every vxlan port carrying a remote_port therefore raised TypeError. Format the value instead. trunks is validated as an integer or an integer list, and _create_bridges iterated port["trunks"] directly, so a bare integer raised TypeError. Normalise it the same way check.py does before joining. Neither was caught by the tests added in #22 because test_ovs.py drives _create_bridges directly and happened to pass remote_port as a string, a value configuration_check rejects. The tests now use the types the check actually produces, and cover the scalar trunks case. Both defects date back to 6c83aa0, "init repo from meta-seapath folder", except the trunks list form, which arrived with the attribute. Signed-off-by: Florent Carli --- setup_ovs/ovs.py | 12 +++++++++--- tests/test_ovs.py | 3 ++- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/setup_ovs/ovs.py b/setup_ovs/ovs.py index 791f431..715cc15 100644 --- a/setup_ovs/ovs.py +++ b/setup_ovs/ovs.py @@ -209,9 +209,13 @@ def _create_bridges(config, dpdk_bridges): if "tag" in port: cmd_args.append("tag={}".format(port["tag"])) if "trunks" in port: + trunks = ( + [port["trunks"]] + if isinstance(port["trunks"], int) + else port["trunks"] + ) cmd_args.append( - "trunks=" - + ",".join([str(tag) for tag in port["trunks"]]) + "trunks=" + ",".join([str(tag) for tag in trunks]) ) if port_type not in ("tap", "system"): cmd_args += [ @@ -241,7 +245,9 @@ def _create_bridges(config, dpdk_bridges): ] if "remote_port" in port: cmd_args.append( - "options:remote_port=" + port["remote_port"] + "options:remote_port={}".format( + port["remote_port"] + ) ) if "external-ids" in port: diff --git a/tests/test_ovs.py b/tests/test_ovs.py index 5452fe7..43e0c4e 100644 --- a/tests/test_ovs.py +++ b/tests/test_ovs.py @@ -192,7 +192,7 @@ def test_vxlan_remote_port_is_optional(self, run_command): "type": "vxlan", "remote_ip": "10.0.0.1", "key": "42", - "remote_port": "4789", + "remote_port": 4789, } ) ) @@ -205,6 +205,7 @@ def test_vxlan_remote_port_is_optional(self, run_command): ("vlan_mode", "access", "vlan_mode=access"), ("tag", 10, "tag=10"), ("trunks", [1, 2], "trunks=1,2"), + ("trunks", 5, "trunks=5"), ("ofport_request", 7, "ofport_request=7"), ], ) From 8f602fdb97a7ff700f754d563d9f95838a92fa41 Mon Sep 17 00:00:00 2001 From: Florent Carli Date: Mon, 7 Sep 2026 19:08:08 +0200 Subject: [PATCH 5/6] README: drop the paragraph about the xfail markers The five xfail(strict=True) markers documented there pinned the defects this branch fixes. All five are gone, so the paragraph describes a state the suite is no longer in. Signed-off-by: Florent Carli --- README.md | 5 ----- 1 file changed, 5 deletions(-) diff --git a/README.md b/README.md index 3573950..3052bf6 100644 --- a/README.md +++ b/README.md @@ -22,11 +22,6 @@ pytest --cov=setup_ovs --cov-report=term-missing --cov-report=xml Branch coverage is enabled in `pyproject.toml`, so the report covers both the statement and the branch criteria. -A handful of tests are marked `xfail(strict=True)`. Each one documents a bug -found while writing the suite and pins the current, wrong behaviour: the -suite fails again the day the bug is fixed, which forces the marker to be -removed along with the fix. Their `reason` field states the defect. - ## Reproducible build The wheel is byte-for-byte reproducible provided `SOURCE_DATE_EPOCH` is set. From 1a93e5c8b6ca0c081e1923277d23d9a966d48d17 Mon Sep 17 00:00:00 2001 From: Florent Carli Date: Mon, 7 Sep 2026 20:37:45 +0200 Subject: [PATCH 6/6] check: split _check_port_configuration into per-concern helpers The function carried a cognitive complexity of 104 against the 15 SonarCloud allows. The issue dates from 2022-05-21, the repository's first commit, and had been sitting open on main ever since. Fixing the duplicate-NIC defect changed the signature, which re-anchored it onto new code and turned the maintainability rating on this branch to B. Split it along the lines the body already had: name and type, the interface attribute with one helper per port class, then the vxlan, VLAN, integer, string, list and mac attribute groups. _check_port_configuration is now the sequence of those calls and scores 0. The highest of the new helpers is 10. configuration_check keeps its own pre-existing complexity issue. Its definition line is untouched, so it stays out of the new code. No behaviour change. Every error message, every exception type and the order the checks run in are preserved, verified by replaying 2,008 configurations through both versions and comparing the outcomes: no difference. The suite still passes, unchanged, at 100 % statement and 100 % branch coverage. Signed-off-by: Florent Carli --- setup_ovs/check.py | 462 ++++++++++++++++++++++++++++++--------------- 1 file changed, 306 insertions(+), 156 deletions(-) diff --git a/setup_ovs/check.py b/setup_ovs/check.py index 0ebd762..5ef169d 100644 --- a/setup_ovs/check.py +++ b/setup_ovs/check.py @@ -193,18 +193,26 @@ def _attribute_is_a_mac( ) -def _check_port_configuration( - bridge_name, port, dpdk_interfaces, system_interfaces -): +PORT_TYPES = ( + "internal", + "tap", + "system", + "dpdk", + "dpdkvhostuserclient", + "vxlan", +) + +VLAN_MODES = ("access", "native-tagged", "native-untagged", "trunk") + +MAC_CAPABLE_TYPES = ("tap", "dpdkvhostuserclient") + + +def _check_name_and_type(bridge_name, port): """ - Helper method for _configuration_check which checks the port - configuration + Check the two mandatory port attributes :param bridge_name: the bridge name in which the port take from :param port: the port configuration - :param dpdk_interfaces: the DPDK NICs already claimed by another port, - appended to by this call - :param system_interfaces: the system NICs already claimed by another - port, appended to by this call + :return: the port name and the port type """ if "name" not in port: raise SetupOVSConfigException( @@ -217,95 +225,146 @@ def _check_port_configuration( "Bridge {}: Port without type attribute".format(bridge_name) ) port_type = port["type"] - if port_type not in ( - "internal", - "tap", - "system", - "dpdk", - "dpdkvhostuserclient", - "vxlan", - ): + if port_type not in PORT_TYPES: raise SetupOVSConfigException( "Bridge {} Port {}: Bad type value: {}".format( bridge_name, port_name, port_type ) ) - if port_type in ("dpdk", "system"): - if "interface" not in port: - raise SetupOVSConfigException( - "Bridge {} Port {}: attribute interface is required with " - "type {}".format(bridge_name, port_name, port_type) + return port_name, port_type + + +def _lspci_address(bridge_name, port_name, interface): + """ + Convert a NIC PCI address in the lspci format + :param bridge_name: the port bridge name + :param port_name: the port name + :param interface: the interface PCI address + :return: the address in the lspci format + """ + match = helpers.PCI_ADDRESS_MATCHER.match(interface) + if not match: + raise SetupOVSConfigException( + "Bridge {} Port {}: NIC {} is not a PCI address." + " Invalid format".format(bridge_name, port_name, interface) + ) + match_group = match.groupdict() + lspci_nic_address_part1 = int(match_group["part1"], 16) + lspci_nic_address_part2 = int(match_group["part2"], 16) + lspci_nic_address_part3 = int(match_group["part3"], 16) + return ( + f"{lspci_nic_address_part1:02x}:" + f"{lspci_nic_address_part2:02x}." + f"{lspci_nic_address_part3:01x}" + ) + + +def _check_dpdk_interface( + bridge_name, port_name, interface, dpdk_interfaces +): + """ + Check a DPDK port interface and claim the NIC + :param bridge_name: the port bridge name + :param port_name: the port name + :param interface: the interface PCI address + :param dpdk_interfaces: the NICs already claimed, appended to by this call + """ + lspci_nic_address = _lspci_address(bridge_name, port_name, interface) + if not helpers.dry_run: + try: + helpers.run_command( + "/usr/bin/lspci -mm | /bin/grep -q " + lspci_nic_address, + shell=True, ) - interface = port["interface"] - if port_type == "dpdk": - # Check interface is a PCI address - match = helpers.PCI_ADDRESS_MATCHER.match(interface) - if not match: - raise SetupOVSConfigException( - "Bridge {} Port {}: NIC {} is not a PCI address." - " Invalid format".format(bridge_name, port_name, interface) + except subprocess.CalledProcessError: + raise SetupOVSConfigException( + "Bridge {} Port {}: Can't find the NIC {}".format( + bridge_name, port_name, interface ) - - # Convert the NIC PCI address in the lspci format - match_group = match.groupdict() - lspci_nic_address_part1 = int(match_group["part1"], 16) - lspci_nic_address_part2 = int(match_group["part2"], 16) - lspci_nic_address_part3 = int(match_group["part3"], 16) - lspci_nic_address = ( - f"{lspci_nic_address_part1:02x}:" - f"{lspci_nic_address_part2:02x}." - f"{lspci_nic_address_part3:01x}" ) - if not helpers.dry_run: - try: - helpers.run_command( - "/usr/bin/lspci -mm | /bin/grep -q " - + lspci_nic_address, - shell=True, - ) - except subprocess.CalledProcessError: - raise SetupOVSConfigException( - "Bridge {} Port {}: Can't find the NIC {}".format( - bridge_name, port_name, interface - ) - ) + if interface in dpdk_interfaces: + raise SetupOVSConfigException( + "Bridge {} Port {}: NIC {} already used in another " + "port".format(bridge_name, port_name, interface) + ) + dpdk_interfaces.append(interface) - if interface in dpdk_interfaces: - raise SetupOVSConfigException( - "Bridge {} Port {}: NIC {} already used in another " - "port".format(bridge_name, port_name, interface) - ) - dpdk_interfaces.append(interface) + +def _check_system_interface( + bridge_name, port_name, interface, system_interfaces +): + """ + Check a system port interface and claim the NIC + :param bridge_name: the port bridge name + :param port_name: the port name + :param interface: the network interface name + :param system_interfaces: the NICs already claimed, appended to by this + call + """ + if not os.path.isdir( + os.path.join("/proc/sys/net/ipv4/conf/", interface) + ): + message = ( + "Bridge {} Port {}: could not find the network " + "interface {}".format(bridge_name, port_name, interface) + ) + if helpers.dry_run: + logging.error(message) else: - if not os.path.isdir( - os.path.join("/proc/sys/net/ipv4/conf/", interface) - ): - if helpers.dry_run: - logging.error( - "Bridge {} Port {}: could not find the network " - "interface {}".format( - bridge_name, port_name, interface - ) - ) - else: - raise SetupOVSConfigException( - "Bridge {} Port {}: could not find the network " - "interface {}".format( - bridge_name, port_name, interface - ) - ) - if interface in system_interfaces: - raise SetupOVSConfigException( - "Bridge {} Port {}: {} already used in another" - " port".format(bridge_name, port_name, interface) - ) - system_interfaces.append(interface) - else: + raise SetupOVSConfigException(message) + if interface in system_interfaces: + raise SetupOVSConfigException( + "Bridge {} Port {}: {} already used in another" + " port".format(bridge_name, port_name, interface) + ) + system_interfaces.append(interface) + + +def _check_interface( + bridge_name, port_name, port_type, port, dpdk_interfaces, + system_interfaces +): + """ + Check the interface attribute, which only the dpdk and system types use + :param bridge_name: the port bridge name + :param port_name: the port name + :param port_type: the port type + :param port: the port configuration + :param dpdk_interfaces: the DPDK NICs already claimed + :param system_interfaces: the system NICs already claimed + """ + if port_type not in ("dpdk", "system"): if "interface" in port: logging.warning( "Bridge {} Port {}: attribute interface is ignored when " " type is not system nor dpdk".format(bridge_name, port_name) ) + return + if "interface" not in port: + raise SetupOVSConfigException( + "Bridge {} Port {}: attribute interface is required with " + "type {}".format(bridge_name, port_name, port_type) + ) + interface = port["interface"] + if port_type == "dpdk": + _check_dpdk_interface( + bridge_name, port_name, interface, dpdk_interfaces + ) + else: + _check_system_interface( + bridge_name, port_name, interface, system_interfaces + ) + + +def _check_vxlan_attributes(bridge_name, port_name, port_type, port): + """ + Check the attributes a vxlan port requires, and warn about them on any + other type + :param bridge_name: the port bridge name + :param port_name: the port name + :param port_type: the port type + :param port: the port configuration + """ if port_type == "vxlan": for attribute in ("key", "remote_ip"): if attribute not in port: @@ -313,20 +372,31 @@ def _check_port_configuration( "Bridge {} Port {}: {} must be set if type is " "vxlan".format(bridge_name, port_name, attribute) ) - else: - for attribute in ("key", "remote_ip", "remote_port"): - if attribute in port: - logging.warning( - "Bridge {} Port {}: attribute {} is ignored" - " when type is not vxlan".format( - bridge_name, port_name, attribute - ) + return + for attribute in ("key", "remote_ip", "remote_port"): + if attribute in port: + logging.warning( + "Bridge {} Port {}: attribute {} is ignored" + " when type is not vxlan".format( + bridge_name, port_name, attribute ) + ) + + +def _check_vlan_attributes(bridge_name, port_name, port): + """ + Check the tag, trunks and vlan_mode attributes + :param bridge_name: the port bridge name + :param port_name: the port name + :param port: the port configuration + """ if "tag" in port: _attribute_is_a_vlan_tag("tag", port["tag"], bridge_name, port_name) if "trunks" in port: trunks = ( - [port["trunks"]] if isinstance(port["trunks"], int) else port["trunks"] + [port["trunks"]] + if isinstance(port["trunks"], int) + else port["trunks"] ) if not isinstance(trunks, list): raise SetupOVSConfigException( @@ -335,80 +405,160 @@ def _check_port_configuration( ) for trunk in trunks: _attribute_is_a_vlan_tag("trunks", trunk, bridge_name, port_name) - if "vlan_mode" in port and port["vlan_mode"] not in ( - "access", - "native-tagged", - "native-untagged", - "trunk", - ): + if "vlan_mode" in port and port["vlan_mode"] not in VLAN_MODES: raise SetupOVSConfigException( "Bridge {} Port {}: Bad vlan_mode value: {}".format( bridge_name, port_name, port["vlan_mode"] ) ) - for attribute in ( - "ingress_policing_rate", - "ingress_policing_burst", - ): - if attribute in port: - if not isinstance(port[attribute], int): - raise SetupOVSConfigException( - "Bridge {} Port {}: attribute {} must be an " - "integer".format(bridge_name, port_name, attribute) - ) + + +def _check_integer_attributes(bridge_name, port_name, port): + """ + Check the policing attributes and remote_port + :param bridge_name: the port bridge name + :param port_name: the port name + :param port: the port configuration + """ + for attribute in ("ingress_policing_rate", "ingress_policing_burst"): + if attribute in port and not isinstance(port[attribute], int): + raise SetupOVSConfigException( + "Bridge {} Port {}: attribute {} must be an " + "integer".format(bridge_name, port_name, attribute) + ) if "remote_port" in port: _attribute_is_a_port( "remote_port", port["remote_port"], bridge_name, port_name ) + + +def _check_string_attributes(bridge_name, port_name, port): + """ + Check the attributes which must be plain strings + :param bridge_name: the port bridge name + :param port_name: the port name + :param port: the port configuration + """ for attribute in ("key", "remote_ip", "hook_file"): - if attribute in port: - if not isinstance(port[attribute], str): - raise SetupOVSConfigException( - "Bridge {} Port {}: attribute {} must be a " - "string".format(bridge_name, port_name, attribute) - ) - if attribute == "remote_ip": - _attribute_is_an_ipv4( - attribute, port[attribute], bridge_name, port_name - ) - for attribute in ("other_config", "ip"): - if attribute in port: - attribute_value = ( - [port[attribute]] - if isinstance(port[attribute], str) - else port[attribute] + if attribute not in port: + continue + if not isinstance(port[attribute], str): + raise SetupOVSConfigException( + "Bridge {} Port {}: attribute {} must be a " + "string".format(bridge_name, port_name, attribute) ) - if not isinstance(attribute_value, list): + if attribute == "remote_ip": + _attribute_is_an_ipv4( + attribute, port[attribute], bridge_name, port_name + ) + + +def _check_ip_element(bridge_name, port_name, port, attribute, element): + """ + Check one element of the ip attribute + :param bridge_name: the port bridge name + :param port_name: the port name + :param port: the port configuration + :param attribute: the attribute name + :param element: the element to check + """ + _attribute_is_an_ipv4(attribute, element, bridge_name, port_name) + if port["type"] not in MAC_CAPABLE_TYPES: + raise SetupOVSConfigException( + "Bridge {} Port {}: attribute {} only works if" + " interface is tap or" + " dpdkvhostuserclient".format(bridge_name, port_name, attribute) + ) + + +def _as_string_list(bridge_name, port_name, attribute, port): + """ + Read an attribute accepting a string or a string list, as a list + :param bridge_name: the port bridge name + :param port_name: the port name + :param attribute: the attribute name + :param port: the port configuration + :return: the attribute value as a list + """ + attribute_value = ( + [port[attribute]] + if isinstance(port[attribute], str) + else port[attribute] + ) + if not isinstance(attribute_value, list): + raise SetupOVSConfigException( + "Bridge {} Port {}: attribute {} must be a string or " + "a string list".format(bridge_name, port_name, attribute) + ) + return attribute_value + + +def _check_list_attributes(bridge_name, port_name, port): + """ + Check the attributes accepting a string or a string list + :param bridge_name: the port bridge name + :param port_name: the port name + :param port: the port configuration + """ + if "other_config" in port: + for element in _as_string_list( + bridge_name, port_name, "other_config", port + ): + if not isinstance(element, str): raise SetupOVSConfigException( - "Bridge {} Port {}: attribute {} must be a string or " - "a string list".format(bridge_name, port_name, attribute) - ) - for element in attribute_value: - if attribute == "other_config": - if not isinstance(element, str): - raise SetupOVSConfigException( - "Bridge {} Port {}: attribute {} must be a string" - " or a string list".format( - bridge_name, port_name, attribute - ) - ) - else: - _attribute_is_an_ipv4( - attribute, element, bridge_name, port_name + "Bridge {} Port {}: attribute {} must be a string" + " or a string list".format( + bridge_name, port_name, "other_config" ) - if port["type"] not in ("tap", "dpdkvhostuserclient"): - raise SetupOVSConfigException( - "Bridge {} Port {}: attribute {} only works if" - " interface is tap or" - " dpdkvhostuserclient".format( - bridge_name, port_name, attribute - ) - ) - if "mac" in port: - _attribute_is_a_mac("mac", port["mac"], bridge_name, port_name) - if port["type"] not in ("tap", "dpdkvhostuserclient"): - raise SetupOVSConfigException( - "Bridge {} Port {}: attribute mac only works if" - " interface is tap or" - " dpdkvhostuserclient".format(bridge_name, port_name) - ) + ) + if "ip" in port: + for element in _as_string_list(bridge_name, port_name, "ip", port): + _check_ip_element(bridge_name, port_name, port, "ip", element) + + +def _check_mac_attribute(bridge_name, port_name, port): + """ + Check the mac attribute + :param bridge_name: the port bridge name + :param port_name: the port name + :param port: the port configuration + """ + if "mac" not in port: + return + _attribute_is_a_mac("mac", port["mac"], bridge_name, port_name) + if port["type"] not in MAC_CAPABLE_TYPES: + raise SetupOVSConfigException( + "Bridge {} Port {}: attribute mac only works if" + " interface is tap or" + " dpdkvhostuserclient".format(bridge_name, port_name) + ) + + +def _check_port_configuration( + bridge_name, port, dpdk_interfaces, system_interfaces +): + """ + Helper method for _configuration_check which checks the port + configuration + :param bridge_name: the bridge name in which the port take from + :param port: the port configuration + :param dpdk_interfaces: the DPDK NICs already claimed by another port, + appended to by this call + :param system_interfaces: the system NICs already claimed by another + port, appended to by this call + """ + port_name, port_type = _check_name_and_type(bridge_name, port) + _check_interface( + bridge_name, + port_name, + port_type, + port, + dpdk_interfaces, + system_interfaces, + ) + _check_vxlan_attributes(bridge_name, port_name, port_type, port) + _check_vlan_attributes(bridge_name, port_name, port) + _check_integer_attributes(bridge_name, port_name, port) + _check_string_attributes(bridge_name, port_name, port) + _check_list_attributes(bridge_name, port_name, port) + _check_mac_attribute(bridge_name, port_name, port)