On 07/06/2018 06:36 PM, Ben Pfaff wrote:
On Mon, Jun 25, 2018 at 04:09:53PM -0400, Mark Michelson wrote:
OVN offers a method of IP address management that allows for an IPv4 subnet or
IPv6 prefix to be specified on a logical switch. Then by specifying a
switch port's address as "dynamic" or "<mac address> dynamic", OVN will
automatically assign addresses to the switch port.
While this works great for initial assignment of addresses, addresses do
not automatically adjust when changes are made to the switch's
configuration. For instance:
* If the subnet, ipv6_prefix, or exclude_ips for a logical switch
changes, the affected switch ports are not updated.
* If a switch port with a static IP address is added to the switch, and
that address conflicts with a dynamically assigned IP address, the
dynamic address is not updated.
* If a MAC address switched from being statically assigned to
dynamically assigned, the MAC address would not be updated.
* If a statically assigned MAC address changed, then the IPv6 address
would not be updated.
This patch solves all of the above issues by changing the algorithm for
IPAM assignment. There are essentially three steps.
1) While joining logical ports, all statically-assigned addresses (i.e.
any ports without "dynamic" addresses) have their addresses registered
to IPAM. This gives them top priority.
2) All logical ports with dynamic addresses are inspected. Any changes
that must be made to the addresses are collected to be made later. Any
addresses that do not require change are registered to IPAM. This allows
for previously assigned dynamic addresses to be kept.
3) All gathered changes are enacted.
The change contains new tests that ensure that dynamic addresses are
updated when appropriate.
This patch also alters some existing IPAM tests. Those tests assumed
that dynamic addresses would not be updated automatically, so those
tests either had to be altered or removed.
Signed-off-by: Mark Michelson <[email protected]>
---
v2->v3:
Fixed a checkpatch problem (line too long)
v1->v2:
Rebased
Thanks for the new version.
I spent some time trying to understand this. I think one of my issues
with it is that there is an unstated assumption that a logical switch
port has at most one dynamic address, but nothing that checks for or
enforces it. It's possible to request more than one if you put multiple
"xx:xx:xx:xx:xx:xx dynamic" entries in an addresses column, but I don't
think that the code really treats them properly since it tries to
independently diff each one of them against the current set of
dynamically assigned addresses. Is this all correct? If so, would you
figure out some way to fix it?
You're definitely correct that there's an assumption that there is only
a single dynamic address. However, this assumption is not introduced by
my patch. I've done a quick test on a vagrant system using current master:
[root@central vagrant]# ovn-nbctl ls-add switch
[root@central vagrant]# ovn-nbctl lsp-add switch port1
[root@central vagrant]# ovn-nbctl lsp-set-addresses port1
"00:00:00:00:00:01 dynamic" "00:00:00:00:00:02 dynamic"
[root@central vagrant]# ovn-nbctl list logical_switch_port
_uuid : 6b880956-eb8b-49fc-bb0e-cfaa08360cc0
addresses : ["00:00:00:00:00:01 dynamic", "00:00:00:00:00:02
dynamic"]
dhcpv4_options : []
dhcpv6_options : []
dynamic_addresses : []
enabled : []
external_ids : {}
name : "port1"
options : {}
parent_name : []
port_security : []
tag : []
tag_request : []
type : ""
up : false
[root@central vagrant]# ovn-nbctl set Logical_Switch switch
other_config:subnet=10.0.0.0/8
[root@central vagrant]# ovn-nbctl list logical_switch_port
_uuid : 6b880956-eb8b-49fc-bb0e-cfaa08360cc0
addresses : ["00:00:00:00:00:01 dynamic", "00:00:00:00:00:02
dynamic"]
dhcpv4_options : []
dhcpv6_options : []
dynamic_addresses : "00:00:00:00:00:02 10.0.0.2"
enabled : []
external_ids : {}
name : "port1"
options : {}
parent_name : []
port_security : []
tag : []
tag_request : []
type : ""
up : false
So I guess the question is, should there only be a single dynamic
address assigned per switch port? If so, I guess some sort of warning
should be logged if multiple dynamic addresses are requested? Or should
we allow for multiple dynamic addresses to be assigned per switch port?
I'm appending some incrementals that make sense to me. I removed 'od'
from the struct because it's redundant with op->od. I moved the
ovs_list member to the top because that's where we usually put a member
used to show membership in a larger structure, and renamed it from
'list' to 'node' because this ovs_list is used for membership not for
containment. I also added a check in build_ipam() that op->nbsp ==
nbsp, which I think the code assumed and might be ovs_assert()'able but
at least checking on it seems wise.
If this makes sense, would you mind working on a v4?
These incrementals all look good to me. Before doing v4 though, I'll
need to make sure about the matter discussed above.
Thanks,
Ben.
--8<--------------------------cut here-------------------------->8--
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 12c9929da039..80eb0e6bcb3a 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -1065,10 +1065,11 @@ enum dynamic_update_type {
};
struct dynamic_address_update {
+ struct ovs_list node; /* In build_ipam()'s list of updates. */
+
struct ovn_port *op;
- struct ovn_datapath *od;
+
struct lport_addresses current_addresses;
- struct ovs_list list;
struct eth_addr static_mac;
enum dynamic_update_type mac;
enum dynamic_update_type ipv4;
@@ -1102,7 +1103,7 @@ dynamic_mac_changed(const char *lsp_addresses,
static enum dynamic_update_type
dynamic_ip4_changed(struct dynamic_address_update *update)
{
- bool dynamic_ip4 = update->od->ipam_info.allocated_ipv4s != NULL;
+ bool dynamic_ip4 = update->op->od->ipam_info.allocated_ipv4s != NULL;
if (!dynamic_ip4) {
if (update->current_addresses.n_ipv4_addrs) {
@@ -1118,13 +1119,13 @@ dynamic_ip4_changed(struct dynamic_address_update
*update)
}
uint32_t ip4 = ntohl(update->current_addresses.ipv4_addrs[0].addr);
- if (ip4 < update->od->ipam_info.start_ipv4) {
+ if (ip4 < update->op->od->ipam_info.start_ipv4) {
return DYNAMIC;
}
- uint32_t index = ip4 - update->od->ipam_info.start_ipv4;
- if (index > update->od->ipam_info.total_ipv4s ||
- bitmap_is_set(update->od->ipam_info.allocated_ipv4s, index)) {
+ uint32_t index = ip4 - update->op->od->ipam_info.start_ipv4;
+ if (index > update->op->od->ipam_info.total_ipv4s ||
+ bitmap_is_set(update->op->od->ipam_info.allocated_ipv4s, index)) {
/* Previously assigned dynamic IPv4 address can no longer be used.
* It's either outside the subnet, conflicts with an excluded IP,
* or conflicts with a statically-assigned address on the switch
@@ -1138,7 +1139,7 @@ dynamic_ip4_changed(struct dynamic_address_update *update)
static enum dynamic_update_type
dynamic_ip6_changed(struct dynamic_address_update *update)
{
- bool dynamic_ip6 = update->od->ipam_info.ipv6_prefix_set;
+ bool dynamic_ip6 = update->op->od->ipam_info.ipv6_prefix_set;
if (!dynamic_ip6) {
if (update->current_addresses.n_ipv6_addrs) {
@@ -1164,8 +1165,8 @@ dynamic_ip6_changed(struct dynamic_address_update *update)
struct in6_addr masked = ipv6_addr_bitand(
&update->current_addresses.ipv6_addrs[0].addr,
- &update->od->ipam_info.ipv6_prefix);
- if (!IN6_ARE_ADDR_EQUAL(&masked, &update->od->ipam_info.ipv6_prefix)) {
+ &update->op->od->ipam_info.ipv6_prefix);
+ if (!IN6_ARE_ADDR_EQUAL(&masked, &update->op->od->ipam_info.ipv6_prefix)) {
return DYNAMIC;
}
@@ -1191,7 +1192,7 @@ dynamic_addresses_check_for_updates(const char *lsp_addrs,
ipam_insert_mac(&update->current_addresses.ea, false);
}
if (update->ipv4 == NONE && update->current_addresses.n_ipv4_addrs) {
- ipam_insert_ip(update->od,
+ ipam_insert_ip(update->op->od,
ntohl(update->current_addresses.ipv4_addrs[0].addr));
}
if (update->mac == NONE &&
@@ -1228,12 +1229,12 @@ set_dynamic_updates(const char *addrspec,
} else {
update->mac = DYNAMIC;
}
- if (update->od->ipam_info.allocated_ipv4s) {
+ if (update->op->od->ipam_info.allocated_ipv4s) {
update->ipv4 = DYNAMIC;
} else {
update->ipv4 = NONE;
}
- if (update->od->ipam_info.ipv6_prefix_set) {
+ if (update->op->od->ipam_info.ipv6_prefix_set) {
update->ipv6 = DYNAMIC;
} else {
update->ipv6 = NONE;
@@ -1323,16 +1324,11 @@ build_ipam(struct hmap *datapaths, struct hmap *ports)
if (!od->nbs) {
continue;
}
- struct dynamic_address_update *update;
+
struct ovs_list updates;
ovs_list_init(&updates);
for (size_t i = 0; i < od->nbs->n_ports; i++) {
- const struct nbrec_logical_switch_port *nbsp =
- od->nbs->ports[i];
-
- if (!nbsp) {
- continue;
- }
+ const struct nbrec_logical_switch_port *nbsp = od->nbs->ports[i];
if (!od->ipam_info.allocated_ipv4s &&
!od->ipam_info.ipv6_prefix_set) {
@@ -1344,7 +1340,7 @@ build_ipam(struct hmap *datapaths, struct hmap *ports)
}
struct ovn_port *op = ovn_port_find(ports, nbsp->name);
- if (!op || (op->nbsp && op->peer)) {
+ if (!op || op->nbsp != nbsp || op->peer) {
/* Do not allocate addresses for logical switch ports that
* have a peer. */
continue;
@@ -1354,15 +1350,15 @@ build_ipam(struct hmap *datapaths, struct hmap *ports)
if (!is_dynamic_lsp_address(nbsp->addresses[j])) {
continue;
}
- update = xzalloc(sizeof *update);
- update->od = od;
+ struct dynamic_address_update *update
+ = xzalloc(sizeof *update);
update->op = op;
if (nbsp->dynamic_addresses) {
- extract_lsp_addresses(op->nbsp->dynamic_addresses,
+ extract_lsp_addresses(nbsp->dynamic_addresses,
&update->current_addresses);
if
(dynamic_addresses_check_for_updates(nbsp->addresses[j],
update)) {
- ovs_list_push_back(&updates, &update->list);
+ ovs_list_push_back(&updates, &update->node);
} else {
/* No changes to dynamic addresses */
set_lsp_dynamic_addresses(nbsp->dynamic_addresses,
op);
@@ -1371,20 +1367,20 @@ build_ipam(struct hmap *datapaths, struct hmap *ports)
}
} else {
set_dynamic_updates(nbsp->addresses[j], update);
- ovs_list_push_back(&updates, &update->list);
+ ovs_list_push_back(&updates, &update->node);
}
}
if (!nbsp->n_addresses && nbsp->dynamic_addresses) {
- nbrec_logical_switch_port_set_dynamic_addresses(op->nbsp,
- NULL);
+ nbrec_logical_switch_port_set_dynamic_addresses(nbsp, NULL);
}
}
/* After retaining all unchanged dynamic addresses, now assign
* new ones.
*/
- LIST_FOR_EACH_POP (update, list, &updates) {
+ struct dynamic_address_update *update;
+ LIST_FOR_EACH_POP (update, node, &updates) {
update_dynamic_addresses(od, update);
destroy_lport_addresses(&update->current_addresses);
free(update);
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
