I've reviewed the series. Most of the patches are good to go, but 1) HAVE_SCAPY macro should be applied and 2) see replies to some of the patches with suggestions. Let me know if this was helpful.
Ihar On Tue, Oct 10, 2023 at 8:37 PM Ihar Hrachyshka <ihrac...@redhat.com> wrote: > Thanks a lot for the series! I will review it later in more detail, but > for now - a general comment for all (?) of them: right now we have to tag > all scapy tests with the `AT_SKIP_IF([test $HAVE_SCAPY = no])` macro. > > (Note: I think this macro should go away and we should just assume scapy > is present / require it for the test suite. But that's a different > discussion.) > > Re: performance. We can definitely discuss other ideas on how to shave off > more seconds of the total runtime with fmt_pkt. (E.g. running a single > daemon for the whole test suite - saving on daemon initialization for each > new test case using it; or caching / memoizing layer to remember previous > payloads generated; some cases that don't require L7 fields may be better > served by 'ofproto/hexify' [1]; maybe something else.) > > [1] https://patchwork.ozlabs.org/project/openvswitch/list/?series=377011 > > Thanks again, great to see this adopted. > Ihar > > On Tue, Oct 10, 2023 at 4:34 PM Mark Michelson <mmich...@redhat.com> > wrote: > >> The introduction of fmt_pkt has made construction of new tests easier >> since we no longer need to input packet data manually. fmt_pkt can also >> be applied to existing tests to improve their readability and >> debuggability. This set of patches converts some old tests to use >> fmt_pkt. In the author's opinion, all of these tests are now easier to >> read and understand. >> >> The following tests are converted in this series: >> >> * ovn -- allows ACLs to match against vlan-transparent double tagged >> traffic L3 fields >> * 3 HVs, 1 LS, 3 lports/HV >> * VLAN transparency, passthru=true, ARP responder disabled >> * VLAN transparency, passthru=true, ND/NA responder disabled >> * vtep: 3 HVs, 1 VIFs/HV, 1 GW, 1 LS >> * 3 HVs, 3 LS, 3 lports/LS >> * portsecurity : 3 HVs, 1 LS, 3 lports/HV >> * 1 HV, 1 LS, 2 lport/LS, 1 LR >> * 1 HV, 2 LSs, 1 lport/LS, 1 LR >> * 2 HVs, 3 LS, 1 lport/LS, 2 peer LRs, static routes >> * 2 HVs, 3 LRs connected via LS, static routes >> * 2 HVs, 2 LRs connected via LS, gateway router >> * icmp_reply: 1 HVs, 2 LSs, 1 lport/LS, 1 LR >> >> These tests were chosen by opening tests/ovn.at, and then searching for >> hand-constructed packets from the top of the file. These were the first >> 13 tests that I encountered that >> >> * Were not constructing trivial L2 packets. >> * Did not make my brain hurt (I'm looking at you, DHCPv4 test). >> >> Many more tests are potential candidates. These were what I could get >> done in the period of time I had to work on them. >> >> There is a drawback to using fmt_pkt: the execution times of all but one >> test have become longer. For some reason "vtep: 3 HVs, 1 VIFs/HV, 1 GW, >> 1 LS" runs faster when converted to use fmt_pkt. >> >> If all of these tests are run together, then we see the following total >> execution time: >> >> with fmt_pkt: 1m33.325s >> on main branch: 51.591s >> >> Two tests in particular, "3 HVs, 3 LS, 3 lports/LS" and "portsecurity : >> 3 HVs, 1 LS, 3 lports/HV" are much slower with fmt_pkt when compared to >> main. If we eliminate these two tests and run the other 11 together, >> then the execution time is: >> >> with fmt_pkt: 39.124s >> on main branch: 34.496s >> >> Mark Michelson (13): >> tests: Use fmt_pkt in ovn -- allows ACLs to match ... >> tests: Use fmt_pkt in 3 HVs, 1 LS, 3 lports/HV. >> tests: Use fmt_pkt in VLAN transparency, ... >> tests: Use fmt_pkt in VLAN transparency, ... >> tests: Use fmt_pkt in vtep: 3 HVs, 1 VIFs/HV, 1 GW, 1 LS. >> tests: Use fmt_pkt in 3 HVs, 3 LS, 3 lports/LS. >> tests: Use fmt_pkt in portsecurity : 3 HVs, 1 LS, 3 lports/HV. >> tests: Use fmt_pkt in 1 HV, 1 LS, 2 lport/LS, 1 LR. >> tests: Use fmt_pkt in 1 HV, 2 LSs, 1 lport/LS, 1 LR. >> tests: Use fmt_pkt in 2 HVs, 3 LS, 1 lport/LS, ... >> tests: Use fmt_pkt in 2 HVs, 3 LRs connected via LS, static routes. >> tests: Use fmt_pkt in 2 HVs, 2 LRs connected via LS, gateway router. >> tests: Use fmt_pkt in icmp_reply: 1 HVs, 2 LSs, 1 lport/LS, 1 LR. >> >> tests/ovn.at | 699 ++++++++++++++++++++++-------------------- >> tests/scapy-server.py | 1 + >> 2 files changed, 374 insertions(+), 326 deletions(-) >> >> -- >> 2.40.1 >> >> _______________________________________________ >> dev mailing list >> d...@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
