[openstack-dev] [Cinder] Implementation of ABC MetaClasses

John Griffith john.griffith8 at gmail.com
Wed Jul 15 20:26:07 UTC 2015

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:

-- 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.

-- 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/

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20150715/46c8c346/attachment.html>

More information about the OpenStack-dev mailing list