diff mbox series

[ovs-dev] controller-vtep: fix mmr and physical locators create/update

Message ID 20210527192111.66522-1-odivlad@gmail.com
State Superseded
Headers show
Series [ovs-dev] controller-vtep: fix mmr and physical locators create/update | expand

Commit Message

Влад Одинцов May 27, 2021, 7:21 p.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       | 63 +++++++++++++++++++-------------
 tests/ovn-controller-vtep.at | 70 ++++++++++++++++++++++++++++++++++++
 2 files changed, 108 insertions(+), 25 deletions(-)

Comments

0-day Robot May 27, 2021, 7:59 p.m. UTC | #1
Bleep bloop.  Greetings Vladislav Odintsov, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line is 80 characters long (recommended limit is 79)
#88 FILE: controller-vtep/vtep.c:178:
                vtep_create_mmr(vtep_idl_txn, "unknown-dst", vtep_ls, ploc_set);

WARNING: Line is 82 characters long (recommended limit is 79)
#90 FILE: controller-vtep/vtep.c:180:
                vteprec_mcast_macs_remote_set_locator_set(mmr_ext->mmr, ploc_set);

WARNING: Line is 84 characters long (recommended limit is 79)
#118 FILE: controller-vtep/vtep.c:249:
                vteprec_logical_switch_set_replication_mode(vtep_ls, "source_node");

Lines checked: 248, Warnings: 3, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
Odintsov Vladislav May 28, 2021, 8:24 a.m. UTC | #2
I've posted a new version of this patch with requested changes:
https://patchwork.ozlabs.org/project/ovn/patch/20210528082105.70086-1-odivlad@gmail.com/


Regards,
 
Vladislav Odintsov

On 27.05.2021, 23:00, "dev on behalf of 0-day Robot" <ovs-dev-bounces@openvswitch.org on behalf of robot@bytheb.org> wrote:

    Bleep bloop.  Greetings Vladislav Odintsov, I am a robot and I have tried out your patch.
    Thanks for your contribution.

    I encountered some error that I wasn't expecting.  See the details below.


    checkpatch:
    WARNING: Line is 80 characters long (recommended limit is 79)
    #88 FILE: controller-vtep/vtep.c:178:
                    vtep_create_mmr(vtep_idl_txn, "unknown-dst", vtep_ls, ploc_set);

    WARNING: Line is 82 characters long (recommended limit is 79)
    #90 FILE: controller-vtep/vtep.c:180:
                    vteprec_mcast_macs_remote_set_locator_set(mmr_ext->mmr, ploc_set);

    WARNING: Line is 84 characters long (recommended limit is 79)
    #118 FILE: controller-vtep/vtep.c:249:
                    vteprec_logical_switch_set_replication_mode(vtep_ls, "source_node");

    Lines checked: 248, Warnings: 3, Errors: 0


    Please check this out.  If you feel there has been an error, please email aconole@redhat.com

    Thanks,
    0-day Robot
    _______________________________________________
    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..261840f47 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,27 @@  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 +237,18 @@  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 +364,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 +548,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