On 2/9/26 10:36 AM, Rukomoinikova Aleksandra wrote: > Hello Dumitru and Mark! In v1, we discussed whether this patch series > had a chance to make it into the release. I spent a long time editing > the comments and only just submitted v2. I made patches 1-5 with simple > functionality additions and didn't make any unnecessary code changes. I > also made patches 6-8 separately with some attempts to refactor the > code. I don't think 6-8 should be eligible for this release, and I > completely understand if they don't receive any review soon. If they > aren't merged now, I might submit these three patches later, after the > release. > Thanks)
Hi Alexandra, To be honest, I'd accept this patch in 26.03. It's way better than the current code. It does depend on whether we accept all of patch 7/8 though because it changes functions added there so I won't ack it yet. In any case, I'd suggest keeping it in the next revision (v4) and we can always decide then whether we apply it or not to main before branching for 26.03. Regards, Dumitru > > On 08.02.2026 00:48, Alexandra Rukomoinikova wrote: >> Replace individual global service monitor address parameters with unified >> svc_monitor_addresses structure to simplify function signatures. >> >> Signed-off-by: Alexandra Rukomoinikova <[email protected]> >> --- >> northd/en-global-config.c | 47 +++++++++++--------- >> northd/en-global-config.h | 29 ++++++------ >> northd/en-lflow.c | 3 +- >> northd/en-northd.c | 8 +--- >> northd/en-sync-sb.c | 4 +- >> northd/northd.c | 94 +++++++++++++++------------------------ >> northd/northd.h | 8 +--- >> 7 files changed, 84 insertions(+), 109 deletions(-) >> >> diff --git a/northd/en-global-config.c b/northd/en-global-config.c >> index 2556b2888..bad1bcf68 100644 >> --- a/northd/en-global-config.c >> +++ b/northd/en-global-config.c >> @@ -124,14 +124,17 @@ en_global_config_run(struct engine_node *node , void >> *data) >> const char *mac_addr_prefix = set_mac_prefix(smap_get(&nb->options, >> "mac_prefix")); >> >> + struct svc_monitor_addresses *svc_addresses = >> + &config_data->svc_global_addresses; >> + >> const char *monitor_mac = smap_get(&nb->options, "svc_monitor_mac"); >> if (monitor_mac) { >> if (eth_addr_from_string(monitor_mac, >> - &config_data->svc_monitor_mac_ea)) { >> - snprintf(config_data->svc_monitor_mac, >> - sizeof config_data->svc_monitor_mac, >> + &svc_addresses->mac_ea_src)) { >> + snprintf(svc_addresses->mac_src, >> + sizeof svc_addresses->mac_src, >> ETH_ADDR_FMT, >> - ETH_ADDR_ARGS(config_data->svc_monitor_mac_ea)); >> + ETH_ADDR_ARGS(svc_addresses->mac_ea_src)); >> } else { >> monitor_mac = NULL; >> } >> @@ -141,22 +144,22 @@ en_global_config_run(struct engine_node *node , void >> *data) >> "svc_monitor_mac_dst"); >> if (dst_monitor_mac) { >> if (eth_addr_from_string(dst_monitor_mac, >> - &config_data->svc_monitor_mac_ea_dst)) { >> - snprintf(config_data->svc_monitor_mac_dst, >> - sizeof config_data->svc_monitor_mac_dst, >> + &svc_addresses->mac_ea_dst)) { >> + snprintf(svc_addresses->mac_dst, >> + sizeof svc_addresses->mac_dst, >> ETH_ADDR_FMT, >> - ETH_ADDR_ARGS(config_data->svc_monitor_mac_ea_dst)); >> + ETH_ADDR_ARGS(svc_addresses->mac_ea_dst)); >> } else { >> dst_monitor_mac = NULL; >> } >> } >> >> const char *monitor_ip = smap_get(&nb->options, "svc_monitor_ip"); >> - update_svc_monitor_addr(monitor_ip, &config_data->svc_monitor_ip); >> + update_svc_monitor_addr(monitor_ip, &svc_addresses->ip_src); >> >> const char *monitor_ip_dst = smap_get(&nb->options, >> "svc_monitor_ip_dst"); >> update_svc_monitor_addr(monitor_ip_dst, >> - &config_data->svc_monitor_ip_dst); >> + &svc_addresses->ip_dst); >> >> struct smap *options = &config_data->nb_options; >> smap_destroy(options); >> @@ -165,21 +168,21 @@ en_global_config_run(struct engine_node *node , void >> *data) >> smap_replace(options, "mac_prefix", mac_addr_prefix); >> >> if (!monitor_mac) { >> - eth_addr_random(&config_data->svc_monitor_mac_ea); >> - snprintf(config_data->svc_monitor_mac, >> - sizeof config_data->svc_monitor_mac, ETH_ADDR_FMT, >> - ETH_ADDR_ARGS(config_data->svc_monitor_mac_ea)); >> + eth_addr_random(&svc_addresses->mac_ea_src); >> + snprintf(svc_addresses->mac_src, >> + sizeof svc_addresses->mac_src, ETH_ADDR_FMT, >> + ETH_ADDR_ARGS(svc_addresses->mac_ea_src)); >> smap_replace(options, "svc_monitor_mac", >> - config_data->svc_monitor_mac); >> + svc_addresses->mac_src); >> } >> >> if (!dst_monitor_mac) { >> - eth_addr_random(&config_data->svc_monitor_mac_ea_dst); >> - snprintf(config_data->svc_monitor_mac_dst, >> - sizeof config_data->svc_monitor_mac_dst, ETH_ADDR_FMT, >> - ETH_ADDR_ARGS(config_data->svc_monitor_mac_ea_dst)); >> + eth_addr_random(&svc_addresses->mac_ea_dst); >> + snprintf(svc_addresses->mac_dst, >> + sizeof svc_addresses->mac_dst, ETH_ADDR_FMT, >> + ETH_ADDR_ARGS(svc_addresses->mac_ea_dst)); >> smap_replace(options, "svc_monitor_mac_dst", >> - config_data->svc_monitor_mac_dst); >> + svc_addresses->mac_dst); >> } >> >> bool ic_vxlan_mode = false; >> @@ -244,8 +247,8 @@ void en_global_config_cleanup(void *data OVS_UNUSED) >> struct ed_type_global_config *config_data = data; >> smap_destroy(&config_data->nb_options); >> smap_destroy(&config_data->sb_options); >> - free(config_data->svc_monitor_ip); >> - free(config_data->svc_monitor_ip_dst); >> + free(config_data->svc_global_addresses.ip_src); >> + free(config_data->svc_global_addresses.ip_dst); >> destroy_debug_config(); >> } >> >> diff --git a/northd/en-global-config.h b/northd/en-global-config.h >> index 413cd3849..e84b0e9e5 100644 >> --- a/northd/en-global-config.h >> +++ b/northd/en-global-config.h >> @@ -30,27 +30,30 @@ struct global_config_tracked_data { >> bool chassis_features_changed; >> }; >> >> -/* struct which maintains the data of the engine node global_config. */ >> -struct ed_type_global_config { >> - struct smap nb_options; >> - struct smap sb_options; >> - const struct nbrec_nb_global *nb_global; >> - const struct sbrec_sb_global *sb_global; >> - >> +struct svc_monitor_addresses { >> /* MAC allocated for service monitor usage. Just one pair is allocated >> * for this purpose and ovn-controller's on each chassis will make use >> * of this pair when sending out the packets to monitor the services >> * defined in Service_Monitor Southbound table. Since these packets >> * are locally handled, having just one pair is good enough. */ >> - char svc_monitor_mac[ETH_ADDR_STRLEN + 1]; >> - struct eth_addr svc_monitor_mac_ea; >> - char svc_monitor_mac_dst[ETH_ADDR_STRLEN + 1]; >> - struct eth_addr svc_monitor_mac_ea_dst; >> + char mac_src[ETH_ADDR_STRLEN + 1]; >> + struct eth_addr mac_ea_src; >> + char mac_dst[ETH_ADDR_STRLEN + 1]; >> + struct eth_addr mac_ea_dst; >> >> /* IP addresses configured for NF service monitor usage. */ >> - char *svc_monitor_ip; >> - char *svc_monitor_ip_dst; >> + char *ip_src; >> + char *ip_dst; >> +}; >> + >> +/* struct which maintains the data of the engine node global_config. */ >> +struct ed_type_global_config { >> + struct smap nb_options; >> + struct smap sb_options; >> + const struct nbrec_nb_global *nb_global; >> + const struct sbrec_sb_global *sb_global; >> >> + struct svc_monitor_addresses svc_global_addresses; >> struct chassis_features features; >> >> bool ovn_internal_version_changed; >> diff --git a/northd/en-lflow.c b/northd/en-lflow.c >> index 41a5a97ff..0f64aef49 100644 >> --- a/northd/en-lflow.c >> +++ b/northd/en-lflow.c >> @@ -113,7 +113,8 @@ lflow_get_input_data(struct engine_node *node, >> lflow_input->features = &global_config->features; >> lflow_input->ovn_internal_version_changed = >> global_config->ovn_internal_version_changed; >> - lflow_input->svc_monitor_mac = global_config->svc_monitor_mac; >> + lflow_input->svc_monitor_mac = >> + global_config->svc_global_addresses.mac_src; >> >> struct ed_type_sampling_app_data *sampling_app_data = >> engine_get_input_data("sampling_app", node); >> diff --git a/northd/en-northd.c b/northd/en-northd.c >> index 9b37f3eee..e81e4bbb2 100644 >> --- a/northd/en-northd.c >> +++ b/northd/en-northd.c >> @@ -117,12 +117,8 @@ northd_get_input_data(struct engine_node *node, >> engine_get_input_data("global_config", node); >> input_data->nb_options = &global_config->nb_options; >> input_data->sb_options = &global_config->sb_options; >> - input_data->svc_monitor_mac = global_config->svc_monitor_mac; >> - input_data->svc_monitor_mac_ea = global_config->svc_monitor_mac_ea; >> - input_data->svc_monitor_mac_dst = global_config->svc_monitor_mac_dst; >> - input_data->svc_monitor_mac_ea_dst = >> global_config->svc_monitor_mac_ea_dst; >> - input_data->svc_monitor_ip = global_config->svc_monitor_ip; >> - input_data->svc_monitor_ip_dst = global_config->svc_monitor_ip_dst; >> + >> + input_data->svc_global_addresses = &global_config->svc_global_addresses; >> input_data->features = &global_config->features; >> input_data->vxlan_mode = global_config->vxlan_mode; >> >> diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c >> index fbe26eb8c..0b2a85b3b 100644 >> --- a/northd/en-sync-sb.c >> +++ b/northd/en-sync-sb.c >> @@ -106,8 +106,8 @@ en_sync_to_sb_addr_set_run(struct engine_node *node, >> void *data OVS_UNUSED) >> nb_port_group_table, sb_address_set_table, >> &lr_stateful_data->table, >> &northd_data->lr_datapaths, >> - global_config->svc_monitor_mac, >> - global_config->svc_monitor_mac_dst); >> + global_config->svc_global_addresses.mac_src, >> + global_config->svc_global_addresses.mac_dst); >> >> return EN_UPDATED; >> } >> diff --git a/northd/northd.c b/northd/northd.c >> index 7a2c472c0..629651efb 100644 >> --- a/northd/northd.c >> +++ b/northd/northd.c >> @@ -3165,28 +3165,28 @@ svc_mon_update_status_if_online(const struct >> sbrec_service_monitor *sbrec_mon, >> >> static void >> svc_mon_update_mac_ip_addresses(const struct sbrec_service_monitor >> *sbrec_mon, >> - const struct eth_addr *svc_monitor_mac_ea, >> - const char *source_mac, >> - const char *target_mac, >> - const char *source_ip, >> - const char *target_ip) >> + const struct eth_addr *mac_ea_src, >> + const char *mac_src, >> + const char *mac_dst, >> + const char *ip_src, >> + const char *ip_dst) >> { >> struct eth_addr ea; >> if (sbrec_mon->src_mac || >> !eth_addr_from_string(sbrec_mon->src_mac, &ea) || >> - !eth_addr_equals(ea, *svc_monitor_mac_ea)) { >> - sbrec_service_monitor_set_src_mac(sbrec_mon, source_mac); >> + !eth_addr_equals(ea, *mac_ea_src)) { >> + sbrec_service_monitor_set_src_mac(sbrec_mon, mac_src); >> } >> >> - if (target_mac) { >> - svc_mon_update_field_if_changed(sbrec_mon, sbrec_mon->mac, >> target_mac, >> + if (mac_dst) { >> + svc_mon_update_field_if_changed(sbrec_mon, sbrec_mon->mac, mac_dst, >> sbrec_service_monitor_set_mac); >> } >> >> - svc_mon_update_field_if_changed(sbrec_mon, sbrec_mon->src_ip, source_ip, >> + svc_mon_update_field_if_changed(sbrec_mon, sbrec_mon->src_ip, ip_src, >> sbrec_service_monitor_set_src_ip); >> - if (target_ip) { >> - svc_mon_update_field_if_changed(sbrec_mon, sbrec_mon->ip, target_ip, >> + if (ip_dst) { >> + svc_mon_update_field_if_changed(sbrec_mon, sbrec_mon->ip, ip_dst, >> sbrec_service_monitor_set_ip); >> } >> } >> @@ -3194,15 +3194,11 @@ svc_mon_update_mac_ip_addresses(const struct >> sbrec_service_monitor *sbrec_mon, >> static void >> ovn_nf_svc_create(struct ovsdb_idl_txn *ovnsb_txn, >> const struct nbrec_network_function *nbrec_nf, >> + const struct svc_monitor_addresses *svc_global_addresses, >> struct hmap *local_svc_monitors_map, >> struct hmap *ic_learned_svc_monitors_map, >> struct sset *svc_monitor_lsps, >> - struct hmap *ls_ports, >> - const char *global_svc_monitor_mac_src, >> - const struct eth_addr *global_svc_monitor_mac_ea_src, >> - const char *global_svc_monitor_mac_dst, >> - const char *global_svc_monitor_ip_src, >> - const char *global_svc_monitor_ip_dst) >> + struct hmap *ls_ports) >> { >> if (!nbrec_nf->health_check) { >> return; >> @@ -3241,22 +3237,18 @@ ovn_nf_svc_create(struct ovsdb_idl_txn *ovnsb_txn, >> local_svc_monitors_map, >> ic_learned_svc_monitors_map, >> "network-function", >> - global_svc_monitor_ip_dst, >> + svc_global_addresses->ip_dst, >> nbrec_nf->outport->name, >> nbrec_nf->inport->name, >> - 0, >> - "icmp", >> - chassis_name, >> - false); >> + 0, "icmp", chassis_name, false); >> set_service_mon_options(mon_info->sbrec_mon, >> - &nbrec_nf->health_check->options, >> - NULL); >> + &nbrec_nf->health_check->options, NULL); >> svc_mon_update_mac_ip_addresses(mon_info->sbrec_mon, >> - global_svc_monitor_mac_ea_src, >> - global_svc_monitor_mac_src, >> - global_svc_monitor_mac_dst, >> - global_svc_monitor_ip_src, >> - global_svc_monitor_ip_dst); >> + &svc_global_addresses->mac_ea_src, >> + svc_global_addresses->mac_src, >> + svc_global_addresses->mac_dst, >> + svc_global_addresses->ip_src, >> + svc_global_addresses->ip_dst); >> svc_mon_update_status_if_online(mon_info->sbrec_mon, >> !port_up_condition); >> } >> @@ -3264,8 +3256,7 @@ ovn_nf_svc_create(struct ovsdb_idl_txn *ovnsb_txn, >> static void >> ovn_lb_svc_create(struct ovsdb_idl_txn *ovnsb_txn, >> const struct ovn_northd_lb *lb, >> - const char *global_svc_monitor_mac_src, >> - const struct eth_addr *global_svc_monitor_mac_ea_src, >> + const struct svc_monitor_addresses *svc_global_addresses, >> struct hmap *ls_ports, >> struct sset *svc_monitor_lsps, >> struct hmap *local_svc_monitors_map, >> @@ -3311,8 +3302,8 @@ ovn_lb_svc_create(struct ovsdb_idl_txn *ovnsb_txn, >> source_mac_ea = &backend_nb->svc_mon_lrp->lrp_networks.ea; >> source_mac = backend_nb->svc_mon_lrp->lrp_networks.ea_s; >> } else { >> - source_mac_ea = global_svc_monitor_mac_ea_src; >> - source_mac = global_svc_monitor_mac_src; >> + source_mac_ea = &svc_global_addresses->mac_ea_src; >> + source_mac = svc_global_addresses->mac_src; >> } >> >> const char *protocol = lb->nlb->protocol; >> @@ -3554,8 +3545,7 @@ build_lb_datapaths(const struct hmap *lbs, const >> struct hmap *lb_groups, >> static void >> ovn_lsp_svc_monitors_process_port( >> struct ovsdb_idl_txn *ovnsb_txn, const struct ovn_port *op, >> - const char *global_svc_monitor_mac_src, >> - const struct eth_addr *global_svc_monitor_mac_ea_src, >> + const struct svc_monitor_addresses *svc_global_addresses, >> struct hmap *local_svc_monitors_map, >> struct sset *svc_monitor_lsps) >> { >> @@ -3597,8 +3587,8 @@ ovn_lsp_svc_monitors_process_port( >> set_service_mon_options(mon_info->sbrec_mon, >> &lsp_hc->options, NULL); >> svc_mon_update_mac_ip_addresses(mon_info->sbrec_mon, >> - global_svc_monitor_mac_ea_src, >> - global_svc_monitor_mac_src, >> + &svc_global_addresses->mac_ea_src, >> + svc_global_addresses->mac_src, >> NULL, lsp_hc->src_ip, NULL); >> svc_mon_update_status_if_online(mon_info->sbrec_mon, >> port_up_condition); >> @@ -3610,11 +3600,7 @@ build_svc_monitors_data( >> struct ovsdb_idl_txn *ovnsb_txn, >> struct ovsdb_idl_index *sbrec_service_monitor_by_learned_type, >> const struct nbrec_network_function_table >> *nbrec_network_function_table, >> - const char *global_svc_monitor_mac_src, >> - const struct eth_addr *global_svc_monitor_mac_ea_src, >> - const char *global_svc_monitor_mac_dst, >> - const char *global_svc_monitor_ip_src, >> - const char *global_svc_monitor_ip_dst, >> + const struct svc_monitor_addresses *svc_global_addresses, >> struct hmap *ls_ports, struct hmap *lb_dps_map, >> struct sset *svc_monitor_lsps, >> struct hmap *local_svc_monitors_map, >> @@ -3645,8 +3631,7 @@ build_svc_monitors_data( >> struct ovn_lb_datapaths *lb_dps; >> HMAP_FOR_EACH (lb_dps, hmap_node, lb_dps_map) { >> ovn_lb_svc_create(ovnsb_txn, lb_dps->lb, >> - global_svc_monitor_mac_src, >> - global_svc_monitor_mac_ea_src, >> + svc_global_addresses, >> ls_ports, >> svc_monitor_lsps, >> local_svc_monitors_map, >> @@ -3658,15 +3643,11 @@ build_svc_monitors_data( >> nbrec_network_function_table) { >> ovn_nf_svc_create(ovnsb_txn, >> nbrec_nf, >> + svc_global_addresses, >> local_svc_monitors_map, >> ic_learned_svc_monitors_map, >> svc_monitor_lsps, >> - ls_ports, >> - global_svc_monitor_mac_src, >> - global_svc_monitor_mac_ea_src, >> - global_svc_monitor_mac_dst, >> - global_svc_monitor_ip_src, >> - global_svc_monitor_ip_dst); >> + ls_ports); >> } >> >> struct hmapx_node *hmapx_node; >> @@ -3675,8 +3656,7 @@ build_svc_monitors_data( >> op = hmapx_node->data; >> ovn_lsp_svc_monitors_process_port(ovnsb_txn, >> op, >> - global_svc_monitor_mac_src, >> - global_svc_monitor_mac_ea_src, >> + svc_global_addresses, >> local_svc_monitors_map, >> svc_monitor_lsps); >> } >> @@ -20771,11 +20751,7 @@ ovnnb_db_run(struct northd_input *input_data, >> build_svc_monitors_data(ovnsb_txn, >> input_data->sbrec_service_monitor_by_learned_type, >> input_data->nbrec_network_function_table, >> - input_data->svc_monitor_mac, >> - &input_data->svc_monitor_mac_ea, >> - input_data->svc_monitor_mac_dst, >> - input_data->svc_monitor_ip, >> - input_data->svc_monitor_ip_dst, >> + input_data->svc_global_addresses, >> &data->ls_ports, &data->lb_datapaths_map, >> &data->svc_monitor_lsps, &data->local_svc_monitors_map, >> input_data->ic_learned_svc_monitors_map, >> @@ -20785,7 +20761,7 @@ ovnnb_db_run(struct northd_input *input_data, >> input_data->nbrec_network_function_group_table, >> &data->local_svc_monitors_map, >> input_data->ic_learned_svc_monitors_map, >> - input_data->svc_monitor_ip_dst); >> + input_data->svc_global_addresses->ip_dst); >> build_ipam(&data->ls_datapaths.datapaths); >> build_lrouter_groups(&data->lr_ports, &data->lr_datapaths); >> build_ip_mcast(ovnsb_txn, input_data->sbrec_ip_multicast_table, >> diff --git a/northd/northd.h b/northd/northd.h >> index f5c8b17dd..b7cea07c1 100644 >> --- a/northd/northd.h >> +++ b/northd/northd.h >> @@ -23,6 +23,7 @@ >> #include "lib/hmapx.h" >> #include "northd/en-port-group.h" >> #include "northd/en-sync-from-sb.h" >> +#include "northd/en-global-config.h" >> #include "northd/ipam.h" >> #include "northd/lb.h" >> #include "openvswitch/hmap.h" >> @@ -70,12 +71,7 @@ struct northd_input { >> /* Global config data node inputs. */ >> const struct smap *nb_options; >> const struct smap *sb_options; >> - const char *svc_monitor_mac; >> - struct eth_addr svc_monitor_mac_ea; >> - const char *svc_monitor_mac_dst; >> - struct eth_addr svc_monitor_mac_ea_dst; >> - char *svc_monitor_ip; >> - char *svc_monitor_ip_dst; >> + const struct svc_monitor_addresses *svc_global_addresses; >> const struct chassis_features *features; >> bool vxlan_mode; >> > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
