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

Create Canister migration #234

Merged
merged 1 commit into from
Aug 30, 2018

Conversation

EsdrasVP
Copy link
Member

@EsdrasVP EsdrasVP commented Jul 13, 2018

This PR is able to:

  • Create a canister migration
  • Add its reference to hardwares table

A Canister is a controller inside the Physical Storage (which may have more than one Canister). It contains its own firmware, ports, network, power state, etc.

Depends on:

@miq-bot miq-bot added the wip label Jul 13, 2018
@EsdrasVP EsdrasVP changed the title [WIP] Create Canister migration Create Canister migration Aug 17, 2018
@miq-bot miq-bot removed the wip label Aug 17, 2018
@EsdrasVP
Copy link
Member Author

@miq-bot assign @agrare

@agrare
Copy link
Member

agrare commented Aug 22, 2018

@EsdrasVP can you add to the PR description what a canister is?

Create a canister migration

Isn't enough for me to understand what is being added

@EsdrasVP
Copy link
Member Author

@EsdrasVP can you add to the PR description what a canister is?

Create a canister migration

Isn't enough for me to understand what is being added

@agrare I updated the description explaining what a Canister is. Can you see if it's more understandable?

@agrare
Copy link
Member

agrare commented Aug 24, 2018

@EsdrasVP is Canister an industry standard term for this type of object? (A quick google search for "storage canister" yielded a lot of amazon results for air-tight coffee bean containers which might just be my recommended search results 😆)

If it has its own controller it almost sounds like a caniter is its own self-contained storage?

@EsdrasVP
Copy link
Member Author

@EsdrasVP is Canister an industry standard term for this type of object? (A quick google search for "storage canister" yielded a lot of amazon results for air-tight coffee bean containers which might just be my recommended search results )

@agrare Other companies might call it by a different name, so I think that's why it's only appearing as coffee bean containers on search results hahaha.

If it has its own controller it almost sounds like a caniter is its own self-contained storage?

Yes, all the work will be done by the Canisters, which will exist inside of a PhysicalStorage. This distinction is important because a Lenovo PhysicalStorage will contain more than one Canister inside it.

@agrare
Copy link
Member

agrare commented Aug 29, 2018

Okay canister seems like a decent enough name don't want to dwell on that.

class CreateCanister < ActiveRecord::Migration[5.0]
def change
create_table :canisters do |t|
t.string :serial_number
Copy link
Member

Choose a reason for hiding this comment

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

2 things here, can you add ems_ref for a unique reference? Even if e.g. position is unique for lenovo we can use ems_ref generally.
Also I assume this belongs to a physical_storage but you don't have a reference to physical_storage here.

Copy link
Member Author

Choose a reason for hiding this comment

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

@agrare I added now ems_ref and physical_storage_id to the migration.

@@ -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.

This looks unrelated to the rest of this PR

Copy link
Member Author

@EsdrasVP EsdrasVP Aug 30, 2018

Choose a reason for hiding this comment

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

@agrare It is, kinda. Because Canister will have some physical ports in it, and I didn't found any reference to this, so I created one. If you want to, I can put it in another PR.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah lets do this in a new PR since port_status can apply to anything not just canisters. So a canister will have hardware -> guest_devices -> physical_network_ports?

Copy link
Member Author

Choose a reason for hiding this comment

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

So a canister will have hardware -> guest_devices -> physical_network_ports?

@agrare Exactly, this relationship it's currently on the core PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah lets do this in a new PR since port_status can apply to anything not just canisters.

@agrare I've done it and put as dependency on this PR, could you take a look there?

@miq-bot
Copy link
Member

miq-bot commented Aug 30, 2018

Checked commit EsdrasVP@2c6bdd2 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

@agrare
Copy link
Member

agrare commented Aug 30, 2018

@EsdrasVP should a physical_disk have a canister_id then?

@EsdrasVP
Copy link
Member Author

@EsdrasVP should a physical_disk have a canister_id then?

@agrare No, because the PhysicalDisks are on the same level as Canisters on a PhysicalStorage. It's quite bad actually, because doesn't allow us to see which canister a disk is inside.

@agrare agrare merged commit 3599bd1 into ManageIQ:master Aug 30, 2018
@agrare agrare added this to the Sprint 94 Ending Sept 10, 2018 milestone Aug 30, 2018
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.

3 participants