[ovs-dev] [PATCH ovn v3 1/2] controller-vtep: Fix MMR and physical locators create/update
Dumitru Ceara
dceara at redhat.com
Tue Jun 15 15:50:58 UTC 2021
On 6/11/21 1:44 PM, Vladislav Odintsov wrote:
> Before this patch ovn-controller-vtep created Mcast_Macs_Remote
> record for each Port Binding of 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.
>
> Signed-off-by: Vladislav Odintsov <odivlad at gmail.com>
> ---
Thanks for this new revision! I think the changes are OK, I just have a
few minor style comments. If you address them, feel free to add my ack
in v4.
Acked-by: Dumitru Ceara <dceara at redhat.com>
> controller-vtep/vtep.c | 60 ++++++++++++++++++-------------
> tests/ovn-controller-vtep.at | 70 ++++++++++++++++++++++++++++++++++++
> 2 files changed, 105 insertions(+), 25 deletions(-)
>
> diff --git a/controller-vtep/vtep.c b/controller-vtep/vtep.c
> index f3b02f63f..49723b39d 100644
> --- a/controller-vtep/vtep.c
> +++ b/controller-vtep/vtep.c
> @@ -48,7 +48,7 @@ struct mmr_hash_node_data {
> * database.
> *
> */
> -
> +
These are actually part of the coding guidelines [0], we should probably
leave them as they are:
"Use form feeds (control+L) to divide long source files into logical
pieces. A form feed should appear as the only character on a line."
[0]
https://github.com/ovn-org/ovn/blob/master/Documentation/internals/contributing/coding-style.rst#basics
> /* Searches the 'chassis_rec->encaps' for the first vtep tunnel
> * configuration, returns the 'ip'. Unless duplicated, the returned
> * pointer cannot live past current vtep_run() execution. */
> @@ -94,7 +94,7 @@ create_pl(struct ovsdb_idl_txn *vtep_idl_txn, const char *chassis_ip)
>
> return new_pl;
> }
> -
> +
Same here.
> /* 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);
> }
>
> @@ -353,17 +361,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;
> @@ -481,7 +491,7 @@ vtep_mcast_macs_cleanup(struct ovsdb_idl *vtep_idl)
>
> return true;
> }
> -
> +
Form feed can stay.
Regards,
Dumitru
More information about the dev
mailing list