[libvirt] [PATCHv7 0/2] Implement RBD storage pool support
Here is a re-based and fixed up re-submission of my patches to implement RBD storage pool support for QEMU domains. Nothing in it has changed much, other than it has been rebased against the lastest libvirt and two of my patches have been squashed in order to pass a make, make check, and make syntax-check after each individual patch. The code here still works well for me, but I only have the one host to test this on. If possible, I would like to try and get this merged for the 1.2.2 release cycle, so that other users can benefit from the added storage pool support. As always, if you find any problems with my patches, please let me know, and I will fix them up as soon as time permits. Adam Walters (2): qemu: conf Implement domain RBD storage pool support domain: conf: Fix secret type checking for network volumes src/conf/domain_conf.c | 6 +++-- src/qemu/qemu_conf.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 65 insertions(+), 3 deletions(-) -- 1.8.5.2
This patch adds a helper function, qemuAddRBDPoolSourceHost, and implements the usage of this function to allow RBD storage pools in QEMU domain XML. The new function grabs RBD monitor hosts from the storage pool definition and applies them to the domain's disk definition at runtime. This function is used by my modifications to qemuTranslateDiskSourcePool similar to the function used by the iSCSI code. My modifications to qemuTranslateDiskSourcePool is based heavily on the existing iSCSI code, but modified to support RBD install. It will place all relevant information into the domain's disk definition at runtime to allow access to a RBD storage pool. Signed-off-by: Adam Walters <adam@pandorasboxen.com> --- src/qemu/qemu_conf.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 61 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index ac53f6d..b1a6bfe 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1247,6 +1247,45 @@ cleanup: } static int +qemuAddRBDPoolSourceHost(virDomainDiskDefPtr def, + virStoragePoolDefPtr pooldef) +{ + int ret = -1; + size_t i = 0; + char **tokens = NULL; + + def->nhosts = pooldef->source.nhost; + + if (VIR_ALLOC_N(def->hosts, def->nhosts) < 0) + goto cleanup; + + for (i = 0; i < def->nhosts; i++) { + if (VIR_STRDUP(def->hosts[i].name, pooldef->source.hosts[i].name) < 0) + goto cleanup; + + if (virAsprintf(&def->hosts[i].port, "%d", + pooldef->source.hosts[i].port ? + pooldef->source.hosts[i].port : + 6789) < 0) + goto cleanup; + + /* Storage pools have not supported these 2 attributes yet, + * use the defaults. + */ + def->hosts[i].transport = VIR_DOMAIN_DISK_PROTO_TRANS_TCP; + def->hosts[i].socket = NULL; + } + + def->protocol = VIR_DOMAIN_DISK_PROTOCOL_RBD; + + ret = 0; + +cleanup: + virStringFreeList(tokens); + return ret; +} + +static int qemuTranslateDiskSourcePoolAuth(virDomainDiskDefPtr def, virStoragePoolDefPtr pooldef) { @@ -1439,8 +1478,29 @@ qemuTranslateDiskSourcePool(virConnectPtr conn, } break; - case VIR_STORAGE_POOL_MPATH: case VIR_STORAGE_POOL_RBD: + if (def->startupPolicy) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("'startupPolicy' is only valid for " + "'file' file volume")); + goto cleanup; + } + + def->srcpool->actualtype = VIR_DOMAIN_DISK_TYPE_NETWORK; + def->protocol = VIR_DOMAIN_DISK_PROTOCOL_RBD; + + if (!(def->src = virStorageVolGetPath(vol))) + goto cleanup; + + if (qemuTranslateDiskSourcePoolAuth(def, pooldef) < 0) + goto cleanup; + + if (qemuAddRBDPoolSourceHost(def, pooldef) < 0) + goto cleanup; + + break; + + case VIR_STORAGE_POOL_MPATH: case VIR_STORAGE_POOL_SHEEPDOG: case VIR_STORAGE_POOL_GLUSTER: case VIR_STORAGE_POOL_LAST: -- 1.8.5.2
On 01/23/2014 08:45 PM, Adam Walters wrote:
This patch adds a helper function, qemuAddRBDPoolSourceHost, and implements the usage of this function to allow RBD storage pools in QEMU domain XML.
The new function grabs RBD monitor hosts from the storage pool definition and applies them to the domain's disk definition at runtime. This function is used by my modifications to qemuTranslateDiskSourcePool similar to the function used by the iSCSI code.
My modifications to qemuTranslateDiskSourcePool is based heavily on the existing iSCSI code, but modified to support RBD install. It will place all relevant information into the domain's disk definition at runtime to allow access to a RBD storage pool.
Signed-off-by: Adam Walters <adam@pandorasboxen.com> --- src/qemu/qemu_conf.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 61 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index ac53f6d..b1a6bfe 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1439,8 +1478,29 @@ qemuTranslateDiskSourcePool(virConnectPtr conn, } break;
- case VIR_STORAGE_POOL_MPATH: case VIR_STORAGE_POOL_RBD: + if (def->startupPolicy) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("'startupPolicy' is only valid for " + "'file' file volume")); + goto cleanup; + }
You should also check if srcpool->mode is DIRECT and document that it's valid for rbd pools in docs/formatdomain.html.in in the description of the mode attribute. You could also extend the qemuxml2argvtest to check the generated QEMU command line (as we do for iscsi in qemuxml2argv-disk-source-pool-mode test).
+ + def->srcpool->actualtype = VIR_DOMAIN_DISK_TYPE_NETWORK; + def->protocol = VIR_DOMAIN_DISK_PROTOCOL_RBD; + + if (!(def->src = virStorageVolGetPath(vol))) + goto cleanup; + + if (qemuTranslateDiskSourcePoolAuth(def, pooldef) < 0) + goto cleanup; + + if (qemuAddRBDPoolSourceHost(def, pooldef) < 0) + goto cleanup; + + break; + + case VIR_STORAGE_POOL_MPATH: case VIR_STORAGE_POOL_SHEEPDOG: case VIR_STORAGE_POOL_GLUSTER: case VIR_STORAGE_POOL_LAST:
Jan
On Thu, Jan 30, 2014 at 9:46 AM, Ján Tomko <jtomko@redhat.com> wrote:
On 01/23/2014 08:45 PM, Adam Walters wrote:
This patch adds a helper function, qemuAddRBDPoolSourceHost, and implements the usage of this function to allow RBD storage pools in QEMU domain XML.
The new function grabs RBD monitor hosts from the storage pool definition and applies them to the domain's disk definition at runtime. This function is used by my modifications to qemuTranslateDiskSourcePool similar to the function used by the iSCSI code.
My modifications to qemuTranslateDiskSourcePool is based heavily on the existing iSCSI code, but modified to support RBD install. It will place all relevant information into the domain's disk definition at runtime to allow access to a RBD storage pool.
Signed-off-by: Adam Walters <adam@pandorasboxen.com> --- src/qemu/qemu_conf.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 61 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index ac53f6d..b1a6bfe 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1439,8 +1478,29 @@ qemuTranslateDiskSourcePool(virConnectPtr conn, } break;
- case VIR_STORAGE_POOL_MPATH: case VIR_STORAGE_POOL_RBD: + if (def->startupPolicy) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("'startupPolicy' is only valid for " + "'file' file volume")); + goto cleanup; + }
You should also check if srcpool->mode is DIRECT and document that it's valid for rbd pools in docs/formatdomain.html.in in the description of the mode attribute.
One of my early versions of this patch actually did check srcpool->mode (and set it to DIRECT if it wasn't set yet), but it was requested that I remove that check since there was only one mode for RBD pools. The removal of that code is what prompted my modification to the secret type checking in another patch in this series. So which is the correct method of implementation? Should pools that only have one access mode check for srcpool->mode to be set (and set a default if there isn't a setting)? My original thoughts were that the check would allow for a HOST access mode in the future, which might allow use of the kernel rbd module (which creates block devices) while also knowing the volume was served by Ceph.
You could also extend the qemuxml2argvtest to check the generated QEMU command line (as we do for iscsi in qemuxml2argv-disk-source-pool-mode test).
I will certainly look into doing this.
+ + def->srcpool->actualtype = VIR_DOMAIN_DISK_TYPE_NETWORK; + def->protocol = VIR_DOMAIN_DISK_PROTOCOL_RBD; + + if (!(def->src = virStorageVolGetPath(vol))) + goto cleanup; + + if (qemuTranslateDiskSourcePoolAuth(def, pooldef) < 0) + goto cleanup; + + if (qemuAddRBDPoolSourceHost(def, pooldef) < 0) + goto cleanup; + + break; + + case VIR_STORAGE_POOL_MPATH: case VIR_STORAGE_POOL_SHEEPDOG: case VIR_STORAGE_POOL_GLUSTER: case VIR_STORAGE_POOL_LAST:
Jan
On 01/30/2014 06:06 PM, Adam Walters wrote:
On Thu, Jan 30, 2014 at 9:46 AM, Ján Tomko <jtomko@redhat.com <mailto:jtomko@redhat.com>> wrote:
On 01/23/2014 08:45 PM, Adam Walters wrote: > This patch adds a helper function, qemuAddRBDPoolSourceHost, and > implements the usage of this function to allow RBD storage pools in QEMU > domain XML. > > The new function grabs RBD monitor hosts from the storage pool > definition and applies them to the domain's disk definition at runtime. > This function is used by my modifications to qemuTranslateDiskSourcePool > similar to the function used by the iSCSI code. > > My modifications to qemuTranslateDiskSourcePool is based heavily on the > existing iSCSI code, but modified to support RBD install. It will place > all relevant information into the domain's disk definition at runtime to > allow access to a RBD storage pool. > > Signed-off-by: Adam Walters <adam@pandorasboxen.com <mailto:adam@pandorasboxen.com>> > --- > src/qemu/qemu_conf.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 61 insertions(+), 1 deletion(-) > > diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c > index ac53f6d..b1a6bfe 100644 > --- a/src/qemu/qemu_conf.c > +++ b/src/qemu/qemu_conf.c > @@ -1439,8 +1478,29 @@ qemuTranslateDiskSourcePool(virConnectPtr conn, > } > break; > > - case VIR_STORAGE_POOL_MPATH: > case VIR_STORAGE_POOL_RBD: > + if (def->startupPolicy) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("'startupPolicy' is only valid for " > + "'file' file volume")); > + goto cleanup; > + }
You should also check if srcpool->mode is DIRECT and document that it's valid for rbd pools in docs/formatdomain.html.in <http://formatdomain.html.in> in the description of the mode attribute.
One of my early versions of this patch actually did check srcpool->mode (and set it to DIRECT if it wasn't set yet), but it was requested that I remove that check since there was only one mode for RBD pools.
Right, I didn't notice it was already checked above the switch: if (def->srcpool->mode && pooldef->type != VIR_STORAGE_POOL_ISCSI) { virReportError(VIR_ERR_XML_ERROR, "%s", _("disk source mode is only valid when " "storage pool is of iscsi type")); goto cleanup; }
The removal of that code is what prompted my modification to the secret type checking in another patch in this series. So which is the correct method of implementation? Should pools that only have one access mode check for srcpool->mode to be set (and set a default if there isn't a setting)?
If there is only one access mode, there is no need to specify it and no need for a check.
My original thoughts were that the check would allow for a HOST access mode in the future, which might allow use of the kernel rbd module (which creates block devices) while also knowing the volume was served by Ceph.
If that happens, we can allow MODE_DIRECT and MODE_HOST too, along with MODE_DEFAULT. Jan
This patch fixes the secret type checking done in the virDomainDiskDefParseXML function. Previously, it would not allow any volumes that utilized a secret. This patch is a simple bypass of the checking code for volumes. Signed-off-by: Adam Walters <adam@pandorasboxen.com> --- src/conf/domain_conf.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 28e24f9..773dc26 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5418,7 +5418,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, cur = cur->next; } - if (auth_secret_usage != -1 && auth_secret_usage != expected_secret_usage) { + if (auth_secret_usage != -1 && auth_secret_usage != expected_secret_usage && + def->type != VIR_DOMAIN_DISK_TYPE_VOLUME) { virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid secret type '%s'"), virSecretUsageTypeTypeToString(auth_secret_usage)); @@ -18214,7 +18215,8 @@ virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, if (!disk->src || disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK || (disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME && disk->srcpool && - disk->srcpool->mode == VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DIRECT)) + (disk->srcpool->mode == VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DIRECT || + disk->srcpool->mode == VIR_DOMAIN_DISK_TYPE_NETWORK))) return 0; if (iter(disk, disk->src, 0, opaque) < 0) -- 1.8.5.2
On 01/23/2014 08:45 PM, Adam Walters wrote:
This patch fixes the secret type checking done in the virDomainDiskDefParseXML function. Previously, it would not allow any volumes that utilized a secret. This patch is a simple bypass of the checking code for volumes.
Signed-off-by: Adam Walters <adam@pandorasboxen.com> --- src/conf/domain_conf.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 28e24f9..773dc26 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5418,7 +5418,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, cur = cur->next; }
- if (auth_secret_usage != -1 && auth_secret_usage != expected_secret_usage) { + if (auth_secret_usage != -1 && auth_secret_usage != expected_secret_usage && + def->type != VIR_DOMAIN_DISK_TYPE_VOLUME) { virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid secret type '%s'"), virSecretUsageTypeTypeToString(auth_secret_usage));
So an rbd volume can have a secret when the pool has auth set to none? Otherwise it seems the volume's secret data might get overwritten by qemuTranslateDiskSourcePoolAuth. And this could also be added to qemuxml2argvtest.
@@ -18214,7 +18215,8 @@ virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, if (!disk->src || disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK || (disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME && disk->srcpool && - disk->srcpool->mode == VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DIRECT)) + (disk->srcpool->mode == VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DIRECT || + disk->srcpool->mode == VIR_DOMAIN_DISK_TYPE_NETWORK)))
What is the purpose of this? You are comparing the source pool mode against a disk type constant. It seems this can never be true in this case. Jan
On Thu, Jan 30, 2014 at 9:46 AM, Ján Tomko <jtomko@redhat.com> wrote:
On 01/23/2014 08:45 PM, Adam Walters wrote:
This patch fixes the secret type checking done in the virDomainDiskDefParseXML function. Previously, it would not allow any volumes that utilized a secret. This patch is a simple bypass of the checking code for volumes.
Signed-off-by: Adam Walters <adam@pandorasboxen.com> --- src/conf/domain_conf.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 28e24f9..773dc26 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5418,7 +5418,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, cur = cur->next; }
- if (auth_secret_usage != -1 && auth_secret_usage != expected_secret_usage) { + if (auth_secret_usage != -1 && auth_secret_usage != expected_secret_usage && + def->type != VIR_DOMAIN_DISK_TYPE_VOLUME) { virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid secret type '%s'"),
virSecretUsageTypeTypeToString(auth_secret_usage));
So an rbd volume can have a secret when the pool has auth set to none? Otherwise it seems the volume's secret data might get overwritten by qemuTranslateDiskSourcePoolAuth.
The purpose of this is to bypass the secret type checking for volumes (not just RBD volumes). Above this section of code, but in the same function, there is some code that populates the expected_secret_usage variable. Looking back on it now, I think I may have an alternative solution. I think I might be able to set expected_secret_usage properly by referencing def->srcpool->pooltype above this to check it like is done for non-storage pool RBD disks. Without this particular patch hunk, any RBD volume used would throw an error in the logs: "invalid secret type 'ceph'". If you didn't use a storage pool, but defined the RBD disk in the domain XML directly with the secret instead, it worked just fine.
And this could also be added to qemuxml2argvtest.
I'll look into adding this into qemuxml2argvtest, as well.
@@ -18214,7 +18215,8 @@ virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, if (!disk->src || disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK || (disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME && disk->srcpool && - disk->srcpool->mode == VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DIRECT)) + (disk->srcpool->mode == VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DIRECT || + disk->srcpool->mode == VIR_DOMAIN_DISK_TYPE_NETWORK)))
What is the purpose of this? You are comparing the source pool mode against a disk type constant. It seems this can never be true in this case.
That was a typo. The second line should be disk->srcpool->actualtype == VIR_DOMAIN_DISK_TYPE_NETWORK))). I'll fix and submit a new version.
Jan
On 01/30/2014 07:57 PM, Adam Walters wrote:
On Thu, Jan 30, 2014 at 9:46 AM, Ján Tomko <jtomko@redhat.com <mailto:jtomko@redhat.com>> wrote:
On 01/23/2014 08:45 PM, Adam Walters wrote: > This patch fixes the secret type checking done in the > virDomainDiskDefParseXML function. Previously, it would not allow any > volumes that utilized a secret. This patch is a simple bypass of the > checking code for volumes. > > Signed-off-by: Adam Walters <adam@pandorasboxen.com <mailto:adam@pandorasboxen.com>> > --- > src/conf/domain_conf.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 28e24f9..773dc26 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -5418,7 +5418,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, > cur = cur->next; > } > > - if (auth_secret_usage != -1 && auth_secret_usage != expected_secret_usage) { > + if (auth_secret_usage != -1 && auth_secret_usage != expected_secret_usage && > + def->type != VIR_DOMAIN_DISK_TYPE_VOLUME) { > virReportError(VIR_ERR_INTERNAL_ERROR, > _("invalid secret type '%s'"), > virSecretUsageTypeTypeToString(auth_secret_usage));
So an rbd volume can have a secret when the pool has auth set to none? Otherwise it seems the volume's secret data might get overwritten by qemuTranslateDiskSourcePoolAuth.
The purpose of this is to bypass the secret type checking for volumes (not just RBD volumes).
This check is blocking it for iscsi volumes too, even though we have it as an example in formatdomain.html
Above this section of code, but in the same function, there is some code that populates the expected_secret_usage variable. Looking back on it now, I think I may have an alternative solution. I think I might be able to set expected_secret_usage properly by referencing def->srcpool->pooltype above this to check it like is done for non-storage pool RBD disks.
Volume translation is done on domain start-up. This function doing the checking is done on domain definition and the pool might not yet be available. Jan
participants (2)
-
Adam Walters -
Ján Tomko