[ovs-dev] [PATCH v2 08/21] ovn-controller: Tolerate missing integration bridge.

Alex Wang alexw at nicira.com
Wed Jul 29 16:08:34 UTC 2015


Thx, all look good to me~

On Wed, Jul 29, 2015 at 9:02 AM, Ben Pfaff <blp at nicira.com> wrote:

> On Tue, Jul 28, 2015 at 05:48:50PM -0700, Alex Wang wrote:
> > On Tue, Jul 28, 2015 at 4:40 PM, Justin Pettit <jpettit at nicira.com>
> wrote:
> > > > --- a/ovn/controller/binding.c
> > > > +++ b/ovn/controller/binding.c
> > > > @@ -91,7 +91,9 @@ binding_run(struct controller_ctx *ctx, const
> struct
> > > ovsrec_bridge *br_int,
> > > >
> > > >     sset_init(&lports);
> > > >     sset_init(&all_lports);
> > > > -    get_local_iface_ids(br_int, &lports);
> > > > +    if (br_int) {
> > > > +        get_local_iface_ids(br_int, &lports);
> > > > +    }
> > >
> >
> > Could we add a comment here?  If I understand correctly, if br_int is
> NULL,
> > we will reset all binding table entries with chassis column referring to
> > this
> > HV chassis.
>
> Fair enough, I folded this in:
>
> diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
> index b6b345e..849dddf0 100644
> --- a/ovn/controller/binding.c
> +++ b/ovn/controller/binding.c
> @@ -93,6 +93,9 @@ binding_run(struct controller_ctx *ctx, const struct
> ovsrec_bridge *br_int,
>      sset_init(&all_lports);
>      if (br_int) {
>          get_local_iface_ids(br_int, &lports);
> +    } else {
> +        /* We have no integration bridge, therefore no local logical
> ports.
> +         * We'll remove our chassis from all port binding records below.
> */
>      }
>      sset_clone(&all_lports, &lports);
>
> > > >     sset_clone(&all_lports, &lports);
> > > >
> > > >     ovsdb_idl_txn_add_comment(ctx->ovnsb_idl_txn,
> > > > diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c
> > > > index 7aa523f..f150246 100644
> > > > --- a/ovn/controller/encaps.c
> > > > +++ b/ovn/controller/encaps.c
> > > > @@ -231,7 +231,7 @@ void
> > > > encaps_run(struct controller_ctx *ctx, const struct ovsrec_bridge
> > > *br_int,
> > > >            const char *chassis_id)
> > > > {
> > > > -    if (!ctx->ovs_idl_txn) {
> > > > +    if (!ctx->ovs_idl_txn || !br_int) {
> > > >         return;
> > > >     }
> > > >
> > >
> >
> >
> > I think we also need to do this for the encaps_cleanup().  If br_int is
> > NULL,
> > simply return true.~
>
> Good spotting, thanks!  I fixed that:
>
> diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c
> index f150246..5ae9267 100644
> --- a/ovn/controller/encaps.c
> +++ b/ovn/controller/encaps.c
> @@ -298,6 +298,10 @@ encaps_run(struct controller_ctx *ctx, const struct
> ovsrec_bridge *br_int,
>  bool
>  encaps_cleanup(struct controller_ctx *ctx, const struct ovsrec_bridge
> *br_int)
>  {
> +    if (!br_int) {
> +        return true;
> +    }
> +
>      /* Delete all the OVS-created tunnels from the integration bridge. */
>      struct ovsrec_port **ports
>          = xmalloc(sizeof *br_int->ports * br_int->n_ports);
>
> > > > diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
> > > > index c548645..b1c421c 100644
> > > > --- a/ovn/controller/ofctrl.c
> > > > +++ b/ovn/controller/ofctrl.c
> > > > @@ -92,13 +92,17 @@ ofctrl_init(void)
> > > > void
> > > > ofctrl_run(const struct ovsrec_bridge *br_int, struct hmap
> *flow_table)
> > > > {
> > > > -    char *target;
> > > > -    target = xasprintf("unix:%s/%s.mgmt", ovs_rundir(),
> br_int->name);
> > > > -    if (strcmp(target, rconn_get_target(swconn))) {
> > > > -        VLOG_INFO("%s: connecting to switch", target);
> > > > -        rconn_connect(swconn, target, target);
> > > > +    if (br_int) {
> > > > +        char *target;
> > > > +        target = xasprintf("unix:%s/%s.mgmt", ovs_rundir(),
> > > br_int->name);
> > > > +        if (strcmp(target, rconn_get_target(swconn))) {
> > > > +            VLOG_INFO("%s: connecting to switch", target);
> > > > +            rconn_connect(swconn, target, target);
> > > > +        }
> > > > +        free(target);
> > > > +    } else {
> > > > +        rconn_disconnect(swconn);
> > > >     }
> >
> > Curious about the use case when ofctrl_run() is called with br_int being
> > NULL...  And we just close the rconn?  And what if someone call it when
> > swconn is still NULL?
>
> We disconnect the rconn in that case, but disconnecting isn't the same
> as destroying; it still exists and can be reconnected later.  swconn
> never becomes NULL.
>
> > > > -    free(target);
> > > >
> > > >     rconn_run(swconn);
>
> Thanks a lot for the comments!
>
> Ben
>



More information about the dev mailing list