[ovs-dev,RFC,2/2] northd: sync lb applied to logical routers in sb db lb table

Introduce lr_datapath_group column in the load_balancer table of the SB
db.
Sync load_balancers applied to logical_routers to Load_Balancer table in
the SouthBound database.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2193323
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 controller/local_data.c     |   8 +++
 controller/ovn-controller.c |   6 ++
 lib/lb.h                    |   3 +-
 northd/northd.c             |  78 ++++++++++++++++++------
 ovn-sb.ovsschema            |   6 +-
 ovn-sb.xml                  |   6 ++
 tests/ovn-northd.at         |  17 +++++-
 tests/system-ovn.at         | 117 ++++++++++++++++++++++++++++++++++++
 utilities/ovn-sbctl.c       |   1 +
 9 files changed, 219 insertions(+), 23 deletions(-)

Hi Lorenzo, I have one note below

On 6/9/23 12:07, Lorenzo Bianconi wrote:
> Introduce lr_datapath_group column in the load_balancer table of the SB
> db.
> Sync load_balancers applied to logical_routers to Load_Balancer table in
> the SouthBound database.
> 
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2193323
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> ---
>   controller/local_data.c     |   8 +++
>   controller/ovn-controller.c |   6 ++
>   lib/lb.h                    |   3 +-
>   northd/northd.c             |  78 ++++++++++++++++++------
>   ovn-sb.ovsschema            |   6 +-
>   ovn-sb.xml                  |   6 ++
>   tests/ovn-northd.at         |  17 +++++-
>   tests/system-ovn.at         | 117 ++++++++++++++++++++++++++++++++++++
>   utilities/ovn-sbctl.c       |   1 +
>   9 files changed, 219 insertions(+), 23 deletions(-)
> 
> diff --git a/controller/local_data.c b/controller/local_data.c
> index 2950434ac..c8aebcf50 100644
> --- a/controller/local_data.c
> +++ b/controller/local_data.c
> @@ -670,5 +670,13 @@ lb_is_local(const struct sbrec_load_balancer *sbrec_lb,
>           }
>       }
>   
> +    dp_group = sbrec_lb->lr_datapath_group;
> +    for (size_t i = 0; dp_group && i < dp_group->n_datapaths; i++) {
> +        if (get_local_datapath(local_datapaths,
> +                               dp_group->datapaths[i]->tunnel_key)) {
> +            return true;
> +        }
> +    }
> +
>       return false;
>   }
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index ab4896b91..93eaf6d9a 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -2705,6 +2705,12 @@ load_balancers_by_dp_init(const struct hmap *local_datapaths,
>                                            lb->ls_datapath_group->datapaths[i],
>                                            lb, lbs);
>           }
> +        for (size_t i = 0; lb->lr_datapath_group
> +                           && i < lb->lr_datapath_group->n_datapaths; i++) {
> +            load_balancers_by_dp_add_one(local_datapaths,
> +                                         lb->lr_datapath_group->datapaths[i],
> +                                         lb, lbs);
> +        }
>       }
>       return lbs;
>   }
> diff --git a/lib/lb.h b/lib/lb.h
> index 23d8fc9e9..2456423cc 100644
> --- a/lib/lb.h
> +++ b/lib/lb.h
> @@ -85,7 +85,8 @@ struct ovn_northd_lb {
>       size_t n_nb_lr;
>       unsigned long *nb_lr_map;
>   
> -    struct ovn_dp_group *dpg;
> +    struct ovn_dp_group *ls_dpg;
> +    struct ovn_dp_group *lr_dpg;
>   };
>   
>   struct ovn_lb_vip {
> diff --git a/northd/northd.c b/northd/northd.c
> index fad8ab0ec..9341823e3 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -4412,10 +4412,11 @@ ovn_dp_group_get_or_create(struct ovsdb_idl_txn *ovnsb_txn,
>   static void
>   sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
>            const struct sbrec_load_balancer_table *sbrec_load_balancer_table,
> -         struct ovn_datapaths *ls_datapaths, struct hmap *lbs)
> +         struct ovn_datapaths *ls_datapaths,
> +         struct ovn_datapaths *lr_datapaths, struct hmap *lbs)
>   {
> -    struct hmap dp_groups = HMAP_INITIALIZER(&dp_groups);
> -    size_t bitmap_len = ods_size(ls_datapaths);
> +    struct hmap ls_dp_groups = HMAP_INITIALIZER(&ls_dp_groups);
> +    struct hmap lr_dp_groups = HMAP_INITIALIZER(&lr_dp_groups);
>       struct ovn_northd_lb *lb;
>   
>       /* Delete any stale SB load balancer rows and create datapath
> @@ -4440,7 +4441,8 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
>            * are not indexed in any way.
>            */
>           lb = ovn_northd_lb_find(lbs, &lb_uuid);
> -        if (!lb || !lb->n_nb_ls || !hmapx_add(&existing_lbs, lb)) {
> +        if (!lb || (!lb->n_nb_ls && !lb->n_nb_lr) ||
> +            !hmapx_add(&existing_lbs, lb)) {
>               sbrec_load_balancer_delete(sbrec_lb);
>               continue;
>           }
> @@ -4448,11 +4450,20 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
>           lb->slb = sbrec_lb;
>   
>           /* Find or create datapath group for this load balancer. */
> -        lb->dpg = ovn_dp_group_get_or_create(ovnsb_txn, &dp_groups,
> -                                             lb->slb->ls_datapath_group,
> -                                             lb->n_nb_ls, lb->nb_ls_map,
> -                                             bitmap_len, true,
> -                                             ls_datapaths, NULL);
> +        if (lb->n_nb_ls) {
> +            lb->ls_dpg = ovn_dp_group_get_or_create(ovnsb_txn, &ls_dp_groups,
> +                                                    lb->slb->ls_datapath_group,
> +                                                    lb->n_nb_ls, lb->nb_ls_map,
> +                                                    ods_size(ls_datapaths),
> +                                                    true, ls_datapaths, NULL);
> +        }
> +        if (lb->n_nb_lr) {
> +            lb->lr_dpg = ovn_dp_group_get_or_create(ovnsb_txn, &lr_dp_groups,
> +                                                    lb->slb->lr_datapath_group,
> +                                                    lb->n_nb_lr, lb->nb_lr_map,
> +                                                    ods_size(lr_datapaths),
> +                                                    false, NULL, lr_datapaths);
> +        }
>       }
>       hmapx_destroy(&existing_lbs);
>   
> @@ -4460,7 +4471,7 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
>        * the SB load balancer columns. */
>       HMAP_FOR_EACH (lb, hmap_node, lbs) {
>   
> -        if (!lb->n_nb_ls) {
> +        if (!lb->n_nb_ls && !lb->n_nb_lr) {
>               continue;
>           }
>   
> @@ -4483,19 +4494,33 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
>           }
>   
>           /* Find or create datapath group for this load balancer. */
> -        if (!lb->dpg) {
> -            lb->dpg = ovn_dp_group_get_or_create(ovnsb_txn, &dp_groups,
> -                                                 lb->slb->ls_datapath_group,
> -                                                 lb->n_nb_ls, lb->nb_ls_map,
> -                                                 bitmap_len, true,
> -                                                 ls_datapaths, NULL);
> +        if (!lb->ls_dpg && lb->n_nb_ls) {
> +            lb->ls_dpg = ovn_dp_group_get_or_create(ovnsb_txn, &ls_dp_groups,
> +                                                    lb->slb->ls_datapath_group,
> +                                                    lb->n_nb_ls, lb->nb_ls_map,
> +                                                    ods_size(ls_datapaths),
> +                                                    true, ls_datapaths, NULL);
> +        }
> +        if (!lb->lr_dpg && lb->n_nb_lr) {
> +            lb->lr_dpg = ovn_dp_group_get_or_create(ovnsb_txn, &lr_dp_groups,
> +                                                    lb->slb->lr_datapath_group,
> +                                                    lb->n_nb_lr, lb->nb_lr_map,
> +                                                    ods_size(lr_datapaths),
> +                                                    false, NULL, lr_datapaths);
>           }
>   
>           /* Update columns. */
>           sbrec_load_balancer_set_name(lb->slb, lb->nlb->name);
>           sbrec_load_balancer_set_vips(lb->slb, ovn_northd_lb_get_vips(lb));
>           sbrec_load_balancer_set_protocol(lb->slb, lb->nlb->protocol);
> -        sbrec_load_balancer_set_ls_datapath_group(lb->slb, lb->dpg->dp_group);
> +        if (lb->ls_dpg) {
> +            sbrec_load_balancer_set_ls_datapath_group(lb->slb,
> +                                                      lb->ls_dpg->dp_group);
> +        }
> +        if (lb->lr_dpg) {
> +            sbrec_load_balancer_set_lr_datapath_group(lb->slb,
> +                                                      lb->lr_dpg->dp_group);
> +        }
>           sbrec_load_balancer_set_options(lb->slb, &options);
>           /* Clearing 'datapaths' column, since 'dp_group' is in use. */
>           sbrec_load_balancer_set_datapaths(lb->slb, NULL, 0);
> @@ -4503,11 +4528,17 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
>       }
>   
>       struct ovn_dp_group *dpg;
> -    HMAP_FOR_EACH_POP (dpg, node, &dp_groups) {
> +    HMAP_FOR_EACH_POP (dpg, node, &ls_dp_groups) {
> +        bitmap_free(dpg->bitmap);
> +        free(dpg);
> +    }
> +    hmap_destroy(&ls_dp_groups);
> +
> +    HMAP_FOR_EACH_POP (dpg, node, &lr_dp_groups) {
>           bitmap_free(dpg->bitmap);
>           free(dpg);
>       }
> -    hmap_destroy(&dp_groups);
> +    hmap_destroy(&lr_dp_groups);
>   
>       /* Datapath_Binding.load_balancers is not used anymore, it's still in the
>        * schema for compatibility reasons.  Reset it to empty, just in case.
> @@ -4516,6 +4547,13 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
>       HMAP_FOR_EACH (od, key_node, &ls_datapaths->datapaths) {
>           ovs_assert(od->nbs);
>   
> +        if (od->sb->n_load_balancers) {
> +            sbrec_datapath_binding_set_load_balancers(od->sb, NULL, 0);
> +        }
> +    }
> +    HMAP_FOR_EACH (od, key_node, &lr_datapaths->datapaths) {
> +        ovs_assert(od->nbr);
> +
>           if (od->sb->n_load_balancers) {
>               sbrec_datapath_binding_set_load_balancers(od->sb, NULL, 0);
>           }
> @@ -16683,7 +16721,7 @@ ovnnb_db_run(struct northd_input *input_data,
>       ovn_update_ipv6_prefix(&data->lr_ports);
>   
>       sync_lbs(ovnsb_txn, input_data->sbrec_load_balancer_table,
> -             &data->ls_datapaths, &data->lbs);
> +             &data->ls_datapaths, &data->lr_datapaths, &data->lbs);
>       sync_port_groups(ovnsb_txn, input_data->sbrec_port_group_table,
>                        &data->port_groups);
>       sync_meters(ovnsb_txn, input_data->nbrec_meter_table,
> diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
> index e02fbd884..2a81d54ac 100644
> --- a/ovn-sb.ovsschema
> +++ b/ovn-sb.ovsschema
> @@ -1,7 +1,7 @@
>   {
>       "name": "OVN_Southbound",
>       "version": "20.27.2",
> -    "cksum": "2970847454 30465",
> +    "cksum": "1977000278 30679",
>       "tables": {
>           "SB_Global": {
>               "columns": {
> @@ -534,6 +534,10 @@
>                       {"type": {"key": {"type": "uuid",
>                                         "refTable": "Logical_DP_Group"},
>                                 "min": 0, "max": 1}},
> +                "lr_datapath_group":
> +                    {"type": {"key": {"type": "uuid",
> +                                      "refTable": "Logical_DP_Group"},
> +                              "min": 0, "max": 1}},
>                   "options": {
>                        "type": {"key": "string",
>                                 "value": "string",
> diff --git a/ovn-sb.xml b/ovn-sb.xml
> index 5d11d059b..de92a63f7 100644
> --- a/ovn-sb.xml
> +++ b/ovn-sb.xml
> @@ -4885,6 +4885,12 @@ tcp.flags = RST;
>         datapaths in a group.
>       </column>
>   
> +    <column name="lr_datapath_group">
> +      The group of logical router datapaths to which this load balancer
> +      applies to. This means that the same load balancer applies to all
> +      datapaths in a group.
> +    </column>
> +
>       <group title="Load_Balancer options">
>       <column name="options" key="hairpin_snat_ip">
>         IP to be used as source IP for packets that have been hair-pinned after
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 63caf8d66..0a7725c37 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -2811,6 +2811,15 @@ sb:load_balancer vips,protocol name=lbg0
>   
>   check ovn-nbctl lr-add lr0 -- add logical_router lr0 load_balancer_group $lbg
>   check ovn-nbctl --wait=sb lr-lb-add lr0 lb0
> +check_row_count sb:load_balancer 2
> +
> +lr0_sb_uuid=$(fetch_column datapath_binding _uuid external_ids:name=lr0)
> +lb0_lr_dp_group=$(fetch_column sb:load_balancer lr_datapath_group name=lb0)
> +
> +AT_CHECK_UNQUOTED([ovn-sbctl --bare --columns _uuid,datapaths find Logical_DP_Group dnl
> +                    | grep -A1 $lb0_lr_dp_group | tail -1], [0], [dnl
> +$lr0_sb_uuid
> +])
>   
>   echo
>   echo "__file__:__line__: Check that SB lb0 has only sw0 in datapaths column."
> @@ -2856,7 +2865,13 @@ check_row_count sb:load_balancer 2
>   lbg1=$(fetch_column nb:load_balancer _uuid name=lbg1)
>   check ovn-nbctl add load_balancer_group $lbg load_balancer $lbg1
>   check ovn-nbctl --wait=sb lr-lb-add lr0 lb1
> -check_row_count sb:load_balancer 3
> +check_row_count sb:load_balancer 4
> +
> +lb1_lr_dp_group=$(fetch_column sb:load_balancer lr_datapath_group name=lb1)
> +AT_CHECK_UNQUOTED([ovn-sbctl --bare --columns _uuid,datapaths find Logical_DP_Group dnl
> +                    | grep -A1 $lb1_lr_dp_group | tail -1], [0], [dnl
> +$lr0_sb_uuid
> +])
>   
>   echo
>   echo "__file__:__line__: Associate lb1 to sw1 and check that lb1 is created in SB DB."
> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> index 6669c18e7..08b380d10 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -11546,3 +11546,120 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
>   
>   AT_CLEANUP
>   ])
> +
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([ct_flush on logical router load balancer])
> +AT_KEYWORDS([ct-lr-flush])
> +CHECK_CONNTRACK()
> +CHECK_CONNTRACK_NAT()
> +ovn_start
> +OVS_TRAFFIC_VSWITCHD_START()
> +ADD_BR([br-int])
> +#
> +# Set external-ids in br-int needed for ovn-controller
> +ovs-vsctl \
> +        -- set Open_vSwitch . external-ids:system-id=hv1 \
> +        -- set Open_vSwitch . external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
> +        -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
> +        -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
> +        -- set bridge br-int fail-mode=secure other-config:disable-in-band=true
> +
> +start_daemon ovn-controller
> +
> +check ovn-nbctl lr-add R1
> +
> +check ovn-nbctl ls-add sw0
> +check ovn-nbctl ls-add public
> +
> +check ovn-nbctl lrp-add R1 rp-sw0 00:00:01:01:02:03 192.168.1.1/24
> +check ovn-nbctl lrp-add R1 rp-public 00:00:02:01:02:03 172.16.1.1/24
> +
> +check ovn-nbctl set logical_router R1 options:chassis=hv1
> +
> +check ovn-nbctl lsp-add sw0 sw0-rp -- set Logical_Switch_Port sw0-rp \
> +    type=router options:router-port=rp-sw0 \
> +    -- lsp-set-addresses sw0-rp router
> +
> +check ovn-nbctl lsp-add sw0 sw0-vm \
> +    -- lsp-set-addresses sw0-vm "00:00:01:01:02:04 192.168.1.2/24"
> +
> +check ovn-nbctl lsp-add public public-rp -- set Logical_Switch_Port public-rp \
> +    type=router options:router-port=rp-public \
> +    -- lsp-set-addresses public-rp router
> +
> +check ovn-nbctl lsp-add public public-vm \
> +   -- lsp-set-addresses public-vm "00:00:02:01:02:04 172.16.1.2/24"
> +
> +ADD_NAMESPACES(sw0-vm)
> +ADD_VETH(sw0-vm, sw0-vm, br-int, "192.168.1.2/24", "00:00:01:01:02:04", \
> +         "192.168.1.1")
> +OVS_WAIT_UNTIL([test "$(ip netns exec sw0-vm ip a | grep fe80 | grep tentative)" = ""])
> +
> +ADD_NAMESPACES(public-vm)
> +ADD_VETH(public-vm, public-vm, br-int, "172.16.1.2/24", "00:00:02:01:02:04", \
> +         "172.16.1.1")
> +
> +OVS_WAIT_UNTIL([test "$(ip netns exec public-vm ip a | grep fe80 | grep tentative)" = ""])
> +
> +# Start webservers in 'server'.
> +OVS_START_L7([sw0-vm], [http])
> +
> +# Create a load balancer and associate to R1
> +check ovn-nbctl lb-add lb1 172.16.1.150:80 192.168.1.2:80 \
> +    -- set load_balancer lb1 options:ct_flush="true"
> +check ovn-nbctl lr-lb-add R1 lb1
> +
> +check ovn-nbctl --wait=hv sync
> +
> +for i in $(seq 1 5); do
> +    echo Request $i
> +    NS_CHECK_EXEC([public-vm], [wget 172.16.1.150 -t 5 -T 1 --retry-connrefused -v -o wget$i.log])
> +done
> +
> +OVS_WAIT_FOR_OUTPUT([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.150) | wc -l ], [0], [dnl
> +1
> +])
> +
> +check ovn-nbctl lb-del lb1
> +
> +OVS_WAIT_FOR_OUTPUT([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.150) | wc -l ], [0], [dnl
> +0
> +])
> +
> +check ovn-nbctl lb-add lb2 172.16.1.151:80 192.168.1.2:80
> +check ovn-nbctl lr-lb-add R1 lb2
> +
> +check ovn-nbctl --wait=hv sync
> +
> +for i in $(seq 1 5); do
> +    echo Request $i
> +    NS_CHECK_EXEC([public-vm], [wget 172.16.1.151 -t 5 -T 1 --retry-connrefused -v -o wget$i.log])
> +done
> +
> +OVS_WAIT_FOR_OUTPUT([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.151) | wc -l ], [0], [dnl
> +1
> +])
> +
> +check ovn-nbctl lb-del lb2
> +
> +OVS_WAIT_FOR_OUTPUT([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.151) | wc -l ], [0], [dnl
> +1
> +])

The version of this test on the linked BZ expects 0 as the output when 
dumping conntrack at this point. Deleting the load balancer should flush 
the conntrack entry, so there should now be no conntrack entries for the 
load balancer VIP.

Is there a reason why this was changed to 1 instead? The problem is that 
the condition being checked before and after deleting the load balancer 
is the same. It's not clear what is being tested here.

I think one of two changes needs to happen with this test:

1) If the test is not actually exercising the expected 
conntrack-flushing behavior, then the test needs to be adjusted to 
properly test the conntrack flush.

2) If the test is correct, and we actually do expect a conntrack entry 
to still exist, then rather than counting the number of conntrack 
entries, we need to inspect the conntrack entry to ensure that something 
has actually happened as a result of deleting the load balancer.

> +
> +OVS_APP_EXIT_AND_WAIT([ovn-controller])
> +
> +as ovn-sb
> +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> +
> +as ovn-nb
> +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> +
> +as northd
> +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE])
> +
> +as
> +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
> +/Failed to acquire.*/d
> +/connection dropped.*/d"])
> +AT_CLEANUP
> +])
> diff --git a/utilities/ovn-sbctl.c b/utilities/ovn-sbctl.c
> index ddd9a9ca9..0e3afaea7 100644
> --- a/utilities/ovn-sbctl.c
> +++ b/utilities/ovn-sbctl.c
> @@ -397,6 +397,7 @@ pre_get_info(struct ctl_context *ctx)
>   
>       ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_datapaths);
>       ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_ls_datapath_group);
> +    ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_lr_datapath_group);
>       ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_vips);
>       ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_name);
>       ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_protocol);

