[ovs-dev] [PATCH] OVN: introduce mac_prefix support to IPAM

Mark Michelson mmichels at redhat.com
Fri Oct 26 14:17:44 UTC 2018


Hi Lorenzo,

I think for the patch, I'm willing to ack it, so:

Acked-by: Mark Michelson <mmichels at redhat.com>

However,  I have some minor suggestions below that might be worth making 
a second version of the patch for.

On 10/23/2018 06:01 AM, Lorenzo Bianconi wrote:
> Add the possibility to specify a given mac address prefix for
> dynamically generated mac address. Mac address prefix can be
> specified in nbdb NB_Global table, options:mac_prefix=<mac_prefix>
> This patch fix a possible issue of L2 address duplication if
> multiple OVN deployments share a single broadcast domain
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi at redhat.com>
> ---
>   ovn/northd/ovn-northd.c | 76 ++++++++++++++++++++++++++++++++---------
>   ovn/ovn-nb.xml          |  5 +++
>   tests/ovn.at            | 17 +++++++++
>   3 files changed, 81 insertions(+), 17 deletions(-)
> 
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 439651f80..3e8a4a276 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -404,6 +404,9 @@ struct ipam_info {
>       unsigned long *allocated_ipv4s; /* A bitmap of allocated IPv4s */
>       bool ipv6_prefix_set;
>       struct in6_addr ipv6_prefix;
> +    bool mac_prefix_set;
> +    struct eth_addr mac_prefix;
> +
>   };
>   
>   /* The 'key' comes from nbs->header_.uuid or nbr->header_.uuid or
> @@ -534,7 +537,8 @@ lrouter_is_enabled(const struct nbrec_logical_router *lrouter)
>   }
>   
>   static void
> -init_ipam_info_for_datapath(struct ovn_datapath *od)
> +init_ipam_info_for_datapath(struct ovn_datapath *od,
> +                            struct northd_context *ctx)
>   {
>       if (!od->nbs) {
>           return;
> @@ -543,11 +547,27 @@ init_ipam_info_for_datapath(struct ovn_datapath *od)
>       const char *subnet_str = smap_get(&od->nbs->other_config, "subnet");
>       const char *ipv6_prefix = smap_get(&od->nbs->other_config, "ipv6_prefix");
>   
> +    const struct nbrec_nb_global *nb = nbrec_nb_global_first(ctx->ovnnb_idl);
> +    const char *mac_prefix = smap_get(&nb->options, "mac_prefix");
> +
>       if (ipv6_prefix) {
>           od->ipam_info.ipv6_prefix_set = ipv6_parse(
>               ipv6_prefix, &od->ipam_info.ipv6_prefix);
>       }
>   
> +    if (mac_prefix) {
> +        struct eth_addr addr;
> +
> +        memset(&addr, 0, sizeof addr);
> +        if (ovs_scan(mac_prefix, "%"SCNx8":%"SCNx8":%"SCNx8,
> +                     &addr.ea[0], &addr.ea[1], &addr.ea[2])) {
> +            od->ipam_info.mac_prefix_set = true;
> +            od->ipam_info.mac_prefix = addr > +        } else {
> +            od->ipam_info.mac_prefix_set = false;
> +        }
> +    }

This operation will be exactly the same for every datapath since the 
mac_prefix is a global setting. It seems wasteful to do the same thing 
on every datapath. Why not parse the configured mac_prefix once and 
store it as a global in ovn-northd?

> +
>       if (!subnet_str) {
>           return;
>       }
> @@ -703,7 +723,7 @@ join_datapaths(struct northd_context *ctx, struct hmap *datapaths,
>               ovs_list_push_back(nb_only, &od->list);
>           }
>   
> -        init_ipam_info_for_datapath(od);
> +        init_ipam_info_for_datapath(od, ctx);
>       }
>   
>       const struct nbrec_logical_router *nbr;
> @@ -915,17 +935,24 @@ ipam_is_duplicate_mac(struct eth_addr *ea, uint64_t mac64, bool warn)
>   }
>   
>   static void
> -ipam_insert_mac(struct eth_addr *ea, bool check)
> +ipam_insert_mac(struct ovn_datapath *od, struct eth_addr *ea, bool check)

I believe od can be made const.

>   {
>       if (!ea) {
>           return;
>       }
>   
>       uint64_t mac64 = eth_addr_to_uint64(*ea);
> +    uint64_t prefix;
> +
> +    if (od->ipam_info.mac_prefix_set) {
> +        prefix = eth_addr_to_uint64(od->ipam_info.mac_prefix);
> +    } else {
> +        prefix = MAC_ADDR_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 ^ MAC_ADDR_PREFIX) >> 24)
> +    if (((mac64 ^ prefix) >> 24)
>           || (check && ipam_is_duplicate_mac(ea, mac64, true))) {
>           return;
>       }
> @@ -970,7 +997,7 @@ ipam_insert_lsp_addresses(struct ovn_datapath *od, struct ovn_port *op,
>           VLOG_WARN_RL(&rl, "Extract addresses failed.");
>           return;
>       }
> -    ipam_insert_mac(&laddrs.ea, true);
> +    ipam_insert_mac(od, &laddrs.ea, true);
>   
>       /* IP is only added to IPAM if the switch's subnet option
>        * is set, whereas MAC is always added to MACAM. */
> @@ -1007,7 +1034,7 @@ ipam_add_port_addresses(struct ovn_datapath *od, struct ovn_port *op)
>               VLOG_WARN_RL(&rl, "Extract addresses failed.");
>               return;
>           }
> -        ipam_insert_mac(&lrp_networks.ea, true);
> +        ipam_insert_mac(od, &lrp_networks.ea, true);
>   
>           if (!op->peer || !op->peer->nbsp || !op->peer->od || !op->peer->od->nbs
>               || !smap_get(&op->peer->od->nbs->other_config, "subnet")) {
> @@ -1025,7 +1052,7 @@ ipam_add_port_addresses(struct ovn_datapath *od, struct ovn_port *op)
>   }
>   
>   static uint64_t
> -ipam_get_unused_mac(void)
> +ipam_get_unused_mac(struct ovn_datapath *od)

