> On Apr 28, 2015, at 4:16 PM, Ben Pfaff <[email protected]> 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 <[email protected]>
>
> 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
[email protected]
http://openvswitch.org/mailman/listinfo/dev
