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.
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
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
