[libvirt] [PATCH 3/3] lxc: implement connectGetAllDomainStats

John Ferlan jferlan at redhat.com
Wed Mar 7 14:41:42 UTC 2018



On 03/05/2018 12:21 PM, Cédric Bosdonnat wrote:
> LXC containers can also provide some statistics. Allow users to fetch
> them using the existing API.

I think you're fetching more than "some" ;-)!

> ---
>  src/lxc/lxc_driver.c | 203 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 203 insertions(+)
> 

Please don't forget to add a docs/news.xml update for the new functionality

> diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
> index a16dbcc96..c357df927 100644
> --- a/src/lxc/lxc_driver.c
> +++ b/src/lxc/lxc_driver.c
> @@ -80,6 +80,7 @@
>  #include "viraccessapichecklxc.h"
>  #include "virhostdev.h"
>  #include "netdev_bandwidth_conf.h"
> +#include "domain_stats.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_LXC
>  
> @@ -5483,6 +5484,207 @@ lxcDomainHasManagedSaveImage(virDomainPtr dom, unsigned int flags)
>      return ret;
>  }
>  

2 blank lines for each...

> +static int
> +lxcDomainGetStatsState(virLXCDriverPtr driver ATTRIBUTE_UNUSED,
> +                       virDomainObjPtr dom,
> +                       virDomainStatsRecordPtr record,
> +                       int *maxparams)
> +{
> +    return virDomainStatsGetState(dom, record, maxparams);
> +}
> +
> +static int
> +lxcDomainGetStatsCpu(virLXCDriverPtr driver ATTRIBUTE_UNUSED,
> +                     virDomainObjPtr dom,
> +                     virDomainStatsRecordPtr record,
> +                     int *maxparams)
> +{
> +    virLXCDomainObjPrivatePtr priv = dom->privateData;
> +    return virCgroupGetStatsCpu(priv->cgroup, record, maxparams);
> +}
> +
> +static int
> +lxcDomainGetStatsInterface(virLXCDriverPtr driver ATTRIBUTE_UNUSED,
> +                           virDomainObjPtr dom,
> +                           virDomainStatsRecordPtr record,
> +                           int *maxparams)
> +{
> +    return virDomainStatsGetInterface(dom, record, maxparams);
> +}
> +
> +/* expects a LL, but typed parameter must be ULL */
> +#define LXC_ADD_BLOCK_PARAM_LL(record, maxparams, name, value) \
> +do { \
> +    char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
> +    snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
> +             "block.cgroup.%s", name); \
> +    if (virTypedParamsAddULLong(&(record)->params, \
> +                                &(record)->nparams, \
> +                                maxparams, \
> +                                param_name, \
> +                                value) < 0) \
> +        goto cleanup; \
> +} while (0)

I think it'll look cleaner if the do { ... } while gets 4 char indent.
Although I see you've essentially copied QEMU_ADD_BLOCK_PARAM_LL

> +
> +static int
> +lxcDomainGetStatsBlock(virLXCDriverPtr driver,
> +                       virDomainObjPtr dom,
> +                       virDomainStatsRecordPtr record,
> +                       int *maxparams)
> +{
> +    long long rd_req, rd_bytes, wr_req, wr_bytes;

Here could be virDomainBlockStats stats = { 0 }; and of course
subsequent adjustments.

> +
> +    int ret = lxcDomainBlockStatsInternal(driver, dom, "",
> +                                          &rd_bytes,
> +                                          &wr_bytes,
> +                                          &rd_req,
> +                                          &wr_req);
> +
> +    LXC_ADD_BLOCK_PARAM_LL(record, maxparams,
> +                            "rd.reqs", rd_req);
> +    LXC_ADD_BLOCK_PARAM_LL(record, maxparams,
> +                            "rd.bytes", rd_bytes);
> +    LXC_ADD_BLOCK_PARAM_LL(record, maxparams,
> +                            "wr.reqs", wr_req);
> +    LXC_ADD_BLOCK_PARAM_LL(record, maxparams,
> +                            "wr.bytes", wr_bytes);
                              ^^
The above all are off by one...

> +
> + cleanup:
> +    return ret;
> +}
> +
> +#undef LXC_ADD_BLOCK_PARAM_ULL
> +
> +typedef int
> +(*lxcDomainGetStatsFunc)(virLXCDriverPtr driver,
> +                         virDomainObjPtr dom,
> +                         virDomainStatsRecordPtr record,
> +                         int *maxparams);
> +
> +struct lxcDomainGetStatsWorker {
> +    lxcDomainGetStatsFunc func;
> +    unsigned int stats;
> +};
> +
> +static struct lxcDomainGetStatsWorker lxcDomainGetStatsWorkers[] = {
> +    { lxcDomainGetStatsState, VIR_DOMAIN_STATS_STATE },
> +    { lxcDomainGetStatsCpu, VIR_DOMAIN_STATS_CPU_TOTAL },
> +    { lxcDomainGetStatsInterface, VIR_DOMAIN_STATS_INTERFACE },
> +    { lxcDomainGetStatsBlock, VIR_DOMAIN_STATS_BLOCK },
> +    { NULL, 0 }
> +};
> +
> +static int
> +lxcDomainGetStats(virConnectPtr conn,
> +                  virDomainObjPtr dom,
> +                  unsigned int stats,
> +                  virDomainStatsRecordPtr *record)

