Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add driver_options #14

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add driver_options #14

wants to merge 1 commit into from

Conversation

hemna
Copy link
Contributor

@hemna hemna commented Feb 6, 2019

This patch adds a new cinderlib base method to fetch a specific driver's
driver options.

This depends on a cinder patch here:
https://review.openstack.org/635255

This patch adds a new cinderlib base method to fetch a specific driver's
driver options.

This depends on a cinder patch here:
https://review.openstack.org/635255
def driver_options(volume_driver):
"""Return the driver specific configuration options."""
return importutils.import_class(volume_driver).get_driver_options()

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love the idea of being able to report the configuration options for the drivers!!

I think in most cases the caller will want the information for all drivers, and doing it this way would end up with all the drivers loaded in memory, but I still see value in being able to get the options for a specific driver.

That's why I would like to do 2 things:

  • Add this information to the data returned by list_supported_drivers, which already spawns a different process so we free the memory after we get the info.
  • Make this method also use a different process to retrieve the information

If the Cinder patch adds this information to DriverInfo in cinder/interface/util.py we could get this information directly here!
For the time being I'm going to modify the mapping in list_supported_drivers so it returns a dictionary for each entry instead of a single string. That way it'll be more extensible and allow us to add this information.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm working on the next patchset that will add the info to the cinder/interface/util.py

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok I pushed up a new patchset that includes dumping the options in generate_driver_list.py

http://paste.openstack.org/show/744690/

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The output looks amazing!!
With that mechanism in place cinderlib will be able to return not only name, description, and type, but also default value and choices for each configuration option!
Which is going to be a game changer for cinderlib, as it will help get seamless integrations in other systems and also assist developers.

So we'll just need to convert driver_options to something we can serialize, so we could replace the code here:

for driver in mapping.values():
    driver.pop('cls', None)

With:

for driver in mapping.values():
    driver.pop('cls', None)
    options = driver.get('driver_options')
    if options:
        driver['driver_options'] = {
            opt.name: {'help': opt.help, 'type': '%s' % opt.type,
                              'default': opt.default, 'choices': opt.type.choices}
            for opt in options
        }

Or something like that, since I don't know if the default and choices are right.

@rollandf
Copy link
Contributor

rollandf commented Feb 7, 2019

This will be very helpful!

@Akrog Akrog force-pushed the master branch 2 times, most recently from 4949f22 to 6481cd9 Compare March 13, 2019 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants