On Sun, Dec 11, 2022 at 8:54 AM Lorenzo Bianconi
<[email protected]> wrote:
>
> > 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.
>
> does it worth to introduce a difference here?
Yes.
Configuring ovn-egress-iface=true for a phsyical interface connected
to the provider bridge is a one time activity which CMS
can easily do during installation time. I think openstack installer -
tripleo does that during deployment.
But to set ovn-egress-iface=true on every ovs interface would
require some additional changes in CMS.
If we take openstack as an example, if qos has to be enabled on a VM,
then first neutron has to set the QoS options
in the logical switch port and the "nova" compute component also has
to set ovn-egress-iface=true when it creates a libvirt VM.
This seems unnecessary to me.
Thanks
Numan
>
> >
> >
> >
> > >
> > > >
> > > >
> > > > 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.
>
> I am fine to work on a proper solution for lsp and then move localnet port
> too.
>
> Regards,
> Lorenzo
>
> >
> > 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
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
