[libvirt] [PATCH 1/3] virNetDevOpenvswitchInterfaceStats: Optimize for speed

[Michal Prívozník]

On 7/12/19 6:28 PM, Ján Tomko wrote:
> On Wed, Jul 03, 2019 at 09:19:18AM +0200, Michal Privoznik wrote:
>> We run 'ovs-vsctl' nine times (first to find if interface is
>> there and then eight times = for each stats member separately).
>> This is very inefficient. I've found a way to run it once and
>> with a bit of help from virJSON module we can parse out stats
>> we need.
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>> src/util/virnetdevopenvswitch.c | 111 +++++++++++++++++++++-----------
>> 1 file changed, 74 insertions(+), 37 deletions(-)
>>
>> diff --git a/src/util/virnetdevopenvswitch.c
>> b/src/util/virnetdevopenvswitch.c
>> index c99ecfbf15..0fe64bedab 100644
>> --- a/src/util/virnetdevopenvswitch.c
>> +++ b/src/util/virnetdevopenvswitch.c
>> @@ -28,6 +28,7 @@
>> #include "virmacaddr.h"
>> #include "virstring.h"
>> #include "virlog.h"
>> +#include "virjson.h"
>>
>> #define VIR_FROM_THIS VIR_FROM_NONE
>>
>> @@ -311,58 +312,94 @@ int
>> virNetDevOpenvswitchInterfaceStats(const char *ifname,
>>                                    virDomainInterfaceStatsPtr stats)
>> {
>> -    char *tmp;
>> -    bool gotStats = false;
>>     VIR_AUTOPTR(virCommand) cmd = NULL;
>>     VIR_AUTOFREE(char *) output = NULL;
>> +    VIR_AUTOPTR(virJSONValue) jsonStats = NULL;
>> +    virJSONValuePtr jsonMap = NULL;
>> +    size_t i;
>>
>> -    /* Just ensure the interface exists in ovs */
>>     cmd = virCommandNew(OVSVSCTL);
>>     virNetDevOpenvswitchAddTimeout(cmd);
>> -    virCommandAddArgList(cmd, "get", "Interface", ifname, "name", NULL);
>> +    virCommandAddArgList(cmd, "--if-exists", "--format=list",
>> "--data=json",
>> +                         "--no-headings", "--columns=statistics",
>> "list",
>> +                         "Interface", ifname, NULL);
> 
> Can we assume these options are present in all the supported versions of
> ovs-vsctl?

Rough skim through ovs' git log shows that these were added starting
from v1.1.0pre1~233 till v1.1.0~294 (mid of 2010 - beginnig of 2011).
Therefore, I believe it's safe to use them. And if there's somebody
running 10 years old ovs with the latest libvirt we can tell them to update.

> 
>>     virCommandSetOutputBuffer(cmd, &output);
>>
>> -    if (virCommandRun(cmd, NULL) < 0) {
>> +    /* The above command returns either:
>> +     * 1) empty string if @ifname doesn't exist, or
>> +     * 2) a JSON array, for instance:
>> +     *   
>> ["map",[["collisions",0],["rx_bytes",0],["rx_crc_err",0],["rx_dropped",0],
>>
>> +     *   
>> ["rx_errors",0],["rx_frame_err",0],["rx_over_err",0],["rx_packets",0],
>> +     *   
>> ["tx_bytes",12406],["tx_dropped",0],["tx_errors",0],["tx_packets",173]]]
>> +     */
>> +
> 
> This example would look better in tests/ where you test this function
> against actual output O:-)

Well, we don't have virNetDevOpenvswitch test or anything. But even if
we did, the test case would work over the output we obtained at some
point in the time. The input would not change. So effectively, we would
be testing only our JSON parsing skills and not if the function fetches
the correct data. Sorry.

Michal