Re: [PATCH v2] storage: vz storage pool support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 06/12/16 02:59, John Ferlan wrote:

On 12/02/2016 12:09 PM, Olga Krishtal wrote:
On 20/09/16 23:30, John Ferlan wrote:
On 07/14/2016 01:13 PM, Olga Krishtal wrote:
This patch supports pool and volume management within Virtuozzo Storage.
Virtuozzo Storage is a highly-available distributed software defined
storage
with built-in replication and disaster recovery. From client's point of
view it looks like network attached storage (NFS or GlusterFS).
More information about vzstorage can be found here:
https://openvz.org/Virtuozzo_Storage
It supports the same volume formats as directory, nfs, etc.
Default format is raw.

Signed-off-by: Olga Krishtal <okrishtal@xxxxxxxxxxxxx>
---
   configure.ac                      |  31 +++++++++++
   docs/schemas/storagepool.rng      |  29 ++++++++++
   include/libvirt/libvirt-storage.h |   1 +
   src/conf/storage_conf.c           |  16 +++++-
   src/conf/storage_conf.h           |   4 +-
   src/storage/storage_backend.c     |   3 +
   src/storage/storage_backend_fs.c  | 112
+++++++++++++++++++++++++++++++++++++-
   src/storage/storage_backend_fs.h  |   3 +
   src/storage/storage_driver.c      |   2 +
   tools/virsh-pool.c                |   2 +
   tools/virsh.c                     |   3 +
   11 files changed, 203 insertions(+), 3 deletions(-)

In an effort to go through older on list bugs I'm wondering about the
relative importance of this compared to the fspool series.  Shall I
assume this is dropped in favor of that?

Tks -
No, we will not drop it. We want to have Virtuozzo Storage as backend
for storage pool.

To be more specific this code is getting wired in with the 'fs' backend
rather than creating it's own... Don't like that.

Similar to what I recently reviewed for the Veritas/vxfs changes - there
needs to be a separate storage_backend_vstorage.c in order to handle the
vstorage pool. Don't lump it in with the "fs" pool/backend. No different
than what is required for rbd, gluster, mpath, logical, disk, scsi, etc.

I thought of it first, however, after implementing some part of functionality -
I saw that for vstorage backend  I need to implement only 2 callbacks:
virStorageBackendVzStart
virStorageBackendVzfindPoolSources
All volume types and operations are the same as in pool/fs backend,
Basically what we need to use storage is to find it:
vstorage discover - finds all clusters or domain using DNS, zeroconf, etc
Than, we need to mount this cluster (I assume, that the authorization will be done before pool creation): vstorage-mount -c cluster-name mount_point. Then we have an access to this cluster as a filesystem, we
can upload-download image files, etc.
To avoid copy/paste of all vol-ops and nearly half of pool-ops I've choosen pool/fs_backend.
Do you still think that it is necessary to have separate backend?



Don't forget things need to be as separable as possible.  Add the pool
code, add the storage conf code, add the virsh code, etc.  One long
patch is just tough to review.

I will do splitting.


John

FWIW: It compiled, but failed syntax-check.
I have checked it once again - and in my case -syntax-check is passed.
Can you tell me what is wrong in your case? (The patch is ok for upstream)

