[RFC][PATCH] Do not allow user deactivate/delete storage pool used by vms
From: ShaoHe Feng <shaohef@linux.vnet.ibm.com> Make a integrity verification when user try to deactivate or delete a pool. If a volume of a pool is used by a vm, the pool should not be deactivated or deleted to avoid problems. It is no harmful to allow to deactivate/delete dir pool, for kimchi does not delete the pool path when delete this pool. But it is harmful for other type pools, such as logical or nfs pool. Royce has added a ref_cnt for volume, if the sum of ref_cnt in this pool larger than 0, means this pool is used by some vms. Signed-off-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com> --- src/kimchi/i18n.py | 2 ++ src/kimchi/model/storagepools.py | 23 +++++++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index 1ae3889..7c8fbc7 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -144,6 +144,8 @@ messages = { "KCHPOOL0034E": _("Unable to deactivate pool %(name)s as it is associated with some templates"), "KCHPOOL0035E": _("Unable to delete pool %(name)s as it is associated with some templates"), "KCHPOOL0036E": _("A volume group named '%(name)s' already exists. Please, choose another name to create the logical pool."), + "KCHPOOL0037E": _("Unable to deactivate pool %(name)s as it is used by some vms"), + "KCHPOOL0038E": _("Unable to delete pool %(name)s as it is used by some vms"), "KCHVOL0001E": _("Storage volume %(name)s already exists"), "KCHVOL0002E": _("Storage volume %(name)s does not exist in storage pool %(pool)s"), diff --git a/src/kimchi/model/storagepools.py b/src/kimchi/model/storagepools.py index fea19f6..247c374 100644 --- a/src/kimchi/model/storagepools.py +++ b/src/kimchi/model/storagepools.py @@ -179,6 +179,16 @@ class StoragePoolModel(object): else: raise + def _get_volumes_ref_cnt(self, name): + # in order to avoid nested import + from kimchi.model.storagevolumes import StorageVolumesModel + from kimchi.model.storagevolumes import StorageVolumeModel + kwargs = {"conn": self.conn, "objstore": self.objstore} + cnts = [StorageVolumeModel(**kwargs).lookup(name, volume)["ref_cnt"] + for volume in StorageVolumesModel(**kwargs).get_list(name)] + + return sum(cnts) + def _get_storagepool_vols_num(self, pool): try: if pool.isActive(): @@ -345,6 +355,9 @@ class StoragePoolModel(object): raise InvalidOperation('KCHPOOL0034E', {'name': name}) pool = self.get_storagepool(name, self.conn) + if pool.isActive() and self._get_volumes_ref_cnt(name) > 0: + raise InvalidOperation('KCHPOOL0037E', {'name': name}) + #FIXME: nfs workaround - do not try to deactivate a NFS pool # if the NFS server is not reachable. xml = pool.XMLDesc(0) @@ -374,6 +387,16 @@ class StoragePoolModel(object): if pool.isActive(): raise InvalidOperation("KCHPOOL0005E", {'name': name}) try: + try: + pool.create(0) + except Exception: + pass + else: + if self._get_volumes_ref_cnt(name) > 0: + pool.destroy() + raise InvalidOperation('KCHPOOL0038E', {'name': name}) + else: + pool.destroy() pool.undefine() except libvirt.libvirtError as e: raise OperationFailed("KCHPOOL0011E", -- 1.8.4.2
Not sure we still need this. https://github.com/kimchi-project/kimchi/wiki/Todo-1.2#wiki-12-sprint-4-ends... *Backend* Implement integrity verification: * don't disable network/storagpool used by guest/template On 03/12/2014 10:50 PM, shaohef@linux.vnet.ibm.com wrote:
From: ShaoHe Feng <shaohef@linux.vnet.ibm.com>
Make a integrity verification when user try to deactivate or delete a pool. If a volume of a pool is used by a vm, the pool should not be deactivated or deleted to avoid problems.
It is no harmful to allow to deactivate/delete dir pool, for kimchi does not delete the pool path when delete this pool.
But it is harmful for other type pools, such as logical or nfs pool.
Royce has added a ref_cnt for volume, if the sum of ref_cnt in this pool larger than 0, means this pool is used by some vms.
Signed-off-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com> --- src/kimchi/i18n.py | 2 ++ src/kimchi/model/storagepools.py | 23 +++++++++++++++++++++++ 2 files changed, 25 insertions(+)
diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index 1ae3889..7c8fbc7 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -144,6 +144,8 @@ messages = { "KCHPOOL0034E": _("Unable to deactivate pool %(name)s as it is associated with some templates"), "KCHPOOL0035E": _("Unable to delete pool %(name)s as it is associated with some templates"), "KCHPOOL0036E": _("A volume group named '%(name)s' already exists. Please, choose another name to create the logical pool."), + "KCHPOOL0037E": _("Unable to deactivate pool %(name)s as it is used by some vms"), + "KCHPOOL0038E": _("Unable to delete pool %(name)s as it is used by some vms"),
"KCHVOL0001E": _("Storage volume %(name)s already exists"), "KCHVOL0002E": _("Storage volume %(name)s does not exist in storage pool %(pool)s"), diff --git a/src/kimchi/model/storagepools.py b/src/kimchi/model/storagepools.py index fea19f6..247c374 100644 --- a/src/kimchi/model/storagepools.py +++ b/src/kimchi/model/storagepools.py @@ -179,6 +179,16 @@ class StoragePoolModel(object): else: raise
+ def _get_volumes_ref_cnt(self, name): + # in order to avoid nested import + from kimchi.model.storagevolumes import StorageVolumesModel + from kimchi.model.storagevolumes import StorageVolumeModel + kwargs = {"conn": self.conn, "objstore": self.objstore} + cnts = [StorageVolumeModel(**kwargs).lookup(name, volume)["ref_cnt"] + for volume in StorageVolumesModel(**kwargs).get_list(name)] + + return sum(cnts) + def _get_storagepool_vols_num(self, pool): try: if pool.isActive(): @@ -345,6 +355,9 @@ class StoragePoolModel(object): raise InvalidOperation('KCHPOOL0034E', {'name': name})
pool = self.get_storagepool(name, self.conn) + if pool.isActive() and self._get_volumes_ref_cnt(name) > 0: + raise InvalidOperation('KCHPOOL0037E', {'name': name}) + #FIXME: nfs workaround - do not try to deactivate a NFS pool # if the NFS server is not reachable. xml = pool.XMLDesc(0) @@ -374,6 +387,16 @@ class StoragePoolModel(object): if pool.isActive(): raise InvalidOperation("KCHPOOL0005E", {'name': name}) try: + try: + pool.create(0) + except Exception: + pass + else: + if self._get_volumes_ref_cnt(name) > 0: + pool.destroy() + raise InvalidOperation('KCHPOOL0038E', {'name': name}) + else: + pool.destroy() pool.undefine() except libvirt.libvirtError as e: raise OperationFailed("KCHPOOL0011E",
-- Thanks and best regards! Sheldon Feng(冯少合)<shaohef@linux.vnet.ibm.com> IBM Linux Technology Center
On 03/12/2014 11:53 AM, Sheldon wrote:
Not sure we still need this. https://github.com/kimchi-project/kimchi/wiki/Todo-1.2#wiki-12-sprint-4-ends... *Backend* Implement integrity verification:
* don't disable network/storagpool used by guest/template
This is also important. In addition only pointing the invalid fields we should also avoid user to create inconsistent resources. The integrity verification may help us identify inconsistent resources that were managed by other tool (like virt-manager, virsh, etc)
On 03/12/2014 10:50 PM, shaohef@linux.vnet.ibm.com wrote:
From: ShaoHe Feng<shaohef@linux.vnet.ibm.com>
Make a integrity verification when user try to deactivate or delete a pool. If a volume of a pool is used by a vm, the pool should not be deactivated or deleted to avoid problems.
It is no harmful to allow to deactivate/delete dir pool, for kimchi does not delete the pool path when delete this pool.
But it is harmful for other type pools, such as logical or nfs pool.
Royce has added a ref_cnt for volume, if the sum of ref_cnt in this pool larger than 0, means this pool is used by some vms.
Signed-off-by: ShaoHe Feng<shaohef@linux.vnet.ibm.com> --- src/kimchi/i18n.py | 2 ++ src/kimchi/model/storagepools.py | 23 +++++++++++++++++++++++ 2 files changed, 25 insertions(+)
diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index 1ae3889..7c8fbc7 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -144,6 +144,8 @@ messages = { "KCHPOOL0034E": _("Unable to deactivate pool %(name)s as it is associated with some templates"), "KCHPOOL0035E": _("Unable to delete pool %(name)s as it is associated with some templates"), "KCHPOOL0036E": _("A volume group named '%(name)s' already exists. Please, choose another name to create the logical pool."), + "KCHPOOL0037E": _("Unable to deactivate pool %(name)s as it is used by some vms"), + "KCHPOOL0038E": _("Unable to delete pool %(name)s as it is used by some vms"),
"KCHVOL0001E": _("Storage volume %(name)s already exists"), "KCHVOL0002E": _("Storage volume %(name)s does not exist in storage pool %(pool)s"), diff --git a/src/kimchi/model/storagepools.py b/src/kimchi/model/storagepools.py index fea19f6..247c374 100644 --- a/src/kimchi/model/storagepools.py +++ b/src/kimchi/model/storagepools.py @@ -179,6 +179,16 @@ class StoragePoolModel(object): else: raise
+ def _get_volumes_ref_cnt(self, name): + # in order to avoid nested import + from kimchi.model.storagevolumes import StorageVolumesModel + from kimchi.model.storagevolumes import StorageVolumeModel + kwargs = {"conn": self.conn, "objstore": self.objstore} + cnts = [StorageVolumeModel(**kwargs).lookup(name, volume)["ref_cnt"] + for volume in StorageVolumesModel(**kwargs).get_list(name)] + + return sum(cnts) + def _get_storagepool_vols_num(self, pool): try: if pool.isActive(): @@ -345,6 +355,9 @@ class StoragePoolModel(object): raise InvalidOperation('KCHPOOL0034E', {'name': name})
pool = self.get_storagepool(name, self.conn) + if pool.isActive() and self._get_volumes_ref_cnt(name) > 0: + raise InvalidOperation('KCHPOOL0037E', {'name': name}) + #FIXME: nfs workaround - do not try to deactivate a NFS pool # if the NFS server is not reachable. xml = pool.XMLDesc(0) @@ -374,6 +387,16 @@ class StoragePoolModel(object): if pool.isActive(): raise InvalidOperation("KCHPOOL0005E", {'name': name}) try: + try: + pool.create(0) + except Exception: + pass + else: + if self._get_volumes_ref_cnt(name) > 0: + pool.destroy() + raise InvalidOperation('KCHPOOL0038E', {'name': name}) + else: + pool.destroy() pool.undefine() except libvirt.libvirtError as e: raise OperationFailed("KCHPOOL0011E",
-- Thanks and best regards!
Sheldon Feng(???)<shaohef@linux.vnet.ibm.com> IBM Linux Technology Center
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
On 03/12/2014 11:50 AM, shaohef@linux.vnet.ibm.com wrote:
From: ShaoHe Feng <shaohef@linux.vnet.ibm.com>
Make a integrity verification when user try to deactivate or delete a pool. If a volume of a pool is used by a vm, the pool should not be deactivated or deleted to avoid problems.
It is no harmful to allow to deactivate/delete dir pool, for kimchi does not delete the pool path when delete this pool.
We should allow the user deactivate/delete a dir pool if there is any volume used. If reasonable we could add *later* an option to user ask Kimchi to also clean up file system while deleting the pool. But it needs to be discussed as I don't have a right answer for that right now.
But it is harmful for other type pools, such as logical or nfs pool.
Royce has added a ref_cnt for volume, if the sum of ref_cnt in this pool larger than 0, means this pool is used by some vms.
Signed-off-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com> --- src/kimchi/i18n.py | 2 ++ src/kimchi/model/storagepools.py | 23 +++++++++++++++++++++++ 2 files changed, 25 insertions(+)
diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index 1ae3889..7c8fbc7 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -144,6 +144,8 @@ messages = { "KCHPOOL0034E": _("Unable to deactivate pool %(name)s as it is associated with some templates"), "KCHPOOL0035E": _("Unable to delete pool %(name)s as it is associated with some templates"), "KCHPOOL0036E": _("A volume group named '%(name)s' already exists. Please, choose another name to create the logical pool."), + "KCHPOOL0037E": _("Unable to deactivate pool %(name)s as it is used by some vms"), + "KCHPOOL0038E": _("Unable to delete pool %(name)s as it is used by some vms"),
"KCHVOL0001E": _("Storage volume %(name)s already exists"), "KCHVOL0002E": _("Storage volume %(name)s does not exist in storage pool %(pool)s"), diff --git a/src/kimchi/model/storagepools.py b/src/kimchi/model/storagepools.py index fea19f6..247c374 100644 --- a/src/kimchi/model/storagepools.py +++ b/src/kimchi/model/storagepools.py @@ -179,6 +179,16 @@ class StoragePoolModel(object): else: raise
+ def _get_volumes_ref_cnt(self, name): + # in order to avoid nested import + from kimchi.model.storagevolumes import StorageVolumesModel + from kimchi.model.storagevolumes import StorageVolumeModel + kwargs = {"conn": self.conn, "objstore": self.objstore} + cnts = [StorageVolumeModel(**kwargs).lookup(name, volume)["ref_cnt"] + for volume in StorageVolumesModel(**kwargs).get_list(name)] + + return sum(cnts)
We should expose this value to API. So UI can disable the buttons when pool is in use. As I did for network And also marked default pool as always is use.
+ def _get_storagepool_vols_num(self, pool): try: if pool.isActive(): @@ -345,6 +355,9 @@ class StoragePoolModel(object): raise InvalidOperation('KCHPOOL0034E', {'name': name})
pool = self.get_storagepool(name, self.conn) + if pool.isActive() and self._get_volumes_ref_cnt(name) > 0: + raise InvalidOperation('KCHPOOL0037E', {'name': name}) + #FIXME: nfs workaround - do not try to deactivate a NFS pool # if the NFS server is not reachable. xml = pool.XMLDesc(0) @@ -374,6 +387,16 @@ class StoragePoolModel(object): if pool.isActive(): raise InvalidOperation("KCHPOOL0005E", {'name': name}) try: + try: + pool.create(0) + except Exception: + pass + else: + if self._get_volumes_ref_cnt(name) > 0: + pool.destroy() + raise InvalidOperation('KCHPOOL0038E', {'name': name}) + else: + pool.destroy() pool.undefine() except libvirt.libvirtError as e: raise OperationFailed("KCHPOOL0011E",
participants (3)
-
Aline Manera -
shaohef＠linux.vnet.ibm.com -
Sheldon