On 07/20/2015 07:16 AM, Marc Koderer wrote: > Hello Cinder, > > Instead of reverting nearly everything what was done (and is currently > ongoing). > I would strongly suggest to simply reduce the number of the classes stepwise. >
This makes sense, and this was the general plan as I recall -- to collapse things into the base classes as we could. > I spend some time to analyze what it actually implemented for all the drivers. > > Please see: > > https://docs.google.com/spreadsheets/d/1L_GuUCs-NMVbhbOj8Jtt8vjMQ23zhJ1yagSH4zSKWEw/edit?usp=sharing > > The following classes can be moved to BaseVD directly: > > - ClonableImageVD ClonableImageVD doesn't need to exist anyway IMO, since the functionality still works without it via a generic implementation. > - CloneableVD > > For the following only BlockDeviceDriver has no implementation : > > - SnapshopVD > - ExtendVD BlockDeviceDriver is an odd special case. But, NfsDriver is a real driver and Snapshot support is in progress still. > > This would remove 4 sub classes out of 10. > > I used a script to produce this table [1]. Please let me know if you find a > bug :) > NfsDriver is not abstract. :) (I think I'm going to rename RemoteFS[Snap]Driver to something that doesn't end in "Driver".) > Regards > Marc > > [1]: http://paste.openstack.org/show/391303/ > > > Am 15.07.2015 um 22:26 schrieb John Griffith <john.griffi...@gmail.com>: > >> >> Ok, so I spent a little time on this; first gathering some detail around >> what's been done as well as proposing a patch to sort of step back a bit and >> take another look at this [1]. >> >> Here's some more detail on what is bothering me here: >> * Inheritance model >> >> One of the things the work has done is moved us from a mostly singular >> inheritance OO structure for the Volume Drivers where each level of >> inheritance was specifically for a more general differentiation. For >> example, in driver.py we had: >> >> VolumeDriver(object): >> -- ISCSIDriver(VolumeDriver): >> -- FakeISCSIDriver(ISCSIDriver): >> -- ISERDriver(ISCSIDriver): >> -- FakeISERDriver(FakeISCSIDriver): >> -- FibreChannelDriver(VolumeDriver): >> >> Arguably the fakes probably should be done differently and ISCSI, ISER and >> Fibre should be able to go away if we follow through with the target driver >> work we started. >> >> Under the new model we started with ABC, we ended up with 25 base classes to >> work with, and the base VolumeDriver itself is now composed of 12 other >> independent base classes. >> >> BaseVD(object): >> -- LocalVD(object): >> -- SnapshotVD(object): >> -- ConsistencyGroupVD(object): >> -- CloneableVD(object): >> -- CloneableImageVD(object): >> -- MigrateVD(object): >> -- ExtendVD(object): >> -- RetypeVD(object): >> -- TransferVD(object): >> -- ManageableVD(object): >> -- ReplicaVD(object): >> -- VolumeDriver(ConsistencyGroupVD, TransferVD, ManageableVD, ExtendVD, >> -- ProxyVD(object): (* my personal favorite*) >> -- ISCSIDriver(VolumeDriver): >> -- FakeISCSIDriver(ISCSIDriver): >> -- ISERDriver(ISCSIDriver): >> -- FakeISERDriver(FakeISCSIDriver): >> -- FibreChannelDriver(VolumeDriver): >> >> The idea behind this was to break out different functionality into it's own >> "class" so that we could enforce an entire feature based on whether a >> backend implemented it or not, good idea I think, but hindsight is 20/20 and >> I have some problems with this. >> >> I'm not a fan of having the base VolumeDriver that ideally could be used as >> a template and source of truth be composed of 12 different classes. I think >> this has caused some confusion among a number of contributors. >> >> I think this creates a very rigid model, inheritance is not always a good >> answer; it's the most strict form of coupling and in my opinion should be >> used sparingly and with great care. >> >> This doesn't really accomplish what it set out to do anyway and I believe >> there are cleaner, simpler ways to achieve the same goal. Most of the >> drivers have not converted to or cared about using the new metaclass >> objects, however, simply identifying the required methods and marking them >> with the abc decorator in the base driver will accomplish what we originally >> hoped for (at least what I originally interpreted this to be all about). >> Simply a way to ensure that drivers that didn't implement a required method >> would fail to load, rather than raise NotImplemented at run time when called. >> >> The downside of my proposal vs what's in master currently: >> >> One thing that current implementation does quite nicely is group >> functionality into classes. Consistency groups for example is it's own >> class, and once a driver inherits from it, it ensures that every base method >> for CG support is implemented. It turns out I have a problem with this too >> however. The bulk of the classes have a single method in them, so we build >> a class, instantiate and build a composite object just to check that a >> driver implements "extend_volume"? And that assumes they're even using the >> meta class and not just implementing it on their own anyway. >> >> In addition it's not really solving the intended problem anyway. It's quite >> easy for a driver to just implement the methods without inheriting the base >> metaclass and thereby bypassing all of these checks anyway. >> >> Anyway, I've been called "emotional" and all sorts of other things for >> bringing this up. I figured I'd make one last ML posting (at the request of >> others), submit that patch and just move on from there. >> >> I am hoping that people will actually spend some time analyzing the code, >> how the abstract base class libraries work etc. >> >> [1]: https://review.openstack.org/#/c/201812/ >> >> Thanks, >> John __________________________________________________________________________ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
