> On Apr 28, 2015, at 4:16 PM, Ben Pfaff <b...@nicira.com> wrote: > > On Mon, Apr 27, 2015 at 10:14:51PM -0700, Justin Pettit wrote: >> This creates a tunnel to each Chassis's specified Encaps. In the >> future, we may want to limit this to only Chassis that share a logical >> datapath on the local system. >> >> Signed-off-by: Justin Pettit <jpet...@nicira.com> > > I had a little bit of a mental impedance matching problem with the > terminology used in this patch. The patch seems to use the terms > "encaps(ulation)" and "tunnel" interchangeably. I think of GRE, VXLAN, > Geneve, STT, etc., as "encapsulations", but instances of each of these > protocols between a pair of nodes as "tunnels". This threw me for a bit > of a loop until I recognized it.
I did struggle with making a distinction between them. I like your suggestion, and I reworked the patch to match that. > s/generarting/generating/ and s/remaing/remaining/: >> + * generated we remove them. After generarting all the rows, any >> + * remaing in 'port_hmap' must be deleted from the database. */ >> + struct hmap port_hmap; Fixed. > I noticed that the data values in port_names are only used for ports > that are also in a port_hash_node. It seemed slightly counterintuitive > to me. How about this incremental: That's better. Thanks! > Here in update_encaps(), I think that the !encap case here should not > call encap_add(): > > /* Create tunnels to the other chassis. */ > const struct sbrec_encap *encap = preferred_tunnel(chassis_rec); > if (!encap) { > VLOG_INFO("No supported encaps for '%s'", chassis_rec->name); > } > encap_add(&ec, chassis_rec->name, encap); Good catch. > I'm concerned about the growing number of blocking round-trips to the > database server. I think it's OK for now, but I also think it will > eventually bite us. I agree. I have it on my to-do list to switch to a model closer to ovn-northd that does all the commits in main loop. > I guess that preferred_tunnel() will prefer Geneve over STT since it's > first in alphabetical order and the IDL always sorts sets. Maybe that > should be made more obvious? I added a comment. > Here in encap_add(), I think that the !port_name case should bail out > instead of continuing: > port_name = encap_create_name(ec, new_chassis_id); > if (!port_name) { > VLOG_WARN("Unable to allocate unique name for '%s' encap", > new_chassis_id); > } Yep. Thanks. > I think that encap_add() leaks external_ids and options. D'oh! Thanks for the reviews. I'll send out a v2 of the series momentarily. --Justin _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev