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 <[email protected]> > >>--- > >>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@central vagrant]# ovn-nbctl ls-add switch > [root@central vagrant]# ovn-nbctl lsp-add switch port1 > [root@central vagrant]# ovn-nbctl lsp-set-addresses port1 "00:00:00:00:00:01 > dynamic" "00:00:00:00:00:02 dynamic" > [root@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@central vagrant]# ovn-nbctl set Logical_Switch switch > other_config:subnet=10.0.0.0/8 > [root@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. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
