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

Mark Michelson mmichels at redhat.com
Mon Jul 9 13:59:04 UTC 2018


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'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.

> 
> Thanks,
> 
> Ben.
> 
> --8<--------------------------cut here-------------------------->8--
> 
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 12c9929da039..80eb0e6bcb3a 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -1065,10 +1065,11 @@ enum dynamic_update_type {
>   };
>   
>   struct dynamic_address_update {
> +    struct ovs_list node;       /* In build_ipam()'s list of updates. */
> +
>       struct ovn_port *op;
> -    struct ovn_datapath *od;
> +
>       struct lport_addresses current_addresses;
> -    struct ovs_list list;
>       struct eth_addr static_mac;
>       enum dynamic_update_type mac;
>       enum dynamic_update_type ipv4;
> @@ -1102,7 +1103,7 @@ dynamic_mac_changed(const char *lsp_addresses,
>   static enum dynamic_update_type
>   dynamic_ip4_changed(struct dynamic_address_update *update)
>   {
> -    bool dynamic_ip4 = update->od->ipam_info.allocated_ipv4s != NULL;
> +    bool dynamic_ip4 = update->op->od->ipam_info.allocated_ipv4s != NULL;
>   
>       if (!dynamic_ip4) {
>           if (update->current_addresses.n_ipv4_addrs) {
> @@ -1118,13 +1119,13 @@ dynamic_ip4_changed(struct dynamic_address_update *update)
>       }
>   
>       uint32_t ip4 = ntohl(update->current_addresses.ipv4_addrs[0].addr);
> -    if (ip4 < update->od->ipam_info.start_ipv4) {
> +    if (ip4 < update->op->od->ipam_info.start_ipv4) {
>           return DYNAMIC;
>       }
>   
> -    uint32_t index = ip4 - update->od->ipam_info.start_ipv4;
> -    if (index > update->od->ipam_info.total_ipv4s ||
> -        bitmap_is_set(update->od->ipam_info.allocated_ipv4s, index)) {
> +    uint32_t index = ip4 - update->op->od->ipam_info.start_ipv4;
> +    if (index > update->op->od->ipam_info.total_ipv4s ||
> +        bitmap_is_set(update->op->od->ipam_info.allocated_ipv4s, index)) {
>           /* Previously assigned dynamic IPv4 address can no longer be used.
>            * It's either outside the subnet, conflicts with an excluded IP,
>            * or conflicts with a statically-assigned address on the switch
> @@ -1138,7 +1139,7 @@ dynamic_ip4_changed(struct dynamic_address_update *update)
>   static enum dynamic_update_type
>   dynamic_ip6_changed(struct dynamic_address_update *update)
>   {
> -    bool dynamic_ip6 = update->od->ipam_info.ipv6_prefix_set;
> +    bool dynamic_ip6 = update->op->od->ipam_info.ipv6_prefix_set;
>   
>       if (!dynamic_ip6) {
>           if (update->current_addresses.n_ipv6_addrs) {
> @@ -1164,8 +1165,8 @@ dynamic_ip6_changed(struct dynamic_address_update *update)
>   
>       struct in6_addr masked = ipv6_addr_bitand(
>           &update->current_addresses.ipv6_addrs[0].addr,
> -        &update->od->ipam_info.ipv6_prefix);
> -    if (!IN6_ARE_ADDR_EQUAL(&masked, &update->od->ipam_info.ipv6_prefix)) {
> +        &update->op->od->ipam_info.ipv6_prefix);
> +    if (!IN6_ARE_ADDR_EQUAL(&masked, &update->op->od->ipam_info.ipv6_prefix)) {
>           return DYNAMIC;
>       }
>   
> @@ -1191,7 +1192,7 @@ dynamic_addresses_check_for_updates(const char *lsp_addrs,
>           ipam_insert_mac(&update->current_addresses.ea, false);
>       }
>       if (update->ipv4 == NONE && update->current_addresses.n_ipv4_addrs) {
> -        ipam_insert_ip(update->od,
> +        ipam_insert_ip(update->op->od,
>                          ntohl(update->current_addresses.ipv4_addrs[0].addr));
>       }
>       if (update->mac == NONE &&
> @@ -1228,12 +1229,12 @@ set_dynamic_updates(const char *addrspec,
>       } else {
>           update->mac = DYNAMIC;
>       }
> -    if (update->od->ipam_info.allocated_ipv4s) {
> +    if (update->op->od->ipam_info.allocated_ipv4s) {
>           update->ipv4 = DYNAMIC;
>       } else {
>           update->ipv4 = NONE;
>       }
> -    if (update->od->ipam_info.ipv6_prefix_set) {
> +    if (update->op->od->ipam_info.ipv6_prefix_set) {
>           update->ipv6 = DYNAMIC;
>       } else {
>           update->ipv6 = NONE;
> @@ -1323,16 +1324,11 @@ build_ipam(struct hmap *datapaths, struct hmap *ports)
>           if (!od->nbs) {
>               continue;
>           }
> -        struct dynamic_address_update *update;
> +
>           struct ovs_list updates;
>           ovs_list_init(&updates);
>           for (size_t i = 0; i < od->nbs->n_ports; i++) {
> -            const struct nbrec_logical_switch_port *nbsp =
> -                od->nbs->ports[i];
> -
> -            if (!nbsp) {
> -                continue;
> -            }
> +            const struct nbrec_logical_switch_port *nbsp = od->nbs->ports[i];
>   
>               if (!od->ipam_info.allocated_ipv4s &&
>                   !od->ipam_info.ipv6_prefix_set) {
> @@ -1344,7 +1340,7 @@ build_ipam(struct hmap *datapaths, struct hmap *ports)
>               }
>   
>               struct ovn_port *op = ovn_port_find(ports, nbsp->name);
> -            if (!op || (op->nbsp && op->peer)) {
> +            if (!op || op->nbsp != nbsp || op->peer) {
>                   /* Do not allocate addresses for logical switch ports that
>                    * have a peer. */
>                   continue;
> @@ -1354,15 +1350,15 @@ build_ipam(struct hmap *datapaths, struct hmap *ports)
>                   if (!is_dynamic_lsp_address(nbsp->addresses[j])) {
>                       continue;
>                   }
> -                update = xzalloc(sizeof *update);
> -                update->od = od;
> +                struct dynamic_address_update *update
> +                    = xzalloc(sizeof *update);
>                   update->op = op;
>                   if (nbsp->dynamic_addresses) {
> -                    extract_lsp_addresses(op->nbsp->dynamic_addresses,
> +                    extract_lsp_addresses(nbsp->dynamic_addresses,
>                                             &update->current_addresses);
>                       if (dynamic_addresses_check_for_updates(nbsp->addresses[j],
>                                                               update)) {
> -                        ovs_list_push_back(&updates, &update->list);
> +                        ovs_list_push_back(&updates, &update->node);
>                       } else {
>                           /* No changes to dynamic addresses */
>                           set_lsp_dynamic_addresses(nbsp->dynamic_addresses, op);
> @@ -1371,20 +1367,20 @@ build_ipam(struct hmap *datapaths, struct hmap *ports)
>                       }
>                   } else {
>                       set_dynamic_updates(nbsp->addresses[j], update);
> -                    ovs_list_push_back(&updates, &update->list);
> +                    ovs_list_push_back(&updates, &update->node);
>                   }
>               }
>   
>               if (!nbsp->n_addresses && nbsp->dynamic_addresses) {
> -                nbrec_logical_switch_port_set_dynamic_addresses(op->nbsp,
> -                                                                NULL);
> +                nbrec_logical_switch_port_set_dynamic_addresses(nbsp, NULL);
>               }
>           }
>   
>           /* After retaining all unchanged dynamic addresses, now assign
>            * new ones.
>            */
> -        LIST_FOR_EACH_POP (update, list, &updates) {
> +        struct dynamic_address_update *update;
> +        LIST_FOR_EACH_POP (update, node, &updates) {
>               update_dynamic_addresses(od, update);
>               destroy_lport_addresses(&update->current_addresses);
>               free(update);
> 



More information about the dev mailing list