Re: [PATCH] Refactor ESX storage driver and add iSCSI support

[Ata Bohra <ata.husain@xxxxxxxxxxx>]

Please see inline.
 

> Date: Sun, 9 Sep 2012 19:39:34 +0200
> Subject: Re: [PATCH] Refactor ESX storage driver and add iSCSI support
> From: matthias.bolte@xxxxxxxxxxxxxx
> To: ata.husain@xxxxxxxxxxx
> CC: libvir-list@xxxxxxxxxx
>
> 2012/8/20 Ata E Husain Bohra <ata.husain@xxxxxxxxxxx>:
> > The patch refactors the current ESX storage driver due to following reasons:
> >
> > 1. Given most of the public APIs exposed by the storage driver in Libvirt
> > remains same, ESX storage driver should not implement logic specific
> > for only one supported format (current implementation only supports VMFS).
> > 2. Decoupling interface from specific storage implementation gives us an
> > extensible design to hook implementation for other supported storage
> > formats.
> >
> > This patch refactors the current driver to implement it as a facade pattern i.e.
> > the driver exposes all the public libvirt APIs, but uses backend drivers to get
> > the required task done. The backend drivers provide implementation specific to
> > the type of storage device.
> >
> > File changes:
> > ------------------
> > esx_storage_driver.c ----> esx_storage_driver.c (base storage driver)
> > |
> > |---> esx_storage_backend_vmfs.c (VMFS backend)
> > |
> > |---> esx_storage_backend_iscsi.c (iSCSI backend)
> >
> > The patch adds the backend driver to support iSCSI format storage pools
> > and volumes for ESX host. The mapping of ESX iSCSI specifics to Libvirt
> > is as follows:
> >
> > 1. ESX static iSCSI target <------> Libvirt Storage Pools
> > 2. ESX iSCSI LUNs <------> Libvirt Storage Volumes.
> >
> > The above understanding is based on http://libvirt.org/storage.html.
> >
> > The operation supported on iSCSI pools includes:
> >
> > 1. List storage pools & volumes.
> > 2. Get xml descriptor operaion on pools & volumes.
> > 3. Lookup operation on pools & volumes by name, uuid and path (if applicable).
> >
> > iSCSI pools does not support operations such as: Create / remove pools
> > and volumes
> > ---
>
> Sorry, that it took me so long to start reviewing your work.
>
> > diff --git a/src/esx/esx_storage_backend_iscsi.c b/src/esx/esx_storage_backend_iscsi.c
> > new file mode 100644
> > index 0000000..4f79a0f
> > --- /dev/null
> > +++ b/src/esx/esx_storage_backend_iscsi.c
> > @@ -0,0 +1,794 @@
> > +/*
> > + * esx_storage_backend_iscsi.c: ESX storage backend for iSCSI handling
> > + *
> > + * Copyright (C) 2007-2008, 2010-2012 Red Hat, Inc.
>
> I think this is wrong. This is all new code, isn't it? So the
> copyright line should read as this
>
> Copyright (C) 2012 Ata E Husain Bohra <your email address>
>
> > +#include <string.h>
> > +#include <stdio.h>
> > +#include <unistd.h>
> > +#include <sys/stat.h>
>
> You're not using anything from sys/stat.h, so this include can be removed.
[AB]: Sure will remove that.

 

> > +static int
> > +esxStorageBackendISCSINumberOfStoragePools(virConnectPtr conn)
> > +{
> > + int count = 0;
> > + esxPrivate *priv = conn->storagePrivateData;
> > + esxVI_HostInternetScsiHba *hostInternetScsiHba = NULL;
> > + bool success = false;
> > +
> > + if (esxVI_LookupHostInternetScsiHba(
> > + priv->primary, &hostInternetScsiHba) < 0) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("Unable to obtain iSCSI adapter"));
> > + goto cleanup;
> > + }
> > +
> > + if (hostInternetScsiHba == NULL) {
> > + /* iSCSI adapter may not be enabled for this host */
> > + return 0;
> > + }
>
> This logic suggests that there can only be at most one
> HostInternetScsiHba per ESX server. Is that really true?
[AB]: The proposed solution works with software iSCSI adapter, there is only one software iSCSI adapter per ESX host. Thanks for pointing this out, I wanted to add these comments but missed it :).

 

