On Wed, Feb 25, 2026 at 2:38 PM Xavier Simonart <[email protected]> wrote:
> Hi Ales > > Thanks for the patch. > > Two comments about the flakiness below. > LGTM > Acked-by: Xavier Simonart <[email protected]> > Thank you for the review Xavier. > > 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. > That sounds reasonable, I'll give it a try. > > + >> > + 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. > Well it does prove it because the MAC is derived from the address, if the address is assigned twice the MAC would still be derived from the old value. The 01:03 is a great example of that. But I will definitely try the proposed solution. > >> 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 > > Thanks, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
