On Fri, Dec 9, 2022 at 9:43 AM Lorenzo Bianconi
<[email protected]> wrote:
>
> > On Sun, Dec 4, 2022 at 5:06 PM Lorenzo Bianconi
> > <[email protected]> wrote:
> > >
> > > Introduce the capability to apply QoS rules for logical switch ports
> > > claimed by ovn-controller. Rely on shash instead of sset for
> > > egress_ifaces.
> > >
> > > Acked-by: Mark Michelson <[email protected]>
> > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2129742
> > > Signed-off-by: Lorenzo Bianconi <[email protected]>
> >
> > Hi Lorenzo,
>
> Hi Numan,
>
> thx for the review :)
>
> >
> > Thanks for addressing the comments.
> >
> > I tested this version and compared with the present main.
> >
> > I see a few differences.
> >
> > With the main version, when I configure qos params to a localnet
> > logical port, ovn-controller creates a qdisc on the tunnel interface
> > which I guess is expected.
> > But I'm still not sure why we need to configure qdiscs on the tunnel
> > interface. But that's a story. Perhaps I need to see the original
> > QoS commits and understand why it was added.
> >
> > With your patch, I don't see that happening. Perhaps there is a bug
> > in setup_qos() function now that 'shash' is used to store the egress
> > ifaces with the logical port and
> > there is no logical port for the tunnel interfaces.
>
> uhm, I can look into this. Do you think we should keep this behaviour?
Frankly I don't know. We need to find out why qos was configured in
the first place.
I do agree with Ilya's comment that it is odd to configure qos on the
tunnel interface
and that could impact the other traffic.
>
> >
> > Regarding the option - "ovn-egress-iface". I suppose the expectation
> > from the CMS is to set the qos parameters in the logical port and also
> > set this option in the
> > ovs interface right ? I don't really see a need for this. I think if
> > CMS configures the qos parameters in the logical port options,
> > ovn-controller should configure the qos.
> > I think otherwise this would complicate the CMS implementation. Thoughts ?
>
> ovn-egress-iface is mandatory since it is used in setup_qos() to look for the
> proper netdevice pointer to configure ovs qdisc (please note it is used to
> identify ovs interfaces and not ovn ones).
> I think it can be doable (not sure about the complexity) to avoid it for
> logical switch ports (since we have iface-id in them external_ids column of
> ovs interface table) but I do not think we can avoid to set "ovn-egress-iface"
> for localnet port since afaik there is no direct link between ovn localnet
> port
> and ovs interface used to connect the underlay network, right? If so I guess
> it is better to keep it for both localnet and lsp ports. Do you agree?
I think it's ok to expect CMS to set "ovn-egress-iface" for the
physical nic connected to the
provider bridge. But I don't think we should do the same for the
normal logical port VIFs.
>
> >
> >
> > Please see a few comments below
> >
> > > ---
> > > Changes since v3:
> > > - fix typo in new system-ovn test
> > > Changes since v2:
> > > - fix qos configuration restarting ovn-controller
> > > Changes since v1:
> > > - improve ovs interface lookup
> > > - improve system-tests
> > > ---
> > > controller/binding.c | 155 ++++++++++++++++++++++--------------
> > > controller/binding.h | 5 +-
> > > controller/ovn-controller.c | 15 ++--
> > > tests/system-ovn.at | 48 +++++++++++
> > > 4 files changed, 156 insertions(+), 67 deletions(-)
> > >
> > > diff --git a/controller/binding.c b/controller/binding.c
> > > index 5df62baef..53520263c 100644
> > > --- a/controller/binding.c
> > > +++ b/controller/binding.c
> > > @@ -115,6 +115,7 @@ struct qos_queue {
> > > uint32_t min_rate;
> > > uint32_t max_rate;
> > > uint32_t burst;
> > > + char *port_name;
> > > };
> > >
> > > void
> > > @@ -147,25 +148,50 @@ static void update_lport_tracking(const struct
> > > sbrec_port_binding *pb,
> > > struct hmap *tracked_dp_bindings,
> > > bool claimed);
> > >
> > > +static bool is_lport_vif(const struct sbrec_port_binding *pb);
> > > +
> > > +static struct qos_queue *
> > > +get_qos_map_entry(struct hmap *queue_map, const char *name)
> > > +{
> > > + struct qos_queue *qos_node;
> > > + HMAP_FOR_EACH (qos_node, node, queue_map) {
> > > + if (!strcmp(qos_node->port_name, name)) {
> > > + return qos_node;
> > > + }
> > > + }
> > > +
> > > + return NULL;
> > > +}
> > > +
> > > static void
> > > -get_qos_params(const struct sbrec_port_binding *pb, struct hmap
> > > *queue_map)
> > > +update_qos_params(const struct sbrec_port_binding *pb, struct hmap
> > > *queue_map)
> > > {
> > > uint32_t min_rate = smap_get_int(&pb->options, "qos_min_rate", 0);
> > > uint32_t max_rate = smap_get_int(&pb->options, "qos_max_rate", 0);
> > > uint32_t burst = smap_get_int(&pb->options, "qos_burst", 0);
> > > uint32_t queue_id = smap_get_int(&pb->options, "qdisc_queue_id", 0);
> > >
> > > + struct qos_queue *node = get_qos_map_entry(queue_map,
> > > pb->logical_port);
> > > +
> > > if ((!min_rate && !max_rate && !burst) || !queue_id) {
> > > /* Qos is not configured for this port. */
> > > + if (node) {
> > > + hmap_remove(queue_map, &node->node);
> > > + free(node->port_name);
> > > + free(node);
> > > + }
> > > return;
> > > }
> > >
> > > - struct qos_queue *node = xzalloc(sizeof *node);
> > > - hmap_insert(queue_map, &node->node, hash_int(queue_id, 0));
> > > + if (!node) {
> > > + node = xzalloc(sizeof *node);
> > > + hmap_insert(queue_map, &node->node, hash_int(queue_id, 0));
> > > + node->port_name = xstrdup(pb->logical_port);
> > > + }
> > > + node->queue_id = queue_id;
> > > node->min_rate = min_rate;
> > > node->max_rate = max_rate;
> > > node->burst = burst;
> > > - node->queue_id = queue_id;
> > > }
> > >
> > > static const struct ovsrec_qos *
> > > @@ -191,7 +217,7 @@ static bool
> > > set_noop_qos(struct ovsdb_idl_txn *ovs_idl_txn,
> > > const struct ovsrec_port_table *port_table,
> > > const struct ovsrec_qos_table *qos_table,
> > > - struct sset *egress_ifaces)
> > > + struct shash *egress_ifaces)
> > > {
> > > if (!ovs_idl_txn) {
> > > return false;
> > > @@ -206,11 +232,11 @@ set_noop_qos(struct ovsdb_idl_txn *ovs_idl_txn,
> > > size_t count = 0;
> > >
> > > OVSREC_PORT_TABLE_FOR_EACH (port, port_table) {
> > > - if (sset_contains(egress_ifaces, port->name)) {
> > > + if (shash_find(egress_ifaces, port->name)) {
> > > ovsrec_port_set_qos(port, noop_qos);
> > > count++;
> > > }
> > > - if (sset_count(egress_ifaces) == count) {
> > > + if (shash_count(egress_ifaces) == count) {
> > > break;
> > > }
> > > }
> > > @@ -236,7 +262,8 @@ set_qos_type(struct netdev *netdev, const char *type)
> > > }
> > >
> > > static void
> > > -setup_qos(const char *egress_iface, struct hmap *queue_map)
> > > +setup_qos(const char *egress_iface, const char *logical_port,
> > > + struct hmap *queue_map)
> > > {
> > > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
> > > struct netdev *netdev_phy;
> > > @@ -281,7 +308,7 @@ setup_qos(const char *egress_iface, struct hmap
> > > *queue_map)
> > > * a configuration setting.
> > > *
> > > * - Otherwise leave the qdisc alone. */
> > > - if (hmap_is_empty(queue_map)) {
> > > + if (!get_qos_map_entry(queue_map, logical_port)) {
> > > if (!strcmp(qdisc_type, OVN_QOS_TYPE)) {
> > > set_qos_type(netdev_phy, "");
> > > }
> > > @@ -338,6 +365,10 @@ setup_qos(const char *egress_iface, struct hmap
> > > *queue_map)
> > > continue;
> > > }
> > >
> > > + if (strcmp(sb_info->port_name, logical_port)) {
> > > + continue;
> > > + }
> > > +
> > > smap_clear(&queue_details);
> > > smap_add_format(&queue_details, "min-rate", "%d",
> > > sb_info->min_rate);
> > > smap_add_format(&queue_details, "max-rate", "%d",
> > > sb_info->max_rate);
> > > @@ -354,11 +385,12 @@ setup_qos(const char *egress_iface, struct hmap
> > > *queue_map)
> > > netdev_close(netdev_phy);
> > > }
> > >
> > > -static void
> > > +void
> > > destroy_qos_map(struct hmap *qos_map)
> > > {
> > > struct qos_queue *qos_queue;
> > > HMAP_FOR_EACH_POP (qos_queue, node, qos_map) {
> > > + free(qos_queue->port_name);
> > > free(qos_queue);
> > > }
> > >
> > > @@ -404,7 +436,7 @@ sbrec_get_port_encap(const struct sbrec_chassis
> > > *chassis_rec,
> > > static void
> > > add_localnet_egress_interface_mappings(
> > > const struct sbrec_port_binding *port_binding,
> > > - struct shash *bridge_mappings, struct sset *egress_ifaces)
> > > + struct shash *bridge_mappings, struct shash *egress_ifaces)
> > > {
> > > const char *network = smap_get(&port_binding->options,
> > > "network_name");
> > > if (!network) {
> > > @@ -429,7 +461,8 @@ add_localnet_egress_interface_mappings(
> > > if (!is_egress_iface) {
> > > continue;
> > > }
> > > - sset_add(egress_ifaces, iface_rec->name);
> > > + shash_add(egress_ifaces, iface_rec->name,
> > > + port_binding->logical_port);
> > > }
> > > }
> > > }
> > > @@ -474,7 +507,7 @@ update_ld_multichassis_ports(const struct
> > > sbrec_port_binding *binding_rec,
> > > static void
> > > update_ld_localnet_port(const struct sbrec_port_binding *binding_rec,
> > > struct shash *bridge_mappings,
> > > - struct sset *egress_ifaces,
> > > + struct shash *egress_ifaces,
> > > struct hmap *local_datapaths)
> > > {
> > > /* Ignore localnet ports for unplugged networks. */
> > > @@ -1456,7 +1489,7 @@ consider_vif_lport_(const struct sbrec_port_binding
> > > *pb,
> > >
> > > b_ctx_out->tracked_dp_bindings);
> > > }
> > > if (b_lport->lbinding->iface && qos_map &&
> > > b_ctx_in->ovs_idl_txn) {
> > > - get_qos_params(pb, qos_map);
> > > + update_qos_params(pb, qos_map);
> > > }
> > > } else {
> > > /* We could, but can't claim the lport. */
> > > @@ -1519,6 +1552,16 @@ consider_vif_lport(const struct sbrec_port_binding
> > > *pb,
> > > b_lport = local_binding_add_lport(binding_lports, lbinding,
> > > pb,
> > > LP_VIF);
> > > }
> > > +
> > > + if (lbinding->iface &&
> > > + smap_get(&lbinding->iface->external_ids,
> > > "ovn-egress-iface")) {
> > > + const char *iface_id =
> > > smap_get(&lbinding->iface->external_ids,
> > > + "iface-id");
> > > + if (iface_id) {
> > > + shash_add(b_ctx_out->egress_ifaces,
> > > lbinding->iface->name,
> > > + iface_id);
> >
> > When a qos is configured on a logical port, this patch is adding 2
> > entries in the 'egress_ifaces' shash for the same logical port.
> > I think it's because of the above 'shash_add'. I think you should use
> > shash_replace instead.
>
> ack, I will fix it.
>
> >
> > When a full recompute happens, the function build_local_bindings()
> > also adds the qos configured logical ports to the 'egress_ifaces'
> > and later if there are any updates to the logical port, this function
> > - consider_vif_lport() also adds it to the shash.
> >
> > IMO the qos support in OVN needs a different approach. Instead of
> > configuring the qos using netdev() perhaps we should rely on the OVS
> > meters.
> > Maybe this was brought up earlier too.
>
> >
> > I'm of the opinion that instead of supporting Qos for logical ports
> > using netdev, we should use OVS meters (not just for logical ports,
> > even for localnet ports). Thoughts ?
>
> ovn qos is used for egress traffic (shaping) while ovn meters are used
> for incoming one (policing).
> What we can do is to avoid configuring directly netdev qdisc and rely on
> ovs to perform this configuration but this would be quite a massive rework.
> Do you think we should do it now before adding this feature or it is ok
> to do it after we added this feature? I do not have a strong opinion about it.
IMO its better to add this new feature using the native OVS QoS support.
And along with that we can remove the netdev implementation from OVN.
This is what I think. Anyway this feature is out of window for 22.12.
We can perhaps target this feature with the new approach in 23.03.
Although I will not object if other maintainers are fine getting this
feature first
and then reimplement using OVS native QoS support.
Thanks
Numan
>
> Regards,
> Lorenzo
>
> >
> > @Dumitru Ceara @Han Zhou @Mark Michelson @Ilya Maximets Do you have
> > any comments or suggestions here ?
> >
> > Thanks
> > Numan
> >
> >
> >
> > > + }
> > > + }
> > > }
> > >
> > > return consider_vif_lport_(pb, can_bind, b_ctx_in, b_ctx_out,
> > > @@ -1785,7 +1828,7 @@ consider_localnet_lport(const struct
> > > sbrec_port_binding *pb,
> > > update_local_lports(pb->logical_port, b_ctx_out);
> > >
> > > if (qos_map && b_ctx_in->ovs_idl_txn) {
> > > - get_qos_params(pb, qos_map);
> > > + update_qos_params(pb, qos_map);
> > > }
> > >
> > > update_related_lport(pb, b_ctx_out);
> > > @@ -1861,14 +1904,14 @@ build_local_bindings(struct binding_ctx_in
> > > *b_ctx_in,
> > > &b_ctx_out->lbinding_data->bindings;
> > > for (j = 0; j < port_rec->n_interfaces; j++) {
> > > const struct ovsrec_interface *iface_rec;
> > > + struct local_binding *lbinding = NULL;
> > >
> > > iface_rec = port_rec->interfaces[j];
> > > iface_id = smap_get(&iface_rec->external_ids, "iface-id");
> > > int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport :
> > > 0;
> > >
> > > if (iface_id && ofport > 0) {
> > > - struct local_binding *lbinding =
> > > - local_binding_find(local_bindings, iface_id);
> > > + lbinding = local_binding_find(local_bindings, iface_id);
> > > if (!lbinding) {
> > > lbinding = local_binding_create(iface_id, iface_rec);
> > > local_binding_add(local_bindings, lbinding);
> > > @@ -1895,8 +1938,11 @@ build_local_bindings(struct binding_ctx_in
> > > *b_ctx_in,
> > > const char *tunnel_iface
> > > = smap_get(&iface_rec->status,
> > > "tunnel_egress_iface");
> > > if (tunnel_iface) {
> > > - sset_add(b_ctx_out->egress_ifaces, tunnel_iface);
> > > + shash_add(b_ctx_out->egress_ifaces, tunnel_iface,
> > > "");
> > > }
> > > + } else if (lbinding && smap_get(&iface_rec->external_ids,
> > > + "ovn-egress-iface")) {
> > > + shash_add(b_ctx_out->egress_ifaces, iface_rec->name,
> > > iface_id);
> > > }
> > > }
> > > }
> > > @@ -1910,16 +1956,11 @@ binding_run(struct binding_ctx_in *b_ctx_in,
> > > struct binding_ctx_out *b_ctx_out)
> > > }
> > >
> > > struct shash bridge_mappings = SHASH_INITIALIZER(&bridge_mappings);
> > > - struct hmap qos_map;
> > >
> > > - hmap_init(&qos_map);
> > > if (b_ctx_in->br_int) {
> > > build_local_bindings(b_ctx_in, b_ctx_out);
> > > }
> > >
> > > - struct hmap *qos_map_ptr =
> > > - !sset_is_empty(b_ctx_out->egress_ifaces) ? &qos_map : NULL;
> > > -
> > > struct ovs_list localnet_lports =
> > > OVS_LIST_INITIALIZER(&localnet_lports);
> > > struct ovs_list external_lports =
> > > OVS_LIST_INITIALIZER(&external_lports);
> > > struct ovs_list multichassis_ports = OVS_LIST_INITIALIZER(
> > > @@ -1956,7 +1997,8 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct
> > > binding_ctx_out *b_ctx_out)
> > > break;
> > >
> > > case LP_VIF:
> > > - consider_vif_lport(pb, b_ctx_in, b_ctx_out, NULL,
> > > qos_map_ptr);
> > > + consider_vif_lport(pb, b_ctx_in, b_ctx_out, NULL,
> > > + b_ctx_out->qos_map);
> > > if (pb->additional_chassis) {
> > > struct lport *multichassis_lport = xmalloc(
> > > sizeof *multichassis_lport);
> > > @@ -1967,11 +2009,13 @@ binding_run(struct binding_ctx_in *b_ctx_in,
> > > struct binding_ctx_out *b_ctx_out)
> > > break;
> > >
> > > case LP_CONTAINER:
> > > - consider_container_lport(pb, b_ctx_in, b_ctx_out,
> > > qos_map_ptr);
> > > + consider_container_lport(pb, b_ctx_in, b_ctx_out,
> > > + b_ctx_out->qos_map);
> > > break;
> > >
> > > case LP_VIRTUAL:
> > > - consider_virtual_lport(pb, b_ctx_in, b_ctx_out, qos_map_ptr);
> > > + consider_virtual_lport(pb, b_ctx_in, b_ctx_out,
> > > + b_ctx_out->qos_map);
> > > break;
> > >
> > > case LP_L2GATEWAY:
> > > @@ -1994,7 +2038,8 @@ 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, &qos_map);
> > > + consider_localnet_lport(pb, b_ctx_in, b_ctx_out,
> > > + b_ctx_out->qos_map);
> > > struct lport *lnet_lport = xmalloc(sizeof *lnet_lport);
> > > lnet_lport->pb = pb;
> > > ovs_list_push_back(&localnet_lports, &lnet_lport->list_node);
> > > @@ -2051,17 +2096,15 @@ binding_run(struct binding_ctx_in *b_ctx_in,
> > > struct binding_ctx_out *b_ctx_out)
> > >
> > > shash_destroy(&bridge_mappings);
> > >
> > > - if (!sset_is_empty(b_ctx_out->egress_ifaces)
> > > + if (!shash_is_empty(b_ctx_out->egress_ifaces)
> > > && set_noop_qos(b_ctx_in->ovs_idl_txn, b_ctx_in->port_table,
> > > b_ctx_in->qos_table, b_ctx_out->egress_ifaces)) {
> > > - const char *entry;
> > > - SSET_FOR_EACH (entry, b_ctx_out->egress_ifaces) {
> > > - setup_qos(entry, &qos_map);
> > > + struct shash_node *entry;
> > > + SHASH_FOR_EACH (entry, b_ctx_out->egress_ifaces) {
> > > + setup_qos(entry->name, entry->data, b_ctx_out->qos_map);
> > > }
> > > }
> > >
> > > - destroy_qos_map(&qos_map);
> > > -
> > > cleanup_claimed_port_timestamps();
> > > }
> > >
> > > @@ -2447,7 +2490,7 @@ binding_handle_ovs_interface_changes(struct
> > > binding_ctx_in *b_ctx_in,
> > > }
> > >
> > > if (smap_get(&iface_rec->external_ids, "ovn-egress-iface") ||
> > > - sset_contains(b_ctx_out->egress_ifaces, iface_rec->name)) {
> > > + shash_find(b_ctx_out->egress_ifaces, iface_rec->name)) {
> > > handled = false;
> > > break;
> > > }
> > > @@ -2493,10 +2536,6 @@ binding_handle_ovs_interface_changes(struct
> > > binding_ctx_in *b_ctx_in,
> > > return false;
> > > }
> > >
> > > - struct hmap qos_map = HMAP_INITIALIZER(&qos_map);
> > > - struct hmap *qos_map_ptr =
> > > - sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map;
> > > -
> > > /*
> > > * We consider an OVS interface for claiming if the following
> > > * 2 conditions are met:
> > > @@ -2525,24 +2564,22 @@ binding_handle_ovs_interface_changes(struct
> > > binding_ctx_in *b_ctx_in,
> > > if (iface_id && ofport > 0 &&
> > > is_iface_in_int_bridge(iface_rec, b_ctx_in->br_int)) {
> > > handled = consider_iface_claim(iface_rec, iface_id, b_ctx_in,
> > > - b_ctx_out, qos_map_ptr);
> > > + b_ctx_out,
> > > b_ctx_out->qos_map);
> > > if (!handled) {
> > > break;
> > > }
> > > }
> > > }
> > >
> > > - if (handled && qos_map_ptr && set_noop_qos(b_ctx_in->ovs_idl_txn,
> > > - b_ctx_in->port_table,
> > > - b_ctx_in->qos_table,
> > > -
> > > b_ctx_out->egress_ifaces)) {
> > > - const char *entry;
> > > - SSET_FOR_EACH (entry, b_ctx_out->egress_ifaces) {
> > > - setup_qos(entry, &qos_map);
> > > + if (handled &&
> > > + set_noop_qos(b_ctx_in->ovs_idl_txn, b_ctx_in->port_table,
> > > + b_ctx_in->qos_table, b_ctx_out->egress_ifaces)) {
> > > + struct shash_node *entry;
> > > + SHASH_FOR_EACH (entry, b_ctx_out->egress_ifaces) {
> > > + setup_qos(entry->name, entry->data, b_ctx_out->qos_map);
> > > }
> > > }
> > >
> > > - destroy_qos_map(&qos_map);
> > > return handled;
> > > }
> > >
> > > @@ -2977,10 +3014,6 @@ delete_done:
> > > return false;
> > > }
> > >
> > > - struct hmap qos_map = HMAP_INITIALIZER(&qos_map);
> > > - struct hmap *qos_map_ptr =
> > > - sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map;
> > > -
> > > SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb,
> > >
> > > b_ctx_in->port_binding_table) {
> > > /* Loop to handle create and update changes only. */
> > > @@ -2992,7 +3025,8 @@ delete_done:
> > > update_ld_peers(pb, b_ctx_out->local_datapaths);
> > > }
> > >
> > > - handled = handle_updated_port(b_ctx_in, b_ctx_out, pb,
> > > qos_map_ptr);
> > > + handled = handle_updated_port(b_ctx_in, b_ctx_out, pb,
> > > + b_ctx_out->qos_map);
> > > if (!handled) {
> > > break;
> > > }
> > > @@ -3009,7 +3043,8 @@ delete_done:
> > > sset_find_and_delete(b_ctx_out->postponed_ports, port_name);
> > > continue;
> > > }
> > > - handled = handle_updated_port(b_ctx_in, b_ctx_out, pb,
> > > qos_map_ptr);
> > > + handled = handle_updated_port(b_ctx_in, b_ctx_out, pb,
> > > + b_ctx_out->qos_map);
> > > if (!handled) {
> > > break;
> > > }
> > > @@ -3055,17 +3090,15 @@ delete_done:
> > > shash_destroy(&bridge_mappings);
> > > }
> > >
> > > - if (handled && qos_map_ptr && set_noop_qos(b_ctx_in->ovs_idl_txn,
> > > - b_ctx_in->port_table,
> > > - b_ctx_in->qos_table,
> > > -
> > > b_ctx_out->egress_ifaces)) {
> > > - const char *entry;
> > > - SSET_FOR_EACH (entry, b_ctx_out->egress_ifaces) {
> > > - setup_qos(entry, &qos_map);
> > > + if (handled &&
> > > + set_noop_qos(b_ctx_in->ovs_idl_txn, b_ctx_in->port_table,
> > > + b_ctx_in->qos_table, b_ctx_out->egress_ifaces)) {
> > > + struct shash_node *entry;
> > > + SHASH_FOR_EACH (entry, b_ctx_out->egress_ifaces) {
> > > + setup_qos(entry->name, entry->data, b_ctx_out->qos_map);
> > > }
> > > }
> > >
> > > - destroy_qos_map(&qos_map);
> > > return handled;
> > > }
> > >
> > > diff --git a/controller/binding.h b/controller/binding.h
> > > index 6c3a98b02..8be33eddb 100644
> > > --- a/controller/binding.h
> > > +++ b/controller/binding.h
> > > @@ -91,7 +91,8 @@ struct binding_ctx_out {
> > > */
> > > bool non_vif_ports_changed;
> > >
> > > - struct sset *egress_ifaces;
> > > + struct shash *egress_ifaces;
> > > + struct hmap *qos_map;
> > > /* smap of OVS interface name as key and
> > > * OVS interface external_ids:iface-id as value. */
> > > struct smap *local_iface_ids;
> > > @@ -195,6 +196,8 @@ void set_pb_chassis_in_sbrec(const struct
> > > sbrec_port_binding *pb,
> > > const struct sbrec_chassis *chassis_rec,
> > > bool is_set);
> > >
> > > +void destroy_qos_map(struct hmap *qos_map);
> > > +
> > > /* Corresponds to each Port_Binding.type. */
> > > enum en_lport_type {
> > > LP_UNKNOWN,
> > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > > index d6251afb8..066d76869 100644
> > > --- a/controller/ovn-controller.c
> > > +++ b/controller/ovn-controller.c
> > > @@ -1276,7 +1276,8 @@ struct ed_type_runtime_data {
> > > struct sset active_tunnels;
> > >
> > > /* runtime data engine private data. */
> > > - struct sset egress_ifaces;
> > > + struct shash egress_ifaces;
> > > + struct hmap qos_map;
> > > struct smap local_iface_ids;
> > >
> > > /* Tracked data. See below for more details and comments. */
> > > @@ -1372,7 +1373,8 @@ en_runtime_data_init(struct engine_node *node
> > > OVS_UNUSED,
> > > sset_init(&data->local_lports);
> > > related_lports_init(&data->related_lports);
> > > sset_init(&data->active_tunnels);
> > > - sset_init(&data->egress_ifaces);
> > > + shash_init(&data->egress_ifaces);
> > > + hmap_init(&data->qos_map);
> > > smap_init(&data->local_iface_ids);
> > > local_binding_data_init(&data->lbinding_data);
> > > shash_init(&data->local_active_ports_ipv6_pd);
> > > @@ -1392,7 +1394,8 @@ en_runtime_data_cleanup(void *data)
> > > sset_destroy(&rt_data->local_lports);
> > > related_lports_destroy(&rt_data->related_lports);
> > > sset_destroy(&rt_data->active_tunnels);
> > > - sset_destroy(&rt_data->egress_ifaces);
> > > + shash_destroy(&rt_data->egress_ifaces);
> > > + destroy_qos_map(&rt_data->qos_map);
> > > smap_destroy(&rt_data->local_iface_ids);
> > > local_datapaths_destroy(&rt_data->local_datapaths);
> > > shash_destroy(&rt_data->local_active_ports_ipv6_pd);
> > > @@ -1481,6 +1484,7 @@ init_binding_ctx(struct engine_node *node,
> > > b_ctx_out->related_lports_changed = false;
> > > b_ctx_out->non_vif_ports_changed = false;
> > > b_ctx_out->egress_ifaces = &rt_data->egress_ifaces;
> > > + b_ctx_out->qos_map = &rt_data->qos_map;
> > > b_ctx_out->lbinding_data = &rt_data->lbinding_data;
> > > b_ctx_out->local_iface_ids = &rt_data->local_iface_ids;
> > > b_ctx_out->postponed_ports = rt_data->postponed_ports;
> > > @@ -1510,13 +1514,14 @@ en_runtime_data_run(struct engine_node *node,
> > > void *data)
> > > sset_destroy(local_lports);
> > > related_lports_destroy(&rt_data->related_lports);
> > > sset_destroy(active_tunnels);
> > > - sset_destroy(&rt_data->egress_ifaces);
> > > + shash_clear(&rt_data->egress_ifaces);
> > > + destroy_qos_map(&rt_data->qos_map);
> > > + hmap_init(&rt_data->qos_map);
> > > smap_destroy(&rt_data->local_iface_ids);
> > > hmap_init(local_datapaths);
> > > sset_init(local_lports);
> > > related_lports_init(&rt_data->related_lports);
> > > sset_init(active_tunnels);
> > > - sset_init(&rt_data->egress_ifaces);
> > > smap_init(&rt_data->local_iface_ids);
> > > local_binding_data_init(&rt_data->lbinding_data);
> > > }
> > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> > > index 3e904c9dc..c986198a2 100644
> > > --- a/tests/system-ovn.at
> > > +++ b/tests/system-ovn.at
> > > @@ -6335,6 +6335,10 @@ ADD_NAMESPACES(sw01)
> > > ADD_VETH(sw01, sw01, br-int, "192.168.1.2/24", "f0:00:00:01:02:03")
> > > ovn-nbctl lsp-add sw0 sw01 \
> > > -- lsp-set-addresses sw01 "f0:00:00:01:02:03 192.168.1.2"
> > > +ADD_NAMESPACES(sw02)
> > > +ADD_VETH(sw02, sw02, br-int, "192.168.1.3/24", "f2:00:00:01:02:03")
> > > +ovn-nbctl lsp-add sw0 sw02 \
> > > + -- lsp-set-addresses sw02 "f2:00:00:01:02:03 192.168.1.3"
> > >
> > > ADD_NAMESPACES(public)
> > > ADD_VETH(public, public, br-ext, "192.168.2.2/24", "f0:00:00:01:02:05")
> > > @@ -6345,6 +6349,7 @@ ovn-nbctl lsp-add sw0 public \
> > > -- lsp-set-type public localnet \
> > > -- lsp-set-options public network_name=phynet
> > >
> > > +# Setup QoS on a localnet port
> > > AT_CHECK([ovn-nbctl set Logical_Switch_Port public
> > > options:qos_min_rate=200000])
> > > AT_CHECK([ovn-nbctl set Logical_Switch_Port public
> > > options:qos_max_rate=300000])
> > > AT_CHECK([ovn-nbctl set Logical_Switch_Port public
> > > options:qos_burst=3000000])
> > > @@ -6353,15 +6358,58 @@ 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'])
> > >
> > > +# Setup QoS on a logical switch ports
> > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01
> > > options:qos_min_rate=400000])
> > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01
> > > options:qos_max_rate=800000])
> > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01
> > > options:qos_burst=5000000])
> > > +AT_CHECK([ovs-vsctl set interface ovs-sw01
> > > external-ids:ovn-egress-iface=true])
> > > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw01'])
> > > +OVS_WAIT_UNTIL([tc class show dev ovs-sw01 | \
> > > + grep -q 'class htb .* rate 400Kbit ceil 800Kbit burst
> > > 625000b cburst 625000b'])
> > > +
> > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02
> > > options:qos_min_rate=600000])
> > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02
> > > options:qos_max_rate=6000000])
> > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02
> > > options:qos_burst=6000000])
> > > +AT_CHECK([ovs-vsctl set interface ovs-sw02
> > > external-ids:ovn-egress-iface=true])
> > > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw02'])
> > > +OVS_WAIT_UNTIL([tc class show dev ovs-sw02 | \
> > > + grep -q 'class htb .* rate 600Kbit ceil 6Mbit burst
> > > 750000b cburst 750000b'])
> > > +
> > > +AT_CHECK([ovn-appctl -t ovn-controller exit --restart])
> > > +OVS_WAIT_UNTIL([test "$(pidof ovn-controller)" = ""])
> > > +start_daemon ovn-controller
> > > +OVS_WAIT_UNTIL([test "$(pidof ovn-controller)" != ""])
> > > +
> > > +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-sw01'])
> > > +OVS_WAIT_UNTIL([tc class show dev ovs-sw01 | \
> > > + grep -q 'class htb .* rate 400Kbit ceil 800Kbit burst
> > > 625000b cburst 625000b'])
> > > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw02'])
> > > +OVS_WAIT_UNTIL([tc class show dev ovs-sw02 | \
> > > + grep -q 'class htb .* rate 600Kbit ceil 6Mbit burst
> > > 750000b cburst 750000b'])
> > >
> > > AT_CHECK([ovn-nbctl remove Logical_Switch_Port public options
> > > qos_max_rate=300000])
> > > OVS_WAIT_UNTIL([tc class show dev ovs-public | \
> > > grep -q 'class htb .* rate 200Kbit ceil 34359Mbit burst
> > > 375000b .*'])
> > >
> > > AT_CHECK([ovn-nbctl remove Logical_Switch_Port public options
> > > qos_min_rate=200000])
> > > +AT_CHECK([ovn-nbctl remove Logical_Switch_Port public options
> > > qos_max_rate=300000])
> > > AT_CHECK([ovn-nbctl remove Logical_Switch_Port public options
> > > qos_burst=3000000])
> > > OVS_WAIT_UNTIL([test "$(tc qdisc show | grep 'htb 1: dev ovs-public')" =
> > > ""])
> > >
> > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01
> > > options:qos_min_rate=200000])
> > > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw01'])
> > > +OVS_WAIT_UNTIL([tc class show dev ovs-sw01 | \
> > > + grep -q 'class htb .* rate 200Kbit ceil 800Kbit burst
> > > 625000b cburst 625000b'])
> > > +
> > > +# Disable QoS rules from logical switch ports
> > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01
> > > options:qdisc_queue_id=0])
> > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02
> > > options:qdisc_queue_id=0])
> > > +OVS_WAIT_UNTIL([test "$(tc qdisc show | grep 'htb 1: dev ovs-sw01')" =
> > > ""])
> > > +OVS_WAIT_UNTIL([test "$(tc qdisc show | grep 'htb 1: dev ovs-sw02')" =
> > > ""])
> > > +
> > > kill $(pidof ovn-controller)
> > >
> > > as ovn-sb
> > > --
> > > 2.38.1
> > >
> > > _______________________________________________
> > > 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
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
