| Message ID | f6c5b74e2a086014328ef9792e7d8778a64ea048.1540287483.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] OVN: introduce mac_prefix support to IPAM | expand |
Hi Lorenzo, Lorenzo Bianconi <lorenzo.bianconi@redhat.com> writes: > 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 > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > --- Does it make sense for the following failure to be associated with this patch: 2697: ovn -- ensure one gw controller restart in HA doesn't bounce the master FAILED (ovn.at:9568) I'm about to enable reporting 'make distcheck' errors in the bot, and don't want it to be spamming with a known issue. Bala is currently working on factoring out upstream errors, so that will have to come later. > ovn/northd/ovn-northd.c | 76 ++++++++++++++++++++++++++++++++--------- > ovn/ovn-nb.xml | 5 +++ > tests/ovn.at | 17 +++++++++ > 3 files changed, 81 insertions(+), 17 deletions(-) > > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c > index 439651f80..3e8a4a276 100644 > --- a/ovn/northd/ovn-northd.c > +++ b/ovn/northd/ovn-northd.c > @@ -404,6 +404,9 @@ struct ipam_info { > unsigned long *allocated_ipv4s; /* A bitmap of allocated IPv4s */ > bool ipv6_prefix_set; > struct in6_addr ipv6_prefix; > + bool mac_prefix_set; > + struct eth_addr mac_prefix; > + > }; > > /* The 'key' comes from nbs->header_.uuid or nbr->header_.uuid or > @@ -534,7 +537,8 @@ lrouter_is_enabled(const struct nbrec_logical_router *lrouter) > } > > static void > -init_ipam_info_for_datapath(struct ovn_datapath *od) > +init_ipam_info_for_datapath(struct ovn_datapath *od, > + struct northd_context *ctx) > { > if (!od->nbs) { > return; > @@ -543,11 +547,27 @@ init_ipam_info_for_datapath(struct ovn_datapath *od) > const char *subnet_str = smap_get(&od->nbs->other_config, "subnet"); > const char *ipv6_prefix = smap_get(&od->nbs->other_config, "ipv6_prefix"); > > + const struct nbrec_nb_global *nb = nbrec_nb_global_first(ctx->ovnnb_idl); > + const char *mac_prefix = smap_get(&nb->options, "mac_prefix"); > + > if (ipv6_prefix) { > od->ipam_info.ipv6_prefix_set = ipv6_parse( > ipv6_prefix, &od->ipam_info.ipv6_prefix); > } > > + if (mac_prefix) { > + struct eth_addr addr; > + > + memset(&addr, 0, sizeof addr); > + if (ovs_scan(mac_prefix, "%"SCNx8":%"SCNx8":%"SCNx8, > + &addr.ea[0], &addr.ea[1], &addr.ea[2])) { > + od->ipam_info.mac_prefix_set = true; > + od->ipam_info.mac_prefix = addr; > + } else { > + od->ipam_info.mac_prefix_set = false; > + } > + } > + > if (!subnet_str) { > return; > } > @@ -703,7 +723,7 @@ join_datapaths(struct northd_context *ctx, struct hmap *datapaths, > ovs_list_push_back(nb_only, &od->list); > } > > - init_ipam_info_for_datapath(od); > + init_ipam_info_for_datapath(od, ctx); > } > > const struct nbrec_logical_router *nbr; > @@ -915,17 +935,24 @@ ipam_is_duplicate_mac(struct eth_addr *ea, uint64_t mac64, bool warn) > } > > static void > -ipam_insert_mac(struct eth_addr *ea, bool check) > +ipam_insert_mac(struct ovn_datapath *od, struct eth_addr *ea, bool check) > { > if (!ea) { > return; > } > > uint64_t mac64 = eth_addr_to_uint64(*ea); > + uint64_t prefix; > + > + if (od->ipam_info.mac_prefix_set) { > + prefix = eth_addr_to_uint64(od->ipam_info.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; > } > @@ -970,7 +997,7 @@ ipam_insert_lsp_addresses(struct ovn_datapath *od, struct ovn_port *op, > VLOG_WARN_RL(&rl, "Extract addresses failed."); > return; > } > - ipam_insert_mac(&laddrs.ea, true); > + ipam_insert_mac(od, &laddrs.ea, true); > > /* IP is only added to IPAM if the switch's subnet option > * is set, whereas MAC is always added to MACAM. */ > @@ -1007,7 +1034,7 @@ ipam_add_port_addresses(struct ovn_datapath *od, struct ovn_port *op) > VLOG_WARN_RL(&rl, "Extract addresses failed."); > return; > } > - ipam_insert_mac(&lrp_networks.ea, true); > + ipam_insert_mac(od, &lrp_networks.ea, true); > > if (!op->peer || !op->peer->nbsp || !op->peer->od || !op->peer->od->nbs > || !smap_get(&op->peer->od->nbs->other_config, "subnet")) { > @@ -1025,7 +1052,7 @@ ipam_add_port_addresses(struct ovn_datapath *od, struct ovn_port *op) > } > > static uint64_t > -ipam_get_unused_mac(void) > +ipam_get_unused_mac(struct ovn_datapath *od) > { > /* Stores the suffix of the most recently ipam-allocated MAC address. */ > static uint32_t last_mac; > @@ -1036,7 +1063,12 @@ 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 (od->ipam_info.mac_prefix_set) { > + mac64 = eth_addr_to_uint64(od->ipam_info.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; > @@ -1091,7 +1123,7 @@ struct dynamic_address_update { > }; > > static enum dynamic_update_type > -dynamic_mac_changed(const char *lsp_addresses, > +dynamic_mac_changed(struct ovn_datapath *od, const char *lsp_addresses, > struct dynamic_address_update *update) > { > struct eth_addr ea; > @@ -1107,7 +1139,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 (od->ipam_info.mac_prefix_set) { > + prefix = eth_addr_to_uint64(od->ipam_info.mac_prefix); > + } else { > + prefix = MAC_ADDR_PREFIX; > + } > + > + if ((mac64 ^ prefix) >> 24) { > return DYNAMIC; > } else { > return NONE; > @@ -1195,10 +1235,11 @@ dynamic_ip6_changed(struct dynamic_address_update *update) > * Returns true if any changes to dynamic addresses are required > */ > static bool > -dynamic_addresses_check_for_updates(const char *lsp_addrs, > +dynamic_addresses_check_for_updates(struct ovn_datapath *od, > + const char *lsp_addrs, > struct dynamic_address_update *update) > { > - update->mac = dynamic_mac_changed(lsp_addrs, update); > + update->mac = dynamic_mac_changed(od, lsp_addrs, update); > update->ipv4 = dynamic_ip4_changed(update); > update->ipv6 = dynamic_ip6_changed(update); > if (update->mac == NONE && > @@ -1215,10 +1256,11 @@ dynamic_addresses_check_for_updates(const char *lsp_addrs, > * elsewhere later. > */ > static void > -update_unchanged_dynamic_addresses(struct dynamic_address_update *update) > +update_unchanged_dynamic_addresses(struct ovn_datapath *od, > + struct dynamic_address_update *update) > { > if (update->mac == NONE) { > - ipam_insert_mac(&update->current_addresses.ea, false); > + ipam_insert_mac(od, &update->current_addresses.ea, false); > } > if (update->ipv4 == NONE && update->current_addresses.n_ipv4_addrs) { > ipam_insert_ip(update->op->od, > @@ -1278,7 +1320,7 @@ update_dynamic_addresses(struct ovn_datapath *od, > mac = update->static_mac; > break; > case DYNAMIC: > - eth_addr_from_uint64(ipam_get_unused_mac(), &mac); > + eth_addr_from_uint64(ipam_get_unused_mac(od), &mac); > break; > } > > @@ -1390,8 +1432,8 @@ build_ipam(struct hmap *datapaths, struct hmap *ports) > extract_lsp_addresses(nbsp->dynamic_addresses, > &update->current_addresses); > any_changed = dynamic_addresses_check_for_updates( > - nbsp->addresses[j], update); > - update_unchanged_dynamic_addresses(update); > + od, nbsp->addresses[j], update); > + update_unchanged_dynamic_addresses(od, update); > if (any_changed) { > ovs_list_push_back(&updates, &update->node); > } else { > 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])
Aaron Conole <aconole@redhat.com> writes: > Hi Lorenzo, > > Lorenzo Bianconi <lorenzo.bianconi@redhat.com> writes: > >> 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 >> >> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> >> --- > > Does it make sense for the following failure to be associated with this > patch: > > 2697: ovn -- ensure one gw controller restart in HA doesn't bounce the > master FAILED (ovn.at:9568) > > I'm about to enable reporting 'make distcheck' errors in the bot, and > don't want it to be spamming with a known issue. Bala is currently > working on factoring out upstream errors, so that will have to come > later. > Okay - I've rerun the test in question on the build system, and it is no longer failing. Seems like it was either an ephemeral condition or some kind of interaction with whatever was in the repository at the time. Feel free to ignore this error.
Hi Lorenzo, I think for the patch, I'm willing to ack it, so: Acked-by: Mark Michelson <mmichels@redhat.com> However, I have some minor suggestions below that might be worth making a second version of the patch for. On 10/23/2018 06:01 AM, 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 > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > --- > ovn/northd/ovn-northd.c | 76 ++++++++++++++++++++++++++++++++--------- > ovn/ovn-nb.xml | 5 +++ > tests/ovn.at | 17 +++++++++ > 3 files changed, 81 insertions(+), 17 deletions(-) > > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c > index 439651f80..3e8a4a276 100644 > --- a/ovn/northd/ovn-northd.c > +++ b/ovn/northd/ovn-northd.c > @@ -404,6 +404,9 @@ struct ipam_info { > unsigned long *allocated_ipv4s; /* A bitmap of allocated IPv4s */ > bool ipv6_prefix_set; > struct in6_addr ipv6_prefix; > + bool mac_prefix_set; > + struct eth_addr mac_prefix; > + > }; > > /* The 'key' comes from nbs->header_.uuid or nbr->header_.uuid or > @@ -534,7 +537,8 @@ lrouter_is_enabled(const struct nbrec_logical_router *lrouter) > } > > static void > -init_ipam_info_for_datapath(struct ovn_datapath *od) > +init_ipam_info_for_datapath(struct ovn_datapath *od, > + struct northd_context *ctx) > { > if (!od->nbs) { > return; > @@ -543,11 +547,27 @@ init_ipam_info_for_datapath(struct ovn_datapath *od) > const char *subnet_str = smap_get(&od->nbs->other_config, "subnet"); > const char *ipv6_prefix = smap_get(&od->nbs->other_config, "ipv6_prefix"); > > + const struct nbrec_nb_global *nb = nbrec_nb_global_first(ctx->ovnnb_idl); > + const char *mac_prefix = smap_get(&nb->options, "mac_prefix"); > + > if (ipv6_prefix) { > od->ipam_info.ipv6_prefix_set = ipv6_parse( > ipv6_prefix, &od->ipam_info.ipv6_prefix); > } > > + if (mac_prefix) { > + struct eth_addr addr; > + > + memset(&addr, 0, sizeof addr); > + if (ovs_scan(mac_prefix, "%"SCNx8":%"SCNx8":%"SCNx8, > + &addr.ea[0], &addr.ea[1], &addr.ea[2])) { > + od->ipam_info.mac_prefix_set = true; > + od->ipam_info.mac_prefix = addr > + } else { > + od->ipam_info.mac_prefix_set = false; > + } > + } This operation will be exactly the same for every datapath since the mac_prefix is a global setting. It seems wasteful to do the same thing on every datapath. Why not parse the configured mac_prefix once and store it as a global in ovn-northd? > + > if (!subnet_str) { > return; > } > @@ -703,7 +723,7 @@ join_datapaths(struct northd_context *ctx, struct hmap *datapaths, > ovs_list_push_back(nb_only, &od->list); > } > > - init_ipam_info_for_datapath(od); > + init_ipam_info_for_datapath(od, ctx); > } > > const struct nbrec_logical_router *nbr; > @@ -915,17 +935,24 @@ ipam_is_duplicate_mac(struct eth_addr *ea, uint64_t mac64, bool warn) > } > > static void > -ipam_insert_mac(struct eth_addr *ea, bool check) > +ipam_insert_mac(struct ovn_datapath *od, struct eth_addr *ea, bool check) I believe od can be made const. > { > if (!ea) { > return; > } > > uint64_t mac64 = eth_addr_to_uint64(*ea); > + uint64_t prefix; > + > + if (od->ipam_info.mac_prefix_set) { > + prefix = eth_addr_to_uint64(od->ipam_info.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; > } > @@ -970,7 +997,7 @@ ipam_insert_lsp_addresses(struct ovn_datapath *od, struct ovn_port *op, > VLOG_WARN_RL(&rl, "Extract addresses failed."); > return; > } > - ipam_insert_mac(&laddrs.ea, true); > + ipam_insert_mac(od, &laddrs.ea, true); > > /* IP is only added to IPAM if the switch's subnet option > * is set, whereas MAC is always added to MACAM. */ > @@ -1007,7 +1034,7 @@ ipam_add_port_addresses(struct ovn_datapath *od, struct ovn_port *op) > VLOG_WARN_RL(&rl, "Extract addresses failed."); > return; > } > - ipam_insert_mac(&lrp_networks.ea, true); > + ipam_insert_mac(od, &lrp_networks.ea, true); > > if (!op->peer || !op->peer->nbsp || !op->peer->od || !op->peer->od->nbs > || !smap_get(&op->peer->od->nbs->other_config, "subnet")) { > @@ -1025,7 +1052,7 @@ ipam_add_port_addresses(struct ovn_datapath *od, struct ovn_port *op) > } > > static uint64_t > -ipam_get_unused_mac(void) > +ipam_get_unused_mac(struct ovn_datapath *od) I believe od can be made const. > { > /* Stores the suffix of the most recently ipam-allocated MAC address. */ > static uint32_t last_mac; > @@ -1036,7 +1063,12 @@ 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 (od->ipam_info.mac_prefix_set) { > + mac64 = eth_addr_to_uint64(od->ipam_info.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; > @@ -1091,7 +1123,7 @@ struct dynamic_address_update { > }; > > static enum dynamic_update_type > -dynamic_mac_changed(const char *lsp_addresses, > +dynamic_mac_changed(struct ovn_datapath *od, const char *lsp_addresses, > struct dynamic_address_update *update) I believe od can be made const. > { > struct eth_addr ea; > @@ -1107,7 +1139,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 (od->ipam_info.mac_prefix_set) { > + prefix = eth_addr_to_uint64(od->ipam_info.mac_prefix); > + } else { > + prefix = MAC_ADDR_PREFIX; > + } > + > + if ((mac64 ^ prefix) >> 24) { > return DYNAMIC; > } else { > return NONE; > @@ -1195,10 +1235,11 @@ dynamic_ip6_changed(struct dynamic_address_update *update) > * Returns true if any changes to dynamic addresses are required > */ > static bool > -dynamic_addresses_check_for_updates(const char *lsp_addrs, > +dynamic_addresses_check_for_updates(struct ovn_datapath *od, > + const char *lsp_addrs, > struct dynamic_address_update *update) > { > - update->mac = dynamic_mac_changed(lsp_addrs, update); > + update->mac = dynamic_mac_changed(od, lsp_addrs, update); > update->ipv4 = dynamic_ip4_changed(update); > update->ipv6 = dynamic_ip6_changed(update); > if (update->mac == NONE && > @@ -1215,10 +1256,11 @@ dynamic_addresses_check_for_updates(const char *lsp_addrs, > * elsewhere later. > */ > static void > -update_unchanged_dynamic_addresses(struct dynamic_address_update *update) > +update_unchanged_dynamic_addresses(struct ovn_datapath *od, > + struct dynamic_address_update *update) > { > if (update->mac == NONE) { > - ipam_insert_mac(&update->current_addresses.ea, false); > + ipam_insert_mac(od, &update->current_addresses.ea, false); > } > if (update->ipv4 == NONE && update->current_addresses.n_ipv4_addrs) { > ipam_insert_ip(update->op->od, > @@ -1278,7 +1320,7 @@ update_dynamic_addresses(struct ovn_datapath *od, > mac = update->static_mac; > break; > case DYNAMIC: > - eth_addr_from_uint64(ipam_get_unused_mac(), &mac); > + eth_addr_from_uint64(ipam_get_unused_mac(od), &mac); > break; > } > > @@ -1390,8 +1432,8 @@ build_ipam(struct hmap *datapaths, struct hmap *ports) > extract_lsp_addresses(nbsp->dynamic_addresses, > &update->current_addresses); > any_changed = dynamic_addresses_check_for_updates( > - nbsp->addresses[j], update); > - update_unchanged_dynamic_addresses(update); > + od, nbsp->addresses[j], update); > + update_unchanged_dynamic_addresses(od, update); > if (any_changed) { > ovs_list_push_back(&updates, &update->node); > } else { > 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]) > >
> > Hi Lorenzo, > > I think for the patch, I'm willing to ack it, so: > > Acked-by: Mark Michelson <mmichels@redhat.com> > > However, I have some minor suggestions below that might be worth making > a second version of the patch for. Thx for the review Mark, I will address your comments and send a v2 Regards, Lorenzo
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c index 439651f80..3e8a4a276 100644 --- a/ovn/northd/ovn-northd.c +++ b/ovn/northd/ovn-northd.c @@ -404,6 +404,9 @@ struct ipam_info { unsigned long *allocated_ipv4s; /* A bitmap of allocated IPv4s */ bool ipv6_prefix_set; struct in6_addr ipv6_prefix; + bool mac_prefix_set; + struct eth_addr mac_prefix; + }; /* The 'key' comes from nbs->header_.uuid or nbr->header_.uuid or @@ -534,7 +537,8 @@ lrouter_is_enabled(const struct nbrec_logical_router *lrouter) } static void -init_ipam_info_for_datapath(struct ovn_datapath *od) +init_ipam_info_for_datapath(struct ovn_datapath *od, + struct northd_context *ctx) { if (!od->nbs) { return; @@ -543,11 +547,27 @@ init_ipam_info_for_datapath(struct ovn_datapath *od) const char *subnet_str = smap_get(&od->nbs->other_config, "subnet"); const char *ipv6_prefix = smap_get(&od->nbs->other_config, "ipv6_prefix"); + const struct nbrec_nb_global *nb = nbrec_nb_global_first(ctx->ovnnb_idl); + const char *mac_prefix = smap_get(&nb->options, "mac_prefix"); + if (ipv6_prefix) { od->ipam_info.ipv6_prefix_set = ipv6_parse( ipv6_prefix, &od->ipam_info.ipv6_prefix); } + if (mac_prefix) { + struct eth_addr addr; + + memset(&addr, 0, sizeof addr); + if (ovs_scan(mac_prefix, "%"SCNx8":%"SCNx8":%"SCNx8, + &addr.ea[0], &addr.ea[1], &addr.ea[2])) { + od->ipam_info.mac_prefix_set = true; + od->ipam_info.mac_prefix = addr; + } else { + od->ipam_info.mac_prefix_set = false; + } + } + if (!subnet_str) { return; } @@ -703,7 +723,7 @@ join_datapaths(struct northd_context *ctx, struct hmap *datapaths, ovs_list_push_back(nb_only, &od->list); } - init_ipam_info_for_datapath(od); + init_ipam_info_for_datapath(od, ctx); } const struct nbrec_logical_router *nbr; @@ -915,17 +935,24 @@ ipam_is_duplicate_mac(struct eth_addr *ea, uint64_t mac64, bool warn) } static void -ipam_insert_mac(struct eth_addr *ea, bool check) +ipam_insert_mac(struct ovn_datapath *od, struct eth_addr *ea, bool check) { if (!ea) { return; } uint64_t mac64 = eth_addr_to_uint64(*ea); + uint64_t prefix; + + if (od->ipam_info.mac_prefix_set) { + prefix = eth_addr_to_uint64(od->ipam_info.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; } @@ -970,7 +997,7 @@ ipam_insert_lsp_addresses(struct ovn_datapath *od, struct ovn_port *op, VLOG_WARN_RL(&rl, "Extract addresses failed."); return; } - ipam_insert_mac(&laddrs.ea, true); + ipam_insert_mac(od, &laddrs.ea, true); /* IP is only added to IPAM if the switch's subnet option * is set, whereas MAC is always added to MACAM. */ @@ -1007,7 +1034,7 @@ ipam_add_port_addresses(struct ovn_datapath *od, struct ovn_port *op) VLOG_WARN_RL(&rl, "Extract addresses failed."); return; } - ipam_insert_mac(&lrp_networks.ea, true); + ipam_insert_mac(od, &lrp_networks.ea, true); if (!op->peer || !op->peer->nbsp || !op->peer->od || !op->peer->od->nbs || !smap_get(&op->peer->od->nbs->other_config, "subnet")) { @@ -1025,7 +1052,7 @@ ipam_add_port_addresses(struct ovn_datapath *od, struct ovn_port *op) } static uint64_t -ipam_get_unused_mac(void) +ipam_get_unused_mac(struct ovn_datapath *od) { /* Stores the suffix of the most recently ipam-allocated MAC address. */ static uint32_t last_mac; @@ -1036,7 +1063,12 @@ 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 (od->ipam_info.mac_prefix_set) { + mac64 = eth_addr_to_uint64(od->ipam_info.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; @@ -1091,7 +1123,7 @@ struct dynamic_address_update { }; static enum dynamic_update_type -dynamic_mac_changed(const char *lsp_addresses, +dynamic_mac_changed(struct ovn_datapath *od, const char *lsp_addresses, struct dynamic_address_update *update) { struct eth_addr ea; @@ -1107,7 +1139,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 (od->ipam_info.mac_prefix_set) { + prefix = eth_addr_to_uint64(od->ipam_info.mac_prefix); + } else { + prefix = MAC_ADDR_PREFIX; + } + + if ((mac64 ^ prefix) >> 24) { return DYNAMIC; } else { return NONE; @@ -1195,10 +1235,11 @@ dynamic_ip6_changed(struct dynamic_address_update *update) * Returns true if any changes to dynamic addresses are required */ static bool -dynamic_addresses_check_for_updates(const char *lsp_addrs, +dynamic_addresses_check_for_updates(struct ovn_datapath *od, + const char *lsp_addrs, struct dynamic_address_update *update) { - update->mac = dynamic_mac_changed(lsp_addrs, update); + update->mac = dynamic_mac_changed(od, lsp_addrs, update); update->ipv4 = dynamic_ip4_changed(update); update->ipv6 = dynamic_ip6_changed(update); if (update->mac == NONE && @@ -1215,10 +1256,11 @@ dynamic_addresses_check_for_updates(const char *lsp_addrs, * elsewhere later. */ static void -update_unchanged_dynamic_addresses(struct dynamic_address_update *update) +update_unchanged_dynamic_addresses(struct ovn_datapath *od, + struct dynamic_address_update *update) { if (update->mac == NONE) { - ipam_insert_mac(&update->current_addresses.ea, false); + ipam_insert_mac(od, &update->current_addresses.ea, false); } if (update->ipv4 == NONE && update->current_addresses.n_ipv4_addrs) { ipam_insert_ip(update->op->od, @@ -1278,7 +1320,7 @@ update_dynamic_addresses(struct ovn_datapath *od, mac = update->static_mac; break; case DYNAMIC: - eth_addr_from_uint64(ipam_get_unused_mac(), &mac); + eth_addr_from_uint64(ipam_get_unused_mac(od), &mac); break; } @@ -1390,8 +1432,8 @@ build_ipam(struct hmap *datapaths, struct hmap *ports) extract_lsp_addresses(nbsp->dynamic_addresses, &update->current_addresses); any_changed = dynamic_addresses_check_for_updates( - nbsp->addresses[j], update); - update_unchanged_dynamic_addresses(update); + od, nbsp->addresses[j], update); + update_unchanged_dynamic_addresses(od, update); if (any_changed) { ovs_list_push_back(&updates, &update->node); } else { 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])
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 Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> --- ovn/northd/ovn-northd.c | 76 ++++++++++++++++++++++++++++++++--------- ovn/ovn-nb.xml | 5 +++ tests/ovn.at | 17 +++++++++ 3 files changed, 81 insertions(+), 17 deletions(-)