On Fri, Oct 18, 2024 at 4:19 PM Mark Michelson <[email protected]> wrote: > > Acked-by: Mark Michelson <[email protected]>
Acked-by: Numan Siddique <[email protected]> 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 <[email protected]> > > --- > > 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 > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
