[ovs-dev,v4] OVN: select a random mac_prefix if not provided

Message ID 25c60a2af7efa0bbbaad47417c0d8e90b92bfa28.1551791543.git.lorenzo.bianconi@redhat.com
State New
Headers show
Series
  • [ovs-dev,v4] OVN: select a random mac_prefix if not provided
Related show

Commit Message

Lorenzo Bianconi March 5, 2019, 1:22 p.m.
Select a random IPAM mac_prefix if it has not been provided by the user.
With this patch the admin can avoid to configure mac_prefix in order to
avoid L2 address collisions if multiple OVN deployments share the same
broadcast domain.
Remove MAC_ADDR_PREFIX definitions/occurrences since now mac_prefix is
always provided to ovn-northd

Acked-by: Numan Siddique <nusiddiq@redhat.com>
Tested-by: Miguel Duarte de Mora Barroso <mdbarroso@redhat.com>
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
Changes since v3:
- save just the OUI in NB db instead of full mac address
- rebased on top of master branch

Changes since v2:
- add automatic test case

Changes since v1:
- add entry in NEWS
- do not modify the idl object in-place but update it running
  nbrec_nb_global_set_options
---
 NEWS                    |  2 ++
 ovn/northd/ovn-northd.c | 37 ++++++++++++++++++-------------------
 tests/ovn.at            | 13 +++++++++++++
 3 files changed, 33 insertions(+), 19 deletions(-)

Comments

Ben Pfaff March 5, 2019, 11:49 p.m. | #1
On Tue, Mar 05, 2019 at 02:22:50PM +0100, Lorenzo Bianconi wrote:
> Select a random IPAM mac_prefix if it has not been provided by the user.
> With this patch the admin can avoid to configure mac_prefix in order to
> avoid L2 address collisions if multiple OVN deployments share the same
> broadcast domain.
> Remove MAC_ADDR_PREFIX definitions/occurrences since now mac_prefix is
> always provided to ovn-northd
> 
> Acked-by: Numan Siddique <nusiddiq@redhat.com>
> Tested-by: Miguel Duarte de Mora Barroso <mdbarroso@redhat.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>

Thanks.

I applied this to master.  I folded in the following incremental to save
a string copy and free.

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 69c33ad9f20d..3661681dc33b 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -7291,14 +7291,13 @@ ovnnb_db_run(struct northd_context *ctx,
         eth_addr_random(&mac_prefix);
         memset(&mac_prefix.ea[3], 0, 3);
 
-        char *addr_prefix = xasprintf("%02"PRIx8":%02"PRIx8":%02"PRIx8,
-                                      mac_prefix.ea[0], mac_prefix.ea[1],
-                                      mac_prefix.ea[2]);
-        smap_add(&options, "mac_prefix", addr_prefix);
+        smap_add_format(&options, "mac_prefix",
+                        "%02"PRIx8":%02"PRIx8":%02"PRIx8,
+                        mac_prefix.ea[0], mac_prefix.ea[1], mac_prefix.ea[2]);
+        nbrec_nb_global_verify_options(nb);
         nbrec_nb_global_set_options(nb, &options);
 
         smap_destroy(&options);
-        free(addr_prefix);
     }
 
     cleanup_macam(&macam);
Lorenzo Bianconi March 6, 2019, 10:25 a.m. | #2
> > Select a random IPAM mac_prefix if it has not been provided by the user.
> > With this patch the admin can avoid to configure mac_prefix in order to
> > avoid L2 address collisions if multiple OVN deployments share the same
> > broadcast domain.
> > Remove MAC_ADDR_PREFIX definitions/occurrences since now mac_prefix is
> > always provided to ovn-northd
> > 
> > Acked-by: Numan Siddique <nusiddiq@redhat.com>
> > Tested-by: Miguel Duarte de Mora Barroso <mdbarroso@redhat.com>
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> 
> Thanks.
> 
> I applied this to master.  I folded in the following incremental to save
> a string copy and free.

ack, thx Ben.

Regards,
Lorenzo

> 
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 69c33ad9f20d..3661681dc33b 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -7291,14 +7291,13 @@ ovnnb_db_run(struct northd_context *ctx,
>          eth_addr_random(&mac_prefix);
>          memset(&mac_prefix.ea[3], 0, 3);
>  
> -        char *addr_prefix = xasprintf("%02"PRIx8":%02"PRIx8":%02"PRIx8,
> -                                      mac_prefix.ea[0], mac_prefix.ea[1],
> -                                      mac_prefix.ea[2]);
> -        smap_add(&options, "mac_prefix", addr_prefix);
> +        smap_add_format(&options, "mac_prefix",
> +                        "%02"PRIx8":%02"PRIx8":%02"PRIx8,
> +                        mac_prefix.ea[0], mac_prefix.ea[1], mac_prefix.ea[2]);
> +        nbrec_nb_global_verify_options(nb);
>          nbrec_nb_global_set_options(nb, &options);
>  
>          smap_destroy(&options);
> -        free(addr_prefix);
>      }
>  
>      cleanup_macam(&macam);

Patch

diff --git a/NEWS b/NEWS
index 5da6a7405..89d0f19d6 100644
--- a/NEWS
+++ b/NEWS
@@ -22,6 +22,8 @@  Post-v2.11.0
        quickly after a brief disconnection, saving bandwidth and CPU time.
        See section 4.1.15 of ovsdb-server(7) for details of related OVSDB
        protocol extension.
