On Thu, Sep 5, 2019 at 3:19 AM Han Zhou <[email protected]> wrote: > > > On Mon, Sep 2, 2019 at 9:32 AM <[email protected]> wrote: > > > > From: Numan Siddique <[email protected]> > > > > Suppose there is a provider network (with localnet port) and it is > > connected to a logical router via a distributed gateway router port. > > When an external switch sends GARP request packets, these packets > > enter the br-int (via the patch port from the provider bridge) and > > the action - put_arp is applied in the router pipeline only on the > > gateway chassis where the distributed gateway router port is scheduled. > > Other chassis even if they receive don't apply this action. This > functionality > > was added by the commit [1]. > > > > But that is not the case with GARP reply packets sent by the external > > switch. ovn-controllers running on all the chassis configured with > > ovn-bridge-mappings receive these packets as packet-ins because > > of put_arp action. This results in unnecessary processing of these > > packets only to be ignored. pinctrl thread wakes up the main > ovn-controller > > thread wasting few CPU cycles. It is enough for the gateway chassis to > > handle these packets where the distributed router gateway port is > scheduled. > > > > This patch achieves the same - similar to GARP request packets. The below > > logical flows are added > > - table=1 (lr_in_ip_input), priority=92, > > match=(inport == "lrp" && > > !is_chassis_resident("cr-lrp") && eth.bcast && > > arp.op == 2 && arp.spa == PROVIDER_NETWORK_CIDR), > action=(drop;) > > > > - table=1 (lr_in_ip_input), priority=92 > > match=(inport == "lr0-public" && is_chassis_resident("cr-lr0-public") > > && eth.bcast && arp.op == 2 && arp.spa == > PROVIDER_NETWORK_CIDR), > > action=(put_arp(inport, arp.spa, arp.sha);) > > > > This patch doesn't affect the arp replies from the overlay networks in > the > > router pipeline. This only handles GARP replies from the provider > networks. > > > > In the present master this is not much of any issue even if > ovn-controller > > wakes up, thanks to incremental processing engine. But in the older > > versions - 2.11, 2.10 and 2.9, this results in complete flow calculations > > resulting in much more CPU cyles. This patch will definitely help in > saving > > these CPU cyles if backported. > > > > [1] - 3dd9febe953f("ovn-northd: Support learning neighbor from ARP > request.") > > > > CC: Han ZHou <[email protected]> > > Signed-off-by: Numan Siddique <[email protected]> > > --- > > northd/ovn-northd.8.xml | 31 +++++++++++++++++++++++++++---- > > northd/ovn-northd.c | 35 +++++++++++++++++++++++++++++++++++ > > 2 files changed, 62 insertions(+), 4 deletions(-) > > > > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml > > index 442e899bc..a06e404dd 100644 > > --- a/northd/ovn-northd.8.xml > > +++ b/northd/ovn-northd.8.xml > > @@ -1442,10 +1442,33 @@ arp.sha = <var>external_mac</var>; > > </li> > > > > <li> > > - ARP reply handling. This flow uses ARP replies to populate the > > - logical router's ARP table. A priority-90 flow with match > <code>arp.op > > - == 2</code> has actions <code>put_arp(inport, arp.spa, > > - arp.sha);</code>. > > + <p> > > + ARP reply handling. Following flows are added to handle ARP > replies. > > + </p> > > + > > + <p> > > + For each distributed gateway logical router port a > priority-92 flow > > + with match <code>inport == <var>P</var> && > > + is_chassis_resident(cr-<var>P</var>) && eth.bcast > && > > + arp.op == 2 && arp.spa == <var>I</var></code> with the > > + action <code>put_arp(inport, arp.spa, arp.sha);</code> so > that the > > + resident gateway chassis can learn the GARP reply, where > > + <var>P</var> is the distributed gateway router port name, > > + <var>I</var> is the logical router port's network address. > > + </p> > > + > > + <p> > > + For each distributed gateway logical router port a > priority-92 flow > > + with match <code>inport == <var>P</var> && > > + !is_chassis_resident(cr-<var>P</var>) && eth.bcast > && > > + arp.op == 2 && arp.spa == <var>I</var></code> with > the action > > + <code>drop;</code> so that other chassis drop this packet. > > + </p> > > + > > + <p> > > + A priority-90 flow with match <code>arp.op == 2</code> has > actions > > + <code>put_arp(inport, arp.spa, arp.sha);</code>. > > + </p> > > </li> > > > > <li> > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > > index 9a2222282..a83b56362 100644 > > --- a/northd/ovn-northd.c > > +++ b/northd/ovn-northd.c > > @@ -6552,6 +6552,41 @@ build_lrouter_flows(struct hmap *datapaths, > struct hmap *ports, > > > > } > > > > + /* Handle GARP reply packets received on a distributed router > gateway > > + * port. GARP reply broadcast packets could be sent by external > > + * switches. We don't want them to be handled by all the > > + * ovn-controllers if they receive it. So add a priority-92 > flow to > > + * apply the put_arp action on a redirect chassis and drop it on > > + * other chassis. > > + * Note that we are already adding a priority-90 logical flow > in the > > + * table S_ROUTER_IN_IP_INPUT to apply the put_arp action if > > + * arp.op == 2. > > + * */ > > + if (op->od->l3dgw_port && op == op->od->l3dgw_port > > + && op->od->l3redirect_port) { > > + for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) { > > + ds_clear(&match); > > + ds_put_format(&match, > > + "inport == %s && is_chassis_resident(%s) > && " > > + "eth.bcast && arp.op == 2 && arp.spa == > %s/%u", > > + op->json_key, > op->od->l3redirect_port->json_key, > > + op->lrp_networks.ipv4_addrs[i].network_s, > > + op->lrp_networks.ipv4_addrs[i].plen); > > + ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 92, > > + ds_cstr(&match), > > + "put_arp(inport, arp.spa, arp.sha);"); > > + ds_clear(&match); > > + ds_put_format(&match, > > + "inport == %s && !is_chassis_resident(%s) > && " > > + "eth.bcast && arp.op == 2 && arp.spa == > %s/%u", > > + op->json_key, > op->od->l3redirect_port->json_key, > > + op->lrp_networks.ipv4_addrs[i].network_s, > > + op->lrp_networks.ipv4_addrs[i].plen); > > + ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 92, > > + ds_cstr(&match), "drop;"); > > + } > > + } > > + > > /* A set to hold all load-balancer vips that need ARP > responses. */ > > struct sset all_ips = SSET_INITIALIZER(&all_ips); > > int addr_family; > > -- > > 2.21.0 > > > > _______________________________________________ > > dev mailing list > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Thanks Numan. > > Acked-by: Han Zhou <[email protected]> >
Thanks for the review. I pushed this to master. Numan _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
