src/test/test_driver.c | 127 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 127 insertions(+)
Add basic coverage of device update; for now, only support disk updates
until other types are needed or tested.
Signed-off-by: John Levon <john.levon@nutanix.com>
---
src/test/test_driver.c | 127 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 127 insertions(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 40fb8a467d..213fbdbccb 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -41,6 +41,7 @@
#include "domain_conf.h"
#include "domain_driver.h"
#include "domain_event.h"
+#include "domain_postparse.h"
#include "domain_validate.h"
#include "network_event.h"
#include "snapshot_conf.h"
@@ -10237,6 +10238,131 @@ testDomainAttachDevice(virDomainPtr domain, const char *xml)
}
+static int
+testDomainUpdateDeviceConfig(virDomainDef *vmdef,
+ virDomainDeviceDef *dev,
+ unsigned int parse_flags,
+ virDomainXMLOption *xmlopt)
+{
+ virDomainDiskDef *newDisk;
+ virDomainDeviceDef oldDev = { .type = dev->type };
+ int pos;
+
+ switch (dev->type) {
+ case VIR_DOMAIN_DEVICE_DISK:
+ newDisk = dev->data.disk;
+ if ((pos = virDomainDiskIndexByName(vmdef, newDisk->dst, false)) < 0) {
+ virReportError(VIR_ERR_INVALID_ARG,
+ _("target %1$s doesn't exist."), newDisk->dst);
+ return -1;
+ }
+
+ oldDev.data.disk = vmdef->disks[pos];
+ if (virDomainDefCompatibleDevice(vmdef, dev, &oldDev,
+ VIR_DOMAIN_DEVICE_ACTION_UPDATE,
+ false) < 0)
+ return -1;
+
+ virDomainDiskDefFree(vmdef->disks[pos]);
+ vmdef->disks[pos] = newDisk;
+ dev->data.disk = NULL;
+ break;
+
+ // TODO: support these here once tested.
+ case VIR_DOMAIN_DEVICE_GRAPHICS:
+ case VIR_DOMAIN_DEVICE_NET:
+ case VIR_DOMAIN_DEVICE_MEMORY:
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+ _("persistent update of device '%1$s' is not supported by test driver"),
+ virDomainDeviceTypeToString(dev->type));
+ return -1;
+
+ case VIR_DOMAIN_DEVICE_FS:
+ case VIR_DOMAIN_DEVICE_INPUT:
+ case VIR_DOMAIN_DEVICE_SOUND:
+ case VIR_DOMAIN_DEVICE_VIDEO:
+ case VIR_DOMAIN_DEVICE_WATCHDOG:
+ case VIR_DOMAIN_DEVICE_HUB:
+ case VIR_DOMAIN_DEVICE_SMARTCARD:
+ case VIR_DOMAIN_DEVICE_MEMBALLOON:
+ case VIR_DOMAIN_DEVICE_NVRAM:
+ case VIR_DOMAIN_DEVICE_RNG:
+ case VIR_DOMAIN_DEVICE_SHMEM:
+ case VIR_DOMAIN_DEVICE_LEASE:
+ case VIR_DOMAIN_DEVICE_HOSTDEV:
+ case VIR_DOMAIN_DEVICE_CONTROLLER:
+ case VIR_DOMAIN_DEVICE_REDIRDEV:
+ case VIR_DOMAIN_DEVICE_CHR:
+ case VIR_DOMAIN_DEVICE_NONE:
+ case VIR_DOMAIN_DEVICE_TPM:
+ case VIR_DOMAIN_DEVICE_PANIC:
+ case VIR_DOMAIN_DEVICE_IOMMU:
+ case VIR_DOMAIN_DEVICE_VSOCK:
+ case VIR_DOMAIN_DEVICE_AUDIO:
+ case VIR_DOMAIN_DEVICE_CRYPTO:
+ case VIR_DOMAIN_DEVICE_LAST:
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+ _("persistent update of device '%1$s' is not supported"),
+ virDomainDeviceTypeToString(dev->type));
+ return -1;
+ }
+
+ if (virDomainDefPostParse(vmdef, parse_flags, xmlopt, NULL) < 0)
+ return -1;
+
+ return 0;
+}
+
+
+static int
+testDomainUpdateDeviceFlags(virDomainPtr dom,
+ const char *xml,
+ unsigned int flags)
+{
+ testDriver *driver = dom->conn->privateData;
+ virDomainObj *vm = NULL;
+ g_autoptr(virDomainDef) vmdef = NULL;
+ g_autoptr(virDomainDeviceDef) dev = NULL;
+ int ret = -1;
+ unsigned int parse_flags = 0;
+
+ virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
+ VIR_DOMAIN_AFFECT_CONFIG |
+ VIR_DOMAIN_DEVICE_MODIFY_FORCE, -1);
+
+ if (!(vm = testDomObjFromDomain(dom)))
+ goto cleanup;
+
+ if (virDomainObjBeginJob(vm, VIR_JOB_MODIFY) < 0)
+ goto cleanup;
+
+ if (virDomainObjUpdateModificationImpact(vm, &flags) < 0)
+ goto endjob;
+
+ if ((flags & VIR_DOMAIN_AFFECT_CONFIG) &&
+ !(flags & VIR_DOMAIN_AFFECT_LIVE))
+ parse_flags |= VIR_DOMAIN_DEF_PARSE_INACTIVE;
+
+ if (!(dev = virDomainDeviceDefParse(xml, vm->def, driver->xmlopt,
+ NULL, parse_flags)))
+ goto endjob;
+
+ /* virDomainDefCompatibleDevice call is delayed until we know the
+ * device we're going to update. */
+ if ((ret = testDomainUpdateDeviceConfig(vm->def, dev,
+ parse_flags,
+ driver->xmlopt)) < 0)
+ goto endjob;
+
+ endjob:
+ virDomainObjEndJob(vm);
+
+ cleanup:
+ virDomainObjEndAPI(&vm);
+ return ret;
+}
+
+
/* search for a hostdev matching dev and detach it */
static int
testDomainDetachPrepHostdev(virDomainObj *vm,
@@ -10456,6 +10582,7 @@ static virHypervisorDriver testHypervisorDriver = {
.domainAttachDevice = testDomainAttachDevice, /* 10.0.0 */
.domainAttachDeviceFlags = testDomainAttachDeviceFlags, /* 10.0.0 */
.domainDetachDeviceAlias = testDomainDetachDeviceAlias, /* 10.0.0 */
+ .domainUpdateDeviceFlags = testDomainUpdateDeviceFlags, /* 10.6.0 */
.domainCreateXML = testDomainCreateXML, /* 0.1.4 */
.domainCreateXMLWithFiles = testDomainCreateXMLWithFiles, /* 5.7.0 */
.domainLookupByID = testDomainLookupByID, /* 0.1.1 */
--
2.34.1On 7/1/24 22:29, John Levon wrote:
> Add basic coverage of device update; for now, only support disk updates
> until other types are needed or tested.
>
> Signed-off-by: John Levon <john.levon@nutanix.com>
> ---
> src/test/test_driver.c | 127 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 127 insertions(+)
>
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index 40fb8a467d..213fbdbccb 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -41,6 +41,7 @@
> #include "domain_conf.h"
> #include "domain_driver.h"
> #include "domain_event.h"
> +#include "domain_postparse.h"
> #include "domain_validate.h"
> #include "network_event.h"
> #include "snapshot_conf.h"
> @@ -10237,6 +10238,131 @@ testDomainAttachDevice(virDomainPtr domain, const char *xml)
> }
>
>
> +static int
> +testDomainUpdateDeviceConfig(virDomainDef *vmdef,
> + virDomainDeviceDef *dev,
> + unsigned int parse_flags,
> + virDomainXMLOption *xmlopt)
> +{
> + virDomainDiskDef *newDisk;
> + virDomainDeviceDef oldDev = { .type = dev->type };
> + int pos;
> +
> + switch (dev->type) {
> + case VIR_DOMAIN_DEVICE_DISK:
> + newDisk = dev->data.disk;
> + if ((pos = virDomainDiskIndexByName(vmdef, newDisk->dst, false)) < 0) {
> + virReportError(VIR_ERR_INVALID_ARG,
> + _("target %1$s doesn't exist."), newDisk->dst);
> + return -1;
> + }
> +
> + oldDev.data.disk = vmdef->disks[pos];
> + if (virDomainDefCompatibleDevice(vmdef, dev, &oldDev,
> + VIR_DOMAIN_DEVICE_ACTION_UPDATE,
> + false) < 0)
> + return -1;
> +
> + virDomainDiskDefFree(vmdef->disks[pos]);
> + vmdef->disks[pos] = newDisk;
> + dev->data.disk = NULL;
> + break;
> +
> + // TODO: support these here once tested.
> + case VIR_DOMAIN_DEVICE_GRAPHICS:
> + case VIR_DOMAIN_DEVICE_NET:
> + case VIR_DOMAIN_DEVICE_MEMORY:
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> + _("persistent update of device '%1$s' is not supported by test driver"),
> + virDomainDeviceTypeToString(dev->type));
> + return -1;
> +
It's perfectly okay if ...
> + case VIR_DOMAIN_DEVICE_FS:
> + case VIR_DOMAIN_DEVICE_INPUT:
> + case VIR_DOMAIN_DEVICE_SOUND:
> + case VIR_DOMAIN_DEVICE_VIDEO:
> + case VIR_DOMAIN_DEVICE_WATCHDOG:
> + case VIR_DOMAIN_DEVICE_HUB:
> + case VIR_DOMAIN_DEVICE_SMARTCARD:
> + case VIR_DOMAIN_DEVICE_MEMBALLOON:
> + case VIR_DOMAIN_DEVICE_NVRAM:
> + case VIR_DOMAIN_DEVICE_RNG:
> + case VIR_DOMAIN_DEVICE_SHMEM:
> + case VIR_DOMAIN_DEVICE_LEASE:
> + case VIR_DOMAIN_DEVICE_HOSTDEV:
> + case VIR_DOMAIN_DEVICE_CONTROLLER:
> + case VIR_DOMAIN_DEVICE_REDIRDEV:
> + case VIR_DOMAIN_DEVICE_CHR:
> + case VIR_DOMAIN_DEVICE_NONE:
> + case VIR_DOMAIN_DEVICE_TPM:
> + case VIR_DOMAIN_DEVICE_PANIC:
> + case VIR_DOMAIN_DEVICE_IOMMU:
> + case VIR_DOMAIN_DEVICE_VSOCK:
> + case VIR_DOMAIN_DEVICE_AUDIO:
> + case VIR_DOMAIN_DEVICE_CRYPTO:
> + case VIR_DOMAIN_DEVICE_LAST:
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> + _("persistent update of device '%1$s' is not supported"),
> + virDomainDeviceTypeToString(dev->type));
.. this error is reported instead.
> + return -1;
> + }
> +
> + if (virDomainDefPostParse(vmdef, parse_flags, xmlopt, NULL) < 0)
> + return -1;
> +
> + return 0;
> +}
> +
> +
> +static int
> +testDomainUpdateDeviceFlags(virDomainPtr dom,
> + const char *xml,
> + unsigned int flags)
> +{
> + testDriver *driver = dom->conn->privateData;
> + virDomainObj *vm = NULL;
> + g_autoptr(virDomainDef) vmdef = NULL;
> + g_autoptr(virDomainDeviceDef) dev = NULL;
> + int ret = -1;
> + unsigned int parse_flags = 0;
> +
> + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
> + VIR_DOMAIN_AFFECT_CONFIG |
> + VIR_DOMAIN_DEVICE_MODIFY_FORCE, -1);
LIVE is not supported and thus shouldn't be in list of supported flags.
Neither MODIFY_FORCE.
> +
> + if (!(vm = testDomObjFromDomain(dom)))
> + goto cleanup;
> +
> + if (virDomainObjBeginJob(vm, VIR_JOB_MODIFY) < 0)
> + goto cleanup;
> +
> + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0)
> + goto endjob;
After this, flags may contain LIVE and/or CONFIG. We need to recheck
whether LIVE is present and reject it.
> +
> + if ((flags & VIR_DOMAIN_AFFECT_CONFIG) &&
> + !(flags & VIR_DOMAIN_AFFECT_LIVE))
> + parse_flags |= VIR_DOMAIN_DEF_PARSE_INACTIVE;
> +
> + if (!(dev = virDomainDeviceDefParse(xml, vm->def, driver->xmlopt,
> + NULL, parse_flags)))
> + goto endjob;
> +
> + /* virDomainDefCompatibleDevice call is delayed until we know the
> + * device we're going to update. */
> + if ((ret = testDomainUpdateDeviceConfig(vm->def, dev,
> + parse_flags,
> + driver->xmlopt)) < 0)
> + goto endjob;
An event should be emitted to mimic real hypervisor behavior.
I'm squashing in necessary changes and merging.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Michal
On Tue, Jul 02, 2024 at 04:07:48PM +0200, Michal Prívozník wrote:
> > + // TODO: support these here once tested.
> > + case VIR_DOMAIN_DEVICE_GRAPHICS:
> > + case VIR_DOMAIN_DEVICE_NET:
> > + case VIR_DOMAIN_DEVICE_MEMORY:
> > + virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> > + _("persistent update of device '%1$s' is not supported by test driver"),
> > + virDomainDeviceTypeToString(dev->type));
> > + return -1;
> > +
>
> It's perfectly okay if ...
>
> > + virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> > + _("persistent update of device '%1$s' is not supported"),
> > + virDomainDeviceTypeToString(dev->type));
>
> .. this error is reported instead.
OK; I just thought it might be useful to separate out "we should add this" from
"we don't support this ever".
> > + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
> > + VIR_DOMAIN_AFFECT_CONFIG |
> > + VIR_DOMAIN_DEVICE_MODIFY_FORCE, -1);
>
> LIVE is not supported and thus shouldn't be in list of supported flags.
I was a bit unclear on the test hypervisor case - in a sense everything is both
LIVE *and* CONFIG :)
> I'm squashing in necessary changes and merging.
Thanks!
regards
john
On 7/2/24 16:25, John Levon wrote:
> On Tue, Jul 02, 2024 at 04:07:48PM +0200, Michal Prívozník wrote:
>
>>> + // TODO: support these here once tested.
>>> + case VIR_DOMAIN_DEVICE_GRAPHICS:
>>> + case VIR_DOMAIN_DEVICE_NET:
>>> + case VIR_DOMAIN_DEVICE_MEMORY:
>>> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
>>> + _("persistent update of device '%1$s' is not supported by test driver"),
>>> + virDomainDeviceTypeToString(dev->type));
>>> + return -1;
>>> +
>>
>> It's perfectly okay if ...
>>
>>> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
>>> + _("persistent update of device '%1$s' is not supported"),
>>> + virDomainDeviceTypeToString(dev->type));
>>
>> .. this error is reported instead.
>
> OK; I just thought it might be useful to separate out "we should add this" from
> "we don't support this ever".
I can see us having implementation for virtually all devices.
>
>>> + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
>>> + VIR_DOMAIN_AFFECT_CONFIG |
>>> + VIR_DOMAIN_DEVICE_MODIFY_FORCE, -1);
>>
>> LIVE is not supported and thus shouldn't be in list of supported flags.
>
> I was a bit unclear on the test hypervisor case - in a sense everything is both
> LIVE *and* CONFIG :)
I see what you mean, but no. The fact that libvirt doesn't really store
XMLs on disk for inactive domains doesn't necessarily mean there's no
distinction. The aim of test driver is to mimic real life scenarios as
closely as possible, so that when somebody is developing an app on top
of libvirt they can use the test driver instead of spawning real VMs and
thus test their app quickly. Therefore, the distinction between inactive
and live XMLs must be preserved.
>
>> I'm squashing in necessary changes and merging.
>
> Thanks!
You're welcome.
Michal
On Wed, Jul 03, 2024 at 09:44:47AM +0200, Michal Prívozník wrote:
> >> LIVE is not supported and thus shouldn't be in list of supported flags.
> >
> > I was a bit unclear on the test hypervisor case - in a sense everything is both
> > LIVE *and* CONFIG :)
>
> I see what you mean, but no. The fact that libvirt doesn't really store
> XMLs on disk for inactive domains doesn't necessarily mean there's no
> distinction. The aim of test driver is to mimic real life scenarios as
> closely as possible, so that when somebody is developing an app on top
> of libvirt they can use the test driver instead of spawning real VMs and
> thus test their app quickly. Therefore, the distinction between inactive
> and live XMLs must be preserved.
I get the rationale, but now we're inconsistent with our previous contribution:
10247 static int
10248 testDomainAttachDeviceLiveAndConfig(virDomainObj *vm,
10249 testDriver *driver,
10250 const char *xml,
10251 unsigned int flags)
10252 {
10253 g_autoptr(virDomainDeviceDef) devLive = NULL;
10254 unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
10255 VIR_DOMAIN_DEF_PARSE_ABI_UPDATE;
10256
10257 virCheckFlags(VIR_DOMAIN_AFFECT_LIVE, -1);
Given that the test driver does not and cannot have a persisted configuration
(there is only the in-process ->vmdef), I'm confused why you made the new code
be:
10399 virCheckFlags(VIR_DOMAIN_AFFECT_CONFIG, -1);
in testDomainUpdateDeviceFlags().
For our application we actually do want to pass the LIVE flag - we never want to
modify any persistent configuration. We'd prefer not to have to carry a patch on
top for this.
regards
john
On 7/3/24 18:29, John Levon wrote:
> On Wed, Jul 03, 2024 at 09:44:47AM +0200, Michal Prívozník wrote:
>
>>>> LIVE is not supported and thus shouldn't be in list of supported flags.
>>>
>>> I was a bit unclear on the test hypervisor case - in a sense everything is both
>>> LIVE *and* CONFIG :)
>>
>> I see what you mean, but no. The fact that libvirt doesn't really store
>> XMLs on disk for inactive domains doesn't necessarily mean there's no
>> distinction. The aim of test driver is to mimic real life scenarios as
>> closely as possible, so that when somebody is developing an app on top
>> of libvirt they can use the test driver instead of spawning real VMs and
>> thus test their app quickly. Therefore, the distinction between inactive
>> and live XMLs must be preserved.
>
> I get the rationale, but now we're inconsistent with our previous contribution:
>
> 10247 static int
> 10248 testDomainAttachDeviceLiveAndConfig(virDomainObj *vm,
> 10249 testDriver *driver,
> 10250 const char *xml,
> 10251 unsigned int flags)
> 10252 {
> 10253 g_autoptr(virDomainDeviceDef) devLive = NULL;
> 10254 unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
> 10255 VIR_DOMAIN_DEF_PARSE_ABI_UPDATE;
> 10256
> 10257 virCheckFlags(VIR_DOMAIN_AFFECT_LIVE, -1);
>
> Given that the test driver does not and cannot have a persisted configuration
> (there is only the in-process ->vmdef), I'm confused why you made the new code
> be:
>
> 10399 virCheckFlags(VIR_DOMAIN_AFFECT_CONFIG, -1);
>
> in testDomainUpdateDeviceFlags().
The fact that domain XML is not stored on a disk doesn't necessarily
mean it can't have inactive XML. Those are two different things. Just
consider:
virsh -c test:///default
Welcome to virsh, the virtualization interactive terminal.
Type: 'help' for help with commands
'quit' to quit
virsh # domstate test
running
virsh # destroy test
Domain 'test' destroyed
virsh # domstate test
shut off
And in fact, inactive (sometimes called also config) XML can differ from
that of a running domain. Hence '--inactive' flag to 'dumpxml'. And
since the testDomainUpdateDeviceFlags() is made to error out on
VIR_DOMAIN_AFFECT_LIVE flag it makes no sense to accept the flag.
Therefore it's not listed in virCheckFlags().
There's one caveat - if the API is called with flags=0 then it means
"affect current state of the domain". flgas is then updated in
virDomainObjUpdateModificationImpact() and thus _LIVE needs to be
checked again.
>
> For our application we actually do want to pass the LIVE flag - we never want to
> modify any persistent configuration. We'd prefer not to have to carry a patch on
> top for this.
Ah, I thought you're interested only in _CONFIG since that's what your
original patch implemented. But implementing live update should be
pretty trivial. It's a test driver so "hypervisor" will allow just any
change. Do you want to post a patch for that or should I?
Michal
On Thu, Jul 04, 2024 at 08:24:30AM +0200, Michal Prívozník wrote:
> The fact that domain XML is not stored on a disk doesn't necessarily
> mean it can't have inactive XML. Those are two different things.
I spent a bit of time playing, and the behaviour was more implemented than I
thought for the non-LIVE case; I think I was missing how the vmdefs are
retrieved based upon the flags.
> > For our application we actually do want to pass the LIVE flag - we never want to
> > modify any persistent configuration. We'd prefer not to have to carry a patch on
> > top for this.
>
> Ah, I thought you're interested only in _CONFIG since that's what your
> original patch implemented.
I took a look at my original patch with fresh eyes, and see it was a bit
confused in fact. It does implement LIVE (vm->def) (and I tested that), but
picked the wrong function name from the qemu driver sources:
10427 if ((ret = testDomainUpdateDeviceConfig(vm->def, dev,
^^^^^^ ^^^^^^^
10428 parse_flags,
10429 driver->xmlopt)) < 0)
Obviously that makes no sense!
> But implementing live update should be
> pretty trivial. It's a test driver so "hypervisor" will allow just any
> change. Do you want to post a patch for that or should I?
Now I have a better idea of what was wrong, let me have another pass.
thanks
john
© 2016 - 2024 Red Hat, Inc.