Add traffic rules tests by semarie · Pull Request #394 · xcp-ng/xcp-ng-tests

semarie · 2026-02-11T07:44:18Z

proper integration in tests
simple rules
- VIF rule: add/delete with simple vm.start/destroy cycle
- Network rule: add/delete with simple vm.start/destroy cycle
  - FAIL without cookie usage:
    - xo-server-sdn-controller part
    - xapi plugin part
VM migration (VIF / Network rules FAIL without some patches)
tests with VLAN
- VIF rule
- Network rule (FAIL without cookie PR)
tests with Tunnel (aka 'Private Network' in XO)

semarie · 2026-02-20T08:37:52Z

To properly integrate this PR, the following PR should be merged first:

semarie · 2026-07-03T08:35:22Z

rebased to get the xo-cli commits

semarie · 2026-07-15T09:22:44Z

last push : support running on hosts using VLANs. need xcp-ng-xapi-plugins#68

semarie · 2026-07-17T08:44:09Z

Last push:

update minimal requirement of xcp-ng-xapi-plugins to v1.17.0 (upcoming version)

semarie · 2026-08-05T16:50:41Z

last push:

rebased on master
take care of conflicts

semarie · 2026-08-19T06:42:06Z

Last push:

resolving conflicts (lib/vif.py)
rebasing

Marking ready for review as all components are published with 8.3-20260818

glehmann

Some minor comments: I clearly don't have the network knowledge to evaluate the tests.

glehmann · 2026-08-19T12:39:03Z

+# Special requirements for some tests:
+# - TestVLAN needs at least 1 free NICs (see HOST_FREE_NICS in data.py)
+# - TestMigrate needs second XCP-ng host in the same pool
+# - TestTunnel will create encrypted tunnel (and only one could be created at a time)


So it requires no encrypted tunnel already configured?

no. the test will setup a tunnel itself. it is created by the tunnel fixture.

glehmann · 2026-08-19T12:39:03Z

+# - TestMigrate needs second XCP-ng host in the same pool
+# - TestTunnel will create encrypted tunnel (and only one could be created at a time)
+
+CACHE_ovs_vsctl_bridge_to_parent: dict[str, str] = {}


Consider renaming this to cache_ovs_vsctl_bridge_to_parent (lowercase snake_case).
Since this dictionary represents mutable state rather than an immutable module-level constant, PEP 8 recommends standard lowercase naming.

Also, don't we risk leaking data between tests with a module-level cache?

from generic point of view, I agree that module-level cache is not optimal.
I changed it a bit to have a key linked to the host and to the current test. this way, it should still cache data (and avoid too much ssh to the host) and don't be shared between tests.

glehmann · 2026-08-19T12:39:04Z

+        finally:
+            # delete networkRule
+            logging.info("sdnController.deleteNetworkRule")
+            xo_cli('sdnController.deleteNetworkRule', {


Shouldn't it go in a defer()?

it can't go in defer() because it is also part of the test and things are tested after running it.
so if I put it in defer(), it means we can't test that the delete was properly done.

It can still go in a defer, but it needs to be guarded, to make sure it's not already deleted:

xo_cli('sdnController.addNetworkRule', { 'networkId': networkId, 'ipRange': '10.0.0.1', 'direction': 'to', 'protocol': 'icmp', 'allow': 'false', }) fn delete_rule(): xo_cli('sdnController.deleteNetworkRule', { 'networkId': networkId, 'ipRange': '10.0.0.1', 'direction': 'to', 'protocol': 'icmp', }) return True deleted = False defer(lambda: delete_rule() if not deleted else None) # do some stuff deleted = delete_rule() assert …

This is more convoluted than the try/finally, but in case of problem, it prevents the rule from being deleted before reaching the debugger.
Just pick the version you prefer :)

semarie · 2026-08-19T14:14:54Z

Last push:

integrate remarks from @glehmann

semarie · 2026-08-20T09:20:11Z

Last push:

rebased on master
add looseversion dependency, and replace the custom compare_versions function by comparing LooseVersion2 objects

semarie · 2026-08-20T09:25:40Z

Last push:

update requirements/base.txt after looseversion addition

stormi

I'm trying to review the PR, which deserves reaching a point where we can merge it and have it running in CI, but right now most commits are lacking an explanation about the rationale behind them (see https://docs.xcp-ng.org/project/development-process/commit-message-conventions/), and there are important implementation choices made without any explanation that I can find, such as delaying the moment we start checking for rules to apply. Fixed delays are something we avoid, so they must be justified explicitly, and if there's a way to check differently without a delay, this must be preferred.

semarie · 2026-08-20T16:08:50Z

Last push:

use rpm_version (instead of looseversion)

semarie · 2026-08-24T15:51:05Z

last push:

rebased on master
needs lib.xo.xo_cli: don't pass --json argument for 'list-objects' command #679 (for xo_cli fix)
remove change to introduce delay on wait_for and (try to) synchronize with delayed operations
- vm.start() and vm.shutdown() aren't seen immediately by XO : ask XO about the VM status before continuing
- sdnController operations aren't immediate: wait for the log file to be updated before continuing (still with hardcoded max timeout, as XO doesn't necessary contact all hosts in the pool)
update some commits messages

