On Sun, Jul 3, 2016 at 11:16 AM, Ben Pfaff <b...@ovn.org> wrote:
> 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.
Okay, I have fixed that.
> 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.
I have created two helper functions ipam_is_duplicate_mac() and
ipam_is_duplicate_ip()
to fix this.
>
> 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.
ipam_get_unused_mac() will now start its search from just past the
last-assigned
address. For ipam_get_unused_ip(), I am not sure what the operator's
expectation
is for ip address assignment and so for the time being the search
starts from
the beginning of the address space.
>
>
> 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.
>
I have changed the "subnet" configuration to reside in the other_config
column
in the Logical_Switch table.
>
> 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.
>
Dynamically allocated addresses will now reside in a new
"dynamic_addresses"
column in the Logical_Switch_Port table instead of the "addresses"
column.
>
> 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.
>
A logical switch port's "dynamic_addresses" column will now be
populated with a
dynamically allocated MAC and IPv4 address when the corresponding
logical
switch's subnet is set and the "dynamic" keyword is in the logical
switch port's
addresses column.
>
> 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.
>
As of now, I have not been able to provide a solution for this. The
burden of
copying dynamically allocated addresses into "port_security" remains on
the
operator.
>
> Thanks,
>
> Ben.
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
