On 01/21/2014 06:44 AM, Royce Lv wrote:

On 2014年01月21日 13:09, Zhou Zheng Sheng wrote:
> on 2014/01/21 11:31, Zhou Zheng Sheng wrote:
>> on 2014/01/17 10:24, Aline Manera wrote:
>>> From: Aline Manera <alinefm@br.ibm.com>
>>>
>>> I am sending the first patch set to refactor model It is not
>>> completed. I still need to do a lot of tests and so on. But I
>>> would like to share it with you to get your suggestion.
>>>
>>> Basically, the common code between model and mockmodel was placed
>>> into model/<resource>.py and the specific code into
>>> model/libvirtbackend.py and mockbackend.py
>>>
>>> *** It still needs a lot of work *** - Tests all functions -
>>> Update Makefile and spec files - Update test cases
>>>
>>> Aline Manera (13): refactor model: Create a separated model for
>>> task resource refactor model: Create a separated model for
>>> debugreport resource refactor model: Create a separated model for
>>> config resource refactor model: Create a separated model for host
>>> resource refactor model: Create a separated model for plugin
>>> resource refactor model: Create a separated model for libvirt
>>> connection refactor model: Move StoragePooldef from model to
>>> libvirtstoragepools.py refactor model: Create a separated model
>>> for storagepool resource refactor model: Create a separated model
>>> for storagevolume resource refactor model: Create a separated
>>> model for interface and network resources refactor model: Create
>>> a separated model for template resource refactor model: Create a
>>> separated model for vm resource refactor model: Update server.py
>>> and root.py to use new models
>>>
>>> src/kimchi/control/base.py             |   32 +-
>>> src/kimchi/control/config.py           |   30 +-
>>> src/kimchi/control/debugreports.py     |   18 +-
>>> src/kimchi/control/host.py             |   24 +-
>>> src/kimchi/control/interfaces.py       |   11 +-
>>> src/kimchi/control/networks.py         |   11 +-
>>> src/kimchi/control/plugins.py          |   13 +-
>>> src/kimchi/control/storagepools.py     |   32 +-
>>> src/kimchi/control/storagevolumes.py   |   24 +-
>>> src/kimchi/control/tasks.py            |   11 +-
>>> src/kimchi/control/templates.py        |   11 +-
>>> src/kimchi/control/utils.py            |    2 +-
>>> src/kimchi/control/vms.py              |   17 +-
>>> src/kimchi/mockmodel.py                |  784 --------------
>>> src/kimchi/model.py                    | 1827
>>> -------------------------------- src/kimchi/model/__init__.py
>>> |   21 + src/kimchi/model/config.py             |   52 +
>>> src/kimchi/model/debugreports.py       |   86 ++
>>> src/kimchi/model/host.py               |   49 +
>>> src/kimchi/model/interfaces.py         |   48 +
>>> src/kimchi/model/libvirtbackend.py     |  955 +++++++++++++++++
>>> src/kimchi/model/libvirtconnection.py  |  123 +++
>>> src/kimchi/model/libvirtstoragepool.py |  225 ++++
>>> src/kimchi/model/mockbackend.py        |  338 ++++++
>>> src/kimchi/model/networks.py           |  115 ++
>>> src/kimchi/model/plugins.py            |   29 +
>>> src/kimchi/model/storagepools.py       |   86 ++
>>> src/kimchi/model/storagevolumes.py     |   95 ++
>>> src/kimchi/model/tasks.py              |   45 +
>>> src/kimchi/model/templates.py          |   89 ++
>>> src/kimchi/model/vms.py                |  164 +++
>>> src/kimchi/networkxml.py               |    6 +-
>>> src/kimchi/root.py                     |   24 +-
>>> src/kimchi/server.py                   |   16 +-
>>> src/kimchi/vmtemplate.py               |   18 +- 35 files
>>> changed, 2674 insertions(+), 2757 deletions(-) delete mode 100644
>>> src/kimchi/mockmodel.py delete mode 100644 src/kimchi/model.py
>>> create mode 100644 src/kimchi/model/__init__.py create mode
>>> 100644 src/kimchi/model/config.py create mode 100644
>>> src/kimchi/model/debugreports.py create mode 100644
>>> src/kimchi/model/host.py create mode 100644
>>> src/kimchi/model/interfaces.py create mode 100644
>>> src/kimchi/model/libvirtbackend.py create mode 100644
>>> src/kimchi/model/libvirtconnection.py create mode 100644
>>> src/kimchi/model/libvirtstoragepool.py create mode 100644
>>> src/kimchi/model/mockbackend.py create mode 100644
>>> src/kimchi/model/networks.py create mode 100644
>>> src/kimchi/model/plugins.py create mode 100644
>>> src/kimchi/model/storagepools.py create mode 100644
>>> src/kimchi/model/storagevolumes.py create mode 100644
>>> src/kimchi/model/tasks.py create mode 100644
>>> src/kimchi/model/templates.py create mode 100644
>>> src/kimchi/model/vms.py
>>>
>>
>> I do not like this approach. It just put everything into the
>> libvirtbackend instead of in the current monolithic model class.
>> We still get monolithic libvirtbackend module. While the change is
>> big, but we do not benefit much in terms of logic splitting and
>> modulization.
>>
>> I'm proposing another mechanism. Maybe I'll send the patches
>> later.
>>
>
> The libvirtbackend in "Refactor model" patches is still a monolithic
> class. This is due to the following reasons.
>
> 1. Before the split, all controllers use the same model instance, and
> we can feed the root controller a mock model instance to have all
> controllers work on the mock one.
>
> 2. After the controller split and model split, we want concrete
> controllers call concrete models, for example, VM controller calls
> VM model. This seems OK but there is no way to switch all concrete
> controllers to call mock models.
>
> 3. So comes a new layer of abstraction. A "backend" concept was
> introduced and the "backend" does the actual work. The models share
> the same "backend", and we just feed a real "backend" or mock
> "backend" to switch between production mode and test mode. The key
> problem of "share one thing" v.s. "modulization" is still not
> solved.
>
> I think we have many ways to solve this problem. The key problem is
> after split how we enable the concrete controllers call concrete
> models while we can easily switch between real models and mock
> models.
>
> The first way is that we can have it automatically synthesize a
> single module instance from all the concrete instances, and all the
> controllers still call this single model instance. This is feasible
> because the synthesis is automatic, the logic and responsibilities
> are still split to each concrete model.
I think common logic abstraction is good.
While Zhengsheng's point here is to maintain the consistency between old model api and new model api(e.g. vms_get_list),
So that testcases in test_model.py can switch to newest seamless. What about combine 1st and 2nd proposal?

