diff mbox

[ovs-dev,v2,1/2] ovn-northd ipam: Support 'exclude_ips' option

Message ID 20170307014913.2390-1-nusiddiq@redhat.com
State Changes Requested
Headers show

Commit Message

Numan Siddique March 7, 2017, 1:49 a.m. UTC
From: Numan Siddique <nusiddiq@redhat.com>

If the CMS wants to make use of ovn ipam it can now provide a
list of IPv4 addresses and a range of IPv4 addresses which
will be excluded from the dynamic address assignment.
To support this, a new option 'exclude_ips' is added in the
Logical_switch.other_config column.

Eg. ovn-nbctl set Logical_switch sw0
other_config:exclude_ips="10.0.0.2 10.0.0.30-10.0.0.40"

The present code, uses hash maps to store the assigned IP addresses.
In order to support this option, this patch has refactored the IPAM
assignment. It now uses a bitmap to manage the IP assignment with
each bit in the bitmap representing an IPv4 address.

Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
---
 ovn/northd/ovn-northd.c | 250 +++++++++++++++++++++++++++---------------------
 ovn/ovn-nb.xml          |  24 ++++-
 tests/ovn.at            |  44 +++++++++
 3 files changed, 209 insertions(+), 109 deletions(-)

Comments

Ben Pfaff March 8, 2017, 6:49 p.m. UTC | #1
On Tue, Mar 07, 2017 at 07:19:13AM +0530, nusiddiq@redhat.com wrote:
> From: Numan Siddique <nusiddiq@redhat.com>
> 
> If the CMS wants to make use of ovn ipam it can now provide a
> list of IPv4 addresses and a range of IPv4 addresses which
> will be excluded from the dynamic address assignment.
> To support this, a new option 'exclude_ips' is added in the
> Logical_switch.other_config column.
> 
> Eg. ovn-nbctl set Logical_switch sw0
> other_config:exclude_ips="10.0.0.2 10.0.0.30-10.0.0.40"
> 
> The present code, uses hash maps to store the assigned IP addresses.
> In order to support this option, this patch has refactored the IPAM
> assignment. It now uses a bitmap to manage the IP assignment with
> each bit in the bitmap representing an IPv4 address.
> 
> Signed-off-by: Numan Siddique <nusiddiq@redhat.com>

It might be cleaner to use the OVN lexer instead of an ad hoc parser
here.  Also, OVN elsewhere uses .. for ranges, it might be best to stick
with that instead of using - here.

I find myself wondering whether this is the best way to specify address
assignments.  One could invent other ways, for example to allow "subnet"
to specify multiple subnets or ranges and, if necessary, adding an
"except" keyword to subtract subnets or ranges.

I wish we hadn't given "subnet" such a generic name, since it's just for
DHCP address assignment. 

Thank you for working on this!
diff mbox

Patch

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 03dc850..5b471e1 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -372,6 +372,13 @@  port_has_qos_params(const struct smap *opts)
             smap_get(opts, "qos_burst"));
 }
 
