> Hi Lorenzo, > > The top line of the commit message says that qos_physical_network is > configured in the other_config column, but it appears to actually be in the > options column.
ack, I will fix it > > I think this change should include a test in ovn-northd.at. It should ensure > that qos_network_name is set on port_bindings when we expect, and that it is > not set on port_bindings when we expect. ack, I will add it. > > I have one more finding below. > > On 5/15/23 06:03, Lorenzo Bianconi wrote: > > This is a preliminary patch to rework OVN QoS implementation in order to > > configure it through OVS QoS table instead of running tc command > > directly bypassing OVS. > > > > Signed-off-by: Lorenzo Bianconi <[email protected]> > > --- > > controller/binding.c | 4 ++++ > > northd/northd.c | 8 ++++++++ > > ovn-sb.xml | 5 +++++ > > 3 files changed, 17 insertions(+) > > > > diff --git a/controller/binding.c b/controller/binding.c > > index ad19a4092..6dc5a1645 100644 > > --- a/controller/binding.c > > +++ b/controller/binding.c > > @@ -142,6 +142,7 @@ static void update_lport_tracking(const struct > > sbrec_port_binding *pb, > > struct qos_queue { > > struct hmap_node node; > > + char *network; > > char *port; > > uint32_t queue_id; > > @@ -165,6 +166,7 @@ find_qos_queue(struct hmap *queue_map, uint32_t hash, > > const char *port) > > static void > > qos_queue_erase_entry(struct qos_queue *q) > > { > > + free(q->network); > > free(q->port); > > free(q); > > } > > @@ -186,6 +188,7 @@ get_qos_queue(const struct sbrec_port_binding *pb, > > struct hmap *queue_map) > > 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); > > + const char *network = smap_get(&pb->options, "qos_physical_network"); > > if ((!min_rate && !max_rate && !burst) || !queue_id) { > > /* Qos is not configured for this port. */ > > @@ -197,6 +200,7 @@ get_qos_queue(const struct sbrec_port_binding *pb, > > struct hmap *queue_map) > > if (!q) { > > q = xzalloc(sizeof *q); > > hmap_insert(queue_map, &q->node, hash); > > + q->network = xstrdup(network); > > q->port = xstrdup(pb->logical_port); > > q->queue_id = queue_id; > > } > > diff --git a/northd/northd.c b/northd/northd.c > > index 25df0957e..53217347d 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -3505,7 +3505,15 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn > > *ovnsb_txn, > > } > > smap_clone(&options, &op->nbsp->options); > > + > > if (queue_id) { > > + if (op->od->n_localnet_ports) { > > + struct ovn_port *port = op->od->localnet_ports[0]; > > + const char *physical_network = smap_get( > > + &port->nbsp->options, "network_name"); > > + smap_add(&options, "qos_physical_network", > > + physical_network); > > What happens if the localnet port does not have options:network_name set? ack, I will fix it. Regards, Lorenzo > > > + } > > smap_add_format(&options, > > "qdisc_queue_id", "%d", queue_id); > > } > > diff --git a/ovn-sb.xml b/ovn-sb.xml > > index 52705f2be..6a34b75db 100644 > > --- a/ovn-sb.xml > > +++ b/ovn-sb.xml > > @@ -3666,6 +3666,11 @@ tcp.flags = RST; > > interface, in bits. > > </column> > > + <column name="options" key="qos_physical_network"> > > + If set, indicates the name of the egress network name where traffic > > + shaping will be applied. > > + </column> > > + > > <column name="options" key="qdisc_queue_id" > > type='{"type": "integer", "minInteger": 1, "maxInteger": > > 61440}'> > > Indicates the queue number on the physical device. This is same > > as the > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
