diff mbox series

[ovs-dev,RFC,v3,2/3] northd: refactor and split some IPAM functions

Message ID 20201102142828.3444732-3-mmichels@redhat.com
State New
Headers show
Series Unit Testing in OVN | expand

Commit Message

Mark Michelson Nov. 2, 2020, 2:28 p.m. UTC
This refactor focuses on three efforts:
* Break a large function into smaller functions
* Pass more specific data types to functions
* Isolate these functions in their own code unit

Smaller functions have clearer purposes, have fewer chances of unwanted
side effects, and are easier to test. The next commit in this series
will add some unit tests that exercise the new functions created in
this commit.

Note that this is not a full refactor of IPAM. Dynamic address
assignment is complicated and currently tightly coupled with datapath
and port constructs. A refactor of that section of IPAM is difficult
enough that it should have its own patch series separate from this one
that focuses mostly on unit tests.

Signed-off-by: Mark Michelson <mmichels@redhat.com>
---
 northd/automake.mk  |   5 +-
 northd/ipam.c       | 328 ++++++++++++++++++++++++++++++++++++++++++++
 northd/ipam.h       |  37 +++++
 northd/ovn-northd.c | 285 +++-----------------------------------
 4 files changed, 389 insertions(+), 266 deletions(-)
 create mode 100644 northd/ipam.c
 create mode 100644 northd/ipam.h
diff mbox series

Patch

diff --git a/northd/automake.mk b/northd/automake.mk
index 69657e77e..3340178f5 100644
--- a/northd/automake.mk
+++ b/northd/automake.mk
@@ -1,6 +1,9 @@ 
 # ovn-northd
 bin_PROGRAMS += northd/ovn-northd
-northd_ovn_northd_SOURCES = northd/ovn-northd.c
+northd_ovn_northd_SOURCES = \
+	northd/ovn-northd.c \
+	northd/ipam.c \
+	northd/ipam.h
 northd_ovn_northd_LDADD = \
 	lib/libovn.la \
 	$(OVSDB_LIBDIR)/libovsdb.la \
