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

Allow FloatingIp to relate directly to NetworkRouter #217

Merged
merged 1 commit into from
Jul 19, 2018

Conversation

miha-plesko
Copy link
Contributor

With this commit we add foreign key network_router_id on FloatingIp model in order to be able to connect FloatingIp with NetworkRouter. Currently such connection is only possible through CloudNetworks which does not fit well in Nuage inventoring.

RFE BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1574922

@miq-bot assign @Fryguy
@miq-bot add_label schema

/cc @Ladas

@miq-bot miq-bot added the schema label Jun 7, 2018
@miha-plesko miha-plesko changed the title Allow FloatingIp to relate directly to CloudNetwork Allow FloatingIp to relate directly to NetworkRouter Jun 7, 2018
@miha-plesko
Copy link
Contributor Author

PR that needs this new field: ManageIQ/manageiq-providers-nuage#97

@miha-plesko
Copy link
Contributor Author

@agrare I see you're usually pinged for first review on schema change PRs. So here it is, the ping, if you please.

@@ -0,0 +1,5 @@
class AddNetworkRouterIdToFloatingIp < ActiveRecord::Migration[5.0]
def change
add_reference :floating_ips, :network_router, :type => :bigint, index: true, foreign_key: true
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 any foreign key constraints so can you remove that? Otherwise looks good to me.

With this commit we add foreign key `network_router_id` on FloatingIp model
in order to be able to connect FloatingIp with NetworkRouter. Currently such
connection is only possible through CloudNetworks which does not fit well
in Nuage inventoring.

RFE BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1574922

Signed-off-by: Miha Pleško <miha.plesko@xlab.si>
@miha-plesko
Copy link
Contributor Author

Thanks @agrare , I've removed the foreign key constraint.

@Ladas on gitter you suggested that we connect FloatingIp to NetworkRouter via NetworkPort which would work in case when FloatingIp is assigned to a NetworkPort. However, this is not always the case: FloatingIp can be in "unassigned" state meaning it's not related to any NetworkPort yet. With this commit we'd like to make sure that relation between NetworkRouter and FloatingIp exists even in such case.

@Ladas
Copy link
Contributor

Ladas commented Jun 8, 2018

@miha-plesko do you have a drawing somewhere of what the router connections are? The current schema comes mainly from OpenStack, where you route CloudSubnets to CloudNetwork (as gateway), then I think the floating_ips is a list of available floating ips you can assign to VMs connected to the CloudSubnets

@miha-plesko
Copy link
Contributor Author

@Ladas so our current inventoring plan is as follows:

capture

On Nuage GUI it looks like this (example of L3 network):
capture

From left to right:

  • L3 Domain aka NetworkRouter (yellow)
  • Zone (not planned to be inventoried in MIQ for now) (red)
  • L3 Subnet aka CloudSubnetL3 (blue)
  • vPort aka NetworkPort
  • interface (not planned to be inventoried in MIQ for now)

And example of L2 network:

capture

This time, left-most element is L2 Subnet:

  • L2 Subnet aka CloudSubnetL2 (green)
  • vPort aka NetworkPort
  • interface (not planned to be inventoried in MIQ for now)

My thinking is that we should probably inventory "Zone" as CloudNetwork, but I'm not sure if it would be the right thing to do. So for now I'd just allow for FloatingIp to be related to NetworkRouter.

Looking forward to your comment!

@miha-plesko
Copy link
Contributor Author

@Ladas looking forward to your opinion ^

@Ladas
Copy link
Contributor

Ladas commented Jun 11, 2018

@miha-plesko sorry, I've been traveling on Friday

ok, so for OpenStack having network_router :belongs_to cloud_network, the CloudNetwork is the network we can get the FloatingIps from (having CIDR).

So the question is, how is this modeled in Nuage. It looks like the L3 Domain aka NetworkRouter holds the CIDR? Could we maybe split the model, that the network part of the router would be in CloudNetwork? And the FloatingIps would be associated to that?

@miha-plesko
Copy link
Contributor Author

Closing after discussion with @Ladas. We'll rather inventory also CloudNetworks and connect FloatingIps to them, then things will appear as they should.

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.

ok, looks like we need this, so lets reopen

@miha-plesko
Copy link
Contributor Author

Reopening the PR. In fact it turns out we do need to explicitly inventory connection between FloatingIp and Network router to match how Nuage models relations. See here. @Ladas I kindly ask for merge.

@miha-plesko miha-plesko reopened this Jun 13, 2018
@miq-bot
Copy link
Member

miq-bot commented Jun 13, 2018

Checked commit miha-plesko@8b156fa with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍰

@miha-plesko
Copy link
Contributor Author

@Fryguy can we merge this?

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

@miha-plesko
Copy link
Contributor Author

@gtanzillo @agrare so this is the tiny schema PR that we were talking about in Berlin 1/2

Copy link
Member

@gtanzillo gtanzillo left a comment

Choose a reason for hiding this comment

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

This seems fine to me. @Fryguy you ok with merging?

miha-plesko added a commit to miha-plesko/manageiq-providers-nuage that referenced this pull request Jul 13, 2018
With this commit we remove relation which allows NetworkRouter to
relate to multiple FloatingIps directly (instead via CloudNetwork).
Until ManageIQ/manageiq-schema#217 is merged
we are not able to use such direct relation or else UI crashes.

Signed-off-by: Miha Pleško <miha.plesko@xlab.si>
miha-plesko added a commit to miha-plesko/manageiq-providers-nuage that referenced this pull request Jul 13, 2018
With this commit we remove relation which allows NetworkRouter to
relate to multiple FloatingIps directly (instead via CloudNetwork).
Until ManageIQ/manageiq-schema#217 is merged
we are not able to use such direct relation or else UI crashes.

Signed-off-by: Miha Pleško <miha.plesko@xlab.si>
@Loicavenel
Copy link

@Fryguy HELP please

@Fryguy Fryguy merged commit 0889b06 into ManageIQ:master Jul 19, 2018
@Fryguy Fryguy added this to the Sprint 91 Ending Jul 30, 2018 milestone Jul 19, 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.

7 participants