Digging around I found my old review branch (been far too long to
remember) - it was 571 commits old, but still able to rebase without
merges (that's good)... My make syntax-check returns:


preprocessor_indentation
cppi: src/storage/storage_backend_fs.h: line 33: not properly indented
cppi: src/storage/storage_backend_fs.h: line 35: not properly indented
maint.mk: incorrect preprocessor indentation
cfg.mk:684: recipe for target 'sc_preprocessor_indentation' failed
make: *** [sc_preprocessor_indentation] Error 1

This is something you would see on your system because:

#if WITH_STORAGE_VSTORAGE
extern virStorageBackend virStorageBackendVstorage;
#endif

needs have need to change to

# if WITH_STORAGE_VSTORAGE
extern virStorageBackend virStorageBackendVstorage;
# endif

But it doesn't belong there anyway

Thanks a lot.


   Also the name "vstorage"
would seemingly be a bit too generic. I saw and thought virtual storage
not Virtuozzo Storage

All tools we use to manage Virtuozzo storage starts with vstorage*.
However, I can use vzstorage instead.

I suppose vstorage would be OK - I'm letting you know what I thought
when I saw it.

John

diff --git a/configure.ac b/configure.ac
index 2c81c95..2ee8b8c 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1692,6 +1692,10 @@ AC_ARG_WITH([storage-zfs],
     [AS_HELP_STRING([--with-storage-zfs],
       [with ZFS backend for the storage driver @<:@default=check@:>@])],
     [],[with_storage_zfs=check])
+AC_ARG_WITH([storage-vstorage],
+  [AS_HELP_STRING([--with-storage-vstorage],
+    [with VZ backend for the storage driver @<:@default=check@:>@])],
+  [],[with_storage_vstorage=check])
     if test "$with_libvirtd" = "no"; then
     with_storage_dir=no
@@ -1705,6 +1709,7 @@ if test "$with_libvirtd" = "no"; then
     with_storage_sheepdog=no
     with_storage_gluster=no
     with_storage_zfs=no
+  with_storage_vstorage=no
   fi
   if test "$with_storage_dir" = "yes" ; then
     AC_DEFINE_UNQUOTED([WITH_STORAGE_DIR], 1, [whether directory
backend for storage driver is enabled])
@@ -1963,6 +1968,31 @@ if test "$with_storage_fs" = "yes" ||
     fi
   fi
   +if test "$with_storage_vstorage" = "yes" || test
"$with_storage_vstorage" = "check"; then
+   AC_PATH_PROG([VSTORAGE], [vstorage], [], [$PATH:/sbin:/usr/bin])
+   AC_PATH_PROG([VSTORAGE_MOUNT], [vstorage-mount], [],
[$PATH:/sbin/usr/bin])
+  if test "$with_storage_vstorage" = "yes"; then
+    if test -z "$VSTORAGE" ; then AC_MSG_ERROR([We need vstorage
tools for virtuozzo storage driver]); fi
+    if test -z "$VSTORAGE_MOUNT" ; then AC_MSG_ERROR([We need
vstorage mount tool for virtuozzo storage driver]); fi
+    if test  "$with_storage_fs" = "no"; then AC_MSG_ERROR([We need
fs backend for vstorage pool]); fi
+  else
+    if test -z "$VSTORAGE" ; then with_storage_vstorage=no; fi
+    if test -z "$VSTORAGE" ; then with_storage_vstorage=no; fi
+    if test  "$with_storage_fs"= "no"; then
with_storage_vstorage=no; fi
+
+    if test "$with_storage_vstorage" = "check" ; then
with_storage_vstorage=yes ; fi
+  fi
+  if test "$with_storage_vstorage" = "yes"; then
+      AC_DEFINE_UNQUOTED([WITH_STORAGE_VSTORAGE], 1, [whether
virtuozzo storage backend for storage driver is enabled])
+      AC_DEFINE_UNQUOTED([VSTORAGE], ["$VSTORAGE"], [Location or
name of vstorage program])
+      AC_DEFINE_UNQUOTED([VSTORAGE_MOUNT], ["$VSTORAGE_MOUNT"],
[Location or name of vstorage-mount program])
+      AC_DEFINE_UNQUOTED([with_storage_fs], ["$VSTORAGE_MOUNT"],
[must be on])
+  fi
+
+  if test "$with_storage_vstorage" = "check" ; then
with_storage_vstorage=yes ; fi
+fi
+AM_CONDITIONAL([WITH_STORAGE_VSTORAGE], [test
"$with_storage_vstorage" = "yes"])
+
   LIBPARTED_CFLAGS=
   LIBPARTED_LIBS=
   if test "$with_storage_disk" = "yes" ||
@@ -2759,6 +2789,7 @@ AC_MSG_NOTICE([     RBD: $with_storage_rbd])
   AC_MSG_NOTICE([Sheepdog: $with_storage_sheepdog])
   AC_MSG_NOTICE([ Gluster: $with_storage_gluster])
   AC_MSG_NOTICE([     ZFS: $with_storage_zfs])
+AC_MSG_NOTICE([Vstorage: $with_storage_vstorage])
   AC_MSG_NOTICE([])
   AC_MSG_NOTICE([Security Drivers])
   AC_MSG_NOTICE([])
diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng
index 49d212f..8ad5616 100644
--- a/docs/schemas/storagepool.rng
+++ b/docs/schemas/storagepool.rng
@@ -24,6 +24,7 @@
           <ref name='poolsheepdog'/>
           <ref name='poolgluster'/>
           <ref name='poolzfs'/>
+        <ref name='poolvstorage'/>
         </choice>
       </element>
     </define>
@@ -173,6 +174,18 @@
       </interleave>
     </define>
   +  <define name='poolvstorage'>
+     <attribute name='type'>
+      <value>vz</value>
+    </attribute>
+    <interleave>
+      <ref name='commonmetadata'/>
+      <ref name='sizing'/>
+      <ref name='sourcevstorage'/>
+      <ref name='target'/>
+    </interleave>
+  </define>
+
     <define name='sourceinfovendor'>
       <interleave>
         <optional>
@@ -609,6 +622,22 @@
       </element>
     </define>
   +  <define name='clustername'>
+    <interleave>
+      <element name='name'>
+        <ref name='genericName'/>
+      </element>
+    </interleave>
+  </define>
+
+  <define name='sourcevstorage'>
+    <element name='source'>
+      <interleave>
+        <ref name='clustername'/>
+      </interleave>
+    </element>
+  </define>
+
     <define name='IscsiQualifiedName'>
       <data type='string'>
         <param
name="pattern">iqn\.[0-9]{4}-(0[1-9]|1[0-2])\.[a-zA-Z0-9\.\-]+(:.+)?</param>

diff --git a/include/libvirt/libvirt-storage.h
b/include/libvirt/libvirt-storage.h
index 0974f6e..28babf7 100644
--- a/include/libvirt/libvirt-storage.h
+++ b/include/libvirt/libvirt-storage.h
@@ -232,6 +232,7 @@ typedef enum {
       VIR_CONNECT_LIST_STORAGE_POOLS_SHEEPDOG      = 1 << 15,
       VIR_CONNECT_LIST_STORAGE_POOLS_GLUSTER       = 1 << 16,
       VIR_CONNECT_LIST_STORAGE_POOLS_ZFS           = 1 << 17,
+    VIR_CONNECT_LIST_STORAGE_POOLS_VSTORAGE      = 1 << 18,
   } virConnectListAllStoragePoolsFlags;
     int
virConnectListAllStoragePools(virConnectPtr conn,
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 05a1a49..e3c8ac1 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -60,7 +60,7 @@ VIR_ENUM_IMPL(virStoragePool,
                 "dir", "fs", "netfs",
                 "logical", "disk", "iscsi",
                 "scsi", "mpath", "rbd",
-              "sheepdog", "gluster", "zfs")
+              "sheepdog", "gluster", "zfs", "vstorage")
     VIR_ENUM_IMPL(virStoragePoolFormatFileSystem,
                 VIR_STORAGE_POOL_FS_LAST,
@@ -274,6 +274,16 @@ static virStoragePoolTypeInfo poolTypeInfo[] = {
            .defaultFormat = VIR_STORAGE_FILE_RAW,
        },
       },
+    {.poolType = VIR_STORAGE_POOL_VSTORAGE,
+     .poolOptions = {
+        .flags = VIR_STORAGE_POOL_SOURCE_NAME,
+     },
+     .volOptions = {
+        .defaultFormat = VIR_STORAGE_FILE_RAW,
+        .formatFromString = virStorageVolumeFormatFromString,
+        .formatToString = virStorageFileFormatTypeToString,
+     },
+    },
   };
     @@ -2588,6 +2598,10 @@
