On Wed, Dec 2, 2020 at 12:31 AM Dumitru Ceara <[email protected]> wrote:
>
> 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 <[email protected]>
> > ---
>
> 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 <[email protected]>
Thanks Dumitru. I updated the tests as suggested and applied to master,
also backported to branch-20.09.
>
> 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
> >
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
