[ovs-dev] [PATCH] Do not add IPv6 Neigh Adv flows for router ips in switch pipeline
Han Zhou
zhouhan at gmail.com
Fri Aug 24 18:32:47 UTC 2018
On Fri, Aug 24, 2018 at 11:17 AM Numan Siddique <nusiddiq at redhat.com> wrote:
>
>
>
> On Fri, Aug 24, 2018 at 11:19 PM Han Zhou <zhouhan at gmail.com> wrote:
>>
>>
>>
>> On Thu, Aug 23, 2018 at 12:10 PM <nusiddiq at redhat.com> wrote:
>> >
>> > From: Numan Siddique <nusiddiq at 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 <nusiddiq at 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.
Yes, forget about the "extra condition" I mentioned. IP is already there,
my bad :)
>
>>
>>
>> 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
>> > dev at openvswitch.org
>> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>
More information about the dev
mailing list