netapp, nfs, storwize and rbd plugins register opts only after instantiation

Bug #1179159 reported by Tom Fifield
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Cinder
Fix Released
Medium
John Griffith

Bug Description

Hi,

In most plugins, configuration options are defined prior to the class, then registered directly below the definition, eg:

volume_opts = [
    cfg.StrOpt('volume_group',
               default='cinder-volumes',
               help='Name for the VG that will contain exported volumes'),
....
    cfg.IntOpt('lvm_mirrors',
               default=0,
               help='If set, create lvms with multiple mirrors. Note that '
                    'this requires lvm_mirrors + 2 pvs with available space'),
]

FLAGS = flags.FLAGS
FLAGS.register_opts(volume_opts)

class LVMVolumeDriver(driver.VolumeDriver):
...

This practice of registering opts seems to be common across other OpenStack products using oslo.config

However for netapp, nfs, storwize and rbd, the options are registered on instantiation of the class, using self.configuration.append_config_values().

I am not a cinder developer, and there may be good reason for this different way of doing things. However, I find the options being registered on import to be quite convenient - and am actually relying on this practice to automatically generate configuration documentation ;)

Thoughts?

Revision history for this message
John Griffith (john-griffith) wrote :

We actually need to switch over to using cfg everywhere properly anyway and at the same time should fix this back up. We started this a while back (using configuration.append in __init__); and I'm not sure our logic on why we needed to do it was actually justified.

Changed in cinder:
status: New → Confirmed
importance: Undecided → Medium
Changed in cinder:
assignee: nobody → John Griffith (john-griffith)
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to cinder (master)

Fix proposed to branch: master
Review: https://review.openstack.org/31134

Changed in cinder:
status: Confirmed → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to cinder (master)

Reviewed: https://review.openstack.org/31134
Committed: http://github.com/openstack/cinder/commit/0b553dcaf8b158872c472fc0dbd9d46d2fc27d25
Submitter: Jenkins
Branch: master

commit 0b553dcaf8b158872c472fc0dbd9d46d2fc27d25
Author: John Griffith <email address hidden>
Date: Thu May 30 16:05:45 2013 -0600

    Fix config registration in cinder volume drivers.

    The config documentation relies on options being registered
    on a modules import. Our need to move the drivers to using
    self.configuration for multi-backend support means that options
    wouldn't be loaded until object initialization which breaks
    documentation.

    This patch puts a dummy CONF init/load back in the drivers. While putting
    this change together I came across a number of drivers still using FLAGS,
    and even worse a number of drivers using a mixture of FLAGS and CONF and
    self.configuraiton. So most of those are cleaned up here as well.

    Note there are two drivers that were not updated at all here:
      1. windows.py
      2. zadara.py

    The zadara folks have indicated that they're in the process of updating and
    releasing a new version of their driver so I left that as is.

    The windows driver needs a bit of work to switch over.

    Fixes bug: 1179159

    Change-Id: I90165299bf080da17741d027e36e361540da0ff8

Changed in cinder:
status: In Progress → Fix Committed
Thierry Carrez (ttx)
Changed in cinder:
milestone: none → havana-2
status: Fix Committed → Fix Released
Thierry Carrez (ttx)
Changed in cinder:
milestone: havana-2 → 2013.2
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.