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

2015-07-27 Thread Mike Perez 
On 14:26 Jul 15, John Griffith wrote:
 ​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

Some good discussions happened in the Cinder IRC channel today [1] about this.

To sum things up:

1) Cinder has a matrix of optional features.
2) Majority of people in Cinder are OK with the cost of having multiple classes
   representing features that a driver can choose to support.
3) The benefit of this is seeing which drivers support [2] which features.

People are still interested in discussing this at the next Cinder midcycle
sprint [3].

My decision is going to be that unless folks want to go and remove optional
features like consistency group, replication, etc, we need something to keep
track of things. I think there are current problems with the current
implementation [4], and I do see value in John's proposal if we didn't have
these optional features.


[1] - 
http://eavesdrop.openstack.org/irclogs/%23openstack-cinder/%23openstack-cinder.2015-07-27.log.html#t2015-07-27T16:30:28
[2] - https://review.openstack.org/#/c/160346/
[3] - https://wiki.openstack.org/wiki/Sprints/CinderLibertySprint
[4] - http://lists.openstack.org/pipermail/openstack-dev/2015-June/067572.html

-- 
Mike Perez

__
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

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

2015-07-20 Thread Marc Koderer 
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.griffi...@gmail.com:

 
 
 On Wed, Jul 8, 2015 at 12:48 AM, Marc Koderer m...@koderer.com wrote:
 
 Am 08.07.2015 um 01:37 schrieb Mike Perez thin...@gmail.com:
 
  On 18:38 Jun 22, Marc Koderer wrote:
 
  Am 20.06.2015 um 01:59 schrieb John Griffith john.griffi...@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-requ...@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-requ...@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):
 --

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

2015-07-20 Thread Eric Harney 
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

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

2015-07-15 Thread John Griffith 
On Wed, Jul 8, 2015 at 12:48 AM, Marc Koderer m...@koderer.com wrote:


 Am 08.07.2015 um 01:37 schrieb Mike Perez thin...@gmail.com:

  On 18:38 Jun 22, Marc Koderer wrote:
 
  Am 20.06.2015 um 01:59 schrieb John Griffith john.griffi...@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-requ...@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-requ...@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

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

2015-06-22 Thread Marc Koderer 

Am 20.06.2015 um 01:59 schrieb John Griffith john.griffi...@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 :)

 ​ 
 * Some people had thoughts of having a way generate a matrix with this
 information, which I dislike the idea of.
 
 ​Yeah, back to the dreaded Matrix, I can't imagine anything worse for 
 Cinder than a matrix of supported API calls on a driver per driver basis.​ 

I agree that such a matrix tool isn’t the finial goal.
But it helps to see which functions are already common“ and which are 
optional“.
Just try it [1] ;)

Regards
Marc

[1]: https://review.openstack.org/#/c/160346/
 
 __
 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
 
 __
 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


__
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

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

2015-06-22 Thread Marc Koderer 

Am 20.06.2015 um 02:34 schrieb Knight, Clinton clinton.kni...@netapp.com:

 Funny you should mention needing all of the CG methods...
 
 A VD group (ConsistencyGroupVD) was added to contain the 4 CG methods from 
 Juno.  Then more CG methods were added to VolumeDriver in Kilo, but they 
 weren’t added to ConsistencyGroupVD.
 
 But you *can’t* add them to ConsistencyGroupVD until every driver that 
 advertises ConsistencyGroupVD has implemented them, lest ConsistencyGroupVD 
 imply something false.  You could add them to ‘ConsistencyGroupVD_v2’, but 
 that’s ugly.
 
 This whole VD thing seems just a little too neat, since it doesn’t lend 
 itself to evolution of features over time.  I’ve wondered if a little runtime 
 introspection wouldn’t be a cleaner solution.

I agree that this is an open issue with ABC.
Changing an interface only works if all drivers adapt their interface.
IMHO a runtime exception should be only used as interim solution to get all 
drivers ported.
If the driver interfaces changes quite often we need to use versions.

Regards
Marc 

 
 --
 Clinton Knight
 
 From: John Griffith john.griffi...@gmail.com
 Reply-To: OpenStack Development Mailing List (not for usage questions) 
 openstack-dev@lists.openstack.org
 Date: Friday, June 19, 2015 at 7:59 PM
 To: OpenStack Development Mailing List (not for usage questions) 
 openstack-dev@lists.openstack.org
 Subject: Re: [openstack-dev] [Cinder] Implementation of ABC MetaClasses
 
 ​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
 __
 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


__
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

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

2015-06-22 Thread Duncan Thomas 
On 20 June 2015 at 02:59, John Griffith john.griffi...@gmail.com wrote:


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


The ABC code has not made any new things optional, it has just highlighted
them. They are de-facto optional, since not every driver implements them.
The minute that is fixed, the relevant ABC can go away. I'll see if I can
make a start.

-- 
Duncan Thomas
__
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

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

