| Message ID | 1469805529-10267-1-git-send-email-russell@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
On Fri, Jul 29, 2016 at 11:18 AM, Russell Bryant <russell@ovn.org> wrote: > From: Babu Shanmugam <bschanmu@redhat.com> > > Recent commits reorganizing bindings handling and also moving ct zone > assignment to ovn-controller.c caused ct zone assignment to no longer > work. The code relies on an "all_lports" sset that should contain all > logical ports that we should be assigning ct zones for. Prior to this > change, all_lports was always empty. > > Signed-off-by: Babu Shanmugam <bschanmu@redhat.com> > Co-authored-by: Russell Bryant <russell@ovn.org> > Signed-off-by: Russell Bryant <russell@ovn.org> > @@ -434,7 +437,9 @@ main(int argc, char *argv[]) > if (chassis_id) { > chassis = chassis_run(&ctx, chassis_id); > encaps_run(&ctx, br_int, chassis_id); > - binding_run(&ctx, br_int, chassis_id, &local_datapaths); > + binding_run(&ctx, br_int, chassis_id, &local_datapaths, > + &all_lports); > + VLOG_INFO("all_lports size: %lu", sset_count(&all_lports)); > Consider this stray debug entry removed ...
"dev" <dev-bounces@openvswitch.org> wrote on 07/29/2016 10:18:49 AM: > From: Russell Bryant <russell@ovn.org> > To: dev@openvswitch.org > Date: 07/29/2016 10:19 AM > Subject: [ovs-dev] [PATCH v2] ovn-controller: Restore ct zone assignment. > Sent by: "dev" <dev-bounces@openvswitch.org> > > From: Babu Shanmugam <bschanmu@redhat.com> > > Recent commits reorganizing bindings handling and also moving ct zone > assignment to ovn-controller.c caused ct zone assignment to no longer > work. The code relies on an "all_lports" sset that should contain all > logical ports that we should be assigning ct zones for. Prior to this > change, all_lports was always empty. > > Signed-off-by: Babu Shanmugam <bschanmu@redhat.com> > Co-authored-by: Russell Bryant <russell@ovn.org> > Signed-off-by: Russell Bryant <russell@ovn.org> > --- <snip> > @@ -278,6 +303,14 @@ binding_run(struct controller_ctx *ctx, const > struct ovsrec_bridge *br_int, > } > } > hmap_destroy(&keep_local_datapath_by_uuid); > + > + /* Any remaining entries in removed_lports are logical ports that > + * have been deleted and should also be removed from all_ports. */ > + const char *cur_id; > + SSET_FOR_EACH(cur_id, &removed_lports) { > + sset_find_and_delete(all_lports, cur_id); > + } > + > process_full_binding = false; > } else { > SBREC_PORT_BINDING_FOR_EACH_TRACKED(binding_rec, ctx->ovnsb_idl) { > @@ -292,7 +325,8 @@ binding_run(struct controller_ctx *ctx, const > struct ovsrec_bridge *br_int, > } > } else { > consider_local_datapath(ctx, chassis_rec, binding_rec, > - local_datapaths, &lport_to_iface); > + local_datapaths, &lport_to_iface, > + all_lports); > } > } > } A quick look at this leads me to ask about cleaning up entries derived from binding records that are found to be deleted during incremental processing? The code leads me to believe that they will hang around as stale at least until the next full processing cycle and I don't think we want that... Otherwise, this looks good (modulo the debugging log statement :) ) Ryan
On Fri, Jul 29, 2016 at 11:59 AM, Ryan Moats <rmoats@us.ibm.com> wrote: > "dev" <dev-bounces@openvswitch.org> wrote on 07/29/2016 10:18:49 AM: > > > From: Russell Bryant <russell@ovn.org> > > To: dev@openvswitch.org > > Date: 07/29/2016 10:19 AM > > Subject: [ovs-dev] [PATCH v2] ovn-controller: Restore ct zone assignment. > > Sent by: "dev" <dev-bounces@openvswitch.org> > > > > From: Babu Shanmugam <bschanmu@redhat.com> > > > > Recent commits reorganizing bindings handling and also moving ct zone > > assignment to ovn-controller.c caused ct zone assignment to no longer > > work. The code relies on an "all_lports" sset that should contain all > > logical ports that we should be assigning ct zones for. Prior to this > > change, all_lports was always empty. > > > > Signed-off-by: Babu Shanmugam <bschanmu@redhat.com> > > Co-authored-by: Russell Bryant <russell@ovn.org> > > Signed-off-by: Russell Bryant <russell@ovn.org> > > --- > > <snip> > > > @@ -278,6 +303,14 @@ binding_run(struct controller_ctx *ctx, const > > struct ovsrec_bridge *br_int, > > } > > } > > hmap_destroy(&keep_local_datapath_by_uuid); > > + > > + /* Any remaining entries in removed_lports are logical ports > that > > + * have been deleted and should also be removed from all_ports. > */ > > + const char *cur_id; > > + SSET_FOR_EACH(cur_id, &removed_lports) { > > + sset_find_and_delete(all_lports, cur_id); > > + } > > + > > process_full_binding = false; > > } else { > > SBREC_PORT_BINDING_FOR_EACH_TRACKED(binding_rec, > ctx->ovnsb_idl) { > > @@ -292,7 +325,8 @@ binding_run(struct controller_ctx *ctx, const > > struct ovsrec_bridge *br_int, > > } > > } else { > > consider_local_datapath(ctx, chassis_rec, binding_rec, > > - local_datapaths, > &lport_to_iface); > > + local_datapaths, > &lport_to_iface, > > + all_lports); > > } > > } > > } > > A quick look at this leads me to ask about cleaning up entries > derived from binding records that are found to be deleted during > incremental processing? The code leads me to believe that they > will hang around as stale at least until the next full processing > cycle and I don't think we want that... > I think it's handled. See this block. The immediate full bindings refresh will get all_lports cleaned up, as well. SBREC_PORT_BINDING_FOR_EACH_TRACKED(binding_rec, ctx->ovnsb_idl) { if (sbrec_port_binding_is_deleted(binding_rec)) { /* If a port binding was bound to this chassis and removed before * the ovs interface was removed, we'll catch that here and trigger * a full bindings refresh. This is to see if we need to clear * an entry out of local_datapaths. */ if (binding_rec->chassis == chassis_rec) { process_full_binding = true; poll_immediate_wake(); }
Russell Bryant <russell@ovn.org> wrote on 07/29/2016 11:27:49 AM: > From: Russell Bryant <russell@ovn.org> > To: Ryan Moats/Omaha/IBM@IBMUS > Cc: ovs dev <dev@openvswitch.org> > Date: 07/29/2016 11:28 AM > Subject: Re: [ovs-dev] [PATCH v2] ovn-controller: Restore ct zone assignment. > > On Fri, Jul 29, 2016 at 11:59 AM, Ryan Moats <rmoats@us.ibm.com> wrote: > "dev" <dev-bounces@openvswitch.org> wrote on 07/29/2016 10:18:49 AM: > > > From: Russell Bryant <russell@ovn.org> > > To: dev@openvswitch.org > > Date: 07/29/2016 10:19 AM > > Subject: [ovs-dev] [PATCH v2] ovn-controller: Restore ct zone assignment. > > Sent by: "dev" <dev-bounces@openvswitch.org> > > > > From: Babu Shanmugam <bschanmu@redhat.com> > > > > Recent commits reorganizing bindings handling and also moving ct zone > > assignment to ovn-controller.c caused ct zone assignment to no longer > > work. The code relies on an "all_lports" sset that should contain all > > logical ports that we should be assigning ct zones for. Prior to this > > change, all_lports was always empty. > > > > Signed-off-by: Babu Shanmugam <bschanmu@redhat.com> > > Co-authored-by: Russell Bryant <russell@ovn.org> > > Signed-off-by: Russell Bryant <russell@ovn.org> > > --- > > <snip> > > > @@ -278,6 +303,14 @@ binding_run(struct controller_ctx *ctx, const > > struct ovsrec_bridge *br_int, > > } > > } > > hmap_destroy(&keep_local_datapath_by_uuid); > > + > > + /* Any remaining entries in removed_lports are logical ports that > > + * have been deleted and should also be removed from all_ports. */ > > + const char *cur_id; > > + SSET_FOR_EACH(cur_id, &removed_lports) { > > + sset_find_and_delete(all_lports, cur_id); > > + } > > + > > process_full_binding = false; > > } else { > > SBREC_PORT_BINDING_FOR_EACH_TRACKED(binding_rec, ctx-> ovnsb_idl) { > > @@ -292,7 +325,8 @@ binding_run(struct controller_ctx *ctx, const > > struct ovsrec_bridge *br_int, > > } > > } else { > > consider_local_datapath(ctx, chassis_rec, binding_rec, > > - local_datapaths, &lport_to_iface); > > + local_datapaths, &lport_to_iface, > > + all_lports); > > } > > } > > } > > A quick look at this leads me to ask about cleaning up entries > derived from binding records that are found to be deleted during > incremental processing? The code leads me to believe that they > will hang around as stale at least until the next full processing > cycle and I don't think we want that... > I think it's handled. See this block. The immediate full bindings > refresh will get all_lports cleaned up, as well. > > SBREC_PORT_BINDING_FOR_EACH_TRACKED(binding_rec, ctx->ovnsb_idl) { > if (sbrec_port_binding_is_deleted(binding_rec)) { > /* If a port binding was bound to this chassis and > removed before > * the ovs interface was removed, we'll catch that > here and trigger > * a full bindings refresh. This is to see if we > need to clear > * an entry out of local_datapaths. */ > if (binding_rec->chassis == chassis_rec) { > process_full_binding = true; > poll_immediate_wake(); > } > > -- > Russell Bryant Well, I may not entirely like it but yes, it does do it so ... Begrudgingly- Acked-by: Ryan Moats <rmoats@us.ibm.com>
On Fri, Jul 29, 2016 at 3:52 PM, Ryan Moats <rmoats@us.ibm.com> wrote:
> Acked-by: Ryan Moats <rmoats@us.ibm.com>
Thanks, applied to master.
diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c index 41165bc..3073727 100644 --- a/ovn/controller/binding.c +++ b/ovn/controller/binding.c @@ -66,7 +66,8 @@ binding_register_ovs_idl(struct ovsdb_idl *ovs_idl) static bool get_local_iface_ids(const struct ovsrec_bridge *br_int, - struct shash *lport_to_iface) + struct shash *lport_to_iface, + struct sset *all_lports) { int i; bool changed = false; @@ -94,6 +95,7 @@ get_local_iface_ids(const struct ovsrec_bridge *br_int, shash_add(lport_to_iface, iface_id, iface_rec); if (!sset_find_and_delete(&old_local_ids, iface_id)) { sset_add(&local_ids, iface_id); + sset_add(all_lports, iface_id); changed = true; } } @@ -107,6 +109,7 @@ get_local_iface_ids(const struct ovsrec_bridge *br_int, const char *cur_id; SSET_FOR_EACH(cur_id, &old_local_ids) { sset_find_and_delete(&local_ids, cur_id); + sset_find_and_delete(all_lports, cur_id); } } @@ -174,7 +177,8 @@ consider_local_datapath(struct controller_ctx *ctx, const struct sbrec_chassis *chassis_rec, const struct sbrec_port_binding *binding_rec, struct hmap *local_datapaths, - struct shash *lport_to_iface) + struct shash *lport_to_iface, + struct sset *all_lports) { const struct ovsrec_interface *iface_rec = shash_find_data(lport_to_iface, binding_rec->logical_port); @@ -200,6 +204,9 @@ consider_local_datapath(struct controller_ctx *ctx, binding_rec->logical_port); } sbrec_port_binding_set_chassis(binding_rec, chassis_rec); + if (binding_rec->parent_port && binding_rec->parent_port[0]) { + sset_add(all_lports, binding_rec->logical_port); + } } } else if (!strcmp(binding_rec->type, "l2gateway")) { const char *chassis_id = smap_get(&binding_rec->options, @@ -209,6 +216,7 @@ consider_local_datapath(struct controller_ctx *ctx, VLOG_INFO("Releasing l2gateway port %s from this chassis.", binding_rec->logical_port); sbrec_port_binding_set_chassis(binding_rec, NULL); + sset_find_and_delete(all_lports, binding_rec->logical_port); } return; } @@ -221,6 +229,7 @@ consider_local_datapath(struct controller_ctx *ctx, VLOG_INFO("Claiming l2gateway port %s for this chassis.", binding_rec->logical_port); sbrec_port_binding_set_chassis(binding_rec, chassis_rec); + sset_add(all_lports, binding_rec->logical_port); add_local_datapath(local_datapaths, binding_rec); } } else if (chassis_rec && binding_rec->chassis == chassis_rec @@ -229,13 +238,20 @@ consider_local_datapath(struct controller_ctx *ctx, VLOG_INFO("Releasing lport %s from this chassis.", binding_rec->logical_port); sbrec_port_binding_set_chassis(binding_rec, NULL); + sset_find_and_delete(all_lports, binding_rec->logical_port); } + } else if (!binding_rec->chassis + && !strcmp(binding_rec->type, "localnet")) { + /* Add all localnet ports to all_lports so that we allocate ct zones + * for them. */ + sset_add(all_lports, binding_rec->logical_port); } } void binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, - const char *chassis_id, struct hmap *local_datapaths) + const char *chassis_id, struct hmap *local_datapaths, + struct sset *all_lports) { const struct sbrec_chassis *chassis_rec; const struct sbrec_port_binding *binding_rec; @@ -247,7 +263,8 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, } if (br_int) { - if (ctx->ovnsb_idl_txn && get_local_iface_ids(br_int, &lport_to_iface)) { + if (ctx->ovnsb_idl_txn && get_local_iface_ids(br_int, &lport_to_iface, + all_lports)) { process_full_binding = true; } } else { @@ -260,11 +277,19 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, * chassis and update the binding accordingly. This includes both * directly connected logical ports and children of those ports. */ if (process_full_binding) { + /* Detect any entries in all_lports that have been deleted. + * In particular, this will catch localnet ports that we + * put in all_lports. */ + struct sset removed_lports = SSET_INITIALIZER(&removed_lports); + sset_clone(&removed_lports, all_lports); + struct hmap keep_local_datapath_by_uuid = HMAP_INITIALIZER(&keep_local_datapath_by_uuid); SBREC_PORT_BINDING_FOR_EACH(binding_rec, ctx->ovnsb_idl) { + sset_find_and_delete(&removed_lports, binding_rec->logical_port); consider_local_datapath(ctx, chassis_rec, binding_rec, - local_datapaths, &lport_to_iface); + local_datapaths, &lport_to_iface, + all_lports); struct local_datapath *ld = xzalloc(sizeof *ld); memcpy(&ld->uuid, &binding_rec->header_.uuid, sizeof ld->uuid); hmap_insert(&keep_local_datapath_by_uuid, &ld->uuid_hmap_node, @@ -278,6 +303,14 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, } } hmap_destroy(&keep_local_datapath_by_uuid); + + /* Any remaining entries in removed_lports are logical ports that + * have been deleted and should also be removed from all_ports. */ + const char *cur_id; + SSET_FOR_EACH(cur_id, &removed_lports) { + sset_find_and_delete(all_lports, cur_id); + } + process_full_binding = false; } else { SBREC_PORT_BINDING_FOR_EACH_TRACKED(binding_rec, ctx->ovnsb_idl) { @@ -292,7 +325,8 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, } } else { consider_local_datapath(ctx, chassis_rec, binding_rec, - local_datapaths, &lport_to_iface); + local_datapaths, &lport_to_iface, + all_lports); } } } diff --git a/ovn/controller/binding.h b/ovn/controller/binding.h index 8753d44..fbd16c8 100644 --- a/ovn/controller/binding.h +++ b/ovn/controller/binding.h @@ -29,7 +29,8 @@ struct sset; void binding_register_ovs_idl(struct ovsdb_idl *); void binding_reset_processing(void); void binding_run(struct controller_ctx *, const struct ovsrec_bridge *br_int, - const char *chassis_id, struct hmap *local_datapaths); + const char *chassis_id, struct hmap *local_datapaths, + struct sset *all_lports); bool binding_cleanup(struct controller_ctx *, const char *chassis_id); #endif /* ovn/binding.h */ diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c index 5c74186..5a2daa8 100644 --- a/ovn/controller/ovn-controller.c +++ b/ovn/controller/ovn-controller.c @@ -314,6 +314,11 @@ static struct hmap patched_datapaths = HMAP_INITIALIZER(&patched_datapaths); static struct lport_index lports; static struct mcgroup_index mcgroups; +/* Contains the names of all logical ports currently bound to the chassis + * managed by this instance of ovn-controller. The contents are managed + * in binding.c, but consumed elsewhere. */ +static struct sset all_lports = SSET_INITIALIZER(&all_lports); + int main(int argc, char *argv[]) { @@ -425,8 +430,6 @@ main(int argc, char *argv[]) update_probe_interval(&ctx); - struct sset all_lports = SSET_INITIALIZER(&all_lports); - const struct ovsrec_bridge *br_int = get_br_int(&ctx); const char *chassis_id = get_chassis_id(ctx.ovs_idl); @@ -434,7 +437,9 @@ main(int argc, char *argv[]) if (chassis_id) { chassis = chassis_run(&ctx, chassis_id); encaps_run(&ctx, br_int, chassis_id); - binding_run(&ctx, br_int, chassis_id, &local_datapaths); + binding_run(&ctx, br_int, chassis_id, &local_datapaths, + &all_lports); + VLOG_INFO("all_lports size: %lu", sset_count(&all_lports)); } if (br_int && chassis_id) { @@ -466,8 +471,6 @@ main(int argc, char *argv[]) } } - sset_destroy(&all_lports); - unixctl_server_run(unixctl); unixctl_server_wait(unixctl);