<div dir="ltr"><div class="gmail_default" style="font-family:monospace,monospace"><br></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jul 8, 2015 at 12:48 AM, Marc Koderer <span dir="ltr"><<a href="mailto:marc@koderer.com" target="_blank">marc@koderer.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class=""><div class="h5"><br>
Am 08.07.2015 um 01:37 schrieb Mike Perez <<a href="mailto:thingee@gmail.com">thingee@gmail.com</a>>:<br>
<br>
> On 18:38 Jun 22, Marc Koderer wrote:<br>
>><br>
>> Am 20.06.2015 um 01:59 schrieb John Griffith <<a href="mailto:john.griffith8@gmail.com">john.griffith8@gmail.com</a>>:<br>
>>> * The BaseVD represents the functionality we require from all drivers.<br>
>>> Yep<br>
>>> <br>
>>> * The additional ABC classes represent features that are not required yet.<br>
>>> * These are represented by classes because some features require a<br>
>>> bundle of methods for it to be fulfilled. Consistency group is an<br>
>>> example. [2]<br>
>>><br>
>>> 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.<br>
>>><br>
>>> * Any driver that wishes to mark their driver as supporting a<br>
>>> non-required feature inherits this feature and fulfills the required<br>
>>> methods.<br>
>>><br>
>>> Yeah... ok, I guess.<br>
>>><br>
>>> * After communication is done on said feature being required, there<br>
>>> would be time for driver maintainers to begin supporting it.<br>
>>> * Eventually that feature would disappear from it's own class and be<br>
>>> put in the BaseVD. Anybody not supporting it would have a broken<br>
>>> driver, a broken CI, and eventually removed from the release.<br>
>>><br>
>>> 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“.<br>
>><br>
>> Reducing the number of classes/optional functions sounds good to me.<br>
>> I think it’s quite valuable to discuss what are the mandatory functions<br>
>> of a cinder driver. Before ABC nobody really cared because all drivers simply raised an run-time exception :)<br>
><br>
> If Marc is fine with this, I see no harm in us trying out John's proposal of<br>
> using decorators in the volume driver class.<br>
><br>
> --<br>
> Mike Perez<br>
<br>
<br>
</div></div>+1 sure, happy to see the code :)<br>
<br>
Regards<br>
<span class=""><font color="#888888">Marc<br>
</font></span><div class=""><div class="h5"><br>
><br>
> __________________________________________________________________________<br>
> OpenStack Development Mailing List (not for usage questions)<br>
> Unsubscribe: <a href="http://OpenStack-dev-request@lists.openstack.org?subject:unsubscribe" rel="noreferrer" target="_blank">OpenStack-dev-request@lists.openstack.org?subject:unsubscribe</a><br>
> <a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" rel="noreferrer" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
<br>
<br>
__________________________________________________________________________<br>
OpenStack Development Mailing List (not for usage questions)<br>
Unsubscribe: <a href="http://OpenStack-dev-request@lists.openstack.org?subject:unsubscribe" rel="noreferrer" target="_blank">OpenStack-dev-request@lists.openstack.org?subject:unsubscribe</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" rel="noreferrer" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
</div></div></blockquote></div><br></div><div class="gmail_extra"><div class="gmail_default" style="font-family:monospace,monospace">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].</div><div class="gmail_default" style="font-family:monospace,monospace"><br></div><div class="gmail_default" style="font-family:monospace,monospace">Here's some more detail on what is bothering me here:</div><div class="gmail_default" style="font-family:monospace,monospace">* Inheritance model</div><div class="gmail_default" style="font-family:monospace,monospace"> </div><div class="gmail_default" style="font-family:monospace,monospace">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:</div><div class="gmail_default" style="font-family:monospace,monospace"><br></div><div class="gmail_default" style="font-family:monospace,monospace"><div class="gmail_default">VolumeDriver(object):</div><div class="gmail_default">-- ISCSIDriver(VolumeDriver):</div><div class="gmail_default">-- FakeISCSIDriver(ISCSIDriver):</div><div class="gmail_default">-- ISERDriver(ISCSIDriver):</div><div class="gmail_default">-- FakeISERDriver(FakeISCSIDriver):</div><div class="gmail_default">-- FibreChannelDriver(VolumeDriver):</div><div class="gmail_default"><br></div><div class="gmail_default">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.</div><div class="gmail_default"><br></div><div class="gmail_default">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. </div><div class="gmail_default"><br></div><div class="gmail_default"><div class="gmail_default">BaseVD(object):</div><div class="gmail_default">-- LocalVD(object):</div><div class="gmail_default">-- SnapshotVD(object):</div><div class="gmail_default">-- ConsistencyGroupVD(object):</div><div class="gmail_default">-- CloneableVD(object):</div><div class="gmail_default">-- CloneableImageVD(object):</div><div class="gmail_default">-- MigrateVD(object):</div><div class="gmail_default">-- ExtendVD(object):</div><div class="gmail_default">-- RetypeVD(object):</div><div class="gmail_default">-- TransferVD(object):</div><div class="gmail_default">-- ManageableVD(object):</div><div class="gmail_default">-- ReplicaVD(object):</div><div class="gmail_default">-- VolumeDriver(ConsistencyGroupVD, TransferVD, ManageableVD, ExtendVD,</div><div class="gmail_default">-- ProxyVD(object): (* my personal favorite*)</div><div class="gmail_default">-- ISCSIDriver(VolumeDriver):</div><div class="gmail_default">-- FakeISCSIDriver(ISCSIDriver):</div><div class="gmail_default">-- ISERDriver(ISCSIDriver):</div><div class="gmail_default">-- FakeISERDriver(FakeISCSIDriver):</div><div class="gmail_default">-- FibreChannelDriver(VolumeDriver):</div><div class="gmail_default"><br></div></div><div class="gmail_default">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. </div><div class="gmail_default"><br></div><div class="gmail_default">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.</div><div class="gmail_default"><br></div><div class="gmail_default">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.</div><div class="gmail_default"><br></div><div class="gmail_default">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.</div><div class="gmail_default"><br></div><div class="gmail_default">The downside of my proposal vs what's in master currently:</div><div class="gmail_default"><br></div><div class="gmail_default">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.</div><div class="gmail_default"><br></div><div class="gmail_default">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. </div><div class="gmail_default"><br></div><div class="gmail_default">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. </div><div class="gmail_default"><br></div><div class="gmail_default">I am hoping that people will actually spend some time analyzing the code, how the abstract base class libraries work etc.</div><div class="gmail_default"><br></div><div class="gmail_default">[1]: <a href="https://review.openstack.org/#/c/201812/">https://review.openstack.org/#/c/201812/</a></div><div class="gmail_default"><br></div><div class="gmail_default">Thanks,</div><div class="gmail_default">John</div></div></div></div>