Add Cinder storage manager support: backend changes

[hsong-rh]

Purpose or Intent

Add Cinder storage manager to abstract all storage-related information from current Openstack cloud provider's detail page.

Steps for Testing/QA

After Openstack provider is added into appliance, the Cinder storage manager should be added in the 'Storage Providers' tab automatically.

[hsong-rh]

@roliveri @jerryk55 Please review.
@jerryk55 Please replace all Swift related changes with yours.

Thanks,

[jerryk55]

Hui - thanks.

1. Please remove all the swift_storage_manager files. I am working on Swift. Thanks.
2. I find it redundant to name the managers "StorageManager::CinderStorageManager". I believe Rich's original description had the names as "StorageManager::CinderManager" and "StorageManager::SwiftManager" and this makes more sense.

[jerryk55]

These lines - volumes, backups, snapshots - should be moved to the cinder_manager/refresh_parser.rb - which I think you already did - so these are redundant and should be removed.

[hsong-rh]

Yes, will remove these eventually. The reason to keep it is to avoid Travis CI build failure.

[jerryk55]

I don't think you need to worry about Travis failures for WIP PRs. And I'm not sure why leaving Swift in Openstack and not pulling it out into separate files causes Travis failures.

[roliveri]

Does Cinder generate events? What events are we parsing?

[hsong-rh]

I don't know whether Cinder generate events or not, but seems like all kind of providers have their own event parser. If you sure we don't need it, I can remove it.

[jerryk55]

This should be "save_ems_cinder_inventory", right? And there should be a new save_inventory_cinder.rb under app/models/ems_refresh/

[hsong-rh]

Yes, it's better to have separate save_inventory for cinder. I'll add it accordingly. Thanks,

[jerryk55]

Should :vms be saved in cinder?

[roliveri]

vms shouldn't be saved, but I need to change this for the cross linking anyway. I say leave it in for now, and I'll clean it up in my PR.

[roliveri]

Why did you move these into a separate file? These aren't "helper methods" these methods are doing the actual parsing.

[hsong-rh]

Just try to keep the same pattern as other helper_methods. Will combine together in the next PR if needs.

[roliveri]

@hsong-rh Actually, this is contrary to the existing pattern. All of the other parsers have all the parsing code in the same file. As I said, these aren't "helper methods" this is parsing code.

[durandom]

@blomquisg @Fryguy @Ladas we should have a conversation about this one.
We think the layout of the classes is somewhat wrong here.

please dont merge this into euwe yet

[Ladas]

it should not be ManageIQ::Providers::StorageManager::CinderManager

but:
ManageIQ::Providers::Cinder::BlockStorageManager, if there is a separate provider Cinder, otherwise it should be a ManageIQ::Providers::Openstack::BlockStorageManager,

the same for ManageIQ::Providers::StorageManager::SwiftManager:
if there is a separate swift provider it should be ManageIQ::Providers::Swift::ObjectStorageManager, otherwise it should be ManageIQ::Providers::Openstack::ObjectStorageManager,

or you will have a ManageIQ::Providers::Openstack::StorageManager for both block and storage, or subset

[jerryk55]

Several discussions here with @roliveri @Fryguy @hsong-rh and myself about this. I believe the current naming might not be correct but your suggestions may not work either. Both Cinder and Swift can be fully separable and also integrated with other providers. They certainly should not be linked under Openstack. I will let the others here chime in when they log in today.

[roliveri]

@Ladas
The current naming is correct. We intentionally didn't classify storage managers as block or object. The fact is, there are storage managers that support a number of different exported storage types, so name-spacing them doesn't work. Instead storage managers define which exported storage types they support. For example, NetApp storage manager can export block, file share and object. Also any given parent provider can have multiple storage managers, so ManageIQ::Providers::Openstack::StorageManager doesn't work. For example, Openstack may have CInder, Swift, NetApp, EMC, etc.

[chessbyte]

Euwe Backport details:

$ git log
commit 7fdd8b72122181614cf4f52656a013a5eef1b5e9
Author: Richard Oliveri <oliveri.richard.github@gmail.com>
Date:   Wed Sep 21 15:58:35 2016 -0400

    Merge pull request #11088 from hsong-rh/cinder_storage_manager_backend

    Add Cinder storage manager support: backend changes
    (cherry picked from commit 3b6744b19c45f34c3c4eb5df19396ca7e4ee1753)

[Ladas]

If we would follow our current naming and say there can be separate Swift and Cinder managers, then the naming should be

ManageIQ::Providers::Swift::StorageManager and ManageIQ::Providers::Cinder::StorageManager

@Fryguy or have you guys changed the naming schema, so all storage manager are in one dir StorageManager? There was another code lately, assuming 3rd position is always a provider name, but in case of ManageIQ::Providers::StorageManager::CinderManager, it's not.

[durandom]

I still dont like the fact that provider details leak into the main codebase this way. Now I see app/models/ems_refresh/save_inventory_cinder.rb which should be a generic baseclass and the provider should implement a specific subclass.

1. Is there already a know case, where Cinder or Swift is used in another Provider other than OpenStack?
2. If inheritance does not work, we can still use composition. So we could provide modules that enable Swift or Cinder on a providers StorageManager.

@blomquisg @Fryguy @roliveri I recommend to stick to our current naming scheme / where is only the namespace right now, but sometimes also a provider object. And is the defacto EMS or a submanager.

1. Adding another level to this adds complexity (ProviderName/ManagerType/ManagerSubtype).
2. Moving managers for some types removes consistency (StorageManager/ManagerSubtype).

[roliveri]

As I pointed out, that naming scheme doesn't work for the use cases we have for storage managers. You can't just call something "StorageManager" because there can be multiple types of storage managers for any given provider. Also some storage managers can be standalone - so, calling something "storage manager" is like calling a provider "provider".

[roliveri]

BTW, I agree with the save_inventory_cinder part. That was just a stopgap to work around the fact that there are other cloud providers that refresh their storage themselves, and other solutions could impose risk on all cloud providers. This should change going forward.

[Ladas]

@roliveri so what was the exact issue if we would follow the pattern and call them

ManageIQ::Providers::Swift::StorageManager and ManageIQ::Providers::Cinder::StorageManager

(..same for Ceph, Gluster, etc.)

---

Afaik for standalone, you can then have e.g. ManageIQ::Providers::Swift::IdentityManager (following our manager split-up)

The provider here is swift, so it should be wrapped under that.

[roliveri]

Because, logically, StorageManager is a higher-level classification than the specific type of storage manager. Wrapping storage manager implementations under the StorageManager namespace enables us to minimize namespace pollution and enables us to define common StorageManager specific classes and constructs without having to worry about clashing with classes of the same name defined outside the StorageManager namespace (which we've had to deal with int the past). It also enables us to maintain the code for storage managers under a common directory - admittedly, much less an issue once the managers are converted into GEMs.

I think much of the confusion stems from the current provider/manager dichotomy, which seems to have evolved over time - and is probably still evolving. In my view, a Swift storage manager and a swift provider are two different things. The naming scheme you describe eliminates the distinction. A provider is something that can stand on its own and it may employ one or more managers. So, a standalone Swift provider would be under ManageIQ::Providers::Swift, and it would employ the Swift storage manager and whatever identity service it required.

The original intent of the provider/manager scheme was to provide a means to provide multiple views of a given environment. Where the provider defined the environment (An OpenStack instance for example) and the managers provided the views (cloud vs infra). When the concept of manager was extended to include various components of the provider implementation, using provider as the top-level classification became less appropriate - especially when said managers could be mixed and matched between various providers and/or used standalone.

[durandom]

In my view, a Swift storage manager and a swift provider are two different things.

I'm 👍 on this

The naming scheme you describe eliminates the distinction.

I'm all in for your second paragraph, except this line. I dont see why this would be the case.

When the concept of manager was extended to include various components of the provider implementation, using provider as the top-level classification became less appropriate - especially when said managers could be mixed and matched between various providers and/or used standalone.

I still think this scheme makes sense. One Provider, multiple Managers. We still need to improve the visibility of the Provider concept in the UI and we need to work out interconnections of Providers and their managers. But all in all, this concept is, what the providers team is having as their world view :) @blomquisg please correct me if I'm wrong.

