On 2014年03月05日 04:49, Aline Manera wrote: > From: Aline Manera <alinefm(a)br.ibm.com&gt; > > 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(a)br.ibm.com&gt; > --- > 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@localhost ~]$ sudo vgdisplay --- Volume group --- VG Name abc System ID Format lvm2 [lvroyce@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@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.

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?

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

2014/3/5 4:49, Aline Manera: > From: Aline Manera <alinefm(a)br.ibm.com&gt; > > When creating a Logical storage pool, a LVM is created with the same > name. I assume you mean a volume group here.

> 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. I am a bit confusing here. The volume group was created when the user tried to create a storage pool from a raw device like "/dev/sda3". When the storage pool was undefined, it was reasonable to remove the volume group and make the raw device available again. Is it?

> 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. So the raw device will be used after the renaming and will not be available any more?

> > Also convert an error message parameter to string instead of > virStoragePool > instance. > > Signed-off-by: Aline Manera <alinefm(a)br.ibm.com&gt; > --- > 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) > + > except libvirt.libvirtError as e: > raise OperationFailed("KCHPOOL0011E", > {'name': name, 'err': > e.get_error_message()})