[ovs-dev] controller: reconfigure ovs meters for ovn meters updates

At the moment ovs meters are reconfigured by ovn just when a
new a meter is allocated while updates for an already allocated meter are
ignored. This issue can be easily verified with the following reproducer:

$ovn-nbctl meter-add meter0 drop 10 pktps
$ovn-nbctl --log --meter=meter0 acl-add sw0 to-lport 1000 'tcp.dst == 80' drop
$ovn-nbctl --may-exist meter-add meter0 drop 20 pktps
$ovs-ofctl -O OpenFlow15 dump-meters br-int

Fix the issue reconfiguring ovs meters even for ovn meters updates.
Related bz: https://bugzilla.redhat.com/show_bug.cgi?id=1939524

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 controller/ofctrl.c         | 70 +++++++++++++++++++++++++++----------
 controller/ofctrl.h         |  2 ++
 controller/ovn-controller.c | 56 ++++++++++++++++++++++++++++-
 tests/system-ovn.at         | 17 +++++++++
 4 files changed, 126 insertions(+), 19 deletions(-)

On Thu, Nov 25, 2021 at 12:05 PM Lorenzo Bianconi
<lorenzo.bianconi@redhat.com> wrote:
>
> At the moment ovs meters are reconfigured by ovn just when a
> new a meter is allocated while updates for an already allocated meter are
> ignored. This issue can be easily verified with the following reproducer:
>
> $ovn-nbctl meter-add meter0 drop 10 pktps
> $ovn-nbctl --log --meter=meter0 acl-add sw0 to-lport 1000 'tcp.dst == 80' drop
> $ovn-nbctl --may-exist meter-add meter0 drop 20 pktps
> $ovs-ofctl -O OpenFlow15 dump-meters br-int
>
> Fix the issue reconfiguring ovs meters even for ovn meters updates.
> Related bz: https://bugzilla.redhat.com/show_bug.cgi?id=1939524
>
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>

Hi Lorenzo,

Thanks for the fix.  Please see below for a few comments.

Numan

