Skip to content

Commit

Permalink
controller: reconfigure ovs meters for ovn meters
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@LorenzoBianconi @hzhou8
LorenzoBianconi authored and hzhou8 committed Mar 9, 2022
1 parent 74d82e2 commit 885655e
Show file tree
Hide file tree
Showing 7 changed files with 338 additions and 30 deletions.
212 changes: 183 additions & 29 deletions controller/ofctrl.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,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. */
Expand Down Expand Up @@ -397,6 +413,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.
Expand Down Expand Up @@ -640,6 +657,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. */
Expand Down Expand Up @@ -814,6 +832,7 @@ ofctrl_destroy(void)
rconn_packet_counter_destroy(tx_counter);
expr_symtab_destroy(&symtab);
shash_destroy(&symtab);
ofctrl_meter_bands_destroy();
}

uint64_t
Expand Down Expand Up @@ -1979,26 +1998,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")) {
Expand Down Expand Up @@ -2031,6 +2038,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,
Expand Down Expand Up @@ -2409,13 +2562,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);
}
}

Expand Down Expand Up @@ -2505,12 +2664,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);
}

Expand Down
25 changes: 24 additions & 1 deletion controller/ovn-controller.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -968,7 +968,8 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl)
SB_NODE(dhcpv6_options, "dhcpv6_options") \
SB_NODE(dns, "dns") \
SB_NODE(load_balancer, "load_balancer") \
SB_NODE(fdb, "fdb")
SB_NODE(fdb, "fdb") \
SB_NODE(meter, "meter")

enum sb_engine_node {
#define SB_NODE(NAME, NAME_STR) SB_##NAME,
Expand Down Expand Up @@ -2718,6 +2719,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;
Expand Down Expand Up @@ -3316,6 +3337,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);
Expand Down
14 changes: 14 additions & 0 deletions lib/extend-table.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
4 changes: 4 additions & 0 deletions lib/extend-table.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.) */
Expand Down
22 changes: 22 additions & 0 deletions tests/ovn-performance.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -543,6 +543,28 @@ OVN_CONTROLLER_EXPECT_HIT(
[as hv3 ovs-vsctl set interface vgw3 external-ids:ovn-egress-iface=true]
)

ovn-nbctl --wait=hv meter-add meter0 drop 100 pktps 10

OVN_CONTROLLER_EXPECT_NO_HIT(
[hv1 hv2 hv3 hv4], [lflow_run],
[ovn-nbctl --wait=hv copp-add copp0 arp meter0]
)

OVN_CONTROLLER_EXPECT_NO_HIT(
[hv1 hv2 hv3 hv4], [lflow_run],
[ovn-nbctl --wait=hv lr-copp-add copp0 lr1]
)

OVN_CONTROLLER_EXPECT_NO_HIT(
[hv1 hv2 hv3 hv4], [lflow_run],
[ovn-nbctl --wait=hv --may-exist meter-add meter0 drop 200 pktps 10]
)

OVN_CONTROLLER_EXPECT_NO_HIT(
[hv1 hv2 hv3 hv4], [lflow_run],
[ovn-nbctl --wait=hv meter-del meter0]
)

for i in 1 2; do
j=$((i%2 + 1))
lp=lp$i
Expand Down
Loading

0 comments on commit 885655e

Please sign in to comment.