| Message ID | 1469623351-23732-1-git-send-email-rmoats@us.ibm.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Wed, Jul 27, 2016 at 8:42 AM, Ryan Moats <rmoats@us.ibm.com> wrote: > As [1] indicates, commit 263064a (Convert binding_run > to incremental processing.) incorrectly localized > all_lports to the binding module, leaving an empty > set for update_ct_zone to work with. This patch > restores all_lports processing to what existed prior > to that patch. > > [1] http://openvswitch.org/pipermail/dev/2016-July/076171.html > > Signed-off-by: Ryan Moats <rmoats@us.ibm.com> > --- > v1->v2: Added missing reference to commit message > > ovn/controller/binding.c | 30 +++++++++++++++++++++++++++++- > ovn/controller/binding.h | 3 ++- > ovn/controller/ovn-controller.c | 3 ++- > 3 files changed, 33 insertions(+), 3 deletions(-) > > diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c > index e83c1d5..82ee3ba 100644 > --- a/ovn/controller/binding.c > +++ b/ovn/controller/binding.c > @@ -230,7 +230,8 @@ consider_local_datapath(struct controller_ctx *ctx, > > 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 sset *all_lports, > + struct hmap *local_datapaths) > { > const struct sbrec_chassis *chassis_rec; > const struct sbrec_port_binding *binding_rec; > @@ -251,6 +252,33 @@ binding_run(struct controller_ctx *ctx, const struct > ovsrec_bridge *br_int, > process_full_binding = true; > } > > + struct shash_node *node; > + SHASH_FOR_EACH (node, &lport_to_iface) { > + sset_add(all_lports, node->name); > + } > I think you could just sset_clone() local_ids into all_lports and get the same result. > + > + /* Run through all binding records to collect lists of lports > + for later use in updating ct zones. */ > + SBREC_PORT_BINDING_FOR_EACH(binding_rec, ctx->ovnsb_idl) { > + const struct ovsrec_interface *iface_rec > + = shash_find_data(&lport_to_iface, binding_rec->logical_port); > + if (iface_rec > + || (binding_rec->parent_port && binding_rec->parent_port[0] && > + sset_contains(all_lports, binding_rec->parent_port))) { > + if (binding_rec->parent_port && binding_rec->parent_port[0]) { > + /* Add child logical port to the set of all local ports. > */ > + sset_add(all_lports, binding_rec->logical_port); > + } > + } else if (!binding_rec->chassis > + && !strcmp(binding_rec->type, "localnet")) { > + /* localnet ports will never be bound to a chassis, but we > want > + * to list them in all_lports because we want to allocate > + * a conntrack zone ID for each one, as we'll be creating > + * a patch port for each one. */ > + sset_add(all_lports, binding_rec->logical_port); > + } > + } > + > This seems to undo the benefits of the original change to do "incremental procesing" in binding.c. It seems like we weren't that far from a complete fix in Babu's first patch.
Russell Bryant <russell@ovn.org> wrote on 07/27/2016 04:13:34 PM: > From: Russell Bryant <russell@ovn.org> > To: Ryan Moats/Omaha/IBM@IBMUS > Cc: ovs dev <dev@openvswitch.org> > Date: 07/27/2016 04:14 PM > Subject: Re: [ovs-dev] [PATCH v2] ovn-controller: Restore all_lports > for update_ct_zone > > On Wed, Jul 27, 2016 at 8:42 AM, Ryan Moats <rmoats@us.ibm.com> wrote: > As [1] indicates, commit 263064a (Convert binding_run > to incremental processing.) incorrectly localized > all_lports to the binding module, leaving an empty > set for update_ct_zone to work with. This patch > restores all_lports processing to what existed prior > to that patch. > > [1] http://openvswitch.org/pipermail/dev/2016-July/076171.html > > Signed-off-by: Ryan Moats <rmoats@us.ibm.com> > --- > v1->v2: Added missing reference to commit message > > ovn/controller/binding.c | 30 +++++++++++++++++++++++++++++- > ovn/controller/binding.h | 3 ++- > ovn/controller/ovn-controller.c | 3 ++- > 3 files changed, 33 insertions(+), 3 deletions(-) > > diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c > index e83c1d5..82ee3ba 100644 > --- a/ovn/controller/binding.c > +++ b/ovn/controller/binding.c > @@ -230,7 +230,8 @@ consider_local_datapath(struct controller_ctx *ctx, > > 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 sset *all_lports, > + struct hmap *local_datapaths) > { > const struct sbrec_chassis *chassis_rec; > const struct sbrec_port_binding *binding_rec; > @@ -251,6 +252,33 @@ binding_run(struct controller_ctx *ctx, const > struct ovsrec_bridge *br_int, > process_full_binding = true; > } > > + struct shash_node *node; > + SHASH_FOR_EACH (node, &lport_to_iface) { > + sset_add(all_lports, node->name); > + } > > I think you could just sset_clone() local_ids into all_lports and > get the same result. You are correct > > + > + /* Run through all binding records to collect lists of lports > + for later use in updating ct zones. */ > + SBREC_PORT_BINDING_FOR_EACH(binding_rec, ctx->ovnsb_idl) { > + const struct ovsrec_interface *iface_rec > + = shash_find_data(&lport_to_iface, binding_rec-> logical_port); > + if (iface_rec > + || (binding_rec->parent_port && binding_rec->parent_port[0] && > + sset_contains(all_lports, binding_rec->parent_port))) { > + if (binding_rec->parent_port && binding_rec->parent_port[0]) { > + /* Add child logical port to the set of all local ports. */ > + sset_add(all_lports, binding_rec->logical_port); > + } > + } else if (!binding_rec->chassis > + && !strcmp(binding_rec->type, "localnet")) { > + /* localnet ports will never be bound to a chassis, but we want > + * to list them in all_lports because we want to allocate > + * a conntrack zone ID for each one, as we'll be creating > + * a patch port for each one. */ > + sset_add(all_lports, binding_rec->logical_port); > + } > + } > + > > This seems to undo the benefits of the original change to do > "incremental procesing" in binding.c. > > It seems like we weren't that far from a complete fix in Babu's first patch. We weren't/aren't - the last piece is how to handle persisting the information in the above code snippet, which could then be appended to the cloned local_ids for the final result. What I thought of during the day (and I was still looking for something more efficient) would be to use a hashmap that included the binding_rec uuid and the logical_port string - then I could just check to see if the string already existed on the add side, and remove the value based on the uuid on the delete side. Ryan
diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c index e83c1d5..82ee3ba 100644 --- a/ovn/controller/binding.c +++ b/ovn/controller/binding.c @@ -230,7 +230,8 @@ consider_local_datapath(struct controller_ctx *ctx, 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 sset *all_lports, + struct hmap *local_datapaths) { const struct sbrec_chassis *chassis_rec; const struct sbrec_port_binding *binding_rec; @@ -251,6 +252,33 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, process_full_binding = true; } + struct shash_node *node; + SHASH_FOR_EACH (node, &lport_to_iface) { + sset_add(all_lports, node->name); + } + + /* Run through all binding records to collect lists of lports + for later use in updating ct zones. */ + SBREC_PORT_BINDING_FOR_EACH(binding_rec, ctx->ovnsb_idl) { + const struct ovsrec_interface *iface_rec + = shash_find_data(&lport_to_iface, binding_rec->logical_port); + if (iface_rec + || (binding_rec->parent_port && binding_rec->parent_port[0] && + sset_contains(all_lports, binding_rec->parent_port))) { + if (binding_rec->parent_port && binding_rec->parent_port[0]) { + /* Add child logical port to the set of all local ports. */ + sset_add(all_lports, binding_rec->logical_port); + } + } else if (!binding_rec->chassis + && !strcmp(binding_rec->type, "localnet")) { + /* localnet ports will never be bound to a chassis, but we want + * to list them in all_lports because we want to allocate + * a conntrack zone ID for each one, as we'll be creating + * a patch port for each one. */ + sset_add(all_lports, binding_rec->logical_port); + } + } + /* 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. */ diff --git a/ovn/controller/binding.h b/ovn/controller/binding.h index 8753d44..0cb4a0f 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 sset *all_lports, + struct hmap *local_datapaths); 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 ecf1306..fb5d81b 100644 --- a/ovn/controller/ovn-controller.c +++ b/ovn/controller/ovn-controller.c @@ -434,7 +434,8 @@ 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, &all_lports, + &local_datapaths); } if (br_int && chassis_id) {
As [1] indicates, commit 263064a (Convert binding_run to incremental processing.) incorrectly localized all_lports to the binding module, leaving an empty set for update_ct_zone to work with. This patch restores all_lports processing to what existed prior to that patch. [1] http://openvswitch.org/pipermail/dev/2016-July/076171.html Signed-off-by: Ryan Moats <rmoats@us.ibm.com> --- v1->v2: Added missing reference to commit message ovn/controller/binding.c | 30 +++++++++++++++++++++++++++++- ovn/controller/binding.h | 3 ++- ovn/controller/ovn-controller.c | 3 ++- 3 files changed, 33 insertions(+), 3 deletions(-)