virStoragePoolSourceFindDuplicate(virConnectPtr conn,
               /* Only one mpath pool is valid per host */
               matchpool = pool;
               break;
+        case VIR_STORAGE_POOL_VSTORAGE:
+            if (STREQ(pool->def->source.name, def->source.name))
+                matchpool = pool;
+            break;
           case VIR_STORAGE_POOL_RBD:
           case VIR_STORAGE_POOL_LAST:
               break;
diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h
index 185ae5e..bf33b5f 100644
--- a/src/conf/storage_conf.h
+++ b/src/conf/storage_conf.h
@@ -95,6 +95,7 @@ typedef enum {
       VIR_STORAGE_POOL_SHEEPDOG, /* Sheepdog device */
       VIR_STORAGE_POOL_GLUSTER,  /* Gluster device */
       VIR_STORAGE_POOL_ZFS,      /* ZFS */
+    VIR_STORAGE_POOL_VSTORAGE, /* Virtuozzo Storage */
         VIR_STORAGE_POOL_LAST,
   } virStoragePoolType;
@@ -545,7 +546,8 @@ VIR_ENUM_DECL(virStoragePartedFs)
                    VIR_CONNECT_LIST_STORAGE_POOLS_RBD      | \
                    VIR_CONNECT_LIST_STORAGE_POOLS_SHEEPDOG | \
                    VIR_CONNECT_LIST_STORAGE_POOLS_GLUSTER  | \
-                 VIR_CONNECT_LIST_STORAGE_POOLS_ZFS)
+                 VIR_CONNECT_LIST_STORAGE_POOLS_ZFS      | \
+                 VIR_CONNECT_LIST_STORAGE_POOLS_VSTORAGE)
     # define
VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_ALL                  \
                   (VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_ACTIVE     | \
diff --git a/src/storage/storage_backend.c
b/src/storage/storage_backend.c
index 97f6ffe..181d3f5 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -135,6 +135,9 @@ static virStorageBackendPtr backends[] = {
   #if WITH_STORAGE_ZFS
       &virStorageBackendZFS,
   #endif
+#if WITH_STORAGE_VSTORAGE
+    &virStorageBackendVstorage,
+#endif
       NULL
   };
   diff --git a/src/storage/storage_backend_fs.c
b/src/storage/storage_backend_fs.c
index 0a12ecb..b89b3ae 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -371,7 +371,13 @@
virStorageBackendFileSystemIsValid(virStoragePoolObjPtr pool)
                              "%s", _("missing source path"));
               return -1;
           }
-    } else {
+    } else if (pool->def->type == VIR_STORAGE_POOL_VSTORAGE) {
+        if (!pool->def->source.name) {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           "%s", _("missing source cluster name"));
+            return -1;
+        }
+    } else{
           if (pool->def->source.ndevice != 1) {
               if (pool->def->source.ndevice == 0)
                   virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -411,6 +417,9 @@
virStorageBackendFileSystemGetPoolSource(virStoragePoolObjPtr pool)
                               pool->def->source.dir) < 0)
                   return NULL;
           }
+    } else if (pool->def->type == VIR_STORAGE_POOL_VSTORAGE) {
+        if (virAsprintf(&src, "vstorage://%s",
pool->def->source.name) < 0)
+            return NULL;
       } else {
           if (VIR_STRDUP(src, pool->def->source.devices[0].path) < 0)
               return NULL;
@@ -546,7 +555,9 @@
virStorageBackendFileSystemMount(virStoragePoolObjPtr pool)
       VIR_FREE(src);
       return ret;
   }
+#endif /* WITH_STORAGE_FS */
   +#if WITH_STORAGE_FS
   /**
    * @pool storage pool to unmount
    *
@@ -1654,3 +1665,102 @@ virStorageFileBackend
virStorageFileBackendDir = {
   };
     #endif /* WITH_STORAGE_FS */
+
+#if WITH_STORAGE_VSTORAGE
+static int
+virStorageBackendVzStart(virConnectPtr conn ATTRIBUTE_UNUSED,
+                         virStoragePoolObjPtr pool)
+{
+    int ret = -1;
+    virCommandPtr cmd = NULL;
+
+    cmd = virCommandNewArgList(VSTORAGE_MOUNT, "-c",
pool->def->source.name,
+                               pool->def->target.path, NULL);
+    if (virCommandRun(cmd, NULL) < 0)
+        goto cleanup;
+    ret = 0;
+
+ cleanup:
+    virCommandFree(cmd);
+    return ret;
+}
+
+static char*
+virStorageBackendVzfindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED,
+                                   const char *srcSpec
ATTRIBUTE_UNUSED,
+                                   unsigned int flags)
+{
+
+    virCommandPtr cmd = NULL;
+    char *buf = NULL;
+    char *ret = NULL;
+    char **clusters = NULL;
+    size_t clusters_num = 0;
+    size_t i = 0;
+    virStoragePoolSourceList vzcluster_list = {
+                                .type = VIR_STORAGE_POOL_VSTORAGE,
+                                .nsources = 0,
+                                .sources = NULL
+    };
+
+    virCheckFlags(0, NULL);
+
+    cmd = virCommandNewArgList(VSTORAGE, "discover", NULL);
+    virCommandSetOutputBuffer(cmd, &buf);
+
+    if (virCommandRun(cmd, NULL) < 0)
+        goto cleanup;
+
+    if (!(clusters = virStringSplitCount(buf, "\n", 0, &clusters_num)))
+        goto cleanup;
+
+    if (clusters_num > 0) {
+        vzcluster_list.nsources = clusters_num - 1;
+
+        if (VIR_ALLOC_N(vzcluster_list.sources,
vzcluster_list.nsources) < 0)
+            goto cleanup;
+
+        for  (; i < vzcluster_list.nsources; i++) {
+            if (VIR_ALLOC(vzcluster_list.sources[i].devices) < 0)
+                goto cleanup;
+            if (VIR_STRDUP(vzcluster_list.sources[i].name,
clusters[i]) < 0)
+                goto cleanup;
+        }
+    }
+
+    if (!(ret = virStoragePoolSourceListFormat(&vzcluster_list)))
+        goto cleanup;
+
+ cleanup:
+    for (i = 0; i < vzcluster_list.nsources; i++) {
+        virStoragePoolSourceClear(&vzcluster_list.sources[i]);
+        VIR_FREE(clusters[i]);
+    }
+    VIR_FREE(vzcluster_list.sources);
+    VIR_FREE(buf);
+    virCommandFree(cmd);
+    return ret;
+
+}
+virStorageBackend virStorageBackendVstorage = {
+    .type = VIR_STORAGE_POOL_VSTORAGE,
+
+    .startPool = virStorageBackendVzStart,
+    .checkPool = virStorageBackendFileSystemCheck,
+    .stopPool = virStorageBackendFileSystemStop,
+    .findPoolSources = virStorageBackendVzfindPoolSources,
+    .buildPool = virStorageBackendFileSystemBuild,
+    .deletePool = virStorageBackendFileSystemDelete,
+    .refreshPool = virStorageBackendFileSystemRefresh,
+    .buildVol = virStorageBackendFileSystemVolBuild,
+    .buildVolFrom = virStorageBackendFileSystemVolBuildFrom,
+    .createVol = virStorageBackendFileSystemVolCreate,
+    .refreshVol = virStorageBackendFileSystemVolRefresh,
+    .deleteVol = virStorageBackendFileSystemVolDelete,
+    .resizeVol = virStorageBackendFileSystemVolResize,
+    .uploadVol = virStorageBackendVolUploadLocal,
+    .downloadVol = virStorageBackendVolDownloadLocal,
+    .wipeVol = virStorageBackendVolWipeLocal,
+
+};
+#endif /* WITH_STORAGE_VSTORAGE */
diff --git a/src/storage/storage_backend_fs.h
b/src/storage/storage_backend_fs.h
index 347ea9b..23cff66 100644
--- a/src/storage/storage_backend_fs.h
+++ b/src/storage/storage_backend_fs.h
@@ -30,6 +30,9 @@
   extern virStorageBackend virStorageBackendFileSystem;
   extern virStorageBackend virStorageBackendNetFileSystem;
   # endif