diff --git a/northd/ipam.c b/northd/ipam.c
new file mode 100644
index 000000000..e5383c46f
--- /dev/null
+++ b/northd/ipam.c
@@ -0,0 +1,328 @@ 
+#include <config.h>
+
+#include <stddef.h>
+#include <stdlib.h>
+#include <string.h>
+#include <netinet/in.h>
+
+#include "ipam.h"
+#include "lib/unit-test.h"
+#include "ovn/lex.h"
+
+#include "smap.h"
+#include "packets.h"
+#include "bitmap.h"
+#include "openvswitch/vlog.h"
+
+VLOG_DEFINE_THIS_MODULE(ipam)
+
+static void
+init_ipam_ipv6_prefix(const char *ipv6_prefix, struct ipam_info *info)
+{
+    info->ipv6_prefix_set = false;
+    info->ipv6_prefix = in6addr_any;
+
+    if (!ipv6_prefix) {
+        return;
+    }
+
+    /* XXX Since we only accept /64 addresses, why do we even bother
+     * with accepting and trying to analyze a user-provided mask?
+     */
+    if (strchr(ipv6_prefix, '/')) {
+        /* If a prefix length was specified, it must be 64. */
+        struct in6_addr mask;
+        char *error
+            = ipv6_parse_masked(ipv6_prefix,
+                                &info->ipv6_prefix, &mask);
+        if (error) {
+            static struct vlog_rate_limit rl
+                = VLOG_RATE_LIMIT_INIT(5, 1);
+            VLOG_WARN_RL(&rl, "bad 'ipv6_prefix' %s: %s",
+                         ipv6_prefix, error);
+            free(error);
+        } else {
+            if (ipv6_count_cidr_bits(&mask) == 64) {
+                info->ipv6_prefix_set = true;
+            } else {
+                static struct vlog_rate_limit rl
+                    = VLOG_RATE_LIMIT_INIT(5, 1);
+                VLOG_WARN_RL(&rl, "bad 'ipv6_prefix' %s: must be /64",
+                             ipv6_prefix);
+            }
+        }
+    } else {
+        info->ipv6_prefix_set = ipv6_parse(
+            ipv6_prefix, &info->ipv6_prefix);
+        if (!info->ipv6_prefix_set) {
+            static struct vlog_rate_limit rl
+                = VLOG_RATE_LIMIT_INIT(5, 1);
+            VLOG_WARN_RL(&rl, "bad 'ipv6_prefix' %s", ipv6_prefix);
+        }
+    }
+
+    if (info->ipv6_prefix_set) {
+        /* Make sure nothing past first 64 bits are set */
+        struct in6_addr mask = ipv6_create_mask(64);
+        info->ipv6_prefix = ipv6_addr_bitand(&info->ipv6_prefix, &mask);
+    }
+}
+
+static void
+init_ipam_ipv4(const char *subnet_str, const char *exclude_ip_list,
+               struct ipam_info *info)
+{
+    info->start_ipv4 = 0;
+    info->total_ipv4s = 0;
+    info->allocated_ipv4s = NULL;
+
+    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;
+    }
+
+    info->start_ipv4 = ntohl(subnet & mask) + 1;
+    info->total_ipv4s = ~ntohl(mask);
+    info->allocated_ipv4s =
+        bitmap_allocate(info->total_ipv4s);
+
+    /* Mark first IP as taken */
+    bitmap_set1(info->allocated_ipv4s, 0);
+
+    if (!exclude_ip_list) {
+        return;
+    }
+
+    struct lexer lexer;
+    lexer_init(&lexer, exclude_ip_list);
+    /* 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".
+    */
+    lexer_get(&lexer);
+    while (lexer.token.type != LEX_T_END) {
+        if (lexer.token.type != LEX_T_INTEGER) {
+            lexer_syntax_error(&lexer, "expecting address");
+            break;
+        }
+        uint32_t start = ntohl(lexer.token.value.ipv4);
+        lexer_get(&lexer);
+
+        uint32_t end = start + 1;
+        if (lexer_match(&lexer, LEX_T_ELLIPSIS)) {
+            if (lexer.token.type != LEX_T_INTEGER) {
+                lexer_syntax_error(&lexer, "expecting address range");
+                break;
+            }
+            end = ntohl(lexer.token.value.ipv4) + 1;
+            lexer_get(&lexer);
+        }
+
+        /* Clamp start...end to fit the subnet. */
+        start = MAX(info->start_ipv4, start);
+        end = MIN(info->start_ipv4 + info->total_ipv4s, end);
+        if (end > start) {
+            bitmap_set_multiple(info->allocated_ipv4s,
+                                start - info->start_ipv4,
+                                end - start, 1);
+        } else {
+            lexer_error(&lexer, "excluded addresses not in subnet");
+        }
+    }
+    if (lexer.error) {
+        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
+        /*
+         * VLOG_WARN_RL(&rl, "logical switch "UUID_FMT": bad exclude_ips (%s)",
+         *              UUID_ARGS(&od->key), lexer.error);
+         */
+        VLOG_WARN_RL(&rl, "logical switch: bad exclude_ips (%s)", lexer.error);
+    }
+    lexer_destroy(&lexer);
+}
+
+void
+init_ipam_info(struct ipam_info *info, const struct smap *config)
+{
+    const char *subnet_str = smap_get(config, "subnet");
+    const char *ipv6_prefix = smap_get(config, "ipv6_prefix");
+    const char *exclude_ips = smap_get(config, "exclude_ips");
+
+    init_ipam_ipv6_prefix(ipv6_prefix, info);
+
+    if (!subnet_str) {
+        if (!ipv6_prefix) {
+            info->mac_only = smap_get_bool(config, "mac_only", false);
+        }
+        return;
+    }
+
+    init_ipam_ipv4(subnet_str, exclude_ips, info);
+}
+
+void
+destroy_ipam_info(struct ipam_info *info)
+{
+    bitmap_free(info->allocated_ipv4s);
+}
+
+bool
+ipam_insert_ip(struct ipam_info *info, uint32_t ip)
+{
+    if (!info->allocated_ipv4s) {
+        return true;
+    }
+
+    if (ip >= info->start_ipv4 &&
+        ip < (info->start_ipv4 + info->total_ipv4s)) {
+        if (bitmap_is_set(info->allocated_ipv4s,
+                          ip - info->start_ipv4)) {
+            return false;
+        }
+        bitmap_set1(info->allocated_ipv4s,
+                    ip - info->start_ipv4);
+    }
+    return true;
+}
+
+uint32_t
+ipam_get_unused_ip(struct ipam_info *info)
+{
+    if (!info->allocated_ipv4s) {
+        return 0;
+    }
+
+    size_t new_ip_index = bitmap_scan(info->allocated_ipv4s, 0, 0,
+                                      info->total_ipv4s - 1);
+    if (new_ip_index == 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 info->start_ipv4 + new_ip_index;
+}
+
+/* MAC address management (macam) table of "struct eth_addr"s, that holds the
+ * MAC addresses allocated by the OVN ipam module. */
+static struct hmap macam = HMAP_INITIALIZER(&macam);
+
+struct macam_node {
+    struct hmap_node hmap_node;
+    struct eth_addr mac_addr; /* Allocated MAC address. */
+};
+
+#define MAC_ADDR_SPACE 0xffffff
+static struct eth_addr mac_prefix;
+static char mac_prefix_str[18];
+
+void
+cleanup_macam(void)
+{
+    struct macam_node *node;
+    HMAP_FOR_EACH_POP (node, hmap_node, &macam) {
+        free(node);
+    }
+}
+
+const char *
+set_mac_prefix(const char *prefix)
+{
+    mac_prefix = eth_addr_zero;
+    if (prefix) {
+        struct eth_addr addr;
+
+        memset(&addr, 0, sizeof addr);
+        if (ovs_scan(prefix, "%"SCNx8":%"SCNx8":%"SCNx8,
+                     &addr.ea[0], &addr.ea[1], &addr.ea[2])) {
+            mac_prefix = addr;
+        }
+    }
+
+    if (eth_addr_equals(mac_prefix, eth_addr_zero)) {
+        eth_addr_random(&mac_prefix);
+        memset(&mac_prefix.ea[3], 0, 3);
+    }
+
+    snprintf(mac_prefix_str, sizeof(mac_prefix_str),
+             "%02"PRIx8":%02"PRIx8":%02"PRIx8,
+             mac_prefix.ea[0], mac_prefix.ea[1], mac_prefix.ea[2]);
+
+    return mac_prefix_str;
+}
+
+struct eth_addr
+get_mac_prefix(void)
+{
+    return mac_prefix;
+}
+
+static bool
+ipam_is_duplicate_mac(struct eth_addr *ea, uint64_t mac64, bool warn)
+{
+    struct macam_node *macam_node;
+    HMAP_FOR_EACH_WITH_HASH (macam_node, hmap_node, hash_uint64(mac64),
+                             &macam) {
+        if (eth_addr_equals(*ea, macam_node->mac_addr)) {
+            if (warn) {
+                static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
+                VLOG_WARN_RL(&rl, "Duplicate MAC set: "ETH_ADDR_FMT,
+                             ETH_ADDR_ARGS(macam_node->mac_addr));
+            }
+            return true;
+        }
+    }
+    return false;
+}
+
+void
+ipam_insert_mac(struct eth_addr *ea, bool check)
+{
+    if (!ea) {
+        return;
+    }
+
+    uint64_t mac64 = eth_addr_to_uint64(*ea);
+    uint64_t prefix = eth_addr_to_uint64(mac_prefix);
+
+    /* If the new MAC was not assigned by this address management system or
+     * check is true and the new MAC is a duplicate, do not insert it into the
+     * macam hmap. */
+    if (((mac64 ^ prefix) >> 24)
+        || (check && ipam_is_duplicate_mac(ea, mac64, true))) {
+        return;
+    }
+
+    struct macam_node *new_macam_node = xmalloc(sizeof *new_macam_node);
+    new_macam_node->mac_addr = *ea;
+    hmap_insert(&macam, &new_macam_node->hmap_node, hash_uint64(mac64));
+}
+
+uint64_t
+ipam_get_unused_mac(ovs_be32 ip)
+{
+    uint32_t mac_addr_suffix, i, base_addr = ntohl(ip) & MAC_ADDR_SPACE;
+    struct eth_addr mac;
+    uint64_t mac64;
+
+    for (i = 0; i < MAC_ADDR_SPACE - 1; i++) {
+        /* The tentative MAC's suffix will be in the interval (1, 0xfffffe). */
+        mac_addr_suffix = ((base_addr + i) % (MAC_ADDR_SPACE - 1)) + 1;
+        mac64 =  eth_addr_to_uint64(mac_prefix) | mac_addr_suffix;
+        eth_addr_from_uint64(mac64, &mac);
+        if (!ipam_is_duplicate_mac(&mac, mac64, false)) {
+            break;
+        }
+    }
+
+    if (i == MAC_ADDR_SPACE) {
+        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
+        VLOG_WARN_RL(&rl, "MAC address space exhausted.");
+        mac64 = 0;
+    }
+
+    return mac64;
+}
diff --git a/northd/ipam.h b/northd/ipam.h
new file mode 100644
index 000000000..5244cce8f
--- /dev/null
+++ b/northd/ipam.h
@@ -0,0 +1,37 @@ 
+#ifndef NORTHD_IPAM_H
+#define NORTHD_IPAM_H 1
+
+#include <stdint.h>
+#include <stdbool.h>
+
+#include "openvswitch/types.h"
+
+struct ipam_info {
+    uint32_t start_ipv4;
+    size_t total_ipv4s;
+    unsigned long *allocated_ipv4s; /* A bitmap of allocated IPv4s */
+    bool ipv6_prefix_set;
+    struct in6_addr ipv6_prefix;
+    bool mac_only;
+};
+
+struct smap;
+void init_ipam_info(struct ipam_info *info, const struct smap *config);
+
+void destroy_ipam_info(struct ipam_info *info);
+
+bool ipam_insert_ip(struct ipam_info *info, uint32_t ip);
+
+uint32_t ipam_get_unused_ip(struct ipam_info *info);
+
+void ipam_insert_mac(struct eth_addr *ea, bool check);
+
+uint64_t ipam_get_unused_mac(ovs_be32 ip);
+
+void cleanup_macam(void);
+
+struct eth_addr get_mac_prefix(void);
+
+const char *set_mac_prefix(const char *hint);
+
+#endif /* NORTHD_IPAM_H */
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 8800a3d3c..0fb7d0969 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -22,6 +22,7 @@ 
 #include "command-line.h"
 #include "daemon.h"
 #include "dirs.h"
+#include "ipam.h"
 #include "openvswitch/dynamic-string.h"
 #include "fatal-signal.h"
 #include "hash.h"
@@ -79,13 +80,6 @@  static const char *ovnnb_db;
 static const char *ovnsb_db;
 static const char *unixctl_path;
 
-#define MAC_ADDR_SPACE 0xffffff
-
-/* MAC address management (macam) table of "struct eth_addr"s, that holds the
- * MAC addresses allocated by the OVN ipam module. */
-static struct hmap macam = HMAP_INITIALIZER(&macam);
-static struct eth_addr mac_prefix;
-
 static bool controller_event_en;
 
 static bool check_lsp_is_up;
@@ -478,15 +472,6 @@  port_has_qos_params(const struct smap *opts)
 }
 
 
