Open Stack

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 <john.griffith8 at gmail.com>:

> 
> 
> On Wed, Jul 8, 2015 at 12:48 AM, Marc Koderer <marc at koderer.com> wrote:
> 
> Am 08.07.2015 um 01:37 schrieb Mike Perez <thingee at gmail.com>:
> 
> > On 18:38 Jun 22, Marc Koderer wrote:
> >>
> >> Am 20.06.2015 um 01:59 schrieb John Griffith <john.griffith8 at gmail.com>:
> >>> * 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: OpenStack-dev-request at lists.openstack.org?subject:unsubscribe
> > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
> 
> 
> __________________________________________________________________________
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: OpenStack-dev-request at lists.openstack.org?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: OpenStack-dev-request at lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev