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.
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 - CloneableVD For the following only BlockDeviceDriver has no implementation : - SnapshopVD - ExtendVD 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 :) Regards Marc [1]: http://paste.openstack.org/show/391303/ Am 15.07.2015 um 22:26 schrieb John Griffith <[email protected]>: > > > On Wed, Jul 8, 2015 at 12:48 AM, Marc Koderer <[email protected]> wrote: > > Am 08.07.2015 um 01:37 schrieb Mike Perez <[email protected]>: > > > On 18:38 Jun 22, Marc Koderer wrote: > >> > >> Am 20.06.2015 um 01:59 schrieb John Griffith <[email protected]>: > >>> * The BaseVD represents the functionality we require from all drivers. > >>> Yep > >>> > >>> * The additional ABC classes represent features that are not required yet. > >>> * These are represented by classes because some features require a > >>> bundle of methods for it to be fulfilled. Consistency group is an > >>> example. [2] > >>> > >>> Sure, I suppose that's fine for things like CG and Replication. > >>> Although I would think that if somebody implemented something optional > >>> like CG's that they should be able to figure out they need all of the > >>> "cg" methods, it's kinda like that warning on ladders to not stand on the > >>> very top rung. By the way, maybe we should discuss the state of > >>> "optional API methods" at the mid-cycle. > >>> > >>> * Any driver that wishes to mark their driver as supporting a > >>> non-required feature inherits this feature and fulfills the required > >>> methods. > >>> > >>> Yeah... ok, I guess. > >>> > >>> * After communication is done on said feature being required, there > >>> would be time for driver maintainers to begin supporting it. > >>> * Eventually that feature would disappear from it's own class and be > >>> put in the BaseVD. Anybody not supporting it would have a broken > >>> driver, a broken CI, and eventually removed from the release. > >>> > >>> Sure, I guess my question is what's the real value in this intermediate > >>> step. The bulk of these are things that I'd argue shouldn't be optional > >>> anyway (snapshots, transfers, manage, extend, retype and even migrate). > >>> Snapshots in particular I find surprising to be considered as "optional“. > >> > >> Reducing the number of classes/optional functions sounds good to me. > >> I think it’s quite valuable to discuss what are the mandatory functions > >> of a cinder driver. Before ABC nobody really cared because all drivers > >> simply raised an run-time exception :) > > > > If Marc is fine with this, I see no harm in us trying out John's proposal of > > using decorators in the volume driver class. > > > > -- > > Mike Perez > > > +1 sure, happy to see the code :) > > Regards > Marc > > > > > __________________________________________________________________________ > > OpenStack Development Mailing List (not for usage questions) > > Unsubscribe: [email protected]?subject:unsubscribe > > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev > > > __________________________________________________________________________ > OpenStack Development Mailing List (not for usage questions) > Unsubscribe: [email protected]?subject:unsubscribe > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev > > 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: [email protected]?subject:unsubscribe > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev __________________________________________________________________________ OpenStack Development Mailing List (not for usage questions) Unsubscribe: [email protected]?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
