[libvirt] [PATCH] scsi: Need to translate disk source pool in config attach path
https://bugzilla.redhat.com/show_bug.cgi?id=1228007 When attaching a scsi volume lun via the attach-device --config or --persistent options, there was no translation of the source pool like there was for the live path, thus the attempt to modify the config would fail since not enough was known about the disk. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 34e5581..6bb8549 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8016,7 +8016,8 @@ qemuDomainUpdateDeviceLive(virConnectPtr conn, static int qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, virDomainDefPtr vmdef, - virDomainDeviceDefPtr dev) + virDomainDeviceDefPtr dev, + virConnectPtr conn) { virDomainDiskDefPtr disk; virDomainNetDefPtr net; @@ -8033,6 +8034,8 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, _("target %s already exists"), disk->dst); return -1; } + if (virStorageTranslateDiskSourcePool(conn, disk) < 0) + return -1; if (qemuCheckDiskConfig(disk) < 0) return -1; if (virDomainDiskInsert(vmdef, disk)) @@ -8501,7 +8504,8 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, VIR_DOMAIN_DEVICE_ACTION_ATTACH) < 0) goto endjob; - if ((ret = qemuDomainAttachDeviceConfig(qemuCaps, vmdef, dev)) < 0) + if ((ret = qemuDomainAttachDeviceConfig(qemuCaps, vmdef, dev, + dom->conn)) < 0) goto endjob; } -- 2.1.0
On 06/11/2015 11:15 PM, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1228007
When attaching a scsi volume lun via the attach-device --config or --persistent options, there was no translation of the source pool like there was for the live path, thus the attempt to modify the config would fail since not enough was known about the disk.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 34e5581..6bb8549 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8016,7 +8016,8 @@ qemuDomainUpdateDeviceLive(virConnectPtr conn, static int qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, virDomainDefPtr vmdef, - virDomainDeviceDefPtr dev) + virDomainDeviceDefPtr dev, + virConnectPtr conn) { virDomainDiskDefPtr disk; virDomainNetDefPtr net; @@ -8033,6 +8034,8 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, _("target %s already exists"), disk->dst); return -1; } + if (virStorageTranslateDiskSourcePool(conn, disk) < 0) + return -1; if (qemuCheckDiskConfig(disk) < 0) return -1; if (virDomainDiskInsert(vmdef, disk)) @@ -8501,7 +8504,8 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, VIR_DOMAIN_DEVICE_ACTION_ATTACH) < 0) goto endjob;
- if ((ret = qemuDomainAttachDeviceConfig(qemuCaps, vmdef, dev)) < 0) + if ((ret = qemuDomainAttachDeviceConfig(qemuCaps, vmdef, dev, + dom->conn)) < 0) goto endjob; }
I think we should also update the docs in the way that we also support storage type 'volume' with disk device type 'lun'. And that is because we do not really care about the storage type when performing a device attach (we do care about this when doing snapshots), yet we still need to call virStorageDiskSourcePool which returns instantly for every storage type, except for volume. Documentation update should also imply updating our RNG schema (domaincommon I think), otherwise the use case described in the BZ won't validate. ACK if you also update both docs and RNG. Regards, Erik
On 06/12/2015 04:57 AM, Erik Skultety wrote:
On 06/11/2015 11:15 PM, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1228007
When attaching a scsi volume lun via the attach-device --config or --persistent options, there was no translation of the source pool like there was for the live path, thus the attempt to modify the config would fail since not enough was known about the disk.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 34e5581..6bb8549 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8016,7 +8016,8 @@ qemuDomainUpdateDeviceLive(virConnectPtr conn, static int qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, virDomainDefPtr vmdef, - virDomainDeviceDefPtr dev) + virDomainDeviceDefPtr dev, + virConnectPtr conn) { virDomainDiskDefPtr disk; virDomainNetDefPtr net; @@ -8033,6 +8034,8 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, _("target %s already exists"), disk->dst); return -1; } + if (virStorageTranslateDiskSourcePool(conn, disk) < 0) + return -1; if (qemuCheckDiskConfig(disk) < 0) return -1; if (virDomainDiskInsert(vmdef, disk)) @@ -8501,7 +8504,8 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, VIR_DOMAIN_DEVICE_ACTION_ATTACH) < 0) goto endjob;
- if ((ret = qemuDomainAttachDeviceConfig(qemuCaps, vmdef, dev)) < 0) + if ((ret = qemuDomainAttachDeviceConfig(qemuCaps, vmdef, dev, + dom->conn)) < 0) goto endjob; }
I think we should also update the docs in the way that we also support storage type 'volume' with disk device type 'lun'. And that is because we do not really care about the storage type when performing a device attach (we do care about this when doing snapshots), yet we still need to call virStorageDiskSourcePool which returns instantly for every storage type, except for volume. Documentation update should also imply updating our RNG schema (domaincommon I think), otherwise the use case described in the BZ won't validate. ACK if you also update both docs and RNG.
Not sure I understand what the RNG updated requirement is... The RNG already has all the proper formats defined when it was added in commit id 'c00b2f0d' (if you use gitk you can see other similar commits around that time as well). That same commit updated formatdomain.html.in, but changes since then (commit id 'cb3b7dce7') adjusted the wording slightly to the current test to describe use of "<source type='volume'...", although perhaps still not quite precise with respect to indicating how "<disk... type="block|network" ... device='lun'" is described. I suppose I could add something like the following to the paragraph that starts "Use the attribute mode...": Using a LUN from an iSCSI source pool provides the same capabilities as a <code>disk</code> configured using <code>type</code> 'block' or 'network and <code>device</code> of 'lun' with respect to how the LUN is presented to and may used by the guest. Possible replacement for "capabilities" - "features"? Or some other suggestion. John
On 06/12/2015 01:10 PM, John Ferlan wrote:
On 06/12/2015 04:57 AM, Erik Skultety wrote:
On 06/11/2015 11:15 PM, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1228007
When attaching a scsi volume lun via the attach-device --config or --persistent options, there was no translation of the source pool like there was for the live path, thus the attempt to modify the config would fail since not enough was known about the disk.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 34e5581..6bb8549 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8016,7 +8016,8 @@ qemuDomainUpdateDeviceLive(virConnectPtr conn, static int qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, virDomainDefPtr vmdef, - virDomainDeviceDefPtr dev) + virDomainDeviceDefPtr dev, + virConnectPtr conn) { virDomainDiskDefPtr disk; virDomainNetDefPtr net; @@ -8033,6 +8034,8 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, _("target %s already exists"), disk->dst); return -1; } + if (virStorageTranslateDiskSourcePool(conn, disk) < 0) + return -1; if (qemuCheckDiskConfig(disk) < 0) return -1; if (virDomainDiskInsert(vmdef, disk)) @@ -8501,7 +8504,8 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, VIR_DOMAIN_DEVICE_ACTION_ATTACH) < 0) goto endjob;
- if ((ret = qemuDomainAttachDeviceConfig(qemuCaps, vmdef, dev)) < 0) + if ((ret = qemuDomainAttachDeviceConfig(qemuCaps, vmdef, dev, + dom->conn)) < 0) goto endjob; }
I think we should also update the docs in the way that we also support storage type 'volume' with disk device type 'lun'. And that is because we do not really care about the storage type when performing a device attach (we do care about this when doing snapshots), yet we still need to call virStorageDiskSourcePool which returns instantly for every storage type, except for volume. Documentation update should also imply updating our RNG schema (domaincommon I think), otherwise the use case described in the BZ won't validate. ACK if you also update both docs and RNG.
Not sure I understand what the RNG updated requirement is... The RNG already has all the proper formats defined when it was added in commit id 'c00b2f0d' (if you use gitk you can see other similar commits around that time as well).
What I meant is: "Using 'lun' (since 0.9.10) is only valid when type is 'block' or 'network' using iSCSI protocol..." should be updated to allow the source type 'volume' as well, to reflect the nice paragraph below. If you look at the BZ, the device definitions begins with: <disk type='volume' device='lun'> ===> that device is parsed and processed correctly and also will be attached successfully later on (provided that your patch is merged). However it fails the RNG validation right here (domaincommon.rng): <attribute name="device"> <value>lun</value> </attribute> <optional> <ref name="rawIO"/> </optional> <optional> <ref name="sgIO"/> </optional> <interleave> <choice> <ref name="diskSourceNetwork"/> <ref name="diskSourceBlock"/> <<<< diskSourceVolume missing </choice> <ref name="diskSpecsExtra"/> </interleave>
Using a LUN from an iSCSI source pool provides the same capabilities as a <code>disk</code> configured using <code>type</code> 'block' or 'network and <code>device</code> of 'lun' with respect to how the LUN is presented to and may used by the guest.
^^^ This one's nice.
Possible replacement for "capabilities" - "features"? Or some other suggestion.
I like 'features', but that's just a matter of personal preference, so do as you wish.
John
On 06/12/2015 07:52 AM, Erik Skultety wrote:
On 06/12/2015 01:10 PM, John Ferlan wrote:
On 06/12/2015 04:57 AM, Erik Skultety wrote:
On 06/11/2015 11:15 PM, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1228007
When attaching a scsi volume lun via the attach-device --config or --persistent options, there was no translation of the source pool like there was for the live path, thus the attempt to modify the config would fail since not enough was known about the disk.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 34e5581..6bb8549 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8016,7 +8016,8 @@ qemuDomainUpdateDeviceLive(virConnectPtr conn, static int qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, virDomainDefPtr vmdef, - virDomainDeviceDefPtr dev) + virDomainDeviceDefPtr dev, + virConnectPtr conn) { virDomainDiskDefPtr disk; virDomainNetDefPtr net; @@ -8033,6 +8034,8 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, _("target %s already exists"), disk->dst); return -1; } + if (virStorageTranslateDiskSourcePool(conn, disk) < 0) + return -1; if (qemuCheckDiskConfig(disk) < 0) return -1; if (virDomainDiskInsert(vmdef, disk)) @@ -8501,7 +8504,8 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, VIR_DOMAIN_DEVICE_ACTION_ATTACH) < 0) goto endjob;
- if ((ret = qemuDomainAttachDeviceConfig(qemuCaps, vmdef, dev)) < 0) + if ((ret = qemuDomainAttachDeviceConfig(qemuCaps, vmdef, dev, + dom->conn)) < 0) goto endjob; }
I think we should also update the docs in the way that we also support storage type 'volume' with disk device type 'lun'. And that is because we do not really care about the storage type when performing a device attach (we do care about this when doing snapshots), yet we still need to call virStorageDiskSourcePool which returns instantly for every storage type, except for volume. Documentation update should also imply updating our RNG schema (domaincommon I think), otherwise the use case described in the BZ won't validate. ACK if you also update both docs and RNG.
Not sure I understand what the RNG updated requirement is... The RNG already has all the proper formats defined when it was added in commit id 'c00b2f0d' (if you use gitk you can see other similar commits around that time as well).
What I meant is: "Using 'lun' (since 0.9.10) is only valid when type is 'block' or 'network' using iSCSI protocol..." should be updated to allow the source type 'volume' as well, to reflect the nice paragraph below. If you look at the BZ, the device definitions begins with: <disk type='volume' device='lun'> ===> that device is parsed and processed correctly and also will be attached successfully later on (provided that your patch is merged). However it fails the RNG validation right here (domaincommon.rng): <attribute name="device"> <value>lun</value> </attribute> <optional> <ref name="rawIO"/> </optional> <optional> <ref name="sgIO"/> </optional> <interleave> <choice> <ref name="diskSourceNetwork"/> <ref name="diskSourceBlock"/> <<<< diskSourceVolume missing </choice> <ref name="diskSpecsExtra"/> </interleave>
Using a LUN from an iSCSI source pool provides the same capabilities as a <code>disk</code> configured using <code>type</code> 'block' or 'network and <code>device</code> of 'lun' with respect to how the LUN is presented to and may used by the guest.
^^^ This one's nice.
Possible replacement for "capabilities" - "features"? Or some other suggestion.
I like 'features', but that's just a matter of personal preference, so do as you wish.
OK - I'll squash the attached in Thanks - John
participants (2)
-
Erik Skultety -
John Ferlan