Hi Ales Thanks for the patch.
Two comments about the flakiness below. LGTM Acked-by: Xavier Simonart <[email protected]> On Wed, Feb 25, 2026 at 7:13 AM Ales Musil via dev <[email protected]> wrote: > On Tue, Feb 24, 2026 at 5:00 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 > > Signed-off-by: Ales Musil <[email protected]> > > --- > > northd/ipam.c | 79 ++++++++++++++------------------------------- > > northd/ipam.h | 4 ++- > > northd/northd.c | 12 ++++++- > > tests/ovn-macros.at | 11 +++++++ > > tests/ovn-northd.at | 31 ++++++++++++++++++ > > tests/ovn.at | 16 ++++----- > > 6 files changed, 89 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..a7cfc15ea 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -19902,3 +19902,34 @@ 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 > > + sleep_northd > This looks to me like the cause of the flakiness: there is no guarantee that northd has run here. It might have gone to sleep before handling the dynamic lsp-address. We had similar issues in tests where ovn-controller woke up and slept. We just used 'ovn-appctl -t ovn-controller debug/status' twice (between wake_up and sleep) to ensure the loop ran fully at least once. So adding two lines such as OVS_WAIT_UNTIL([test "x$(ovn-appctl -t northd/ovn-northd status)" = "xStatus: active"]) should fix the flakiness, I think. > > + > > + 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 > > > > Unfortunately, this test seems a bit flaky, it's probably > fine to remove it as the changes below should still prove > correctness. I'll wait for the reviews before removing the > test though. > If the proposed change eliminates the flakiness, I would keep the test as the changes below do not obviously prove correctness. But if flakiness is still present, I would not investigate further. Anyhow, I'll let you decide. > > 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 > > > > > Thanks Xavier > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