-struct ipam_info {
-    uint32_t start_ipv4;
-    size_t total_ipv4s;
-    unsigned long *allocated_ipv4s; /* A bitmap of allocated IPv4s */
-    bool ipv6_prefix_set;
-    struct in6_addr ipv6_prefix;
-    bool mac_only;
-};
-
 /*
  * Multicast snooping and querier per datapath configuration.
  */
@@ -792,20 +777,6 @@  struct lrouter_group {
     struct sset ha_chassis_groups;
 };
 
-struct macam_node {
-    struct hmap_node hmap_node;
-    struct eth_addr mac_addr; /* Allocated MAC address. */
-};
-
-static void
-cleanup_macam(struct hmap *macam_)
-{
-    struct macam_node *node;
-    HMAP_FOR_EACH_POP (node, hmap_node, macam_) {
-        free(node);
-    }
-}
-
 static struct ovn_datapath *
 ovn_datapath_create(struct hmap *datapaths, const struct uuid *key,
                     const struct nbrec_logical_switch *nbs,
@@ -837,7 +808,7 @@  ovn_datapath_destroy(struct hmap *datapaths, struct ovn_datapath *od)
          * use it. */
         hmap_remove(datapaths, &od->key_node);
         ovn_destroy_tnlids(&od->port_tnlids);
-        bitmap_free(od->ipam_info.allocated_ipv4s);
+        destroy_ipam_info(&od->ipam_info);
         free(od->router_ports);
         destroy_nat_entries(od);
         free(od->nat_entries);
@@ -901,117 +872,7 @@  init_ipam_info_for_datapath(struct ovn_datapath *od)
         return;
     }
 
