On 06/25/2014 11:47 AM, Petr Pudlák wrote:
There is one subtle side effect of this change that if a disk in 'disks' is
a DRBD disk, it must have its nodes filled - before the function took the
nodes from the instance's existing disks. There are two places where it
could potentially cause problems: In class TemporaryDisk and in
LUInstanceSetParams. In the latter case I believe that GenerateDisktemplate
provides the nodes correctly, but in TemporaryDisk I'll need to check.


For the latter case I also believe that the 'GenerateDiskTemplate' provides
the nodes correctly. As for the 'TemporaryDisk' class, currently it is only
used for creating disks using the 'plain' template. Also it doesn't seem to
support 'drbd' disks creation, since it doesn't generate a complete drbd
device with its children, like does the '_GenerateDRBD8Branch' method.

So, i believe that this modification does not affect any existing
functionality.


Thanks,
Dimitris




On Tue, Jun 24, 2014 at 4:19 PM, Dimitris Bliablias <[email protected]>
wrote:

Currently, the 'all_node_uuids' variable in the 'CreateDisks' method is
computed from all the disks of the instance. This approach will not be
compatible with the upcoming disk template conversion feature. A use
case is the conversions targeting the 'drbd' template. In that case the
'all_node_uuids' will be wrongly computed from the current instance's
disks that do not contain the secondary node's uuid of the new disks,
causing them to be only created in the primary node of the instance.
Removing the 'instance_disks' argument and using the 'disks' argument
instead, solves this issue without affecting existing functionality.

Signed-off-by: Dimitris Bliablias <[email protected]>
---
  lib/cmdlib/instance.py         |    2 +-
  lib/cmdlib/instance_storage.py |   19 +++++++------------
  2 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/lib/cmdlib/instance.py b/lib/cmdlib/instance.py
index 8c31e66..3218801 100644
--- a/lib/cmdlib/instance.py
+++ b/lib/cmdlib/instance.py
@@ -1727,7 +1727,7 @@ class LUInstanceCreate(LogicalUnit):
      else:
        feedback_fn("* creating instance disks...")
        try:
-        CreateDisks(self, iobj, instance_disks=disks)
+        CreateDisks(self, iobj, disks=disks)
        except errors.OpExecError:
          self.LogWarning("Device creation failed")
          self.cfg.ReleaseDRBDMinors(instance_uuid)
diff --git a/lib/cmdlib/instance_storage.py
b/lib/cmdlib/instance_storage.py
index f31806a..8282932 100644
--- a/lib/cmdlib/instance_storage.py
+++ b/lib/cmdlib/instance_storage.py
@@ -202,8 +202,7 @@ def _UndoCreateDisks(lu, disks_created, instance):
                  (disk, lu.cfg.GetNodeName(node_uuid)), logging.warning)


-def CreateDisks(lu, instance, instance_disks=None,
-                to_skip=None, target_node_uuid=None, disks=None):
+def CreateDisks(lu, instance, to_skip=None, target_node_uuid=None,
disks=None):
    """Create all disks for an instance.

    This abstracts away some work from AddInstance.
@@ -216,9 +215,6 @@ def CreateDisks(lu, instance, instance_disks=None,
    @param lu: the logical unit on whose behalf we execute
    @type instance: L{objects.Instance}
    @param instance: the instance whose disks we should create
-  @type instance_disks: list of L{objects.Disk}
-  @param instance_disks: the disks that belong to the instance; if not
-      specified, retrieve them from config file
    @type to_skip: list
    @param to_skip: list of indices to skip
    @type target_node_uuid: string
@@ -232,15 +228,17 @@ def CreateDisks(lu, instance, instance_disks=None,

    """
    info = GetInstanceInfoText(instance)
-  if instance_disks is None:
-    instance_disks = lu.cfg.GetInstanceDisks(instance.uuid)
+
+  if disks is None:
+    disks = lu.cfg.GetInstanceDisks(instance.uuid)
+
    if target_node_uuid is None:
      pnode_uuid = instance.primary_node
      # We cannot use config's 'GetInstanceNodes' here as 'CreateDisks'
      # is used by 'LUInstanceCreate' and the instance object is not
      # stored in the config yet.
      all_node_uuids = []
-    for disk in instance_disks:
+    for disk in disks:
        all_node_uuids.extend(disk.all_nodes)
      all_node_uuids = set(all_node_uuids)
      # ensure that primary node is always the first
@@ -250,13 +248,10 @@ def CreateDisks(lu, instance, instance_disks=None,
      pnode_uuid = target_node_uuid
      all_node_uuids = [pnode_uuid]

-  if disks is None:
-    disks = instance_disks
-
    CheckDiskTemplateEnabled(lu.cfg.GetClusterInfo(),
instance.disk_template)

    if instance.disk_template in constants.DTS_FILEBASED:
-    file_storage_dir = os.path.dirname(instance_disks[0].logical_id[1])
+    file_storage_dir = os.path.dirname(disks[0].logical_id[1])
      result = lu.rpc.call_file_storage_dir_create(pnode_uuid,
file_storage_dir)

      result.Raise("Failed to create directory '%s' on"
--
1.7.10.4



Reply via email to