Message ID | 635353f3a63d4e1fc776ed9dea1f1403caab426d.1540570685.git.lorenzo.bianconi@redhat.com |
---|---|
State | Accepted |
Headers | show |
Series | [ovs-dev,v2] OVN: introduce mac_prefix support to IPAM | expand |
Looks good, Lorenzo, Just to reaaffirm my previous ack: Acked-by: Mark Michelson <mmichels@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 <mmichels@redhat.com> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@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]) > >
Thanks for the patch. 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. - 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. 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 <mmichels@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 <mmichels@redhat.com> > >Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > 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 <mmichels@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 <mmichels@redhat.com> > > >Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@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 > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Mon, Nov 05, 2018 at 06:17:54PM +0100, Lorenzo Bianconi wrote: > > > > 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? Yes, of course, it's a 22-bit space, collisions can happen. It sounds like we should stick with the proposed approach, but I'll give this a little while for others to comment.
On Mon, Nov 05, 2018 at 11:42:18AM -0800, Ben Pfaff wrote: > On Mon, Nov 05, 2018 at 06:17:54PM +0100, Lorenzo Bianconi wrote: > > > > > > 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? > > Yes, of course, it's a 22-bit space, collisions can happen. > > It sounds like we should stick with the proposed approach, but I'll > give this a little while for others to comment. Applied to master, thanks.
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])