[ovs-dev,v8] controller: reconfigure ovs meters for ovn meters

At the moment ovs meters are reconfigured by ovn just when a
meter is allocated or removed 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

In order to fix the issue introduce SB_meter node in the incremetal
processing engine and add it as input for lflow_output one.
Sync OVS meters in ofctl_put() to push changes in SB meter/SB meter
bands tables.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1939524
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
Changes since v7:
- implement full-compare support in ofctrl module
- drop SB_meter IP management

Changes since v6:
- move ovs sync code back to ofctrl_put()
- remove meter IP node and link SB_meter node to lflow_output one
- add new unit-tests

Changes since v5:
- add size parameter to extend_table in order to reduce the size

Changes since v4:
- add offset parameter to ovn_extend_table_init
- rebase on top of ovn master

Changes since v3:
- move full meter management in the IP engine

Changes since v2:
- move meter ip into lflow ip
- add new test in ovn-performance.at
- manage metering ip full-recompute

Changes since v1:
- add IP refactor to the series
- rebase on top of ovn master
---
 controller/ofctrl.c         | 200 +++++++++++++++++++++++++++++++-----
 controller/ovn-controller.c |  12 ++-
 tests/ovn-performance.at    |  22 ++++
 tests/ovn.at                |  74 +++++++++++++
 tests/system-ovn.at         |  17 +++
 5 files changed, 301 insertions(+), 24 deletions(-)

On Fri, Mar 4, 2022 at 2:54 PM Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
wrote:
>
> At the moment ovs meters are reconfigured by ovn just when a
> meter is allocated or removed 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
>
> In order to fix the issue introduce SB_meter node in the incremetal
> processing engine and add it as input for lflow_output one.
> Sync OVS meters in ofctl_put() to push changes in SB meter/SB meter
> bands tables.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1939524
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>

Thanks Lorenzo. This looks much simpler and good overall. I have some minor
suggestions inlined.

> ---
> Changes since v7:
> - implement full-compare support in ofctrl module
> - drop SB_meter IP management
>
> Changes since v6:
> - move ovs sync code back to ofctrl_put()
> - remove meter IP node and link SB_meter node to lflow_output one
> - add new unit-tests
>
> Changes since v5:
> - add size parameter to extend_table in order to reduce the size
>
> Changes since v4:
> - add offset parameter to ovn_extend_table_init
> - rebase on top of ovn master
>
> Changes since v3:
> - move full meter management in the IP engine
>
> Changes since v2:
> - move meter ip into lflow ip
> - add new test in ovn-performance.at
> - manage metering ip full-recompute
>
> Changes since v1:
> - add IP refactor to the series
> - rebase on top of ovn master
> ---
>  controller/ofctrl.c         | 200 +++++++++++++++++++++++++++++++-----
>  controller/ovn-controller.c |  12 ++-
>  tests/ovn-performance.at    |  22 ++++
>  tests/ovn.at                |  74 +++++++++++++
>  tests/system-ovn.at         |  17 +++
>  5 files changed, 301 insertions(+), 24 deletions(-)
>
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index 19aa787f9..5b4c823ef 100644
> --- a/controller/ofctrl.c
> +++ b/controller/ofctrl.c
> @@ -360,6 +360,21 @@ static struct ovn_extend_table *groups;
>  /* A reference to the meter_table. */
>  static struct ovn_extend_table *meters;
>
> +/* Installed meter bands. */
> +struct meter_band_data {
> +    int64_t burst_size;
> +    int64_t rate;
> +};
> +
> +struct meter_band_entry {
> +    struct meter_band_data *bands;
> +    size_t n_bands;
> +};
> +
> +static struct shash meter_bands;
> +
> +static void ofctrl_meter_bands_destroy(void);
> +
>  /* MFF_* field ID for our Geneve option.  In S_TLV_TABLE_MOD_SENT, this
is
>   * the option we requested (we don't know whether we obtained it yet).
In
>   * S_CLEAR_FLOWS or S_UPDATE_FLOWS, this is really the option we have. */
> @@ -397,6 +412,7 @@ ofctrl_init(struct ovn_extend_table *group_table,
>      ovn_init_symtab(&symtab);
>      groups = group_table;
>      meters = meter_table;
> +    shash_init(&meter_bands);
>  }
>
>  /* S_NEW, for a new connection.
> @@ -814,6 +830,7 @@ ofctrl_destroy(void)
>      rconn_packet_counter_destroy(tx_counter);
>      expr_symtab_destroy(&symtab);
>      shash_destroy(&symtab);
> +    ofctrl_meter_bands_destroy();
>  }
>
>  uint64_t
> @@ -1979,26 +1996,14 @@ 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)
> +update_ovs_meter(struct ovn_extend_table_info *entry,
> +                 const struct sbrec_meter *sb_meter, int cmd,
> +                 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;
> -    }
>
>      struct ofputil_meter_mod mm;
> -    mm.command = OFPMC13_ADD;
> -    mm.meter.meter_id = m_desired->table_id;
> +    mm.command = cmd;
> +    mm.meter.meter_id = entry->table_id;
>      mm.meter.flags = OFPMF13_STATS;
>
>      if (!strcmp(sb_meter->unit, "pktps")) {
> @@ -2031,6 +2036,158 @@ add_meter(struct ovn_extend_table_info *m_desired,
>      free(mm.meter.bands);
>  }
>
> +static void
> +ofctrl_meter_bands_destroy(void)
> +{
> +    struct shash_node *node, *next;
> +    SHASH_FOR_EACH_SAFE (node, next, &meter_bands) {
> +        struct meter_band_entry *mb = node->data;
> +        shash_delete(&meter_bands, node);
> +        free(mb->bands);
> +        free(mb);
> +    }
> +    shash_destroy(&meter_bands);
> +}
> +
> +static bool
> +ofctrl_meter_bands_check(const struct sbrec_meter *sb_meter,
> +                         struct meter_band_entry *mb)

It would be better to rename the function to "...is_equal" (and reverse the
return value), otherwise add comments to tell what the return value means.

> +{
> +    if (mb->n_bands != sb_meter->n_bands) {
> +        return true;
> +    }
> +
> +    for (int i = 0; i < sb_meter->n_bands; i++) {
> +        int j;
> +        for (j = 0; j < mb->n_bands; j++) {
> +            if (sb_meter->bands[i]->rate == mb->bands[j].rate &&
> +                sb_meter->bands[i]->burst_size ==
mb->bands[j].burst_size) {
> +                break;
> +            }
> +        }
> +        if (j == mb->n_bands) {
> +            return true;
> +        }
> +    }
> +    return false;
> +}
> +
> +static void
> +ofctrl_meter_bands_alloc(const struct sbrec_meter *sb_meter,
> +                         struct ovn_extend_table_info *entry,
> +                         struct ovs_list *msgs)
> +{
> +    struct meter_band_entry *mb = mb = xzalloc(sizeof *mb);
> +    mb->n_bands = sb_meter->n_bands;
> +    mb->bands = xcalloc(mb->n_bands, sizeof *mb->bands);
> +    for (int i = 0; i < sb_meter->n_bands; i++) {
> +        mb->bands[i].rate = sb_meter->bands[i]->rate;
> +        mb->bands[i].burst_size = sb_meter->bands[i]->burst_size;
> +    }
> +    shash_add(&meter_bands, entry->name, mb);
> +    update_ovs_meter(entry, sb_meter, OFPMC13_ADD, msgs);
> +}
> +
> +static void
> +ofctrl_meter_bands_update(const struct sbrec_meter *sb_meter,
> +                          struct ovn_extend_table_info *entry,
> +                          struct ovs_list *msgs)
> +{
> +    struct meter_band_entry *mb =
> +        shash_find_data(&meter_bands, entry->name);
> +    if (!mb) {
> +        ofctrl_meter_bands_alloc(sb_meter, entry, msgs);
> +        return;
> +    }
> +
> +    if (!ofctrl_meter_bands_check(sb_meter, mb)) {
> +        return;
> +    }
> +
> +    free(mb->bands);
> +    mb->n_bands = sb_meter->n_bands;
> +    mb->bands = xcalloc(mb->n_bands, sizeof *mb->bands);
> +    for (int i = 0; i < sb_meter->n_bands; i++) {
> +        mb->bands[i].rate = sb_meter->bands[i]->rate;
> +        mb->bands[i].burst_size = sb_meter->bands[i]->burst_size;
> +    }
> +
> +    update_ovs_meter(entry, sb_meter, OFPMC13_MODIFY, msgs);
> +}
> +
> +static void
> +ofctrl_meter_bands_erase(struct ovn_extend_table_info *entry,
> +                         struct ovs_list *msgs)
> +{
> +    struct meter_band_entry *mb =
> +        shash_find_and_delete(&meter_bands, entry->name);
> +    if (mb) {
> +        /* Delete the meter. */
> +        struct ofputil_meter_mod mm = {
> +            .command = OFPMC13_DELETE,
> +            .meter = { .meter_id = entry->table_id },
> +        };
> +        add_meter_mod(&mm, msgs);
> +
> +        free(mb->bands);
> +        free(mb);
> +    }
> +}
> +
> +static void
> +ofctrl_meter_bands_sync(const struct sbrec_meter_table *meter_table,
> +                        struct ovs_list *msgs)
> +{
> +    struct ovn_extend_table_info *m_desired;
> +    HMAP_FOR_EACH (m_desired, hmap_node, &meters->desired) {
> +        struct ovn_extend_table_info *m_existing =
> +            ovn_extend_table_lookup(&meters->existing, m_desired);
> +        if (!m_existing) {
> +            /* Here we are assuming desired and existing are always
aligned.
> +             * ofctrl_meter_bands_sync() must run after
> +             * ovn_extend_table_sync() on meter extend table.
> +             */

Thanks for the comment. I can see why this function requires desired and
existing are in sync, but if they are in sync, this check should never be
false, in which case we don't need the check, or just assert it.
However, I think this tricky point can be avoided. This is supposed to
compare the existing meters with the desired state, and update to OVS if
there is any difference. Would it be better to move this
"compare-and-update" logic to the place when new meters are checked and
added instead of calling this function after the ovn_extend_table_sync()?
We can change the loop "EXTEND_TABLE_FOR_EACH_UNINSTALLED" to:

HMAP_FOR_EACH (..., desired) {
    if (NOT found in existing) {
        add_meter_string or add_meter;
    } else {
        compare_and_update; /* what's performed by the
ofctrl_meter_bands_sync() function but for just one meter. */
    }
}

In addition, there are some wasted checks when this function is called
after ovn_extend_table_sync(), because the newly added meters are now
indistinguishable from the existing meters, and they will be compared with
SB data again. The above pseudo code would avoid this waste, too. What do
you think?

Thanks,
Han

> +            continue;
> +        }
> +
> +        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) {
> +            /* OFPMC13_ADD or OFPMC13_MODIFY */
> +            ofctrl_meter_bands_update(sb_meter, m_existing, msgs);
> +        } else {
> +            /* OFPMC13_DELETE */
> +            ofctrl_meter_bands_erase(m_existing, msgs);
> +        }
> +    }
> +}
> +
> +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;
> +    }
> +
> +    ofctrl_meter_bands_alloc(sb_meter, m_desired, msgs);
> +}
> +
>  static void
>  installed_flow_add(struct ovn_flow *d,
>                     struct ofputil_bundle_ctrl_msg *bc,
> @@ -2505,17 +2662,14 @@ ofctrl_put(struct ovn_desired_flow_table
*lflow_table,
>      struct ovn_extend_table_info *m_installed, *next_meter;
>      EXTEND_TABLE_FOR_EACH_INSTALLED (m_installed, next_meter, meters) {
>          /* Delete the meter. */
> -        struct ofputil_meter_mod mm = {
> -            .command = OFPMC13_DELETE,
> -            .meter = { .meter_id = m_installed->table_id },
> -        };
> -        add_meter_mod(&mm, &msgs);
> -
> +        ofctrl_meter_bands_erase(m_installed, &msgs);
>          ovn_extend_table_remove_existing(meters, m_installed);
>      }
>
>      /* Sync the contents of meters->desired to meters->existing. */
>      ovn_extend_table_sync(meters);
> +    /* Sync installed meter bands. */
> +    ofctrl_meter_bands_sync(meter_table, &msgs);
>
>      if (!ovs_list_is_empty(&msgs)) {
>          /* Add a barrier to the list of messages. */
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index c09018243..6f0c095c9 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -968,7 +968,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,
> @@ -2718,6 +2719,13 @@ lflow_output_sb_fdb_handler(struct engine_node
*node, void *data)
>      return handled;
>  }
>
> +static bool
> +lflow_output_sb_meter_handler(struct engine_node *node, void *data
OVS_UNUSED)
> +{
> +    engine_set_node_state(node, EN_UPDATED);
> +    return true;
> +}
> +
>  struct ed_type_pflow_output {
>      /* Desired physical flows. */
>      struct ovn_desired_flow_table flow_table;
> @@ -3316,6 +3324,8 @@ 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_sb_meter,
> +                     lflow_output_sb_meter_handler);
>
>      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/ovn-performance.at b/tests/ovn-performance.at
> index 10341ad72..bc133f93b 100644
> --- a/tests/ovn-performance.at
> +++ b/tests/ovn-performance.at
> @@ -543,6 +543,28 @@ OVN_CONTROLLER_EXPECT_HIT(
>      [as hv3 ovs-vsctl set interface vgw3
external-ids:ovn-egress-iface=true]
>  )
>
> +ovn-nbctl --wait=hv meter-add meter0 drop 100 pktps 10
> +
> +OVN_CONTROLLER_EXPECT_NO_HIT(
> +    [hv1 hv2 hv3 hv4], [lflow_run],
> +    [ovn-nbctl --wait=hv copp-add copp0 arp meter0]
> +)
> +
> +OVN_CONTROLLER_EXPECT_NO_HIT(
> +    [hv1 hv2 hv3 hv4], [lflow_run],
> +    [ovn-nbctl --wait=hv lr-copp-add copp0 lr1]
> +)
> +
> +OVN_CONTROLLER_EXPECT_NO_HIT(
> +    [hv1 hv2 hv3 hv4], [lflow_run],
> +    [ovn-nbctl --wait=hv --may-exist meter-add meter0 drop 200 pktps 10]
> +)
> +
> +OVN_CONTROLLER_EXPECT_NO_HIT(
> +    [hv1 hv2 hv3 hv4], [lflow_run],
> +    [ovn-nbctl --wait=hv meter-del meter0]
> +)
> +
>  for i in 1 2; do
>      j=$((i%2 + 1))
>      lp=lp$i
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 69270601a..d5ecb3fb4 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -29744,3 +29744,77 @@ OVS_WAIT_UNTIL([
>  OVN_CLEANUP([hv1],[hv2])
>  AT_CLEANUP
>  ])
> +
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([ovn-controller - check meters update])
> +AT_KEYWORDS([meters-update])
> +
> +ovn_start
> +
> +net_add n1
> +sim_add hv1
> +as hv1
> +ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.10
> +
> +check ovn-nbctl ls-add sw0
> +check ovn-nbctl lsp-add sw0 lsp
> +
> +as hv1 ovs-vsctl \
> +    -- add-port br-int vif1 \
> +    -- set Interface vif1 external_ids:iface-id=lsp
> +
> +# Wait for port to be bound.
> +wait_row_count Chassis 1 name=hv1
> +ch=$(fetch_column Chassis _uuid name=hv1)
> +wait_row_count Port_Binding 1 logical_port=lsp chassis=$ch
> +
> +# Add a new meter
> +check ovn-nbctl --event lb-add lb0 192.168.1.100:80 ""
> +check ovn-nbctl ls-lb-add sw0 lb0
> +check ovn-nbctl meter-add meter0 drop 10 pktps
> +ovn-nbctl --wait=hv copp-add copp0 event-elb meter0
> +ovn-nbctl --wait=hv ls-copp-add copp0 sw0
> +check ovn-nbctl --wait=hv sync
> +
> +AT_CHECK([as hv1 ovs-ofctl -OOpenFlow15 dump-meters br-int | grep -q
rate=10], [0])
> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep -q meter_id=1], [0])
> +
> +# Update existing meter
> +check ovn-nbctl --may-exist meter-add meter0 drop 20 pktps
> +check ovn-nbctl --wait=hv sync
> +
> +AT_CHECK([as hv1 ovs-ofctl -OOpenFlow15 dump-meters br-int | grep -q
rate=20], [0])
> +
> +# Add a new meter
> +check ovn-nbctl meter-add meter1 drop 30 pktps
> +check ovn-nbctl --log --severity=alert --meter=meter1 \
> +                --name=dns acl-add sw0 to-lport 1000 'udp.dst == 53' drop
> +check ovn-nbctl --wait=hv sync
> +AT_CHECK([as hv1 ovs-ofctl -OOpenFlow15 dump-meters br-int | grep -q
rate=30], [0])
> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep -q meter_id=2], [0])
> +
> +# Remove meter0
> +check ovn-nbctl meter-del meter0
> +check ovn-nbctl --wait=hv sync
> +AT_CHECK([as hv1 ovs-ofctl -OOpenFlow15 dump-meters br-int | grep -q
rate=10], [1])
> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep -q meter_id=1], [1])
> +
> +check ovn-nbctl meter-del meter1
> +check ovn-nbctl --wait=hv sync
> +AT_CHECK([as hv1 ovs-ofctl -OOpenFlow15 dump-meters br-int | grep -q
rate=30], [1])
> +
> +# create meters in the opposite order
> +check ovn-nbctl --log --severity=alert --meter=meter2 \
> +                --name=dns acl-add sw0 to-lport 1000 'tcp.dst == 80' drop
> +check ovn-nbctl meter-add meter2 drop 100 pktps
> +check ovn-nbctl --wait=hv sync
> +AT_CHECK([as hv1 ovs-ofctl -OOpenFlow15 dump-meters br-int | grep -q
rate=100], [0])
> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep -q meter_id=1], [0])
> +
> +check ovn-nbctl meter-del meter2
> +AT_CHECK([as hv1 ovs-ofctl -OOpenFlow15 dump-meters br-int | grep -q
rate=100], [1])
> +
> +OVN_CLEANUP([hv1])
> +AT_CLEANUP
> +])
> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> index f57d752d4..d1770408e 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -6661,8 +6661,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 copp-del copp0 reject
>
> --
> 2.35.1
>

> On Fri, Mar 4, 2022 at 2:54 PM Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> wrote:
> >
> > At the moment ovs meters are reconfigured by ovn just when a
> > meter is allocated or removed 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
> >
> > In order to fix the issue introduce SB_meter node in the incremetal
> > processing engine and add it as input for lflow_output one.
> > Sync OVS meters in ofctl_put() to push changes in SB meter/SB meter
> > bands tables.
> >
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1939524
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> 
> Thanks Lorenzo. This looks much simpler and good overall. I have some minor
> suggestions inlined.

Hi Han,

> 
> > ---
> > Changes since v7:
> > - implement full-compare support in ofctrl module
> > - drop SB_meter IP management
> >
> > Changes since v6:
> > - move ovs sync code back to ofctrl_put()
> > - remove meter IP node and link SB_meter node to lflow_output one
> > - add new unit-tests
> >
> > Changes since v5:
> > - add size parameter to extend_table in order to reduce the size
> >
> > Changes since v4:
> > - add offset parameter to ovn_extend_table_init
> > - rebase on top of ovn master
> >
> > Changes since v3:
> > - move full meter management in the IP engine
> >
> > Changes since v2:
> > - move meter ip into lflow ip
> > - add new test in ovn-performance.at
> > - manage metering ip full-recompute
> >
> > Changes since v1:
> > - add IP refactor to the series
> > - rebase on top of ovn master
> > ---
> >  controller/ofctrl.c         | 200 +++++++++++++++++++++++++++++++-----
> >  controller/ovn-controller.c |  12 ++-
> >  tests/ovn-performance.at    |  22 ++++
> >  tests/ovn.at                |  74 +++++++++++++
> >  tests/system-ovn.at         |  17 +++
> >  5 files changed, 301 insertions(+), 24 deletions(-)
> >
> > diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> > index 19aa787f9..5b4c823ef 100644
> > --- a/controller/ofctrl.c
> > +++ b/controller/ofctrl.c
> > @@ -360,6 +360,21 @@ static struct ovn_extend_table *groups;
> >  /* A reference to the meter_table. */
> >  static struct ovn_extend_table *meters;
> >
> > +/* Installed meter bands. */
> > +struct meter_band_data {
> > +    int64_t burst_size;
> > +    int64_t rate;
> > +};
> > +
> > +struct meter_band_entry {
> > +    struct meter_band_data *bands;
> > +    size_t n_bands;
> > +};
> > +
> > +static struct shash meter_bands;
> > +
> > +static void ofctrl_meter_bands_destroy(void);
> > +
> >  /* MFF_* field ID for our Geneve option.  In S_TLV_TABLE_MOD_SENT, this
> is
> >   * the option we requested (we don't know whether we obtained it yet).
> In
> >   * S_CLEAR_FLOWS or S_UPDATE_FLOWS, this is really the option we have. */
> > @@ -397,6 +412,7 @@ ofctrl_init(struct ovn_extend_table *group_table,
> >      ovn_init_symtab(&symtab);
> >      groups = group_table;
> >      meters = meter_table;
> > +    shash_init(&meter_bands);
> >  }
> >
> >  /* S_NEW, for a new connection.
> > @@ -814,6 +830,7 @@ ofctrl_destroy(void)
> >      rconn_packet_counter_destroy(tx_counter);
> >      expr_symtab_destroy(&symtab);
> >      shash_destroy(&symtab);
> > +    ofctrl_meter_bands_destroy();
> >  }
> >
> >  uint64_t
> > @@ -1979,26 +1996,14 @@ 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)
> > +update_ovs_meter(struct ovn_extend_table_info *entry,
> > +                 const struct sbrec_meter *sb_meter, int cmd,
> > +                 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;
> > -    }
> >
> >      struct ofputil_meter_mod mm;
> > -    mm.command = OFPMC13_ADD;
> > -    mm.meter.meter_id = m_desired->table_id;
> > +    mm.command = cmd;
> > +    mm.meter.meter_id = entry->table_id;
> >      mm.meter.flags = OFPMF13_STATS;
> >
> >      if (!strcmp(sb_meter->unit, "pktps")) {
> > @@ -2031,6 +2036,158 @@ add_meter(struct ovn_extend_table_info *m_desired,
> >      free(mm.meter.bands);
> >  }
> >
> > +static void
> > +ofctrl_meter_bands_destroy(void)
> > +{
> > +    struct shash_node *node, *next;
> > +    SHASH_FOR_EACH_SAFE (node, next, &meter_bands) {
> > +        struct meter_band_entry *mb = node->data;
> > +        shash_delete(&meter_bands, node);
> > +        free(mb->bands);
> > +        free(mb);
> > +    }
> > +    shash_destroy(&meter_bands);
> > +}
> > +
> > +static bool
> > +ofctrl_meter_bands_check(const struct sbrec_meter *sb_meter,
> > +                         struct meter_band_entry *mb)
> 
> It would be better to rename the function to "...is_equal" (and reverse the
> return value), otherwise add comments to tell what the return value means.

ack, I will fix it in v9.

> 
> > +{
> > +    if (mb->n_bands != sb_meter->n_bands) {
> > +        return true;
> > +    }
> > +
> > +    for (int i = 0; i < sb_meter->n_bands; i++) {
> > +        int j;
> > +        for (j = 0; j < mb->n_bands; j++) {
> > +            if (sb_meter->bands[i]->rate == mb->bands[j].rate &&
> > +                sb_meter->bands[i]->burst_size ==
> mb->bands[j].burst_size) {
> > +                break;
> > +            }
> > +        }
> > +        if (j == mb->n_bands) {
> > +            return true;
> > +        }
> > +    }
> > +    return false;
> > +}
> > +
> > +static void
> > +ofctrl_meter_bands_alloc(const struct sbrec_meter *sb_meter,
> > +                         struct ovn_extend_table_info *entry,
> > +                         struct ovs_list *msgs)
> > +{
> > +    struct meter_band_entry *mb = mb = xzalloc(sizeof *mb);
> > +    mb->n_bands = sb_meter->n_bands;
> > +    mb->bands = xcalloc(mb->n_bands, sizeof *mb->bands);
> > +    for (int i = 0; i < sb_meter->n_bands; i++) {
> > +        mb->bands[i].rate = sb_meter->bands[i]->rate;
> > +        mb->bands[i].burst_size = sb_meter->bands[i]->burst_size;
> > +    }
> > +    shash_add(&meter_bands, entry->name, mb);
> > +    update_ovs_meter(entry, sb_meter, OFPMC13_ADD, msgs);
> > +}
> > +
> > +static void
> > +ofctrl_meter_bands_update(const struct sbrec_meter *sb_meter,
> > +                          struct ovn_extend_table_info *entry,
> > +                          struct ovs_list *msgs)
> > +{
> > +    struct meter_band_entry *mb =
> > +        shash_find_data(&meter_bands, entry->name);
> > +    if (!mb) {
> > +        ofctrl_meter_bands_alloc(sb_meter, entry, msgs);
> > +        return;
> > +    }
> > +
> > +    if (!ofctrl_meter_bands_check(sb_meter, mb)) {
> > +        return;
> > +    }
> > +
> > +    free(mb->bands);
> > +    mb->n_bands = sb_meter->n_bands;
> > +    mb->bands = xcalloc(mb->n_bands, sizeof *mb->bands);
> > +    for (int i = 0; i < sb_meter->n_bands; i++) {
> > +        mb->bands[i].rate = sb_meter->bands[i]->rate;
> > +        mb->bands[i].burst_size = sb_meter->bands[i]->burst_size;
> > +    }
> > +
> > +    update_ovs_meter(entry, sb_meter, OFPMC13_MODIFY, msgs);
> > +}
> > +
> > +static void
> > +ofctrl_meter_bands_erase(struct ovn_extend_table_info *entry,
> > +                         struct ovs_list *msgs)
> > +{
> > +    struct meter_band_entry *mb =
> > +        shash_find_and_delete(&meter_bands, entry->name);
> > +    if (mb) {
> > +        /* Delete the meter. */
> > +        struct ofputil_meter_mod mm = {
> > +            .command = OFPMC13_DELETE,
> > +            .meter = { .meter_id = entry->table_id },
> > +        };
> > +        add_meter_mod(&mm, msgs);
> > +
> > +        free(mb->bands);
> > +        free(mb);
> > +    }
> > +}
> > +
> > +static void
> > +ofctrl_meter_bands_sync(const struct sbrec_meter_table *meter_table,
> > +                        struct ovs_list *msgs)
> > +{
> > +    struct ovn_extend_table_info *m_desired;
> > +    HMAP_FOR_EACH (m_desired, hmap_node, &meters->desired) {
> > +        struct ovn_extend_table_info *m_existing =
> > +            ovn_extend_table_lookup(&meters->existing, m_desired);
> > +        if (!m_existing) {
> > +            /* Here we are assuming desired and existing are always
> aligned.
> > +             * ofctrl_meter_bands_sync() must run after
> > +             * ovn_extend_table_sync() on meter extend table.
> > +             */
> 
> Thanks for the comment. I can see why this function requires desired and
> existing are in sync, but if they are in sync, this check should never be
> false, in which case we don't need the check, or just assert it.
> However, I think this tricky point can be avoided. This is supposed to
> compare the existing meters with the desired state, and update to OVS if
> there is any difference. Would it be better to move this
> "compare-and-update" logic to the place when new meters are checked and
> added instead of calling this function after the ovn_extend_table_sync()?
> We can change the loop "EXTEND_TABLE_FOR_EACH_UNINSTALLED" to:
> 
> HMAP_FOR_EACH (..., desired) {
>     if (NOT found in existing) {
>         add_meter_string or add_meter;
>     } else {
>         compare_and_update; /* what's performed by the
> ofctrl_meter_bands_sync() function but for just one meter. */
>     }
> }
> 
> In addition, there are some wasted checks when this function is called
> after ovn_extend_table_sync(), because the newly added meters are now
> indistinguishable from the existing meters, and they will be compared with
> SB data again. The above pseudo code would avoid this waste, too. What do
> you think?

ack, it sounds good to me. I will fix it in v9.
Regards,

Lorenzo

> 
> Thanks,
> Han
> 
> > +            continue;
> > +        }
> > +
> > +        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) {
> > +            /* OFPMC13_ADD or OFPMC13_MODIFY */
> > +            ofctrl_meter_bands_update(sb_meter, m_existing, msgs);
> > +        } else {
> > +            /* OFPMC13_DELETE */
> > +            ofctrl_meter_bands_erase(m_existing, msgs);
> > +        }
> > +    }
> > +}
> > +
> > +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;
> > +    }
> > +
> > +    ofctrl_meter_bands_alloc(sb_meter, m_desired, msgs);
> > +}
> > +
> >  static void
> >  installed_flow_add(struct ovn_flow *d,
> >                     struct ofputil_bundle_ctrl_msg *bc,
> > @@ -2505,17 +2662,14 @@ ofctrl_put(struct ovn_desired_flow_table
> *lflow_table,
> >      struct ovn_extend_table_info *m_installed, *next_meter;
> >      EXTEND_TABLE_FOR_EACH_INSTALLED (m_installed, next_meter, meters) {
> >          /* Delete the meter. */
> > -        struct ofputil_meter_mod mm = {
> > -            .command = OFPMC13_DELETE,
> > -            .meter = { .meter_id = m_installed->table_id },
> > -        };
> > -        add_meter_mod(&mm, &msgs);
> > -
> > +        ofctrl_meter_bands_erase(m_installed, &msgs);
> >          ovn_extend_table_remove_existing(meters, m_installed);
> >      }
> >
> >      /* Sync the contents of meters->desired to meters->existing. */
> >      ovn_extend_table_sync(meters);
> > +    /* Sync installed meter bands. */
> > +    ofctrl_meter_bands_sync(meter_table, &msgs);
> >
> >      if (!ovs_list_is_empty(&msgs)) {
> >          /* Add a barrier to the list of messages. */
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index c09018243..6f0c095c9 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -968,7 +968,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,
> > @@ -2718,6 +2719,13 @@ lflow_output_sb_fdb_handler(struct engine_node
> *node, void *data)
> >      return handled;
> >  }
> >
> > +static bool
> > +lflow_output_sb_meter_handler(struct engine_node *node, void *data
> OVS_UNUSED)
> > +{
> > +    engine_set_node_state(node, EN_UPDATED);
> > +    return true;
> > +}
> > +
> >  struct ed_type_pflow_output {
> >      /* Desired physical flows. */
> >      struct ovn_desired_flow_table flow_table;
> > @@ -3316,6 +3324,8 @@ 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_sb_meter,
> > +                     lflow_output_sb_meter_handler);
> >
> >      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/ovn-performance.at b/tests/ovn-performance.at
> > index 10341ad72..bc133f93b 100644
> > --- a/tests/ovn-performance.at
> > +++ b/tests/ovn-performance.at
> > @@ -543,6 +543,28 @@ OVN_CONTROLLER_EXPECT_HIT(
> >      [as hv3 ovs-vsctl set interface vgw3
> external-ids:ovn-egress-iface=true]
> >  )
> >
> > +ovn-nbctl --wait=hv meter-add meter0 drop 100 pktps 10
> > +
> > +OVN_CONTROLLER_EXPECT_NO_HIT(
> > +    [hv1 hv2 hv3 hv4], [lflow_run],
> > +    [ovn-nbctl --wait=hv copp-add copp0 arp meter0]
> > +)
> > +
> > +OVN_CONTROLLER_EXPECT_NO_HIT(
> > +    [hv1 hv2 hv3 hv4], [lflow_run],
> > +    [ovn-nbctl --wait=hv lr-copp-add copp0 lr1]
> > +)
> > +
> > +OVN_CONTROLLER_EXPECT_NO_HIT(
> > +    [hv1 hv2 hv3 hv4], [lflow_run],
> > +    [ovn-nbctl --wait=hv --may-exist meter-add meter0 drop 200 pktps 10]
> > +)
> > +
> > +OVN_CONTROLLER_EXPECT_NO_HIT(
> > +    [hv1 hv2 hv3 hv4], [lflow_run],
> > +    [ovn-nbctl --wait=hv meter-del meter0]
> > +)
> > +
> >  for i in 1 2; do
> >      j=$((i%2 + 1))
> >      lp=lp$i
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 69270601a..d5ecb3fb4 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -29744,3 +29744,77 @@ OVS_WAIT_UNTIL([
> >  OVN_CLEANUP([hv1],[hv2])
> >  AT_CLEANUP
> >  ])
> > +
> > +OVN_FOR_EACH_NORTHD([
> > +AT_SETUP([ovn-controller - check meters update])
> > +AT_KEYWORDS([meters-update])
> > +
> > +ovn_start
> > +
> > +net_add n1
> > +sim_add hv1
> > +as hv1
> > +ovs-vsctl add-br br-phys
> > +ovn_attach n1 br-phys 192.168.0.10
> > +
> > +check ovn-nbctl ls-add sw0
> > +check ovn-nbctl lsp-add sw0 lsp
> > +
> > +as hv1 ovs-vsctl \
> > +    -- add-port br-int vif1 \
> > +    -- set Interface vif1 external_ids:iface-id=lsp
> > +
> > +# Wait for port to be bound.
> > +wait_row_count Chassis 1 name=hv1
> > +ch=$(fetch_column Chassis _uuid name=hv1)
> > +wait_row_count Port_Binding 1 logical_port=lsp chassis=$ch
> > +
> > +# Add a new meter
> > +check ovn-nbctl --event lb-add lb0 192.168.1.100:80 ""
> > +check ovn-nbctl ls-lb-add sw0 lb0
> > +check ovn-nbctl meter-add meter0 drop 10 pktps
> > +ovn-nbctl --wait=hv copp-add copp0 event-elb meter0
> > +ovn-nbctl --wait=hv ls-copp-add copp0 sw0
> > +check ovn-nbctl --wait=hv sync
> > +
> > +AT_CHECK([as hv1 ovs-ofctl -OOpenFlow15 dump-meters br-int | grep -q
> rate=10], [0])
> > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep -q meter_id=1], [0])
> > +
> > +# Update existing meter
> > +check ovn-nbctl --may-exist meter-add meter0 drop 20 pktps
> > +check ovn-nbctl --wait=hv sync
> > +
> > +AT_CHECK([as hv1 ovs-ofctl -OOpenFlow15 dump-meters br-int | grep -q
> rate=20], [0])
> > +
> > +# Add a new meter
> > +check ovn-nbctl meter-add meter1 drop 30 pktps
> > +check ovn-nbctl --log --severity=alert --meter=meter1 \
> > +                --name=dns acl-add sw0 to-lport 1000 'udp.dst == 53' drop
> > +check ovn-nbctl --wait=hv sync
> > +AT_CHECK([as hv1 ovs-ofctl -OOpenFlow15 dump-meters br-int | grep -q
> rate=30], [0])
> > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep -q meter_id=2], [0])
> > +
> > +# Remove meter0
> > +check ovn-nbctl meter-del meter0
> > +check ovn-nbctl --wait=hv sync
> > +AT_CHECK([as hv1 ovs-ofctl -OOpenFlow15 dump-meters br-int | grep -q
> rate=10], [1])
> > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep -q meter_id=1], [1])
> > +
> > +check ovn-nbctl meter-del meter1
> > +check ovn-nbctl --wait=hv sync
> > +AT_CHECK([as hv1 ovs-ofctl -OOpenFlow15 dump-meters br-int | grep -q
> rate=30], [1])
> > +
> > +# create meters in the opposite order
> > +check ovn-nbctl --log --severity=alert --meter=meter2 \
> > +                --name=dns acl-add sw0 to-lport 1000 'tcp.dst == 80' drop
> > +check ovn-nbctl meter-add meter2 drop 100 pktps
> > +check ovn-nbctl --wait=hv sync
> > +AT_CHECK([as hv1 ovs-ofctl -OOpenFlow15 dump-meters br-int | grep -q
> rate=100], [0])
> > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep -q meter_id=1], [0])
> > +
> > +check ovn-nbctl meter-del meter2
> > +AT_CHECK([as hv1 ovs-ofctl -OOpenFlow15 dump-meters br-int | grep -q
> rate=100], [1])
> > +
> > +OVN_CLEANUP([hv1])
> > +AT_CLEANUP
> > +])
> > diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> > index f57d752d4..d1770408e 100644
> > --- a/tests/system-ovn.at
> > +++ b/tests/system-ovn.at
> > @@ -6661,8 +6661,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 copp-del copp0 reject
> >
> > --
> > 2.35.1
> >