Message ID | 3ea0c31c625993d8670cfc6e01aecb7af4647d6a.1601473450.git.lorenzo.bianconi@redhat.com |
---|---|
State | Superseded |
Headers | show |
Series | [ovs-dev] northd: properly reconfigure ipam when subnet is changed | expand |
On 9/30/20 9:46 AM, Lorenzo Bianconi wrote: > Reconfigure dynamic assigned addresses if subnet is modified > removing IP out of configured netmask if present > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > --- > northd/ovn-northd.c | 48 ++++++++++++++++++++++++++------------------- > tests/ovn.at | 10 ++++++++++ > 2 files changed, 38 insertions(+), 20 deletions(-) > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index 5fddc5e3e..e527e90fa 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -1665,24 +1665,6 @@ ipam_get_unused_mac(ovs_be32 ip) > return mac64; > } > > -static uint32_t > -ipam_get_unused_ip(struct ovn_datapath *od) > -{ > - if (!od || !od->ipam_info.allocated_ipv4s) { > - return 0; > - } > - > - size_t new_ip_index = bitmap_scan(od->ipam_info.allocated_ipv4s, 0, 0, > - od->ipam_info.total_ipv4s - 1); > - if (new_ip_index == od->ipam_info.total_ipv4s - 1) { > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > - VLOG_WARN_RL( &rl, "Subnet address space has been exhausted."); > - return 0; > - } > - > - return od->ipam_info.start_ipv4 + new_ip_index; > -} > - > enum dynamic_update_type { > NONE, /* No change to the address */ > REMOVE, /* Address is no longer dynamic */ > @@ -1705,6 +1687,32 @@ struct dynamic_address_update { > enum dynamic_update_type ipv6; > }; > > +static uint32_t > +ipam_get_unused_ip(struct dynamic_address_update *update) > +{ > + struct ovn_datapath *od = update->od; > + > + if (!od || !od->ipam_info.allocated_ipv4s) { > + return 0; > + } > + > + size_t new_ip_index = bitmap_scan(od->ipam_info.allocated_ipv4s, 0, 0, > + od->ipam_info.total_ipv4s - 1); > + if (new_ip_index == od->ipam_info.total_ipv4s - 1) { > + const struct nbrec_logical_switch_port *nbsp = update->op->nbsp; > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > + VLOG_WARN_RL( &rl, "Subnet address space has been exhausted."); > + > + if (nbsp->dynamic_addresses) { > + nbrec_logical_switch_port_set_dynamic_addresses(nbsp, NULL); > + } Adding this side effect to ipam_get_unused_ip() makes it less clear what this function's purpose is. Before this change, this function would attempt to get the next unused IP and return it. Now, it does that but it can also change the logical switch port dynamic addresses. In the case where you're setting the dynamic addresses, the function already returns a "bad" value of 0. The caller of ipam_get_unused_ip() should be able to use that return value to set the dynamic addresses of the switch port appropriately. I think update_dynamic_addresses() is the only caller of ipam_get_unused_ip(). update_dynamic_addresses() already is attempting to set dynamic_addresses on the switch port. It probably just needs to be adjusted to set the dynamic_addresses NULL in the case where update_dynamic_addresses() does not get a valid IPv4 or IPv6 address to use. > + > + return 0; > + } > + > + return od->ipam_info.start_ipv4 + new_ip_index; > +} > + > static enum dynamic_update_type > dynamic_mac_changed(const char *lsp_addresses, > struct dynamic_address_update *update) > @@ -1758,7 +1766,7 @@ dynamic_ip4_changed(const char *lsp_addrs, > } > > uint32_t index = ip4 - ipam->start_ipv4; > - if (index > ipam->total_ipv4s || > + if (index >= ipam->total_ipv4s - 1 || > bitmap_is_set(ipam->allocated_ipv4s, index)) { > /* Previously assigned dynamic IPv4 address can no longer be used. > * It's either outside the subnet, conflicts with an excluded IP, > @@ -1964,7 +1972,7 @@ update_dynamic_addresses(struct dynamic_address_update *update) > ip4 = update->static_ip; > break; > case DYNAMIC: > - ip4 = htonl(ipam_get_unused_ip(update->od)); > + ip4 = htonl(ipam_get_unused_ip(update)); > VLOG_INFO("Assigned dynamic IPv4 address '"IP_FMT"' to port '%s'", > IP_ARGS(ip4), update->op->nbsp->name); > } > diff --git a/tests/ovn.at b/tests/ovn.at > index 7769b69ed..e27a74081 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -7473,6 +7473,16 @@ AT_CHECK([ovn-nbctl get Logical-Switch-Port p103 dynamic_addresses], [0], > ["22:33:44:55:66:77 172.16.1.250" > ]) > > +ovn-nbctl ls-add sw12 > +for i in $(seq 1 200); do > + ovn-nbctl lsp-add sw12 sw12-p$i -- lsp-set-addresses sw12-p$i "00:00:00:00:00:$i dynamic" > +done > +ovn-nbctl set Logical-Switch sw12 other_config:subnet=192.10.2.0/24 > +ovn-nbctl set Logical-Switch sw12 other_config:subnet=192.10.2.0/25 > +AT_CHECK([ovn-nbctl list Logical-Switch-Port | grep -q 192.10.2.127], [1]) > +AT_CHECK([ovn-nbctl list Logical-Switch-Port | grep -q 192.10.2.128], [1]) > +AT_CHECK([ovn-nbctl list Logical-Switch-Port | grep -q 192.10.2.180], [1]) > + > as ovn-sb > OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > >
> > On 9/30/20 9:46 AM, Lorenzo Bianconi wrote: [...] > > Adding this side effect to ipam_get_unused_ip() makes it less clear what > this function's purpose is. Before this change, this function would > attempt to get the next unused IP and return it. Now, it does that but > it can also change the logical switch port dynamic addresses. > > In the case where you're setting the dynamic addresses, the function > already returns a "bad" value of 0. The caller of ipam_get_unused_ip() > should be able to use that return value to set the dynamic addresses of > the switch port appropriately. > > I think update_dynamic_addresses() is the only caller of > ipam_get_unused_ip(). update_dynamic_addresses() already is attempting > to set dynamic_addresses on the switch port. It probably just needs to > be adjusted to set the dynamic_addresses NULL in the case where > update_dynamic_addresses() does not get a valid IPv4 or IPv6 address to use. > Hi Mark, thx for the review. Ack, I will fix it in v2. Regards, Lorenzo > > + > > + return 0; > > + } > > + > > + return od->ipam_info.start_ipv4 + new_ip_index; > > +} > > + > > static enum dynamic_update_type > > dynamic_mac_changed(const char *lsp_addresses, > > struct dynamic_address_update *update) > > @@ -1758,7 +1766,7 @@ dynamic_ip4_changed(const char *lsp_addrs, > > } > > > > uint32_t index = ip4 - ipam->start_ipv4; > > - if (index > ipam->total_ipv4s || > > + if (index >= ipam->total_ipv4s - 1 || > > bitmap_is_set(ipam->allocated_ipv4s, index)) { > > /* Previously assigned dynamic IPv4 address can no longer be used. > > * It's either outside the subnet, conflicts with an excluded IP, > > @@ -1964,7 +1972,7 @@ update_dynamic_addresses(struct dynamic_address_update *update) > > ip4 = update->static_ip; > > break; > > case DYNAMIC: > > - ip4 = htonl(ipam_get_unused_ip(update->od)); > > + ip4 = htonl(ipam_get_unused_ip(update)); > > VLOG_INFO("Assigned dynamic IPv4 address '"IP_FMT"' to port '%s'", > > IP_ARGS(ip4), update->op->nbsp->name); > > } > > diff --git a/tests/ovn.at b/tests/ovn.at > > index 7769b69ed..e27a74081 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -7473,6 +7473,16 @@ AT_CHECK([ovn-nbctl get Logical-Switch-Port p103 dynamic_addresses], [0], > > ["22:33:44:55:66:77 172.16.1.250" > > ]) > > > > +ovn-nbctl ls-add sw12 > > +for i in $(seq 1 200); do > > + ovn-nbctl lsp-add sw12 sw12-p$i -- lsp-set-addresses sw12-p$i "00:00:00:00:00:$i dynamic" > > +done > > +ovn-nbctl set Logical-Switch sw12 other_config:subnet=192.10.2.0/24 > > +ovn-nbctl set Logical-Switch sw12 other_config:subnet=192.10.2.0/25 > > +AT_CHECK([ovn-nbctl list Logical-Switch-Port | grep -q 192.10.2.127], [1]) > > +AT_CHECK([ovn-nbctl list Logical-Switch-Port | grep -q 192.10.2.128], [1]) > > +AT_CHECK([ovn-nbctl list Logical-Switch-Port | grep -q 192.10.2.180], [1]) > > + > > as ovn-sb > > OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > > > > >
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index 5fddc5e3e..e527e90fa 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -1665,24 +1665,6 @@ ipam_get_unused_mac(ovs_be32 ip) return mac64; } -static uint32_t -ipam_get_unused_ip(struct ovn_datapath *od) -{ - if (!od || !od->ipam_info.allocated_ipv4s) { - return 0; - } - - size_t new_ip_index = bitmap_scan(od->ipam_info.allocated_ipv4s, 0, 0, - od->ipam_info.total_ipv4s - 1); - if (new_ip_index == od->ipam_info.total_ipv4s - 1) { - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); - VLOG_WARN_RL( &rl, "Subnet address space has been exhausted."); - return 0; - } - - return od->ipam_info.start_ipv4 + new_ip_index; -} - enum dynamic_update_type { NONE, /* No change to the address */ REMOVE, /* Address is no longer dynamic */ @@ -1705,6 +1687,32 @@ struct dynamic_address_update { enum dynamic_update_type ipv6; }; +static uint32_t +ipam_get_unused_ip(struct dynamic_address_update *update) +{ + struct ovn_datapath *od = update->od; + + if (!od || !od->ipam_info.allocated_ipv4s) { + return 0; + } + + size_t new_ip_index = bitmap_scan(od->ipam_info.allocated_ipv4s, 0, 0, + od->ipam_info.total_ipv4s - 1); + if (new_ip_index == od->ipam_info.total_ipv4s - 1) { + const struct nbrec_logical_switch_port *nbsp = update->op->nbsp; + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); + VLOG_WARN_RL( &rl, "Subnet address space has been exhausted."); + + if (nbsp->dynamic_addresses) { + nbrec_logical_switch_port_set_dynamic_addresses(nbsp, NULL); + } + + return 0; + } + + return od->ipam_info.start_ipv4 + new_ip_index; +} + static enum dynamic_update_type dynamic_mac_changed(const char *lsp_addresses, struct dynamic_address_update *update) @@ -1758,7 +1766,7 @@ dynamic_ip4_changed(const char *lsp_addrs, } uint32_t index = ip4 - ipam->start_ipv4; - if (index > ipam->total_ipv4s || + if (index >= ipam->total_ipv4s - 1 || bitmap_is_set(ipam->allocated_ipv4s, index)) { /* Previously assigned dynamic IPv4 address can no longer be used. * It's either outside the subnet, conflicts with an excluded IP, @@ -1964,7 +1972,7 @@ update_dynamic_addresses(struct dynamic_address_update *update) ip4 = update->static_ip; break; case DYNAMIC: - ip4 = htonl(ipam_get_unused_ip(update->od)); + ip4 = htonl(ipam_get_unused_ip(update)); VLOG_INFO("Assigned dynamic IPv4 address '"IP_FMT"' to port '%s'", IP_ARGS(ip4), update->op->nbsp->name); } diff --git a/tests/ovn.at b/tests/ovn.at index 7769b69ed..e27a74081 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -7473,6 +7473,16 @@ AT_CHECK([ovn-nbctl get Logical-Switch-Port p103 dynamic_addresses], [0], ["22:33:44:55:66:77 172.16.1.250" ]) +ovn-nbctl ls-add sw12 +for i in $(seq 1 200); do + ovn-nbctl lsp-add sw12 sw12-p$i -- lsp-set-addresses sw12-p$i "00:00:00:00:00:$i dynamic" +done +ovn-nbctl set Logical-Switch sw12 other_config:subnet=192.10.2.0/24 +ovn-nbctl set Logical-Switch sw12 other_config:subnet=192.10.2.0/25 +AT_CHECK([ovn-nbctl list Logical-Switch-Port | grep -q 192.10.2.127], [1]) +AT_CHECK([ovn-nbctl list Logical-Switch-Port | grep -q 192.10.2.128], [1]) +AT_CHECK([ovn-nbctl list Logical-Switch-Port | grep -q 192.10.2.180], [1]) + as ovn-sb OVS_APP_EXIT_AND_WAIT([ovsdb-server])
Reconfigure dynamic assigned addresses if subnet is modified removing IP out of configured netmask if present Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> --- northd/ovn-northd.c | 48 ++++++++++++++++++++++++++------------------- tests/ovn.at | 10 ++++++++++ 2 files changed, 38 insertions(+), 20 deletions(-)