[ovs-dev] [PATCH ovn] northd: properly reconfigure ipam when subnet is changed

Mark Michelson mmichels at redhat.com
Thu Oct 1 14:02:17 UTC 2020


On 9/30/20 9:46 AM, Lorenzo Bianconi wrote:
> Reconfigure dynamic assigned addresses if subnet is modified
> removing IP out of configured netmask if present
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi at redhat.com>
> ---
>   northd/ovn-northd.c | 48 ++++++++++++++++++++++++++-------------------
>   tests/ovn.at        | 10 ++++++++++
>   2 files changed, 38 insertions(+), 20 deletions(-)
> 
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 5fddc5e3e..e527e90fa 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -1665,24 +1665,6 @@ ipam_get_unused_mac(ovs_be32 ip)
>       return mac64;
>   }
>   
> -static uint32_t
> -ipam_get_unused_ip(struct ovn_datapath *od)
> -{
> -    if (!od || !od->ipam_info.allocated_ipv4s) {
> -        return 0;
> -    }
> -
> -    size_t new_ip_index = bitmap_scan(od->ipam_info.allocated_ipv4s, 0, 0,
> -                                      od->ipam_info.total_ipv4s - 1);
> -    if (new_ip_index == od->ipam_info.total_ipv4s - 1) {
> -        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> -        VLOG_WARN_RL( &rl, "Subnet address space has been exhausted.");
> -        return 0;
> -    }
> -
> -    return od->ipam_info.start_ipv4 + new_ip_index;
> -}
> -
>   enum dynamic_update_type {
>       NONE,    /* No change to the address */
>       REMOVE,  /* Address is no longer dynamic */
> @@ -1705,6 +1687,32 @@ struct dynamic_address_update {
>       enum dynamic_update_type ipv6;
>   };
>   
> +static uint32_t
> +ipam_get_unused_ip(struct dynamic_address_update *update)
> +{
> +    struct ovn_datapath *od = update->od;
> +
> +    if (!od || !od->ipam_info.allocated_ipv4s) {
> +        return 0;
> +    }
> +
> +    size_t new_ip_index = bitmap_scan(od->ipam_info.allocated_ipv4s, 0, 0,
> +                                      od->ipam_info.total_ipv4s - 1);
> +    if (new_ip_index == od->ipam_info.total_ipv4s - 1) {
> +        const struct nbrec_logical_switch_port *nbsp = update->op->nbsp;
> +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> +        VLOG_WARN_RL( &rl, "Subnet address space has been exhausted.");
> +
> +        if (nbsp->dynamic_addresses) {
> +            nbrec_logical_switch_port_set_dynamic_addresses(nbsp, NULL);
> +        }

Adding this side effect to ipam_get_unused_ip() makes it less clear what 
this function's purpose is. Before this change, this function would 
attempt to get the next unused IP and return it. Now, it does that but 
it can also change the logical switch port dynamic addresses.

In the case where you're setting the dynamic addresses, the function 
already returns a "bad" value of 0. The caller of ipam_get_unused_ip() 
should be able to use that return value to set the dynamic addresses of 
the switch port appropriately.

I think update_dynamic_addresses() is the only caller of 
ipam_get_unused_ip(). update_dynamic_addresses() already is attempting 
to set dynamic_addresses on the switch port. It probably just needs to 
be adjusted to set the dynamic_addresses NULL in the case where 
update_dynamic_addresses() does not get a valid IPv4 or IPv6 address to use.

> +
> +        return 0;
> +    }
> +
> +    return od->ipam_info.start_ipv4 + new_ip_index;
> +}
> +
>   static enum dynamic_update_type
>   dynamic_mac_changed(const char *lsp_addresses,
>                       struct dynamic_address_update *update)
> @@ -1758,7 +1766,7 @@ dynamic_ip4_changed(const char *lsp_addrs,
>       }
>   
>       uint32_t index = ip4 - ipam->start_ipv4;
> -    if (index > ipam->total_ipv4s ||
> +    if (index >= ipam->total_ipv4s - 1 ||
>           bitmap_is_set(ipam->allocated_ipv4s, index)) {
>           /* Previously assigned dynamic IPv4 address can no longer be used.
>            * It's either outside the subnet, conflicts with an excluded IP,
> @@ -1964,7 +1972,7 @@ update_dynamic_addresses(struct dynamic_address_update *update)
>           ip4 = update->static_ip;
>           break;
>       case DYNAMIC:
> -        ip4 = htonl(ipam_get_unused_ip(update->od));
> +        ip4 = htonl(ipam_get_unused_ip(update));
>           VLOG_INFO("Assigned dynamic IPv4 address '"IP_FMT"' to port '%s'",
>                     IP_ARGS(ip4), update->op->nbsp->name);
>       }
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 7769b69ed..e27a74081 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -7473,6 +7473,16 @@ AT_CHECK([ovn-nbctl get Logical-Switch-Port p103 dynamic_addresses], [0],
>       ["22:33:44:55:66:77 172.16.1.250"
>   ])
>   
> +ovn-nbctl ls-add sw12
> +for i in $(seq 1 200); do
> +    ovn-nbctl lsp-add sw12 sw12-p$i -- lsp-set-addresses sw12-p$i "00:00:00:00:00:$i dynamic"
> +done
> +ovn-nbctl set Logical-Switch sw12 other_config:subnet=192.10.2.0/24
> +ovn-nbctl set Logical-Switch sw12 other_config:subnet=192.10.2.0/25
> +AT_CHECK([ovn-nbctl list Logical-Switch-Port | grep -q 192.10.2.127], [1])
> +AT_CHECK([ovn-nbctl list Logical-Switch-Port | grep -q 192.10.2.128], [1])
> +AT_CHECK([ovn-nbctl list Logical-Switch-Port | grep -q 192.10.2.180], [1])
> +
>   as ovn-sb
>   OVS_APP_EXIT_AND_WAIT([ovsdb-server])
>   
> 



More information about the dev mailing list