Thanks Guru
I will address and resubmit
On 1/28/16, 2:16 PM, "dev on behalf of Guru Shetty"
<dev-boun...@openvswitch.org on behalf of g...@ovn.org> wrote:
>You will need to add a commit message to this commit.
>You will need to add your Signed-off-by. You should also add yourself to
>the AUTHORS file.
>
>Since ovs-vtep is already supposed to work for L2 related things, and has
>been tested well before, I suggest splitting the change for ovs-vtep into a
>separate patch with a proper description (in commit message) on what is the
>behavior that you are fixing. Looking at the code, I can't make out the
>reason for the changes.
>
>
>
>
>On 11 January 2016 at 17:52, Darrell Ball <dlu...@gmail.com> wrote:
>
>> ---
>> ovn/controller-vtep/vtep.c | 207
>> ++++++++++++++++++++++++++++++++++++++++-----
>> tests/ovn.at | 6 +-
>> vtep/ovs-vtep | 18 +++-
>> 3 files changed, 204 insertions(+), 27 deletions(-)
>>
>> diff --git a/ovn/controller-vtep/vtep.c b/ovn/controller-vtep/vtep.c
>> index 016c2e0..04251c4 100644
>> --- a/ovn/controller-vtep/vtep.c
>> +++ b/ovn/controller-vtep/vtep.c
>> @@ -30,6 +30,16 @@
>>
>> VLOG_DEFINE_THIS_MODULE(vtep);
>>
>> +struct vtep_rec_physical_locator_list_entry{
>> + struct ovs_list locators_node;
>> + const struct vteprec_physical_locator *vteprec_ploc;
>> +};
>> +
>> +struct mmr_hash_node_data {
>> + const struct vteprec_mcast_macs_remote *mmr;
>> + struct shash physical_locators;
>> +};
>> +
>> /*
>> * Scans through the Binding table in ovnsb, and updates the vtep logical
>> * switch tunnel keys and the 'Ucast_Macs_Remote' table in the VTEP
>> @@ -84,6 +94,98 @@ create_pl(struct ovsdb_idl_txn *vtep_idl_txn, const
>> char *chassis_ip)
>> }
>>
>>
>> +/* Creates a new 'Mcast_Macs_Remote'. entry */
>> +static void
>> +vtep_create_mmr(struct ovsdb_idl_txn *vtep_idl_txn,
>> + const char *mac,
>> + const struct vteprec_logical_switch *vtep_ls,
>> + const struct vteprec_physical_locator_set *ploc_set)
>> +{
>> + struct vteprec_mcast_macs_remote *new_mmr =
>> + vteprec_mcast_macs_remote_insert(vtep_idl_txn);
>> +
>> + 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);
>> +}
>> +
>> +/* Compares prev and new mmr locator sets and return true if they
>> + * differ; false otherwise; also preps new locator set
>> + * for database write */
>> +static bool
>> +vtep_process_pls(
>> + const struct ovs_list *locators_list,
>> + const struct mmr_hash_node_data *mmr_ext,
>> + struct vteprec_physical_locator **locators)
>> +{
>> + int i;
>> + size_t n_locators_prev = 0;
>> + size_t n_locators_new = list_size(locators_list);
>> + struct vtep_rec_physical_locator_list_entry *ploc_entry;
>> + const struct vteprec_physical_locator *pl = NULL;
>> + bool prev_and_new_locator_lists_differ = false;
>> +
>> + if(mmr_ext) {
>> + n_locators_prev = mmr_ext->mmr->locator_set->n_locators;
>> + }
>> + if(n_locators_prev != n_locators_new) {
>> + prev_and_new_locator_lists_differ = true;
>> + }
>> +
>> + if(n_locators_new) {
>> + i = 0;
>> + LIST_FOR_EACH (ploc_entry, locators_node, locators_list) {
>> + locators[i] = (struct vteprec_physical_locator *)
>> + ploc_entry->vteprec_ploc;
>> + if(mmr_ext) {
>> + pl = shash_find_data(&mmr_ext->physical_locators,
>> + locators[i]->dst_ip);
>> + if(!pl) {
>> + prev_and_new_locator_lists_differ = true;
>> + }
>> + }
>> + i++;
>> + }
>> + }
>> +
>> + return(prev_and_new_locator_lists_differ);
>> +}
>> +
>> +/* Creates a new 'Mcast_Macs_Remote' entry if needed.
>> + * Also cleans prev remote mcast mac entries as needed */
>> +static void
>> +vtep_update_mmr(struct ovsdb_idl_txn *vtep_idl_txn,
>> + struct ovs_list *locators_list,
>> + const struct vteprec_logical_switch *vtep_ls,
>> + const struct mmr_hash_node_data *mmr_ext)
>> +{
>> + struct vteprec_physical_locator **locators = NULL;
>> + size_t n_locators_new = list_size(locators_list);
>> + bool mmr_changed = false;
>> + const struct vteprec_physical_locator_set *ploc_set;
>> +
>> + locators = xmalloc(n_locators_new * sizeof(*locators));
>> +
>> + if(vtep_process_pls(locators_list, mmr_ext, locators)) {
>> + mmr_changed = true;
>> + }
>> +
>> + 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)) {
>> +
>> + ploc_set = vteprec_physical_locator_set_insert(vtep_idl_txn);
>> +
>> + vtep_create_mmr(vtep_idl_txn, "unknown-dst", vtep_ls, ploc_set);
>> +
>> + vteprec_physical_locator_set_set_locators(ploc_set, locators,
>> + n_locators_new);
>> +
>> + }
>> + free(locators);
>> +}
>> +
>> /* Updates the vtep Logical_Switch table entries' tunnel keys based
>> * on the port bindings. */
>> static void
>> @@ -153,12 +255,15 @@ vtep_lswitch_run(struct shash *vtep_pbs, struct sset
>> *vtep_pswitches,
>> * bindings. */
>> static void
>> vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn, struct shash
>> *ucast_macs_rmts,
>> - struct shash *physical_locators, struct shash
>> *vtep_lswitches,
>> - struct shash *non_vtep_pbs)
>> + struct shash *mcast_macs_rmts, struct shash
>> *physical_locators,
>> + struct shash *vtep_lswitches, struct shash *non_vtep_pbs)
>> {
>> struct shash_node *node;
>> struct hmap ls_map;
>>
>> + struct vtep_rec_physical_locator_list_entry *ploc_entry;
>> + const struct vteprec_physical_locator *pl;
>> +
>> /* Maps from ovn logical datapath tunnel key (which is also the vtep
>> * logical switch tunnel key) to the corresponding vtep logical switch
>> * instance. Also, the shash map 'added_macs' is used for checking
>> @@ -168,6 +273,9 @@ vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn,
>> struct shash *ucast_macs_rmts,
>>
>> const struct vteprec_logical_switch *vtep_ls;
>> struct shash added_macs;
>> +
>> + struct ovs_list locators_list;
>> + struct mmr_hash_node_data *mmr_ext;
>> };
>>
>> hmap_init(&ls_map);
>> @@ -181,6 +289,8 @@ vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn,
>> struct shash *ucast_macs_rmts,
>> ls_node = xmalloc(sizeof *ls_node);
>> ls_node->vtep_ls = vtep_ls;
>> shash_init(&ls_node->added_macs);
>> + list_init(&ls_node->locators_list);
>> + ls_node->mmr_ext = NULL;
>> hmap_insert(&ls_map, &ls_node->hmap_node,
>> hash_uint64((uint64_t) vtep_ls->tunnel_key[0]));
>> }
>> @@ -222,18 +332,31 @@ vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn,
>> struct shash *ucast_macs_rmts,
>> continue;
>> }
>>
>> + 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);
>> + }
>> +
>> + 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) {
>> + shash_find_and_delete(mcast_macs_rmts, mac_tnlkey);
>> + }
>> + free(mac_tnlkey);
>> + ploc_entry = xmalloc(sizeof *ploc_entry);
>> + ploc_entry->vteprec_ploc = pl;
>> + list_push_back(&ls_node->locators_list,
>> + &ploc_entry->locators_node);
>> +
>> for (i = 0; i < port_binding_rec->n_mac; i++) {
>> const struct vteprec_ucast_macs_remote *umr;
>> - const struct vteprec_physical_locator *pl;
>> const struct sbrec_port_binding *conflict;
>> char *mac = port_binding_rec->mac[i];
>>
>> - /* xxx Need to address this later when we support
>> - * update of 'Mcast_Macs_Remote' table in VTEP. */
>> - if (!strcmp(mac, "unknown")) {
>> - continue;
>> - }
>> -
>> /* Checks for duplicate MAC in the same vtep logical switch.
>> */
>> conflict = shash_find_data(&ls_node->added_macs, mac);
>> if (conflict) {
>> @@ -257,19 +380,8 @@ vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn,
>> struct shash *ucast_macs_rmts,
>> 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);
>> - pl = shash_find_data(physical_locators, chassis_ip);
>> - if (pl) {
>> - vteprec_ucast_macs_remote_set_locator(new_umr, pl);
>> - } else {
>> - const struct vteprec_physical_locator *new_pl;
>> -
>> - new_pl = create_pl(vtep_idl_txn, chassis_ip);
>> - vteprec_ucast_macs_remote_set_locator(new_umr,
>> new_pl);
>> - /* Updates the 'physical_locators'. */
>> - shash_add(physical_locators, chassis_ip, new_pl);
>> - }
>> + vteprec_ucast_macs_remote_set_locator(new_umr, pl);
>> }
>> free(mac_ip_tnlkey);
>> }
>> @@ -281,11 +393,25 @@ vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn,
>> struct shash *ucast_macs_rmts,
>> }
>> struct ls_hash_node *iter, *next;
>> HMAP_FOR_EACH_SAFE (iter, next, hmap_node, &ls_map) {
>> + vtep_update_mmr(vtep_idl_txn,&iter->locators_list,
>> + iter->vtep_ls, iter->mmr_ext);
>> + LIST_FOR_EACH_POP(ploc_entry,locators_node,
>> + &iter->locators_list) {
>> + free(ploc_entry);
>> + }
>> hmap_remove(&ls_map, &iter->hmap_node);
>> shash_destroy(&iter->added_macs);
>> free(iter);
>> }
>> hmap_destroy(&ls_map);
>> + /* Clean stale MMRs */
>> + struct mmr_hash_node_data *mmr_ext;
>> + SHASH_FOR_EACH (node, mcast_macs_rmts) {
>> + mmr_ext = node->data;
>> + shash_destroy(&mmr_ext->physical_locators);
>> + vteprec_mcast_macs_remote_delete(mmr_ext->mmr);
>> + free(mmr_ext);
>> + }
>> }
>>
>> /* Resets all logical switches' 'tunnel_key' to NULL */
>> @@ -311,11 +437,16 @@ static bool
>> vtep_macs_cleanup(struct ovsdb_idl *vtep_idl)
>> {
>> const struct vteprec_ucast_macs_remote *umr;
>> + const struct vteprec_mcast_macs_remote *mmr;
>>
>> VTEPREC_UCAST_MACS_REMOTE_FOR_EACH (umr, vtep_idl) {
>> vteprec_ucast_macs_remote_delete(umr);
>> return false;
>> }
>> + VTEPREC_MCAST_MACS_REMOTE_FOR_EACH (mmr, vtep_idl) {
>> + vteprec_mcast_macs_remote_delete(mmr);
>> + return false;
>> + }
>> return true;
>> }
>>
>> @@ -330,6 +461,7 @@ vtep_run(struct controller_vtep_ctx *ctx)
>> struct sset vtep_pswitches = SSET_INITIALIZER(&vtep_pswitches);
>> struct shash vtep_lswitches = SHASH_INITIALIZER(&vtep_lswitches);
>> struct shash ucast_macs_rmts = SHASH_INITIALIZER(&ucast_macs_rmts);
>> + struct shash mcast_macs_rmts = SHASH_INITIALIZER(&mcast_macs_rmts);
>> struct shash physical_locators =
>> SHASH_INITIALIZER(&physical_locators);
>> struct shash vtep_pbs = SHASH_INITIALIZER(&vtep_pbs);
>> struct shash non_vtep_pbs = SHASH_INITIALIZER(&non_vtep_pbs);
>> @@ -338,6 +470,12 @@ vtep_run(struct controller_vtep_ctx *ctx)
>> const struct vteprec_ucast_macs_remote *umr;
>> const struct vteprec_physical_locator *pl;
>> const struct sbrec_port_binding *port_binding_rec;
>> + const struct vteprec_mcast_macs_remote *mmr;
>> + struct mmr_hash_node_data *mmr_ext;
>> + const struct vteprec_physical_locator_set *locator_set;
>> + struct vteprec_physical_locator **locators_list;
>> + size_t n_locators;
>> + size_t i;
>>
>> /* Collects 'Physical_Switch's. */
>> VTEPREC_PHYSICAL_SWITCH_FOR_EACH (vtep_ps, ctx->vtep_idl) {
>> @@ -360,6 +498,29 @@ vtep_run(struct controller_vtep_ctx *ctx)
>> shash_add(&ucast_macs_rmts, mac_ip_tnlkey, umr);
>> free(mac_ip_tnlkey);
>> }
>> + /* Collects 'Mcast_Macs_Remote's. */
>> + VTEPREC_MCAST_MACS_REMOTE_FOR_EACH (mmr, ctx->vtep_idl) {
>> + char *mac_tnlkey =
>> + xasprintf("%s_%"PRId64, mmr->MAC,
>> + mmr->logical_switch &&
>> mmr->logical_switch->n_tunnel_key
>> + ? mmr->logical_switch->tunnel_key[0] :
>> INT64_MAX);
>> +
>> + mmr_ext = xmalloc(sizeof *mmr_ext);
>> + shash_add(&mcast_macs_rmts, mac_tnlkey, mmr_ext);
>> + mmr_ext->mmr = mmr;
>> + locator_set = mmr_ext->mmr->locator_set;
>> + n_locators = locator_set->n_locators;
>> + locators_list = locator_set->locators;
>> +
>> + shash_init(&mmr_ext->physical_locators);
>> + for(i = 0; i < n_locators; i++) {
>> + shash_add(&mmr_ext->physical_locators,
>> + locators_list[i]->dst_ip,
>> + locators_list[i]);
>> + }
>> +
>> + free(mac_tnlkey);
>> + }
>> /* Collects 'Physical_Locator's. */
>> VTEPREC_PHYSICAL_LOCATOR_FOR_EACH (pl, ctx->vtep_idl) {
>> shash_add(&physical_locators, pl->dst_ip, pl);
>> @@ -380,12 +541,14 @@ vtep_run(struct controller_vtep_ctx *ctx)
>> "tunnel keys and 'ucast_macs_remote's");
>>
>> vtep_lswitch_run(&vtep_pbs, &vtep_pswitches, &vtep_lswitches);
>> - vtep_macs_run(ctx->vtep_idl_txn, &ucast_macs_rmts, &physical_locators,
>> + vtep_macs_run(ctx->vtep_idl_txn, &ucast_macs_rmts,
>> + &mcast_macs_rmts, &physical_locators,
>> &vtep_lswitches, &non_vtep_pbs);
>>
>> sset_destroy(&vtep_pswitches);
>> shash_destroy(&vtep_lswitches);
>> shash_destroy(&ucast_macs_rmts);
>> + shash_destroy(&mcast_macs_rmts);
>> shash_destroy(&physical_locators);
>> shash_destroy(&vtep_pbs);
>> shash_destroy(&non_vtep_pbs);
>> diff --git a/tests/ovn.at b/tests/ovn.at
>> index e00674d..a60ab56 100644
>> --- a/tests/ovn.at
>> +++ b/tests/ovn.at
>> @@ -870,10 +870,8 @@ for s in 1 2 3; do
>> done
>>
>> # Broadcast and multicast.
>> - # xxx ovn-controller-vtep doesn't handle multicast traffic that is
>> - # xxx sourced from the gateway properly.
>> - #test_packet $s ffffffffffff f0000000000$s 0${s}ff $bcast
>> #2
>> - #test_packet $s 010000000000 f0000000000$s 0${s}ff $bcast
>> #2
>> + test_packet $s ffffffffffff f0000000000$s 0${s}ff $bcast
>> #2
>> + test_packet $s 010000000000 f0000000000$s 0${s}ff $bcast
>> #2
>>
>> test_packet $s f0000000ffff f0000000000$s 0${s}66 $unknown
>> #3
>> done
>> diff --git a/vtep/ovs-vtep b/vtep/ovs-vtep
>> index 46a5692..a479a3a 100755
>> --- a/vtep/ovs-vtep
>> +++ b/vtep/ovs-vtep
>> @@ -135,15 +135,31 @@ class Logical_Switch(object):
>> ovs_ofctl("add-flow %s
>> table=1,priority=1,in_port=%s,action=%s"
>> % (self.short_name, port_no,
>> ",".join(flood_ports)))
>>
>> + existing_flow = 0
>> + appended_flood_port = 0
>> + flows = ovs_ofctl("dump-flows %s table=1,cookie=0x4848/-1"
>> + % self.short_name).splitlines()
>> + for f in flows:
>> + if ("table" in f):
>> + existing_flow = 1
>> + break
>> +
>> # Traffic coming from a VTEP physical port should only be flooded
>> to
>> # one 'unknown-dst' and to all other physical ports that belong
>> to that
>> # VTEP device and this logical switch.
>> for tunnel in self.unknown_dsts:
>> port_no = self.tunnels[tunnel][0]
>> flood_ports.append(port_no)
>> + appended_flood_port = 1
>> break
>>
>> - ovs_ofctl("add-flow %s table=1,priority=0,action=%s"
>> + if ((existing_flow == 1) and (appended_flood_port == 1)):
>> + ovs_ofctl(
>> + "mod-flows %s
>> table=1,priority=0,cookie=0x4848/-1,action=%s"
>> + % (self.short_name, ",".join(flood_ports)))
>> + elif (existing_flow == 0):
>> + ovs_ofctl(
>> + "add-flow %s table=1,priority=0,cookie=0x4848,action=%s"
>> % (self.short_name, ",".join(flood_ports)))
>>
>> def add_lbinding(self, lbinding):
>> --
>> 1.9.1
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>>
>_______________________________________________
>dev mailing list
>dev@openvswitch.org
>http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