semarie · 2026-09-04T13:22:47Z

Last push:

rebase on master
be more explicit on commit message, about what I needed and what I did for abstractions

stormi · 2026-09-04T14:13:48Z

+            "A pool with at least 1 host (if more, with same network configuration).",
            "At least 2 free NICs on every host.",
            "A small VM that can be imported on the SRs.",


It is important to mention here that xo_cli is necessary. This PR changes a situation where only one job used to require XO, and now we have another job that requires XO (unless XO was already required by this job previously but not mentioned at the time in the requirements).

Let's add it here to the list of requirements, and I'll make sure it's clear to the team managing CI that this job has such as requirement.

I don't expect any issue, but they need to know.

I sent the message. Only remains the need to update the list of requirements.

I added XO mention to the jobs entry (in the last commit I pushed).

in order to have a way to parse rpm version, add rpm-version dependency. it will be used by traffic rules tests for checking xcp-ng-xapi-plugins package version. Signed-off-by: Sebastien Rodot <sebastien.rodot@vates.tech>

For several tests related to network (mostly traffic rules for now), we need more classes and functions to manipulate xapi objects. - create and manipulate Tunnel - create and manipulate VLAN - additional properties on Network, PIF, VIF - additional function on Host to get service status Signed-off-by: Sebastien Rodot <sebastien.rodot@vates.tech>

tests: - simple VIF rule (add/delete with simple vm.start/destroy cycle) - simple Network rule (add/delete with simple vm.start/destroy cycle) - migrate with simple VIF rule - migrate with Network rule rule - VLAN with simple VIF rule - VLAN with simple Network rule - Tunnel with simple VIF rule - Tunnel with simple Network rule on each tests: - a traffic rule to block some traffic is added and after that removed - VM lifecycle is tested with the rule applied - each step is verified (is the traffic blocked as expected ?) - the initial state is verified (to ensure no currently active traffic rule will pertubate the test) - the final state is verified (to ensure no rules are left) the traffic check is done using low-level debug cli tool from OVS. It permits to test the openflow rule application without sending any packet. a check is done for each port plugged on the virtual switch. the two first "simple" rules are the more complexes: - interleaving two rules addition/removal is tested - VM is started, stopped, restarted (rules are managed dynamically and on the fly by XO) fixtures: - add Tunnel fixture: returns a configured Tunnel network (Private Network in XO) - add VLAN fixture: returns a configured VLAN network Signed-off-by: Sebastien Rodot <sebastien.rodot@vates.tech>

semarie · 2026-09-04T14:42:35Z

Last push:

updated commit messages

stormi · 2026-09-04T15:09:20Z

+from typing import Callable
+
+# Requirements:
+# xo-cli (on the host running the test) is expected to be usable


As discussed together, we don't want to add this requirement to the main job, so we'll have to move all traffic rules tests, even the simple ones, to the network-advanced job.

We discussed a possible implementation for that: make xo_cli a fixture and create a marker named the same, automatically set when the fixture is used. Then use the marker to exclude or include tests in jobs.py.

I used the existing hosts_with_xo fixture for automatically add a hosts_with_xo mark and exclude it from 'main' jobs.

stormi · 2026-09-04T15:10:39Z

I voided @bleader's review and @glehmann's review so that they may re-review once we've removed the dependency to xo_cli from the main job.

semarie · 2026-09-04T16:14:54Z

Last push:

