On Tue, Feb 26, 2019 at 11:41 PM Lorenzo Bianconi < [email protected]> 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 <[email protected]> > Signed-off-by: Lorenzo Bianconi <[email protected]> > 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 > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
