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.

Sign up for GitHub

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

Jump to bottom

Create Physical Disks migration #233

Merged
merged 1 commit into from
Aug 10, 2018
Merged

Create Physical Disks migration #233

merged 1 commit into from
Aug 10, 2018

Conversation

EsdrasVP
Copy link
Member

@EsdrasVP EsdrasVP commented Jul 12, 2018 •
edited
Loading

This PR is able to:

  • Add PhysicalDisks migration into schema with reference to PhysicalStorage

This was referenced Jul 12, 2018
Merged
Merged
@EsdrasVP EsdrasVP changed the title Create Storage Drivers migration [WIP] Create Storage Drivers migration Jul 12, 2018
@EsdrasVP EsdrasVP changed the title [WIP] Create Storage Drivers migration Create Storage Drivers migration Jul 12, 2018
This was referenced Jul 13, 2018
Merged
Merged
Merged
Closed
@EsdrasVP
Copy link
Member Author

@miq-bot add_reviewer @Fryguy

@miq-bot miq-bot requested a review from Fryguy July 18, 2018 16:41
@Fryguy
Copy link
Member

Fryguy commented Jul 25, 2018

@agrare Please review.

@Fryguy Fryguy requested a review from agrare July 25, 2018 15:15
@agrare
Copy link
Member

agrare commented Jul 25, 2018

@EsdrasVP what is a storage driver exactly in this context? Can you give some examples?

@EsdrasVP
Copy link
Member Author

@agrare A storage driver is a disk driver inside a physical storage. It has its own health state (being something like: "Ok" or "Degraded"), size, model, serial number, etc. Initially I thought making Physical Storages have Storages as guest devices, since it is an existing component in ManageIQ, but I noticed they are way more complex than this ones. What do you think of this? We should still consider it as a Storage even when it have less information?

@agrare
Copy link
Member

agrare commented Jul 25, 2018

@EsdrasVP So is it a physical disk drive, a group of drives on a controller, a LUN, a single read/write head? "A storage driver is a disk driver inside a physical storage" is still a little vague.

@EsdrasVP
Copy link
Member Author

@EsdrasVP So is it a physical disk drive, a group of drives on a controller, a LUN, a single read/write head? "A storage driver is a disk driver inside a physical storage" is still a little vague.

@agrare It is a group of physical disk drivers in a controller.

@agrare
Copy link
Member

agrare commented Jul 25, 2018

@agrare It is a group of physical disk drivers in a controller.

Okay thanks, I don't want to spend too much time on a name but IMO storage_driver isn't descriptive enough. We should also try to make sure that we're modeling something that can be realistically applied to other vendor's physical storage arrays.

@agrare
Copy link
Member

agrare commented Jul 26, 2018

@EsdrasVP offline corrected me that a "storage_drive" is actually a single disk, so I proposed "physical_disks" as the table name

@EsdrasVP EsdrasVP changed the title Create Storage Drivers migration Create Physical Disks migration Jul 26, 2018
@EsdrasVP
Copy link
Member Author

@agrare Are the changes ok now?

agrare
agrare reviewed Jul 30, 2018
View reviewed changes
db/migrate/20180720121837_add_port_status_to_physical_network_ports.rb Outdated
@@ -0,0 +1,5 @@
class AddPortStatusToPhysicalNetworkPorts < ActiveRecord::Migration[5.0]
def change
add_column :physical_network_ports, :port_status, :string
Copy link
Member

Choose a reason for hiding this comment

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

Is this related to this change or did this just sneak in?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry for it, this should be in another PR of mine. I'm removing it from here.

agrare
agrare approved these changes Jul 30, 2018
View reviewed changes
Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

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

LGTM

Ladas
Ladas approved these changes Jul 31, 2018
View reviewed changes
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.

👍

@EsdrasVP
Copy link
Member Author

EsdrasVP commented Aug 8, 2018

ping @Fryguy

@agrare
Copy link
Member

agrare commented Aug 8, 2018

@EsdrasVP can you add an index for :physical_storage_id and :location? Them I'm 👍 on this

@bdunne
Copy link
Member

bdunne commented Aug 9, 2018

Is this significantly different from the existing Disk model?

@miq-bot
Copy link
Member

miq-bot commented Aug 9, 2018

Checked commit EsdrasVP@e139888 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 2 offenses detected

db/migrate/20180726142030_create_physical_disk.rb

@agrare
Copy link
Member

agrare commented Aug 9, 2018

@bdunne a lot/most of the properties in the Disk model only apply to virtual/cloud envs:

  • filename
  • mode
  • size_on_disk
  • start_connected
  • backing

Plus Storage and PhysicalStorage are already two different things for similar reasons.

@agrare agrare assigned agrare and unassigned Fryguy Aug 10, 2018
@agrare agrare merged commit 46a826e into ManageIQ:master Aug 10, 2018
@agrare agrare added this to the Sprint 92 Ending Aug 13, 2018 milestone Sep 11, 2018
slemrmartin
slemrmartin reviewed Sep 24, 2018
View reviewed changes
db/migrate/20180726142030_create_physical_disk.rb
t.index :location
t.string :serial_number
t.string :health_state
t.string :type
Copy link
Contributor

Choose a reason for hiding this comment

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

@EsdrasVP what is this type column? It seems it's not related to definition of STI subclass, but some custom value...
When I ran specs, there is for example "SAS" value

Copy link
Member

Choose a reason for hiding this comment

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

Good catch @slemrmartin. @agrare ?

Copy link
Member

Choose a reason for hiding this comment

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

Ah you're right I assumed it was going to be used for STI. We'll need to rename this something like disk_type or actually do STI

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah you're right I assumed it was going to be used for STI. We'll need to rename this something like disk_type or actually do STI

I think we can go for disk_type then, since this has nothing to do with existing STI classes.

@agrare agrare mentioned this pull request Sep 24, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agrare agrare agrare approved these changes

@carbonin carbonin carbonin left review comments

@slemrmartin slemrmartin slemrmartin left review comments

@Ladas Ladas Ladas approved these changes

@Fryguy Fryguy Awaiting requested review from Fryguy

Assignees

@agrare agrare

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@EsdrasVP @Fryguy @agrare @bdunne @miq-bot @Ladas @slemrmartin @carbonin