| Message ID | 20241018100930.826293-6-amusil@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Reuse UUID for SB rows if there is 1:1 mapping | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
Acked-by: Mark Michelson <mmichels@redhat.com> On 10/18/24 06:09, Ales Musil wrote: > The NB Load_Balancer has exactly one corresponding row in SB database. > Use the NB UUID for the SB representation which makes the processing easier > and the link between the rows is obvious at first glance. > > Signed-off-by: Ales Musil <amusil@redhat.com> > --- > northd/en-sync-sb.c | 80 ++++++---------------------------------- > northd/en-sync-sb.h | 2 - > northd/inc-proc-northd.c | 4 +- > tests/ovn-northd.at | 3 +- > 4 files changed, 16 insertions(+), 73 deletions(-) > > diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c > index 9bd8a1fc6..d9dc25eb8 100644 > --- a/northd/en-sync-sb.c > +++ b/northd/en-sync-sb.c > @@ -225,7 +225,6 @@ struct sb_lb_record { > const struct sbrec_load_balancer *sbrec_lb; > struct ovn_dp_group *ls_dpg; > struct ovn_dp_group *lr_dpg; > - struct uuid sb_uuid; > }; > > struct sb_lb_table { > @@ -268,7 +267,6 @@ static bool sync_changed_lbs(struct sb_lb_table *, > struct ovn_datapaths *ls_datapaths, > struct ovn_datapaths *lr_datapaths, > struct chassis_features *); > -static bool check_sb_lb_duplicates(const struct sbrec_load_balancer_table *); > > void * > en_sync_to_sb_lb_init(struct engine_node *node OVS_UNUSED, > @@ -346,23 +344,6 @@ sync_to_sb_lb_northd_handler(struct engine_node *node, void *data_) > return true; > } > > -bool > -sync_to_sb_lb_sb_load_balancer(struct engine_node *node, void *data OVS_UNUSED) > -{ > - const struct sbrec_load_balancer_table *sb_load_balancer_table = > - EN_OVSDB_GET(engine_get_input("SB_load_balancer", node)); > - > - /* The only reason to handle SB.Load_Balancer updates is to detect > - * spurious records being created in clustered databases due to > - * lack of indexing on the SB.Load_Balancer table. All other changes > - * are valid and performed by northd, the only write-client for > - * this table. */ > - if (check_sb_lb_duplicates(sb_load_balancer_table)) { > - return false; > - } > - return true; > -} > - > /* sync_to_sb_pb engine node functions. > * This engine node syncs the SB Port Bindings (partly). > * en_northd engine create the SB Port binding rows and > @@ -690,14 +671,7 @@ sb_lb_table_build_and_sync( > const struct sbrec_load_balancer *sbrec_lb; > SBREC_LOAD_BALANCER_TABLE_FOR_EACH_SAFE (sbrec_lb, > sb_lb_table) { > - const char *nb_lb_uuid = smap_get(&sbrec_lb->external_ids, "lb_id"); > - struct uuid lb_uuid; > - if (!nb_lb_uuid || !uuid_from_string(&lb_uuid, nb_lb_uuid)) { > - sbrec_load_balancer_delete(sbrec_lb); > - continue; > - } > - > - sb_lb = sb_lb_table_find(&tmp_sb_lbs, &lb_uuid); > + sb_lb = sb_lb_table_find(&tmp_sb_lbs, &sbrec_lb->header_.uuid); > if (sb_lb) { > sb_lb->sbrec_lb = sbrec_lb; > bool success = sync_sb_lb_record(sb_lb, sbrec_lb, sb_dpgrp_table, > @@ -751,17 +725,9 @@ sync_sb_lb_record(struct sb_lb_record *sb_lb, > pre_sync_lr_dpg = sb_lb->lr_dpg; > > if (!sbrec_lb) { > - sb_lb->sb_uuid = uuid_random(); > - sbrec_lb = sbrec_load_balancer_insert_persist_uuid(ovnsb_txn, > - &sb_lb->sb_uuid); > - char *lb_id = xasprintf( > - UUID_FMT, UUID_ARGS(&lb_dps->lb->nlb->header_.uuid)); > - const struct smap external_ids = > - SMAP_CONST1(&external_ids, "lb_id", lb_id); > - sbrec_load_balancer_set_external_ids(sbrec_lb, &external_ids); > - free(lb_id); > + const struct uuid *nb_uuid = &lb_dps->lb->nlb->header_.uuid; > + sbrec_lb = sbrec_load_balancer_insert_persist_uuid(ovnsb_txn, nb_uuid); > } else { > - sb_lb->sb_uuid = sbrec_lb->header_.uuid; > sbrec_ls_dp_group = > chassis_features->ls_dpg_column > ? sbrec_lb->ls_datapath_group > @@ -923,13 +889,11 @@ sync_changed_lbs(struct sb_lb_table *sb_lbs, > > HMAPX_FOR_EACH (hmapx_node, &trk_lbs->deleted) { > lb_dps = hmapx_node->data; > - > - sb_lb = sb_lb_table_find(&sb_lbs->entries, > - &lb_dps->lb->nlb->header_.uuid); > + const struct uuid *nb_uuid = &lb_dps->lb->nlb->header_.uuid; > + sb_lb = sb_lb_table_find(&sb_lbs->entries, nb_uuid); > if (sb_lb) { > const struct sbrec_load_balancer *sbrec_lb = > - sbrec_load_balancer_table_get_for_uuid(sb_lb_table, > - &sb_lb->sb_uuid); > + sbrec_load_balancer_table_get_for_uuid(sb_lb_table, nb_uuid); > if (sbrec_lb) { > sbrec_load_balancer_delete(sbrec_lb); > } > @@ -941,9 +905,9 @@ sync_changed_lbs(struct sb_lb_table *sb_lbs, > > HMAPX_FOR_EACH (hmapx_node, &trk_lbs->crupdated) { > lb_dps = hmapx_node->data; > + const struct uuid *nb_uuid = &lb_dps->lb->nlb->header_.uuid; > > - sb_lb = sb_lb_table_find(&sb_lbs->entries, > - &lb_dps->lb->nlb->header_.uuid); > + sb_lb = sb_lb_table_find(&sb_lbs->entries, nb_uuid); > > if (!sb_lb && !lb_dps->n_nb_ls && !lb_dps->n_nb_lr) { > continue; > @@ -953,17 +917,15 @@ sync_changed_lbs(struct sb_lb_table *sb_lbs, > sb_lb = xzalloc(sizeof *sb_lb); > sb_lb->lb_dps = lb_dps; > hmap_insert(&sb_lbs->entries, &sb_lb->key_node, > - uuid_hash(&lb_dps->lb->nlb->header_.uuid)); > + uuid_hash(nb_uuid)); > } else { > sb_lb->sbrec_lb = > - sbrec_load_balancer_table_get_for_uuid(sb_lb_table, > - &sb_lb->sb_uuid); > + sbrec_load_balancer_table_get_for_uuid(sb_lb_table, nb_uuid); > } > > if (sb_lb && !lb_dps->n_nb_ls && !lb_dps->n_nb_lr) { > const struct sbrec_load_balancer *sbrec_lb = > - sbrec_load_balancer_table_get_for_uuid(sb_lb_table, > - &sb_lb->sb_uuid); > + sbrec_load_balancer_table_get_for_uuid(sb_lb_table, nb_uuid); > if (sbrec_lb) { > sbrec_load_balancer_delete(sbrec_lb); > } > @@ -981,23 +943,3 @@ sync_changed_lbs(struct sb_lb_table *sb_lbs, > > return true; > } > - > -static bool > -check_sb_lb_duplicates(const struct sbrec_load_balancer_table *table) > -{ > - struct sset existing_nb_lb_uuids = > - SSET_INITIALIZER(&existing_nb_lb_uuids); > - const struct sbrec_load_balancer *sbrec_lb; > - bool duplicates = false; > - > - SBREC_LOAD_BALANCER_TABLE_FOR_EACH (sbrec_lb, table) { > - const char *nb_lb_uuid = smap_get(&sbrec_lb->external_ids, "lb_id"); > - if (nb_lb_uuid && !sset_add(&existing_nb_lb_uuids, nb_lb_uuid)) { > - duplicates = true; > - break; > - } > - } > - > - sset_destroy(&existing_nb_lb_uuids); > - return duplicates; > -} > diff --git a/northd/en-sync-sb.h b/northd/en-sync-sb.h > index 3bcbb8259..f08565eee 100644 > --- a/northd/en-sync-sb.h > +++ b/northd/en-sync-sb.h > @@ -21,8 +21,6 @@ void *en_sync_to_sb_lb_init(struct engine_node *, struct engine_arg *); > void en_sync_to_sb_lb_run(struct engine_node *, void *data); > void en_sync_to_sb_lb_cleanup(void *data); > bool sync_to_sb_lb_northd_handler(struct engine_node *, void *data OVS_UNUSED); > -bool sync_to_sb_lb_sb_load_balancer(struct engine_node *, > - void *data OVS_UNUSED); > > void *en_sync_to_sb_pb_init(struct engine_node *, struct engine_arg *); > void en_sync_to_sb_pb_run(struct engine_node *, void *data); > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > index 480100d3c..f5f414959 100644 > --- a/northd/inc-proc-northd.c > +++ b/northd/inc-proc-northd.c > @@ -308,8 +308,10 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > node_global_config_handler); > engine_add_input(&en_sync_to_sb_lb, &en_northd, > sync_to_sb_lb_northd_handler); > + /* There is nothing to handle as duplicates are prevented by using > + * UUID mapping between NB and SB. */ > engine_add_input(&en_sync_to_sb_lb, &en_sb_load_balancer, > - sync_to_sb_lb_sb_load_balancer); > + engine_noop_handler); > engine_add_input(&en_sync_to_sb_lb, &en_sb_logical_dp_group, NULL); > > engine_add_input(&en_sync_to_sb_pb, &en_northd, > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index ab65ddb44..f4aa23042 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -6456,7 +6456,8 @@ check ovn-nbctl --wait=sb sync > > dps=$(fetch_column Load_Balancer ls_datapath_group) > nlb=$(fetch_column nb:Load_Balancer _uuid) > -AT_CHECK([ovn-sbctl create Load_Balancer name=lb1 ls_datapath_group="$dps" external_ids="lb_id=$nlb"], [0], [ignore]) > +AT_CHECK([ovn-sbctl --id="$nlb" create Load_Balancer name=lb1 ls_datapath_group="$dps"], [1], [ignore], [stderr]) > +check grep -q "duplicate uuid" stderr > > check ovn-nbctl --wait=sb sync > check_row_count Load_Balancer 1
On Fri, Oct 18, 2024 at 4:19 PM Mark Michelson <mmichels@redhat.com> wrote: > > Acked-by: Mark Michelson <mmichels@redhat.com> Acked-by: Numan Siddique <numans@ovn.org> In this whole patch series, I think only this patch has some potential to disrupt the existing datapath connections during ovn-northd upgrade. Numan > > On 10/18/24 06:09, Ales Musil wrote: > > The NB Load_Balancer has exactly one corresponding row in SB database. > > Use the NB UUID for the SB representation which makes the processing easier > > and the link between the rows is obvious at first glance. > > > > Signed-off-by: Ales Musil <amusil@redhat.com> > > --- > > northd/en-sync-sb.c | 80 ++++++---------------------------------- > > northd/en-sync-sb.h | 2 - > > northd/inc-proc-northd.c | 4 +- > > tests/ovn-northd.at | 3 +- > > 4 files changed, 16 insertions(+), 73 deletions(-) > > > > diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c > > index 9bd8a1fc6..d9dc25eb8 100644 > > --- a/northd/en-sync-sb.c > > +++ b/northd/en-sync-sb.c > > @@ -225,7 +225,6 @@ struct sb_lb_record { > > const struct sbrec_load_balancer *sbrec_lb; > > struct ovn_dp_group *ls_dpg; > > struct ovn_dp_group *lr_dpg; > > - struct uuid sb_uuid; > > }; > > > > struct sb_lb_table { > > @@ -268,7 +267,6 @@ static bool sync_changed_lbs(struct sb_lb_table *, > > struct ovn_datapaths *ls_datapaths, > > struct ovn_datapaths *lr_datapaths, > > struct chassis_features *); > > -static bool check_sb_lb_duplicates(const struct sbrec_load_balancer_table *); > > > > void * > > en_sync_to_sb_lb_init(struct engine_node *node OVS_UNUSED, > > @@ -346,23 +344,6 @@ sync_to_sb_lb_northd_handler(struct engine_node *node, void *data_) > > return true; > > } > > > > -bool > > -sync_to_sb_lb_sb_load_balancer(struct engine_node *node, void *data OVS_UNUSED) > > -{ > > - const struct sbrec_load_balancer_table *sb_load_balancer_table = > > - EN_OVSDB_GET(engine_get_input("SB_load_balancer", node)); > > - > > - /* The only reason to handle SB.Load_Balancer updates is to detect > > - * spurious records being created in clustered databases due to > > - * lack of indexing on the SB.Load_Balancer table. All other changes > > - * are valid and performed by northd, the only write-client for > > - * this table. */ > > - if (check_sb_lb_duplicates(sb_load_balancer_table)) { > > - return false; > > - } > > - return true; > > -} > > - > > /* sync_to_sb_pb engine node functions. > > * This engine node syncs the SB Port Bindings (partly). > > * en_northd engine create the SB Port binding rows and > > @@ -690,14 +671,7 @@ sb_lb_table_build_and_sync( > > const struct sbrec_load_balancer *sbrec_lb; > > SBREC_LOAD_BALANCER_TABLE_FOR_EACH_SAFE (sbrec_lb, > > sb_lb_table) { > > - const char *nb_lb_uuid = smap_get(&sbrec_lb->external_ids, "lb_id"); > > - struct uuid lb_uuid; > > - if (!nb_lb_uuid || !uuid_from_string(&lb_uuid, nb_lb_uuid)) { > > - sbrec_load_balancer_delete(sbrec_lb); > > - continue; > > - } > > - > > - sb_lb = sb_lb_table_find(&tmp_sb_lbs, &lb_uuid); > > + sb_lb = sb_lb_table_find(&tmp_sb_lbs, &sbrec_lb->header_.uuid); > > if (sb_lb) { > > sb_lb->sbrec_lb = sbrec_lb; > > bool success = sync_sb_lb_record(sb_lb, sbrec_lb, sb_dpgrp_table, > > @@ -751,17 +725,9 @@ sync_sb_lb_record(struct sb_lb_record *sb_lb, > > pre_sync_lr_dpg = sb_lb->lr_dpg; > > > > if (!sbrec_lb) { > > - sb_lb->sb_uuid = uuid_random(); > > - sbrec_lb = sbrec_load_balancer_insert_persist_uuid(ovnsb_txn, > > - &sb_lb->sb_uuid); > > - char *lb_id = xasprintf( > > - UUID_FMT, UUID_ARGS(&lb_dps->lb->nlb->header_.uuid)); > > - const struct smap external_ids = > > - SMAP_CONST1(&external_ids, "lb_id", lb_id); > > - sbrec_load_balancer_set_external_ids(sbrec_lb, &external_ids); > > - free(lb_id); > > + const struct uuid *nb_uuid = &lb_dps->lb->nlb->header_.uuid; > > + sbrec_lb = sbrec_load_balancer_insert_persist_uuid(ovnsb_txn, nb_uuid); > > } else { > > - sb_lb->sb_uuid = sbrec_lb->header_.uuid; > > sbrec_ls_dp_group = > > chassis_features->ls_dpg_column > > ? sbrec_lb->ls_datapath_group > > @@ -923,13 +889,11 @@ sync_changed_lbs(struct sb_lb_table *sb_lbs, > > > > HMAPX_FOR_EACH (hmapx_node, &trk_lbs->deleted) { > > lb_dps = hmapx_node->data; > > - > > - sb_lb = sb_lb_table_find(&sb_lbs->entries, > > - &lb_dps->lb->nlb->header_.uuid); > > + const struct uuid *nb_uuid = &lb_dps->lb->nlb->header_.uuid; > > + sb_lb = sb_lb_table_find(&sb_lbs->entries, nb_uuid); > > if (sb_lb) { > > const struct sbrec_load_balancer *sbrec_lb = > > - sbrec_load_balancer_table_get_for_uuid(sb_lb_table, > > - &sb_lb->sb_uuid); > > + sbrec_load_balancer_table_get_for_uuid(sb_lb_table, nb_uuid); > > if (sbrec_lb) { > > sbrec_load_balancer_delete(sbrec_lb); > > } > > @@ -941,9 +905,9 @@ sync_changed_lbs(struct sb_lb_table *sb_lbs, > > > > HMAPX_FOR_EACH (hmapx_node, &trk_lbs->crupdated) { > > lb_dps = hmapx_node->data; > > + const struct uuid *nb_uuid = &lb_dps->lb->nlb->header_.uuid; > > > > - sb_lb = sb_lb_table_find(&sb_lbs->entries, > > - &lb_dps->lb->nlb->header_.uuid); > > + sb_lb = sb_lb_table_find(&sb_lbs->entries, nb_uuid); > > > > if (!sb_lb && !lb_dps->n_nb_ls && !lb_dps->n_nb_lr) { > > continue; > > @@ -953,17 +917,15 @@ sync_changed_lbs(struct sb_lb_table *sb_lbs, > > sb_lb = xzalloc(sizeof *sb_lb); > > sb_lb->lb_dps = lb_dps; > > hmap_insert(&sb_lbs->entries, &sb_lb->key_node, > > - uuid_hash(&lb_dps->lb->nlb->header_.uuid)); > > + uuid_hash(nb_uuid)); > > } else { > > sb_lb->sbrec_lb = > > - sbrec_load_balancer_table_get_for_uuid(sb_lb_table, > > - &sb_lb->sb_uuid); > > + sbrec_load_balancer_table_get_for_uuid(sb_lb_table, nb_uuid); > > } > > > > if (sb_lb && !lb_dps->n_nb_ls && !lb_dps->n_nb_lr) { > > const struct sbrec_load_balancer *sbrec_lb = > > - sbrec_load_balancer_table_get_for_uuid(sb_lb_table, > > - &sb_lb->sb_uuid); > > + sbrec_load_balancer_table_get_for_uuid(sb_lb_table, nb_uuid); > > if (sbrec_lb) { > > sbrec_load_balancer_delete(sbrec_lb); > > } > > @@ -981,23 +943,3 @@ sync_changed_lbs(struct sb_lb_table *sb_lbs, > > > > return true; > > } > > - > > -static bool > > -check_sb_lb_duplicates(const struct sbrec_load_balancer_table *table) > > -{ > > - struct sset existing_nb_lb_uuids = > > - SSET_INITIALIZER(&existing_nb_lb_uuids); > > - const struct sbrec_load_balancer *sbrec_lb; > > - bool duplicates = false; > > - > > - SBREC_LOAD_BALANCER_TABLE_FOR_EACH (sbrec_lb, table) { > > - const char *nb_lb_uuid = smap_get(&sbrec_lb->external_ids, "lb_id"); > > - if (nb_lb_uuid && !sset_add(&existing_nb_lb_uuids, nb_lb_uuid)) { > > - duplicates = true; > > - break; > > - } > > - } > > - > > - sset_destroy(&existing_nb_lb_uuids); > > - return duplicates; > > -} > > diff --git a/northd/en-sync-sb.h b/northd/en-sync-sb.h > > index 3bcbb8259..f08565eee 100644 > > --- a/northd/en-sync-sb.h > > +++ b/northd/en-sync-sb.h > > @@ -21,8 +21,6 @@ void *en_sync_to_sb_lb_init(struct engine_node *, struct engine_arg *); > > void en_sync_to_sb_lb_run(struct engine_node *, void *data); > > void en_sync_to_sb_lb_cleanup(void *data); > > bool sync_to_sb_lb_northd_handler(struct engine_node *, void *data OVS_UNUSED); > > -bool sync_to_sb_lb_sb_load_balancer(struct engine_node *, > > - void *data OVS_UNUSED); > > > > void *en_sync_to_sb_pb_init(struct engine_node *, struct engine_arg *); > > void en_sync_to_sb_pb_run(struct engine_node *, void *data); > > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > > index 480100d3c..f5f414959 100644 > > --- a/northd/inc-proc-northd.c > > +++ b/northd/inc-proc-northd.c > > @@ -308,8 +308,10 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > > node_global_config_handler); > > engine_add_input(&en_sync_to_sb_lb, &en_northd, > > sync_to_sb_lb_northd_handler); > > + /* There is nothing to handle as duplicates are prevented by using > > + * UUID mapping between NB and SB. */ > > engine_add_input(&en_sync_to_sb_lb, &en_sb_load_balancer, > > - sync_to_sb_lb_sb_load_balancer); > > + engine_noop_handler); > > engine_add_input(&en_sync_to_sb_lb, &en_sb_logical_dp_group, NULL); > > > > engine_add_input(&en_sync_to_sb_pb, &en_northd, > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > index ab65ddb44..f4aa23042 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -6456,7 +6456,8 @@ check ovn-nbctl --wait=sb sync > > > > dps=$(fetch_column Load_Balancer ls_datapath_group) > > nlb=$(fetch_column nb:Load_Balancer _uuid) > > -AT_CHECK([ovn-sbctl create Load_Balancer name=lb1 ls_datapath_group="$dps" external_ids="lb_id=$nlb"], [0], [ignore]) > > +AT_CHECK([ovn-sbctl --id="$nlb" create Load_Balancer name=lb1 ls_datapath_group="$dps"], [1], [ignore], [stderr]) > > +check grep -q "duplicate uuid" stderr > > > > check ovn-nbctl --wait=sb sync > > check_row_count Load_Balancer 1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c index 9bd8a1fc6..d9dc25eb8 100644 --- a/northd/en-sync-sb.c +++ b/northd/en-sync-sb.c @@ -225,7 +225,6 @@ struct sb_lb_record { const struct sbrec_load_balancer *sbrec_lb; struct ovn_dp_group *ls_dpg; struct ovn_dp_group *lr_dpg; - struct uuid sb_uuid; }; struct sb_lb_table { @@ -268,7 +267,6 @@ static bool sync_changed_lbs(struct sb_lb_table *, struct ovn_datapaths *ls_datapaths, struct ovn_datapaths *lr_datapaths, struct chassis_features *); -static bool check_sb_lb_duplicates(const struct sbrec_load_balancer_table *); void * en_sync_to_sb_lb_init(struct engine_node *node OVS_UNUSED, @@ -346,23 +344,6 @@ sync_to_sb_lb_northd_handler(struct engine_node *node, void *data_) return true; } -bool -sync_to_sb_lb_sb_load_balancer(struct engine_node *node, void *data OVS_UNUSED) -{ - const struct sbrec_load_balancer_table *sb_load_balancer_table = - EN_OVSDB_GET(engine_get_input("SB_load_balancer", node)); - - /* The only reason to handle SB.Load_Balancer updates is to detect - * spurious records being created in clustered databases due to - * lack of indexing on the SB.Load_Balancer table. All other changes - * are valid and performed by northd, the only write-client for - * this table. */ - if (check_sb_lb_duplicates(sb_load_balancer_table)) { - return false; - } - return true; -} - /* sync_to_sb_pb engine node functions. * This engine node syncs the SB Port Bindings (partly). * en_northd engine create the SB Port binding rows and @@ -690,14 +671,7 @@ sb_lb_table_build_and_sync( const struct sbrec_load_balancer *sbrec_lb; SBREC_LOAD_BALANCER_TABLE_FOR_EACH_SAFE (sbrec_lb, sb_lb_table) { - const char *nb_lb_uuid = smap_get(&sbrec_lb->external_ids, "lb_id"); - struct uuid lb_uuid; - if (!nb_lb_uuid || !uuid_from_string(&lb_uuid, nb_lb_uuid)) { - sbrec_load_balancer_delete(sbrec_lb); - continue; - } - - sb_lb = sb_lb_table_find(&tmp_sb_lbs, &lb_uuid); + sb_lb = sb_lb_table_find(&tmp_sb_lbs, &sbrec_lb->header_.uuid); if (sb_lb) { sb_lb->sbrec_lb = sbrec_lb; bool success = sync_sb_lb_record(sb_lb, sbrec_lb, sb_dpgrp_table, @@ -751,17 +725,9 @@ sync_sb_lb_record(struct sb_lb_record *sb_lb, pre_sync_lr_dpg = sb_lb->lr_dpg; if (!sbrec_lb) { - sb_lb->sb_uuid = uuid_random(); - sbrec_lb = sbrec_load_balancer_insert_persist_uuid(ovnsb_txn, - &sb_lb->sb_uuid); - char *lb_id = xasprintf( - UUID_FMT, UUID_ARGS(&lb_dps->lb->nlb->header_.uuid)); - const struct smap external_ids = - SMAP_CONST1(&external_ids, "lb_id", lb_id); - sbrec_load_balancer_set_external_ids(sbrec_lb, &external_ids); - free(lb_id); + const struct uuid *nb_uuid = &lb_dps->lb->nlb->header_.uuid; + sbrec_lb = sbrec_load_balancer_insert_persist_uuid(ovnsb_txn, nb_uuid); } else { - sb_lb->sb_uuid = sbrec_lb->header_.uuid; sbrec_ls_dp_group = chassis_features->ls_dpg_column ? sbrec_lb->ls_datapath_group @@ -923,13 +889,11 @@ sync_changed_lbs(struct sb_lb_table *sb_lbs, HMAPX_FOR_EACH (hmapx_node, &trk_lbs->deleted) { lb_dps = hmapx_node->data; - - sb_lb = sb_lb_table_find(&sb_lbs->entries, - &lb_dps->lb->nlb->header_.uuid); + const struct uuid *nb_uuid = &lb_dps->lb->nlb->header_.uuid; + sb_lb = sb_lb_table_find(&sb_lbs->entries, nb_uuid); if (sb_lb) { const struct sbrec_load_balancer *sbrec_lb = - sbrec_load_balancer_table_get_for_uuid(sb_lb_table, - &sb_lb->sb_uuid); + sbrec_load_balancer_table_get_for_uuid(sb_lb_table, nb_uuid); if (sbrec_lb) { sbrec_load_balancer_delete(sbrec_lb); } @@ -941,9 +905,9 @@ sync_changed_lbs(struct sb_lb_table *sb_lbs, HMAPX_FOR_EACH (hmapx_node, &trk_lbs->crupdated) { lb_dps = hmapx_node->data; + const struct uuid *nb_uuid = &lb_dps->lb->nlb->header_.uuid; - sb_lb = sb_lb_table_find(&sb_lbs->entries, - &lb_dps->lb->nlb->header_.uuid); + sb_lb = sb_lb_table_find(&sb_lbs->entries, nb_uuid); if (!sb_lb && !lb_dps->n_nb_ls && !lb_dps->n_nb_lr) { continue; @@ -953,17 +917,15 @@ sync_changed_lbs(struct sb_lb_table *sb_lbs, sb_lb = xzalloc(sizeof *sb_lb); sb_lb->lb_dps = lb_dps; hmap_insert(&sb_lbs->entries, &sb_lb->key_node, - uuid_hash(&lb_dps->lb->nlb->header_.uuid)); + uuid_hash(nb_uuid)); } else { sb_lb->sbrec_lb = - sbrec_load_balancer_table_get_for_uuid(sb_lb_table, - &sb_lb->sb_uuid); + sbrec_load_balancer_table_get_for_uuid(sb_lb_table, nb_uuid); } if (sb_lb && !lb_dps->n_nb_ls && !lb_dps->n_nb_lr) { const struct sbrec_load_balancer *sbrec_lb = - sbrec_load_balancer_table_get_for_uuid(sb_lb_table, - &sb_lb->sb_uuid); + sbrec_load_balancer_table_get_for_uuid(sb_lb_table, nb_uuid); if (sbrec_lb) { sbrec_load_balancer_delete(sbrec_lb); } @@ -981,23 +943,3 @@ sync_changed_lbs(struct sb_lb_table *sb_lbs, return true; } - -static bool -check_sb_lb_duplicates(const struct sbrec_load_balancer_table *table) -{ - struct sset existing_nb_lb_uuids = - SSET_INITIALIZER(&existing_nb_lb_uuids); - const struct sbrec_load_balancer *sbrec_lb; - bool duplicates = false; - - SBREC_LOAD_BALANCER_TABLE_FOR_EACH (sbrec_lb, table) { - const char *nb_lb_uuid = smap_get(&sbrec_lb->external_ids, "lb_id"); - if (nb_lb_uuid && !sset_add(&existing_nb_lb_uuids, nb_lb_uuid)) { - duplicates = true; - break; - } - } - - sset_destroy(&existing_nb_lb_uuids); - return duplicates; -} diff --git a/northd/en-sync-sb.h b/northd/en-sync-sb.h index 3bcbb8259..f08565eee 100644 --- a/northd/en-sync-sb.h +++ b/northd/en-sync-sb.h @@ -21,8 +21,6 @@ void *en_sync_to_sb_lb_init(struct engine_node *, struct engine_arg *); void en_sync_to_sb_lb_run(struct engine_node *, void *data); void en_sync_to_sb_lb_cleanup(void *data); bool sync_to_sb_lb_northd_handler(struct engine_node *, void *data OVS_UNUSED); -bool sync_to_sb_lb_sb_load_balancer(struct engine_node *, - void *data OVS_UNUSED); void *en_sync_to_sb_pb_init(struct engine_node *, struct engine_arg *); void en_sync_to_sb_pb_run(struct engine_node *, void *data); diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c index 480100d3c..f5f414959 100644 --- a/northd/inc-proc-northd.c +++ b/northd/inc-proc-northd.c @@ -308,8 +308,10 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, node_global_config_handler); engine_add_input(&en_sync_to_sb_lb, &en_northd, sync_to_sb_lb_northd_handler); + /* There is nothing to handle as duplicates are prevented by using + * UUID mapping between NB and SB. */ engine_add_input(&en_sync_to_sb_lb, &en_sb_load_balancer, - sync_to_sb_lb_sb_load_balancer); + engine_noop_handler); engine_add_input(&en_sync_to_sb_lb, &en_sb_logical_dp_group, NULL); engine_add_input(&en_sync_to_sb_pb, &en_northd, diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index ab65ddb44..f4aa23042 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -6456,7 +6456,8 @@ check ovn-nbctl --wait=sb sync dps=$(fetch_column Load_Balancer ls_datapath_group) nlb=$(fetch_column nb:Load_Balancer _uuid) -AT_CHECK([ovn-sbctl create Load_Balancer name=lb1 ls_datapath_group="$dps" external_ids="lb_id=$nlb"], [0], [ignore]) +AT_CHECK([ovn-sbctl --id="$nlb" create Load_Balancer name=lb1 ls_datapath_group="$dps"], [1], [ignore], [stderr]) +check grep -q "duplicate uuid" stderr check ovn-nbctl --wait=sb sync check_row_count Load_Balancer 1
The NB Load_Balancer has exactly one corresponding row in SB database. Use the NB UUID for the SB representation which makes the processing easier and the link between the rows is obvious at first glance. Signed-off-by: Ales Musil <amusil@redhat.com> --- northd/en-sync-sb.c | 80 ++++++---------------------------------- northd/en-sync-sb.h | 2 - northd/inc-proc-northd.c | 4 +- tests/ovn-northd.at | 3 +- 4 files changed, 16 insertions(+), 73 deletions(-)