[ovs-dev] [PATCH v3] ovn-northd, tests: Adding IPAM to ovn-northd.

[Nimay Desai]

On Sun, Jul 3, 2016 at 11:16 AM, Ben Pfaff <blp at 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 <nimaydesai1 at 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.
>