-    const char *subnet_str = smap_get(&od->nbs->other_config, "subnet");
-    const char *ipv6_prefix = smap_get(&od->nbs->other_config, "ipv6_prefix");
-
-    if (ipv6_prefix) {
-        if (strstr(ipv6_prefix, "/")) {
-            /* If a prefix length was specified, it must be 64. */
-            struct in6_addr mask;
-            char *error
-                = ipv6_parse_masked(ipv6_prefix,
-                                    &od->ipam_info.ipv6_prefix, &mask);
-            if (error) {
-                static struct vlog_rate_limit rl
-                    = VLOG_RATE_LIMIT_INIT(5, 1);
-                VLOG_WARN_RL(&rl, "bad 'ipv6_prefix' %s: %s",
-                             ipv6_prefix, error);
-                free(error);
-            } else {
-                if (ipv6_count_cidr_bits(&mask) == 64) {
-                    od->ipam_info.ipv6_prefix_set = true;
-                } else {
-                    static struct vlog_rate_limit rl
-                        = VLOG_RATE_LIMIT_INIT(5, 1);
-                    VLOG_WARN_RL(&rl, "bad 'ipv6_prefix' %s: must be /64",
-                                 ipv6_prefix);
-                }
-            }
-        } else {
-            od->ipam_info.ipv6_prefix_set = ipv6_parse(
-                ipv6_prefix, &od->ipam_info.ipv6_prefix);
-            if (!od->ipam_info.ipv6_prefix_set) {
-                static struct vlog_rate_limit rl
-                    = VLOG_RATE_LIMIT_INIT(5, 1);
-                VLOG_WARN_RL(&rl, "bad 'ipv6_prefix' %s", ipv6_prefix);
-            }
-        }
-    }
-
-    if (!subnet_str) {
-        if (!ipv6_prefix) {
-            od->ipam_info.mac_only = smap_get_bool(&od->nbs->other_config,
-                                                   "mac_only", false);
-        }
-        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.start_ipv4 = ntohl(subnet & mask) + 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;
-    }
-
-    struct lexer lexer;
-    lexer_init(&lexer, exclude_ip_list);
-    /* 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".
-    */
-    lexer_get(&lexer);
-    while (lexer.token.type != LEX_T_END) {
-        if (lexer.token.type != LEX_T_INTEGER) {
-            lexer_syntax_error(&lexer, "expecting address");
-            break;
-        }
-        uint32_t start = ntohl(lexer.token.value.ipv4);
-        lexer_get(&lexer);
-
-        uint32_t end = start + 1;
-        if (lexer_match(&lexer, LEX_T_ELLIPSIS)) {
-            if (lexer.token.type != LEX_T_INTEGER) {
-                lexer_syntax_error(&lexer, "expecting address range");
-                break;
-            }
-            end = ntohl(lexer.token.value.ipv4) + 1;
-            lexer_get(&lexer);
-        }
-
-        /* Clamp start...end to fit the subnet. */
-        start = MAX(od->ipam_info.start_ipv4, start);
-        end = MIN(od->ipam_info.start_ipv4 + od->ipam_info.total_ipv4s, end);
-        if (end > start) {
-            bitmap_set_multiple(od->ipam_info.allocated_ipv4s,
-                                start - od->ipam_info.start_ipv4,
-                                end - start, 1);
-        } else {
-            lexer_error(&lexer, "excluded addresses not in subnet");
-        }
-    }
-    if (lexer.error) {
-        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
-        VLOG_WARN_RL(&rl, "logical switch "UUID_FMT": bad exclude_ips (%s)",
-                     UUID_ARGS(&od->key), lexer.error);
-    }
-    lexer_destroy(&lexer);
+    init_ipam_info(&od->ipam_info, &od->nbs->other_config);
 }
 
 static void
