diff mbox series

[ovs-dev,v2] ovn-controller-vtep: Fix MMR create/update

Message ID 20210528082105.70086-1-odivlad@gmail.com
State Superseded
Headers show
Series [ovs-dev,v2] ovn-controller-vtep: Fix MMR create/update | expand

Commit Message

Влад Одинцов May 28, 2021, 8:21 a.m. UTC
Before this patch ovn-controller-vtep created Mcast_Macs_Remote
record for each Port Binding in the ovn Logical Switch, to which
vtep Logical Switch was attached.
With this patch there is only one Mcast_Macs_Remote record per datapath.
Physical Locator set is created every time when physical locators for
associated datapath are changed. Next, this newly-created physical
locator set is updated in the MMR record.

Also, update logical switch's tunnel key and replication method only
if needed.

Signed-off-by: Vladislav Odintsov <odivlad@gmail.com>
---
 controller-vtep/vtep.c       | 66 +++++++++++++++++++++-------------
 tests/ovn-controller-vtep.at | 70 ++++++++++++++++++++++++++++++++++++
 2 files changed, 111 insertions(+), 25 deletions(-)

Comments

Dumitru Ceara June 10, 2021, 3:50 p.m. UTC | #1
On 5/28/21 10:21 AM, Vladislav Odintsov wrote:
> Before this patch ovn-controller-vtep created Mcast_Macs_Remote
> record for each Port Binding in the ovn Logical Switch, to which
> vtep Logical Switch was attached.
> With this patch there is only one Mcast_Macs_Remote record per datapath.
> Physical Locator set is created every time when physical locators for
> associated datapath are changed. Next, this newly-created physical
> locator set is updated in the MMR record.
> 
> Also, update logical switch's tunnel key and replication method only
> if needed.
> 
> Signed-off-by: Vladislav Odintsov <odivlad@gmail.com>
> ---

Hi Vladislav,

I'm not too familiar with the vtep code so the comments below are
somewhat general.  Thanks for adding a test!

>  controller-vtep/vtep.c       | 66 +++++++++++++++++++++-------------
>  tests/ovn-controller-vtep.at | 70 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 111 insertions(+), 25 deletions(-)
> 
> diff --git a/controller-vtep/vtep.c b/controller-vtep/vtep.c
> index f3b02f63f..92e6bec92 100644
> --- a/controller-vtep/vtep.c
> +++ b/controller-vtep/vtep.c
> @@ -94,7 +94,7 @@ create_pl(struct ovsdb_idl_txn *vtep_idl_txn, const char *chassis_ip)
>  
>      return new_pl;
>  }
> -
> +
>  /* Creates a new 'Mcast_Macs_Remote'. */
>  static void
>  vtep_create_mmr(struct ovsdb_idl_txn *vtep_idl_txn, const char *mac,
> @@ -104,6 +104,7 @@ vtep_create_mmr(struct ovsdb_idl_txn *vtep_idl_txn, const char *mac,
>      struct vteprec_mcast_macs_remote *new_mmr =
>         vteprec_mcast_macs_remote_insert(vtep_idl_txn);
>  
> +    VLOG_DBG("Inserting MMR for LS '%s'", vtep_ls->name);
>      vteprec_mcast_macs_remote_set_MAC(new_mmr, mac);
>      vteprec_mcast_macs_remote_set_logical_switch(new_mmr, vtep_ls);
>      vteprec_mcast_macs_remote_set_locator_set(new_mmr, ploc_set);
> @@ -140,7 +141,7 @@ vtep_process_pls(const struct ovs_list *locators_list,
>                             ploc_entry->vteprec_ploc;
>              if (mmr_ext && !shash_find_data(&mmr_ext->physical_locators,
>                                              locators[i]->dst_ip)) {
> -                    locator_lists_differ = true;
> +                locator_lists_differ = true;
>              }
>              i++;
>          }
> @@ -149,8 +150,8 @@ vtep_process_pls(const struct ovs_list *locators_list,
>      return locator_lists_differ;
>  }
>  
> -/* Creates a new 'Mcast_Macs_Remote' entry if needed and also cleans up
> - * out-dated remote mcast mac entries as needed. */
> +/* Creates a new 'Mcast_Macs_Remote' entry or modifies existing if needed
> + * and also cleans up out-dated remote mcast mac entries as needed. */
>  static void
>  vtep_update_mmr(struct ovsdb_idl_txn *vtep_idl_txn,
>                  struct ovs_list *locators_list,
> @@ -162,22 +163,29 @@ vtep_update_mmr(struct ovsdb_idl_txn *vtep_idl_txn,
>      bool mmr_changed;
>  
>      locators = xmalloc(n_locators_new * sizeof *locators);
> -
>      mmr_changed = vtep_process_pls(locators_list, mmr_ext, locators);
>  
> -    if (mmr_ext && !n_locators_new) {
> -        vteprec_mcast_macs_remote_delete(mmr_ext->mmr);
> -    } else if ((mmr_ext && mmr_changed) ||
> -               (!mmr_ext && n_locators_new)) {
> +    if (mmr_changed) {
> +        if (n_locators_new) {
> +            const struct vteprec_physical_locator_set *ploc_set =
> +                vteprec_physical_locator_set_insert(vtep_idl_txn);
>  
> -        const struct vteprec_physical_locator_set *ploc_set =
> -            vteprec_physical_locator_set_insert(vtep_idl_txn);
> +            vteprec_physical_locator_set_set_locators(ploc_set, locators,
> +                                                      n_locators_new);
>  
> -        vtep_create_mmr(vtep_idl_txn, "unknown-dst", vtep_ls, ploc_set);
> +            if (!mmr_ext) {  /* create new mmr */
> +                vtep_create_mmr(vtep_idl_txn, "unknown-dst", vtep_ls,
> +                                ploc_set);
> +            } else {  /* update existing mmr */
> +                vteprec_mcast_macs_remote_set_locator_set(mmr_ext->mmr,
> +                                                          ploc_set);
> +            }
>  
> -        vteprec_physical_locator_set_set_locators(ploc_set, locators,
> -                                                  n_locators_new);
> +        } else if (mmr_ext) {  /* remove old mmr */
> +            vteprec_mcast_macs_remote_delete(mmr_ext->mmr);
> +        }
>      }
> +
>      free(locators);
>  }
>  
> @@ -231,13 +239,19 @@ vtep_lswitch_run(struct shash *vtep_pbs, struct sset *vtep_pswitches,
>                  VLOG_DBG("set vtep logical switch (%s) tunnel key from "
>                           "(%"PRId64") to (%"PRId64")", vtep_ls->name,
>                           vtep_ls->tunnel_key[0], tnl_key);
> +                vteprec_logical_switch_set_tunnel_key(vtep_ls, &tnl_key, 1);
>              }
> -            vteprec_logical_switch_set_tunnel_key(vtep_ls, &tnl_key, 1);

This seems incorrect to me.  If the tunnel_key column is cleared
externally, with your change, we won't be resetting it.

>  
>              /* OVN is expected to always use source node replication mode,
>               * hence the replication mode is hard-coded for each logical
>               * switch in the context of ovn-controller-vtep. */
> -            vteprec_logical_switch_set_replication_mode(vtep_ls, "source_node");
> +            if (!vtep_ls->replication_mode
> +                || strcmp(vtep_ls->replication_mode, "source_node")) {
> +
> +                vteprec_logical_switch_set_replication_mode(vtep_ls,
> +                                                            "source_node");
> +            }
> +

This seems like an optimization at best and should probably be in a
separate patch.

>              sset_add(&used_ls, lswitch_name);
>          }
>      }
> @@ -353,17 +367,19 @@ vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn, struct shash *ucast_macs_rmts,
>              shash_add(&ls_node->physical_locators, chassis_ip, pl);
>          }
>  
> -        char *mac_tnlkey = xasprintf("%s_%"PRId64, "unknown-dst", tnl_key);
> -        ls_node->mmr_ext = shash_find_data(mcast_macs_rmts, mac_tnlkey);
> +        if (!ls_node->mmr_ext) {
> +            char *mac_tnlkey = xasprintf("%s_%"PRId64, "unknown-dst", tnl_key);
> +            ls_node->mmr_ext = shash_find_data(mcast_macs_rmts, mac_tnlkey);
>  
> -        if (ls_node->mmr_ext &&
> -            ls_node->mmr_ext->mmr->logical_switch == ls_node->vtep_ls) {
> +            if (ls_node->mmr_ext &&
> +                ls_node->mmr_ext->mmr->logical_switch == ls_node->vtep_ls) {
>  
> -            /* Delete the entry from the hash table so the mmr does not get
> -             * removed from the DB later on during stale checking. */
> -            shash_find_and_delete(mcast_macs_rmts, mac_tnlkey);
> +                /* Delete the entry from the hash table so the mmr does not get
> +                * removed from the DB later on during stale checking. */
> +                shash_find_and_delete(mcast_macs_rmts, mac_tnlkey);
> +            }
> +            free(mac_tnlkey);
>          }
> -        free(mac_tnlkey);
>  
>          for (i = 0; i < port_binding_rec->n_mac; i++) {
>              const struct vteprec_ucast_macs_remote *umr;
> @@ -535,7 +551,7 @@ vtep_run(struct controller_vtep_ctx *ctx)
>                        mmr->logical_switch && mmr->logical_switch->n_tunnel_key
>                            ? mmr->logical_switch->tunnel_key[0] : INT64_MAX);
>  
> -        shash_add(&mcast_macs_rmts, mac_tnlkey, mmr_ext);
> +        shash_add_once(&mcast_macs_rmts, mac_tnlkey, mmr_ext);
>          mmr_ext->mmr = mmr;
>  
>          shash_init(&mmr_ext->physical_locators);
> diff --git a/tests/ovn-controller-vtep.at b/tests/ovn-controller-vtep.at
> index b2261d285..d870e6e06 100644
> --- a/tests/ovn-controller-vtep.at
> +++ b/tests/ovn-controller-vtep.at
> @@ -415,6 +415,20 @@ AT_CHECK([vtep-ctl --columns=MAC list Ucast_Macs_Remote | cut -d ':' -f2- | tr -
>  "f0:ab:cd:ef:01:03"
>  ])
>  
> +# checks multiple vifs
> +# add new vif to br-test lswitch and check all UMRs exist
> +AT_CHECK([ovn-nbctl lsp-add br-test vif2])
> +AT_CHECK([ovn-nbctl lsp-set-addresses vif2 f0:ab:cd:ef:02:02])
> +AT_CHECK([ovn-nbctl --wait=sb sync])
> +AT_CHECK([ovn-sbctl chassis-add ch2 vxlan 1.2.3.7])
> +AT_CHECK([ovn-sbctl lsp-bind vif2 ch2])
> +AT_CHECK([vtep-ctl --columns=MAC list Ucast_Macs_Remote | cut -d ':' -f2- | tr -d ' ' | sort], [0], [dnl
> +
> +"f0:ab:cd:ef:01:03"
> +"f0:ab:cd:ef:02:02"
> +])
> +AT_CHECK([ovn-nbctl lsp-del vif2])
> +

There's a new "check" helper in ovn-macros.at, all these AT_CHECK calls
can be replaced by:

check *ctl ...

>  # migrates mac to logical switch port vif1 on 'br-void'.
>  AT_CHECK([ovn-nbctl lsp-set-addresses vif0])
>  AT_CHECK([ovn-nbctl --wait=sb lsp-set-addresses vif1 f0:ab:cd:ef:01:03])
> @@ -492,3 +506,59 @@ AT_CHECK([vtep-ctl --columns=MAC list Ucast_Macs_Remote | cut -d ':' -f2- | tr -
>  
>  OVN_CONTROLLER_VTEP_STOP([/has already been known to be on logical port/d])
>  AT_CLEANUP
> +
> +# Tests vtep module 'Mcast_Macs_Remote's.
> +AT_SETUP([ovn-controller-vtep - vtep-Mcast_Macs_Remote])
> +OVN_CONTROLLER_VTEP_START
> +
> +# creates a simple logical network with the vtep device and a fake hv chassis
> +# 'ch0'.
> +AT_CHECK([ovn-nbctl lsp-add br-test vif0])
> +AT_CHECK([ovn-nbctl lsp-set-addresses vif0 f0:ab:cd:ef:01:00])
> +AT_CHECK([ovn-nbctl --timeout=10 --wait=sb sync])

The timeout is not really required, ovn-nbctl will timeout after
OVS_CTL_TIMEOUT seconds (which is set in tests/atlocal).

> +AT_CHECK([ovn-sbctl chassis-add ch0 vxlan 1.2.3.5])
> +AT_CHECK([ovn-sbctl lsp-bind vif0 ch0])
> +
> +# creates the logical switch in vtep and adds the corresponding logical
> +# port to 'br-test'.
> +AT_CHECK([vtep-ctl add-ls lswitch0 -- bind-ls br-vtep p0 100 lswitch0])
> +OVN_NB_ADD_VTEP_PORT([br-test], [br-vtep_lswitch0], [br-vtep], [lswitch0])
> +OVS_WAIT_UNTIL([test -n "`ovn-sbctl list Port_Binding  | grep br-vtep_lswitch0`"])
> +
> +# checks Mcast_Macs_Remote creation.
> +OVS_WAIT_UNTIL([test `vtep-ctl list Mcast_Macs_Remote | grep _uuid | wc -l` -eq 1])
> +AT_CHECK([vtep-ctl --columns=MAC list Mcast_Macs_Remote | cut -d ':' -f2- | tr -d ' '], [0], [dnl
> +unknown-dst
> +])
> +
> +# check physical locator and physical locator set updates
> +OVS_WAIT_UNTIL([test -n "`vtep-ctl list Physical_Locator | grep _uuid`"])
> +AT_CHECK([for i in `vtep-ctl --columns=locators list Physical_Locator_Set $(vtep-ctl --columns=locator_set list Mcast_Macs_Remote | cut -d ':' -f2) | cut -d ':' -f2 | tr -d '[[ ]]' | sed 's/,/ /g'`; do
> +   vtep-ctl --columns=dst_ip list Physical_Locator $i | cut -d ':' -f2 | tr -d ' '
> +done], [0], [dnl
> +"1.2.3.5"
> +])
> +
> +# add new lport and bind it to another fake chassis 'ch1'.
> +AT_CHECK([ovn-nbctl lsp-add br-test vif1])
> +AT_CHECK([ovn-nbctl lsp-set-addresses vif0 f0:ab:cd:ef:01:01])
> +AT_CHECK([ovn-nbctl --timeout=10 --wait=sb sync])
> +AT_CHECK([ovn-sbctl chassis-add ch1 vxlan 1.2.3.6])
> +AT_CHECK([ovn-sbctl lsp-bind vif1 ch1])
> +
> +# checks there is still only one Mcast_Macs_Remote record.
> +OVS_WAIT_UNTIL([test `vtep-ctl list Mcast_Macs_Remote | grep _uuid | wc -l` -eq 1])
> +AT_CHECK([vtep-ctl --columns=MAC list Mcast_Macs_Remote | cut -d ':' -f2- | tr -d ' '], [0], [dnl
> +unknown-dst
> +])
> +
> +# check physical locator and physical locator set updates
> +AT_CHECK([for i in `vtep-ctl --columns=locators list Physical_Locator_Set $(vtep-ctl --columns=locator_set list Mcast_Macs_Remote | cut -d ':' -f2) | cut -d ':' -f2 | tr -d '[[ ]]' | sed 's/,/ /g'`; do
> +   vtep-ctl --columns=dst_ip list Physical_Locator $i | cut -d ':' -f2 | tr -d ' '
> +done | sort], [0], [dnl
> +"1.2.3.5"
> +"1.2.3.6"
> +])
> +
> +OVN_CONTROLLER_VTEP_STOP
> +AT_CLEANUP
> 

Regards,
Dumitru
Влад Одинцов June 10, 2021, 5:57 p.m. UTC | #2
Hi Dumitru,

First of all, thanks for the review. My comments are inline.

Regards,
Vladislav Odintsov

> On 10 Jun 2021, at 18:50, Dumitru Ceara <dceara@redhat.com> wrote:
> 
> On 5/28/21 10:21 AM, Vladislav Odintsov wrote:
>> Before this patch ovn-controller-vtep created Mcast_Macs_Remote
>> record for each Port Binding in the ovn Logical Switch, to which
>> vtep Logical Switch was attached.
>> With this patch there is only one Mcast_Macs_Remote record per datapath.
>> Physical Locator set is created every time when physical locators for
>> associated datapath are changed. Next, this newly-created physical
>> locator set is updated in the MMR record.
>> 
>> Also, update logical switch's tunnel key and replication method only
>> if needed.
>> 
>> Signed-off-by: Vladislav Odintsov <odivlad@gmail.com>
>> ---
> 
> Hi Vladislav,
> 
> I'm not too familiar with the vtep code so the comments below are
> somewhat general.  Thanks for adding a test!
> 
>> controller-vtep/vtep.c       | 66 +++++++++++++++++++++-------------
>> tests/ovn-controller-vtep.at | 70 ++++++++++++++++++++++++++++++++++++
>> 2 files changed, 111 insertions(+), 25 deletions(-)
>> 
>> diff --git a/controller-vtep/vtep.c b/controller-vtep/vtep.c
>> index f3b02f63f..92e6bec92 100644
>> --- a/controller-vtep/vtep.c
>> +++ b/controller-vtep/vtep.c
>> @@ -94,7 +94,7 @@ create_pl(struct ovsdb_idl_txn *vtep_idl_txn, const char *chassis_ip)
>> 
>>     return new_pl;
>> }
>> -
>> +
>> /* Creates a new 'Mcast_Macs_Remote'. */
>> static void
>> vtep_create_mmr(struct ovsdb_idl_txn *vtep_idl_txn, const char *mac,
>> @@ -104,6 +104,7 @@ vtep_create_mmr(struct ovsdb_idl_txn *vtep_idl_txn, const char *mac,
>>     struct vteprec_mcast_macs_remote *new_mmr =
>>        vteprec_mcast_macs_remote_insert(vtep_idl_txn);
>> 
>> +    VLOG_DBG("Inserting MMR for LS '%s'", vtep_ls->name);
>>     vteprec_mcast_macs_remote_set_MAC(new_mmr, mac);
>>     vteprec_mcast_macs_remote_set_logical_switch(new_mmr, vtep_ls);
>>     vteprec_mcast_macs_remote_set_locator_set(new_mmr, ploc_set);
>> @@ -140,7 +141,7 @@ vtep_process_pls(const struct ovs_list *locators_list,
>>                            ploc_entry->vteprec_ploc;
>>             if (mmr_ext && !shash_find_data(&mmr_ext->physical_locators,
>>                                             locators[i]->dst_ip)) {
>> -                    locator_lists_differ = true;
>> +                locator_lists_differ = true;
>>             }
>>             i++;
>>         }
>> @@ -149,8 +150,8 @@ vtep_process_pls(const struct ovs_list *locators_list,
>>     return locator_lists_differ;
>> }
>> 
>> -/* Creates a new 'Mcast_Macs_Remote' entry if needed and also cleans up
>> - * out-dated remote mcast mac entries as needed. */
>> +/* Creates a new 'Mcast_Macs_Remote' entry or modifies existing if needed
>> + * and also cleans up out-dated remote mcast mac entries as needed. */
>> static void
>> vtep_update_mmr(struct ovsdb_idl_txn *vtep_idl_txn,
>>                 struct ovs_list *locators_list,
>> @@ -162,22 +163,29 @@ vtep_update_mmr(struct ovsdb_idl_txn *vtep_idl_txn,
>>     bool mmr_changed;
>> 
>>     locators = xmalloc(n_locators_new * sizeof *locators);
>> -
>>     mmr_changed = vtep_process_pls(locators_list, mmr_ext, locators);
>> 
>> -    if (mmr_ext && !n_locators_new) {
>> -        vteprec_mcast_macs_remote_delete(mmr_ext->mmr);
>> -    } else if ((mmr_ext && mmr_changed) ||
>> -               (!mmr_ext && n_locators_new)) {
>> +    if (mmr_changed) {
>> +        if (n_locators_new) {
>> +            const struct vteprec_physical_locator_set *ploc_set =
>> +                vteprec_physical_locator_set_insert(vtep_idl_txn);
>> 
>> -        const struct vteprec_physical_locator_set *ploc_set =
>> -            vteprec_physical_locator_set_insert(vtep_idl_txn);
>> +            vteprec_physical_locator_set_set_locators(ploc_set, locators,
>> +                                                      n_locators_new);
>> 
>> -        vtep_create_mmr(vtep_idl_txn, "unknown-dst", vtep_ls, ploc_set);
>> +            if (!mmr_ext) {  /* create new mmr */
>> +                vtep_create_mmr(vtep_idl_txn, "unknown-dst", vtep_ls,
>> +                                ploc_set);
>> +            } else {  /* update existing mmr */
>> +                vteprec_mcast_macs_remote_set_locator_set(mmr_ext->mmr,
>> +                                                          ploc_set);
>> +            }
>> 
>> -        vteprec_physical_locator_set_set_locators(ploc_set, locators,
>> -                                                  n_locators_new);
>> +        } else if (mmr_ext) {  /* remove old mmr */
>> +            vteprec_mcast_macs_remote_delete(mmr_ext->mmr);
>> +        }
>>     }
>> +
>>     free(locators);
>> }
>> 
>> @@ -231,13 +239,19 @@ vtep_lswitch_run(struct shash *vtep_pbs, struct sset *vtep_pswitches,
>>                 VLOG_DBG("set vtep logical switch (%s) tunnel key from "
>>                          "(%"PRId64") to (%"PRId64")", vtep_ls->name,
>>                          vtep_ls->tunnel_key[0], tnl_key);
>> +                vteprec_logical_switch_set_tunnel_key(vtep_ls, &tnl_key, 1);
>>             }
>> -            vteprec_logical_switch_set_tunnel_key(vtep_ls, &tnl_key, 1);
> 
> This seems incorrect to me.  If the tunnel_key column is cleared
> externally, with your change, we won't be resetting it.

There is a check if (vtep_ls->tunnel_key[0] != port_binding_rec->datapath->tunnel_key).
So if it differs for some reason, vtep-controller will rewrite it. I’ve additionally checked this:
Set via vtep-ctl set logical-switch <uuid> tunnel_key=<another id>, then checked a value
(It returned to previous one) and in debug log there was:

2021-06-10T17:39:11Z|00131|vtep|DBG|set vtep logical switch (subnet-789C6560) tunnel key from (11) to (10)

Maybe I didn’t get the idea of incorrect behaviour?

> 
>> 
>>             /* OVN is expected to always use source node replication mode,
>>              * hence the replication mode is hard-coded for each logical
>>              * switch in the context of ovn-controller-vtep. */
>> -            vteprec_logical_switch_set_replication_mode(vtep_ls, "source_node");
>> +            if (!vtep_ls->replication_mode
>> +                || strcmp(vtep_ls->replication_mode, "source_node")) {
>> +
>> +                vteprec_logical_switch_set_replication_mode(vtep_ls,
>> +                                                            "source_node");
>> +            }
>> +
> 
> This seems like an optimization at best and should probably be in a
> separate patch.

Okay, I’ll address this in v3.
If upper vteprec_logical_switch_set_tunnel_key will remain in the patch series, I’ll move it to separate patch as well.

> 
>>             sset_add(&used_ls, lswitch_name);
>>         }
>>     }
>> @@ -353,17 +367,19 @@ vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn, struct shash *ucast_macs_rmts,
>>             shash_add(&ls_node->physical_locators, chassis_ip, pl);
>>         }
>> 
>> -        char *mac_tnlkey = xasprintf("%s_%"PRId64, "unknown-dst", tnl_key);
>> -        ls_node->mmr_ext = shash_find_data(mcast_macs_rmts, mac_tnlkey);
>> +        if (!ls_node->mmr_ext) {
>> +            char *mac_tnlkey = xasprintf("%s_%"PRId64, "unknown-dst", tnl_key);
>> +            ls_node->mmr_ext = shash_find_data(mcast_macs_rmts, mac_tnlkey);
>> 
>> -        if (ls_node->mmr_ext &&
>> -            ls_node->mmr_ext->mmr->logical_switch == ls_node->vtep_ls) {
>> +            if (ls_node->mmr_ext &&
>> +                ls_node->mmr_ext->mmr->logical_switch == ls_node->vtep_ls) {
>> 
>> -            /* Delete the entry from the hash table so the mmr does not get
>> -             * removed from the DB later on during stale checking. */
>> -            shash_find_and_delete(mcast_macs_rmts, mac_tnlkey);
>> +                /* Delete the entry from the hash table so the mmr does not get
>> +                * removed from the DB later on during stale checking. */
>> +                shash_find_and_delete(mcast_macs_rmts, mac_tnlkey);
>> +            }
>> +            free(mac_tnlkey);
>>         }
>> -        free(mac_tnlkey);
>> 
>>         for (i = 0; i < port_binding_rec->n_mac; i++) {
>>             const struct vteprec_ucast_macs_remote *umr;
>> @@ -535,7 +551,7 @@ vtep_run(struct controller_vtep_ctx *ctx)
>>                       mmr->logical_switch && mmr->logical_switch->n_tunnel_key
>>                           ? mmr->logical_switch->tunnel_key[0] : INT64_MAX);
>> 
>> -        shash_add(&mcast_macs_rmts, mac_tnlkey, mmr_ext);
>> +        shash_add_once(&mcast_macs_rmts, mac_tnlkey, mmr_ext);
>>         mmr_ext->mmr = mmr;
>> 
>>         shash_init(&mmr_ext->physical_locators);
>> diff --git a/tests/ovn-controller-vtep.at b/tests/ovn-controller-vtep.at
>> index b2261d285..d870e6e06 100644
>> --- a/tests/ovn-controller-vtep.at
>> +++ b/tests/ovn-controller-vtep.at
>> @@ -415,6 +415,20 @@ AT_CHECK([vtep-ctl --columns=MAC list Ucast_Macs_Remote | cut -d ':' -f2- | tr -
>> "f0:ab:cd:ef:01:03"
>> ])
>> 
>> +# checks multiple vifs
>> +# add new vif to br-test lswitch and check all UMRs exist
>> +AT_CHECK([ovn-nbctl lsp-add br-test vif2])
>> +AT_CHECK([ovn-nbctl lsp-set-addresses vif2 f0:ab:cd:ef:02:02])
>> +AT_CHECK([ovn-nbctl --wait=sb sync])
>> +AT_CHECK([ovn-sbctl chassis-add ch2 vxlan 1.2.3.7])
>> +AT_CHECK([ovn-sbctl lsp-bind vif2 ch2])
>> +AT_CHECK([vtep-ctl --columns=MAC list Ucast_Macs_Remote | cut -d ':' -f2- | tr -d ' ' | sort], [0], [dnl
>> +
>> +"f0:ab:cd:ef:01:03"
>> +"f0:ab:cd:ef:02:02"
>> +])
>> +AT_CHECK([ovn-nbctl lsp-del vif2])
>> +
> 
> There's a new "check" helper in ovn-macros.at, all these AT_CHECK calls
> can be replaced by:
> 
> check *ctl ...

In v3.


> 
>> # migrates mac to logical switch port vif1 on 'br-void'.
>> AT_CHECK([ovn-nbctl lsp-set-addresses vif0])
>> AT_CHECK([ovn-nbctl --wait=sb lsp-set-addresses vif1 f0:ab:cd:ef:01:03])
>> @@ -492,3 +506,59 @@ AT_CHECK([vtep-ctl --columns=MAC list Ucast_Macs_Remote | cut -d ':' -f2- | tr -
>> 
>> OVN_CONTROLLER_VTEP_STOP([/has already been known to be on logical port/d])
>> AT_CLEANUP
>> +
>> +# Tests vtep module 'Mcast_Macs_Remote's.
>> +AT_SETUP([ovn-controller-vtep - vtep-Mcast_Macs_Remote])
>> +OVN_CONTROLLER_VTEP_START
>> +
>> +# creates a simple logical network with the vtep device and a fake hv chassis
>> +# 'ch0'.
>> +AT_CHECK([ovn-nbctl lsp-add br-test vif0])
>> +AT_CHECK([ovn-nbctl lsp-set-addresses vif0 f0:ab:cd:ef:01:00])
>> +AT_CHECK([ovn-nbctl --timeout=10 --wait=sb sync])
> 
> The timeout is not really required, ovn-nbctl will timeout after
> OVS_CTL_TIMEOUT seconds (which is set in tests/atlocal).

It was just a copy-paste from neighbouring test cases. I’ll remove it from my ones. Thanks.

> 
>> +AT_CHECK([ovn-sbctl chassis-add ch0 vxlan 1.2.3.5])
>> +AT_CHECK([ovn-sbctl lsp-bind vif0 ch0])
>> +
>> +# creates the logical switch in vtep and adds the corresponding logical
>> +# port to 'br-test'.
>> +AT_CHECK([vtep-ctl add-ls lswitch0 -- bind-ls br-vtep p0 100 lswitch0])
>> +OVN_NB_ADD_VTEP_PORT([br-test], [br-vtep_lswitch0], [br-vtep], [lswitch0])
>> +OVS_WAIT_UNTIL([test -n "`ovn-sbctl list Port_Binding  | grep br-vtep_lswitch0`"])
>> +
>> +# checks Mcast_Macs_Remote creation.
>> +OVS_WAIT_UNTIL([test `vtep-ctl list Mcast_Macs_Remote | grep _uuid | wc -l` -eq 1])
>> +AT_CHECK([vtep-ctl --columns=MAC list Mcast_Macs_Remote | cut -d ':' -f2- | tr -d ' '], [0], [dnl
>> +unknown-dst
>> +])
>> +
>> +# check physical locator and physical locator set updates
>> +OVS_WAIT_UNTIL([test -n "`vtep-ctl list Physical_Locator | grep _uuid`"])
>> +AT_CHECK([for i in `vtep-ctl --columns=locators list Physical_Locator_Set $(vtep-ctl --columns=locator_set list Mcast_Macs_Remote | cut -d ':' -f2) | cut -d ':' -f2 | tr -d '[[ ]]' | sed 's/,/ /g'`; do
>> +   vtep-ctl --columns=dst_ip list Physical_Locator $i | cut -d ':' -f2 | tr -d ' '
>> +done], [0], [dnl
>> +"1.2.3.5"
>> +])
>> +
>> +# add new lport and bind it to another fake chassis 'ch1'.
>> +AT_CHECK([ovn-nbctl lsp-add br-test vif1])
>> +AT_CHECK([ovn-nbctl lsp-set-addresses vif0 f0:ab:cd:ef:01:01])
>> +AT_CHECK([ovn-nbctl --timeout=10 --wait=sb sync])
>> +AT_CHECK([ovn-sbctl chassis-add ch1 vxlan 1.2.3.6])
>> +AT_CHECK([ovn-sbctl lsp-bind vif1 ch1])
>> +
>> +# checks there is still only one Mcast_Macs_Remote record.
>> +OVS_WAIT_UNTIL([test `vtep-ctl list Mcast_Macs_Remote | grep _uuid | wc -l` -eq 1])
>> +AT_CHECK([vtep-ctl --columns=MAC list Mcast_Macs_Remote | cut -d ':' -f2- | tr -d ' '], [0], [dnl
>> +unknown-dst
>> +])
>> +
>> +# check physical locator and physical locator set updates
>> +AT_CHECK([for i in `vtep-ctl --columns=locators list Physical_Locator_Set $(vtep-ctl --columns=locator_set list Mcast_Macs_Remote | cut -d ':' -f2) | cut -d ':' -f2 | tr -d '[[ ]]' | sed 's/,/ /g'`; do
>> +   vtep-ctl --columns=dst_ip list Physical_Locator $i | cut -d ':' -f2 | tr -d ' '
>> +done | sort], [0], [dnl
>> +"1.2.3.5"
>> +"1.2.3.6"
>> +])
>> +
>> +OVN_CONTROLLER_VTEP_STOP
>> +AT_CLEANUP
>> 
> 
> Regards,
> Dumitru
Dumitru Ceara June 10, 2021, 6:18 p.m. UTC | #3
On 6/10/21 7:57 PM, Vladislav Odintsov wrote:

[...]

>>>
>>> @@ -231,13 +239,19 @@ vtep_lswitch_run(struct shash *vtep_pbs, struct sset *vtep_pswitches,
>>>                 VLOG_DBG("set vtep logical switch (%s) tunnel key from "
>>>                          "(%"PRId64") to (%"PRId64")", vtep_ls->name,
>>>                          vtep_ls->tunnel_key[0], tnl_key);
>>> +                vteprec_logical_switch_set_tunnel_key(vtep_ls, &tnl_key, 1);
>>>             }
>>> -            vteprec_logical_switch_set_tunnel_key(vtep_ls, &tnl_key, 1);
>>
>> This seems incorrect to me.  If the tunnel_key column is cleared
>> externally, with your change, we won't be resetting it.
> 
> There is a check if (vtep_ls->tunnel_key[0] != port_binding_rec->datapath->tunnel_key).
> So if it differs for some reason, vtep-controller will rewrite it. I’ve additionally checked this:
> Set via vtep-ctl set logical-switch <uuid> tunnel_key=<another id>, then checked a value
> (It returned to previous one) and in debug log there was:
> 
> 2021-06-10T17:39:11Z|00131|vtep|DBG|set vtep logical switch (subnet-789C6560) tunnel key from (11) to (10)
> 
> Maybe I didn’t get the idea of incorrect behaviour?
> 

If you run "vtep-ctl clear logical-switch <uuid> tunnel_key" with your
change then the tunnel_key will not be reset by ovn-controller-vtep.

Thanks,
Dumitru
Влад Одинцов June 10, 2021, 9:28 p.m. UTC | #4
Oh, I didn’t think about this case. I’ll address this too.

A small question.
After patch would be accepted I’d like to send backport bugfix patch down to
supported branches. Is it better to use old-style AT_CHECK in tests to cleanly
apply this patch to branches without `check` support? Or in master branch I
should only use new-style and then rewrite it while backporting?


Regards,

Vladislav Odintsov

> On 10 Jun 2021, at 21:18, Dumitru Ceara <dceara@redhat.com> wrote:
> 
> On 6/10/21 7:57 PM, Vladislav Odintsov wrote:
> 
> [...]
> 
>>>> 
>>>> @@ -231,13 +239,19 @@ vtep_lswitch_run(struct shash *vtep_pbs, struct sset *vtep_pswitches,
>>>>                VLOG_DBG("set vtep logical switch (%s) tunnel key from "
>>>>                         "(%"PRId64") to (%"PRId64")", vtep_ls->name,
>>>>                         vtep_ls->tunnel_key[0], tnl_key);
>>>> +                vteprec_logical_switch_set_tunnel_key(vtep_ls, &tnl_key, 1);
>>>>            }
>>>> -            vteprec_logical_switch_set_tunnel_key(vtep_ls, &tnl_key, 1);
>>> 
>>> This seems incorrect to me.  If the tunnel_key column is cleared
>>> externally, with your change, we won't be resetting it.
>> 
>> There is a check if (vtep_ls->tunnel_key[0] != port_binding_rec->datapath->tunnel_key).
>> So if it differs for some reason, vtep-controller will rewrite it. I’ve additionally checked this:
>> Set via vtep-ctl set logical-switch <uuid> tunnel_key=<another id>, then checked a value
>> (It returned to previous one) and in debug log there was:
>> 
>> 2021-06-10T17:39:11Z|00131|vtep|DBG|set vtep logical switch (subnet-789C6560) tunnel key from (11) to (10)
>> 
>> Maybe I didn’t get the idea of incorrect behaviour?
>> 
> 
> If you run "vtep-ctl clear logical-switch <uuid> tunnel_key" with your
> change then the tunnel_key will not be reset by ovn-controller-vtep.
> 
> Thanks,
> Dumitru
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Ben Pfaff June 10, 2021, 9:47 p.m. UTC | #5
On Fri, Jun 11, 2021 at 12:28:47AM +0300, Vladislav Odintsov wrote:
> After patch would be accepted I’d like to send backport bugfix patch down to
> supported branches. Is it better to use old-style AT_CHECK in tests to cleanly
> apply this patch to branches without `check` support? Or in master branch I
> should only use new-style and then rewrite it while backporting?

One reasonable option for backporting is to add the definition of
check().  It's a really simple shell function:

    check() {
        echo "$@"
        AT_CHECK(["$@"])
    }

If you're only changing a few tests and you didn't want to update
ovn-macros.at (I see that the commit that added check() is pretty big),
then you could just add that definition to each one.
Dumitru Ceara June 10, 2021, 10:13 p.m. UTC | #6
On 6/10/21 11:47 PM, Ben Pfaff wrote:
> On Fri, Jun 11, 2021 at 12:28:47AM +0300, Vladislav Odintsov wrote:
>> After patch would be accepted I’d like to send backport bugfix patch down to
>> supported branches. Is it better to use old-style AT_CHECK in tests to cleanly
>> apply this patch to branches without `check` support? Or in master branch I
>> should only use new-style and then rewrite it while backporting?
> 
> One reasonable option for backporting is to add the definition of
> check().  It's a really simple shell function:
> 
>     check() {
>         echo "$@"
>         AT_CHECK(["$@"])
>     }
> 
> If you're only changing a few tests and you didn't want to update
> ovn-macros.at (I see that the commit that added check() is pretty big),
> then you could just add that definition to each one.
> 

Luckily Numan already backported commit 4afe409e95c7 ("tests: Introduce
new testing helpers.") to all stable branches so check() is available on
all branches >= 20.03.
Ben Pfaff June 10, 2021, 10:45 p.m. UTC | #7
On Fri, Jun 11, 2021 at 12:13:46AM +0200, Dumitru Ceara wrote:
> On 6/10/21 11:47 PM, Ben Pfaff wrote:
> > On Fri, Jun 11, 2021 at 12:28:47AM +0300, Vladislav Odintsov wrote:
> >> After patch would be accepted I’d like to send backport bugfix patch down to
> >> supported branches. Is it better to use old-style AT_CHECK in tests to cleanly
> >> apply this patch to branches without `check` support? Or in master branch I
> >> should only use new-style and then rewrite it while backporting?
> > 
> > One reasonable option for backporting is to add the definition of
> > check().  It's a really simple shell function:
> > 
> >     check() {
> >         echo "$@"
> >         AT_CHECK(["$@"])
> >     }
> > 
> > If you're only changing a few tests and you didn't want to update
> > ovn-macros.at (I see that the commit that added check() is pretty big),
> > then you could just add that definition to each one.
> > 
> 
> Luckily Numan already backported commit 4afe409e95c7 ("tests: Introduce
> new testing helpers.") to all stable branches so check() is available on
> all branches >= 20.03.
> 

Even better.
Влад Одинцов June 15, 2021, 9:07 a.m. UTC | #8
Hi,

I’ve posted a new version of this patch: https://patchwork.ozlabs.org/project/ovn/list/?series=248346

Regards,
Vladislav Odintsov

> On 11 Jun 2021, at 01:45, Ben Pfaff <blp@ovn.org> wrote:
> 
> On Fri, Jun 11, 2021 at 12:13:46AM +0200, Dumitru Ceara wrote:
>> On 6/10/21 11:47 PM, Ben Pfaff wrote:
>>> On Fri, Jun 11, 2021 at 12:28:47AM +0300, Vladislav Odintsov wrote:
>>>> After patch would be accepted I’d like to send backport bugfix patch down to
>>>> supported branches. Is it better to use old-style AT_CHECK in tests to cleanly
>>>> apply this patch to branches without `check` support? Or in master branch I
>>>> should only use new-style and then rewrite it while backporting?
>>> 
>>> One reasonable option for backporting is to add the definition of
>>> check().  It's a really simple shell function:
>>> 
>>>    check() {
>>>        echo "$@"
>>>        AT_CHECK(["$@"])
>>>    }
>>> 
>>> If you're only changing a few tests and you didn't want to update
>>> ovn-macros.at (I see that the commit that added check() is pretty big),
>>> then you could just add that definition to each one.
>>> 
>> 
>> Luckily Numan already backported commit 4afe409e95c7 ("tests: Introduce
>> new testing helpers.") to all stable branches so check() is available on
>> all branches >= 20.03.
>> 
> 
> Even better.
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff mbox series

Patch

diff --git a/controller-vtep/vtep.c b/controller-vtep/vtep.c
index f3b02f63f..92e6bec92 100644
--- a/controller-vtep/vtep.c
+++ b/controller-vtep/vtep.c
@@ -94,7 +94,7 @@  create_pl(struct ovsdb_idl_txn *vtep_idl_txn, const char *chassis_ip)
 
     return new_pl;
 }
-
+
 /* Creates a new 'Mcast_Macs_Remote'. */
 static void
 vtep_create_mmr(struct ovsdb_idl_txn *vtep_idl_txn, const char *mac,
@@ -104,6 +104,7 @@  vtep_create_mmr(struct ovsdb_idl_txn *vtep_idl_txn, const char *mac,
     struct vteprec_mcast_macs_remote *new_mmr =
        vteprec_mcast_macs_remote_insert(vtep_idl_txn);
 
+    VLOG_DBG("Inserting MMR for LS '%s'", vtep_ls->name);
     vteprec_mcast_macs_remote_set_MAC(new_mmr, mac);
     vteprec_mcast_macs_remote_set_logical_switch(new_mmr, vtep_ls);
     vteprec_mcast_macs_remote_set_locator_set(new_mmr, ploc_set);
@@ -140,7 +141,7 @@  vtep_process_pls(const struct ovs_list *locators_list,
                            ploc_entry->vteprec_ploc;
             if (mmr_ext && !shash_find_data(&mmr_ext->physical_locators,
                                             locators[i]->dst_ip)) {
-                    locator_lists_differ = true;
+                locator_lists_differ = true;
             }
             i++;
         }
@@ -149,8 +150,8 @@  vtep_process_pls(const struct ovs_list *locators_list,
     return locator_lists_differ;
 }
 
-/* Creates a new 'Mcast_Macs_Remote' entry if needed and also cleans up
- * out-dated remote mcast mac entries as needed. */
+/* Creates a new 'Mcast_Macs_Remote' entry or modifies existing if needed
+ * and also cleans up out-dated remote mcast mac entries as needed. */
 static void
 vtep_update_mmr(struct ovsdb_idl_txn *vtep_idl_txn,
                 struct ovs_list *locators_list,
@@ -162,22 +163,29 @@  vtep_update_mmr(struct ovsdb_idl_txn *vtep_idl_txn,
     bool mmr_changed;
 
     locators = xmalloc(n_locators_new * sizeof *locators);
-
     mmr_changed = vtep_process_pls(locators_list, mmr_ext, locators);
 
-    if (mmr_ext && !n_locators_new) {
-        vteprec_mcast_macs_remote_delete(mmr_ext->mmr);
-    } else if ((mmr_ext && mmr_changed) ||
-               (!mmr_ext && n_locators_new)) {
+    if (mmr_changed) {
+        if (n_locators_new) {
+            const struct vteprec_physical_locator_set *ploc_set =
+                vteprec_physical_locator_set_insert(vtep_idl_txn);
 
-        const struct vteprec_physical_locator_set *ploc_set =
-            vteprec_physical_locator_set_insert(vtep_idl_txn);
+            vteprec_physical_locator_set_set_locators(ploc_set, locators,
+                                                      n_locators_new);
 
-        vtep_create_mmr(vtep_idl_txn, "unknown-dst", vtep_ls, ploc_set);
+            if (!mmr_ext) {  /* create new mmr */
+                vtep_create_mmr(vtep_idl_txn, "unknown-dst", vtep_ls,
+                                ploc_set);
+            } else {  /* update existing mmr */
+                vteprec_mcast_macs_remote_set_locator_set(mmr_ext->mmr,
+                                                          ploc_set);
+            }
 
-        vteprec_physical_locator_set_set_locators(ploc_set, locators,
-                                                  n_locators_new);
+        } else if (mmr_ext) {  /* remove old mmr */
+            vteprec_mcast_macs_remote_delete(mmr_ext->mmr);
+        }
     }
+
     free(locators);
 }
 
@@ -231,13 +239,19 @@  vtep_lswitch_run(struct shash *vtep_pbs, struct sset *vtep_pswitches,
                 VLOG_DBG("set vtep logical switch (%s) tunnel key from "
                          "(%"PRId64") to (%"PRId64")", vtep_ls->name,
                          vtep_ls->tunnel_key[0], tnl_key);
+                vteprec_logical_switch_set_tunnel_key(vtep_ls, &tnl_key, 1);
             }
-            vteprec_logical_switch_set_tunnel_key(vtep_ls, &tnl_key, 1);
 
             /* OVN is expected to always use source node replication mode,
              * hence the replication mode is hard-coded for each logical
              * switch in the context of ovn-controller-vtep. */
-            vteprec_logical_switch_set_replication_mode(vtep_ls, "source_node");
+            if (!vtep_ls->replication_mode
+                || strcmp(vtep_ls->replication_mode, "source_node")) {
+
+                vteprec_logical_switch_set_replication_mode(vtep_ls,
+                                                            "source_node");
+            }
+
             sset_add(&used_ls, lswitch_name);
         }
     }
@@ -353,17 +367,19 @@  vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn, struct shash *ucast_macs_rmts,
             shash_add(&ls_node->physical_locators, chassis_ip, pl);
         }
 
-        char *mac_tnlkey = xasprintf("%s_%"PRId64, "unknown-dst", tnl_key);
-        ls_node->mmr_ext = shash_find_data(mcast_macs_rmts, mac_tnlkey);
+        if (!ls_node->mmr_ext) {
+            char *mac_tnlkey = xasprintf("%s_%"PRId64, "unknown-dst", tnl_key);
+            ls_node->mmr_ext = shash_find_data(mcast_macs_rmts, mac_tnlkey);
 
-        if (ls_node->mmr_ext &&
-            ls_node->mmr_ext->mmr->logical_switch == ls_node->vtep_ls) {
+            if (ls_node->mmr_ext &&
+                ls_node->mmr_ext->mmr->logical_switch == ls_node->vtep_ls) {
 
-            /* Delete the entry from the hash table so the mmr does not get
-             * removed from the DB later on during stale checking. */
-            shash_find_and_delete(mcast_macs_rmts, mac_tnlkey);
+                /* Delete the entry from the hash table so the mmr does not get
+                * removed from the DB later on during stale checking. */
+                shash_find_and_delete(mcast_macs_rmts, mac_tnlkey);
+            }
+            free(mac_tnlkey);
         }
-        free(mac_tnlkey);
 
         for (i = 0; i < port_binding_rec->n_mac; i++) {
             const struct vteprec_ucast_macs_remote *umr;
@@ -535,7 +551,7 @@  vtep_run(struct controller_vtep_ctx *ctx)
                       mmr->logical_switch && mmr->logical_switch->n_tunnel_key
                           ? mmr->logical_switch->tunnel_key[0] : INT64_MAX);
 
-        shash_add(&mcast_macs_rmts, mac_tnlkey, mmr_ext);
+        shash_add_once(&mcast_macs_rmts, mac_tnlkey, mmr_ext);
         mmr_ext->mmr = mmr;
 
         shash_init(&mmr_ext->physical_locators);
diff --git a/tests/ovn-controller-vtep.at b/tests/ovn-controller-vtep.at
index b2261d285..d870e6e06 100644
--- a/tests/ovn-controller-vtep.at
+++ b/tests/ovn-controller-vtep.at
@@ -415,6 +415,20 @@  AT_CHECK([vtep-ctl --columns=MAC list Ucast_Macs_Remote | cut -d ':' -f2- | tr -
 "f0:ab:cd:ef:01:03"
 ])
 
+# checks multiple vifs
+# add new vif to br-test lswitch and check all UMRs exist
+AT_CHECK([ovn-nbctl lsp-add br-test vif2])
+AT_CHECK([ovn-nbctl lsp-set-addresses vif2 f0:ab:cd:ef:02:02])
+AT_CHECK([ovn-nbctl --wait=sb sync])
+AT_CHECK([ovn-sbctl chassis-add ch2 vxlan 1.2.3.7])
+AT_CHECK([ovn-sbctl lsp-bind vif2 ch2])
+AT_CHECK([vtep-ctl --columns=MAC list Ucast_Macs_Remote | cut -d ':' -f2- | tr -d ' ' | sort], [0], [dnl
+
+"f0:ab:cd:ef:01:03"
+"f0:ab:cd:ef:02:02"
+])
+AT_CHECK([ovn-nbctl lsp-del vif2])
+
 # migrates mac to logical switch port vif1 on 'br-void'.
 AT_CHECK([ovn-nbctl lsp-set-addresses vif0])
 AT_CHECK([ovn-nbctl --wait=sb lsp-set-addresses vif1 f0:ab:cd:ef:01:03])
@@ -492,3 +506,59 @@  AT_CHECK([vtep-ctl --columns=MAC list Ucast_Macs_Remote | cut -d ':' -f2- | tr -
 
 OVN_CONTROLLER_VTEP_STOP([/has already been known to be on logical port/d])
 AT_CLEANUP
+
+# Tests vtep module 'Mcast_Macs_Remote's.
+AT_SETUP([ovn-controller-vtep - vtep-Mcast_Macs_Remote])
+OVN_CONTROLLER_VTEP_START
+
+# creates a simple logical network with the vtep device and a fake hv chassis
+# 'ch0'.
+AT_CHECK([ovn-nbctl lsp-add br-test vif0])
+AT_CHECK([ovn-nbctl lsp-set-addresses vif0 f0:ab:cd:ef:01:00])
+AT_CHECK([ovn-nbctl --timeout=10 --wait=sb sync])
+AT_CHECK([ovn-sbctl chassis-add ch0 vxlan 1.2.3.5])
+AT_CHECK([ovn-sbctl lsp-bind vif0 ch0])
+
+# creates the logical switch in vtep and adds the corresponding logical
+# port to 'br-test'.
+AT_CHECK([vtep-ctl add-ls lswitch0 -- bind-ls br-vtep p0 100 lswitch0])
+OVN_NB_ADD_VTEP_PORT([br-test], [br-vtep_lswitch0], [br-vtep], [lswitch0])
+OVS_WAIT_UNTIL([test -n "`ovn-sbctl list Port_Binding  | grep br-vtep_lswitch0`"])
+
+# checks Mcast_Macs_Remote creation.
+OVS_WAIT_UNTIL([test `vtep-ctl list Mcast_Macs_Remote | grep _uuid | wc -l` -eq 1])
+AT_CHECK([vtep-ctl --columns=MAC list Mcast_Macs_Remote | cut -d ':' -f2- | tr -d ' '], [0], [dnl
+unknown-dst
+])
+
+# check physical locator and physical locator set updates
+OVS_WAIT_UNTIL([test -n "`vtep-ctl list Physical_Locator | grep _uuid`"])
+AT_CHECK([for i in `vtep-ctl --columns=locators list Physical_Locator_Set $(vtep-ctl --columns=locator_set list Mcast_Macs_Remote | cut -d ':' -f2) | cut -d ':' -f2 | tr -d '[[ ]]' | sed 's/,/ /g'`; do
+   vtep-ctl --columns=dst_ip list Physical_Locator $i | cut -d ':' -f2 | tr -d ' '
+done], [0], [dnl
+"1.2.3.5"
+])
+
+# add new lport and bind it to another fake chassis 'ch1'.
+AT_CHECK([ovn-nbctl lsp-add br-test vif1])
+AT_CHECK([ovn-nbctl lsp-set-addresses vif0 f0:ab:cd:ef:01:01])
+AT_CHECK([ovn-nbctl --timeout=10 --wait=sb sync])
+AT_CHECK([ovn-sbctl chassis-add ch1 vxlan 1.2.3.6])
+AT_CHECK([ovn-sbctl lsp-bind vif1 ch1])
+
+# checks there is still only one Mcast_Macs_Remote record.
+OVS_WAIT_UNTIL([test `vtep-ctl list Mcast_Macs_Remote | grep _uuid | wc -l` -eq 1])
+AT_CHECK([vtep-ctl --columns=MAC list Mcast_Macs_Remote | cut -d ':' -f2- | tr -d ' '], [0], [dnl
+unknown-dst
+])
+
+# check physical locator and physical locator set updates
+AT_CHECK([for i in `vtep-ctl --columns=locators list Physical_Locator_Set $(vtep-ctl --columns=locator_set list Mcast_Macs_Remote | cut -d ':' -f2) | cut -d ':' -f2 | tr -d '[[ ]]' | sed 's/,/ /g'`; do
+   vtep-ctl --columns=dst_ip list Physical_Locator $i | cut -d ':' -f2 | tr -d ' '
+done | sort], [0], [dnl
+"1.2.3.5"
+"1.2.3.6"
+])
+
+OVN_CONTROLLER_VTEP_STOP
+AT_CLEANUP