On 12.02.2026 14:31, Dumitru Ceara wrote:
> On 2/7/26 10:48 PM, Alexandra Rukomoinikova wrote:
>> 1) Extract common service monitor update logic into helper functions:
>> - svc_mon_update_field_if_changed() for field updates with change detection
>> - svc_mon_update_status_if_online() for status transition to offline
>> - svc_mon_update_mac_ip_addresses() for MAC/IP address updates
>>
>> 2) Simplify Network Function service monitor creation by passing the
>> nbrec_network_function object directly instead of individual fields.
>>
>> 3) Improve parameter naming consistency across LB, LSP, and NF service
>> monitor creation functions.
>>
>> Signed-off-by: Alexandra Rukomoinikova <[email protected]>
>> ---
>> v2: After adding lsp hc, its ended up with three part of nearly identical
>> code for creating service monitors. I split it all into functions to
>> make it more uniform.
>> ---
> Hi Alexandra,
>
> I guess we could include most of this already in 26.03 but for the
> svc_mon_update_field_if_changed() I wonder if we should have a more
> generic solution (maybe post 26.03). Please see below for some ideas.
>
> Also, Mark might have nice suggestions too about how we could avoid
> some of the bloat.
>
>> northd/northd.c | 270 +++++++++++++++++++++++-------------------------
>> 1 file changed, 131 insertions(+), 139 deletions(-)
>>
>> diff --git a/northd/northd.c b/northd/northd.c
>> index b11a57ca9..7a2c472c0 100644
>> --- a/northd/northd.c
>> +++ b/northd/northd.c
>> @@ -3110,6 +3110,7 @@ create_or_get_service_mon(struct ovsdb_idl_txn
>> *ovnsb_txn,
>> */
>> sbrec_service_monitor_set_ic_learned(mon_info->sbrec_mon,
>> false);
>> + mon_info->required = true;
>> return mon_info;
>> }
>>
>> @@ -3137,32 +3138,80 @@ create_or_get_service_mon(struct ovsdb_idl_txn
>> *ovnsb_txn,
>> mon_info = xzalloc(sizeof *mon_info);
>> mon_info->sbrec_mon = sbrec_mon;
>> hmap_insert(local_svc_monitors_map, &mon_info->hmap_node, hash);
>> + mon_info->required = true;
>> return mon_info;
>> }
>>
>> +static inline void
>> +svc_mon_update_field_if_changed(
> Would this be useful for more tables/columns. What if we change it into
> a macro, e.g., in ovn-util.h:
> #define DBREC_SET_IF_CHANGED6(DB, TABLE, FIELD, OBJ, NEW_VAL, COND) \
> do { \
> if ((COND) && ((!(OBJ)->FIELD && (NEW_VAL)) || \
> ((OBJ)->FIELD && !(NEW_VAL)) || \
> strcmp((OBJ)->FIELD, (NEW_VAL)))) { \
> DB##rec_##TABLE##_set_##FIELD((OBJ), (NEW_VAL)); \
> }; \
> } while (0)
>
> #define DBREC_SET_IF_CHANGED5(DB, TABLE, FIELD, OBJ, NEW_VAL) \
> DBREC_SET_IF_CHANGED6(DB, TABLE, FIELD, OBJ, NEW_VAL, true)
>
> #define DBREC_SET_IF_CHANGED(...) \
> VFUNC(DBREC_SET_IF_CHANGED, __VA_ARGS__)
>
> This is just a suggestion and only works with string fields. I guess
> to make it even more generic we'd have to pass a generic comparer but
> that would make it even more complex. I'm not sure, maybe you have
> ideas on how to improve this further.
Hi, I couldn't come up with anything fancy with this function yet. I've
submitted a new version without these changes—only the ones you
confirmed. I'll take some time to think about it.
>
>> + const struct sbrec_service_monitor *sbrec_mon,
>> + const char *old_value, const char *new_value,
>> + void (*setter)(const struct sbrec_service_monitor *, const char *))
>> +{
>> + if (old_value && new_value && strcmp(old_value, new_value)) {
> Do we need to take into account the case when old_value == NULL?
>
>> + setter(sbrec_mon, new_value);
>> + }
>> +}
>> +
>> +static inline void
> No need for "inline".
>
>> +svc_mon_update_status_if_online(const struct sbrec_service_monitor
>> *sbrec_mon,
>> + bool condition)
>> +{
>> + if (condition && sbrec_mon->status &&
>> + !strcmp(sbrec_mon->status, "online")) {
>> + sbrec_service_monitor_set_status(sbrec_mon, "offline");
>> + }
> With the suggested macro above this could be:
>
> DBREC_SET_IF_CHANGED(sb, service_monitor, status, sbrec_mon, "offline",
> condition && sbrec_mon->status &&
> !strcmp(sbrec_mon->status, "online"));
>
> But again, I'm not sure if this doesn't overcomplicate the
> code too much.
>
>> +}
>> +
>> +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)
>> +{
>> + 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);
>> + }
> With the macros I suggested above:
>
> DBREC_SET_IF_CHANGED(sb, service_monitor, src_mac, sbrec_mon, source_mac,
> sbrec_mon->src_mac ||
> !eth_addr_from_string(sbrec_mon->src_mac, &ea) ||
> !eth_addr_equals(ea, *svc_monitor_mac_ea));
>
> Doesn't seem too readable though. :)
>
>> +
>> + if (target_mac) {
> We could just call svc_mon_update_field_if_changed() directly.
>
>> + svc_mon_update_field_if_changed(sbrec_mon, sbrec_mon->mac,
>> target_mac,
>> + sbrec_service_monitor_set_mac);
>> + }
>> +
>> + svc_mon_update_field_if_changed(sbrec_mon, sbrec_mon->src_ip, source_ip,
>> + sbrec_service_monitor_set_src_ip);
>> + if (target_ip) {
>> + svc_mon_update_field_if_changed(sbrec_mon, sbrec_mon->ip, target_ip,
>> + sbrec_service_monitor_set_ip);
>> + }
> Or, if using the macro I suggested above:
>
> DBREC_SET_IF_CHANGED(sb, service_monitor, mac, sbrec_mon, target_mac);
> DBREC_SET_IF_CHANGED(sb, service_monitor, src_ip, sbrec_mon, source_ip);
> DBREC_SET_IF_CHANGED(sb, service_monitor, ip, sbrec_mon, target_ip);
>
> Otherwise, we could just let the expanded code as is in the new
> svc_mon_update_mac_ip_addresses() for now until we find a better
> way of doing this in a generic manner.
>
>> +}
>> +
>> static void
>> ovn_nf_svc_create(struct ovsdb_idl_txn *ovnsb_txn,
>> + const struct nbrec_network_function *nbrec_nf,
>> struct hmap *local_svc_monitors_map,
>> struct hmap *ic_learned_svc_monitors_map,
>> struct sset *svc_monitor_lsps,
>> struct hmap *ls_ports,
>> - const char *mac_src, const char *mac_dst,
>> - const char *ip_src, const char *ip_dst,
>> - const char *logical_port, const char *logical_input_port,
>> - const struct smap *health_check_options)
>> + 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)
>> {
>> - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
>> -
>> - if (!ip_src || !ip_dst || !mac_src || !mac_dst) {
>> - VLOG_ERR_RL(&rl, "NetworkFunction: invalid service monitor "
>> - "src_mac: %s dst_mac:%s src_ip:%s dst_ip:%s",
>> - mac_src, mac_dst, ip_src, ip_dst);
>> + if (!nbrec_nf->health_check) {
>> return;
>> }
>>
>> - const char *ports[] = {logical_port, logical_input_port};
>> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
>> + const char *ports[] = {nbrec_nf->outport->name, nbrec_nf->inport->name};
>> const char *chassis_name = NULL;
>> - bool port_up = true;
>> + bool port_up_condition = true;
>>
>> for (size_t i = 0; i < ARRAY_SIZE(ports); i++) {
>> const char *port = ports[i];
>> @@ -3183,58 +3232,40 @@ ovn_nf_svc_create(struct ovsdb_idl_txn *ovnsb_txn,
>> op->sb->chassis->name, port);
>> }
>> }
>> - port_up = port_up && (op->sb->n_up && op->sb->up[0]);
>> + port_up_condition = port_up_condition &&
>> + (op->sb->n_up && op->sb->up[0]);
>> }
>>
>> struct service_monitor_info *mon_info =
>> create_or_get_service_mon(ovnsb_txn,
>> local_svc_monitors_map,
>> ic_learned_svc_monitors_map,
>> - "network-function", ip_dst,
>> - logical_port,
>> - logical_input_port,
>> + "network-function",
>> + global_svc_monitor_ip_dst,
>> + nbrec_nf->outport->name,
>> + nbrec_nf->inport->name,
>> 0,
>> "icmp",
>> chassis_name,
>> false);
>> - ovs_assert(mon_info);
>> - sbrec_service_monitor_set_options(
>> - mon_info->sbrec_mon, health_check_options);
>> -
>> - if (!mon_info->sbrec_mon->src_mac ||
>> - strcmp(mon_info->sbrec_mon->src_mac, mac_src)) {
>> - sbrec_service_monitor_set_src_mac(mon_info->sbrec_mon,
>> - mac_src);
>> - }
>> -
>> - if (!mon_info->sbrec_mon->mac ||
>> - strcmp(mon_info->sbrec_mon->mac, mac_dst)) {
>> - sbrec_service_monitor_set_mac(mon_info->sbrec_mon,
>> - mac_dst);
>> - }
>> -
>> - if (!mon_info->sbrec_mon->src_ip ||
>> - strcmp(mon_info->sbrec_mon->src_ip, ip_src)) {
>> - sbrec_service_monitor_set_src_ip(mon_info->sbrec_mon, ip_src);
>> - }
>> -
>> - if (!mon_info->sbrec_mon->ip ||
>> - strcmp(mon_info->sbrec_mon->ip, ip_dst)) {
>> - sbrec_service_monitor_set_ip(mon_info->sbrec_mon, ip_dst);
>> - }
>> -
>> - if (!port_up && mon_info->sbrec_mon->status
>> - && !strcmp(mon_info->sbrec_mon->status, "online")) {
>> - sbrec_service_monitor_set_status(mon_info->sbrec_mon, "offline");
>> - }
>> - mon_info->required = true;
>> + set_service_mon_options(mon_info->sbrec_mon,
>> + &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_mon_update_status_if_online(mon_info->sbrec_mon,
>> + !port_up_condition);
>> }
>>
>> static void
>> ovn_lb_svc_create(struct ovsdb_idl_txn *ovnsb_txn,
>> const struct ovn_northd_lb *lb,
>> - const char *svc_monitor_mac,
>> - const struct eth_addr *svc_monitor_mac_ea,
>> + const char *global_svc_monitor_mac_src,
>> + const struct eth_addr *global_svc_monitor_mac_ea_src,
>> struct hmap *ls_ports,
>> struct sset *svc_monitor_lsps,
>> struct hmap *local_svc_monitors_map,
>> @@ -3267,6 +3298,9 @@ ovn_lb_svc_create(struct ovsdb_idl_txn *ovnsb_txn,
>> continue;
>> }
>>
>> + bool port_up_condition = !backend_nb->remote_backend &&
>> + (!op->sb->n_up || !op->sb->up[0]);
>> +
>> ovn_northd_lb_backend_set_mon_port(op, backend_nb);
>>
>> /* If the service monitor is backed by a real port, use its MAC
>> @@ -3277,8 +3311,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 = svc_monitor_mac_ea;
>> - source_mac = svc_monitor_mac;
>> + source_mac_ea = global_svc_monitor_mac_ea_src;
>> + source_mac = global_svc_monitor_mac_src;
>> }
>>
>> const char *protocol = lb->nlb->protocol;
>> @@ -3286,52 +3320,26 @@ ovn_lb_svc_create(struct ovsdb_idl_txn *ovnsb_txn,
>> protocol = "tcp";
>> }
>>
>> - const char *chassis_name = NULL;
>> - if (!backend_nb->remote_backend && op->sb->chassis) {
>> - chassis_name = op->sb->chassis->name;
>> - }
>> -
>> struct service_monitor_info *mon_info =
>> create_or_get_service_mon(ovnsb_txn,
>> local_svc_monitors_map,
>> ic_learned_svc_monitors_map,
>> - "load-balancer",
>> - backend->ip_str,
>> + "load-balancer", backend->ip_str,
>> backend_nb->logical_port,
>> - NULL,
>> - backend->port,
>> - protocol,
>> - chassis_name,
>> + NULL, backend->port, protocol,
>> + (!backend_nb->remote_backend &&
>> + op->sb->chassis) ?
>> + op->sb->chassis->name : NULL,
>> backend_nb->remote_backend);
>> - ovs_assert(mon_info);
>> +
>> set_service_mon_options(mon_info->sbrec_mon,
>> &lb_vip_nb->lb_health_check->options,
>> backend_nb->az_name);
>> - struct eth_addr ea;
>> - if (!mon_info->sbrec_mon->src_mac ||
>> - !eth_addr_from_string(mon_info->sbrec_mon->src_mac, &ea) ||
>> - !eth_addr_equals(ea, *source_mac_ea)) {
>> - sbrec_service_monitor_set_src_mac(mon_info->sbrec_mon,
>> - source_mac);
>> - }
>> -
>> - if (!mon_info->sbrec_mon->src_ip ||
>> - strcmp(mon_info->sbrec_mon->src_ip,
>> - backend_nb->svc_mon_src_ip)) {
>> - sbrec_service_monitor_set_src_ip(
>> - mon_info->sbrec_mon,
>> - backend_nb->svc_mon_src_ip);
>> - }
>> -
>> - if (!backend_nb->remote_backend &&
>> - (!op->sb->n_up || !op->sb->up[0])
>> - && mon_info->sbrec_mon->status
>> - && !strcmp(mon_info->sbrec_mon->status, "online")) {
>> - sbrec_service_monitor_set_status(mon_info->sbrec_mon,
>> - "offline");
>> - }
>> -
>> - mon_info->required = true;
>> + svc_mon_update_mac_ip_addresses(mon_info->sbrec_mon,
>> + source_mac_ea, source_mac, NULL,
>> + backend_nb->svc_mon_src_ip,
>> NULL);
>> + svc_mon_update_status_if_online(mon_info->sbrec_mon,
>> + port_up_condition);
>> }
>> }
>> }
>> @@ -3544,13 +3552,14 @@ 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 *svc_monitor_mac,
>> - const struct eth_addr *svc_monitor_mac_ea,
>> - struct hmap *local_svc_monitors_map,
>> - struct sset *svc_monitor_lsps)
>> +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,
>> + struct hmap *local_svc_monitors_map,
>> + struct sset *svc_monitor_lsps)
>> {
>> + bool port_up_condition = !op->sb->n_up || !op->sb->up[0];
>> sset_add(svc_monitor_lsps, op->key);
>>
>> for (size_t i = 0; i < op->nbsp->n_health_checks; i++) {
>> @@ -3585,31 +3594,14 @@ ovn_lsp_svc_monitors_process_port(struct
>> ovsdb_idl_txn *ovnsb_txn,
>> (op->sb && op->sb->chassis) ?
>> op->sb->chassis->name : NULL,
>> false);
>> -
>> - mon_info->required = true;
>> set_service_mon_options(mon_info->sbrec_mon,
>> &lsp_hc->options, NULL);
>> -
>> - struct eth_addr ea;
>> - if (!mon_info->sbrec_mon->src_mac ||
>> - !eth_addr_from_string(mon_info->sbrec_mon->src_mac, &ea) ||
>> - !eth_addr_equals(ea, *svc_monitor_mac_ea)) {
>> - sbrec_service_monitor_set_src_mac(mon_info->sbrec_mon,
>> - svc_monitor_mac);
>> - }
>> -
>> - if (!mon_info->sbrec_mon->src_ip ||
>> - strcmp(mon_info->sbrec_mon->src_ip, lsp_hc->src_ip)) {
>> - sbrec_service_monitor_set_src_ip(mon_info->sbrec_mon,
>> - lsp_hc->src_ip);
>> - }
>> -
>> - if ((!op->sb->n_up || !op->sb->up[0]) &&
>> - mon_info->sbrec_mon->status &&
>> - !strcmp(mon_info->sbrec_mon->status, "online")) {
>> - sbrec_service_monitor_set_status(mon_info->sbrec_mon,
>> - "offline");
>> - }
>> + svc_mon_update_mac_ip_addresses(mon_info->sbrec_mon,
>> + global_svc_monitor_mac_ea_src,
>> + global_svc_monitor_mac_src,
>> + NULL, lsp_hc->src_ip, NULL);
>> + svc_mon_update_status_if_online(mon_info->sbrec_mon,
>> + port_up_condition);
>> }
>> }
>>
>> @@ -3617,13 +3609,13 @@ static void
>> build_svc_monitors_data(
>> struct ovsdb_idl_txn *ovnsb_txn,
>> struct ovsdb_idl_index *sbrec_service_monitor_by_learned_type,
>> - const char *svc_monitor_mac,
>> - const struct eth_addr *svc_monitor_mac_ea,
>> - const char *svc_monitor_mac_dst,
>> - const char *svc_monitor_ip,
>> - const char *svc_monitor_ip_dst,
>> - struct hmap *ls_ports, struct hmap *lb_dps_map,
>> 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,
>> + struct hmap *ls_ports, struct hmap *lb_dps_map,
>> struct sset *svc_monitor_lsps,
>> struct hmap *local_svc_monitors_map,
>> struct hmap *ic_learned_svc_monitors_map,
>> @@ -3653,8 +3645,8 @@ 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,
>> - svc_monitor_mac,
>> - svc_monitor_mac_ea,
>> + global_svc_monitor_mac_src,
>> + global_svc_monitor_mac_ea_src,
>> ls_ports,
>> svc_monitor_lsps,
>> local_svc_monitors_map,
>> @@ -3664,17 +3656,17 @@ build_svc_monitors_data(
>> const struct nbrec_network_function *nbrec_nf;
>> NBREC_NETWORK_FUNCTION_TABLE_FOR_EACH (nbrec_nf,
>> nbrec_network_function_table) {
>> - if (nbrec_nf->health_check) {
>> - ovn_nf_svc_create(ovnsb_txn,
>> - local_svc_monitors_map,
>> - ic_learned_svc_monitors_map,
>> - svc_monitor_lsps,
>> - ls_ports,
>> - svc_monitor_mac, svc_monitor_mac_dst,
>> - svc_monitor_ip, svc_monitor_ip_dst,
>> - nbrec_nf->outport->name,
>> nbrec_nf->inport->name,
>> - &nbrec_nf->health_check->options);
>> - }
>> + ovn_nf_svc_create(ovnsb_txn,
>> + nbrec_nf,
>> + 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);
>> }
>>
>> struct hmapx_node *hmapx_node;
>> @@ -3683,8 +3675,8 @@ build_svc_monitors_data(
>> op = hmapx_node->data;
>> ovn_lsp_svc_monitors_process_port(ovnsb_txn,
>> op,
>> - svc_monitor_mac,
>> - svc_monitor_mac_ea,
>> + global_svc_monitor_mac_src,
>> + global_svc_monitor_mac_ea_src,
>> local_svc_monitors_map,
>> svc_monitor_lsps);
>> }
>> @@ -20778,13 +20770,13 @@ ovnnb_db_run(struct northd_input *input_data,
>> &data->lb_group_datapaths_map);
>> 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,
>> &data->ls_ports, &data->lb_datapaths_map,
>> - input_data->nbrec_network_function_table,
>> &data->svc_monitor_lsps, &data->local_svc_monitors_map,
>> input_data->ic_learned_svc_monitors_map,
>> &data->monitored_ports_map);
> Regards,
> Dumitru
>
--
regards,
Alexandra.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
