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

Re: [libvirt] [PATCH 11/12] storage: rbd: qemu: Add support for specifying internal RBD snapshots




On 11/12/2014 08:47 AM, Peter Krempa wrote:
> Some storage systems have internal support for snapshots. Libvirt should
> be able to select a correct snapshot when starting a VM.
> 
> This patch adds a XML element to select a storage source snapshot for
> the RBD protocol which supports this feature.
> ---
>  docs/formatdomain.html.in                          | 18 +++++++---
>  docs/schemas/domaincommon.rng                      |  8 +++++
>  src/conf/domain_conf.c                             | 38 +++++++++++++++++++---
>  src/conf/domain_conf.h                             |  1 +
>  src/conf/snapshot_conf.c                           |  6 ++--
>  src/qemu/qemu_command.c                            |  3 ++
>  src/util/virstoragefile.c                          |  8 +++++
>  src/util/virstoragefile.h                          |  1 +
>  .../qemuxml2argv-disk-drive-network-rbd.args       |  4 +++
>  .../qemuxml2argv-disk-drive-network-rbd.xml        | 17 ++++++++++
>  10 files changed, 94 insertions(+), 10 deletions(-)
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 4f44bc0..fc35c5a 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -1676,6 +1676,7 @@
>        <driver name="qemu" type="raw"/>
>        <source protocol="rbd" name="image_name2">
>          <host name="hostname" port="7000"/>
> +        <snapshot name="snapname"/>
>        </source>
>        <target dev="hdc" bus="ide"/>
>        <auth username='myuser'>
> @@ -1949,14 +1950,16 @@
>          is only valid when the specified storage volume is of 'file' or
>          'block' type).
>          <p>
> -        When the disk <code>type</code> is "network", the <code>source</code>
> -        may have zero or more <code>host</code> sub-elements used to
> -        specify the hosts to connect.
> +        The <code>source</code> element may contain the following sub elements:
>          </p>
> 
>          <dl>
>            <dt><code>host</code></dt>
> -          <dd>The <code>host</code> element supports 4 attributes, viz.  "name",
> +          <dd>When the disk <code>type</code> is "network", the <code>source</code>
> +            may have zero or more <code>host</code> sub-elements used to
> +            specify the hosts to connect.
> +
> +            The <code>host</code> element supports 4 attributes, viz.  "name",
>              "port", "transport" and "socket", which specify the hostname,
>              the port number, transport type and path to socket, respectively.
>              The meaning of this element and the number of the elements depend
> @@ -2005,6 +2008,13 @@
>              transport is "unix", the socket attribute specifies the path to an
>              AF_UNIX socket.
>            </dd>
> +          <dt><code>snapshot</code></dt>
> +          <dd>
> +            The <code>name</code> attribute of <code>snapshot</code> element can
> +            optionally specify an internal snapshot name to be used as the
> +            source for storage systems such as rbd.
> +            (<span class="since">Since 1.2.11 for the qemu driver.</span>)

To follow other usages (adjust last 2 lines):

source for storage protocols.
Supported for 'rbd' <span class="since">since 1.2.11 (QEMU only).</span>


