[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