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@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@linux.vnet.ibm.com Telephone: 86-10-82454397