Hi Xavier, The explanation for this change makes sense. The downside to this patch is that it is now checking ACL log fields that we previously did not care about. The reason for the python script was to eliminate data that could be system-dependent or that we did not explicitly specify when sending our packets.
In this case, the test is now checking: * severity * direction * nw_tos * nw_ecn * nw_ttl * nw_frag The "severity" and "direction" are fine to check since we specify this information in the ACL definition. The other fields are likely also not a problem. However, there is a small chance we could see test failures if system defaults change, if a user has altered their system's defaults, or if defaults are not the same across all distros or OSes (e.g. Windows uses a default IP TTL of 128 instead of 64, and older Linux kernels use a default TTL of 32 instead of 64). To fix this, I think the test_ping() function should be updated so that we explicitly specify these fields. The current ping is specified as: ping -q -c 1 -i 0.3 -w 2 10.0.0.2 However, I think we should change it to the following: ping -q -c 1 -i 0.3 -w 2 -t 64 -Q 0 -M probe 10.0.0.2 This way, the nw_ttl, nw_ecn, nw_tos, and nw_frag values are made explicit by the ping command, and we should be able to guarantee a match in our ACL log. ---------- That was the only issue I saw with this patch that might cause tests to still fail. However, I don't like the fact that this patch makes it so that the test is sometimes using the check_acl_log.py script, and sometimes is doing an in-place check of the log using grep. I would suggest one of the following: 1. Keep the changes you've started with, and make the change to test_ping() I suggested above. Change the other calls to check_acl_log.py to just use grep | sed, and delete the check_acl_log.py script from the testsuite. 2. Revert the changes you've made so far and ignore my suggestion about changing test_ping(). Instead, change the check_acl_log.py to sort the entries it parses so that the --entry-num is guaranteed to line up properly, and the current test will work as it is written. It's up to you about whether you do (1) or (2), but this way it makes the test's checks more uniform. On Mon, Jan 19, 2026 at 4:57 AM Xavier Simonart via dev <[email protected]> wrote: > > Test was sometimes failing as logging two ACLs (one from request > traffic and the other one from reply traffic) misordered. > PACKET_IN from OVS might be misordered in this case as being generated > by two different OVS threads. > > Signed-off-by: Xavier Simonart <[email protected]> > --- > tests/system-ovn.at | 120 +++++++------------------------------------- > 1 file changed, 18 insertions(+), 102 deletions(-) > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > index fc601dd1b..d24fe05a5 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -8225,31 +8225,12 @@ check ovn-nbctl --wait=hv sync > test_ping > > # The allow ACL should match on the request and reply traffic, resulting in > 2 logs. > -check_acl_log_count 2 > - > -check $PYTHON $srcdir/check_acl_log.py \ > - --entry-num=1 \ > - --name=allow_acl \ > - --verdict=allow \ > - --protocol=icmp \ > - --dl_src=00:00:00:00:00:01 \ > - --dl_dst=00:00:00:00:00:02 \ > - --nw_src=10.0.0.1 \ > - --nw_dst=10.0.0.2 \ > - --icmp_type=8 \ > - --icmp_code=0 > - > -check $PYTHON $srcdir/check_acl_log.py \ > - --entry-num=2 \ > - --name=allow_acl \ > - --verdict=allow \ > - --protocol=icmp \ > - --dl_src=00:00:00:00:00:02 \ > - --dl_dst=00:00:00:00:00:01 \ > - --nw_src=10.0.0.2 \ > - --nw_dst=10.0.0.1 \ > - --icmp_type=0 \ > - --icmp_code=0 > +# Request and reply traffic upcall might be handled by two different threads > in OVS. > +# Hence order is not guaranteed in log. > +AT_CHECK([grep -r acl_log ovn-controller.log | sed 's/.*name=/name=/' | > sort] , [0], [dnl > +name="allow_acl", verdict=allow, severity=info, direction=from-lport: > icmp,vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=10.0.0.1,nw_dst=10.0.0.2,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,icmp_type=8,icmp_code=0 > +name="allow_acl", verdict=allow, severity=info, direction=from-lport: > icmp,vlan_tci=0x0000,dl_src=00:00:00:00:00:02,dl_dst=00:00:00:00:00:01,nw_src=10.0.0.2,nw_dst=10.0.0.1,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,icmp_type=0,icmp_code=0 > +]) > > # Now add a higher-priority stateful ACL that matches on the same > # parameters. Don't enable reply logging. > @@ -8328,32 +8309,10 @@ clear_log > test_ping > > # Now we should have the request and reply logged. > -check_acl_log_count 2 > - > -check $PYTHON $srcdir/check_acl_log.py \ > - --entry-num=1 \ > - --name=allow_related_acl \ > - --verdict=allow \ > - --protocol=icmp \ > - --dl_src=00:00:00:00:00:01 \ > - --dl_dst=00:00:00:00:00:02 \ > - --nw_src=10.0.0.1 \ > - --nw_dst=10.0.0.2 \ > - --icmp_type=8 \ > - --icmp_code=0 > - > -check $PYTHON $srcdir/check_acl_log.py \ > - --entry-num=2 \ > - --name=allow_related_acl \ > - --verdict=allow \ > - --protocol=icmp \ > - --dl_src=00:00:00:00:00:02 \ > - --dl_dst=00:00:00:00:00:01 \ > - --nw_src=10.0.0.2 \ > - --nw_dst=10.0.0.1 \ > - --icmp_type=0 \ > - --icmp_code=0 > - > +AT_CHECK([grep -r acl_log ovn-controller.log | sed 's/.*name=/name=/' | > sort] , [0], [dnl > +name="allow_related_acl", verdict=allow, severity=info, > direction=from-lport: > icmp,vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=10.0.0.1,nw_dst=10.0.0.2,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,icmp_type=8,icmp_code=0 > +name="allow_related_acl", verdict=allow, severity=info, direction=to-lport: > icmp,vlan_tci=0x0000,dl_src=00:00:00:00:00:02,dl_dst=00:00:00:00:00:01,nw_src=10.0.0.2,nw_dst=10.0.0.1,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,icmp_type=0,icmp_code=0 > +]) > > # And now, let's start from scratch but make sure everything works when > # using egress ACLs. > @@ -8368,31 +8327,10 @@ clear_log > test_ping > > # The allow ACL should match on the request and reply traffic, resulting in > 2 logs. > -check_acl_log_count 2 > - > -check $PYTHON $srcdir/check_acl_log.py \ > - --entry-num=1 \ > - --name=allow_acl \ > - --verdict=allow \ > - --protocol=icmp \ > - --dl_src=00:00:00:00:00:01 \ > - --dl_dst=00:00:00:00:00:02 \ > - --nw_src=10.0.0.1 \ > - --nw_dst=10.0.0.2 \ > - --icmp_type=8 \ > - --icmp_code=0 > - > -check $PYTHON $srcdir/check_acl_log.py \ > - --entry-num=2 \ > - --name=allow_acl \ > - --verdict=allow \ > - --protocol=icmp \ > - --dl_src=00:00:00:00:00:02 \ > - --dl_dst=00:00:00:00:00:01 \ > - --nw_src=10.0.0.2 \ > - --nw_dst=10.0.0.1 \ > - --icmp_type=0 \ > - --icmp_code=0 > +AT_CHECK([grep -r acl_log ovn-controller.log | sed 's/.*name=/name=/' | > sort] , [0], [dnl > +name="allow_acl", verdict=allow, severity=info, direction=to-lport: > icmp,vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=10.0.0.1,nw_dst=10.0.0.2,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,icmp_type=8,icmp_code=0 > +name="allow_acl", verdict=allow, severity=info, direction=to-lport: > icmp,vlan_tci=0x0000,dl_src=00:00:00:00:00:02,dl_dst=00:00:00:00:00:01,nw_src=10.0.0.2,nw_dst=10.0.0.1,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,icmp_type=0,icmp_code=0 > +]) > > # Now add a higher-priority stateful ACL that matches on the same > # parameters. Don't enable reply logging. > @@ -8471,32 +8409,10 @@ clear_log > test_ping > > # Now we should have the request and reply logged. > -check_acl_log_count 2 > - > -check $PYTHON $srcdir/check_acl_log.py \ > - --entry-num=1 \ > - --name=allow_related_acl \ > - --verdict=allow \ > - --protocol=icmp \ > - --dl_src=00:00:00:00:00:01 \ > - --dl_dst=00:00:00:00:00:02 \ > - --nw_src=10.0.0.1 \ > - --nw_dst=10.0.0.2 \ > - --icmp_type=8 \ > - --icmp_code=0 > - > -check $PYTHON $srcdir/check_acl_log.py \ > - --entry-num=2 \ > - --name=allow_related_acl \ > - --verdict=allow \ > - --protocol=icmp \ > - --dl_src=00:00:00:00:00:02 \ > - --dl_dst=00:00:00:00:00:01 \ > - --nw_src=10.0.0.2 \ > - --nw_dst=10.0.0.1 \ > - --icmp_type=0 \ > - --icmp_code=0 > - > +AT_CHECK([grep -r acl_log ovn-controller.log | sed 's/.*name=/name=/' | > sort] , [0], [dnl > +name="allow_related_acl", verdict=allow, severity=info, > direction=from-lport: > icmp,vlan_tci=0x0000,dl_src=00:00:00:00:00:02,dl_dst=00:00:00:00:00:01,nw_src=10.0.0.2,nw_dst=10.0.0.1,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,icmp_type=0,icmp_code=0 > +name="allow_related_acl", verdict=allow, severity=info, direction=to-lport: > icmp,vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=10.0.0.1,nw_dst=10.0.0.2,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,icmp_type=8,icmp_code=0 > +]) > > OVN_CLEANUP_CONTROLLER([hv1]) > > -- > 2.47.1 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