@@ -1564,64 +1425,17 @@  lrport_is_enabled(const struct nbrec_logical_router_port *lrport)
     return !lrport->enabled || *lrport->enabled;
 }
 
-static bool
-ipam_is_duplicate_mac(struct eth_addr *ea, uint64_t mac64, bool warn)
-{
-    struct macam_node *macam_node;
-    HMAP_FOR_EACH_WITH_HASH (macam_node, hmap_node, hash_uint64(mac64),
-                             &macam) {
-        if (eth_addr_equals(*ea, macam_node->mac_addr)) {
-            if (warn) {
-                static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
-                VLOG_WARN_RL(&rl, "Duplicate MAC set: "ETH_ADDR_FMT,
-                             ETH_ADDR_ARGS(macam_node->mac_addr));
-            }
-            return true;
-        }
-    }
-    return false;
-}
-
 static void
-ipam_insert_mac(struct eth_addr *ea, bool check)
+ipam_insert_ip_for_datapath(struct ovn_datapath *od, uint32_t ip)
 {
-    if (!ea) {
-        return;
-    }
-
-    uint64_t mac64 = eth_addr_to_uint64(*ea);
-    uint64_t prefix = eth_addr_to_uint64(mac_prefix);
-
-    /* If the new MAC was not assigned by this address management system or
-     * check is true and the new MAC is a duplicate, do not insert it into the
-     * macam hmap. */
-    if (((mac64 ^ prefix) >> 24)
-        || (check && ipam_is_duplicate_mac(ea, mac64, true))) {
+    if (!od) {
         return;
     }
 
-    struct macam_node *new_macam_node = xmalloc(sizeof *new_macam_node);
-    new_macam_node->mac_addr = *ea;
-    hmap_insert(&macam, &new_macam_node->hmap_node, hash_uint64(mac64));
-}
-
-static void
-ipam_insert_ip(struct ovn_datapath *od, uint32_t ip)
-{
-    if (!od || !od->ipam_info.allocated_ipv4s) {
-        return;
-    }
-
-    if (ip >= od->ipam_info.start_ipv4 &&
-        ip < (od->ipam_info.start_ipv4 + od->ipam_info.total_ipv4s)) {
-        if (bitmap_is_set(od->ipam_info.allocated_ipv4s,
-                          ip - od->ipam_info.start_ipv4)) {
-            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
-            VLOG_WARN_RL(&rl, "Duplicate IP set on switch %s: "IP_FMT,
-                         od->nbs->name, IP_ARGS(htonl(ip)));
-        }
-        bitmap_set1(od->ipam_info.allocated_ipv4s,
-                    ip - od->ipam_info.start_ipv4);
+    if (!ipam_insert_ip(&od->ipam_info, ip)) {
+        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
+        VLOG_WARN_RL(&rl, "Duplicate IP set on switch %s: "IP_FMT,
+                     od->nbs->name, IP_ARGS(htonl(ip)));
     }
 }
 
@@ -1651,7 +1465,7 @@  ipam_insert_lsp_addresses(struct ovn_datapath *od, struct ovn_port *op,
 
     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);
+        ipam_insert_ip_for_datapath(od, ip);
     }
 
     destroy_lport_addresses(&laddrs);
@@ -1693,7 +1507,7 @@  ipam_add_port_addresses(struct ovn_datapath *od, struct ovn_port *op)
              * about a duplicate IP address.
              */
             if (ip != op->peer->od->ipam_info.start_ipv4) {
-                ipam_insert_ip(op->peer->od, ip);
+                ipam_insert_ip_for_datapath(op->peer->od, ip);
             }
         }
 
