| Message ID | ecbc96444afa6e565ffe28ae3447e6b39d355e22.1726567092.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] ipam: Do not report error for static assigned IPs. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
Aside from some typos in the commit message ("staticatically" and
"swith"), this looks good to me. Thanks Lorenzo!
Acked-by: Mark Michelson <mmichels@redhat.com>
On 9/17/24 06:20, Lorenzo Bianconi wrote:
> Do not report error in ipam_insert_ip routine for addresses
> staticatically assigned to ovn logical {swith,router} ports since they
> have precedence on addressed dynamically assigned by ipam.
>
> Reported-at: https://issues.redhat.com/browse/FDP-752
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> ---
> northd/ipam.c | 6 +++---
> northd/ipam.h | 2 +-
> northd/northd.c | 13 +++++++------
> northd/test-ipam.c | 2 +-
> 4 files changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/northd/ipam.c b/northd/ipam.c
> index 4448a7607..04fa357a5 100644
> --- a/northd/ipam.c
> +++ b/northd/ipam.c
> @@ -48,7 +48,7 @@ destroy_ipam_info(struct ipam_info *info)
> }
>
> bool
> -ipam_insert_ip(struct ipam_info *info, uint32_t ip)
> +ipam_insert_ip(struct ipam_info *info, uint32_t ip, bool dynamic)
> {
> if (!info->allocated_ipv4s) {
> return true;
> @@ -56,8 +56,8 @@ ipam_insert_ip(struct ipam_info *info, uint32_t ip)
>
> if (ip >= info->start_ipv4 &&
> ip < (info->start_ipv4 + info->total_ipv4s)) {
> - if (bitmap_is_set(info->allocated_ipv4s,
> - ip - info->start_ipv4)) {
> + if (dynamic && bitmap_is_set(info->allocated_ipv4s,
> + ip - info->start_ipv4)) {
> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> VLOG_WARN_RL(&rl, "%s: Duplicate IP set: " IP_FMT,
> info->id, IP_ARGS(htonl(ip)));
> diff --git a/northd/ipam.h b/northd/ipam.h
> index 447412769..6240f9ab7 100644
> --- a/northd/ipam.h
> +++ b/northd/ipam.h
> @@ -22,7 +22,7 @@ void init_ipam_info(struct ipam_info *info, const struct smap *config,
>
> void destroy_ipam_info(struct ipam_info *info);
>
> -bool ipam_insert_ip(struct ipam_info *info, uint32_t ip);
> +bool ipam_insert_ip(struct ipam_info *info, uint32_t ip, bool dynamic);
>
> uint32_t ipam_get_unused_ip(struct ipam_info *info);
>
> diff --git a/northd/northd.c b/northd/northd.c
> index a267cd5f8..95c3f0e07 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -1464,13 +1464,13 @@ ovn_port_get_peer(const struct hmap *lr_ports, struct ovn_port *op)
> }
>
> static void
> -ipam_insert_ip_for_datapath(struct ovn_datapath *od, uint32_t ip)
> +ipam_insert_ip_for_datapath(struct ovn_datapath *od, uint32_t ip, bool dynamic)
> {
> if (!od) {
> return;
> }
>
> - ipam_insert_ip(&od->ipam_info, ip);
> + ipam_insert_ip(&od->ipam_info, ip, dynamic);
> }
>
> static void
> @@ -1487,7 +1487,7 @@ ipam_insert_lsp_addresses(struct ovn_datapath *od,
>
> for (size_t j = 0; j < laddrs->n_ipv4_addrs; j++) {
> uint32_t ip = ntohl(laddrs->ipv4_addrs[j].addr);
> - ipam_insert_ip_for_datapath(od, ip);
> + ipam_insert_ip_for_datapath(od, ip, false);
> }
> }
>
> @@ -1519,7 +1519,7 @@ ipam_add_port_addresses(struct ovn_datapath *od, struct ovn_port *op)
> * about a duplicate IP address.
> */
> if (ip != op->peer->od->ipam_info.start_ipv4) {
> - ipam_insert_ip_for_datapath(op->peer->od, ip);
> + ipam_insert_ip_for_datapath(op->peer->od, ip, false);
> }
> }
> }
> @@ -1744,7 +1744,8 @@ update_unchanged_dynamic_addresses(struct dynamic_address_update *update)
> }
> if (update->ipv4 == NONE && update->current_addresses.n_ipv4_addrs) {
> ipam_insert_ip_for_datapath(update->op->od,
> - ntohl(update->current_addresses.ipv4_addrs[0].addr));
> + ntohl(update->current_addresses.ipv4_addrs[0].addr),
> + true);
> }
> }
>
> @@ -1872,7 +1873,7 @@ update_dynamic_addresses(struct dynamic_address_update *update)
> ipam_insert_mac(&mac, true);
>
> if (ip4) {
> - ipam_insert_ip_for_datapath(update->od, ntohl(ip4));
> + ipam_insert_ip_for_datapath(update->od, ntohl(ip4), true);
> ds_put_format(&new_addr, " "IP_FMT, IP_ARGS(ip4));
> }
> if (!IN6_ARE_ADDR_EQUAL(&ip6, &in6addr_any)) {
> diff --git a/northd/test-ipam.c b/northd/test-ipam.c
> index c449599e3..3b6ad1891 100644
> --- a/northd/test-ipam.c
> +++ b/northd/test-ipam.c
> @@ -44,7 +44,7 @@ test_ipam_get_unused_ip(struct ovs_cmdl_context *ctx)
> uint32_t next_ip = ipam_get_unused_ip(&info);
> ds_put_format(&output, IP_FMT "\n", IP_ARGS(htonl(next_ip)));
> if (next_ip) {
> - ovs_assert(ipam_insert_ip(&info, next_ip));
> + ovs_assert(ipam_insert_ip(&info, next_ip, true));
> }
> }
>
On Wed, Oct 16, 2024 at 4:13 PM Mark Michelson <mmichels@redhat.com> wrote: > > Aside from some typos in the commit message ("staticatically" and > "swith"), this looks good to me. Thanks Lorenzo! > > Acked-by: Mark Michelson <mmichels@redhat.com> Thanks. I corrected the typos and applied the patch to main and backported till 23.09. Numan > > On 9/17/24 06:20, Lorenzo Bianconi wrote: > > Do not report error in ipam_insert_ip routine for addresses > > staticatically assigned to ovn logical {swith,router} ports since they > > have precedence on addressed dynamically assigned by ipam. > > > > Reported-at: https://issues.redhat.com/browse/FDP-752 > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > --- > > northd/ipam.c | 6 +++--- > > northd/ipam.h | 2 +- > > northd/northd.c | 13 +++++++------ > > northd/test-ipam.c | 2 +- > > 4 files changed, 12 insertions(+), 11 deletions(-) > > > > diff --git a/northd/ipam.c b/northd/ipam.c > > index 4448a7607..04fa357a5 100644 > > --- a/northd/ipam.c > > +++ b/northd/ipam.c > > @@ -48,7 +48,7 @@ destroy_ipam_info(struct ipam_info *info) > > } > > > > bool > > -ipam_insert_ip(struct ipam_info *info, uint32_t ip) > > +ipam_insert_ip(struct ipam_info *info, uint32_t ip, bool dynamic) > > { > > if (!info->allocated_ipv4s) { > > return true; > > @@ -56,8 +56,8 @@ ipam_insert_ip(struct ipam_info *info, uint32_t ip) > > > > if (ip >= info->start_ipv4 && > > ip < (info->start_ipv4 + info->total_ipv4s)) { > > - if (bitmap_is_set(info->allocated_ipv4s, > > - ip - info->start_ipv4)) { > > + if (dynamic && bitmap_is_set(info->allocated_ipv4s, > > + ip - info->start_ipv4)) { > > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > > VLOG_WARN_RL(&rl, "%s: Duplicate IP set: " IP_FMT, > > info->id, IP_ARGS(htonl(ip))); > > diff --git a/northd/ipam.h b/northd/ipam.h > > index 447412769..6240f9ab7 100644 > > --- a/northd/ipam.h > > +++ b/northd/ipam.h > > @@ -22,7 +22,7 @@ void init_ipam_info(struct ipam_info *info, const struct smap *config, > > > > void destroy_ipam_info(struct ipam_info *info); > > > > -bool ipam_insert_ip(struct ipam_info *info, uint32_t ip); > > +bool ipam_insert_ip(struct ipam_info *info, uint32_t ip, bool dynamic); > > > > uint32_t ipam_get_unused_ip(struct ipam_info *info); > > > > diff --git a/northd/northd.c b/northd/northd.c > > index a267cd5f8..95c3f0e07 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -1464,13 +1464,13 @@ ovn_port_get_peer(const struct hmap *lr_ports, struct ovn_port *op) > > } > > > > static void > > -ipam_insert_ip_for_datapath(struct ovn_datapath *od, uint32_t ip) > > +ipam_insert_ip_for_datapath(struct ovn_datapath *od, uint32_t ip, bool dynamic) > > { > > if (!od) { > > return; > > } > > > > - ipam_insert_ip(&od->ipam_info, ip); > > + ipam_insert_ip(&od->ipam_info, ip, dynamic); > > } > > > > static void > > @@ -1487,7 +1487,7 @@ ipam_insert_lsp_addresses(struct ovn_datapath *od, > > > > for (size_t j = 0; j < laddrs->n_ipv4_addrs; j++) { > > uint32_t ip = ntohl(laddrs->ipv4_addrs[j].addr); > > - ipam_insert_ip_for_datapath(od, ip); > > + ipam_insert_ip_for_datapath(od, ip, false); > > } > > } > > > > @@ -1519,7 +1519,7 @@ ipam_add_port_addresses(struct ovn_datapath *od, struct ovn_port *op) > > * about a duplicate IP address. > > */ > > if (ip != op->peer->od->ipam_info.start_ipv4) { > > - ipam_insert_ip_for_datapath(op->peer->od, ip); > > + ipam_insert_ip_for_datapath(op->peer->od, ip, false); > > } > > } > > } > > @@ -1744,7 +1744,8 @@ update_unchanged_dynamic_addresses(struct dynamic_address_update *update) > > } > > if (update->ipv4 == NONE && update->current_addresses.n_ipv4_addrs) { > > ipam_insert_ip_for_datapath(update->op->od, > > - ntohl(update->current_addresses.ipv4_addrs[0].addr)); > > + ntohl(update->current_addresses.ipv4_addrs[0].addr), > > + true); > > } > > } > > > > @@ -1872,7 +1873,7 @@ update_dynamic_addresses(struct dynamic_address_update *update) > > ipam_insert_mac(&mac, true); > > > > if (ip4) { > > - ipam_insert_ip_for_datapath(update->od, ntohl(ip4)); > > + ipam_insert_ip_for_datapath(update->od, ntohl(ip4), true); > > ds_put_format(&new_addr, " "IP_FMT, IP_ARGS(ip4)); > > } > > if (!IN6_ARE_ADDR_EQUAL(&ip6, &in6addr_any)) { > > diff --git a/northd/test-ipam.c b/northd/test-ipam.c > > index c449599e3..3b6ad1891 100644 > > --- a/northd/test-ipam.c > > +++ b/northd/test-ipam.c > > @@ -44,7 +44,7 @@ test_ipam_get_unused_ip(struct ovs_cmdl_context *ctx) > > uint32_t next_ip = ipam_get_unused_ip(&info); > > ds_put_format(&output, IP_FMT "\n", IP_ARGS(htonl(next_ip))); > > if (next_ip) { > > - ovs_assert(ipam_insert_ip(&info, next_ip)); > > + ovs_assert(ipam_insert_ip(&info, next_ip, true)); > > } > > } > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/northd/ipam.c b/northd/ipam.c index 4448a7607..04fa357a5 100644 --- a/northd/ipam.c +++ b/northd/ipam.c @@ -48,7 +48,7 @@ destroy_ipam_info(struct ipam_info *info) } bool -ipam_insert_ip(struct ipam_info *info, uint32_t ip) +ipam_insert_ip(struct ipam_info *info, uint32_t ip, bool dynamic) { if (!info->allocated_ipv4s) { return true; @@ -56,8 +56,8 @@ ipam_insert_ip(struct ipam_info *info, uint32_t ip) if (ip >= info->start_ipv4 && ip < (info->start_ipv4 + info->total_ipv4s)) { - if (bitmap_is_set(info->allocated_ipv4s, - ip - info->start_ipv4)) { + if (dynamic && bitmap_is_set(info->allocated_ipv4s, + ip - info->start_ipv4)) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); VLOG_WARN_RL(&rl, "%s: Duplicate IP set: " IP_FMT, info->id, IP_ARGS(htonl(ip))); diff --git a/northd/ipam.h b/northd/ipam.h index 447412769..6240f9ab7 100644 --- a/northd/ipam.h +++ b/northd/ipam.h @@ -22,7 +22,7 @@ void init_ipam_info(struct ipam_info *info, const struct smap *config, void destroy_ipam_info(struct ipam_info *info); -bool ipam_insert_ip(struct ipam_info *info, uint32_t ip); +bool ipam_insert_ip(struct ipam_info *info, uint32_t ip, bool dynamic); uint32_t ipam_get_unused_ip(struct ipam_info *info); diff --git a/northd/northd.c b/northd/northd.c index a267cd5f8..95c3f0e07 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -1464,13 +1464,13 @@ ovn_port_get_peer(const struct hmap *lr_ports, struct ovn_port *op) } static void -ipam_insert_ip_for_datapath(struct ovn_datapath *od, uint32_t ip) +ipam_insert_ip_for_datapath(struct ovn_datapath *od, uint32_t ip, bool dynamic) { if (!od) { return; } - ipam_insert_ip(&od->ipam_info, ip); + ipam_insert_ip(&od->ipam_info, ip, dynamic); } static void @@ -1487,7 +1487,7 @@ ipam_insert_lsp_addresses(struct ovn_datapath *od, for (size_t j = 0; j < laddrs->n_ipv4_addrs; j++) { uint32_t ip = ntohl(laddrs->ipv4_addrs[j].addr); - ipam_insert_ip_for_datapath(od, ip); + ipam_insert_ip_for_datapath(od, ip, false); } } @@ -1519,7 +1519,7 @@ ipam_add_port_addresses(struct ovn_datapath *od, struct ovn_port *op) * about a duplicate IP address. */ if (ip != op->peer->od->ipam_info.start_ipv4) { - ipam_insert_ip_for_datapath(op->peer->od, ip); + ipam_insert_ip_for_datapath(op->peer->od, ip, false); } } } @@ -1744,7 +1744,8 @@ update_unchanged_dynamic_addresses(struct dynamic_address_update *update) } if (update->ipv4 == NONE && update->current_addresses.n_ipv4_addrs) { ipam_insert_ip_for_datapath(update->op->od, - ntohl(update->current_addresses.ipv4_addrs[0].addr)); + ntohl(update->current_addresses.ipv4_addrs[0].addr), + true); } } @@ -1872,7 +1873,7 @@ update_dynamic_addresses(struct dynamic_address_update *update) ipam_insert_mac(&mac, true); if (ip4) { - ipam_insert_ip_for_datapath(update->od, ntohl(ip4)); + ipam_insert_ip_for_datapath(update->od, ntohl(ip4), true); ds_put_format(&new_addr, " "IP_FMT, IP_ARGS(ip4)); } if (!IN6_ARE_ADDR_EQUAL(&ip6, &in6addr_any)) { diff --git a/northd/test-ipam.c b/northd/test-ipam.c index c449599e3..3b6ad1891 100644 --- a/northd/test-ipam.c +++ b/northd/test-ipam.c @@ -44,7 +44,7 @@ test_ipam_get_unused_ip(struct ovs_cmdl_context *ctx) uint32_t next_ip = ipam_get_unused_ip(&info); ds_put_format(&output, IP_FMT "\n", IP_ARGS(htonl(next_ip))); if (next_ip) { - ovs_assert(ipam_insert_ip(&info, next_ip)); + ovs_assert(ipam_insert_ip(&info, next_ip, true)); } }
Do not report error in ipam_insert_ip routine for addresses staticatically assigned to ovn logical {swith,router} ports since they have precedence on addressed dynamically assigned by ipam. Reported-at: https://issues.redhat.com/browse/FDP-752 Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> --- northd/ipam.c | 6 +++--- northd/ipam.h | 2 +- northd/northd.c | 13 +++++++------ northd/test-ipam.c | 2 +- 4 files changed, 12 insertions(+), 11 deletions(-)