+
+struct ipam_info {
+    uint32_t start_ipv4;
+    size_t total_ipv4s;
+    unsigned long *allocated_ipv4s; /* A bitmap of allocated IPv4s */
+};
+
 /* The 'key' comes from nbs->header_.uuid or nbr->header_.uuid or
  * sb->external_ids:logical-switch. */
 struct ovn_datapath {
@@ -394,7 +401,7 @@  struct ovn_datapath {
     bool has_unknown;
 
     /* IPAM data. */
-    struct hmap ipam;
+    struct ipam_info *ipam_info;
 
     /* OVN northd only needs to know about the logical router gateway port for
      * NAT on a distributed router.  This "distributed gateway port" is
@@ -420,21 +427,6 @@  cleanup_macam(struct hmap *macam)
     }
 }
 
-struct ipam_node {
-    struct hmap_node hmap_node;
-    uint32_t ip_addr; /* Allocated IP address. */
-};
-
-static void
-destroy_ipam(struct hmap *ipam)
-{
-    struct ipam_node *node;
-    HMAP_FOR_EACH_POP (node, hmap_node, ipam) {
-        free(node);
-    }
-    hmap_destroy(ipam);
-}
-
 static struct ovn_datapath *
 ovn_datapath_create(struct hmap *datapaths, const struct uuid *key,
                     const struct nbrec_logical_switch *nbs,
@@ -447,7 +439,6 @@  ovn_datapath_create(struct hmap *datapaths, const struct uuid *key,
     od->nbs = nbs;
     od->nbr = nbr;
     hmap_init(&od->port_tnlids);
-    hmap_init(&od->ipam);
     od->port_key_hint = 0;
     hmap_insert(datapaths, &od->key_node, uuid_hash(&od->key));
     return od;
@@ -462,7 +453,10 @@  ovn_datapath_destroy(struct hmap *datapaths, struct ovn_datapath *od)
          * use it. */
         hmap_remove(datapaths, &od->key_node);
         destroy_tnlids(&od->port_tnlids);
-        destroy_ipam(&od->ipam);
+        if (od->ipam_info) {
+            bitmap_free(od->ipam_info->allocated_ipv4s);
+            free(od->ipam_info);
+        }
         free(od->router_ports);
         free(od);
     }
@@ -508,6 +502,89 @@  lrouter_is_enabled(const struct nbrec_logical_router *lrouter)
 }
 
 static void
+init_ipam_info_for_datapath(struct ovn_datapath *od)
+{
+    if (!od->nbs) {
+        return;
+    }
+
+    const char *subnet_str = smap_get(&od->nbs->other_config, "subnet");
+    if (!subnet_str) {
+        return;
+    }
+
+    ovs_be32 subnet, mask;
+    char *error = ip_parse_masked(subnet_str, &subnet, &mask);
+    if (error || mask == OVS_BE32_MAX || !ip_is_cidr(mask)) {
+        static struct vlog_rate_limit rl
+            = VLOG_RATE_LIMIT_INIT(5, 1);
+        VLOG_WARN_RL(&rl, "bad 'subnet' %s", subnet_str);
+        free(error);
+        return;
+    }
+
+    od->ipam_info = xzalloc(sizeof *od->ipam_info);
+    od->ipam_info->start_ipv4 = ntohl(subnet) + 1;
+    od->ipam_info->total_ipv4s = ~ntohl(mask);
+    od->ipam_info->allocated_ipv4s =
+        bitmap_allocate(od->ipam_info->total_ipv4s);
+
+    /* Mark first IP as taken */
+    bitmap_set1(od->ipam_info->allocated_ipv4s, 0);
+
+    /* Check if there are any reserver IPs (list) to be excluded from IPAM */
+    const char *exclude_ip_list = smap_get(&od->nbs->other_config,
+                                           "exclude_ips");
+    if (!exclude_ip_list) {
+        return;
+    }
+
+    const char *buf_end = exclude_ip_list + strlen(exclude_ip_list);
+    const char *buf = exclude_ip_list;
+    int buf_index;
+    ovs_be32 ip1, ip2;
+    /* exclude_ip_list could be in the format -
+     *  "10.0.0.4,10.0.0.10,10.0.0.20-10.0.0.50,10.0.0.100-10.0.0.110".
+     */
+    while (buf < buf_end) {
+        buf_index = 0;
+        if (ovs_scan_len(buf, &buf_index, IP_SCAN_FMT"-"IP_SCAN_FMT,
+                         IP_SCAN_ARGS(&ip1), IP_SCAN_ARGS(&ip2))) {
+            /* OK. */
+        } else if (ovs_scan_len(buf, &buf_index, IP_SCAN_FMT,
+                                IP_SCAN_ARGS(&ip1))) {
+            ip2 = 0;
+        } else {
+            static struct vlog_rate_limit rl
+                        = VLOG_RATE_LIMIT_INIT(5, 1);
+            VLOG_WARN_RL(&rl, "bad 'exclude ip list format' %s",
+                         exclude_ip_list);
+            break;
+        }
+
+        uint32_t ip_start = ntohl(ip1);
+        uint32_t ip_end = ntohl(ip2);
+
+        /* Validate ip_start. */
+        if ((ip_end && ip_start > ip_end) ||
+            ip_start < od->ipam_info->start_ipv4 ||
+            ip_start > (od->ipam_info->start_ipv4 + od->ipam_info->total_ipv4s)) {
+            /* Invalid format. */
+            break;
+        }
+
+        if (ip_end > od->ipam_info->start_ipv4 + od->ipam_info->total_ipv4s) {
+            break;
+        }
+
+        size_t count = ip_end ? (ip_end - ip_start) + 1 : 1;
+        bitmap_set_multiple(od->ipam_info->allocated_ipv4s,
+                            ip_start - od->ipam_info->start_ipv4, count, 1);
+        buf += buf_index +1;
+    }
+}
+
+static void
 join_datapaths(struct northd_context *ctx, struct hmap *datapaths,
                struct ovs_list *sb_only, struct ovs_list *nb_only,
                struct ovs_list *both)
@@ -560,6 +637,8 @@  join_datapaths(struct northd_context *ctx, struct hmap *datapaths,
                                      nbs, NULL, NULL);
             ovs_list_push_back(nb_only, &od->list);
         }
