Skip to content

Commit

Permalink
QoS: Properly set qos when ovs db is read only
Browse files Browse the repository at this point in the history 
QoS was not configured in OVS db when db was read only: the configuration
was just ignored and not done later when OVS db became writable.
It was sometimes set later, if/when a recompute happened.
This is now fixed: when OVS db is read only, the ports on which qos
must be applied are stored and qos will be applied when OVS db becomes writable.
To avoid race conditions between delayed qos and new qos changes (e.g. a qos
configuration delayed in one loop as ovs is ro, followed in next loop, when ovs
becomes rw, by another qos on the same port), all qos changes are done at the
same time.

This issue was identified by some random failures in system test
"egress qos".

Reported-at: https://bugzilla.redhat.com/2234349
Signed-off-by: Xavier Simonart <xsimonar@redhat.com>
Acked-by: Ales Musil <amusil@redhat.com>
Signed-off-by: Numan Siddique <numans@ovn.org>
  • Loading branch information
@simonartxavier @numansiddique
simonartxavier authored and numansiddique committed Sep 15, 2023
1 parent b1f8d72 commit b039d26
Show file tree
Hide file tree
Showing 5 changed files with 125 additions and 46 deletions.
133 changes: 88 additions & 45 deletions controller/binding.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,13 @@ struct claimed_port {
long long int last_claimed;
};

struct qos_port {
bool added;
};

static struct shash _claimed_ports = SHASH_INITIALIZER(&_claimed_ports);
static struct sset _postponed_ports = SSET_INITIALIZER(&_postponed_ports);
static struct shash _qos_ports = SHASH_INITIALIZER(&_qos_ports);

