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". Signed-off-by: Xavier Simonart <[email protected]> --- v2: - rebased on origin/main - handled comments from Ales --- controller/binding.c | 133 ++++++++++++++++++++++++------------ controller/binding.h | 8 +++ controller/ovn-controller.c | 7 ++ tests/ovn.at | 1 - tests/system-ovn.at | 22 ++++++ 5 files changed, 125 insertions(+), 46 deletions(-) diff --git a/controller/binding.c b/controller/binding.c index a521f2828..fd08aaafa 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -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, @@ -218,6 +223,17 @@ 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. @@ -225,7 +241,7 @@ get_qos_egress_port_interface(struct shash *bridge_mappings, * 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, @@ -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) { @@ -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; } @@ -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); @@ -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; @@ -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) @@ -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. */ @@ -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 @@ -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); } @@ -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) { @@ -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); @@ -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. */ @@ -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); @@ -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; @@ -3140,7 +3182,7 @@ delete_done: 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) { @@ -3614,5 +3656,6 @@ void binding_destroy(void) { shash_destroy_free_data(&_claimed_ports); + shash_destroy_free_data(&_qos_ports); sset_clear(&_postponed_ports); } diff --git a/controller/binding.h b/controller/binding.h index 24bc84079..47df668a2 100644 --- a/controller/binding.h +++ b/controller/binding.h @@ -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 */ diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index b3e4e0da8..859d9cab9 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -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) { diff --git a/tests/ovn.at b/tests/ovn.at index e127530f6..ba5ce298a 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -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 diff --git a/tests/system-ovn.at b/tests/system-ovn.at index 59d0cb2a0..75611c1d5 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -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']) -- 2.31.1 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
