[Kimchi-devel] [PATCH 00/13][WIP] Refactor model

Tue Jan 21 18:22:14 UTC 2014

On 01/21/2014 03:09 AM, 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 at 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.
>
> 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...
>
Perhaps this change could be done in a second round of refactoring? My 
point is that Aline have already changed *a lot* of code and it would be 
nice to test these changes ASAP for possible bugs and errors. When we're 
confident that these changes were successful then we can start further 
rounds of refactoring.