Hi Lorenzo, Please find my replies inline.
Regards, Ankur ________________________________ From: Lorenzo Bianconi Sent: Friday, May 22, 2020 12:48 AM To: Ankur Sharma Cc: ovs-dev@openvswitch.org Subject: Re: [ovs-dev] [PATCH ovn] controller: fix ip buffering with static routes > Hi Lorenzo, Hi Ankur, > > Good catch. > > a. If not already considered, then i think it is a candidate for 20.06 > b. Need not be done in this patch, but it will be good to have a testcase > which validates the draining of buffered packets. We already have some tests in system-ovn.at and in ovn.at. Do you mean something more specific? [ANKUR]: Yes, looking at the testcase "[ovn -- IP packet buffering]", looks like it is testing buffering for connected subnets (logical switches), i.e EW traffic, using "unknown" address. While scenario is not invalid, however feature is mostly applicable where "get_arp" will be called and hence buffering will happen is N-S traffic. Having a testcase where get_arp is called for a gateway, while destination ip is that of atleast more than a hop away endpoint. > c. Not sure about the commit header. I mean this issue is not specific to > static route right? By default there will be a gateway and destination ip > will not be that of gateway ip. > That's a regular NS workflow. I think ovn does not add any "default" route by default. You need to add it doing something like: ovn-nbctl lr-route-add <logical-router> 0.0.0.0/0 <next-hop-ip> [ANKUR]: Not a big deal, i was trying to convey that having a default route (i.e static route which you have called out) is implicit for NS traffic. This point is not worth further discussion. Regards, Lorenzo > > Acked-by: Ankur Sharma <ankur.sha...@nutanix.com> > > Regards, > Ankur > ________________________________ > From: dev <ovs-dev-boun...@openvswitch.org> on behalf of Lorenzo Bianconi > <lorenzo.bianc...@redhat.com> > Sent: Thursday, May 21, 2020 6:46 AM > To: ovs-dev@openvswitch.org <ovs-dev@openvswitch.org> > Subject: [ovs-dev] [PATCH ovn] controller: fix ip buffering with static routes > > When the ARP request is sent to a gw router and not to the final > destination of the packet buffered_packets_map needs to be updated using > next-hop IP address and not the destination one. > > Fixes: 2e5cdb4b1392 ("OVN: add buffering support for ip packets") > Signed-off-by: Lorenzo Bianconi <lorenzo.bianc...@redhat.com> > --- > controller/pinctrl.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index bea446c89..bb90edd1f 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -1381,8 +1381,7 @@ pinctrl_find_buffered_packets(const struct in6_addr > *ip, uint32_t hash) > > /* Called with in the pinctrl_handler thread context. */ > static int > -pinctrl_handle_buffered_packets(const struct flow *ip_flow, > - struct dp_packet *pkt_in, > +pinctrl_handle_buffered_packets(struct dp_packet *pkt_in, > const struct match *md, bool is_arp) > OVS_REQUIRES(pinctrl_mutex) > { > @@ -1391,9 +1390,10 @@ pinctrl_handle_buffered_packets(const struct flow > *ip_flow, > struct in6_addr addr; > > if (is_arp) { > - addr = in6_addr_mapped_ipv4(ip_flow->nw_dst); > + addr = in6_addr_mapped_ipv4(htonl(md->flow.regs[0])); > } else { > - addr = ip_flow->ipv6_dst; > + ovs_be128 ip6 = hton128(flow_get_xxreg(&md->flow, 0)); > + memcpy(&addr, &ip6, sizeof addr); > } > > uint32_t hash = hash_bytes(&addr, sizeof addr, 0); > @@ -1434,7 +1434,7 @@ pinctrl_handle_arp(struct rconn *swconn, const struct > flow *ip_flow, > } > > ovs_mutex_lock(&pinctrl_mutex); > - pinctrl_handle_buffered_packets(ip_flow, pkt_in, md, true); > + pinctrl_handle_buffered_packets(pkt_in, md, true); > ovs_mutex_unlock(&pinctrl_mutex); > > /* Compose an ARP packet. */ > @@ -5281,7 +5281,7 @@ pinctrl_handle_nd_ns(struct rconn *swconn, const struct > flow *ip_flow, > } > > ovs_mutex_lock(&pinctrl_mutex); > - pinctrl_handle_buffered_packets(ip_flow, pkt_in, md, false); > + pinctrl_handle_buffered_packets(pkt_in, md, false); > ovs_mutex_unlock(&pinctrl_mutex); > > uint64_t packet_stub[128 / 8]; > -- > 2.26.2 > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=s883GpUCOChKOHiocYtGcg&r=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY&m=M_tzprKGSufeFSzeZOYjY5JUCnecaZWqQmYWNJazeiY&s=NQAYRfJ3Svolg8UWgwkkDxAH8FTT4PKayFV7Kw--fN8&e= _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev