| Message ID | 20240703122029.1265688-1-amusil@redhat.com |
|---|---|
| State | Deferred |
| Headers | show |
| Series | [ovs-dev] northd: Detect if SB LB was removed and re-sync that row. | 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 |
On Wed, Jul 3, 2024 at 8:20 AM Ales Musil <amusil@redhat.com> wrote: > > When someone removes SB LB it won't be detected until the next > recompute of the "sync_to_sb_lb" node. This will cause traffic > disruptions in case of hairpin as the flows directly depend on > the SB LB entries. Add check to trigger recompute when we detect > that the row is missing in SB, but still present in NB. > > Reported-at: https://issues.redhat.com/browse/FDP-682 > Signed-off-by: Ales Musil <amusil@redhat.com> Thanks for the patch. I'm not sure if we need to fix this. Ideally a user is not supposed to destroy records in SB DB. And if the user does it so, then I'd assume the user should trigger a recompute. You'd see the same behavior if you delete a logical flow manually from SB DB. ovn-northd doesn't fix this causing traffic disruptions until ovn-northd reconciles the SB DB. I'm not against this fix. This patch is straightforward and we already check for duplicate entries in SB LB in the sync_to_sb_lb_sb_load_balancer(). But maybe we should discuss if we want to take a similar approach for other tables as well or not. Thanks Numan > --- > northd/en-sync-sb.c | 38 +++++++++++++++++++++++++++++++++----- > tests/ovn-macros.at | 2 +- > tests/ovn-northd.at | 38 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 72 insertions(+), 6 deletions(-) > > diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c > index 9bd8a1fc6..12dbd139e 100644 > --- a/northd/en-sync-sb.c > +++ b/northd/en-sync-sb.c > @@ -20,6 +20,7 @@ > > /* OVS includes. */ > #include "lib/svec.h" > +#include "lib/uuidset.h" > #include "openvswitch/util.h" > > /* OVN includes. */ > @@ -232,6 +233,7 @@ struct sb_lb_table { > struct hmap entries; /* Stores struct sb_lb_record. */ > struct hmap ls_dp_groups; > struct hmap lr_dp_groups; > + struct uuidset sb_entries; > }; > > struct ed_type_sync_to_sb_lb_data { > @@ -347,16 +349,29 @@ sync_to_sb_lb_northd_handler(struct engine_node *node, void *data_) > } > > bool > -sync_to_sb_lb_sb_load_balancer(struct engine_node *node, void *data OVS_UNUSED) > +sync_to_sb_lb_sb_load_balancer(struct engine_node *node, void *data_) > { > const struct sbrec_load_balancer_table *sb_load_balancer_table = > EN_OVSDB_GET(engine_get_input("SB_load_balancer", node)); > + struct ed_type_sync_to_sb_lb_data *data = data_; > > - /* The only reason to handle SB.Load_Balancer updates is to detect > + /* The reasons 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. */ > + * lack of indexing on the SB.Load_Balancer table. The other reason might > + * be when someone removes the SB row while the NB row is still valid. > + * All other changes are valid and performed by northd. */ > + > + const struct sbrec_load_balancer *sb_lb; > + SBREC_LOAD_BALANCER_TABLE_FOR_EACH_TRACKED (sb_lb, > + sb_load_balancer_table) { > + if (sbrec_load_balancer_is_deleted(sb_lb) && > + uuidset_find(&data->sb_lbs.sb_entries, &sb_lb->header_.uuid)) { > + VLOG_WARN("A SB LB for \"%s\" is deleted but the NB LB entry " > + "still exists.", sb_lb->name); > + return false; > + } > + } > + > if (check_sb_lb_duplicates(sb_load_balancer_table)) { > return false; > } > @@ -626,6 +641,7 @@ sb_lb_table_init(struct sb_lb_table *sb_lbs) > hmap_init(&sb_lbs->entries); > ovn_dp_groups_init(&sb_lbs->ls_dp_groups); > ovn_dp_groups_init(&sb_lbs->lr_dp_groups); > + uuidset_init(&sb_lbs->sb_entries); > } > > static void > @@ -638,6 +654,7 @@ sb_lb_table_clear(struct sb_lb_table *sb_lbs) > > ovn_dp_groups_clear(&sb_lbs->ls_dp_groups); > ovn_dp_groups_clear(&sb_lbs->lr_dp_groups); > + uuidset_clear(&sb_lbs->sb_entries); > } > > static void > @@ -647,6 +664,7 @@ sb_lb_table_destroy(struct sb_lb_table *sb_lbs) > hmap_destroy(&sb_lbs->entries); > ovn_dp_groups_destroy(&sb_lbs->ls_dp_groups); > ovn_dp_groups_destroy(&sb_lbs->lr_dp_groups); > + uuidset_destroy(&sb_lbs->sb_entries); > } > > static struct sb_lb_record * > @@ -693,6 +711,8 @@ sb_lb_table_build_and_sync( > 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)) { > + uuidset_find_and_delete(&sb_lbs->sb_entries, > + &sbrec_lb->header_.uuid); > sbrec_load_balancer_delete(sbrec_lb); > continue; > } > @@ -711,6 +731,8 @@ sb_lb_table_build_and_sync( > hmap_insert(&sb_lbs->entries, &sb_lb->key_node, > uuid_hash(&sb_lb->lb_dps->lb->nlb->header_.uuid)); > } else { > + uuidset_find_and_delete(&sb_lbs->sb_entries, > + &sbrec_lb->header_.uuid); > sbrec_load_balancer_delete(sbrec_lb); > } > } > @@ -770,6 +792,8 @@ sync_sb_lb_record(struct sb_lb_record *sb_lb, > sbrec_lr_dp_group = sbrec_lb->lr_datapath_group; > } > > + uuidset_insert(&sb_lbs->sb_entries, &sb_lb->sb_uuid); > + > if (lb_dps->n_nb_ls) { > sb_lb->ls_dpg = ovn_dp_group_get(&sb_lbs->ls_dp_groups, > lb_dps->n_nb_ls, > @@ -931,6 +955,8 @@ sync_changed_lbs(struct sb_lb_table *sb_lbs, > sbrec_load_balancer_table_get_for_uuid(sb_lb_table, > &sb_lb->sb_uuid); > if (sbrec_lb) { > + uuidset_find_and_delete(&sb_lbs->sb_entries, > + &sbrec_lb->header_.uuid); > sbrec_load_balancer_delete(sbrec_lb); > } > > @@ -965,6 +991,8 @@ sync_changed_lbs(struct sb_lb_table *sb_lbs, > sbrec_load_balancer_table_get_for_uuid(sb_lb_table, > &sb_lb->sb_uuid); > if (sbrec_lb) { > + uuidset_find_and_delete(&sb_lbs->sb_entries, > + &sbrec_lb->header_.uuid); > sbrec_load_balancer_delete(sbrec_lb); > } > > diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at > index 47ada5c70..65379ea57 100644 > --- a/tests/ovn-macros.at > +++ b/tests/ovn-macros.at > @@ -249,7 +249,7 @@ ovn_start_northd() { > local name=${d_prefix}northd${suffix} > echo "${prefix}starting $name" > test -d "$ovs_base/$name" || mkdir "$ovs_base/$name" > - as $name start_daemon ovn-northd $northd_args -vjsonrpc \ > + as $name start_daemon ovn-northd $northd_args \ > --ovnnb-db=$OVN_NB_DB --ovnsb-db=$OVN_SB_DB > } > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index a389d1988..74726f449 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -12721,3 +12721,41 @@ AT_CHECK([ovn-sbctl dump-flows lr | grep lr_in_dnat | ovn_strip_lflows], [0], [d > > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD_NO_HV([ > +AT_SETUP([LB re-sync]) > +ovn_start > + > +check ovn-sbctl chassis-add hv1 geneve 127.0.0.1 > + > +check ovn-nbctl lr-add lr -- \ > + lrp-add lr lr-ls 02:ac:10:01:00:01 172.16.1.1/24 > + > +check ovn-nbctl ls-add ls -- \ > + lsp-add ls ls-lr -- \ > + lsp-set-type ls-lr router -- \ > + lsp-set-addresses ls-lr router -- \ > + lsp-set-options ls-lr router-port=lr-ls > + > +check ovn-nbctl lb-add lb 172.16.10.10 192.168.10.10 > +check ovn-nbctl --wait=sb lr-lb-add lr lb > + > +check_row_count Load_Balancer 1 > + > +check ovn-sbctl --all destroy Load_Balancer > +check ovn-nbctl --wait=sb sync > +check_row_count Load_Balancer 1 > + > +check ovn-nbctl --wait=sb lb-del lb > +wait_row_count Load_Balancer 0 > + > +check ovn-nbctl lb-add lb 172.16.10.10 192.168.10.10 > +check ovn-nbctl --wait=sb lr-lb-add lr lb > +check_row_count Load_Balancer 1 > + > +check ovn-sbctl --all destroy Load_Balancer > +check ovn-nbctl --wait=sb sync > +check_row_count Load_Balancer 1 > + > +AT_CLEANUP > +]) > -- > 2.45.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 7/4/24 18:01, Numan Siddique wrote: > On Wed, Jul 3, 2024 at 8:20 AM Ales Musil <amusil@redhat.com> wrote: >> >> When someone removes SB LB it won't be detected until the next >> recompute of the "sync_to_sb_lb" node. This will cause traffic >> disruptions in case of hairpin as the flows directly depend on >> the SB LB entries. Add check to trigger recompute when we detect >> that the row is missing in SB, but still present in NB. >> >> Reported-at: https://issues.redhat.com/browse/FDP-682 >> Signed-off-by: Ales Musil <amusil@redhat.com> > > Thanks for the patch. > > I'm not sure if we need to fix this. Ideally a user is not supposed > to destroy records in SB DB. > And if the user does it so, then I'd assume the user should trigger a recompute. > I agree, it doesn't seem like a correct use of OVN if users mangle the SB database directly. > You'd see the same behavior if you delete a logical flow manually from > SB DB. ovn-northd doesn't fix this > causing traffic disruptions until ovn-northd reconciles the SB DB. > > I'm not against this fix. This patch is straightforward and we > already check for duplicate entries in SB LB > in the sync_to_sb_lb_sb_load_balancer(). > The check for duplicates, on the other hand, needs to be kept because the SB LB table doesn't have an index so a single insert operation from northd might create duplicates in the SB (when running SB with raft). > But maybe we should discuss if we want to take a similar approach for > other tables as well or not. > In my opinion we shouldn't handle these cases. Users shouldn't change the southbound. > Thanks > Numan > Regards, Dumitru >> --- >> northd/en-sync-sb.c | 38 +++++++++++++++++++++++++++++++++----- >> tests/ovn-macros.at | 2 +- >> tests/ovn-northd.at | 38 ++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 72 insertions(+), 6 deletions(-) >> >> diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c >> index 9bd8a1fc6..12dbd139e 100644 >> --- a/northd/en-sync-sb.c >> +++ b/northd/en-sync-sb.c >> @@ -20,6 +20,7 @@ >> >> /* OVS includes. */ >> #include "lib/svec.h" >> +#include "lib/uuidset.h" >> #include "openvswitch/util.h" >> >> /* OVN includes. */ >> @@ -232,6 +233,7 @@ struct sb_lb_table { >> struct hmap entries; /* Stores struct sb_lb_record. */ >> struct hmap ls_dp_groups; >> struct hmap lr_dp_groups; >> + struct uuidset sb_entries; >> }; >> >> struct ed_type_sync_to_sb_lb_data { >> @@ -347,16 +349,29 @@ sync_to_sb_lb_northd_handler(struct engine_node *node, void *data_) >> } >> >> bool >> -sync_to_sb_lb_sb_load_balancer(struct engine_node *node, void *data OVS_UNUSED) >> +sync_to_sb_lb_sb_load_balancer(struct engine_node *node, void *data_) >> { >> const struct sbrec_load_balancer_table *sb_load_balancer_table = >> EN_OVSDB_GET(engine_get_input("SB_load_balancer", node)); >> + struct ed_type_sync_to_sb_lb_data *data = data_; >> >> - /* The only reason to handle SB.Load_Balancer updates is to detect >> + /* The reasons 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. */ >> + * lack of indexing on the SB.Load_Balancer table. The other reason might >> + * be when someone removes the SB row while the NB row is still valid. >> + * All other changes are valid and performed by northd. */ >> + >> + const struct sbrec_load_balancer *sb_lb; >> + SBREC_LOAD_BALANCER_TABLE_FOR_EACH_TRACKED (sb_lb, >> + sb_load_balancer_table) { >> + if (sbrec_load_balancer_is_deleted(sb_lb) && >> + uuidset_find(&data->sb_lbs.sb_entries, &sb_lb->header_.uuid)) { >> + VLOG_WARN("A SB LB for \"%s\" is deleted but the NB LB entry " >> + "still exists.", sb_lb->name); >> + return false; >> + } >> + } >> + >> if (check_sb_lb_duplicates(sb_load_balancer_table)) { >> return false; >> } >> @@ -626,6 +641,7 @@ sb_lb_table_init(struct sb_lb_table *sb_lbs) >> hmap_init(&sb_lbs->entries); >> ovn_dp_groups_init(&sb_lbs->ls_dp_groups); >> ovn_dp_groups_init(&sb_lbs->lr_dp_groups); >> + uuidset_init(&sb_lbs->sb_entries); >> } >> >> static void >> @@ -638,6 +654,7 @@ sb_lb_table_clear(struct sb_lb_table *sb_lbs) >> >> ovn_dp_groups_clear(&sb_lbs->ls_dp_groups); >> ovn_dp_groups_clear(&sb_lbs->lr_dp_groups); >> + uuidset_clear(&sb_lbs->sb_entries); >> } >> >> static void >> @@ -647,6 +664,7 @@ sb_lb_table_destroy(struct sb_lb_table *sb_lbs) >> hmap_destroy(&sb_lbs->entries); >> ovn_dp_groups_destroy(&sb_lbs->ls_dp_groups); >> ovn_dp_groups_destroy(&sb_lbs->lr_dp_groups); >> + uuidset_destroy(&sb_lbs->sb_entries); >> } >> >> static struct sb_lb_record * >> @@ -693,6 +711,8 @@ sb_lb_table_build_and_sync( >> 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)) { >> + uuidset_find_and_delete(&sb_lbs->sb_entries, >> + &sbrec_lb->header_.uuid); >> sbrec_load_balancer_delete(sbrec_lb); >> continue; >> } >> @@ -711,6 +731,8 @@ sb_lb_table_build_and_sync( >> hmap_insert(&sb_lbs->entries, &sb_lb->key_node, >> uuid_hash(&sb_lb->lb_dps->lb->nlb->header_.uuid)); >> } else { >> + uuidset_find_and_delete(&sb_lbs->sb_entries, >> + &sbrec_lb->header_.uuid); >> sbrec_load_balancer_delete(sbrec_lb); >> } >> } >> @@ -770,6 +792,8 @@ sync_sb_lb_record(struct sb_lb_record *sb_lb, >> sbrec_lr_dp_group = sbrec_lb->lr_datapath_group; >> } >> >> + uuidset_insert(&sb_lbs->sb_entries, &sb_lb->sb_uuid); >> + >> if (lb_dps->n_nb_ls) { >> sb_lb->ls_dpg = ovn_dp_group_get(&sb_lbs->ls_dp_groups, >> lb_dps->n_nb_ls, >> @@ -931,6 +955,8 @@ sync_changed_lbs(struct sb_lb_table *sb_lbs, >> sbrec_load_balancer_table_get_for_uuid(sb_lb_table, >> &sb_lb->sb_uuid); >> if (sbrec_lb) { >> + uuidset_find_and_delete(&sb_lbs->sb_entries, >> + &sbrec_lb->header_.uuid); >> sbrec_load_balancer_delete(sbrec_lb); >> } >> >> @@ -965,6 +991,8 @@ sync_changed_lbs(struct sb_lb_table *sb_lbs, >> sbrec_load_balancer_table_get_for_uuid(sb_lb_table, >> &sb_lb->sb_uuid); >> if (sbrec_lb) { >> + uuidset_find_and_delete(&sb_lbs->sb_entries, >> + &sbrec_lb->header_.uuid); >> sbrec_load_balancer_delete(sbrec_lb); >> } >> >> diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at >> index 47ada5c70..65379ea57 100644 >> --- a/tests/ovn-macros.at >> +++ b/tests/ovn-macros.at >> @@ -249,7 +249,7 @@ ovn_start_northd() { >> local name=${d_prefix}northd${suffix} >> echo "${prefix}starting $name" >> test -d "$ovs_base/$name" || mkdir "$ovs_base/$name" >> - as $name start_daemon ovn-northd $northd_args -vjsonrpc \ >> + as $name start_daemon ovn-northd $northd_args \ >> --ovnnb-db=$OVN_NB_DB --ovnsb-db=$OVN_SB_DB >> } >> >> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at >> index a389d1988..74726f449 100644 >> --- a/tests/ovn-northd.at >> +++ b/tests/ovn-northd.at >> @@ -12721,3 +12721,41 @@ AT_CHECK([ovn-sbctl dump-flows lr | grep lr_in_dnat | ovn_strip_lflows], [0], [d >> >> AT_CLEANUP >> ]) >> + >> +OVN_FOR_EACH_NORTHD_NO_HV([ >> +AT_SETUP([LB re-sync]) >> +ovn_start >> + >> +check ovn-sbctl chassis-add hv1 geneve 127.0.0.1 >> + >> +check ovn-nbctl lr-add lr -- \ >> + lrp-add lr lr-ls 02:ac:10:01:00:01 172.16.1.1/24 >> + >> +check ovn-nbctl ls-add ls -- \ >> + lsp-add ls ls-lr -- \ >> + lsp-set-type ls-lr router -- \ >> + lsp-set-addresses ls-lr router -- \ >> + lsp-set-options ls-lr router-port=lr-ls >> + >> +check ovn-nbctl lb-add lb 172.16.10.10 192.168.10.10 >> +check ovn-nbctl --wait=sb lr-lb-add lr lb >> + >> +check_row_count Load_Balancer 1 >> + >> +check ovn-sbctl --all destroy Load_Balancer >> +check ovn-nbctl --wait=sb sync >> +check_row_count Load_Balancer 1 >> + >> +check ovn-nbctl --wait=sb lb-del lb >> +wait_row_count Load_Balancer 0 >> + >> +check ovn-nbctl lb-add lb 172.16.10.10 192.168.10.10 >> +check ovn-nbctl --wait=sb lr-lb-add lr lb >> +check_row_count Load_Balancer 1 >> + >> +check ovn-sbctl --all destroy Load_Balancer >> +check ovn-nbctl --wait=sb sync >> +check_row_count Load_Balancer 1 >> + >> +AT_CLEANUP >> +]) >> -- >> 2.45.1 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > _______________________________________________ > 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..12dbd139e 100644 --- a/northd/en-sync-sb.c +++ b/northd/en-sync-sb.c @@ -20,6 +20,7 @@ /* OVS includes. */ #include "lib/svec.h" +#include "lib/uuidset.h" #include "openvswitch/util.h" /* OVN includes. */ @@ -232,6 +233,7 @@ struct sb_lb_table { struct hmap entries; /* Stores struct sb_lb_record. */ struct hmap ls_dp_groups; struct hmap lr_dp_groups; + struct uuidset sb_entries; }; struct ed_type_sync_to_sb_lb_data { @@ -347,16 +349,29 @@ sync_to_sb_lb_northd_handler(struct engine_node *node, void *data_) } bool -sync_to_sb_lb_sb_load_balancer(struct engine_node *node, void *data OVS_UNUSED) +sync_to_sb_lb_sb_load_balancer(struct engine_node *node, void *data_) { const struct sbrec_load_balancer_table *sb_load_balancer_table = EN_OVSDB_GET(engine_get_input("SB_load_balancer", node)); + struct ed_type_sync_to_sb_lb_data *data = data_; - /* The only reason to handle SB.Load_Balancer updates is to detect + /* The reasons 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. */ + * lack of indexing on the SB.Load_Balancer table. The other reason might + * be when someone removes the SB row while the NB row is still valid. + * All other changes are valid and performed by northd. */ + + const struct sbrec_load_balancer *sb_lb; + SBREC_LOAD_BALANCER_TABLE_FOR_EACH_TRACKED (sb_lb, + sb_load_balancer_table) { + if (sbrec_load_balancer_is_deleted(sb_lb) && + uuidset_find(&data->sb_lbs.sb_entries, &sb_lb->header_.uuid)) { + VLOG_WARN("A SB LB for \"%s\" is deleted but the NB LB entry " + "still exists.", sb_lb->name); + return false; + } + } + if (check_sb_lb_duplicates(sb_load_balancer_table)) { return false; } @@ -626,6 +641,7 @@ sb_lb_table_init(struct sb_lb_table *sb_lbs) hmap_init(&sb_lbs->entries); ovn_dp_groups_init(&sb_lbs->ls_dp_groups); ovn_dp_groups_init(&sb_lbs->lr_dp_groups); + uuidset_init(&sb_lbs->sb_entries); } static void @@ -638,6 +654,7 @@ sb_lb_table_clear(struct sb_lb_table *sb_lbs) ovn_dp_groups_clear(&sb_lbs->ls_dp_groups); ovn_dp_groups_clear(&sb_lbs->lr_dp_groups); + uuidset_clear(&sb_lbs->sb_entries); } static void @@ -647,6 +664,7 @@ sb_lb_table_destroy(struct sb_lb_table *sb_lbs) hmap_destroy(&sb_lbs->entries); ovn_dp_groups_destroy(&sb_lbs->ls_dp_groups); ovn_dp_groups_destroy(&sb_lbs->lr_dp_groups); + uuidset_destroy(&sb_lbs->sb_entries); } static struct sb_lb_record * @@ -693,6 +711,8 @@ sb_lb_table_build_and_sync( 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)) { + uuidset_find_and_delete(&sb_lbs->sb_entries, + &sbrec_lb->header_.uuid); sbrec_load_balancer_delete(sbrec_lb); continue; } @@ -711,6 +731,8 @@ sb_lb_table_build_and_sync( hmap_insert(&sb_lbs->entries, &sb_lb->key_node, uuid_hash(&sb_lb->lb_dps->lb->nlb->header_.uuid)); } else { + uuidset_find_and_delete(&sb_lbs->sb_entries, + &sbrec_lb->header_.uuid); sbrec_load_balancer_delete(sbrec_lb); } } @@ -770,6 +792,8 @@ sync_sb_lb_record(struct sb_lb_record *sb_lb, sbrec_lr_dp_group = sbrec_lb->lr_datapath_group; } + uuidset_insert(&sb_lbs->sb_entries, &sb_lb->sb_uuid); + if (lb_dps->n_nb_ls) { sb_lb->ls_dpg = ovn_dp_group_get(&sb_lbs->ls_dp_groups, lb_dps->n_nb_ls, @@ -931,6 +955,8 @@ sync_changed_lbs(struct sb_lb_table *sb_lbs, sbrec_load_balancer_table_get_for_uuid(sb_lb_table, &sb_lb->sb_uuid); if (sbrec_lb) { + uuidset_find_and_delete(&sb_lbs->sb_entries, + &sbrec_lb->header_.uuid); sbrec_load_balancer_delete(sbrec_lb); } @@ -965,6 +991,8 @@ sync_changed_lbs(struct sb_lb_table *sb_lbs, sbrec_load_balancer_table_get_for_uuid(sb_lb_table, &sb_lb->sb_uuid); if (sbrec_lb) { + uuidset_find_and_delete(&sb_lbs->sb_entries, + &sbrec_lb->header_.uuid); sbrec_load_balancer_delete(sbrec_lb); } diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at index 47ada5c70..65379ea57 100644 --- a/tests/ovn-macros.at +++ b/tests/ovn-macros.at @@ -249,7 +249,7 @@ ovn_start_northd() { local name=${d_prefix}northd${suffix} echo "${prefix}starting $name" test -d "$ovs_base/$name" || mkdir "$ovs_base/$name" - as $name start_daemon ovn-northd $northd_args -vjsonrpc \ + as $name start_daemon ovn-northd $northd_args \ --ovnnb-db=$OVN_NB_DB --ovnsb-db=$OVN_SB_DB } diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index a389d1988..74726f449 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -12721,3 +12721,41 @@ AT_CHECK([ovn-sbctl dump-flows lr | grep lr_in_dnat | ovn_strip_lflows], [0], [d AT_CLEANUP ]) + +OVN_FOR_EACH_NORTHD_NO_HV([ +AT_SETUP([LB re-sync]) +ovn_start + +check ovn-sbctl chassis-add hv1 geneve 127.0.0.1 + +check ovn-nbctl lr-add lr -- \ + lrp-add lr lr-ls 02:ac:10:01:00:01 172.16.1.1/24 + +check ovn-nbctl ls-add ls -- \ + lsp-add ls ls-lr -- \ + lsp-set-type ls-lr router -- \ + lsp-set-addresses ls-lr router -- \ + lsp-set-options ls-lr router-port=lr-ls + +check ovn-nbctl lb-add lb 172.16.10.10 192.168.10.10 +check ovn-nbctl --wait=sb lr-lb-add lr lb + +check_row_count Load_Balancer 1 + +check ovn-sbctl --all destroy Load_Balancer +check ovn-nbctl --wait=sb sync +check_row_count Load_Balancer 1 + +check ovn-nbctl --wait=sb lb-del lb +wait_row_count Load_Balancer 0 + +check ovn-nbctl lb-add lb 172.16.10.10 192.168.10.10 +check ovn-nbctl --wait=sb lr-lb-add lr lb +check_row_count Load_Balancer 1 + +check ovn-sbctl --all destroy Load_Balancer +check ovn-nbctl --wait=sb sync +check_row_count Load_Balancer 1 + +AT_CLEANUP +])
When someone removes SB LB it won't be detected until the next recompute of the "sync_to_sb_lb" node. This will cause traffic disruptions in case of hairpin as the flows directly depend on the SB LB entries. Add check to trigger recompute when we detect that the row is missing in SB, but still present in NB. Reported-at: https://issues.redhat.com/browse/FDP-682 Signed-off-by: Ales Musil <amusil@redhat.com> --- northd/en-sync-sb.c | 38 +++++++++++++++++++++++++++++++++----- tests/ovn-macros.at | 2 +- tests/ovn-northd.at | 38 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 72 insertions(+), 6 deletions(-)