[ovs-dev] [RFC ovn 6/6] ovn-northd.c: Support optionally disabling neighbor learning from ARP request/NS.

Mark Michelson mmichels at redhat.com
Thu Jun 11 12:54:31 UTC 2020


On 6/10/20 3:00 PM, Han Zhou wrote:
> Support a new logical router option "learn_from_arp_request" that controls
> behavior when handling ARP requests or IPv4 ND-NS packets.
> 
> "true" - Always learn the MAC/IP binding and add a new MAC_Binding entry
> (default behavior)
> 
> "false" - If there is a MAC_binding for that IP and the MAC is different, or,
> if TPA of ARP request belongs to any router port on this router, then
> update/add that MAC/IP binding. Otherwise, don't update/add entries.

Hi Han,

The "learn_from_arp_requests" option is not documented in this patch.

The option values are misleading. As a user, I would expect that setting 
"learn_from_arp_requests" to false would mean *never* learning from ARP 
requests. Perhaps instead of "true/false" this could be a string such as 
"always/local_only" that better indicates what the option does.

> 
> Reported-by: Girish Moodalbail <gmoodalbail at nvidia.com>
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2020-May/049995.html
> Signed-off-by: Han Zhou <hzhou at ovn.org>
> ---
>   northd/ovn-northd.c | 78 ++++++++++++++++++++++++++++++++++++++++++++---------
>   tests/ovn.at        | 18 +++++++++++++
>   2 files changed, 84 insertions(+), 12 deletions(-)
> 
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 9a4e884..8a1f490 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -219,6 +219,7 @@ enum ovn_stage {
>   /* Register to store the result of check_pkt_larger action. */
>   #define REGBIT_PKT_LARGER        "reg9[1]"
>   #define REGBIT_LOOKUP_NEIGHBOR_RESULT "reg9[2]"
> +#define REGBIT_LOOKUP_NEIGHBOR_IP_RESULT "reg9[3]"
>   
>   /* Register for ECMP bucket selection. */
>   #define REG_ECMP_GROUP_ID       "reg8[0..15]"
> @@ -7964,18 +7965,33 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>            * */
>   
>           /* Flows for LOOKUP_NEIGHBOR. */
> +        bool learn_from_arp_request = smap_get_bool(&od->nbr->options,
> +                                                    "learn_from_arp_request",
> +                                                    true);
> +        ds_clear(&actions);
> +        ds_put_format(&actions, REGBIT_LOOKUP_NEIGHBOR_RESULT
> +                      " = lookup_arp(inport, arp.spa, arp.sha); %snext;",
> +                      learn_from_arp_request? "":
> +                      REGBIT_LOOKUP_NEIGHBOR_IP_RESULT" = 1; ");
>           ovn_lflow_add(lflows, od, S_ROUTER_IN_LOOKUP_NEIGHBOR, 100,
> -                      "arp.op == 2",
> -                      REGBIT_LOOKUP_NEIGHBOR_RESULT" = "
> -                      "lookup_arp(inport, arp.spa, arp.sha); next;");
> +                      "arp.op == 2", ds_cstr(&actions));
>   
> +        ds_clear(&actions);
> +        ds_put_format(&actions, REGBIT_LOOKUP_NEIGHBOR_RESULT
> +                      " = lookup_nd(inport, nd.target, nd.tll); %snext;",
> +                      learn_from_arp_request? "":

Minor nitpick, but coding guidelines suggest putting a space on either 
side of the "?" and ":" operators.

> +                      REGBIT_LOOKUP_NEIGHBOR_IP_RESULT" = 1; ");
>           ovn_lflow_add(lflows, od, S_ROUTER_IN_LOOKUP_NEIGHBOR, 100, "nd_na",
> -                      REGBIT_LOOKUP_NEIGHBOR_RESULT" = "
> -                      "lookup_nd(inport, nd.target, nd.tll); next;");
> +                      ds_cstr(&actions));
>   
> +        ds_clear(&actions);
> +        ds_put_format(&actions, REGBIT_LOOKUP_NEIGHBOR_RESULT
> +                      " = lookup_nd(inport, ip6.src, nd.sll); %snext;",
> +                      learn_from_arp_request? "":
> +                      REGBIT_LOOKUP_NEIGHBOR_IP_RESULT
> +                      " = lookup_nd_ip(inport, ip6.src); ");
>           ovn_lflow_add(lflows, od, S_ROUTER_IN_LOOKUP_NEIGHBOR, 100, "nd_ns",
> -                      REGBIT_LOOKUP_NEIGHBOR_RESULT" = "
> -                      "lookup_nd(inport, ip6.src, nd.sll); next;");
> +                      ds_cstr(&actions));
>   
>           /* For other packet types, we can skip neighbor learning.
>            * So set REGBIT_LOOKUP_NEIGHBOR_RESULT to 1. */
> @@ -7984,8 +8000,12 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>   
>           /* Flows for LEARN_NEIGHBOR. */
>           /* Skip Neighbor learning if not required. */
> +        ds_clear(&match);
> +        ds_put_format(&match, REGBIT_LOOKUP_NEIGHBOR_RESULT" == 1%s",
> +                      learn_from_arp_request? "":
> +                      " || "REGBIT_LOOKUP_NEIGHBOR_IP_RESULT" == 0");
>           ovn_lflow_add(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 100,
> -                      REGBIT_LOOKUP_NEIGHBOR_RESULT" == 1", "next;");
> +                      ds_cstr(&match), "next;");
>   
>           ovn_lflow_add(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 90,
>                         "arp", "put_arp(inport, arp.spa, arp.sha); next;");
> @@ -8002,8 +8022,38 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>               continue;
>           }
>   
> +        bool learn_from_arp_request = smap_get_bool(&op->od->nbr->options,
> +                                                    "learn_from_arp_request",
> +                                                    true);
> +
>           /* Check if we need to learn mac-binding from ARP requests. */
>           for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
> +            if (!learn_from_arp_request) {
> +                /* ARP request to this address should always get learned,
> +                 * so add a priority-110 flow to set
> +                 * REGBIT_LOOKUP_NEIGHBOR_IP_RESULT to 1. */
> +                ds_clear(&match);
> +                ds_put_format(&match,
> +                              "inport == %s && arp.spa == %s/%u && "
> +                              "arp.tpa == %s && arp.op == 1",
> +                              op->json_key,
> +                              op->lrp_networks.ipv4_addrs[i].network_s,
> +                              op->lrp_networks.ipv4_addrs[i].plen,
> +                              op->lrp_networks.ipv4_addrs[i].addr_s);
> +                if (op->od->l3dgw_port && op == op->od->l3dgw_port
> +                    && op->od->l3redirect_port) {
> +                    ds_put_format(&match, " && is_chassis_resident(%s)",
> +                                  op->od->l3redirect_port->json_key);
> +                }
> +                const char *actions_s = REGBIT_LOOKUP_NEIGHBOR_RESULT
> +                                  " = lookup_arp(inport, arp.spa, arp.sha); "
> +                                  REGBIT_LOOKUP_NEIGHBOR_IP_RESULT" = 1;"
> +                                  " next;";
> +                ovn_lflow_add_with_hint(lflows, op->od,
> +                                        S_ROUTER_IN_LOOKUP_NEIGHBOR, 110,
> +                                        ds_cstr(&match), actions_s,
> +                                        &op->nbrp->header_);
> +            }
>               ds_clear(&match);
>               ds_put_format(&match,
>                             "inport == %s && arp.spa == %s/%u && arp.op == 1",
> @@ -8015,12 +8065,16 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>                   ds_put_format(&match, " && is_chassis_resident(%s)",
>                                 op->od->l3redirect_port->json_key);
>               }
> +            ds_clear(&actions);
> +            ds_put_format(&actions, REGBIT_LOOKUP_NEIGHBOR_RESULT
> +                          " = lookup_arp(inport, arp.spa, arp.sha); %snext;",
> +                          learn_from_arp_request? "":
> +                          REGBIT_LOOKUP_NEIGHBOR_IP_RESULT
> +                          " = lookup_arp_ip(inport, arp.spa); ");
>               ovn_lflow_add_with_hint(lflows, op->od,
>                                       S_ROUTER_IN_LOOKUP_NEIGHBOR, 100,
> -                                    ds_cstr(&match),
> -                                    REGBIT_LOOKUP_NEIGHBOR_RESULT" = "
> -                                    "lookup_arp(inport, arp.spa, arp.sha); "
> -                                    "next;", &op->nbrp->header_);
> +                                    ds_cstr(&match), ds_cstr(&actions),
> +                                    &op->nbrp->header_);
>           }
>       }
>   
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 12849b4..c7e1cdc 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -3967,6 +3967,18 @@ ip_to_hex() {
>   sha=f00000000011
>   spa=`ip_to_hex 192 168 1 100`
>   tpa=$spa
> +
> +# When learn_from_arp_request=false, the new mac-binding will not be learned
> +# through GARP request.
> +ovn-nbctl --wait=hv set logical_router lr0 options:learn_from_arp_request=false
> +
> +test_arp 11 $sha $spa $tpa
> +sleep 1
> +AT_CHECK([ovn-sbctl find mac_binding ip="192.168.1.100"], [0], [])
> +
> +# When learn_from_arp_request=true, the new mac-binding will be learned.
> +ovn-nbctl --wait=hv set logical_router lr0 options:learn_from_arp_request=true
> +
>   test_arp 11 $sha $spa $tpa
>   OVS_WAIT_UNTIL([test `ovn-sbctl find mac_binding ip="192.168.1.100" | wc -l` -gt 0])
>   ovn-nbctl --wait=hv sync
> @@ -3981,6 +3993,12 @@ test_ip 21 $smac $dmac $sip $dip 11
>   
>   # lp12 send GARP request to announce ownership of 192.168.1.100.
>   
> +# Even when learn_from_arp_request=false, the existing mac-binding should be
> +# updated through GARP request.
> +ovn-nbctl --wait=hv set logical_router lr0 options:learn_from_arp_request=false
> +ovn-sbctl lflow-list
> +as hv2 ovs-ofctl dump-flows br-int
> +
>   sha=f00000000012
>   test_arp 12 $sha $spa $tpa
>   OVS_WAIT_UNTIL([ovn-sbctl find mac_binding ip="192.168.1.100" | grep f0:00:00:00:00:12])
> 



More information about the dev mailing list