+   - OVN:
+     * Select IPAM mac_prefix in a random manner if not provided by the user
 
 v2.11.0 - 19 Feb 2019
 ---------------------
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 3569ea2be..69c33ad9f 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -62,7 +62,6 @@  static const char *ovnnb_db;
 static const char *ovnsb_db;
 static const char *unixctl_path;
 
-#define MAC_ADDR_PREFIX 0x0A0000000000ULL
 #define MAC_ADDR_SPACE 0xffffff
 
 /* MAC address management (macam) table of "struct eth_addr"s, that holds the
@@ -937,13 +936,8 @@  ipam_insert_mac(struct eth_addr *ea, bool check)
     }
 
     uint64_t mac64 = eth_addr_to_uint64(*ea);
-    uint64_t prefix;
+    uint64_t prefix = eth_addr_to_uint64(mac_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. */
@@ -1056,11 +1050,7 @@  ipam_get_unused_mac(ovs_be32 ip)
     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;
-        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;
-        }
+        mac64 =  eth_addr_to_uint64(mac_prefix) | mac_addr_suffix;
         eth_addr_from_uint64(mac64, &mac);
         if (!ipam_is_duplicate_mac(&mac, mac64, true)) {
             break;
@@ -1132,13 +1122,7 @@  dynamic_mac_changed(const char *lsp_addresses,
    }
 
    uint64_t mac64 = eth_addr_to_uint64(update->current_addresses.ea);
-   uint64_t prefix;
-
-   if (!eth_addr_is_zero(mac_prefix)) {
-       prefix = eth_addr_to_uint64(mac_prefix);
-   } else {
-       prefix = MAC_ADDR_PREFIX;
-   }
+   uint64_t prefix = eth_addr_to_uint64(mac_prefix);
 
    if ((mac64 ^ prefix) >> 24) {
        return DYNAMIC;
@@ -7300,6 +7284,21 @@  ovnnb_db_run(struct northd_context *ctx,
                      &addr.ea[0], &addr.ea[1], &addr.ea[2])) {
             mac_prefix = addr;
         }
+    } else {
+        struct smap options;
+
+        smap_clone(&options, &nb->options);
+        eth_addr_random(&mac_prefix);
+        memset(&mac_prefix.ea[3], 0, 3);
+
+        char *addr_prefix = xasprintf("%02"PRIx8":%02"PRIx8":%02"PRIx8,
+                                      mac_prefix.ea[0], mac_prefix.ea[1],
+                                      mac_prefix.ea[2]);
+        smap_add(&options, "mac_prefix", addr_prefix);
+        nbrec_nb_global_set_options(nb, &options);
+
+        smap_destroy(&options);
+        free(addr_prefix);
     }
 
     cleanup_macam(&macam);
diff --git a/tests/ovn.at b/tests/ovn.at
index ec79651bd..2af225a67 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -5596,6 +5596,7 @@  ovn_start
 
 # Add a port to a switch that does not have a subnet set, then set the
 # subnet which should result in an address being allocated for the port.
+ovn-nbctl --wait=hv set NB_Global . options:mac_prefix="0a:00:00:00:00:00"
 ovn-nbctl ls-add sw0
 ovn-nbctl lsp-add sw0 p0 -- lsp-set-addresses p0 dynamic
 ovn-nbctl --wait=sb add Logical-Switch sw0 other_config subnet=192.168.1.0/24
@@ -5963,6 +5964,16 @@  AT_CHECK([ovn-nbctl get Logical-Switch-Port p83 dynamic_addresses], [0],
     ["00:11:22:00:00:08"
 ])
 
+# clear mac_prefix and check it is allocated in a random manner
+ovn-nbctl --wait=hv remove NB_Global . options mac_prefix
+ovn-nbctl ls-add sw9
+ovn-nbctl --wait=sb set Logical-Switch sw9 other_config:mac_only=true
+ovn-nbctl --wait=sb lsp-add sw9 p91 -- lsp-set-addresses p91 dynamic
+
+mac_prefix=$(ovn-nbctl --wait=sb get NB_Global . options:mac_prefix | tr -d \")
+port_addr=$(ovn-nbctl get Logical-Switch-Port p91 dynamic_addresses | tr -d \")
+AT_CHECK([test "$port_addr" = "${mac_prefix}:00:00:09"], [0], [])
+
 as ovn-sb
 OVS_APP_EXIT_AND_WAIT([ovsdb-server])
 
@@ -5981,6 +5992,7 @@  ovn_start
 ovn-nbctl lr-add R1
 
 # Test for a ping using dynamically allocated addresses.
+ovn-nbctl --wait=hv set NB_Global . options:mac_prefix="0a:00:00:00:00:00"
 ovn-nbctl ls-add foo -- add Logical_Switch foo other_config subnet=192.168.1.0/24
 ovn-nbctl ls-add alice -- add Logical_Switch alice other_config subnet=192.168.2.0/24
 
@@ -12232,6 +12244,7 @@  AT_CLEANUP
 AT_SETUP([ovn -- ipam to non-ipam])
 ovn_start
 
+ovn-nbctl --wait=hv set NB_Global . options:mac_prefix="0a:00:00:00:00:00"
 ovn-nbctl ls-add sw0
 ovn-nbctl lsp-add sw0 p0 -- lsp-set-addresses p0 dynamic
 ovn-nbctl --wait=sb add Logical-Switch sw0 other_config subnet=192.168.1.0/24