> > + if (hostInternetScsiHba->configuredStaticTarget) {
> > + const esxVI_HostInternetScsiHbaStaticTarget *target = NULL;
> > + for (target = hostInternetScsiHba->configuredStaticTarget;
> > + target != NULL; target = target->_next) {
> > + ++count;
> > + }
> > + }
>
> The if around the for loop is not necessary ans can be removed.
[AB]: I guess I am spoiled with the coding standards followed at my workplace; we try to protect conditional loops with parenthesis even if its a single line code statements. I will make necessary correction if this is not acceptable style with Libvirt community.

> > +static int
> > +esxStorageBackendISCSIListStoragePools(virConnectPtr conn,
> > + char **const names,
> > + const int maxnames)
> > +{
>
> > + if (hostInternetScsiHba->configuredStaticTarget) {
> > + const esxVI_HostInternetScsiHbaStaticTarget *target = NULL;
> > + for (target = hostInternetScsiHba->configuredStaticTarget;
> > + target != NULL && count < maxnames;
> > + target = target->_next, ++count) {
> > + names[count] = strdup(target->iScsiName);
> > +
> > + if (names[count] == NULL) {
> > + virReportOOMError();
> > + goto cleanup;
> > + }
> > + }
> > + }
>
> The if around the for loop is not necessary ans can be removed.
>
> > +
> > +static virStoragePoolPtr
> > +esxStorageBackendISCSIPoolLookupByName(virConnectPtr conn,
> > + const char *name)
> > +{
> > + esxPrivate *priv = conn->storagePrivateData;
> > + esxVI_HostInternetScsiHbaStaticTarget *target = NULL;
> > + /* MD5_DIGEST_SIZE = VIR_UUID_BUFLEN = 16 */
> > + unsigned char md5[MD5_DIGEST_SIZE];
> > + virStoragePoolPtr pool = NULL;
> > +
> > + if (esxVI_LookupHostInternetScsiHbaStaticTargetByName(
> > + priv->primary, name, &target, esxVI_Occurrence_OptionalItem) < 0) {
> > + goto cleanup;
> > + }
> > +
> > + /**
> > + * HostInternetScsiHbaStaticTarget does not provide a uuid field,
> > + * but iSsiName (or widely known as IQN) is unique across the multiple
>
> Typo in 'iSsiName'.
>
> > + * hosts, using it to compute key
> > + */
> > +
> > + md5_buffer(target->iScsiName, strlen(target->iScsiName), md5);
>
> esxVI_LookupHostInternetScsiHbaStaticTargetByName has an occurrence
> parameter that is set to esxVI_Occurrence_OptionalItem. This means
> that esxVI_LookupHostInternetScsiHbaStaticTargetByName is allowed to
> return target as NULL. But you're blindly dereferencing it here. I
> think occurrence should be passed as esxVI_Occurrence_RequiredItem
> here instead.
[AB]:  "poolLookupByName" is also used by esx_storage_drive.c:esxStorageGetBackendDriver(), in this function we want to go over both VMFS as well iSCSI pools list to determine the matching backend driver. I think the flaw is I need to check for valid target pointer (NON-NULL) before deferencing it.

> > +static virStoragePoolPtr
> > +esxStorageBackendISCSIPoolLookupByUUID(virConnectPtr conn,
> > + const unsigned char *uuid)
> > +{
>
> > + if (hostInternetScsiHba->configuredStaticTarget) {
> > + for (target = hostInternetScsiHba->configuredStaticTarget;
> > + target != NULL; target = target->_next) {
> > + md5_buffer(target->iScsiName, strlen(target->iScsiName), md5);
> > +
> > + if (memcmp(uuid, md5, VIR_UUID_STRING_BUFLEN) == 0) {
> > + break;
> > + }
> > + }
> > + }
>
> Again, the if around the for is not necessary here.
>
> > + if (target == NULL) {
> > + /* pool not found */
> > + goto cleanup;
> > + }
>
> Error reporting is missing here.
>
> > +static int
> > +esxStorageBackendISCSIPoolGetInfo(virStoragePoolPtr pool ATTRIBUTE_UNUSED,
> > + virStoragePoolInfoPtr info)
> > +{
> > + /* these fields are not valid for iSCSI pool */
> > + info->allocation = info->capacity = info->available = 0;
> > + info->state = esxVI_Boolean_True;
>
> This is wrong. info->state should be VIR_STORAGE_POOL_RUNNING or
> another value from the virStoragePoolState if the state can be
> determined in more detail.

> > +static char *
> > +esxStorageBackendISCSIPoolGetXMLDesc(virStoragePoolPtr pool, unsigned int flags)
> > +{
>
> > + if (hostInternetScsiHba->configuredStaticTarget) {
> > + for (target = hostInternetScsiHba->configuredStaticTarget;
> > + target != NULL; target = target->_next) {
> > + if (STREQ(target->iScsiName, pool->name)) {
> > + break;
> > + }
> > + }
> > + }
>
> Again, the if around the for is not necessary here.
>
> > + if (target == NULL) {
> > + goto cleanup;
> > + }
>
> Error reporting is missing here.
>
> > +static int
> > +esxStorageBackendISCSIPoolListStorageVolumes(virStoragePoolPtr pool,
> > + char **const names,
> > + int maxnames)
> > +{
>
> > + if (esxVI_LookupScsiLunList(priv->primary, &scsiLunList) < 0) {
> > + goto cleanup;
> > + }
> > +
> > + /* O^2 but still faster than hash given N is not that large */
> > + for (scsiLun = scsiLunList; scsiLun != NULL && count < maxnames;
> > + scsiLun = scsiLun->_next) {
> > + for (hostScsiTopologyLun = hostScsiTopologyLunList;
> > + hostScsiTopologyLun != NULL && count < maxnames;
> > + hostScsiTopologyLun = hostScsiTopologyLun->_next) {
> > + if (STREQ(hostScsiTopologyLun->scsiLun, scsiLun->key)) {
> > + names[count] = strdup(scsiLun->deviceName);
>
> What does a typical device name look like? Is it easily
> distinguishable from the volume names of the VMFS backend driver? Can
> you give some examples?
[AB]: Device name looks similar to a device path in linux, for instance on my sample machine device path for a iSCSI lun is "/vmfs/devices/disks/t10.F405E46494C45425F49615840723D214D476A4D2E457B416".

 

As mentioned in one of my comments, technically we can use "scsiLun->canonicalName" as "t10.F405E46494C45425F49615840723D214D476A4D2E457B416" is nothing but a canonicalName for that iSCSI lun. Only reason I used deviceName was it is a mandatory field, whereas, canonicalName is an options field inside scsiLun data object.

>
> > +static virStorageVolPtr
> > +esxStorageBackendISCSIVolumeLookupByName(virStoragePoolPtr pool,
> > + const char *name)
> > +{
>
> > + for (scsiLun = scsiLunList; scsiLun != NULL;
> > + scsiLun = scsiLun->_next) {
> > + if (STREQ(scsiLun->deviceName, name)) {
> > + /**
> > + * ScsiLun provides an UUID field that is unique accross
> > + * multiple servers. But this field length is ~55 characters
> > + * compute MD5 hash to transform it to an acceptable
> > + * libvirt format
> > + */
> > + md5_buffer(scsiLun->uuid, strlen(scsiLun->uuid), md5);
> > +
> > + virUUIDFormat(md5, uuid_string);
>
> I'm not sure if this is the best approach. What does a typical ScsiLun
> look like? Can you give some examples?
[AB]: I think you mean sample scssilun->uuid, but I am printing the compelte structure just incase along with specific mention of scsilun->uuid:

 

(gdb) p *scsiLun
$2 = {_next = 0x0, _type = esxVI_Type_HostScsiDisk,
  deviceName = 0x682da0 "/vmfs/devices/disks/t10.F405E46494C45425F49615840723D214D476A4D2E457B416", deviceType = 0x704af0 "disk",
  lunType = 0x704b70 "disk", operationalState = 0x707570, uuid = 0x6832a0 "01000000004f69514870322d414d674a2d4e754b61564952545541",
  alternateName = 0x618660, canonicalName = 0x683160 "t10.F405E46494C45425F49615840723D214D476A4D2E457B416", capabilities = 0x0,
  descriptor = 0x0, displayName = 0x0, durableName = 0x61b340,
  key = 0x704b10 "key-vim.host.ScsiDisk-01000000004f69514870322d414d674a2d4e754b61564952545541",
  model = 0x704bb0 "VIRTUAL-DISK    ", queueDepth = 0x0, revision = 0x704bd0 "0   ", scsiLevel = 0x704bf0,
  serialNumber = 0x704c10 "unavailable", standardInquiry = 0x705590, vendor = 0x704b90 "OPNFILER"}

scsilun->uuid looks like:

(gdb) p scsiLun->uuid
$4 = 0x6832a0 "01000000004f69514870322d414d674a2d4e754b61564952545541".

>
> > +static virStorageVolPtr
> > +esxStorageBackendISCSIVolumeLookupByPath(virConnectPtr conn, const char *path)
> > +{
> > + virStorageVolPtr volume = NULL;
> > + esxPrivate *priv = conn->storagePrivateData;
> > + char *poolName = NULL;
> > + esxVI_ScsiLun *scsiLunList = NULL;
> > + const esxVI_ScsiLun *scsiLun = NULL;
> > + const esxVI_HostScsiDisk *hostScsiDisk = NULL;
>
> Remove the const here...

[AB]: As the variable is used just to iterate the list IMHO "const" assures the reader that the memory need not to be freed. I aggree there is an extra static_cast needed to perform "dynamic_cast" operation but I thought code readibility will surpass ths cost. Please let me know if this can be accepted.
>
> > + /* MD5_DIGEST_SIZE = VIR_UUID_BUFLEN = 16 */
> > + unsigned char md5[MD5_DIGEST_SIZE];
> > + char uuid_string[VIR_UUID_STRING_BUFLEN] = "";
> > +
> > + if (esxVI_LookupScsiLunList(priv->primary, &scsiLunList) < 0) {
> > + goto cleanup;
> > + }
> > +
> > + for (scsiLun = scsiLunList ; scsiLun != NULL;
> > + scsiLun = scsiLun->_next) {
> > + hostScsiDisk =
> > + esxVI_HostScsiDisk_DynamicCast((esxVI_ScsiLun *)scsiLun);
>
> .. so that the (esxVI_ScsiLun *) cast is no longer necessary.
>
> > + if (hostScsiDisk != NULL &&
> > + STREQ(hostScsiDisk->devicePath, path)) {
> > + /* Found matching device */
> > + if (esxVI_LookupStoragePoolNameByScsiLunKey(
> > + priv->primary, hostScsiDisk->key, &poolName) < 0) {
> > + goto cleanup;
> > + }
> > +
> > + md5_buffer(scsiLun->uuid, strlen(scsiLun->uuid), md5);
> > +
> > + virUUIDFormat(md5, uuid_string);
> > +
> > + volume = virGetStorageVol(conn, poolName, path, uuid_string);
>
> Passing the path is not correct I think, assuming that the volume name
> and path are not the same for iSCSI.
[AB]: For user deviceName and devicePath should be same. HostDevice dataobjects defines deviceName as:

"deviceName

xsd:string

The name of the device on the host. For example, /dev/cdrom or \\serverX\device_name.

"

As mentioned earlier, sample deviceName looks like:

"/vmfs/devices/disks/t10.F405E46494C45425F49615840723D214D476A4D2E457B416"; it is a valid ESX device path as well.

 

I will document it to make it more clear.

> > +virStorageDriver esxStorageBackendISCSIDrv = {
> > + .name = "ESX ISCSI backend",
> > + .open = NULL, /* 0.10.0 */
> > + .close = NULL, /* 0.10.0 */
> > + .numOfPools = esxStorageBackendISCSINumberOfStoragePools, /* 0.10.0 */
> > + .listPools = esxStorageBackendISCSIListStoragePools, /* 0.10.0 */
> > + .poolLookupByName = esxStorageBackendISCSIPoolLookupByName, /* 0.10.0 */
> > + .poolLookupByUUID = esxStorageBackendISCSIPoolLookupByUUID, /* 0.10.0 */
> > + .poolRefresh = esxStorageBackendISCSIPoolRefresh, /* 0.10.0 */
> > + .poolGetInfo = esxStorageBackendISCSIPoolGetInfo, /* 0.10.0 */
> > + .poolGetXMLDesc = esxStorageBackendISCSIPoolGetXMLDesc, /* 0.10.0 */
> > + .poolNumOfVolumes = esxStorageBackendISCSIPoolNumberOfStorageVolumes, /* 0.10.0 */
> > + .poolListVolumes = esxStorageBackendISCSIPoolListStorageVolumes, /* 0.10.0 */
> > + .volLookupByName = esxStorageBackendISCSIVolumeLookupByName, /* 0.10.0 */
> > + .volLookupByKey = esxStorageBackendISCSIVolumeLookupByKey, /* 0.10.0 */
> > + .volLookupByPath = esxStorageBackendISCSIVolumeLookupByPath, /* 0.10.0 */
> > + .volCreateXML = esxStorageBackendISCSIVolumeCreateXML, /* 0.10.0 */
> > + .volCreateXMLFrom = esxStorageBackendISCSIVolumeCreateXMLFrom, /* 0.10.0 */
> > + .volGetXMLDesc = esxStorageBackendISCSIVolumeGetXMLDesc, /* 0.10.0 */
> > + .volDelete = esxStorageBackendISCSIVolumeDelete, /* 0.10.0 */
> > + .volWipe = esxStorageBackendISCSIVolumeWipe, /* 0.10.0 */
> > + .volGetPath = esxStorageBackendISCSIVolumeGetPath, /* 0.10.0 */
> > +
> > +};
>
> The version number here are outdated now. It should be 0.10.2.
>
> > diff --git a/src/esx/esx_storage_backend_iscsi.h b/src/esx/esx_storage_backend_iscsi.h
> > new file mode 100644
> > index 0000000..ca756ac
> > --- /dev/null
> > +++ b/src/esx/esx_storage_backend_iscsi.h
> > @@ -0,0 +1,31 @@
> > +/*
> > + * esx_storage_backend_iscsi.h: ESX storage backend for iSCSI handling
> > + *
> > + * Copyright (C) 2007-2008 Red Hat, Inc.
>
> Again, that's not the right copyright notice.
>
> > diff --git a/src/esx/esx_storage_backend_vmfs.c b/src/esx/esx_storage_backend_vmfs.c
> > new file mode 100644
> > index 0000000..6550196
> > --- /dev/null
> > +++ b/src/esx/esx_storage_backend_vmfs.c
> > @@ -0,0 +1,1471 @@
> > +
> > +/*
> > + * esx_storage_backend_vmfs.c: ESX storage backend for VMFS datastores
> > + *
> > + * Copyright (C) 2010-2011 Red Hat, Inc.
> > + * Copyright (C) 2010 Matthias Bolte <matthias.bolte@xxxxxxxxxxxxxx>
>
> Put your copyright line here in the form of
[AB]: I intentionally left it "unchanged". Almost all the code written in this file is written by you, I just renamed the file and I do not want to put myself as an author just for renaming the filename :).
> Copyright (C) 2012 Ata E Husain Bohra <your email address>
>
> instead of an author list on the end of the license statement.
>
> > +#include <string.h>
> > +#include <stdio.h>
> > +#include <unistd.h>
> > +#include <sys/stat.h>
>
> This include is unused and can be removed.
>
>
> This is as far as I got today with the review. I'll continue in the
> next days, hopefully it won't take me weeks again to get back to this
> :)
[AB]: I highly appreciate your attention and time on my patches. I'm looking forward for your comments on above mentioned points before updating a newer version for the patch.
> --
> Matthias Bolte
> http://photron.blogspot.com

Thanks again!

 

Regards,

Ata

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list