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

Re: [libvirt] [PATCH 1/2] migration: Introduce <migration> element for cdrom and floppy

On 09/27/2011 08:39 AM, Michal Privoznik wrote:
This element says what to do with cdrom (or floppy) on migration.
Currently, only one attribute is supported: 'optional'. It accepts
'yes' and 'no' values. Setting a cdrom to be optional means, if
destination cannot access disk source, it simply gets free()'d/ejected.

This functionality is important for users, whose machines get buggy.
So they decide to save as much as possible and migrate the machine
even they won't be able to access (readonly) cdrom on destination.
  docs/schemas/domaincommon.rng |   16 ++++++++

Missing docs/formatdomain.html.in, so I can't ack this. That said, I'll still review...

  src/conf/domain_conf.c        |   85 ++++++++++++++++++++++++++++++++++++++++-
  src/conf/domain_conf.h        |   16 ++++++++
  src/libvirt_private.syms      |    2 +
  4 files changed, 118 insertions(+), 1 deletions(-)

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index be98be0..1150297 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -630,6 +630,9 @@
        <ref name="encryption"/>
+<ref name="migration"/>

This allows <migration> for all disk types. With a bit more RNG magic, it might be possible to enforce that <migration> only appears for cdrom and floppy. On the other hand, it might be possible to further extend <migration> for how it interacts with snapshots and/or copy-on-read behaviors (for example, a future patch might make it possible to request that migration creates a qcow2 wrapper on the destination with the source as its backing file, then after migration completes, perform a block pull to break the remote link), so I'm okay with leaving <migration> for all three disk types with future extensibility in mind.

+++ b/src/conf/domain_conf.c
@@ -560,6 +560,12 @@ VIR_ENUM_IMPL(virDomainNumatuneMemMode, VIR_DOMAIN_NUMATUNE_MEM_LAST,

+              "default",
+              "yes",
+              "no");

Are these the right values for the attribute? I see at least three choices, not two:

1. require - destination must see the same cd as source
2. optional - if destination has same cd, use it, if not, eject
3. drop - on migration, destination will eject, even if it has same cd

Also, does this play well with cdroms that the host has locked? That is, since there is a difference between eject (which fails if guest has lock) and eject -f (which succeeds at ripping out the disk regardless of the guest's current use of the disk), we may need to factor in some description of whether we use force, or whether the migration fails if a guest lock is in place, and so forth.

  #define virDomainReportError(code, ...)                              \
      virReportErrorHelper(VIR_FROM_DOMAIN, code, __FILE__,            \
                           __FUNCTION__, __LINE__, __VA_ARGS__)
@@ -764,6 +770,7 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def)
+    VIR_FREE(def->migration);

Store def->migration by value, not by reference, and life gets simpler. More on this later...


@@ -2247,6 +2254,56 @@ cleanup:

+static int
+virDomainDeviceMigrationParseXML(xmlNodePtr node,
+                                 virDomainDiskDefPtr def)
+    int ret = -1;
+    char *optional = NULL;
+    int i;
+    if (!node || !def) {
+        virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                             _("invalid argument supplied"));
+        return -1;
+    }
+    if (VIR_ALLOC(def->migration)<  0) {
+        virReportOOMError();
+        return -1;
+    }

No need to alloc if you store by value.

@@ -9076,6 +9140,21 @@ virDomainLeaseDefFormat(virBufferPtr buf,

  static int
+virDomainDeviceMigrationFormat(virBufferPtr buf,
+                               virDomainDeviceMigrationInfoPtr mig)
+    if (!buf)
+        return -1;
+    if (!mig)
+        return 0;
+    virBufferAsprintf(buf, "<migration optional='%s'/>\n",
+                      virDomainDeviceMigrationOptionalTypeToString(mig->optional));

We shall see whether this needs a tweak once my v2 indentation patches are in :)

+typedef struct _virDomainDeviceMigrationInfo virDomainDeviceMigrationInfo;
+typedef virDomainDeviceMigrationInfo *virDomainDeviceMigrationInfoPtr;
+struct _virDomainDeviceMigrationInfo {
+    int optional;

I guess it makes sense to break this into a separate struct, given my argument above that we may extend <migration> further for other migration-related aspects of how to handle disks.

  /* Stores the virtual disk configuration */
  typedef struct _virDomainDiskDef virDomainDiskDef;
  typedef virDomainDiskDef *virDomainDiskDefPtr;
@@ -286,6 +300,7 @@ struct _virDomainDiskDef {
      unsigned int transient : 1;
      virDomainDeviceInfo info;
      virStorageEncryptionPtr encryption;
+    virDomainDeviceMigrationInfoPtr migration;

Rather than storing a virDomainDeviceMigrationInfoPtr that needs migration, you can get away with virDomainDeviceMigrationInfo by value. Allocating a pointer to a sub-struct is only needed when we want to be able to easily pass around just the sub-struct, but I don't see that happening here.

I think the biggest hurdle to clear is to get consensus on the xml representation - I think your overall idea of dropping the disk source of ejectable disks on migration makes sense, but am not convinced we have the best xml representation for that notion yet.

Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

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