| Message ID | 20230726134355.3012154-1-xsimonar@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] QoS: Properly set qos when ovs db is read only | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On Wed, Jul 26, 2023 at 3:44 PM Xavier Simonart <xsimonar@redhat.com> 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 <xsimonar@redhat.com> > 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Thanks, Ales
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, + 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); + } 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'])
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 <xsimonar@redhat.com> --- 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(-)