[ovs-dev] [PATCH v2] ovn: Allow for automatic dynamic updates of IPAM

[Ben Pfaff]

On Mon, Jul 09, 2018 at 09:59:04AM -0400, Mark Michelson wrote:
> On 07/06/2018 06:36 PM, Ben Pfaff wrote:
> >On Mon, Jun 25, 2018 at 04:09:53PM -0400, Mark Michelson wrote:
> >>OVN offers a method of IP address management that allows for an IPv4 subnet or
> >>IPv6 prefix to be specified on a logical switch. Then by specifying a
> >>switch port's address as "dynamic" or "<mac address> dynamic", OVN will
> >>automatically assign addresses to the switch port.
> >>
> >>While this works great for initial assignment of addresses, addresses do
> >>not automatically adjust when changes are made to the switch's
> >>configuration. For instance:
> >>* If the subnet, ipv6_prefix, or exclude_ips for a logical switch
> >>changes, the affected switch ports are not updated.
> >>* If a switch port with a static IP address is added to the switch, and
> >>that address conflicts with a dynamically assigned IP address, the
> >>dynamic address is not updated.
> >>* If a MAC address switched from being statically assigned to
> >>dynamically assigned, the MAC address would not be updated.
> >>* If a statically assigned MAC address changed, then the IPv6 address
> >>would not be updated.
> >>
> >>This patch solves all of the above issues by changing the algorithm for
> >>IPAM assignment. There are essentially three steps.
> >>1) While joining logical ports, all statically-assigned addresses (i.e.
> >>any ports without "dynamic" addresses) have their addresses registered
> >>to IPAM. This gives them top priority.
> >>2) All logical ports with dynamic addresses are inspected. Any changes
> >>that must be made to the addresses are collected to be made later. Any
> >>addresses that do not require change are registered to IPAM. This allows
> >>for previously assigned dynamic addresses to be kept.
> >>3) All gathered changes are enacted.
> >>
> >>The change contains new tests that ensure that dynamic addresses are
> >>updated when appropriate.
> >>
> >>This patch also alters some existing IPAM tests. Those tests assumed
> >>that dynamic addresses would not be updated automatically, so those
> >>tests either had to be altered or removed.
> >>
> >>Signed-off-by: Mark Michelson <mmichels at redhat.com>
> >>---
> >>v2->v3:
> >>  Fixed a checkpatch problem (line too long)
> >>
> >>v1->v2:
> >>  Rebased
> >
> >Thanks for the new version.
> >
> >I spent some time trying to understand this.  I think one of my issues
> >with it is that there is an unstated assumption that a logical switch
> >port has at most one dynamic address, but nothing that checks for or
> >enforces it.  It's possible to request more than one if you put multiple
> >"xx:xx:xx:xx:xx:xx dynamic" entries in an addresses column, but I don't
> >think that the code really treats them properly since it tries to
> >independently diff each one of them against the current set of
> >dynamically assigned addresses.  Is this all correct?  If so, would you
> >figure out some way to fix it?
> 
> You're definitely correct that there's an assumption that there is only a
> single dynamic address. However, this assumption is not introduced by my
> patch. I've done a quick test on a vagrant system using current master:
> 
> [root at central vagrant]# ovn-nbctl ls-add switch
> [root at central vagrant]# ovn-nbctl lsp-add switch port1
> [root at central vagrant]# ovn-nbctl lsp-set-addresses port1 "00:00:00:00:00:01
> dynamic" "00:00:00:00:00:02 dynamic"
> [root at central vagrant]# ovn-nbctl list logical_switch_port
> _uuid               : 6b880956-eb8b-49fc-bb0e-cfaa08360cc0
> addresses           : ["00:00:00:00:00:01 dynamic", "00:00:00:00:00:02
> dynamic"]
> dhcpv4_options      : []
> dhcpv6_options      : []
> dynamic_addresses   : []
> enabled             : []
> external_ids        : {}
> name                : "port1"
> options             : {}
> parent_name         : []
> port_security       : []
> tag                 : []
> tag_request         : []
> type                : ""
> up                  : false
> [root at central vagrant]# ovn-nbctl set Logical_Switch switch
> other_config:subnet=10.0.0.0/8
> [root at central vagrant]# ovn-nbctl list logical_switch_port
> _uuid               : 6b880956-eb8b-49fc-bb0e-cfaa08360cc0
> addresses           : ["00:00:00:00:00:01 dynamic", "00:00:00:00:00:02
> dynamic"]
> dhcpv4_options      : []
> dhcpv6_options      : []
> dynamic_addresses   : "00:00:00:00:00:02 10.0.0.2"
> enabled             : []
> external_ids        : {}
> name                : "port1"
> options             : {}
> parent_name         : []
> port_security       : []
> tag                 : []
> tag_request         : []
> type                : ""
> up                  : false
> 
> So I guess the question is, should there only be a single dynamic address
> assigned per switch port? If so, I guess some sort of warning should be
> logged if multiple dynamic addresses are requested? Or should we allow for
> multiple dynamic addresses to be assigned per switch port?

I don't think it's necessary to support multiple dynamic addresses,
although I wouldn't object.  A warning would be a nice enhancement.

> >I'm appending some incrementals that make sense to me.  I removed 'od'
> >from the struct because it's redundant with op->od.  I moved the
> >ovs_list member to the top because that's where we usually put a member
> >used to show membership in a larger structure, and renamed it from
> >'list' to 'node' because this ovs_list is used for membership not for
> >containment.  I also added a check in build_ipam() that op->nbsp ==
> >nbsp, which I think the code assumed and might be ovs_assert()'able but
> >at least checking on it seems wise.
> >
> >If this makes sense, would you mind working on a v4?
> 
> These incrementals all look good to me. Before doing v4 though, I'll need to
> make sure about the matter discussed above.

Sure.