[ovs-dev] [PATCH V9 1/3] ovn-controller-vtep: Add vtep module.

[Justin Pettit]

> On Aug 27, 2015, at 11:21 PM, Alex Wang <ee07b291 at gmail.com> wrote:
> 
> 
> --- a/ovn/controller-vtep/binding.c
> +++ b/ovn/controller-vtep/binding.c
> @@ -226,7 +226,8 @@ binding_run(struct controller_vtep_ctx *ctx)
> }
> 
> /* Removes all port binding association with vtep gateway chassis.
> - * Returns true when all done. */
> + * Returns true when done (i.e. there is no change made to 'ovnsb_idl'),
> + * otherwise returns false. */

I think it might be nice to be explicit that it's 'ctx->ovnsb_idl'.

> --- a/ovn/controller-vtep/gateway.c
> +++ b/ovn/controller-vtep/gateway.c
> @@ -189,7 +189,8 @@ gateway_run(struct controller_vtep_ctx *ctx)
> }
> 
> /* Destroys the chassis table entries for vtep physical switches.
> - * Returns true when all done. */
> + * Returns true when done (i.e. there is no change made to 'ovnsb_idl'),
> + * otherwise returns false. */

Same here about 'ctx->ovnsb_idl'.

> +/* Updates the vtep Logical_Switch table entries' tunnel keys based
> + * on the port bindings. */
> +static void
> +vtep_lswitch_run(struct controller_vtep_ctx *ctx)
> +{
> +    struct shash vtep_lswitches = SHASH_INITIALIZER(&vtep_lswitches);
> +    struct sset vtep_pswitches = SSET_INITIALIZER(&vtep_pswitches);
> +    struct sset used_ls = SSET_INITIALIZER(&used_ls);
> +    const struct vteprec_physical_switch *pswitch;
> +    const struct sbrec_port_binding *port_binding_rec;
> +    const struct vteprec_logical_switch *vtep_ls;
> +
> +    /* Registers all vtep physical switch names in the vtep database. */
> +    VTEPREC_PHYSICAL_SWITCH_FOR_EACH (pswitch, ctx->vtep_idl) {
> +        sset_add(&vtep_pswitches, pswitch->name);
> +    }

I think this comment is sort of ambiguous.  My first reading was that it was storing the switches into the vtep database, which is clearly not happening.  I think you could just drop the comment, since the variable name is good.

> +        const char *pswitch_name = smap_get(&port_binding_rec->options,
> +                                            "vtep-physical-switch");
> +        const char *lswitch_name = smap_get(&port_binding_rec->options,
> +                                            "vtep-logical-switch");

It's not worth reworking the patches now, but it's nice to have the documentation combined with the code that uses it.  For example , these options are described in the third patch of this series, but they could have been included in this one.

> +
> +        /* If 'port_binding_rec->chassis' exists then 'pswitch_name'
> +         * and 'lswitch_name' must also exist. */
> +        if (!pswitch_name || !lswitch_name) {
> +            VLOG_ERR("logical port (%s) with no 'options:vtep-physical-"
> +                     "switch' or 'options:vtep-logical-switch' specified "
> +                     "is somehow bound to chassis (%s).  this could only "
> +                     "happen when someone is messing up using ovn-sbctl",

I think you can drop "somehow".  The last sentence is a bit accusatory.  I think you may be able to just drop the sentence.  You could note in the code that it shouldn't happen unless the database has been directly modified.

> +/* Since we do not own any vtep logical switch, just sets their tunnel key
> + * to 0. */
> +static bool
> +vtep_lswitch_cleanup(struct ovsdb_idl *vtep_idl)
> +{
> +    const struct vteprec_logical_switch *vtep_ls;
> +    int64_t tnl_key = 0;
> +    bool done = true;
> +
> +    VTEPREC_LOGICAL_SWITCH_FOR_EACH (vtep_ls, vtep_idl) {
> +        if (vtep_ls->n_tunnel_key != 1
> +            || vtep_ls->tunnel_key[0] != tnl_key) {
> +            vteprec_logical_switch_set_tunnel_key(vtep_ls, &tnl_key, 1);

It looks like the tunnel key is optional.  This code will set a tunnel key even if one wasn't previously set.  Is this intentional?  I was just surprised to see this in a cleanup function.

> +/* Updates vtep logical switch tunnel keys. */
> +void
> +vtep_run(struct controller_vtep_ctx *ctx)
> +{
> +    if (!ctx->vtep_idl_txn) {
> +        return;
> +    }
> +    vtep_lswitch_run(ctx);
> +}

Is there a reason not to fold the logic in vtep_lswitch_run() into this function?  I see that in the next patch that vtep_cleanup() does more than just call vtep_lswitch_cleanup(), but it doesn't look like vtep_lswitch_run() gains any logic later.

> +/* Cleans up all related entries in vtep.  Returns true when done (i.e.
> + * there is no change made to 'vtep_idl'), otherwise returns false. */

Once again, might be good to mention 'ctx->vtep_idl'.

> +bool
> +vtep_cleanup(struct controller_vtep_ctx *ctx)
> +{
> +    if (!ctx->vtep_idl_txn) {
> +        return false;
> +    }
> +
> +    ovsdb_idl_txn_add_comment(ctx->vtep_idl_txn,
> +                              "cleans up vtep configuration");

I think it would be more consistent with the other cleanup functions to use "cleaning" instead of "cleans".  It would be good to prepend this comment with "ovn-controller-vtep:" so we know who did it.

Acked-by: Justin Pettit <jpettit at nicira.com>

--Justin