diff mbox series

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

Message ID d561b4a6862449507d83aaf0b196a4f48ae1744c.1646927785.git.lorenzo.bianconi@redhat.com
State Accepted
Headers show
Series [ovs-dev,21.12] controller: reconfigure ovs meters for ovn meters | expand

Checks

Context Check Description
ovsrobot/apply-robot fail apply and check: fail

Commit Message

Lorenzo Bianconi March 10, 2022, 4:01 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
Acked-by: Mark Michelson <mmichels@redhat.com>
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
Signed-off-by: Han Zhou <hzhou@ovn.org>
---
 controller/ofctrl.c         | 212 +++++++++++++++++++++++++++++++-----
 controller/ovn-controller.c |  25 ++++-
 lib/extend-table.c          |  14 +++
 lib/extend-table.h          |   4 +
 tests/ovn-performance.at    |  17 +++
 tests/ovn.at                |  73 +++++++++++++
 tests/system-ovn.at         |  17 +++
 7 files changed, 332 insertions(+), 30 deletions(-)

Comments

Mark Michelson March 10, 2022, 4:39 p.m. UTC | #1
Thanks Lorenzo for this backport. I did a quick sanity check on the 
21.12 branch and it looks good to me. So I Acked and pushed the patch to 
branch-21.12.

On 3/10/22 11:01, Lorenzo Bianconi 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
> Acked-by: Mark Michelson <mmichels@redhat.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> Signed-off-by: Han Zhou <hzhou@ovn.org>
> ---
>   controller/ofctrl.c         | 212 +++++++++++++++++++++++++++++++-----
>   controller/ovn-controller.c |  25 ++++-
>   lib/extend-table.c          |  14 +++
>   lib/extend-table.h          |   4 +
>   tests/ovn-performance.at    |  17 +++
>   tests/ovn.at                |  73 +++++++++++++
>   tests/system-ovn.at         |  17 +++
>   7 files changed, 332 insertions(+), 30 deletions(-)
> 
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index 08fcfed8b..22f80620a 100644
> --- a/controller/ofctrl.c
> +++ b/controller/ofctrl.c
> @@ -335,6 +335,22 @@ 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);
> +static void ofctrl_meter_bands_clear(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. */
> @@ -372,6 +388,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.
> @@ -615,6 +632,7 @@ run_S_CLEAR_FLOWS(void)
>       /* Clear existing meters, to match the state of the switch. */
>       if (meters) {
>           ovn_extend_table_clear(meters, true);
> +        ofctrl_meter_bands_clear();
>       }
>   
>       /* All flow updates are irrelevant now. */
> @@ -789,6 +807,7 @@ ofctrl_destroy(void)
>       rconn_packet_counter_destroy(tx_counter);
>       expr_symtab_destroy(&symtab);
>       shash_destroy(&symtab);
> +    ofctrl_meter_bands_destroy();
>   }
>   
>   uint64_t
> @@ -1802,26 +1821,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")) {
> @@ -1854,6 +1861,152 @@ add_meter(struct ovn_extend_table_info *m_desired,
>       free(mm.meter.bands);
>   }
>   
> +static void
> +ofctrl_meter_bands_clear(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);
> +    }
> +}
> +
> +static void
> +ofctrl_meter_bands_destroy(void)
> +{
> +    ofctrl_meter_bands_clear();
> +    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_existing,
> +                        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_existing->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,
> @@ -2232,13 +2385,19 @@ 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) {
> +        struct ovn_extend_table_info *m_existing =
> +            ovn_extend_table_lookup(&meters->existing, m_desired);
> +        if (!m_existing) {
> +            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_existing, meter_table, &msgs);
>           }
>       }
>   
> @@ -2328,12 +2487,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 5069aedfc..f85af9353 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -962,7 +962,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,
> @@ -2713,6 +2714,26 @@ 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)
> +{
> +    struct ed_type_lflow_output *fo = data;
> +    struct sbrec_meter_table *meter_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, meter_table) {
> +        if (ovn_extend_table_desired_lookup_by_name(&fo->meter_table,
> +                                                    iter->name)) {
> +            engine_set_node_state(node, EN_UPDATED);
> +            break;
> +        }
> +    }
> +
> +    return true;
> +}
> +
>   struct ed_type_pflow_output {
>       /* Desired physical flows. */
>       struct ovn_desired_flow_table flow_table;
> @@ -3303,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/lib/extend-table.c b/lib/extend-table.c
> index c708e24b9..32d541b55 100644
> --- a/lib/extend-table.c
> +++ b/lib/extend-table.c
> @@ -337,3 +337,17 @@ ovn_extend_table_assign_id(struct ovn_extend_table *table, const char *name,
>   
>       return table_id;
>   }
> +
> +struct ovn_extend_table_info *
> +ovn_extend_table_desired_lookup_by_name(struct ovn_extend_table * table,
> +                                        const char *name)
> +{
> +    uint32_t hash = hash_string(name, 0);
> +    struct ovn_extend_table_info *m_desired;
> +    HMAP_FOR_EACH_WITH_HASH (m_desired, hmap_node, hash, &table->desired) {
> +        if (!strcmp(m_desired->name, name)) {
> +            return m_desired;
> +        }
> +    }
> +    return NULL;
> +}
> diff --git a/lib/extend-table.h b/lib/extend-table.h
> index 4d80cfd80..6240b946e 100644
> --- a/lib/extend-table.h
> +++ b/lib/extend-table.h
> @@ -94,6 +94,10 @@ uint32_t ovn_extend_table_assign_id(struct ovn_extend_table *,
>                                       const char *name,
>                                       struct uuid lflow_uuid);
>   
> +struct ovn_extend_table_info *
> +ovn_extend_table_desired_lookup_by_name(struct ovn_extend_table * table,
> +                                        const char *name);
> +
>   /* Iterates 'DESIRED' through all of the 'ovn_extend_table_info's in
>    * 'TABLE'->desired that are not in 'TABLE'->existing.  (The loop body
>    * presumably adds them.) */
> diff --git a/tests/ovn-performance.at b/tests/ovn-performance.at
> index 10341ad72..f92fbebf1 100644
> --- a/tests/ovn-performance.at
> +++ b/tests/ovn-performance.at
> @@ -543,6 +543,23 @@ 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 lr-copp-add lr1 arp meter0]
> +)
> +
> +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 52a1758c7..5625f7767 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -29768,3 +29768,76 @@ 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 ls-copp-add sw0 event-elb meter0
> +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 dd2c1800a..5f41722d9 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -6927,8 +6927,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
>   
>
diff mbox series

