[libvirt] [PATCH 0/4] Improve QoS and unit-test it
*** BLURB HERE *** Michal Privoznik (4): networkAllocateActualDevice: Set QoS for bridgeless networks too tests: Introduce virnetdevbandwidthtest virnetdevbandwidthtest: Introduce mocking virnetdevbandwidthtest: Introduce testVirNetDevBandwidthSet src/libvirt_private.syms | 1 + src/network/bridge_driver.c | 25 ++++- src/util/virnetdevbandwidth.c | 88 +++++++++++++++ src/util/virnetdevbandwidth.h | 6 + tests/Makefile.am | 14 +++ tests/virnetdevbandwidthmock.c | 106 ++++++++++++++++++ tests/virnetdevbandwidthtest.c | 244 +++++++++++++++++++++++++++++++++++++++++ 7 files changed, 482 insertions(+), 2 deletions(-) create mode 100644 tests/virnetdevbandwidthmock.c create mode 100644 tests/virnetdevbandwidthtest.c -- 1.8.5.2
https://bugzilla.redhat.com/show_bug.cgi?id=1055484 Currently, libvirt's XML schema of network allows QoS to be defined for every network even though it has no bridge. For instance: <network> <name>vdsm-no-bridge</name> <forward mode='passthrough'> <interface dev='em1.10'/> </forward> <bandwidth> <inbound average='1000' peak='5000' burst='1024'/> <outbound average='1000' burst='1024'/> </bandwidth> </network> The bandwidth limitations can be, however, applied even on such networks. In fact, they are gonna be applied on the interface that will be connected to the network on a domain startup. This approach, however, has one limitation. With bridged networks, there are two points where QoS can be set: bridge and domain interface. The lower limit of the two is enforced then. For instance, if the interface has 10Mbps average, but the network only 1Mbps, there's no way for interface to transmit packets faster than the 1Mbps limit. With two points this is enforced by kernel. With only one point, we must combine both QoS settings into one which is set afterwards. Look at virNetDevBandwidthMinimal() and you'll understand immediately what I mean. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/network/bridge_driver.c | 25 +++++++++++- src/util/virnetdevbandwidth.c | 88 +++++++++++++++++++++++++++++++++++++++++++ src/util/virnetdevbandwidth.h | 6 +++ 4 files changed, 118 insertions(+), 2 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 13caf93..0c16343 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1469,6 +1469,7 @@ virNetDevBandwidthClear; virNetDevBandwidthCopy; virNetDevBandwidthEqual; virNetDevBandwidthFree; +virNetDevBandwidthMinimal; virNetDevBandwidthPlug; virNetDevBandwidthSet; virNetDevBandwidthUnplug; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 0b43a67..904aad5 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -3284,9 +3284,30 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) else if (portgroup && portgroup->bandwidth) bandwidth = portgroup->bandwidth; - if (bandwidth && virNetDevBandwidthCopy(&iface->data.network.actual->bandwidth, - bandwidth) < 0) + /* For a bridgeless networks we must take into account any QoS set to them. + * This is, however, a bit tricky. With bridged network there are two + * points where QoS can be set: domain's interface and the bridge. The + * limits on those differ (in general) in which case the lower limit gets + * into effect. For example, if domain's interface average is 10Mbps but + * the network's is just 1Mbps, the domain will not be able to send data + * any faster than 1Mbps. However, on bridgeless networks we can't just set + * those two points and let kernel do its job since we have only single + * point. Therefore, we must combine the QoS settings from both domain's + * interface and network and choose the minimal value from pairs. + */ + if (netdef->forward.type == VIR_NETWORK_FORWARD_PRIVATE || + netdef->forward.type == VIR_NETWORK_FORWARD_VEPA || + netdef->forward.type == VIR_NETWORK_FORWARD_PASSTHROUGH || + netdef->forward.type == VIR_NETWORK_FORWARD_HOSTDEV) { + if (virNetDevBandwidthMinimal(&iface->data.network.actual->bandwidth, + bandwidth, + netdef->bandwidth) < 0) + goto error; + } else if (bandwidth && + virNetDevBandwidthCopy(&iface->data.network.actual->bandwidth, + bandwidth) < 0) { goto error; + } /* copy appropriate vlan info to actualNet */ if (iface->vlan.nTags > 0) diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c index ed6a19d..05f3d6e 100644 --- a/src/util/virnetdevbandwidth.c +++ b/src/util/virnetdevbandwidth.c @@ -27,6 +27,7 @@ #include "viralloc.h" #include "virerror.h" #include "virstring.h" +#include "virutil.h" #define VIR_FROM_THIS VIR_FROM_NONE @@ -588,3 +589,90 @@ cleanup: VIR_FREE(ceil); return ret; } + +static void +virNetDevBandwidthRateMinimal(virNetDevBandwidthRatePtr result, + virNetDevBandwidthRatePtr band1, + virNetDevBandwidthRatePtr band2) +{ + if (!band1 || !band2) { + memcpy(result, band1 ? band1 : band2, sizeof(*result)); + return; + } + + /* We can't do a simple MIN() here, because zero value doesn't mean the + * narrowest limit, but and unset value. Hence we need symmetric F(a, b) + * so that: + * F(a, 0) = F(0, a) = a; special corner case: F(0, 0) = 0 + * F(a, b) = MIN(a, b) for a != 0 and b != 0 + */ +#define NON_ZERO_MIN(a, b) \ + ((a) && (b) ? MIN(a, b) : (a) ? (a) : (b)) + + result->average = NON_ZERO_MIN(band1->average, band2->average); + result->peak = NON_ZERO_MIN(band1->peak, band2->peak); + result->floor = NON_ZERO_MIN(band1->floor, band2->floor); + result->burst = NON_ZERO_MIN(band1->burst, band2->burst); +} + +/** + * virNetDevBandwidthMinimal: + * @result: the minimal intersect of @band1 and @band2 + * @band1: the first bandwidth + * @band2: the second bandwidth + * + * Combine the two bandwidths into one with choosing the minimal value for each + * pair of items within bandwidth structure. The resulting bandwidth is stored + * into @result. In case of both @band1 and @band2 being NULL pointers, the + * @ret is set to NULL as well as there's no bandwidth to calculate (this is + * not considered an error). The caller is responsible for freeing @result when + * no longer needed. + * + * Returns 0 on success, -1 otherwise. + */ +int +virNetDevBandwidthMinimal(virNetDevBandwidthPtr *result, + virNetDevBandwidthPtr band1, + virNetDevBandwidthPtr band2) +{ + int ret = -1; + + if (!band1 && !band2) { + /* Nothing to compute */ + *result = NULL; + return 0; + } + + if (!band1 || !band2) { + /* Sweet, one of the args is NULL. The non-NULL one + * is our minimum then. */ + return virNetDevBandwidthCopy(result, band1 ? band1 : band2); + } + + if (VIR_ALLOC(*result) < 0) + goto cleanup; + + if (!band1->in && !band2->in) { + /* nada */ + } else { + if (VIR_ALLOC((*result)->in) < 0) + goto cleanup; + + virNetDevBandwidthRateMinimal((*result)->in, band1->in, band2->in); + } + + if (!band1->out && !band2->out) { + /* nada */ + } else { + if (VIR_ALLOC((*result)->out) < 0) + goto cleanup; + + virNetDevBandwidthRateMinimal((*result)->out, band1->out, band2->out); + } + + ret = 0; +cleanup: + if (ret < 0) + VIR_FREE(*result); + return ret; +} diff --git a/src/util/virnetdevbandwidth.h b/src/util/virnetdevbandwidth.h index 4606a07..ea40df4 100644 --- a/src/util/virnetdevbandwidth.h +++ b/src/util/virnetdevbandwidth.h @@ -74,4 +74,10 @@ int virNetDevBandwidthUpdateRate(const char *ifname, unsigned long long new_rate) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; + +int +virNetDevBandwidthMinimal(virNetDevBandwidthPtr *result, + virNetDevBandwidthPtr band1, + virNetDevBandwidthPtr band2) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; #endif /* __VIR_NETDEV_BANDWIDTH_H__ */ -- 1.8.5.2
On 01/23/2014 06:44 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1055484
Currently, libvirt's XML schema of network allows QoS to be defined for every network even though it has no bridge. For instance:
<network> <name>vdsm-no-bridge</name> <forward mode='passthrough'> <interface dev='em1.10'/> </forward> <bandwidth> <inbound average='1000' peak='5000' burst='1024'/> <outbound average='1000' burst='1024'/> </bandwidth> </network>
The bandwidth limitations can be, however, applied even on such networks. In fact, they are gonna be applied on the interface that will
s/gonna/going to/
be connected to the network on a domain startup. This approach, however, has one limitation. With bridged networks, there are two points where QoS can be set: bridge and domain interface. The lower limit of the two is enforced then. For instance, if the interface has 10Mbps average, but the network only 1Mbps, there's no way for interface to transmit packets faster than the 1Mbps limit. With two points this is enforced by kernel. With only one point, we must combine both QoS settings into one which is set afterwards. Look at virNetDevBandwidthMinimal() and you'll understand immediately what I mean.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/network/bridge_driver.c | 25 +++++++++++- src/util/virnetdevbandwidth.c | 88 +++++++++++++++++++++++++++++++++++++++++++ src/util/virnetdevbandwidth.h | 6 +++ 4 files changed, 118 insertions(+), 2 deletions(-)
- if (bandwidth && virNetDevBandwidthCopy(&iface->data.network.actual->bandwidth, - bandwidth) < 0) + /* For a bridgeless networks we must take into account any QoS set to them.
s/For a/For/
+ +static void +virNetDevBandwidthRateMinimal(virNetDevBandwidthRatePtr result, + virNetDevBandwidthRatePtr band1, + virNetDevBandwidthRatePtr band2) +{ + if (!band1 || !band2) { + memcpy(result, band1 ? band1 : band2, sizeof(*result));
Isn't this a NULL deref if both band1 and band2 are NULL? [1]
+ return; + } + + /* We can't do a simple MIN() here, because zero value doesn't mean the + * narrowest limit, but and unset value. Hence we need symmetric F(a, b)
s/and/an/
+ * so that: + * F(a, 0) = F(0, a) = a; special corner case: F(0, 0) = 0 + * F(a, b) = MIN(a, b) for a != 0 and b != 0 + */ +#define NON_ZERO_MIN(a, b) \ + ((a) && (b) ? MIN(a, b) : (a) ? (a) : (b))
Nice.
+virNetDevBandwidthMinimal(virNetDevBandwidthPtr *result, + virNetDevBandwidthPtr band1, + virNetDevBandwidthPtr band2) +{ + int ret = -1; + + if (!band1 && !band2) { + /* Nothing to compute */ + *result = NULL; + return 0; + }
[1] Ahh, nice, you never call the helper function with both sides NULL.
+ + if (!band1->in && !band2->in) { + /* nada */ + } else {
This looks odd; I would use deMorgan's law and write it as a one-liner condition instead of an awkward 3-line if/else: if (band1->in || band2->in) {
+ if (VIR_ALLOC((*result)->in) < 0) + goto cleanup; + + virNetDevBandwidthRateMinimal((*result)->in, band1->in, band2->in); + } + + if (!band1->out && !band2->out) { + /* nada */ + } else {
and again. ACK with those fixes. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/23/2014 03:44 PM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1055484
Currently, libvirt's XML schema of network allows QoS to be defined for every network even though it has no bridge. For instance:
<network> <name>vdsm-no-bridge</name> <forward mode='passthrough'> <interface dev='em1.10'/>
You've picked a bad example config - "passthrough" forward mode requires exclusive use of a host interface by a single guest, so this network could never have more than a single guest interface using it. To see the problem in your method of solution, you would need to have multiple <interface> elements (or <forward mode='bridge'/>). In either of those cases you end up with an aggregate network bandwidth of "Netlimit" * "# of attached guests". My understanding of the <bandwidth> setting in a <network> is that it is specifying the maximum aggregate bandwidth of all guest interfaces connected to that network not the maximum bandwidth of a single guest interface. If you want to easily specify the individual bandwidth to all guest interfaces on a network, you can just add a default <portgroup> to the network, and put a <bandwidth> element in there. (Of course, any <bandwidth> in a guest's <interface> definition would override the bandwidth in the portgroup (we discussed precedence at the time that code was added, and decided this was the more useful way, since at the time anyone who could modify any domain definition could modify any network definition too), but if you want a method for a <network> definition to override individual bandwidth limits, that could be added as an option to the <portgroup> definition.)
</forward> <bandwidth> <inbound average='1000' peak='5000' burst='1024'/> <outbound average='1000' burst='1024'/> </bandwidth> </network>
The bandwidth limitations can be, however, applied even on such networks. In fact, they are gonna be applied on the interface that will be connected to the network on a domain startup. This approach, however, has one limitation. With bridged networks, there are two points where QoS can be set: bridge and domain interface. The lower limit of the two is enforced then. For instance, if the interface has 10Mbps average, but the network only 1Mbps, there's no way for interface to transmit packets faster than the 1Mbps limit. With two points this is enforced by kernel. With only one point, we must combine both QoS settings into one which is set afterwards. Look at virNetDevBandwidthMinimal() and you'll understand immediately what I mean.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/network/bridge_driver.c | 25 +++++++++++- src/util/virnetdevbandwidth.c | 88 +++++++++++++++++++++++++++++++++++++++++++ src/util/virnetdevbandwidth.h | 6 +++ 4 files changed, 118 insertions(+), 2 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 13caf93..0c16343 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1469,6 +1469,7 @@ virNetDevBandwidthClear; virNetDevBandwidthCopy; virNetDevBandwidthEqual; virNetDevBandwidthFree; +virNetDevBandwidthMinimal; virNetDevBandwidthPlug; virNetDevBandwidthSet; virNetDevBandwidthUnplug; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 0b43a67..904aad5 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -3284,9 +3284,30 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) else if (portgroup && portgroup->bandwidth) bandwidth = portgroup->bandwidth;
- if (bandwidth && virNetDevBandwidthCopy(&iface->data.network.actual->bandwidth, - bandwidth) < 0) + /* For a bridgeless networks we must take into account any QoS set to them. + * This is, however, a bit tricky. With bridged network there are two + * points where QoS can be set: domain's interface and the bridge. The + * limits on those differ (in general) in which case the lower limit gets + * into effect. For example, if domain's interface average is 10Mbps but + * the network's is just 1Mbps, the domain will not be able to send data + * any faster than 1Mbps. However, on bridgeless networks we can't just set + * those two points and let kernel do its job since we have only single + * point. Therefore, we must combine the QoS settings from both domain's + * interface and network and choose the minimal value from pairs.
From this description, it sounds like if you have a network with a limit of 1Mbps, and 20 guest interfaces connected to that network, then you will set a limit of 1Mbps on each guest interface, meaning that the maximum bandwidth that could come from the guests on that network is 20Mbps, not 1Mbps. So in fact this is not implementing the intent of the XML.
I know this has already been pushed (I was unfortunately out of town), but if it hadn't been, I would NACK it pending a correction of my interpretation.
On 29.01.2014 15:26, Laine Stump wrote:
On 01/23/2014 03:44 PM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1055484
Currently, libvirt's XML schema of network allows QoS to be defined for every network even though it has no bridge. For instance:
<network> <name>vdsm-no-bridge</name> <forward mode='passthrough'> <interface dev='em1.10'/>
You've picked a bad example config - "passthrough" forward mode requires exclusive use of a host interface by a single guest, so this network could never have more than a single guest interface using it.
To see the problem in your method of solution, you would need to have multiple <interface> elements (or <forward mode='bridge'/>). In either of those cases you end up with an aggregate network bandwidth of "Netlimit" * "# of attached guests".
My understanding of the <bandwidth> setting in a <network> is that it is specifying the maximum aggregate bandwidth of all guest interfaces connected to that network not the maximum bandwidth of a single guest interface. If you want to easily specify the individual bandwidth to all guest interfaces on a network, you can just add a default <portgroup> to the network, and put a <bandwidth> element in there.
(Of course, any <bandwidth> in a guest's <interface> definition would override the bandwidth in the portgroup (we discussed precedence at the time that code was added, and decided this was the more useful way, since at the time anyone who could modify any domain definition could modify any network definition too), but if you want a method for a <network> definition to override individual bandwidth limits, that could be added as an option to the <portgroup> definition.)
</forward> <bandwidth> <inbound average='1000' peak='5000' burst='1024'/> <outbound average='1000' burst='1024'/> </bandwidth> </network>
The bandwidth limitations can be, however, applied even on such networks. In fact, they are gonna be applied on the interface that will be connected to the network on a domain startup. This approach, however, has one limitation. With bridged networks, there are two points where QoS can be set: bridge and domain interface. The lower limit of the two is enforced then. For instance, if the interface has 10Mbps average, but the network only 1Mbps, there's no way for interface to transmit packets faster than the 1Mbps limit. With two points this is enforced by kernel. With only one point, we must combine both QoS settings into one which is set afterwards. Look at virNetDevBandwidthMinimal() and you'll understand immediately what I mean.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/network/bridge_driver.c | 25 +++++++++++- src/util/virnetdevbandwidth.c | 88 +++++++++++++++++++++++++++++++++++++++++++ src/util/virnetdevbandwidth.h | 6 +++ 4 files changed, 118 insertions(+), 2 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 13caf93..0c16343 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1469,6 +1469,7 @@ virNetDevBandwidthClear; virNetDevBandwidthCopy; virNetDevBandwidthEqual; virNetDevBandwidthFree; +virNetDevBandwidthMinimal; virNetDevBandwidthPlug; virNetDevBandwidthSet; virNetDevBandwidthUnplug; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 0b43a67..904aad5 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -3284,9 +3284,30 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) else if (portgroup && portgroup->bandwidth) bandwidth = portgroup->bandwidth;
- if (bandwidth && virNetDevBandwidthCopy(&iface->data.network.actual->bandwidth, - bandwidth) < 0) + /* For a bridgeless networks we must take into account any QoS set to them. + * This is, however, a bit tricky. With bridged network there are two + * points where QoS can be set: domain's interface and the bridge. The + * limits on those differ (in general) in which case the lower limit gets + * into effect. For example, if domain's interface average is 10Mbps but + * the network's is just 1Mbps, the domain will not be able to send data + * any faster than 1Mbps. However, on bridgeless networks we can't just set + * those two points and let kernel do its job since we have only single + * point. Therefore, we must combine the QoS settings from both domain's + * interface and network and choose the minimal value from pairs.
From this description, it sounds like if you have a network with a limit of 1Mbps, and 20 guest interfaces connected to that network, then you will set a limit of 1Mbps on each guest interface, meaning that the maximum bandwidth that could come from the guests on that network is 20Mbps, not 1Mbps. So in fact this is not implementing the intent of the XML.
I know this has already been pushed (I was unfortunately out of town), but if it hadn't been, I would NACK it pending a correction of my interpretation.
Your interpretation is indeed correct. I must have gone mad when writing this patch. Now that I'm thinking over the problem again, there's no easy way how to achieve what's being asked. Hence, I'm sending patch to revert the commit. Sorry for the noise. Michal
The only API tested so far would be virNetDevBandwidthMinimal. But there's more to come! Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/Makefile.am | 5 ++ tests/virnetdevbandwidthtest.c | 155 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 160 insertions(+) create mode 100644 tests/virnetdevbandwidthtest.c diff --git a/tests/Makefile.am b/tests/Makefile.am index 568b7a0..0930073 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -141,6 +141,7 @@ test_programs = virshtest sockettest \ virportallocatortest \ sysinfotest \ virstoragetest \ + virnetdevbandwidthtest \ $(NULL) if WITH_REMOTE @@ -650,6 +651,10 @@ commandhelper_SOURCES = \ commandhelper_LDADD = $(LDADDS) commandhelper_LDFLAGS = -static +virnetdevbandwidthtest_SOURCES = \ + virnetdevbandwidthtest.c testutils.h testutils.c +virnetdevbandwidthtest_LDADD = $(LDADDS) + if WITH_LIBVIRTD libvirtdconftest_SOURCES = \ libvirtdconftest.c testutils.h testutils.c \ diff --git a/tests/virnetdevbandwidthtest.c b/tests/virnetdevbandwidthtest.c new file mode 100644 index 0000000..989018e --- /dev/null +++ b/tests/virnetdevbandwidthtest.c @@ -0,0 +1,155 @@ +/* + * Copyright (C) 2014 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Michal Privoznik <mprivozn@redhat.com> + */ + +#include <config.h> + +#include "testutils.h" +#include "virnetdevbandwidth.h" +#include "netdev_bandwidth_conf.c" + +#define VIR_FROM_THIS VIR_FROM_NONE + +struct testMinimalStruct { + const char *expected_result; + const char *band1; + const char *band2; +}; + +#define PARSE(xml, var) \ + do { \ + xmlDocPtr doc; \ + xmlXPathContextPtr ctxt = NULL; \ + \ + if (!xml) \ + break; \ + \ + if (!(doc = virXMLParseStringCtxt((xml), \ + "bandwidth definition", \ + &ctxt))) \ + goto cleanup; \ + \ + (var) = virNetDevBandwidthParse(ctxt->node, \ + VIR_DOMAIN_NET_TYPE_NETWORK); \ + xmlFreeDoc(doc); \ + xmlXPathFreeContext(ctxt); \ + if (!(var)) \ + goto cleanup; \ + } while (0) + +static int +testVirNetDevBandwidthMinimal(const void *data) +{ + int ret = -1; + const struct testMinimalStruct *info = data; + virNetDevBandwidthPtr expected_result = NULL, result = NULL, + band1 = NULL, band2 = NULL; + + + /* Parse given XMLs */ + PARSE(info->expected_result, expected_result); + PARSE(info->band1, band1); + PARSE(info->band2, band2); + + if (virNetDevBandwidthMinimal(&result, band1, band2) < 0) + goto cleanup; + + if (!virNetDevBandwidthEqual(expected_result, result)) { + virBuffer exp_buf = VIR_BUFFER_INITIALIZER, + res_buf = VIR_BUFFER_INITIALIZER; + char *exp = NULL, *res = NULL; + + fprintf(stderr, "expected_result != result"); + + if (virNetDevBandwidthFormat(expected_result, &exp_buf) < 0 || + virNetDevBandwidthFormat(result, &res_buf) < 0 || + !(exp = virBufferContentAndReset(&exp_buf)) || + !(res = virBufferContentAndReset(&res_buf))) { + fprintf(stderr, "Failed to fail"); + virBufferFreeAndReset(&exp_buf); + virBufferFreeAndReset(&res_buf); + VIR_FREE(exp); + VIR_FREE(res); + goto cleanup; + } + + virtTestDifference(stderr, exp, res); + VIR_FREE(exp); + VIR_FREE(res); + } + + ret = 0; +cleanup: + virNetDevBandwidthFree(expected_result); + virNetDevBandwidthFree(result); + virNetDevBandwidthFree(band1); + virNetDevBandwidthFree(band2); + return ret; +} + +static int +mymain(void) +{ + int ret = 0; + +#define DO_TEST_MINIMAL(r, ...) \ + do { \ + struct testMinimalStruct data = {r, __VA_ARGS__}; \ + if (virtTestRun("virNetDevBandwidthMinimal", \ + testVirNetDevBandwidthMinimal, \ + &data) < 0) \ + ret = -1; \ + } while (0) + + + DO_TEST_MINIMAL(NULL, NULL, NULL); + + DO_TEST_MINIMAL("<bandwidth>" + " <inbound average='1000' peak='5000' burst='5120'/>" + " <outbound average='128' peak='256' burst='256'/>" + "</bandwidth>", + .band1 = "<bandwidth>" + " <inbound average='1000' peak='5000' burst='5120'/>" + " <outbound average='128' peak='256' burst='256'/>" + "</bandwidth>"); + + DO_TEST_MINIMAL("<bandwidth>" + " <inbound average='1000' peak='5000' burst='5120'/>" + " <outbound average='128' peak='256' burst='256'/>" + "</bandwidth>", + .band2 = "<bandwidth>" + " <inbound average='1000' peak='5000' burst='5120'/>" + " <outbound average='128' peak='256' burst='256'/>" + "</bandwidth>"); + DO_TEST_MINIMAL("<bandwidth>" + " <inbound average='1' peak='2' floor='3' burst='4'/>" + " <outbound average='5' peak='6' burst='7'/>" + "</bandwidth>", + "<bandwidth>" + " <inbound average='1' peak='2' burst='4'/>" + " <outbound average='0' burst='7'/>" + "</bandwidth>", + "<bandwidth>" + " <inbound average='1' peak='2' floor='3'/>" + " <outbound average='5' peak='6'/>" + "</bandwidth>"); + return ret; +} + +VIRT_TEST_MAIN(mymain); -- 1.8.5.2
On 01/23/2014 06:44 AM, Michal Privoznik wrote:
The only API tested so far would be virNetDevBandwidthMinimal. But there's more to come!
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/Makefile.am | 5 ++ tests/virnetdevbandwidthtest.c | 155 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 160 insertions(+) create mode 100644 tests/virnetdevbandwidthtest.c
+ +#define PARSE(xml, var) \ + do { \ + xmlDocPtr doc; \ + xmlXPathContextPtr ctxt = NULL; \ + \ + if (!xml) \ + break; \ + \ + if (!(doc = virXMLParseStringCtxt((xml), \
The () around xml is redundant here, but doesn't hurt to leave it in. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
The mocking will be used in later commits to mock all calls to the virCommandRun(). This is easier to do than cutting off the command creation and run into two separate pieces. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/Makefile.am | 9 ++++ tests/virnetdevbandwidthmock.c | 106 +++++++++++++++++++++++++++++++++++++++++ tests/virnetdevbandwidthtest.c | 21 +++++++- 3 files changed, 135 insertions(+), 1 deletion(-) create mode 100644 tests/virnetdevbandwidthmock.c diff --git a/tests/Makefile.am b/tests/Makefile.am index 0930073..f914244 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -319,6 +319,7 @@ test_libraries = libshunload.la \ virnetserverclientmock.la \ vircgroupmock.la \ virpcimock.la \ + virnetdevbandwidthmock.la \ $(NULL) if WITH_QEMU test_libraries += libqemumonitortestutils.la @@ -655,6 +656,14 @@ virnetdevbandwidthtest_SOURCES = \ virnetdevbandwidthtest.c testutils.h testutils.c virnetdevbandwidthtest_LDADD = $(LDADDS) +virnetdevbandwidthmock_la_SOURCES = \ + virnetdevbandwidthmock.c +virnetdevbandwidthmock_la_CFLAGS = $(AM_CFLAGS) +virnetdevbandwidthmock_la_LIBADD = $(GNULIB_LIBS) \ + ../src/libvirt.la +virnetdevbandwidthmock_la_LDFLAGS = -module -avoid-version \ + -rpath /evil/libtool/hack/to/force/shared/lib/creation + if WITH_LIBVIRTD libvirtdconftest_SOURCES = \ libvirtdconftest.c testutils.h testutils.c \ diff --git a/tests/virnetdevbandwidthmock.c b/tests/virnetdevbandwidthmock.c new file mode 100644 index 0000000..8bb590b --- /dev/null +++ b/tests/virnetdevbandwidthmock.c @@ -0,0 +1,106 @@ +/* + * Copyright (C) 2014 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Michal Privoznik <mprivozn@redhat.com> + */ + +#include <config.h> + +#include <dlfcn.h> +#include <fcntl.h> +#include <stdlib.h> +#include <sys/stat.h> +#include <sys/types.h> +#include "viralloc.h" +#include "vircommand.h" +#include "virfile.h" + +static int (*realvirCommandRun)(virCommandPtr cmd, int *exitstatus); + +/* Don't make static, since it causes problems with clang + * when passed as an arg to virAsprintf() + * vircgroupmock.c:462:22: error: static variable 'fakesysfsdir' is used in an inline function with external linkage [-Werror,-Wstatic-in-inline] + */ +char *outfile; + +#define STDERR(...) \ + fprintf(stderr, "%s %zu: ", __FUNCTION__, (size_t) __LINE__); \ + fprintf(stderr, __VA_ARGS__); \ + fprintf(stderr, "\n"); \ + +#define ABORT(...) \ + do { \ + STDERR(__VA_ARGS__); \ + abort(); \ + } while (0) + +static void +init(void) +{ + if (outfile) + return; + + if (!(outfile = getenv("LIBVIRT_OUTFILE"))) + ABORT("Missing LIBVIRT_OUTDIR env variable\n"); + +#define LOAD_SYM(name) \ + do { \ + if (!(real ## name = dlsym(RTLD_NEXT, #name))) \ + ABORT("Cannot find real '%s' symbol\n", #name); \ + } while (0) + + LOAD_SYM(virCommandRun); +} + +/* Here we don't really run the command, but instead get its string + * representation which is then written to a temporary file referenced + * by @outfile. + */ +int +virCommandRun(virCommandPtr cmd, int *exitstatus) +{ + int ret = -1; + int fd = 0; + char *buf = NULL; + + if (!outfile) + init(); + + if (!(buf = virCommandToString(cmd))) { + *exitstatus = EXIT_FAILURE; + return 0; + } + + if ((fd = open(outfile, O_WRONLY | O_APPEND | O_CREAT, S_IRUSR | S_IWUSR)) == -1) { + STDERR("unable to open file: %s %d", outfile, errno); + goto cleanup; + } + + if (safewrite(fd, buf, strlen(buf)) < 0 || + safewrite(fd, "\n", 1) < 0) { + STDERR("unable to write to file: %s %d", outfile, errno); + goto cleanup; + } + + ret = 0; +cleanup: + VIR_FORCE_CLOSE(fd); + VIR_FREE(buf); + if (exitstatus) + *exitstatus = ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; + return ret; +} diff --git a/tests/virnetdevbandwidthtest.c b/tests/virnetdevbandwidthtest.c index 989018e..09cc2ec 100644 --- a/tests/virnetdevbandwidthtest.c +++ b/tests/virnetdevbandwidthtest.c @@ -103,10 +103,25 @@ cleanup: return ret; } +#define OUTFILETEMPLATE abs_builddir "/virnetdevbandwidthtest-XXXXXX" + static int mymain(void) { int ret = 0; + char *outfile; + + if (VIR_STRDUP_QUIET(outfile, OUTFILETEMPLATE) < 0) { + fprintf(stderr, "Out of memory\n"); + abort(); + } + + if (!mktemp(outfile)) { + fprintf(stderr, "Cannot create outfile"); + abort(); + } + + setenv("LIBVIRT_OUTFILE", outfile, 1); #define DO_TEST_MINIMAL(r, ...) \ do { \ @@ -149,7 +164,11 @@ mymain(void) " <inbound average='1' peak='2' floor='3'/>" " <outbound average='5' peak='6'/>" "</bandwidth>"); + + if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL) + unlink(outfile); + return ret; } -VIRT_TEST_MAIN(mymain); +VIRT_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/virnetdevbandwidthmock.so") -- 1.8.5.2
On 01/23/2014 06:44 AM, Michal Privoznik wrote:
The mocking will be used in later commits to mock all calls to the virCommandRun(). This is easier to do than cutting off the command creation and run into two separate pieces.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/Makefile.am | 9 ++++ tests/virnetdevbandwidthmock.c | 106 +++++++++++++++++++++++++++++++++++++++++ tests/virnetdevbandwidthtest.c | 21 +++++++- 3 files changed, 135 insertions(+), 1 deletion(-) create mode 100644 tests/virnetdevbandwidthmock.c
+ + if ((fd = open(outfile, O_WRONLY | O_APPEND | O_CREAT, S_IRUSR | S_IWUSR)) == -1) { + STDERR("unable to open file: %s %d", outfile, errno); + goto cleanup; + } + + if (safewrite(fd, buf, strlen(buf)) < 0 || + safewrite(fd, "\n", 1) < 0) { + STDERR("unable to write to file: %s %d", outfile, errno); + goto cleanup; + }
This could be simplified with virFileWriteStr().
+ + if (VIR_STRDUP_QUIET(outfile, OUTFILETEMPLATE) < 0) { + fprintf(stderr, "Out of memory\n"); + abort(); + } + + if (!mktemp(outfile)) {
I'd prefer mkstemp(), for safety. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 24.01.2014 23:02, Eric Blake wrote:
On 01/23/2014 06:44 AM, Michal Privoznik wrote:
The mocking will be used in later commits to mock all calls to the virCommandRun(). This is easier to do than cutting off the command creation and run into two separate pieces.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/Makefile.am | 9 ++++ tests/virnetdevbandwidthmock.c | 106 +++++++++++++++++++++++++++++++++++++++++ tests/virnetdevbandwidthtest.c | 21 +++++++- 3 files changed, 135 insertions(+), 1 deletion(-) create mode 100644 tests/virnetdevbandwidthmock.c
+ + if ((fd = open(outfile, O_WRONLY | O_APPEND | O_CREAT, S_IRUSR | S_IWUSR)) == -1) { + STDERR("unable to open file: %s %d", outfile, errno); + goto cleanup; + } + + if (safewrite(fd, buf, strlen(buf)) < 0 || + safewrite(fd, "\n", 1) < 0) { + STDERR("unable to write to file: %s %d", outfile, errno); + goto cleanup; + }
This could be simplified with virFileWriteStr().
Not really. I need to append to the file, while virFileWriteStr() truncate the file prior to writing to it. Always.
+ + if (VIR_STRDUP_QUIET(outfile, OUTFILETEMPLATE) < 0) { + fprintf(stderr, "Out of memory\n"); + abort(); + } + + if (!mktemp(outfile)) {
I'd prefer mkstemp(), for safety.
Uh.. okay. Michal
On 01/27/2014 04:17 AM, Michal Privoznik wrote:
This could be simplified with virFileWriteStr().
Not really. I need to append to the file, while virFileWriteStr() truncate the file prior to writing to it. Always.
That sounds like something that could be fixed, though, if it is a common enough pattern to want to append rather than truncate.
+ + if (VIR_STRDUP_QUIET(outfile, OUTFILETEMPLATE) < 0) { + fprintf(stderr, "Out of memory\n"); + abort(); + } + + if (!mktemp(outfile)) {
I'd prefer mkstemp(), for safety.
Uh.. okay.
Michal
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
The test tries to set some QoS limits and check if the commands that are actually executed are the expected ones. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virnetdevbandwidthtest.c | 70 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/tests/virnetdevbandwidthtest.c b/tests/virnetdevbandwidthtest.c index 09cc2ec..b30e0fc 100644 --- a/tests/virnetdevbandwidthtest.c +++ b/tests/virnetdevbandwidthtest.c @@ -32,6 +32,14 @@ struct testMinimalStruct { const char *band2; }; +struct testSetStruct { + const char *outfile; + const char *band; + const char *exp_cmd; + const char *iface; + const bool hierarchical_class; +}; + #define PARSE(xml, var) \ do { \ xmlDocPtr doc; \ @@ -103,6 +111,42 @@ cleanup: return ret; } +static int +testVirNetDevBandwidthSet(const void *data) +{ + int ret = -1; + const struct testSetStruct *info = data; + const char *iface = info->iface; + virNetDevBandwidthPtr band = NULL; + char *actual_cmd = NULL; + + PARSE(info->band, band); + + if (!iface) + iface = "eth0"; + + /* Clear the temporary file between runs */ + truncate(info->outfile, 0); + + if (virNetDevBandwidthSet(iface, band, info->hierarchical_class) < 0) + goto cleanup; + + if (virFileReadAll(info->outfile, 2048, &actual_cmd) < 0) + goto cleanup; + + if (STRNEQ(info->exp_cmd, actual_cmd)) { + virtTestDifference(stderr, info->exp_cmd, actual_cmd); + goto cleanup; + } + + ret = 0; +cleanup: + virNetDevBandwidthFree(band); + VIR_FREE(actual_cmd); + return ret; +} + + #define OUTFILETEMPLATE abs_builddir "/virnetdevbandwidthtest-XXXXXX" static int @@ -132,6 +176,18 @@ mymain(void) ret = -1; \ } while (0) +#define DO_TEST_SET(Band, Exp_cmd, ...) \ + do { \ + struct testSetStruct data = {.outfile = outfile, \ + .band = Band, \ + .exp_cmd = Exp_cmd, \ + __VA_ARGS__}; \ + if (virtTestRun("virNetDevBandwidthSet", \ + testVirNetDevBandwidthSet, \ + &data) < 0) \ + ret = -1; \ + } while (0) + DO_TEST_MINIMAL(NULL, NULL, NULL); @@ -165,6 +221,20 @@ mymain(void) " <outbound average='5' peak='6'/>" "</bandwidth>"); + DO_TEST_SET("<bandwidth>" + " <inbound average='1' peak='2' floor='3' burst='4'/>" + " <outbound average='5' peak='6' burst='7'/>" + "</bandwidth>", + "/sbin/tc qdisc del dev eth0 root\n" + "/sbin/tc qdisc del dev eth0 ingress\n" + "/sbin/tc qdisc add dev eth0 root handle 1: htb default 1\n" + "/sbin/tc class add dev eth0 parent 1: classid 1:1 htb rate 1kbps ceil 2kbps burst 4kb\n" + "/sbin/tc qdisc add dev eth0 parent 1:1 handle 2: sfq perturb 10\n" + "/sbin/tc filter add dev eth0 parent 1:0 protocol ip handle 1 fw flowid 1\n" + "/sbin/tc qdisc add dev eth0 ingress\n" + "/sbin/tc filter add dev eth0 parent ffff: protocol ip u32 match ip src 0.0.0.0/0 " + "police rate 5kbps burst 7kb mtu 64kb drop flowid :1\n"); + if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL) unlink(outfile); -- 1.8.5.2
On 01/23/2014 06:44 AM, Michal Privoznik wrote:
The test tries to set some QoS limits and check if the commands that are actually executed are the expected ones.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virnetdevbandwidthtest.c | 70 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+)
Hmm - the idea of making virCommand have a dry-run mode might be useful in other testsuites. Rather than mock things up with LD_PRELOAD, would it be worth adding an internal entry point to vircommand.c where we can pass in a file name from the testsuite code; if that file name is set, we dump the command name to the file instead of executing the command. While LD_PRELOAD hacks are nice, it would be even nicer to have the reusable testing framework not depend on a Linux-only solution. That is more against 3/4, although if you do go with that approach, this patch might be impacted on calling into the internal hook to register the dry-run filename to write into.
+ DO_TEST_SET("<bandwidth>" + " <inbound average='1' peak='2' floor='3' burst='4'/>" + " <outbound average='5' peak='6' burst='7'/>" + "</bandwidth>", + "/sbin/tc qdisc del dev eth0 root\n" + "/sbin/tc qdisc del dev eth0 ingress\n" + "/sbin/tc qdisc add dev eth0 root handle 1: htb default 1\n"
I'm finding it a bit hard to visually see the argument separation. I would have written it: DO_TEST_SET(("<bandwidth" " <inbound .../>" " <outbound .../>" "</bandwidth"), ("/sbin/tc qdisc del dev eth0 root\n" "/sbin/tc ..."...) to draw a bit more of a visual clue where the string concats are divided into separate arguments. But that's cosmetic. ACK to this patch, once we figure out whether patch 3 is the ideal solution to the virCommand dry-run problem. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 24.01.2014 23:12, Eric Blake wrote:
On 01/23/2014 06:44 AM, Michal Privoznik wrote:
The test tries to set some QoS limits and check if the commands that are actually executed are the expected ones.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virnetdevbandwidthtest.c | 70 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+)
Hmm - the idea of making virCommand have a dry-run mode might be useful in other testsuites. Rather than mock things up with LD_PRELOAD, would it be worth adding an internal entry point to vircommand.c where we can pass in a file name from the testsuite code; if that file name is set, we dump the command name to the file instead of executing the command. While LD_PRELOAD hacks are nice, it would be even nicer to have the reusable testing framework not depend on a Linux-only solution. That is more against 3/4, although if you do go with that approach, this patch might be impacted on calling into the internal hook to register the dry-run filename to write into.
Makes sense. However, it will require to have a global variable to indicate that any virCommand is not to be executed but rather dry-run. We don't want to modify virCommandNew* I assume - hence the global variable. So we need two internal APIs then: virCommandSetDryRun(const char *file); virCommandCancelDryRun(); The first one just sets the global variable, while the other one un-sets it. Perhaps the latter can be merged into the former: virCommandSetDryRun(NULL); but that's just cosmetics. Let me respin the 3/4 and 4/4. Meanwhile, I'm pushing the first two patches with all the small nits you've pointed out fixed. Thanks so far! Michal
participants (3)
-
Eric Blake -
Laine Stump -
Michal Privoznik