On Fri, Aug 24, 2018 at 11:19 PM Han Zhou <zhou...@gmail.com> wrote:
>
>
> On Thu, Aug 23, 2018 at 12:10 PM <nusid...@redhat.com> wrote:
> >
> > From: Numan Siddique <nusid...@redhat.com>
> >
> > Commit [1] added a new action 'nd_na_router' to set the router bit
> > in the 'flags' field of the Neighbour Adv packet in the router
> > pipeline. But the logical switch pipeline was also adding the
> > Neighbour Adv flows for router IPs with 'nd_na' action, which the
> > commit [1] didn't handle.
> >
> > This patch fixes this.
> >
> > [1] - "c9756229ed: ovn: Set proper Neighbour Adv flag when replying
> > for NS request for router IP"
> >
> > Signed-off-by: Numan Siddique <nusid...@redhat.com>
> > ---
>
> Hi Numan, thanks for the fix. This patch fixes the issue by skipping
> adding the flow in LS pipeline, but this would make the ND responder not
> effective. It was supposed to suppress the broadcast packets by directly
> respond from the LS pipeline, now that the flow is skipped, it will end up
> flooding the broadcast to all ports on the LS. Would it be better to add a
> higher priority flow in same table in the LS pipeline to match an extra
> condition, the router IP, for the ND packet and use nd_na_router action
> there? This way we can avoid flooding when VM sending ND for router IP and
> at the same time respond correctly with the router bit set. What do you
> think?
>
Hi Han,
Your suggestion makes sense. I think we can use the same priority and use
"nd_nd_router" if the logical switch port is of type router.
>
> Please also find one minor comment inlined.
>
> Thanks,
> Han
>
> > ovn/northd/ovn-northd.c | 70 ++++++++++++++++++++++-------------------
> > tests/ovn.at | 24 +++++++++++++-
> > 2 files changed, 61 insertions(+), 33 deletions(-)
> >
> > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> > index 1d020a739..acc0d586e 100644
> > --- a/ovn/northd/ovn-northd.c
> > +++ b/ovn/northd/ovn-northd.c
> > @@ -4150,40 +4150,46 @@ build_lswitch_flows(struct hmap *datapaths,
> struct hmap *ports,
> > ds_cstr(&match), "next;");
> > }
> >
> > - /* For ND solicitations, we need to listen for both the
> > - * unicast IPv6 address and its all-nodes multicast address,
> > - * but always respond with the unicast IPv6 address. */
> > - for (size_t j = 0; j < op->lsp_addrs[i].n_ipv6_addrs; j++) {
> > - ds_clear(&match);
> > - ds_put_format(&match,
> > - "nd_ns && ip6.dst == {%s, %s} && nd.target ==
> %s",
> > - op->lsp_addrs[i].ipv6_addrs[j].addr_s,
> > - op->lsp_addrs[i].ipv6_addrs[j].sn_addr_s,
> > - op->lsp_addrs[i].ipv6_addrs[j].addr_s);
> > + /* Skip adding Neighbor Adv flows for ports with router
> peers.
> > + * Router pipeline takes care of adding those with
> nd_na_router
> > + * action.
> > + */
> > + if (!op->peer) {
>
> I am not sure if it is more straightforward to use: if
> (!strcmp(op->nbsp->type, "router"))
>
I thought about it. I will change it to strcmp. Seems more clearer.
Thanks for the review.
Numan
>
> > + /* For ND solicitations, we need to listen for both the
> > + * unicast IPv6 address and its all-nodes multicast
> address,
> > + * but always respond with the unicast IPv6 address. */
> > + for (size_t j = 0; j < op->lsp_addrs[i].n_ipv6_addrs;
> j++) {
> > + ds_clear(&match);
> > + ds_put_format(&match,
> > + "nd_ns && ip6.dst == {%s, %s} && nd.target
> == %s",
> > + op->lsp_addrs[i].ipv6_addrs[j].addr_s,
> > + op->lsp_addrs[i].ipv6_addrs[j].sn_addr_s,
> > + op->lsp_addrs[i].ipv6_addrs[j].addr_s);
> >
> > - ds_clear(&actions);
> > - ds_put_format(&actions,
> > - "nd_na { "
> > - "eth.src = %s; "
> > - "ip6.src = %s; "
> > - "nd.target = %s; "
> > - "nd.tll = %s; "
> > - "outport = inport; "
> > - "flags.loopback = 1; "
> > - "output; "
> > - "};",
> > - op->lsp_addrs[i].ea_s,
> > - op->lsp_addrs[i].ipv6_addrs[j].addr_s,
> > - op->lsp_addrs[i].ipv6_addrs[j].addr_s,
> > - op->lsp_addrs[i].ea_s);
> > - ovn_lflow_add(lflows, op->od, S_SWITCH_IN_ARP_ND_RSP,
> 50,
> > - ds_cstr(&match), ds_cstr(&actions));
> > + ds_clear(&actions);
> > + ds_put_format(&actions,
> > + "nd_na { "
> > + "eth.src = %s; "
> > + "ip6.src = %s; "
> > + "nd.target = %s; "
> > + "nd.tll = %s; "
> > + "outport = inport; "
> > + "flags.loopback = 1; "
> > + "output; "
> > + "};",
> > + op->lsp_addrs[i].ea_s,
> > + op->lsp_addrs[i].ipv6_addrs[j].addr_s,
> > + op->lsp_addrs[i].ipv6_addrs[j].addr_s,
> > + op->lsp_addrs[i].ea_s);
> > + ovn_lflow_add(lflows, op->od,
> S_SWITCH_IN_ARP_ND_RSP, 50,
> > + ds_cstr(&match), ds_cstr(&actions));
> >
> > - /* Do not reply to a solicitation from the port that
> owns the
> > - * address (otherwise DAD detection will fail). */
> > - ds_put_format(&match, " && inport == %s", op->json_key);
> > - ovn_lflow_add(lflows, op->od, S_SWITCH_IN_ARP_ND_RSP,
> 100,
> > - ds_cstr(&match), "next;");
> > + /* Do not reply to a solicitation from the port
> that owns
> > + * the address (otherwise DAD detection will fail).
> */
> > + ds_put_format(&match, " && inport == %s",
> op->json_key);
> > + ovn_lflow_add(lflows, op->od,
> S_SWITCH_IN_ARP_ND_RSP, 100,
> > + ds_cstr(&match), "next;");
> > + }
> > }
> > }
> > }
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index c5d054c21..2894ce28c 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -9530,7 +9530,7 @@ ovn-nbctl lr-add lr0_ip6
> > ovn-nbctl lrp-add lr0_ip6 lrp0_ip6 00:00:00:00:af:01
> aef0:0:0:0:0:0:0:0/64
> > ovn-nbctl lsp-add sw0_ip6 lrp0_ip6-attachment
> > ovn-nbctl lsp-set-type lrp0_ip6-attachment router
> > -ovn-nbctl lsp-set-addresses lrp0_ip6-attachment 00:00:00:00:af:01
> > +ovn-nbctl lsp-set-addresses lrp0_ip6-attachment router
> > ovn-nbctl lsp-set-options lrp0_ip6-attachment router-port=lrp0_ip6
> > ovn-nbctl set logical_router_port lrp0_ip6
> ipv6_ra_configs:address_mode=slaac
> >
> > @@ -9563,6 +9563,28 @@ ovs-vsctl -- add-port br-int hv1-vif1 -- \
> > ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
> >
> > OVS_WAIT_UNTIL([test x`ovn-nbctl lsp-get-up sw0_ip6-port1` = xup])
> > +
> > +# There should no Neighbor Advertisement flow for the router port
> > +# aef0:: ip address in logical switch pipeline.
> > +AT_CHECK([ovn-sbctl dump-flows sw0_ip6 | grep ls_in_arp_rsp | \
> > +grep "priority=50" | grep "nd.target == aef0::" | wc -l], [0], [1
> > +])
> > +
> > +# There should no Neighbor Advertisement flow for link local
> > +# address (fe80::200:ff:fe00:af01) of the router port in logical switch
> > +# pipeline.
> > +
> > +AT_CHECK([ovn-sbctl dump-flows sw0_ip6 | grep ls_in_arp_rsp | \
> > +grep "priority=50" | grep "nd.target == fe80::200:ff:fe00:af01" | wc
> -l],
> > +[0], [0
> > +])
> > +
> > +# There should be 4 Neighbor Advertisement flows with action
> nd_na_router
> > +# in the router pipeline for the router lr0_ip6.
> > +AT_CHECK([ovn-sbctl dump-flows lr0_ip6 | grep nd_na_router | \
> > +wc -l], [0], [4
> > +])
> > +
> > cr_uuid=`ovn-sbctl find port_binding logical_port=cr-ip6_public | grep
> _uuid | cut -f2 -d ":"`
> >
> > # There is only one chassis.
> > --
> > 2.17.1
> >
> > _______________________________________________
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
