Hi Dumitru! Thanks for the review!
I will correct your comments regarding the need to remove checks for
zero returned arguments. Regarding the MAC address check - I'll fix that
too. thanks! I'll send you the patch as soon as possible. do you have
any comments regarding the other patches in this patch series?
On 18.08.2025 16:19, Dumitru Ceara wrote:
> 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
>
--
regards,
Alexandra.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
