> On Thu, Nov 24, 2022 at 4:33 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
>
> I did some testing with your patch. I found a few issues and I've few
> comments/questions.
>
> 1. I configured qos on a logical port - sw0-port1
> I ran these commands
> ovn-nbctl set Logical_Switch_Port sw0-port1 options:qos_burst=5000000
> ovn-nbctl set Logical_Switch_Port sw0-port1 options:qos_max_rate=800000
> ovn-nbctl set Logical_Switch_Port sw0-port1 options:qos_min_rate=400000
> ovs-vsctl set interface sw0p1-p external_ids:ovn-egress-iface="true"
>
> After this I see qdisc configured
> # tc qdisc show
> qdisc htb 1: dev eth1 root refcnt 13 r2q 10 default 0x1
> direct_packets_stat 0 direct_qlen 1000
> qdisc htb 1: dev sw0p1-p root refcnt 13 r2q 10 default 0x1
> direct_packets_stat 0 direct_qlen 1000
>
> I see that qdisc is also configured on eth1 which is the geneve
> tunnel interface. Is this expected ?
>
> Since Qos is configured only on the logical port, why does
> ovn-controller create qdiscs for the tunnel interface ?
we always add tunnel interfaces to egress_ifaces in build_local_bindings().
I do not know exactly why it is done this way but this is not introduced by
this patch.
>
> 2. After (1), if I restart ovn-controller, the qdiscs created
> earlier are deleted by ovn-controller
> and not created again even though qos is configured on the logical port
> and
> external_ids:ovn-egress-iface=true is set on the ovs interface.
>
> Can you please cover this scenario in the system test. Stop and
> start ovn-controller
> and make sure that the qdiscs are configured properly as expected.
ack, I will fix it in v3.
Regards,
Lorenzo
>
> > ---
> > Changes since v1:
> > - improve ovs interface lookup
> > - improve system-tests
> > ---
> > controller/binding.c | 72 ++++++++++++++++++++++++-------------
> > controller/binding.h | 2 +-
> > controller/ovn-controller.c | 9 +++--
> > tests/system-ovn.at | 31 ++++++++++++++++
> > 4 files changed, 84 insertions(+), 30 deletions(-)
> >
> > diff --git a/controller/binding.c b/controller/binding.c
> > index 5df62baef..054fa3c18 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,6 +148,8 @@ 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 void
> > get_qos_params(const struct sbrec_port_binding *pb, struct hmap *queue_map)
> > {
> > @@ -166,6 +169,7 @@ get_qos_params(const struct sbrec_port_binding *pb,
> > struct hmap *queue_map)
> > node->max_rate = max_rate;
> > node->burst = burst;
> > node->queue_id = queue_id;
> > + node->port_name = xstrdup(pb->logical_port);
> > }
> >
> > static const struct ovsrec_qos *
> > @@ -191,7 +195,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 +210,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 +240,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;
> > @@ -338,6 +343,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);
> > @@ -359,6 +368,7 @@ 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 +414,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 +439,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 +485,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. */
> > @@ -1519,6 +1530,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);
> > + }
> > + }
> > }
> >
> > return consider_vif_lport_(pb, can_bind, b_ctx_in, b_ctx_out,
> > @@ -1861,14 +1882,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 +1916,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);
> > }
> > }
> > }
> > @@ -1918,7 +1942,7 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct
> > binding_ctx_out *b_ctx_out)
> > }
> >
> > struct hmap *qos_map_ptr =
> > - !sset_is_empty(b_ctx_out->egress_ifaces) ? &qos_map : NULL;
> > + !shash_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);
> > @@ -2051,12 +2075,12 @@ 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, &qos_map);
> > }
> > }
> >
> > @@ -2447,7 +2471,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;
> > }
> > @@ -2495,7 +2519,7 @@ binding_handle_ovs_interface_changes(struct
> > binding_ctx_in *b_ctx_in,
> >
> > struct hmap qos_map = HMAP_INITIALIZER(&qos_map);
> > struct hmap *qos_map_ptr =
> > - sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map;
> > + shash_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map;
> >
> > /*
> > * We consider an OVS interface for claiming if the following
> > @@ -2536,9 +2560,9 @@ binding_handle_ovs_interface_changes(struct
> > binding_ctx_in *b_ctx_in,
> > 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, &qos_map);
> > }
> > }
> >
> > @@ -2979,7 +3003,7 @@ delete_done:
> >
> > struct hmap qos_map = HMAP_INITIALIZER(&qos_map);
> > struct hmap *qos_map_ptr =
> > - sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map;
> > + shash_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map;
> >
> > SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb,
> >
> > b_ctx_in->port_binding_table) {
> > @@ -3059,9 +3083,9 @@ delete_done:
> > 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, &qos_map);
> > }
> > }
> >
> > diff --git a/controller/binding.h b/controller/binding.h
> > index 6c3a98b02..f9a3964e9 100644
> > --- a/controller/binding.h
> > +++ b/controller/binding.h
> > @@ -91,7 +91,7 @@ struct binding_ctx_out {
> > */
> > bool non_vif_ports_changed;
> >
> > - struct sset *egress_ifaces;
> > + struct shash *egress_ifaces;
> > /* smap of OVS interface name as key and
> > * OVS interface external_ids:iface-id as value. */
> > struct smap *local_iface_ids;
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index 0752a71ad..a2652b1d6 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -1265,7 +1265,7 @@ struct ed_type_runtime_data {
> > struct sset active_tunnels;
> >
> > /* runtime data engine private data. */
> > - struct sset egress_ifaces;
> > + struct shash egress_ifaces;
> > struct smap local_iface_ids;
> >
> > /* Tracked data. See below for more details and comments. */
> > @@ -1361,7 +1361,7 @@ 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);
> > smap_init(&data->local_iface_ids);
> > local_binding_data_init(&data->lbinding_data);
> > shash_init(&data->local_active_ports_ipv6_pd);
> > @@ -1381,7 +1381,7 @@ 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);
> > smap_destroy(&rt_data->local_iface_ids);
> > local_datapaths_destroy(&rt_data->local_datapaths);
> > shash_destroy(&rt_data->local_active_ports_ipv6_pd);
> > @@ -1499,13 +1499,12 @@ 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);
> > 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 cb3412717..335c039a7 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")
> > @@ -6353,15 +6357,42 @@ 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'])
> >
> > +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-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'])
> > +
> > +AT_CHECK([set interface ovs-sw01 external-ids:ovn-egress-iface=false])
> > +AT_CHECK([set interface ovs-sw02 external-ids:ovn-egress-iface=false])
> > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw01'],[1])
> > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw02'],[1])
>
> 3. Running the below command doesn't delete the qdisc for the logical port.
>
> ovs-vsctl set interface sw0p1-p external_ids:ovn-egress-iface="false"
>
> So the test case you've added is not correct. Please take a look.
> Also there's a typo in the below
>
> AT_CHECK([set interface ovs-sw01 external-ids:ovn-egress-iface=false])
> AT_CHECK([set interface ovs-sw02 external-ids:ovn-egress-iface=false])
>
> It should be
> AT_CHECK([ovs-vsctl set interface ovs-sw01
> external-ids:ovn-egress-iface=false])
> AT_CHECK([ovs-vsctl set interface ovs-sw02
> external-ids:ovn-egress-iface=false])
>
> And the below checks are not working as expected.
> OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw01'],[1])
> OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw02'],[1])
>
> There seems to be a bug in your test case.
>
> Thanks
> Numan
>
> > +
> > 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
