On Wed, Jul 26, 2023 at 3:44 PM Xavier Simonart <[email protected]> wrote:
> 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 and 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]> > Hi Xavier, I have two small comments below. --- > controller/binding.c | 131 ++++++++++++++++++++++++------------ > controller/binding.h | 8 +++ > controller/ovn-controller.c | 7 ++ > tests/ovn.at | 1 - > tests/system-ovn.at | 22 ++++++ > 5 files changed, 124 insertions(+), 45 deletions(-) > > diff --git a/controller/binding.c b/controller/binding.c > index 9aa3fc6c4..0e13624c1 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, > nit: space after * + 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) > @@ -1599,8 +1648,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. */ > @@ -1926,17 +1975,13 @@ 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) > { > /* Add all localnet ports to local_ifaces so that we allocate ct zones > * 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); > } > > @@ -2057,6 +2102,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) { > @@ -2143,7 +2189,7 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct > binding_ctx_out *b_ctx_out) > break; > > case LP_LOCALNET: { > - consider_localnet_lport(pb, b_ctx_in, b_ctx_out); > + consider_localnet_lport(pb, b_ctx_out); > struct lport *lnet_lport = xmalloc(sizeof *lnet_lport); > lnet_lport->pb = pb; > ovs_list_push_back(&localnet_lports, &lnet_lport->list_node); > @@ -2432,9 +2478,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. */ > @@ -2992,7 +3036,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); > @@ -3080,9 +3124,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; > @@ -3667,5 +3709,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 235e5860d..59a4654fb 100644 > --- a/controller/binding.h > +++ b/controller/binding.h > @@ -268,4 +268,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 236974f4f..21e03273c 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -5434,6 +5434,13 @@ main(int argc, char *argv[]) > ovsrec_interface_table_get( > ovs_idl_loop.idl), > !ovnsb_idl_txn, !ovs_idl_txn); > + if (runtime_data && 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); > + } > > Can we move this block into the already existing if (runtime_data)? If not it should at least be under the stopwatch_stop. > stopwatch_stop(IF_STATUS_MGR_RUN_STOPWATCH_NAME, > time_msec()); > } > diff --git a/tests/ovn.at b/tests/ovn.at > index 24da9174e..c8dd0978b 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -36596,7 +36596,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 fef3cfbf0..8533149f8 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 > > Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] IM: amusil <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
