On 02/28/2015 07:28 AM, Ramakrishnan G wrote:

Hello All,
Hi!


This is about adding vendor drivers in Ironic.

In Kilo, we have many vendor drivers getting added in Ironic which is a
very good thing.  But something I noticed is that, most of these reviews
have lots of hardware-specific code in them.  This is something most of
the other Ironic folks cannot understand unless they go and read the
hardware manuals of the vendor hardware about what is being done.
Otherwise we just need to blindly mark the file as reviewed.

Now let me pitch in with our story about this.  We added a vendor driver
for HP Proliant hardware (the *ilo drivers in Ironic).  Initially we
proposed this same thing in Ironic that we will add all the hardware
specific code in Ironic itself under the directory drivers/modules/ilo.
But few of the Ironic folks didn't agree on this (Devananda especially
who is from my company :)). So we created a new module proliantutils,
hosted in our own github and recently moved it to stackforge.  We gave a
limited set of APIs for Ironic to use - like get_host_power_status(),
set_host_power(), get_one_time_boot(), set_one_time_boot(), etc. (Entire
list is here
https://github.com/stackforge/proliantutils/blob/master/proliantutils/ilo/operations.py).

We have only seen benefits in doing it.  Let me bring in some examples:

1) We tried to add support for some lower version of servers.  We could
do this without making any changes in Ironic (Review in proliantutils
https://review.openstack.org/#/c/153945/)
2) We are adding support for newer models of servers (earlier we use to
talk to servers in protocol called RIBCL, newer servers we will use a
protocol called RIS) - We could do this with just 14 lines of actual
code change in Ironic (this was needed mainly because we didn't think we
will have to use a new protocol itself when we started) -
https://review.openstack.org/#/c/154403/

Now talking about the advantages of putting hardware-specific code in
Ironic:

*1) It's reviewed by Openstack community and tested:*
No. I doubt if I throw in 600 lines of new iLO specific code that is
here
(https://github.com/stackforge/proliantutils/blob/master/proliantutils/ilo/ris.py)
for Ironic folks, they will hardly take a look at it.  And regarding
testing, it's not tested in the gate unless we have a 3rd party CI for
it.  [We (iLO drivers) also don't have 3rd party CI right now, but we
are working on it.]

*2) Everything gets packaged into distributions automatically:*
Now the hardware-specific code that we add in Ironic under
drivers/modules/<vendor>/ will get packaged into distributions, but this
code in turn will have dependencies  which needs to be installed
manually by the operator (I assume vendor specific dependencies are not
considered by Linux distributions while packaging Openstack Ironic).
Anyone installing Ironic and wanting to manage my company's servers will
again need to install these dependencies manually.  Why not install the
wrapper if there is one too.

I assume we only get these advantages by moving all of hardware-specific
code to a wrapper module in stackforge and just exposing some APIs for
Ironic to use:
* Ironic code would be much cleaner and easier to maintain
* Any changes related to your hardware - support for newer hardware, bug
fixes in particular models of hardware, would be very easy. You don't
need to change Ironic code for that. You could just fix the bug in your
module, release a new version and ask your users to install a newer
version of the module.
* python-fooclient could be used outside Ironic to easily manage foo
servers.
* Openstack CI for free if you are in stackforge - unit tests, flake
tests, doc generation, merge, pypi release everything handled automatically.

I don't see any disadvantages.

Now regarding the time taken to do this, if you have all the code ready
now in Ironic (which assume you will already have), perhaps it will take
a day to do this - half a day for putting into a separate module in
python/github and half a day for stackforge. The request to add
stackforge should get approved in the same day (if everything is all-right).

Let me know all of your thoughts on this.  If we agree, I feel we should
have some documentation on it in our Ironic docs directory.

Thanks for writing this out!

I understand the concern about splitting this community effort, however, I tend to agree with you. Reviewing vendor-specific code does make me feel weird: on one hand I do my best to check it, on another - I realize that I can approve code that has no chances of working just because I can and will miss some hardware-specific detail.

My biggest concern in making it a rule is support level for this external code. I'm not questioning quality of proliantutils :) but I can envision people not putting too much effort into maintaining their 3rd part stuff. E.g. I already saw 3rd party module that is not even on PyPI (and does not have any tests). I don't know what to do in this case. Of course we can say "that's none of our business", but in the end, if Ironic does not work for their consumers, it's our shared fault.

So in the end, I'm +1 to the procedure itself, but -0 to making it a rule. But yeah, it's worth discussing. Thanks!


Thanks for reading :)

Regards,
Ramesh



__________________________________________________________________________
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

Reply via email to