[PATCH 0/2] Make vms model singleton
From: Royce Lv <lvroyce@linux.vnet.ibm.com> When using get_list function of vms model, Background Task will run multiple times because of started in vm init function. Fix this by making vms model singleton. Royce Lv (2): Make vm model to be a singleton Hack test_model to support vm model singleton src/kimchi/model/vms.py | 3 +++ tests/test_model.py | 2 ++ 2 files changed, 5 insertions(+) -- 1.8.1.2
From: Royce Lv <lvroyce@linux.vnet.ibm.com> When refer vm model get_list method, the stats collection task will run multiple times. Make vms model class a singleton so that this task just run once. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/model/vms.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/kimchi/model/vms.py b/src/kimchi/model/vms.py index 3ae2048..449acdb 100644 --- a/src/kimchi/model/vms.py +++ b/src/kimchi/model/vms.py @@ -27,6 +27,7 @@ from cherrypy.process.plugins import BackgroundTask from kimchi import vnc from kimchi import xmlutils +from kimchi.basemodel import Singleton from kimchi.exception import InvalidOperation, InvalidParameter from kimchi.exception import MissingParameter, NotFoundError, OperationFailed from kimchi.model.config import CapabilitiesModel @@ -52,6 +53,8 @@ stats = {} class VMsModel(object): + __metaclass__ = Singleton + def __init__(self, **kargs): self.conn = kargs['conn'] self.objstore = kargs['objstore'] -- 1.8.1.2
Reviewed-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com> do you means this BackgroundTask? self.guests_stats_thread = BackgroundTask(GUESTS_STATS_INTERVAL, self._update_guests_stats) we should avoid multiple times to run this BackgroundTask. when refer get_list method? I'm afraid the there are server instances of VMsModel, but need to check where are these instances from the code. On 03/04/2014 06:14 PM, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
When refer vm model get_list method, the stats collection task will run multiple times. Make vms model class a singleton so that this task just run once.
Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/model/vms.py | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/kimchi/model/vms.py b/src/kimchi/model/vms.py index 3ae2048..449acdb 100644 --- a/src/kimchi/model/vms.py +++ b/src/kimchi/model/vms.py @@ -27,6 +27,7 @@ from cherrypy.process.plugins import BackgroundTask
from kimchi import vnc from kimchi import xmlutils +from kimchi.basemodel import Singleton from kimchi.exception import InvalidOperation, InvalidParameter from kimchi.exception import MissingParameter, NotFoundError, OperationFailed from kimchi.model.config import CapabilitiesModel @@ -52,6 +53,8 @@ stats = {}
class VMsModel(object): + __metaclass__ = Singleton + def __init__(self, **kargs): self.conn = kargs['conn'] self.objstore = kargs['objstore']
-- Thanks and best regards! Sheldon Feng(冯少合)<shaohef@linux.vnet.ibm.com> IBM Linux Technology Center
2014/3/4 20:23, Sheldon:
Reviewed-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com>
do you means this BackgroundTask? self.guests_stats_thread = BackgroundTask(GUESTS_STATS_INTERVAL, self._update_guests_stats)
we should avoid multiple times to run this BackgroundTask.
when refer get_list method? I get the same question here. Does that mean class VMsModel will be instantiated multiples times and VMsModel.__init__() will be called multiple times?
I'm afraid the there are server instances of VMsModel, but need to check where are these instances from the code.
Agree. Not sure where are the multiple instances of class VMsModel coming from.
On 03/04/2014 06:14 PM, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
When refer vm model get_list method, the stats collection task will run multiple times. Make vms model class a singleton so that this task just run once.
Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/model/vms.py | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/kimchi/model/vms.py b/src/kimchi/model/vms.py index 3ae2048..449acdb 100644 --- a/src/kimchi/model/vms.py +++ b/src/kimchi/model/vms.py @@ -27,6 +27,7 @@ from cherrypy.process.plugins import BackgroundTask
from kimchi import vnc from kimchi import xmlutils +from kimchi.basemodel import Singleton from kimchi.exception import InvalidOperation, InvalidParameter from kimchi.exception import MissingParameter, NotFoundError, OperationFailed from kimchi.model.config import CapabilitiesModel @@ -52,6 +53,8 @@ stats = {}
class VMsModel(object): + __metaclass__ = Singleton + def __init__(self, **kargs): self.conn = kargs['conn'] self.objstore = kargs['objstore']
On 2014年03月04日 20:23, Sheldon wrote:
Reviewed-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com>
do you means this BackgroundTask? self.guests_stats_thread = BackgroundTask(GUESTS_STATS_INTERVAL, self._update_guests_stats)
we should avoid multiple times to run this BackgroundTask.
when refer get_list method? I'm afraid the there are server instances of VMsModel, but need to check where are these instances from the code. Thanks for your review, Sheldon, I will modify my commit msg on next version.
On 03/04/2014 06:14 PM, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
When refer vm model get_list method, the stats collection task will run multiple times. Make vms model class a singleton so that this task just run once.
Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/model/vms.py | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/kimchi/model/vms.py b/src/kimchi/model/vms.py index 3ae2048..449acdb 100644 --- a/src/kimchi/model/vms.py +++ b/src/kimchi/model/vms.py @@ -27,6 +27,7 @@ from cherrypy.process.plugins import BackgroundTask
from kimchi import vnc from kimchi import xmlutils +from kimchi.basemodel import Singleton from kimchi.exception import InvalidOperation, InvalidParameter from kimchi.exception import MissingParameter, NotFoundError, OperationFailed from kimchi.model.config import CapabilitiesModel @@ -52,6 +53,8 @@ stats = {}
class VMsModel(object): + __metaclass__ = Singleton + def __init__(self, **kargs): self.conn = kargs['conn'] self.objstore = kargs['objstore']
From: Royce Lv <lvroyce@linux.vnet.ibm.com> Because test_model have singleton import for its whole life cycle, the dict of singleton class not cleared even though instance deleted. Hack it so that model instance can be generated again. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- tests/test_model.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_model.py b/tests/test_model.py index a08b3d9..f5989a9 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -33,6 +33,7 @@ import iso_gen import kimchi.objectstore import utils from kimchi import netinfo +from kimchi.basemodel import Singleton from kimchi.exception import InvalidOperation, InvalidParameter from kimchi.exception import NotFoundError, OperationFailed from kimchi.iscsi import TargetClient @@ -46,6 +47,7 @@ class ModelTests(unittest.TestCase): self.tmp_store = '/tmp/kimchi-store-test' def tearDown(self): + Singleton._instances = {} os.unlink(self.tmp_store) def test_vm_info(self): -- 1.8.1.2
On 03/04/2014 07:14 AM, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
When using get_list function of vms model,
I could not find an occurrence when we use VMsModel.get_list() outside its class.
Background Task will run multiple times because of started in vm init function. Fix this by making vms model singleton.
Royce Lv (2): Make vm model to be a singleton Hack test_model to support vm model singleton
src/kimchi/model/vms.py | 3 +++ tests/test_model.py | 2 ++ 2 files changed, 5 insertions(+)
On 2014年03月05日 02:48, Aline Manera wrote:
On 03/04/2014 07:14 AM, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
When using get_list function of vms model,
I could not find an occurrence when we use VMsModel.get_list() outside its class. Aline,
I used this when counting volume reference--query all vm disks and see if there is path match between volume and vmstorage. And for functionality like restart host, host.py: def _get_vms_list_by_state(self, state): conn = self.conn.get() names = [dom.name().decode('utf-8') for dom in conn.listAllDomains(0)] ret_list = [] for name in names: dom = conn.lookupByName(name.encode("utf-8")) info = dom.info() if (DOM_STATE_MAP[info[0]]) == state: ret_list.append(name) return ret_list This is duplicate code for vms.get_list(). I mean, in future extension we may also introduce vms.get_list() and we don't want multiple background task running right? So based on this making vms model singleton make sense to me. And also, for me there is no need to have multiple instance of every model because in run time we just need one. What do you think?
Background Task will run multiple times because of started in vm init function. Fix this by making vms model singleton.
Royce Lv (2): Make vm model to be a singleton Hack test_model to support vm model singleton
src/kimchi/model/vms.py | 3 +++ tests/test_model.py | 2 ++ 2 files changed, 5 insertions(+)
On 03/04/2014 10:56 PM, Royce Lv wrote:
On 2014年03月05日 02:48, Aline Manera wrote:
On 03/04/2014 07:14 AM, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
When using get_list function of vms model,
I could not find an occurrence when we use VMsModel.get_list() outside its class. Aline,
I used this when counting volume reference--query all vm disks and see if there is path match between volume and vmstorage. And for functionality like restart host, host.py:
def _get_vms_list_by_state(self, state): conn = self.conn.get() names = [dom.name().decode('utf-8') for dom in conn.listAllDomains(0)]
ret_list = [] for name in names: dom = conn.lookupByName(name.encode("utf-8")) info = dom.info() if (DOM_STATE_MAP[info[0]]) == state: ret_list.append(name) return ret_list
This is duplicate code for vms.get_list().
I mean, in future extension we may also introduce vms.get_list() and we don't want multiple background task running right? So based on this making vms model singleton make sense to me. And also, for me there is no need to have multiple instance of every model because in run time we just need one. What do you think?
I am OK with it. I was just trying to understand if it is a bug fix patch or a preventive one. As you commented you will need it on future patches.
Background Task will run multiple times because of started in vm init function. Fix this by making vms model singleton.
Royce Lv (2): Make vm model to be a singleton Hack test_model to support vm model singleton
src/kimchi/model/vms.py | 3 +++ tests/test_model.py | 2 ++ 2 files changed, 5 insertions(+)
participants (5)
-
Aline Manera -
lvroyce＠linux.vnet.ibm.com -
Royce Lv
-
Sheldon -
Shu Ming