| Message ID | 1462202818-13777-2-git-send-email-rmoats@us.ibm.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Mon, May 02, 2016 at 10:26:54AM -0500, Ryan Moats wrote: > As a side effect, tunnel context is persisted. > > Signed-off-by: Ryan Moats <rmoats@us.ibm.com> Thanks for updating this. In a couple of places, this uses hmap_first_with_hash() to find an element in a hash table. ovn-controller uses this method in some special cases where the hash value is known to be unique; for example, I think that it's used for a hash table where the "hash" is the assigned logical datapath ID, which is a unique 32-bit (maybe shorter? I don't recall at the moment) number. But that trick doesn't work when the hash value is really a hash. For example, it can't be used in this code where the hash is taken from a UUID, because there might be multiple UUIDs with the same hash value. It's necessary, instead, to iterate through the items that have the desired hash value, with HMAP_FOR_EACH_WITH_HASH, and then compare the item's full key instead of just the hash. In the process_full_encaps case, I don't see what removes tunnels that are no longer needed. This has some TODOs and commented-out code in it, so I suspect that it's not really ready for full review? Thanks, Ben.
Ben Pfaff <blp@ovn.org> wrote on 05/17/2016 10:13:19 PM: > From: Ben Pfaff <blp@ovn.org> > To: Ryan Moats/Omaha/IBM@IBMUS > Cc: dev@openvswitch.org > Date: 05/17/2016 10:14 PM > Subject: Re: [ovs-dev,v16,1/5] Change encaps_run to work incrementally > > On Mon, May 02, 2016 at 10:26:54AM -0500, Ryan Moats wrote: > > As a side effect, tunnel context is persisted. > > > > Signed-off-by: Ryan Moats <rmoats@us.ibm.com> > > Thanks for updating this. and thanks for looking - sorry for the delayed reply (I've been doing OVN training the past couple of days) > In a couple of places, this uses hmap_first_with_hash() to find an > element in a hash table. ovn-controller uses this method in some > special cases where the hash value is known to be unique; for example, I > think that it's used for a hash table where the "hash" is the assigned > logical datapath ID, which is a unique 32-bit (maybe shorter? I don't > recall at the moment) number. But that trick doesn't work when the hash > value is really a hash. For example, it can't be used in this code > where the hash is taken from a UUID, because there might be multiple > UUIDs with the same hash value. It's necessary, instead, to iterate > through the items that have the desired hash value, with > HMAP_FOR_EACH_WITH_HASH, and then compare the item's full key instead of > just the hash. Ugh - I thought I had changed that, but when I couple this with your comments below, I'm thinking I've confused myself as to what patches I have and haven't pushed > In the process_full_encaps case, I don't see what removes tunnels that > are no longer needed. > > This has some TODOs and commented-out code in it, so I suspect that it's > not really ready for full review? As I implied above, those shouldn't be there, so now I'm suspicious if I've lost track of a ball that I've been juggling... Since I've got to rebase the rest of the patches anyway, adding this one to the list won't add that much additional effort... Ryan
On Thu, May 19, 2016 at 10:02:08AM -0500, Ryan Moats wrote: > Ben Pfaff <blp@ovn.org> wrote on 05/17/2016 10:13:19 PM: > > > From: Ben Pfaff <blp@ovn.org> > > To: Ryan Moats/Omaha/IBM@IBMUS > > Cc: dev@openvswitch.org > > Date: 05/17/2016 10:14 PM > > Subject: Re: [ovs-dev,v16,1/5] Change encaps_run to work incrementally > > > > On Mon, May 02, 2016 at 10:26:54AM -0500, Ryan Moats wrote: > > > As a side effect, tunnel context is persisted. > > > > > > Signed-off-by: Ryan Moats <rmoats@us.ibm.com> > > > > Thanks for updating this. > > and thanks for looking - sorry for the delayed reply (I've been > doing OVN training the past couple of days) > > > In a couple of places, this uses hmap_first_with_hash() to find an > > element in a hash table. ovn-controller uses this method in some > > special cases where the hash value is known to be unique; for example, I > > think that it's used for a hash table where the "hash" is the assigned > > logical datapath ID, which is a unique 32-bit (maybe shorter? I don't > > recall at the moment) number. But that trick doesn't work when the hash > > value is really a hash. For example, it can't be used in this code > > where the hash is taken from a UUID, because there might be multiple > > UUIDs with the same hash value. It's necessary, instead, to iterate > > through the items that have the desired hash value, with > > HMAP_FOR_EACH_WITH_HASH, and then compare the item's full key instead of > > just the hash. > > Ugh - I thought I had changed that, but when I couple this with your > comments below, I'm thinking I've confused myself as to what patches > I have and haven't pushed > > > In the process_full_encaps case, I don't see what removes tunnels that > > are no longer needed. > > > > This has some TODOs and commented-out code in it, so I suspect that it's > > not really ready for full review? > > As I implied above, those shouldn't be there, so now I'm suspicious if I've > lost track of a ball that I've been juggling... Since I've got to rebase > the rest of the patches anyway, adding this one to the list won't add > that much additional effort... Yeah, it did seem weird, since going into it I expected that this patch was ready. I'll look forward to the next version.
Ben Pfaff <blp@ovn.org> wrote on 05/19/2016 10:32:53 AM: > From: Ben Pfaff <blp@ovn.org> > To: Ryan Moats/Omaha/IBM@IBMUS > Cc: dev@openvswitch.org > Date: 05/19/2016 10:33 AM > Subject: Re: [ovs-dev,v16,1/5] Change encaps_run to work incrementally > > On Thu, May 19, 2016 at 10:02:08AM -0500, Ryan Moats wrote: > > Ben Pfaff <blp@ovn.org> wrote on 05/17/2016 10:13:19 PM: > > > > > From: Ben Pfaff <blp@ovn.org> > > > To: Ryan Moats/Omaha/IBM@IBMUS > > > Cc: dev@openvswitch.org > > > Date: 05/17/2016 10:14 PM > > > Subject: Re: [ovs-dev,v16,1/5] Change encaps_run to work incrementally > > > > > > On Mon, May 02, 2016 at 10:26:54AM -0500, Ryan Moats wrote: > > > > As a side effect, tunnel context is persisted. > > > > > > > > Signed-off-by: Ryan Moats <rmoats@us.ibm.com> > > > > > > Thanks for updating this. > > > > and thanks for looking - sorry for the delayed reply (I've been > > doing OVN training the past couple of days) > > > > > In a couple of places, this uses hmap_first_with_hash() to find an > > > element in a hash table. ovn-controller uses this method in some > > > special cases where the hash value is known to be unique; for example, I > > > think that it's used for a hash table where the "hash" is the assigned > > > logical datapath ID, which is a unique 32-bit (maybe shorter? I don't > > > recall at the moment) number. But that trick doesn't work when the hash > > > value is really a hash. For example, it can't be used in this code > > > where the hash is taken from a UUID, because there might be multiple > > > UUIDs with the same hash value. It's necessary, instead, to iterate > > > through the items that have the desired hash value, with > > > HMAP_FOR_EACH_WITH_HASH, and then compare the item's full key instead of > > > just the hash. > > > > Ugh - I thought I had changed that, but when I couple this with your > > comments below, I'm thinking I've confused myself as to what patches > > I have and haven't pushed > > > > > In the process_full_encaps case, I don't see what removes tunnels that > > > are no longer needed. > > > > > > This has some TODOs and commented-out code in it, so I suspect that it's > > > not really ready for full review? > > > > As I implied above, those shouldn't be there, so now I'm suspicious if I've > > lost track of a ball that I've been juggling... Since I've got to rebase > > the rest of the patches anyway, adding this one to the list won't add > > that much additional effort... > > Yeah, it did seem weird, since going into it I expected that this patch > was ready. I'll look forward to the next version. > I've found the right code for encaps.c and re-looking at the tunnel update path, I'm really dubious about it being correct - I'm out this afternoon (CDT) for a HS graduation, but if I post just the first patch as an RFC how soon would it capture eyeballs? I ask because if it will be a while, then I'll just work through the rest of the patches first. Ryan
On Thu, May 19, 2016 at 11:52:05AM -0500, Ryan Moats wrote: > Ben Pfaff <blp@ovn.org> wrote on 05/19/2016 10:32:53 AM: > > > From: Ben Pfaff <blp@ovn.org> > > To: Ryan Moats/Omaha/IBM@IBMUS > > Cc: dev@openvswitch.org > > Date: 05/19/2016 10:33 AM > > Subject: Re: [ovs-dev,v16,1/5] Change encaps_run to work incrementally > > > > On Thu, May 19, 2016 at 10:02:08AM -0500, Ryan Moats wrote: > > > Ben Pfaff <blp@ovn.org> wrote on 05/17/2016 10:13:19 PM: > > > > > > > From: Ben Pfaff <blp@ovn.org> > > > > To: Ryan Moats/Omaha/IBM@IBMUS > > > > Cc: dev@openvswitch.org > > > > Date: 05/17/2016 10:14 PM > > > > Subject: Re: [ovs-dev,v16,1/5] Change encaps_run to work > incrementally > > > > > > > > On Mon, May 02, 2016 at 10:26:54AM -0500, Ryan Moats wrote: > > > > > As a side effect, tunnel context is persisted. > > > > > > > > > > Signed-off-by: Ryan Moats <rmoats@us.ibm.com> > > > > > > > > Thanks for updating this. > > > > > > and thanks for looking - sorry for the delayed reply (I've been > > > doing OVN training the past couple of days) > > > > > > > In a couple of places, this uses hmap_first_with_hash() to find an > > > > element in a hash table. ovn-controller uses this method in some > > > > special cases where the hash value is known to be unique; for > example, I > > > > think that it's used for a hash table where the "hash" is the > assigned > > > > logical datapath ID, which is a unique 32-bit (maybe shorter? I don't > > > > recall at the moment) number. But that trick doesn't work when the > hash > > > > value is really a hash. For example, it can't be used in this code > > > > where the hash is taken from a UUID, because there might be multiple > > > > UUIDs with the same hash value. It's necessary, instead, to iterate > > > > through the items that have the desired hash value, with > > > > HMAP_FOR_EACH_WITH_HASH, and then compare the item's full key instead > of > > > > just the hash. > > > > > > Ugh - I thought I had changed that, but when I couple this with your > > > comments below, I'm thinking I've confused myself as to what patches > > > I have and haven't pushed > > > > > > > In the process_full_encaps case, I don't see what removes tunnels > that > > > > are no longer needed. > > > > > > > > This has some TODOs and commented-out code in it, so I suspect that > it's > > > > not really ready for full review? > > > > > > As I implied above, those shouldn't be there, so now I'm suspicious if > I've > > > lost track of a ball that I've been juggling... Since I've got to > rebase > > > the rest of the patches anyway, adding this one to the list won't add > > > that much additional effort... > > > > Yeah, it did seem weird, since going into it I expected that this patch > > was ready. I'll look forward to the next version. > > > > I've found the right code for encaps.c and re-looking at the tunnel > update path, I'm really dubious about it being correct - I'm out this > afternoon (CDT) for a HS graduation, but if I post just the first patch > as an RFC how soon would it capture eyeballs? I ask because if it will > be a while, then I'll just work through the rest of the patches first. I think it'll be a while, because I'm trying to get from code review back to coding.
Ben Pfaff <blp@ovn.org> wrote on 05/19/2016 12:02:50 PM: > From: Ben Pfaff <blp@ovn.org> > To: Ryan Moats/Omaha/IBM@IBMUS > Cc: dev@openvswitch.org > Date: 05/19/2016 12:03 PM > Subject: Re: [ovs-dev,v16,1/5] Change encaps_run to work incrementally > > On Thu, May 19, 2016 at 11:52:05AM -0500, Ryan Moats wrote: > > Ben Pfaff <blp@ovn.org> wrote on 05/19/2016 10:32:53 AM: > > > > > From: Ben Pfaff <blp@ovn.org> > > > To: Ryan Moats/Omaha/IBM@IBMUS > > > Cc: dev@openvswitch.org > > > Date: 05/19/2016 10:33 AM > > > Subject: Re: [ovs-dev,v16,1/5] Change encaps_run to work incrementally > > > > > > On Thu, May 19, 2016 at 10:02:08AM -0500, Ryan Moats wrote: > > > > Ben Pfaff <blp@ovn.org> wrote on 05/17/2016 10:13:19 PM: > > > > > > > > > From: Ben Pfaff <blp@ovn.org> > > > > > To: Ryan Moats/Omaha/IBM@IBMUS > > > > > Cc: dev@openvswitch.org > > > > > Date: 05/17/2016 10:14 PM > > > > > Subject: Re: [ovs-dev,v16,1/5] Change encaps_run to work > > incrementally > > > > > > > > > > On Mon, May 02, 2016 at 10:26:54AM -0500, Ryan Moats wrote: > > > > > > As a side effect, tunnel context is persisted. > > > > > > > > > > > > Signed-off-by: Ryan Moats <rmoats@us.ibm.com> > > > > > > > > > > Thanks for updating this. > > > > > > > > and thanks for looking - sorry for the delayed reply (I've been > > > > doing OVN training the past couple of days) > > > > > > > > > In a couple of places, this uses hmap_first_with_hash() to find an > > > > > element in a hash table. ovn-controller uses this method in some > > > > > special cases where the hash value is known to be unique; for > > example, I > > > > > think that it's used for a hash table where the "hash" is the > > assigned > > > > > logical datapath ID, which is a unique 32-bit (maybe shorter? I don't > > > > > recall at the moment) number. But that trick doesn't work when the > > hash > > > > > value is really a hash. For example, it can't be used in this code > > > > > where the hash is taken from a UUID, because there might be multiple > > > > > UUIDs with the same hash value. It's necessary, instead, to iterate > > > > > through the items that have the desired hash value, with > > > > > HMAP_FOR_EACH_WITH_HASH, and then compare the item's full key instead > > of > > > > > just the hash. > > > > > > > > Ugh - I thought I had changed that, but when I couple this with your > > > > comments below, I'm thinking I've confused myself as to what patches > > > > I have and haven't pushed > > > > > > > > > In the process_full_encaps case, I don't see what removes tunnels > > that > > > > > are no longer needed. > > > > > > > > > > This has some TODOs and commented-out code in it, so I suspect that > > it's > > > > > not really ready for full review? > > > > > > > > As I implied above, those shouldn't be there, so now I'm suspicious if > > I've > > > > lost track of a ball that I've been juggling... Since I've got to > > rebase > > > > the rest of the patches anyway, adding this one to the list won't add > > > > that much additional effort... > > > > > > Yeah, it did seem weird, since going into it I expected that this patch > > > was ready. I'll look forward to the next version. > > > > > > > I've found the right code for encaps.c and re-looking at the tunnel > > update path, I'm really dubious about it being correct - I'm out this > > afternoon (CDT) for a HS graduation, but if I post just the first patch > > as an RFC how soon would it capture eyeballs? I ask because if it will > > be a while, then I'll just work through the rest of the patches first. > > I think it'll be a while, because I'm trying to get from code review > back to coding. Ok, then I'll just go ahead and work through the whole set then...
diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c index 149698e..5adba12 100644 --- a/ovn/controller/encaps.c +++ b/ovn/controller/encaps.c @@ -15,6 +15,7 @@ #include <config.h> #include "encaps.h" +#include "lflow.h" #include "lib/hash.h" #include "lib/sset.h" @@ -49,6 +50,7 @@ struct tunnel_ctx { * generated we remove them. After generating all the rows, any * remaining in 'tunnel_hmap' must be deleted from the database. */ struct hmap tunnel_hmap; + struct hmap tunnel_hmap_by_uuid; /* Names of all ports in the bridge, to allow checking uniqueness when * adding a new tunnel. */ @@ -58,8 +60,18 @@ struct tunnel_ctx { const struct ovsrec_bridge *br_int; }; +static struct tunnel_ctx tc = { + .tunnel_hmap = HMAP_INITIALIZER(&tc.tunnel_hmap), + .tunnel_hmap_by_uuid = HMAP_INITIALIZER(&tc.tunnel_hmap_by_uuid), + .port_names = SSET_INITIALIZER(&tc.port_names), +}; + +static bool process_full_encaps = false; + struct port_hash_node { struct hmap_node node; + struct hmap_node uuid_node; + const struct uuid *uuid; const struct ovsrec_port *port; const struct ovsrec_bridge *bridge; }; @@ -92,7 +104,7 @@ port_hash_rec(const struct ovsrec_port *port) } static char * -tunnel_create_name(struct tunnel_ctx *tc, const char *chassis_id) +tunnel_create_name(const char *chassis_id) { int i; @@ -100,7 +112,7 @@ tunnel_create_name(struct tunnel_ctx *tc, const char *chassis_id) char *port_name; port_name = xasprintf("ovn-%.6s-%x", chassis_id, i); - if (!sset_contains(&tc->port_names, port_name)) { + if (!sset_contains(&tc.port_names, port_name)) { return port_name; } @@ -110,19 +122,32 @@ tunnel_create_name(struct tunnel_ctx *tc, const char *chassis_id) return NULL; } +static struct port_hash_node * +port_lookup_by_uuid(const struct uuid *uuid) +{ + struct hmap_node *node = hmap_first_with_hash(&tc.tunnel_hmap_by_uuid, + uuid_hash(uuid)); + if (node) { + return CONTAINER_OF(node, struct port_hash_node, uuid_node); + } + return NULL; +} static void -tunnel_add(struct tunnel_ctx *tc, const char *new_chassis_id, +tunnel_add(const struct sbrec_chassis *chassis_rec, const struct sbrec_encap *encap) { struct port_hash_node *hash_node; + const char *new_chassis_id = chassis_rec->name; + + /* Check whether such a row already exists in OVS. If so, update + * the uuid field and insert into the by uuid hashmap. If not, + * create the tunnel */ - /* Check whether such a row already exists in OVS. If so, remove it - * from 'tc->tunnel_hmap' and we're done. */ HMAP_FOR_EACH_WITH_HASH (hash_node, node, port_hash(new_chassis_id, encap->type, encap->ip), - &tc->tunnel_hmap) { + &tc.tunnel_hmap) { const struct ovsrec_port *port = hash_node->port; const char *chassis_id = smap_get(&port->external_ids, "ovn-chassis-id"); @@ -142,8 +167,12 @@ tunnel_add(struct tunnel_ctx *tc, const char *new_chassis_id, if (!strcmp(new_chassis_id, chassis_id) && !strcmp(encap->type, iface->type) && !strcmp(encap->ip, ip)) { - hmap_remove(&tc->tunnel_hmap, &hash_node->node); - free(hash_node); + + hash_node->uuid = &chassis_rec->header_.uuid; + if (!port_lookup_by_uuid(hash_node->uuid)) { + hmap_insert(&tc.tunnel_hmap_by_uuid, &hash_node->uuid_node, + uuid_hash(hash_node->uuid)); + } return; } } @@ -155,14 +184,14 @@ tunnel_add(struct tunnel_ctx *tc, const char *new_chassis_id, char *port_name; size_t i; - port_name = tunnel_create_name(tc, new_chassis_id); + port_name = tunnel_create_name(new_chassis_id); if (!port_name) { VLOG_WARN("Unable to allocate unique name for '%s' tunnel", new_chassis_id); return; } - iface = ovsrec_interface_insert(tc->ovs_txn); + iface = ovsrec_interface_insert(tc.ovs_txn); ovsrec_interface_set_name(iface, port_name); ovsrec_interface_set_type(iface, encap->type); smap_add(&options, "remote_ip", encap->ip); @@ -170,23 +199,25 @@ tunnel_add(struct tunnel_ctx *tc, const char *new_chassis_id, ovsrec_interface_set_options(iface, &options); smap_destroy(&options); - port = ovsrec_port_insert(tc->ovs_txn); + port = ovsrec_port_insert(tc.ovs_txn); ovsrec_port_set_name(port, port_name); ovsrec_port_set_interfaces(port, &iface, 1); const struct smap id = SMAP_CONST1(&id, "ovn-chassis-id", new_chassis_id); ovsrec_port_set_external_ids(port, &id); - ports = xmalloc(sizeof *tc->br_int->ports * (tc->br_int->n_ports + 1)); - for (i = 0; i < tc->br_int->n_ports; i++) { - ports[i] = tc->br_int->ports[i]; + ports = xmalloc(sizeof *tc.br_int->ports * (tc.br_int->n_ports + 1)); + for (i = 0; i < tc.br_int->n_ports; i++) { + ports[i] = tc.br_int->ports[i]; } - ports[tc->br_int->n_ports] = port; - ovsrec_bridge_verify_ports(tc->br_int); - ovsrec_bridge_set_ports(tc->br_int, ports, tc->br_int->n_ports + 1); + ports[tc.br_int->n_ports] = port; + ovsrec_bridge_verify_ports(tc.br_int); + ovsrec_bridge_set_ports(tc.br_int, ports, tc.br_int->n_ports + 1); - sset_add(&tc->port_names, port_name); + sset_add(&tc.port_names, port_name); free(port_name); free(ports); + // reset_flow_processing(); + process_full_encaps = true; } static void @@ -224,6 +255,21 @@ preferred_encap(const struct sbrec_chassis *chassis_rec) return best_encap; } +static void +check_and_add_tunnel(const struct sbrec_chassis *chassis_rec, + const char *chassis_id) +{ + if (strcmp(chassis_rec->name, chassis_id)) { + /* Create tunnels to the other chassis. */ + const struct sbrec_encap *encap = preferred_encap(chassis_rec); + if (!encap) { + VLOG_INFO("No supported encaps for '%s'", chassis_rec->name); + return; + } + tunnel_add(chassis_rec, encap); + } +} + void encaps_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, const char *chassis_id) @@ -235,12 +281,7 @@ encaps_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, const struct sbrec_chassis *chassis_rec; const struct ovsrec_bridge *br; - struct tunnel_ctx tc = { - .tunnel_hmap = HMAP_INITIALIZER(&tc.tunnel_hmap), - .port_names = SSET_INITIALIZER(&tc.port_names), - .br_int = br_int - }; - + tc.br_int = br_int; tc.ovs_txn = ctx->ovs_idl_txn; ovsdb_idl_txn_add_comment(tc.ovs_txn, "ovn-controller: modifying OVS tunnels '%s'", @@ -257,36 +298,71 @@ encaps_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, sset_add(&tc.port_names, port->name); - if (smap_get(&port->external_ids, "ovn-chassis-id")) { - struct port_hash_node *hash_node = xzalloc(sizeof *hash_node); - hash_node->bridge = br; - hash_node->port = port; - hmap_insert(&tc.tunnel_hmap, &hash_node->node, - port_hash_rec(port)); + const char *old_chassis_id = smap_get(&port->external_ids, + "ovn-chassis-id"); + if (old_chassis_id) { + if (!hmap_first_with_hash(&tc.tunnel_hmap, + port_hash_rec(port))) { + struct port_hash_node *hash_node = + xzalloc(sizeof *hash_node); + hash_node->bridge = br; + hash_node->port = port; + hmap_insert(&tc.tunnel_hmap, &hash_node->node, + port_hash_rec(port)); + process_full_encaps = true; + } } } } - SBREC_CHASSIS_FOR_EACH(chassis_rec, ctx->ovnsb_idl) { - if (strcmp(chassis_rec->name, chassis_id)) { - /* Create tunnels to the other chassis. */ - const struct sbrec_encap *encap = preferred_encap(chassis_rec); - if (!encap) { - VLOG_INFO("No supported encaps for '%s'", chassis_rec->name); + if (process_full_encaps) { + SBREC_CHASSIS_FOR_EACH (chassis_rec, ctx->ovnsb_idl) { + check_and_add_tunnel(chassis_rec, chassis_id); + } + process_full_encaps = false; + } else { + SBREC_CHASSIS_FOR_EACH_TRACKED (chassis_rec, ctx->ovnsb_idl) { + bool is_deleted = sbrec_chassis_row_get_seqno(chassis_rec, + OVSDB_IDL_CHANGE_DELETE) > 0; + bool is_new = sbrec_chassis_row_get_seqno(chassis_rec, + OVSDB_IDL_CHANGE_MODIFY) == 0; + + if (is_deleted) { + /* lookup the tunnel by row uuid and remove it */ + struct port_hash_node *port_hash = + port_lookup_by_uuid(&chassis_rec->header_.uuid); + if (port_hash) { + bridge_delete_port(port_hash->bridge, port_hash->port); + sset_find_and_delete(&tc.port_names, + port_hash->port->name); + hmap_remove(&tc.tunnel_hmap, &port_hash->node); + hmap_remove(&tc.tunnel_hmap_by_uuid, + &port_hash->uuid_node); + free(port_hash); + } continue; } - tunnel_add(&tc, chassis_rec->name, encap); + if (!is_new) { + if (strcmp(chassis_rec->name, chassis_id)) { + /* TODO: find the tunnel by looking it up based on its + * uuid and then change it. */ + ; + } + } else { + check_and_add_tunnel(chassis_rec, chassis_id); + continue; + } + if (!is_new) { + if (strcmp(chassis_rec->name, chassis_id)) { + /* TODO: find the tunnel by looking it up based on its + * uuid and then change it. */ + ; + } + } else { + check_and_add_tunnel(chassis_rec, chassis_id); + } } } - - /* Delete any existing OVN tunnels that were not still around. */ - struct port_hash_node *hash_node; - HMAP_FOR_EACH_POP (hash_node, node, &tc.tunnel_hmap) { - bridge_delete_port(hash_node->bridge, hash_node->port); - free(hash_node); - } - hmap_destroy(&tc.tunnel_hmap); - sset_destroy(&tc.port_names); } /* Returns true if the database is all cleaned up, false if more work is diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c index f68f842..6912132 100644 --- a/ovn/controller/ovn-controller.c +++ b/ovn/controller/ovn-controller.c @@ -313,6 +313,10 @@ main(int argc, char *argv[]) char *ovnsb_remote = get_ovnsb_remote(ovs_idl_loop.idl); struct ovsdb_idl_loop ovnsb_idl_loop = OVSDB_IDL_LOOP_INITIALIZER( ovsdb_idl_create(ovnsb_remote, &sbrec_idl_class, true, true)); + + /* track the southbound idl */ + ovsdb_idl_track_add_all(ovnsb_idl_loop.idl); + ovsdb_idl_get_initial_snapshot(ovnsb_idl_loop.idl); int probe_interval = 0; @@ -417,6 +421,7 @@ main(int argc, char *argv[]) } ovsdb_idl_loop_commit_and_wait(&ovnsb_idl_loop); ovsdb_idl_loop_commit_and_wait(&ovs_idl_loop); + ovsdb_idl_track_clear(ovnsb_idl_loop.idl); poll_block(); if (should_service_stop()) { exiting = true;
As a side effect, tunnel context is persisted. Signed-off-by: Ryan Moats <rmoats@us.ibm.com> --- ovn/controller/encaps.c | 168 +++++++++++++++++++++++++++++----------- ovn/controller/ovn-controller.c | 5 ++ 2 files changed, 127 insertions(+), 46 deletions(-)