@@ -1701,50 +1515,6 @@  ipam_add_port_addresses(struct ovn_datapath *od, struct ovn_port *op)
     }
 }
 
-static uint64_t
-ipam_get_unused_mac(ovs_be32 ip)
-{
-    uint32_t mac_addr_suffix, i, base_addr = ntohl(ip) & MAC_ADDR_SPACE;
-    struct eth_addr mac;
-    uint64_t mac64;
-
-    for (i = 0; i < MAC_ADDR_SPACE - 1; i++) {
-        /* The tentative MAC's suffix will be in the interval (1, 0xfffffe). */
-        mac_addr_suffix = ((base_addr + i) % (MAC_ADDR_SPACE - 1)) + 1;
-        mac64 =  eth_addr_to_uint64(mac_prefix) | mac_addr_suffix;
-        eth_addr_from_uint64(mac64, &mac);
-        if (!ipam_is_duplicate_mac(&mac, mac64, false)) {
-            break;
-        }
-    }
-
-    if (i == MAC_ADDR_SPACE) {
-        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
-        VLOG_WARN_RL(&rl, "MAC address space exhausted.");
-        mac64 = 0;
-    }
-
-    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 */
@@ -1784,7 +1554,7 @@  dynamic_mac_changed(const char *lsp_addresses,
    }
 
    uint64_t mac64 = eth_addr_to_uint64(update->current_addresses.ea);
-   uint64_t prefix = eth_addr_to_uint64(mac_prefix);
+   uint64_t prefix = eth_addr_to_uint64(get_mac_prefix());
 
    if ((mac64 ^ prefix) >> 24) {
        return DYNAMIC;
@@ -1946,7 +1716,7 @@  update_unchanged_dynamic_addresses(struct dynamic_address_update *update)
         ipam_insert_mac(&update->current_addresses.ea, false);
     }
     if (update->ipv4 == NONE && update->current_addresses.n_ipv4_addrs) {
-        ipam_insert_ip(update->op->od,
+        ipam_insert_ip_for_datapath(update->op->od,
                        ntohl(update->current_addresses.ipv4_addrs[0].addr));
     }
 }
@@ -2026,7 +1796,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->od->ipam_info));
         VLOG_INFO("Assigned dynamic IPv4 address '"IP_FMT"' to port '%s'",
                   IP_ARGS(ip4), update->op->nbsp->name);
     }
