On Tue, Jun 24, 2014 at 4:19 PM, Dimitris Bliablias <[email protected]> wrote:
> Extend the 'RemoveDisks' method to accept the disk template, and the > disks of the instance we want to remove, as extra parameters. > > This method will be used by the conversion mechanism to remove the old > disks of an instance after we successfully converted to the new disk > template. Instead of directly calling the 'blockdev_remove' RPC every > time we want to remove a sub-set of the instance's disks, we modify this > method accordingly to save repeated lines of code. > > Besides the 'disks' parameter that specifies the disks we want to > remove, we also add the 'disk_template' parameter. At the time we remove > the old block devices, the instance has already converted to the new > template; in that case the old disk template must be passed as an extra > argument. > > Signed-off-by: Dimitris Bliablias <[email protected]> > --- > lib/cmdlib/instance_utils.py | 33 +++++++++++++++++++++++++-------- > 1 file changed, 25 insertions(+), 8 deletions(-) > > diff --git a/lib/cmdlib/instance_utils.py b/lib/cmdlib/instance_utils.py > index d2418a5..1e8f9f4 100644 > --- a/lib/cmdlib/instance_utils.py > +++ b/lib/cmdlib/instance_utils.py > @@ -247,17 +247,28 @@ def RemoveInstance(lu, feedback_fn, instance, > ignore_failures): > lu.cfg.RemoveInstance(instance.uuid) > > > -def RemoveDisks(lu, instance, target_node_uuid=None, > ignore_failures=False): > +def RemoveDisks(lu, instance, disk_template=None, disks=None, > + target_node_uuid=None, ignore_failures=False): > """Remove all disks for an instance. > > This abstracts away some work from `AddInstance()` and > `RemoveInstance()`. Note that in case some of the devices couldn't > be removed, the removal will continue with the other ones. > > + This function is also used by the disk template conversion mechanism to > + remove the old block devices of the instance. Since the instance has > + changed its template at the time we remove the original disks, we must > + specify the template of the disks we are about to remove as an argument. > + > @type lu: L{LogicalUnit} > @param lu: the logical unit on whose behalf we execute > @type instance: L{objects.Instance} > @param instance: the instance whose disks we should remove > + @type disk_template: string > + @param disk_template: if passed, overrides the instance's disk_template > + @type disks: list of L{objects.Disk} > + @param disks: the disks to remove; if not specified, all the disks of > the > + instance are removed > @type target_node_uuid: string > @param target_node_uuid: used to override the node on which to remove > the > disks > @@ -269,8 +280,14 @@ def RemoveDisks(lu, instance, target_node_uuid=None, > ignore_failures=False): > > all_result = True > ports_to_release = set() > - inst_disks = lu.cfg.GetInstanceDisks(instance.uuid) > - anno_disks = AnnotateDiskParams(instance, inst_disks, lu.cfg) > + > + if disks is None: > + disks = lu.cfg.GetInstanceDisks(instance.uuid) > + > + if disk_template is None: > + disk_template = instance.disk_template > + > + anno_disks = AnnotateDiskParams(instance, disks, lu.cfg) > for (idx, device) in enumerate(anno_disks): > if target_node_uuid: > edata = [(target_node_uuid, device)] > @@ -293,13 +310,13 @@ def RemoveDisks(lu, instance, target_node_uuid=None, > ignore_failures=False): > for port in ports_to_release: > lu.cfg.AddTcpUdpPort(port) > > - CheckDiskTemplateEnabled(lu.cfg.GetClusterInfo(), > instance.disk_template) > + CheckDiskTemplateEnabled(lu.cfg.GetClusterInfo(), disk_template) > > - if instance.disk_template in [constants.DT_FILE, > constants.DT_SHARED_FILE]: > - if len(inst_disks) > 0: > - file_storage_dir = os.path.dirname(inst_disks[0].logical_id[1]) > + if disk_template in [constants.DT_FILE, constants.DT_SHARED_FILE]: > + if len(disks) > 0: > + file_storage_dir = os.path.dirname(disks[0].logical_id[1]) > else: > - if instance.disk_template == constants.DT_SHARED_FILE: > + if disk_template == constants.DT_SHARED_FILE: > file_storage_dir = > utils.PathJoin(lu.cfg.GetSharedFileStorageDir(), > instance.name) > else: > If the disk template is file or shared file, then the whole directory is removed. This means that if we pass just some of (shared-)file-disks, all will be removed. So I'd suggest one of these options: (1) Pass just the disk_template and remove all disks that use this template. (2) Remove only the passed disks and if they're file-based, remove only their files, and remove the directory only if there are no other disks left. Option (1) seams to be simpler, as currently we only ever need to remove all disks of the same template. Would it be feasible to compute the disk template from a Disk object directly? This way, in case (1), RemoveDisks could filter out on its own which disks to remove, and in case (2) figure out which method to use for removing a particular disk. > -- > 1.7.10.4 > >
