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 Cloud Volume Type model #17610

Merged
merged 2 commits into from
Jul 25, 2018
Merged

Conversation

mansam
Copy link
Contributor

@mansam mansam commented Jun 19, 2018

https://bugzilla.redhat.com/show_bug.cgi?id=1592900

Adds a basic model for Cloud Volume Types, which are needed for mapping new Cinder volumes to specific backends. This is especially important for Openstack V2V infrastructure mapping. Depends on the schema migration in ManageIQ/manageiq-schema#223

@mansam
Copy link
Contributor Author

mansam commented Jun 19, 2018

@roliveri This is Cinder-related, your feedback would be appreciated.

@mansam mansam changed the title Add Cloud Volume Type model [WIP] Add Cloud Volume Type model Jun 19, 2018
@miq-bot miq-bot added the wip label Jun 19, 2018
@mansam mansam force-pushed the create-cloud-volume-types branch from b90010c to a71a1f6 Compare June 19, 2018 19:37
@roliveri
Copy link
Member

I'm not sure I understand the need for a basic model for volume types. Can't volume types be represented as subclasses of a common base class, and queried accordingly?
It seems the data stored in this new table will not vary within type, so they can probably just be defined as class-specific values of the type subclasses.

@mansam
Copy link
Contributor Author

mansam commented Jun 20, 2018

Volume types are user defined, so I believe they would need to be collected from the Cinder API like any other inventory: https://developer.openstack.org/api-ref/block-storage/v3/index.html#volume-types-types

@roliveri
Copy link
Member

@mansam
Ah, I see. It's a bit confusing because type is overloaded. There are types of volumes like: Cinder, EBS, etc. These are user-defined sub-types within type. Do you think it's worthwhile renaming these classes (and tables) to make this distinction more clear?

@mansam
Copy link
Contributor Author

mansam commented Jun 20, 2018

@roliveri Do you think calling this class/table "CloudVolumeBackingType" would avoid the possible confusion?

@roliveri
Copy link
Member

@mansam I'm not sure. To me, backing type indicates the type of storage on which a virtual volume is based.

Are we really talking about sub-types or sub categories of a given storage type?

I'm not sure it's worth spending too much time on. Maybe a comment in the model class definition will be enough to clear up any confusion.

@mansam
Copy link
Contributor Author

mansam commented Jun 20, 2018

Cinder Volume Types are mostly a way to set the backend on which to create a volume, so using BackingType in the name probably isn't too confusing. But I agree with you that a comment is probably more than sufficient.

@miq-bot
Copy link
Member

miq-bot commented Jun 20, 2018

Checked commits mansam/manageiq@a71a1f6~...a204524 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 3 offenses detected

app/models/cloud_volume_type.rb

app/models/manageiq/providers/cloud_manager.rb

app/models/manageiq/providers/storage_manager/block_mixin.rb

@mansam mansam changed the title [WIP] Add Cloud Volume Type model (Depends on ManageIQ/manageiq-schema#223) Add Cloud Volume Type model Jun 22, 2018
@miq-bot miq-bot removed the wip label Jun 22, 2018
@aufi
Copy link
Member

aufi commented Jun 28, 2018

👍

@mansam mansam changed the title (Depends on ManageIQ/manageiq-schema#223) Add Cloud Volume Type model Add Cloud Volume Type model Jul 24, 2018
@mansam
Copy link
Contributor Author

mansam commented Jul 24, 2018

ManageIQ/manageiq-schema#223 has been merged, so this can be merged.

Copy link
Contributor

@Ladas Ladas left a comment

Choose a reason for hiding this comment

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

👍 looks good

@agrare agrare merged commit ddf8fca into ManageIQ:master Jul 25, 2018
@agrare agrare added this to the Sprint 91 Ending Jul 30, 2018 milestone Jul 25, 2018
aufi added a commit to ManageIQ/manageiq-providers-openstack that referenced this pull request Jul 25, 2018
(Depends on ManageIQ/manageiq#17610) Collect Cinder Volume Types during Inventory Refresh
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.

6 participants