| Message ID | 1584966886-11053-1-git-send-email-dceara@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,ovn,v2] ovn-northd: Forward ARP requests on localnet ports. | expand |
On Mon, Mar 23, 2020 at 6:05 PM Dumitru Ceara <dceara@redhat.com> wrote: > > Commit 32f5ebb06226 limited the ARP/ND broadcast domain but in scenarios > where ARP responder flows are installed only on chassis that own the > associated logical ports ARP requests should still be forwarded on > localnet ports because the router pipeline should be executed on the > chassis that owns the logical port. Only that chassis will reply to the > ARP/ND request. > > Reported-by: Michael Plato <michael.plato@tu-berlin.de> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2020-March/049856.html > Fixes: 32f5ebb06226 ("ovn-northd: Limit ARP/ND broadcast domain whenever possible.") > Signed-off-by: Dumitru Ceara <dceara@redhat.com> Thanks for the v2. The patch LGTM. How about adding a test case which replicates the scenario reported by Michael ? so that we don't regress later ? Thanks Numan > > --- > V2: > - Address Numan's comment and update ovn-northd documentation. > --- > northd/ovn-northd.8.xml | 3 ++- > northd/ovn-northd.c | 6 +++++- > 2 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml > index 7d03cbc..1e0993e 100644 > --- a/northd/ovn-northd.8.xml > +++ b/northd/ovn-northd.8.xml > @@ -1194,7 +1194,8 @@ output; > Priority-75 flows for each IP address/VIP/NAT address owned by a > router port connected to the switch. These flows match ARP requests > and ND packets for the specific IP addresses. Matched packets are > - forwarded only to the router that owns the IP address. > + forwarded only to the router that owns the IP address and, if > + present, to the localnet port of the logical switch. > </li> > > <li> > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index f648d2e..b76df05 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -5903,8 +5903,12 @@ build_lswitch_rport_arp_req_flow_for_ip(struct sset *ips, > ds_put_cstr(&match, "}"); > > /* Send a the packet only to the router pipeline and skip flooding it > - * in the broadcast domain. > + * in the broadcast domain (except for the localnet port). > */ > + if (od->localnet_port) { > + ds_put_format(&actions, "clone { outport = %s; output; }; ", > + od->localnet_port->json_key); > + } > ds_put_format(&actions, "outport = %s; output;", patch_op->json_key); > ovn_lflow_add_with_hint(lflows, od, S_SWITCH_IN_L2_LKUP, priority, > ds_cstr(&match), ds_cstr(&actions), stage_hint); > -- > 1.8.3.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 3/23/20 1:45 PM, Numan Siddique wrote: > On Mon, Mar 23, 2020 at 6:05 PM Dumitru Ceara <dceara@redhat.com> wrote: >> >> Commit 32f5ebb06226 limited the ARP/ND broadcast domain but in scenarios >> where ARP responder flows are installed only on chassis that own the >> associated logical ports ARP requests should still be forwarded on >> localnet ports because the router pipeline should be executed on the >> chassis that owns the logical port. Only that chassis will reply to the >> ARP/ND request. >> >> Reported-by: Michael Plato <michael.plato@tu-berlin.de> >> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2020-March/049856.html >> Fixes: 32f5ebb06226 ("ovn-northd: Limit ARP/ND broadcast domain whenever possible.") >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > Thanks for the v2. The patch LGTM. > How about adding a test case which replicates the scenario reported by > Michael ? so that > we don't regress later ? > You're right, thanks for reviewing the patch. I sent a V3 which now fixes the tests to cover this scenario too: https://patchwork.ozlabs.org/patch/1260545/ Thanks, Dumitru > Thanks > Numan > >> >> --- >> V2: >> - Address Numan's comment and update ovn-northd documentation. >> --- >> northd/ovn-northd.8.xml | 3 ++- >> northd/ovn-northd.c | 6 +++++- >> 2 files changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml >> index 7d03cbc..1e0993e 100644 >> --- a/northd/ovn-northd.8.xml >> +++ b/northd/ovn-northd.8.xml >> @@ -1194,7 +1194,8 @@ output; >> Priority-75 flows for each IP address/VIP/NAT address owned by a >> router port connected to the switch. These flows match ARP requests >> and ND packets for the specific IP addresses. Matched packets are >> - forwarded only to the router that owns the IP address. >> + forwarded only to the router that owns the IP address and, if >> + present, to the localnet port of the logical switch. >> </li> >> >> <li> >> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c >> index f648d2e..b76df05 100644 >> --- a/northd/ovn-northd.c >> +++ b/northd/ovn-northd.c >> @@ -5903,8 +5903,12 @@ build_lswitch_rport_arp_req_flow_for_ip(struct sset *ips, >> ds_put_cstr(&match, "}"); >> >> /* Send a the packet only to the router pipeline and skip flooding it >> - * in the broadcast domain. >> + * in the broadcast domain (except for the localnet port). >> */ >> + if (od->localnet_port) { >> + ds_put_format(&actions, "clone { outport = %s; output; }; ", >> + od->localnet_port->json_key); >> + } >> ds_put_format(&actions, "outport = %s; output;", patch_op->json_key); >> ovn_lflow_add_with_hint(lflows, od, S_SWITCH_IN_L2_LKUP, priority, >> ds_cstr(&match), ds_cstr(&actions), stage_hint); >> -- >> 1.8.3.1 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >
diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml index 7d03cbc..1e0993e 100644 --- a/northd/ovn-northd.8.xml +++ b/northd/ovn-northd.8.xml @@ -1194,7 +1194,8 @@ output; Priority-75 flows for each IP address/VIP/NAT address owned by a router port connected to the switch. These flows match ARP requests and ND packets for the specific IP addresses. Matched packets are - forwarded only to the router that owns the IP address. + forwarded only to the router that owns the IP address and, if + present, to the localnet port of the logical switch. </li> <li> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index f648d2e..b76df05 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -5903,8 +5903,12 @@ build_lswitch_rport_arp_req_flow_for_ip(struct sset *ips, ds_put_cstr(&match, "}"); /* Send a the packet only to the router pipeline and skip flooding it - * in the broadcast domain. + * in the broadcast domain (except for the localnet port). */ + if (od->localnet_port) { + ds_put_format(&actions, "clone { outport = %s; output; }; ", + od->localnet_port->json_key); + } ds_put_format(&actions, "outport = %s; output;", patch_op->json_key); ovn_lflow_add_with_hint(lflows, od, S_SWITCH_IN_L2_LKUP, priority, ds_cstr(&match), ds_cstr(&actions), stage_hint);
Commit 32f5ebb06226 limited the ARP/ND broadcast domain but in scenarios where ARP responder flows are installed only on chassis that own the associated logical ports ARP requests should still be forwarded on localnet ports because the router pipeline should be executed on the chassis that owns the logical port. Only that chassis will reply to the ARP/ND request. Reported-by: Michael Plato <michael.plato@tu-berlin.de> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2020-March/049856.html Fixes: 32f5ebb06226 ("ovn-northd: Limit ARP/ND broadcast domain whenever possible.") Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- V2: - Address Numan's comment and update ovn-northd documentation. --- northd/ovn-northd.8.xml | 3 ++- northd/ovn-northd.c | 6 +++++- 2 files changed, 7 insertions(+), 2 deletions(-)