static void
remove_additional_chassis(const struct sbrec_port_binding *pb,
Expand Down Expand Up @@ -218,14 +223,25 @@ get_qos_egress_port_interface(struct shash *bridge_mappings,
return NULL;
}

static void
add_or_del_qos_port(const char *ovn_port, bool add)
{
struct qos_port *qos_port = shash_find_data(&_qos_ports, ovn_port);
if (!qos_port) {
qos_port = xzalloc(sizeof *qos_port);
shash_add(&_qos_ports, ovn_port, qos_port);
}
qos_port->added = add;
}

/* 34359738360 == (2^32 - 1) * 8. netdev_set_qos() doesn't support
* 64-bit rate netlink attributes, so the maximum value is 2^32 - 1
* bytes. The 'max-rate' config option is in bits, so multiplying by 8.
* Without setting max-rate the reported link speed will be used, which
* can be unrecognized for certain NICs or reported too low for virtual
* interfaces. */
#define OVN_QOS_MAX_RATE 34359738360ULL
static void
static bool
add_ovs_qos_table_entry(struct ovsdb_idl_txn *ovs_idl_txn,
const struct ovsrec_port *port,
unsigned long long min_rate,
Expand All @@ -239,7 +255,7 @@ add_ovs_qos_table_entry(struct ovsdb_idl_txn *ovs_idl_txn,
const struct ovsrec_qos *qos = port->qos;
if (qos && !smap_get_bool(&qos->external_ids, "ovn_qos", false)) {
/* External configured QoS, do not overwrite it. */
return;
return false;
}

if (!qos) {
Expand Down Expand Up @@ -282,22 +298,18 @@ add_ovs_qos_table_entry(struct ovsdb_idl_txn *ovs_idl_txn,
ovsrec_queue_verify_external_ids(queue);
ovsrec_queue_set_external_ids(queue, &external_ids);
smap_destroy(&external_ids);
return true;
}

static void
remove_stale_qos_entry(struct ovsdb_idl_txn *ovs_idl_txn,
const struct sbrec_port_binding *pb,
remove_stale_qos_entry( const char *logical_port,
struct ovsdb_idl_index *ovsrec_port_by_qos,
const struct ovsrec_qos_table *qos_table,
struct hmap *queue_map)
{
if (!ovs_idl_txn) {
return;
}

struct qos_queue *q = find_qos_queue(
queue_map, hash_string(pb->logical_port, 0),
pb->logical_port);
queue_map, hash_string(logical_port, 0),
logical_port);
if (!q) {
return;
}
Expand Down Expand Up @@ -338,8 +350,12 @@ remove_stale_qos_entry(struct ovsdb_idl_txn *ovs_idl_txn,

static void
configure_qos(const struct sbrec_port_binding *pb,
struct binding_ctx_in *b_ctx_in,
struct binding_ctx_out *b_ctx_out)
struct ovsdb_idl_txn *ovs_idl_txn,
struct ovsdb_idl_index *ovsrec_port_by_qos,
const struct ovsrec_qos_table *qos_table,
struct hmap *qos_map,
const struct ovsrec_open_vswitch_table *ovs_table,
const struct ovsrec_bridge_table *bridge_table)
{
unsigned long long min_rate = smap_get_ullong(
&pb->options, "qos_min_rate", 0);
Expand All @@ -351,20 +367,20 @@ configure_qos(const struct sbrec_port_binding *pb,

if ((!min_rate && !max_rate && !burst) || !queue_id) {
/* Qos is not configured for this port. */
remove_stale_qos_entry(b_ctx_in->ovs_idl_txn, pb,
b_ctx_in->ovsrec_port_by_qos,
b_ctx_in->qos_table, b_ctx_out->qos_map);
remove_stale_qos_entry(pb->logical_port,
ovsrec_port_by_qos,
qos_table, qos_map);
return;
}

const char *network = smap_get(&pb->options, "qos_physical_network");
uint32_t hash = hash_string(pb->logical_port, 0);
struct qos_queue *q = find_qos_queue(b_ctx_out->qos_map, hash,
struct qos_queue *q = find_qos_queue(qos_map, hash,
pb->logical_port);
if (!q || q->min_rate != min_rate || q->max_rate != max_rate ||
q->burst != burst || (network && strcmp(network, q->network))) {
struct shash bridge_mappings = SHASH_INITIALIZER(&bridge_mappings);
add_ovs_bridge_mappings(b_ctx_in->ovs_table, b_ctx_in->bridge_table,
add_ovs_bridge_mappings(ovs_table, bridge_table,
&bridge_mappings);

const struct ovsrec_port *port = NULL;
Expand All @@ -375,25 +391,58 @@ configure_qos(const struct sbrec_port_binding *pb,
}
if (iface) {
/* Add new QoS entries. */
add_ovs_qos_table_entry(b_ctx_in->ovs_idl_txn, port, min_rate,
if (add_ovs_qos_table_entry(ovs_idl_txn, port, min_rate,
max_rate, burst, queue_id,
pb->logical_port);
if (!q) {
q = xzalloc(sizeof *q);
hmap_insert(b_ctx_out->qos_map, &q->node, hash);
q->port = xstrdup(pb->logical_port);
q->queue_id = queue_id;
pb->logical_port)) {
if (!q) {
q = xzalloc(sizeof *q);
hmap_insert(qos_map, &q->node, hash);
q->port = xstrdup(pb->logical_port);
q->queue_id = queue_id;
}
free(q->network);
q->network = network ? xstrdup(network) : NULL;
q->min_rate = min_rate;
q->max_rate = max_rate;
q->burst = burst;
}
free(q->network);
q->network = network ? xstrdup(network) : NULL;
q->min_rate = min_rate;
q->max_rate = max_rate;
q->burst = burst;
}
shash_destroy(&bridge_mappings);
}
}

void
update_qos(struct ovsdb_idl_index *sbrec_port_binding_by_name,
struct ovsdb_idl_txn *ovs_idl_txn,
struct ovsdb_idl_index *ovsrec_port_by_qos,
const struct ovsrec_qos_table *qos_table,
struct hmap *qos_map,
const struct ovsrec_open_vswitch_table *ovs_table,
const struct ovsrec_bridge_table *bridge_table)
{
/* Remove qos for any ports for which we could not do it before */
const struct sbrec_port_binding *pb;

struct shash_node *node;
SHASH_FOR_EACH_SAFE (node, &_qos_ports) {
struct qos_port *qos_port = (struct qos_port *) node->data;
if (qos_port->added) {
pb = lport_lookup_by_name(sbrec_port_binding_by_name,
node->name);
if (pb) {
configure_qos(pb, ovs_idl_txn, ovsrec_port_by_qos, qos_table,
qos_map, ovs_table, bridge_table);
}
} else {
remove_stale_qos_entry(node->name,
ovsrec_port_by_qos,
qos_table, qos_map);
}
free(qos_port);
shash_delete(&_qos_ports, node);
}
}

static const struct ovsrec_queue *
find_qos_queue_by_external_ids(const struct smap *external_ids,
struct ovsdb_idl_index *ovsrec_queue_by_external_ids)
Expand Down Expand Up @@ -1524,8 +1573,8 @@ consider_vif_lport_(const struct sbrec_port_binding *pb,
tracked_datapath_lport_add(pb, TRACKED_RESOURCE_UPDATED,
b_ctx_out->tracked_dp_bindings);
}
if (b_lport->lbinding->iface && b_ctx_in->ovs_idl_txn) {
configure_qos(pb, b_ctx_in, b_ctx_out);
if (b_lport->lbinding->iface) {
add_or_del_qos_port(pb->logical_port, true);
}
} else {
/* We could, but can't claim the lport. */
Expand Down Expand Up @@ -1851,7 +1900,6 @@ consider_l3gw_lport(const struct sbrec_port_binding *pb,

static void
consider_localnet_lport(const struct sbrec_port_binding *pb,
struct binding_ctx_in *b_ctx_in,
struct binding_ctx_out *b_ctx_out)
{
struct local_datapath *ld
Expand All @@ -1864,10 +1912,7 @@ consider_localnet_lport(const struct sbrec_port_binding *pb,
* for them. */
update_local_lports(pb->logical_port, b_ctx_out);

if (b_ctx_in->ovs_idl_txn) {
configure_qos(pb, b_ctx_in, b_ctx_out);
}

add_or_del_qos_port(pb->logical_port, true);
update_related_lport(pb, b_ctx_out);
}

Expand Down Expand Up @@ -1988,6 +2033,7 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out)
return;
}

shash_clear_free_data(&_qos_ports);
struct shash bridge_mappings = SHASH_INITIALIZER(&bridge_mappings);

if (b_ctx_in->br_int) {
Expand Down Expand Up @@ -2103,7 +2149,7 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out)
* accordingly. */
struct lport *lnet_lport;
LIST_FOR_EACH_POP (lnet_lport, list_node, &localnet_lports) {
consider_localnet_lport(lnet_lport->pb, b_ctx_in, b_ctx_out);
consider_localnet_lport(lnet_lport->pb, b_ctx_out);
update_ld_localnet_port(lnet_lport->pb, &bridge_mappings,
b_ctx_out->local_datapaths);
free(lnet_lport);
Expand Down Expand Up @@ -2378,9 +2424,7 @@ consider_iface_release(const struct ovsrec_interface *iface_rec,
b_ctx_out, ld);
}

remove_stale_qos_entry(b_ctx_in->ovs_idl_txn, b_lport->pb,
b_ctx_in->ovsrec_port_by_qos,
b_ctx_in->qos_table, b_ctx_out->qos_map);
add_or_del_qos_port(b_lport->pb->logical_port, false);

/* Release the primary binding lport and other children lports if
* any. */
Expand Down Expand Up @@ -2938,7 +2982,7 @@ handle_updated_port(struct binding_ctx_in *b_ctx_in,
break;

case LP_LOCALNET: {
consider_localnet_lport(pb, b_ctx_in, b_ctx_out);
consider_localnet_lport(pb, b_ctx_out);

struct shash bridge_mappings =
SHASH_INITIALIZER(&bridge_mappings);
Expand Down Expand Up @@ -3026,9 +3070,7 @@ binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in,
shash_add(&deleted_other_pbs, pb->logical_port, pb);
}

remove_stale_qos_entry(b_ctx_in->ovs_idl_txn, pb,
b_ctx_in->ovsrec_port_by_qos,
b_ctx_in->qos_table, b_ctx_out->qos_map);
add_or_del_qos_port(pb->logical_port, false);
}

struct shash_node *node;
Expand Down Expand Up @@ -3140,7 +3182,7 @@ binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in,
b_ctx_in->sbrec_port_binding_by_datapath) {
enum en_lport_type lport_type = get_lport_type(pb);
if (lport_type == LP_LOCALNET) {
consider_localnet_lport(pb, b_ctx_in, b_ctx_out);
consider_localnet_lport(pb, b_ctx_out);
update_ld_localnet_port(pb, &bridge_mappings,
b_ctx_out->local_datapaths);
} else if (lport_type == LP_EXTERNAL) {
Expand Down Expand Up @@ -3614,5 +3656,6 @@ void
binding_destroy(void)
{
shash_destroy_free_data(&_claimed_ports);
shash_destroy_free_data(&_qos_ports);
sset_clear(&_postponed_ports);
}
8 changes: 8 additions & 0 deletions controller/binding.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -250,4 +250,12 @@ void binding_destroy(void);

void destroy_qos_map(struct hmap *);

void update_qos(struct ovsdb_idl_index * sbrec_port_binding_by_name,
struct ovsdb_idl_txn *ovs_idl_txn,
struct ovsdb_idl_index *ovsrec_port_by_qos,
const struct ovsrec_qos_table *qos_table,
struct hmap *qos_map,
const struct ovsrec_open_vswitch_table *ovs_table,
const struct ovsrec_bridge_table *bridge_table);

#endif /* controller/binding.h */
7 changes: 7 additions & 0 deletions controller/ovn-controller.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5809,6 +5809,13 @@ main(int argc, char *argv[])
&runtime_data->local_datapaths,
sb_monitor_all);
}
if (ovs_idl_txn) {
update_qos(sbrec_port_binding_by_name, ovs_idl_txn,
ovsrec_port_by_qos,
ovsrec_qos_table_get(ovs_idl_loop.idl),
&runtime_data->qos_map,
ovs_table, bridge_table);
}
}

if (mac_cache_data) {
Expand Down
1 change: 0 additions & 1 deletion tests/ovn.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36728,7 +36728,6 @@ check ovn-nbctl lsp-add ls2 public2
check ovn-nbctl lsp-set-addresses public2 unknown
check ovn-nbctl lsp-set-type public2 localnet
check ovn-nbctl --wait=sb set Logical_Switch_Port public2 options:qos_min_rate=6000000000 options:qos_max_rate=7000000000 options:qos_burst=8000000000 options:network_name=phys
check ovn-nbctl --wait=sb lsp-set-options public2 qos_min_rate=6000000000 qos_max_rate=7000000000 qos_burst=8000000000

# Let's now send ovn controller to sleep, so it will receive both ofport notification and ls deletion simultaneously
sleep_controller hv-1
Expand Down
22 changes: 22 additions & 0 deletions tests/system-ovn.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6630,6 +6630,28 @@ AT_CHECK([ovn-nbctl set Logical_Switch_Port ext options:qos_min_rate=400000])
AT_CHECK([ovn-nbctl set Logical_Switch_Port ext options:qos_max_rate=600000])
AT_CHECK([ovn-nbctl set Logical_Switch_Port ext options:qos_burst=6000000])

OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-public'])
OVS_WAIT_UNTIL([tc class show dev ovs-public | \
grep -q 'class htb .* rate 200Kbit ceil 300Kbit burst 375000b cburst 375000b'])

OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-ext'])
OVS_WAIT_UNTIL([tc class show dev ovs-ext | \
grep -q 'class htb .* rate 400Kbit ceil 600Kbit burst 750000b cburst 750000b'])

# The same now with ovs db read only
#
AT_CHECK([ovn-nbctl remove Logical_Switch_Port ext options qos_min_rate=400000])
AT_CHECK([ovn-nbctl remove Logical_Switch_Port ext options qos_max_rate=600000])
AT_CHECK([ovn-nbctl remove Logical_Switch_Port ext options qos_burst=6000000])
OVS_WAIT_UNTIL([test "$(tc class show dev ovs-ext | grep 'class htb')" == ""])

sleep_ovsdb .

AT_CHECK([ovn-nbctl set Logical_Switch_Port ext options:qos_min_rate=400000])
AT_CHECK([ovn-nbctl set Logical_Switch_Port ext options:qos_max_rate=600000])
AT_CHECK([ovn-nbctl set Logical_Switch_Port ext options:qos_burst=6000000])
wake_up_ovsdb .

OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-public'])
OVS_WAIT_UNTIL([tc class show dev ovs-public | \
grep -q 'class htb .* rate 200Kbit ceil 300Kbit burst 375000b cburst 375000b'])
Expand Down

0 comments on commit b039d26

Please sign in to comment.