> Happy to see that CMS won't need to set anything on LSP except the > existing qos_ options, and that northd will itself negotiate with > ovn-controller which interface to apply TC rules at. It's important > that the interface of OVN exposed to CMS doesn't change. Thanks for > added test cases too. > > Before the series, qos options defined on regular LSP ports were > applied to all egress interfaces. I think this made it work even when > e.g. a router was in the way between LSP and fabric. Now with the > series, only direct topology (LSP - LS - public) gets qos configured. > It may be fine / the right thing to do, but perhaps we can document > the change in behavior in the commit message and NEWS file.
ack, will do. > > > On Thu, Apr 13, 2023 at 10:45 AM Lorenzo Bianconi > <[email protected]> wrote: > > > > This patch allows to apply QoS rules on the localnet port related to > > logical switch ports running on the same datapath. Considering the > > following netowrk configuration: > > > > LSP{0,1} -- LogicalSwitch -- Localnet0 > > > > It is possible to apply the following QoS rules on Localnet0 on egress > > traffic > > entering the cluster from LSP{0,1}: > > - LSP0: min-rate r0, max_rate R0 > > - LSP1: min-rate r1, max_rate R1 > > > > https://bugzilla.redhat.com/show_bug.cgi?id=2129742 > > Tested-by: Rodolfo Alonso <[email protected]> > > Signed-off-by: Lorenzo Bianconi <[email protected]> > > --- > > controller/binding.c | 21 ++++++++++- > > northd/northd.c | 34 ++++++++++++++---- > > northd/ovn-northd.8.xml | 12 +++++++ > > tests/ovn-northd.at | 2 ++ > > tests/system-ovn.at | 79 +++++++++++++++++++++++++++++++++++++++-- > > 5 files changed, 139 insertions(+), 9 deletions(-) > > > > diff --git a/controller/binding.c b/controller/binding.c > > index aa16c802b..3403f74bb 100644 > > --- a/controller/binding.c > > +++ b/controller/binding.c > > @@ -199,7 +199,7 @@ get_qos_egress_interface_name(struct shash > > *bridge_mappings, > > > > bool is_egress_iface = smap_get_bool(&iface_rec->external_ids, > > "ovn-egress-iface", > > false); > > - if (is_egress_iface) { > > + if (is_egress_iface || !strcmp(iface_rec->type, "")) { > > return iface_rec->name; > > } > > } > > @@ -453,6 +453,13 @@ add_localnet_egress_interface_mappings( > > return; > > } > > > > + const char *qos_physical_network = smap_get( > > + &port_binding->options, "qos_physical_network"); > > + if (qos_physical_network && !strcmp(qos_physical_network, network)) { > > + smap_replace(egress_ifaces, port_binding->logical_port, > > network); > > + return; > > + } > > + > > /* Add egress-ifaces from the connected bridge */ > > for (size_t i = 0; i < br_ln->n_ports; i++) { > > const struct ovsrec_port *port_rec = br_ln->ports[i]; > > @@ -1493,6 +1500,14 @@ consider_vif_lport_(const struct sbrec_port_binding > > *pb, > > tracked_datapath_lport_add(pb, TRACKED_RESOURCE_UPDATED, > > b_ctx_out->tracked_dp_bindings); > > } > > + > > + const char *qos_physical_network = > > + smap_get(&pb->options, "qos_physical_network"); > > + if (qos_physical_network) { > > + smap_replace(b_ctx_out->egress_ifaces, pb->logical_port, > > + qos_physical_network); > > + } > > + > > if (b_lport->lbinding->iface && qos_map && > > b_ctx_in->ovs_idl_txn) { > > get_qos_params(pb, qos_map, b_ctx_out->egress_ifaces); > > } > > @@ -2945,6 +2960,10 @@ binding_handle_port_binding_changes(struct > > binding_ctx_in *b_ctx_in, > > } else { > > shash_add(&deleted_other_pbs, pb->logical_port, pb); > > } > > + > > + if (smap_get(&pb->options, "qos_physical_network")) { > > + smap_remove(b_ctx_out->egress_ifaces, pb->logical_port); > > + } > > } > > > > struct shash_node *node; > > diff --git a/northd/northd.c b/northd/northd.c > > index da3c8cf77..386073bf0 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -3498,7 +3498,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_format(&options, "qos_physical_network", "%s", > > smap_add should be enough, no? ack, I will fix it. > > > + physical_network); > > + } > > smap_add_format(&options, > > "qdisc_queue_id", "%d", queue_id); > > } > > @@ -5785,15 +5793,29 @@ build_lswitch_port_sec_op(struct ovn_port *op, > > struct hmap *lflows, > > ds_cstr(match), ds_cstr(actions), > > op->key, &op->nbsp->header_); > > > > + if (!lsp_is_localnet(op->nbsp) && !op->od->n_localnet_ports) { > > + return; > > + } > > + > > + ds_clear(actions); > > + ds_put_format(actions, "set_queue(%s); output;", queue_id); > > + > > + ds_clear(match); > > if (lsp_is_localnet(op->nbsp)) { > > - ds_clear(match); > > - ds_clear(actions); > > ds_put_format(match, "outport == %s", op->json_key); > > - ds_put_format(actions, "set_queue(%s); output;", queue_id); > > ovn_lflow_add_with_lport_and_hint(lflows, op->od, > > - S_SWITCH_OUT_APPLY_PORT_SEC, > > 100, > > - ds_cstr(match), > > ds_cstr(actions), > > - op->key, &op->nbsp->header_); > > + S_SWITCH_OUT_APPLY_PORT_SEC, > > 100, > > + ds_cstr(match), > > ds_cstr(actions), > > + op->key, &op->nbsp->header_); > > + } else if (op->od->n_localnet_ports) { > > + ds_put_format(match, "outport == %s && inport == %s", > > + op->od->localnet_ports[0]->json_key, > > + op->json_key); > > + ovn_lflow_add_with_lport_and_hint(lflows, op->od, > > + S_SWITCH_OUT_APPLY_PORT_SEC, 110, > > + ds_cstr(match), ds_cstr(actions), > > + op->od->localnet_ports[0]->key, > > + &op->od->localnet_ports[0]->nbsp->header_); > > } > > } > > } > > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml > > index 98aa060b9..443b3a0bc 100644 > > --- a/northd/ovn-northd.8.xml > > +++ b/northd/ovn-northd.8.xml > > @@ -2217,6 +2217,18 @@ output; > > </p> > > > > <ul> > > + <li> > > + <p> > > + For each port configured with egress qos in the > > + <ref column="options:qdisc_queue_id" table="Logical_Switch_Port" > > + db="OVN_Northbound"/> column of <ref table="Logical_Switch_Port" > > + db="OVN_Northbound"/>, running a localnet port on the same logical > > + switch, a priority 110 flow is added which matches on the localnet > > + <code>outport</code> and on the port <code>inport</code> and > > + applies the action <code>set_queue(id); output;"</code>. > > + </p> > > + </li> > > + > > Should the new qos_physical_network option be documented in ovn-sb.xml? ack, will do. Regards, Lorenzo > > > > > <li> > > <p> > > For each localnet port configured with egress qos in the > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > index 9fee00094..562b41b8d 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -7923,6 +7923,7 @@ sort | sed 's/table=../table=??/' ], [0], [dnl > > table=??(ls_out_check_port_sec), priority=0 , match=(1), > > action=(reg0[[15]] = check_out_port_sec(); next;) > > table=??(ls_out_check_port_sec), priority=100 , match=(eth.mcast), > > action=(reg0[[15]] = 0; next;) > > table=??(ls_out_apply_port_sec), priority=0 , match=(1), > > action=(output;) > > + table=??(ls_out_apply_port_sec), priority=110 , match=(outport == > > "localnetport" && inport == "sw0p2"), action=(set_queue(10); output;) > > table=??(ls_out_apply_port_sec), priority=50 , match=(reg0[[15]] == > > 1), action=(drop;) > > ]) > > > > @@ -7953,6 +7954,7 @@ sort | sed 's/table=../table=??/' ], [0], [dnl > > table=??(ls_out_check_port_sec), priority=100 , match=(eth.mcast), > > action=(reg0[[15]] = 0; next;) > > table=??(ls_out_apply_port_sec), priority=0 , match=(1), > > action=(output;) > > table=??(ls_out_apply_port_sec), priority=100 , match=(outport == > > "localnetport"), action=(set_queue(10); output;) > > + table=??(ls_out_apply_port_sec), priority=110 , match=(outport == > > "localnetport" && inport == "sw0p2"), action=(set_queue(10); output;) > > table=??(ls_out_apply_port_sec), priority=50 , match=(reg0[[15]] == > > 1), action=(drop;) > > ]) > > > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > > index 3e84b6dcb..9737b699a 100644 > > --- a/tests/system-ovn.at > > +++ b/tests/system-ovn.at > > @@ -6556,6 +6556,11 @@ 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", "f0:00:00:01:02:44") > > +ovn-nbctl lsp-add sw0 sw02 \ > > + -- lsp-set-addresses sw02 "f0:00:00:01:02:44 192.168.1.3" > > + > > ovn-nbctl ls-add sw1 > > > > ADD_NAMESPACES(sw11) > > @@ -6563,6 +6568,11 @@ ADD_VETH(sw11, sw11, br-int, "192.168.4.2/24", > > "f0:00:00:01:04:03") > > ovn-nbctl lsp-add sw1 sw11 \ > > -- lsp-set-addresses sw11 "f0:00:00:01:04:03 192.168.4.2" > > > > +ADD_NAMESPACES(sw12) > > +ADD_VETH(sw12, sw12, br-int, "192.168.4.3/24", "f0:00:00:03:04:03") > > +ovn-nbctl lsp-add sw1 sw12 \ > > + -- lsp-set-addresses sw11 "f0:00:00:03:04:03 192.168.4.3" > > + > > ADD_NAMESPACES(public) > > ADD_VETH(public, public, br-public, "192.168.2.2/24", "f0:00:00:01:02:05") > > AT_CHECK([ovs-vsctl remove interface ovs-public external-ids > > iface-id=public]) > > @@ -6585,12 +6595,10 @@ ovn-nbctl lsp-add sw1 ext \ > > 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]) > > -AT_CHECK([ovs-vsctl set interface ovs-public > > external-ids:ovn-egress-iface=true]) > > > > AT_CHECK([ovn-nbctl set Logical_Switch_Port ext > > options:qos_min_rate=400000]) > > AT_CHECK([ovn-nbctl set Logical_Switch_Port ext > > options:qos_max_rate=600000]) > > AT_CHECK([ovn-nbctl set Logical_Switch_Port ext options:qos_burst=6000000]) > > -AT_CHECK([ovs-vsctl set interface ovs-ext > > external-ids:ovn-egress-iface=true]) > > > > OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-public']) > > OVS_WAIT_UNTIL([tc class show dev ovs-public | \ > > @@ -6637,6 +6645,73 @@ AT_CHECK([ovn-nbctl remove Logical_Switch_Port > > public options qos_burst=6000000] > > > > 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]) > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01 > > options:qos_max_rate=350000]) > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01 > > options:qos_burst=3000000]) > > + > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw11 > > options:qos_min_rate=400000]) > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw11 > > options:qos_max_rate=700000]) > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw11 > > options:qos_burst=6000000]) > > + > > +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 350Kbit burst > > 375000b cburst 374999b']) > > + > > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-ext']) > > +OVS_WAIT_UNTIL([tc class show dev ovs-ext | \ > > + grep -q 'class htb .* prio 0 rate 400Kbit ceil 700Kbit > > burst 750000b cburst 749999b']) > > + > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02 > > options:qos_min_rate=300000]) > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02 > > options:qos_max_rate=500000]) > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02 > > options:qos_burst=3000000]) > > + > > +OVS_WAIT_UNTIL([tc class show dev ovs-public | \ > > + grep -q 'class htb .* prio 0 rate 300Kbit ceil 500Kbit > > burst 375000b cburst 375000b']) > > + > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw12 > > options:qos_min_rate=400000]) > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw12 > > options:qos_max_rate=500000]) > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw12 > > options:qos_burst=3000000]) > > + > > +OVS_WAIT_UNTIL([tc class show dev ovs-ext | \ > > + grep -q 'class htb .* prio 0 rate 400Kbit ceil 500Kbit > > burst 375000b cburst 375000b']) > > + > > +AT_CHECK([ovn-nbctl remove Logical_Switch_Port sw02 options > > qos_min_rate=300000]) > > +AT_CHECK([ovn-nbctl remove Logical_Switch_Port sw02 options > > qos_max_rate=500000]) > > +AT_CHECK([ovn-nbctl remove Logical_Switch_Port sw02 options > > qos_burst=3000000]) > > + > > +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 350Kbit burst > > 375000b cburst 374999b']) > > +OVS_WAIT_UNTIL([test "$(tc class show dev ovs-public | \ > > + grep 'class htb .* prio 0 rate 300Kbit ceil 500Kbit burst > > 375000b cburst 375000b')" = ""]) > > + > > +AT_CHECK([ovn-nbctl remove Logical_Switch_Port sw01 options > > qos_min_rate=200000]) > > +AT_CHECK([ovn-nbctl remove Logical_Switch_Port sw01 options > > qos_max_rate=350000]) > > +AT_CHECK([ovn-nbctl remove Logical_Switch_Port sw01 options > > qos_burst=3000000]) > > +OVS_WAIT_UNTIL([test "$(tc qdisc show | grep 'htb 1: dev ovs-public')" = > > ""]) > > + > > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-ext']) > > +OVS_WAIT_UNTIL([tc class show dev ovs-ext | \ > > + grep -q 'class htb .* prio 0 rate 400Kbit ceil 700Kbit > > burst 750000b cburst 749999b']) > > +OVS_WAIT_UNTIL([tc class show dev ovs-ext | \ > > + grep -q 'class htb .* prio 0 rate 400Kbit ceil 500Kbit > > burst 375000b cburst 375000b']) > > + > > +AT_CHECK([ovn-nbctl remove Logical_Switch_Port sw11 options > > qos_min_rate=400000]) > > +AT_CHECK([ovn-nbctl remove Logical_Switch_Port sw11 options > > qos_max_rate=700000]) > > +AT_CHECK([ovn-nbctl remove Logical_Switch_Port sw11 options > > qos_burst=6000000]) > > + > > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-ext']) > > +OVS_WAIT_UNTIL([test "$(tc class show dev ovs-ext | \ > > + grep 'class htb .* prio 0 rate 400Kbit ceil 700Kbit burst > > 750000b cburst 749999b')" = ""]) > > +OVS_WAIT_UNTIL([tc class show dev ovs-ext | \ > > + grep -q 'class htb .* prio 0 rate 400Kbit ceil 500Kbit > > burst 375000b cburst 375000b']) > > + > > +AT_CHECK([ovn-nbctl remove Logical_Switch_Port sw12 options > > qos_min_rate=400000]) > > +AT_CHECK([ovn-nbctl remove Logical_Switch_Port sw12 options > > qos_max_rate=500000]) > > +AT_CHECK([ovn-nbctl remove Logical_Switch_Port sw12 options > > qos_burst=3000000]) > > + > > +OVS_WAIT_UNTIL([test "$(tc qdisc show | grep 'htb 1: dev ovs-ext')" = ""]) > > + > > kill $(pidof ovn-controller) > > > > as ovn-sb > > -- > > 2.39.2 > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
