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 <dce...@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 <odiv...@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
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
