On 6/28/21 11:18 AM, zhangjl02 wrote:
> From: zhangjl02 <zhangjl02@xxxxxxxxxx>
>
> Introduce qos setting and cleaning method. Use ovs command to set qos
> parameters on specific interface of qemu virtual machine.
>
> When an ovs port is created, we add 'ifname' to external-ids. When setting
> qos on an ovs port, query its qos and queue. If found, change qos on queried
> queue and qos, otherwise create new queue and qos. When cleaning qos, query
> and clean queues and qos in ovs table record by 'ifname' and 'vmid'.
> ---
> src/libvirt_private.syms | 2 +
> src/util/virnetdevopenvswitch.c | 269 ++++++++++++++++++++++++++++++++
> src/util/virnetdevopenvswitch.h | 11 ++
> 3 files changed, 282 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 68e4b6aab8..775f6706b3 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2805,8 +2805,10 @@ virNetDevMidonetUnbindPort;
> virNetDevOpenvswitchAddPort;
> virNetDevOpenvswitchGetMigrateData;
> virNetDevOpenvswitchGetVhostuserIfname;
> +virNetDevOpenvswitchInterfaceClearQos;
> virNetDevOpenvswitchInterfaceGetMaster;
> virNetDevOpenvswitchInterfaceParseStats;
> +virNetDevOpenvswitchInterfaceSetQos;
> virNetDevOpenvswitchInterfaceStats;
> virNetDevOpenvswitchMaybeUnescapeReply;
> virNetDevOpenvswitchRemovePort;
> diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
> index eac68d9556..f27b74f35f 100644
> --- a/src/util/virnetdevopenvswitch.c
> +++ b/src/util/virnetdevopenvswitch.c
> @@ -30,6 +30,7 @@
> #include "virlog.h"
> #include "virjson.h"
> #include "virfile.h"
> +#include "virutil.h"
>
> #define VIR_FROM_THIS VIR_FROM_NONE
>
> @@ -140,6 +141,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname,
> g_autofree char *ifaceid_ex_id = NULL;
> g_autofree char *profile_ex_id = NULL;
> g_autofree char *vmid_ex_id = NULL;
> + g_autofree char *ifname_ex_id = NULL;
>
> virMacAddrFormat(macaddr, macaddrstr);
> virUUIDFormat(ovsport->interfaceID, ifuuidstr);
> @@ -149,6 +151,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname,
> macaddrstr);
> ifaceid_ex_id = g_strdup_printf("external-ids:iface-id=\"%s\"", ifuuidstr);
> vmid_ex_id = g_strdup_printf("external-ids:vm-id=\"%s\"", vmuuidstr);
> + ifname_ex_id = g_strdup_printf("external-ids:ifname=\"%s\"", ifname);
> if (ovsport->profileID[0] != '\0') {
> profile_ex_id = g_strdup_printf("external-ids:port-profile=\"%s\"",
> ovsport->profileID);
> @@ -174,6 +177,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname,
> "--", "set", "Interface", ifname, ifaceid_ex_id,
> "--", "set", "Interface", ifname, vmid_ex_id,
> "--", "set", "Interface", ifname, profile_ex_id,
> + "--", "set", "Interface", ifname, ifname_ex_id,
> "--", "set", "Interface", ifname,
> "external-ids:iface-status=active",
> NULL);
> @@ -614,3 +618,268 @@ int virNetDevOpenvswitchUpdateVlan(const char *ifname,
>
> return 0;
> }
> +
> +
> +/**
> + * virNetDevOpenvswitchInterfaceSetQos:
> + * @ifname: on which interface
> + * @bandwidth: rates to set (may be NULL)
> + * @swapped: true if IN/OUT should be set contrariwise
> + *
> + * Update qos configuration of an OVS port.
> + *
> + * If @swapped is set, the IN part of @bandwidth is set on
> + * @ifname's TX, and vice versa. If it is not set, IN is set on
> + * RX and OUT on TX. This is because for some types of interfaces
> + * domain and the host live on the same side of the interface (so
> + * domain's RX/TX is host's RX/TX), and for some it's swapped
> + * (domain's RX/TX is hosts's TX/RX).
> + *
> + * Return 0 on success, -1 otherwise.
> + */
> +int virNetDevOpenvswitchInterfaceSetQos(const char *ifname,
> + const virNetDevBandwidth *bandwidth,
> + const unsigned char *vmid,
> + bool swapped)
> +{
> + int ret = -1;
> + char vmuuidstr[VIR_UUID_STRING_BUFLEN];
> + virNetDevBandwidthRate *rx = NULL; /* From domain POV */
> + virNetDevBandwidthRate *tx = NULL; /* From domain POV */
> + virCommand *cmd = NULL;
> + char *vmid_ex_id = NULL;
> + char *ifname_ex_id = NULL;
> + char *average = NULL;
> + char *peak = NULL;
> + char *burst = NULL;
> + char *qos_uuid = NULL;
> + char *queue_uuid = NULL;
> + char **lines = NULL;
> +
> + if (!bandwidth) {
> + /* nothing to be enabled */
> + ret = 0;
> + goto cleanup;
> + }
> +
> + if (geteuid() != 0) {
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> + _("Network bandwidth tuning is not available"
> + " in session mode"));
> + return -1;
> + }
> +
> + if (!ifname) {
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> + _("Unable to set bandwidth for interface because "
> + "device name is unknown"));
> + return -1;
> + }
> +
> + if (swapped) {
> + rx = bandwidth->out;
> + tx = bandwidth->in;
> + } else {
> + rx = bandwidth->in;
> + tx = bandwidth->out;
> + }
> + if(!bandwidth->out && !bandwidth->in) {
> + ret = virNetDevOpenvswitchInterfaceClearQos(ifname, vmid);
> + // virNetDevBandwidthClear(iframe);
A leftover probably? Also, we don't really use C99 like comments. We're old school and use C89 style.
> + }
> + if (tx && tx->average) {
> + average = g_strdup_printf("%llu", tx->average * 8192);
> + if (tx->burst)
> + burst = g_strdup_printf("%llu", tx->burst * 8192);
> + if (tx->peak)
> + peak = g_strdup_printf("%llu", tx->peak * 8192);
Let me just say that finding ANY documentation for this part of OVS wasn't easy. Initially, I though that these multiplications (to get bits/s) were wrong, but after digging through OVS code - they are indeed correct. Which is not at all consistent with .. [1]
> +
> + // find queue
> + cmd = virNetDevOpenvswitchCreateCmd();
> + virUUIDFormat(vmid, 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) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to find queue on port %s"), ifname);
> + }
> +
> + // find qos
> + cmd = virNetDevOpenvswitchCreateCmd();
You need to free cmd before reusing it, like this: virCommandFree(cmd);
The same applies for other variables, that are reused within the funcion (average, burst, peak, ...).
> + virCommandAddArgList(cmd, "--no-heading", "--columns=_uuid", "find", "qos", vmid_ex_id, ifname_ex_id,NULL);
> + virCommandSetOutputBuffer(cmd, &qos_uuid);
> + if (virCommandRun(cmd, NULL) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to find qos on port %s"), ifname);
> + }
> +
> + // create qos and set
> + cmd = virNetDevOpenvswitchCreateCmd();
> + if (queue_uuid && *queue_uuid) {
> + lines = g_strsplit(queue_uuid, "\n", 0);
> + virCommandAddArgList(cmd, "set", "queue", lines[0], NULL);
> + }else{
> + virCommandAddArgList(cmd, "set", "port", ifname, "qos=@qos1", vmid_ex_id, ifname_ex_id,
> + "--", "--id=@qos1", "create", "qos", "type=linux-htb", NULL);
> + virCommandAddArgFormat(cmd, "other_config:min-rate=%s", average);
> + if(burst){
> + virCommandAddArgFormat(cmd, "other_config:burst=%s", burst);
> + }
> + if(peak){
> + virCommandAddArgFormat(cmd, "other_config:max-rate=%s", peak);
> + }
> + virCommandAddArgList(cmd, "queues:0=@queue0", vmid_ex_id, ifname_ex_id,
> + "--", "--id=@queue0", "create", "queue", NULL);
> + }
> + virCommandAddArgFormat(cmd, "other_config:min-rate=%s", average);
> + if(burst){
> + virCommandAddArgFormat(cmd, "other_config:burst=%s", burst);
> + }
> + if(peak){
> + virCommandAddArgFormat(cmd, "other_config:max-rate=%s", peak);
> + }
> + virCommandAddArgList(cmd, vmid_ex_id, ifname_ex_id, NULL);
> + if (virCommandRun(cmd, NULL) < 0) {
> + if(*queue_uuid){
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to set queue configuration on port %s"), ifname);
> + }
> + else
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to create and set qos configuration on port %s"), ifname);
If one branch of if() has curly brackets then the other must have them too. Moreover, if a branch doesn't fit into one line then it must have curly braces. So effectivelly, this should be written as:
if (*queue_uuid) {
virReportError(..
..);
} else {
virReportError(..
..);
}
> + goto cleanup;
> + }
> +
> + if(qos_uuid && *qos_uuid){
> + cmd = virNetDevOpenvswitchCreateCmd();
> + lines = g_strsplit(qos_uuid, "\n", 0);
> + virCommandAddArgList(cmd, "set", "qos", lines[0], NULL);
> + virCommandAddArgFormat(cmd, "other_config:min-rate=%s", average);
> + if(burst){
> + virCommandAddArgFormat(cmd, "other_config:burst=%s", burst);
> + }
> + if(peak){
> + virCommandAddArgFormat(cmd, "other_config:max-rate=%s", peak);
> + }
> + virCommandAddArgList(cmd, vmid_ex_id, ifname_ex_id, NULL);
> + if (virCommandRun(cmd, NULL) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to set qos configuration on port %s"), ifname);
> + goto cleanup;
> + }
> + }
> +
> + // ret = virNetDevBandwidthSet(ifname,bandwidth,false,swapped);
> + }
> +
> + if (rx) {
> + average = g_strdup_printf("%llu", rx->average * 8);
> + if (rx->burst)
> + burst = g_strdup_printf("%llu", rx->burst * 8);
1: this. Here the values are in Kbps. Thus we need to multiply by 8. Le sigh.
> + cmd = virNetDevOpenvswitchCreateCmd();
> + virCommandAddArgList(cmd, "set", "Interface", ifname, NULL);
> + virCommandAddArgFormat(cmd, "ingress_policing_rate=%s", average);
> + if (burst)
> + virCommandAddArgFormat(cmd, "ingress_policing_burst=%s", burst);
> +
> + if (virCommandRun(cmd, NULL) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to set vlan configuration on port %s"), ifname);
> + goto cleanup;
> + }
> + }
> +
> + return 0;
> +
> + cleanup:
> + virCommandFree(cmd);
> + VIR_FREE(average);
> + VIR_FREE(peak);
> + VIR_FREE(burst);
> + VIR_FREE(qos_uuid);
> + VIR_FREE(queue_uuid);
> + VIR_FREE(vmid_ex_id);
> + VIR_FREE(ifname_ex_id);
> + VIR_FREE(lines);
All these free calls can be left out if you'd use g_auto* for those variables. Then, this cleanup label won't be needed really, becuase it would contain just one line ..
> + return ret;
.. return ret;
> +}
> +
> +int virNetDevOpenvswitchInterfaceClearQos(const char *ifname,
> + const unsigned char *vmid){
My comments from above apply for this function too.
> + char vmuuidstr[VIR_UUID_STRING_BUFLEN];
> + virCommand *cmd = NULL;
> + char *vmid_ex_id = NULL;
> + char *qos_uuid = NULL;
> + char *queue_uuid = NULL;
> + char **lines = NULL;
> + size_t i;
> +
> + // find qos
> + cmd = virNetDevOpenvswitchCreateCmd();
> + virUUIDFormat(vmid, 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) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to find qos on port %s"), ifname);
Neither of these virReportError() are coupled with return -1. I kind of understand that, because you want this to be best effort, but maybe this function should be void then?
> + }
> +
> + // find queue
> + 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) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to find queue on port %s"), ifname);
> + }
> +
> + if (qos_uuid && *qos_uuid) {
> + lines = g_strsplit(qos_uuid, "\n", 0);
> + // destroy qos
> + for (i = 0; lines[i] != NULL; i++) {
> + const char *line = lines[i];
> + if (!*line) {
> + continue;
> + }
> + cmd = virNetDevOpenvswitchCreateCmd();
> + virCommandAddArgList(cmd, "remove", "port", ifname, "qos", line, NULL);
> + if (virCommandRun(cmd, NULL) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to remove port qos on port %s"), ifname);
This is slightly weird. After 4/4 this code is executed when a guest is shut off. However, I see that the port is removed sonned than this function is called thus running this command fails. This is what I see in the log:
2021-06-29 12:52:32.658+0000: 15442: debug : virCommandRunAsync:2627 : About to run /usr/bin/ovs-vsctl --timeout=5 -- --if-exists del-port vnet2
2021-06-29 12:52:32.662+0000: 15442: debug : virCommandRunAsync:2629 : Command result 0, with PID 29124
2021-06-29 12:52:32.689+0000: 15442: debug : virCommandRun:2473 : Result status 0, stdout: '' stderr: ''
2021-06-29 12:52:32.689+0000: 15442: debug : virCommandRunAsync:2627 : About to run /usr/bin/ovs-vsctl --timeout=5 --no-heading --columns=_uuid find qos 'external-ids:vm-id="4c42854e-3c84-43ed-ae87-9af0df0ecd16"'
2021-06-29 12:52:32.693+0000: 15442: debug : virCommandRunAsync:2629 : Command result 0, with PID 29125
2021-06-29 12:52:32.717+0000: 15442: debug : virCommandRun:2473 : Result status 0, stdout: 'f8b63e05-018d-44bb-9e74-a1ee4a40de79
' stderr: ''
2021-06-29 12:52:32.717+0000: 15442: debug : virCommandRunAsync:2627 : About to run /usr/bin/ovs-vsctl --timeout=5 --no-heading --columns=_uuid find queue 'external-ids:vm-id="4c42854e-3c84-43ed-ae87-9af0df0ecd16"'
2021-06-29 12:52:32.721+0000: 15442: debug : virCommandRunAsync:2629 : Command result 0, with PID 29126
2021-06-29 12:52:32.745+0000: 15442: debug : virCommandRun:2473 : Result status 0, stdout: 'f098939d-c404-49aa-a9b0-b72631b662a2
' stderr: ''
2021-06-29 12:52:32.745+0000: 15442: debug : virCommandRunAsync:2627 : About to run /usr/bin/ovs-vsctl --timeout=5 remove port vnet2 qos f8b63e05-018d-44bb-9e74-a1ee4a40de79
2021-06-29 12:52:32.749+0000: 15442: debug : virCommandRunAsync:2629 : Command result 0, with PID 29127
2021-06-29 12:52:32.772+0000: 15442: error : virCommandWait:2745 : internal error: Child process (/usr/bin/ovs-vsctl --timeout=5 remove port vnet2 qos f8b63e05-018d-44bb-9e74-a1ee4a40de79) unexpected exit status 1: ovs-vsctl: no row "vnet2" in table Port
2021-06-29 12:52:32.772+0000: 15442: debug : virCommandRun:2473 : Result status 0, stdout: '' stderr: 'ovs-vsctl: no row "vnet2" in table Port
'
2021-06-29 12:52:32.772+0000: 15442: error : virNetDevOpenvswitchInterfaceClearQos:851 : internal error: Unable to remove port qos on port vnet2
> + }
> + 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);
> + }
> + }
> + }
> + // destroy queue
> + if (queue_uuid && *queue_uuid) {
> + lines = g_strsplit(queue_uuid, "\n", 0);
> + for (i = 0; lines[i] != NULL; i++) {
> + const char *line = lines[i];
> + if (!*line) {
> + continue;
> + }
> + 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);
> + }
> + }
> + }
> +
> + virCommandFree(cmd);
> + VIR_FREE(qos_uuid);
> + VIR_FREE(queue_uuid);
> + VIR_FREE(vmid_ex_id);
> + VIR_FREE(lines);
> + return 0;
> +}
> \ No newline at end of file
> diff --git a/src/util/virnetdevopenvswitch.h b/src/util/virnetdevopenvswitch.h
> index 7525376855..2dcd1aec6b 100644
> --- a/src/util/virnetdevopenvswitch.h
> +++ b/src/util/virnetdevopenvswitch.h
> @@ -21,6 +21,7 @@
> #pragma once
>
> #include "internal.h"
> +#include "virnetdevbandwidth.h"
> #include "virnetdevvportprofile.h"
> #include "virnetdevvlan.h"
>
> @@ -69,3 +70,13 @@ int virNetDevOpenvswitchGetVhostuserIfname(const char *path,
> int virNetDevOpenvswitchUpdateVlan(const char *ifname,
> const virNetDevVlan *virtVlan)
> ATTRIBUTE_NONNULL(1) G_GNUC_WARN_UNUSED_RESULT;
> +
> +int virNetDevOpenvswitchInterfaceSetQos(const char *ifname,
> + const virNetDevBandwidth *bandwidth,
> + const unsigned char *vmid,
> + bool swapped)
> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) G_GNUC_WARN_UNUSED_RESULT;
> +
> +int virNetDevOpenvswitchInterfaceClearQos(const char *ifname,
> + const unsigned char *vmid)
> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT;
>
Michal
