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. 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? > + 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? > <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