Yeap! I think that way we will have a more modularized and reusable code

Our aim is:
1. reuse common logic between mockmodel implementation and model implementation.
2. make libvirt or mock implementation confined to there own class and transparent to caller.
3. keep consistency between old model api and refactored api to make test_model.py testcases reusable.

To address 1 and 2 we can adopt Zhengsheng's proposal of factory pattern.
    1. Abstract common logic in a parent class, like we did in VMScreenshot(we need to limit the lookup inside)
    2. libvirt and mock reload their own logic as we do in LibvirtVMScreenshot and MockVMScreenshot.

To address 3 we can use a synthesize single module together with Sheldon's module function probe:
(REF: [Kimchi-devel] [PATCH 1/4] improve controller: add a method to load root sub collections/resouces automatically)

Under model directory: there are vm.py, storagepool.py, etc

in __init__.py:

In fact, in __init__.py I was thinking to add the code made by Zhengsheng (in plugins/__init__.py)
and libvirtmodel.py will have


class LibvirtModel(Model):
    self.vms = VMsModel()
    self.templates = TemplatesModel()
    ....

The same way, Zhengsheng did for RootModel() in "[PATCH] Break the 'sample' plugin's monolithic model into several smaller ones"

class LibvirtModel(object):
    model_type = 'real'

    def __init__(self):

         modules = listPathModules('./')
         for module in modules:
             # probe libvirt vm modules and classes, see sheldon's patch for module probe implementation
             module = imp.load_module()
             cls_names = self._get_classes(module)

             # assgin these class methods to Model class, see zhengsheng's patch for synthesize:
             ....
             m =getattr(model_instance,member_name,None)
             setattr(self,'%s_%s'%(method_prefix,member_name),m)
           

    def _get_classes(module):
        klasses = []
        for name, obj in inspect.getmembers(module):
            if inspect.isclass(obj):
                klasses.append(name)

The whole refactor thing is huge, I suggest we do it one by one, starting from parts which is easy to decouple, like distro, host
and things depend less on each other, not swallow it all at once.
Also Mark has proposal to make the db and libvirt connection to be a singleton so that class does not need to keep instance of it.
He will follow up with this thread.|

>
>
> The second way is to introduce an abstract model factory. We can have
> a "real" model factory instance and a "mock" one to produce only
> "mock" model instance. We let the concrete controller request the
> model factory instance to construct a concrete model instance. Then
> we can easily switch between the production mode and test mode by
> feeding the root controller with a real model factory instance or a
> mock one. This is feasible because the factory just cares the
> creation of a concrete model, the inner model responsibilities are
> still wrapped within the concrete model.
>
> I'm proposing the first way because the change will be minimum and
> non-intrusive. I'm trying to send the patches later...
>