[Kimchi-devel] [PATCH] issue #312: Rename LVM while deleting logical storage pool

[Aline Manera]

On 03/05/2014 04:13 AM, Royce Lv wrote:
> On 2014年03月05日 04:49, Aline Manera wrote:
>> From: Aline Manera <alinefm at br.ibm.com>
>>
>> When creating a Logical storage pool, a LVM is created with the same
>> name.
>> And when deleting the storage pool the LVM keeps on the system. Which is
>> the correct behavior as the storage pool is only a concept and 
>> deleting it
>> should not affect the file system.
>> But it prevents the user to create a new logical pool with a name used
>> before which is odd.
>> So when deleting a logical pool, rename the LVM to a single name to
>> allow user create a logical pool with any name.
>>
>> Also convert an error message parameter to string instead of 
>> virStoragePool
>> instance.
>>
>> Signed-off-by: Aline Manera <alinefm at br.ibm.com>
>> ---
>>   src/kimchi/model/storagepools.py |   15 ++++++++++++++-
>>   1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/kimchi/model/storagepools.py 
>> b/src/kimchi/model/storagepools.py
>> index 76ab2d6..dd3c9a2 100644
>> --- a/src/kimchi/model/storagepools.py
>> +++ b/src/kimchi/model/storagepools.py
>> @@ -17,6 +17,8 @@
>>   # License along with this library; if not, write to the Free Software
>>   # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  
>> 02110-1301 USA
>>
>> +import time
>> +
>>   import libvirt
>>
>>   from kimchi import xmlutils
>> @@ -172,7 +174,8 @@ class StoragePoolModel(object):
>>                   return 0
>>           except libvirt.libvirtError as e:
>>               raise OperationFailed("KCHPOOL0008E",
>> -                                  {'name': pool, 'err': 
>> e.get_error_message()})
>> +                                  {'name': pool.name(),
>> +                                   'err': e.get_error_message()})
>>
>>       def _get_storage_source(self, pool_type, pool_xml):
>>           source = {}
>> @@ -348,7 +351,17 @@ class StoragePoolModel(object):
>>           if pool.isActive():
>>               raise InvalidOperation("KCHPOOL0005E", {'name': name})
>>           try:
>> +            xml = pool.XMLDesc(0)
>> +            pool_type = xmlutils.xpath_get_text(xml, "/pool/@type")[0]
>>               pool.undefine()
>> +            # The user may want to create a logical pool with the 
>> same name
>> +            # used before but a LVM will already exist with this name
>> +            # So rename the former LVM to avoid future conflicts
>> +            if pool_type == 'logical':
>> +                lvm_name = "%s-%s" % (name, str(int(time.time()*1000)))
>> +                vgrename_cmd = ["vgrename", name, lvm_name]
>> +                run_command(vgrename_cmd)
> Well, I would say when this VG is renamed, user would felt confused 
> and wondered if it is a different VG from previous one.
>
> Also some referrence to the LV path will change:
>
> suppose I have a volume group called "abc" and has two LV "root" and 
> "swap":
>
> [lvroyce at localhost ~]$ sudo vgdisplay
> --- Volume group ---
> VG Name abc
> System ID
> Format lvm2
>
> [lvroyce at localhost ~]$ sudo lvdisplay
> --- Logical volume ---
> LV Path /dev/abc/swap
> LV Name swap
> VG Name abc
>
>
> --- Logical volume ---
> LV Path /dev/abc/root
> LV Name root
> VG Name abc
>
> After change the vgname from "abc" to "fedora", all its logical 
> volumes path change:
> [lvroyce at localhost ~]$ sudo lvdisplay
> --- Logical volume ---
> LV Path /dev/fedora/swap
>
>
> --- Logical volume ---
> LV Path /dev/fedora/root
> LV Name root
> VG Name fedora
>
> This means all application depend on these volume path will be unsafe: 
> e.g. a VM use an LV as its disk will not able to find its disk any more.
>

Good point.
Most of systems use UUID to mount devices but we can have problem on 
systems does not follow this principle.

> Based on this, I suppose maybe when storage pool type is "logical" and 
> not import an existent VG,
> can we validate the name is used by some VG and give out a clear 
> indication?

ACK. I will do that instead of renaming the vg.
Thanks.

>
>> +
>>           except libvirt.libvirtError as e:
>>               raise OperationFailed("KCHPOOL0011E",
>>                                     {'name': name, 'err': 
>> e.get_error_message()})
>