On 09/10/14 15:55, Francesco Romani wrote:
----- Original Message -----
> From: "Peter Krempa" <pkrempa(a)redhat.com>
> To: "Francesco Romani" <fromani(a)redhat.com>, libvir-list(a)redhat.com
> Sent: Wednesday, September 10, 2014 10:07:33 AM
> Subject: Re: [libvirt] [PATCH 1/8] qemu: bulk stats: extend internal collection API
[...]
>>> Hmm this skips offline domains entirely if one of the stats groups needs
>>> the monitor.
>>>
>>> I think we should rather skip individual stats groups, or better stats
>>> fields that we can't provide.
>>>
>>> Any ideas?
>>
>> What about this (pseudo-C):
>>
>> unsigned int privflags = 0;
>>
>> if (needmon && qemuDomainObjBeginJob(driver, dom, QEMU_JOB_QUERY)
>> needmon = false;
>> /* auto disable monitoring, the remainder of the function should be
>> unchanged */
>> else
>> privflags |= MONITOR_AVAILABLE;
>>
>> if ((needmon && virDomainObjIsActive(dom)) || !needmon) {
Drop this condition ...
>> if (qemuDomainGetStats(conn, dom, stats, &tmp,
privflags) < 0)
The individual stats groups will decide if they need the VM alive they
can skip it individually rather than omitting the vm completely.
>> /* pass monitor availability down the chain.
Individual workers
>> will
>> bail out immediately and silently if they need monitor but
>> it is
>> not available
>> */
>> goto endjob;
>>
>> if (tmp)
>> tmpstats[nstats++] = tmp;
>> }
>>
>>
>> No other change should be needed to this patch, and with trivial changes
>> all the others can be fixed.
>
> Also you can just grab the lock always and the workers will exit if the
> VM is not alive. Having a domain job should be fine.
As you prefer. For oVirt, we do care of all the stats group implemented, and always all
of them,
(actually I may have missed some bits, e.g. the pininfo as you pointed out elsewhere -
going to fix),
so for our needs we'll always need to enter the monitor.
But other users of this API may beg to differ, and I believe is fair to assume that other
consumers
of this API could just use stats which doesn't require to enter the monitor: e.g.
CPU/VCPU/interface.
In case you don't have any stat group that requires the monitor it is
imho fine not to get a job. If there is a group where we can enter it we
can just grab the monitor always ... see above.
Hence, I was trying to be a good citizen and do not require monitor access unless it is
actually
needed; but if turns out it is OK to do a domain job anyway, I'll happily simplify my
code :)
Thanks and bests,
Peter