[ovs-dev] [PATCH ovn] pinctrl.c: Fix always_learn_from_arp_request for self created MAC_Bindings.

Dumitru Ceara dceara at redhat.com
Wed Dec 2 08:31:15 UTC 2020


On 12/2/20 8:45 AM, Han Zhou wrote:
> Even when the option always_learn_from_arp_request is set to false for a
> logical router, stale entries in MAC_Binding table should still be updated.
> (This behavior is defined in the ovn-nb(5) for the option.)
> 
> Fixes: fdf295d5eb3 ("pinctrl: Honor always_learn_from_arp_request for self created MAC_Bindings.")
> Signed-off-by: Han Zhou <hzhou at ovn.org>
> ---

Hi Han,

I had misinterpreted that always_learn_from_arp_request==false means
"never learn from arp request" instead of "only learn from arp request
if the IP was already known".

Thanks for the fix!

I only have a couple minor nits on the test case below, with those
addressed:

Acked-by: Dumitru Ceara <dceara at redhat.com>

Thanks,
Dumitru

>  controller/pinctrl.c | 20 +++++++++++---------
>  tests/ovn.at         | 11 ++++++++++-
>  2 files changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index c6540c109..7e3abf0a4 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -3861,7 +3861,8 @@ mac_binding_add(struct ovsdb_idl_txn *ovnsb_idl_txn,
>                  struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
>                  const char *logical_port,
>                  const struct sbrec_datapath_binding *dp,
> -                struct eth_addr ea, const char *ip)
> +                struct eth_addr ea, const char *ip,
> +                bool update_only)
>  {
>      /* Convert ethernet argument to string form for database. */
>      char mac_string[ETH_ADDR_STRLEN + 1];
> @@ -3870,6 +3871,9 @@ mac_binding_add(struct ovsdb_idl_txn *ovnsb_idl_txn,
>      const struct sbrec_mac_binding *b =
>          mac_binding_lookup(sbrec_mac_binding_by_lport_ip, logical_port, ip);
>      if (!b) {
> +        if (update_only) {
> +            return;
> +        }
>          b = sbrec_mac_binding_insert(ovnsb_idl_txn);
>          sbrec_mac_binding_set_logical_port(b, logical_port);
>          sbrec_mac_binding_set_ip(b, ip);
> @@ -3907,19 +3911,16 @@ send_garp_locally(struct ovsdb_idl_txn *ovnsb_idl_txn,
>              continue;
>          }
>  
> -        /* Skip datapaths that don't automatically learn ARPs from requests. */
> -        if (!smap_get_bool(&remote->datapath->external_ids,
> -                           "always_learn_from_arp_request",
> -                           true)) {
> -            continue;
> -        }
> +        bool update_only = !smap_get_bool(&remote->datapath->external_ids,
> +                                          "always_learn_from_arp_request",
> +                                          true);
>  
>          struct ds ip_s = DS_EMPTY_INITIALIZER;
>  
>          ip_format_masked(ip, OVS_BE32_MAX, &ip_s);
>          mac_binding_add(ovnsb_idl_txn, sbrec_mac_binding_by_lport_ip,
>                          remote->logical_port, remote->datapath,
> -                        ea, ds_cstr(&ip_s));
> +                        ea, ds_cstr(&ip_s), update_only);
>          ds_destroy(&ip_s);
>      }
>  }
> @@ -3951,7 +3952,8 @@ run_put_mac_binding(struct ovsdb_idl_txn *ovnsb_idl_txn,
>      struct ds ip_s = DS_EMPTY_INITIALIZER;
>      ipv6_format_mapped(&pmb->ip_key, &ip_s);
>      mac_binding_add(ovnsb_idl_txn, sbrec_mac_binding_by_lport_ip,
> -                    pb->logical_port, pb->datapath, pmb->mac, ds_cstr(&ip_s));
> +                    pb->logical_port, pb->datapath, pmb->mac, ds_cstr(&ip_s),
> +                    false);
>      ds_destroy(&ip_s);
>  }
>  
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 97e3e49e6..640ae85aa 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -15974,12 +15974,21 @@ check ovn-nbctl lr-nat-del lr1 dnat_and_snat 172.24.4.200
>  
>  check ovn-sbctl --all destroy mac_binding
>  
> +# Create a mac_binding entry on lr0-pub for 172.24.4.200 with a
> +# wrong mac, expecting it to be updated with the real mac.
> +lr0_dp=$(ovn-sbctl --bare --columns _uuid find datapath external_ids:name=lr0)

This could use the helper functions, e.g.:

lr0_dp=$(fetch_column Datapath_Binding _uuid external_ids:name=lr0)

> +ovn-sbctl create mac_binding datapath=$lr0_dp logical_port=lr0-pub \
> +    ip=172.24.4.200 mac=\"aa:aa:aa:aa:aa:aa\"
> +
>  check ovn-nbctl lr-nat-add lr0 dnat_and_snat 172.24.4.100 10.0.0.10
>  check ovn-nbctl lr-nat-add lr1 dnat_and_snat 172.24.4.200 20.0.0.10
>  check ovn-nbctl --wait=hv sync
>  
> -check_row_count MAC_Binding 0 logical_port=lr0-pub ip=172.24.4.200
> +check_row_count MAC_Binding 1 logical_port=lr0-pub ip=172.24.4.200
>  check_row_count MAC_Binding 0 logical_port=lr1-pub ip=172.24.4.100
> +AT_CHECK([ovn-sbctl --bare --columns mac \
> +         find mac_binding logical_port=lr0-pub ip=172.24.4.200], [0], [f0:00:00:00:01:01
> +])

Same here:
check_column "f0:00:00:00:01:01" MAC_Binding mac logical_port=lr0-pub
ip=172.24.4.200

>  
>  OVN_CLEANUP([hv1])
>  AT_CLEANUP
> 



More information about the dev mailing list