On Wed, Jun 29, 2016 at 01:53:24PM -0700, Nimay Desai wrote: > Added an IPv4 and MAC addresses management system to ovn-northd. When a > logical > switch's options:subnet field is set, logical ports attached to that switch > that do not have a MAC/IPv4 address will automatically be allocated a globally > unique MAC address/unused IPv4 address within the provided subnet. This > can be useful for a user who wants to deploy many VM's or containers with > networking capabilities, but does not care about the specific MAC/IPv4 > addresses that are assigned. > > Added tests in ovn.at for ipam. > > Signed-off-by: Nimay Desai <nimaydes...@gmail.com>
Thanks for working on this! "sparse" reports that MAC_ADDR_PREFIX is "long long" even though it's suffixed with plain "L". I'd use a "ULL" suffix instead. There seems to be multiple instances of open-coded HMAP_FOR_EACH_WITH_HASH searches for IP and MAC addresses. I recommend adding helper functions. It seems inefficient in ipam_get_unused_mac() and ipam_get_unused_ip() to start the search from the beginning of the space instead of from just past the last-assigned address, or in random order. I don't know whether it matters though. Our usual pattern, I think, is that a column is named "options" if the meaning of its key-value pairs depend on the type of an entity, and "other_config" if the meaning does not depend on a type. I think that this "subnet" configuration is independent of type (there is only one type of Logical_Switch), so I would put it in an other_config column. Everything following is about a few things that concern me a little. None of these are necessarily big problems, but I want to bring them up so that they can be thought through if necessary. Usually we do not design OVSDB schemas so that daemons with different purposes modify the same column, because it can be a recipe for confusion. For example, we use separate columns in the Open_vSwitch schema to report the MAC address for a port and to request that a port be assigned a specific MAC address, because with a single column it's difficult to distinguish whether its value is meant for one purpose or the other. I am not sure that this is exactly the same case, but if we were going to follow this principle here, too, I would add a new column to Logical_Switch_Port for northd-assigned addresses. It could be called "dynamic_addresses", for example. I am not sure that I am comfortable with the idea of assigning a dynamic address automatically when there is nothing in the addresses column. It seems somewhat surprising. I would consider adding a new "addresses" syntax to request a dynamic MAC/IP; for example, having it requested by specifying "dynamic" in addresses. I suspect that many deployments would want to enable port security for dynamic addresses, but I don't see a way to do that with the current design, except in a race-prone way where the CMS copies "addresses" into "port_security" once it's populated. Thanks, Ben. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