On 6/9/23 12:58, Mark Michelson wrote:
> Hi Lorenzo, I have one note below
> 
> On 6/9/23 12:07, Lorenzo Bianconi wrote:
>> Introduce lr_datapath_group column in the load_balancer table of the SB
>> db.
>> Sync load_balancers applied to logical_routers to Load_Balancer table in
>> the SouthBound database.
>>
>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2193323
>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
>> ---
>>   controller/local_data.c     |   8 +++
>>   controller/ovn-controller.c |   6 ++
>>   lib/lb.h                    |   3 +-
>>   northd/northd.c             |  78 ++++++++++++++++++------
>>   ovn-sb.ovsschema            |   6 +-
>>   ovn-sb.xml                  |   6 ++
>>   tests/ovn-northd.at         |  17 +++++-
>>   tests/system-ovn.at         | 117 ++++++++++++++++++++++++++++++++++++
>>   utilities/ovn-sbctl.c       |   1 +
>>   9 files changed, 219 insertions(+), 23 deletions(-)
>>
>> diff --git a/controller/local_data.c b/controller/local_data.c
>> index 2950434ac..c8aebcf50 100644
>> --- a/controller/local_data.c
>> +++ b/controller/local_data.c
>> @@ -670,5 +670,13 @@ lb_is_local(const struct sbrec_load_balancer 
>> *sbrec_lb,
>>           }
>>       }
>> +    dp_group = sbrec_lb->lr_datapath_group;
>> +    for (size_t i = 0; dp_group && i < dp_group->n_datapaths; i++) {
>> +        if (get_local_datapath(local_datapaths,
>> +                               dp_group->datapaths[i]->tunnel_key)) {
>> +            return true;
>> +        }
>> +    }
>> +
>>       return false;
>>   }
>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
>> index ab4896b91..93eaf6d9a 100644
>> --- a/controller/ovn-controller.c
>> +++ b/controller/ovn-controller.c
>> @@ -2705,6 +2705,12 @@ load_balancers_by_dp_init(const struct hmap 
>> *local_datapaths,
>>                                            
>> lb->ls_datapath_group->datapaths[i],
>>                                            lb, lbs);
>>           }
>> +        for (size_t i = 0; lb->lr_datapath_group
>> +                           && i < lb->lr_datapath_group->n_datapaths; 
>> i++) {
>> +            load_balancers_by_dp_add_one(local_datapaths,
>> +                                         
>> lb->lr_datapath_group->datapaths[i],
>> +                                         lb, lbs);
>> +        }
>>       }
>>       return lbs;
>>   }
>> diff --git a/lib/lb.h b/lib/lb.h
>> index 23d8fc9e9..2456423cc 100644
>> --- a/lib/lb.h
>> +++ b/lib/lb.h
>> @@ -85,7 +85,8 @@ struct ovn_northd_lb {
>>       size_t n_nb_lr;
>>       unsigned long *nb_lr_map;
>> -    struct ovn_dp_group *dpg;
>> +    struct ovn_dp_group *ls_dpg;
>> +    struct ovn_dp_group *lr_dpg;
>>   };
>>   struct ovn_lb_vip {
>> diff --git a/northd/northd.c b/northd/northd.c
>> index fad8ab0ec..9341823e3 100644
>> --- a/northd/northd.c
>> +++ b/northd/northd.c
>> @@ -4412,10 +4412,11 @@ ovn_dp_group_get_or_create(struct 
>> ovsdb_idl_txn *ovnsb_txn,
>>   static void
>>   sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
>>            const struct sbrec_load_balancer_table 
>> *sbrec_load_balancer_table,
>> -         struct ovn_datapaths *ls_datapaths, struct hmap *lbs)
>> +         struct ovn_datapaths *ls_datapaths,
>> +         struct ovn_datapaths *lr_datapaths, struct hmap *lbs)
>>   {
>> -    struct hmap dp_groups = HMAP_INITIALIZER(&dp_groups);
>> -    size_t bitmap_len = ods_size(ls_datapaths);
>> +    struct hmap ls_dp_groups = HMAP_INITIALIZER(&ls_dp_groups);
>> +    struct hmap lr_dp_groups = HMAP_INITIALIZER(&lr_dp_groups);
>>       struct ovn_northd_lb *lb;
>>       /* Delete any stale SB load balancer rows and create datapath
>> @@ -4440,7 +4441,8 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
>>            * are not indexed in any way.
>>            */
>>           lb = ovn_northd_lb_find(lbs, &lb_uuid);
>> -        if (!lb || !lb->n_nb_ls || !hmapx_add(&existing_lbs, lb)) {
>> +        if (!lb || (!lb->n_nb_ls && !lb->n_nb_lr) ||
>> +            !hmapx_add(&existing_lbs, lb)) {
>>               sbrec_load_balancer_delete(sbrec_lb);
>>               continue;
>>           }
>> @@ -4448,11 +4450,20 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
>>           lb->slb = sbrec_lb;
>>           /* Find or create datapath group for this load balancer. */
>> -        lb->dpg = ovn_dp_group_get_or_create(ovnsb_txn, &dp_groups,
>> -                                             lb->slb->ls_datapath_group,
>> -                                             lb->n_nb_ls, lb->nb_ls_map,
>> -                                             bitmap_len, true,
>> -                                             ls_datapaths, NULL);
>> +        if (lb->n_nb_ls) {
>> +            lb->ls_dpg = ovn_dp_group_get_or_create(ovnsb_txn, 
>> &ls_dp_groups,
>> +                                                    
>> lb->slb->ls_datapath_group,
>> +                                                    lb->n_nb_ls, 
>> lb->nb_ls_map,
>> +                                                    
>> ods_size(ls_datapaths),
>> +                                                    true, 
>> ls_datapaths, NULL);
>> +        }
>> +        if (lb->n_nb_lr) {
>> +            lb->lr_dpg = ovn_dp_group_get_or_create(ovnsb_txn, 
>> &lr_dp_groups,
>> +                                                    
>> lb->slb->lr_datapath_group,
>> +                                                    lb->n_nb_lr, 
>> lb->nb_lr_map,
>> +                                                    
>> ods_size(lr_datapaths),
>> +                                                    false, NULL, 
>> lr_datapaths);
>> +        }
>>       }
>>       hmapx_destroy(&existing_lbs);
>> @@ -4460,7 +4471,7 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
>>        * the SB load balancer columns. */
>>       HMAP_FOR_EACH (lb, hmap_node, lbs) {
>> -        if (!lb->n_nb_ls) {
>> +        if (!lb->n_nb_ls && !lb->n_nb_lr) {
>>               continue;
>>           }
>> @@ -4483,19 +4494,33 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
>>           }
>>           /* Find or create datapath group for this load balancer. */
>> -        if (!lb->dpg) {
>> -            lb->dpg = ovn_dp_group_get_or_create(ovnsb_txn, &dp_groups,
>> -                                                 
>> lb->slb->ls_datapath_group,
>> -                                                 lb->n_nb_ls, 
>> lb->nb_ls_map,
>> -                                                 bitmap_len, true,
>> -                                                 ls_datapaths, NULL);
>> +        if (!lb->ls_dpg && lb->n_nb_ls) {
>> +            lb->ls_dpg = ovn_dp_group_get_or_create(ovnsb_txn, 
>> &ls_dp_groups,
>> +                                                    
>> lb->slb->ls_datapath_group,
>> +                                                    lb->n_nb_ls, 
>> lb->nb_ls_map,
>> +                                                    
>> ods_size(ls_datapaths),
>> +                                                    true, 
>> ls_datapaths, NULL);
>> +        }
>> +        if (!lb->lr_dpg && lb->n_nb_lr) {
>> +            lb->lr_dpg = ovn_dp_group_get_or_create(ovnsb_txn, 
>> &lr_dp_groups,
>> +                                                    
>> lb->slb->lr_datapath_group,
>> +                                                    lb->n_nb_lr, 
>> lb->nb_lr_map,
>> +                                                    
>> ods_size(lr_datapaths),
>> +                                                    false, NULL, 
>> lr_datapaths);
>>           }
>>           /* Update columns. */
>>           sbrec_load_balancer_set_name(lb->slb, lb->nlb->name);
>>           sbrec_load_balancer_set_vips(lb->slb, 
>> ovn_northd_lb_get_vips(lb));
>>           sbrec_load_balancer_set_protocol(lb->slb, lb->nlb->protocol);
>> -        sbrec_load_balancer_set_ls_datapath_group(lb->slb, 
>> lb->dpg->dp_group);
>> +        if (lb->ls_dpg) {
>> +            sbrec_load_balancer_set_ls_datapath_group(lb->slb,
>> +                                                      
>> lb->ls_dpg->dp_group);
>> +        }
>> +        if (lb->lr_dpg) {
>> +            sbrec_load_balancer_set_lr_datapath_group(lb->slb,
>> +                                                      
>> lb->lr_dpg->dp_group);
>> +        }
>>           sbrec_load_balancer_set_options(lb->slb, &options);
>>           /* Clearing 'datapaths' column, since 'dp_group' is in use. */
>>           sbrec_load_balancer_set_datapaths(lb->slb, NULL, 0);
>> @@ -4503,11 +4528,17 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
>>       }
>>       struct ovn_dp_group *dpg;
>> -    HMAP_FOR_EACH_POP (dpg, node, &dp_groups) {
>> +    HMAP_FOR_EACH_POP (dpg, node, &ls_dp_groups) {
>> +        bitmap_free(dpg->bitmap);
>> +        free(dpg);
>> +    }
>> +    hmap_destroy(&ls_dp_groups);
>> +
>> +    HMAP_FOR_EACH_POP (dpg, node, &lr_dp_groups) {
>>           bitmap_free(dpg->bitmap);
>>           free(dpg);
>>       }
>> -    hmap_destroy(&dp_groups);
>> +    hmap_destroy(&lr_dp_groups);
>>       /* Datapath_Binding.load_balancers is not used anymore, it's 
>> still in the
>>        * schema for compatibility reasons.  Reset it to empty, just in 
>> case.
>> @@ -4516,6 +4547,13 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
>>       HMAP_FOR_EACH (od, key_node, &ls_datapaths->datapaths) {
>>           ovs_assert(od->nbs);
>> +        if (od->sb->n_load_balancers) {
>> +            sbrec_datapath_binding_set_load_balancers(od->sb, NULL, 0);
>> +        }
>> +    }
>> +    HMAP_FOR_EACH (od, key_node, &lr_datapaths->datapaths) {
>> +        ovs_assert(od->nbr);
>> +
>>           if (od->sb->n_load_balancers) {
>>               sbrec_datapath_binding_set_load_balancers(od->sb, NULL, 0);
>>           }
>> @@ -16683,7 +16721,7 @@ ovnnb_db_run(struct northd_input *input_data,
>>       ovn_update_ipv6_prefix(&data->lr_ports);
>>       sync_lbs(ovnsb_txn, input_data->sbrec_load_balancer_table,
>> -             &data->ls_datapaths, &data->lbs);
>> +             &data->ls_datapaths, &data->lr_datapaths, &data->lbs);
>>       sync_port_groups(ovnsb_txn, input_data->sbrec_port_group_table,
>>                        &data->port_groups);
>>       sync_meters(ovnsb_txn, input_data->nbrec_meter_table,
>> diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
>> index e02fbd884..2a81d54ac 100644
>> --- a/ovn-sb.ovsschema
>> +++ b/ovn-sb.ovsschema
>> @@ -1,7 +1,7 @@
>>   {
>>       "name": "OVN_Southbound",
>>       "version": "20.27.2",
>> -    "cksum": "2970847454 30465",
>> +    "cksum": "1977000278 30679",
>>       "tables": {
>>           "SB_Global": {
>>               "columns": {
>> @@ -534,6 +534,10 @@
>>                       {"type": {"key": {"type": "uuid",
>>                                         "refTable": "Logical_DP_Group"},
>>                                 "min": 0, "max": 1}},
>> +                "lr_datapath_group":
>> +                    {"type": {"key": {"type": "uuid",
>> +                                      "refTable": "Logical_DP_Group"},
>> +                              "min": 0, "max": 1}},
>>                   "options": {
>>                        "type": {"key": "string",
>>                                 "value": "string",
>> diff --git a/ovn-sb.xml b/ovn-sb.xml
>> index 5d11d059b..de92a63f7 100644
>> --- a/ovn-sb.xml
>> +++ b/ovn-sb.xml
>> @@ -4885,6 +4885,12 @@ tcp.flags = RST;
>>         datapaths in a group.
>>       </column>
>> +    <column name="lr_datapath_group">
>> +      The group of logical router datapaths to which this load balancer
>> +      applies to. This means that the same load balancer applies to all
>> +      datapaths in a group.
>> +    </column>
>> +
>>       <group title="Load_Balancer options">
>>       <column name="options" key="hairpin_snat_ip">
>>         IP to be used as source IP for packets that have been 
>> hair-pinned after
>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
>> index 63caf8d66..0a7725c37 100644
>> --- a/tests/ovn-northd.at
>> +++ b/tests/ovn-northd.at
>> @@ -2811,6 +2811,15 @@ sb:load_balancer vips,protocol name=lbg0
>>   check ovn-nbctl lr-add lr0 -- add logical_router lr0 
>> load_balancer_group $lbg
>>   check ovn-nbctl --wait=sb lr-lb-add lr0 lb0
>> +check_row_count sb:load_balancer 2
>> +
>> +lr0_sb_uuid=$(fetch_column datapath_binding _uuid external_ids:name=lr0)
>> +lb0_lr_dp_group=$(fetch_column sb:load_balancer lr_datapath_group 
>> name=lb0)
>> +
>> +AT_CHECK_UNQUOTED([ovn-sbctl --bare --columns _uuid,datapaths find 
>> Logical_DP_Group dnl
>> +                    | grep -A1 $lb0_lr_dp_group | tail -1], [0], [dnl
>> +$lr0_sb_uuid
>> +])
>>   echo
>>   echo "__file__:__line__: Check that SB lb0 has only sw0 in datapaths 
>> column."
>> @@ -2856,7 +2865,13 @@ check_row_count sb:load_balancer 2
>>   lbg1=$(fetch_column nb:load_balancer _uuid name=lbg1)
>>   check ovn-nbctl add load_balancer_group $lbg load_balancer $lbg1
>>   check ovn-nbctl --wait=sb lr-lb-add lr0 lb1
>> -check_row_count sb:load_balancer 3
>> +check_row_count sb:load_balancer 4
>> +
>> +lb1_lr_dp_group=$(fetch_column sb:load_balancer lr_datapath_group 
>> name=lb1)
>> +AT_CHECK_UNQUOTED([ovn-sbctl --bare --columns _uuid,datapaths find 
>> Logical_DP_Group dnl
>> +                    | grep -A1 $lb1_lr_dp_group | tail -1], [0], [dnl
>> +$lr0_sb_uuid
>> +])
>>   echo
>>   echo "__file__:__line__: Associate lb1 to sw1 and check that lb1 is 
>> created in SB DB."
>> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
>> index 6669c18e7..08b380d10 100644
>> --- a/tests/system-ovn.at
>> +++ b/tests/system-ovn.at
>> @@ -11546,3 +11546,120 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query 
>> port patch-.*/d
>>   AT_CLEANUP
>>   ])
>> +
>> +OVN_FOR_EACH_NORTHD([
>> +AT_SETUP([ct_flush on logical router load balancer])
>> +AT_KEYWORDS([ct-lr-flush])
>> +CHECK_CONNTRACK()
>> +CHECK_CONNTRACK_NAT()
>> +ovn_start
>> +OVS_TRAFFIC_VSWITCHD_START()
>> +ADD_BR([br-int])
>> +#
>> +# Set external-ids in br-int needed for ovn-controller
>> +ovs-vsctl \
>> +        -- set Open_vSwitch . external-ids:system-id=hv1 \
>> +        -- set Open_vSwitch . 
>> external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
>> +        -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
>> +        -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
>> +        -- set bridge br-int fail-mode=secure 
>> other-config:disable-in-band=true
>> +
>> +start_daemon ovn-controller
>> +
>> +check ovn-nbctl lr-add R1
>> +
>> +check ovn-nbctl ls-add sw0
>> +check ovn-nbctl ls-add public
>> +
>> +check ovn-nbctl lrp-add R1 rp-sw0 00:00:01:01:02:03 192.168.1.1/24
>> +check ovn-nbctl lrp-add R1 rp-public 00:00:02:01:02:03 172.16.1.1/24
>> +
>> +check ovn-nbctl set logical_router R1 options:chassis=hv1
>> +
>> +check ovn-nbctl lsp-add sw0 sw0-rp -- set Logical_Switch_Port sw0-rp \
>> +    type=router options:router-port=rp-sw0 \
>> +    -- lsp-set-addresses sw0-rp router
>> +
>> +check ovn-nbctl lsp-add sw0 sw0-vm \
>> +    -- lsp-set-addresses sw0-vm "00:00:01:01:02:04 192.168.1.2/24"
>> +
>> +check ovn-nbctl lsp-add public public-rp -- set Logical_Switch_Port 
>> public-rp \
>> +    type=router options:router-port=rp-public \
>> +    -- lsp-set-addresses public-rp router
>> +
>> +check ovn-nbctl lsp-add public public-vm \
>> +   -- lsp-set-addresses public-vm "00:00:02:01:02:04 172.16.1.2/24"
>> +
>> +ADD_NAMESPACES(sw0-vm)
>> +ADD_VETH(sw0-vm, sw0-vm, br-int, "192.168.1.2/24", 
>> "00:00:01:01:02:04", \
>> +         "192.168.1.1")
>> +OVS_WAIT_UNTIL([test "$(ip netns exec sw0-vm ip a | grep fe80 | grep 
>> tentative)" = ""])
>> +
>> +ADD_NAMESPACES(public-vm)
>> +ADD_VETH(public-vm, public-vm, br-int, "172.16.1.2/24", 
>> "00:00:02:01:02:04", \
>> +         "172.16.1.1")
>> +
>> +OVS_WAIT_UNTIL([test "$(ip netns exec public-vm ip a | grep fe80 | 
>> grep tentative)" = ""])
>> +
>> +# Start webservers in 'server'.
>> +OVS_START_L7([sw0-vm], [http])
>> +
>> +# Create a load balancer and associate to R1
>> +check ovn-nbctl lb-add lb1 172.16.1.150:80 192.168.1.2:80 \
>> +    -- set load_balancer lb1 options:ct_flush="true"
>> +check ovn-nbctl lr-lb-add R1 lb1
>> +
>> +check ovn-nbctl --wait=hv sync
>> +
>> +for i in $(seq 1 5); do
>> +    echo Request $i
>> +    NS_CHECK_EXEC([public-vm], [wget 172.16.1.150 -t 5 -T 1 
>> --retry-connrefused -v -o wget$i.log])
>> +done
>> +
>> +OVS_WAIT_FOR_OUTPUT([ovs-appctl dpctl/dump-conntrack | 
>> FORMAT_CT(172.16.1.150) | wc -l ], [0], [dnl
>> +1
>> +])
>> +
>> +check ovn-nbctl lb-del lb1
>> +
>> +OVS_WAIT_FOR_OUTPUT([ovs-appctl dpctl/dump-conntrack | 
>> FORMAT_CT(172.16.1.150) | wc -l ], [0], [dnl
>> +0
>> +])
>> +
>> +check ovn-nbctl lb-add lb2 172.16.1.151:80 192.168.1.2:80
>> +check ovn-nbctl lr-lb-add R1 lb2
>> +
>> +check ovn-nbctl --wait=hv sync
>> +
>> +for i in $(seq 1 5); do
>> +    echo Request $i
>> +    NS_CHECK_EXEC([public-vm], [wget 172.16.1.151 -t 5 -T 1 
>> --retry-connrefused -v -o wget$i.log])
>> +done
>> +
>> +OVS_WAIT_FOR_OUTPUT([ovs-appctl dpctl/dump-conntrack | 
>> FORMAT_CT(172.16.1.151) | wc -l ], [0], [dnl
>> +1
>> +])
>> +
>> +check ovn-nbctl lb-del lb2
>> +
>> +OVS_WAIT_FOR_OUTPUT([ovs-appctl dpctl/dump-conntrack | 
>> FORMAT_CT(172.16.1.151) | wc -l ], [0], [dnl
>> +1
>> +])
> 
> The version of this test on the linked BZ expects 0 as the output when 
> dumping conntrack at this point. Deleting the load balancer should flush 
> the conntrack entry, so there should now be no conntrack entries for the 
> load balancer VIP.
> 
> Is there a reason why this was changed to 1 instead? The problem is that 
> the condition being checked before and after deleting the load balancer 
> is the same. It's not clear what is being tested here.
> 
> I think one of two changes needs to happen with this test:
> 
> 1) If the test is not actually exercising the expected 
> conntrack-flushing behavior, then the test needs to be adjusted to 
> properly test the conntrack flush.
> 
> 2) If the test is correct, and we actually do expect a conntrack entry 
> to still exist, then rather than counting the number of conntrack 
> entries, we need to inspect the conntrack entry to ensure that something 
> has actually happened as a result of deleting the load balancer.
> 

Ignore my feedback completely. I did not look closely enough at the 
test. You are deleting a load balancer that does not have ct_flush=true 
set on it in this scenario, so it is absolutely correct that the number 
of CT entries should be the same after deleting the load balancer.

>> +
>> +OVS_APP_EXIT_AND_WAIT([ovn-controller])
>> +
>> +as ovn-sb
>> +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
>> +
>> +as ovn-nb
>> +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
>> +
>> +as northd
>> +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE])
>> +
>> +as
>> +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
>> +/Failed to acquire.*/d
>> +/connection dropped.*/d"])
>> +AT_CLEANUP
>> +])
>> diff --git a/utilities/ovn-sbctl.c b/utilities/ovn-sbctl.c
>> index ddd9a9ca9..0e3afaea7 100644
>> --- a/utilities/ovn-sbctl.c
>> +++ b/utilities/ovn-sbctl.c
>> @@ -397,6 +397,7 @@ pre_get_info(struct ctl_context *ctx)
>>       ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_datapaths);
>>       ovsdb_idl_add_column(ctx->idl, 
>> &sbrec_load_balancer_col_ls_datapath_group);
>> +    ovsdb_idl_add_column(ctx->idl, 
>> &sbrec_load_balancer_col_lr_datapath_group);
>>       ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_vips);
>>       ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_name);
>>       ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_protocol);
>

