Message ID | 60e204ca0aa16d89599eb4cd5ad0be6550458f7a.1550677847.git.lorenzo.bianconi@redhat.com |
---|---|
State | Superseded |
Headers | show |
Series | [ovs-dev] OVN: select a random mac_prefix if not provided | expand |
Looks good to me. The only potential issue here is that this constitutes a change in behavior with no configuration change. Is there a good place to note this so that it is clear to people who upgrade? Perhaps a NEWS entry? On 2/20/19 10:53 AM, 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 > > Tested-by: Miguel Duarte de Mora Barroso <mdbarroso@redhat.com> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > --- > ovn/northd/ovn-northd.c | 30 +++++++++++------------------- > tests/ovn.at | 3 +++ > 2 files changed, 14 insertions(+), 19 deletions(-) > > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c > index 3569ea2be..2b7a3634e 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,14 @@ ovnnb_db_run(struct northd_context *ctx, > &addr.ea[0], &addr.ea[1], &addr.ea[2])) { > mac_prefix = addr; > } > + } else { > + 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((struct smap *)&nb->options, "mac_prefix", addr_prefix); > + nbrec_nb_global_set_options(nb, &nb->options); > + free(addr_prefix) > } > > cleanup_macam(&macam); > diff --git a/tests/ovn.at b/tests/ovn.at > index cfdbf412c..7e69808ed 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -5416,6 +5416,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 > @@ -5801,6 +5802,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 > > @@ -12052,6 +12054,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 >
> Looks good to me. The only potential issue here is that this constitutes a > change in behavior with no configuration change. Is there a good place to > note this so that it is clear to people who upgrade? Perhaps a NEWS entry? > ack, fine. I will post a v2 adding an entry in NEWS. Ben do you agree? Regards, Lorenzo > On 2/20/19 10:53 AM, 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 > > > > Tested-by: Miguel Duarte de Mora Barroso <mdbarroso@redhat.com> > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > --- > > ovn/northd/ovn-northd.c | 30 +++++++++++------------------- > > tests/ovn.at | 3 +++ > > 2 files changed, 14 insertions(+), 19 deletions(-) > > > > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c > > index 3569ea2be..2b7a3634e 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,14 @@ ovnnb_db_run(struct northd_context *ctx, > > &addr.ea[0], &addr.ea[1], &addr.ea[2])) { > > mac_prefix = addr; > > } > > + } else { > > + 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((struct smap *)&nb->options, "mac_prefix", addr_prefix); > > + nbrec_nb_global_set_options(nb, &nb->options); > > + free(addr_prefix) > > } > > cleanup_macam(&macam); > > diff --git a/tests/ovn.at b/tests/ovn.at > > index cfdbf412c..7e69808ed 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -5416,6 +5416,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 > > @@ -5801,6 +5802,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 > > @@ -12052,6 +12054,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 > > >
On Wed, Feb 20, 2019 at 04:53:45PM +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 > > Tested-by: Miguel Duarte de Mora Barroso <mdbarroso@redhat.com> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> It's really not a good idea to modify an idl object in-place like this. That's why they're all 'const'! The IDL doesn't expect this kind of thing and if it's not causing trouble now it could easily do so later: > + } else { > + 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((struct smap *)&nb->options, "mac_prefix", addr_prefix); > + nbrec_nb_global_set_options(nb, &nb->options); > + free(addr_prefix); > }
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c index 3569ea2be..2b7a3634e 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,14 @@ ovnnb_db_run(struct northd_context *ctx, &addr.ea[0], &addr.ea[1], &addr.ea[2])) { mac_prefix = addr; } + } else { + 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((struct smap *)&nb->options, "mac_prefix", addr_prefix); + nbrec_nb_global_set_options(nb, &nb->options); + free(addr_prefix); } cleanup_macam(&macam); diff --git a/tests/ovn.at b/tests/ovn.at index cfdbf412c..7e69808ed 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -5416,6 +5416,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 @@ -5801,6 +5802,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 @@ -12052,6 +12054,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