On Tue, Jul 28, 2015 at 05:48:50PM -0700, Alex Wang wrote:
> On Tue, Jul 28, 2015 at 4:40 PM, Justin Pettit <jpet...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
