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