> Hi Lorenzo, I have one note below
> 
> On 6/9/23 12:07, Lorenzo Bianconi wrote:
> > Introduce lr_datapath_group column in the load_balancer table of the SB
> > db.
> > Sync load_balancers applied to logical_routers to Load_Balancer table in
> > the SouthBound database.
> > 
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2193323
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> > ---
> >   controller/local_data.c     |   8 +++
> >   controller/ovn-controller.c |   6 ++
> >   lib/lb.h                    |   3 +-
> >   northd/northd.c             |  78 ++++++++++++++++++------
> >   ovn-sb.ovsschema            |   6 +-
> >   ovn-sb.xml                  |   6 ++
> >   tests/ovn-northd.at         |  17 +++++-
> >   tests/system-ovn.at         | 117 ++++++++++++++++++++++++++++++++++++
> >   utilities/ovn-sbctl.c       |   1 +
> >   9 files changed, 219 insertions(+), 23 deletions(-)
> > 
> > diff --git a/controller/local_data.c b/controller/local_data.c
> > index 2950434ac..c8aebcf50 100644
> > --- a/controller/local_data.c
> > +++ b/controller/local_data.c
> > @@ -670,5 +670,13 @@ lb_is_local(const struct sbrec_load_balancer *sbrec_lb,
> >           }
> >       }
> > +    dp_group = sbrec_lb->lr_datapath_group;
> > +    for (size_t i = 0; dp_group && i < dp_group->n_datapaths; i++) {
> > +        if (get_local_datapath(local_datapaths,
> > +                               dp_group->datapaths[i]->tunnel_key)) {
> > +            return true;
> > +        }
> > +    }
> > +
> >       return false;
> >   }
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index ab4896b91..93eaf6d9a 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -2705,6 +2705,12 @@ load_balancers_by_dp_init(const struct hmap *local_datapaths,
> >                                            lb->ls_datapath_group->datapaths[i],
> >                                            lb, lbs);
> >           }
> > +        for (size_t i = 0; lb->lr_datapath_group
> > +                           && i < lb->lr_datapath_group->n_datapaths; i++) {
> > +            load_balancers_by_dp_add_one(local_datapaths,
> > +                                         lb->lr_datapath_group->datapaths[i],
> > +                                         lb, lbs);
> > +        }
> >       }
> >       return lbs;
> >   }
> > diff --git a/lib/lb.h b/lib/lb.h
> > index 23d8fc9e9..2456423cc 100644
> > --- a/lib/lb.h
> > +++ b/lib/lb.h
> > @@ -85,7 +85,8 @@ struct ovn_northd_lb {
> >       size_t n_nb_lr;
> >       unsigned long *nb_lr_map;
> > -    struct ovn_dp_group *dpg;
> > +    struct ovn_dp_group *ls_dpg;
> > +    struct ovn_dp_group *lr_dpg;
> >   };
> >   struct ovn_lb_vip {
> > diff --git a/northd/northd.c b/northd/northd.c
> > index fad8ab0ec..9341823e3 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -4412,10 +4412,11 @@ ovn_dp_group_get_or_create(struct ovsdb_idl_txn *ovnsb_txn,
> >   static void
> >   sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
> >            const struct sbrec_load_balancer_table *sbrec_load_balancer_table,
> > -         struct ovn_datapaths *ls_datapaths, struct hmap *lbs)
> > +         struct ovn_datapaths *ls_datapaths,
> > +         struct ovn_datapaths *lr_datapaths, struct hmap *lbs)
> >   {
> > -    struct hmap dp_groups = HMAP_INITIALIZER(&dp_groups);
> > -    size_t bitmap_len = ods_size(ls_datapaths);
> > +    struct hmap ls_dp_groups = HMAP_INITIALIZER(&ls_dp_groups);
> > +    struct hmap lr_dp_groups = HMAP_INITIALIZER(&lr_dp_groups);
> >       struct ovn_northd_lb *lb;
> >       /* Delete any stale SB load balancer rows and create datapath
> > @@ -4440,7 +4441,8 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
> >            * are not indexed in any way.
> >            */
> >           lb = ovn_northd_lb_find(lbs, &lb_uuid);
> > -        if (!lb || !lb->n_nb_ls || !hmapx_add(&existing_lbs, lb)) {
> > +        if (!lb || (!lb->n_nb_ls && !lb->n_nb_lr) ||
> > +            !hmapx_add(&existing_lbs, lb)) {
> >               sbrec_load_balancer_delete(sbrec_lb);
> >               continue;
> >           }
> > @@ -4448,11 +4450,20 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
> >           lb->slb = sbrec_lb;
> >           /* Find or create datapath group for this load balancer. */
> > -        lb->dpg = ovn_dp_group_get_or_create(ovnsb_txn, &dp_groups,
> > -                                             lb->slb->ls_datapath_group,
> > -                                             lb->n_nb_ls, lb->nb_ls_map,
> > -                                             bitmap_len, true,
> > -                                             ls_datapaths, NULL);
> > +        if (lb->n_nb_ls) {
> > +            lb->ls_dpg = ovn_dp_group_get_or_create(ovnsb_txn, &ls_dp_groups,
> > +                                                    lb->slb->ls_datapath_group,
> > +                                                    lb->n_nb_ls, lb->nb_ls_map,
> > +                                                    ods_size(ls_datapaths),
> > +                                                    true, ls_datapaths, NULL);
> > +        }
> > +        if (lb->n_nb_lr) {
> > +            lb->lr_dpg = ovn_dp_group_get_or_create(ovnsb_txn, &lr_dp_groups,
> > +                                                    lb->slb->lr_datapath_group,
> > +                                                    lb->n_nb_lr, lb->nb_lr_map,
> > +                                                    ods_size(lr_datapaths),
> > +                                                    false, NULL, lr_datapaths);
> > +        }
> >       }
> >       hmapx_destroy(&existing_lbs);
> > @@ -4460,7 +4471,7 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
> >        * the SB load balancer columns. */
> >       HMAP_FOR_EACH (lb, hmap_node, lbs) {
> > -        if (!lb->n_nb_ls) {
> > +        if (!lb->n_nb_ls && !lb->n_nb_lr) {
> >               continue;
> >           }
> > @@ -4483,19 +4494,33 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
> >           }
> >           /* Find or create datapath group for this load balancer. */
> > -        if (!lb->dpg) {
> > -            lb->dpg = ovn_dp_group_get_or_create(ovnsb_txn, &dp_groups,
> > -                                                 lb->slb->ls_datapath_group,
> > -                                                 lb->n_nb_ls, lb->nb_ls_map,
> > -                                                 bitmap_len, true,
> > -                                                 ls_datapaths, NULL);
> > +        if (!lb->ls_dpg && lb->n_nb_ls) {
> > +            lb->ls_dpg = ovn_dp_group_get_or_create(ovnsb_txn, &ls_dp_groups,
> > +                                                    lb->slb->ls_datapath_group,
> > +                                                    lb->n_nb_ls, lb->nb_ls_map,
> > +                                                    ods_size(ls_datapaths),
> > +                                                    true, ls_datapaths, NULL);
> > +        }
> > +        if (!lb->lr_dpg && lb->n_nb_lr) {
> > +            lb->lr_dpg = ovn_dp_group_get_or_create(ovnsb_txn, &lr_dp_groups,
> > +                                                    lb->slb->lr_datapath_group,
> > +                                                    lb->n_nb_lr, lb->nb_lr_map,
> > +                                                    ods_size(lr_datapaths),
> > +                                                    false, NULL, lr_datapaths);
> >           }
> >           /* Update columns. */
> >           sbrec_load_balancer_set_name(lb->slb, lb->nlb->name);
> >           sbrec_load_balancer_set_vips(lb->slb, ovn_northd_lb_get_vips(lb));
> >           sbrec_load_balancer_set_protocol(lb->slb, lb->nlb->protocol);
> > -        sbrec_load_balancer_set_ls_datapath_group(lb->slb, lb->dpg->dp_group);
> > +        if (lb->ls_dpg) {
> > +            sbrec_load_balancer_set_ls_datapath_group(lb->slb,
> > +                                                      lb->ls_dpg->dp_group);
> > +        }
> > +        if (lb->lr_dpg) {
> > +            sbrec_load_balancer_set_lr_datapath_group(lb->slb,
> > +                                                      lb->lr_dpg->dp_group);
> > +        }
> >           sbrec_load_balancer_set_options(lb->slb, &options);
> >           /* Clearing 'datapaths' column, since 'dp_group' is in use. */
> >           sbrec_load_balancer_set_datapaths(lb->slb, NULL, 0);
> > @@ -4503,11 +4528,17 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
> >       }
> >       struct ovn_dp_group *dpg;
> > -    HMAP_FOR_EACH_POP (dpg, node, &dp_groups) {
> > +    HMAP_FOR_EACH_POP (dpg, node, &ls_dp_groups) {
> > +        bitmap_free(dpg->bitmap);
> > +        free(dpg);
> > +    }
> > +    hmap_destroy(&ls_dp_groups);
> > +
> > +    HMAP_FOR_EACH_POP (dpg, node, &lr_dp_groups) {
> >           bitmap_free(dpg->bitmap);
> >           free(dpg);
> >       }
> > -    hmap_destroy(&dp_groups);
> > +    hmap_destroy(&lr_dp_groups);
> >       /* Datapath_Binding.load_balancers is not used anymore, it's still in the
> >        * schema for compatibility reasons.  Reset it to empty, just in case.
> > @@ -4516,6 +4547,13 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
> >       HMAP_FOR_EACH (od, key_node, &ls_datapaths->datapaths) {
> >           ovs_assert(od->nbs);
> > +        if (od->sb->n_load_balancers) {
> > +            sbrec_datapath_binding_set_load_balancers(od->sb, NULL, 0);
> > +        }
> > +    }
> > +    HMAP_FOR_EACH (od, key_node, &lr_datapaths->datapaths) {
> > +        ovs_assert(od->nbr);
> > +
> >           if (od->sb->n_load_balancers) {
> >               sbrec_datapath_binding_set_load_balancers(od->sb, NULL, 0);
> >           }
> > @@ -16683,7 +16721,7 @@ ovnnb_db_run(struct northd_input *input_data,
> >       ovn_update_ipv6_prefix(&data->lr_ports);
> >       sync_lbs(ovnsb_txn, input_data->sbrec_load_balancer_table,
> > -             &data->ls_datapaths, &data->lbs);
> > +             &data->ls_datapaths, &data->lr_datapaths, &data->lbs);
> >       sync_port_groups(ovnsb_txn, input_data->sbrec_port_group_table,
> >                        &data->port_groups);
> >       sync_meters(ovnsb_txn, input_data->nbrec_meter_table,
> > diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
> > index e02fbd884..2a81d54ac 100644
> > --- a/ovn-sb.ovsschema
> > +++ b/ovn-sb.ovsschema
> > @@ -1,7 +1,7 @@
> >   {
> >       "name": "OVN_Southbound",
> >       "version": "20.27.2",
> > -    "cksum": "2970847454 30465",
> > +    "cksum": "1977000278 30679",
> >       "tables": {
> >           "SB_Global": {
> >               "columns": {
> > @@ -534,6 +534,10 @@
> >                       {"type": {"key": {"type": "uuid",
> >                                         "refTable": "Logical_DP_Group"},
> >                                 "min": 0, "max": 1}},
> > +                "lr_datapath_group":
> > +                    {"type": {"key": {"type": "uuid",
> > +                                      "refTable": "Logical_DP_Group"},
> > +                              "min": 0, "max": 1}},
> >                   "options": {
> >                        "type": {"key": "string",
> >                                 "value": "string",
> > diff --git a/ovn-sb.xml b/ovn-sb.xml
> > index 5d11d059b..de92a63f7 100644
> > --- a/ovn-sb.xml
> > +++ b/ovn-sb.xml
> > @@ -4885,6 +4885,12 @@ tcp.flags = RST;
> >         datapaths in a group.
> >       </column>
> > +    <column name="lr_datapath_group">
> > +      The group of logical router datapaths to which this load balancer
> > +      applies to. This means that the same load balancer applies to all
> > +      datapaths in a group.
> > +    </column>
> > +
> >       <group title="Load_Balancer options">
> >       <column name="options" key="hairpin_snat_ip">
> >         IP to be used as source IP for packets that have been hair-pinned after
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index 63caf8d66..0a7725c37 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -2811,6 +2811,15 @@ sb:load_balancer vips,protocol name=lbg0
> >   check ovn-nbctl lr-add lr0 -- add logical_router lr0 load_balancer_group $lbg
> >   check ovn-nbctl --wait=sb lr-lb-add lr0 lb0
> > +check_row_count sb:load_balancer 2
> > +
> > +lr0_sb_uuid=$(fetch_column datapath_binding _uuid external_ids:name=lr0)
> > +lb0_lr_dp_group=$(fetch_column sb:load_balancer lr_datapath_group name=lb0)
> > +
> > +AT_CHECK_UNQUOTED([ovn-sbctl --bare --columns _uuid,datapaths find Logical_DP_Group dnl
> > +                    | grep -A1 $lb0_lr_dp_group | tail -1], [0], [dnl
> > +$lr0_sb_uuid
> > +])
> >   echo
> >   echo "__file__:__line__: Check that SB lb0 has only sw0 in datapaths column."
> > @@ -2856,7 +2865,13 @@ check_row_count sb:load_balancer 2
> >   lbg1=$(fetch_column nb:load_balancer _uuid name=lbg1)
> >   check ovn-nbctl add load_balancer_group $lbg load_balancer $lbg1
> >   check ovn-nbctl --wait=sb lr-lb-add lr0 lb1
> > -check_row_count sb:load_balancer 3
> > +check_row_count sb:load_balancer 4
> > +
> > +lb1_lr_dp_group=$(fetch_column sb:load_balancer lr_datapath_group name=lb1)
> > +AT_CHECK_UNQUOTED([ovn-sbctl --bare --columns _uuid,datapaths find Logical_DP_Group dnl
> > +                    | grep -A1 $lb1_lr_dp_group | tail -1], [0], [dnl
> > +$lr0_sb_uuid
> > +])
> >   echo
> >   echo "__file__:__line__: Associate lb1 to sw1 and check that lb1 is created in SB DB."
> > diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> > index 6669c18e7..08b380d10 100644
> > --- a/tests/system-ovn.at
> > +++ b/tests/system-ovn.at
> > @@ -11546,3 +11546,120 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
> >   AT_CLEANUP
> >   ])
> > +
> > +OVN_FOR_EACH_NORTHD([
> > +AT_SETUP([ct_flush on logical router load balancer])
> > +AT_KEYWORDS([ct-lr-flush])
> > +CHECK_CONNTRACK()
> > +CHECK_CONNTRACK_NAT()
> > +ovn_start
> > +OVS_TRAFFIC_VSWITCHD_START()
> > +ADD_BR([br-int])
> > +#
> > +# Set external-ids in br-int needed for ovn-controller
> > +ovs-vsctl \
> > +        -- set Open_vSwitch . external-ids:system-id=hv1 \
> > +        -- set Open_vSwitch . external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
> > +        -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
> > +        -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
> > +        -- set bridge br-int fail-mode=secure other-config:disable-in-band=true
> > +
> > +start_daemon ovn-controller
> > +
> > +check ovn-nbctl lr-add R1
> > +
> > +check ovn-nbctl ls-add sw0
> > +check ovn-nbctl ls-add public
> > +
> > +check ovn-nbctl lrp-add R1 rp-sw0 00:00:01:01:02:03 192.168.1.1/24
> > +check ovn-nbctl lrp-add R1 rp-public 00:00:02:01:02:03 172.16.1.1/24
> > +
> > +check ovn-nbctl set logical_router R1 options:chassis=hv1
> > +
> > +check ovn-nbctl lsp-add sw0 sw0-rp -- set Logical_Switch_Port sw0-rp \
> > +    type=router options:router-port=rp-sw0 \
> > +    -- lsp-set-addresses sw0-rp router
> > +
> > +check ovn-nbctl lsp-add sw0 sw0-vm \
> > +    -- lsp-set-addresses sw0-vm "00:00:01:01:02:04 192.168.1.2/24"
> > +
> > +check ovn-nbctl lsp-add public public-rp -- set Logical_Switch_Port public-rp \
> > +    type=router options:router-port=rp-public \
> > +    -- lsp-set-addresses public-rp router
> > +
> > +check ovn-nbctl lsp-add public public-vm \
> > +   -- lsp-set-addresses public-vm "00:00:02:01:02:04 172.16.1.2/24"
> > +
> > +ADD_NAMESPACES(sw0-vm)
> > +ADD_VETH(sw0-vm, sw0-vm, br-int, "192.168.1.2/24", "00:00:01:01:02:04", \
> > +         "192.168.1.1")
> > +OVS_WAIT_UNTIL([test "$(ip netns exec sw0-vm ip a | grep fe80 | grep tentative)" = ""])
> > +
> > +ADD_NAMESPACES(public-vm)
> > +ADD_VETH(public-vm, public-vm, br-int, "172.16.1.2/24", "00:00:02:01:02:04", \
> > +         "172.16.1.1")
> > +
> > +OVS_WAIT_UNTIL([test "$(ip netns exec public-vm ip a | grep fe80 | grep tentative)" = ""])
> > +
> > +# Start webservers in 'server'.
> > +OVS_START_L7([sw0-vm], [http])
> > +
> > +# Create a load balancer and associate to R1
> > +check ovn-nbctl lb-add lb1 172.16.1.150:80 192.168.1.2:80 \
> > +    -- set load_balancer lb1 options:ct_flush="true"
> > +check ovn-nbctl lr-lb-add R1 lb1
> > +
> > +check ovn-nbctl --wait=hv sync
> > +
> > +for i in $(seq 1 5); do
> > +    echo Request $i
> > +    NS_CHECK_EXEC([public-vm], [wget 172.16.1.150 -t 5 -T 1 --retry-connrefused -v -o wget$i.log])
> > +done
> > +
> > +OVS_WAIT_FOR_OUTPUT([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.150) | wc -l ], [0], [dnl
> > +1
> > +])
> > +
> > +check ovn-nbctl lb-del lb1
> > +
> > +OVS_WAIT_FOR_OUTPUT([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.150) | wc -l ], [0], [dnl
> > +0
> > +])
> > +
> > +check ovn-nbctl lb-add lb2 172.16.1.151:80 192.168.1.2:80
> > +check ovn-nbctl lr-lb-add R1 lb2
> > +
> > +check ovn-nbctl --wait=hv sync
> > +
> > +for i in $(seq 1 5); do
> > +    echo Request $i
> > +    NS_CHECK_EXEC([public-vm], [wget 172.16.1.151 -t 5 -T 1 --retry-connrefused -v -o wget$i.log])
> > +done
> > +
> > +OVS_WAIT_FOR_OUTPUT([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.151) | wc -l ], [0], [dnl
> > +1
> > +])
> > +
> > +check ovn-nbctl lb-del lb2
> > +
> > +OVS_WAIT_FOR_OUTPUT([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.151) | wc -l ], [0], [dnl
> > +1
> > +])
> 
> The version of this test on the linked BZ expects 0 as the output when
> dumping conntrack at this point. Deleting the load balancer should flush the
> conntrack entry, so there should now be no conntrack entries for the load
> balancer VIP.
> 
> Is there a reason why this was changed to 1 instead? The problem is that the
> condition being checked before and after deleting the load balancer is the
> same. It's not clear what is being tested here.

Hi Mark,

please note this test refers to a different load balancer (lb2) where I have
not set ct_flush to "true". Am I missing something?

Regards,
Lorenzo

> 
> I think one of two changes needs to happen with this test:
> 
> 1) If the test is not actually exercising the expected conntrack-flushing
> behavior, then the test needs to be adjusted to properly test the conntrack
> flush.
> 
> 2) If the test is correct, and we actually do expect a conntrack entry to
> still exist, then rather than counting the number of conntrack entries, we
> need to inspect the conntrack entry to ensure that something has actually
> happened as a result of deleting the load balancer.
> 
> > +
> > +OVS_APP_EXIT_AND_WAIT([ovn-controller])
> > +
> > +as ovn-sb
> > +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> > +
> > +as ovn-nb
> > +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> > +
> > +as northd
> > +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE])
> > +
> > +as
> > +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
> > +/Failed to acquire.*/d
> > +/connection dropped.*/d"])
> > +AT_CLEANUP
> > +])
> > diff --git a/utilities/ovn-sbctl.c b/utilities/ovn-sbctl.c
> > index ddd9a9ca9..0e3afaea7 100644
> > --- a/utilities/ovn-sbctl.c
> > +++ b/utilities/ovn-sbctl.c
> > @@ -397,6 +397,7 @@ pre_get_info(struct ctl_context *ctx)
> >       ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_datapaths);
> >       ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_ls_datapath_group);
> > +    ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_lr_datapath_group);
> >       ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_vips);
> >       ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_name);
> >       ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_protocol);
>

