Hi Dumitru, First of all, many thanks for your time spent on this patch!
While Alexandra is trying to survive in a battle with the thunderbird, let me copy-paste her answers, which accidentally jumped into another thread, to save the conversation thread with proper indentation. Also, I see the patch is currently marked as "Changes Requested" in patchwork, but I don't see any particular code changes, which should be addressed. If you changed the status, could you point, please, what should be addressed? This change is very important for us to be merged 'cause it fixes the totally-broken setup for us on OVN >= 22.09.2, which at the same time quite difficult to support as our own local patch. We'd be very thankful all you guys (Dumitru, Venu, Han, ovn community) if we can make it accepted as soon as we resolve all your questions and concerns. Please, see below. On 22.04.2025 14:11, Dumitru Ceara via dev wrote: > On 4/18/25 2:42 PM, Rukomoinikova Aleksandra wrote: >>>> + /* For cases when we have statefull ACLs but no load >>>> + balancer configured on logical switch - we should >>>> + completely bypass conntrack on egress, otherwise >>>> + it is necessary to check the balanced traffic. */ >>> So what's the point of stateless to-lport ACLs (egress) if you still >>> commit to conntrack in the egress pipeline whenever there's a load >>> balancer configured on the switch/ >> I find this valid, if we want load balancers to work, then we realize >> that in egress traffic will get into the conntrack, but it will not be >> committed in it, because: >> table=6 (ls_out_acl_eval ), priority= , match=((stateless acl >> match)), action=(reg8[16] = 1; next;) >> >> conntrack lookup time is constant, it seems to me that such behavior is >> valid, what do you think? Only connections related to the work of lb >> will be committed >> I will correct 3 of your comments in version 4 if you generally support >> this idea. > Hi Alexandra, > > Side note: I think there's something weird going on with your email > client - at least with the html version of this email - the reply above > has white font color which on my white background is "hidden" :) > > To your comment: > > Conntrack lookup is "constant" but quite costly. It actually translates > to a recirculation in the datapath. After conntrack state has been > populated the packet is reinjected in the OVS datapath with that > information attached. That means packet headers need to be reparsed and > a new datapath flow lookup must happen. Alexandra's answer: Regarding the fact that conntrack lookup is an expensive operation due to recirculation. That's true, but this is the price we pay for using balancers under the condition of stateless acl. A lookup by conntrack in egress will not occur if we do not have balancers, it will only occur if we have balancers under the condition of stateless acl. This is a decent payment for the work of balancers, as it seems to us. Also, in this mode we return to the behavior as it was before the original commit, which changed the pipeline order [0]. Before this commit all packets were sent to conntrack that got through according to stateless rules. > > Another thing that's important is that without conntrack commit those > packets are forwarded by a datapath flow that matches on ct_state=+inv > which is not offloadable on certain HW (e.g., as far as I know, NVidia > NICs). Alexandra's answer: If I understand everything correctly, then hardware offloading on NICs is not supported during matching on the ct_state field +inv or -inv, but I remove the match on the invalid flow from the pipelines, in this case. Please see below (this is from my tests in ovn-northd). Before we add lb (have only stateful + stateless acl): AT_CHECK( [grep -E 'ls_out_acl_eval' sw0flows | grep 65532 | ovn_strip_lflows], [0], [dnl table=??(ls_out_acl_eval ), priority=65532, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_mark.blocked == 0), action=(reg8[[16]] = 1; ct_commit_nat;) table=??(ls_out_acl_eval ), priority=65532, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_mark.blocked == 0), action=(reg8[[16]] = 1; next;) table=??(ls_out_acl_eval ), priority=65532, match=(ct.est && ct_mark.allow_established == 1), action=(reg8[[16]] = 1; next;) table=??(ls_out_acl_eval ), priority=65532, match=(ct.inv || (ct.est && ct.rpl && ct_mark.blocked == 1)), action=(reg8[[17]] = 1; next;) table=??(ls_out_acl_eval ), priority=65532, match=(nd || nd_ra || nd_rs || mldv1 || mldv2), action=(reg8[[16]] = 1; next;) ]) In the stateless_inv_match() we just check the need to add this flow. After adding load balancers: # We do not match conntrack invalid packets in case of load balancers with stateless ACLs. AT_CHECK( [grep -E 'ls_out_acl_eval' sw0flows | grep 65532 | ovn_strip_lflows], [0], [dnl table=??(ls_out_acl_eval ), priority=65532, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_mark.blocked == 0), action=(reg8[[16]] = 1; ct_commit_nat;) table=??(ls_out_acl_eval ), priority=65532, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_mark.blocked == 0), action=(reg8[[16]] = 1; next;) table=??(ls_out_acl_eval ), priority=65532, match=(ct.est && ct.rpl && ct_mark.blocked == 1), action=(reg8[[17]] = 1; next;) table=??(ls_out_acl_eval ), priority=65532, match=(ct.est && ct_mark.allow_established == 1), action=(reg8[[16]] = 1; next;) table=??(ls_out_acl_eval ), priority=65532, match=(nd || nd_ra || nd_rs || mldv1 || mldv2), action=(reg8[[16]] = 1; next;) ]) That is, it seems to me that this should not break offloading. > >> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml >> index 1a70ba579..600eea824 100644 >> --- a/northd/ovn-northd.8.xml >> +++ b/northd/ovn-northd.8.xml >> @@ -507,7 +507,7 @@ >> <ref table="Logical_Switch_Port" db="OVN_Northbound"/>. Multicast, >> IPv6 >> Neighbor Discovery and MLD traffic also skips stateful ACLs. For >> "allow-stateless" ACLs, a flow is added to bypass setting the hint >> for >> - connection tracker processing when there are stateful ACLs or LB >> rules; >> + connection tracker processing when there are stateful ACLs without LB; >> <code>REGBIT_ACL_STATELESS</code> is set for traffic matching >> stateless >> ACL flows. >> </p> >> @@ -624,6 +624,14 @@ >> <code>ct_lb_mark;</code> action. >> </li> >> >> + <li> >> + A priority-115 flow sends all packet directed to VIP to connection >> + tracker. Packets that match this rule would still be subject to >> + connection tracking via lower-priority rules in the absence of >> + stateless ACLs. However, with stateless ACLs in place, this rule >> + enables load balancing when the client balances traffic to itself. >> + </li> >> + > One more thing I still don't completely understand is the actual use > case: do we also want to cover general load balancing in the presence of > stateless ACLs? The doc update above talks about "load balancing when > the client balances traffic to itself". Isn't your patch enabling load > balancing in general even when stateless ACLs are used? Alexandra's answer: Yes, of course, it doesn't break any cases, it's just that this flow is necessary for the hairpin case. I'll make a clearer description and correct the priority (I forgot to change it to 105 in the documentation in v3). > > Thanks, > Dumitru > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev 0: https://github.com/ovn-org/ovn/commit/a0f82efdd9dfd3ef2d9606c1890e353df1097a51 -- regards, Vladislav Odintsov _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
