On 8/15/25 10:06 AM, Alexandra Rukomoinikova wrote:
> This commit adds support for synchronizing load balancer health checks
> across interconnected availability zones via OVN-IC. Key features include:
>
> 1. Added new Service_Monitor table to ICSB database
> - Tracks service monitors across AZs with source/target zone info
> - Supports status propagation between zones
>
> 2. Implemented synchronization logic in ovn-ic:
> - Pushes local service monitors to remote AZs via ICSB
> - Pulls remote monitors into local SBDB as IC-learned
> - Handles status updates in both directions
> - Maintains consistency during configuration changes
>
> 3. Added comprehensive test casesю
>
> 4. Extended OVSDB indexes for efficient lookups.
>
> Signed-off-by: Alexandra Rukomoinikova <arukomoinikova@k2.cloud>
> ---
> v1 --> v2: 1) added test for parsing ip_port_mapping for ipv4
> 2) corrected comments regarding tests
> ---
Hi Alexandra,
I know the patch was merged but I think we might need some small follow
ups, please see below.
> ic/ovn-ic.c | 503 ++++++++++++++++++++++++++++++++++++++++++++
> ovn-ic-sb.ovsschema | 32 ++-
> ovn-ic-sb.xml | 56 +++++
> tests/ovn-ic.at | 404 +++++++++++++++++++++++++++++++++++
> tests/ovn-northd.at | 105 +++++----
> 5 files changed, 1058 insertions(+), 42 deletions(-)
>
> diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
> index ac245a8ed..cfdbd4b40 100644
> --- a/ic/ovn-ic.c
> +++ b/ic/ovn-ic.c
[...]
> +static void
> +create_pushed_svcs_mon(struct ic_context *ctx,
> + struct hmap *pushed_svcs_map)
> +{
> + struct sbrec_service_monitor *key =
> + sbrec_service_monitor_index_init_row(
> + ctx->sbrec_service_monitor_by_local_type);
> +
> + sbrec_service_monitor_index_set_local(key, false);
> +
> + const struct sbrec_service_monitor *sb_rec;
> + SBREC_SERVICE_MONITOR_FOR_EACH_EQUAL (sb_rec, key,
> + ctx->sbrec_service_monitor_by_local_type) {
> + const char *target_az_name = smap_get_def(&sb_rec->options,
> + "az-name", "");
> + if (!target_az_name) {
> + continue;
> + }
smap_get_def() never returns NULL and, as far as I can tell, the actual
goal here is to really skip SB.service_monitor records that don't have
az-name set. That doesn't happen with this code, we'd just create a
"pushed_svc" with target_az_name = "".
Do we need to change this to something like below?
const char *target_az_name = smap_get(&sb_rec->options, "az-name");
if (!target_az_name) {
continue;
}
If so, do you mind posting a follow up patch? We might also need a test
for this scenario.
> + create_service_monitor_info(pushed_svcs_map, sb_rec,
> + &sb_rec->header_.uuid,
> + ctx->runned_az->name, target_az_name,
> + NULL, false);
> + }
> +
> + sbrec_service_monitor_index_destroy_row(key);
> +}
> +
> +static void
> +create_synced_svcs_mon(struct ic_context *ctx,
> + struct hmap *synced_svcs_map)
> +{
> + struct icsbrec_service_monitor *key =
> + icsbrec_service_monitor_index_init_row(
> + ctx->icsbrec_service_monitor_by_target_az);
> +
> + icsbrec_service_monitor_index_set_target_availability_zone(
> + key, ctx->runned_az->name);
> +
> + const struct icsbrec_service_monitor *ic_rec;
> + ICSBREC_SERVICE_MONITOR_FOR_EACH_EQUAL (ic_rec, key,
> + ctx->icsbrec_service_monitor_by_target_az) {
> +
> + const struct sbrec_port_binding *pb =
> + find_sb_pb_by_name(ctx->sbrec_port_binding_by_name,
> + ic_rec->logical_port);
> +
> + if (!pb || !pb->up) {
> + continue;
> + }
> +
> + const char *chassis_name = pb->chassis ? pb->chassis->name : NULL;
> + create_service_monitor_info(synced_svcs_map, ic_rec,
> + &ic_rec->header_.uuid,
> + ctx->runned_az->name,
> + NULL, chassis_name, true);
> + }
> +
> + icsbrec_service_monitor_index_destroy_row(key);
> +}
> +
> +static void
> +create_local_ic_svcs_map(struct ic_context *ctx,
> + struct hmap *owned_svc_map)
> +{
> + struct icsbrec_service_monitor *key =
> + icsbrec_service_monitor_index_init_row(
> + ctx->icsbrec_service_monitor_by_source_az);
> +
> + icsbrec_service_monitor_index_set_source_availability_zone(
> + key, ctx->runned_az->name);
> +
> + const struct icsbrec_service_monitor *ic_rec;
> + ICSBREC_SERVICE_MONITOR_FOR_EACH_EQUAL (ic_rec, key,
> + ctx->icsbrec_service_monitor_by_source_az) {
> + create_service_monitor_info(owned_svc_map, ic_rec,
> + &ic_rec->header_.uuid,
> + ctx->runned_az->name, NULL,
> + NULL, true);
> + }
> +
> + icsbrec_service_monitor_index_destroy_row(key);
> +}
> +
> +static void
> +create_local_sb_svcs_map(struct ic_context *ctx,
> + struct hmap *owned_svc_map)
> +{
> + struct sbrec_service_monitor *key =
> + sbrec_service_monitor_index_init_row(
> + ctx->sbrec_service_monitor_by_ic_learned);
> +
> + sbrec_service_monitor_index_set_ic_learned(
> + key, true);
> +
> + const struct sbrec_service_monitor *sb_rec;
> + SBREC_SERVICE_MONITOR_FOR_EACH_EQUAL (sb_rec, key,
> + ctx->sbrec_service_monitor_by_ic_learned) {
> + create_service_monitor_info(owned_svc_map, sb_rec,
> + &sb_rec->header_.uuid,
> + ctx->runned_az->name, NULL,
> + NULL, false);
> + }
> +
> + sbrec_service_monitor_index_destroy_row(key);
> +}
> +
> +static const struct sbrec_service_monitor *
> +lookup_sb_svc_rec(struct ic_context *ctx,
> + const struct service_monitor_info *svc_mon)
> +{
> + const struct icsbrec_service_monitor *db_rec =
> + svc_mon->db_rec.ic_rec;
> + struct sbrec_service_monitor *key =
> + sbrec_service_monitor_index_init_row(
> + ctx->sbrec_service_monitor_by_local_type_logical_port);
> +
> + sbrec_service_monitor_index_set_local(key, true);
> + sbrec_service_monitor_index_set_logical_port(key, db_rec->logical_port);
> +
> + const struct sbrec_service_monitor *sb_rec;
> + SBREC_SERVICE_MONITOR_FOR_EACH_EQUAL (sb_rec, key,
> + ctx->sbrec_service_monitor_by_local_type_logical_port) {
> + if (db_rec->port == sb_rec->port &&
> + !strcmp(db_rec->ip, sb_rec->ip) &&
> + !strcmp(db_rec->src_ip, sb_rec->src_ip) &&
> + !strcmp(db_rec->protocol, sb_rec->protocol)) {
> + sbrec_service_monitor_index_destroy_row(key);
> + return sb_rec;
> + }
> + }
> +
> + sbrec_service_monitor_index_destroy_row(key);
> +
> + return NULL;
> +}
> +
> +static const struct icsbrec_service_monitor *
> +lookup_icsb_svc_rec(struct ic_context *ctx,
> + const struct service_monitor_info *svc_mon)
> +{
> + const struct sbrec_service_monitor *db_rec =
> + svc_mon->db_rec.sb_rec;
> + struct icsbrec_service_monitor *key =
> + icsbrec_service_monitor_index_init_row(
> + ctx->icsbrec_service_monitor_by_target_az_logical_port);
> +
> + ovs_assert(svc_mon->dst_az_name);
> + icsbrec_service_monitor_index_set_target_availability_zone(
> + key, svc_mon->dst_az_name);
> +
> + icsbrec_service_monitor_index_set_logical_port(
> + key, db_rec->logical_port);
> +
> + const struct icsbrec_service_monitor *ic_rec;
> + ICSBREC_SERVICE_MONITOR_FOR_EACH_EQUAL (ic_rec, key,
> + ctx->icsbrec_service_monitor_by_target_az_logical_port) {
> + if (db_rec->port == ic_rec->port &&
> + !strcmp(db_rec->ip, ic_rec->ip) &&
> + !strcmp(db_rec->src_ip, ic_rec->src_ip) &&
> + !strcmp(db_rec->protocol, ic_rec->protocol) &&
> + !strcmp(db_rec->logical_port, ic_rec->logical_port)) {
> + icsbrec_service_monitor_index_destroy_row(key);
> + return ic_rec;
> + }
> + }
> +
> + icsbrec_service_monitor_index_destroy_row(key);
> +
> + return NULL;
> +}
> +
> +static void
> +create_service_monitor_data(struct ic_context *ctx,
> + struct sync_service_monitor_data *sync_data)
> +{
> + const struct sbrec_sb_global *ic_sb = sbrec_sb_global_first(
> + ctx->ovnsb_idl);
> + const char *svc_monitor_mac = smap_get_def(&ic_sb->options,
> + "svc_monitor_mac", "");
> +
Here too, smap_get_def() can never return NULL.
> + if (svc_monitor_mac) {
> + sync_data->prpg_svc_monitor_mac = xstrdup(svc_monitor_mac);
So, if SB.Options:svc_monitor_mac is not present
sync_data->prpg_svc_monitor_mac will be "".
I know in practice this should be at most transient (because ovn-northd
will probably reconcile the SB.Options:svc_monitor_mac to an actual MAC
address value) but should we just return early here if the mac is not
currently set?
Or do we need to something else?
> + }
> +
> + create_pushed_svcs_mon(ctx, &sync_data->pushed_svcs_map);
> + create_synced_svcs_mon(ctx, &sync_data->synced_svcs_map);
> + create_local_ic_svcs_map(ctx, &sync_data->local_ic_svcs_map);
> + create_local_sb_svcs_map(ctx, &sync_data->local_sb_svcs_map);
> +}
> +
Regards,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
