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

Add parent/child support for lans, as well as subnets #69

Merged
merged 1 commit into from
Oct 3, 2017
Merged

Add parent/child support for lans, as well as subnets #69

merged 1 commit into from
Oct 3, 2017

Conversation

djberg96
Copy link
Contributor

This adds a parent_id column to the lans table which will be used to support VM networks for SCVMM and possibly other providers. A VM network will be represented as a child of a LAN.

It also adds a vm_subnets table with a FK to the lans table. The plan is for the AR model to scope the linkage.

@djberg96 djberg96 changed the title [WIP] Add Infra VM networks and subnets [WIP] Add parent/child support for lans, as well as subnets Sep 19, 2017
@miq-bot miq-bot added the wip label Sep 19, 2017
@djberg96
Copy link
Contributor Author

@bronaghs @Ladas @agrare Thoughts on names, columns, etc?

Ladas
Ladas reviewed Sep 20, 2017
View reviewed changes
db/migrate/20170919211745_create_vm_subnets.rb Outdated
@@ -0,0 +1,11 @@
class CreateVmSubnets < ActiveRecord::Migration[5.0]
def change
create_table :vm_subnets do |t|
Copy link
Contributor

Choose a reason for hiding this comment

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

@agrare should we reuse our cloud_subnet model? Ignoring the bad naming for now...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@agrare @Ladas I think that would result in more confusion, especially as that table seems to expect relationships to other cloud_xxx tables.

Copy link
Contributor

Choose a reason for hiding this comment

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

right, so best to get together with @Fryguy, since there seems to be some architecture shift. Not sure if we should use different Model for very similar things (AFAIK subnet is just subnet)

@Fryguy
Copy link
Member

Fryguy commented Sep 20, 2017

We are in the process of modeling a new (alternate?) network model with respect to physical networking, so once we get through that, I'd like to discuss in more detail to see where this and cloud networking fits in.

@Fryguy
Copy link
Member

Fryguy commented Sep 20, 2017

cc @rodneyhbrown7

@bronaghs
Copy link

bronaghs commented Sep 25, 2017 •
edited
Loading

@Fryguy -
Re. your comment above. Can Dan proceed with this SCVMM work as designed or should we put it on hold to determine the impact the Lenovo networking work might have on it.
Note - these SCVMM networking enhancements are slated for the G release - feature freeze is sprint 72.

@agrare
Copy link
Member

agrare commented Sep 25, 2017

Lets confirm with @Fryguy but I think we should make this a generic subnet model that can be used by physical when they get there and make it STI-able so we can subclass it as needed.

When we get further into a generic set of network models we can look at migrating the cloud_* network models over.

@blomquisg
Copy link
Member

blomquisg commented Sep 25, 2017 •
edited
Loading

Ah, I read @Fryguy's comment to mean, after this gets merged for the Gaprindashvili release, let's revisit this and see what refactoring needs to take place. Let's definitely get on the same page for this.

@djberg96
Copy link
Contributor Author

Ok, I've renamed the table to just "subnets" for now.

@djberg96 djberg96 changed the title [WIP] Add parent/child support for lans, as well as subnets Add parent/child support for lans, as well as subnets Sep 25, 2017
@djberg96
Copy link
Contributor Author

Removed the WIP label. @agrare @Fryguy Unless you want something else, I think this is all we need on the schema side for now.

@miq-bot miq-bot removed the wip label Sep 25, 2017
@Fryguy
Copy link
Member

Fryguy commented Sep 25, 2017

@bdunne Thoughts?

Ladas
Ladas approved these changes Sep 26, 2017
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.

Looks good to me, as @agrare said, later we should unify network&subnet models (fun with migrations :-))

@agrare
Copy link
Member

agrare commented Sep 26, 2017

How about adding a cidr field? Maybe type so we can subclass it later?

Fryguy
Fryguy reviewed Sep 26, 2017
View reviewed changes
db/migrate/20170919211745_create_subnets.rb Outdated
t.integer :lan_id
end

add_foreign_key :subnets, :lans
Copy link
Member

Choose a reason for hiding this comment

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

we don't use foreign keys, so please remove

Copy link
Contributor

Choose a reason for hiding this comment

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

We should use foreign keys though, for 5.0, I will be adding them to multiple places. Since it's a new table, it might be worth to treat the DB the right way. ;-) (subnet should always be under network, it's one of the easy models)

That being said, it might open an ugly can of worms, so might be good to have a full release to solve it. :-)

Copy link
Member

Choose a reason for hiding this comment

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

Sure, once we go to 5.0, we can discuss...the original reason was because of replication which is a moot point, but I don't feel comfortable adding them until a major release

@djberg96
Copy link
Contributor Author

@agrare @Fryguy Ok, updated.

Fryguy
Fryguy requested changes Sep 27, 2017
View reviewed changes
Copy link
Member

@Fryguy Fryguy left a comment

Choose a reason for hiding this comment

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

All foreign key _id columns must be a bigint.

db/migrate/20170919211256_add_parent_id_to_lans.rb Outdated
@@ -0,0 +1,5 @@
class AddParentIdToLans < ActiveRecord::Migration[5.0]
def change
add_column :lans, :parent_id, :integer
Copy link
Member

Choose a reason for hiding this comment

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

Must be a bigint

db/migrate/20170919211745_create_subnets.rb Outdated
t.string :name
t.string :cidr
t.string :type
t.integer :lan_id
Copy link
Member

Choose a reason for hiding this comment

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

Must be a bigint

@djberg96
Copy link
Contributor Author

@Fryguy Updated to bigint.

Not sure what the failures are about. The specs pass locally.

@agrare
Copy link
Member

agrare commented Sep 29, 2017

@djberg96 you need to add the rest of the new columns (cidr, type) to the db/schema file

Add VM network and subnet changes.
ceaed23 
Add FK.

Change from vm_subnets to just subnets.

Added cidr and type columns, removed FK.

Update schema.yml
@miq-bot
Copy link
Member

miq-bot commented Sep 29, 2017

Checked commit https://github.com/djberg96/manageiq-schema/commit/ceaed23f2045a7af5a30802ba592837f23b5ace8 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

@djberg96
Copy link
Contributor Author

@agrare I thought I had. Anyway, updated. Thanks!

@djberg96
Copy link
Contributor Author

djberg96 commented Oct 2, 2017

I don't understand what codeclimate is whining about.

@djberg96
Copy link
Contributor Author

djberg96 commented Oct 2, 2017

@Fryguy Updated to bigint.

@Fryguy Fryguy merged commit 6d11d07 into ManageIQ:master Oct 3, 2017
@Fryguy Fryguy added this to the Sprint 71 Ending Oct 16, 2017 milestone Oct 3, 2017
@bronaghs bronaghs mentioned this pull request Oct 5, 2017
Closed
@agrare agrare mentioned this pull request Oct 10, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ladas Ladas Ladas approved these changes

@Fryguy Fryguy Fryguy approved these changes

Assignees
No one assigned
Labels
enhancement schema
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@djberg96 @Fryguy @bronaghs @agrare @blomquisg @miq-bot @Ladas