[ovs-dev] [PATCH V8 2/3] ovn-controller-vtep: Extend vtep module to install Ucast_Macs_Remote.

[Alex Wang]

Hey Russell,

Sorry for this very delayed reply,~

Please see my reply inline,

Thanks,
Alex Wang,

On Mon, Aug 24, 2015 at 10:48 AM, Russell Bryant <rbryant at redhat.com> wrote:

> On 08/23/2015 02:06 PM, Alex Wang wrote:
> > This commit extends the vtep module to support creating the
> > 'Ucast_Macs_Remote' table entries in the vtep database for
> > MAC addresses on the ovn logical ports.
> >
> > Signed-off-by: Alex Wang <alexw at nicira.com>
>
> Acked-by: Russell Bryant <rbryant at redhat.com>
>
> > +/* Updates the vtep 'Ucast_Macs_Remote' table based on non-vtep port
> > + * 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 *other_pbs)
> > +{
> > +    struct shash_node *node;
> > +    struct hmap ls_map;
> > +
> > +    /* 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
> > +     * duplicated MAC addresses in the same ovn logical datapath. */
> > +    struct ls_hash_node {
> > +        struct hmap_node hmap_node;
> > +
> > +        const struct vteprec_logical_switch *vtep_ls;
> > +        struct shash added_macs;
> > +    };
> > +
> > +    hmap_init(&ls_map);
> > +    SHASH_FOR_EACH (node, vtep_lswitches) {
> > +        struct ls_hash_node *ls_node = xmalloc(sizeof *ls_node);
> > +
> > +        ls_node->vtep_ls = node->data;
> > +        shash_init(&ls_node->added_macs);
> > +        hmap_insert(&ls_map, &ls_node->hmap_node,
> > +                    hash_uint64((uint64_t)
> ls_node->vtep_ls->tunnel_key[0]));
> > +    }
> > +
> > +    SHASH_FOR_EACH (node, other_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;
> > +        size_t i;
> > +
> > +        chassis_rec = port_binding_rec->chassis;
> > +        chassis_ip = get_chassis_vtep_ip(chassis_rec);
> > +
> > +        /* Unreachable chassis, continue. */
> > +        if (!chassis_ip) {
>
> It may be worth logging a rate-limited warning here if chassis_rec is
> non-NULL.  It seems like this could be pretty easy to hit with a config
> mistake (forgetting to configure the vxlan encap on a regular chassis).
>
>

At this stage, we do not know if this port_binding entry is in a logical
network extended by vtep switch or not...  so this could be a false alarm,

So, I move it a bit later and add the VLOG_INFO_RL().



> > +            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.
> > +         * 'ls_node' will be NULL, if the previous loop does not break,
> > +         * since the 'hmap_node' is the first member of the struct. */
>
> It's not a big deal, but Ben merged my patch that ensures ls_node will
> always be NULL, even if it isn't the first struct member, so the comment
> isn't needed anymore.
>
>
Yeah, I removed the part explaining 'hmap_node' is the first member of
struct.



>
>
> --
> Russell Bryant
>