[ovs-dev] OVN: introduce mac_prefix support to IPAM
diff mbox series

Message ID f6c5b74e2a086014328ef9792e7d8778a64ea048.1540287483.git.lorenzo.bianconi@redhat.com
State Superseded
Headers show
Series
  • [ovs-dev] OVN: introduce mac_prefix support to IPAM
Related show

Commit Message

Lorenzo Bianconi Oct. 23, 2018, 10:01 a.m. UTC
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@redhat.com>
---
 ovn/northd/ovn-northd.c | 76 ++++++++++++++++++++++++++++++++---------
 ovn/ovn-nb.xml          |  5 +++
 tests/ovn.at            | 17 +++++++++
 3 files changed, 81 insertions(+), 17 deletions(-)

Comments

Aaron Conole Oct. 23, 2018, 12:27 p.m. UTC | #1
Hi Lorenzo,

Lorenzo Bianconi <lorenzo.bianconi@redhat.com> writes:

> 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@redhat.com>
> ---

Does it make sense for the following failure to be associated with this
patch:

2697: ovn -- ensure one gw controller restart in HA doesn't bounce the master FAILED (ovn.at:9568)

I'm about to enable reporting 'make distcheck' errors in the bot, and
don't want it to be spamming with a known issue.  Bala is currently
working on factoring out upstream errors, so that will have to come
later.

>  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;
> +        }
> +    }
> +
>      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)
>  {
>      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)
>  {
>      /* 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)
>  {
>     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])
Aaron Conole Oct. 23, 2018, 1:10 p.m. UTC | #2
Aaron Conole <aconole@redhat.com> writes:

> Hi Lorenzo,
>
> Lorenzo Bianconi <lorenzo.bianconi@redhat.com> writes:
>
>> 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@redhat.com>
>> ---
>
> Does it make sense for the following failure to be associated with this
> patch:
>
> 2697: ovn -- ensure one gw controller restart in HA doesn't bounce the
> master FAILED (ovn.at:9568)
>
> I'm about to enable reporting 'make distcheck' errors in the bot, and
> don't want it to be spamming with a known issue.  Bala is currently
> working on factoring out upstream errors, so that will have to come
> later.
>

Okay - I've rerun the test in question on the build system, and it is no
longer failing.  Seems like it was either an ephemeral condition or some
kind of interaction with whatever was in the repository at the time.

Feel free to ignore this error.
Mark Michelson Oct. 26, 2018, 2:17 p.m. UTC | #3
Hi Lorenzo,

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

Acked-by: Mark Michelson <mmichels@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@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])
>   
>
Lorenzo Bianconi Oct. 26, 2018, 2:51 p.m. UTC | #4
>
> Hi Lorenzo,
>
> I think for the patch, I'm willing to ack it, so:
>
> Acked-by: Mark Michelson <mmichels@redhat.com>
>
> However,  I have some minor suggestions below that might be worth making
> a second version of the patch for.

Thx for the review Mark, I will address your comments and send a v2

Regards,
Lorenzo

Patch
diff mbox series

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;
+        }
+    }
+
     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)
 {
     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)
 {
     /* 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)
 {
    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])