+#if WITH_STORAGE_VSTORAGE
+extern virStorageBackend virStorageBackendVstorage;
+#endif
     typedef enum {
       FILESYSTEM_PROBE_FOUND,
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index cb9d578..3b6c7b4 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1618,6 +1618,7 @@ storageVolLookupByPath(virConnectPtr conn,
               case VIR_STORAGE_POOL_ISCSI:
               case VIR_STORAGE_POOL_SCSI:
               case VIR_STORAGE_POOL_MPATH:
+            case VIR_STORAGE_POOL_VSTORAGE:
                   stable_path = virStorageBackendStablePath(pool,
                                                             cleanpath,
                                                             false);
@@ -3485,6 +3486,7 @@ virStorageTranslateDiskSourcePool(virConnectPtr
conn,
       case VIR_STORAGE_POOL_DISK:
       case VIR_STORAGE_POOL_SCSI:
       case VIR_STORAGE_POOL_ZFS:
+    case VIR_STORAGE_POOL_VSTORAGE:
           if (!(def->src->path = virStorageVolGetPath(vol)))
               goto cleanup;
   diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c
index 6045331..6ea2424 100644
--- a/tools/virsh-pool.c
+++ b/tools/virsh-pool.c
@@ -1166,6 +1166,8 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd
ATTRIBUTE_UNUSED)
               case VIR_STORAGE_POOL_ZFS:
                   flags |= VIR_CONNECT_LIST_STORAGE_POOLS_ZFS;
                   break;
+            case VIR_STORAGE_POOL_VSTORAGE:
+                flags |= VIR_CONNECT_LIST_STORAGE_POOLS_VSTORAGE;
               case VIR_STORAGE_POOL_LAST:
                   break;
               }
diff --git a/tools/virsh.c b/tools/virsh.c
index 5dc482d..61c8e2f 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -682,6 +682,9 @@ virshShowVersion(vshControl *ctl ATTRIBUTE_UNUSED)
   #ifdef WITH_STORAGE_ZFS
       vshPrint(ctl, " ZFS");
   #endif
+#ifdef WITH_STORAGE_VSTORAGE
+    vshPrint(ctl, " Vstorage");
+#endif
       vshPrint(ctl, "\n");
         vshPrint(ctl, "%s", _(" Miscellaneous:"));




--
Best regards,
Olga

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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux