On 5/22/25 10:40 AM, Vasyl Saienko wrote:
> Populate Ucast_Macs_Remote table from MAC addresses learned via
> FDB table. This will allow communication betwen different
> switches plugged to same logical network via VTEP.
>
> Signed-Off-By: Vasyl Saienko <vsaie...@mirantis.com>
> ---
Hi Vasyl,
Thanks for the patch!
> controller-vtep/vtep.c | 127 ++++++++++++++++++++++++++++++++++-
> tests/ovn-controller-vtep.at | 28 ++++++++
> 2 files changed, 153 insertions(+), 2 deletions(-)
>
> diff --git a/controller-vtep/vtep.c b/controller-vtep/vtep.c
> index f86cb679f..bc260b83b 100644
> --- a/controller-vtep/vtep.c
> +++ b/controller-vtep/vtep.c
> @@ -275,7 +275,9 @@ vtep_lswitch_run(struct shash *vtep_pbs, struct sset
> *vtep_pswitches,
> static void
> vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn, struct shash
> *ucast_macs_rmts,
> struct shash *mcast_macs_rmts, struct shash *physical_locators,
> - struct shash *vtep_lswitches, struct shash *non_vtep_pbs)
> + struct shash *vtep_lswitches, struct shash *non_vtep_pbs,
> + struct sset *vtep_pswitches, struct shash *vtep_pbs,
> + struct shash *lp_fdbs)
> {
> struct shash_node *node;
> struct hmap ls_map;
> @@ -451,6 +453,125 @@ vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn,
> struct shash *ucast_macs_rmts,
> }
> }
>
> + /* Handle dynamically leart MACs from remote VTEPs registered in
> + * FDB table. */
> + SHASH_FOR_EACH (node, vtep_pbs) {
> + const struct sbrec_port_binding *port_binding_rec = node->data;
> + const struct sbrec_chassis *chassis_rec;
> + struct ls_hash_node *ls_node;
> + const char *chassis_ip;
> + int64_t tnl_key;
Nit: I'd move the declarations lower, where we assign the value.
> +
> + chassis_rec = port_binding_rec->chassis;
Nit: I'd prefer if we declare the variable here directly, e.g.:
const struct sbrec_chassis *chassis_rec = port_binding_rec->chassis;
> + if (!chassis_rec) {
> + continue;
> + }
> +
> + if (!port_binding_rec->datapath->tunnel_key ||
> + !port_binding_rec->tunnel_key) {
> + continue;
> + }
> +
These tunnel_keys can never be 0. We can probably skip this check.
> + const char *pswitch_name = smap_get(&port_binding_rec->options,
> + "vtep-physical-switch");
> + /* Ignore macs learned by ourselfs */
> + if (sset_find(vtep_pswitches, pswitch_name)) {
> + continue;
> + }
> + tnl_key = port_binding_rec->datapath->tunnel_key;
> +
> + HMAP_FOR_EACH_WITH_HASH (ls_node, hmap_node,
> + hash_uint64((uint64_t) tnl_key),
> + &ls_map) {
> + if (ls_node->vtep_ls->tunnel_key[0] == tnl_key) {
> + break;
> + }
> + }
> + /* If 'ls_node' is NULL, that means no vtep logical switch is
> + * attached to the corresponding ovn logical datapath, so pass.
> + */
> + if (!ls_node) {
> + continue;
> + }
> +
> + chassis_ip = get_chassis_vtep_ip(chassis_rec);
> + /* Unreachable chassis, continue. */
> + if (!chassis_ip) {
> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> + VLOG_INFO_RL(&rl, "VTEP tunnel encap on chassis (%s) not found",
> + chassis_rec->name);
> + continue;
> + }
> +
> + const struct vteprec_physical_locator *pl =
> + shash_find_data(physical_locators, chassis_ip);
> + if (!pl) {
> + pl = create_pl(vtep_idl_txn, chassis_ip);
> + shash_add(physical_locators, chassis_ip, pl);
> + }
> +
> + const struct vteprec_physical_locator *ls_pl =
> + shash_find_data(&ls_node->physical_locators, chassis_ip);
> + if (!ls_pl) {
> + struct vtep_rec_physical_locator_list_entry *ploc_entry =
> + xmalloc(sizeof *ploc_entry);
> + ploc_entry->vteprec_ploc = pl;
> + ovs_list_push_back(&ls_node->locators_list,
> + &ploc_entry->locators_node);
> + shash_add(&ls_node->physical_locators, chassis_ip, pl);
> + }
> +
> + char *fdb_dp_port_key = xasprintf(
> + "%"PRId64"_%"PRId64, port_binding_rec->datapath->tunnel_key,
> + port_binding_rec->tunnel_key);
> +
> + struct lp_fdb_node_data *sbrec_lp_fdb = shash_find_data(
> + lp_fdbs, fdb_dp_port_key);
> + free(fdb_dp_port_key);
Nit: I'd indent this as:
char *fdb_dp_port_key =
xasprintf("%"PRId64"_%"PRId64,
port_binding_rec->datapath->tunnel_key,
port_binding_rec->tunnel_key);
struct lp_fdb_node_data *sbrec_lp_fdb =
shash_find_data(lp_fdbs, fdb_dp_port_key);
free(fdb_dp_port_key);
> +
> + struct shash_node *sbrec_fdb_node;
Nit: this can go lower, just before SHASH_FOR_EACH.
> +
> + if (!sbrec_lp_fdb) {
> + continue;
> + }
> + SHASH_FOR_EACH (sbrec_fdb_node, &sbrec_lp_fdb->mac_fdbs) {
> + const struct sbrec_fdb *fdb = sbrec_fdb_node->data;
> +
> + const struct vteprec_ucast_macs_remote *umr;
> + const struct sbrec_port_binding *conflict;
> +
> + char *mac = fdb->mac;
> +
> + /* Checks for duplicate MAC in the same vtep logical switch. */
> + conflict = shash_find_data(&ls_node->added_macs, mac);
> + if (conflict) {
> + VLOG_WARN("MAC address (%s) has already been known to be "
Nit: VLOG_WARN_RL() - ovn-controller-vtep does a full recompute every
time so we'd be spamming with warnings otherwise.
> + "on logical port (%s) in the same logical "
> + "datapath, so just ignore this logical port (%s)",
> + mac, conflict->logical_port,
> + port_binding_rec->logical_port);
> + continue;
> + }
> + shash_add(&ls_node->added_macs, mac, port_binding_rec);
> +
> + char *mac_ip_tnlkey = xasprintf("%s_%s_%"PRId64, mac, chassis_ip,
> + tnl_key);
> + umr = shash_find_data(ucast_macs_rmts, mac_ip_tnlkey);
> + /* If finds the 'umr' entry for the mac, ip, and tnl_key, deletes
> + * the entry from shash so that it is not garbage collected.
> + *
> + * If not found, creates a new 'umr' entry. */
> + if (umr && umr->logical_switch == ls_node->vtep_ls) {
> + shash_find_and_delete(ucast_macs_rmts, mac_ip_tnlkey);
> + } else {
> + const struct vteprec_ucast_macs_remote *new_umr;
> + new_umr = create_umr(vtep_idl_txn, mac, ls_node->vtep_ls);
> + vteprec_ucast_macs_remote_set_locator(new_umr, pl);
> + }
> + free(mac_ip_tnlkey);
> + }
> + }
> +
> /* Removes all remaining 'umr's, since they do not exist anymore. */
> SHASH_FOR_EACH (node, ucast_macs_rmts) {
> vteprec_ucast_macs_remote_delete(node->data);
> @@ -775,7 +896,9 @@ vtep_run(struct controller_vtep_ctx *ctx)
> vtep_lswitch_run(&vtep_pbs, &vtep_pswitches, &vtep_lswitches);
> vtep_macs_run(ctx->vtep_idl_txn, &ucast_macs_rmts,
> &mcast_macs_rmts, &physical_locators,
> - &vtep_lswitches, &non_vtep_pbs);
> + &vtep_lswitches, &non_vtep_pbs,
> + &vtep_pswitches, &vtep_pbs,
> + &lp_fdbs);
> vtep_local_macs(ctx, &vtep_pbs, &vtep_pswitches, &vtep_lswitches,
> &lp_fdbs);
>
> diff --git a/tests/ovn-controller-vtep.at b/tests/ovn-controller-vtep.at
> index 2434d9126..25c47d6d8 100644
> --- a/tests/ovn-controller-vtep.at
> +++ b/tests/ovn-controller-vtep.at
> @@ -604,6 +604,34 @@ OVS_WAIT_UNTIL([test -n "`vtep-ctl list Ucast_Macs_Local
> | grep aa:bb:cc:dd:ff:0
> OVS_WAIT_UNTIL([test -n "`ovn-sbctl find Fdb mac='"aa:bb:cc:dd:ff:01"'`"])
> OVS_WAIT_UNTIL([test -n "`ovn-sbctl find Fdb mac='"aa:bb:cc:dd:ff:02"'`"])
>
> +# Switch context
> +as
> +
> +# Make sure we see
> +OVS_WAIT_UNTIL([test -n "`vtep-ctl list-remote-macs lswitch0 | grep
> aa:bb:cc:dd:ff:01`"])
> +OVS_WAIT_UNTIL([test -z "`vtep-ctl list-remote-macs lswitch1 | grep
> aa:bb:cc:dd:ff:01`"])
> +
> +OVS_WAIT_UNTIL([test -n "`vtep-ctl list-remote-macs lswitch1 | grep
> aa:bb:cc:dd:ff:02`"])
> +OVS_WAIT_UNTIL([test -z "`vtep-ctl list-remote-macs lswitch0 | grep
> aa:bb:cc:dd:ff:02`"])
> +
> +# Switch context to br-vtep1
> +as br-vtep1
> +
> +# Remove local MACs
> +AT_CHECK([ovs-ofctl -O OpenFlow14 del-flows br-vtep1_vtep_ls1
> "table=mac_learning_table,dl_dst=aa:bb:cc:dd:ff:01"])
> +AT_CHECK([ovs-ofctl -O OpenFlow14 del-flows br-vtep1_vtep_ls2
> "table=mac_learning_table,dl_dst=aa:bb:cc:dd:ff:02"])
> +
> +# Switch context
> +as
> +
> +# Ensure remote MACs gone
> +OVS_WAIT_UNTIL([test -z "`vtep-ctl list-remote-macs lswitch0 | grep
> aa:bb:cc:dd:ff:01`"])
> +OVS_WAIT_UNTIL([test -z "`vtep-ctl list-remote-macs lswitch1 | grep
> aa:bb:cc:dd:ff:02`"])
> +
> +# Check that MACs are removed from FDB table
> +OVS_WAIT_UNTIL([test -z "`ovn-sbctl find Fdb mac='"aa:bb:cc:dd:ff:01"'`"])
> +OVS_WAIT_UNTIL([test -z "`ovn-sbctl find Fdb mac='"aa:bb:cc:dd:ff:02"'`"])
> +
> OVN_CONTROLLER_VTEP_STOP
> AT_CLEANUP
>
Regards,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
