> On Fri, Feb 25, 2022 at 12:09 PM Mark Michelson <[email protected]> wrote:
> >
> > On 2/25/22 13:44, Han Zhou wrote:
> > > On Fri, Feb 25, 2022 at 9:15 AM Lorenzo Bianconi <
> > > [email protected]> 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 incremental processing for ovn
> > >> metering configured through sb db meter/meter-band tables and remove
> > >> meter extend table management (extend table is now considering just
> > >> meters created through the set_meter() action since we do not have
> > >> an entry in the sb db for them).
> > >>
> > >> Related bz: https://bugzilla.redhat.com/show_bug.cgi?id=1939524
> > >> Acked-by: Mark Michelson <[email protected]>
> > >>
> > >> Signed-off-by: Lorenzo Bianconi <[email protected]>
> > >
> > > Thanks Lorenzo for fixing this. I am concerned with sending messages to
> OVS
> > > directly within the I-P engine. That has been the responsibility of
> ofctrl
> > > module, and the engine was supposed to compute the output data only.
> Sorry
> > > for the late response, but can we avoid this?
> >
> > Hi Han, I didn't realize when reviewing this that this was something we
> > were not supposed to do. Just to be certain I understand, the idea here
> > would be for the engine node to calculate the new data, then pass that
> > data as an input to ofctrl_put()?
> >
>
> Hi Mark, Lorenzo,
Hi Han,
>
> Yes, the idea is ofctrl module manages the communication with ovs-vswitch
> via the OpenFlow connection, including the state machine and OF
> transactions. I think it would cause problems if ovn-controller.c send
> messages directly to ovs-vswitchd without considering the state machine,
> the in-flight messages and barrier-replies, etc.
>
> The I-P engine is supposed to compute the desired state of OpenFlow
> objects, including flows, meters and groups. The output data of the I-P
> engine can be passed as inputs to ofctrl_put(). ofctrl module will send to
> ovs-vswitchd within the ofctrl_put(), and handle responses in ofctrl_run(),
> which also handles sending pending messages, if ofctrl_put() can't complete
> the message sending.
>
> The ofctrl module also maintains both the desired state and the installed
> state of the objects. In this implementation it has a single copy of the
> data, assuming it is always the same as what's installed. The SB meter
> table is regarded as the desired state and is converged directly in the I-P
> engine handlers. I can't say it is fundamentally wrong, but at least it
> would have problems e.g. when OVS restarts. It may add code to clear the
> data in the run_S_CLEAR_FLOWS, but then both ofctrl and ovn-controller are
> updating the data.
>
> I thought about the problem again. It is in fact missing SB meter handling
> in the I-P engine. The existing implementation installs meters to OVS only
> if a meter is used by any flows, otherwise even if SB has the meter, it is
> not installed to OVS. So the meter data is only derived from the lflows,
> and checking for SB meters happens at the ofctrl module while installing
> the meters, which is outside of the I-P engine. There are problems of that
> implementation:
> 1) When a meter was referenced by a lflow but not created in SB yet, the
> meter won't be installed to OVS, which is reasonable. But if the meter is
> then added to SB, since we don't have handler for SB meter table, this is
> ignored, and adding the meter in SB won't correct the missing meter in OVS.
> 2) When a meter used by a flow is deleted (probably by mistake), there is
> no OVS update, either. This may be ok, but still a confusing behavior.
> 3) When a meter is updated, it is not reflected to the OVS, as the problem
> stated by this patch.
>
> So, I believe adding meter handling to the I-P engine is the right
> approach, but may I suggest a solution more aligned with the existing
> implementation with fewer changes?
>
> 1) The existing meter table already maintains both the desired and
> installed states, for both named and unnamed meters, and maintains the
> lflow <-> meter references. We can reuse it, and let ofctrl module take
> care of syncing to OVS. We just need to change the code that compares the
> desired and installed meters, not just check if it existed, but also
> compare the data of the meter. If desired is different from the installed,
> create a modify message for it. These messages would be sent to OVS within
> the same bundle of the other meters, groups and flows, as it is today.
>
> 2) In the I-P engine, we need SB_meter node to be the input of the
> lflow_output node, but maybe we don't need a separate meter node (the
> current meter data is part of the lflow_output). To handle the SB_meter
> change, we need to incrementally handle the add/delete/update of a meter in
> SB, without triggering lflow recompute. This should be straightforward,
> with the help of the link between meters and lflows.
ack, thx for the detailed explanation :).
We actually need the info provided by IP-engine to understand if a given meter
has to be updated since both desired and installed extend_table_info meters
can refer to SB meter table just through the meter name and they do not maintain
any "copy" of the meter bands.
I will add post v7 implementing this approach and I will cover the issue you
described in the unit-tests I added.
>
> Would this solve the problems and at the same time make most use of the
> existing mechanism? Again, sorry that I couldn't review it earlier before
> this many revisions.
no worries :)
Regards,
Lorenzo
>
> Thanks,
> Han
>
> > >
> > > Thanks,
> > > Han
> > >
> > >> ---
> > >> controller/lflow.c | 28 +----
> > >> controller/lflow.h | 1 +
> > >> controller/ofctrl.c | 34 ++----
> > >> controller/ofctrl.h | 6 +-
> > >> controller/ovn-controller.c | 230
> +++++++++++++++++++++++++++++++++++-
> > >> include/ovn/actions.h | 1 +
> > >> lib/actions.c | 7 +-
> > >> lib/ovn-util.c | 9 ++
> > >> lib/ovn-util.h | 15 +++
> > >> tests/ovn-controller.at | 6 +-
> > >> tests/ovn-performance.at | 22 ++++
> > >> tests/ovn.at | 69 ++++++++++-
> > >> tests/system-ovn.at | 17 +++
> > >> 13 files changed, 382 insertions(+), 63 deletions(-)
> > >>
> > >> diff --git a/controller/lflow.c b/controller/lflow.c
> > >> index e169edef1..83cbc5b87 100644
> > >> --- a/controller/lflow.c
> > >> +++ b/controller/lflow.c
> > >> @@ -1083,27 +1083,6 @@ lflow_handle_changed_ref(enum ref_type ref_type,
> > > const char *ref_name,
> > >> return ret;
> > >> }
> > >>
> > >> -static void
> > >> -lflow_parse_ctrl_meter(const struct sbrec_logical_flow *lflow,
> > >> - struct ovn_extend_table *meter_table,
> > >> - uint32_t *meter_id)
> > >> -{
> > >> - ovs_assert(meter_id);
> > >> - *meter_id = NX_CTLR_NO_METER;
> > >> -
> > >> - if (lflow->controller_meter) {
> > >> - *meter_id = ovn_extend_table_assign_id(meter_table,
> > >> -
> lflow->controller_meter,
> > >> - lflow->header_.uuid);
> > >> - if (*meter_id == EXT_TABLE_ID_INVALID) {
> > >> - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5,
> > > 1);
> > >> - VLOG_WARN_RL(&rl, "Unable to assign id for meter: %s",
> > >> - lflow->controller_meter);
> > >> - return;
> > >> - }
> > >> - }
> > >> -}
> > >> -
> > >> static void
> > >> add_matches_to_flow_table(const struct sbrec_logical_flow *lflow,
> > >> const struct local_datapath *ldp,
> > >> @@ -1126,8 +1105,10 @@ add_matches_to_flow_table(const struct
> > > sbrec_logical_flow *lflow,
> > >> * controller.
> > >> */
> > >> uint32_t ctrl_meter_id = NX_CTLR_NO_METER;
> > >> - lflow_parse_ctrl_meter(lflow, l_ctx_out->meter_table,
> > >> - &ctrl_meter_id);
> > >> + if (lflow->controller_meter) {
> > >> + ctrl_meter_id =
> > > ovn_controller_get_meter_id(l_ctx_in->meter_table,
> > >> +
> > > lflow->controller_meter);
> > >> + }
> > >>
> > >> /* Encode OVN logical actions into OpenFlow. */
> > >> uint64_t ofpacts_stub[1024 / 8];
> > >> @@ -1153,6 +1134,7 @@ add_matches_to_flow_table(const struct
> > > sbrec_logical_flow *lflow,
> > >> .fdb_ptable = OFTABLE_GET_FDB,
> > >> .fdb_lookup_ptable = OFTABLE_LOOKUP_FDB,
> > >> .ctrl_meter_id = ctrl_meter_id,
> > >> + .meter_hash = l_ctx_in->meter_table,
> > >> };
> > >> ovnacts_encode(ovnacts->data, ovnacts->size, &ep, &ofpacts);
> > >>
> > >> diff --git a/controller/lflow.h b/controller/lflow.h
> > >> index d61733bc2..4760c5f62 100644
> > >> --- a/controller/lflow.h
> > >> +++ b/controller/lflow.h
> > >> @@ -153,6 +153,7 @@ struct lflow_ctx_in {
> > >> const struct sset *active_tunnels;
> > >> const struct sset *related_lport_ids;
> > >> const struct hmap *chassis_tunnels;
> > >> + const struct shash *meter_table;
> > >> };
> > >>
> > >> struct lflow_ctx_out {
> > >> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> > >> index 19aa787f9..2f484c662 100644
> > >> --- a/controller/ofctrl.c
> > >> +++ b/controller/ofctrl.c
> > >> @@ -369,8 +369,6 @@ static enum mf_field_id mff_ovn_geneve;
> > >> * is restarted, even if there is no change in the desired flow
> table. */
> > >> static bool need_reinstall_flows;
> > >>
> > >> -static ovs_be32 queue_msg(struct ofpbuf *);
> > >> -
> > >> static struct ofpbuf *encode_flow_mod(struct ofputil_flow_mod *);
> > >>
> > >> static struct ofpbuf *encode_group_mod(const struct
> ofputil_group_mod *);
> > >> @@ -822,7 +820,7 @@ ofctrl_get_cur_cfg(void)
> > >> return cur_cfg;
> > >> }
> > >>
> > >> -static ovs_be32
> > >> +ovs_be32
> > >> queue_msg(struct ofpbuf *msg)
> > >> {
> > >> const struct ofp_header *oh = msg->data;
> > >> @@ -1978,29 +1976,16 @@ add_meter_string(struct ovn_extend_table_info
> > > *m_desired,
> > >> free(meter_string);
> > >> }
> > >>
> > >> -static void
> > >> -add_meter(struct ovn_extend_table_info *m_desired,
> > >> - const struct sbrec_meter_table *meter_table,
> > >> - struct ovs_list *msgs)
> > >> +void
> > >> +meter_create_msg(struct ovs_list *msgs,
> > >> + const struct sbrec_meter *sb_meter,
> > >> + int cmd, int id)
> > >> {
> > >> - 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.meter.flags = OFPMF13_STATS;
> > >> + mm.command = cmd;
> > >> + mm.meter.meter_id = id;
> > >>
> > >> + mm.meter.flags = OFPMF13_STATS;
> > >> if (!strcmp(sb_meter->unit, "pktps")) {
> > >> mm.meter.flags |= OFPMF13_PKTPS;
> > >> } else {
> > >> @@ -2329,7 +2314,6 @@ void
> > >> ofctrl_put(struct ovn_desired_flow_table *lflow_table,
> > >> struct ovn_desired_flow_table *pflow_table,
> > >> struct shash *pending_ct_zones,
> > >> - const struct sbrec_meter_table *meter_table,
> > >> uint64_t req_cfg,
> > >> bool lflows_changed,
> > >> bool pflows_changed)
> > >> @@ -2414,8 +2398,6 @@ ofctrl_put(struct ovn_desired_flow_table
> > > *lflow_table,
> > >> /* 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);
> > >> }
> > >> }
> > >>
> > >> diff --git a/controller/ofctrl.h b/controller/ofctrl.h
> > >> index ad8f4be65..7c1844a72 100644
> > >> --- a/controller/ofctrl.h
> > >> +++ b/controller/ofctrl.h
> > >> @@ -29,6 +29,7 @@ struct match;
> > >> struct ofpbuf;
> > >> struct ovsrec_bridge;
> > >> struct sbrec_meter_table;
> > >> +struct sbrec_meter;
> > >> struct shash;
> > >>
> > >> struct ovn_desired_flow_table {
> > >> @@ -55,7 +56,6 @@ enum mf_field_id ofctrl_get_mf_field_id(void);
> > >> void ofctrl_put(struct ovn_desired_flow_table *lflow_table,
> > >> struct ovn_desired_flow_table *pflow_table,
> > >> struct shash *pending_ct_zones,
> > >> - const struct sbrec_meter_table *,
> > >> uint64_t nb_cfg,
> > >> bool lflow_changed,
> > >> bool pflow_changed);
> > >> @@ -145,5 +145,9 @@ void ofctrl_check_and_add_flow_metered(struct
> > > ovn_desired_flow_table *,
> > >> bool ofctrl_is_connected(void);
> > >> void ofctrl_set_probe_interval(int probe_interval);
> > >> void ofctrl_get_memory_usage(struct simap *usage);
> > >> +ovs_be32 queue_msg(struct ofpbuf *msg);
> > >> +void meter_create_msg(struct ovs_list *msgs,
> > >> + const struct sbrec_meter *sb_meter,
> > >> + int cmd, int id);
> > >>
> > >> #endif /* controller/ofctrl.h */
> > >> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > >> index e4b8b1bdd..805f5dbf3 100644
> > >> --- a/controller/ovn-controller.c
> > >> +++ b/controller/ovn-controller.c
> > >> @@ -76,6 +76,8 @@
> > >> #include "stopwatch.h"
> > >> #include "lib/inc-proc-eng.h"
> > >> #include "hmapx.h"
> > >> +#include "openvswitch/ofp-util.h"
> > >> +#include "openvswitch/ofp-meter.h"
> > >>
> > >> VLOG_DEFINE_THIS_MODULE(main);
> > >>
> > >> @@ -968,7 +970,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,
> > >> @@ -1543,6 +1546,221 @@ addr_sets_sb_address_set_handler(struct
> > > engine_node *node, void *data)
> > >> return true;
> > >> }
> > >>
> > >> +struct ed_type_meter {
> > >> + unsigned long *ids;
> > >> + struct shash meter_sets;
> > >> +};
> > >> +
> > >> +static void *
> > >> +en_meter_init(struct engine_node *node OVS_UNUSED,
> > >> + struct engine_arg *arg OVS_UNUSED)
> > >> +{
> > >> + struct ed_type_meter *m = xzalloc(sizeof *m);
> > >> + m->ids = bitmap_allocate(MAX_METER_ID);
> > >> + bitmap_set1(m->ids, 0); /* table id 0 is invalid. */
> > >> + shash_init(&m->meter_sets);
> > >> +
> > >> + return m;
> > >> +}
> > >> +
> > >> +static bool
> > >> +en_meter_data_check_bands(struct ed_meter_data *md,
> > >> + const struct sbrec_meter *sb_meter)
> > >> +{
> > >> + if (md->n_bands != sb_meter->n_bands) {
> > >> + return true;
> > >> + }
> > >> +
> > >> + for (int i = 0; i < sb_meter->n_bands; i++) {
> > >> + int j;
> > >> + for (j = 0; j < md->n_bands; j++) {
> > >> + if (sb_meter->bands[i]->rate == md->bands[j].rate &&
> > >> + sb_meter->bands[i]->burst_size ==
> > > md->bands[j].burst_size) {
> > >> + break;
> > >> + }
> > >> + }
> > >> + if (j == md->n_bands) {
> > >> + return true;
> > >> + }
> > >> + }
> > >> +
> > >> + return false;
> > >> +}
> > >> +
> > >> +static void
> > >> +en_meter_data_update(struct ed_meter_data *md,
> > >> + const struct sbrec_meter *sb_meter)
> > >> +{
> > >> + free(md->bands);
> > >> + md->n_bands = sb_meter->n_bands;
> > >> + md->bands = xcalloc(md->n_bands, sizeof *md->bands);
> > >> + for (int i = 0; i < sb_meter->n_bands; i++) {
> > >> + md->bands[i].rate = sb_meter->bands[i]->rate;
> > >> + md->bands[i].burst_size = sb_meter->bands[i]->burst_size;
> > >> + }
> > >> +}
> > >> +
> > >> +static struct ed_meter_data *
> > >> +en_meter_data_alloc(struct ed_type_meter *m,
> > >> + const struct sbrec_meter *sb_meter)
> > >> +{
> > >> + uint32_t id = bitmap_scan(m->ids, 0, 1, MAX_METER_ID + 1);
> > >> + if (id == MAX_METER_ID + 1) {
> > >> + static struct vlog_rate_limit rl =
> > >> + VLOG_RATE_LIMIT_INIT(1, 1);
> > >> + VLOG_ERR_RL(&rl, "%"PRIu32" out of meter ids.", id);
> > >> + return NULL;
> > >> + }
> > >> +
> > >> + struct ed_meter_data *md = xzalloc(sizeof *md);
> > >> + bitmap_set1(m->ids, id);
> > >> + md->id = id;
> > >> + en_meter_data_update(md, sb_meter);
> > >> + shash_add(&m->meter_sets, sb_meter->name, md);
> > >> +
> > >> + return md;
> > >> +}
> > >> +
> > >> +static void
> > >> +en_meter_data_destroy(struct ed_type_meter *m, struct ed_meter_data
> *md)
> > >> +{
> > >> + bitmap_set0(m->ids, md->id);
> > >> + free(md->bands);
> > >> + free(md);
> > >> +}
> > >> +
> > >> +static void
> > >> +en_meter_send_msgs(struct ovs_list *msgs)
> > >> +{
> > >> + if (ovs_list_is_empty(msgs)) {
> > >> + return;
> > >> + }
> > >> +
> > >> + struct ofpbuf *barrier =
> > > ofputil_encode_barrier_request(OFP15_VERSION);
> > >> + ovs_list_push_back(msgs, &barrier->list_node);
> > >> + /* Queue the messages. */
> > >> + struct ofpbuf *msg;
> > >> + LIST_FOR_EACH_POP (msg, list_node, msgs) {
> > >> + queue_msg(msg);
> > >> + }
> > >> +}
> > >> +
> > >> +static void
> > >> +en_meter_delete_msg(struct ovs_list *msgs, uint32_t id)
> > >> +{
> > >> + struct ofputil_meter_mod mm = {
> > >> + .command = OFPMC13_DELETE,
> > >> + .meter = { .meter_id = id },
> > >> + };
> > >> + struct ofpbuf *msg = ofputil_encode_meter_mod(OFP15_VERSION, &mm);
> > >> + ovs_list_push_back(msgs, &msg->list_node);
> > >> +}
> > >> +
> > >> +static void
> > >> +en_meter_cleanup(void *data)
> > >> +{
> > >> + struct ed_type_meter *m = data;
> > >> +
> > >> + struct shash_node *node, *next;
> > >> + SHASH_FOR_EACH_SAFE (node, next, &m->meter_sets) {
> > >> + struct ed_meter_data *md = node->data;
> > >> + shash_delete(&m->meter_sets, node);
> > >> + en_meter_data_destroy(m, md);
> > >> + }
> > >> + shash_destroy(&m->meter_sets);
> > >> + bitmap_free(m->ids);
> > >> +}
> > >> +
> > >> +static void
> > >> +en_meter_run(struct engine_node *node, void *data)
> > >> +{
> > >> + struct ovs_list msgs = OVS_LIST_INITIALIZER(&msgs);
> > >> + const struct sbrec_meter *sb_meter;
> > >> + struct ed_type_meter *m = data;
> > >> + struct ed_meter_data *md;
> > >> +
> > >> + struct sbrec_meter_table *m_table =
> > >> + (struct sbrec_meter_table *)EN_OVSDB_GET(
> > >> + engine_get_input("SB_meter", node));
> > >> +
> > >> + /* remove stale entries */
> > >> + struct shash_node *iter, *next;
> > >> + SHASH_FOR_EACH_SAFE (iter, next, &m->meter_sets) {
> > >> + bool found = false;
> > >> + SBREC_METER_TABLE_FOR_EACH (sb_meter, m_table) {
> > >> + if (!strcmp(sb_meter->name, iter->name)) {
> > >> + found = true;
> > >> + break;
> > >> + }
> > >> + }
> > >> + if (!found) {
> > >> + md = iter->data;
> > >> + shash_delete(&m->meter_sets, iter);
> > >> + en_meter_delete_msg(&msgs, md->id);
> > >> + en_meter_data_destroy(m, md);
> > >> + }
> > >> + }
> > >> +
> > >> + /* add new entries or update current ones */
> > >> + SBREC_METER_TABLE_FOR_EACH (sb_meter, m_table) {
> > >> + md = shash_find_data(&m->meter_sets, sb_meter->name);
> > >> + if (md) {
> > >> + if (en_meter_data_check_bands(md, sb_meter)) {
> > >> + en_meter_data_update(md, sb_meter);
> > >> + meter_create_msg(&msgs, sb_meter, OFPMC13_MODIFY,
> > > md->id);
> > >> + }
> > >> + } else {
> > >> + md = en_meter_data_alloc(m, sb_meter);
> > >> + if (md) {
> > >> + meter_create_msg(&msgs, sb_meter, OFPMC13_ADD,
> md->id);
> > >> + }
> > >> + }
> > >> + }
> > >> +
> > >> + en_meter_send_msgs(&msgs);
> > >> + engine_set_node_state(node, EN_UPDATED);
> > >> +}
> > >> +
> > >> +static bool
> > >> +meter_sb_meter_handler(struct engine_node *node, void *data)
> > >> +{
> > >> + struct ovs_list msgs = OVS_LIST_INITIALIZER(&msgs);
> > >> + struct ed_type_meter *m = data;
> > >> + struct sbrec_meter_table *m_table =
> > >> + (struct sbrec_meter_table *)EN_OVSDB_GET(
> > >> + engine_get_input("SB_meter", node));
> > >> +
> > >> + const struct sbrec_meter *iter;
> > >> + SBREC_METER_TABLE_FOR_EACH_TRACKED (iter, m_table) {
> > >> + struct ed_meter_data *md;
> > >> + if (sbrec_meter_is_deleted(iter)) {
> > >> + md = shash_find_and_delete(&m->meter_sets, iter->name);
> > >> + if (md) {
> > >> + en_meter_delete_msg(&msgs, md->id);
> > >> + en_meter_data_destroy(m, md);
> > >> + engine_set_node_state(node, EN_UPDATED);
> > >> + }
> > >> + } else {
> > >> + if (sbrec_meter_is_new(iter)) {
> > >> + md = en_meter_data_alloc(m, iter);
> > >> + if (md) {
> > >> + meter_create_msg(&msgs, iter, OFPMC13_ADD,
> md->id);
> > >> + engine_set_node_state(node, EN_UPDATED);
> > >> + }
> > >> + } else {
> > >> + md = shash_find_data(&m->meter_sets, iter->name);
> > >> + if (md) {
> > >> + en_meter_data_update(md, iter);
> > >> + }
> > >> + meter_create_msg(&msgs, iter, OFPMC13_MODIFY, md->id);
> > >> + }
> > >> + }
> > >> + }
> > >> + en_meter_send_msgs(&msgs);
> > >> +
> > >> + return true;
> > >> +}
> > >> +
> > >> struct ed_type_port_groups{
> > >> /* A copy of SB port_groups, each converted as a sset for
> efficient
> > > lport
> > >> * lookup. */
> > >> @@ -2298,6 +2516,10 @@ init_lflow_ctx(struct engine_node *node,
> > >> (struct ovsrec_open_vswitch_table *)EN_OVSDB_GET(
> > >> engine_get_input("OVS_open_vswitch", node));
> > >>
> > >> + struct ed_type_meter *meter_data =
> > >> + engine_get_input_data("meter", node);
> > >> + struct shash *meter_sets = &meter_data->meter_sets;
> > >> +
> > >> const char *chassis_id = get_ovs_chassis_id(ovs_table);
> > >> const struct sbrec_chassis *chassis = NULL;
> > >> struct ovsdb_idl_index *sbrec_chassis_by_name =
> > >> @@ -2348,6 +2570,7 @@ init_lflow_ctx(struct engine_node *node,
> > >> l_ctx_in->active_tunnels = &rt_data->active_tunnels;
> > >> l_ctx_in->related_lport_ids = &rt_data->related_lports.lport_ids;
> > >> l_ctx_in->chassis_tunnels = &non_vif_data->chassis_tunnels;
> > >> + l_ctx_in->meter_table = meter_sets;
> > >>
> > >> l_ctx_out->flow_table = &fo->flow_table;
> > >> l_ctx_out->group_table = &fo->group_table;
> > >> @@ -3222,6 +3445,7 @@ main(int argc, char *argv[])
> > >> ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lflow_output,
> > > "logical_flow_output");
> > >> ENGINE_NODE(flow_output, "flow_output");
> > >> ENGINE_NODE_WITH_CLEAR_TRACK_DATA(addr_sets, "addr_sets");
> > >> + ENGINE_NODE(meter, "meter");
> > >> ENGINE_NODE_WITH_CLEAR_TRACK_DATA(port_groups, "port_groups");
> > >>
> > >> #define SB_NODE(NAME, NAME_STR) ENGINE_NODE_SB(NAME, NAME_STR);
> > >> @@ -3243,6 +3467,8 @@ main(int argc, char *argv[])
> > >> engine_add_input(&en_port_groups, &en_runtime_data,
> > >> port_groups_runtime_data_handler);
> > >>
> > >> + engine_add_input(&en_meter, &en_sb_meter, meter_sb_meter_handler);
> > >> +
> > >> engine_add_input(&en_non_vif_data, &en_ovs_open_vswitch, NULL);
> > >> engine_add_input(&en_non_vif_data, &en_ovs_bridge, NULL);
> > >> engine_add_input(&en_non_vif_data, &en_sb_chassis, NULL);
> > >> @@ -3287,6 +3513,7 @@ main(int argc, char *argv[])
> > >> lflow_output_runtime_data_handler);
> > >> engine_add_input(&en_lflow_output, &en_non_vif_data,
> > >> NULL);
> > >> + engine_add_input(&en_lflow_output, &en_meter, NULL);
> > >>
> > >> engine_add_input(&en_lflow_output, &en_sb_multicast_group,
> > >> lflow_output_sb_multicast_group_handler);
> > >> @@ -3768,7 +3995,6 @@ main(int argc, char *argv[])
> > >> ofctrl_put(&lflow_output_data->flow_table,
> > >> &pflow_output_data->flow_table,
> > >> &ct_zones_data->pending,
> > >> -
> > > sbrec_meter_table_get(ovnsb_idl_loop.idl),
> > >> ofctrl_seqno_get_req_cfg(),
> > >>
> engine_node_changed(&en_lflow_output),
> > >>
> > > engine_node_changed(&en_pflow_output));
> > >> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> > >> index 0641b927e..bed24d9d1 100644
> > >> --- a/include/ovn/actions.h
> > >> +++ b/include/ovn/actions.h
> > >> @@ -799,6 +799,7 @@ struct ovnact_encode_params {
> > >> * 'lookup_fdb' to resubmit. */
> > >> uint32_t ctrl_meter_id; /* Meter to be used if the resulting
> flow
> > >> sends packets to controller. */
> > >> + const struct shash *meter_hash;
> > >> };
> > >>
> > >> void ovnacts_encode(const struct ovnact[], size_t ovnacts_len,
> > >> diff --git a/lib/actions.c b/lib/actions.c
> > >> index 5d3caaf2b..f7f5aae52 100644
> > >> --- a/lib/actions.c
> > >> +++ b/lib/actions.c
> > >> @@ -3411,10 +3411,9 @@ encode_LOG(const struct ovnact_log *log,
> > >> {
> > >> uint32_t meter_id = NX_CTLR_NO_METER;
> > >>
> > >> - if (log->meter) {
> > >> - meter_id = ovn_extend_table_assign_id(ep->meter_table,
> > > log->meter,
> > >> - ep->lflow_uuid);
> > >> - if (meter_id == EXT_TABLE_ID_INVALID) {
> > >> + if (ep->meter_hash && log->meter) {
> > >> + meter_id = ovn_controller_get_meter_id(ep->meter_hash,
> > > log->meter);
> > >> + if (meter_id == MAX_METER_ID + 1) {
> > >> VLOG_WARN("Unable to assign id for log meter: %s",
> > > log->meter);
> > >> return;
> > >> }
> > >> diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> > >> index a22ae84fe..b7e9d9b2c 100644
> > >> --- a/lib/ovn-util.c
> > >> +++ b/lib/ovn-util.c
> > >> @@ -30,6 +30,8 @@
> > >> #include "socket-util.h"
> > >> #include "svec.h"
> > >> #include "unixctl.h"
> > >> +#include "controller/ovn-controller.h"
> > >> +#include "openvswitch/ofp-actions.h"
> > >>
> > >> VLOG_DEFINE_THIS_MODULE(ovn_util);
> > >>
> > >> @@ -806,3 +808,10 @@ get_bridge(const struct ovsrec_bridge_table
> > > *bridge_table, const char *br_name)
> > >> }
> > >> return NULL;
> > >> }
> > >> +
> > >> +uint32_t
> > >> +ovn_controller_get_meter_id(const struct shash *meter_sets, const char
> > > *name)
> > >> +{
> > >> + struct ed_meter_data *md = shash_find_data(meter_sets, name);
> > >> + return md ? md->id : NX_CTLR_NO_METER;
> > >> +}
> > >> diff --git a/lib/ovn-util.h b/lib/ovn-util.h
> > >> index 1fe91ba99..c5bb21d2d 100644
> > >> --- a/lib/ovn-util.h
> > >> +++ b/lib/ovn-util.h
> > >> @@ -303,5 +303,20 @@ struct ovsrec_bridge_table;
> > >> const struct ovsrec_bridge *get_bridge(const struct
> ovsrec_bridge_table
> > > *,
> > >> const char *br_name);
> > >>
> > >> +#define MAX_METER_ID (MAX_EXT_TABLE_ID / 2)
> > >> +struct ed_meter_band_data {
> > >> + int64_t burst_size;
> > >> + int64_t rate;
> > >> +};
> > >> +
> > >> +struct ed_meter_data {
> > >> + uint32_t id; /* ovs meter id */
> > >> + struct ed_meter_band_data *bands;
> > >> + size_t n_bands;
> > >> +};
> > >> +
> > >> +struct shash;
> > >> +uint32_t ovn_controller_get_meter_id(const struct shash *meter_sets,
> > >> + const char *name);
> > >>
> > >> #endif /* OVN_UTIL_H */
> > >> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> > >> index f5e5448a4..6e4c24f0f 100644
> > >> --- a/tests/ovn-controller.at
> > >> +++ b/tests/ovn-controller.at
> > >> @@ -725,7 +725,7 @@ check ovn-nbctl meter-add event-elb drop 100 pktps
> 10
> > >> check ovn-nbctl --wait=hv copp-add copp0 event-elb event-elb
> > >> check ovn-nbctl --wait=hv ls-copp-add copp0 ls1
> > >>
> > >> -AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep controller | grep
> > > userdata=00.00.00.0f | grep -q meter_id=32768])
> > >> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep controller | grep
> > > userdata=00.00.00.0f | grep -q meter_id=1])
> > >>
> > >> check ovn-nbctl copp-del copp0
> > >> AT_CHECK([ovn-nbctl copp-list copp0], [0], [dnl
> > >> @@ -738,13 +738,13 @@ check ovn-nbctl --wait=hv copp-add copp1 reject
> > > acl-meter
> > >> check ovn-nbctl ls-copp-add copp1 ls1
> > >> check ovn-nbctl --wait=hv acl-add ls1 from-lport 1002 'inport ==
> "lsp1"
> > > && ip && udp' reject
> > >>
> > >> -AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep controller | grep
> > > userdata=00.00.00.16 | grep -q meter_id=32768])
> > >> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep controller | grep
> > > userdata=00.00.00.16 | grep -q meter_id=1])
> > >>
> > >> # arp metering
> > >> check ovn-nbctl meter-add arp-meter drop 200 pktps 0
> > >> check ovn-nbctl --wait=hv copp-add copp2 arp-resolve arp-meter
> > >> check ovn-nbctl --wait=hv lr-copp-add copp2 lr1
> > >> -AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep controller | grep
> > > userdata=00.00.00.00 | grep -q meter_id=32769])
> > >> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep controller | grep
> > > userdata=00.00.00.00 | grep -q meter_id=2])
> > >>
> > >> OVN_CLEANUP([hv1])
> > >> AT_CLEANUP
> > >> diff --git a/tests/ovn-performance.at b/tests/ovn-performance.at
> > >> index 10341ad72..aed1cb53a 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_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 7adf6003d..37bd4fce7 100644
> > >> --- a/tests/ovn.at
> > >> +++ b/tests/ovn.at
> > >> @@ -9158,9 +9158,10 @@ echo "Meter duration: $d_secs"
> > >> AT_SKIP_IF([test $d_secs -gt 9])
> > >>
> > >> # Print some information that may help debugging.
> > >> -AT_CHECK([as hv ovs-appctl -t ovn-controller meter-table-list], [0],
> [dnl
> > >> -http-rl1: 32768
> > >> -http-rl2: 32769
> > >> +
> > >> +AT_CHECK([as hv ovs-ofctl dump-meters br-int -O OpenFlow13 | grep
> > > meter], [0], [dnl
> > >> +meter=1 pktps stats bands=
> > >> +meter=2 pktps stats bands=
> > >> ])
> > >> as hv ovs-ofctl -O OpenFlow13 meter-stats br-int
> > >>
> > >> @@ -9223,9 +9224,12 @@ ovn-nbctl acl-del lsw0 to-lport 1000
> 'tcp.dst==80'
> > >> ovn-nbctl --wait=hv sync
> > >> AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br-int | grep meter],
> [0],
> > > [ignore], [ignore])
> > >>
> > >> -# Delete acl2, meter should be deleted in OVS
> > >> ovn-nbctl acl-del lsw0 to-lport 1000 'tcp.dst==81'
> > >> ovn-nbctl --wait=hv sync
> > >> +AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br-int | grep meter],
> [0],
> > > [ignore], [ignore])
> > >> +
> > >> +ovn-nbctl meter-del http-rl1
> > >> +ovn-nbctl --wait=hv sync
> > >> AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br-int | grep meter],
> [1])
> > >>
> > >> OVN_CLEANUP([hv])
> > >> @@ -29647,3 +29651,60 @@ 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 meter-add meter0 drop 10 pktps
> > >> +check ovn-nbctl --log --severity=alert --meter=meter0 \
> > >> + --name=http acl-add sw0 to-lport 1000 'tcp.dst == 80'
> > > drop
> > >> +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 20 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=20], [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])
> > >> +
> > >> +OVN_CLEANUP([hv1])
> > >> +AT_CLEANUP
> > >> +])
> > >> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> > >> index 2dcd7e906..e6c317083 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
> > >>
> > >> _______________________________________________
> > >> dev mailing list
> > >> [email protected]
> > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > _______________________________________________
> > > dev mailing list
> > > [email protected]
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
> >
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
