[libvirt] [PATCH] virNetDevOpenvswitchInterfaceStats: Be more forgiving when fetching stats

[Michal Privoznik]

On 06/23/2017 11:32 AM, Martin Kletzander wrote:
> On Thu, Jun 22, 2017 at 03:44:46PM +0200, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1461270
>>
>> When fetching stats for a vhost-user type of interface, we run
>> couple of ovs-vsctl commands and parse their output. However, not
>> all stats exist at all times, for instance "rx_dropped" or
>> "tx_errors" can be missing. Thing is, we ask for a bulk of
>> statistics and if one of them is missing an error is reported
>> instead of returning the rest. Since we ignore errors, we fail to
>> set statistics. Fix this by asking for each piece alone.
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>> src/util/virnetdevopenvswitch.c | 96
>> +++++++++++++++--------------------------
>> 1 file changed, 34 insertions(+), 62 deletions(-)
>>
>> diff --git a/src/util/virnetdevopenvswitch.c
>> b/src/util/virnetdevopenvswitch.c
>> index 8f7215e06..6848f65b7 100644
>> --- a/src/util/virnetdevopenvswitch.c
>> +++ b/src/util/virnetdevopenvswitch.c
>> @@ -340,67 +334,45 @@ virNetDevOpenvswitchInterfaceStats(const char
>> *ifname,
> 
> [...]
> 
>> +    if (!gotStats) {
>>         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> -                       _("Fail to parse ovs-vsctl output"));
>> +                       _("Interface doesn't have statistics"));
> 
> s/have/have any/ or s/have/support/
> 
>>         goto cleanup;
> 
> Is it possible that the version of ovs-vsctl command doesn't return
> non-zero value and the right fix is just setting the values to -1
> instead of the 'goto cleanup;' here (and leave it to fail if one of the
> first four values are missing)?

Well, if it doesn't return a non-zero value on an error, it's ovs bug
because of their documentation:

EXIT STATUS
       0      Successful program execution.
       1      Usage, syntax, or configuration file error.

> 
> Anyway, your approach is more error-prone, even though we're running the
> command 8 times (ewww). 

Yes, I don't like it either.

>  But the output of `ovs-vsctl get interface
> $ifname statistics` is not in a format that we would have parsers for.
> Even though it wouldn't be that hard to parse it (wiki:BiteSizedTasks?),

Good idea, I'll add it there.

> it's not that big of a deal except the constant statistics-gathering
> APIs.  But it would be nice if we fixed that in the (hopefully near)
> future.

Well, the ultimate goal would be to communicate with ovs directly via
socket instead of spawning ovs-vsctl command repeatedly (which is
something we do in other areas of the code btw - e.g. when plugging a
nic into an ovs bridge). However, that is a long shot from here, because
so far OVS devels consider their protocol the same way we do - internal
implementation. There is no stable library for communicating with ovs
yet. So I'm afraid for now we're stuck with spawning binaries :(

> 
> Reviewed-by: Martin Kletzander <mkletzan at redhat.com>

Thanks, pushed.

Michal