| Message ID | aada288d40f119d647eb5d43c17d9eab97cbce03.1590068692.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,ovn] controller: fix ip buffering with static routes | expand |
Hi Lorenzo,
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.
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.
Acked-by: Ankur Sharma <ankur.sharma@nutanix.com>
Regards,
Ankur
> 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? > 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> Regards, Lorenzo > > Acked-by: Ankur Sharma <ankur.sharma@nutanix.com> > > Regards, > Ankur > ________________________________ > From: dev <ovs-dev-bounces@openvswitch.org> on behalf of Lorenzo Bianconi <lorenzo.bianconi@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.bianconi@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 > dev@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=
On Fri, May 22, 2020 at 1:19 PM Lorenzo Bianconi < lorenzo.bianconi@redhat.com> wrote: > > 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? > > > 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> > > Regards, > Lorenzo > > > > > Acked-by: Ankur Sharma <ankur.sharma@nutanix.com> > Thanks Lorenzo and Ankur (for the reviews). I applied this patch to master. I agree with Ankur that having a test case covering this scenario will be helpful. @Lorenzo - Would you mind a follow up patch for this if it's possible ? Do we need to apply this patch to 20.03 ? Thanks Numan > > > Regards, > > Ankur > > ________________________________ > > From: dev <ovs-dev-bounces@openvswitch.org> on behalf of Lorenzo > Bianconi <lorenzo.bianconi@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.bianconi@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 > > dev@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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
> On Fri, May 22, 2020 at 1:19 PM Lorenzo Bianconi < > lorenzo.bianconi@redhat.com> wrote: > > > > 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? > > > > > 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> > > > > Regards, > > Lorenzo > > > > > > > > Acked-by: Ankur Sharma <ankur.sharma@nutanix.com> > > > > Thanks Lorenzo and Ankur (for the reviews). > I applied this patch to master. > > I agree with Ankur that having a test case covering this scenario will be > helpful. > > @Lorenzo - Would you mind a follow up patch for this if it's possible ? Hi Numan, I have already intoduced a test case for this scenario here: https://github.com/ovn-org/ovn/blob/master/tests/ovn.at#L15022 and here: https://github.com/ovn-org/ovn/blob/master/tests/system-ovn.at#L2751 Do you mean something more specific? Regards, Lorenzo > > Do we need to apply this patch to 20.03 ? > > Thanks > Numan > > > > > > Regards, > > > Ankur > > > ________________________________ > > > From: dev <ovs-dev-bounces@openvswitch.org> on behalf of Lorenzo > > Bianconi <lorenzo.bianconi@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.bianconi@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 > > > dev@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 > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > >
On Thu, May 28, 2020 at 5:45 PM Lorenzo Bianconi < lorenzo.bianconi@redhat.com> wrote: > > On Fri, May 22, 2020 at 1:19 PM Lorenzo Bianconi < > > lorenzo.bianconi@redhat.com> wrote: > > > > > > 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? > > > > > > > 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> > > > > > > Regards, > > > Lorenzo > > > > > > > > > > > Acked-by: Ankur Sharma <ankur.sharma@nutanix.com> > > > > > > > Thanks Lorenzo and Ankur (for the reviews). > > I applied this patch to master. > > > > I agree with Ankur that having a test case covering this scenario will > be > > helpful. > > > > @Lorenzo - Would you mind a follow up patch for this if it's possible ? > > Hi Numan, > > I have already intoduced a test case for this scenario here: > https://github.com/ovn-org/ovn/blob/master/tests/ovn.at#L15022 > and here: > https://github.com/ovn-org/ovn/blob/master/tests/system-ovn.at#L2751 > > Do you mean something more specific? > > Can you please see the reply from Ankur here - https://mail.openvswitch.org/pipermail/ovs-dev/2020-May/370908.html I think he has some suggestions about the test cases. I should have replied in that thread. Thanks Numan Regards, > Lorenzo > > > > > Do we need to apply this patch to 20.03 ? > > > > Thanks > > Numan > > > > > > > > > Regards, > > > > Ankur > > > > ________________________________ > > > > From: dev <ovs-dev-bounces@openvswitch.org> on behalf of Lorenzo > > > Bianconi <lorenzo.bianconi@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.bianconi@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 > > > > dev@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 > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
> On Thu, May 28, 2020 at 5:45 PM Lorenzo Bianconi < > lorenzo.bianconi@redhat.com> wrote: > > > > On Fri, May 22, 2020 at 1:19 PM Lorenzo Bianconi < > > > lorenzo.bianconi@redhat.com> wrote: > > > > > > > > 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? > > > > > > > > > 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> > > > > > > > > Regards, > > > > Lorenzo > > > > > > > > > > > > > > Acked-by: Ankur Sharma <ankur.sharma@nutanix.com> > > > > > > > > > > Thanks Lorenzo and Ankur (for the reviews). > > > I applied this patch to master. > > > > > > I agree with Ankur that having a test case covering this scenario will > > be > > > helpful. > > > > > > @Lorenzo - Would you mind a follow up patch for this if it's possible ? > > > > Hi Numan, > > > > I have already intoduced a test case for this scenario here: > > https://github.com/ovn-org/ovn/blob/master/tests/ovn.at#L15022 > > and here: > > https://github.com/ovn-org/ovn/blob/master/tests/system-ovn.at#L2751 > > > > Do you mean something more specific? > > > > > Can you please see the reply from Ankur here - > https://mail.openvswitch.org/pipermail/ovs-dev/2020-May/370908.html > I think he has some suggestions about the test cases. I should have replied > in that thread. since these tests [1] and [2] have been added in commit: fda9a1dd3c995f25cad9e828e701f8b41d347bbb ("northd: manage ARP request locally for FIP traffic") they were not merged when I sent this particular fix. @Ankur: could you please take a look now at ovn.at/system-ovn.at? Do you think tests are fine now? If not I am 100% fine to add some more tests. Regards, Lorenzo [1] https://github.com/ovn-org/ovn/blob/master/tests/ovn.at#L15022 [2] https://github.com/ovn-org/ovn/blob/master/tests/system-ovn.at#L2751 > > Thanks > Numan > > Regards, > > Lorenzo > > > > > > > > Do we need to apply this patch to 20.03 ? > > > > > > Thanks > > > Numan > > > > > > > > > > > > Regards, > > > > > Ankur > > > > > ________________________________ > > > > > From: dev <ovs-dev-bounces@openvswitch.org> on behalf of Lorenzo > > > > Bianconi <lorenzo.bianconi@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.bianconi@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 > > > > > dev@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 > > > > dev@openvswitch.org > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > >
Hi Lorenzo, Numan, Thanks a lot for the followup. I went through the tests added in: fda9a1dd3c995f25cad9e828e701f8b41d347bbb Only missing scenario in the added testcase is to validate draining of buffered ICMP packets. Regarding applying to 20.03, i think we should, otherwise buffering is not working for the most common use case. Regards, Ankur
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];
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.bianconi@redhat.com> --- controller/pinctrl.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)