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

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.

> + > 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.