On Mon, May 10, 2021 at 11:43 AM Dumitru Ceara <dce...@redhat.com> wrote: > > On 5/10/21 5:39 PM, Dumitru Ceara wrote: > > On 5/7/21 4:42 PM, num...@ovn.org wrote: > >> From: Numan Siddique <num...@ovn.org> > >> > >> Some smart NICs can't offload datapath flows matching on conntrack > >> fields. If a deployment desires to make use of such smart NICs > >> then it cannot configure ACLs on the logical switches. If suppose > >> a logical switch S1 has no ACLs configured and a logical switch S2 > >> has ACLs configured, then the CMS would expect the datapath flows > >> belonging to S1 logical ports are offloaded since it has no ACLs. > >> But this is not working as expected (even if S1 and S2 are > >> not connected via a logical router). > >> > >> ovn-northd generates the below logical flows in ls_in_acl_hint > >> and ls_in_acl stages for S1 > >> > >> table=8 (ls_in_acl_hint ), priority=0 , match=(1), action=(next;) > >> table=9 (ls_in_acl ), priority=0 , match=(1), action=(next;) > >> > >> And the below for S2 > >> > >> table=8 (ls_in_acl_hint ), priority=7 , match=(ct.new && !ct.est), > >> action=(reg0[7] = 1; reg0[9] = 1; next;) > >> table=8 (ls_in_acl_hint ), priority=6 , match=(!ct.new && ct.est && > >> !ct.rpl && ct_label.blocked == 1), action=(reg0[7] = 1; reg0[9] = 1; next;) > >> table=8 (ls_in_acl_hint ), priority=5 , match=(!ct.trk), > >> action=(reg0[8] = 1; reg0[9] = 1; next;) > >> table=8 (ls_in_acl_hint ), priority=4 , match=(!ct.new && ct.est && > >> !ct.rpl && ct_label.blocked == 0), action=(reg0[8] = 1; reg0[10] = 1; > >> next;) > >> table=8 (ls_in_acl_hint ), priority=3 , match=(!ct.est), > >> action=(reg0[9] = 1; next;) > >> table=8 (ls_in_acl_hint ), priority=2 , match=(ct.est && > >> ct_label.blocked == 1), action=(reg0[9] = 1; next;) > >> table=8 (ls_in_acl_hint ), priority=1 , match=(ct.est && > >> ct_label.blocked == 0), action=(reg0[10] = 1; next;) > >> table=8 (ls_in_acl_hint ), priority=0 , match=(1), action=(next;) > >> table=9 (ls_in_acl ), priority=65535, match=(!ct.est && ct.rel && > >> !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;) > >> table=9 (ls_in_acl ), priority=65535, match=(ct.est && !ct.rel && > >> !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;) > >> table=9 (ls_in_acl ), priority=65535, match=(ct.inv || (ct.est && > >> ct.rpl && ct_label.blocked == 1)), action=(drop;) > >> table=9 (ls_in_acl ), priority=65535, match=(nd || nd_ra || nd_rs > >> || mldv1 || mldv2), action=(next;) > >> table=9 (ls_in_acl ), priority=34000, match=(eth.dst == > >> $svc_monitor_mac), action=(next;) > >> table=9 (ls_in_acl ), priority=1 , match=(ip && (!ct.est || > >> (ct.est && ct_label.blocked == 1))), action=(reg0[1] = 1; next;) > >> table=9 (ls_in_acl ), priority=0 , match=(1), action=(next;) > >> > >> Because there are higher priority flows in 'ls_in_acl_hint' and > >> 'ls_in_acl' with the match on conntrack fields, > >> ovs-vswitchd will generate a datapath flow with the match on ct_state > >> fields as - > >> 'ct_state(-new-est-rel-rpl-inv-trk)' for the packet from S1, even though > >> the S1 pipeline doesn't have logical flows which match on conntrack > >> fields. [1] has more information. > >> > >> Modifying the below flows if a logical switch has no ACLs solves this > >> problem. > >> > >> table=8 (ls_in_acl_hint ), priority=65535 , match=(1), > >> action=(next;) > >> table=9 (ls_in_acl ), priority=65535 , match=(1), > >> action=(next;) > >> > >> With the above flows with higher priority, ovs-vswitchd will not > >> consider other flows in the same table during translation. > >> > >> This patch addresses this issue by using higher prioriy flows (for both > >> ls_in_acl* and ls_out_acl* stages). > >> > >> [1] - https://bugzilla.redhat.com/show_bug.cgi?id=1955191#c8 > >> > >> Signed-off-by: Numan Siddique <num...@ovn.org> > >> --- > > > > Hi Numan, > > > > A couple of tests are failing after rebase: > > > > 789: ovn -- ct.inv usage -- ovn-northd -- dp-groups=yes FAILED > > (ovn-northd.at:3147) > > 790: ovn -- ct.inv usage -- ovn-northd FAILED > > (ovn-northd.at:3147) > > > >> v1 -> v2 > >> ---- > >> * Rebased to resolve conflicts. > >> * Addressed review comment from Dumitru. Combined ls_has_stateful_acl() > >> and ls_has_acl() into one single function - od_ls_update_acls_flags(). > > > > Nit: There's no other function in ovn_northd that's prefixed with > > od_ls_.*(). Maybe it makes sense to rename this to ls_get_acl_flags() > > to be inline with ls_has_lb_vip()? > > > > I assume the test failure is just due to the rebase and can be easily > fixed. The function rename can be done at apply time then: > > Acked-by: Dumitru Ceara <dce...@redhat.com>
Thanks Dumitru. I applied this patch to the main branch with the below changes. ***** diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index b137cba6b6..97cdfa7d69 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -4760,7 +4760,7 @@ ovn_ls_port_group_destroy(struct hmap *nb_pgs) } static void -od_ls_update_acls_flags(struct ovn_datapath *od) +ls_get_acl_flags(struct ovn_datapath *od) { od->has_acls = false; od->has_stateful_acl = false; @@ -6818,7 +6818,7 @@ build_lswitch_lflows_pre_acl_and_acl(struct ovn_datapath *od, { if (od->nbs) { od->has_lb_vip = ls_has_lb_vip(od); - od_ls_update_acls_flags(od); + ls_get_acl_flags(od); build_pre_acls(od, lflows); build_pre_lb(od, lflows, meter_groups, lbs); diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index bedd556d53..ed3c6106fb 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -3156,18 +3156,18 @@ check ovn-nbctl --wait=sb acl-add sw0 to-lport 1002 ip allow-related ovn-sbctl dump-flows sw0 > sw0flows AT_CAPTURE_FILE([sw0flows]) -AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 65535 | sort], [0], [dnl - table=9 (ls_in_acl ), priority=65535, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;) - table=9 (ls_in_acl ), priority=65535, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;) - table=9 (ls_in_acl ), priority=65535, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;) - table=9 (ls_in_acl ), priority=65535, match=(nd || nd_ra || nd_rs || mldv1 || mldv2), action=(next;) +AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 6553 | sort], [0], [dnl + table=9 (ls_in_acl ), priority=65532, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;) + table=9 (ls_in_acl ), priority=65532, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;) + table=9 (ls_in_acl ), priority=65532, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;) + table=9 (ls_in_acl ), priority=65532, match=(nd || nd_ra || nd_rs || mldv1 || mldv2), action=(next;) ]) -AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 65535 | sort], [0], [dnl - table=4 (ls_out_acl ), priority=65535, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;) - table=4 (ls_out_acl ), priority=65535, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;) - table=4 (ls_out_acl ), priority=65535, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;) - table=4 (ls_out_acl ), priority=65535, match=(nd || nd_ra || nd_rs || mldv1 || mldv2), action=(next;) +AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 6553 | sort], [0], [dnl + table=4 (ls_out_acl ), priority=65532, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;) + table=4 (ls_out_acl ), priority=65532, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;) + table=4 (ls_out_acl ), priority=65532, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;) + table=4 (ls_out_acl ), priority=65532, match=(nd || nd_ra || nd_rs || mldv1 || mldv2), action=(next;) ]) # Disable ct.inv usage. @@ -3176,18 +3176,18 @@ check ovn-nbctl --wait=sb set NB_Global . options:use_ct_inv_match=false ovn-sbctl dump-flows sw0 > sw0flows AT_CAPTURE_FILE([sw0flows]) -AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 65535 | sort], [0], [dnl - table=9 (ls_in_acl ), priority=65535, match=(!ct.est && ct.rel && !ct.new && ct_label.blocked == 0), action=(next;) - table=9 (ls_in_acl ), priority=65535, match=((ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;) - table=9 (ls_in_acl ), priority=65535, match=(ct.est && !ct.rel && !ct.new && ct.rpl && ct_label.blocked == 0), action=(next;) - table=9 (ls_in_acl ), priority=65535, match=(nd || nd_ra || nd_rs || mldv1 || mldv2), action=(next;) +AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 6553 | sort], [0], [dnl + table=9 (ls_in_acl ), priority=65532, match=(!ct.est && ct.rel && !ct.new && ct_label.blocked == 0), action=(next;) + table=9 (ls_in_acl ), priority=65532, match=((ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;) + table=9 (ls_in_acl ), priority=65532, match=(ct.est && !ct.rel && !ct.new && ct.rpl && ct_label.blocked == 0), action=(next;) + table=9 (ls_in_acl ), priority=65532, match=(nd || nd_ra || nd_rs || mldv1 || mldv2), action=(next;) ]) -AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 65535 | sort], [0], [dnl - table=4 (ls_out_acl ), priority=65535, match=(!ct.est && ct.rel && !ct.new && ct_label.blocked == 0), action=(next;) - table=4 (ls_out_acl ), priority=65535, match=((ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;) - table=4 (ls_out_acl ), priority=65535, match=(ct.est && !ct.rel && !ct.new && ct.rpl && ct_label.blocked == 0), action=(next;) - table=4 (ls_out_acl ), priority=65535, match=(nd || nd_ra || nd_rs || mldv1 || mldv2), action=(next;) +AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 6553 | sort], [0], [dnl + table=4 (ls_out_acl ), priority=65532, match=(!ct.est && ct.rel && !ct.new && ct_label.blocked == 0), action=(next;) + table=4 (ls_out_acl ), priority=65532, match=((ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;) + table=4 (ls_out_acl ), priority=65532, match=(ct.est && !ct.rel && !ct.new && ct.rpl && ct_label.blocked == 0), action=(next;) + table=4 (ls_out_acl ), priority=65532, match=(nd || nd_ra || nd_rs || mldv1 || mldv2), action=(next;) ]) AT_CHECK([grep -c "ct.inv" sw0flows], [1], [dnl @@ -3200,18 +3200,18 @@ check ovn-nbctl --wait=sb set NB_Global . options:use_ct_inv_match=true ovn-sbctl dump-flows sw0 > sw0flows AT_CAPTURE_FILE([sw0flows]) -AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 65535 | sort], [0], [dnl - table=9 (ls_in_acl ), priority=65535, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;) - table=9 (ls_in_acl ), priority=65535, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;) - table=9 (ls_in_acl ), priority=65535, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;) - table=9 (ls_in_acl ), priority=65535, match=(nd || nd_ra || nd_rs || mldv1 || mldv2), action=(next;) +AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 6553 | sort], [0], [dnl + table=9 (ls_in_acl ), priority=65532, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;) + table=9 (ls_in_acl ), priority=65532, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;) + table=9 (ls_in_acl ), priority=65532, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;) + table=9 (ls_in_acl ), priority=65532, match=(nd || nd_ra || nd_rs || mldv1 || mldv2), action=(next;) ]) -AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 65535 | sort], [0], [dnl - table=4 (ls_out_acl ), priority=65535, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;) - table=4 (ls_out_acl ), priority=65535, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;) - table=4 (ls_out_acl ), priority=65535, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;) - table=4 (ls_out_acl ), priority=65535, match=(nd || nd_ra || nd_rs || mldv1 || mldv2), action=(next;) +AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 6553 | sort], [0], [dnl + table=4 (ls_out_acl ), priority=65532, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;) + table=4 (ls_out_acl ), priority=65532, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;) + table=4 (ls_out_acl ), priority=65532, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;) + table=4 (ls_out_acl ), priority=65532, match=(nd || nd_ra || nd_rs || mldv1 || mldv2), action=(next;) ]) AT_CHECK([grep -c "ct.inv" sw0flows], [0], [dnl ***** Numan > > Regards, > Dumitru > > _______________________________________________ > 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