On Thu, Jun 26, 2014 at 12:04 PM, Dimitris Bliablias <[email protected]> wrote:
> On 06/25/2014 12:22 PM, Petr Pudlák wrote: > >> 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. >> > > Actually, if the directory is not empty we will get a warning and the > directory will not be removed. The 'call_file_storage_dir_remove' will > fail. A use case of this behavior would be the '_RemoveDisk' method in > the LUInstanceSetParams, where I've intentionally avoided to apply the > 'RemoveDisks' method, exactly for this reason. Do I understand it correctly that if (2) below is implemented, this would allow it to be used also to simplify _RemoveDisk in LUInstanceSetParams? > > > >> 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. >> > > I prefer the 2nd solution. We could add a check to remove the directory in > the file-based instances only if the length of the disks we remove equals > the instance disks' length. > > > >> 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. >> > > Yes it would be feasible, but this would cause an error in case we have an > instance with no disks attached to it, i.e., the min disk-count in ipolicy > equals zero. > > So, I propose keeping the 'disk_template' parameter and add an extra check > before calling the 'file_storage_dir_remove' RPC. > You're right, I didn't think of that. Let's do it as you say - option (2) and add a check that the removal part is performed only if all disks are removed. > > > What do you think? > Dimitris > > >> >> -- >>> 1.7.10.4 >>> >>> >>> >
