[ovs-dev] [PATCH ovn] lflow: Relax the condition that detects Load_Balancer hairpin.

[Dumitru Ceara]

On 12/14/20 10:34 AM, Numan Siddique wrote:
> On Fri, Dec 11, 2020 at 3:46 PM Dumitru Ceara <dceara at redhat.com> wrote:
>>
>> Commit fc219d84b667 [0] optimized Load Balancer hairpin support from an
>> OVN Southbound perspective by moving generation of flows to detect
>> hairpin traffic in ovn-controller.
>>
>> However, this can be further optimized by relaxing the condition for
>> flows that detect hairpin traffic in the "original" direction.
>>
>> It is safe to exclude the match on datapath.tunnel_key in the original
>> direction if we make sure that we only match on traffic that was
>> successfully load balanced (i.e., ct.natted = 1).  At this point it
>> would be enough to check that ip.src == ip.dst in order to determine
>> that traffic needs to be hairpinned.  Because OVS doesn't allow such
>> checks between fields we instead check against the known load balancer
>> backend IPs: ip.src == backend-ip && ip.dst == backend-ip.
>>
>> This change improves scalability by reducing the number of hairpin OF
>> flows by ~50%.  For example, on a setup with:
>> - N Load Balancer VIPs.
>> - M Backends per VIP.
>> - All N VIPs applied to S logical switches.
>>
>> Before this change the number of hairpin OF flows was:
>> - N x M x S (for original direction) + N x M x S (for reply direction)
>>
>> With this change the number of hairpin OF flows is:
>> - N x M (for original direction ) + N x M x S (for reply direction)
>>
>> That means that on a setup with N=100 VIPs, M=10 backends, S=100
>> switches the number of hairpin OF flows will be 101K vs 200K before this
>> change.
>>
>> [0] fc219d84b667 ("actions: Add new actions chk_lb_hairpin, chk_lb_hairpin_reply and ct_snat_to_vip.")
>>
>> Signed-off-by: Dumitru Ceara <dceara at redhat.com>
> 
> Thanks Dumitru for addressing this scale concern and reducing the OF
> flows by half.
> 
> Could we also do the same even for OFTABLE_CHK_LB_HAIRPIN_REPLY ? Did
> you explore that too ?
> 

Hi Numan,

Not really, at least not exactly in the same way.  The problem is that
reply to hairpinned traffic has ip.src == lb_backend.ip && ip.dst ==
lb.vip.  So we can't just remove the match on metadata (like we did for
packets in the original direction) because a load balancer might not be
applied on all datapaths.

One option is to send all traffic with ip.dst == lb.vip to conntrack to
see if it matches the SNAT session we created for the hairpinned packet
in the original direction.  However, this additional recirculation has
implications on throughput and latency for all non-hairpinned load
balancer traffic.

I don't have a better solution yet but I'll give it more thought.

> I applied this patch to master and also to branch-20.12 as it seems an
> important fix.

Thanks.

> 
> There was one nit which I fixed before applying
> 
> ---
> diff --git a/controller/lflow.c b/controller/lflow.c
> index f63a6f56a5..c02585b1eb 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -1226,7 +1226,7 @@ add_lb_vip_hairpin_flows(struct ovn_controller_lb *lb,
>       * to not include the datapath tunnel_key in the match when determining
>       * that a packet needs to be hairpinned because the rest of the match is
>       * restrictive enough:
> -     * - traffic must have already been load balancedu
> +     * - traffic must have already been load balanced.
>       * - packets must have ip.src == ip.dst at this point.
>       * - the destination protocol and port must be of a valid backend that
>       *   has the same IP as ip.dst.

This looks good to me, thanks for catching the typo!

Thanks,
Dumitru