> ---
>  controller/ofctrl.c         | 70 +++++++++++++++++++++++++++----------
>  controller/ofctrl.h         |  2 ++
>  controller/ovn-controller.c | 56 ++++++++++++++++++++++++++++-
>  tests/system-ovn.at         | 17 +++++++++
>  4 files changed, 126 insertions(+), 19 deletions(-)
>
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index 08fcfed8b..25c939fa3 100644
> --- a/controller/ofctrl.c
> +++ b/controller/ofctrl.c
> @@ -1802,26 +1802,13 @@ add_meter_string(struct ovn_extend_table_info *m_desired,
>  }
>
>  static void
> -add_meter(struct ovn_extend_table_info *m_desired,
> -          const struct sbrec_meter_table *meter_table,
> -          struct ovs_list *msgs)
> +alloc_meter_mod(struct ovn_extend_table_info *m,
> +                const struct sbrec_meter *sb_meter,
> +                struct ovs_list *msgs, bool update)
>  {
> -    const struct sbrec_meter *sb_meter;
> -    SBREC_METER_TABLE_FOR_EACH (sb_meter, meter_table) {
> -        if (!strcmp(m_desired->name, sb_meter->name)) {
> -            break;
> -        }
> -    }
> -
> -    if (!sb_meter) {
> -        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> -        VLOG_ERR_RL(&rl, "could not find meter named \"%s\"", m_desired->name);
> -        return;
> -    }
> -
>      struct ofputil_meter_mod mm;
> -    mm.command = OFPMC13_ADD;
> -    mm.meter.meter_id = m_desired->table_id;
> +    mm.command = update ? OFPMC13_MODIFY : OFPMC13_ADD;
> +    mm.meter.meter_id = m->table_id;
>      mm.meter.flags = OFPMF13_STATS;
>
>      if (!strcmp(sb_meter->unit, "pktps")) {
> @@ -1854,6 +1841,53 @@ add_meter(struct ovn_extend_table_info *m_desired,
>      free(mm.meter.bands);
>  }
>
> +void
> +update_meter(const struct sbrec_meter *meter)
> +{
> +    struct ovs_list msgs = OVS_LIST_INITIALIZER(&msgs);
> +    struct ovn_extend_table_info *m_iter;
> +
> +    HMAP_FOR_EACH (m_iter, hmap_node, &meters->desired) {
> +        if (!strcmp(meter->name, m_iter->name) &&
> +            ovn_extend_table_lookup(&meters->existing, m_iter)) {
> +            alloc_meter_mod(m_iter, meter, &msgs, true);
> +        }
> +    }
> +
> +    if (!ovs_list_is_empty(&msgs)) {
> +        /* Add a barrier to the list of messages. */
> +        struct ofpbuf *barrier = ofputil_encode_barrier_request(OFP15_VERSION);
> +
> +        ovs_list_push_back(&msgs, &barrier->list_node);
> +        /* Queue the messages. */
> +        struct ofpbuf *msg;
> +        LIST_FOR_EACH_POP (msg, list_node, &msgs) {
> +            queue_msg(msg);
> +        }
> +    }
> +}
> +
> +static void
> +add_meter(struct ovn_extend_table_info *m_desired,
> +          const struct sbrec_meter_table *meter_table,
> +          struct ovs_list *msgs)
> +{
> +    const struct sbrec_meter *sb_meter;
> +    SBREC_METER_TABLE_FOR_EACH (sb_meter, meter_table) {
> +        if (!strcmp(m_desired->name, sb_meter->name)) {
> +            break;
> +        }
> +    }
> +
> +    if (!sb_meter) {
> +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> +        VLOG_ERR_RL(&rl, "could not find meter named \"%s\"", m_desired->name);
> +        return;
> +    }
> +
> +    alloc_meter_mod(m_desired, sb_meter, msgs, false);
> +}
> +
>  static void
>  installed_flow_add(struct ovn_flow *d,
>                     struct ofputil_bundle_ctrl_msg *bc,
> diff --git a/controller/ofctrl.h b/controller/ofctrl.h
> index 014de210d..60eeb9dbb 100644
> --- a/controller/ofctrl.h
> +++ b/controller/ofctrl.h
> @@ -29,6 +29,7 @@ struct match;
>  struct ofpbuf;
>  struct ovsrec_bridge;
>  struct sbrec_meter_table;
> +struct sbrec_meter;
>  struct shash;
>
>  struct ovn_desired_flow_table {
> @@ -129,5 +130,6 @@ void ofctrl_check_and_add_flow_metered(struct ovn_desired_flow_table *,
>  bool ofctrl_is_connected(void);
>  void ofctrl_set_probe_interval(int probe_interval);
>  void ofctrl_get_memory_usage(struct simap *usage);
> +void update_meter(const struct sbrec_meter *meter);
>
>  #endif /* controller/ofctrl.h */
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 29c1a05b2..f2fa6aee8 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -76,6 +76,7 @@
>  #include "stopwatch.h"
>  #include "lib/inc-proc-eng.h"
>  #include "hmapx.h"
> +#include "openvswitch/ofp-util.h"
>
>  VLOG_DEFINE_THIS_MODULE(main);
>
> @@ -957,7 +958,8 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl)
>      SB_NODE(dhcpv6_options, "dhcpv6_options") \
>      SB_NODE(dns, "dns") \
>      SB_NODE(load_balancer, "load_balancer") \
> -    SB_NODE(fdb, "fdb")
> +    SB_NODE(fdb, "fdb") \
> +    SB_NODE(meter, "meter")
>
>  enum sb_engine_node {
>  #define SB_NODE(NAME, NAME_STR) SB_##NAME,
> @@ -1500,6 +1502,55 @@ addr_sets_sb_address_set_handler(struct engine_node *node, void *data)
>      return true;
>  }
>
> +struct ed_type_meter {
> +    bool change_tracked;
> +};
> +
> +static void *
> +en_meter_init(struct engine_node *node OVS_UNUSED,
> +              struct engine_arg *arg OVS_UNUSED)
> +{
> +    struct ed_type_meter *m = xzalloc(sizeof *m);
> +
> +    m->change_tracked = false;
> +    return m;
> +}
> +
> +static void
> +en_meter_cleanup(void *data OVS_UNUSED)
> +{
> +}
> +
> +static void
> +en_meter_run(struct engine_node *node, void *data)
> +{
> +    struct ed_type_meter *m = data;
> +
> +    engine_set_node_state(node, EN_UPDATED);
> +    m->change_tracked = false;
> +}
> +
> +static bool
> +meter_sb_meter_handler(struct engine_node *node, void *data)
> +{
> +    struct ovs_list msgs = OVS_LIST_INITIALIZER(&msgs);
> +    struct ed_type_meter *m = data;
> +
> +    struct sbrec_meter_table *m_table =
> +        (struct sbrec_meter_table *)EN_OVSDB_GET(
> +            engine_get_input("SB_meter", node));
> +
> +    const struct sbrec_meter *iter;
> +    SBREC_METER_TABLE_FOR_EACH_TRACKED (iter, m_table) {
> +        if (!sbrec_meter_is_deleted(iter) && !sbrec_meter_is_new(iter)) {
> +            update_meter(iter);
> +        }
> +    }
> +    m->change_tracked = true;
> +
> +    return true;
> +}
> +
>  struct ed_type_port_groups{
>      /* A copy of SB port_groups, each converted as a sset for efficient lport
>       * lookup. */
> @@ -3202,6 +3253,7 @@ main(int argc, char *argv[])
>      ENGINE_NODE(lflow_output, "logical_flow_output");
>      ENGINE_NODE(flow_output, "flow_output");
>      ENGINE_NODE(addr_sets, "addr_sets");
> +    ENGINE_NODE(meter, "meter");
>      ENGINE_NODE_WITH_CLEAR_TRACK_DATA(port_groups, "port_groups");
>
>  #define SB_NODE(NAME, NAME_STR) ENGINE_NODE_SB(NAME, NAME_STR);
> @@ -3216,6 +3268,7 @@ main(int argc, char *argv[])
>
>      engine_add_input(&en_addr_sets, &en_sb_address_set,
>                       addr_sets_sb_address_set_handler);
> +    engine_add_input(&en_meter, &en_sb_meter, meter_sb_meter_handler);

What is the reason to add a new meter engine node ?


>      engine_add_input(&en_port_groups, &en_sb_port_group,
>                       port_groups_sb_port_group_handler);
>      /* port_groups computation requires runtime_data's lbinding_data for the
> @@ -3289,6 +3342,7 @@ main(int argc, char *argv[])
>                       lflow_output_sb_load_balancer_handler);
>      engine_add_input(&en_lflow_output, &en_sb_fdb,
>                       lflow_output_sb_fdb_handler);
> +    engine_add_input(&en_lflow_output, &en_meter, NULL);

Instead of adding a new engine node,  you could have just added a
handler here i.e en_lflow_output_meter_handler().

What is the reason ?  Is it because when a sb meter is updated along
with some other sb change ( which results in a full
recompute) then the OF flow for the updated meter is not MODIFIED ?

If that is so,  then I think your approach makes sense.

Can you please add a test case scenario where in ovn-controller has to
process a sb meter update and an another update (which
results in full recompute, for example a chassis is added)  in the
same loop and the meter flow is updated accordingly ?

Numan

>
>      engine_add_input(&en_ct_zones, &en_ovs_open_vswitch, NULL);
>      engine_add_input(&en_ct_zones, &en_ovs_bridge, NULL);
> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> index 7f6cb32dc..80d5192ca 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -6667,8 +6667,25 @@ OVS_WAIT_UNTIL([
>      test "${n_reject}" = "2"
>  ])
>  kill $(pidof tcpdump)
> +rm -f reject.pcap
> +
> +# Let's update the meter
> +NS_EXEC([sw01], [tcpdump -l -n -i sw01 icmp -Q in > reject.pcap &])
> +check ovn-nbctl --may-exist meter-add acl-meter drop 10 pktps 0
> +ip netns exec sw01 scapy -H <<-EOF
> +p = IP(src="192.168.1.2", dst="192.168.1.1") / UDP(dport = 12345) / Raw(b"X"*64)
> +send (p, iface='sw01', loop = 0, verbose = 0, count = 100)
> +EOF
>
> +# 10pps + 1 burst size
> +OVS_WAIT_UNTIL([
> +    n_reject=$(grep unreachable reject.pcap | wc -l)
> +    test "${n_reject}" = "20"
> +])
> +
> +kill $(pidof tcpdump)
>  rm -f reject.pcap
> +
>  NS_EXEC([sw01], [tcpdump -l -n -i sw01 icmp -Q in > reject.pcap &])
>  check ovn-nbctl --wait=hv ls-copp-del sw0 reject
>
> --
> 2.31.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

> On Thu, Nov 25, 2021 at 12:05 PM Lorenzo Bianconi
> <lorenzo.bianconi@redhat.com> wrote:
> >
> > At the moment ovs meters are reconfigured by ovn just when a
> > new a meter is allocated while updates for an already allocated meter are
> > ignored. This issue can be easily verified with the following reproducer:
> >
> > $ovn-nbctl meter-add meter0 drop 10 pktps
> > $ovn-nbctl --log --meter=meter0 acl-add sw0 to-lport 1000 'tcp.dst == 80' drop
> > $ovn-nbctl --may-exist meter-add meter0 drop 20 pktps
> > $ovs-ofctl -O OpenFlow15 dump-meters br-int
> >
> > Fix the issue reconfiguring ovs meters even for ovn meters updates.
> > Related bz: https://bugzilla.redhat.com/show_bug.cgi?id=1939524
> >
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> 
> Hi Lorenzo,
> 
> Thanks for the fix.  Please see below for a few comments.

Hi Numan,

thx for review. I posted a RFC series to introduce a dedicated IP engine for ovn meters
(doing so we remove possible issues due to a full recompute).

https://patchwork.ozlabs.org/project/ovn/cover/cover.1638574680.git.lorenzo.bianconi@redhat.com/

Regards,
Lorenzo


> 
> Numan
> 
> > ---
> >  controller/ofctrl.c         | 70 +++++++++++++++++++++++++++----------
> >  controller/ofctrl.h         |  2 ++
> >  controller/ovn-controller.c | 56 ++++++++++++++++++++++++++++-
> >  tests/system-ovn.at         | 17 +++++++++
> >  4 files changed, 126 insertions(+), 19 deletions(-)
> >
> > diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> > index 08fcfed8b..25c939fa3 100644
> > --- a/controller/ofctrl.c
> > +++ b/controller/ofctrl.c
> > @@ -1802,26 +1802,13 @@ add_meter_string(struct ovn_extend_table_info *m_desired,
> >  }
> >
> >  static void
> > -add_meter(struct ovn_extend_table_info *m_desired,
> > -          const struct sbrec_meter_table *meter_table,
> > -          struct ovs_list *msgs)
> > +alloc_meter_mod(struct ovn_extend_table_info *m,
> > +                const struct sbrec_meter *sb_meter,
> > +                struct ovs_list *msgs, bool update)
> >  {
> > -    const struct sbrec_meter *sb_meter;
> > -    SBREC_METER_TABLE_FOR_EACH (sb_meter, meter_table) {
> > -        if (!strcmp(m_desired->name, sb_meter->name)) {
> > -            break;
> > -        }
> > -    }
> > -
> > -    if (!sb_meter) {
> > -        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> > -        VLOG_ERR_RL(&rl, "could not find meter named \"%s\"", m_desired->name);
> > -        return;
> > -    }
> > -
> >      struct ofputil_meter_mod mm;
> > -    mm.command = OFPMC13_ADD;
> > -    mm.meter.meter_id = m_desired->table_id;
> > +    mm.command = update ? OFPMC13_MODIFY : OFPMC13_ADD;
> > +    mm.meter.meter_id = m->table_id;
> >      mm.meter.flags = OFPMF13_STATS;
> >
> >      if (!strcmp(sb_meter->unit, "pktps")) {
> > @@ -1854,6 +1841,53 @@ add_meter(struct ovn_extend_table_info *m_desired,
> >      free(mm.meter.bands);
> >  }
> >
> > +void
> > +update_meter(const struct sbrec_meter *meter)
> > +{
> > +    struct ovs_list msgs = OVS_LIST_INITIALIZER(&msgs);
> > +    struct ovn_extend_table_info *m_iter;
> > +
> > +    HMAP_FOR_EACH (m_iter, hmap_node, &meters->desired) {
> > +        if (!strcmp(meter->name, m_iter->name) &&
> > +            ovn_extend_table_lookup(&meters->existing, m_iter)) {
> > +            alloc_meter_mod(m_iter, meter, &msgs, true);
> > +        }
> > +    }
> > +
> > +    if (!ovs_list_is_empty(&msgs)) {
> > +        /* Add a barrier to the list of messages. */
> > +        struct ofpbuf *barrier = ofputil_encode_barrier_request(OFP15_VERSION);
> > +
> > +        ovs_list_push_back(&msgs, &barrier->list_node);
> > +        /* Queue the messages. */
> > +        struct ofpbuf *msg;
> > +        LIST_FOR_EACH_POP (msg, list_node, &msgs) {
> > +            queue_msg(msg);
> > +        }
> > +    }
> > +}
> > +
> > +static void
> > +add_meter(struct ovn_extend_table_info *m_desired,
> > +          const struct sbrec_meter_table *meter_table,
> > +          struct ovs_list *msgs)
> > +{
> > +    const struct sbrec_meter *sb_meter;
> > +    SBREC_METER_TABLE_FOR_EACH (sb_meter, meter_table) {
> > +        if (!strcmp(m_desired->name, sb_meter->name)) {
> > +            break;
> > +        }
> > +    }
> > +
> > +    if (!sb_meter) {
> > +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> > +        VLOG_ERR_RL(&rl, "could not find meter named \"%s\"", m_desired->name);
> > +        return;
> > +    }
> > +
> > +    alloc_meter_mod(m_desired, sb_meter, msgs, false);
> > +}
> > +
> >  static void
> >  installed_flow_add(struct ovn_flow *d,
> >                     struct ofputil_bundle_ctrl_msg *bc,
> > diff --git a/controller/ofctrl.h b/controller/ofctrl.h
> > index 014de210d..60eeb9dbb 100644
> > --- a/controller/ofctrl.h
> > +++ b/controller/ofctrl.h
> > @@ -29,6 +29,7 @@ struct match;
> >  struct ofpbuf;
> >  struct ovsrec_bridge;
> >  struct sbrec_meter_table;
> > +struct sbrec_meter;
> >  struct shash;
> >
> >  struct ovn_desired_flow_table {
> > @@ -129,5 +130,6 @@ void ofctrl_check_and_add_flow_metered(struct ovn_desired_flow_table *,
> >  bool ofctrl_is_connected(void);
> >  void ofctrl_set_probe_interval(int probe_interval);
> >  void ofctrl_get_memory_usage(struct simap *usage);
> > +void update_meter(const struct sbrec_meter *meter);
> >
> >  #endif /* controller/ofctrl.h */
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index 29c1a05b2..f2fa6aee8 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -76,6 +76,7 @@
> >  #include "stopwatch.h"
> >  #include "lib/inc-proc-eng.h"
> >  #include "hmapx.h"
> > +#include "openvswitch/ofp-util.h"
> >
> >  VLOG_DEFINE_THIS_MODULE(main);
> >
> > @@ -957,7 +958,8 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl)
> >      SB_NODE(dhcpv6_options, "dhcpv6_options") \
> >      SB_NODE(dns, "dns") \
> >      SB_NODE(load_balancer, "load_balancer") \
> > -    SB_NODE(fdb, "fdb")
> > +    SB_NODE(fdb, "fdb") \
> > +    SB_NODE(meter, "meter")
> >
> >  enum sb_engine_node {
> >  #define SB_NODE(NAME, NAME_STR) SB_##NAME,
> > @@ -1500,6 +1502,55 @@ addr_sets_sb_address_set_handler(struct engine_node *node, void *data)
> >      return true;
> >  }
> >
> > +struct ed_type_meter {
> > +    bool change_tracked;
> > +};
> > +
> > +static void *
> > +en_meter_init(struct engine_node *node OVS_UNUSED,
> > +              struct engine_arg *arg OVS_UNUSED)
> > +{
> > +    struct ed_type_meter *m = xzalloc(sizeof *m);
> > +
> > +    m->change_tracked = false;
> > +    return m;
> > +}
> > +
> > +static void
> > +en_meter_cleanup(void *data OVS_UNUSED)
> > +{
> > +}
> > +
> > +static void
> > +en_meter_run(struct engine_node *node, void *data)
> > +{
> > +    struct ed_type_meter *m = data;
> > +
> > +    engine_set_node_state(node, EN_UPDATED);
> > +    m->change_tracked = false;
> > +}
> > +
> > +static bool
> > +meter_sb_meter_handler(struct engine_node *node, void *data)
> > +{
> > +    struct ovs_list msgs = OVS_LIST_INITIALIZER(&msgs);
> > +    struct ed_type_meter *m = data;
> > +
> > +    struct sbrec_meter_table *m_table =
> > +        (struct sbrec_meter_table *)EN_OVSDB_GET(
> > +            engine_get_input("SB_meter", node));
> > +
> > +    const struct sbrec_meter *iter;
> > +    SBREC_METER_TABLE_FOR_EACH_TRACKED (iter, m_table) {
> > +        if (!sbrec_meter_is_deleted(iter) && !sbrec_meter_is_new(iter)) {
> > +            update_meter(iter);
> > +        }
> > +    }
> > +    m->change_tracked = true;
> > +
> > +    return true;
> > +}
> > +
> >  struct ed_type_port_groups{
> >      /* A copy of SB port_groups, each converted as a sset for efficient lport
> >       * lookup. */
> > @@ -3202,6 +3253,7 @@ main(int argc, char *argv[])
> >      ENGINE_NODE(lflow_output, "logical_flow_output");
> >      ENGINE_NODE(flow_output, "flow_output");
> >      ENGINE_NODE(addr_sets, "addr_sets");
> > +    ENGINE_NODE(meter, "meter");
> >      ENGINE_NODE_WITH_CLEAR_TRACK_DATA(port_groups, "port_groups");
> >
> >  #define SB_NODE(NAME, NAME_STR) ENGINE_NODE_SB(NAME, NAME_STR);
> > @@ -3216,6 +3268,7 @@ main(int argc, char *argv[])
> >
> >      engine_add_input(&en_addr_sets, &en_sb_address_set,
> >                       addr_sets_sb_address_set_handler);
> > +    engine_add_input(&en_meter, &en_sb_meter, meter_sb_meter_handler);
> 
> What is the reason to add a new meter engine node ?
> 
> 
> >      engine_add_input(&en_port_groups, &en_sb_port_group,
> >                       port_groups_sb_port_group_handler);
> >      /* port_groups computation requires runtime_data's lbinding_data for the
> > @@ -3289,6 +3342,7 @@ main(int argc, char *argv[])
> >                       lflow_output_sb_load_balancer_handler);
> >      engine_add_input(&en_lflow_output, &en_sb_fdb,
> >                       lflow_output_sb_fdb_handler);
> > +    engine_add_input(&en_lflow_output, &en_meter, NULL);
> 
> Instead of adding a new engine node,  you could have just added a
> handler here i.e en_lflow_output_meter_handler().
> 
> What is the reason ?  Is it because when a sb meter is updated along
> with some other sb change ( which results in a full
> recompute) then the OF flow for the updated meter is not MODIFIED ?
> 
> If that is so,  then I think your approach makes sense.
> 
> Can you please add a test case scenario where in ovn-controller has to
> process a sb meter update and an another update (which
> results in full recompute, for example a chassis is added)  in the
> same loop and the meter flow is updated accordingly ?
> 
> Numan
> 
> >
> >      engine_add_input(&en_ct_zones, &en_ovs_open_vswitch, NULL);
> >      engine_add_input(&en_ct_zones, &en_ovs_bridge, NULL);
> > diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> > index 7f6cb32dc..80d5192ca 100644
> > --- a/tests/system-ovn.at
> > +++ b/tests/system-ovn.at
> > @@ -6667,8 +6667,25 @@ OVS_WAIT_UNTIL([
> >      test "${n_reject}" = "2"
> >  ])
> >  kill $(pidof tcpdump)
> > +rm -f reject.pcap
> > +
> > +# Let's update the meter
> > +NS_EXEC([sw01], [tcpdump -l -n -i sw01 icmp -Q in > reject.pcap &])
> > +check ovn-nbctl --may-exist meter-add acl-meter drop 10 pktps 0
> > +ip netns exec sw01 scapy -H <<-EOF
> > +p = IP(src="192.168.1.2", dst="192.168.1.1") / UDP(dport = 12345) / Raw(b"X"*64)
> > +send (p, iface='sw01', loop = 0, verbose = 0, count = 100)
> > +EOF
> >
> > +# 10pps + 1 burst size
> > +OVS_WAIT_UNTIL([
> > +    n_reject=$(grep unreachable reject.pcap | wc -l)
> > +    test "${n_reject}" = "20"
> > +])
> > +
> > +kill $(pidof tcpdump)
> >  rm -f reject.pcap
> > +
> >  NS_EXEC([sw01], [tcpdump -l -n -i sw01 icmp -Q in > reject.pcap &])
> >  check ovn-nbctl --wait=hv ls-copp-del sw0 reject
> >
> > --
> > 2.31.1
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>