[libvirt] Domain XML format using defined storage volume
From the standpoint: we make it easy for the user I would prefer not to
I have now a 'netapp'-pool, and lun-0..4. They all have a path. <disk type='block'> <source dev='?path?'> <target dev='xvda' bus='xen'/> </disk> Now I wonder, is it possible to have here somethinglike storage://netapp/lun-0? Or should I use the path that comes out of the vol-list? provide a Linux specific path. Stefan
On Thu, 15 May 2008, Stefan de Konink wrote:
Now I wonder, is it possible to have here somethinglike storage://netapp/lun-0?
[snip]
From the standpoint: we make it easy for the user I would prefer not to provide a Linux specific path.
http://libvirt.org/formatdomain.html#elementsDisks I propose an extension to the current <source> tag specifying where a disk should come from. In my humble opinion something that is already available inside libvirt should be reused. Therefore: source If the disk type is "file", then the file attribute specifies the fully-qualified path to the file holding the disk. If the disk type is "block", then the dev attribute specifies the path to the host device to serve as the disk. If the disk type is "pool", then the pool attribute and the volume attibute specify what volume should be used for this disk. I'm currently playing with xml.c to get it supported. One thing that would be interesting is a 'start' parameter, so when the pool is defined but not yet started it should start. Any comments to the idea? Stefan
On Thu, May 15, 2008 at 03:20:06PM +0200, Stefan de Konink wrote:
On Thu, 15 May 2008, Stefan de Konink wrote:
Now I wonder, is it possible to have here somethinglike storage://netapp/lun-0?
[snip]
From the standpoint: we make it easy for the user I would prefer not to provide a Linux specific path.
http://libvirt.org/formatdomain.html#elementsDisks
I propose an extension to the current <source> tag specifying where a disk should come from. In my humble opinion something that is already available inside libvirt should be reused.
Therefore:
source If the disk type is "file", then the file attribute specifies the fully-qualified path to the file holding the disk. If the disk type is "block", then the dev attribute specifies the path to the host device to serve as the disk.
If the disk type is "pool", then the pool attribute and the volume attibute specify what volume should be used for this disk.
I'd support a syntax like this: <disk type="pool"> <source pool="myfiler" vol="lun-4"/> <target dev="xvda"/> </disk> Since this mirrors the concept we use for mapping virtual networks into the domain <interface> element.
I'm currently playing with xml.c to get it supported. One thing that would be interesting is a 'start' parameter, so when the pool is defined but not yet started it should start. Any comments to the idea?
No, lifecycle changes in one object (the domain) should not effect lifecycle changes on another (the storage). We already provide ability for pools to be auto-started at time libvirtd starts. Pools will typically be serving more than one guest anyway, so starting a pool upon starting a domain is a niche use case. Management tools can provide policy such as auto-starting pools in this scenario if desired. Regards, Daniel. -- |: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Thu, 15 May 2008, Daniel P. Berrange wrote:
On Thu, May 15, 2008 at 03:20:06PM +0200, Stefan de Konink wrote:
On Thu, 15 May 2008, Stefan de Konink wrote:
Now I wonder, is it possible to have here somethinglike storage://netapp/lun-0?
[snip]
From the standpoint: we make it easy for the user I would prefer not to provide a Linux specific path.
http://libvirt.org/formatdomain.html#elementsDisks
I propose an extension to the current <source> tag specifying where a disk should come from. In my humble opinion something that is already available inside libvirt should be reused.
Therefore:
source If the disk type is "file", then the file attribute specifies the fully-qualified path to the file holding the disk. If the disk type is "block", then the dev attribute specifies the path to the host device to serve as the disk.
If the disk type is "pool", then the pool attribute and the volume attibute specify what volume should be used for this disk.
I'd support a syntax like this:
<disk type="pool"> <source pool="myfiler" vol="lun-4"/> <target dev="xvda"/> </disk>
I understand your point :) Nevermind, I think you are right, making a CD that is backed by a LUN is non-sence anyway.
I'm currently playing with xml.c to get it supported. One thing that would be interesting is a 'start' parameter, so when the pool is defined but not yet started it should start. Any comments to the idea?
No, lifecycle changes in one object (the domain) should not effect lifecycle changes on another (the storage). We already provide ability for pools to be auto-started at time libvirtd starts. Pools will typically be serving more than one guest anyway, so starting a pool upon starting a domain is a niche use case. Management tools can provide policy such as auto-starting pools in this scenario if desired.
Expect a RFC patch soon ;) Stefan
On Thu, May 15, 2008 at 03:37:14PM +0200, Stefan de Konink wrote:
On Thu, 15 May 2008, Daniel P. Berrange wrote:
On Thu, May 15, 2008 at 03:20:06PM +0200, Stefan de Konink wrote:
On Thu, 15 May 2008, Stefan de Konink wrote:
Now I wonder, is it possible to have here somethinglike storage://netapp/lun-0?
[snip]
From the standpoint: we make it easy for the user I would prefer not to provide a Linux specific path.
http://libvirt.org/formatdomain.html#elementsDisks
I propose an extension to the current <source> tag specifying where a disk should come from. In my humble opinion something that is already available inside libvirt should be reused.
Therefore:
source If the disk type is "file", then the file attribute specifies the fully-qualified path to the file holding the disk. If the disk type is "block", then the dev attribute specifies the path to the host device to serve as the disk.
If the disk type is "pool", then the pool attribute and the volume attibute specify what volume should be used for this disk.
I'd support a syntax like this:
<disk type="pool"> <source pool="myfiler" vol="lun-4"/> <target dev="xvda"/> </disk>
I understand your point :) Nevermind, I think you are right, making a CD that is backed by a LUN is non-sence anyway.
A storage pool is not just SCSI. It can be a directory of ISO files, an NFS mount of ISO files, local device nodes (including CDROM). So it is perfectly possible that we'll use a pool to back a virtual CD device. This is dealt with by simply specifying the 'device='cdrom' attribute. This shouldn't impact the implementation since the choice of backend storage is completely independant of the type of disk device emulated in the guest. For a CD you'd just use: <disk type="pool" device="cdrom"> <source pool="myfiler" vol="lun-4"/> <target dev="hdc"/> </disk> Regards, Daniel. -- |: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Thu, 15 May 2008, Daniel P. Berrange wrote:
A storage pool is not just SCSI. It can be a directory of ISO files, an NFS mount of ISO files, local device nodes (including CDROM). So it is perfectly possible that we'll use a pool to back a virtual CD device. This is dealt with by simply specifying the 'device='cdrom' attribute. This shouldn't impact the implementation since the choice of backend storage is completely independant of the type of disk device emulated in the guest. For a CD you'd just use:
<disk type="pool" device="cdrom"> <source pool="myfiler" vol="lun-4"/> <target dev="hdc"/> </disk>
Nevermind, my reading skills are almost as bad as my coding skills. I mixed up the type and device attributed. But implemented it directly in the type attribute and realised the true meaning of your comment too late. The code I produced is still *untested*. It does compile, but I still did not define any normal domain with libvirt, and if this works I hope my problems are all over. I don't know if you picked up the message about open-iscsi. The libopen-iscsi idea was accepted. So technically we can use their functions directly never having migrate from one sysfs to another, because 'they' fix it. So if the user has a recent version of open-iscsi, they will automatically have the right lookup algorithm. It might be interesting to 'natively' do the scanning too, without invoking the binary. If you want to do this, I'm happy to put all the functions we need in one binary and update their Makefile. The iscsi dependancy should then check for a recent open-iscsi version. Now is this a good idea? For Linux probably... but I hope you can still make friends with Solaris guys. I hope they can provide some native method too :) Sign-off-by: Stefan de Konink <dekonink@kinkrsoftware.nl> Stefan ps. please update the AUTHORS with the above e-mail adress. I try to migrate all my email from 'personal' to 'work'.
On Thu, May 15, 2008 at 03:58:33PM +0200, Stefan de Konink wrote:
On Thu, 15 May 2008, Daniel P. Berrange wrote:
A storage pool is not just SCSI. It can be a directory of ISO files, an NFS mount of ISO files, local device nodes (including CDROM). So it is perfectly possible that we'll use a pool to back a virtual CD device. This is dealt with by simply specifying the 'device='cdrom' attribute. This shouldn't impact the implementation since the choice of backend storage is completely independant of the type of disk device emulated in the guest. For a CD you'd just use:
<disk type="pool" device="cdrom"> <source pool="myfiler" vol="lun-4"/> <target dev="hdc"/> </disk>
Nevermind, my reading skills are almost as bad as my coding skills. I mixed up the type and device attributed. But implemented it directly in the type attribute and realised the true meaning of your comment too late.
The code I produced is still *untested*. It does compile, but I still did not define any normal domain with libvirt, and if this works I hope my problems are all over.
I generally like code dealing with XML parsing to have test cases defined in the tests/qemu*test.c files...
I don't know if you picked up the message about open-iscsi. The libopen-iscsi idea was accepted. So technically we can use their functions directly never having migrate from one sysfs to another, because 'they' fix it. So if the user has a recent version of open-iscsi, they will automatically have the right lookup algorithm.
It might be interesting to 'natively' do the scanning too, without invoking the binary. If you want to do this, I'm happy to put all the functions we need in one binary and update their Makefile. The iscsi dependancy should then check for a recent open-iscsi version.
Now is this a good idea? For Linux probably... but I hope you can still make friends with Solaris guys. I hope they can provide some native method too :)
The code is such that we can trivially have multiple backend drivers for iSCSI if that becomes neccessary. So I've no problem with us providing one that's based on openiscsi, even if its not (yet) supported on Solaris, because we can easily drop in an alternate Solaris iSCSI driver as needed.
Sign-off-by: Stefan de Konink <dekonink@kinkrsoftware.nl>
ps. please update the AUTHORS with the above e-mail adress. I try to migrate all my email from 'personal' to 'work'.
I've changed that. Dan. -- |: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
I almost started crying why it didn't work. But it is fixed, and it works as a charm :) See updated patch! Sign-off-by: Stefan de Konink <dekonink@kinkrsoftware.nl> Stefan ps. One extra suggestion: I would consider it a good thing if the virLog function in util.c would get a \n
On Thu, 15 May 2008, Stefan de Konink wrote:
I almost started crying why it didn't work. But it is fixed, and it works as a charm :) See updated patch!
I guess I need to write the 'check' tool and documentation update too? Stefan
On Thu, 15 May 2008, Stefan de Konink wrote:
On Thu, 15 May 2008, Stefan de Konink wrote:
I almost started crying why it didn't work. But it is fixed, and it works as a charm :) See updated patch!
I guess I need to write the 'check' tool and documentation update too?
Since there is currently no check build for pools in that directory, I'm passing this to someother person. Patch incorporates now a documentation update. Sign-off-by: Stefan de Konink <dekonink@kinkrsoftware.nl> Stefan
And before I forget... If I would write a patch doing: <disk type='iscsi'> <source target='iqn.1986-03.com.sun:02:1fec0625-cde9-4232-e287-eaedf26de136'/> <target dev='sda1'/> </disk> Constistently fetching lun-0, would that be accepted? I see one problem that wasn't there with Xen. Xen knows when a node is gone, you do not. block-iscsi could logout for me, is there currently a management task is called after a destroy? Stefan
Can this be added to cvs/git? Stefan On Fri, 16 May 2008, Stefan de Konink wrote:
On Thu, 15 May 2008, Stefan de Konink wrote:
On Thu, 15 May 2008, Stefan de Konink wrote:
I almost started crying why it didn't work. But it is fixed, and it works as a charm :) See updated patch!
I guess I need to write the 'check' tool and documentation update too?
Since there is currently no check build for pools in that directory, I'm passing this to someother person.
Patch incorporates now a documentation update.
Sign-off-by: Stefan de Konink <dekonink@kinkrsoftware.nl>
Stefan
On Fri, May 16, 2008 at 01:00:16AM +0200, Stefan de Konink wrote:
On Thu, 15 May 2008, Stefan de Konink wrote:
On Thu, 15 May 2008, Stefan de Konink wrote:
I almost started crying why it didn't work. But it is fixed, and it works as a charm :) See updated patch!
I guess I need to write the 'check' tool and documentation update too?
Since there is currently no check build for pools in that directory, I'm passing this to someother person.
Yes, testing the interaction with pools is actually harder that I anticipated, because the libvirtd isn't available in context of the test suite. I'll have to think about best way todo it. Might be able to stub out a dummy impl for purposes of testing.
@@ -1309,11 +1311,24 @@ virDomainParseXMLDiskDesc(virConnectPtr conn, xmlNodePtr node, if (cur->type == XML_ELEMENT_NODE) { if ((source == NULL) && (xmlStrEqual(cur->name, BAD_CAST "source"))) { - if (typ == 0) source = xmlGetProp(cur, BAD_CAST "file"); - else + else if (typ == 1) source = xmlGetProp(cur, BAD_CAST "dev"); + else if (typ == 2) { + xmlChar *pool = xmlGetProp(cur, BAD_CAST "pool"); + xmlChar *volume = xmlGetProp(cur, BAD_CAST "volume"); + if (pool != NULL && volume != NULL) { + virStoragePoolPtr virPool; + virPool = virStoragePoolLookupByName(conn, (const char *) pool); + if (virPool != NULL) { + virStorageVolPtr virVol; + virVol = virStorageVolLookupByName(virPool, (const char *) volume); + if (virVol != NULL) + source = BAD_CAST virStorageVolGetPath(virVol);
This is leaking the pool and volume objects.
+ } + } + } } else if ((target == NULL) && (xmlStrEqual(cur->name, BAD_CAST "target"))) { target = xmlGetProp(cur, BAD_CAST "dev"); @@ -1411,7 +1426,8 @@ virDomainParseXMLDiskDesc(virConnectPtr conn, xmlNodePtr node, virBufferVSprintf(buf, "(uname 'phy:%s')", source); else virBufferVSprintf(buf, "(uname 'phy:/dev/%s')", source); - } + } else if (typ == 2) + virBufferVSprintf(buf, "(uname 'phy:%s')", source);
This is leaking the 'source' string, and a volume can be either a file or a physical device, so fixing it to be 'phy:' is not correct. We also need to apply the reverse mapping when generating the XML. eg do a virStorageVolLookupByPath() to discover the volume/pool. And really need todo the same for the QEMU driver Regards, Daniel. -- |: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Daniel P. Berrange schreef:
On Fri, May 16, 2008 at 01:00:16AM +0200, Stefan de Konink wrote:
On Thu, 15 May 2008, Stefan de Konink wrote:
On Thu, 15 May 2008, Stefan de Konink wrote:
I almost started crying why it didn't work. But it is fixed, and it works as a charm :) See updated patch! I guess I need to write the 'check' tool and documentation update too? Since there is currently no check build for pools in that directory, I'm passing this to someother person.
Yes, testing the interaction with pools is actually harder that I anticipated, because the libvirtd isn't available in context of the test suite. I'll have to think about best way todo it. Might be able to stub out a dummy impl for purposes of testing.
Ok.
@@ -1309,11 +1311,24 @@ virDomainParseXMLDiskDesc(virConnectPtr conn, xmlNodePtr node, if (cur->type == XML_ELEMENT_NODE) { if ((source == NULL) && (xmlStrEqual(cur->name, BAD_CAST "source"))) { - if (typ == 0) source = xmlGetProp(cur, BAD_CAST "file"); - else + else if (typ == 1) source = xmlGetProp(cur, BAD_CAST "dev"); + else if (typ == 2) { + xmlChar *pool = xmlGetProp(cur, BAD_CAST "pool"); + xmlChar *volume = xmlGetProp(cur, BAD_CAST "volume"); + if (pool != NULL && volume != NULL) { + virStoragePoolPtr virPool; + virPool = virStoragePoolLookupByName(conn, (const char *) pool); + if (virPool != NULL) { + virStorageVolPtr virVol; + virVol = virStorageVolLookupByName(virPool, (const char *) volume); + if (virVol != NULL) + source = BAD_CAST virStorageVolGetPath(virVol);
This is leaking the pool and volume objects.
Ok. Appended xmlFree's. http://kinkrsoftware.nl/contrib/libvirt/pool.patch
+ } + } + } } else if ((target == NULL) && (xmlStrEqual(cur->name, BAD_CAST "target"))) { target = xmlGetProp(cur, BAD_CAST "dev"); @@ -1411,7 +1426,8 @@ virDomainParseXMLDiskDesc(virConnectPtr conn, xmlNodePtr node, virBufferVSprintf(buf, "(uname 'phy:%s')", source); else virBufferVSprintf(buf, "(uname 'phy:/dev/%s')", source); - } + } else if (typ == 2) + virBufferVSprintf(buf, "(uname 'phy:%s')", source);
This is leaking the 'source' string,
It is not leaking imho, since 'down under' there is an xmlFree(source).
and a volume can be either a file or a physical device, so fixing it to be 'phy:' is not correct.
How can we know if the volume is a file or a device?
We also need to apply the reverse mapping when generating the XML. eg do a virStorageVolLookupByPath() to discover the volume/pool.
Mmm... that sounds not trivial... (I mean duplicate wise). Stefan
On Mon, May 19, 2008 at 11:53:07PM +0200, Stefan de Konink wrote:
Daniel P. Berrange schreef:
On Fri, May 16, 2008 at 01:00:16AM +0200, Stefan de Konink wrote:
+ } + } + } } else if ((target == NULL) && (xmlStrEqual(cur->name, BAD_CAST "target"))) { target = xmlGetProp(cur, BAD_CAST "dev"); @@ -1411,7 +1426,8 @@ virDomainParseXMLDiskDesc(virConnectPtr conn, xmlNodePtr node, virBufferVSprintf(buf, "(uname 'phy:%s')", source); else virBufferVSprintf(buf, "(uname 'phy:/dev/%s')", source); - } + } else if (typ == 2) + virBufferVSprintf(buf, "(uname 'phy:%s')", source);
This is leaking the 'source' string,
It is not leaking imho, since 'down under' there is an xmlFree(source).
Oh yes, you are correct.
and a volume can be either a file or a physical device, so fixing it to be 'phy:' is not correct.
How can we know if the volume is a file or a device?
virStorageVolGetInfo() will tell you via the 'info' field of the struct it fills
We also need to apply the reverse mapping when generating the XML. eg do a virStorageVolLookupByPath() to discover the volume/pool.
Mmm... that sounds not trivial... (I mean duplicate wise).
This is true - until we merge all the XML processing for drivers together there will be a non-trivial bit of duplication Dan. -- |: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Daniel P. Berrange schreef:
and a volume can be either a file or a physical device, so fixing it to be 'phy:' is not correct. How can we know if the volume is a file or a device?
virStorageVolGetInfo() will tell you via the 'info' field of the struct it fills
As attached. I updated the 'typ' option.
We also need to apply the reverse mapping when generating the XML. eg do a virStorageVolLookupByPath() to discover the volume/pool. Mmm... that sounds not trivial... (I mean duplicate wise).
This is true - until we merge all the XML processing for drivers together there will be a non-trivial bit of duplication
Not done yet... and I prefer the last thing to be a different patch. Stefan diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 02ca509..8e2a7b6 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -344,7 +344,7 @@ <dl> <dt><code>disk</code></dt> <dd>The <code>disk</code> element is the main container for describing - disks. The <code>type</code> attribute is either "file" or "block" + disks. The <code>type</code> attribute is either "file", "block" or "pool" and refers to the underlying source for the disk. The optional <code>device</code> attribute indicates how the disk is to be exposed to the guest OS. Possible values for this attribute are "floppy", "disk" @@ -354,7 +354,11 @@ <dd>If the disk <code>type</code> is "file", then the <code>file</code> attribute specifies the fully-qualified path to the file holding the disk. If the disk <code>type</code> is "block", then the <code>dev</code> attribute specifies - the path to the host device to serve as the disk. <span class="since">Since 0.0.3</span></dd> + the path to the host device to serve as the disk. If the disk <code>type</code> + is "pool", then the <code>pool</code> and <code>volume</code> specify the + virtual location of the disk, the path is automatically resolved if the pool + and the volume exist. + <span class="since">Since 0.0.3</span></dd> <dt><code>target</code></dt> <dd>The <code>target</code> element controls the bus / device under which the disk is exposed to the guest OS. The <code>dev</code> attribute indicates diff --git a/src/xml.c b/src/xml.c index 22dc211..598bfb3 100644 --- a/src/xml.c +++ b/src/xml.c @@ -1300,6 +1300,8 @@ virDomainParseXMLDiskDesc(virConnectPtr conn, xmlNodePtr node, typ = 0; else if (xmlStrEqual(type, BAD_CAST "block")) typ = 1; + else if (xmlStrEqual(type, BAD_CAST "pool")) + typ = 2; xmlFree(type); } device = xmlGetProp(node, BAD_CAST "device"); @@ -1309,11 +1311,32 @@ virDomainParseXMLDiskDesc(virConnectPtr conn, xmlNodePtr node, if (cur->type == XML_ELEMENT_NODE) { if ((source == NULL) && (xmlStrEqual(cur->name, BAD_CAST "source"))) { - if (typ == 0) source = xmlGetProp(cur, BAD_CAST "file"); - else + else if (typ == 1) source = xmlGetProp(cur, BAD_CAST "dev"); + else if (typ == 2) { + xmlChar *pool = xmlGetProp(cur, BAD_CAST "pool"); + xmlChar *volume = xmlGetProp(cur, BAD_CAST "volume"); + if (pool != NULL && volume != NULL) { + virStoragePoolPtr virPool; + virPool = virStoragePoolLookupByName(conn, (const char *) pool); + if (virPool != NULL) { + virStorageVolPtr virVol; + virVol = virStorageVolLookupByName(virPool, (const char *) volume); + if (virVol != NULL) { + virStorageVolInfo info; + if (virStorageVolGetInfo(virVol, &info) == 0) { + source = BAD_CAST virStorageVolGetPath(virVol); + /* We set the type to the 'native' type */ + typ = info.type; + } + } + } + } + xmlFree(pool); + xmlFree(volume); + } } else if ((target == NULL) && (xmlStrEqual(cur->name, BAD_CAST "target"))) { target = xmlGetProp(cur, BAD_CAST "dev"); @@ -1411,7 +1434,7 @@ virDomainParseXMLDiskDesc(virConnectPtr conn, xmlNodePtr node, virBufferVSprintf(buf, "(uname 'phy:%s')", source); else virBufferVSprintf(buf, "(uname 'phy:/dev/%s')", source); - } + } } if (ro == 1) virBufferAddLit(buf, "(mode 'r')");
participants (2)
-
Daniel P. Berrange -
Stefan de Konink