On Tue, Dec 13, 2022 at 2:55 PM Xavier Simonart <[email protected]> wrote:
> Hi Ales > > Thanks for the patch and the nice speed-up > > > On Mon, Dec 12, 2022 at 4:57 PM Ales Musil <[email protected]> wrote: > >> Instead of iterating over the logical flows in a loop >> just check the flows directly filtering the portion the >> tests is interested in. Reducing the duration ~17x. >> >> Reported-at: https://bugzilla.redhat.com/2149851 >> Signed-off-by: Ales Musil <[email protected]> >> --- >> tests/ovn-northd.at | 115 ++++++++++++++++++++++++++++++++++++-------- >> 1 file changed, 95 insertions(+), 20 deletions(-) >> >> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at >> index 71eda9ff4..554a9b63b 100644 >> --- a/tests/ovn-northd.at >> +++ b/tests/ovn-northd.at >> @@ -8193,20 +8193,6 @@ OVN_FOR_EACH_NORTHD([ >> AT_SETUP([Check default drop]) >> AT_KEYWORDS([drop]) >> >> -# Check that there is an explicit drop lflow in all tables. >> -check_default_lflow() { >> - dps=$(ovn-sbctl --bare --columns=_uuid list Datapath_Binding | >> xargs) >> - for dp in $dps; do >> - for pipeline in ingress egress; do >> - for table in $(ovn-sbctl --bare --columns=table_id find >> Logical_Flow logical_datapath="$dp" pipeline="$pipeline" | xargs | sort | >> uniq); do >> - echo "Checking if datapath $dp pipeline $pipeline table >> $table has a default action" >> - AT_CHECK([ovn-sbctl --columns=_uuid find Logical_Flow >> logical_datapath="$dp" pipeline="$pipeline" table_id=$table match="1" >> priority">="0 | wc -l | tr -d "\n\r" ], [0], [1], [ignore], >> - [echo "Datapath $dp pipeline $pipeline table $table does >> not have a default action"]) >> - done >> - done >> - done >> -} >> - >> ovn_start >> >> # Create LS + LR >> @@ -8221,13 +8207,84 @@ check ovn-nbctl --wait=sb \ >> -- lsp-add S1 p1 \ >> -- lsp-set-addresses p1 "02:ac:10:01:00:0a 172.16.1.100" >> >> -check_default_lflow >> +AT_CHECK([ovn-sbctl dump-flows | grep "match=(1)" | sed >> 's/table=../table=??/' | sort], [0], [dnl >> + table=??(lr_in_admission ), priority=0 , match=(1), >> action=(drop;) >> + table=??(lr_in_arp_request ), priority=0 , match=(1), >> action=(output;) >> + table=??(lr_in_arp_resolve ), priority=0 , match=(1), >> action=(drop;) >> + table=??(lr_in_chk_pkt_len ), priority=0 , match=(1), >> action=(next;) >> + table=??(lr_in_defrag ), priority=0 , match=(1), >> action=(next;) >> + table=??(lr_in_dnat ), priority=0 , match=(1), >> action=(next;) >> + table=??(lr_in_ecmp_stateful), priority=0 , match=(1), >> action=(next;) >> + table=??(lr_in_gw_redirect ), priority=0 , match=(1), >> action=(next;) >> + table=??(lr_in_ip_input ), priority=0 , match=(1), >> action=(next;) >> + table=??(lr_in_ip_routing ), priority=0 , match=(1), >> action=(drop;) >> + table=??(lr_in_ip_routing_ecmp), priority=0 , match=(1), >> action=(drop;) >> + table=??(lr_in_ip_routing_pre), priority=0 , match=(1), >> action=(reg7 = 0; next;) >> + table=??(lr_in_larger_pkts ), priority=0 , match=(1), >> action=(next;) >> + table=??(lr_in_lb_aff_check ), priority=0 , match=(1), >> action=(next;) >> + table=??(lr_in_lb_aff_learn ), priority=0 , match=(1), >> action=(next;) >> + table=??(lr_in_learn_neighbor), priority=0 , match=(1), >> action=(drop;) >> + table=??(lr_in_lookup_neighbor), priority=0 , match=(1), >> action=(reg9[[2]] = 1; next;) >> + table=??(lr_in_nd_ra_options), priority=0 , match=(1), >> action=(next;) >> + table=??(lr_in_nd_ra_response), priority=0 , match=(1), >> action=(next;) >> + table=??(lr_in_policy ), priority=0 , match=(1), >> action=(reg8[[0..15]] = 0; next;) >> + table=??(lr_in_policy_ecmp ), priority=0 , match=(1), >> action=(drop;) >> + table=??(lr_in_unsnat ), priority=0 , match=(1), >> action=(next;) >> + table=??(lr_out_chk_dnat_local), priority=0 , match=(1), >> action=(reg9[[4]] = 0; next;) >> + table=??(lr_out_delivery ), priority=0 , match=(1), >> action=(drop;) >> + table=??(lr_out_egr_loop ), priority=0 , match=(1), >> action=(next;) >> + table=??(lr_out_post_snat ), priority=0 , match=(1), >> action=(next;) >> + table=??(lr_out_post_undnat ), priority=0 , match=(1), >> action=(next;) >> + table=??(lr_out_snat ), priority=0 , match=(1), >> action=(next;) >> + table=??(lr_out_undnat ), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_in_acl ), priority=65535, match=(1), >> action=(next;) >> + table=??(ls_in_acl_after_lb ), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_in_acl_hint ), priority=65535, match=(1), >> action=(next;) >> + table=??(ls_in_apply_port_sec), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_in_arp_rsp ), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_in_check_port_sec), priority=50 , match=(1), >> action=(reg0[[15]] = check_in_port_sec(); next;) >> + table=??(ls_in_dhcp_options ), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_in_dhcp_response), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_in_dns_lookup ), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_in_dns_response ), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_in_external_port), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_in_hairpin ), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_in_l2_lkup ), priority=0 , match=(1), >> action=(outport = get_fdb(eth.dst); next;) >> + table=??(ls_in_l2_unknown ), priority=0 , match=(1), >> action=(output;) >> + table=??(ls_in_lb ), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_in_lb_aff_check ), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_in_lb_aff_learn ), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_in_lookup_fdb ), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_in_nat_hairpin ), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_in_pre_acl ), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_in_pre_hairpin ), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_in_pre_lb ), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_in_pre_stateful ), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_in_put_fdb ), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_in_qos_mark ), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_in_qos_meter ), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_in_stateful ), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_out_acl ), priority=65535, match=(1), >> action=(next;) >> + table=??(ls_out_acl_hint ), priority=65535, match=(1), >> action=(next;) >> + table=??(ls_out_apply_port_sec), priority=0 , match=(1), >> action=(output;) >> + table=??(ls_out_check_port_sec), priority=0 , match=(1), >> action=(reg0[[15]] = check_out_port_sec(); next;) >> + table=??(ls_out_pre_acl ), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_out_pre_lb ), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_out_pre_stateful), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_out_qos_mark ), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_out_qos_meter ), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_out_stateful ), priority=0 , match=(1), >> action=(next;) >> +]) >> >> Are we not changing the purpose of the test? > IIUC, before the patch, a commit adding a new table, but missing a default > action would have been considered as a regression. Now, it would be fine. > Also, a new commit adding a new table, with a proper default drop, would > now fail the test. > Can we not count the number of unique tables in each datapath/pipeline, > and compare this count with the number of flows in each dp/pipeline > containing match="1" in different tables? > So, we would avoid the loop on each table, hence still reducing > dramatically the number of calls to ovn-sbctl. More than probably slower > than your approach, but I would still expect a 10x speed-up > Hi Xavier, for the first problem we could compare a total number of tables with a number of tables with default action, WDYT? That should be way faster and could work even without looping. The second problem, a new table with proper action would fail this test is IMO ok (we do that in other tests too), but this verbose check could be replaced by just the before mentioned comparison of flow numbers. Thanks, Ales > > # Add stateless ACL >> check ovn-nbctl --wait=sb \ >> -- acl-add S1 from-lport 100 'inport=p1 && ip4' >> allow-stateless >> >> -check_default_lflow >> +AT_CHECK([ovn-sbctl dump-flows | grep "ls_in_acl" | grep "match=(1)" | >> sed 's/table=../table=??/' | sort], [0], [dnl >> + table=??(ls_in_acl ), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_in_acl_after_lb ), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_in_acl_hint ), priority=0 , match=(1), >> action=(next;) >> +]) >> >> check ovn-nbctl --wait=sb acl-del S1 >> >> @@ -8236,7 +8293,11 @@ check ovn-nbctl --wait=sb acl-del S1 >> check ovn-nbctl --wait=sb \ >> -- acl-add S1 from-lport 2 "udp" allow-related >> >> -check_default_lflow >> +AT_CHECK([ovn-sbctl dump-flows | grep "ls_in_acl" | grep "match=(1)" | >> sed 's/table=../table=??/' | sort], [0], [dnl >> + table=??(ls_in_acl ), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_in_acl_after_lb ), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_in_acl_hint ), priority=0 , match=(1), >> action=(next;) >> +]) >> >> check ovn-nbctl --wait=sb acl-del S1 >> >> @@ -8245,12 +8306,22 @@ check ovn-nbctl --wait=sb \ >> -- lb-add lb "10.0.0.1" "10.0.0.2" \ >> -- ls-lb-add S1 lb >> >> -check_default_lflow >> +AT_CHECK([ovn-sbctl dump-flows | grep "ls_in_acl" | grep "match=(1)" | >> sed 's/table=../table=??/' | sort], [0], [dnl >> + table=??(ls_in_acl ), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_in_acl_after_lb ), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_in_acl_hint ), priority=0 , match=(1), >> action=(next;) >> +]) >> + >> >> # Check LB + stateless ACL >> check ovn-nbctl --wait=sb \ >> -- acl-add S1 from-lport 100 'inport=p1 && ip4' >> allow-stateless >> -check_default_lflow >> + >> +AT_CHECK([ovn-sbctl dump-flows | grep "ls_in_acl" | grep "match=(1)" | >> sed 's/table=../table=??/' | sort], [0], [dnl >> + table=??(ls_in_acl ), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_in_acl_after_lb ), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_in_acl_hint ), priority=0 , match=(1), >> action=(next;) >> +]) >> >> check ovn-nbctl --wait=sb acl-del S1 >> >> @@ -8258,7 +8329,11 @@ check ovn-nbctl --wait=sb acl-del S1 >> check ovn-nbctl --wait=sb \ >> -- acl-add S1 from-lport 2 "udp" allow-related >> >> -check_default_lflow >> +AT_CHECK([ovn-sbctl dump-flows | grep "ls_in_acl" | grep "match=(1)" | >> sed 's/table=../table=??/' | sort], [0], [dnl >> + table=??(ls_in_acl ), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_in_acl_after_lb ), priority=0 , match=(1), >> action=(next;) >> + table=??(ls_in_acl_hint ), priority=0 , match=(1), >> action=(next;) >> +]) >> >> AT_CLEANUP >> ]) >> -- >> 2.38.1 >> >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> Thanks > Xavier > -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] IM: amusil <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
