[libvirt] [PATCHv3] lib: Introduce API for retrieving bulk domain stats

[Peter Krempa]

On 08/26/14 21:41, Eric Blake wrote:
> On 08/26/2014 01:11 PM, Peter Krempa wrote:
>> The motivation for this API is that management layers that use libvirt
>> usually poll for statistics using various split up APIs we currently
>> provide. To get all the necessary stuff, the app needs to issue a lot of
>> calls and aggregate the results.
>>
>> The APIs I'm introducing here:
>> 1) Returns data in a format that we can expand in the future and is
>> (pseudo) hierarchical. The data is returned as typed parameters where
>> the fields are constructed as dot-separated strings containing names and
>> other stuff in a list of typed params.
>>
>> 2) Stats for multiple (all) domains can be queried at once and are
>> returned in one call. This will allow to decrease the overhead necessary
> 
> s/allow to //
> 
>> to issue multiple calls per domain multiplied by the count of domains.
>>
>> 3) Selectable (bit mask) fields in the returned format. This will allow
>> to retrieve only specific stats according to the apps need.
> 
> s/apps/app's/
> 
>>
>> The stats groups will be enabled using a bit field @stats passed as the
>> function argument. A few sample stats groups that this API will support:
>>
>> VIR_DOMAIN_STATS_STATE
>> VIR_DOMAIN_STATS_CPU
>> VIR_DOMAIN_STATS_BLOCK
>> VIR_DOMAIN_STATS_INTERFACE
>>
>> (Note that this is only an example, the initial implementation supports
>>  only VIR_DOMAIN_STATS_STATE while others will be added later)
>>
>> the returned typed params will use the following scheme
>>
>> state.state = running
>> state.reason = started
> 
> Actually, you return int enum values, rather than a stringized state
> name.  So this would better be listed as
>  state.state = VIR_DOMAIN_RUNNING
>  state.reason = VIR_DOMAIN_RUNNING_BOOTED
> 
>> cpu.count = 8
>> cpu.0.state = running
>> cpu.0.time = 1234
>> ---
> 
>> +++ b/src/driver.h
>> @@ -1197,6 +1197,14 @@ typedef int
>>                            unsigned int flags);
>>
>>
>> +typedef int
>> +(*virDrvDomainListGetStats)(virConnectPtr conn,
>> +                            virDomainPtr *doms,
>> +                            unsigned int ndoms,
>> +                            unsigned int stats,
>> +                            virDomainStatsRecordPtr **retStats,
>> +                            unsigned int flags);
> 
> I'm still worried that this may be generating the wrong ACL functions,
> and that it should be named virDrvConnectDomainListGetStats so as to
> give a hint to the generators that this is an operation on the
> connection, that happens to take an optional DomainList argument.

I had to rename it to virDrvConnectGetAllDomainStats to make the API doc
generator happy.

> 
>> +
>>  typedef struct _virDriver virDriver;
>>  typedef virDriver *virDriverPtr;
>>
>> @@ -1418,6 +1426,7 @@ struct _virDriver {
>>      virDrvDomainSetTime domainSetTime;
>>      virDrvNodeGetFreePages nodeGetFreePages;
>>      virDrvConnectGetDomainCapabilities connectGetDomainCapabilities;
>> +    virDrvDomainListGetStats domainListGetStats;
> 
> I'd name this callback member connectDomainListGetStats.
> 
>> + * The statistic groups are enabled using the @stats parameter which is a
>> + * binary-OR of enum virDomainStatsTypes. The following groups are available
>> + * (although not necessarily implemented for each hypervisor):
>> + *
>> + * VIR_DOMAIN_STATS_STATE: Return domain state and reason for entering that
>> + * state. The typed parameter keys are in format:
> 
> s/in/in this/
> 
>> + * "state.state" - state of the VM, returned as int from virDomainState enum
>> + * "state.reason" - reason for entering given state, returned as int from
>> + *                  virDomain*Reason enum corresponding to given state.
>> + *
>> + * Using 0 for @stats returns all stats groups supported by given hypervisor.
> 
> s/given/the given/
> 
>> + *
>> + * Returns the count of returned statistics strucutres on success, -1 on error.
> 
> s/strucutres/structures/
> 
>> + * The requested data are returned in the @retStats parameter. The returned
>> + * array should be freed by the caller. See virDomainStatsRecordListFree.
>> + */
>> +int
>> +virConnectGetAllDomainStats(virConnectPtr conn,
>> +                            unsigned int stats,
>> +                            virDomainStatsRecordPtr **retStats,
>> +                            unsigned int flags)
>> +{
> 
> implementation looks okay
> 
>> +/**
>> + * virDomainListGetStats:
>> + * @doms: NULL terminated array of domains
>> + * @stats: stats to return, binary-OR of virDomainStatsTypes
>> + * @retStats: Pointer that will be filled with the array of returned stats.
>> + * @flags: extra flags; not used yet, so callers should always pass 0
>> + *
> 
>> + * Using 0 for @stats returns all stats groups supported by given hypervisor.
>> + *
>> + * Returns the count of returned statistics structures on success, -1 on error.
>> + * The requested data are returned in the @retStats parameter. The returned
>> + * array should be freed by the caller. See virDomainStatsRecordListFree.
>> + *
>> + * Note that the count of returned stats may be less than the domain count provided
>> + * via @doms.
> 
> The docs generator doesn't like paragraphs that appear after a sentence
> starting with "Returns"; drop the blank line so that it is part of the
> returns paragraph and I think you'll be fine.
> 
> 
>> +
>> +/**
>> + * virDomainStatsRecordListFree:
>> + * @stats: NULL terminated array of virDomainStatsRecords to free
>> + *
>> + * Convenience function to free a list of domain stats returned by
>> + * virDomainListGetStats and virConnectGetAllDomainStats.
>> + */
>> +void
>> +virDomainStatsRecordListFree(virDomainStatsRecordPtr *stats)
>> +{
>> +    virDomainStatsRecordPtr *next;
>> +
>> +    for (next = stats; *next; next++) {
> 
> Missing a lead-in:
> 
> if (!stats)
>     return;
> 
> which will let users do virDomainStatsRecordListFree(NULL) and ease
> their cleanup code.
> 
>>
>>  LIBVIRT_1.2.8 {
>> -     global:
>> -         virDomainOpenGraphicsFD;
>> +    global:
>> +        virDomainOpenGraphicsFD;
>> +        virDomainListGetStats;
>> +        virConnectGetAllDomainStats;
>> +        virDomainStatsRecordListFree;
> 
> I tend to sort these alphabetically, but it doesn't strictly matter.
> 
> ACK with the comments above addressed.  (The biggest impact may be that
> renaming the driver.h callback name will impact the ACL generator and
> cause you some headaches as you respin patch 3 in the series)
> 
> Let's commit the API now, so we can do the release candidate, and the
> rest of the series can dribble in between now and rc2 as needed.
> 

Pushed; I'll follow up with the rest as soon as possible.

Thanks and sorry for messing with the release cycle :/

Peter

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140826/85d7d2f8/attachment-0001.sig>