>
> 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 <mmich...@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 <mmich...@redhat.com>
> > >Signed-off-by: Lorenzo Bianconi <lorenzo.bianc...@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
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