@@ -2075,7 +1845,7 @@  update_dynamic_addresses(struct dynamic_address_update *update)
     ipam_insert_mac(&mac, true);
 
     if (ip4) {
-        ipam_insert_ip(update->od, ntohl(ip4));
+        ipam_insert_ip_for_datapath(update->od, ntohl(ip4));
         ds_put_format(&new_addr, " "IP_FMT, IP_ARGS(ip4));
     }
     if (!IN6_ARE_ADDR_EQUAL(&ip6, &in6addr_any)) {
@@ -12197,16 +11967,8 @@  ovnnb_db_run(struct northd_context *ctx,
     sbrec_sb_global_set_options(sb, &nb->options);
     sb_loop->next_cfg = nb->nb_cfg;
 
-    const char *mac_addr_prefix = smap_get(&nb->options, "mac_prefix");
-    if (mac_addr_prefix) {
-        struct eth_addr addr;
-
-        memset(&addr, 0, sizeof addr);
-        if (ovs_scan(mac_addr_prefix, "%"SCNx8":%"SCNx8":%"SCNx8,
-                     &addr.ea[0], &addr.ea[1], &addr.ea[2])) {
-            mac_prefix = addr;
-        }
-    }
+    const char *mac_addr_prefix = set_mac_prefix(smap_get(&nb->options,
+                                                          "mac_prefix"));
 
     const char *monitor_mac = smap_get(&nb->options, "svc_monitor_mac");
     if (monitor_mac) {
@@ -12221,15 +11983,7 @@  ovnnb_db_run(struct northd_context *ctx,
     struct smap options;
     smap_clone(&options, &nb->options);
 
-    if (!mac_addr_prefix) {
-        eth_addr_random(&mac_prefix);
-        memset(&mac_prefix.ea[3], 0, 3);
-
-        smap_add_format(&options, "mac_prefix",
-                        "%02"PRIx8":%02"PRIx8":%02"PRIx8,
-                        mac_prefix.ea[0], mac_prefix.ea[1],
-                        mac_prefix.ea[2]);
-    }
+    smap_add(&options, "mac_prefix", mac_addr_prefix);
 
     if (!monitor_mac) {
         eth_addr_random(&svc_monitor_mac_ea);
@@ -12296,7 +12050,8 @@  ovnnb_db_run(struct northd_context *ctx,
     }
     shash_destroy(&meter_groups);
 
-    cleanup_macam(&macam);
+    /* XXX This is awkward */
+    cleanup_macam();
 }
 
 /* Stores the list of chassis which references an ha_chassis_group.