| Message ID | 1457730385-28923-6-git-send-email-rmoats@us.ibm.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Fri, Mar 11, 2016 at 03:06:20PM -0600, Ryan Moats wrote: > From: RYAN D. MOATS <rmoats@us.ibm.com> > > Persist local_datapaths across runs so that a change can be used > as a trigger to reset incremental flow processing. > > Signed-off-by: RYAN D. MOATS <rmoats@us.ibm.com> One thing I'm trying to understand in this series is the reliance on seqnos that come from the IDL. I'm surprised that they're used so much. I would have guessed that the typical use of change tracking would be something like this: For each row that changed, If it's new, create a new object to track it; otherwise, it's modified or deleted, so look up an existing object based on the row's uuid and update or delete it as appropriate But instead logic seems to look at these seqnos a lot. What is the principle that you're following? In this patch, I suspect that 'local_datapaths' should be static.
On Fri, Mar 11, 2016 at 03:06:20PM -0600, Ryan Moats wrote: > From: RYAN D. MOATS <rmoats@us.ibm.com> > > Persist local_datapaths across runs so that a change can be used > as a trigger to reset incremental flow processing. > > Signed-off-by: RYAN D. MOATS <rmoats@us.ibm.com> This patch makes me think I really didn't understand what's going on in the previous patch to lflow_run(). There's more magical arithmetic on seqno values here, too; I guess I'll have to get the explanation from a previous patch on that. I think I'll have to pass on reviewing the remainder of this series until it's rebased, so that I can experiment with applying the patches.
Ben Pfaff <blp@ovn.org> wrote on 03/22/2016 05:05:22 PM: > From: Ben Pfaff <blp@ovn.org> > To: Ryan Moats/Omaha/IBM@IBMUS > Cc: dev@openvswitch.org > Date: 03/22/2016 05:05 PM > Subject: Re: [ovs-dev,v9,05/10] Persist local_datapaths > > On Fri, Mar 11, 2016 at 03:06:20PM -0600, Ryan Moats wrote: > > From: RYAN D. MOATS <rmoats@us.ibm.com> > > > > Persist local_datapaths across runs so that a change can be used > > as a trigger to reset incremental flow processing. > > > > Signed-off-by: RYAN D. MOATS <rmoats@us.ibm.com> > > One thing I'm trying to understand in this series is the reliance on > seqnos that come from the IDL. I'm surprised that they're used so > much. I would have guessed that the typical use of change tracking > would be something like this: > > For each row that changed, > If it's new, create a new object to track it; > otherwise, it's modified or deleted, so look up an existing > object based on the row's uuid and update or delete it as > appropriate > > But instead logic seems to look at these seqnos a lot. What is the > principle that you're following? > > In this patch, I suspect that 'local_datapaths' should be static. > I chose to use IDLs instead of the row's uuid because the insert seqno acts as pretty much as a uuid and I was sure that I could trust them even when a row is deleted. Unfortunately, that led to a bunch of "magic" that I really didn't like. I'll go back and double check to make sure that a row uuid's remain usable after a row is deleted - if they do, then I'm all in favor of using uuids as it should simplify a bunch of things... Ryan (regXboi)
On Wed, Mar 23, 2016 at 07:46:57AM -0600, Ryan Moats wrote: > Ben Pfaff <blp@ovn.org> wrote on 03/22/2016 05:05:22 PM: > > One thing I'm trying to understand in this series is the reliance on > > seqnos that come from the IDL. I'm surprised that they're used so > > much. I would have guessed that the typical use of change tracking > > would be something like this: > > > > For each row that changed, > > If it's new, create a new object to track it; > > otherwise, it's modified or deleted, so look up an existing > > object based on the row's uuid and update or delete it as > > appropriate > > > > But instead logic seems to look at these seqnos a lot. What is the > > principle that you're following? > > I chose to use IDLs instead of the row's uuid because the insert seqno > acts as pretty much as a uuid and I was sure that I could trust them > even when a row is deleted. Unfortunately, that led to a bunch of > "magic" that I really didn't like. I'll go back and double check to > make sure that a row uuid's remain usable after a row is deleted - if > they do, then I'm all in favor of using uuids as it should simplify > a bunch of things... I thought that the change-tracking code retained deleted rows for inspection until the tracking cache was flushed. If it doesn't, then maybe that should be changed. At the very least, I would think that the uuid of the deleted row would be available.
> > I thought that the change-tracking code retained deleted rows for > inspection until the tracking cache was flushed. If it doesn't, then > maybe that should be changed. > > At the very least, I would think that the uuid of the deleted row would > be available. The deleted row's values are unparsed so they are not available for inspection. I did it this so as to ensure the user of the idl does not reference "stale" values. I can rework this to leave the values around (to be cleared when the track cache is flushed) if we think that is a better approach. The uuid of the deleted row is available though.
On Wed, Mar 23, 2016 at 04:40:03PM +0000, Ansari, Shad wrote: > > I thought that the change-tracking code retained deleted rows for > > inspection until the tracking cache was flushed. If it doesn't, then > > maybe that should be changed. > > > > At the very least, I would think that the uuid of the deleted row would > > be available. > > The deleted row's values are unparsed so they are not available for > inspection. I did it this so as to ensure the user of the idl does not > reference "stale" values. I can rework this to leave the values around > (to be cleared when the track cache is flushed) if we think that is a > better approach. It's kind of difficult to come up with a perfect answer here. Are the raw ovsdb_datum values still valid? Looking at those might be a reasonable approach in some cases. > The uuid of the deleted row is available though. Great.
Ben Pfaff <blp@ovn.org> wrote on 03/23/2016 11:23:38 AM: > From: Ben Pfaff <blp@ovn.org> > To: Ryan Moats/Omaha/IBM@IBMUS > Cc: dev@openvswitch.org > Date: 03/23/2016 11:23 AM > Subject: Re: [ovs-dev,v9,05/10] Persist local_datapaths > > On Wed, Mar 23, 2016 at 07:46:57AM -0600, Ryan Moats wrote: > > Ben Pfaff <blp@ovn.org> wrote on 03/22/2016 05:05:22 PM: > > > One thing I'm trying to understand in this series is the reliance on > > > seqnos that come from the IDL. I'm surprised that they're used so > > > much. I would have guessed that the typical use of change tracking > > > would be something like this: > > > > > > For each row that changed, > > > If it's new, create a new object to track it; > > > otherwise, it's modified or deleted, so look up an existing > > > object based on the row's uuid and update or delete it as > > > appropriate > > > > > > But instead logic seems to look at these seqnos a lot. What is the > > > principle that you're following? > > > > I chose to use IDLs instead of the row's uuid because the insert seqno > > acts as pretty much as a uuid and I was sure that I could trust them > > even when a row is deleted. Unfortunately, that led to a bunch of > > "magic" that I really didn't like. I'll go back and double check to > > make sure that a row uuid's remain usable after a row is deleted - if > > they do, then I'm all in favor of using uuids as it should simplify > > a bunch of things... > > I thought that the change-tracking code retained deleted rows for > inspection until the tracking cache was flushed. If it doesn't, then > maybe that should be changed. It doesn't (and I'm not sure it should because of the issues I've seen with trying to flush the tracing cache - I'm still not doing it in any of this code) > At the very least, I would think that the uuid of the deleted row would > be available. Yes, I've verified that is available, and so I've happily dumped all of the sequence number magic for using row uuids - I still have to generate one for the four flows that aren't associated with any database row, but I've already added comments to that effect in the patch sets I'm working on.
On Wed, Mar 23, 2016 at 11:22:00AM -0600, Ryan Moats wrote: > Ben Pfaff <blp@ovn.org> wrote on 03/23/2016 11:23:38 AM: > > At the very least, I would think that the uuid of the deleted row would > > be available. > > Yes, I've verified that is available, and so I've happily dumped all of > the sequence number magic for using row uuids - I still have to generate > one for the four flows that aren't associated with any database row, but > I've already added comments to that effect in the patch sets I'm working > on. Great! Thank you.
diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c index d3ca9c9..602a8fe 100644 --- a/ovn/controller/binding.c +++ b/ovn/controller/binding.c @@ -121,9 +121,31 @@ update_ct_zones(struct sset *lports, struct simap *ct_zones, } } +/* Contains "struct local_datpath" nodes whose hash values are the + * ins_seqno of datapaths with at least one local port binding. */ +struct hmap local_datapaths_by_seqno = + HMAP_INITIALIZER(&local_datapaths_by_seqno); + +static struct local_datapath * +local_datapath_lookup_by_seqno(unsigned int ins_seqno) +{ + return hmap_first_with_hash(&local_datapaths_by_seqno, ins_seqno); +} + +static void +remove_local_datapath(struct hmap *local_datapaths, unsigned int ins_seqno) +{ + struct local_datapath *ld = local_datapath_lookup_by_seqno(ins_seqno); + if (ld) { + hmap_remove(local_datapaths, &ld->hmap_node); + hmap_remove(&local_datapaths_by_seqno, &ld->seqno_hmap_node); + } +} + static void add_local_datapath(struct hmap *local_datapaths, - const struct sbrec_port_binding *binding_rec) + const struct sbrec_port_binding *binding_rec, + unsigned int ins_seqno) { if (hmap_first_with_hash(local_datapaths, binding_rec->datapath->tunnel_key)) { @@ -133,6 +155,7 @@ add_local_datapath(struct hmap *local_datapaths, struct local_datapath *ld = xzalloc(sizeof *ld); hmap_insert(local_datapaths, &ld->hmap_node, binding_rec->datapath->tunnel_key); + hmap_insert(&local_datapaths_by_seqno, &ld->seqno_hmap_node, ins_seqno); } static void @@ -176,7 +199,19 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, /* 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) { + SBREC_PORT_BINDING_FOR_EACH_TRACKED(binding_rec, ctx->ovnsb_idl) { + unsigned int del_seqno = sbrec_port_binding_row_get_seqno(binding_rec, + OVSDB_IDL_CHANGE_DELETE); + unsigned int ins_seqno = sbrec_port_binding_row_get_seqno(binding_rec, + OVSDB_IDL_CHANGE_INSERT); + + /* if the row has a del_seqno > 0, then trying to process the row + * isn't going to work (as it has already been freed) */ + if (del_seqno > 0) { + remove_local_datapath(local_datapaths, ins_seqno); + continue; + } + const struct ovsrec_interface *iface_rec = shash_find_and_delete(&lports, binding_rec->logical_port); if (iface_rec @@ -186,7 +221,7 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, /* Add child logical port to the set of all local ports. */ sset_add(&all_lports, binding_rec->logical_port); } - add_local_datapath(local_datapaths, binding_rec); + add_local_datapath(local_datapaths, binding_rec, ins_seqno); if (iface_rec && ctx->ovs_idl_txn) { update_qos(iface_rec, binding_rec); } diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c index 8f3873d..cb8536b 100644 --- a/ovn/controller/ovn-controller.c +++ b/ovn/controller/ovn-controller.c @@ -198,6 +198,10 @@ get_ovnsb_remote(struct ovsdb_idl *ovs_idl) } } +/* Contains "struct local_datpath" nodes whose hash values are the + * tunnel_key of datapaths with at least one local port binding. */ +struct hmap local_datapaths = HMAP_INITIALIZER(&local_datapaths); + int main(int argc, char *argv[]) { @@ -282,10 +286,6 @@ main(int argc, char *argv[]) .ovnsb_idl_txn = ovsdb_idl_loop_run(&ovnsb_idl_loop), }; - /* Contains "struct local_datpath" nodes whose hash values are the - * tunnel_key of datapaths with at least one local port binding. */ - struct hmap local_datapaths = HMAP_INITIALIZER(&local_datapaths); - const struct ovsrec_bridge *br_int = get_br_int(&ctx); const char *chassis_id = get_chassis_id(ctx.ovs_idl); @@ -312,13 +312,6 @@ main(int argc, char *argv[]) ofctrl_put(); } - struct local_datapath *cur_node, *next_node; - HMAP_FOR_EACH_SAFE (cur_node, next_node, hmap_node, &local_datapaths) { - hmap_remove(&local_datapaths, &cur_node->hmap_node); - free(cur_node); - } - hmap_destroy(&local_datapaths); - unixctl_server_run(unixctl); unixctl_server_wait(unixctl); diff --git a/ovn/controller/ovn-controller.h b/ovn/controller/ovn-controller.h index a3465eb..04faf93 100644 --- a/ovn/controller/ovn-controller.h +++ b/ovn/controller/ovn-controller.h @@ -38,6 +38,7 @@ struct controller_ctx { * the localnet port */ struct local_datapath { struct hmap_node hmap_node; + struct hmap_node seqno_hmap_node; const struct sbrec_port_binding *localnet_port; }; diff --git a/ovn/controller/patch.c b/ovn/controller/patch.c index 753ce3e..9c519b0 100644 --- a/ovn/controller/patch.c +++ b/ovn/controller/patch.c @@ -190,7 +190,8 @@ add_bridge_mappings(struct controller_ctx *ctx, * to create patch ports for it. */ continue; } - if (ld->localnet_port) { + if (ld->localnet_port && strcmp(ld->localnet_port->logical_port, + binding->logical_port)) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); VLOG_WARN_RL(&rl, "localnet port '%s' already set for datapath " "'%"PRId64"', skipping the new port '%s'.",