diff mbox series

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

Message ID 86f895f7e2e1fa0488ab675622e7640b21d3cd7c.1551203968.git.lorenzo.bianconi@redhat.com
State Superseded
Headers show
Series [ovs-dev,v2] OVN: select a random mac_prefix if not provided | expand

Commit Message

Lorenzo Bianconi Feb. 26, 2019, 6:04 p.m. UTC 
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

Tested-by: Miguel Duarte de Mora Barroso <mdbarroso@redhat.com>
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
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 | 35 ++++++++++++++++-------------------
 tests/ovn.at            |  3 +++
 3 files changed, 21 insertions(+), 19 deletions(-)

Comments

Numan Siddique Feb. 27, 2019, 7:43 a.m. UTC | #1 
On Tue, Feb 26, 2019 at 11:41 PM Lorenzo Bianconi <
lorenzo.bianconi@redhat.com> 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
>
> Tested-by: Miguel Duarte de Mora Barroso <mdbarroso@redhat.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
>

Looks good to me. Just a  comment below.



> ---
> 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 | 35 ++++++++++++++++-------------------
>  tests/ovn.at            |  3 +++
>  3 files changed, 21 insertions(+), 19 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index c86c13a23..b3b347036 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -17,6 +17,8 @@ Post-v2.11.0
>         conntrack fragmentation support.
>       * New "ovs-appctl dpctl/ipf-get-status" command for userspace
> datapath
>         conntrack fragmentation support.
> +   - 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..373e45573 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,19 @@ 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(ETH_ADDR_FMT,
> ETH_ADDR_ARGS(mac_prefix));
> +        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..3688ca7aa 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"
>

There is no test case which  tests that ovn-northd has set the
options:mac_prefix in the NB DB table.
My suggestion would be to either add another test case or enhance this test
case itself
to  wait for the options:mac_prefix is set in NB_Global by ovn-northd ,
read the prefix value
and validate that the mac address of logical port "p0" is set accordingly.
And then you can set the mac_prefix to "0a:00:00..."
and expect that "p0" 's mac address is updated accordingly.

I tested manually this patch and it works as expected. It would be nice to
have a test case so that we don't have any regressions
later.

Thanks
Numan


 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
> @@ -5981,6 +5982,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 +12234,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
> --
> 2.20.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Lorenzo Bianconi Feb. 27, 2019, 4:07 p.m. UTC | #2 
> On Tue, Feb 26, 2019 at 11:41 PM Lorenzo Bianconi <
> lorenzo.bianconi@redhat.com> 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
> >
> > Tested-by: Miguel Duarte de Mora Barroso <mdbarroso@redhat.com>
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> >
> 
> Looks good to me. Just a  comment below.
> 
> 
> 
> > ---
> > 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 | 35 ++++++++++++++++-------------------
> >  tests/ovn.at            |  3 +++
> >  3 files changed, 21 insertions(+), 19 deletions(-)
> >
> > diff --git a/NEWS b/NEWS
> > index c86c13a23..b3b347036 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -17,6 +17,8 @@ Post-v2.11.0
> >         conntrack fragmentation support.
> >       * New "ovs-appctl dpctl/ipf-get-status" command for userspace
> > datapath
> >         conntrack fragmentation support.
> > +   - OVN:
> > +     * Select IPAM mac_prefix in a random manner if not provided by the
> > user
> >

[...]

> 
> There is no test case which  tests that ovn-northd has set the
> options:mac_prefix in the NB DB table.
> My suggestion would be to either add another test case or enhance this test
> case itself
> to  wait for the options:mac_prefix is set in NB_Global by ovn-northd ,
> read the prefix value
> and validate that the mac address of logical port "p0" is set accordingly.
> And then you can set the mac_prefix to "0a:00:00..."
> and expect that "p0" 's mac address is updated accordingly.
> 
> I tested manually this patch and it works as expected. It would be nice to
> have a test case so that we don't have any regressions
> later.
> 
> Thanks
> Numan

Hi Numan,

thx for the review.
Maybe I did not get exactly what you mean here but I guess 'mac_prefix' is
already tested when we create sw6
(https://github.com/openvswitch/ovs/blob/master/tests/ovn.at#L5921) since we
switch mac_prefix from 0a:00:00 to 00:11:22. Am I missing something?

Regards,
Lorenzo

> 
> 
>  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
> > @@ -5981,6 +5982,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 +12234,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
> > --
> > 2.20.1
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
diff mbox series

Patch

diff --git a/NEWS b/NEWS
index c86c13a23..b3b347036 100644
--- a/NEWS
+++ b/NEWS
@@ -17,6 +17,8 @@  Post-v2.11.0
        conntrack fragmentation support.
      * New "ovs-appctl dpctl/ipf-get-status" command for userspace datapath
        conntrack fragmentation support.
+   - 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..373e45573 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,19 @@  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(ETH_ADDR_FMT, ETH_ADDR_ARGS(mac_prefix));
+        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..3688ca7aa 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
@@ -5981,6 +5982,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 +12234,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