I believe od can be made const.

>   {
>       /* Stores the suffix of the most recently ipam-allocated MAC address. */
>       static uint32_t last_mac;
> @@ -1036,7 +1063,12 @@ ipam_get_unused_mac(void)
>       for (i = 0; i < MAC_ADDR_SPACE - 1; i++) {
>           /* The tentative MAC's suffix will be in the interval (1, 0xfffffe). */
>           mac_addr_suffix = ((last_mac + i) % (MAC_ADDR_SPACE - 1)) + 1;
> -        mac64 = MAC_ADDR_PREFIX | mac_addr_suffix;
> +        if (od->ipam_info.mac_prefix_set) {
> +            mac64 =  eth_addr_to_uint64(od->ipam_info.mac_prefix) |
> +                     mac_addr_suffix;
> +        } else {
> +            mac64 = MAC_ADDR_PREFIX | mac_addr_suffix;
> +        }
>           eth_addr_from_uint64(mac64, &mac);
>           if (!ipam_is_duplicate_mac(&mac, mac64, false)) {
>               last_mac = mac_addr_suffix;
> @@ -1091,7 +1123,7 @@ struct dynamic_address_update {
>   };
>   
>   static enum dynamic_update_type
> -dynamic_mac_changed(const char *lsp_addresses,
> +dynamic_mac_changed(struct ovn_datapath *od, const char *lsp_addresses,
>                       struct dynamic_address_update *update)

I believe od can be made const.

>   {
>      struct eth_addr ea;
> @@ -1107,7 +1139,15 @@ dynamic_mac_changed(const char *lsp_addresses,
>      }
>   
>      uint64_t mac64 = eth_addr_to_uint64(update->current_addresses.ea);
> -   if ((mac64 ^ MAC_ADDR_PREFIX) >> 24) {
> +   uint64_t prefix;
> +
> +   if (od->ipam_info.mac_prefix_set) {
> +       prefix = eth_addr_to_uint64(od->ipam_info.mac_prefix);
> +   } else {
> +       prefix = MAC_ADDR_PREFIX;
> +   }
> +
> +   if ((mac64 ^ prefix) >> 24) {
>          return DYNAMIC;
>      } else {
>          return NONE;
> @@ -1195,10 +1235,11 @@ dynamic_ip6_changed(struct dynamic_address_update *update)
>    * Returns true if any changes to dynamic addresses are required
>    */
>   static bool
> -dynamic_addresses_check_for_updates(const char *lsp_addrs,
> +dynamic_addresses_check_for_updates(struct ovn_datapath *od,
> +                                    const char *lsp_addrs,
>                                       struct dynamic_address_update *update)
>   {
> -    update->mac = dynamic_mac_changed(lsp_addrs, update);
> +    update->mac = dynamic_mac_changed(od, lsp_addrs, update);
>       update->ipv4 = dynamic_ip4_changed(update);
>       update->ipv6 = dynamic_ip6_changed(update);
>       if (update->mac == NONE &&
> @@ -1215,10 +1256,11 @@ dynamic_addresses_check_for_updates(const char *lsp_addrs,
>    * elsewhere later.
>    */
>   static void
> -update_unchanged_dynamic_addresses(struct dynamic_address_update *update)
> +update_unchanged_dynamic_addresses(struct ovn_datapath *od,
> +                                   struct dynamic_address_update *update)
>   {
>       if (update->mac == NONE) {
> -        ipam_insert_mac(&update->current_addresses.ea, false);
> +        ipam_insert_mac(od, &update->current_addresses.ea, false);
>       }
>       if (update->ipv4 == NONE && update->current_addresses.n_ipv4_addrs) {
>           ipam_insert_ip(update->op->od,
> @@ -1278,7 +1320,7 @@ update_dynamic_addresses(struct ovn_datapath *od,
>           mac = update->static_mac;
>           break;
>       case DYNAMIC:
> -        eth_addr_from_uint64(ipam_get_unused_mac(), &mac);
> +        eth_addr_from_uint64(ipam_get_unused_mac(od), &mac);
>           break;
>       }
>   
> @@ -1390,8 +1432,8 @@ build_ipam(struct hmap *datapaths, struct hmap *ports)
>                       extract_lsp_addresses(nbsp->dynamic_addresses,
>                                             &update->current_addresses);
>                       any_changed = dynamic_addresses_check_for_updates(
> -                        nbsp->addresses[j], update);
> -                    update_unchanged_dynamic_addresses(update);
> +                        od, nbsp->addresses[j], update);
> +                    update_unchanged_dynamic_addresses(od, update);
>                       if (any_changed) {
>                           ovs_list_push_back(&updates, &update->node);
>                       } else {
> diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
> index c0739fe57..f309b3b86 100644
> --- a/ovn/ovn-nb.xml
> +++ b/ovn/ovn-nb.xml
> @@ -102,6 +102,11 @@
>             tunnel interfaces.
>           </column>
>         </group>
> +
> +      <column name="options" key="mac_prefix">
> +        Configure a given OUI to be used as prefix when L2 address is
> +        dynamically assigned, e.g. <code>00:11:22</code>
> +      </column>
>       </group>
>   
>       <group title="Connection Options">
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 8825beca3..e512f94aa 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -5616,6 +5616,23 @@ AT_CHECK([ovn-nbctl get Logical-Switch-Port p41 dynamic_addresses], [0],
>            ["f0:00:00:00:10:2b 192.168.1.3"
>   ])
>   
> +# define a mac address prefix
> +ovn-nbctl ls-add sw6
> +ovn-nbctl --wait=hv set NB_Global . options:mac_prefix="00:11:22:33:44:55"
> +ovn-nbctl --wait=sb set Logical-Switch sw6 other_config:subnet=192.168.100.0/24
> +for n in $(seq 1 3); do
> +    ovn-nbctl --wait=sb lsp-add sw6 "p5$n" -- lsp-set-addresses "p5$n" dynamic
> +done
> +AT_CHECK([ovn-nbctl get Logical-Switch-Port p51 dynamic_addresses], [0],
> +    ["00:11:22:00:00:4d 192.168.100.2"
> +])
> +AT_CHECK([ovn-nbctl get Logical-Switch-Port p52 dynamic_addresses], [0],
> +    ["00:11:22:00:00:4e 192.168.100.3"
> +])
> +AT_CHECK([ovn-nbctl get Logical-Switch-Port p53 dynamic_addresses], [0],
> +    ["00:11:22:00:00:4f 192.168.100.4"
> +])
> +
>   as ovn-sb
>   OVS_APP_EXIT_AND_WAIT([ovsdb-server])
>   
> 



More information about the dev mailing list