| Message ID | 20170710180505.GI23376@ovn.org |
|---|---|
| State | Deferred |
| Headers | show |
On Mon, Jul 10, 2017 at 11:05 AM, Ben Pfaff <blp@ovn.org> wrote: > > On Wed, Jun 07, 2017 at 09:32:46AM -0700, Han Zhou wrote: > > This change is to prepare for the future change for multi-threading. > > Both binding_run() and get_br_int() are needed by pinctrl thread, > > but we don't want to update SB DB or create bridges in that scenario, > > so need "readonly" mode for these functions. > > > > Signed-off-by: Han Zhou <zhouhan@gmail.com> > > I prefer to separate code that just reads data from code that might > write to it, because my experience is that it's useful to be able to > spot the difference without looking closely at a function invocation. > Here is a version of the patch that does that. What do you think? > > --8<--------------------------cut here-------------------------->8-- > > From: Han Zhou <zhouhan@gmail.com> > Date: Wed, 7 Jun 2017 09:32:46 -0700 > Subject: [PATCH] ovn-controller: readonly mode binding_run and get_br_int > > This change is to prepare for the future change for multi-threading. > Both binding_run() and get_br_int() are needed by pinctrl thread, > but we don't want to update SB DB or create bridges in that scenario, > so need "readonly" mode for these functions. > > Signed-off-by: Han Zhou <zhouhan@gmail.com> > Signed-off-by: Ben Pfaff <blp@ovn.org> > --- > ovn/controller/binding.c | 247 +++++++++++++++++++++++----------------- > ovn/controller/binding.h | 4 + > ovn/controller/ovn-controller.c | 30 +++-- > 3 files changed, 167 insertions(+), 114 deletions(-) > > diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c > index 11145dd4cb22..59d06d306ef4 100644 > --- a/ovn/controller/binding.c > +++ b/ovn/controller/binding.c > @@ -67,36 +67,36 @@ binding_register_ovs_idl(struct ovsdb_idl *ovs_idl) > static void > get_local_iface_ids(const struct ovsrec_bridge *br_int, > struct shash *lport_to_iface, > - struct sset *local_lports, > - struct sset *egress_ifaces) > + struct sset *local_lports) > { > - int i; > - > - for (i = 0; i < br_int->n_ports; i++) { > - const struct ovsrec_port *port_rec = br_int->ports[i]; > - const char *iface_id; > - int j; > - > - if (!strcmp(port_rec->name, br_int->name)) { > - continue; > - } > - > - for (j = 0; j < port_rec->n_interfaces; j++) { > - const struct ovsrec_interface *iface_rec; > - > - iface_rec = port_rec->interfaces[j]; > - iface_id = smap_get(&iface_rec->external_ids, "iface-id"); > - int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0; > + for (int i = 0; i < br_int->n_ports; i++) { > + const struct ovsrec_port *port = br_int->ports[i]; > + for (int j = 0; j < port->n_interfaces; j++) { > + const struct ovsrec_interface *iface > + = port->interfaces[j]; > + const char *iface_id = smap_get(&iface->external_ids, "iface-id"); > + int64_t ofport = iface->n_ofport ? *iface->ofport : 0; > > if (iface_id && ofport > 0) { > - shash_add(lport_to_iface, iface_id, iface_rec); > + shash_add(lport_to_iface, iface_id, iface); > sset_add(local_lports, iface_id); > } > + } > + } > +} > + > +static void > +get_egress_ifaces(const struct ovsrec_bridge *br_int, > + struct sset *egress_ifaces) > +{ > + for (int i = 0; i < br_int->n_ports; i++) { > + const struct ovsrec_port *port = br_int->ports[i]; > + for (int j = 0; j < port->n_interfaces; j++) { > + const struct ovsrec_interface *iface = port->interfaces[j]; > > - /* Check if this is a tunnel interface. */ > - if (smap_get(&iface_rec->options, "remote_ip")) { > + if (smap_get(&iface->options, "remote_ip")) { > const char *tunnel_iface > - = smap_get(&iface_rec->status, "tunnel_egress_iface"); > + = smap_get(&iface->status, "tunnel_egress_iface"); > if (tunnel_iface) { > sset_add(egress_ifaces, tunnel_iface); > } > @@ -353,7 +353,19 @@ setup_qos(const char *egress_iface, struct hmap *queue_map) > netdev_close(netdev_phy); > } > > -static void > +static bool > +is_specially_bound(const struct sbrec_port_binding *binding_rec, > + const struct sbrec_chassis *chassis_rec, > + const char *type, const char *option) > +{ > + return (!strcmp(binding_rec->type, type) > + && !strcmp(smap_get_def(&binding_rec->options, option, ""), > + chassis_rec->name)); > +} > + > +/* Returns true if this chassis owns 'binding_rec', that is, it should set > + * 'binding_rec->chassis' to point to 'chassis_rec'. */ > +static bool > consider_local_datapath(struct controller_ctx *ctx, > const struct ldatapath_index *ldatapaths, > const struct lport_index *lports, > @@ -367,7 +379,6 @@ consider_local_datapath(struct controller_ctx *ctx, > const struct ovsrec_interface *iface_rec > = shash_find_data(lport_to_iface, binding_rec->logical_port); > > - bool our_chassis = false; > if (iface_rec > || (binding_rec->parent_port && binding_rec->parent_port[0] && > sset_contains(local_lports, binding_rec->parent_port))) { > @@ -380,65 +391,61 @@ consider_local_datapath(struct controller_ctx *ctx, > if (iface_rec && qos_map && ctx->ovs_idl_txn) { > get_qos_params(binding_rec, qos_map); > } > + > /* This port is in our chassis unless it is a localport. */ > - if (strcmp(binding_rec->type, "localport")) { > - our_chassis = true; > - } > - } else if (!strcmp(binding_rec->type, "l2gateway")) { > - const char *chassis_id = smap_get(&binding_rec->options, > - "l2gateway-chassis"); > - our_chassis = chassis_id && !strcmp(chassis_id, chassis_rec->name); > - if (our_chassis) { > - sset_add(local_lports, binding_rec->logical_port); > - add_local_datapath(ldatapaths, lports, binding_rec->datapath, > - false, local_datapaths); > - } > - } else if (!strcmp(binding_rec->type, "chassisredirect")) { > - const char *chassis_id = smap_get(&binding_rec->options, > - "redirect-chassis"); > - our_chassis = chassis_id && !strcmp(chassis_id, chassis_rec->name); > - if (our_chassis) { > - add_local_datapath(ldatapaths, lports, binding_rec->datapath, > - false, local_datapaths); > - } > - } else if (!strcmp(binding_rec->type, "l3gateway")) { > - const char *chassis_id = smap_get(&binding_rec->options, > - "l3gateway-chassis"); > - our_chassis = chassis_id && !strcmp(chassis_id, chassis_rec->name); > - if (our_chassis) { > - add_local_datapath(ldatapaths, lports, binding_rec->datapath, > - true, local_datapaths); > - } > + return strcmp(binding_rec->type, "localport"); > + } else if (is_specially_bound(binding_rec, chassis_rec, > + "l2gateway", "l2gateway-chassis")) { > + sset_add(local_lports, binding_rec->logical_port); > + add_local_datapath(ldatapaths, lports, binding_rec->datapath, > + false, local_datapaths); > + return true; > + } else if (is_specially_bound(binding_rec, chassis_rec, > + "chassisredirect", "redirect-chassis")) { > + add_local_datapath(ldatapaths, lports, binding_rec->datapath, > + false, local_datapaths); > + return true; > + } else if (is_specially_bound(binding_rec, chassis_rec, > + "l3gateway", "l3gateway-chassis")) { > + add_local_datapath(ldatapaths, lports, binding_rec->datapath, > + true, local_datapaths); > + return true; > } else if (!strcmp(binding_rec->type, "localnet")) { > /* Add all localnet ports to local_lports so that we allocate ct zones > * for them. */ > sset_add(local_lports, binding_rec->logical_port); > - our_chassis = false; > - } > - > - if (ctx->ovnsb_idl_txn) { > - if (our_chassis) { > - if (binding_rec->chassis != chassis_rec) { > - if (binding_rec->chassis) { > - VLOG_INFO("Changing chassis for lport %s from %s to %s.", > - binding_rec->logical_port, > - binding_rec->chassis->name, > - chassis_rec->name); > - } else { > - VLOG_INFO("Claiming lport %s for this chassis.", > - binding_rec->logical_port); > - } > - for (int i = 0; i < binding_rec->n_mac; i++) { > - VLOG_INFO("%s: Claiming %s", > - binding_rec->logical_port, binding_rec->mac[i]); > - } > - sbrec_port_binding_set_chassis(binding_rec, chassis_rec); > + return false; > + } else { > + return false; > + } > +} > + > +static void > +update_binding_ownership(const struct sbrec_chassis *chassis_rec, > + const struct sbrec_port_binding *binding_rec, > + bool should_own) > +{ > + if (should_own) { > + if (binding_rec->chassis != chassis_rec) { > + if (binding_rec->chassis) { > + VLOG_INFO("Changing chassis for lport %s from %s to %s.", > + binding_rec->logical_port, > + binding_rec->chassis->name, > + chassis_rec->name); > + } else { > + VLOG_INFO("Claiming lport %s for this chassis.", > + binding_rec->logical_port); > } > - } else if (binding_rec->chassis == chassis_rec) { > - VLOG_INFO("Releasing lport %s from this chassis.", > - binding_rec->logical_port); > - sbrec_port_binding_set_chassis(binding_rec, NULL); > + for (int i = 0; i < binding_rec->n_mac; i++) { > + VLOG_INFO("%s: Claiming %s", > + binding_rec->logical_port, binding_rec->mac[i]); > + } > + sbrec_port_binding_set_chassis(binding_rec, chassis_rec); > } > + } else if (binding_rec->chassis == chassis_rec) { > + VLOG_INFO("Releasing lport %s from this chassis.", > + binding_rec->logical_port); > + sbrec_port_binding_set_chassis(binding_rec, NULL); > } > } > > @@ -466,38 +473,30 @@ consider_localnet_port(const struct sbrec_port_binding *binding_rec, > ld->localnet_port = binding_rec; > } > > -void > -binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, > - const struct sbrec_chassis *chassis_rec, > - const struct ldatapath_index *ldatapaths, > - const struct lport_index *lports, struct hmap *local_datapaths, > - struct sset *local_lports) > +static void > +binding_run__(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, > + const struct sbrec_chassis *chassis_rec, > + const struct ldatapath_index *ldatapaths, > + const struct lport_index *lports, struct hmap *qos_map, > + struct hmap *local_datapaths, > + struct sset *local_lports, bool update_sb) > { > - if (!chassis_rec) { > - return; > - } > - > const struct sbrec_port_binding *binding_rec; > struct shash lport_to_iface = SHASH_INITIALIZER(&lport_to_iface); > - struct sset egress_ifaces = SSET_INITIALIZER(&egress_ifaces); > - struct hmap qos_map; > - > - hmap_init(&qos_map); > if (br_int) { > - get_local_iface_ids(br_int, &lport_to_iface, local_lports, > - &egress_ifaces); > + get_local_iface_ids(br_int, &lport_to_iface, local_lports); > } > > /* Run through each binding record to see if it is resident on this > * chassis and update the binding accordingly. This includes both > * directly connected logical ports and children of those ports. */ > SBREC_PORT_BINDING_FOR_EACH(binding_rec, ctx->ovnsb_idl) { > - consider_local_datapath(ctx, ldatapaths, lports, > - chassis_rec, binding_rec, > - sset_is_empty(&egress_ifaces) ? NULL : > - &qos_map, local_datapaths, &lport_to_iface, > - local_lports); > - > + bool should_own = consider_local_datapath( > + ctx, ldatapaths, lports, chassis_rec, binding_rec, > + qos_map, local_datapaths, &lport_to_iface, local_lports); > + if (ctx->ovnsb_idl_txn && update_sb) { > + update_binding_ownership(chassis_rec, binding_rec, should_own); > + } > } > > /* Run through each binding record to see if it is a localnet port > @@ -509,6 +508,34 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, > } > } > > + shash_destroy(&lport_to_iface); > +} > + > +/* Initializes 'local_datapaths' and 'local_lports' to the sets of logical > + * datapaths and logical ports, respectively, that are relevant to this > + * machine. Updates Port_Binding records 'chassis' columns to point to > + * 'chassis_rec' where appropriate. Sets up QoS appropriately on tunnel egress > + * interfaces. */ > +void > +binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, > + const struct sbrec_chassis *chassis_rec, > + const struct ldatapath_index *ldatapaths, > + const struct lport_index *lports, struct hmap *local_datapaths, > + struct sset *local_lports) > +{ > + if (!chassis_rec) { > + return; > + } > + > + struct sset egress_ifaces = SSET_INITIALIZER(&egress_ifaces); > + if (br_int) { > + get_egress_ifaces(br_int, &egress_ifaces); > + } > + > + struct hmap qos_map = HMAP_INITIALIZER(&qos_map); > + binding_run__(ctx, br_int, chassis_rec, ldatapaths, lports, > + sset_is_empty(&egress_ifaces) ? NULL : &qos_map, > + local_datapaths, local_lports, true); > if (!sset_is_empty(&egress_ifaces) > && set_noop_qos(ctx, &egress_ifaces)) { > const char *entry; > @@ -516,10 +543,26 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, > setup_qos(entry, &qos_map); > } > } > - > - shash_destroy(&lport_to_iface); > - sset_destroy(&egress_ifaces); > hmap_destroy(&qos_map); > + sset_destroy(&egress_ifaces); > +} > + > +/* Initializes 'local_datapaths' and 'local_lports' to the sets of logical > + * datapaths and logical ports, respectively, that are relevant to this > + * machine. */ > +void > +binding_get(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, > + const struct sbrec_chassis *chassis_rec, > + const struct ldatapath_index *ldatapaths, > + const struct lport_index *lports, struct hmap *local_datapaths, > + struct sset *local_lports) > +{ > + if (!chassis_rec) { > + return; > + } > + > + binding_run__(ctx, br_int, chassis_rec, ldatapaths, lports, NULL, > + local_datapaths, local_lports, false); > } > > /* Returns true if the database is all cleaned up, false if more work is > diff --git a/ovn/controller/binding.h b/ovn/controller/binding.h > index 3bfa7d1a924e..98a45a8c6233 100644 > --- a/ovn/controller/binding.h > +++ b/ovn/controller/binding.h > @@ -33,6 +33,10 @@ void binding_run(struct controller_ctx *, const struct ovsrec_bridge *br_int, > const struct sbrec_chassis *, const struct ldatapath_index *, > const struct lport_index *, struct hmap *local_datapaths, > struct sset *all_lports); > +void binding_get(struct controller_ctx *, const struct ovsrec_bridge *br_int, > + const struct sbrec_chassis *, const struct ldatapath_index *, > + const struct lport_index *, struct hmap *local_datapaths, > + struct sset *all_lports); > bool binding_cleanup(struct controller_ctx *, const struct sbrec_chassis *); > > #endif /* ovn/binding.h */ > diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c > index 45a670b5d754..cc01fe9e3477 100644 > --- a/ovn/controller/ovn-controller.c > +++ b/ovn/controller/ovn-controller.c > @@ -201,15 +201,26 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl, > ovsdb_idl_condition_destroy(&dns); > } > > +static const char * > +br_int_name(const struct ovsrec_open_vswitch *cfg) > +{ > + return smap_get_def(&cfg->external_ids, "ovn-bridge", DEFAULT_BRIDGE_NAME); > +} > + > static const struct ovsrec_bridge * > -create_br_int(struct controller_ctx *ctx, > - const struct ovsrec_open_vswitch *cfg, > - const char *bridge_name) > +create_br_int(struct controller_ctx *ctx) > { > if (!ctx->ovs_idl_txn) { > return NULL; > } > > + const struct ovsrec_open_vswitch *cfg; > + cfg = ovsrec_open_vswitch_first(ctx->ovs_idl); > + if (!cfg) { > + return NULL; > + } > + const char *bridge_name = br_int_name(cfg); > + > ovsdb_idl_txn_add_comment(ctx->ovs_idl_txn, > "ovn-controller: creating integration bridge '%s'", bridge_name); > > @@ -252,15 +263,7 @@ get_br_int(struct controller_ctx *ctx) > return NULL; > } > > - const char *br_int_name = smap_get_def(&cfg->external_ids, "ovn-bridge", > - DEFAULT_BRIDGE_NAME); > - > - const struct ovsrec_bridge *br; > - br = get_bridge(ctx->ovs_idl, br_int_name); > - if (!br) { > - return create_br_int(ctx, cfg, br_int_name); > - } > - return br; > + return get_bridge(ctx->ovs_idl, br_int_name(cfg)); > } > > static const char * > @@ -622,6 +625,9 @@ main(int argc, char *argv[]) > struct sset local_lports = SSET_INITIALIZER(&local_lports); > > const struct ovsrec_bridge *br_int = get_br_int(&ctx); > + if (!br_int) { > + br_int = create_br_int(&ctx); > + } > const char *chassis_id = get_chassis_id(ctx.ovs_idl); > > struct ldatapath_index ldatapaths; > -- > 2.10.2 > Ben, thanks for the suggestion. I agree it is better. I will test it and refactor if needed.
diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c index 11145dd4cb22..59d06d306ef4 100644 --- a/ovn/controller/binding.c +++ b/ovn/controller/binding.c @@ -67,36 +67,36 @@ binding_register_ovs_idl(struct ovsdb_idl *ovs_idl) static void get_local_iface_ids(const struct ovsrec_bridge *br_int, struct shash *lport_to_iface, - struct sset *local_lports, - struct sset *egress_ifaces) + struct sset *local_lports) { - int i; - - for (i = 0; i < br_int->n_ports; i++) { - const struct ovsrec_port *port_rec = br_int->ports[i]; - const char *iface_id; - int j; - - if (!strcmp(port_rec->name, br_int->name)) { - continue; - } - - for (j = 0; j < port_rec->n_interfaces; j++) { - const struct ovsrec_interface *iface_rec; - - iface_rec = port_rec->interfaces[j]; - iface_id = smap_get(&iface_rec->external_ids, "iface-id"); - int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0; + for (int i = 0; i < br_int->n_ports; i++) { + const struct ovsrec_port *port = br_int->ports[i]; + for (int j = 0; j < port->n_interfaces; j++) { + const struct ovsrec_interface *iface + = port->interfaces[j]; + const char *iface_id = smap_get(&iface->external_ids, "iface-id"); + int64_t ofport = iface->n_ofport ? *iface->ofport : 0; if (iface_id && ofport > 0) { - shash_add(lport_to_iface, iface_id, iface_rec); + shash_add(lport_to_iface, iface_id, iface); sset_add(local_lports, iface_id); } + } + } +} + +static void +get_egress_ifaces(const struct ovsrec_bridge *br_int, + struct sset *egress_ifaces) +{ + for (int i = 0; i < br_int->n_ports; i++) { + const struct ovsrec_port *port = br_int->ports[i]; + for (int j = 0; j < port->n_interfaces; j++) { + const struct ovsrec_interface *iface = port->interfaces[j]; - /* Check if this is a tunnel interface. */ - if (smap_get(&iface_rec->options, "remote_ip")) { + if (smap_get(&iface->options, "remote_ip")) { const char *tunnel_iface - = smap_get(&iface_rec->status, "tunnel_egress_iface"); + = smap_get(&iface->status, "tunnel_egress_iface"); if (tunnel_iface) { sset_add(egress_ifaces, tunnel_iface); } @@ -353,7 +353,19 @@ setup_qos(const char *egress_iface, struct hmap *queue_map) netdev_close(netdev_phy); } -static void +static bool +is_specially_bound(const struct sbrec_port_binding *binding_rec, + const struct sbrec_chassis *chassis_rec, + const char *type, const char *option) +{ + return (!strcmp(binding_rec->type, type) + && !strcmp(smap_get_def(&binding_rec->options, option, ""), + chassis_rec->name)); +} + +/* Returns true if this chassis owns 'binding_rec', that is, it should set + * 'binding_rec->chassis' to point to 'chassis_rec'. */ +static bool consider_local_datapath(struct controller_ctx *ctx, const struct ldatapath_index *ldatapaths, const struct lport_index *lports, @@ -367,7 +379,6 @@ consider_local_datapath(struct controller_ctx *ctx, const struct ovsrec_interface *iface_rec = shash_find_data(lport_to_iface, binding_rec->logical_port); - bool our_chassis = false; if (iface_rec || (binding_rec->parent_port && binding_rec->parent_port[0] && sset_contains(local_lports, binding_rec->parent_port))) { @@ -380,65 +391,61 @@ consider_local_datapath(struct controller_ctx *ctx, if (iface_rec && qos_map && ctx->ovs_idl_txn) { get_qos_params(binding_rec, qos_map); } + /* This port is in our chassis unless it is a localport. */ - if (strcmp(binding_rec->type, "localport")) { - our_chassis = true; - } - } else if (!strcmp(binding_rec->type, "l2gateway")) { - const char *chassis_id = smap_get(&binding_rec->options, - "l2gateway-chassis"); - our_chassis = chassis_id && !strcmp(chassis_id, chassis_rec->name); - if (our_chassis) { - sset_add(local_lports, binding_rec->logical_port); - add_local_datapath(ldatapaths, lports, binding_rec->datapath, - false, local_datapaths); - } - } else if (!strcmp(binding_rec->type, "chassisredirect")) { - const char *chassis_id = smap_get(&binding_rec->options, - "redirect-chassis"); - our_chassis = chassis_id && !strcmp(chassis_id, chassis_rec->name); - if (our_chassis) { - add_local_datapath(ldatapaths, lports, binding_rec->datapath, - false, local_datapaths); - } - } else if (!strcmp(binding_rec->type, "l3gateway")) { - const char *chassis_id = smap_get(&binding_rec->options, - "l3gateway-chassis"); - our_chassis = chassis_id && !strcmp(chassis_id, chassis_rec->name); - if (our_chassis) { - add_local_datapath(ldatapaths, lports, binding_rec->datapath, - true, local_datapaths); - } + return strcmp(binding_rec->type, "localport"); + } else if (is_specially_bound(binding_rec, chassis_rec, + "l2gateway", "l2gateway-chassis")) { + sset_add(local_lports, binding_rec->logical_port); + add_local_datapath(ldatapaths, lports, binding_rec->datapath, + false, local_datapaths); + return true; + } else if (is_specially_bound(binding_rec, chassis_rec, + "chassisredirect", "redirect-chassis")) { + add_local_datapath(ldatapaths, lports, binding_rec->datapath, + false, local_datapaths); + return true; + } else if (is_specially_bound(binding_rec, chassis_rec, + "l3gateway", "l3gateway-chassis")) { + add_local_datapath(ldatapaths, lports, binding_rec->datapath, + true, local_datapaths); + return true; } else if (!strcmp(binding_rec->type, "localnet")) { /* Add all localnet ports to local_lports so that we allocate ct zones * for them. */ sset_add(local_lports, binding_rec->logical_port); - our_chassis = false; - } - - if (ctx->ovnsb_idl_txn) { - if (our_chassis) { - if (binding_rec->chassis != chassis_rec) { - if (binding_rec->chassis) { - VLOG_INFO("Changing chassis for lport %s from %s to %s.", - binding_rec->logical_port, - binding_rec->chassis->name, - chassis_rec->name); - } else { - VLOG_INFO("Claiming lport %s for this chassis.", - binding_rec->logical_port); - } - for (int i = 0; i < binding_rec->n_mac; i++) { - VLOG_INFO("%s: Claiming %s", - binding_rec->logical_port, binding_rec->mac[i]); - } - sbrec_port_binding_set_chassis(binding_rec, chassis_rec); + return false; + } else { + return false; + } +} + +static void +update_binding_ownership(const struct sbrec_chassis *chassis_rec, + const struct sbrec_port_binding *binding_rec, + bool should_own) +{ + if (should_own) { + if (binding_rec->chassis != chassis_rec) { + if (binding_rec->chassis) { + VLOG_INFO("Changing chassis for lport %s from %s to %s.", + binding_rec->logical_port, + binding_rec->chassis->name, + chassis_rec->name); + } else { + VLOG_INFO("Claiming lport %s for this chassis.", + binding_rec->logical_port); } - } else if (binding_rec->chassis == chassis_rec) { - VLOG_INFO("Releasing lport %s from this chassis.", - binding_rec->logical_port); - sbrec_port_binding_set_chassis(binding_rec, NULL); + for (int i = 0; i < binding_rec->n_mac; i++) { + VLOG_INFO("%s: Claiming %s", + binding_rec->logical_port, binding_rec->mac[i]); + } + sbrec_port_binding_set_chassis(binding_rec, chassis_rec); } + } else if (binding_rec->chassis == chassis_rec) { + VLOG_INFO("Releasing lport %s from this chassis.", + binding_rec->logical_port); + sbrec_port_binding_set_chassis(binding_rec, NULL); } } @@ -466,38 +473,30 @@ consider_localnet_port(const struct sbrec_port_binding *binding_rec, ld->localnet_port = binding_rec; } -void -binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, - const struct sbrec_chassis *chassis_rec, - const struct ldatapath_index *ldatapaths, - const struct lport_index *lports, struct hmap *local_datapaths, - struct sset *local_lports) +static void +binding_run__(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, + const struct sbrec_chassis *chassis_rec, + const struct ldatapath_index *ldatapaths, + const struct lport_index *lports, struct hmap *qos_map, + struct hmap *local_datapaths, + struct sset *local_lports, bool update_sb) { - if (!chassis_rec) { - return; - } - const struct sbrec_port_binding *binding_rec; struct shash lport_to_iface = SHASH_INITIALIZER(&lport_to_iface); - struct sset egress_ifaces = SSET_INITIALIZER(&egress_ifaces); - struct hmap qos_map; - - hmap_init(&qos_map); if (br_int) { - get_local_iface_ids(br_int, &lport_to_iface, local_lports, - &egress_ifaces); + get_local_iface_ids(br_int, &lport_to_iface, local_lports); } /* Run through each binding record to see if it is resident on this * chassis and update the binding accordingly. This includes both * directly connected logical ports and children of those ports. */ SBREC_PORT_BINDING_FOR_EACH(binding_rec, ctx->ovnsb_idl) { - consider_local_datapath(ctx, ldatapaths, lports, - chassis_rec, binding_rec, - sset_is_empty(&egress_ifaces) ? NULL : - &qos_map, local_datapaths, &lport_to_iface, - local_lports); - + bool should_own = consider_local_datapath( + ctx, ldatapaths, lports, chassis_rec, binding_rec, + qos_map, local_datapaths, &lport_to_iface, local_lports); + if (ctx->ovnsb_idl_txn && update_sb) { + update_binding_ownership(chassis_rec, binding_rec, should_own); + } } /* Run through each binding record to see if it is a localnet port @@ -509,6 +508,34 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, } } + shash_destroy(&lport_to_iface); +} + +/* Initializes 'local_datapaths' and 'local_lports' to the sets of logical + * datapaths and logical ports, respectively, that are relevant to this + * machine. Updates Port_Binding records 'chassis' columns to point to + * 'chassis_rec' where appropriate. Sets up QoS appropriately on tunnel egress + * interfaces. */ +void +binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, + const struct sbrec_chassis *chassis_rec, + const struct ldatapath_index *ldatapaths, + const struct lport_index *lports, struct hmap *local_datapaths, + struct sset *local_lports) +{ + if (!chassis_rec) { + return; + } + + struct sset egress_ifaces = SSET_INITIALIZER(&egress_ifaces); + if (br_int) { + get_egress_ifaces(br_int, &egress_ifaces); + } + + struct hmap qos_map = HMAP_INITIALIZER(&qos_map); + binding_run__(ctx, br_int, chassis_rec, ldatapaths, lports, + sset_is_empty(&egress_ifaces) ? NULL : &qos_map, + local_datapaths, local_lports, true); if (!sset_is_empty(&egress_ifaces) && set_noop_qos(ctx, &egress_ifaces)) { const char *entry; @@ -516,10 +543,26 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, setup_qos(entry, &qos_map); } } - - shash_destroy(&lport_to_iface); - sset_destroy(&egress_ifaces); hmap_destroy(&qos_map); + sset_destroy(&egress_ifaces); +} + +/* Initializes 'local_datapaths' and 'local_lports' to the sets of logical + * datapaths and logical ports, respectively, that are relevant to this + * machine. */ +void +binding_get(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, + const struct sbrec_chassis *chassis_rec, + const struct ldatapath_index *ldatapaths, + const struct lport_index *lports, struct hmap *local_datapaths, + struct sset *local_lports) +{ + if (!chassis_rec) { + return; + } + + binding_run__(ctx, br_int, chassis_rec, ldatapaths, lports, NULL, + local_datapaths, local_lports, false); } /* Returns true if the database is all cleaned up, false if more work is diff --git a/ovn/controller/binding.h b/ovn/controller/binding.h index 3bfa7d1a924e..98a45a8c6233 100644 --- a/ovn/controller/binding.h +++ b/ovn/controller/binding.h @@ -33,6 +33,10 @@ void binding_run(struct controller_ctx *, const struct ovsrec_bridge *br_int, const struct sbrec_chassis *, const struct ldatapath_index *, const struct lport_index *, struct hmap *local_datapaths, struct sset *all_lports); +void binding_get(struct controller_ctx *, const struct ovsrec_bridge *br_int, + const struct sbrec_chassis *, const struct ldatapath_index *, + const struct lport_index *, struct hmap *local_datapaths, + struct sset *all_lports); bool binding_cleanup(struct controller_ctx *, const struct sbrec_chassis *); #endif /* ovn/binding.h */ diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c index 45a670b5d754..cc01fe9e3477 100644 --- a/ovn/controller/ovn-controller.c +++ b/ovn/controller/ovn-controller.c @@ -201,15 +201,26 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl, ovsdb_idl_condition_destroy(&dns); } +static const char * +br_int_name(const struct ovsrec_open_vswitch *cfg) +{ + return smap_get_def(&cfg->external_ids, "ovn-bridge", DEFAULT_BRIDGE_NAME); +} + static const struct ovsrec_bridge * -create_br_int(struct controller_ctx *ctx, - const struct ovsrec_open_vswitch *cfg, - const char *bridge_name) +create_br_int(struct controller_ctx *ctx) { if (!ctx->ovs_idl_txn) { return NULL; } + const struct ovsrec_open_vswitch *cfg; + cfg = ovsrec_open_vswitch_first(ctx->ovs_idl); + if (!cfg) { + return NULL; + } + const char *bridge_name = br_int_name(cfg); + ovsdb_idl_txn_add_comment(ctx->ovs_idl_txn, "ovn-controller: creating integration bridge '%s'", bridge_name); @@ -252,15 +263,7 @@ get_br_int(struct controller_ctx *ctx) return NULL; } - const char *br_int_name = smap_get_def(&cfg->external_ids, "ovn-bridge", - DEFAULT_BRIDGE_NAME); - - const struct ovsrec_bridge *br; - br = get_bridge(ctx->ovs_idl, br_int_name); - if (!br) { - return create_br_int(ctx, cfg, br_int_name); - } - return br; + return get_bridge(ctx->ovs_idl, br_int_name(cfg)); } static const char * @@ -622,6 +625,9 @@ main(int argc, char *argv[]) struct sset local_lports = SSET_INITIALIZER(&local_lports); const struct ovsrec_bridge *br_int = get_br_int(&ctx); + if (!br_int) { + br_int = create_br_int(&ctx); + } const char *chassis_id = get_chassis_id(ctx.ovs_idl); struct ldatapath_index ldatapaths;