I see qemu doesn't use the @flags anywhere - your call if you think it
could be important in the future - right now it's not could add @flags
here so that if it is ever used

> +{
> +    virLXCDriverPtr driver = conn->privateData;
> +    int maxparams = 0;
> +    virDomainStatsRecordPtr tmp;
> +    size_t i;
> +    int ret = -1;
> +
> +    if (VIR_ALLOC(tmp) < 0)
> +        goto cleanup;
> +
> +    for (i = 0; lxcDomainGetStatsWorkers[i].func; i++) {
> +        if (stats & lxcDomainGetStatsWorkers[i].stats) {
> +            if (lxcDomainGetStatsWorkers[i].func(driver, dom, tmp, &maxparams) < 0)
> +                goto cleanup;
> +        }
> +    }
> +
> +    if (!(tmp->dom = virGetDomain(conn, dom->def->name,
> +                                  dom->def->uuid, dom->def->id)))
> +        goto cleanup;
> +
> +    *record = tmp;
> +    tmp = NULL;
> +    ret = 0;
> +
> + cleanup:
> +    if (tmp) {
> +        virTypedParamsFree(tmp->params, tmp->nparams);
> +        VIR_FREE(tmp);
> +    }
> +
> +    return ret;
> +}
> +
> +static int
> +lxcConnectGetAllDomainStats(virConnectPtr conn,
> +                            virDomainPtr *doms,
> +                            unsigned int ndoms,
> +                            unsigned int stats,
> +                            virDomainStatsRecordPtr **retStats,
> +                            unsigned int flags)
> +{
> +    virLXCDriverPtr driver = conn->privateData;
> +    virDomainObjPtr *vms = NULL;
> +    virDomainObjPtr vm;
> +    size_t nvms;
> +    virDomainStatsRecordPtr *tmpstats = NULL;
> +    int nstats = 0;
> +    size_t i;
> +    int ret = -1;
> +    unsigned int lflags = flags & (VIR_CONNECT_LIST_DOMAINS_FILTERS_ACTIVE |
> +                                   VIR_CONNECT_LIST_DOMAINS_FILTERS_PERSISTENT |
> +                                   VIR_CONNECT_LIST_DOMAINS_FILTERS_STATE);
> +
> +    virCheckFlags(VIR_CONNECT_LIST_DOMAINS_FILTERS_ACTIVE |
> +                  VIR_CONNECT_LIST_DOMAINS_FILTERS_PERSISTENT |
> +                  VIR_CONNECT_LIST_DOMAINS_FILTERS_STATE, -1);
> +
> +    if (virConnectGetAllDomainStatsEnsureACL(conn) < 0)
> +        return -1;
> +
> +    /* TODO Check stats support */

Is this a stray or work still to be done?  Seems this is important for
the VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS flag in qemu, which
isn't being supported here.

> +
> +    if (ndoms) {
> +        if (virDomainObjListConvert(driver->domains, conn, doms, ndoms, &vms,
> +                                    &nvms, virConnectGetAllDomainStatsCheckACL,
> +                                    lflags, true) < 0)
> +            return -1;
> +    } else {
> +        if (virDomainObjListCollect(driver->domains, conn, &vms, &nvms,
> +                                    virConnectGetAllDomainStatsCheckACL,
> +                                    lflags) < 0)
> +            return -1;
> +    }

Ironically if nvms == 0, we return an empty tmpstats with ret = 1...
Same with qemu. I'll post a patch.

> +
> +    if (VIR_ALLOC_N(tmpstats, nvms + 1) < 0)
> +        return -1;

This would leak vms... same bug exists in qemu.

> +
> +    for (i = 0; i < nvms; i++) {
> +        virDomainStatsRecordPtr tmp = NULL;
> +        vm = vms[i];
> +
> +        virObjectLock(vm);
> +
> +        if (lxcDomainGetStats(conn, vm, stats, &tmp) < 0) {
> +            virObjectUnlock(vm);
> +            goto cleanup;
> +        }
> +
> +        if (tmp)
> +            tmpstats[nstats++] = tmp;
> +
> +        virObjectUnlock(vm);
> +    }
> +
> +    *retStats = tmpstats;
> +    tmpstats = NULL;

VIR_STEAL_PTR(*retStats, tmpstats);

John

> +
> +    ret = nstats;
> +
> + cleanup:
> +    virDomainStatsRecordListFree(tmpstats);
> +    virObjectListFreeCount(vms, nvms);
> +
> +    return ret;
> +}
>  
>  /* Function Tables */
>  static virHypervisorDriver lxcHypervisorDriver = {
> @@ -5577,6 +5779,7 @@ static virHypervisorDriver lxcHypervisorDriver = {
>      .nodeGetFreePages = lxcNodeGetFreePages, /* 1.2.6 */
>      .nodeAllocPages = lxcNodeAllocPages, /* 1.2.9 */
>      .domainHasManagedSaveImage = lxcDomainHasManagedSaveImage, /* 1.2.13 */
> +    .connectGetAllDomainStats = lxcConnectGetAllDomainStats, /* 4.2.0 */
>  };
>  
>  static virConnectDriver lxcConnectDriver = {
> 




More information about the libvir-list mailing list