Open Stack

On 07/23/2014 03:04 PM, Dan Smith wrote:
>> FWIW, I do actually agree with not exposing plugin points to things
>> that are not stable APIs and if they didn't already exist, I'd not
>> approve adding them. I'd actually go further and say not even the
>> virt driver API should be a plugin point, since we arbitrarily change
>> it during development any time we need to. The latter is not a serious
>> or practical view right now though given our out of tree Docker/Ironic
>> drivers. I'm just concerned that we've had these various extension
>> points exposed for a long time and we've not clearly articulated
>> that they are liable to be killed off (besides marking vif_driver
>> as deprecated)
>
> Yep, I think we agree. I think that as a project we've identified
> exposing plug points that aren't stable (or intended to be replaceable)
> as a bad thing, and thus we should be iterating on removing them.
> Especially if we're generous with our deprecate-before-remove rules,
> then I think that we're not likely to bite anyone suddenly with
> something they're shipping while working it upstream in parallel. I
> *really* thought we had called this one out on the ReleaseNotes, but
> apparently that didn't happen (probably because we decide to throw in
> those helper classes to avoid breaking configs). Going forward, marking
> it deprecated in the code for a cycle, noting it on the release notes,
> and then removing it the next cycle seems like plenty of warning.

The following are "plugin" points that I feel should be scrapped (sorry, 
I mean deprecated over a release cycle), as they really are not things 
that anyone actually provides extensions for and, IMO, they just add 
needless code abstraction, noise and indirection:

All of these are pointless:

* metadata_manager=nova.api.manager.MetadataManager
* compute_manager=nova.compute.manager.ComputeManager
* console_manager=nova.console.manager.ConsoleProxyManager
* consoleauth_manager=nova.consoleauth.manager.ConsoleAuthManager
* cert_manager=nova.cert.manager.CertManager
* scheduler_manager=nova.scheduler.manager.SchedulerManager
* db_driver=nova.db (pretty sure that ship has long since sailed)
* network_api_class=nova.network.api.API
* volume_api_class=nova.volume.cinder.API
* manager=nova.cells.manager.CellsManager
* manager=nova.conductor.manager.ConductorManager

Then there are the funnies:

This should not be a manager class at all, but rather a selector that 
switches the behaviour of the underlying network implementation -- i.e. 
it should not be swapped out by custom code but instead just have a 
switch option to indicate the type of network model in use):

* network_manager=nova.network.manager.VlanManager

Same goes for this one, which should just be selected based on the 
network model:

* l3_lib=nova.network.l3.LinuxNetL3

These ones should similarly be selected based on the binding_type, not 
provided as a plugin point (as Ian Wells alluded to):

* vif_driver=nova.virt.libvirt.vif.LibvirtGenericVIFDriver
* vif_driver=nova.virt.xenapi.vif.XenAPIBridgeDriver

These config options should be renamed to use driver, not manager:

* floating_ip_dns_manager=nova.network.noop_dns_driver.NoopDNSDriver
* instance_dns_manager=nova.network.noop_dns_driver.NoopDNSDriver
* scheduler_host_manager=nova.scheduler.host_manager.HostManager
* power_manager=nova.virt.baremetal.ipmi.IPMI

This config option should be renamed to use driver, not api_class:

* api_class=nova.keymgr.conf_key_mgr.ConfKeyManager

This one should be renamed to use driver, not handler:

* image_upload_handler=nova.virt.xenapi.image.glance.GlanceStore

This one... who knows? There are no other schedulers for the cells 
module other than this one, and it doesn't follow the same manager -> 
driver pattern as most of Nova, so, should it be called scheduler_driver 
or just scrapped?:

* scheduler=nova.cells.scheduler.CellsScheduler

This one isn't properly set up as a driver-based system but actually 
implements an API, which you'd have to then subclass identically and 
there would be zero point in doing that since you would need to return 
the same data as is set in the Stats class' methods:

* compute_stats_class=nova.compute.stats.Stats

I think it's pretty clear there's lots of room for consistency and 
improvements.

All the best,
-jay