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

[ALeX Wang]

On 12 September 2015 at 16:43, Justin Pettit <jpettit at nicira.com> wrote:

>
> > 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'.
>


Fixed and added 'ctx->' prefix to all mentioned places,




> > +/* 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.
>
>
Removed the comments,



> > +        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.
>
>
will do that in future development,



> > +
> > +        /* 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.
>
>
Dropped the "somehow" and moved the last sentence as comment.



> > +/* 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.
>
>

I'm also unclear about if this is necessary.  The reason I wanted to reset
it, is
because the controller sets it.  And since there is not a good way to mark
which
tunnel key is set by controller, I just reset them all.

Maybe we can just remove this 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.
>

I think I did this after finishing the next patch.  And since vtep_run()
changes a lot in the next patch, I just moved it in this patch.



>
> > +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.
>
>
Fixed it,



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


-- 
Alex Wang,
Open vSwitch developer