[libvirt] [PATCH 2/2] test_driver: implement virDomainSetBlockIoTune
Erik Skultety
eskultet at redhat.com
Sun Aug 4 16:32:44 UTC 2019
On Sat, Jul 27, 2019 at 05:04:38PM +0200, Ilias Stamatis wrote:
> Signed-off-by: Ilias Stamatis <stamatis.iliass at gmail.com>
> ---
>
> I deliberately omitted the logic from qemuDomainSetBlockIoTuneDefaults
> in order to leave the function simpler. I think that in the case of the
> test driver it is ok.
>
> After all, how we handle the parameters it is supposed to be
> hypervisor-specific.
>
> src/test/test_driver.c | 196 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 196 insertions(+)
>
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index 9f4e255e35..3272ecf4b7 100755
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -3502,6 +3502,201 @@ testDomainGetInterfaceParameters(virDomainPtr dom,
> }
>
>
> +static int
> +testDomainSetBlockIoTune(virDomainPtr dom,
> + const char *path,
> + virTypedParameterPtr params,
> + int nparams,
> + unsigned int flags)
> +{
> + virDomainObjPtr vm = NULL;
> + virDomainDefPtr def = NULL;
> + virDomainBlockIoTuneInfo info = {0};
> + virDomainDiskDefPtr conf_disk = NULL;
> + virTypedParameterPtr eventParams = NULL;
> + int eventNparams = 0;
> + int eventMaxparams = 0;
> + size_t i;
> + int ret = -1;
> +
> + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
> + VIR_DOMAIN_AFFECT_CONFIG, -1);
> +
> + if (virTypedParamsValidate(params, nparams,
> + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME,
> + VIR_TYPED_PARAM_STRING,
> + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX_LENGTH,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX_LENGTH,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX_LENGTH,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX_LENGTH,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX_LENGTH,
> + VIR_TYPED_PARAM_ULLONG,
> + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX_LENGTH,
> + VIR_TYPED_PARAM_ULLONG,
> + NULL) < 0)
> + return -1;
> +
> + if (!(vm = testDomObjFromDomain(dom)))
> + return -1;
> +
> + if (!(def = virDomainObjGetOneDef(vm, flags)))
> + goto cleanup;
> +
> + if (!(conf_disk = virDomainDiskByName(def, path, true))) {
> + virReportError(VIR_ERR_INVALID_ARG,
> + _("missing persistent configuration for disk '%s'"),
> + path);
> + goto cleanup;
> + }
> +
> + info = conf_disk->blkdeviotune;
> + if (VIR_STRDUP(info.group_name, conf_disk->blkdeviotune.group_name) < 0)
> + goto cleanup;
> +
> + if (virTypedParamsAddString(&eventParams, &eventNparams, &eventMaxparams,
> + VIR_DOMAIN_TUNABLE_BLKDEV_DISK, path) < 0)
> + goto cleanup;
> +
> +#define SET_IOTUNE_FIELD(FIELD, BOOL, CONST) \
redundant BOOL argument...
> + if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_##CONST)) { \
> + info.FIELD = param->value.ul; \
> + if (virTypedParamsAddULLong(&eventParams, &eventNparams, \
> + &eventMaxparams, \
> + VIR_DOMAIN_TUNABLE_BLKDEV_##CONST, \
We've already discussed this and I believe we decided we're not going to
oncatenate the enum names, so let's do it differently in the test driver.
> + param->value.ul) < 0) \
> + goto cleanup; \
> + continue; \
> + }
> +
> + for (i = 0; i < nparams; i++) {
> + virTypedParameterPtr param = ¶ms[i];
> +
> + if (param->value.ul > 1000000000000000LL) {
I think we may want to define ^this as a macro constant for test driver as
well.
> + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED,
> + _("block I/O throttle limit value must"
> + " be no more than %llu"), 1000000000000000LL);
> + goto cleanup;
> + }
> +
> + SET_IOTUNE_FIELD(total_bytes_sec, BYTES, TOTAL_BYTES_SEC);
> + SET_IOTUNE_FIELD(read_bytes_sec, BYTES, READ_BYTES_SEC);
> + SET_IOTUNE_FIELD(write_bytes_sec, BYTES, WRITE_BYTES_SEC);
> + SET_IOTUNE_FIELD(total_iops_sec, IOPS, TOTAL_IOPS_SEC);
> + SET_IOTUNE_FIELD(read_iops_sec, IOPS, READ_IOPS_SEC);
> + SET_IOTUNE_FIELD(write_iops_sec, IOPS, WRITE_IOPS_SEC);
> +
> + SET_IOTUNE_FIELD(total_bytes_sec_max, BYTES_MAX, TOTAL_BYTES_SEC_MAX);
> + SET_IOTUNE_FIELD(read_bytes_sec_max, BYTES_MAX, READ_BYTES_SEC_MAX);
> + SET_IOTUNE_FIELD(write_bytes_sec_max, BYTES_MAX, WRITE_BYTES_SEC_MAX);
> + SET_IOTUNE_FIELD(total_iops_sec_max, IOPS_MAX, TOTAL_IOPS_SEC_MAX);
> + SET_IOTUNE_FIELD(read_iops_sec_max, IOPS_MAX, READ_IOPS_SEC_MAX);
> + SET_IOTUNE_FIELD(write_iops_sec_max, IOPS_MAX, WRITE_IOPS_SEC_MAX);
> + SET_IOTUNE_FIELD(size_iops_sec, SIZE_IOPS, SIZE_IOPS_SEC);
> +
> + if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME)) {
> + VIR_FREE(info.group_name);
> + if (VIR_STRDUP(info.group_name, param->value.s) < 0)
> + goto cleanup;
> + if (virTypedParamsAddString(&eventParams,
> + &eventNparams,
> + &eventMaxparams,
> + VIR_DOMAIN_TUNABLE_BLKDEV_GROUP_NAME,
> + param->value.s) < 0)
> + goto cleanup;
> + continue;
> + }
> +
> + SET_IOTUNE_FIELD(total_bytes_sec_max_length, BYTES_MAX_LENGTH,
> + TOTAL_BYTES_SEC_MAX_LENGTH);
> + SET_IOTUNE_FIELD(read_bytes_sec_max_length, BYTES_MAX_LENGTH,
> + READ_BYTES_SEC_MAX_LENGTH);
> + SET_IOTUNE_FIELD(write_bytes_sec_max_length, BYTES_MAX_LENGTH,
> + WRITE_BYTES_SEC_MAX_LENGTH);
> + SET_IOTUNE_FIELD(total_iops_sec_max_length, IOPS_MAX_LENGTH,
> + TOTAL_IOPS_SEC_MAX_LENGTH);
> + SET_IOTUNE_FIELD(read_iops_sec_max_length, IOPS_MAX_LENGTH,
> + READ_IOPS_SEC_MAX_LENGTH);
> + SET_IOTUNE_FIELD(write_iops_sec_max_length, IOPS_MAX_LENGTH,
> + WRITE_IOPS_SEC_MAX_LENGTH);
> + }
> +#undef SET_IOTUNE_FIELD
> +
> + if ((info.total_bytes_sec && info.read_bytes_sec) ||
> + (info.total_bytes_sec && info.write_bytes_sec)) {
> + virReportError(VIR_ERR_INVALID_ARG, "%s",
> + _("total and read/write of bytes_sec "
> + "cannot be set at the same time"));
> + goto cleanup;
> + }
> +
> + if ((info.total_iops_sec && info.read_iops_sec) ||
> + (info.total_iops_sec && info.write_iops_sec)) {
> + virReportError(VIR_ERR_INVALID_ARG, "%s",
> + _("total and read/write of iops_sec "
> + "cannot be set at the same time"));
> + goto cleanup;
> + }
> +
> + if ((info.total_bytes_sec_max && info.read_bytes_sec_max) ||
> + (info.total_bytes_sec_max && info.write_bytes_sec_max)) {
> + virReportError(VIR_ERR_INVALID_ARG, "%s",
> + _("total and read/write of bytes_sec_max "
> + "cannot be set at the same time"));
> + goto cleanup;
> + }
> +
> + if ((info.total_iops_sec_max && info.read_iops_sec_max) ||
> + (info.total_iops_sec_max && info.write_iops_sec_max)) {
> + virReportError(VIR_ERR_INVALID_ARG, "%s",
> + _("total and read/write of iops_sec_max "
> + "cannot be set at the same time"));
> + goto cleanup;
> + }
> +
> + if (virDomainDiskSetBlockIOTune(conf_disk, &info) < 0)
> + goto cleanup;
> + info.group_name = NULL;
You should also be checking violations against the max settings, like you
mentioned in the commit note, some of the checks are hypervisor dependent, but
some are purely logical, e.g. setting one of the limits higher than the
corresponding MAX feels odd. I'm not insisting on the need to specify the limit
along the max (that really feels like per-hypervisor detail).
> +
> + ret = 0;
> + cleanup:
> + VIR_FREE(info.group_name);
> + virDomainObjEndAPI(&vm);
> + if (eventNparams)
> + virTypedParamsFree(eventParams, eventNparams);
> + return ret;
> +}
> +
> +
> static int
> testDomainGetBlockIoTune(virDomainPtr dom,
> const char *path,
> @@ -8600,6 +8795,7 @@ static virHypervisorDriver testHypervisorDriver = {
> .domainSetNumaParameters = testDomainSetNumaParameters, /* 5.6.0 */
> .domainGetNumaParameters = testDomainGetNumaParameters, /* 5.6.0 */
> .domainSetInterfaceParameters = testDomainSetInterfaceParameters, /* 5.6.0 */
> + .domainSetBlockIoTune = testDomainSetBlockIoTune, /* 5.6.0 */
Since you're going to re-spin, don't forget to update ^this.
Erik
> .domainGetInterfaceParameters = testDomainGetInterfaceParameters, /* 5.6.0 */
> .domainGetBlockIoTune = testDomainGetBlockIoTune, /* 5.6.0 */
> .connectListDefinedDomains = testConnectListDefinedDomains, /* 0.1.11 */
> --
> 2.22.0
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
More information about the libvir-list
mailing list