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

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

Acked-by: Mark Michelson <mmichels@redhat.com>
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
Changes since v1:
- use a global definition for mac_prefix
---
 ovn/northd/ovn-northd.c | 37 ++++++++++++++++++++++++++++++++++---
 ovn/ovn-nb.xml          |  5 +++++
 tests/ovn.at            | 17 +++++++++++++++++
 3 files changed, 56 insertions(+), 3 deletions(-)

Looks good, Lorenzo,

Just to reaaffirm my previous ack:

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

On 10/26/2018 12:20 PM, 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
> 
> Acked-by: Mark Michelson <mmichels@redhat.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> ---
> Changes since v1:
> - use a global definition for mac_prefix
> ---
>   ovn/northd/ovn-northd.c | 37 ++++++++++++++++++++++++++++++++++---
>   ovn/ovn-nb.xml          |  5 +++++
>   tests/ovn.at            | 17 +++++++++++++++++
>   3 files changed, 56 insertions(+), 3 deletions(-)
> 
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 439651f80..816c72311 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -68,6 +68,7 @@ static const char *unixctl_path;
>   /* 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;
>   
>   #define MAX_OVN_TAGS 4096
>   
> @@ -922,10 +923,17 @@ ipam_insert_mac(struct eth_addr *ea, bool check)
>       }
>   
>       uint64_t mac64 = eth_addr_to_uint64(*ea);
> +    uint64_t prefix;
> +
> +    if (!eth_addr_is_zero(mac_prefix)) {
> +        prefix = eth_addr_to_uint64(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;
>       }
> @@ -1036,7 +1044,11 @@ 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 (!eth_addr_is_zero(mac_prefix)) {
> +            mac64 =  eth_addr_to_uint64(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;
> @@ -1107,7 +1119,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 (!eth_addr_is_zero(mac_prefix)) {
> +       prefix = eth_addr_to_uint64(mac_prefix);
> +   } else {
> +       prefix = MAC_ADDR_PREFIX;
> +   }
> +
> +   if ((mac64 ^ prefix) >> 24) {
>          return DYNAMIC;
>      } else {
>          return NONE;
> @@ -7141,6 +7161,17 @@ 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;
> +        }
> +    }
> +
>       cleanup_macam(&macam);
>   }
>   
> 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])
>   
>

Thanks for the patch.

I'm not sure in what circumstances a broadcast domain would be shared
among deployments.  I tend to think of OVN L2 networks as contained.
But I guess this patch was written for a reason, so it must be common
enough.

I can see at least two possible routes here:

    - The current one, in which there is a default MAC_ADDR_PREFIX.  Two
      OVN deployments cannot coexist using MACAM, supposing they somehow
      share a broadcast domain, unless at least one of them manually
      selects an alternate MAC prefix.  This may be desirable if it is
      important that the default MAC prefix 0A-00-00 be recognizable.

    - An alternative would be for every OVN deployment to automatically
      select a random MAC address prefix.  When ovn-northd starts, it
      would read the MAC address prefix out of NB_Global, and if there
      isn't one, generate one randomly and store it into NB_Global.
      Then there would be fewer pitfalls in setting up multiple
      deployments.

I am OK with the one currently proposed, but I want to make sure that
the other approach was at least considered.

On Fri, Oct 26, 2018 at 04:15:27PM -0400, Mark Michelson wrote:
> Looks good, Lorenzo,
> 
> Just to reaaffirm my previous ack:
> 
> Acked-by: Mark Michelson <mmichels@redhat.com>
> 
> On 10/26/2018 12:20 PM, 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
> >
> >Acked-by: Mark Michelson <mmichels@redhat.com>
> >Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> >---
> >Changes since v1:
> >- use a global definition for mac_prefix
> >---
> >  ovn/northd/ovn-northd.c | 37 ++++++++++++++++++++++++++++++++++---
> >  ovn/ovn-nb.xml          |  5 +++++
> >  tests/ovn.at            | 17 +++++++++++++++++
> >  3 files changed, 56 insertions(+), 3 deletions(-)
> >
> >diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> >index 439651f80..816c72311 100644
> >--- a/ovn/northd/ovn-northd.c
> >+++ b/ovn/northd/ovn-northd.c
> >@@ -68,6 +68,7 @@ static const char *unixctl_path;
> >  /* 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;
> >  #define MAX_OVN_TAGS 4096
> >  
> >@@ -922,10 +923,17 @@ ipam_insert_mac(struct eth_addr *ea, bool check)
> >      }
> >      uint64_t mac64 = eth_addr_to_uint64(*ea);
> >+    uint64_t prefix;
> >+
> >+    if (!eth_addr_is_zero(mac_prefix)) {
> >+        prefix = eth_addr_to_uint64(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;
> >      }
> >@@ -1036,7 +1044,11 @@ 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 (!eth_addr_is_zero(mac_prefix)) {
> >+            mac64 =  eth_addr_to_uint64(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;
> >@@ -1107,7 +1119,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 (!eth_addr_is_zero(mac_prefix)) {
> >+       prefix = eth_addr_to_uint64(mac_prefix);
> >+   } else {
> >+       prefix = MAC_ADDR_PREFIX;
> >+   }
> >+
> >+   if ((mac64 ^ prefix) >> 24) {
> >         return DYNAMIC;
> >     } else {
> >         return NONE;
> >@@ -7141,6 +7161,17 @@ 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;
> >+        }
> >+    }
> >+
> >      cleanup_macam(&macam);
> >  }
> >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])
> >
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

>
> Thanks for the patch.

Hi Ben,

thx for the review. Few comments inline.

Regards,
Lorenzo

>
> I'm not sure in what circumstances a broadcast domain would be shared
> among deployments.  I tend to think of OVN L2 networks as contained.
> But I guess this patch was written for a reason, so it must be common
> enough.
>
> I can see at least two possible routes here:
>
>     - The current one, in which there is a default MAC_ADDR_PREFIX.  Two
>       OVN deployments cannot coexist using MACAM, supposing they somehow
>       share a broadcast domain, unless at least one of them manually
>       selects an alternate MAC prefix.  This may be desirable if it is
>       important that the default MAC prefix 0A-00-00 be recognizable.
>

I preferred to maintain a default mac prefix for compatibility with
older deployments
that maybe assume to have a default mac prefix for MACAM, but if you
prefer we can
switch to the second implementation

>     - An alternative would be for every OVN deployment to automatically
>       select a random MAC address prefix.  When ovn-northd starts, it
>       would read the MAC address prefix out of NB_Global, and if there
>       isn't one, generate one randomly and store it into NB_Global.
>       Then there would be fewer pitfalls in setting up multiple
>       deployments.
>

the only concern that come to my mind is that, is possible to have a
mac prefix collision on multiple
deployments when mac prefix is randomly chosen?

> I am OK with the one currently proposed, but I want to make sure that
> the other approach was at least considered.
>
> On Fri, Oct 26, 2018 at 04:15:27PM -0400, Mark Michelson wrote:
> > Looks good, Lorenzo,
> >
> > Just to reaaffirm my previous ack:
> >
> > Acked-by: Mark Michelson <mmichels@redhat.com>
> >
> > On 10/26/2018 12:20 PM, 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
> > >
> > >Acked-by: Mark Michelson <mmichels@redhat.com>
> > >Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> > >---
> > >Changes since v1:
> > >- use a global definition for mac_prefix
> > >---
> > >  ovn/northd/ovn-northd.c | 37 ++++++++++++++++++++++++++++++++++---
> > >  ovn/ovn-nb.xml          |  5 +++++
> > >  tests/ovn.at            | 17 +++++++++++++++++
> > >  3 files changed, 56 insertions(+), 3 deletions(-)
> > >
> > >diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> > >index 439651f80..816c72311 100644
> > >--- a/ovn/northd/ovn-northd.c
> > >+++ b/ovn/northd/ovn-northd.c
> > >@@ -68,6 +68,7 @@ static const char *unixctl_path;
> > >  /* 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;
> > >  #define MAX_OVN_TAGS 4096
> > >
> > >@@ -922,10 +923,17 @@ ipam_insert_mac(struct eth_addr *ea, bool check)
> > >      }
> > >      uint64_t mac64 = eth_addr_to_uint64(*ea);
> > >+    uint64_t prefix;
> > >+
> > >+    if (!eth_addr_is_zero(mac_prefix)) {
> > >+        prefix = eth_addr_to_uint64(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;
> > >      }
> > >@@ -1036,7 +1044,11 @@ 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 (!eth_addr_is_zero(mac_prefix)) {
> > >+            mac64 =  eth_addr_to_uint64(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;
> > >@@ -1107,7 +1119,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 (!eth_addr_is_zero(mac_prefix)) {
> > >+       prefix = eth_addr_to_uint64(mac_prefix);
> > >+   } else {
> > >+       prefix = MAC_ADDR_PREFIX;
> > >+   }
> > >+
> > >+   if ((mac64 ^ prefix) >> 24) {
> > >         return DYNAMIC;
> > >     } else {
> > >         return NONE;
> > >@@ -7141,6 +7161,17 @@ 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;
> > >+        }
> > >+    }
> > >+
> > >      cleanup_macam(&macam);
> > >  }
> > >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])
> > >
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On Mon, Nov 05, 2018 at 06:17:54PM +0100, Lorenzo Bianconi wrote:
> >
> > Thanks for the patch.
> 
> Hi Ben,
> 
> thx for the review. Few comments inline.
> 
> Regards,
> Lorenzo
> 
> >
> > I'm not sure in what circumstances a broadcast domain would be shared
> > among deployments.  I tend to think of OVN L2 networks as contained.
> > But I guess this patch was written for a reason, so it must be common
> > enough.
> >
> > I can see at least two possible routes here:
> >
> >     - The current one, in which there is a default MAC_ADDR_PREFIX.  Two
> >       OVN deployments cannot coexist using MACAM, supposing they somehow
> >       share a broadcast domain, unless at least one of them manually
> >       selects an alternate MAC prefix.  This may be desirable if it is
> >       important that the default MAC prefix 0A-00-00 be recognizable.
> >
> 
> I preferred to maintain a default mac prefix for compatibility with
> older deployments
> that maybe assume to have a default mac prefix for MACAM, but if you
> prefer we can
> switch to the second implementation
> 
> >     - An alternative would be for every OVN deployment to automatically
> >       select a random MAC address prefix.  When ovn-northd starts, it
> >       would read the MAC address prefix out of NB_Global, and if there
> >       isn't one, generate one randomly and store it into NB_Global.
> >       Then there would be fewer pitfalls in setting up multiple
> >       deployments.
> >
> 
> the only concern that come to my mind is that, is possible to have a
> mac prefix collision on multiple
> deployments when mac prefix is randomly chosen?

Yes, of course, it's a 22-bit space, collisions can happen.

It sounds like we should stick with the  proposed approach, but I'll
give this a little while for others to comment.

On Mon, Nov 05, 2018 at 11:42:18AM -0800, Ben Pfaff wrote:
> On Mon, Nov 05, 2018 at 06:17:54PM +0100, Lorenzo Bianconi wrote:
> > >
> > > Thanks for the patch.
> > 
> > Hi Ben,
> > 
> > thx for the review. Few comments inline.
> > 
> > Regards,
> > Lorenzo
> > 
> > >
> > > I'm not sure in what circumstances a broadcast domain would be shared
> > > among deployments.  I tend to think of OVN L2 networks as contained.
> > > But I guess this patch was written for a reason, so it must be common
> > > enough.
> > >
> > > I can see at least two possible routes here:
> > >
> > >     - The current one, in which there is a default MAC_ADDR_PREFIX.  Two
> > >       OVN deployments cannot coexist using MACAM, supposing they somehow
> > >       share a broadcast domain, unless at least one of them manually
> > >       selects an alternate MAC prefix.  This may be desirable if it is
> > >       important that the default MAC prefix 0A-00-00 be recognizable.
> > >
> > 
> > I preferred to maintain a default mac prefix for compatibility with
> > older deployments
> > that maybe assume to have a default mac prefix for MACAM, but if you
> > prefer we can
> > switch to the second implementation
> > 
> > >     - An alternative would be for every OVN deployment to automatically
> > >       select a random MAC address prefix.  When ovn-northd starts, it
> > >       would read the MAC address prefix out of NB_Global, and if there
> > >       isn't one, generate one randomly and store it into NB_Global.
> > >       Then there would be fewer pitfalls in setting up multiple
> > >       deployments.
> > >
> > 
> > the only concern that come to my mind is that, is possible to have a
> > mac prefix collision on multiple
> > deployments when mac prefix is randomly chosen?
> 
> Yes, of course, it's a 22-bit space, collisions can happen.
> 
> It sounds like we should stick with the  proposed approach, but I'll
> give this a little while for others to comment.

Applied to master, thanks.