On Tue, Jul 28, 2015 at 4:40 PM, Justin Pettit <jpet...@nicira.com> wrote:
> Do you think we should print a warning (at least once) to indicate that
> the integration bridge is missing? Otherwise, it could be confusing to
> someone why nothing's happening if they typo-ed their bridge name.
>
> Acked-by: Justin Pettit <jpet...@nicira.com>
>
> --Justin
>
>
> > On Jul 28, 2015, at 8:44 AM, Ben Pfaff <b...@nicira.com> wrote:
> >
> > Until now, if the integration bridge was missing, ovn-controller exited.
> > This commit makes it wait until the integration bridge is created.
> >
> > Signed-off-by: Ben Pfaff <b...@nicira.com>
> > ---
> > ovn/controller/binding.c | 4 +++-
> > ovn/controller/encaps.c | 2 +-
> > ovn/controller/ofctrl.c | 16 ++++++++++------
> > ovn/controller/ovn-controller.c | 39
> ++++++++++-----------------------------
> > 4 files changed, 24 insertions(+), 37 deletions(-)
> >
> > diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
> > index 2cb0b42..b6b345e 100644
> > --- 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.
> > 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.~
> > 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?
> > - free(target);
> >
> > rconn_run(swconn);
> >
> > diff --git a/ovn/controller/ovn-controller.c
> b/ovn/controller/ovn-controller.c
> > index e3c745a..d7ab7a3 100644
> > --- a/ovn/controller/ovn-controller.c
> > +++ b/ovn/controller/ovn-controller.c
> > @@ -108,7 +108,6 @@ get_core_config(struct controller_ctx *ctx, char
> **br_int_namep,
> > exit(EXIT_FAILURE);
> > }
> >
> > - const struct ovsrec_bridge *br_int;
> > const char *remote, *system_id, *br_int_name;
> >
> > br_int_name = smap_get(&cfg->external_ids, "ovn-bridge");
> > @@ -116,13 +115,6 @@ get_core_config(struct controller_ctx *ctx, char
> **br_int_namep,
> > br_int_name = DEFAULT_BRIDGE_NAME;
> > }
> >
> > - br_int = get_bridge(ctx, br_int_name);
> > - if (!br_int) {
> > - VLOG_INFO("Integration bridge '%s' does not exist.
> Waiting...",
> > - br_int_name);
> > - goto try_again;
> > - }
> > -
> > remote = smap_get(&cfg->external_ids, "ovn-remote");
> > if (!remote) {
> > VLOG_INFO("OVN OVSDB remote not specified. Waiting...");
> > @@ -280,24 +272,19 @@ main(int argc, char *argv[])
> > ctx.ovnsb_idl_txn = idl_loop_run(&ovnsb_idl_loop);
> > ctx.ovs_idl_txn = idl_loop_run(&ovs_idl_loop);
> >
> > - /* xxx If run into any surprising changes, we exit. We should
> > - * xxx handle this more gracefully. */
> > const struct ovsrec_bridge *br_int = get_bridge(&ctx,
> br_int_name);
> > - if (!br_int) {
> > - VLOG_ERR("Integration bridge '%s' disappeared",
> br_int_name);
> > - retval = EXIT_FAILURE;
> > - goto exit;
> > - }
> >
> > chassis_run(&ctx, chassis_id);
> > encaps_run(&ctx, br_int, chassis_id);
> > binding_run(&ctx, br_int, chassis_id);
> >
> > - struct hmap flow_table = HMAP_INITIALIZER(&flow_table);
> > - pipeline_run(&ctx, &flow_table);
> > - physical_run(&ctx, br_int, chassis_id, &flow_table);
> > - ofctrl_run(br_int, &flow_table);
> > - hmap_destroy(&flow_table);
> > + if (br_int) {
> > + struct hmap flow_table = HMAP_INITIALIZER(&flow_table);
> > + pipeline_run(&ctx, &flow_table);
> > + physical_run(&ctx, br_int, chassis_id, &flow_table);
> > + ofctrl_run(br_int, &flow_table);
> > + hmap_destroy(&flow_table);
> > + }
> >
> > unixctl_server_run(unixctl);
> >
> > @@ -309,7 +296,9 @@ main(int argc, char *argv[])
> > idl_loop_commit_and_wait(&ovnsb_idl_loop);
> > idl_loop_commit_and_wait(&ovs_idl_loop);
> >
> > - ofctrl_wait();
> > + if (br_int) {
> > + ofctrl_wait();
> > + }
> > poll_block();
> > }
> >
> > @@ -319,14 +308,7 @@ main(int argc, char *argv[])
> > ctx.ovnsb_idl_txn = idl_loop_run(&ovnsb_idl_loop);
> > ctx.ovs_idl_txn = idl_loop_run(&ovs_idl_loop);
> >
> > - /* xxx If run into any surprising changes, we exit. We should
> > - * xxx handle this more gracefully. */
> > const struct ovsrec_bridge *br_int = get_bridge(&ctx,
> br_int_name);
> > - if (!br_int) {
> > - VLOG_ERR("Integration bridge '%s' disappeared",
> br_int_name);
> > - retval = EXIT_FAILURE;
> > - goto exit;
> > - }
> >
> > /* Run all of the cleanup functions, even if one of them returns
> false.
> > * We're done if all of them return true. */
> > @@ -342,7 +324,6 @@ main(int argc, char *argv[])
> > poll_block();
> > }
> >
> > -exit:
> > unixctl_server_destroy(unixctl);
> > pipeline_destroy(&ctx);
> > ofctrl_destroy();
> > --
> > 2.1.3
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
