Please update the current patch-set, prefferably by adding another small patch before this one that introduces the new method in ConfigWriter. We really want to get rid of all Update calls wherever it's possible.
Thanks, Petr On Thu, Jun 26, 2014 at 12:07 PM, Dimitris Bliablias <[email protected]> wrote: > On 06/25/2014 05:31 PM, Petr Pudlák wrote: > >> On Tue, Jun 24, 2014 at 4:19 PM, Dimitris Bliablias < >> [email protected]> >> wrote: >> >> This patch introduces the generic method that will perform the disk >>> template conversion of an instance, named '_ConvertInstanceTemplate'. >>> >>> This method is not functional yet, since it is not accessible from the >>> 'LUInstanceSetParams' LU. When all the appropriate checks will be made, >>> it will be exposed to the Logical Unit to support the disk template >>> conversion feature. >>> >>> The procedure for converting the disk template works as follows: >>> >>> - a completely new disk template is generated by calling the >>> 'GenerateDiskTemplate' method. The new template matches the count and >>> the size, mode, and name parameters of the current instance's disks. >>> - the new block devices are created by calling the 'CreateDisks' method. >>> - the data from the old instance's disks are copied to the newly created >>> disks. In case a disk copy operation fails, the operation aborts and >>> the newly created disks are removed. The instance will remain intact. >>> - the original disks are detached from the instance and then are removed >>> from the cluster config by calling the 'RemoveInstanceDisk' for each >>> disk of the instance. >>> - the new disks are added to the cluster config and then attached to the >>> instance by calling the 'AddInstanceDisk' method for each new disk. >>> - the old block devices are removed from the node on which they reside >>> by calling the 'RemoveDisks' method. >>> >>> Signed-off-by: Dimitris Bliablias <[email protected]> >>> --- >>> lib/cmdlib/instance.py | 139 >>> +++++++++++++++++++++++++++++++++++++++++++++++- >>> 1 file changed, 137 insertions(+), 2 deletions(-) >>> >>> diff --git a/lib/cmdlib/instance.py b/lib/cmdlib/instance.py >>> index 7522ccf..a2132ee 100644 >>> --- a/lib/cmdlib/instance.py >>> +++ b/lib/cmdlib/instance.py >>> @@ -3605,11 +3605,146 @@ class LUInstanceSetParams(LogicalUnit): >>> utils.CommaJoin(set(res_max + res_min)))) >>> raise errors.OpPrereqError(msg, errors.ECODE_INVAL) >>> >>> + def _ConvertInstanceTemplate(self, feedback_fn): >>> + """Converts the disk template of an instance. >>> + >>> + This function converts the disk template of an instance. It supports >>> + conversions among all the available disk templates except >>> conversions >>> + between the LVM-based disk templates, that use their separate code >>> path. >>> + Also, this method does not support conversions that include the >>> 'diskless' >>> + template and those targeting the 'blockdev' template. >>> + >>> + @type feedback_fn: callable >>> + @param feedback_fn: function used to send feedback back to the >>> caller >>> + >>> + @rtype: NoneType >>> + @return: None >>> + @raise errors.OpPrereqError: in case of failure >>> + >>> + """ >>> + feedback_fn("Converting disk template from '%s' to '%s'" % >>> + (self.instance.disk_template, self.op.disk_template)) >>> + >>> + assert not (self.op.disk_template in (constants.DT_BLOCK, >>> + constants.DT_DISKLESS) or >>> + self.instance.disk_template == constants.DT_DISKLESS), \ >>> + ("Unsupported disk template conversion from '%s' to '%s'" % >>> + (self.instance.disk_template, self.op.disk_template)) >>> + >>> + pnode_uuid = self.instance.primary_node >>> + snode_uuid = [] >>> + if self.op.remote_node_uuid: >>> + snode_uuid = [self.op.remote_node_uuid] >>> + >>> + old_disks = self.cfg.GetInstanceDisks(self.instance.uuid) >>> + >>> + feedback_fn("Generating new '%s' disk template..." % >>> self.op.disk_template) >>> + new_disks = GenerateDiskTemplate(self, >>> + self.op.disk_template, >>> + self.instance.uuid, >>> + pnode_uuid, >>> + snode_uuid, >>> + self.disks_info, >>> + None, # to be updated with the >>> file_path >>> + None, # to be updated with the >>> file_driver >>> + 0, >>> + feedback_fn, >>> + self.diskparams) >>> + >>> + # Create the new block devices for the instance. >>> + feedback_fn("Creating new empty disks of type '%s'..." % >>> + self.op.disk_template) >>> + try: >>> + CreateDisks(self, self.instance, >>> disk_template=self.op.disk_template, >>> + disks=new_disks) >>> + except errors.OpExecError: >>> + self.LogWarning("Device creation failed") >>> + self.cfg.ReleaseDRBDMinors(self.instance.uuid) >>> + raise >>> + >>> + # Transfer the data from the old to the newly created disks of the >>> instance. >>> + feedback_fn("Populating the new empty disks of type '%s'..." % >>> + self.op.disk_template) >>> + for idx, (old, new) in enumerate(zip(old_disks, new_disks)): >>> + feedback_fn(" - copying data from disk %s (%s), size %s" % >>> + (idx, self.instance.disk_template, >>> + utils.FormatUnit(new.size, "h"))) >>> + if self.instance.disk_template == constants.DT_DRBD8: >>> + old = old.children[0] >>> + result = self.rpc.call_blockdev_convert(pnode_uuid, (old, >>> self.instance), >>> + (new, self.instance)) >>> + msg = result.fail_msg >>> + if msg: >>> + # A disk failed to copy. Abort the conversion operation and >>> rollback >>> + # the modifications to the previous state. The instance will >>> remain >>> + # intact. >>> + if self.op.disk_template == constants.DT_DRBD8: >>> + new = new.children[0] >>> + self.Log(" - ERROR: Could not copy disk '%s' to '%s'" % >>> + (old.logical_id[1], new.logical_id[1])) >>> + try: >>> + self.LogInfo("Some disks failed to copy") >>> + self.LogInfo("The instance will not be affected, aborting >>> operation") >>> + self.LogInfo("Removing newly created disks of type '%s'..." % >>> + self.op.disk_template) >>> + RemoveDisks(self, self.instance, >>> disk_template=self.op.disk_template, >>> + disks=new_disks) >>> + self.LogInfo("Newly created disks removed successfully") >>> + finally: >>> + self.cfg.ReleaseDRBDMinors(self.instance.uuid) >>> + result.Raise("Error while converting the instance's template") >>> + >>> + # In case of DRBD disk, return its port to the pool >>> + # NOTE: this must be done right before the call to cfg.Update! >>> + if self.instance.disk_template == constants.DT_DRBD8: >>> + for disk in old_disks: >>> + tcp_port = disk.logical_id[2] >>> + self.cfg.AddTcpUdpPort(tcp_port) >>> + >>> + # Remove old disks from the instance. >>> + feedback_fn("Detaching old disks (%s) from the instance and >>> removing" >>> + " them from cluster config" % >>> self.instance.disk_template) >>> + for old_disk in old_disks: >>> + self.cfg.RemoveInstanceDisk(self.instance.uuid, old_disk.uuid) >>> + >>> + # Update the instance structure. >>> + self.instance = self.cfg.GetInstanceInfo(self.instance.uuid) >>> + # The old disk_template will be needed to remove the old block >>> devices. >>> + old_disk_template = self.instance.disk_template >>> + self.instance.disk_template = self.op.disk_template >>> + self.cfg.Update(self.instance, feedback_fn) >>> >>> I'd prefer if we added a new method to ConfigWriter, something like >> SetInstanceDiskTemplate. Method cfg.Update is sort of a last resort and is >> a bit problematic, we should rather try to eliminate its use completely. >> > > ACK. > > Do you think that this should be included in the next patch series, or > should I update the current patch-set accordingly? > > > Thanks, > Dimitris > > > >> >> + >>> + # Attach the new disks to the instance. >>> + feedback_fn("Adding new disks (%s) to cluster config and attaching" >>> + " them to the instance" % self.instance.disk_template) >>> + for (idx, new_disk) in enumerate(new_disks): >>> + self.cfg.AddInstanceDisk(self.instance.uuid, new_disk, idx=idx) >>> + >>> + # Re-read the instance from the configuration. >>> + self.instance = self.cfg.GetInstanceInfo(self.instance.uuid) >>> + >>> + # Release node locks while waiting for sync and disks removal. >>> + ReleaseLocks(self, locking.LEVEL_NODE) >>> + >>> + disk_abort = not WaitForSync(self, self.instance, >>> + oneshot=not self.op.wait_for_sync) >>> + if disk_abort: >>> + raise errors.OpExecError("There are some degraded disks for" >>> + " this instance, please cleanup >>> manually") >>> + >>> + feedback_fn("Removing old block devices of type '%s'..." % >>> + old_disk_template) >>> + RemoveDisks(self, self.instance, disk_template=old_disk_template, >>> + disks=old_disks) >>> + >>> + # Node resource locks will be released by the caller. >>> + >>> def _ConvertPlainToDrbd(self, feedback_fn): >>> """Converts an instance from plain to drbd. >>> >>> """ >>> - feedback_fn("Converting template to drbd") >>> + feedback_fn("Converting disk template from 'plain' to 'drbd'") >>> + >>> pnode_uuid = self.instance.primary_node >>> snode_uuid = self.op.remote_node_uuid >>> >>> @@ -3697,7 +3832,7 @@ class LUInstanceSetParams(LogicalUnit): >>> assert self.instance.disk_template == constants.DT_DRBD8 >>> >>> snode_uuid = secondary_nodes[0] >>> - feedback_fn("Converting template to plain") >>> + feedback_fn("Converting disk template from 'drbd' to 'plain'") >>> >>> disks = self.cfg.GetInstanceDisks(self.instance.uuid) >>> old_disks = AnnotateDiskParams(self.instance, disks, self.cfg) >>> -- >>> 1.7.10.4 >>> >>> >>> Rest LGTM, thanks. >> >> >
