Hi Dumitru, What I had in mind was something like ic-adv-address-set option in the NB port-group entity. If that is set to true, the internally generated address-set for the port-group would be advertised. Similar for VIPs. If I understand correctly you are also saying the same.
Thanks, Sragdhara From: Dumitru Ceara <[email protected]> Date: Thursday, July 16, 2026 at 6:01 AM To: Sragdhara Datta Chaudhuri <[email protected]>; [email protected] <[email protected]> Cc: Numan Siddique <[email protected]> Subject: Re: [ovs-dev] [PATCH OVN v1] ovn-ic: Address set synchronization across AZs. !-------------------------------------------------------------------| CAUTION: External Email |-------------------------------------------------------------------! On 7/15/26 10:53 AM, Sragdhara Datta Chaudhuri wrote: > Hi Dumitru, > Hi Sragdhara, > Thanks a lot for your comments. I have addressed all except the first > one. The reason for advertising the SB Address_Set table is because it > is a superset of what's available in NB Address_Set. Northd synthesizes > address sets in SB that have no NB Address_Set counterpart — e.g. the > port-group-derived sets (<pg>_ip4, <pg>_ip6). By sourcing from SB, we > can support advertising those synthesized sets in the future (e.g. a > user might want to advertise the IPs of a port group to remote AZs for > use in ACL policies). Going NB-only restricts the feature to only user- > defined NB address sets. > But in your implementation you only advertise address sets that have NB.Address_Set.options:ic-adv=true. If we really want to advertise the implicit SB Address_Sets how would you envision doing that? They don't have a NB counterpart. How would users opt-in for their advertisement? A global option seems easy but not so nice. So I guess we'll need options to opt-in for the following types of implicit address sets: - port group v4/v6 ones - LB VIPs address sets (would this be per load balancer or per switch?) I'll think some more about it when reviewing your v2 patch. Let's continue the discussion there. Thanks, Dumitru > > Thanks, > > Sragdhara > > > > *From: *Dumitru Ceara <[email protected]> > *Date: *Thursday, July 9, 2026 at 6:26 AM > *To: *Sragdhara Datta Chaudhuri <[email protected]>; ovs- > [email protected] <[email protected]> > *Cc: *Numan Siddique <[email protected]> > *Subject: *Re: [ovs-dev] [PATCH OVN v1] ovn-ic: Address set > synchronization across AZs. > > !-------------------------------------------------------------------| > CAUTION: External Email > > |-------------------------------------------------------------------! > > Hi Sragdhara, > > Sorry for the delay in reviewing this patch and thanks for the new feature! > > On 5/18/26 5:07 AM, Sragdhara Datta Chaudhuri wrote: >> Advertise address_set from one AZ to others via ovn-ic. >> The address sets from SB are advertised to the IC-SB Address_Set. >> The address sets from IC SB that belong to remote AZ are synced >> to the local NB Address_Set. >> >> A new table Address_Set has been added to IC-SB. >> "Address_Set": { >> "columns": { >> "name": {"type": "string"}, >> "addresses": {"type": {"key": "string", >> "min": 0, >> "max": "unlimited"}}, >> "availability_zone": {"type": {"key": {"type": "uuid", >> "refTable": "Availability_Zone"}}}, >> "external_ids": { >> "type": {"key": "string", "value": "string", >> "min": 0, "max": "unlimited"}}}, >> "indexes": [["name"]], >> "isRoot": true} >> } >> >> New "options" field has been added to NB Address_Set table. >> ic-adv - A boolean value that enables advertisement of this >> Address_Set to IC-SB. Default is false. >> New "options" field has been added to SB Address_Set table. >> The options from NB Address_Set gets synced to the SB entry. >> >> Two new nb_global options have been added: >> ic-as-adv - If this is set to true, the Address_Sets in this AZ that have >> option ic-adv set to true get advertised to the IC-SB. >> Default is false. >> ic-as-learn - If this is set to true, the Address_Sets in IC-SB get >> synced to the NB Address_Set table of this AZ. Default >> value of the option is false. The synced address_sets >> would have external-id "ic-learnt" set to true. >> >> The existing northd code that syncs Address_Set from NB to SB has been >> enhanced to sync "options". >> >> New logic has been added in ovn-ic to sync address sets with IC-SB. >> Advertise logic (from SB to IC-SB): >> - Each SB address set that needs to be advertised (ic-adv option set), >> check if it is already present in IC-SB. If not, create new entry in >> IC-SB. Otherwise sync addresses from local address set to IC-SB entry. > > Why do we do that based on SB address sets? What's preventing us from > advertising NB address sets from a given AZ? Like that we won't have to > change anything in ovn-northd, we won't need the SB.Address_Set.options > column, etc. > > >> - Delete extra address sets in IC-SB that were earlier learnt from this >> AZ, but is no longer present, or not enabled for advertisement. >> >> Learning logic (from IC-SB to NB): >> - For each NB Address set entries that were earlier learnt from IC-SB >> (external-id "ic-learnt" set to true), check if it is still present >> in IC-SB. If not, delete local entry in NB. If yes, sync addresses >> from IC-SB to NB. >> - Any remote address sets in IC-SB (AZ not same as local AZ) that is not >> present in local AZ, create local entry in NB with external-id >> "ic-learnt" set to true. >> >> Signed-off-by: Sragdhara Datta Chaudhuri <[email protected]> >> Acked-by: Priyankar Jain <[email protected]> >> --- >> ic/inc-proc-ic.c | 12 ++- >> ic/ovn-ic.c | 188 ++++++++++++++++++++++++++++++++++++++++++++ >> northd/en-sync-sb.c | 35 +++++++-- >> ovn-ic-sb.ovsschema | 17 +++- >> ovn-ic-sb.xml | 26 ++++++ >> ovn-nb.ovsschema | 9 ++- >> ovn-nb.xml | 31 ++++++++ >> ovn-sb.ovsschema | 9 ++- >> ovn-sb.xml | 4 + >> tests/ovn-ic.at | 172 ++++++++++++++++++++++++++++++++++++++++ > > We should add a NEWS item for this new feature. > >> 10 files changed, 486 insertions(+), 17 deletions(-) >> >> diff --git a/ic/inc-proc-ic.c b/ic/inc-proc-ic.c >> index 2c0420292..bbcbcdd17 100644 >> --- a/ic/inc-proc-ic.c >> +++ b/ic/inc-proc-ic.c >> @@ -41,7 +41,8 @@ VLOG_DEFINE_THIS_MODULE(inc_proc_ic); >> NB_NODE(logical_switch, "logical_switch") \ >> NB_NODE(logical_switch_port, "logical_switch_port") \ >> NB_NODE(load_balancer, "load_balancer") \ >> - NB_NODE(load_balancer_group, "load_balancer_group") >> + NB_NODE(load_balancer_group, "load_balancer_group") \ >> + NB_NODE(address_set, "address_set") >> >> enum nb_engine_node { >> #define NB_NODE(NAME, NAME_STR) NB_##NAME, >> @@ -66,7 +67,8 @@ VLOG_DEFINE_THIS_MODULE(inc_proc_ic); >> SB_NODE(datapath_binding, "datapath_binding") \ >> SB_NODE(port_binding, "port_binding") \ >> SB_NODE(service_monitor, "service_monitor") \ >> - SB_NODE(learned_route, "learned_route") >> + SB_NODE(learned_route, "learned_route") \ >> + SB_NODE(address_set, "address_set") >> >> enum sb_engine_node { >> #define SB_NODE(NAME, NAME_STR) SB_##NAME, >> @@ -114,7 +116,8 @@ VLOG_DEFINE_THIS_MODULE(inc_proc_ic); >> ICSB_NODE(datapath_binding, "datapath_binding") \ >> ICSB_NODE(encap, "encap") \ >> ICSB_NODE(gateway, "gateway") \ >> - ICSB_NODE(port_binding, "port_binding") >> + ICSB_NODE(port_binding, "port_binding") \ >> + ICSB_NODE(address_set, "address_set") >> >> enum icsb_engine_node { >> #define ICSB_NODE(NAME, NAME_STR) ICSB_##NAME, >> @@ -176,6 +179,7 @@ void inc_proc_ic_init(struct ovsdb_idl_loop *nb, >> engine_add_input(&en_ic, &en_nb_logical_switch_port, NULL); >> engine_add_input(&en_ic, &en_nb_load_balancer, NULL); >> engine_add_input(&en_ic, &en_nb_load_balancer_group, NULL); >> + engine_add_input(&en_ic, &en_nb_address_set, NULL); >> >> engine_add_input(&en_ic, &en_sb_sb_global, NULL); >> engine_add_input(&en_ic, &en_sb_chassis, NULL); >> @@ -184,6 +188,7 @@ void inc_proc_ic_init(struct ovsdb_idl_loop *nb, >> engine_add_input(&en_ic, &en_sb_port_binding, NULL); >> engine_add_input(&en_ic, &en_sb_service_monitor, NULL); >> engine_add_input(&en_ic, &en_sb_learned_route, NULL); >> + engine_add_input(&en_ic, &en_sb_address_set, NULL); >> >> engine_add_input(&en_ic, &en_icnb_ic_nb_global, NULL); >> engine_add_input(&en_ic, &en_icnb_transit_switch, NULL); >> @@ -198,6 +203,7 @@ void inc_proc_ic_init(struct ovsdb_idl_loop *nb, >> engine_add_input(&en_ic, &en_icsb_gateway, NULL); >> engine_add_input(&en_ic, &en_icsb_route, NULL); >> engine_add_input(&en_ic, &en_icsb_datapath_binding, NULL); >> + engine_add_input(&en_ic, &en_icsb_address_set, NULL); >> >> struct engine_arg engine_arg = { >> .nb_idl = nb->idl, >> diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c >> index ba9490658..c1718cca8 100644 >> --- a/ic/ovn-ic.c >> +++ b/ic/ovn-ic.c >> @@ -526,6 +526,173 @@ sync_sb_gw_to_isb(struct ic_context *ctx, >> free(isb_encaps); >> } >> >> +static void >> +nb_addr_set_apply_diff(const void *arg, const char *item, bool add) >> +{ >> + const struct nbrec_address_set *as = arg; >> + if (add) { >> + nbrec_address_set_update_addresses_addvalue(as, item); >> + } else { >> + nbrec_address_set_update_addresses_delvalue(as, item); >> + } >> +} >> + >> +static void >> +update_nb_addr_set(const struct icsbrec_address_set *icsb_as, >> + const struct nbrec_address_set *nb_as) >> +{ >> + struct sorted_array nb_addrs = >> + sorted_array_from_dbrec(nb_as, addresses); >> + struct sorted_array icsb_addrs = >> + sorted_array_from_dbrec(icsb_as, addresses); >> + sorted_array_apply_diff(&icsb_addrs, &nb_addrs, >> + nb_addr_set_apply_diff, nb_as); >> + sorted_array_destroy(&icsb_addrs); >> + sorted_array_destroy(&nb_addrs); >> +} >> + >> +static void >> +icsb_addr_set_apply_diff(const void *arg, const char *item, bool add) >> +{ >> + const struct icsbrec_address_set *as = arg; >> + if (add) { >> + icsbrec_address_set_update_addresses_addvalue(as, item); >> + } else { >> + icsbrec_address_set_update_addresses_delvalue(as, item); >> + } >> +} >> + >> +static void >> +update_icsb_addr_set(struct sorted_array *nb_addrs, >> + const struct icsbrec_address_set *icsb_as) >> +{ >> + struct sorted_array icsb_addrs = >> + sorted_array_from_dbrec(icsb_as, addresses); >> + sorted_array_apply_diff(nb_addrs, &icsb_addrs, >> + icsb_addr_set_apply_diff, icsb_as); >> + sorted_array_destroy(&icsb_addrs); >> +} >> + >> +static void >> +sync_addr_set_to_icsb(struct ovsdb_idl_txn *ovnisb_txn, >> + const struct sbrec_address_set *sb_as, >> + const struct icsbrec_address_set *icsb_as, >> + const struct icsbrec_availability_zone *az) >> +{ >> + struct sorted_array addrs = >> + sorted_array_from_dbrec(sb_as, addresses); >> + if (!icsb_as) { >> + icsb_as = icsbrec_address_set_insert(ovnisb_txn); >> + icsbrec_address_set_set_name(icsb_as, sb_as->name); >> + icsbrec_address_set_set_availability_zone(icsb_as, az); >> + icsbrec_address_set_set_addresses(icsb_as, addrs.arr, addrs.n); >> + } else { >> + update_icsb_addr_set(&addrs, icsb_as); >> + } >> + sorted_array_destroy(&addrs); >> +} >> + >> +static void >> +sync_addr_set_from_icsb(struct ovsdb_idl_txn *ovnnb_txn, >> + const struct icsbrec_address_set *icsb_as) >> +{ >> + struct nbrec_address_set *nb_as; >> + struct sorted_array addrs = >> + sorted_array_from_dbrec(icsb_as, addresses); >> + >> + nb_as = nbrec_address_set_insert(ovnnb_txn); >> + nbrec_address_set_set_name(nb_as, icsb_as->name); >> + nbrec_address_set_update_external_ids_setkey(nb_as, "ic-learnt", > "true"); >> + nbrec_address_set_set_addresses(nb_as, addrs.arr, addrs.n); >> + sorted_array_destroy(&addrs); >> +} >> + >> +static void >> +address_set_run(struct ic_context *ctx) >> +{ >> + if (!ctx->ovnisb_unlocked_txn || !ctx->ovnnb_txn || !ctx- >>ovnsb_txn) { >> + return; >> + } >> + >> + struct shash ic_local_as = SHASH_INITIALIZER(&ic_local_as); >> + struct shash ic_remote_as = SHASH_INITIALIZER(&ic_remote_as); >> + const struct icsbrec_address_set *ic_as; >> + ICSBREC_ADDRESS_SET_FOR_EACH (ic_as, ctx->ovnisb_unlocked_idl) { >> + if (ic_as->availability_zone == ctx->runned_az) { >> + shash_add(&ic_local_as, ic_as->name, ic_as); >> + } else { >> + shash_add(&ic_remote_as, ic_as->name, ic_as); >> + } >> + } >> + >> + const struct nbrec_nb_global *nb_global = >> + nbrec_nb_global_first(ctx->ovnnb_idl); >> + ovs_assert(nb_global); >> + bool global_learn = smap_get_bool(&nb_global->options, "ic-as-learn", >> + false); >> + bool global_adv = smap_get_bool(&nb_global->options, "ic-as-adv", > false); >> + >> + /* Advertise address set - from SB to IC-SB: >> + * - Each SB address set that needs to be advertised (ic-adv > option set), >> + * check if it is already present in IC-SB. If not, create new > entry in >> + * IC-SB. Otherwise sync addresses from local address set to IC- > SB entry. >> + * - Delete extra address sets in IC-SB that were earlier learnt > from this >> + * AZ, but is no longer present, or not enabled for advertisement. >> + */ >> + if (global_adv) { >> + const struct sbrec_address_set *sb_as; >> + SBREC_ADDRESS_SET_FOR_EACH (sb_as, ctx->ovnsb_idl) { >> + if (smap_get_bool(&sb_as->options, "ic-adv", false)) { >> + const struct icsbrec_address_set *icsb_as; >> + icsb_as = shash_find_and_delete(&ic_local_as, sb_as- >>name); >> + sync_addr_set_to_icsb(ctx->ovnisb_unlocked_txn, > sb_as, icsb_as, >> + ctx->runned_az); >> + } >> + } >> + } >> + struct shash_node *node; >> + SHASH_FOR_EACH (node, &ic_local_as) { >> + icsbrec_address_set_delete(node->data); >> + } >> + shash_destroy(&ic_local_as); >> + >> + /* Learn address set - from IC-SB to NB: >> + * - For each NB Address set entries that were earlier learnt > from IC-SB >> + * (external-id "ic-learnt" set to true), check if it is still > present >> + * in IC-SB. If not, delete local entry in NB. If yes, sync > addresses >> + * from IC-SB to NB. >> + * - Any remote address sets in IC-SB (AZ not same as local AZ) > that is not >> + * present in local AZ, create local entry in NB with external-id >> + * "ic-learnt" set to true. >> + */ >> + const struct nbrec_address_set *nb_as; >> + NBREC_ADDRESS_SET_FOR_EACH (nb_as, ctx->ovnnb_idl) { > > This needs to be NBREC_ADDRESS_SET_FOR_EACH_SAFE() as we might delete nb_as. > >> + const struct icsbrec_address_set *icsb_as; >> + icsb_as = shash_find_and_delete(&ic_remote_as, nb_as->name); >> + if (smap_get_bool(&nb_as->external_ids, "ic-learnt", false)) { >> + if (!icsb_as || !global_learn) { >> + nbrec_address_set_delete(nb_as); >> + } else { >> + update_nb_addr_set(icsb_as, nb_as); >> + } >> + } >> + } >> + >> + if (global_learn) { >> + SHASH_FOR_EACH (node, &ic_remote_as) { >> + /* In case local address-set with same name exists, we >> + * will not overwrite it because such address sets are > already >> + * removed from ic_remote_as in the loop above. >> + */ >> + if (node->data) { > > node->data can never be NULL here, right? > >> + sync_addr_set_from_icsb(ctx->ovnnb_txn, node->data); >> + } >> + } >> + } >> + shash_destroy(&ic_remote_as); >> +} >> + >> + > > Nit: one empty line too many. > >> static void >> gateway_run(struct ic_context *ctx) >> { >> @@ -3199,6 +3366,7 @@ ovn_db_run(struct ic_context *ctx) >> port_binding_run(ctx); >> route_run(ctx); >> sync_service_monitor(ctx); >> + address_set_run(ctx); >> >> ovn_destroy_tnlids(&dp_tnlids); >> shash_destroy(&isb_ts_dps); >> @@ -3536,6 +3704,17 @@ main(int argc, char *argv[]) >> ovsdb_idl_track_add_column(ovnnb_idl_loop.idl, >> > &nbrec_load_balancer_group_col_load_balancer); >> >> + ovsdb_idl_add_table(ovnnb_idl_loop.idl, &nbrec_table_address_set); >> + ovsdb_idl_track_add_column(ovnnb_idl_loop.idl, >> + &nbrec_address_set_col_name); >> + ovsdb_idl_track_add_column(ovnnb_idl_loop.idl, >> + &nbrec_address_set_col_addresses); >> + ovsdb_idl_track_add_column(ovnnb_idl_loop.idl, >> + &nbrec_address_set_col_options); >> + ovsdb_idl_track_add_column(ovnnb_idl_loop.idl, >> + &nbrec_address_set_col_external_ids); >> + >> + >> /* ovn-sb db. */ >> struct ovsdb_idl_loop ovnsb_idl_loop = OVSDB_IDL_LOOP_INITIALIZER( >> ovsdb_idl_create(ovnsb_db, &sbrec_idl_class, false, true)); >> @@ -3621,6 +3800,15 @@ main(int argc, char *argv[]) >> &sbrec_learned_route_col_ip_prefix); >> ovsdb_idl_track_add_column(ovnsb_idl_loop.idl, >> &sbrec_learned_route_col_datapath); >> + >> + ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_address_set); >> + ovsdb_idl_track_add_column(ovnsb_idl_loop.idl, >> + &sbrec_address_set_col_name); >> + ovsdb_idl_track_add_column(ovnsb_idl_loop.idl, >> + &sbrec_address_set_col_addresses); >> + ovsdb_idl_track_add_column(ovnsb_idl_loop.idl, >> + &sbrec_address_set_col_options); >> + >> /* Create IDL indexes */ >> struct ovsdb_idl_index *nbrec_ls_by_name >> = ovsdb_idl_index_create1(ovnnb_idl_loop.idl, >> diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c >> index 0b2a85b3b..b3d96af16 100644 >> --- a/northd/en-sync-sb.c >> +++ b/northd/en-sync-sb.c >> @@ -43,7 +43,8 @@ VLOG_DEFINE_THIS_MODULE(en_sync_to_sb); >> >> static void sync_addr_set(struct ovsdb_idl_txn *ovnsb_txn, const char > *name, >> struct sorted_array *addresses, >> - struct shash *sb_address_sets); >> + struct shash *sb_address_sets, >> + const struct smap *options); >> static void sync_addr_sets(struct ovsdb_idl_txn *ovnsb_txn, >> const struct nbrec_address_set_table *, >> const struct nbrec_port_group_table *, >> @@ -118,6 +119,18 @@ en_sync_to_sb_addr_set_cleanup(void *data OVS_UNUSED) >> >> } >> >> +static void >> +sync_address_set_options(const struct sbrec_address_set *sb_addr_set, >> + const struct smap *nb_options) > > Instead of this function I'd just pass the 'nb_options' smap to > update_sb_addr_set(). And for the creation part I'd just call > sbrec_address_set_set_options() directly. > >> +{ >> + if (!smap_equal(&sb_addr_set->options, nb_options)) { >> + struct smap new_options; >> + smap_clone(&new_options, nb_options); >> + sbrec_address_set_set_options(sb_addr_set, &new_options); >> + smap_destroy(&new_options); >> + } >> +} >> + >> enum engine_input_handler_result >> sync_to_sb_addr_set_nb_address_set_handler(struct engine_node *node, >> void *data OVS_UNUSED) >> @@ -153,6 +166,7 @@ sync_to_sb_addr_set_nb_address_set_handler(struct > engine_node *node, >> sorted_array_from_dbrec(nb_addr_set, addresses); >> update_sb_addr_set(&addrs, sb_addr_set); >> sorted_array_destroy(&addrs); >> + sync_address_set_options(sb_addr_set, &nb_addr_set->options); >> } >> >> return EN_HANDLED_UNCHANGED; >> @@ -449,7 +463,8 @@ sync_to_sb_pb_lr_stateful_handler(struct > engine_node *node, >> static void >> sync_addr_set(struct ovsdb_idl_txn *ovnsb_txn, const char *name, >> struct sorted_array *addresses, >> - struct shash *sb_address_sets) >> + struct shash *sb_address_sets, >> + const struct smap *options) >> { >> const struct sbrec_address_set *sb_address_set; >> sb_address_set = shash_find_and_delete(sb_address_sets, >> @@ -462,6 +477,9 @@ sync_addr_set(struct ovsdb_idl_txn *ovnsb_txn, > const char *name, >> } else { >> update_sb_addr_set(addresses, sb_address_set); >> } >> + if (options) { >> + sync_address_set_options(sb_address_set, options); >> + } >> } >> >> /* OVN_Southbound Address_Set table contains same records as in north >> @@ -500,7 +518,7 @@ sync_addr_sets(struct ovsdb_idl_txn *ovnsb_txn, >> const char *svc_macs[] = {svc_monitor_macp, svc_monitor_macp_dst}; >> struct sorted_array svc = >> sorted_array_from_unsorted(svc_macs, ARRAY_SIZE(svc_macs), > false); >> - sync_addr_set(ovnsb_txn, "svc_monitor_mac", &svc, &sb_address_sets); >> + sync_addr_set(ovnsb_txn, "svc_monitor_mac", &svc, > &sb_address_sets, NULL); >> sorted_array_destroy(&svc); >> >> /* sync port group generated address sets first */ >> @@ -519,9 +537,9 @@ sync_addr_sets(struct ovsdb_idl_txn *ovnsb_txn, >> sorted_array_from_svec(&ipv6_addrs); >> >> sync_addr_set(ovnsb_txn, ipv4_addrs_name, >> - &ipv4_addrs_sorted, &sb_address_sets); >> + &ipv4_addrs_sorted, &sb_address_sets, NULL); >> sync_addr_set(ovnsb_txn, ipv6_addrs_name, >> - &ipv6_addrs_sorted, &sb_address_sets); >> + &ipv6_addrs_sorted, &sb_address_sets, NULL); >> sorted_array_destroy(&ipv4_addrs_sorted); >> sorted_array_destroy(&ipv6_addrs_sorted); >> svec_destroy(&ipv4_addrs); >> @@ -544,7 +562,7 @@ sync_addr_sets(struct ovsdb_idl_txn *ovnsb_txn, >> &lr_stateful_rec->lb_ips->ips_v4_reachable); >> >> sync_addr_set(ovnsb_txn, ipv4_addrs_name, >> - &ipv4_addrs_sorted, &sb_address_sets); >> + &ipv4_addrs_sorted, &sb_address_sets, NULL); >> sorted_array_destroy(&ipv4_addrs_sorted); >> free(ipv4_addrs_name); >> } >> @@ -556,7 +574,7 @@ sync_addr_sets(struct ovsdb_idl_txn *ovnsb_txn, >> &lr_stateful_rec->lb_ips->ips_v6_reachable); >> >> sync_addr_set(ovnsb_txn, ipv6_addrs_name, >> - &ipv6_addrs_sorted, &sb_address_sets); >> + &ipv6_addrs_sorted, &sb_address_sets, NULL); >> sorted_array_destroy(&ipv6_addrs_sorted); >> free(ipv6_addrs_name); >> } >> @@ -570,7 +588,8 @@ sync_addr_sets(struct ovsdb_idl_txn *ovnsb_txn, >> struct sorted_array addrs = >> sorted_array_from_dbrec(nb_address_set, addresses); >> sync_addr_set(ovnsb_txn, nb_address_set->name, >> - &addrs, &sb_address_sets); >> + &addrs, &sb_address_sets, >> + &nb_address_set->options); >> sorted_array_destroy(&addrs); >> } >> >> diff --git a/ovn-ic-sb.ovsschema b/ovn-ic-sb.ovsschema >> index e0e0fef5e..dd1b2533c 100644 >> --- a/ovn-ic-sb.ovsschema >> +++ b/ovn-ic-sb.ovsschema >> @@ -1,7 +1,7 @@ >> { >> "name": "OVN_IC_Southbound", >> - "version": "2.5.0", >> - "cksum": "1892994110 9713", >> + "version": "2.6.0", >> + "cksum": "3333694935 10343", >> "tables": { >> "IC_SB_Global": { >> "columns": { >> @@ -192,6 +192,19 @@ >> "external_ids": { >> "type": {"key": "string", "value": "string", >> "min": 0, "max": "unlimited"}}}, >> + "isRoot": true}, >> + "Address_Set": { >> + "columns": { >> + "name": {"type": "string"}, >> + "addresses": {"type": {"key": "string", >> + "min": 0, >> + "max": "unlimited"}}, >> + "availability_zone": {"type": {"key": {"type": "uuid", >> + "refTable": "Availability_Zone"}}}, >> + "external_ids": { >> + "type": {"key": "string", "value": "string", >> + "min": 0, "max": "unlimited"}}}, >> + "indexes": [["name"]], > > How are we going to handle potential conflicts, i.e., the same address > set name being advertised from different availability zones? Should we > include "Availability_zone" into the index? I.e.: > > "indexes": [["name", "availability_zone"]] > > Or do we expect that to be a misconfiguration from the CMS side? In > which case we should document it. > > Maybe we should do both, what do you think? > >> "isRoot": true} >> } >> } >> diff --git a/ovn-ic-sb.xml b/ovn-ic-sb.xml >> index f30760100..4fe491fac 100644 >> --- a/ovn-ic-sb.xml >> +++ b/ovn-ic-sb.xml >> @@ -775,4 +775,30 @@ >> Copy from source SBDB record. >> </column> >> </table> >> + >> + <table name="Address_Set" title="Address Sets"> >> + <p> >> + Each row in this table represents an Address_Set advertised. >> + </p> >> + >> + <column name="name"> >> + A name for the address set. Names are ASCII and must match >> + <code>[a-zA-Z_.][a-zA-Z_.0-9]*</code>. >> + </column> >> + >> + <column name="addresses"> >> + The set of addresses in string form. >> + </column> >> + >> + <column name="availability_zone"> >> + The availability zone that has advertised the Address_Set. >> + </column> >> + >> + <group title="Common Columns"> >> + <column name="external_ids"> >> + See <em>External IDs</em> at the beginning of this document. >> + </column> >> + </group> >> + </table> >> + >> </database> >> diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema >> index e5945b831..00909fff4 100644 >> --- a/ovn-nb.ovsschema >> +++ b/ovn-nb.ovsschema >> @@ -1,7 +1,7 @@ >> { >> "name": "OVN_Northbound", >> - "version": "7.18.0", >> - "cksum": "1537030958 45190", >> + "version": "7.18.1", >> + "cksum": "2442107800 45407", >> "tables": { >> "NB_Global": { >> "columns": { >> @@ -292,6 +292,11 @@ >> "addresses": {"type": {"key": "string", >> "min": 0, >> "max": "unlimited"}}, >> + "options": { >> + "type": {"key": "string", >> + "value": "string", >> + "min": 0, >> + "max": "unlimited"}}, >> "external_ids": { >> "type": {"key": "string", "value": "string", >> "min": 0, "max": "unlimited"}}}, >> diff --git a/ovn-nb.xml b/ovn-nb.xml >> index 2e8a6a6f1..d98f3244d 100644 >> --- a/ovn-nb.xml >> +++ b/ovn-nb.xml >> @@ -583,6 +583,25 @@ >> </column> >> </group> >> >> + <group title="Options for interconnection address_set > advertisement"> >> + <p> >> + These options control whether address_sets propagate from > one OVN >> + deployment to another via the global <ref > db="OVN_IC_Southbound"/>. >> + </p> >> + >> + <column name="options" key="ic-as-adv"> >> + A boolean value that enables address_set advertisement to > the global >> + <ref db="OVN_IC_Southbound"/> database. Default is >> + <code>false</code>. >> + </column> >> + >> + <column name="options" key="ic-as-learn"> >> + A boolean value that enables address_set learning from the > global >> + <ref db="OVN_IC_Southbound"/> database. Default is >> + <code>false</code>. >> + </column> >> + </group> >> + >> </group> >> >> <group title="Connection Options"> >> @@ -2409,6 +2428,18 @@ >> See <em>External IDs</em> at the beginning of this document. >> </column> >> </group> >> + >> + <group title="Common options"> > > I'd remove the "group" and just document the "options" column directly. > >> + <column name="options"> >> + This column provides general key/value settings. The supported >> + options are described individually below. >> + </column> >> + >> + <column name="options" key="ic-adv"> >> + A boolean value that enables advertisement of this Address_Set to >> + the global OVN_IC_Southbound database. Default is false. > > nit: s/false/<code>false</code> > >> + </column> >> + </group> >> </table> >> >> <table name="Port_Group" title="Port Groups"> >> diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema >> index d9a91739c..7057178ce 100644 >> --- a/ovn-sb.ovsschema >> +++ b/ovn-sb.ovsschema >> @@ -1,7 +1,7 @@ >> { >> "name": "OVN_Southbound", >> "version": "21.8.0", >> - "cksum": "614397313 36713", >> + "cksum": "2227484712 36930", >> "tables": { >> "SB_Global": { >> "columns": { >> @@ -77,7 +77,12 @@ >> "name": {"type": "string"}, >> "addresses": {"type": {"key": "string", >> "min": 0, >> - "max": "unlimited"}}}, >> + "max": "unlimited"}}, >> + "options": { >> + "type": {"key": "string", >> + "value": "string", >> + "min": 0, >> + "max": "unlimited"}}}, > > We should omit alerting on changes of the new column in ovn-northd's > main() function: > > ovsdb_idl_omit_alert(ovnsb_idl_loop.idl, &sbrec_address_set_col_options); > > or even better (instead of omitting individual columns): > > for (size_t i = 0; i < SBREC_ADDRESS_SET_N_COLUMNS; i++) { > ovsdb_idl_omit_alert(ovnsb_idl_loop.idl, > &sbrec_address_set_columns[i]); > } > >> "indexes": [["name"]], >> "isRoot": true}, >> "Port_Group": { >> diff --git a/ovn-sb.xml b/ovn-sb.xml >> index 46568da62..51c16600d 100644 >> --- a/ovn-sb.xml >> +++ b/ovn-sb.xml >> @@ -588,6 +588,10 @@ >> >> <column name="name"/> >> <column name="addresses"/> >> + <group title="Common Columns"> >> + <column name="options"> >> + </column> >> + </group> >> </table> >> >> <table name="Port_Group" title="Port Groups"> >> diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at >> index 0fa7c4f29..45f64e551 100644 >> --- a/tests/ovn-ic.at >> +++ b/tests/ovn-ic.at >> @@ -487,6 +487,178 @@ OVN_CLEANUP_IC([az1], [az2]) >> AT_CLEANUP >> ]) >> >> +OVN_FOR_EACH_NORTHD([ >> +AT_SETUP([ovn-ic -- address-set sync]) >> + >> +ovn_init_ic_db >> +ovn-ic-nbctl ts-add ts1 >> + >> +for i in 1 2; do >> + ovn_start az$i >> + ovn_as az$i >> + check ovn-ic-nbctl --wait=sb sync >> + # Enable address-set learning at AZ level >> + check ovn-nbctl set nb_global . options:ic-as-learn=true >> + # Enable address-set advertising at AZ level >> + check ovn-nbctl set nb_global . options:ic-as-adv=true >> + >> + # Create address-set to advertise >> + check_uuid ovn-nbctl create Address_Set name=az$i-as1 > addresses=\"$i.1.1.1\",\"$i.1.1.2\" options:ic-adv=true >> +done >> + >> +wait_row_count ic-sb:Address_Set 2 >> +AT_CHECK([ovn-ic-sbctl --columns=addresses find Address_Set name=az1- > as1], [0], [dnl >> +addresses : [["1.1.1.1", "1.1.1.2"]] >> +]) >> + >> +AT_CHECK([ovn-ic-sbctl --columns=addresses find Address_Set name=az2- > as1], [0], [dnl >> +addresses : [["2.1.1.1", "2.1.1.2"]] >> +]) >> + >> +for i in 1 2; do >> + OVS_WAIT_UNTIL([ovn_as az$i ovn-nbctl list Address_Set | grep > learnt]) >> +done >> + >> +AT_CHECK([ovn_as az1 ovn-nbctl --columns=addresses,external_ids list > Address_Set az2-as1], [0], [dnl >> +addresses : [["2.1.1.1", "2.1.1.2"]] >> +external_ids : {ic-learnt="true"} >> +]) >> + >> +AT_CHECK([ovn_as az2 ovn-nbctl --columns=addresses,external_ids list > Address_Set az1-as1], [0], [dnl >> +addresses : [["1.1.1.1", "1.1.1.2"]] >> +external_ids : {ic-learnt="true"} >> +]) >> + >> +# Update AZ1 address_set (add 2 new IPs and delete one) and check it > is synced to IC and AZ2. >> +ovn_as az1 ovn-nbctl set Address_Set az1-as1 addresses=\"1.1.1.1\", > \"1.1.1.3\",\"1.1.1.4\" >> +AT_CHECK([ovn-ic-sbctl --columns=addresses find Address_Set name=az1- > as1], [0], [dnl > > This is racy, ovn-northd needs to run first, ovn-ic needs to run first. > We're missing some --wait calls and we should probably use > OVS_WAIT_FOR_OUTPUT() or wait_row_count ic-sb as you do below. > >> +addresses : [["1.1.1.1", "1.1.1.3", "1.1.1.4"]] >> +]) >> +AT_CHECK([ovn_as az2 ovn-nbctl --columns=addresses,external_ids list > Address_Set az1-as1], [0], [dnl >> +addresses : [["1.1.1.1", "1.1.1.3", "1.1.1.4"]] >> +external_ids : {ic-learnt="true"} >> +]) >> + >> +# Add 2 new address-sets in AZ1 - one to advertise, another not to. >> +ovn_as az1 ovn-nbctl create Address_Set name=az1-as2 > addresses=\"1.1.2.1\",\"1.1.2.2\" options:ic-adv=true >> +ovn_as az1 ovn-nbctl create Address_Set name=az1-as3 > addresses=\"1.1.3.1\",\"1.1.3.2\" >> +wait_row_count ic-sb:Address_Set 3 >> +AT_CHECK([ovn-ic-sbctl --columns=addresses find Address_Set name=az1- > as2], [0], [dnl >> +addresses : [["1.1.2.1", "1.1.2.2"]] >> +]) >> +AT_CHECK([ovn-ic-sbctl --columns=addresses find Address_Set name=az1- > as3], [0], [dnl >> +]) >> +AT_CHECK([ovn_as az2 ovn-nbctl --columns=addresses,external_ids list > Address_Set az1-as2], [0], [dnl >> +addresses : [["1.1.2.1", "1.1.2.2"]] >> +external_ids : {ic-learnt="true"} >> +]) >> +AT_CHECK([ovn_as az2 ovn-nbctl --columns=addresses,external_ids find > Address_Set name=az1-as3], [0], [dnl >> +]) >> + >> +# Now set the option field of the above address-set to advertise it. >> +ovn_as az1 ovn-nbctl set Address_Set az1-as3 options={ic-adv=true} >> +wait_row_count ic-sb:Address_Set 4 >> +AT_CHECK([ovn-ic-sbctl --columns=addresses find Address_Set name=az1- > as3], [0], [dnl >> +addresses : [["1.1.3.1", "1.1.3.2"]] >> +]) >> +AT_CHECK([ovn_as az2 ovn-nbctl --columns=addresses,external_ids list > Address_Set az1-as3], [0], [dnl >> +addresses : [["1.1.3.1", "1.1.3.2"]] >> +external_ids : {ic-learnt="true"} >> +]) >> + >> +# Change the options of one of the address-sets to not advertise. >> +ovn_as az1 ovn-nbctl set Address_Set az1-as2 options={ic-adv=false} >> +wait_row_count ic-sb:Address_Set 3 >> +AT_CHECK([ovn-ic-sbctl --columns=addresses find Address_Set name=az1- > as2], [0], [dnl >> +]) >> +AT_CHECK([ovn_as az2 ovn-nbctl --columns=addresses,external_ids find > Address_Set name=az1-as2], [0], [dnl >> +]) >> + >> +# Delete one of the advertised address-sets in AZ1. >> +ovn_as az1 ovn-nbctl destroy Address_Set az1-as3 >> +wait_row_count ic-sb:Address_Set 2 >> +AT_CHECK([ovn-ic-sbctl --columns=addresses find Address_Set name=az1- > as3], [0], [dnl >> +]) >> +AT_CHECK([ovn_as az2 ovn-nbctl --columns=addresses,external_ids find > Address_Set name=az1-as3], [0], [dnl >> +]) > > This is also racy, even if the IC_SB is updated, what guarantees that > the NB transaction succeeded already? > >> + >> +# Add an IPv6 address-set in AZ2. >> +ovn_as az2 ovn-nbctl create Address_Set name=az2-as2 > addresses=\"2001:0db8:85a3:0000:0000:8a2e:0370:7334\", > \"2001:0db8:85a3:0000:0000:8a2e:0370:7335\" options:ic-adv=true >> +wait_row_count ic-sb:Address_Set 3 >> +AT_CHECK([ovn-ic-sbctl --columns=addresses find Address_Set name=az2- > as2], [0], [dnl >> +addresses : [["2001:0db8:85a3:0000:0000:8a2e:0370:7334", > "2001:0db8:85a3:0000:0000:8a2e:0370:7335"]] >> +]) >> +AT_CHECK([ovn_as az1 ovn-nbctl --columns=addresses,external_ids list > Address_Set az2-as2], [0], [dnl >> +addresses : [["2001:0db8:85a3:0000:0000:8a2e:0370:7334", > "2001:0db8:85a3:0000:0000:8a2e:0370:7335"]] >> +external_ids : {ic-learnt="true"} >> +]) >> + >> +# Set the global option in AZ1 to not learn. >> +ovn_as az1 ovn-nbctl set nb_global . options:ic-as-learn=false >> +OVS_WAIT_WHILE([ovn_as az1 ovn-nbctl list Address_Set | grep learnt]) >> +ovn_as az1 wait_row_count nb:Address_Set 2 >> + >> +# Set the global option in AZ1 back to learn. >> +ovn_as az1 ovn-nbctl set nb_global . options:ic-as-learn=true >> +OVS_WAIT_UNTIL([ovn_as az1 ovn-nbctl list Address_Set | grep learnt]) >> +ovn_as az1 wait_row_count nb:Address_Set 4 >> +AT_CHECK([ovn_as az1 ovn-nbctl --columns=addresses,external_ids list > Address_Set az2-as2], [0], [dnl >> +addresses : [["2001:0db8:85a3:0000:0000:8a2e:0370:7334", > "2001:0db8:85a3:0000:0000:8a2e:0370:7335"]] >> +external_ids : {ic-learnt="true"} >> +]) >> +AT_CHECK([ovn_as az1 ovn-nbctl --columns=addresses,external_ids list > Address_Set az2-as1], [0], [dnl >> +addresses : [["2.1.1.1", "2.1.1.2"]] >> +external_ids : {ic-learnt="true"} >> +]) >> + >> +# Set the global option in AZ2 to not advertise. >> +ovn_as az2 ovn-nbctl set nb_global . options:ic-as-adv=false >> +wait_row_count ic-sb:Address_Set 1 >> +AT_CHECK([ovn-ic-sbctl --columns=addresses find Address_Set name=az2- > as1], [0], [dnl]) >> +AT_CHECK([ovn-ic-sbctl --columns=addresses find Address_Set name=az2- > as2], [0], [dnl]) >> +OVS_WAIT_WHILE([ovn_as az1 ovn-nbctl list Address_Set | grep learnt]) >> + >> +# Set the global option in AZ2 back to advertise. >> +ovn_as az2 ovn-nbctl set nb_global . options:ic-as-adv=true >> +wait_row_count ic-sb:Address_Set 3 >> +ovn_as az1 wait_row_count nb:Address_Set 4 >> +AT_CHECK([ovn-ic-sbctl --columns=addresses find Address_Set name=az2- > as1], [0], [dnl >> +addresses : [["2.1.1.1", "2.1.1.2"]] >> +]) >> +AT_CHECK([ovn-ic-sbctl --columns=addresses find Address_Set name=az2- > as2], [0], [dnl >> +addresses : [["2001:0db8:85a3:0000:0000:8a2e:0370:7334", > "2001:0db8:85a3:0000:0000:8a2e:0370:7335"]] >> +]) >> +AT_CHECK([ovn_as az1 ovn-nbctl --columns=addresses,external_ids list > Address_Set az2-as1], [0], [dnl >> +addresses : [["2.1.1.1", "2.1.1.2"]] >> +external_ids : {ic-learnt="true"} >> +]) >> +AT_CHECK([ovn_as az1 ovn-nbctl --columns=addresses,external_ids list > Address_Set az2-as2], [0], [dnl >> +addresses : [["2001:0db8:85a3:0000:0000:8a2e:0370:7334", > "2001:0db8:85a3:0000:0000:8a2e:0370:7335"]] >> +external_ids : {ic-learnt="true"} >> +]) > > Same concern about raciness here too (and probably in more places in the > test). > >> + >> +# Create an address-set in AZ1 and then try to advertise an address- > set with same name from AZ2. >> +# It will sync to IC-SB but will not propagate to AZ1. >> +ovn_as az1 ovn-nbctl create Address_Set name=dup-as addresses=\"5.5.5.1\" >> +ovn_as az2 ovn-nbctl create Address_Set name=dup-as > addresses=\"5.5.5.2\" options:ic-adv=true >> +wait_row_count ic-sb:Address_Set 4 >> +AT_CHECK([ovn-ic-sbctl --columns=addresses find Address_Set name=dup- > as], [0], [dnl >> +addresses : [["5.5.5.2"]] >> +]) >> +AT_CHECK([ovn_as az1 ovn-nbctl --columns=addresses,external_ids list > Address_Set dup-as], [0], [dnl >> +addresses : [["5.5.5.1"]] >> +external_ids : {} >> +]) >> +AT_CHECK([ovn_as az2 ovn-nbctl --columns=addresses,external_ids list > Address_Set dup-as], [0], [dnl >> +addresses : [["5.5.5.2"]] >> +external_ids : {} >> +]) >> + >> +OVN_CLEANUP_IC([az1], [az2]) >> + >> +AT_CLEANUP >> +]) >> + >> OVN_FOR_EACH_NORTHD([ >> AT_SETUP([ovn-ic -- route sync]) >> > > Regards, > Dumitru > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
