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,
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.
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.
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
