[ovs-dev,v2,ovn] northd: add empty_lb controller_event for logical router

Add empty load balancer controller_event support to logical router
pipeline. Update northd documentation even for logical switch pipeline

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
Changes since v1:
- rebase on-top of current ovn master branch
---
 northd/ovn-northd.8.xml | 10 ++++++++
 northd/ovn-northd.c     | 26 ++++++++++++-------
 tests/ovn.at            | 56 +++++++++++++++++++++++++++++++++++------
 3 files changed, 76 insertions(+), 16 deletions(-)

Hi Lorenzo, I only have one finding down below

On 9/6/19 11:00 AM, Lorenzo Bianconi wrote:
> Add empty load balancer controller_event support to logical router
> pipeline. Update northd documentation even for logical switch pipeline
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> ---
> Changes since v1:
> - rebase on-top of current ovn master branch
> ---
>   northd/ovn-northd.8.xml | 10 ++++++++
>   northd/ovn-northd.c     | 26 ++++++++++++-------
>   tests/ovn.at            | 56 +++++++++++++++++++++++++++++++++++------
>   3 files changed, 76 insertions(+), 16 deletions(-)
> 
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index b34ef687a..0f4f1c112 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -1785,6 +1785,16 @@ icmp6 {
>       </p>
>   
>       <ul>
> +      <li>
> +        If controller_event has been enabled for all the configured load
> +        balancing rules for a Gateway router or Router with gateway port
> +        in <code>OVN_Northbound</code> database that does not have configured
> +        backends, a priority-130 flow is added to trigger ovn-controller events
> +        whenever the chassis receives a packet for that particular VIP.
> +        If <code>event-elb</code> meter has been previously created, it will be
> +        associated to the empty_lb logical flow
> +      </li>
> +
>         <li>
>           For all the configured load balancing rules for a Gateway router or
>           Router with gateway port in <code>OVN_Northbound</code> database that
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index c24e4d864..9bb1e7dd5 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -6148,9 +6148,15 @@ get_force_snat_ip(struct ovn_datapath *od, const char *key_type, ovs_be32 *ip)
>   static void
>   add_router_lb_flow(struct hmap *lflows, struct ovn_datapath *od,
>                      struct ds *match, struct ds *actions, int priority,
> -                   const char *lb_force_snat_ip, char *backend_ips,
> -                   bool is_udp, int addr_family)
> +                   const char *lb_force_snat_ip, struct smap_node *node,

I don't understand the change from "backend_ips" to "node" here. You 
only ever access node->value in the function, and the name "node" is not 
very descriptive. I think this should stay how it currently is.

> +                   bool is_udp, int addr_family, char *ip_addr,
> +                   uint16_t l4_port, struct nbrec_load_balancer *lb,
> +                   struct shash *meter_groups)
>   {
> +    build_empty_lb_event_flow(od, lflows, node, ip_addr, lb,
> +                              l4_port, addr_family, S_ROUTER_IN_DNAT,
> +                              meter_groups);
> +
>       /* A match and actions for new connections. */
>       char *new_match = xasprintf("ct.new && %s", ds_cstr(match));
>       if (lb_force_snat_ip) {
> @@ -6177,7 +6183,7 @@ add_router_lb_flow(struct hmap *lflows, struct ovn_datapath *od,
>       free(new_match);
>       free(est_match);
>   
> -    if (!od->l3dgw_port || !od->l3redirect_port || !backend_ips) {
> +    if (!od->l3dgw_port || !od->l3redirect_port || !node->value) {
>           return;
>       }
>   
> @@ -6192,7 +6198,7 @@ add_router_lb_flow(struct hmap *lflows, struct ovn_datapath *od,
>           ds_put_cstr(&undnat_match, "ip6 && (");
>       }
>       char *start, *next, *ip_str;
> -    start = next = xstrdup(backend_ips);
> +    start = next = xstrdup(node->value);
>       ip_str = strsep(&next, ",");
>       bool backend_ips_found = false;
>       while (ip_str && ip_str[0]) {
> @@ -6308,7 +6314,7 @@ copy_ra_to_sb(struct ovn_port *op, const char *address_mode)
>   
>   static void
>   build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
> -                    struct hmap *lflows)
> +                    struct hmap *lflows, struct shash *meter_groups)
>   {
>       /* This flow table structure is documented in ovn-northd(8), so please
>        * update ovn-northd.8.xml if you change anything. */
> @@ -7525,7 +7531,6 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>                       ds_put_format(&match, "ip && ip6.dst == %s",
>                                   ip_address);
>                   }
> -                free(ip_address);
>   
>                   int prio = 110;
>                   bool is_udp = lb->protocol && !strcmp(lb->protocol, "udp") ?
> @@ -7546,8 +7551,11 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>                                     od->l3redirect_port->json_key);
>                   }
>                   add_router_lb_flow(lflows, od, &match, &actions, prio,
> -                                   lb_force_snat_ip, node->value, is_udp,
> -                                   addr_family);
> +                                   lb_force_snat_ip, node, is_udp,
> +                                   addr_family, ip_address, port, lb,
> +                                   meter_groups);
> +
> +                free(ip_address);
>               }
>           }
>           sset_destroy(&all_ips);
> @@ -8328,7 +8336,7 @@ build_lflows(struct northd_context *ctx, struct hmap *datapaths,
>   
>       build_lswitch_flows(datapaths, ports, port_groups, &lflows, mcgroups,
>                           igmp_groups, meter_groups);
> -    build_lrouter_flows(datapaths, ports, &lflows);
> +    build_lrouter_flows(datapaths, ports, &lflows, meter_groups);
>   
>       /* Push changes to the Logical_Flow table to database. */
>       const struct sbrec_logical_flow *sbflow, *next_sbflow;
> diff --git a/tests/ovn.at b/tests/ovn.at
> index de1b3b3ba..2749184eb 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -14683,9 +14683,22 @@ ovn_start
>   # Create hypervisors hv[12].
>   # Add vif1[12] to hv1, vif2[12] to hv2
>   # Add all of the vifs to a single logical switch sw0.
> +# Create logical router lr0
>   
>   net_add n1
> -ovn-nbctl ls-add sw0
> +
> +ovn-nbctl create Logical_Router name=lr0 options:chassis=hv1
> +for i in 0 1; do
> +    idx=$((i+1))
> +    ovn-nbctl ls-add sw$i
> +    ovn-nbctl lrp-add lr0 lrp$i 00:00:00:00:ff:0$idx 192.168.$idx.254/24
> +    ovn-nbctl \
> +        -- lsp-add sw$i lrp$i-attachment \
> +        -- set Logical_Switch_Port lrp$i-attachment type=router \
> +                         options:router-port=lrp$i \
> +                         addresses='"00:00:00:00:ff:'0$idx'"'
> +done
> +
>   for i in 1 2; do
>       sim_add hv$i
>       as hv$i
> @@ -14705,10 +14718,24 @@ for i in 1 2; do
>       done
>   done
>   
> +as hv1
> +ovn-nbctl lsp-add sw1 sw1-p0 \
> +    -- lsp-set-addresses sw1-p0 "00:00:00:00:00:33 192.168.2.11"
> +ovs-vsctl -- add-port br-int vif33 -- \
> +        set interface vif33 \
> +        external-ids:iface-id=sw1-p0 \
> +        options:tx_pcap=hv$i/vif33-tx.pcap \
> +        options:rxq_pcap=hv$i/vif33-rx.pcap \
> +        ofport-request=33
> +
>   ovn-nbctl --wait=hv set NB_Global . options:controller_event=true
>   ovn-nbctl lb-add lb0 192.168.1.100:80 ""
>   ovn-nbctl ls-lb-add sw0 lb0
> -uuid_lb=$(ovn-nbctl --bare --columns=_uuid find load_balancer name=lb0)
> +uuid_lb0=$(ovn-nbctl --bare --columns=_uuid find load_balancer name=lb0)
> +
> +ovn-nbctl lb-add lb1 192.168.2.100:80 ""
> +ovn-nbctl lr-lb-add lr0 lb1
> +uuid_lb1=$(ovn-nbctl --bare --columns=_uuid find load_balancer name=lb1)
>   ovn-nbctl --wait=hv meter-add event-elb drop 100 pktps 10
>   
>   OVN_POPULATE_ARP
> @@ -14716,10 +14743,10 @@ ovn-nbctl --timeout=3 --wait=hv sync
>   ovn-sbctl lflow-list
>   as hv1 ovs-ofctl dump-flows br-int
>   
> -packet="inport==\"sw0-p11\" && eth.src==00:00:00:00:00:11 && eth.dst==00:00:00:00:00:21 &&
> -       ip4 && ip.ttl==64 && ip4.src==192.168.1.11 && ip4.dst==192.168.1.100 &&
> -       tcp && tcp.src==10000 && tcp.dst==80"
> -as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet"
> +packet0="inport==\"sw0-p11\" && eth.src==00:00:00:00:00:11 && eth.dst==00:00:00:00:00:21 &&
> +         ip4 && ip.ttl==64 && ip4.src==192.168.1.11 && ip4.dst==192.168.1.100 &&
> +         tcp && tcp.src==10000 && tcp.dst==80"
> +as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet0"
>   
>   ovn-sbctl list controller_event
>   uuid=$(ovn-sbctl list controller_event | awk '/_uuid/{print $3}')
> @@ -14733,12 +14760,27 @@ AT_CHECK([ovn-sbctl get controller_event $uuid event_info:protocol], [0], [dnl
>   tcp
>   ])
>   AT_CHECK_UNQUOTED([ovn-sbctl get controller_event $uuid event_info:load_balancer], [0], [dnl
> -"$uuid_lb"
> +"$uuid_lb0"
>   ])
>   AT_CHECK([ovn-sbctl get controller_event $uuid seq_num], [0], [dnl
>   1
>   ])
>   
> +ovn-sbctl destroy controller_event $uuid
> +packet1="inport==\"sw1-p0\" && eth.src==00:00:00:00:00:33 && eth.dst==00:00:00:00:ff:02 &&
> +         ip4 && ip.ttl==64 && ip4.src==192.168.2.11 && ip4.dst==192.168.2.100 &&
> +         tcp && tcp.src==10000 && tcp.dst==80"
> +
> +as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet1"
> +ovn-sbctl list controller_event
> +uuid=$(ovn-sbctl list controller_event | awk '/_uuid/{print $3}')
> +AT_CHECK([ovn-sbctl get controller_event $uuid event_type], [0], [dnl
> +empty_lb_backends
> +])
> +AT_CHECK([ovn-sbctl get controller_event $uuid event_info:vip], [0], [dnl
> +"192.168.2.100:80"
> +])
> +
>   OVN_CLEANUP([hv1], [hv2])
>   AT_CLEANUP
>   
>

>
> Hi Lorenzo, I only have one finding down below
>
> On 9/6/19 11:00 AM, Lorenzo Bianconi wrote:
> > Add empty load balancer controller_event support to logical router
> > pipeline. Update northd documentation even for logical switch pipeline
> >
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> > ---
> > Changes since v1:
> > - rebase on-top of current ovn master branch
> > ---
> >   northd/ovn-northd.8.xml | 10 ++++++++
> >   northd/ovn-northd.c     | 26 ++++++++++++-------
> >   tests/ovn.at            | 56 +++++++++++++++++++++++++++++++++++------
> >   3 files changed, 76 insertions(+), 16 deletions(-)
> >

[...]

> > --- a/northd/ovn-northd.c
> > +++ b/northd/ovn-northd.c
> > @@ -6148,9 +6148,15 @@ get_force_snat_ip(struct ovn_datapath *od, const char *key_type, ovs_be32 *ip)
> >   static void
> >   add_router_lb_flow(struct hmap *lflows, struct ovn_datapath *od,
> >                      struct ds *match, struct ds *actions, int priority,
> > -                   const char *lb_force_snat_ip, char *backend_ips,
> > -                   bool is_udp, int addr_family)
> > +                   const char *lb_force_snat_ip, struct smap_node *node,
>
> I don't understand the change from "backend_ips" to "node" here. You
> only ever access node->value in the function, and the name "node" is not
> very descriptive. I think this should stay how it currently is.
>

Hi Mark,

thx for the review. I did this change since I would reuse
build_empty_lb_event_flow() and it will need both node->value and
node->key.

Regards,
Lorenzo

> > +                   bool is_udp, int addr_family, char *ip_addr,
> > +                   uint16_t l4_port, struct nbrec_load_balancer *lb,
> > +                   struct shash *meter_groups)
> >   {
> > +    build_empty_lb_event_flow(od, lflows, node, ip_addr, lb,
> > +                              l4_port, addr_family, S_ROUTER_IN_DNAT,
> > +                              meter_groups);
> > +
> >       /* A match and actions for new connections. */
> >       char *new_match = xasprintf("ct.new && %s", ds_cstr(match));
> >       if (lb_force_snat_ip) {
> > @@ -6177,7 +6183,7 @@ add_router_lb_flow(struct hmap *lflows, struct ovn_datapath *od,
> >       free(new_match);
> >       free(est_match);
> >
> > -    if (!od->l3dgw_port || !od->l3redirect_port || !backend_ips) {
> > +    if (!od->l3dgw_port || !od->l3redirect_port || !node->value) {
> >           return;
> >       }
> >
> > @@ -6192,7 +6198,7 @@ add_router_lb_flow(struct hmap *lflows, struct ovn_datapath *od,
> >           ds_put_cstr(&undnat_match, "ip6 && (");
> >       }
> >       char *start, *next, *ip_str;
> > -    start = next = xstrdup(backend_ips);
> > +    start = next = xstrdup(node->value);
> >       ip_str = strsep(&next, ",");
> >       bool backend_ips_found = false;
> >       while (ip_str && ip_str[0]) {
> > @@ -6308,7 +6314,7 @@ copy_ra_to_sb(struct ovn_port *op, const char *address_mode)
> >
> >   static void
> >   build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
> > -                    struct hmap *lflows)
> > +                    struct hmap *lflows, struct shash *meter_groups)
> >   {
> >       /* This flow table structure is documented in ovn-northd(8), so please
> >        * update ovn-northd.8.xml if you change anything. */
> > @@ -7525,7 +7531,6 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
> >                       ds_put_format(&match, "ip && ip6.dst == %s",
> >                                   ip_address);
> >                   }
> > -                free(ip_address);
> >
> >                   int prio = 110;
> >                   bool is_udp = lb->protocol && !strcmp(lb->protocol, "udp") ?
> > @@ -7546,8 +7551,11 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
> >                                     od->l3redirect_port->json_key);
> >                   }
> >                   add_router_lb_flow(lflows, od, &match, &actions, prio,
> > -                                   lb_force_snat_ip, node->value, is_udp,
> > -                                   addr_family);
> > +                                   lb_force_snat_ip, node, is_udp,
> > +                                   addr_family, ip_address, port, lb,
> > +                                   meter_groups);
> > +
> > +                free(ip_address);
> >               }
> >           }
> >           sset_destroy(&all_ips);
> > @@ -8328,7 +8336,7 @@ build_lflows(struct northd_context *ctx, struct hmap *datapaths,
> >
> >       build_lswitch_flows(datapaths, ports, port_groups, &lflows, mcgroups,
> >                           igmp_groups, meter_groups);
> > -    build_lrouter_flows(datapaths, ports, &lflows);
> > +    build_lrouter_flows(datapaths, ports, &lflows, meter_groups);
> >
> >       /* Push changes to the Logical_Flow table to database. */
> >       const struct sbrec_logical_flow *sbflow, *next_sbflow;
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index de1b3b3ba..2749184eb 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -14683,9 +14683,22 @@ ovn_start
> >   # Create hypervisors hv[12].
> >   # Add vif1[12] to hv1, vif2[12] to hv2
> >   # Add all of the vifs to a single logical switch sw0.
> > +# Create logical router lr0
> >
> >   net_add n1
> > -ovn-nbctl ls-add sw0
> > +
> > +ovn-nbctl create Logical_Router name=lr0 options:chassis=hv1
> > +for i in 0 1; do
> > +    idx=$((i+1))
> > +    ovn-nbctl ls-add sw$i
> > +    ovn-nbctl lrp-add lr0 lrp$i 00:00:00:00:ff:0$idx 192.168.$idx.254/24
> > +    ovn-nbctl \
> > +        -- lsp-add sw$i lrp$i-attachment \
> > +        -- set Logical_Switch_Port lrp$i-attachment type=router \
> > +                         options:router-port=lrp$i \
> > +                         addresses='"00:00:00:00:ff:'0$idx'"'
> > +done
> > +
> >   for i in 1 2; do
> >       sim_add hv$i
> >       as hv$i
> > @@ -14705,10 +14718,24 @@ for i in 1 2; do
> >       done
> >   done
> >
> > +as hv1
> > +ovn-nbctl lsp-add sw1 sw1-p0 \
> > +    -- lsp-set-addresses sw1-p0 "00:00:00:00:00:33 192.168.2.11"
> > +ovs-vsctl -- add-port br-int vif33 -- \
> > +        set interface vif33 \
> > +        external-ids:iface-id=sw1-p0 \
> > +        options:tx_pcap=hv$i/vif33-tx.pcap \
> > +        options:rxq_pcap=hv$i/vif33-rx.pcap \
> > +        ofport-request=33
> > +
> >   ovn-nbctl --wait=hv set NB_Global . options:controller_event=true
> >   ovn-nbctl lb-add lb0 192.168.1.100:80 ""
> >   ovn-nbctl ls-lb-add sw0 lb0
> > -uuid_lb=$(ovn-nbctl --bare --columns=_uuid find load_balancer name=lb0)
> > +uuid_lb0=$(ovn-nbctl --bare --columns=_uuid find load_balancer name=lb0)
> > +
> > +ovn-nbctl lb-add lb1 192.168.2.100:80 ""
> > +ovn-nbctl lr-lb-add lr0 lb1
> > +uuid_lb1=$(ovn-nbctl --bare --columns=_uuid find load_balancer name=lb1)
> >   ovn-nbctl --wait=hv meter-add event-elb drop 100 pktps 10
> >
> >   OVN_POPULATE_ARP
> > @@ -14716,10 +14743,10 @@ ovn-nbctl --timeout=3 --wait=hv sync
> >   ovn-sbctl lflow-list
> >   as hv1 ovs-ofctl dump-flows br-int
> >
> > -packet="inport==\"sw0-p11\" && eth.src==00:00:00:00:00:11 && eth.dst==00:00:00:00:00:21 &&
> > -       ip4 && ip.ttl==64 && ip4.src==192.168.1.11 && ip4.dst==192.168.1.100 &&
> > -       tcp && tcp.src==10000 && tcp.dst==80"
> > -as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet"
> > +packet0="inport==\"sw0-p11\" && eth.src==00:00:00:00:00:11 && eth.dst==00:00:00:00:00:21 &&
> > +         ip4 && ip.ttl==64 && ip4.src==192.168.1.11 && ip4.dst==192.168.1.100 &&
> > +         tcp && tcp.src==10000 && tcp.dst==80"
> > +as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet0"
> >
> >   ovn-sbctl list controller_event
> >   uuid=$(ovn-sbctl list controller_event | awk '/_uuid/{print $3}')
> > @@ -14733,12 +14760,27 @@ AT_CHECK([ovn-sbctl get controller_event $uuid event_info:protocol], [0], [dnl
> >   tcp
> >   ])
> >   AT_CHECK_UNQUOTED([ovn-sbctl get controller_event $uuid event_info:load_balancer], [0], [dnl
> > -"$uuid_lb"
> > +"$uuid_lb0"
> >   ])
> >   AT_CHECK([ovn-sbctl get controller_event $uuid seq_num], [0], [dnl
> >   1
> >   ])
> >
> > +ovn-sbctl destroy controller_event $uuid
> > +packet1="inport==\"sw1-p0\" && eth.src==00:00:00:00:00:33 && eth.dst==00:00:00:00:ff:02 &&
> > +         ip4 && ip.ttl==64 && ip4.src==192.168.2.11 && ip4.dst==192.168.2.100 &&
> > +         tcp && tcp.src==10000 && tcp.dst==80"
> > +
> > +as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet1"
> > +ovn-sbctl list controller_event
> > +uuid=$(ovn-sbctl list controller_event | awk '/_uuid/{print $3}')
> > +AT_CHECK([ovn-sbctl get controller_event $uuid event_type], [0], [dnl
> > +empty_lb_backends
> > +])
> > +AT_CHECK([ovn-sbctl get controller_event $uuid event_info:vip], [0], [dnl
> > +"192.168.2.100:80"
> > +])
> > +
> >   OVN_CLEANUP([hv1], [hv2])
> >   AT_CLEANUP
> >
> >
>

On 9/7/19 1:28 PM, Lorenzo Bianconi wrote:
>>
>> Hi Lorenzo, I only have one finding down below
>>
>> On 9/6/19 11:00 AM, Lorenzo Bianconi wrote:
>>> Add empty load balancer controller_event support to logical router
>>> pipeline. Update northd documentation even for logical switch pipeline
>>>
>>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
>>> ---
>>> Changes since v1:
>>> - rebase on-top of current ovn master branch
>>> ---
>>>    northd/ovn-northd.8.xml | 10 ++++++++
>>>    northd/ovn-northd.c     | 26 ++++++++++++-------
>>>    tests/ovn.at            | 56 +++++++++++++++++++++++++++++++++++------
>>>    3 files changed, 76 insertions(+), 16 deletions(-)
>>>
> 
> [...]
> 
>>> --- a/northd/ovn-northd.c
>>> +++ b/northd/ovn-northd.c
>>> @@ -6148,9 +6148,15 @@ get_force_snat_ip(struct ovn_datapath *od, const char *key_type, ovs_be32 *ip)
>>>    static void
>>>    add_router_lb_flow(struct hmap *lflows, struct ovn_datapath *od,
>>>                       struct ds *match, struct ds *actions, int priority,
>>> -                   const char *lb_force_snat_ip, char *backend_ips,
>>> -                   bool is_udp, int addr_family)
>>> +                   const char *lb_force_snat_ip, struct smap_node *node,
>>
>> I don't understand the change from "backend_ips" to "node" here. You
>> only ever access node->value in the function, and the name "node" is not
>> very descriptive. I think this should stay how it currently is.
>>
> 
> Hi Mark,
> 
> thx for the review. I did this change since I would reuse
> build_empty_lb_event_flow() and it will need both node->value and
> node->key.
> 
> Regards,
> Lorenzo

OK, that's fine. But could you still rename "node" to something more 
descriptive? Or alternatively, maybe at the top of the function set some 
local pointers to the node parts so that it is more clear what is being 
referenced, e.g:

const char *vip = node->key;
const char *backend_ips = node->value;

(in this particular function, you wouldn't need "vip" but hopefully you 
get what I'm going for here)

> 
>>> +                   bool is_udp, int addr_family, char *ip_addr,
>>> +                   uint16_t l4_port, struct nbrec_load_balancer *lb,
>>> +                   struct shash *meter_groups)
>>>    {
>>> +    build_empty_lb_event_flow(od, lflows, node, ip_addr, lb,
>>> +                              l4_port, addr_family, S_ROUTER_IN_DNAT,
>>> +                              meter_groups);
>>> +
>>>        /* A match and actions for new connections. */
>>>        char *new_match = xasprintf("ct.new && %s", ds_cstr(match));
>>>        if (lb_force_snat_ip) {
>>> @@ -6177,7 +6183,7 @@ add_router_lb_flow(struct hmap *lflows, struct ovn_datapath *od,
>>>        free(new_match);
>>>        free(est_match);
>>>
>>> -    if (!od->l3dgw_port || !od->l3redirect_port || !backend_ips) {
>>> +    if (!od->l3dgw_port || !od->l3redirect_port || !node->value) {
>>>            return;
>>>        }
>>>
>>> @@ -6192,7 +6198,7 @@ add_router_lb_flow(struct hmap *lflows, struct ovn_datapath *od,
>>>            ds_put_cstr(&undnat_match, "ip6 && (");
>>>        }
>>>        char *start, *next, *ip_str;
>>> -    start = next = xstrdup(backend_ips);
>>> +    start = next = xstrdup(node->value);
>>>        ip_str = strsep(&next, ",");
>>>        bool backend_ips_found = false;
>>>        while (ip_str && ip_str[0]) {
>>> @@ -6308,7 +6314,7 @@ copy_ra_to_sb(struct ovn_port *op, const char *address_mode)
>>>
>>>    static void
>>>    build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>>> -                    struct hmap *lflows)
>>> +                    struct hmap *lflows, struct shash *meter_groups)
>>>    {
>>>        /* This flow table structure is documented in ovn-northd(8), so please
>>>         * update ovn-northd.8.xml if you change anything. */
>>> @@ -7525,7 +7531,6 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>>>                        ds_put_format(&match, "ip && ip6.dst == %s",
>>>                                    ip_address);
>>>                    }
>>> -                free(ip_address);
>>>
>>>                    int prio = 110;
>>>                    bool is_udp = lb->protocol && !strcmp(lb->protocol, "udp") ?
>>> @@ -7546,8 +7551,11 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>>>                                      od->l3redirect_port->json_key);
>>>                    }
>>>                    add_router_lb_flow(lflows, od, &match, &actions, prio,
>>> -                                   lb_force_snat_ip, node->value, is_udp,
>>> -                                   addr_family);
>>> +                                   lb_force_snat_ip, node, is_udp,
>>> +                                   addr_family, ip_address, port, lb,
>>> +                                   meter_groups);
>>> +
>>> +                free(ip_address);
>>>                }
>>>            }
>>>            sset_destroy(&all_ips);
>>> @@ -8328,7 +8336,7 @@ build_lflows(struct northd_context *ctx, struct hmap *datapaths,
>>>
>>>        build_lswitch_flows(datapaths, ports, port_groups, &lflows, mcgroups,
>>>                            igmp_groups, meter_groups);
>>> -    build_lrouter_flows(datapaths, ports, &lflows);
>>> +    build_lrouter_flows(datapaths, ports, &lflows, meter_groups);
>>>
>>>        /* Push changes to the Logical_Flow table to database. */
>>>        const struct sbrec_logical_flow *sbflow, *next_sbflow;
>>> diff --git a/tests/ovn.at b/tests/ovn.at
>>> index de1b3b3ba..2749184eb 100644
>>> --- a/tests/ovn.at
>>> +++ b/tests/ovn.at
>>> @@ -14683,9 +14683,22 @@ ovn_start
>>>    # Create hypervisors hv[12].
>>>    # Add vif1[12] to hv1, vif2[12] to hv2
>>>    # Add all of the vifs to a single logical switch sw0.
>>> +# Create logical router lr0
>>>
>>>    net_add n1
>>> -ovn-nbctl ls-add sw0
>>> +
>>> +ovn-nbctl create Logical_Router name=lr0 options:chassis=hv1
>>> +for i in 0 1; do
>>> +    idx=$((i+1))
>>> +    ovn-nbctl ls-add sw$i
>>> +    ovn-nbctl lrp-add lr0 lrp$i 00:00:00:00:ff:0$idx 192.168.$idx.254/24
>>> +    ovn-nbctl \
>>> +        -- lsp-add sw$i lrp$i-attachment \
>>> +        -- set Logical_Switch_Port lrp$i-attachment type=router \
>>> +                         options:router-port=lrp$i \
>>> +                         addresses='"00:00:00:00:ff:'0$idx'"'
>>> +done
>>> +
>>>    for i in 1 2; do
>>>        sim_add hv$i
>>>        as hv$i
>>> @@ -14705,10 +14718,24 @@ for i in 1 2; do
>>>        done
>>>    done
>>>
>>> +as hv1
>>> +ovn-nbctl lsp-add sw1 sw1-p0 \
>>> +    -- lsp-set-addresses sw1-p0 "00:00:00:00:00:33 192.168.2.11"
>>> +ovs-vsctl -- add-port br-int vif33 -- \
>>> +        set interface vif33 \
>>> +        external-ids:iface-id=sw1-p0 \
>>> +        options:tx_pcap=hv$i/vif33-tx.pcap \
>>> +        options:rxq_pcap=hv$i/vif33-rx.pcap \
>>> +        ofport-request=33
>>> +
>>>    ovn-nbctl --wait=hv set NB_Global . options:controller_event=true
>>>    ovn-nbctl lb-add lb0 192.168.1.100:80 ""
>>>    ovn-nbctl ls-lb-add sw0 lb0
>>> -uuid_lb=$(ovn-nbctl --bare --columns=_uuid find load_balancer name=lb0)
>>> +uuid_lb0=$(ovn-nbctl --bare --columns=_uuid find load_balancer name=lb0)
>>> +
>>> +ovn-nbctl lb-add lb1 192.168.2.100:80 ""
>>> +ovn-nbctl lr-lb-add lr0 lb1
>>> +uuid_lb1=$(ovn-nbctl --bare --columns=_uuid find load_balancer name=lb1)
>>>    ovn-nbctl --wait=hv meter-add event-elb drop 100 pktps 10
>>>
>>>    OVN_POPULATE_ARP
>>> @@ -14716,10 +14743,10 @@ ovn-nbctl --timeout=3 --wait=hv sync
>>>    ovn-sbctl lflow-list
>>>    as hv1 ovs-ofctl dump-flows br-int
>>>
>>> -packet="inport==\"sw0-p11\" && eth.src==00:00:00:00:00:11 && eth.dst==00:00:00:00:00:21 &&
>>> -       ip4 && ip.ttl==64 && ip4.src==192.168.1.11 && ip4.dst==192.168.1.100 &&
>>> -       tcp && tcp.src==10000 && tcp.dst==80"
>>> -as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet"
>>> +packet0="inport==\"sw0-p11\" && eth.src==00:00:00:00:00:11 && eth.dst==00:00:00:00:00:21 &&
>>> +         ip4 && ip.ttl==64 && ip4.src==192.168.1.11 && ip4.dst==192.168.1.100 &&
>>> +         tcp && tcp.src==10000 && tcp.dst==80"
>>> +as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet0"
>>>
>>>    ovn-sbctl list controller_event
>>>    uuid=$(ovn-sbctl list controller_event | awk '/_uuid/{print $3}')
>>> @@ -14733,12 +14760,27 @@ AT_CHECK([ovn-sbctl get controller_event $uuid event_info:protocol], [0], [dnl
>>>    tcp
>>>    ])
>>>    AT_CHECK_UNQUOTED([ovn-sbctl get controller_event $uuid event_info:load_balancer], [0], [dnl
>>> -"$uuid_lb"
>>> +"$uuid_lb0"
>>>    ])
>>>    AT_CHECK([ovn-sbctl get controller_event $uuid seq_num], [0], [dnl
>>>    1
>>>    ])
>>>
>>> +ovn-sbctl destroy controller_event $uuid
>>> +packet1="inport==\"sw1-p0\" && eth.src==00:00:00:00:00:33 && eth.dst==00:00:00:00:ff:02 &&
>>> +         ip4 && ip.ttl==64 && ip4.src==192.168.2.11 && ip4.dst==192.168.2.100 &&
>>> +         tcp && tcp.src==10000 && tcp.dst==80"
>>> +
>>> +as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet1"
>>> +ovn-sbctl list controller_event
>>> +uuid=$(ovn-sbctl list controller_event | awk '/_uuid/{print $3}')
>>> +AT_CHECK([ovn-sbctl get controller_event $uuid event_type], [0], [dnl
>>> +empty_lb_backends
>>> +])
>>> +AT_CHECK([ovn-sbctl get controller_event $uuid event_info:vip], [0], [dnl
>>> +"192.168.2.100:80"
>>> +])
>>> +
>>>    OVN_CLEANUP([hv1], [hv2])
>>>    AT_CLEANUP
>>>
>>>
>>