[PATCH 0/4] virnetdevbandwidthtest: Test QoS for OVS
Cleanup virnetdevbandwidthtest and introduce new test cases. However, Martin suggested (after glancing over the last patch) that arguments could be stored in a file (just like qemuxml2argvtest does). I'm not against that, but QEMU cmd line args change more often than those of tc/ovs-vsctl. But if desired I can do the change (either as a follow up or in v2). Michal Prívozník (4): virnetdevbandwidthtest: Drop unnecessary brackets virnetdevbandwidthtest: Drop unused testMinimalStruct virnetdevbandwidthtest: Reformat TC cmd line virnetdevbandwidthtest: Test QoS for OVS tests/virnetdevbandwidthtest.c | 171 +++++++++++++++++++++------------ 1 file changed, 109 insertions(+), 62 deletions(-) -- 2.35.1
Some cases that call DO_TEST_SET() macro wrap each argument in curved brackets. This is unnecessary, drop the brackets. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virnetdevbandwidthtest.c | 88 +++++++++++++++++----------------- 1 file changed, 44 insertions(+), 44 deletions(-) diff --git a/tests/virnetdevbandwidthtest.c b/tests/virnetdevbandwidthtest.c index eb4f47dfce..419793a4ae 100644 --- a/tests/virnetdevbandwidthtest.c +++ b/tests/virnetdevbandwidthtest.c @@ -117,53 +117,53 @@ mymain(void) DO_TEST_SET("<bandwidth/>", NULL); - DO_TEST_SET(("<bandwidth>" - " <inbound average='1024'/>" - "</bandwidth>"), - (TC " qdisc del dev eth0 root\n" - TC " qdisc del dev eth0 ingress\n" - TC " qdisc add dev eth0 root handle 1: htb default 1\n" - TC " class add dev eth0 parent 1: classid 1:1 htb rate 1024kbps quantum 87\n" - TC " qdisc add dev eth0 parent 1:1 handle 2: sfq perturb 10\n" - TC " filter add dev eth0 parent 1:0 protocol all prio 1 handle 1 fw flowid 1\n")); + DO_TEST_SET("<bandwidth>" + " <inbound average='1024'/>" + "</bandwidth>", + TC " qdisc del dev eth0 root\n" + TC " qdisc del dev eth0 ingress\n" + TC " qdisc add dev eth0 root handle 1: htb default 1\n" + TC " class add dev eth0 parent 1: classid 1:1 htb rate 1024kbps quantum 87\n" + TC " qdisc add dev eth0 parent 1:1 handle 2: sfq perturb 10\n" + TC " filter add dev eth0 parent 1:0 protocol all prio 1 handle 1 fw flowid 1\n"); - DO_TEST_SET(("<bandwidth>" - " <outbound average='1024'/>" - "</bandwidth>"), - (TC " qdisc del dev eth0 root\n" - TC " qdisc del dev eth0 ingress\n" - TC " qdisc add dev eth0 ingress\n" - TC " filter add dev eth0 parent ffff: protocol all u32 match u32 0 0 " - "police rate 1024kbps burst 1024kb mtu 64kb drop flowid :1\n")); + DO_TEST_SET("<bandwidth>" + " <outbound average='1024'/>" + "</bandwidth>", + TC " qdisc del dev eth0 root\n" + TC " qdisc del dev eth0 ingress\n" + TC " qdisc add dev eth0 ingress\n" + TC " filter add dev eth0 parent ffff: protocol all u32 match u32 0 0 " + "police rate 1024kbps burst 1024kb mtu 64kb drop flowid :1\n"); - DO_TEST_SET(("<bandwidth>" - " <inbound average='1' peak='2' floor='3' burst='4'/>" - " <outbound average='5' peak='6' burst='7'/>" - "</bandwidth>"), - (TC " qdisc del dev eth0 root\n" - TC " qdisc del dev eth0 ingress\n" - TC " qdisc add dev eth0 root handle 1: htb default 1\n" - TC " class add dev eth0 parent 1: classid 1:1 htb rate 1kbps ceil 2kbps burst 4kb quantum 1\n" - TC " qdisc add dev eth0 parent 1:1 handle 2: sfq perturb 10\n" - TC " filter add dev eth0 parent 1:0 protocol all prio 1 handle 1 fw flowid 1\n" - TC " qdisc add dev eth0 ingress\n" - TC " filter add dev eth0 parent ffff: protocol all u32 match u32 0 0 " - "police rate 5kbps burst 7kb mtu 64kb drop flowid :1\n")); + DO_TEST_SET("<bandwidth>" + " <inbound average='1' peak='2' floor='3' burst='4'/>" + " <outbound average='5' peak='6' burst='7'/>" + "</bandwidth>", + TC " qdisc del dev eth0 root\n" + TC " qdisc del dev eth0 ingress\n" + TC " qdisc add dev eth0 root handle 1: htb default 1\n" + TC " class add dev eth0 parent 1: classid 1:1 htb rate 1kbps ceil 2kbps burst 4kb quantum 1\n" + TC " qdisc add dev eth0 parent 1:1 handle 2: sfq perturb 10\n" + TC " filter add dev eth0 parent 1:0 protocol all prio 1 handle 1 fw flowid 1\n" + TC " qdisc add dev eth0 ingress\n" + TC " filter add dev eth0 parent ffff: protocol all u32 match u32 0 0 " + "police rate 5kbps burst 7kb mtu 64kb drop flowid :1\n"); - DO_TEST_SET(("<bandwidth>" - " <inbound average='4294967295'/>" - " <outbound average='4294967295'/>" - "</bandwidth>"), - (TC " qdisc del dev eth0 root\n" - TC " qdisc del dev eth0 ingress\n" - TC " qdisc add dev eth0 root handle 1: htb default 1\n" - TC " class add dev eth0 parent 1: classid 1:1 htb rate 4294967295kbps quantum 366503875\n" - TC " qdisc add dev eth0 parent 1:1 handle 2: sfq perturb 10\n" - TC " filter add dev eth0 parent 1:0 protocol all prio 1 handle 1 fw flowid 1\n" - TC " qdisc add dev eth0 ingress\n" - TC " filter add dev eth0 parent ffff: protocol all u32 match " - "u32 0 0 police rate 4294967295kbps burst 4194303kb mtu 64kb " - "drop flowid :1\n")); + DO_TEST_SET("<bandwidth>" + " <inbound average='4294967295'/>" + " <outbound average='4294967295'/>" + "</bandwidth>", + TC " qdisc del dev eth0 root\n" + TC " qdisc del dev eth0 ingress\n" + TC " qdisc add dev eth0 root handle 1: htb default 1\n" + TC " class add dev eth0 parent 1: classid 1:1 htb rate 4294967295kbps quantum 366503875\n" + TC " qdisc add dev eth0 parent 1:1 handle 2: sfq perturb 10\n" + TC " filter add dev eth0 parent 1:0 protocol all prio 1 handle 1 fw flowid 1\n" + TC " qdisc add dev eth0 ingress\n" + TC " filter add dev eth0 parent ffff: protocol all u32 match " + "u32 0 0 police rate 4294967295kbps burst 4194303kb mtu 64kb " + "drop flowid :1\n"); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.35.1
The last usage of the testMinimalStruct struct was removed in v1.2.2-rc1~206 which forgot to remove the struct as well. Remove it now. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virnetdevbandwidthtest.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/tests/virnetdevbandwidthtest.c b/tests/virnetdevbandwidthtest.c index 419793a4ae..f0873743b4 100644 --- a/tests/virnetdevbandwidthtest.c +++ b/tests/virnetdevbandwidthtest.c @@ -26,12 +26,6 @@ #define VIR_FROM_THIS VIR_FROM_NONE -struct testMinimalStruct { - const char *expected_result; - const char *band1; - const char *band2; -}; - struct testSetStruct { const char *band; const char *exp_cmd; -- 2.35.1
Our coding style expects a long line to be broken into shorter lines which are then aligned on the first character, for instance: "some string that's broken " "into multiple lines" However, one can argue that there are few cases where shifting the alignment makes the code more readable. And this is the case of expected cmd line for DO_TEST_SET() where a long cmd line can be aligned on the arguments rather than the binary: TC " filter ..." " police ..." Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virnetdevbandwidthtest.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/virnetdevbandwidthtest.c b/tests/virnetdevbandwidthtest.c index f0873743b4..51e5ec016b 100644 --- a/tests/virnetdevbandwidthtest.c +++ b/tests/virnetdevbandwidthtest.c @@ -127,8 +127,8 @@ mymain(void) TC " qdisc del dev eth0 root\n" TC " qdisc del dev eth0 ingress\n" TC " qdisc add dev eth0 ingress\n" - TC " filter add dev eth0 parent ffff: protocol all u32 match u32 0 0 " - "police rate 1024kbps burst 1024kb mtu 64kb drop flowid :1\n"); + TC " filter add dev eth0 parent ffff: protocol all u32 match u32 0 0" + " police rate 1024kbps burst 1024kb mtu 64kb drop flowid :1\n"); DO_TEST_SET("<bandwidth>" " <inbound average='1' peak='2' floor='3' burst='4'/>" @@ -141,8 +141,8 @@ mymain(void) TC " qdisc add dev eth0 parent 1:1 handle 2: sfq perturb 10\n" TC " filter add dev eth0 parent 1:0 protocol all prio 1 handle 1 fw flowid 1\n" TC " qdisc add dev eth0 ingress\n" - TC " filter add dev eth0 parent ffff: protocol all u32 match u32 0 0 " - "police rate 5kbps burst 7kb mtu 64kb drop flowid :1\n"); + TC " filter add dev eth0 parent ffff: protocol all u32 match u32 0 0" + " police rate 5kbps burst 7kb mtu 64kb drop flowid :1\n"); DO_TEST_SET("<bandwidth>" " <inbound average='4294967295'/>" @@ -155,9 +155,9 @@ mymain(void) TC " qdisc add dev eth0 parent 1:1 handle 2: sfq perturb 10\n" TC " filter add dev eth0 parent 1:0 protocol all prio 1 handle 1 fw flowid 1\n" TC " qdisc add dev eth0 ingress\n" - TC " filter add dev eth0 parent ffff: protocol all u32 match " - "u32 0 0 police rate 4294967295kbps burst 4194303kb mtu 64kb " - "drop flowid :1\n"); + TC " filter add dev eth0 parent ffff: protocol all u32 match" + " u32 0 0 police rate 4294967295kbps burst 4194303kb mtu 64kb" + " drop flowid :1\n"); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.35.1
Ever since v7.6.0-rc1~235 we can use ovs-vsctl to set QoS instead of tc. However, we don't have a test that's verifying generated cmd line for ovs-vsctl. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virnetdevbandwidthtest.c | 85 +++++++++++++++++++++++++++------- 1 file changed, 69 insertions(+), 16 deletions(-) diff --git a/tests/virnetdevbandwidthtest.c b/tests/virnetdevbandwidthtest.c index 51e5ec016b..d5e4092fc3 100644 --- a/tests/virnetdevbandwidthtest.c +++ b/tests/virnetdevbandwidthtest.c @@ -22,13 +22,17 @@ #define LIBVIRT_VIRCOMMANDPRIV_H_ALLOW #include "vircommandpriv.h" #include "virnetdevbandwidth.h" +#include "virnetdevopenvswitch.h" #include "netdev_bandwidth_conf.c" #define VIR_FROM_THIS VIR_FROM_NONE struct testSetStruct { const char *band; - const char *exp_cmd; + const char *exp_cmd_tc; + const char *exp_cmd_ovs; + bool ovs; + const unsigned char *uuid; const char *iface; const bool hierarchical_class; }; @@ -59,6 +63,7 @@ testVirNetDevBandwidthSet(const void *data) { const struct testSetStruct *info = data; const char *iface = info->iface; + const char *exp_cmd = NULL; g_autoptr(virNetDevBandwidth) band = NULL; g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; g_autofree char *actual_cmd = NULL; @@ -72,17 +77,24 @@ testVirNetDevBandwidthSet(const void *data) virCommandSetDryRun(dryRunToken, &buf, false, false, NULL, NULL); - if (virNetDevBandwidthSet(iface, band, info->hierarchical_class, true) < 0) - return -1; + if (info->ovs) { + exp_cmd = info->exp_cmd_ovs; + if (virNetDevOpenvswitchInterfaceSetQos(iface, band, info->uuid, true) < 0) + return -1; + } else { + exp_cmd = info->exp_cmd_tc; + if (virNetDevBandwidthSet(iface, band, info->hierarchical_class, true) < 0) + return -1; + } if (!(actual_cmd = virBufferContentAndReset(&buf))) { /* This is interesting, no command has been executed. * Maybe that's expected, actually. */ } - if (STRNEQ_NULLABLE(info->exp_cmd, actual_cmd)) { + if (STRNEQ_NULLABLE(exp_cmd, actual_cmd)) { virTestDifference(stderr, - NULLSTR(info->exp_cmd), + NULLSTR(exp_cmd), NULLSTR(actual_cmd)); return -1; } @@ -94,22 +106,37 @@ static int mymain(void) { int ret = 0; + unsigned char uuid[VIR_UUID_BUFLEN] = { 0 }; -#define DO_TEST_SET(Band, Exp_cmd, ...) \ +#define VMUUID "c1018351-a229-4209-9faf-42446e0b53e5" + + if (virUUIDParse(VMUUID, uuid) < 0) + return -1; + +#define DO_TEST_SET(Band, Exp_cmd_tc, Exp_cmd_ovs, ...) \ do { \ struct testSetStruct data = {.band = Band, \ - .exp_cmd = Exp_cmd, \ + .exp_cmd_tc = Exp_cmd_tc, \ + .exp_cmd_ovs = Exp_cmd_ovs, \ + .ovs = false, \ + .uuid = uuid, \ __VA_ARGS__}; \ - if (virTestRun("virNetDevBandwidthSet", \ + if (virTestRun("virNetDevBandwidthSet TC", \ testVirNetDevBandwidthSet, \ - &data) < 0) \ + &data) < 0) { \ ret = -1; \ + } \ + data.ovs = true; \ + if (virTestRun("virNetDevBandwidthSet OVS", \ + testVirNetDevBandwidthSet, \ + &data) < 0) { \ + ret = -1; \ + } \ } while (0) + DO_TEST_SET(NULL, NULL, NULL); - DO_TEST_SET(NULL, NULL); - - DO_TEST_SET("<bandwidth/>", NULL); + DO_TEST_SET("<bandwidth/>", NULL, NULL); DO_TEST_SET("<bandwidth>" " <inbound average='1024'/>" @@ -119,7 +146,14 @@ mymain(void) TC " qdisc add dev eth0 root handle 1: htb default 1\n" TC " class add dev eth0 parent 1: classid 1:1 htb rate 1024kbps quantum 87\n" TC " qdisc add dev eth0 parent 1:1 handle 2: sfq perturb 10\n" - TC " filter add dev eth0 parent 1:0 protocol all prio 1 handle 1 fw flowid 1\n"); + TC " filter add dev eth0 parent 1:0 protocol all prio 1 handle 1 fw flowid 1\n", + OVS_VSCTL " --timeout=5 --no-heading --columns=_uuid find queue 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" + OVS_VSCTL " --timeout=5 --no-heading --columns=_uuid find qos 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" + OVS_VSCTL " --timeout=5 set port eth0 qos=@qos1 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"' --" + " --id=@qos1 create qos type=linux-htb other_config:min-rate=8192000 queues:0=@queue0 'external-ids:vm-id=\"" VMUUID "\"'" + " 'external-ids:ifname=\"eth0\"' --" + " --id=@queue0 create queue other_config:min-rate=8192000 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" + OVS_VSCTL " --timeout=5 set Interface eth0 ingress_policing_rate=0 ingress_policing_burst=0\n"); DO_TEST_SET("<bandwidth>" " <outbound average='1024'/>" @@ -128,7 +162,10 @@ mymain(void) TC " qdisc del dev eth0 ingress\n" TC " qdisc add dev eth0 ingress\n" TC " filter add dev eth0 parent ffff: protocol all u32 match u32 0 0" - " police rate 1024kbps burst 1024kb mtu 64kb drop flowid :1\n"); + " police rate 1024kbps burst 1024kb mtu 64kb drop flowid :1\n", + OVS_VSCTL " --timeout=5 --no-heading --columns=_uuid find queue 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" + OVS_VSCTL " --timeout=5 --no-heading --columns=_uuid find qos 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" + OVS_VSCTL " --timeout=5 set Interface eth0 ingress_policing_rate=8192\n"); DO_TEST_SET("<bandwidth>" " <inbound average='1' peak='2' floor='3' burst='4'/>" @@ -142,7 +179,15 @@ mymain(void) TC " filter add dev eth0 parent 1:0 protocol all prio 1 handle 1 fw flowid 1\n" TC " qdisc add dev eth0 ingress\n" TC " filter add dev eth0 parent ffff: protocol all u32 match u32 0 0" - " police rate 5kbps burst 7kb mtu 64kb drop flowid :1\n"); + " police rate 5kbps burst 7kb mtu 64kb drop flowid :1\n", + OVS_VSCTL " --timeout=5 --no-heading --columns=_uuid find queue 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" + OVS_VSCTL " --timeout=5 --no-heading --columns=_uuid find qos 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" + OVS_VSCTL " --timeout=5 set port eth0 qos=@qos1 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"' --" + " --id=@qos1 create qos type=linux-htb other_config:min-rate=8000 other_config:burst=32768 other_config:max-rate=16000" + " queues:0=@queue0 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"' --" + " --id=@queue0 create queue other_config:min-rate=8000 other_config:burst=32768 other_config:max-rate=16000" + " 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" + OVS_VSCTL " --timeout=5 set Interface eth0 ingress_policing_rate=40 ingress_policing_burst=56\n"); DO_TEST_SET("<bandwidth>" " <inbound average='4294967295'/>" @@ -157,7 +202,15 @@ mymain(void) TC " qdisc add dev eth0 ingress\n" TC " filter add dev eth0 parent ffff: protocol all u32 match" " u32 0 0 police rate 4294967295kbps burst 4194303kb mtu 64kb" - " drop flowid :1\n"); + " drop flowid :1\n", + OVS_VSCTL " --timeout=5 --no-heading --columns=_uuid find queue 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" + OVS_VSCTL " --timeout=5 --no-heading --columns=_uuid find qos 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n" + OVS_VSCTL " --timeout=5 set port eth0 qos=@qos1 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"' --" + " --id=@qos1 create qos type=linux-htb other_config:min-rate=34359738360000" + " queues:0=@queue0 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"' --" + " --id=@queue0 create queue other_config:min-rate=34359738360000 'external-ids:vm-id=\"" VMUUID "\"'" + " 'external-ids:ifname=\"eth0\"'\n" + OVS_VSCTL " --timeout=5 set Interface eth0 ingress_policing_rate=34359738360\n"); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.35.1
On Tue, Jun 28, 2022 at 04:17:31PM +0200, Michal Privoznik wrote:
Cleanup virnetdevbandwidthtest and introduce new test cases. However, Martin suggested (after glancing over the last patch) that arguments could be stored in a file (just like qemuxml2argvtest does). I'm not against that, but QEMU cmd line args change more often than those of tc/ovs-vsctl. But if desired I can do the change (either as a follow up or in v2).
I, personally, do not need that, I just figured it could be nicer for someone. Anyway, good that you added the tests, I remember the units were not fun to figure out with the inconsistencies wrt tc, so it's good we have it in the tests too now.
Michal Prívozník (4): virnetdevbandwidthtest: Drop unnecessary brackets virnetdevbandwidthtest: Drop unused testMinimalStruct virnetdevbandwidthtest: Reformat TC cmd line virnetdevbandwidthtest: Test QoS for OVS
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
tests/virnetdevbandwidthtest.c | 171 +++++++++++++++++++++------------ 1 file changed, 109 insertions(+), 62 deletions(-)
-- 2.35.1
participants (2)
-
Martin Kletzander -
Michal Privoznik