| Message ID | 1472424698-487-1-git-send-email-rmoats@us.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
Ryan Moats/Omaha/IBM@IBMUS wrote on 08/28/2016 05:51:38 PM: > From: Ryan Moats/Omaha/IBM@IBMUS > To: dev@openvswitch.org > Cc: blp@ovn.org, Ryan Moats/Omaha/IBM@IBMUS > Date: 08/28/2016 05:51 PM > Subject: [PATCH] ovn-controller: Convert encaps module back to full processing > > Commit f5d916cb ("ovn-controller: Back out incremental processing") > left the conversion of the encaps module to a future patch set. > This patch converts this module back to full processing, but > does not remove all persistence of associated data strcutures. > > This commit depends on f5d916cb ("ovn-controller: > Back out incremental processing"). > > Signed-off-by: Ryan Moats <rmoats@us.ibm.com> > --- This patch set does not remove persistence because I'm not sure how to do it and still maintain correct behavior for the "change Encap properties" unit test case. In fact, my original attempt was to just go back to the original code base, but that failed on that particular test case...
On Sun, Aug 28, 2016 at 3:51 PM, Ryan Moats <rmoats@us.ibm.com> wrote: > diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c > index d99ba05..844447f 100644 > --- a/ovn/controller/encaps.c > +++ b/ovn/controller/encaps.c > + SBREC_CHASSIS_FOR_EACH (chassis_rec, ctx->ovnsb_idl) { > + for (int i = 0; i < chassis_rec->n_encaps; i++) { > + encap_rec = chassis_rec->encaps[i]; > + > + struct encap_hash_node *encap_hash_node; > + encap_hash_node = lookup_encap_uuid(&encap_rec->header_.uuid); > + if (encap_hash_node) { > + /* A change might have invalidated our mapping. Process the > + * new version and then iterate over everything to see if it > + * is OK. */ > + delete_encap_uuid(encap_hash_node); > + poll_immediate_wake(); > } Doesn't this result in essentially infinite wakeups? It used to be that this loop would run only when a chassis was add/removed/changed and so the if (encap_hash_node) condition would only trigger when an existing chassis is modified. However, after this change every wakeup will loop through all chassises and any existing ones will trigger another wakeup and loop, etc. In general, I don't think it makes sense to remove incremental processing without removing persistent state. The result ends up being not very logical and actually more complicated.
On Tue, Aug 30, 2016 at 09:04:10AM -0700, Jesse Gross wrote: > On Sun, Aug 28, 2016 at 3:51 PM, Ryan Moats <rmoats@us.ibm.com> wrote: > > diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c > > index d99ba05..844447f 100644 > > --- a/ovn/controller/encaps.c > > +++ b/ovn/controller/encaps.c > > + SBREC_CHASSIS_FOR_EACH (chassis_rec, ctx->ovnsb_idl) { > > + for (int i = 0; i < chassis_rec->n_encaps; i++) { > > + encap_rec = chassis_rec->encaps[i]; > > + > > + struct encap_hash_node *encap_hash_node; > > + encap_hash_node = lookup_encap_uuid(&encap_rec->header_.uuid); > > + if (encap_hash_node) { > > + /* A change might have invalidated our mapping. Process the > > + * new version and then iterate over everything to see if it > > + * is OK. */ > > + delete_encap_uuid(encap_hash_node); > > + poll_immediate_wake(); > > } > > Doesn't this result in essentially infinite wakeups? It used to be > that this loop would run only when a chassis was add/removed/changed > and so the if (encap_hash_node) condition would only trigger when an > existing chassis is modified. However, after this change every wakeup > will loop through all chassises and any existing ones will trigger > another wakeup and loop, etc. > > In general, I don't think it makes sense to remove incremental > processing without removing persistent state. The result ends up being > not very logical and actually more complicated. I want to second Jesse's feedback here. I think that this should really be stateless, not half-incremental. Thanks, Ben.
Ben Pfaff <blp@ovn.org> wrote on 08/31/2016 12:46:17 PM: > From: Ben Pfaff <blp@ovn.org> > To: Jesse Gross <jesse@kernel.org> > Cc: Ryan Moats/Omaha/IBM@IBMUS, ovs dev <dev@openvswitch.org> > Date: 08/31/2016 12:46 PM > Subject: Re: [ovs-dev] [PATCH] ovn-controller: Convert encaps module > back to full processing > > On Tue, Aug 30, 2016 at 09:04:10AM -0700, Jesse Gross wrote: > > On Sun, Aug 28, 2016 at 3:51 PM, Ryan Moats <rmoats@us.ibm.com> wrote: > > > diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c > > > index d99ba05..844447f 100644 > > > --- a/ovn/controller/encaps.c > > > +++ b/ovn/controller/encaps.c > > > + SBREC_CHASSIS_FOR_EACH (chassis_rec, ctx->ovnsb_idl) { > > > + for (int i = 0; i < chassis_rec->n_encaps; i++) { > > > + encap_rec = chassis_rec->encaps[i]; > > > + > > > + struct encap_hash_node *encap_hash_node; > > > + encap_hash_node = > lookup_encap_uuid(&encap_rec->header_.uuid); > > > + if (encap_hash_node) { > > > + /* A change might have invalidated our mapping. > Process the > > > + * new version and then iterate over everything > to see if it > > > + * is OK. */ > > > + delete_encap_uuid(encap_hash_node); > > > + poll_immediate_wake(); > > > } > > > > Doesn't this result in essentially infinite wakeups? It used to be > > that this loop would run only when a chassis was add/removed/changed > > and so the if (encap_hash_node) condition would only trigger when an > > existing chassis is modified. However, after this change every wakeup > > will loop through all chassises and any existing ones will trigger > > another wakeup and loop, etc. > > > > In general, I don't think it makes sense to remove incremental > > processing without removing persistent state. The result ends up being > > not very logical and actually more complicated. > > I want to second Jesse's feedback here. I think that this should really > be stateless, not half-incremental. Yes that poll_immediate_wake does result in infinite wakeups and yes it has been removed from v2 of the patch set. I explained the situation in [1] and that "cry for help" still holds... [1] http://openvswitch.org/pipermail/dev/2016-August/078692.html
On Wed, Aug 31, 2016 at 12:59:05PM -0500, Ryan Moats wrote: > Ben Pfaff <blp@ovn.org> wrote on 08/31/2016 12:46:17 PM: > > > From: Ben Pfaff <blp@ovn.org> > > To: Jesse Gross <jesse@kernel.org> > > Cc: Ryan Moats/Omaha/IBM@IBMUS, ovs dev <dev@openvswitch.org> > > Date: 08/31/2016 12:46 PM > > Subject: Re: [ovs-dev] [PATCH] ovn-controller: Convert encaps module > > back to full processing > > > > On Tue, Aug 30, 2016 at 09:04:10AM -0700, Jesse Gross wrote: > > > On Sun, Aug 28, 2016 at 3:51 PM, Ryan Moats <rmoats@us.ibm.com> wrote: > > > > diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c > > > > index d99ba05..844447f 100644 > > > > --- a/ovn/controller/encaps.c > > > > +++ b/ovn/controller/encaps.c > > > > + SBREC_CHASSIS_FOR_EACH (chassis_rec, ctx->ovnsb_idl) { > > > > + for (int i = 0; i < chassis_rec->n_encaps; i++) { > > > > + encap_rec = chassis_rec->encaps[i]; > > > > + > > > > + struct encap_hash_node *encap_hash_node; > > > > + encap_hash_node = > > lookup_encap_uuid(&encap_rec->header_.uuid); > > > > + if (encap_hash_node) { > > > > + /* A change might have invalidated our mapping. > > Process the > > > > + * new version and then iterate over everything > > to see if it > > > > + * is OK. */ > > > > + delete_encap_uuid(encap_hash_node); > > > > + poll_immediate_wake(); > > > > } > > > > > > Doesn't this result in essentially infinite wakeups? It used to be > > > that this loop would run only when a chassis was add/removed/changed > > > and so the if (encap_hash_node) condition would only trigger when an > > > existing chassis is modified. However, after this change every wakeup > > > will loop through all chassises and any existing ones will trigger > > > another wakeup and loop, etc. > > > > > > In general, I don't think it makes sense to remove incremental > > > processing without removing persistent state. The result ends up being > > > not very logical and actually more complicated. > > > > I want to second Jesse's feedback here. I think that this should really > > be stateless, not half-incremental. > > Yes that poll_immediate_wake does result in infinite wakeups and yes > it has been removed from v2 of the patch set. > > I explained the situation in [1] and that "cry for help" still holds... > > [1] http://openvswitch.org/pipermail/dev/2016-August/078692.html Ah, OK, didn't understand that, I'll try to help.
On Wed, Aug 31, 2016 at 12:59:05PM -0500, Ryan Moats wrote: > Ben Pfaff <blp@ovn.org> wrote on 08/31/2016 12:46:17 PM: > > > From: Ben Pfaff <blp@ovn.org> > > To: Jesse Gross <jesse@kernel.org> > > Cc: Ryan Moats/Omaha/IBM@IBMUS, ovs dev <dev@openvswitch.org> > > Date: 08/31/2016 12:46 PM > > Subject: Re: [ovs-dev] [PATCH] ovn-controller: Convert encaps module > > back to full processing > > > > On Tue, Aug 30, 2016 at 09:04:10AM -0700, Jesse Gross wrote: > > > On Sun, Aug 28, 2016 at 3:51 PM, Ryan Moats <rmoats@us.ibm.com> wrote: > > > > diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c > > > > index d99ba05..844447f 100644 > > > > --- a/ovn/controller/encaps.c > > > > +++ b/ovn/controller/encaps.c > > > > + SBREC_CHASSIS_FOR_EACH (chassis_rec, ctx->ovnsb_idl) { > > > > + for (int i = 0; i < chassis_rec->n_encaps; i++) { > > > > + encap_rec = chassis_rec->encaps[i]; > > > > + > > > > + struct encap_hash_node *encap_hash_node; > > > > + encap_hash_node = > > lookup_encap_uuid(&encap_rec->header_.uuid); > > > > + if (encap_hash_node) { > > > > + /* A change might have invalidated our mapping. > > Process the > > > > + * new version and then iterate over everything > > to see if it > > > > + * is OK. */ > > > > + delete_encap_uuid(encap_hash_node); > > > > + poll_immediate_wake(); > > > > } > > > > > > Doesn't this result in essentially infinite wakeups? It used to be > > > that this loop would run only when a chassis was add/removed/changed > > > and so the if (encap_hash_node) condition would only trigger when an > > > existing chassis is modified. However, after this change every wakeup > > > will loop through all chassises and any existing ones will trigger > > > another wakeup and loop, etc. > > > > > > In general, I don't think it makes sense to remove incremental > > > processing without removing persistent state. The result ends up being > > > not very logical and actually more complicated. > > > > I want to second Jesse's feedback here. I think that this should really > > be stateless, not half-incremental. > > Yes that poll_immediate_wake does result in infinite wakeups and yes > it has been removed from v2 of the patch set. > > I explained the situation in [1] and that "cry for help" still holds... > > [1] http://openvswitch.org/pipermail/dev/2016-August/078692.html I sent my suggestion as a 2-patch series: https://patchwork.ozlabs.org/patch/664685/ https://patchwork.ozlabs.org/patch/664686/
Ben Pfaff <blp@ovn.org> wrote on 08/31/2016 04:26:37 PM: > From: Ben Pfaff <blp@ovn.org> > To: Ryan Moats/Omaha/IBM@IBMUS > Cc: ovs dev <dev@openvswitch.org>, Jesse Gross <jesse@kernel.org> > Date: 08/31/2016 04:26 PM > Subject: Re: [ovs-dev] [PATCH] ovn-controller: Convert encaps module > back to full processing > > On Wed, Aug 31, 2016 at 12:59:05PM -0500, Ryan Moats wrote: > > Ben Pfaff <blp@ovn.org> wrote on 08/31/2016 12:46:17 PM: > > > > > From: Ben Pfaff <blp@ovn.org> > > > To: Jesse Gross <jesse@kernel.org> > > > Cc: Ryan Moats/Omaha/IBM@IBMUS, ovs dev <dev@openvswitch.org> > > > Date: 08/31/2016 12:46 PM > > > Subject: Re: [ovs-dev] [PATCH] ovn-controller: Convert encaps module > > > back to full processing > > > > > > On Tue, Aug 30, 2016 at 09:04:10AM -0700, Jesse Gross wrote: > > > > On Sun, Aug 28, 2016 at 3:51 PM, Ryan Moats <rmoats@us.ibm.com> wrote: > > > > > diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c > > > > > index d99ba05..844447f 100644 > > > > > --- a/ovn/controller/encaps.c > > > > > +++ b/ovn/controller/encaps.c > > > > > + SBREC_CHASSIS_FOR_EACH (chassis_rec, ctx->ovnsb_idl) { > > > > > + for (int i = 0; i < chassis_rec->n_encaps; i++) { > > > > > + encap_rec = chassis_rec->encaps[i]; > > > > > + > > > > > + struct encap_hash_node *encap_hash_node; > > > > > + encap_hash_node = > > > lookup_encap_uuid(&encap_rec->header_.uuid); > > > > > + if (encap_hash_node) { > > > > > + /* A change might have invalidated our mapping. > > > Process the > > > > > + * new version and then iterate over everything > > > to see if it > > > > > + * is OK. */ > > > > > + delete_encap_uuid(encap_hash_node); > > > > > + poll_immediate_wake(); > > > > > } > > > > > > > > Doesn't this result in essentially infinite wakeups? It used to be > > > > that this loop would run only when a chassis was add/removed/changed > > > > and so the if (encap_hash_node) condition would only trigger when an > > > > existing chassis is modified. However, after this change every wakeup > > > > will loop through all chassises and any existing ones will trigger > > > > another wakeup and loop, etc. > > > > > > > > In general, I don't think it makes sense to remove incremental > > > > processing without removing persistent state. The result ends up being > > > > not very logical and actually more complicated. > > > > > > I want to second Jesse's feedback here. I think that this should really > > > be stateless, not half-incremental. > > > > Yes that poll_immediate_wake does result in infinite wakeups and yes > > it has been removed from v2 of the patch set. > > > > I explained the situation in [1] and that "cry for help" still holds... > > > > [1] http://openvswitch.org/pipermail/dev/2016-August/078692.html > > I sent my suggestion as a 2-patch series: > https://patchwork.ozlabs.org/patch/664685/ > https://patchwork.ozlabs.org/patch/664686/ > I will give them a review first chance I get...
diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c index d99ba05..844447f 100644 --- a/ovn/controller/encaps.c +++ b/ovn/controller/encaps.c @@ -412,25 +412,21 @@ encaps_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, /* Maintain a mapping backwards from encap entries to their parent * chassis. Most changes happen at the encap row entry but tunnels need * to be established on the basis of the overall chassis. */ - SBREC_CHASSIS_FOR_EACH_TRACKED (chassis_rec, ctx->ovnsb_idl) { - /* Defer deletion of mapping until we have cleaned up associated - * ports. */ - if (!sbrec_chassis_is_deleted(chassis_rec)) { - for (int i = 0; i < chassis_rec->n_encaps; i++) { - encap_rec = chassis_rec->encaps[i]; - - struct encap_hash_node *encap_hash_node; - encap_hash_node = lookup_encap_uuid(&encap_rec->header_.uuid); - if (encap_hash_node) { - /* A change might have invalidated our mapping. Process the - * new version and then iterate over everything to see if it - * is OK. */ - delete_encap_uuid(encap_hash_node); - poll_immediate_wake(); - } - - insert_encap_uuid(&encap_rec->header_.uuid, chassis_rec); + SBREC_CHASSIS_FOR_EACH (chassis_rec, ctx->ovnsb_idl) { + for (int i = 0; i < chassis_rec->n_encaps; i++) { + encap_rec = chassis_rec->encaps[i]; + + struct encap_hash_node *encap_hash_node; + encap_hash_node = lookup_encap_uuid(&encap_rec->header_.uuid); + if (encap_hash_node) { + /* A change might have invalidated our mapping. Process the + * new version and then iterate over everything to see if it + * is OK. */ + delete_encap_uuid(encap_hash_node); + poll_immediate_wake(); } + + insert_encap_uuid(&encap_rec->header_.uuid, chassis_rec); } } @@ -440,7 +436,7 @@ encaps_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, * might actually result in the creation of a different type tunnel if * that type is preferred. That's OK - when we process the other encap * rows, we'll just skip over the new tunnels. */ - SBREC_ENCAP_FOR_EACH_TRACKED (encap_rec, ctx->ovnsb_idl) { + SBREC_ENCAP_FOR_EACH (encap_rec, ctx->ovnsb_idl) { struct encap_hash_node *encap_hash_node; struct chassis_hash_node *chassis_hash_node; const struct ovsrec_port *port_rec = NULL;
Commit f5d916cb ("ovn-controller: Back out incremental processing") left the conversion of the encaps module to a future patch set. This patch converts this module back to full processing, but does not remove all persistence of associated data strcutures. This commit depends on f5d916cb ("ovn-controller: Back out incremental processing"). Signed-off-by: Ryan Moats <rmoats@us.ibm.com> --- ovn/controller/encaps.c | 34 +++++++++++++++------------------- 1 file changed, 15 insertions(+), 19 deletions(-)