| Message ID | 3b2e776f2d1b6d7e99203af68e7af71074f83131.1669325525.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2] binding: add the capability to apply QoS for lsp | 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 Thu, Nov 24, 2022 at 4:33 PM Lorenzo Bianconi <lorenzo.bianconi@redhat.com> 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 <mmichels@redhat.com> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2129742 > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> Hi Lorenzo, 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 ? 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. > --- > 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
> On Thu, Nov 24, 2022 at 4:33 PM Lorenzo Bianconi > <lorenzo.bianconi@redhat.com> 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 <mmichels@redhat.com> > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2129742 > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > 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 > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > >
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]) + kill $(pidof ovn-controller) as ovn-sb