check, ovs, helpers: fix the seven defects found by the test suite - #23
Merged
Merged
Conversation
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 <florent.carli@rte-france.com>
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 <florent.carli@rte-france.com>
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 <florent.carli@rte-france.com>
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 <florent.carli@rte-france.com>
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 <florent.carli@rte-france.com>
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 <florent.carli@rte-france.com>
|
eroussy
approved these changes
Sep 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #22, which pinned five defects with
xfail(strict=True)instead of fixing them. This fixes those five, plus two of the same kind found while readingovs.py. All date back to6c83aa0(2022-05-21).helpers.py:60return subprocess.run(...)sits insideif "check" not in kargscheck=Falseruns nothingcheck.py:283tag range guarded by"vlan" in port, not"tag"check.py:270"must be set if type is vxlan" nested insideif attribute in portKeyErrorlater atovs.py:239check.py:111_attribute_is_a_portenforces the VLAN range 0-4095check.py:165dup-NIC lists are locals of a per-port functionovs.py:244"options:remote_port=" + port["remote_port"]on a validated intTypeErrorovs.py:214iteratesport["trunks"], but a bare int is validTypeError6 and 7 escaped #22 because
test_ovs.pydrives_create_bridgesdirectly and passedremote_portas a string, a valueconfiguration_checkrejects.One behaviour change: vxlan ports now require
keyandremote_ip. Such configs never worked, they crashed in_create_bridgeswith aKeyError. They now fail the check with the missing attribute named.Coverage: the four statements #22 could not cover were this dead code. Now 100 % statement, 100 % branch, 239 tests, no
xfailleft.Five commits, one per defect. Defects 2 to 4 share one: same forty lines of
_check_port_configuration, same new range checkers._check_port_configurationis also split into per-concern helpers, in the last commit. Its cognitive complexity was 104 against the 15 SonarCloud allows, an issue open since 2022-05-21; fixing defect 5 changed the signature, which re-anchored it onto new code. It now scores 0, and the highest new helper 10. No behaviour change: 2,008 configurations replayed through both versions give identical outcomes, messages included.