On 7/4/24 18:01, Numan Siddique wrote:
> On Wed, Jul 3, 2024 at 8:20 AM Ales Musil <[email protected]> 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 <[email protected]>
>
> 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
>> [email protected]
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
> _______________________________________________
> 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
