On Tue, Apr 28, 2020 at 6:10 PM Dumitru Ceara <[email protected]> wrote:
> On 4/28/20 2:19 PM, Numan Siddique wrote: > > > > > > On Tue, Apr 28, 2020 at 4:14 PM Dumitru Ceara <[email protected] > > <mailto:[email protected]>> wrote: > > > > In order to detect that traffic was hairpinned due to logical switch > > load > > balancers we need to match on source and destination IPs of packets > (and > > protocol ports potentially) in table ls_in_pre_hairpin. > > > > For this, until now, we created 2 logical flows for each backend of > > a load > > balancer VIP. However, in scenarios where large load balancers (i.e., > > with large numbers of backends) are applied to multiple logical > > switches, this might generate logical flow count explosion. > > > > One optimization is to generate a single logical flow per VIP that > > combines all conditions generated for each backend. This reduces > load on > > the SB DB because of lower number of logical flows and also reduces > > overall DB size because of less overhead due to other fields on the > > logical_flow records. > > > > Comparison of various performance aspects when running OVN with the > NB > > database attached to the bug report on a deployment with all VIFs > bound > > to a single node (62 load balancer VIPs with 513 load balancer > > backends, applied on 106 logical switches): > > > > Without this patch: > > - SB database size: 60MB > > - # of pre-hairpin logical flows: 109074 > > - # of logical flows: 159414 > > - ovn-controller max loop iteration time when processing SB DB: > 8803ms > > - ovn-northd max loop iteration time: 3988ms > > > > With this patch: > > - SB database size: 29MB (~50% decrease) > > - # of pre-hairpin logical flows: 13250 (~88% decrease) > > - # of logical flows: 63590 (~60% decrease) > > - ovn-controller max loop iteration time when processing SB DB: > 5585ms > > - ovn-northd max loop iteration time: 1594ms > > > > Reported-by: Aniket Bhat <[email protected] <mailto: > [email protected]>> > > Reported-at: https://bugzilla.redhat.com/1827403 > > Fixes: 1be8ac65bc60 ("ovn-northd: Support hairpinning for logical > > switch load balancing.") > > Signed-off-by: Dumitru Ceara <[email protected] > > <mailto:[email protected]>> > > > > > > > > Thanks for the fix. I applied this patch to master and branch 20.03. > > > > Thanks Numan! > > > With this patch we will see one logical flow with many ORs. I think to > > address the db explosion issue, > > this patch is fine. > > Although It would be good if we find another way to address the hair pin > > issue, like using learn action as suggested > > by Ilya. > > Yes, I'll work on a follow up patch. > > OVS "learn" action is an option but it also has some disadvantages: for > example flows generated by learn can't have "resubmit" as action so that > would require a separate OVS table for hairpin detection. > > An alternative to "learn" would be to add an "is_hairpin(VIP[:vip-port], > B1[:BP1], ... BN[:BPN])" composite expression to logical flow > expressions. Similarly we'd need an "is_hairpin_reply(...)". > > For a LB VIP of the form: > VIP = 42.42.42.42:4200 > Backends: [80.80.80.80:8000, 80:80:80:81:8100] > > ovn-northd would generate the following two logical flows: > LF1: table=pre-hairpin, priority=2, match="is_hairpin(42.42.42.42:4200, > 80.80.80.80:8000, 80:80:80:81:8100)" action="reg0[6]=1; > ct_snat(42.42.42.42);" > LF2: table=pre-hairpin, priority=1, > match="is_hairpin_reply(42.42.42.42:4200, 80.80.80.80:8000, > 80:80:80:81:8100)" action="reg0[6]=1; ct_snat;" > > ovn-controller, in expr.c, will then expand is_hairpin() and > is_hairpin_reply() to the corresponding expressions, i.e.: > - is_hairpin: (ip.src == B1 && ip.dst == B1 && tcp.dport == BP1) || ... > || (ip.src == BN && ip.dst == BN && tcp.dport == BPN) > - is_reply_hairpin: ip.dst == VIP && ((ip.src == B1 && tcp.sport == BP1) > || ... || (ip.src == BN && tcp.sport == BPN)) > > This approach also lowers the number of logical flows and preserves the > current OVN approach to always have an explicit mapping between SB > entities and openflow entries. It would also follow the current OVN > architecture. Also, adding support for hairpin at router level would > just be a matter of adding logical flows that use the new composite > expressions in the router pipelines. > > What do you think? > Explicit mapping between logical flows and the OF entries is definitely preferred. Looking forward to the patch. Thanks Numan > > > > > Thanks > > Numan > > Thanks, > Dumitru > > > > > --- > > northd/ovn-northd.8.xml | 4 +-- > > northd/ovn-northd.c | 79 > > ++++++++++++++++++++++++++++++++----------------- > > 2 files changed, 54 insertions(+), 29 deletions(-) > > > > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml > > index d39e259..1f81742 100644 > > --- a/northd/ovn-northd.8.xml > > +++ b/northd/ovn-northd.8.xml > > @@ -559,14 +559,14 @@ > > <h3>Ingress Table 11: Pre-Hairpin</h3> > > <ul> > > <li> > > - For all configured load balancer backends a priority-2 flow > > that > > + For all configured load balancer VIPs a priority-2 flow that > > matches on traffic that needs to be hairpinned, i.e., after > > load > > balancing the destination IP matches the source IP, which > sets > > <code>reg0[6] = 1 </code> and executes > > <code>ct_snat(VIP)</code> > > to force replies to these packets to come back through OVN. > > </li> > > <li> > > - For all configured load balancer backends a priority-1 flow > > that > > + For all configured load balancer VIPs a priority-1 flow that > > matches on replies to hairpinned traffic, i.e., destination > > IP is VIP, > > source IP is the backend IP and source L4 port is backend > > port, which > > sets <code>reg0[6] = 1 </code> and executes > > <code>ct_snat;</code>. > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > > index 63753ac..742aad8 100644 > > --- a/northd/ovn-northd.c > > +++ b/northd/ovn-northd.c > > @@ -5550,52 +5550,77 @@ build_lb_hairpin_rules(struct ovn_datapath > > *od, struct hmap *lflows, > > struct ovn_lb *lb, struct lb_vip *lb_vip, > > const char *ip_match, const char *proto) > > { > > + if (lb_vip->n_backends == 0) { > > + return; > > + } > > + > > + struct ds action = DS_EMPTY_INITIALIZER; > > + struct ds match_initiator = DS_EMPTY_INITIALIZER; > > + struct ds match_reply = DS_EMPTY_INITIALIZER; > > + struct ds proto_match = DS_EMPTY_INITIALIZER; > > + > > /* Ingress Pre-Hairpin table. > > - * - Priority 2: SNAT load balanced traffic that needs to be > > hairpinned. > > + * - Priority 2: SNAT load balanced traffic that needs to be > > hairpinned: > > + * - Both SRC and DST IP match backend->ip and destination > port > > + * matches backend->port. > > * - Priority 1: unSNAT replies to hairpinned load balanced > > traffic. > > + * - SRC IP matches backend->ip, DST IP matches LB VIP and > > source port > > + * matches backend->port. > > */ > > + ds_put_char(&match_reply, '('); > > for (size_t i = 0; i < lb_vip->n_backends; i++) { > > struct lb_vip_backend *backend = &lb_vip->backends[i]; > > - struct ds action = DS_EMPTY_INITIALIZER; > > - struct ds match = DS_EMPTY_INITIALIZER; > > - struct ds proto_match = DS_EMPTY_INITIALIZER; > > > > /* Packets that after load balancing have equal source and > > - * destination IPs should be hairpinned. SNAT them so that > > the reply > > - * traffic is directed also through OVN. > > + * destination IPs should be hairpinned. > > */ > > if (lb_vip->vip_port) { > > - ds_put_format(&proto_match, " && %s && %s.dst == > %"PRIu16, > > - proto, proto, backend->port); > > + ds_put_format(&proto_match, " && %s.dst == %"PRIu16, > > + proto, backend->port); > > } > > - ds_put_format(&match, "%s.src == %s && %s.dst == %s%s", > > + ds_put_format(&match_initiator, "(%s.src == %s && %s.dst == > > %s%s)", > > ip_match, backend->ip, ip_match, backend->ip, > > ds_cstr(&proto_match)); > > - ds_put_format(&action, REGBIT_HAIRPIN " = 1; ct_snat(%s);", > > - lb_vip->vip); > > - ovn_lflow_add_with_hint(lflows, od, > S_SWITCH_IN_PRE_HAIRPIN, 2, > > - ds_cstr(&match), ds_cstr(&action), > > - &lb->nlb->header_); > > > > - /* If the packets are replies for hairpinned traffic, > > UNSNAT them. */ > > + /* Replies to hairpinned traffic are originated by > > backend->ip:port. */ > > ds_clear(&proto_match); > > - ds_clear(&match); > > if (lb_vip->vip_port) { > > - ds_put_format(&proto_match, " && %s && %s.src == > %"PRIu16, > > - proto, proto, backend->port); > > + ds_put_format(&proto_match, " && %s.src == %"PRIu16, > proto, > > + backend->port); > > } > > - ds_put_format(&match, "%s.src == %s && %s.dst == %s%s", > > - ip_match, backend->ip, ip_match, lb_vip->vip, > > + ds_put_format(&match_reply, "(%s.src == %s%s)", ip_match, > > backend->ip, > > ds_cstr(&proto_match)); > > - ovn_lflow_add_with_hint(lflows, od, > S_SWITCH_IN_PRE_HAIRPIN, 1, > > - ds_cstr(&match), > > - REGBIT_HAIRPIN " = 1; ct_snat;", > > - &lb->nlb->header_); > > + ds_clear(&proto_match); > > > > - ds_destroy(&action); > > - ds_destroy(&match); > > - ds_destroy(&proto_match); > > + if (i < lb_vip->n_backends - 1) { > > + ds_put_cstr(&match_initiator, " || "); > > + ds_put_cstr(&match_reply, " || "); > > + } > > } > > + ds_put_char(&match_reply, ')'); > > + > > + /* SNAT hairpinned initiator traffic so that the reply traffic > is > > + * also directed through OVN. > > + */ > > + ds_put_format(&action, REGBIT_HAIRPIN " = 1; ct_snat(%s);", > > + lb_vip->vip); > > + ovn_lflow_add_with_hint(lflows, od, S_SWITCH_IN_PRE_HAIRPIN, 2, > > + ds_cstr(&match_initiator), > > ds_cstr(&action), > > + &lb->nlb->header_); > > + > > + /* Replies to hairpinned traffic are destined to the LB VIP. */ > > + ds_put_format(&match_reply, " && %s.dst == %s", ip_match, > > lb_vip->vip); > > + > > + /* UNSNAT replies for hairpinned traffic. */ > > + ovn_lflow_add_with_hint(lflows, od, S_SWITCH_IN_PRE_HAIRPIN, 1, > > + ds_cstr(&match_reply), > > + REGBIT_HAIRPIN " = 1; ct_snat;", > > + &lb->nlb->header_); > > + > > + ds_destroy(&action); > > + ds_destroy(&match_initiator); > > + ds_destroy(&match_reply); > > + ds_destroy(&proto_match); > > } > > > > static void > > -- > > 1.8.3.1 > > > > _______________________________________________ > > dev mailing list > > [email protected] <mailto:[email protected]> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > _______________________________________________ > 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
