Open Stack

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