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 storage manager mixins #13384

Merged
merged 1 commit into from
Jan 11, 2017
Merged

Conversation

gberginc
Copy link
Contributor

@gberginc gberginc commented Jan 6, 2017

Previously the only provider supporting the block and object storage
were Cinder and Swift managers. OpenStack provider was the one actually
integrating them for managing cloud volumes, snapshots, objects, ...

We are now extending the Amazon provider, integrating the EBS (Elastic
Block Store) and S3 (object store). This patch adds two mixins for block
and object store managers. It also alters the save inventory to allow
storage managers to support multiple storage types.

Since Amazon services will be using the same inventory structure,
corresponding save_inventory_cinder and save_inventory_swift have
been renamed to suit block and object storage.

This PR replaces #13351 based on the comments received.

@miq-bot add_label providers, providers/storage, refactoring
@miq-bot assign roliveri

@gberginc
Copy link
Contributor Author

gberginc commented Jan 6, 2017

@roliveri This PR replaces #13351. It introduces mixins for block and object storage managers, handles the inventory saving and migrates existing cinder and swift managers to use the mixins.

Initial implementation for Amazon EBS (block storage) is available in a separate branch until we finalise this.

@roliveri
Copy link
Member

roliveri commented Jan 9, 2017

@gberginc This looks good for the most part. I have one small change request. I think it would be better for the new mixins to reside under the storage_manager directory (and namespace). For example:

the definition of BlockStorageManagerMixin
defined in app/models/mixins/block_storage_manager_mixin.rb

Would become:

ManageIQ::Providers::StorageManager::BlockMixin
defined in: app/models/manageiq/providers/storage_manager/block_mixin.rb.

The same holds true for the ObjectStorageManagerMixin.

@jerryk55 @hsong-rh - Aside from the change requested in this comment, this looks good to me. Please review.

@jerryk55
Copy link
Member

jerryk55 commented Jan 9, 2017

I double-checked to make sure there weren't any Swift- or Cinder-specific specs and apparently that is all rolled into the OpenStack provider - therefore LGTM.

Previously the only provider supporting the block and object storage
were Cinder and Swift managers. OpenStack provider was the one actually
integrating them for managing cloud volumes, snapshots, objects, ...

We are now extending the Amazon provider, integrating the EBS (Elastic
Block Store) and S3 (object store). This patch adds two mixins for block
and object store managers. It also alters the save inventory to allow
storage managers to support multiple storage types.

Since Amazon services will be using the same inventory structure,
corresponding `save_inventory_cinder` and `save_inventory_swift` have
been renamed to suit block and object storage.

Signed-off-by: Gregor Berginc <gregor.berginc@xlab.si>
@gberginc
Copy link
Contributor Author

gberginc commented Jan 9, 2017

@roliveri I've moved mixins into app/models/manageiq/providers/storage_manager and updated the references in Swift and Cinder manager.

@miq-bot
Copy link
Member

miq-bot commented Jan 9, 2017

Checked commit xlab-si@1bbb4fc with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
8 files checked, 5 offenses detected

app/models/ems_refresh/save_inventory_block_storage.rb

  • ❗ - Line 113, Col 29 - Rails/FindBy - Use find_by instead of where.first.
  • ❕ - Line 11, Col 3 - Metrics/AbcSize - Assignment Branch Condition size for save_ems_block_storage_inventory is too high. [20.78/20]

app/models/ems_refresh/save_inventory_object_storage.rb

@gberginc
Copy link
Contributor Author

@roliveri Do you have any other comments on this one? Can it be merged?

@roliveri roliveri merged commit cbeb656 into ManageIQ:master Jan 11, 2017
@chessbyte chessbyte added this to the Sprint 52 Ending Jan 16, 2017 milestone Jan 31, 2017
@tadeboro tadeboro deleted the storage_manager_mixins branch March 30, 2018 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants