diff mbox series

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

Message ID 888859c823fc194a9a82a05021698bdba22dc294.1646658431.git.lorenzo.bianconi@redhat.com
State Superseded
Headers show
Series [ovs-dev,v9] controller: reconfigure ovs meters for ovn meters | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test fail github build: failed
ovsrobot/github-robot-_ovn-kubernetes fail github build: failed

Commit Message

Lorenzo Bianconi March 7, 2022, 1:11 p.m. UTC
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 v8:
- rename ofctrl_meter_bands_check() to ofctrl_meter_bands_is_equal()
- move ofctrl_meter_bands_sync() in add_meter loop in order to avoid a
  dedicated loop over desired extend_table meter hash

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         | 206 +++++++++++++++++++++++++++++++-----
 controller/ovn-controller.c |  12 ++-
 tests/ovn-performance.at    |  22 ++++
 tests/ovn.at                |  74 +++++++++++++
 tests/system-ovn.at         |  17 +++
 5 files changed, 301 insertions(+), 30 deletions(-)

Comments

Han Zhou March 7, 2022, 6:28 p.m. UTC | #1
On Mon, Mar 7, 2022 at 5:11 AM 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 for v9! I left 3 minor comments below. With these addressed:
Acked-by: Han Zhou <hzhou@ovn.org>

> Changes since v8:
> - rename ofctrl_meter_bands_check() to ofctrl_meter_bands_is_equal()
> - move ofctrl_meter_bands_sync() in add_meter loop in order to avoid a
>   dedicated loop over desired extend_table meter hash
>
> 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         | 206 +++++++++++++++++++++++++++++++-----
>  controller/ovn-controller.c |  12 ++-
>  tests/ovn-performance.at    |  22 ++++
>  tests/ovn.at                |  74 +++++++++++++
>  tests/system-ovn.at         |  17 +++
>  5 files changed, 301 insertions(+), 30 deletions(-)
>
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index 19aa787f9..5a93c1844 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);

Sorry that I forgot to mention one problem in v8. The lifecycle of
meter_bands should follow strictly that of the meters->existing. It needs
to be cleared in run_S_CLEAR_FLOWS():

    /* Clear existing meters, to match the state of the switch. */
    if (meters) {
        ovn_extend_table_clear(meters, true);
    }

At this same place we should clear the meter_bands, i.e. destroy and init
again.

>  }
>
>  /* 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,150 @@ 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_is_equal(const struct sbrec_meter *sb_meter,
> +                            struct meter_band_entry *mb)
> +{
> +    if (mb->n_bands != sb_meter->n_bands) {
> +        return false;
> +    }
> +
> +    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 false;
> +        }
> +    }
> +    return true;
> +}
> +
> +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_is_equal(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(struct ovn_extend_table_info *m_desired,
> +                        const struct sbrec_meter_table *meter_table,
> +                        struct ovs_list *msgs)
> +{
> +    struct ovn_extend_table_info *m_existing =
> +        ovn_extend_table_lookup(&meters->existing, m_desired);
> +    ovs_assert(m_existing);

Since we performed a lookup right before calling the function, it would be
better to pass the "m_existing" directly to this function, instead of
passing m_desired and lookup again.

> +
> +    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,
> @@ -2409,13 +2558,17 @@ ofctrl_put(struct ovn_desired_flow_table
*lflow_table,
>      /* Iterate through all the desired meters. If there are new ones,
>       * add them to the switch. */
>      struct ovn_extend_table_info *m_desired;
> -    EXTEND_TABLE_FOR_EACH_UNINSTALLED (m_desired, meters) {
> -        if (!strncmp(m_desired->name, "__string: ", 10)) {
> -            /* The "set-meter" action creates a meter entry name that
> -             * describes the meter itself. */
> -            add_meter_string(m_desired, &msgs);
> +    HMAP_FOR_EACH (m_desired, hmap_node, &meters->desired) {
> +        if (!ovn_extend_table_lookup(&meters->existing, m_desired)) {
> +            if (!strncmp(m_desired->name, "__string: ", 10)) {
> +                /* The "set-meter" action creates a meter entry name that
> +                 * describes the meter itself. */
> +                add_meter_string(m_desired, &msgs);
> +            } else {
> +                add_meter(m_desired, meter_table, &msgs);
> +            }
>          } else {
> -            add_meter(m_desired, meter_table, &msgs);
> +            ofctrl_meter_bands_sync(m_desired, meter_table, &msgs);
>          }
>      }
>
> @@ -2505,12 +2658,7 @@ 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);
>      }
>
> 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)
> +{

Sorry for missing this in v8, too. I think we'd still want to look up the
desired meters, and ignore the change if it doesn't belong to the desired
meters, because in that case ofctrl_put wouldn't do anything.

Thanks,
Han

> +    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
>
diff mbox series

Patch

diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index 19aa787f9..5a93c1844 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,150 @@  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_is_equal(const struct sbrec_meter *sb_meter,
+                            struct meter_band_entry *mb)
+{
+    if (mb->n_bands != sb_meter->n_bands) {
+        return false;
+    }
+
+    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 false;
+        }
+    }
+    return true;
+}
+
+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_is_equal(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(struct ovn_extend_table_info *m_desired,
+                        const struct sbrec_meter_table *meter_table,
+                        struct ovs_list *msgs)
+{
+    struct ovn_extend_table_info *m_existing =
+        ovn_extend_table_lookup(&meters->existing, m_desired);
+    ovs_assert(m_existing);
+
+    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,
@@ -2409,13 +2558,17 @@  ofctrl_put(struct ovn_desired_flow_table *lflow_table,
     /* Iterate through all the desired meters. If there are new ones,
      * add them to the switch. */
     struct ovn_extend_table_info *m_desired;
-    EXTEND_TABLE_FOR_EACH_UNINSTALLED (m_desired, meters) {
-        if (!strncmp(m_desired->name, "__string: ", 10)) {
-            /* The "set-meter" action creates a meter entry name that
-             * describes the meter itself. */
-            add_meter_string(m_desired, &msgs);
+    HMAP_FOR_EACH (m_desired, hmap_node, &meters->desired) {
+        if (!ovn_extend_table_lookup(&meters->existing, m_desired)) {
+            if (!strncmp(m_desired->name, "__string: ", 10)) {
+                /* The "set-meter" action creates a meter entry name that
+                 * describes the meter itself. */
+                add_meter_string(m_desired, &msgs);
+            } else {
+                add_meter(m_desired, meter_table, &msgs);
+            }
         } else {
-            add_meter(m_desired, meter_table, &msgs);
+            ofctrl_meter_bands_sync(m_desired, meter_table, &msgs);
         }
     }
 
@@ -2505,12 +2658,7 @@  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);
     }
 
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