Re: [libvirt] [PATCH 2/2] test_driver: implement virDomainSetBlockIoTune

On Sat, Jul 27, 2019 at 05:04:38PM +0200, Ilias Stamatis wrote: > Signed-off-by: Ilias Stamatis <stamatis.iliass(a)gmail.com&gt; > --- > > 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 = &params[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(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list