On Wed, Jul 23, 2025 at 3:21 PM Lucas Vargas Dias via dev <
ovs-dev@openvswitch.org> wrote:
> Handle the changes from LSP with dynamic addreses when its Logical Switch
> has mac, subnet or ipv6 prefix configured.
> Before, all logical switches were iterated even when they don't have
> changes.
>
> Test with 30000 LS and 70000 LSPs creating a LSP with dynamic address:
> ovn-nbctl --print-wait-time --wait=sb lsp-add -- lsp-set-addresses
> dynamic
>
> Without the I-P:
> Time spent on processing nb_cfg 272726:
> ovn-northd delay before processing: 10ms
> ovn-northd completion: 21422ms
>
> With the I-P:
> Time spent on processing nb_cfg 272733:
> ovn-northd delay before processing: 20ms
> ovn-northd completion: 101ms
>
> Signed-off-by: Lucas Vargas Dias <lucas.vd...@luizalabs.com>
> ---
>
Hi Lucas,
thank you for the v2, I have additional comments below.
> northd/en-northd.c | 5 +++++
> northd/ipam.c | 7 +++---
> northd/northd.c | 54 ++++++++++++++++++++++++++++++++++++++++++++--
> northd/northd.h | 6 ++++++
> tests/ovn.at | 16 +++++++-------
> 5 files changed, 75 insertions(+), 13 deletions(-)
>
> diff --git a/northd/en-northd.c b/northd/en-northd.c
> index 8402ab943..b0ce6cfab 100644
> --- a/northd/en-northd.c
> +++ b/northd/en-northd.c
> @@ -157,6 +157,11 @@ northd_nb_logical_switch_handler(struct engine_node
> *node,
> }
>
> if (northd_has_tracked_data(&nd->trk_data)) {
> + if (northd_has_lsps_in_tracked_data(&nd->trk_data) &&
> + !hmapx_is_empty(&nd->trk_data.ls_with_changed_ipam) &&
> + !northd_handle_ipam_changes(nd)) {
> + return EN_HANDLED_UNCHANGED;
> + }
>
This can be simplified to:
bool ipam_update = northd_handle_ipam_changes(nd);
if (northd_has_tracked_data(&nd->trk_data) || ipam_update) {
return EN_HANDLED_UPDATED;
}
> return EN_HANDLED_UPDATED;
> }
>
> diff --git a/northd/ipam.c b/northd/ipam.c
> index 863f61afa..087c29aaa 100644
> --- a/northd/ipam.c
> +++ b/northd/ipam.c
> @@ -71,13 +71,14 @@ init_ipam_info(struct ipam_info *info, const struct
> smap *config, const char *id
> void
> init_ipam_info_for_datapath(struct ovn_datapath *od)
> {
> - if (!od->nbs) {
> + if (!od->nbs || od->ipam_info_initialized) {
> return;
> }
>
> char uuid_s[UUID_LEN + 1];
> sprintf(uuid_s, UUID_FMT, UUID_ARGS(&od->key));
> init_ipam_info(&od->ipam_info, &od->nbs->other_config, uuid_s);
> + od->ipam_info_initialized = true;
> }
>
> void
> @@ -740,7 +741,7 @@ void update_ipam_ls(struct ovn_datapath *od, struct
> hmap *ls_ports,
> continue;
> }
>
> - int num_dynamic_addresses = 0;
> + bool num_dynamic_addresses = false;
> for (size_t j = 0; j < nbsp->n_addresses; j++) {
> if (!is_dynamic_lsp_address(nbsp->addresses[j])) {
> continue;
> @@ -753,7 +754,7 @@ void update_ipam_ls(struct ovn_datapath *od, struct
> hmap *ls_ports,
> nbsp->name);
> continue;
> }
> - num_dynamic_addresses++;
> + num_dynamic_addresses = true;
> struct dynamic_address_update update;
> update.op = op;
> update.od = od;
> diff --git a/northd/northd.c b/northd/northd.c
> index 890ac6e43..0622e8ff8 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -504,6 +504,7 @@ ovn_datapath_create(struct hmap *datapaths, const
> struct uuid *key,
> od->l3dgw_ports = VECTOR_EMPTY_INITIALIZER(struct ovn_port *);
> od->localnet_ports = VECTOR_EMPTY_INITIALIZER(struct ovn_port *);
> od->lb_with_stateless_mode = false;
> + od->ipam_info_initialized = false;
> return od;
> }
>
> @@ -918,6 +919,10 @@ join_datapaths(const struct
> nbrec_logical_switch_table *nbrec_ls_table,
> ovs_list_remove(&od->list);
> ovs_list_push_back(both, &od->list);
> ovn_datapath_update_external_ids(od);
> + if (od->ipam_info_initialized) {
> + destroy_ipam_info(&od->ipam_info);
> + od->ipam_info_initialized = false;
> + }
> } else {
> od = ovn_datapath_create(datapaths, &nbs->header_.uuid,
> nbs, NULL, NULL);
> @@ -4139,6 +4144,7 @@ destroy_northd_data_tracked_changes(struct
> northd_data *nd)
> hmapx_clear(&trk_changes->trk_nat_lrs);
> hmapx_clear(&trk_changes->ls_with_changed_lbs);
> hmapx_clear(&trk_changes->ls_with_changed_acls);
> + hmapx_clear(&trk_changes->ls_with_changed_ipam);
> trk_changes->type = NORTHD_TRACKED_NONE;
> }
>
> @@ -4155,6 +4161,7 @@ init_northd_tracked_data(struct northd_data *nd)
> hmapx_init(&trk_data->trk_nat_lrs);
> hmapx_init(&trk_data->ls_with_changed_lbs);
> hmapx_init(&trk_data->ls_with_changed_acls);
> + hmapx_init(&trk_data->ls_with_changed_ipam);
> }
>
> static void
> @@ -4170,6 +4177,7 @@ destroy_northd_tracked_data(struct northd_data *nd)
> hmapx_destroy(&trk_data->trk_nat_lrs);
> hmapx_destroy(&trk_data->ls_with_changed_lbs);
> hmapx_destroy(&trk_data->ls_with_changed_acls);
> + hmapx_destroy(&trk_data->ls_with_changed_ipam);
> }
>
> /* Check if a changed LSP can be handled incrementally within the I-P
> engine
> @@ -4196,8 +4204,9 @@ lsp_can_be_inc_processed(const struct
> nbrec_logical_switch_port *nbsp)
> }
>
> for (size_t j = 0; j < nbsp->n_addresses; j++) {
> - /* Dynamic address handling is not supported for now. */
> - if (is_dynamic_lsp_address(nbsp->addresses[j])) {
> + /* Dynamic address was assigned in the last iteration. */
> + if (is_dynamic_lsp_address(nbsp->addresses[j]) &&
> + nbsp->dynamic_addresses) {
> return false;
> }
> /* "unknown" address handling is not supported for now. XXX:
> Need to
> @@ -4665,6 +4674,13 @@ northd_handle_ls_changes(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
> if (is_ls_acls_changed(changed_ls)) {
> hmapx_add(&trk_data->ls_with_changed_acls, od);
> }
> + init_ipam_info_for_datapath(od);
> + bool ls_has_ipam = od->ipam_info.allocated_ipv4s ||
> + od->ipam_info.ipv6_prefix_set ||
> + od->ipam_info.mac_only;
> + if (ls_has_ipam) {
> + hmapx_add(&trk_data->ls_with_changed_ipam, od);
> + }
> }
>
> if (!hmapx_is_empty(&trk_data->trk_lsps.created)
> @@ -4746,6 +4762,40 @@ fail:
> return false;
> }
>
> +bool northd_handle_ipam_changes(struct northd_data *nd)
> +{
>
You can add the check for updates here to avoid it in the handler above:
struct northd_tracked_data *nd_changes = &nd->trk_data;
if (hmapx_is_empty(&nd_changes->ls_with_changed_ipam)) {
return false;
}
> + struct northd_tracked_data *nd_changes = &nd->trk_data;
> + struct vector updates =
> + VECTOR_EMPTY_INITIALIZER(struct dynamic_address_update);
> +
> + struct hmapx_node *hmapx_node;
> + HMAPX_FOR_EACH (hmapx_node, &nd_changes->ls_with_changed_ipam) {
> + struct ovn_datapath *od = hmapx_node->data;
> + if (od->ipam_info_initialized) {
> + destroy_ipam_info(&od->ipam_info);
> + od->ipam_info_initialized = false;
> + }
> + init_ipam_info_for_datapath(od);
> + update_ipam_ls(od, &nd->ls_ports, &updates, false);
> + }
>
nit: Please add an empty line.
> + struct dynamic_address_update *update;
>
nit: Please move the update right above the FOR_EACH loop.
> + size_t lsps_changed = 0;
>
This can be changed to bool.
> + VECTOR_FOR_EACH_PTR (&updates, update) {
> + if (hmapx_find(&nd_changes->trk_lsps.updated, update->op) ||
> + hmapx_find(&nd_changes->trk_lsps.created, update->op)) {
> + update_dynamic_addresses(update);
> + lsps_changed++;
> + }
> + destroy_lport_addresses(&update->current_addresses);
> + }
> + vector_destroy(&updates);
> +
> + if (!lsps_changed) {
> + return false;
> + }
>
If 'lsps_changed' is bool you can return it right away avoiding the if:
return lsps_changed;
> + return true;
>
+}
> +
> /* Returns true if the logical router has changes which can be
> * incrementally handled.
> * Presently supports i-p for the below changes:
> diff --git a/northd/northd.h b/northd/northd.h
> index b1bf5cf9f..7eb2931c8 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -161,6 +161,10 @@ struct northd_tracked_data {
> /* Tracked logical switches whose ACLs have changed.
> * hmapx node is 'struct ovn_datapath *'. */
> struct hmapx ls_with_changed_acls;
> +
> + /* Tracked logical switches with IPAM whose LSPs have changed.
> + * hmapx node is 'struct ovn_datapath *'. */
> + struct hmapx ls_with_changed_ipam;
> };
>
> struct northd_data {
> @@ -386,6 +390,7 @@ struct ovn_datapath {
>
> /* IPAM data. */
> struct ipam_info ipam_info;
> + bool ipam_info_initialized;
>
> /* Multicast data. */
> struct mcast_info mcast_info;
> @@ -838,6 +843,7 @@ bool northd_handle_lr_changes(const struct
> northd_input *,
> struct northd_data *);
> bool northd_handle_pgs_acl_changes(const struct northd_input *ni,
> struct northd_data *nd);
> +bool northd_handle_ipam_changes(struct northd_data *nd);
> void destroy_northd_data_tracked_changes(struct northd_data *);
> void northd_destroy(struct northd_data *data);
> void northd_init(struct northd_data *data);
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 108de260e..264dfebbc 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -9218,7 +9218,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:18 192.168.1.13"
> +check_dynamic_addresses p20 "0a:00:00:a8:01:03 192.168.1.13"
>
> # Test for logical router port address management.
> check_uuid ovn-nbctl create Logical_Router name=R1
> @@ -9227,12 +9227,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:1a 192.168.1.15"
> +check_dynamic_addresses p21 "0a:00:00:a8:01:18 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:03 192.168.1.2"
> +check_dynamic_addresses p23 "0a:00:00:a8:01:1a 192.168.1.2"
>
> # Test for multiple addresses to one logical port.
> check ovn-nbctl lsp-add sw0 p25 -- lsp-set-addresses p25 \
> @@ -9292,19 +9292,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:34 192.168.1.51"
> +check_dynamic_addresses p34 "0a:00:00:a8:01:20 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:20 192.168.1.19"
> +check_dynamic_addresses p35 "0a:00:00:a8:01:21 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:21 192.168.1.21"
> +check_dynamic_addresses p36 "0a:00:00:a8:01:22 192.168.1.21"
>
> # Clear the dynamic addresses assignment request.
> check ovn-nbctl --wait=sb clear logical_switch_port p36 addresses
> @@ -9316,7 +9316,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:21 192.168.1.21
> aef0::800:ff:fea8:121"
> +check_dynamic_addresses p37 "0a:00:00:a8:01:22 192.168.1.21
> aef0::800:ff:fea8:122"
>
> check ovn-nbctl --wait=sb ls-add sw4
> check ovn-nbctl --wait=sb set Logical-switch sw4
> other_config:ipv6_prefix="bef0::" \
> @@ -9346,7 +9346,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:22 192.168.1.2"
> +check_dynamic_addresses p41 "0a:00:00:a8:01:23 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.34.1
>
>
> --
>
>
>
>
> _'Esta mensagem é direcionada apenas para os endereços constantes no
> cabeçalho inicial. Se você não está listado nos endereços constantes no
> cabeçalho, pedimos-lhe que desconsidere completamente o conteúdo dessa
> mensagem e cuja cópia, encaminhamento e/ou execução das ações citadas
> estão
> imediatamente anuladas e proibidas'._
>
>
> * **'Apesar do Magazine Luiza tomar
> todas as precauções razoáveis para assegurar que nenhum vírus esteja
> presente nesse e-mail, a empresa não poderá aceitar a responsabilidade por
> quaisquer perdas ou danos causados por esse e-mail ou por seus anexos'.*
>
>
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Thanks,
Ales
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
