[libvirt] [PATCH] Support transient attribute on vmware disks
From: Wout Mertens <Wout.Mertens@gmail.com> vmx/vmx.c ignores the transient attribute on the disk xml format. This patch adds a 1-1 relationship between it and [disk].mode = "independent-nonpersistent". The other modes are ignored as before. It works in my testing. https://bugzilla.redhat.com/show_bug.cgi?id=1044023 --- src/vmx/vmx.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index 48487f8..4282390 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -1954,6 +1954,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con * busType = VIR_DOMAIN_DISK_BUS_FDC * controllerOrBus = [0] * unit = [0..1] + * */ int result = -1; @@ -1980,6 +1981,9 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con char writeThrough_name[32] = ""; bool writeThrough = false; + char mode_name[32] = ""; + char *mode = NULL; + if (def == NULL || *def != NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); return -1; @@ -2093,6 +2097,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con VMX_BUILD_NAME(fileType); VMX_BUILD_NAME(fileName); VMX_BUILD_NAME(writeThrough); + VMX_BUILD_NAME(mode); /* vmx:present */ if (virVMXGetConfigBoolean(conf, present_name, &present, false, true) < 0) { @@ -2121,6 +2126,11 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con goto cleanup; } + /* vmx:mode -> def:transient */ + if (virVMXGetConfigString(conf, mode_name, &mode, true) < 0) { + goto cleanup; + } + if (clientDevice) { /* * Just ignore devices in client mode, because I have no clue how to @@ -2172,6 +2182,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con (*def)->src = ctx->parseFileName(fileName, ctx->opaque); (*def)->cachemode = writeThrough ? VIR_DOMAIN_DISK_CACHE_WRITETHRU : VIR_DOMAIN_DISK_CACHE_DEFAULT; + (*def)->transient = STRCASEEQ(mode, "independent-nonpersistent") ? true: false; if ((*def)->src == NULL) { goto cleanup; @@ -3497,7 +3508,11 @@ virVMXFormatDisk(virVMXContext *ctx, virDomainDiskDefPtr def, return -1; } } - + + if (def->transient) { + virBufferAsprintf(buffer, "%s%d:%d.mode = \"independent-nonpersistent\"\n", + busType, controllerOrBus, unit); + } return 0; } -- 1.8.2.3
On 12/17/2013 10:04 AM, Wout Mertens wrote:
From: Wout Mertens <Wout.Mertens@gmail.com>
vmx/vmx.c ignores the transient attribute on the disk xml format. This patch adds a 1-1 relationship between it and [disk].mode = "independent-nonpersistent".
The other modes are ignored as before. It works in my testing.
https://bugzilla.redhat.com/show_bug.cgi?id=1044023
--- src/vmx/vmx.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
Congrats on your first libvirt patch. It failed 'make syntax-check': TAB_in_indentation src/vmx/vmx.c:3511: src/vmx/vmx.c:3512: if (def->transient) { src/vmx/vmx.c:3513: virBufferAsprintf(buffer, "%s%d:%d.mode = \"independent-nonpersistent\"\n", src/vmx/vmx.c:3514: busType, controllerOrBus, unit); src/vmx/vmx.c:3515: } maint.mk: indent with space, not TAB, in C, sh, html, py, syms and RNG schemas src/vmx/vmx.c:3511: maint.mk: found trailing blank(s) make: *** [sc_trailing_blank] Error 1 But I don't mind fixing those on a first-time submission.
+++ b/src/vmx/vmx.c @@ -1954,6 +1954,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con * busType = VIR_DOMAIN_DISK_BUS_FDC * controllerOrBus = [0] * unit = [0..1] + * */
int result = -1;
This hunk looks spurious.
@@ -2172,6 +2182,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con (*def)->src = ctx->parseFileName(fileName, ctx->opaque); (*def)->cachemode = writeThrough ? VIR_DOMAIN_DISK_CACHE_WRITETHRU : VIR_DOMAIN_DISK_CACHE_DEFAULT; + (*def)->transient = STRCASEEQ(mode, "independent-nonpersistent") ? true: false;
'bool_expr ? true : false' is a waste of typing. It's just as easy to write 'bool_expr'. Pushed with the following squashed in: diff --git i/src/vmx/vmx.c w/src/vmx/vmx.c index 4282390..c04e397 100644 --- i/src/vmx/vmx.c +++ w/src/vmx/vmx.c @@ -1954,7 +1954,6 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con * busType = VIR_DOMAIN_DISK_BUS_FDC * controllerOrBus = [0] * unit = [0..1] - * */ int result = -1; @@ -2182,7 +2181,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con (*def)->src = ctx->parseFileName(fileName, ctx->opaque); (*def)->cachemode = writeThrough ? VIR_DOMAIN_DISK_CACHE_WRITETHRU : VIR_DOMAIN_DISK_CACHE_DEFAULT; - (*def)->transient = STRCASEEQ(mode, "independent-nonpersistent") ? true: false; + (*def)->transient = STRCASEEQ(mode, "independent-nonpersistent"); if ((*def)->src == NULL) { goto cleanup; @@ -3508,11 +3507,11 @@ virVMXFormatDisk(virVMXContext *ctx, virDomainDiskDefPtr def, return -1; } } - - if (def->transient) { - virBufferAsprintf(buffer, "%s%d:%d.mode = \"independent-nonpersistent\"\n", - busType, controllerOrBus, unit); - } + + if (def->transient) + virBufferAsprintf(buffer, + "%s%d:%d.mode = \"independent-nonpersistent\"\n", + busType, controllerOrBus, unit); return 0; } -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 12/17/2013 01:42 PM, Eric Blake wrote:
On 12/17/2013 10:04 AM, Wout Mertens wrote:
From: Wout Mertens <Wout.Mertens@gmail.com>
vmx/vmx.c ignores the transient attribute on the disk xml format. This patch adds a 1-1 relationship between it and [disk].mode = "independent-nonpersistent".
The other modes are ignored as before. It works in my testing.
https://bugzilla.redhat.com/show_bug.cgi?id=1044023
--- src/vmx/vmx.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
Congrats on your first libvirt patch. It failed 'make syntax-check':
But I don't mind fixing those on a first-time submission.
Oh, I spoke too soon. It also fails 'make check': ../build-aux/test-driver: line 95: 19280 Segmentation fault (core dumped) "$@" > $log_file 2>&1 FAIL: vmx2xmltest Not a good sign. I can still try and fix that before pushing, but for now your patch is not upstream. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 12/17/2013 01:45 PM, Eric Blake wrote:
On 12/17/2013 01:42 PM, Eric Blake wrote:
On 12/17/2013 10:04 AM, Wout Mertens wrote:
From: Wout Mertens <Wout.Mertens@gmail.com>
vmx/vmx.c ignores the transient attribute on the disk xml format. This patch adds a 1-1 relationship between it and [disk].mode = "independent-nonpersistent".
The other modes are ignored as before. It works in my testing.
https://bugzilla.redhat.com/show_bug.cgi?id=1044023
--- src/vmx/vmx.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
Congrats on your first libvirt patch. It failed 'make syntax-check':
But I don't mind fixing those on a first-time submission.
Oh, I spoke too soon. It also fails 'make check':
../build-aux/test-driver: line 95: 19280 Segmentation fault (core dumped) "$@" > $log_file 2>&1 FAIL: vmx2xmltest
Not a good sign. I can still try and fix that before pushing, but for now your patch is not upstream.
Found it. Would you also be willing to do a followup patch to enhance the testsuite to add a case of a vmx file that gets translated into the <transient> libvirt xml? Pushed now, with this added: diff --git i/src/vmx/vmx.c w/src/vmx/vmx.c index c04e397..8fb2a93 100644 --- i/src/vmx/vmx.c +++ w/src/vmx/vmx.c @@ -2181,7 +2181,9 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con (*def)->src = ctx->parseFileName(fileName, ctx->opaque); (*def)->cachemode = writeThrough ? VIR_DOMAIN_DISK_CACHE_WRITETHRU : VIR_DOMAIN_DISK_CACHE_DEFAULT; - (*def)->transient = STRCASEEQ(mode, "independent-nonpersistent"); + if (mode) + (*def)->transient = STRCASEEQ(mode, + "independent-nonpersistent"); if ((*def)->src == NULL) { goto cleanup; @@ -2300,6 +2302,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con VIR_FREE(deviceType); VIR_FREE(fileType); VIR_FREE(fileName); + VIR_FREE(mode); return result; -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Hi Eric, thank you very much for your assistance! I'll take a look at writing that test tomorrow. Wout. On Tue, Dec 17, 2013 at 10:24 PM, Eric Blake <eblake@redhat.com> wrote:
On 12/17/2013 01:42 PM, Eric Blake wrote:
On 12/17/2013 10:04 AM, Wout Mertens wrote:
From: Wout Mertens <Wout.Mertens@gmail.com>
vmx/vmx.c ignores the transient attribute on the disk xml format. This
On 12/17/2013 01:45 PM, Eric Blake wrote: patch
adds a 1-1 relationship between it and [disk].mode = "independent-nonpersistent".
The other modes are ignored as before. It works in my testing.
https://bugzilla.redhat.com/show_bug.cgi?id=1044023
--- src/vmx/vmx.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
Congrats on your first libvirt patch. It failed 'make syntax-check':
But I don't mind fixing those on a first-time submission.
Oh, I spoke too soon. It also fails 'make check':
../build-aux/test-driver: line 95: 19280 Segmentation fault (core dumped) "$@" > $log_file 2>&1 FAIL: vmx2xmltest
Not a good sign. I can still try and fix that before pushing, but for now your patch is not upstream.
Found it. Would you also be willing to do a followup patch to enhance the testsuite to add a case of a vmx file that gets translated into the <transient> libvirt xml? Pushed now, with this added:
diff --git i/src/vmx/vmx.c w/src/vmx/vmx.c index c04e397..8fb2a93 100644 --- i/src/vmx/vmx.c +++ w/src/vmx/vmx.c @@ -2181,7 +2181,9 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con (*def)->src = ctx->parseFileName(fileName, ctx->opaque); (*def)->cachemode = writeThrough ? VIR_DOMAIN_DISK_CACHE_WRITETHRU : VIR_DOMAIN_DISK_CACHE_DEFAULT; - (*def)->transient = STRCASEEQ(mode, "independent-nonpersistent"); + if (mode) + (*def)->transient = STRCASEEQ(mode, + "independent-nonpersistent");
if ((*def)->src == NULL) { goto cleanup; @@ -2300,6 +2302,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con VIR_FREE(deviceType); VIR_FREE(fileType); VIR_FREE(fileName); + VIR_FREE(mode);
return result;
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Hi Eric, haven’t had time for writing the test yet, since I’m trying to get an app out the door that uses libvirt for running “instant clones” of a VM. I ran into a big issue and I’m hoping you or anyone on this list can help: When I create a new domain with a transient disk like this, the .vmx file and all supporting files will be stored in the same directory as the transient disk. One of the supporting files is the “nvram” file and you can only have one, which means you can only have one VM using the same disk, transient or otherwise :-( :-( :-( The code that selects the location of the new .vmx file is vmwareVmxPath in src/vmware/vmware_conf.c and it picks the directory of the first disk Workarounds I can think of: - Augment the domain xml description so it also defines the storage pool on which the created domain should reside - Change vmwareVmxPath so it uses a temporary directory on the first disk’s datastore if the first disk is transient - Somehow encode the .vmx storage location in a device type that isn’t used by vmware. The first way seems the cleanest but is probably also the most work and touches more code. The second way seems hackish but makes sense when you think about it - the disk is transient so the vm definition should be, too. Sadly you can’t specify a different datastore then. The third way is a bit of a middle ground but I wouldn’t know what device to use. Help! :-( Wout. On Dec 17, 2013, at 23:06 , Wout Mertens <Wout.Mertens@gmail.com> wrote:
Hi Eric,
thank you very much for your assistance! I'll take a look at writing that test tomorrow.
Wout.
On Tue, Dec 17, 2013 at 10:24 PM, Eric Blake <eblake@redhat.com> wrote: On 12/17/2013 01:45 PM, Eric Blake wrote:
On 12/17/2013 01:42 PM, Eric Blake wrote:
On 12/17/2013 10:04 AM, Wout Mertens wrote:
From: Wout Mertens <Wout.Mertens@gmail.com>
vmx/vmx.c ignores the transient attribute on the disk xml format. This patch adds a 1-1 relationship between it and [disk].mode = "independent-nonpersistent".
The other modes are ignored as before. It works in my testing.
https://bugzilla.redhat.com/show_bug.cgi?id=1044023
--- src/vmx/vmx.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
Congrats on your first libvirt patch. It failed 'make syntax-check':
But I don't mind fixing those on a first-time submission.
Oh, I spoke too soon. It also fails 'make check':
../build-aux/test-driver: line 95: 19280 Segmentation fault (core dumped) "$@" > $log_file 2>&1 FAIL: vmx2xmltest
Not a good sign. I can still try and fix that before pushing, but for now your patch is not upstream.
Found it. Would you also be willing to do a followup patch to enhance the testsuite to add a case of a vmx file that gets translated into the <transient> libvirt xml? Pushed now, with this added:
diff --git i/src/vmx/vmx.c w/src/vmx/vmx.c index c04e397..8fb2a93 100644 --- i/src/vmx/vmx.c +++ w/src/vmx/vmx.c @@ -2181,7 +2181,9 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con (*def)->src = ctx->parseFileName(fileName, ctx->opaque); (*def)->cachemode = writeThrough ? VIR_DOMAIN_DISK_CACHE_WRITETHRU : VIR_DOMAIN_DISK_CACHE_DEFAULT; - (*def)->transient = STRCASEEQ(mode, "independent-nonpersistent"); + if (mode) + (*def)->transient = STRCASEEQ(mode, + "independent-nonpersistent");
if ((*def)->src == NULL) { goto cleanup; @@ -2300,6 +2302,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con VIR_FREE(deviceType); VIR_FREE(fileType); VIR_FREE(fileName); + VIR_FREE(mode);
return result;
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 12/26/2013 04:02 PM, Wout Mertens wrote:
Workarounds I can think of: - Augment the domain xml description so it also defines the storage pool on which the created domain should reside
This is a good idea which I've wanted for other reasons - the qemu driver also has instances where we are sticking large files in a hard-coded directory (such as 'virsh managedsave') but where people have complained that they would like to control what directory gets used for it. Adding an element to the <domain> xml that gives the name of the preferred virStoragePool for all operations requiring potentially large files associated with the domain is a change worth making.
The first way seems the cleanest but is probably also the most work and touches more code.
Maybe, but it is the right thing to do. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
OK… So what is the next step? I just noticed the <metadata> node (http://libvirt.org/formatdomain.html#elementsMetadata), could that be used for this? E.g. <metadata> <vpx:vmxpath xmlns:vpx=“http://libvirt.org/vmware/“>[foo] bar/bar.vmx</vpx:vmxpath> </metadata> Otherwise it seems to me that this would be part of the general section, perhaps simply a “path” node? Wout. On Dec 27, 2013, at 21:26 , Eric Blake <eblake@redhat.com> wrote:
On 12/26/2013 04:02 PM, Wout Mertens wrote:
Workarounds I can think of: - Augment the domain xml description so it also defines the storage pool on which the created domain should reside
This is a good idea which I've wanted for other reasons - the qemu driver also has instances where we are sticking large files in a hard-coded directory (such as 'virsh managedsave') but where people have complained that they would like to control what directory gets used for it. Adding an element to the <domain> xml that gives the name of the preferred virStoragePool for all operations requiring potentially large files associated with the domain is a change worth making.
The first way seems the cleanest but is probably also the most work and touches more code.
Maybe, but it is the right thing to do.
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 12/27/2013 02:40 PM, Wout Mertens wrote:
OK… So what is the next step?
[please don't top-post on technical lists]
I just noticed the <metadata> node (http://libvirt.org/formatdomain.html#elementsMetadata), could that be used for this? E.g. <metadata> <vpx:vmxpath xmlns:vpx=“http://libvirt.org/vmware/“>[foo] bar/bar.vmx</vpx:vmxpath> </metadata>
No. That would tie your solution to something that is opaque to libvirt.
Otherwise it seems to me that this would be part of the general section, perhaps simply a “path” node?
Not quite a path node. I was thinking more like: <domain type='kvm'> <name>foo</name> <pool name='mypool'/> ... where the new <pool> element says that anything that the 'foo' domain does that requires creation of new files will do so within the already existing <pool> object with the name 'mypool'. We can also support <pool uuid='...'> (or both name and uuid at once). If the <pool> element is missing, that's when the domain uses defaults and/or refuses to do tasks where we don't know where to stick the files. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
(Renamed subject from [PATCH] Support transient attribute on vmware disks) On Dec 27, 2013, at 22:59 , Eric Blake <eblake@redhat.com> wrote:
On 12/27/2013 02:40 PM, Wout Mertens wrote:
Otherwise it seems to me that this would be part of the general section, perhaps simply a “path” node?
Not quite a path node. I was thinking more like:
<domain type='kvm'> <name>foo</name> <pool name='mypool'/> ...
where the new <pool> element says that anything that the 'foo' domain does that requires creation of new files will do so within the already existing <pool> object with the name 'mypool'. We can also support <pool uuid='...'> (or both name and uuid at once). If the <pool> element is missing, that's when the domain uses defaults and/or refuses to do tasks where we don’t know where to stick the files.
Yes, that seems like a nice interface. It does throw away some info (the full path to the .vmx file in the case of VMWare) but that doesn’t really matter very much. I had a look at implementing this but I’m stuck. I don’t know the code very well, I figure there are at least three places this impacts (for vmware): - (1) the virDomainDef definition - (2) the translation from virDomainDef to .vmx file path - (3) (optionally I suppose) the translation from .vmx file to virDomainDef For (1), I added a sub-struct to _virDomainDef in domain_conf.h: struct { char *name; char *uuid; } pool; …thinking this would be easier than managing a pool object pointer and a more direct representation of the domain xml. Is there any change to definition or initialization code etc needed elsewhere? Testing code? For (2): The translation from virDomainDef to path happens in vmware_conf.c:vmwareVmxPath() and there the problem is that regular VMWare uses a full path but ESX uses the ‘[storage] ‘ prefix instead. - I presume there’s somewhere in the pool management code that translates this, any pointers? - vmware_conf.c then needs to pull in the storage_conf.h file, is that a problem? - I propose that if both UUID and name are present, UUID takes precedence over name, but if no device is found then name is tried. I haven’t tackled (3) yet and I suppose it doesn’t really add much, certainly not for my use case. Happy 2014! :) Wout.
On 01/02/2014 02:58 AM, Wout Mertens wrote:
Yes, that seems like a nice interface. It does throw away some info (the full path to the .vmx file in the case of VMWare) but that doesn’t really matter very much.
I had a look at implementing this but I’m stuck. I don’t know the code very well,
That's okay; keep asking questions here or on IRC, and we can try and help guide you. I figure there are at least three places this impacts (for vmware):
- (1) the virDomainDef definition - (2) the translation from virDomainDef to .vmx file path - (3) (optionally I suppose) the translation from .vmx file to virDomainDef
For (1), I added a sub-struct to _virDomainDef in domain_conf.h:
struct { char *name; char *uuid; } pool;
…thinking this would be easier than managing a pool object pointer and a more direct representation of the domain xml. Is there any change to definition or initialization code etc needed elsewhere? Testing code?
Yes, that actually looks like the right way to do it. For adding new XML, you'll also want to touch at least docs/formatdomain.html.in to document it, docs/schemas/domaincommon.rng to let virt-xml-validate accept it, and add something to the testsuite (I often add things in tests/qemuxml2argvdata, but as your implementation is targeting vmware rather than qemu, that doesn't have to be your first choice).
For (2): The translation from virDomainDef to path happens in vmware_conf.c:vmwareVmxPath() and there the problem is that regular VMWare uses a full path but ESX uses the ‘[storage] ‘ prefix instead.
- I presume there’s somewhere in the pool management code that translates this, any pointers?
I'm not as familiar with the vmware storage driver, so we're both learning here.
- vmware_conf.c then needs to pull in the storage_conf.h file, is that a problem?
Should be okay.
- I propose that if both UUID and name are present, UUID takes precedence over name, but if no device is found then name is tried.
If both are present, then the matching pool must match on both attributes.
I haven’t tackled (3) yet and I suppose it doesn’t really add much, certainly not for my use case.
Happy 2014! :)
Wout.
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Wout Mertens