Patch

diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index 08fcfed8b..22f80620a 100644
--- a/controller/ofctrl.c
+++ b/controller/ofctrl.c
@@ -335,6 +335,22 @@  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);
+static void ofctrl_meter_bands_clear(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. */
@@ -372,6 +388,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.
@@ -615,6 +632,7 @@  run_S_CLEAR_FLOWS(void)
     /* Clear existing meters, to match the state of the switch. */
     if (meters) {
         ovn_extend_table_clear(meters, true);
+        ofctrl_meter_bands_clear();
     }
 
     /* All flow updates are irrelevant now. */
@@ -789,6 +807,7 @@  ofctrl_destroy(void)
     rconn_packet_counter_destroy(tx_counter);
     expr_symtab_destroy(&symtab);
     shash_destroy(&symtab);
+    ofctrl_meter_bands_destroy();
 }
 
 uint64_t
@@ -1802,26 +1821,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")) {
@@ -1854,6 +1861,152 @@  add_meter(struct ovn_extend_table_info *m_desired,
     free(mm.meter.bands);
 }
 
+static void
+ofctrl_meter_bands_clear(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);
+    }
+}
+
+static void
+ofctrl_meter_bands_destroy(void)
+{
+    ofctrl_meter_bands_clear();
+    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_existing,
+                        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_existing->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,
@@ -2232,13 +2385,19 @@  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) {
+        struct ovn_extend_table_info *m_existing =
+            ovn_extend_table_lookup(&meters->existing, m_desired);
+        if (!m_existing) {
+            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_existing, meter_table, &msgs);
         }
     }
 
@@ -2328,12 +2487,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 5069aedfc..f85af9353 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -962,7 +962,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,
@@ -2713,6 +2714,26 @@  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)
+{
+    struct ed_type_lflow_output *fo = data;
+    struct sbrec_meter_table *meter_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, meter_table) {
+        if (ovn_extend_table_desired_lookup_by_name(&fo->meter_table,
+                                                    iter->name)) {
+            engine_set_node_state(node, EN_UPDATED);
+            break;
+        }
+    }
+
+    return true;
+}
+
 struct ed_type_pflow_output {
     /* Desired physical flows. */
     struct ovn_desired_flow_table flow_table;
@@ -3303,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/lib/extend-table.c b/lib/extend-table.c
index c708e24b9..32d541b55 100644
--- a/lib/extend-table.c
+++ b/lib/extend-table.c
@@ -337,3 +337,17 @@  ovn_extend_table_assign_id(struct ovn_extend_table *table, const char *name,
 
     return table_id;
 }
+
+struct ovn_extend_table_info *
+ovn_extend_table_desired_lookup_by_name(struct ovn_extend_table * table,
+                                        const char *name)
+{
+    uint32_t hash = hash_string(name, 0);
+    struct ovn_extend_table_info *m_desired;
+    HMAP_FOR_EACH_WITH_HASH (m_desired, hmap_node, hash, &table->desired) {
+        if (!strcmp(m_desired->name, name)) {
+            return m_desired;
+        }
+    }
+    return NULL;
+}
diff --git a/lib/extend-table.h b/lib/extend-table.h
index 4d80cfd80..6240b946e 100644
--- a/lib/extend-table.h
+++ b/lib/extend-table.h
@@ -94,6 +94,10 @@  uint32_t ovn_extend_table_assign_id(struct ovn_extend_table *,
                                     const char *name,
                                     struct uuid lflow_uuid);
 
+struct ovn_extend_table_info *
+ovn_extend_table_desired_lookup_by_name(struct ovn_extend_table * table,
+                                        const char *name);
+
 /* Iterates 'DESIRED' through all of the 'ovn_extend_table_info's in
  * 'TABLE'->desired that are not in 'TABLE'->existing.  (The loop body
  * presumably adds them.) */
diff --git a/tests/ovn-performance.at b/tests/ovn-performance.at
index 10341ad72..f92fbebf1 100644
--- a/tests/ovn-performance.at
+++ b/tests/ovn-performance.at
@@ -543,6 +543,23 @@  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 lr-copp-add lr1 arp meter0]
+)
+
+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 52a1758c7..5625f7767 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -29768,3 +29768,76 @@  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 ls-copp-add sw0 event-elb meter0
+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 dd2c1800a..5f41722d9 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -6927,8 +6927,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