On 6/9/23 13:10, Lorenzo Bianconi wrote:
>> Hi Lorenzo, I have one note below
>>
>> On 6/9/23 12:07, Lorenzo Bianconi wrote:
>>> Introduce lr_datapath_group column in the load_balancer table of the SB
>>> db.
>>> Sync load_balancers applied to logical_routers to Load_Balancer table in
>>> the SouthBound database.
>>>
>>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2193323
>>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
>>> ---
>>>    controller/local_data.c     |   8 +++
>>>    controller/ovn-controller.c |   6 ++
>>>    lib/lb.h                    |   3 +-
>>>    northd/northd.c             |  78 ++++++++++++++++++------
>>>    ovn-sb.ovsschema            |   6 +-
>>>    ovn-sb.xml                  |   6 ++
>>>    tests/ovn-northd.at         |  17 +++++-
>>>    tests/system-ovn.at         | 117 ++++++++++++++++++++++++++++++++++++
>>>    utilities/ovn-sbctl.c       |   1 +
>>>    9 files changed, 219 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/controller/local_data.c b/controller/local_data.c
>>> index 2950434ac..c8aebcf50 100644
>>> --- a/controller/local_data.c
>>> +++ b/controller/local_data.c
>>> @@ -670,5 +670,13 @@ lb_is_local(const struct sbrec_load_balancer *sbrec_lb,
>>>            }
>>>        }
>>> +    dp_group = sbrec_lb->lr_datapath_group;
>>> +    for (size_t i = 0; dp_group && i < dp_group->n_datapaths; i++) {
>>> +        if (get_local_datapath(local_datapaths,
>>> +                               dp_group->datapaths[i]->tunnel_key)) {
>>> +            return true;
>>> +        }
>>> +    }
>>> +
>>>        return false;
>>>    }
>>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
>>> index ab4896b91..93eaf6d9a 100644
>>> --- a/controller/ovn-controller.c
>>> +++ b/controller/ovn-controller.c
>>> @@ -2705,6 +2705,12 @@ load_balancers_by_dp_init(const struct hmap *local_datapaths,
>>>                                             lb->ls_datapath_group->datapaths[i],
>>>                                             lb, lbs);
>>>            }
>>> +        for (size_t i = 0; lb->lr_datapath_group
>>> +                           && i < lb->lr_datapath_group->n_datapaths; i++) {
>>> +            load_balancers_by_dp_add_one(local_datapaths,
>>> +                                         lb->lr_datapath_group->datapaths[i],
>>> +                                         lb, lbs);
>>> +        }
>>>        }
>>>        return lbs;
>>>    }
>>> diff --git a/lib/lb.h b/lib/lb.h
>>> index 23d8fc9e9..2456423cc 100644
>>> --- a/lib/lb.h
>>> +++ b/lib/lb.h
>>> @@ -85,7 +85,8 @@ struct ovn_northd_lb {
>>>        size_t n_nb_lr;
>>>        unsigned long *nb_lr_map;
>>> -    struct ovn_dp_group *dpg;
>>> +    struct ovn_dp_group *ls_dpg;
>>> +    struct ovn_dp_group *lr_dpg;
>>>    };
>>>    struct ovn_lb_vip {
>>> diff --git a/northd/northd.c b/northd/northd.c
>>> index fad8ab0ec..9341823e3 100644
>>> --- a/northd/northd.c
>>> +++ b/northd/northd.c
>>> @@ -4412,10 +4412,11 @@ ovn_dp_group_get_or_create(struct ovsdb_idl_txn *ovnsb_txn,
>>>    static void
>>>    sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
>>>             const struct sbrec_load_balancer_table *sbrec_load_balancer_table,
>>> -         struct ovn_datapaths *ls_datapaths, struct hmap *lbs)
>>> +         struct ovn_datapaths *ls_datapaths,
>>> +         struct ovn_datapaths *lr_datapaths, struct hmap *lbs)
>>>    {
>>> -    struct hmap dp_groups = HMAP_INITIALIZER(&dp_groups);
>>> -    size_t bitmap_len = ods_size(ls_datapaths);
>>> +    struct hmap ls_dp_groups = HMAP_INITIALIZER(&ls_dp_groups);
>>> +    struct hmap lr_dp_groups = HMAP_INITIALIZER(&lr_dp_groups);
>>>        struct ovn_northd_lb *lb;
>>>        /* Delete any stale SB load balancer rows and create datapath
>>> @@ -4440,7 +4441,8 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
>>>             * are not indexed in any way.
>>>             */
>>>            lb = ovn_northd_lb_find(lbs, &lb_uuid);
>>> -        if (!lb || !lb->n_nb_ls || !hmapx_add(&existing_lbs, lb)) {
>>> +        if (!lb || (!lb->n_nb_ls && !lb->n_nb_lr) ||
>>> +            !hmapx_add(&existing_lbs, lb)) {
>>>                sbrec_load_balancer_delete(sbrec_lb);
>>>                continue;
>>>            }
>>> @@ -4448,11 +4450,20 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
>>>            lb->slb = sbrec_lb;
>>>            /* Find or create datapath group for this load balancer. */
>>> -        lb->dpg = ovn_dp_group_get_or_create(ovnsb_txn, &dp_groups,
>>> -                                             lb->slb->ls_datapath_group,
>>> -                                             lb->n_nb_ls, lb->nb_ls_map,
>>> -                                             bitmap_len, true,
>>> -                                             ls_datapaths, NULL);
>>> +        if (lb->n_nb_ls) {
>>> +            lb->ls_dpg = ovn_dp_group_get_or_create(ovnsb_txn, &ls_dp_groups,
>>> +                                                    lb->slb->ls_datapath_group,
>>> +                                                    lb->n_nb_ls, lb->nb_ls_map,
>>> +                                                    ods_size(ls_datapaths),
>>> +                                                    true, ls_datapaths, NULL);
>>> +        }
>>> +        if (lb->n_nb_lr) {
>>> +            lb->lr_dpg = ovn_dp_group_get_or_create(ovnsb_txn, &lr_dp_groups,
>>> +                                                    lb->slb->lr_datapath_group,
>>> +                                                    lb->n_nb_lr, lb->nb_lr_map,
>>> +                                                    ods_size(lr_datapaths),
>>> +                                                    false, NULL, lr_datapaths);
>>> +        }
>>>        }
>>>        hmapx_destroy(&existing_lbs);
>>> @@ -4460,7 +4471,7 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
>>>         * the SB load balancer columns. */
>>>        HMAP_FOR_EACH (lb, hmap_node, lbs) {
>>> -        if (!lb->n_nb_ls) {
>>> +        if (!lb->n_nb_ls && !lb->n_nb_lr) {
>>>                continue;
>>>            }
>>> @@ -4483,19 +4494,33 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
>>>            }
>>>            /* Find or create datapath group for this load balancer. */
>>> -        if (!lb->dpg) {
>>> -            lb->dpg = ovn_dp_group_get_or_create(ovnsb_txn, &dp_groups,
>>> -                                                 lb->slb->ls_datapath_group,
>>> -                                                 lb->n_nb_ls, lb->nb_ls_map,
>>> -                                                 bitmap_len, true,
>>> -                                                 ls_datapaths, NULL);
>>> +        if (!lb->ls_dpg && lb->n_nb_ls) {
>>> +            lb->ls_dpg = ovn_dp_group_get_or_create(ovnsb_txn, &ls_dp_groups,
>>> +                                                    lb->slb->ls_datapath_group,
>>> +                                                    lb->n_nb_ls, lb->nb_ls_map,
>>> +                                                    ods_size(ls_datapaths),
>>> +                                                    true, ls_datapaths, NULL);
>>> +        }
>>> +        if (!lb->lr_dpg && lb->n_nb_lr) {
>>> +            lb->lr_dpg = ovn_dp_group_get_or_create(ovnsb_txn, &lr_dp_groups,
>>> +                                                    lb->slb->lr_datapath_group,
>>> +                                                    lb->n_nb_lr, lb->nb_lr_map,
>>> +                                                    ods_size(lr_datapaths),
>>> +                                                    false, NULL, lr_datapaths);
>>>            }
>>>            /* Update columns. */
>>>            sbrec_load_balancer_set_name(lb->slb, lb->nlb->name);
>>>            sbrec_load_balancer_set_vips(lb->slb, ovn_northd_lb_get_vips(lb));
>>>            sbrec_load_balancer_set_protocol(lb->slb, lb->nlb->protocol);
>>> -        sbrec_load_balancer_set_ls_datapath_group(lb->slb, lb->dpg->dp_group);
>>> +        if (lb->ls_dpg) {
>>> +            sbrec_load_balancer_set_ls_datapath_group(lb->slb,
>>> +                                                      lb->ls_dpg->dp_group);
>>> +        }
>>> +        if (lb->lr_dpg) {
>>> +            sbrec_load_balancer_set_lr_datapath_group(lb->slb,
>>> +                                                      lb->lr_dpg->dp_group);
>>> +        }
>>>            sbrec_load_balancer_set_options(lb->slb, &options);
>>>            /* Clearing 'datapaths' column, since 'dp_group' is in use. */
>>>            sbrec_load_balancer_set_datapaths(lb->slb, NULL, 0);
>>> @@ -4503,11 +4528,17 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
>>>        }
>>>        struct ovn_dp_group *dpg;
>>> -    HMAP_FOR_EACH_POP (dpg, node, &dp_groups) {
>>> +    HMAP_FOR_EACH_POP (dpg, node, &ls_dp_groups) {
>>> +        bitmap_free(dpg->bitmap);
>>> +        free(dpg);
>>> +    }
>>> +    hmap_destroy(&ls_dp_groups);
>>> +
>>> +    HMAP_FOR_EACH_POP (dpg, node, &lr_dp_groups) {
>>>            bitmap_free(dpg->bitmap);
>>>            free(dpg);
>>>        }
>>> -    hmap_destroy(&dp_groups);
>>> +    hmap_destroy(&lr_dp_groups);
>>>        /* Datapath_Binding.load_balancers is not used anymore, it's still in the
>>>         * schema for compatibility reasons.  Reset it to empty, just in case.
>>> @@ -4516,6 +4547,13 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
>>>        HMAP_FOR_EACH (od, key_node, &ls_datapaths->datapaths) {
>>>            ovs_assert(od->nbs);
>>> +        if (od->sb->n_load_balancers) {
>>> +            sbrec_datapath_binding_set_load_balancers(od->sb, NULL, 0);
>>> +        }
>>> +    }
>>> +    HMAP_FOR_EACH (od, key_node, &lr_datapaths->datapaths) {
>>> +        ovs_assert(od->nbr);
>>> +
>>>            if (od->sb->n_load_balancers) {
>>>                sbrec_datapath_binding_set_load_balancers(od->sb, NULL, 0);
>>>            }
>>> @@ -16683,7 +16721,7 @@ ovnnb_db_run(struct northd_input *input_data,
>>>        ovn_update_ipv6_prefix(&data->lr_ports);
>>>        sync_lbs(ovnsb_txn, input_data->sbrec_load_balancer_table,
>>> -             &data->ls_datapaths, &data->lbs);
>>> +             &data->ls_datapaths, &data->lr_datapaths, &data->lbs);
>>>        sync_port_groups(ovnsb_txn, input_data->sbrec_port_group_table,
>>>                         &data->port_groups);
>>>        sync_meters(ovnsb_txn, input_data->nbrec_meter_table,
>>> diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
>>> index e02fbd884..2a81d54ac 100644
>>> --- a/ovn-sb.ovsschema
>>> +++ b/ovn-sb.ovsschema
>>> @@ -1,7 +1,7 @@
>>>    {
>>>        "name": "OVN_Southbound",
>>>        "version": "20.27.2",
>>> -    "cksum": "2970847454 30465",
>>> +    "cksum": "1977000278 30679",
>>>        "tables": {
>>>            "SB_Global": {
>>>                "columns": {
>>> @@ -534,6 +534,10 @@
>>>                        {"type": {"key": {"type": "uuid",
>>>                                          "refTable": "Logical_DP_Group"},
>>>                                  "min": 0, "max": 1}},
>>> +                "lr_datapath_group":
>>> +                    {"type": {"key": {"type": "uuid",
>>> +                                      "refTable": "Logical_DP_Group"},
>>> +                              "min": 0, "max": 1}},
>>>                    "options": {
>>>                         "type": {"key": "string",
>>>                                  "value": "string",
>>> diff --git a/ovn-sb.xml b/ovn-sb.xml
>>> index 5d11d059b..de92a63f7 100644
>>> --- a/ovn-sb.xml
>>> +++ b/ovn-sb.xml
>>> @@ -4885,6 +4885,12 @@ tcp.flags = RST;
>>>          datapaths in a group.
>>>        </column>
>>> +    <column name="lr_datapath_group">
>>> +      The group of logical router datapaths to which this load balancer
>>> +      applies to. This means that the same load balancer applies to all
>>> +      datapaths in a group.
>>> +    </column>
>>> +
>>>        <group title="Load_Balancer options">
>>>        <column name="options" key="hairpin_snat_ip">
>>>          IP to be used as source IP for packets that have been hair-pinned after
>>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
>>> index 63caf8d66..0a7725c37 100644
>>> --- a/tests/ovn-northd.at
>>> +++ b/tests/ovn-northd.at
>>> @@ -2811,6 +2811,15 @@ sb:load_balancer vips,protocol name=lbg0
>>>    check ovn-nbctl lr-add lr0 -- add logical_router lr0 load_balancer_group $lbg
>>>    check ovn-nbctl --wait=sb lr-lb-add lr0 lb0
>>> +check_row_count sb:load_balancer 2
>>> +
>>> +lr0_sb_uuid=$(fetch_column datapath_binding _uuid external_ids:name=lr0)
>>> +lb0_lr_dp_group=$(fetch_column sb:load_balancer lr_datapath_group name=lb0)
>>> +
>>> +AT_CHECK_UNQUOTED([ovn-sbctl --bare --columns _uuid,datapaths find Logical_DP_Group dnl
>>> +                    | grep -A1 $lb0_lr_dp_group | tail -1], [0], [dnl
>>> +$lr0_sb_uuid
>>> +])
>>>    echo
>>>    echo "__file__:__line__: Check that SB lb0 has only sw0 in datapaths column."
>>> @@ -2856,7 +2865,13 @@ check_row_count sb:load_balancer 2
>>>    lbg1=$(fetch_column nb:load_balancer _uuid name=lbg1)
>>>    check ovn-nbctl add load_balancer_group $lbg load_balancer $lbg1
>>>    check ovn-nbctl --wait=sb lr-lb-add lr0 lb1
>>> -check_row_count sb:load_balancer 3
>>> +check_row_count sb:load_balancer 4
>>> +
>>> +lb1_lr_dp_group=$(fetch_column sb:load_balancer lr_datapath_group name=lb1)
>>> +AT_CHECK_UNQUOTED([ovn-sbctl --bare --columns _uuid,datapaths find Logical_DP_Group dnl
>>> +                    | grep -A1 $lb1_lr_dp_group | tail -1], [0], [dnl
>>> +$lr0_sb_uuid
>>> +])
>>>    echo
>>>    echo "__file__:__line__: Associate lb1 to sw1 and check that lb1 is created in SB DB."
>>> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
>>> index 6669c18e7..08b380d10 100644
>>> --- a/tests/system-ovn.at
>>> +++ b/tests/system-ovn.at
>>> @@ -11546,3 +11546,120 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
>>>    AT_CLEANUP
>>>    ])
>>> +
>>> +OVN_FOR_EACH_NORTHD([
>>> +AT_SETUP([ct_flush on logical router load balancer])
>>> +AT_KEYWORDS([ct-lr-flush])
>>> +CHECK_CONNTRACK()
>>> +CHECK_CONNTRACK_NAT()
>>> +ovn_start
>>> +OVS_TRAFFIC_VSWITCHD_START()
>>> +ADD_BR([br-int])
>>> +#
>>> +# Set external-ids in br-int needed for ovn-controller
>>> +ovs-vsctl \
>>> +        -- set Open_vSwitch . external-ids:system-id=hv1 \
>>> +        -- set Open_vSwitch . external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
>>> +        -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
>>> +        -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
>>> +        -- set bridge br-int fail-mode=secure other-config:disable-in-band=true
>>> +
>>> +start_daemon ovn-controller
>>> +
>>> +check ovn-nbctl lr-add R1
>>> +
>>> +check ovn-nbctl ls-add sw0
>>> +check ovn-nbctl ls-add public
>>> +
>>> +check ovn-nbctl lrp-add R1 rp-sw0 00:00:01:01:02:03 192.168.1.1/24
>>> +check ovn-nbctl lrp-add R1 rp-public 00:00:02:01:02:03 172.16.1.1/24
>>> +
>>> +check ovn-nbctl set logical_router R1 options:chassis=hv1
>>> +
>>> +check ovn-nbctl lsp-add sw0 sw0-rp -- set Logical_Switch_Port sw0-rp \
>>> +    type=router options:router-port=rp-sw0 \
>>> +    -- lsp-set-addresses sw0-rp router
>>> +
>>> +check ovn-nbctl lsp-add sw0 sw0-vm \
>>> +    -- lsp-set-addresses sw0-vm "00:00:01:01:02:04 192.168.1.2/24"
>>> +
>>> +check ovn-nbctl lsp-add public public-rp -- set Logical_Switch_Port public-rp \
>>> +    type=router options:router-port=rp-public \
>>> +    -- lsp-set-addresses public-rp router
>>> +
>>> +check ovn-nbctl lsp-add public public-vm \
>>> +   -- lsp-set-addresses public-vm "00:00:02:01:02:04 172.16.1.2/24"
>>> +
>>> +ADD_NAMESPACES(sw0-vm)
>>> +ADD_VETH(sw0-vm, sw0-vm, br-int, "192.168.1.2/24", "00:00:01:01:02:04", \
>>> +         "192.168.1.1")
>>> +OVS_WAIT_UNTIL([test "$(ip netns exec sw0-vm ip a | grep fe80 | grep tentative)" = ""])
>>> +
>>> +ADD_NAMESPACES(public-vm)
>>> +ADD_VETH(public-vm, public-vm, br-int, "172.16.1.2/24", "00:00:02:01:02:04", \
>>> +         "172.16.1.1")
>>> +
>>> +OVS_WAIT_UNTIL([test "$(ip netns exec public-vm ip a | grep fe80 | grep tentative)" = ""])
>>> +
>>> +# Start webservers in 'server'.
>>> +OVS_START_L7([sw0-vm], [http])
>>> +
>>> +# Create a load balancer and associate to R1
>>> +check ovn-nbctl lb-add lb1 172.16.1.150:80 192.168.1.2:80 \
>>> +    -- set load_balancer lb1 options:ct_flush="true"
>>> +check ovn-nbctl lr-lb-add R1 lb1
>>> +
>>> +check ovn-nbctl --wait=hv sync
>>> +
>>> +for i in $(seq 1 5); do
>>> +    echo Request $i
>>> +    NS_CHECK_EXEC([public-vm], [wget 172.16.1.150 -t 5 -T 1 --retry-connrefused -v -o wget$i.log])
>>> +done
>>> +
>>> +OVS_WAIT_FOR_OUTPUT([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.150) | wc -l ], [0], [dnl
>>> +1
>>> +])
>>> +
>>> +check ovn-nbctl lb-del lb1
>>> +
>>> +OVS_WAIT_FOR_OUTPUT([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.150) | wc -l ], [0], [dnl
>>> +0
>>> +])
>>> +
>>> +check ovn-nbctl lb-add lb2 172.16.1.151:80 192.168.1.2:80
>>> +check ovn-nbctl lr-lb-add R1 lb2
>>> +
>>> +check ovn-nbctl --wait=hv sync
>>> +
>>> +for i in $(seq 1 5); do
>>> +    echo Request $i
>>> +    NS_CHECK_EXEC([public-vm], [wget 172.16.1.151 -t 5 -T 1 --retry-connrefused -v -o wget$i.log])
>>> +done
>>> +
>>> +OVS_WAIT_FOR_OUTPUT([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.151) | wc -l ], [0], [dnl
>>> +1
>>> +])
>>> +
>>> +check ovn-nbctl lb-del lb2
>>> +
>>> +OVS_WAIT_FOR_OUTPUT([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.151) | wc -l ], [0], [dnl
>>> +1
>>> +])
>>
>> The version of this test on the linked BZ expects 0 as the output when
>> dumping conntrack at this point. Deleting the load balancer should flush the
>> conntrack entry, so there should now be no conntrack entries for the load
>> balancer VIP.
>>
>> Is there a reason why this was changed to 1 instead? The problem is that the
>> condition being checked before and after deleting the load balancer is the
>> same. It's not clear what is being tested here.
> 
> Hi Mark,
> 
> please note this test refers to a different load balancer (lb2) where I have
> not set ct_flush to "true". Am I missing something?
> 
> Regards,
> Lorenzo

Yes I noticed this about a minute after I sent my reply and sent a 
follow-up saying to ignore my feedback. Sorry for the noise :)

> 
>>
>> I think one of two changes needs to happen with this test:
>>
>> 1) If the test is not actually exercising the expected conntrack-flushing
>> behavior, then the test needs to be adjusted to properly test the conntrack
>> flush.
>>
>> 2) If the test is correct, and we actually do expect a conntrack entry to
>> still exist, then rather than counting the number of conntrack entries, we
>> need to inspect the conntrack entry to ensure that something has actually
>> happened as a result of deleting the load balancer.
>>
>>> +
>>> +OVS_APP_EXIT_AND_WAIT([ovn-controller])
>>> +
>>> +as ovn-sb
>>> +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
>>> +
>>> +as ovn-nb
>>> +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
>>> +
>>> +as northd
>>> +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE])
>>> +
>>> +as
>>> +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
>>> +/Failed to acquire.*/d
>>> +/connection dropped.*/d"])
>>> +AT_CLEANUP
>>> +])
>>> diff --git a/utilities/ovn-sbctl.c b/utilities/ovn-sbctl.c
>>> index ddd9a9ca9..0e3afaea7 100644
>>> --- a/utilities/ovn-sbctl.c
>>> +++ b/utilities/ovn-sbctl.c
>>> @@ -397,6 +397,7 @@ pre_get_info(struct ctl_context *ctx)
>>>        ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_datapaths);
>>>        ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_ls_datapath_group);
>>> +    ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_lr_datapath_group);
>>>        ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_vips);
>>>        ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_name);
>>>        ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_protocol);
>>

