On Wed, Feb 25, 2026 at 4:17 PM Ales Musil <[email protected]> wrote:
> The IPAM is keeping track of used addresses for given LS to avoid
> conflicts and potentially assigning already used address to the
> next LSP. However during I-P the IPAM was destroyed and created
> again from scratch without adding back the already used IPs. Make
> sure we add those IPs back.
>
> Fixes: 31c704a388a7 ("northd: Incremental processing of LSPs IPAM in
> "northd" node.")
> Reported-at: https://issues.redhat.com/browse/FDP-2982
> Acked-by: Xavier Simonart <[email protected]>
> Signed-off-by: Ales Musil <[email protected]>
> ---
> v2: Rebase on top of latest main.
> Update the test according to suggestion from Xavier.
> Add Xavier's ack.
> ---
> northd/ipam.c | 79 ++++++++++++++-------------------------------
> northd/ipam.h | 4 ++-
> northd/northd.c | 12 ++++++-
> tests/ovn-macros.at | 11 +++++++
> tests/ovn-northd.at | 37 +++++++++++++++++++++
> tests/ovn.at | 16 ++++-----
> 6 files changed, 95 insertions(+), 64 deletions(-)
>
> diff --git a/northd/ipam.c b/northd/ipam.c
> index a1801e554..5f80649c5 100644
> --- a/northd/ipam.c
> +++ b/northd/ipam.c
> @@ -24,11 +24,6 @@ static void init_ipam_ipv4(const char *subnet_str,
> static bool ipam_is_duplicate_mac(struct eth_addr *ea, uint64_t mac64,
> bool warn);
>
> -static void ipam_insert_ip_for_datapath(struct ovn_datapath *, uint32_t,
> bool);
> -
> -static void ipam_insert_lsp_addresses(struct ovn_datapath *,
> - struct lport_addresses *);
> -
> static enum dynamic_update_type dynamic_mac_changed(const char *,
> struct dynamic_address_update *);
>
> @@ -109,17 +104,6 @@ ipam_insert_ip(struct ipam_info *info, uint32_t ip,
> bool dynamic)
> return true;
> }
>
> -static void
> -ipam_insert_ip_for_datapath(struct ovn_datapath *od, uint32_t ip,
> - bool dynamic)
> -{
> - if (!od) {
> - return;
> - }
> -
> - ipam_insert_ip(&od->ipam_info, ip, dynamic);
> -}
> -
> uint32_t
> ipam_get_unused_ip(struct ipam_info *info)
> {
> @@ -568,7 +552,7 @@ update_unchanged_dynamic_addresses(struct
> dynamic_address_update *update)
> ipam_insert_mac(&update->current_addresses.ea, false);
> }
> if (update->ipv4 == NONE && update->current_addresses.n_ipv4_addrs) {
> - ipam_insert_ip_for_datapath(update->op->od,
> + ipam_insert_ip(&update->op->od->ipam_info,
>
> ntohl(update->current_addresses.ipv4_addrs[0].addr),
> true);
> }
> @@ -698,7 +682,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), true);
> + ipam_insert_ip(&update->od->ipam_info, ntohl(ip4), true);
> ds_put_format(&new_addr, " "IP_FMT, IP_ARGS(ip4));
> }
> if (!IN6_ARE_ADDR_EQUAL(&ip6, &in6addr_any)) {
> @@ -783,54 +767,41 @@ update_ipam_ls(struct ovn_datapath *od, struct
> vector *updates,
> }
> }
>
> -void
> -ipam_add_port_addresses(struct ovn_datapath *od, struct ovn_port *op)
> +static void
> +ipam_add_lport_addresses(struct ipam_info *info,
> + const struct lport_addresses *laddrs)
> {
> - if (!od || !op) {
> - return;
> + for (size_t j = 0; j < laddrs->n_ipv4_addrs; j++) {
> + ipam_insert_ip(info, ntohl(laddrs->ipv4_addrs[j].addr), false);
> }
> +}
> +
> +void
> +ipam_add_lsp_port_addresses(struct ovn_port *op, bool check_mac_duplicate)
> +{
> + ovs_assert(op->nbsp);
> +
> + struct ipam_info *info = &op->od->ipam_info;
>
> if (op->n_lsp_non_router_addrs) {
> /* Add all the port's addresses to address data structures. */
> for (size_t i = 0; i < op->n_lsp_non_router_addrs; i++) {
> - ipam_insert_lsp_addresses(od, &op->lsp_addrs[i]);
> - }
> - } else if (op->lrp_networks.ea_s[0]) {
> - ipam_insert_mac(&op->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")) {
> - return;
> + ipam_insert_mac(&op->lsp_addrs[i].ea, check_mac_duplicate);
> + ipam_add_lport_addresses(info, &op->lsp_addrs[i]);
> }
> + }
>
> - for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
> - uint32_t ip = ntohl(op->lrp_networks.ipv4_addrs[i].addr);
> - /* If the router has the first IP address of the subnet,
> don't add
> - * it to IPAM. We already added this when we initialized IPAM
> for
> - * the datapath. This will just result in an erroneous message
> - * about a duplicate IP address.
> - */
> - if (ip != op->peer->od->ipam_info.start_ipv4) {
> - ipam_insert_ip_for_datapath(op->peer->od, ip, false);
> - }
> - }
> + if (op->peer && op->peer->nbrp) {
> + ipam_add_lport_addresses(info, &op->peer->lrp_networks);
> }
> }
>
> -static void
> -ipam_insert_lsp_addresses(struct ovn_datapath *od,
> - struct lport_addresses *laddrs)
> +void
> +ipam_add_lrp_port_addresses(struct ovn_port *op)
> {
> - ipam_insert_mac(&laddrs->ea, true);
> + ovs_assert(op->nbrp);
>
> - /* IP is only added to IPAM if the switch's subnet option
> - * is set, whereas MAC is always added to MACAM. */
> - if (!od->ipam_info.allocated_ipv4s) {
> - return;
> - }
> -
> - 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, false);
> + if (op->lrp_networks.ea_s[0]) {
> + ipam_insert_mac(&op->lrp_networks.ea, true);
> }
> }
> diff --git a/northd/ipam.h b/northd/ipam.h
> index c825875a8..24dd23232 100644
> --- a/northd/ipam.h
> +++ b/northd/ipam.h
> @@ -69,6 +69,8 @@ void update_ipam_ls(struct ovn_datapath *, struct vector
> *, bool);
>
> void update_dynamic_addresses(struct dynamic_address_update *);
>
> -void ipam_add_port_addresses(struct ovn_datapath *, struct ovn_port *);
> +void ipam_add_lrp_port_addresses(struct ovn_port *);
> +
> +void ipam_add_lsp_port_addresses(struct ovn_port *, bool
> check_mac_duplicate);
>
> #endif /* NORTHD_IPAM_H */
> diff --git a/northd/northd.c b/northd/northd.c
> index 983975dac..b1371e4bf 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -2034,7 +2034,11 @@ join_logical_ports(const struct
> sbrec_port_binding_table *sbrec_pb_table,
> * it relies on proper peers to be set
> */
> HMAP_FOR_EACH (op, key_node, ports) {
> - ipam_add_port_addresses(op->od, op);
> + if (op->nbsp) {
> + ipam_add_lsp_port_addresses(op, true);
> + } else if (op->nbrp) {
> + ipam_add_lrp_port_addresses(op);
> + }
> }
> }
>
> @@ -5217,6 +5221,12 @@ bool northd_handle_ipam_changes(struct northd_data
> *nd)
> od->ipam_info_initialized = false;
> }
> init_ipam_info_for_datapath(od);
> +
> + struct ovn_port *op;
> + HMAP_FOR_EACH (op, dp_node, &od->ports) {
> + ipam_add_lsp_port_addresses(op, false);
> + }
> +
> update_ipam_ls(od, &updates, false);
> }
>
> diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at
> index 8e77be3af..aeb4149d5 100644
> --- a/tests/ovn-macros.at
> +++ b/tests/ovn-macros.at
> @@ -1442,6 +1442,17 @@ restart_ovsdb_controller_updates() {
> AT_CHECK([nft delete table ip ovn-test])
> }
>
> +sleep_nb() {
> + echo NB going to sleep
> + AT_CHECK([kill -STOP $(cat ovn-nb/ovsdb-server.pid)])
> + on_exit "test -e ovn-nb/ovsdb-server.pid && kill -CONT $(cat
> ovn-nb/ovsdb-server.pid)"
> +}
> +
> +wake_up_nb() {
> + echo NB waking up
> + AT_CHECK([kill -CONT $(cat ovn-nb/ovsdb-server.pid)])
> +}
> +
> trim_zeros() {
> sed 's/\(00\)\{1,\}$//'
> }
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index e29f6d7b5..a8b5436ba 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -19902,3 +19902,40 @@ check ovn-nbctl --wait=sb lsp-set-addresses
> lport1 "f0:00:0f:01:02:04 192.168.0.
> check_row_count sb:Service_Monitor 2
>
> AT_CLEANUP
> +
> +AT_SETUP([ipam router ports - incremental processing])
> +ovn_start
> +check ovn-nbctl ls-add sw
> +check ovn-nbctl set logical_switch sw other-config:subnet=192.168.1.0/24
> +for i in 2 3 4; do
> + check ovn-nbctl lr-add ro$i
> + check ovn-nbctl lsp-add sw swp$i
> + check ovn-nbctl --wait=sb sync
> +
> + sleep_northd
> + check ovn-nbctl lsp-set-addresses swp$i "02:00:00:00:00:0$i dynamic"
> + sleep_nb
> +
> +
> + wake_up_northd
> + # Make sure that the LSP change is handled by northd.
> + # Without this, northd could just go to sleep before it has the chance
> + # to process the new LSP. Use 2 ovn-appctl to guarantee that northd
> runs
> + # the full loop, and not just the unixctl handling.
> + OVS_WAIT_UNTIL([test "$(as northd ovn-appctl -t ovn-northd status)" =
> "Status: active"])
> + OVS_WAIT_UNTIL([test "$(as northd ovn-appctl -t ovn-northd status)" =
> "Status: active"])
> + sleep_northd
> +
> + wake_up_nb
> + cidr=$(ovn-nbctl get logical_switch_port swp$i dynamic_addresses |cut
> -f2 -d' '|cut -f1 -d\")
> + wake_up_northd
> +
> + check ovn-nbctl --wait=sb lrp-add ro$i rop$i 02:00:00:00:00:0$i
> $cidr/24 \
> + -- set logical_switch_port swp$i type=router \
> + options:router-port=rop$i addresses=router
> + AT_CHECK_UNQUOTED([ovn-nbctl get logical_router_port rop$i networks],
> [0], [[["192.168.1.$i/24"]]
> +])
> +done
> +
> +OVN_CLEANUP_NORTHD
> +AT_CLEANUP
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 9082bba82..979c6d192 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -9335,7 +9335,7 @@ done
> # and addresses set by the user.
> check ovn-nbctl lsp-set-addresses p0 "0a:00:00:a8:01:17 192.168.1.2
> 192.168.1.12 192.168.1.14"
> check ovn-nbctl --wait=sb lsp-add sw0 p20 -- lsp-set-addresses p20 dynamic
> -check_dynamic_addresses p20 "0a:00:00:a8:01:03 192.168.1.13"
> +check_dynamic_addresses p20 "0a:00:00:a8:01:18 192.168.1.13"
>
> # Test for logical router port address management.
> check_uuid ovn-nbctl create Logical_Router name=R1
> @@ -9344,12 +9344,12 @@ network="192.168.1.1/24"
> mac=\"0a:00:00:a8:01:19\" \
> -- add Logical_Router R1 ports @lrp -- lsp-add sw0 rp-sw0 \
> -- set Logical_Switch_Port rp-sw0 type=router options:router-port=sw0
> check ovn-nbctl --wait=sb lsp-add sw0 p21 -- lsp-set-addresses p21 dynamic
> -check_dynamic_addresses p21 "0a:00:00:a8:01:18 192.168.1.15"
> +check_dynamic_addresses p21 "0a:00:00:a8:01:1a 192.168.1.15"
>
> # Test for address reuse after logical port is deleted.
> check ovn-nbctl lsp-del p0
> check ovn-nbctl --wait=sb lsp-add sw0 p23 -- lsp-set-addresses p23 dynamic
> -check_dynamic_addresses p23 "0a:00:00:a8:01:1a 192.168.1.2"
> +check_dynamic_addresses p23 "0a:00:00:a8:01:03 192.168.1.2"
>
> # Test for multiple addresses to one logical port.
> check ovn-nbctl lsp-add sw0 p25 -- lsp-set-addresses p25 \
> @@ -9409,19 +9409,19 @@ check_dynamic_addresses p33 "0a:00:00:a8:01:1f
> 192.168.1.22"
> check ovn-nbctl --wait=sb lsp-add sw0 p34 -- lsp-set-addresses p34 \
> "dynamic"
> # 192.168.1.51 should be assigned as 192.168.1.23-192.168.1.50 is
> excluded.
> -check_dynamic_addresses p34 "0a:00:00:a8:01:20 192.168.1.51"
> +check_dynamic_addresses p34 "0a:00:00:a8:01:34 192.168.1.51"
>
> # Now clear the exclude_ips list. 192.168.1.19 should be assigned.
> check ovn-nbctl --wait=sb set Logical-switch sw0
> other_config:exclude_ips="invalid"
> check ovn-nbctl --wait=sb lsp-add sw0 p35 -- lsp-set-addresses p35
> "dynamic"
> -check_dynamic_addresses p35 "0a:00:00:a8:01:21 192.168.1.19"
> +check_dynamic_addresses p35 "0a:00:00:a8:01:20 192.168.1.19"
>
> # Set invalid data in exclude_ips list. It should be ignored.
> check ovn-nbctl --wait=sb set Logical-switch sw0
> other_config:exclude_ips="182.168.1.30"
> check ovn-nbctl --wait=sb lsp-add sw0 p36 -- lsp-set-addresses p36 \
> "dynamic"
> # 192.168.1.21 should be assigned as that's the next free one.
> -check_dynamic_addresses p36 "0a:00:00:a8:01:22 192.168.1.21"
> +check_dynamic_addresses p36 "0a:00:00:a8:01:21 192.168.1.21"
>
> # Clear the dynamic addresses assignment request.
> check ovn-nbctl --wait=sb clear logical_switch_port p36 addresses
> @@ -9433,7 +9433,7 @@ check ovn-nbctl --wait=sb lsp-add sw0 p37 --
> lsp-set-addresses p37 "dynamic"
>
> # With prefix aef0 and mac 0a:00:00:00:00:26, the dynamic IPv6 should be
> # - aef0::800:ff:fe00:26 (EUI64)
> -check_dynamic_addresses p37 "0a:00:00:a8:01:22 192.168.1.21
> aef0::800:ff:fea8:122"
> +check_dynamic_addresses p37 "0a:00:00:a8:01:21 192.168.1.21
> aef0::800:ff:fea8:121"
>
> check ovn-nbctl --wait=sb ls-add sw4
> check ovn-nbctl --wait=sb set Logical-switch sw4
> other_config:ipv6_prefix="bef0::" \
> @@ -9463,7 +9463,7 @@ check_dynamic_addresses p41 ''
>
> # Set a subnet. Now p41 should have an ipv4 address, too
> check ovn-nbctl --wait=sb add Logical-Switch sw5 other_config subnet=
> 192.168.1.0/24
> -check_dynamic_addresses <http://192.168.1.0/24-check_dynamic_addresses>
> p41 "0a:00:00:a8:01:23 192.168.1.2"
> +check_dynamic_addresses p41 "0a:00:00:a8:01:22 192.168.1.2"
>
> # Clear the other_config. The IPv4 address should be gone
> check ovn-nbctl --wait=sb clear Logical-Switch sw5 other_config
> --
> 2.53.0
>
>
Thank you Xavier,
I went ahead, merged this into main and backported it down to 25.09.
Regards,
Ales
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
