Hi Numan, Ales Thanks for the review. While the patch covers a similar issue than bz https://bugzilla.redhat.com/2234349, I think that the bz is a different issue: QoS does not work anymore (even w/ the patch) on tunnel interfaces (see NEWS from commit 87514ac0). So I do not think that the tag should have been added
Thanks Xavier On Fri, Sep 15, 2023 at 11:59 PM Numan Siddique <[email protected]> wrote: > On Fri, Sep 15, 2023 at 3:09 AM Ales Musil <[email protected]> wrote: > > > > On Fri, Sep 15, 2023 at 8:21 AM 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 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]> > > > > Hi Xavier, > > > > I believe that we are missing Reported-at: > https://bugzilla.redhat.com/2234349. > > I added this tag to the commit message and applied the patch to main > and branch-23.09. > Let me know if this needs to be backported further down. > > Thanks > Numan > > > > > > > > > --- > > > 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 > > > > > > > Other than that it looks good. > > > > Acked-by: Ales Musil <[email protected]> > > > > Thanks, > > Ales > > > > > > -- > > > > Ales Musil > > > > Senior Software Engineer - OVN Core > > > > Red Hat EMEA > > > > [email protected] IM: amusil > > > > _______________________________________________ > > 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
