I talked with riba@ and either we'll prohibit DRBD disks for TemporaryDisk completely (as it does really make much sense), or make a white-list of allowed templates.
So yes, it should work in both cases, so LGTM On Thu, Jun 26, 2014 at 11:59 AM, Dimitris Bliablias <[email protected]> wrote: > 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 >>> >>> >>> >