On Jun 09, Mark Michelson wrote:
> On 6/9/23 13:10, Lorenzo Bianconi wrote:
> > > Hi Lorenzo, I have one note below
> > > 
> > > On 6/9/23 12:07, Lorenzo Bianconi wrote:
> > > > Introduce lr_datapath_group column in the load_balancer table of the SB
> > > > db.
> > > > Sync load_balancers applied to logical_routers to Load_Balancer table in
> > > > the SouthBound database.
> > > > 
> > > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2193323
> > > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> > > > ---
> > > >    controller/local_data.c     |   8 +++
> > > >    controller/ovn-controller.c |   6 ++
> > > >    lib/lb.h                    |   3 +-
> > > >    northd/northd.c             |  78 ++++++++++++++++++------
> > > >    ovn-sb.ovsschema            |   6 +-
> > > >    ovn-sb.xml                  |   6 ++
> > > >    tests/ovn-northd.at         |  17 +++++-
> > > >    tests/system-ovn.at         | 117 ++++++++++++++++++++++++++++++++++++
> > > >    utilities/ovn-sbctl.c       |   1 +
> > > >    9 files changed, 219 insertions(+), 23 deletions(-)
> > > > 
> > > > diff --git a/controller/local_data.c b/controller/local_data.c
> > > > index 2950434ac..c8aebcf50 100644
> > > > --- a/controller/local_data.c
> > > > +++ b/controller/local_data.c
> > > > @@ -670,5 +670,13 @@ lb_is_local(const struct sbrec_load_balancer *sbrec_lb,
> > > >            }
> > > >        }
> > > > +    dp_group = sbrec_lb->lr_datapath_group;
> > > > +    for (size_t i = 0; dp_group && i < dp_group->n_datapaths; i++) {
> > > > +        if (get_local_datapath(local_datapaths,
> > > > +                               dp_group->datapaths[i]->tunnel_key)) {
> > > > +            return true;
> > > > +        }
> > > > +    }
> > > > +
> > > >        return false;
> > > >    }
> > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > > > index ab4896b91..93eaf6d9a 100644
> > > > --- a/controller/ovn-controller.c
> > > > +++ b/controller/ovn-controller.c
> > > > @@ -2705,6 +2705,12 @@ load_balancers_by_dp_init(const struct hmap *local_datapaths,
> > > >                                             lb->ls_datapath_group->datapaths[i],
> > > >                                             lb, lbs);
> > > >            }
> > > > +        for (size_t i = 0; lb->lr_datapath_group
> > > > +                           && i < lb->lr_datapath_group->n_datapaths; i++) {
> > > > +            load_balancers_by_dp_add_one(local_datapaths,
> > > > +                                         lb->lr_datapath_group->datapaths[i],
> > > > +                                         lb, lbs);
> > > > +        }
> > > >        }
> > > >        return lbs;
> > > >    }
> > > > diff --git a/lib/lb.h b/lib/lb.h
> > > > index 23d8fc9e9..2456423cc 100644
> > > > --- a/lib/lb.h
> > > > +++ b/lib/lb.h
> > > > @@ -85,7 +85,8 @@ struct ovn_northd_lb {
> > > >        size_t n_nb_lr;
> > > >        unsigned long *nb_lr_map;
> > > > -    struct ovn_dp_group *dpg;
> > > > +    struct ovn_dp_group *ls_dpg;
> > > > +    struct ovn_dp_group *lr_dpg;
> > > >    };
> > > >    struct ovn_lb_vip {
> > > > diff --git a/northd/northd.c b/northd/northd.c
> > > > index fad8ab0ec..9341823e3 100644
> > > > --- a/northd/northd.c
> > > > +++ b/northd/northd.c
> > > > @@ -4412,10 +4412,11 @@ ovn_dp_group_get_or_create(struct ovsdb_idl_txn *ovnsb_txn,
> > > >    static void
> > > >    sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
> > > >             const struct sbrec_load_balancer_table *sbrec_load_balancer_table,
> > > > -         struct ovn_datapaths *ls_datapaths, struct hmap *lbs)
> > > > +         struct ovn_datapaths *ls_datapaths,
> > > > +         struct ovn_datapaths *lr_datapaths, struct hmap *lbs)
> > > >    {
> > > > -    struct hmap dp_groups = HMAP_INITIALIZER(&dp_groups);
> > > > -    size_t bitmap_len = ods_size(ls_datapaths);
> > > > +    struct hmap ls_dp_groups = HMAP_INITIALIZER(&ls_dp_groups);
> > > > +    struct hmap lr_dp_groups = HMAP_INITIALIZER(&lr_dp_groups);
> > > >        struct ovn_northd_lb *lb;
> > > >        /* Delete any stale SB load balancer rows and create datapath
> > > > @@ -4440,7 +4441,8 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
> > > >             * are not indexed in any way.
> > > >             */
> > > >            lb = ovn_northd_lb_find(lbs, &lb_uuid);
> > > > -        if (!lb || !lb->n_nb_ls || !hmapx_add(&existing_lbs, lb)) {
> > > > +        if (!lb || (!lb->n_nb_ls && !lb->n_nb_lr) ||
> > > > +            !hmapx_add(&existing_lbs, lb)) {
> > > >                sbrec_load_balancer_delete(sbrec_lb);
> > > >                continue;
> > > >            }
> > > > @@ -4448,11 +4450,20 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
> > > >            lb->slb = sbrec_lb;
> > > >            /* Find or create datapath group for this load balancer. */
> > > > -        lb->dpg = ovn_dp_group_get_or_create(ovnsb_txn, &dp_groups,
> > > > -                                             lb->slb->ls_datapath_group,
> > > > -                                             lb->n_nb_ls, lb->nb_ls_map,
> > > > -                                             bitmap_len, true,
> > > > -                                             ls_datapaths, NULL);
> > > > +        if (lb->n_nb_ls) {
> > > > +            lb->ls_dpg = ovn_dp_group_get_or_create(ovnsb_txn, &ls_dp_groups,
> > > > +                                                    lb->slb->ls_datapath_group,
> > > > +                                                    lb->n_nb_ls, lb->nb_ls_map,
> > > > +                                                    ods_size(ls_datapaths),
> > > > +                                                    true, ls_datapaths, NULL);
> > > > +        }
> > > > +        if (lb->n_nb_lr) {
> > > > +            lb->lr_dpg = ovn_dp_group_get_or_create(ovnsb_txn, &lr_dp_groups,
> > > > +                                                    lb->slb->lr_datapath_group,
> > > > +                                                    lb->n_nb_lr, lb->nb_lr_map,
> > > > +                                                    ods_size(lr_datapaths),
> > > > +                                                    false, NULL, lr_datapaths);
> > > > +        }
> > > >        }
> > > >        hmapx_destroy(&existing_lbs);
> > > > @@ -4460,7 +4471,7 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
> > > >         * the SB load balancer columns. */
> > > >        HMAP_FOR_EACH (lb, hmap_node, lbs) {
> > > > -        if (!lb->n_nb_ls) {
> > > > +        if (!lb->n_nb_ls && !lb->n_nb_lr) {
> > > >                continue;
> > > >            }
> > > > @@ -4483,19 +4494,33 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
> > > >            }
> > > >            /* Find or create datapath group for this load balancer. */
> > > > -        if (!lb->dpg) {
> > > > -            lb->dpg = ovn_dp_group_get_or_create(ovnsb_txn, &dp_groups,
> > > > -                                                 lb->slb->ls_datapath_group,
> > > > -                                                 lb->n_nb_ls, lb->nb_ls_map,
> > > > -                                                 bitmap_len, true,
> > > > -                                                 ls_datapaths, NULL);
> > > > +        if (!lb->ls_dpg && lb->n_nb_ls) {
> > > > +            lb->ls_dpg = ovn_dp_group_get_or_create(ovnsb_txn, &ls_dp_groups,
> > > > +                                                    lb->slb->ls_datapath_group,
> > > > +                                                    lb->n_nb_ls, lb->nb_ls_map,
> > > > +                                                    ods_size(ls_datapaths),
> > > > +                                                    true, ls_datapaths, NULL);
> > > > +        }
> > > > +        if (!lb->lr_dpg && lb->n_nb_lr) {
> > > > +            lb->lr_dpg = ovn_dp_group_get_or_create(ovnsb_txn, &lr_dp_groups,
> > > > +                                                    lb->slb->lr_datapath_group,
> > > > +                                                    lb->n_nb_lr, lb->nb_lr_map,
> > > > +                                                    ods_size(lr_datapaths),
> > > > +                                                    false, NULL, lr_datapaths);
> > > >            }
> > > >            /* Update columns. */
> > > >            sbrec_load_balancer_set_name(lb->slb, lb->nlb->name);
> > > >            sbrec_load_balancer_set_vips(lb->slb, ovn_northd_lb_get_vips(lb));
> > > >            sbrec_load_balancer_set_protocol(lb->slb, lb->nlb->protocol);
> > > > -        sbrec_load_balancer_set_ls_datapath_group(lb->slb, lb->dpg->dp_group);
> > > > +        if (lb->ls_dpg) {
> > > > +            sbrec_load_balancer_set_ls_datapath_group(lb->slb,
> > > > +                                                      lb->ls_dpg->dp_group);
> > > > +        }
> > > > +        if (lb->lr_dpg) {
> > > > +            sbrec_load_balancer_set_lr_datapath_group(lb->slb,
> > > > +                                                      lb->lr_dpg->dp_group);
> > > > +        }
> > > >            sbrec_load_balancer_set_options(lb->slb, &options);
> > > >            /* Clearing 'datapaths' column, since 'dp_group' is in use. */
> > > >            sbrec_load_balancer_set_datapaths(lb->slb, NULL, 0);
> > > > @@ -4503,11 +4528,17 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
> > > >        }
> > > >        struct ovn_dp_group *dpg;
> > > > -    HMAP_FOR_EACH_POP (dpg, node, &dp_groups) {
> > > > +    HMAP_FOR_EACH_POP (dpg, node, &ls_dp_groups) {
> > > > +        bitmap_free(dpg->bitmap);
> > > > +        free(dpg);
> > > > +    }
> > > > +    hmap_destroy(&ls_dp_groups);
> > > > +
> > > > +    HMAP_FOR_EACH_POP (dpg, node, &lr_dp_groups) {
> > > >            bitmap_free(dpg->bitmap);
> > > >            free(dpg);
> > > >        }
> > > > -    hmap_destroy(&dp_groups);
> > > > +    hmap_destroy(&lr_dp_groups);
> > > >        /* Datapath_Binding.load_balancers is not used anymore, it's still in the
> > > >         * schema for compatibility reasons.  Reset it to empty, just in case.
> > > > @@ -4516,6 +4547,13 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn,
> > > >        HMAP_FOR_EACH (od, key_node, &ls_datapaths->datapaths) {
> > > >            ovs_assert(od->nbs);
> > > > +        if (od->sb->n_load_balancers) {
> > > > +            sbrec_datapath_binding_set_load_balancers(od->sb, NULL, 0);
> > > > +        }
> > > > +    }
> > > > +    HMAP_FOR_EACH (od, key_node, &lr_datapaths->datapaths) {
> > > > +        ovs_assert(od->nbr);
> > > > +
> > > >            if (od->sb->n_load_balancers) {
> > > >                sbrec_datapath_binding_set_load_balancers(od->sb, NULL, 0);
> > > >            }
> > > > @@ -16683,7 +16721,7 @@ ovnnb_db_run(struct northd_input *input_data,
> > > >        ovn_update_ipv6_prefix(&data->lr_ports);
> > > >        sync_lbs(ovnsb_txn, input_data->sbrec_load_balancer_table,
> > > > -             &data->ls_datapaths, &data->lbs);
> > > > +             &data->ls_datapaths, &data->lr_datapaths, &data->lbs);
> > > >        sync_port_groups(ovnsb_txn, input_data->sbrec_port_group_table,
> > > >                         &data->port_groups);
> > > >        sync_meters(ovnsb_txn, input_data->nbrec_meter_table,
> > > > diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
> > > > index e02fbd884..2a81d54ac 100644
> > > > --- a/ovn-sb.ovsschema
> > > > +++ b/ovn-sb.ovsschema
> > > > @@ -1,7 +1,7 @@
> > > >    {
> > > >        "name": "OVN_Southbound",
> > > >        "version": "20.27.2",
> > > > -    "cksum": "2970847454 30465",
> > > > +    "cksum": "1977000278 30679",
> > > >        "tables": {
> > > >            "SB_Global": {
> > > >                "columns": {
> > > > @@ -534,6 +534,10 @@
> > > >                        {"type": {"key": {"type": "uuid",
> > > >                                          "refTable": "Logical_DP_Group"},
> > > >                                  "min": 0, "max": 1}},
> > > > +                "lr_datapath_group":
> > > > +                    {"type": {"key": {"type": "uuid",
> > > > +                                      "refTable": "Logical_DP_Group"},
> > > > +                              "min": 0, "max": 1}},
> > > >                    "options": {
> > > >                         "type": {"key": "string",
> > > >                                  "value": "string",
> > > > diff --git a/ovn-sb.xml b/ovn-sb.xml
> > > > index 5d11d059b..de92a63f7 100644
> > > > --- a/ovn-sb.xml
> > > > +++ b/ovn-sb.xml
> > > > @@ -4885,6 +4885,12 @@ tcp.flags = RST;
> > > >          datapaths in a group.
> > > >        </column>
> > > > +    <column name="lr_datapath_group">
> > > > +      The group of logical router datapaths to which this load balancer
> > > > +      applies to. This means that the same load balancer applies to all
> > > > +      datapaths in a group.
> > > > +    </column>
> > > > +
> > > >        <group title="Load_Balancer options">
> > > >        <column name="options" key="hairpin_snat_ip">
> > > >          IP to be used as source IP for packets that have been hair-pinned after
> > > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > > > index 63caf8d66..0a7725c37 100644
> > > > --- a/tests/ovn-northd.at
> > > > +++ b/tests/ovn-northd.at
> > > > @@ -2811,6 +2811,15 @@ sb:load_balancer vips,protocol name=lbg0
> > > >    check ovn-nbctl lr-add lr0 -- add logical_router lr0 load_balancer_group $lbg
> > > >    check ovn-nbctl --wait=sb lr-lb-add lr0 lb0
> > > > +check_row_count sb:load_balancer 2
> > > > +
> > > > +lr0_sb_uuid=$(fetch_column datapath_binding _uuid external_ids:name=lr0)
> > > > +lb0_lr_dp_group=$(fetch_column sb:load_balancer lr_datapath_group name=lb0)
> > > > +
> > > > +AT_CHECK_UNQUOTED([ovn-sbctl --bare --columns _uuid,datapaths find Logical_DP_Group dnl
> > > > +                    | grep -A1 $lb0_lr_dp_group | tail -1], [0], [dnl
> > > > +$lr0_sb_uuid
> > > > +])
> > > >    echo
> > > >    echo "__file__:__line__: Check that SB lb0 has only sw0 in datapaths column."
> > > > @@ -2856,7 +2865,13 @@ check_row_count sb:load_balancer 2
> > > >    lbg1=$(fetch_column nb:load_balancer _uuid name=lbg1)
> > > >    check ovn-nbctl add load_balancer_group $lbg load_balancer $lbg1
> > > >    check ovn-nbctl --wait=sb lr-lb-add lr0 lb1
> > > > -check_row_count sb:load_balancer 3
> > > > +check_row_count sb:load_balancer 4
> > > > +
> > > > +lb1_lr_dp_group=$(fetch_column sb:load_balancer lr_datapath_group name=lb1)
> > > > +AT_CHECK_UNQUOTED([ovn-sbctl --bare --columns _uuid,datapaths find Logical_DP_Group dnl
> > > > +                    | grep -A1 $lb1_lr_dp_group | tail -1], [0], [dnl
> > > > +$lr0_sb_uuid
> > > > +])
> > > >    echo
> > > >    echo "__file__:__line__: Associate lb1 to sw1 and check that lb1 is created in SB DB."
> > > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> > > > index 6669c18e7..08b380d10 100644
> > > > --- a/tests/system-ovn.at
> > > > +++ b/tests/system-ovn.at
> > > > @@ -11546,3 +11546,120 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
> > > >    AT_CLEANUP
> > > >    ])
> > > > +
> > > > +OVN_FOR_EACH_NORTHD([
> > > > +AT_SETUP([ct_flush on logical router load balancer])
> > > > +AT_KEYWORDS([ct-lr-flush])
> > > > +CHECK_CONNTRACK()
> > > > +CHECK_CONNTRACK_NAT()
> > > > +ovn_start
> > > > +OVS_TRAFFIC_VSWITCHD_START()
> > > > +ADD_BR([br-int])
> > > > +#
> > > > +# Set external-ids in br-int needed for ovn-controller
> > > > +ovs-vsctl \
> > > > +        -- set Open_vSwitch . external-ids:system-id=hv1 \
> > > > +        -- set Open_vSwitch . external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
> > > > +        -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
> > > > +        -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
> > > > +        -- set bridge br-int fail-mode=secure other-config:disable-in-band=true
> > > > +
> > > > +start_daemon ovn-controller
> > > > +
> > > > +check ovn-nbctl lr-add R1
> > > > +
> > > > +check ovn-nbctl ls-add sw0
> > > > +check ovn-nbctl ls-add public
> > > > +
> > > > +check ovn-nbctl lrp-add R1 rp-sw0 00:00:01:01:02:03 192.168.1.1/24
> > > > +check ovn-nbctl lrp-add R1 rp-public 00:00:02:01:02:03 172.16.1.1/24
> > > > +
> > > > +check ovn-nbctl set logical_router R1 options:chassis=hv1
> > > > +
> > > > +check ovn-nbctl lsp-add sw0 sw0-rp -- set Logical_Switch_Port sw0-rp \
> > > > +    type=router options:router-port=rp-sw0 \
> > > > +    -- lsp-set-addresses sw0-rp router
> > > > +
> > > > +check ovn-nbctl lsp-add sw0 sw0-vm \
> > > > +    -- lsp-set-addresses sw0-vm "00:00:01:01:02:04 192.168.1.2/24"
> > > > +
> > > > +check ovn-nbctl lsp-add public public-rp -- set Logical_Switch_Port public-rp \
> > > > +    type=router options:router-port=rp-public \
> > > > +    -- lsp-set-addresses public-rp router
> > > > +
> > > > +check ovn-nbctl lsp-add public public-vm \
> > > > +   -- lsp-set-addresses public-vm "00:00:02:01:02:04 172.16.1.2/24"
> > > > +
> > > > +ADD_NAMESPACES(sw0-vm)
> > > > +ADD_VETH(sw0-vm, sw0-vm, br-int, "192.168.1.2/24", "00:00:01:01:02:04", \
> > > > +         "192.168.1.1")
> > > > +OVS_WAIT_UNTIL([test "$(ip netns exec sw0-vm ip a | grep fe80 | grep tentative)" = ""])
> > > > +
> > > > +ADD_NAMESPACES(public-vm)
> > > > +ADD_VETH(public-vm, public-vm, br-int, "172.16.1.2/24", "00:00:02:01:02:04", \
> > > > +         "172.16.1.1")
> > > > +
> > > > +OVS_WAIT_UNTIL([test "$(ip netns exec public-vm ip a | grep fe80 | grep tentative)" = ""])
> > > > +
> > > > +# Start webservers in 'server'.
> > > > +OVS_START_L7([sw0-vm], [http])
> > > > +
> > > > +# Create a load balancer and associate to R1
> > > > +check ovn-nbctl lb-add lb1 172.16.1.150:80 192.168.1.2:80 \
> > > > +    -- set load_balancer lb1 options:ct_flush="true"
> > > > +check ovn-nbctl lr-lb-add R1 lb1
> > > > +
> > > > +check ovn-nbctl --wait=hv sync
> > > > +
> > > > +for i in $(seq 1 5); do
> > > > +    echo Request $i
> > > > +    NS_CHECK_EXEC([public-vm], [wget 172.16.1.150 -t 5 -T 1 --retry-connrefused -v -o wget$i.log])
> > > > +done
> > > > +
> > > > +OVS_WAIT_FOR_OUTPUT([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.150) | wc -l ], [0], [dnl
> > > > +1
> > > > +])
> > > > +
> > > > +check ovn-nbctl lb-del lb1
> > > > +
> > > > +OVS_WAIT_FOR_OUTPUT([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.150) | wc -l ], [0], [dnl
> > > > +0
> > > > +])
> > > > +
> > > > +check ovn-nbctl lb-add lb2 172.16.1.151:80 192.168.1.2:80
> > > > +check ovn-nbctl lr-lb-add R1 lb2
> > > > +
> > > > +check ovn-nbctl --wait=hv sync
> > > > +
> > > > +for i in $(seq 1 5); do
> > > > +    echo Request $i
> > > > +    NS_CHECK_EXEC([public-vm], [wget 172.16.1.151 -t 5 -T 1 --retry-connrefused -v -o wget$i.log])
> > > > +done
> > > > +
> > > > +OVS_WAIT_FOR_OUTPUT([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.151) | wc -l ], [0], [dnl
> > > > +1
> > > > +])
> > > > +
> > > > +check ovn-nbctl lb-del lb2
> > > > +
> > > > +OVS_WAIT_FOR_OUTPUT([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.151) | wc -l ], [0], [dnl
> > > > +1
> > > > +])
> > > 
> > > The version of this test on the linked BZ expects 0 as the output when
> > > dumping conntrack at this point. Deleting the load balancer should flush the
> > > conntrack entry, so there should now be no conntrack entries for the load
> > > balancer VIP.
> > > 
> > > Is there a reason why this was changed to 1 instead? The problem is that the
> > > condition being checked before and after deleting the load balancer is the
> > > same. It's not clear what is being tested here.
> > 
> > Hi Mark,
> > 
> > please note this test refers to a different load balancer (lb2) where I have
> > not set ct_flush to "true". Am I missing something?
> > 
> > Regards,
> > Lorenzo
> 
> Yes I noticed this about a minute after I sent my reply and sent a follow-up
> saying to ignore my feedback. Sorry for the noise :)

no worries,

Regards,
Lorenzo

> 
> > 
> > > 
> > > I think one of two changes needs to happen with this test:
> > > 
> > > 1) If the test is not actually exercising the expected conntrack-flushing
> > > behavior, then the test needs to be adjusted to properly test the conntrack
> > > flush.
> > > 
> > > 2) If the test is correct, and we actually do expect a conntrack entry to
> > > still exist, then rather than counting the number of conntrack entries, we
> > > need to inspect the conntrack entry to ensure that something has actually
> > > happened as a result of deleting the load balancer.
> > > 
> > > > +
> > > > +OVS_APP_EXIT_AND_WAIT([ovn-controller])
> > > > +
> > > > +as ovn-sb
> > > > +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> > > > +
> > > > +as ovn-nb
> > > > +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> > > > +
> > > > +as northd
> > > > +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE])
> > > > +
> > > > +as
> > > > +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
> > > > +/Failed to acquire.*/d
> > > > +/connection dropped.*/d"])
> > > > +AT_CLEANUP
> > > > +])
> > > > diff --git a/utilities/ovn-sbctl.c b/utilities/ovn-sbctl.c
> > > > index ddd9a9ca9..0e3afaea7 100644
> > > > --- a/utilities/ovn-sbctl.c
> > > > +++ b/utilities/ovn-sbctl.c
> > > > @@ -397,6 +397,7 @@ pre_get_info(struct ctl_context *ctx)
> > > >        ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_datapaths);
> > > >        ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_ls_datapath_group);
> > > > +    ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_lr_datapath_group);
> > > >        ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_vips);
> > > >        ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_name);
> > > >        ovsdb_idl_add_column(ctx->idl, &sbrec_load_balancer_col_protocol);
> > > 
>