@roliveri and please dont see this as an offense, I can understand your reasoning and view on this architectural decision. Its a valid point of view on this.
But nevertheless I think we should align our views, storage and provider team wise.

[durandom]

Linking @Fryguy ancient Provider Architecture issue #519 here.

I mean, in the end, maybe this whole Provider -> Managers relation doesnt make sense, as our UI and users are thinking only in terms of Managers (which they call providers, heck)?!

But as I said, we should align ourselves.

[durandom]

@roliveri just saw the comment #11593 (comment) by @Fryguy that you had that intense discussion already :)

[roliveri]

@durandom I agree, we should be in alignment. Toward that end, we may need to consider adapting the current naming scheme to accommodate types of managers that don't conform to the original assumptions.

The current scheme assumes a provider-specific view of managers - where the implementation of the manager is specific to the provider. For managers that are not provider-specific, the current scheme makes little sense. Forcing these managers into the current scheme results in a sub-category::category (Swift::StorageManagr) structure that isn't appropriate. From a logical, structural and code-reuse standpoint, a category::sub-category (StorageManager::Swift) makes much more sense for these managers. While there may be ways to shoe-horn said managers into the current scheme, I don't think it's the right thing to do.

In my opinion, a single naming scheme may not be able to accommodate all types of managers in a logically optimal way. We may need one scheme that applies to provider-specific managers and another that applies to non-provider-specific managers. Going forward, we may encounter new types of managers that don't conform to our current set of assumptions. Not restricting ourselves to a single naming scheme would enable us to accommodate them more easily.

[blomquisg]

@roliveri I don't see Swift as a category. I see Swift as an implementation. I think the category is ObjectStorage. And, it includes other implementations like IBM Cloud Object Storage or Amazon S3.

But, you're bringing up an important point. Currently, we only really group Provider things together. And, we've sort of tacked on Manager things into that namespace (for instance, CloudManager lives under ManageIQ::Providers).

Maybe it's time to create a ManageIQ::Managers space? /cc @Fryguy

Then, we move the abstract manager things (and, I guess, the implementations) to ManageIQ::Managers and mix those in with the various ManageIQ::Providers. So, things like ManageIQ::Managers::ObjectStorage::AmazonS3 could be used by ManageIQ::Providers::Amazon and ManageIQ::Managers::ObjectStorage::Swift could be used by ManageIQ::Providers::OpenStack.

Ugh, I'm also just seeing this overlapping with #11593 (comment).

We probably need to either start a github issue to collect this, or have a face to face to get the basic points on the table and start over from there.

[roliveri]

@blomquisg I didn't say Swift was category, I said it's a sub-category - in that it should be under a category, and not the other way around (I was pointing out that Swift::StorageManager is backwards). I think we're in agreement here.

You're right, it makes a lot of sense to hammer things out at the Mangers level, before discuss different types of managers. Having a Managers namespace is probably a good idea. At least, it makes a lot of sense for managers that are not provider-specific - but that goes back to my comment that a single namespace scheme may not be sufficient.

Given the examples in your comment, I think we're pretty much in agreement, with some small changes.

Just to give you an idea of some of the requirements we were considering for storage: given the addition of the Managers namespace, I would say that the storage definitions would change to something like: ManageIQ::Managers::Storage::Swift. We don't categorize storage managers by the type of storage they export: object, volume, etc., because a given storage manager may export multiple types of storage.

Given we've probably broken the record for post-merge comments on a single PR, your suggestion to change venue is well taken. We used to discuss things like this in talk topics, but whatever venue people think is most appropriate is fine with me.