[PATCHv2 0/6] virDomain: fix problems when setting qos on ovs managed
--- diff to v1: - Modify commit message to no longer then 80 characters. - Split patch into several commits - Add notes on some function and defination - Extract common code block to a single function Thanks to Pavel Hrdina for helping to review these patches. Two problems are found and fixed below: 1. Ingress rules is not clean on previous version of virNetDevOpenvswitchInterfaceClearQos. 2. If errors occurs when removing inbound qos on multi interfaces vm, some rules may not be delete as aspected. Fix by: 1.Instead of cleaning all qos rules each time new qos is set, tx and rx's qos are set or cleaned respectively. 2.Replace virReportError with VIR_WARN to let the cleaning process continue when error occurs. 3.Add ifname into ovs querying statements, which will reduce failure of removing qos on the other interfaces of the same vm. Test virNetDevOpenvswitchInterfaceSetQos and virNetDevOpenvswitchInterfaceClearQos with dryrun method. Since commands in tests are not actually run, it is difficult to emulate some complex senario, such as set and then update qos. So basic tests are added in patches. Jinsheng Zhang (6): virnetdevovs: Add vmuuid notes on virNetDevOpenvswitchInterfaceSetQos virnetdevovs: Extract conversion parameters between virNetDevBandwidth and ovs virnetdevovs: Extract common code block to a single function virnetdevovs: Introduce virNetDevOpenvswitchInterfaceClearTxQos and virNetDevOpenvswitchInterfaceClearRxQos virnetdevovs: Fix qos cleaning residual on multi interfaces tests: add test on virNetDevOpenvswitchInterfaceSetQos and virNetDevOpenvswitchInterfaceClearQos src/libvirt_private.syms | 2 + src/util/virnetdevopenvswitch.c | 142 ++++++++++++++-------- src/util/virnetdevopenvswitch.h | 14 +++ tests/virnetdevopenvswitchtest.c | 196 ++++++++++++++++++++++++++++++- 4 files changed, 302 insertions(+), 52 deletions(-) -- 2.30.2.windows.1
From: Jinsheng Zhang <zhangjl02@inspur.com> Add vmuuid notes on virNetDevOpenvswitchInterfaceSetQos, and change vmid to vmuuid. Signed-off-by: Jinsheng Zhang <zhangjl02@inspur.com> --- src/util/virnetdevopenvswitch.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c index 7a64a8dbe6..d86ad0eafd 100644 --- a/src/util/virnetdevopenvswitch.c +++ b/src/util/virnetdevopenvswitch.c @@ -624,6 +624,7 @@ int virNetDevOpenvswitchUpdateVlan(const char *ifname, * virNetDevOpenvswitchInterfaceSetQos: * @ifname: on which interface * @bandwidth: rates to set (may be NULL) + * @vmuuid: the Domain UUID that has this interface * @swapped: true if IN/OUT should be set contrariwise * * Update qos configuration of an OVS port. @@ -640,7 +641,7 @@ int virNetDevOpenvswitchUpdateVlan(const char *ifname, int virNetDevOpenvswitchInterfaceSetQos(const char *ifname, const virNetDevBandwidth *bandwidth, - const unsigned char *vmid, + const unsigned char *vmuuid, bool swapped) { virNetDevBandwidthRate *rx = NULL; /* From domain POV */ @@ -674,7 +675,7 @@ virNetDevOpenvswitchInterfaceSetQos(const char *ifname, } if (!bandwidth->out && !bandwidth->in) { - if (virNetDevOpenvswitchInterfaceClearQos(ifname, vmid) < 0) { + if (virNetDevOpenvswitchInterfaceClearQos(ifname, vmuuid) < 0) { VIR_WARN("Clean qos for interface %s failed", ifname); } return 0; @@ -699,7 +700,7 @@ virNetDevOpenvswitchInterfaceSetQos(const char *ifname, /* find queue */ cmd = virNetDevOpenvswitchCreateCmd(); - virUUIDFormat(vmid, vmuuidstr); + virUUIDFormat(vmuuid, vmuuidstr); vmid_ex_id = g_strdup_printf("external-ids:vm-id=\"%s\"", vmuuidstr); ifname_ex_id = g_strdup_printf("external-ids:ifname=\"%s\"", ifname); virCommandAddArgList(cmd, "--no-heading", "--columns=_uuid", "find", "queue", @@ -801,7 +802,7 @@ virNetDevOpenvswitchInterfaceSetQos(const char *ifname, int virNetDevOpenvswitchInterfaceClearQos(const char *ifname, - const unsigned char *vmid) + const unsigned char *vmuuid) { char vmuuidstr[VIR_UUID_STRING_BUFLEN]; g_autoptr(virCommand) cmd = NULL; @@ -813,7 +814,7 @@ virNetDevOpenvswitchInterfaceClearQos(const char *ifname, /* find qos */ cmd = virNetDevOpenvswitchCreateCmd(); - virUUIDFormat(vmid, vmuuidstr); + virUUIDFormat(vmuuid, vmuuidstr); vmid_ex_id = g_strdup_printf("external-ids:vm-id=\"%s\"", vmuuidstr); virCommandAddArgList(cmd, "--no-heading", "--columns=_uuid", "find", "qos", vmid_ex_id, NULL); virCommandSetOutputBuffer(cmd, &qos_uuid); -- 2.30.2.windows.1
On 8/17/21 6:38 AM, jx8zjs wrote:
From: Jinsheng Zhang <zhangjl02@inspur.com>
Add vmuuid notes on virNetDevOpenvswitchInterfaceSetQos, and change vmid to vmuuid.
Signed-off-by: Jinsheng Zhang <zhangjl02@inspur.com> --- src/util/virnetdevopenvswitch.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
I think src/util/virnetdevopenvswitch.h could use the same treatment. Michal
From: Jinsheng Zhang <zhangjl02@inspur.com> Signed-off-by: zhangjl02 <zhangjl02@inspur.com> --- src/util/virnetdevopenvswitch.c | 12 +++++++----- src/util/virnetdevopenvswitch.h | 7 +++++++ 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c index d86ad0eafd..dfbebf8535 100644 --- a/src/util/virnetdevopenvswitch.c +++ b/src/util/virnetdevopenvswitch.c @@ -692,11 +692,11 @@ virNetDevOpenvswitchInterfaceSetQos(const char *ifname, g_autofree char *qos_uuid = NULL; g_autofree char *queue_uuid = NULL; - average = g_strdup_printf("%llu", tx->average * 8192); + average = g_strdup_printf("%llu", tx->average * VIR_NETDEV_TX_TO_OVS); if (tx->burst) - burst = g_strdup_printf("%llu", tx->burst * 8192); + burst = g_strdup_printf("%llu", tx->burst * VIR_NETDEV_TX_TO_OVS); if (tx->peak) - peak = g_strdup_printf("%llu", tx->peak * 8192); + peak = g_strdup_printf("%llu", tx->peak * VIR_NETDEV_TX_TO_OVS); /* find queue */ cmd = virNetDevOpenvswitchCreateCmd(); @@ -786,9 +786,11 @@ virNetDevOpenvswitchInterfaceSetQos(const char *ifname, cmd = virNetDevOpenvswitchCreateCmd(); virCommandAddArgList(cmd, "set", "Interface", ifname, NULL); - virCommandAddArgFormat(cmd, "ingress_policing_rate=%llu", rx->average * 8); + virCommandAddArgFormat(cmd, "ingress_policing_rate=%llu", + rx->average * VIR_NETDEV_RX_TO_OVS); if (rx->burst) - virCommandAddArgFormat(cmd, "ingress_policing_burst=%llu", rx->burst * 8); + virCommandAddArgFormat(cmd, "ingress_policing_burst=%llu", + rx->burst * VIR_NETDEV_RX_TO_OVS); if (virCommandRun(cmd, NULL) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/util/virnetdevopenvswitch.h b/src/util/virnetdevopenvswitch.h index 2dcd1aec6b..33e52f57c7 100644 --- a/src/util/virnetdevopenvswitch.h +++ b/src/util/virnetdevopenvswitch.h @@ -26,6 +26,13 @@ #include "virnetdevvlan.h" #define VIR_NETDEV_OVS_DEFAULT_TIMEOUT 5 +/* + * Average, peak, floor and burst in virNetDevBandwidth are in kbytes. + * However other_config in ovs qos is in bit. + * ingress_policing_rate in ovs interface is in kbit. + */ +#define VIR_NETDEV_TX_TO_OVS 8192 +#define VIR_NETDEV_RX_TO_OVS 8 void virNetDevOpenvswitchSetTimeout(unsigned int timeout); -- 2.30.2.windows.1
On 8/17/21 6:38 AM, jx8zjs wrote:
From: Jinsheng Zhang <zhangjl02@inspur.com>
Signed-off-by: zhangjl02 <zhangjl02@inspur.com> --- src/util/virnetdevopenvswitch.c | 12 +++++++----- src/util/virnetdevopenvswitch.h | 7 +++++++ 2 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/src/util/virnetdevopenvswitch.h b/src/util/virnetdevopenvswitch.h index 2dcd1aec6b..33e52f57c7 100644 --- a/src/util/virnetdevopenvswitch.h +++ b/src/util/virnetdevopenvswitch.h @@ -26,6 +26,13 @@ #include "virnetdevvlan.h"
#define VIR_NETDEV_OVS_DEFAULT_TIMEOUT 5 +/* + * Average, peak, floor and burst in virNetDevBandwidth are in kbytes. + * However other_config in ovs qos is in bit. + * ingress_policing_rate in ovs interface is in kbit. + */ +#define VIR_NETDEV_TX_TO_OVS 8192 +#define VIR_NETDEV_RX_TO_OVS 8
void virNetDevOpenvswitchSetTimeout(unsigned int timeout);
These macros seem to be used only from corresponding .c file. I think they can be defined there. That way we can avoid polluting header files. Michal
From: Jinsheng Zhang <zhangjl02@inspur.com> Signed-off-by: zhangjl02 <zhangjl02@inspur.com> s Signed-off-by: zhangjl02 <zhangjl02@inspur.com> s Signed-off-by: zhangjl02 <zhangjl02@inspur.com> --- src/util/virnetdevopenvswitch.c | 60 ++++++++++++++------------------- 1 file changed, 26 insertions(+), 34 deletions(-) diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c index dfbebf8535..e8ec06d3db 100644 --- a/src/util/virnetdevopenvswitch.c +++ b/src/util/virnetdevopenvswitch.c @@ -619,6 +619,23 @@ int virNetDevOpenvswitchUpdateVlan(const char *ifname, return 0; } +static char* +virNetDevOpenvswitchFindUUID(const char *table, + const char *vmid_ex_id, + const char *ifname_ex_id) +{ + g_autoptr(virCommand) cmd = NULL; + char *uuid = NULL; + + cmd = virNetDevOpenvswitchCreateCmd(); + virCommandAddArgList(cmd, "--no-heading", "--columns=_uuid", "find", table, + vmid_ex_id, ifname_ex_id, NULL); + virCommandSetOutputBuffer(cmd, &uuid); + if (virCommandRun(cmd, NULL) < 0) { + VIR_WARN("Unable to find queue on port with %s", ifname_ex_id); + } + return uuid; +} /** * virNetDevOpenvswitchInterfaceSetQos: @@ -698,30 +715,15 @@ virNetDevOpenvswitchInterfaceSetQos(const char *ifname, if (tx->peak) peak = g_strdup_printf("%llu", tx->peak * VIR_NETDEV_TX_TO_OVS); - /* find queue */ - cmd = virNetDevOpenvswitchCreateCmd(); virUUIDFormat(vmuuid, vmuuidstr); vmid_ex_id = g_strdup_printf("external-ids:vm-id=\"%s\"", vmuuidstr); ifname_ex_id = g_strdup_printf("external-ids:ifname=\"%s\"", ifname); - virCommandAddArgList(cmd, "--no-heading", "--columns=_uuid", "find", "queue", - vmid_ex_id, ifname_ex_id, NULL); - virCommandSetOutputBuffer(cmd, &queue_uuid); - if (virCommandRun(cmd, NULL) < 0) { - VIR_WARN("Unable to find queue on port %s", ifname); - } - + /* find queue */ + queue_uuid = virNetDevOpenvswitchFindUUID("queue", vmid_ex_id, ifname_ex_id); /* find qos */ - virCommandFree(cmd); - cmd = virNetDevOpenvswitchCreateCmd(); - virCommandAddArgList(cmd, "--no-heading", "--columns=_uuid", "find", "qos", - vmid_ex_id, ifname_ex_id, NULL); - virCommandSetOutputBuffer(cmd, &qos_uuid); - if (virCommandRun(cmd, NULL) < 0) { - VIR_WARN("Unable to find qos on port %s", ifname); - } + qos_uuid = virNetDevOpenvswitchFindUUID("qos", vmid_ex_id, ifname_ex_id); /* create qos and set */ - virCommandFree(cmd); cmd = virNetDevOpenvswitchCreateCmd(); if (queue_uuid && *queue_uuid) { g_auto(GStrv) lines = g_strsplit(queue_uuid, "\n", 0); @@ -808,6 +810,7 @@ virNetDevOpenvswitchInterfaceClearQos(const char *ifname, { char vmuuidstr[VIR_UUID_STRING_BUFLEN]; g_autoptr(virCommand) cmd = NULL; + g_autofree char *ifname_ex_id = NULL; g_autofree char *vmid_ex_id = NULL; g_autofree char *qos_uuid = NULL; g_autofree char *queue_uuid = NULL; @@ -815,24 +818,13 @@ virNetDevOpenvswitchInterfaceClearQos(const char *ifname, size_t i; /* find qos */ - cmd = virNetDevOpenvswitchCreateCmd(); virUUIDFormat(vmuuid, vmuuidstr); vmid_ex_id = g_strdup_printf("external-ids:vm-id=\"%s\"", vmuuidstr); - virCommandAddArgList(cmd, "--no-heading", "--columns=_uuid", "find", "qos", vmid_ex_id, NULL); - virCommandSetOutputBuffer(cmd, &qos_uuid); - if (virCommandRun(cmd, NULL) < 0) { - VIR_WARN("Unable to find qos on port %s", ifname); - } - + ifname_ex_id = g_strdup_printf("external-ids:ifname=\"%s\"", ifname); /* find queue */ - virCommandFree(cmd); - cmd = virNetDevOpenvswitchCreateCmd(); - vmid_ex_id = g_strdup_printf("external-ids:vm-id=\"%s\"", vmuuidstr); - virCommandAddArgList(cmd, "--no-heading", "--columns=_uuid", "find", "queue", vmid_ex_id, NULL); - virCommandSetOutputBuffer(cmd, &queue_uuid); - if (virCommandRun(cmd, NULL) < 0) { - VIR_WARN("Unable to find queue on port %s", ifname); - } + queue_uuid = virNetDevOpenvswitchFindUUID("queue", vmid_ex_id, ifname_ex_id); + /* find qos */ + qos_uuid = virNetDevOpenvswitchFindUUID("qos", vmid_ex_id, ifname_ex_id); if (qos_uuid && *qos_uuid) { g_auto(GStrv) lines = g_strsplit(qos_uuid, "\n", 0); @@ -846,7 +838,7 @@ virNetDevOpenvswitchInterfaceClearQos(const char *ifname, virCommandFree(cmd); cmd = virNetDevOpenvswitchCreateCmd(); virCommandAddArgList(cmd, "--no-heading", "--columns=_uuid", "--if-exists", - "list", "port", ifname, "qos", NULL); + "list", "port", ifname, "qos", NULL); virCommandSetOutputBuffer(cmd, &port_qos); if (virCommandRun(cmd, NULL) < 0) { VIR_WARN("Unable to remove port qos on port %s", ifname); -- 2.30.2.windows.1
On 8/17/21 6:38 AM, jx8zjs wrote:
From: Jinsheng Zhang <zhangjl02@inspur.com>
Signed-off-by: zhangjl02 <zhangjl02@inspur.com>
s
Signed-off-by: zhangjl02 <zhangjl02@inspur.com>
s
Signed-off-by: zhangjl02 <zhangjl02@inspur.com>
A bit weird commit message :-) Michal
From: Jinsheng Zhang <zhangjl02@inspur.com> Separate virNetDevOpenvswitchInterfaceClearQos into two steps. When setting qos, we can set only rx or tx and the other one should be cleared. Signed-off-by: zhangjl02 <zhangjl02@inspur.com> --- src/libvirt_private.syms | 2 ++ src/util/virnetdevopenvswitch.c | 50 +++++++++++++++++++++++++++++++-- src/util/virnetdevopenvswitch.h | 7 +++++ 3 files changed, 57 insertions(+), 2 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 51a400ba59..841cd08435 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2816,6 +2816,8 @@ virNetDevOpenvswitchAddPort; virNetDevOpenvswitchGetMigrateData; virNetDevOpenvswitchGetVhostuserIfname; virNetDevOpenvswitchInterfaceClearQos; +virNetDevOpenvswitchInterfaceClearRxQos; +virNetDevOpenvswitchInterfaceClearTxQos; virNetDevOpenvswitchInterfaceGetMaster; virNetDevOpenvswitchInterfaceParseStats; virNetDevOpenvswitchInterfaceSetQos; diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c index e8ec06d3db..7c13e1764f 100644 --- a/src/util/virnetdevopenvswitch.c +++ b/src/util/virnetdevopenvswitch.c @@ -781,6 +781,10 @@ virNetDevOpenvswitchInterfaceSetQos(const char *ifname, return -1; } } + } else { + if (virNetDevOpenvswitchInterfaceClearTxQos(ifname, vmuuid) < 0) { + VIR_WARN("Clean tx qos for interface %s failed", ifname); + } } if (rx) { @@ -799,14 +803,18 @@ virNetDevOpenvswitchInterfaceSetQos(const char *ifname, _("Unable to set vlan configuration on port %s"), ifname); return -1; } + } else { + if (virNetDevOpenvswitchInterfaceClearRxQos(ifname) < 0) { + VIR_WARN("Clean rx qos for interface %s failed", ifname); + } } return 0; } int -virNetDevOpenvswitchInterfaceClearQos(const char *ifname, - const unsigned char *vmuuid) +virNetDevOpenvswitchInterfaceClearTxQos(const char *ifname, + const unsigned char *vmuuid) { char vmuuidstr[VIR_UUID_STRING_BUFLEN]; g_autoptr(virCommand) cmd = NULL; @@ -883,3 +891,41 @@ virNetDevOpenvswitchInterfaceClearQos(const char *ifname, return 0; } + +int +virNetDevOpenvswitchInterfaceClearRxQos(const char *ifname) +{ + g_autoptr(virCommand) cmd = NULL; + + cmd = virNetDevOpenvswitchCreateCmd(); + virCommandAddArgList(cmd, "set", "Interface", ifname, NULL); + virCommandAddArgFormat(cmd, "ingress_policing_rate=%llu", 0llu); + virCommandAddArgFormat(cmd, "ingress_policing_burst=%llu", 0llu); + + if (virCommandRun(cmd, NULL) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to set vlan configuration on port %s"), ifname); + return -1; + } + + return 0; +} + +int +virNetDevOpenvswitchInterfaceClearQos(const char *ifname, + const unsigned char *vmuuid) +{ + int ret = 0; + + if (virNetDevOpenvswitchInterfaceClearTxQos(ifname, vmuuid) < 0) { + VIR_WARN("Clean tx qos for interface %s failed", ifname); + ret = -1; + } + + if (virNetDevOpenvswitchInterfaceClearRxQos(ifname) < 0) { + VIR_WARN("Clean rx qos for interface %s failed", ifname); + ret = -1; + } + + return ret; +} diff --git a/src/util/virnetdevopenvswitch.h b/src/util/virnetdevopenvswitch.h index 33e52f57c7..cea5f4616d 100644 --- a/src/util/virnetdevopenvswitch.h +++ b/src/util/virnetdevopenvswitch.h @@ -87,3 +87,10 @@ int virNetDevOpenvswitchInterfaceSetQos(const char *ifname, int virNetDevOpenvswitchInterfaceClearQos(const char *ifname, const unsigned char *vmid) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT; + +int virNetDevOpenvswitchInterfaceClearRxQos(const char *ifname) +ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT; + +int virNetDevOpenvswitchInterfaceClearTxQos(const char *ifname, + const unsigned char *vmid) +ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT; -- 2.30.2.windows.1
From: Jinsheng Zhang <zhangjl02@inspur.com> Warn these error instead of return when removing qos or queues. This will avoid residual qos clearance on multiple interfaces. Signed-off-by: zhangjl02 <zhangjl02@inspur.com> --- src/util/virnetdevopenvswitch.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c index 7c13e1764f..e014a158f1 100644 --- a/src/util/virnetdevopenvswitch.c +++ b/src/util/virnetdevopenvswitch.c @@ -816,6 +816,7 @@ int virNetDevOpenvswitchInterfaceClearTxQos(const char *ifname, const unsigned char *vmuuid) { + int ret = 0; char vmuuidstr[VIR_UUID_STRING_BUFLEN]; g_autoptr(virCommand) cmd = NULL; g_autofree char *ifname_ex_id = NULL; @@ -863,9 +864,8 @@ virNetDevOpenvswitchInterfaceClearTxQos(const char *ifname, cmd = virNetDevOpenvswitchCreateCmd(); virCommandAddArgList(cmd, "destroy", "qos", line, NULL); if (virCommandRun(cmd, NULL) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to destroy qos on port %s"), ifname); - return -1; + VIR_WARN("Unable to destroy qos on port %s", ifname); + ret = -1; } } } @@ -882,14 +882,13 @@ virNetDevOpenvswitchInterfaceClearTxQos(const char *ifname, cmd = virNetDevOpenvswitchCreateCmd(); virCommandAddArgList(cmd, "destroy", "queue", line, NULL); if (virCommandRun(cmd, NULL) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to destroy queue on port %s"), ifname); - return -1; + VIR_WARN("Unable to destroy queue on port %s", ifname); + ret = -1; } } } - return 0; + return ret; } int @@ -904,7 +903,7 @@ virNetDevOpenvswitchInterfaceClearRxQos(const char *ifname) if (virCommandRun(cmd, NULL) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to set vlan configuration on port %s"), ifname); + _("Unable to reset ingress on port %s"), ifname); return -1; } -- 2.30.2.windows.1
From: Jinsheng Zhang <zhangjl02@inspur.com> Test virNetDevOpenvswitchInterfaceSetQos and virNetDevOpenvswitchInterfaceClearQos with dryrun method. Signed-off-by: zhangjl02 <zhangjl02@inspur.com> --- tests/virnetdevopenvswitchtest.c | 196 ++++++++++++++++++++++++++++++- 1 file changed, 195 insertions(+), 1 deletion(-) diff --git a/tests/virnetdevopenvswitchtest.c b/tests/virnetdevopenvswitchtest.c index 46172dae90..f7d16b4f67 100644 --- a/tests/virnetdevopenvswitchtest.c +++ b/tests/virnetdevopenvswitchtest.c @@ -19,7 +19,11 @@ #include <config.h> #include "testutils.h" +#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 @@ -29,6 +33,43 @@ struct _InterfaceParseStatsData { const virDomainInterfaceStatsStruct stats; }; +struct testSetQosStruct { + const char *band; + const char *exp_cmd; + const char *iface; +}; + +struct testClearQosStruct { + const char *exp_cmd; + const char *iface; + const unsigned char *vmid; +}; + +#define PARSE(xml, var) \ + do { \ + int rc; \ + xmlDocPtr doc; \ + xmlXPathContextPtr ctxt = NULL; \ + \ + if (!xml) \ + break; \ + \ + if (!(doc = virXMLParseStringCtxt((xml), \ + "bandwidth definition", \ + &ctxt))) \ + goto cleanup; \ + \ + rc = virNetDevBandwidthParse(&(var), \ + NULL, \ + ctxt->node, \ + true); \ + xmlFreeDoc(doc); \ + xmlXPathFreeContext(ctxt); \ + if (rc < 0) \ + goto cleanup; \ + } while (0) + +static const unsigned char vm_id[VIR_UUID_BUFLEN] = "fakeuuid"; static int testInterfaceParseStats(const void *opaque) @@ -111,6 +152,80 @@ testNameEscape(const void *opaque) } +static int +testVirNetDevOpenvswitchInterfaceSetQos(const void *data) +{ + int ret = -1; + const struct testSetQosStruct *info = data; + const char *iface = info->iface; + g_autoptr(virNetDevBandwidth) band = NULL; + g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; + char *actual_cmd = NULL; + g_autoptr(virCommandDryRunToken) dryRunToken = virCommandDryRunTokenNew(); + + PARSE(info->band, band); + + if (!iface) + iface = "tap-fake"; + + virCommandSetDryRun(dryRunToken, &buf, false, false, NULL, NULL); + + if (virNetDevOpenvswitchInterfaceSetQos(iface, band, vm_id, true) < 0) + goto cleanup; + + 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)) { + virTestDifference(stderr, + NULLSTR(info->exp_cmd), + NULLSTR(actual_cmd)); + goto cleanup; + } + + ret = 0; + cleanup: + VIR_FREE(actual_cmd); + return ret; +} + + +static int +testVirNetDevOpenvswitchInterfaceClearQos(const void *data) +{ + int ret = -1; + const struct testClearQosStruct *info = data; + g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; + char *actual_cmd = NULL; + const char *iface = info->iface; + const unsigned char *vmid = info->vmid; + g_autoptr(virCommandDryRunToken) dryRunToken = virCommandDryRunTokenNew(); + + virCommandSetDryRun(dryRunToken, &buf, false, false, NULL, NULL); + + if (virNetDevOpenvswitchInterfaceClearQos(iface, vmid) < 0) + goto cleanup; + + 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)) { + virTestDifference(stderr, + NULLSTR(info->exp_cmd), + NULLSTR(actual_cmd)); + goto cleanup; + } + + ret = 0; + cleanup: + VIR_FREE(actual_cmd); + return ret; +} + static int mymain(void) { @@ -146,7 +261,86 @@ mymain(void) TEST_NAME_ESCAPE("\"vhost\"user1\"", NULL); TEST_NAME_ESCAPE("\"\\\\", NULL); +#define DO_TEST_SET(Band, Exp_cmd, ...) \ + do { \ + struct testSetQosStruct data = {.band = Band, \ + .exp_cmd = Exp_cmd, \ + __VA_ARGS__}; \ + if (virTestRun("virNetDevOpenvswitchInterfaceSetQos", \ + testVirNetDevOpenvswitchInterfaceSetQos, \ + &data) < 0) \ + ret = -1; \ + } while (0) + + DO_TEST_SET(("<bandwidth>" + " <inbound average='20000'/>" + "</bandwidth>"), + (OVS_VSCTL " --timeout=5 --no-heading --columns=_uuid find queue" + " 'external-ids:vm-id=\"66616b65-7575-6964-0000-000000000000\"'" + " 'external-ids:ifname=\"tap-fake\"'\n" + OVS_VSCTL " --timeout=5 --no-heading --columns=_uuid find qos" + " 'external-ids:vm-id=\"66616b65-7575-6964-0000-000000000000\"'" + " 'external-ids:ifname=\"tap-fake\"'\n" + OVS_VSCTL " --timeout=5 set port tap-fake qos=@qos1" + " 'external-ids:vm-id=\"66616b65-7575-6964-0000-000000000000\"'" + " 'external-ids:ifname=\"tap-fake\"'" + " -- --id=@qos1 create qos type=linux-htb other_config:min-rate=163840000" + " queues:0=@queue0 'external-ids:vm-id=\"66616b65-7575-6964-0000-000000000000\"'" + " 'external-ids:ifname=\"tap-fake\"'" + " -- --id=@queue0 create queue other_config:min-rate=163840000 " + "'external-ids:vm-id=\"66616b65-7575-6964-0000-000000000000\"'" + " 'external-ids:ifname=\"tap-fake\"'\n" + OVS_VSCTL " --timeout=5 set Interface tap-fake ingress_policing_rate=0 ingress_policing_burst=0\n")); + + DO_TEST_SET(NULL, NULL); + + DO_TEST_SET("<bandwidth/>", NULL); + + DO_TEST_SET(("<bandwidth>" + " <inbound average='0' />" + "</bandwidth>"), + (OVS_VSCTL " --timeout=5 --no-heading --columns=_uuid find queue" + " 'external-ids:vm-id=\"66616b65-7575-6964-0000-000000000000\"'" + " 'external-ids:ifname=\"tap-fake\"'\n" + OVS_VSCTL " --timeout=5 --no-heading --columns=_uuid find qos" + " 'external-ids:vm-id=\"66616b65-7575-6964-0000-000000000000\"'" + " 'external-ids:ifname=\"tap-fake\"'\n" + OVS_VSCTL " --timeout=5 set Interface tap-fake ingress_policing_rate=0 ingress_policing_burst=0\n")); + + DO_TEST_SET(("<bandwidth>" + " <inbound average='0' />" + " <outbound average='5000' />" + "</bandwidth>"), + (OVS_VSCTL " --timeout=5 --no-heading --columns=_uuid find queue" + " 'external-ids:vm-id=\"66616b65-7575-6964-0000-000000000000\"'" + " 'external-ids:ifname=\"tap-fake\"'\n" + OVS_VSCTL " --timeout=5 --no-heading --columns=_uuid find qos" + " 'external-ids:vm-id=\"66616b65-7575-6964-0000-000000000000\"'" + " 'external-ids:ifname=\"tap-fake\"'\n" + OVS_VSCTL " --timeout=5 set Interface tap-fake ingress_policing_rate=40000\n")); + +#define DO_TEST_CLEAR_QOS(Iface, Vmid, Exp_cmd, ...) \ + do { \ + struct testClearQosStruct data = {.iface = Iface, \ + .vmid = Vmid, \ + .exp_cmd = Exp_cmd, \ + __VA_ARGS__}; \ + if (virTestRun("virNetDevOpenvswitchInterfaceClearQos", \ + testVirNetDevOpenvswitchInterfaceClearQos, \ + &data) < 0) \ + ret = -1; \ + } while (0) + + DO_TEST_CLEAR_QOS(("fake-iface"), vm_id, + (OVS_VSCTL " --timeout=5 --no-heading --columns=_uuid find queue" + " 'external-ids:vm-id=\"66616b65-7575-6964-0000-000000000000\"'" + " 'external-ids:ifname=\"fake-iface\"'\n" + OVS_VSCTL " --timeout=5 --no-heading --columns=_uuid find qos" + " 'external-ids:vm-id=\"66616b65-7575-6964-0000-000000000000\"'" + " 'external-ids:ifname=\"fake-iface\"'\n" + OVS_VSCTL " --timeout=5 set Interface fake-iface ingress_policing_rate=0 ingress_policing_burst=0\n")); + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -VIR_TEST_MAIN(mymain); +VIR_TEST_MAIN_PRELOAD(mymain, VIR_TEST_MOCK("virnetdevbandwidth")) -- 2.30.2.windows.1
On 8/17/21 6:38 AM, jx8zjs wrote:
From: Jinsheng Zhang <zhangjl02@inspur.com>
Test virNetDevOpenvswitchInterfaceSetQos and virNetDevOpenvswitchInterfaceClearQos with dryrun method.
Signed-off-by: zhangjl02 <zhangjl02@inspur.com> --- tests/virnetdevopenvswitchtest.c | 196 ++++++++++++++++++++++++++++++- 1 file changed, 195 insertions(+), 1 deletion(-)
-VIR_TEST_MAIN(mymain); +VIR_TEST_MAIN_PRELOAD(mymain, VIR_TEST_MOCK("virnetdevbandwidth"))
Nit pick - here we like to put VIR_TEST_MOCK() on a separate line. And huge thank you for introducing the test. It was on my todo list. Michal
On 8/17/21 6:38 AM, jx8zjs wrote:
---
diff to v1: - Modify commit message to no longer then 80 characters. - Split patch into several commits - Add notes on some function and defination - Extract common code block to a single function
Thanks to Pavel Hrdina for helping to review these patches.
Two problems are found and fixed below: 1. Ingress rules is not clean on previous version of virNetDevOpenvswitchInterfaceClearQos. 2. If errors occurs when removing inbound qos on multi interfaces vm, some rules may not be delete as aspected.
Fix by: 1.Instead of cleaning all qos rules each time new qos is set, tx and rx's qos are set or cleaned respectively. 2.Replace virReportError with VIR_WARN to let the cleaning process continue when error occurs. 3.Add ifname into ovs querying statements, which will reduce failure of removing qos on the other interfaces of the same vm.
Test virNetDevOpenvswitchInterfaceSetQos and virNetDevOpenvswitchInterfaceClearQos with dryrun method. Since commands in tests are not actually run, it is difficult to emulate some complex senario, such as set and then update qos. So basic tests are added in patches.
Jinsheng Zhang (6): virnetdevovs: Add vmuuid notes on virNetDevOpenvswitchInterfaceSetQos virnetdevovs: Extract conversion parameters between virNetDevBandwidth and ovs virnetdevovs: Extract common code block to a single function virnetdevovs: Introduce virNetDevOpenvswitchInterfaceClearTxQos and virNetDevOpenvswitchInterfaceClearRxQos virnetdevovs: Fix qos cleaning residual on multi interfaces tests: add test on virNetDevOpenvswitchInterfaceSetQos and virNetDevOpenvswitchInterfaceClearQos
src/libvirt_private.syms | 2 + src/util/virnetdevopenvswitch.c | 142 ++++++++++++++-------- src/util/virnetdevopenvswitch.h | 14 +++ tests/virnetdevopenvswitchtest.c | 196 ++++++++++++++++++++++++++++++- 4 files changed, 302 insertions(+), 52 deletions(-)
I'm fixing all the small issues I've found and pushing. Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
jx8zjs -
Michal Prívozník