[Kimchi-devel] [PATCH 00/13][WIP] Refactor model
Zhou Zheng Sheng
zhshzhou at linux.vnet.ibm.com
Wed Jan 22 02:31:42 UTC 2014
on 2014/01/22 02:22, Daniel H Barboza wrote:
>
> 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.
>
>
I like most of Aline's change except the backend idea, and this
monolithic libvirtbackend makes the whole refactor not useful at all. It
introduces extra layer of abstraction but does not solve the problem. We
should not merge it, otherwise we will have to run another round of big
change to solve the new problem, and this new round of change has to
work with the complexity added in the previous run, so it makes the
things worse. I am not against merging a half solution. We can always
merge a half solution when the solution is towards the better way. The
current situation is different. The monolithic backend is a bad idea,
and the solution is not towards a better way, so I'm against it.
Maybe you think we can merge the current patches and break the
monolithic backend later. I think why don't we break the monolithic
model class now, just as the same way we would have to break the backend
later?
It's a technical debt we have to bear. We should not make new debt at
this point even if we have to delay some tasks. Slowing down and solving
this problem cleanly would make benefit for us in the long run.
Maybe we can just drop the idea of introducing a "backend", and apply my
proposed mechanism in the patch "Break the 'sample' plugin's monolithic
model into several smaller ones" into Aline's patches, and see if it
solves the problem.
--
Thanks and best regards!
Zhou Zheng Sheng / 周征晟
E-mail: zhshzhou at linux.vnet.ibm.com
Telephone: 86-10-82454397
More information about the Kimchi-devel
mailing list