Hi Mark, Thanks for reviewing this patch and providing those options.
On Mon, Jan 19, 2026 at 7:36 PM Mark Michelson <[email protected]> wrote: > 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 was also hesitating between making the tests always follow the same approach versus more, not strictly needed, changes. > . 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. > I did not think about it, and it looks to me the best one, as it does not require changing all tests but still has them consistent0. While at it, I added a --direction as well in the tests and the py script, so we can check the from-lport and to-lport. It also makes the sort more obvious. > > It's up to you about whether you do (1) or (2), but this way it makes > the test's checks more uniform. > I'll send v2. Thanks again Xavier > > 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