new commit
- automatically add hosts_with_xo mark on tests using hosts_with_xo fixture
- exclude tests wih hosts_with_xo mark to run in main jobs
- run tests with hosts_with_xo mark in tests/network in network-advanced jobs
- add a check in xo_cli() function to run only on tests using the hosts_with_xo fixture

semarie · 2026-09-04T16:20:01Z

Tip for reviewers: by running uv run ./jobs.py collect network-advanced and uv run ./jobs.py collect main, you could see the tests that will be run (and check that hosts_with_xo marker is properly propagated to tests)

@stormi

With traffic rules tests, we are adding more tests depending on XO (traffic rules are managed by XO). In order to avoid this strong dependency in 'main' jobs, automatically mark tests using 'hosts_with_xo' fixture with 'hosts_with_xo' mark. It permits to avoid running them in 'main' jobs, and run them in 'advanced-network' jobs instead of (for networks tests). Additionally, restrict the use of xo_cli() function to tests that are using 'hosts_with_xo' fixture. It is done by modifying a global in lib.xo module, and checking it at xo_cli() beginning. Based on idea from @stormi Signed-off-by: Sebastien Rodot <sebastien.rodot@vates.tech>

semarie · 2026-09-04T16:23:46Z

Last push:

fix style issues (line too long on markers used by 'main' tests, and import order)

semarie self-assigned this Feb 11, 2026

semarie force-pushed the srt/traffic-rules branch 11 times, most recently from 136ec08 to fbad09b Compare February 17, 2026 07:48

semarie force-pushed the srt/traffic-rules branch 13 times, most recently from 38e4a18 to b07bf94 Compare February 20, 2026 07:54

semarie force-pushed the srt/traffic-rules branch 2 times, most recently from e73de29 to f6f16eb Compare February 24, 2026 13:43

semarie force-pushed the srt/traffic-rules branch 2 times, most recently from a18123b to ec9468f Compare March 13, 2026 14:19

semarie force-pushed the srt/traffic-rules branch from 2d17182 to 15ff2e8 Compare July 2, 2026 16:46

semarie force-pushed the srt/traffic-rules branch from 15ff2e8 to ea14d95 Compare July 15, 2026 09:21

semarie force-pushed the srt/traffic-rules branch from ea14d95 to adbb6db Compare July 17, 2026 08:27

semarie force-pushed the srt/traffic-rules branch from adbb6db to dddd48f Compare August 5, 2026 16:50

glehmann reviewed Aug 19, 2026

View reviewed changes

glehmann approved these changes Aug 19, 2026

View reviewed changes

stormi requested changes Aug 20, 2026

View reviewed changes

bleader approved these changes Sep 3, 2026

View reviewed changes

stormi reviewed Sep 4, 2026

View reviewed changes

semarie added 3 commits September 4, 2026 16:24

add rpm-version dependency

0845a30

in order to have a way to parse rpm version, add rpm-version dependency. it will be used by traffic rules tests for checking xcp-ng-xapi-plugins package version. Signed-off-by: Sebastien Rodot <sebastien.rodot@vates.tech>

stormi reviewed Sep 4, 2026

View reviewed changes

Uh oh!

Conversation

semarie commented Feb 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

semarie commented Feb 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

semarie commented Jul 3, 2026

Uh oh!

semarie commented Jul 15, 2026

Uh oh!

semarie commented Jul 17, 2026

Uh oh!

semarie commented Aug 5, 2026

Uh oh!

semarie commented Aug 19, 2026

Uh oh!

glehmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glehmann Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

semarie commented Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

semarie commented Aug 20, 2026

Uh oh!

semarie commented Aug 20, 2026

Uh oh!

stormi left a comment

Choose a reason for hiding this comment

Uh oh!

semarie commented Aug 20, 2026

Uh oh!

semarie commented Aug 24, 2026

Uh oh!

semarie commented Sep 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

semarie commented Sep 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stormi commented Sep 4, 2026

Uh oh!

semarie commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

semarie commented Sep 4, 2026

Uh oh!

semarie commented Sep 4, 2026

semarie commented Feb 11, 2026 •

edited

Loading

semarie commented Feb 20, 2026 •

edited

Loading

glehmann Aug 19, 2026 •

edited

Loading

semarie commented Aug 19, 2026 •

edited

Loading

semarie commented Sep 4, 2026 •

edited

Loading