2015-06-19 Thread John Griffith 
On Fri, Jun 19, 2015 at 5:31 PM, Mike Perez thin...@gmail.com wrote:

 On Fri, Jun 19, 2015 at 3:43 PM, John Griffith john.griffi...@gmail.com
 wrote:
  Anyway, maybe I'm missing a big advantage here, but without understanding
  what this gains it makes the code structure a bit hard to maintain and
  follow in a number of ways.  For example, in order to implement this in
 the
  existing drivers they need to be changed to have an inheritance structure
  like this: [5].  As opposed to just using the decorator which everything
  would automatically propagate to any sub-class and enforce implementation
  with no change or maintenance to existing drivers; but it still enforces
  implementation of the required methods.
 
  I'd love if folks could study some of this a bit and let me know what it
 is
  that I'm missing here.  Is there some value that I'm unaware of that the
  muti-inheritance paradigm in the drivers provides us?

 I remember from the Kilo midcycle meetup [1] that you were part of, we
 discussed this accomplishing a few things:


​Sure, I was there, I even proposed the use of abc as a great idea.  I
didn't catch the part about the optional classes and what that would look
like however.​



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


 * Some people had thoughts of having a way generate a matrix with this
 information, which I dislike the idea of.


​Yeah, back to the dreaded Matrix, I can't imagine anything worse for
Cinder than a matrix of supported API calls on a driver per driver basis.
​



 Why have the middle step of having the non-required features be
 represented in some abc class? I guess it's a form of reference and
 failing sooner for not fulfilling the methods with the correct
 signature. This doesn't check if those methods return the right
 information though. So maybe it's not worth it.


​I'm not sure that works anyway, I mean... I can still implement whatever
methods in my driver I want, if I don't inherit from the VD class it
doesn't matter, it's not checked anywhere otherwise.
​



 Marc provided a patch that would show RBD switching to this model.
 This revealed my main objection to the change which was the ugliness
 of having to inherit from all these classes, however, no one else
 really cared.


​Hmm... I wish I would've caught this at the time, because I most CERTAINLY
would have agreed with you 100% as you can probably tell by my thoughts on
the subject in reviews, IRC and this ML post.
​



 [1] - https://etherpad.openstack.org/p/cinder-meetup-winter-2015
 [2] -
 https://github.com/openstack/cinder/blob/master/cinder/volume/driver.py#L902

 --
 Mike Perez

 __
 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

__
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

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

2015-06-19 Thread Mike Perez 
On Fri, Jun 19, 2015 at 3:43 PM, John Griffith john.griffi...@gmail.com wrote:
 Anyway, maybe I'm missing a big advantage here, but without understanding
 what this gains it makes the code structure a bit hard to maintain and
 follow in a number of ways.  For example, in order to implement this in the
 existing drivers they need to be changed to have an inheritance structure
 like this: [5].  As opposed to just using the decorator which everything
 would automatically propagate to any sub-class and enforce implementation
 with no change or maintenance to existing drivers; but it still enforces
 implementation of the required methods.

 I'd love if folks could study some of this a bit and let me know what it is
 that I'm missing here.  Is there some value that I'm unaware of that the
 muti-inheritance paradigm in the drivers provides us?

I remember from the Kilo midcycle meetup [1] that you were part of, we
discussed this accomplishing a few things:

* The BaseVD represents the functionality we require from all drivers.
* 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]
  * Any driver that wishes to mark their driver as supporting a
non-required feature inherits this feature and fulfills the required
methods.
* 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.
* Some people had thoughts of having a way generate a matrix with this
information, which I dislike the idea of.

Why have the middle step of having the non-required features be
represented in some abc class? I guess it's a form of reference and
failing sooner for not fulfilling the methods with the correct
signature. This doesn't check if those methods return the right
information though. So maybe it's not worth it.

Marc provided a patch that would show RBD switching to this model.
This revealed my main objection to the change which was the ugliness
of having to inherit from all these classes, however, no one else
really cared.

[1] - https://etherpad.openstack.org/p/cinder-meetup-winter-2015
[2] - 
https://github.com/openstack/cinder/blob/master/cinder/volume/driver.py#L902

--
Mike Perez

__
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

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

2015-06-19 Thread Knight, Clinton 
Funny you should mention needing all of the CG methods...

A VD group (ConsistencyGroupVD) was added to contain the 4 CG methods from 
Juno.  Then more CG methods were added to VolumeDriver in Kilo, but they 
weren’t added to ConsistencyGroupVD.

But you *can’t* add them to ConsistencyGroupVD until every driver that 
advertises ConsistencyGroupVD has implemented them, lest ConsistencyGroupVD 
imply something false.  You could add them to ‘ConsistencyGroupVD_v2’, but 
that’s ugly.

This whole VD thing seems just a little too neat, since it doesn’t lend itself 
to evolution of features over time.  I’ve wondered if a little runtime 
introspection wouldn’t be a cleaner solution.

--
Clinton Knight

From: John Griffith john.griffi...@gmail.commailto:john.griffi...@gmail.com
Reply-To: OpenStack Development Mailing List (not for usage questions) 
openstack-dev@lists.openstack.orgmailto:openstack-dev@lists.openstack.org
Date: Friday, June 19, 2015 at 7:59 PM
To: OpenStack Development Mailing List (not for usage questions) 
openstack-dev@lists.openstack.orgmailto:openstack-dev@lists.openstack.org
Subject: Re: [openstack-dev] [Cinder] Implementation of ABC MetaClasses

​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
__
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