> +          </dd>
>          </dl>
> 
>          <p>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 6863ec6..154d222 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1452,6 +1452,14 @@
>              </choice>
>            </element>
>          </zeroOrMore>
> +        <optional>
> +          <element name="snapshot">
> +            <attribute name="name">
> +              <ref name="genericName"/>
> +            </attribute>
> +            <empty/>
> +          </element>
> +        </optional>
>          <empty/>
>        </element>
>      </interleave>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 2c65276..37a8042 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -3162,6 +3162,22 @@ virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev,
>              return -1;
>      }
> 
> +    /* verify disk source */
> +    if (dev->type == VIR_DOMAIN_DEVICE_DISK) {
> +        virDomainDiskDefPtr disk = dev->data.disk;
> +
> +        /* internal snapshots are currently supported only with rbd: */
> +        if (virStorageSourceGetActualType(disk->src) != VIR_STORAGE_TYPE_NETWORK &&
> +            disk->src->protocol != VIR_STORAGE_NET_PROTOCOL_RBD) {
> +            if (disk->src->snapshot) {
> +                virReportError(VIR_ERR_XML_ERROR, "%s",
> +                               _("<snapshot> element is currently supported "
> +                                 "only with 'rbd' disks"));
> +                return -1;
> +            }
> +        }
> +    }
> +
>      return 0;
>  }
> 
> @@ -5316,10 +5332,14 @@ virDomainDiskSourcePoolDefParse(xmlNodePtr node,
> 
>  int
>  virDomainDiskSourceParse(xmlNodePtr node,
> +                         xmlXPathContextPtr ctxt,
>                           virStorageSourcePtr src)
>  {
>      int ret = -1;
>      char *protocol = NULL;
> +    xmlNodePtr saveNode = ctxt->node;
> +
> +    ctxt->node = node;
> 
>      switch ((virStorageType)src->type) {
>      case VIR_STORAGE_TYPE_FILE:
> @@ -5372,6 +5392,9 @@ virDomainDiskSourceParse(xmlNodePtr node,
>              tmp[0] = '\0';
>          }
> 
> +        /* snapshot currently works only for remote disks */
> +        src->snapshot = virXPathString("string(./snapshot/@name)", ctxt);
> +
>          if (virDomainStorageHostParse(node, &src->hosts, &src->nhosts) < 0)
>              goto cleanup;
>          break;
> @@ -5397,6 +5420,7 @@ virDomainDiskSourceParse(xmlNodePtr node,
> 
>   cleanup:
>      VIR_FREE(protocol);
> +    ctxt->node = saveNode;
>      return ret;
>  }
> 
> @@ -5452,7 +5476,7 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt,
>          goto cleanup;
>      }
> 
> -    if (virDomainDiskSourceParse(source, backingStore) < 0 ||
> +    if (virDomainDiskSourceParse(source, ctxt, backingStore) < 0 ||
>          virDomainDiskBackingStoreParse(ctxt, backingStore) < 0)
>          goto cleanup;
> 
> @@ -5562,7 +5586,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
>                  xmlStrEqual(cur->name, BAD_CAST "source")) {
>                  sourceNode = cur;
> 
> -                if (virDomainDiskSourceParse(cur, def->src) < 0)
> +                if (virDomainDiskSourceParse(cur, ctxt, def->src) < 0)
>                      goto error;
>                  source = def->src->path;
> 
> @@ -5728,7 +5752,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
>                                         _("mirror requires source element"));
>                          goto error;
>                      }
> -                    if (virDomainDiskSourceParse(mirrorNode, def->mirror) < 0)
> +                    if (virDomainDiskSourceParse(mirrorNode, ctxt,
> +                                                 def->mirror) < 0)
>                          goto error;
>                  }
>                  ready = virXMLPropString(cur, "ready");
> @@ -16154,11 +16179,12 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf,
> 
>              VIR_FREE(path);
> 
> -            if (src->nhosts == 0) {
> +            if (src->nhosts == 0 && !src->snapshot) {
>                  virBufferAddLit(buf, "/>\n");
>              } else {
>                  virBufferAddLit(buf, ">\n");
>                  virBufferAdjustIndent(buf, 2);
> +
>                  for (n = 0; n < src->nhosts; n++) {
>                      virBufferAddLit(buf, "<host");
>                      virBufferEscapeString(buf, " name='%s'",
> @@ -16175,6 +16201,10 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf,
> 
>                      virBufferAddLit(buf, "/>\n");
>                  }
> +
    if (src->snapshot)

Rest is fine, ACK

John

> +                virBufferEscapeString(buf, "<snapshot name='%s'/>\n",
> +                                      src->snapshot);
> +
>                  virBufferAdjustIndent(buf, -2);
>                  virBufferAddLit(buf, "</source>\n");
>              }
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 530a3ca..acbf13b 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2477,6 +2477,7 @@ virDomainDiskRemove(virDomainDefPtr def, size_t i);
>  virDomainDiskDefPtr
>  virDomainDiskRemoveByName(virDomainDefPtr def, const char *name);
>  int virDomainDiskSourceParse(xmlNodePtr node,
> +                             xmlXPathContextPtr ctxt,
>                               virStorageSourcePtr src);
> 
>  bool virDomainHasDiskMirror(virDomainObjPtr vm);
> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
> index 1f83b2c..66fc2e6 100644
> --- a/src/conf/snapshot_conf.c
> +++ b/src/conf/snapshot_conf.c
> @@ -107,6 +107,7 @@ void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def)
> 
>  static int
>  virDomainSnapshotDiskDefParseXML(xmlNodePtr node,
> +                                 xmlXPathContextPtr ctxt,
>                                   virDomainSnapshotDiskDefPtr def)
>  {
>      int ret = -1;
> @@ -154,7 +155,7 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node,
>          if (!def->src->path &&
>              xmlStrEqual(cur->name, BAD_CAST "source")) {
> 
> -            if (virDomainDiskSourceParse(cur, def->src) < 0)
> +            if (virDomainDiskSourceParse(cur, ctxt, def->src) < 0)
>                  goto cleanup;
> 
>          } else if (!def->src->format &&
> @@ -352,7 +353,8 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt,
>              goto cleanup;
>          def->ndisks = n;
>          for (i = 0; i < def->ndisks; i++) {
> -            if (virDomainSnapshotDiskDefParseXML(nodes[i], &def->disks[i]) < 0)
> +            if (virDomainSnapshotDiskDefParseXML(nodes[i], ctxt,
> +                                                 &def->disks[i]) < 0)
>                  goto cleanup;
>          }
>          VIR_FREE(nodes);
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 021ec07..7923842 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -2988,6 +2988,9 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src,
> 
>              virBufferStrcat(&buf, "rbd:", src->path, NULL);
> 
> +            if (src->snapshot)
> +                virBufferEscape(&buf, '\\', ":", "@%s", src->snapshot);
> +
>              if (username) {
>                  virBufferEscape(&buf, '\\', ":", ":id=%s", username);
>                  virBufferEscape(&buf, '\\', ":",
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index bc7c51d..efd51d2 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -1849,6 +1849,7 @@ virStorageSourceCopy(const virStorageSource *src,
>          VIR_STRDUP(ret->driverName, src->driverName) < 0 ||
>          VIR_STRDUP(ret->relPath, src->relPath) < 0 ||
>          VIR_STRDUP(ret->backingStoreRaw, src->backingStoreRaw) < 0 ||
> +        VIR_STRDUP(ret->snapshot, src->snapshot) < 0 ||
>          VIR_STRDUP(ret->compat, src->compat) < 0)
>          goto error;
> 
> @@ -2280,6 +2281,13 @@ virStorageSourceParseRBDColonString(const char *rbdstr,
>          *p = '\0';
>      }
> 
> +    /* snapshot name */
> +    if ((p = strchr(src->path, '@'))) {
> +        if (VIR_STRDUP(src->snapshot, p + 1) < 0)
> +            goto error;
> +        *p = '\0';
> +    }
> +
>      /* options */
>      if (!options)
>          return 0; /* all done */
> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
> index b1ba73a..caab0b4 100644
> --- a/src/util/virstoragefile.h
> +++ b/src/util/virstoragefile.h
> @@ -238,6 +238,7 @@ struct _virStorageSource {
>      char *path;
>      int protocol; /* virStorageNetProtocol */
>      char *volume; /* volume name for remote storage */
> +    char *snapshot; /* for storage systems supporting internal snapshots */
>      size_t nhosts;
>      virStorageNetHostDefPtr hosts;
>      virStorageSourcePoolDefPtr srcpool;
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args
> index 21d7b64..e4f1389 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args
> @@ -5,4 +5,8 @@ unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb \
>  -drive 'file=rbd:pool/image:auth_supported=none:\
>  mon_host=mon1.example.org\:6321\;mon2.example.org\:6322\;\
>  mon3.example.org\:6322,if=virtio,format=raw' \
> +-drive file=rbd:pool/image asdf:auth_supported=none,if=virtio,format=raw \
> +-drive 'file=rbd:pool/image foo:auth_supported=none:\
> +mon_host=mon1.example.org\:6321\;mon2.example.org\:6322\;\
> +mon3.example.org\:6322,if=virtio,format=raw' \
>  -net none -serial none -parallel none
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.xml
> index 37e9db5..f6accd8 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.xml
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.xml
> @@ -29,6 +29,23 @@
>        </source>
>        <target dev='vda' bus='virtio'/>
>      </disk>
> +    <disk type='network' device='disk'>
> +      <driver name='qemu' type='raw'/>
> +      <source protocol='rbd' name='pool/image'>
> +        <snapshot name='asdf'/>
> +      </source>
> +      <target dev='vdb' bus='virtio'/>
> +    </disk>
> +    <disk type='network' device='disk'>
> +      <driver name='qemu' type='raw'/>
> +      <source protocol='rbd' name='pool/image'>
> +        <host name='mon1.example.org' port='6321'/>
> +        <host name='mon2.example.org' port='6322'/>
> +        <host name='mon3.example.org' port='6322'/>
> +        <snapshot name='foo'/>
> +      </source>
> +      <target dev='vdc' bus='virtio'/>
> +    </disk>
>      <controller type='usb' index='0'/>
>      <controller type='ide' index='0'/>
>      <controller type='pci' index='0' model='pci-root'/>
> 


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