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 = {
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