+
+        init_ipam_info_for_datapath(od);
     }
 
     const struct nbrec_logical_router *nbr;
@@ -787,24 +866,6 @@  ipam_is_duplicate_mac(struct eth_addr *ea, uint64_t mac64, bool warn)
     return false;
 }
 
-static bool
-ipam_is_duplicate_ip(struct ovn_datapath *od, uint32_t ip, bool warn)
-{
-    struct ipam_node *ipam_node;
-    HMAP_FOR_EACH_WITH_HASH (ipam_node, hmap_node, hash_int(ip, 0),
-                             &od->ipam) {
-        if (ipam_node->ip_addr == ip) {
-            if (warn) {
-                static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
-                VLOG_WARN_RL(&rl, "Duplicate IP set: "IP_FMT,
-                             IP_ARGS(htonl(ip)));
-            }
-            return true;
-        }
-    }
-    return false;
-}
-
 static void
 ipam_insert_mac(struct eth_addr *ea, bool check)
 {
@@ -827,19 +888,16 @@  ipam_insert_mac(struct eth_addr *ea, bool check)
 }
 
 static void
-ipam_insert_ip(struct ovn_datapath *od, uint32_t ip, bool check)
+ipam_insert_ip(struct ovn_datapath *od, uint32_t ip)
 {
-    if (!od) {
+    if (!od || !od->ipam_info || !od->ipam_info->allocated_ipv4s) {
         return;
     }
 
-    if (check && ipam_is_duplicate_ip(od, ip, true)) {
-        return;
+    if (ip >= od->ipam_info->start_ipv4 &&
+        ip < (od->ipam_info->start_ipv4 + od->ipam_info->total_ipv4s)) {
+        bitmap_set1(od->ipam_info->allocated_ipv4s, ip - od->ipam_info->start_ipv4);
     }
-
-    struct ipam_node *new_ipam_node = xmalloc(sizeof *new_ipam_node);
-    new_ipam_node->ip_addr = ip;
-    hmap_insert(&od->ipam, &new_ipam_node->hmap_node, hash_int(ip, 0));
 }
 
 static void
@@ -861,14 +919,14 @@  ipam_insert_lsp_addresses(struct ovn_datapath *od, struct ovn_port *op,
 
     /* IP is only added to IPAM if the switch's subnet option
      * is set, whereas MAC is always added to MACAM. */
-    if (!smap_get(&od->nbs->other_config, "subnet")) {
+    if (!od->ipam_info || !od->ipam_info->allocated_ipv4s) {
         destroy_lport_addresses(&laddrs);
         return;
     }
 
     for (size_t j = 0; j < laddrs.n_ipv4_addrs; j++) {
         uint32_t ip = ntohl(laddrs.ipv4_addrs[j].addr);
-        ipam_insert_ip(od, ip, true);
+        ipam_insert_ip(od, ip);
     }
 
     destroy_lport_addresses(&laddrs);
@@ -907,7 +965,7 @@  ipam_add_port_addresses(struct ovn_datapath *od, struct ovn_port *op)
 
         for (size_t i = 0; i < lrp_networks.n_ipv4_addrs; i++) {
             uint32_t ip = ntohl(lrp_networks.ipv4_addrs[i].addr);
-            ipam_insert_ip(op->peer->od, ip, true);
+            ipam_insert_ip(op->peer->od, ip);
         }
 
         destroy_lport_addresses(&lrp_networks);
@@ -944,41 +1002,32 @@  ipam_get_unused_mac(void)
 }
 
 static uint32_t
-ipam_get_unused_ip(struct ovn_datapath *od, uint32_t subnet, uint32_t mask)
+ipam_get_unused_ip(struct ovn_datapath *od)
 {
-    if (!od) {
+    if (!od || !od->ipam_info || !od->ipam_info->allocated_ipv4s) {
         return 0;
     }
 
-    uint32_t ip = 0;
-
-    /* Find an unused IP address in subnet. x.x.x.1 is reserved for a
-     * logical router port. */
-    for (uint32_t i = 2; i < ~mask; i++) {
-        uint32_t tentative_ip = subnet + i;
-        if (!ipam_is_duplicate_ip(od, tentative_ip, false)) {
-            ip = tentative_ip;
-            break;
-        }
-    }
-
-    if (!ip) {
+    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 ip;
+    return od->ipam_info->start_ipv4 + new_ip_index;
 }
 
 static bool
 ipam_allocate_addresses(struct ovn_datapath *od, struct ovn_port *op,
-                        const char *addrspec, ovs_be32 subnet, ovs_be32 mask)
+                        const char *addrspec)
 {
     if (!od || !op || !op->nbsp) {
         return false;
     }
 
-    uint32_t ip = ipam_get_unused_ip(od, ntohl(subnet), ntohl(mask));
+    uint32_t ip = ipam_get_unused_ip(od);
     if (!ip) {
         return false;
     }
@@ -1000,9 +1049,9 @@  ipam_allocate_addresses(struct ovn_datapath *od, struct ovn_port *op,
         check_mac = false;
     }
 
-    /* Add MAC/IP to MACAM/IPAM hmaps if both addresses were allocated
+    /* Add MAC to MACAM and IP to IPAM bitmap if both addresses were allocated
      * successfully. */
-    ipam_insert_ip(od, ip, false);
+    ipam_insert_ip(od, ip);
     ipam_insert_mac(&mac, check_mac);
 
     char *new_addr = xasprintf(ETH_ADDR_FMT" "IP_FMT,
@@ -1026,54 +1075,39 @@  build_ipam(struct hmap *datapaths, struct hmap *ports)
      * ports that have the "dynamic" keyword in their addresses column. */
     struct ovn_datapath *od;
     HMAP_FOR_EACH (od, key_node, datapaths) {
-        if (od->nbs) {
-            const char *subnet_str = smap_get(&od->nbs->other_config,
-                                              "subnet");
-            if (!subnet_str) {
+        if (!od->nbs || !od->ipam_info || !od->ipam_info->allocated_ipv4s) {
+            continue;
+        }
+
+        struct ovn_port *op;
+        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;
             }
 
-            ovs_be32 subnet, mask;
-            char *error = ip_parse_masked(subnet_str, &subnet, &mask);
-            if (error || mask == OVS_BE32_MAX || !ip_is_cidr(mask)) {
-                static struct vlog_rate_limit rl
-                    = VLOG_RATE_LIMIT_INIT(5, 1);
-                VLOG_WARN_RL(&rl, "bad 'subnet' %s", subnet_str);
-                free(error);
+            op = ovn_port_find(ports, nbsp->name);
+            if (!op || (op->nbsp && op->peer)) {
+                /* Do not allocate addresses for logical switch ports that
+                 * have a peer. */
                 continue;
             }
 
-            struct ovn_port *op;
-            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;
-                }
-
-                op = ovn_port_find(ports, nbsp->name);
-                if (!op || (op->nbsp && op->peer)) {
-                    /* Do not allocate addresses for logical switch ports that
-                     * have a peer. */
-                    continue;
-                }
-
-                for (size_t j = 0; j < nbsp->n_addresses; j++) {
-                    if (is_dynamic_lsp_address(nbsp->addresses[j])
-                        && !nbsp->dynamic_addresses) {
-                        if (!ipam_allocate_addresses(od, op,
-                                             nbsp->addresses[j], subnet, mask)
-                            || !extract_lsp_addresses(nbsp->dynamic_addresses,
-                                            &op->lsp_addrs[op->n_lsp_addrs])) {
-                            static struct vlog_rate_limit rl
-                                = VLOG_RATE_LIMIT_INIT(1, 1);
-                            VLOG_INFO_RL(&rl, "Failed to allocate address.");
-                        } else {
-                            op->n_lsp_addrs++;
-                        }
-                        break;
+            for (size_t j = 0; j < nbsp->n_addresses; j++) {
+                if (is_dynamic_lsp_address(nbsp->addresses[j])
+                    && !nbsp->dynamic_addresses) {
+                    if (!ipam_allocate_addresses(od, op, nbsp->addresses[j])
+                        || !extract_lsp_addresses(nbsp->dynamic_addresses,
+                                        &op->lsp_addrs[op->n_lsp_addrs])) {
+                        static struct vlog_rate_limit rl
+                            = VLOG_RATE_LIMIT_INIT(1, 1);
+                        VLOG_INFO_RL(&rl, "Failed to allocate address.");
+                    } else {
+                        op->n_lsp_addrs++;
                     }
+                    break;
                 }
             }
         }
diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
index c5ebbea..f759ee9 100644
--- a/ovn/ovn-nb.xml
+++ b/ovn/ovn-nb.xml
@@ -147,6 +147,28 @@ 
         column="addresses"/> column to request dynamic address assignment for a
         particular port.
       </column>
+
+      <column name="other_config" key="exclude_ips">
+        Set this to a list of IPv4 addresses from the subnet if defined in the
+        <ref table="Logical_Switch_Port"/> table's <ref table="Logical_Switch_Port"
+        column="other_config:subnet"/> column. The defined list of IPs will
+        be excluded from the dynamic address assignment. A range can also be
+        specified.
+        <p>
+          Examples:
+        </p>
+        <dl>
+          <dt><code>192.168.0.2 192.168.0.10</code></dt>
+          <dd>
+            Both the above IPs will be excluded from the dynamic address
+            assignment.
+          </dd>
+
+          <dt><code>192.168.0.4 192.168.0.30-192.168.0.60 192.168.0.110-192.168.0.120</code></dt>
+          <dd></dd>
+          <dt><code>192.168.0.110-192.168.0.120 192.168.0.25-192.168.0.30 192.168.0.144</code></dt>
+        </dl>
+      </column>
     </group>
 
     <group title="Common Columns">
@@ -1376,7 +1398,7 @@ 
         <column name="options" key="lease_time"
                 type='{"type": "integer", "minInteger": 0, "maxInteger": 4294967295}'>
           <p>
-            The offered lease time in seconds, 
+            The offered lease time in seconds,
           </p>
 
           <p>
diff --git a/tests/ovn.at b/tests/ovn.at
index c821574..0834f03 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -4912,6 +4912,50 @@  AT_CHECK([ovn-nbctl get Logical-Switch-Port p31 dynamic_addresses], [0],
      ["fe:dc:ba:98:76:56 192.168.1.18"
 ])
 
+
+# Test the exclude_ips from the IPAM list
+ovn-nbctl --wait=sb set logical_switch sw0 \
+other_config:exclude_ips="192.168.1.19,192.168.1.21,192.168.1.23-192.168.1.50"
+
+ovn-nbctl --wait=sb lsp-add sw0 p32 -- lsp-set-addresses p32 \
+"dynamic"
+# 192.168.1.20 should be assigned as 192.168.1.19 is excluded.
+AT_CHECK([ovn-nbctl get Logical-Switch-Port p32 dynamic_addresses], [0],
+     ["0a:00:00:00:00:21 192.168.1.20"
+])
+
+ovn-nbctl --wait=sb lsp-add sw0 p33 -- lsp-set-addresses p33 \
+"dynamic"
+# 192.168.1.22 should be assigned as 192.168.1.21 is excluded.
+AT_CHECK([ovn-nbctl get Logical-Switch-Port p33 dynamic_addresses], [0],
+     ["0a:00:00:00:00:22 192.168.1.22"
+])
+
+ovn-nbctl --wait=sb lsp-add sw0 p34 -- lsp-set-addresses p34 \
+"dynamic"
+# 192.168.1.51 should be assigned as 192.168.1.23-192.168.1.50 is excluded.
+AT_CHECK([ovn-nbctl get Logical-Switch-Port p34 dynamic_addresses], [0],
+     ["0a:00:00:00:00:23 192.168.1.51"
+])
+
+# Now clear the exclude_ips list. 192.168.1.19 should be assigned.
+ovn-nbctl --wait=sb set Logical-switch sw0 other_config:exclude_ips="invalid"
+ovn-nbctl --wait=sb lsp-add sw0 p35 -- lsp-set-addresses p35 \
+"dynamic"
+AT_CHECK([ovn-nbctl get Logical-Switch-Port p35 dynamic_addresses], [0],
+     ["0a:00:00:00:00:24 192.168.1.19"
+])
+
+# Set invalid data in exclude_ips list. It should be ignored.
+ovn-nbctl --wait=sb set Logical-switch sw0 other_config:exclude_ips="182.168.1.30"
+ovn-nbctl --wait=sb lsp-add sw0 p36 -- lsp-set-addresses p36 \
+"dynamic"
+# 192.168.1.21 should be assigned as that's the next free one.
+AT_CHECK([ovn-nbctl get Logical-Switch-Port p36 dynamic_addresses], [0],
+     ["0a:00:00:00:00:25 192.168.1.21"
+])
+
+
 as ovn-sb
 OVS_APP_EXIT_AND_WAIT([ovsdb-server])