Inventory FloatingIps #97

miha-plesko · 2018-06-07T14:52:34Z

With this commit we inventory FloatingIps together with CloudNetworks where IPs originate from. loatingIps are related to NetworkRouter through CloudNetwork.

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

Depends on: ManageIQ/manageiq-schema#217

gberginc

Generally fine, just few minor comments.

Consider marking it WIP if the schema change should be merged before this one.

gberginc · 2018-06-08T08:05:12Z

app/models/manageiq/providers/nuage/inventory/collector/target_collection.rb

@@ -37,6 +37,10 @@ def network_routers
    @network_routers_map.values.compact
  end

+  def floating_ips
+    [] # TODO(miha-plesko): implement targeted refresh for floating ips


Please create issues for this so that we don't forget on them. I see another TODO above.

gberginc · 2018-06-08T08:06:38Z

app/models/manageiq/providers/nuage/inventory/parser/network_manager.rb

+    collector.floating_ips.each do |ip|
+      persister.floating_ips.find_or_build(ip['ID']).assign_attributes(
+        :address        => ip['address'],
+        :network_router => persister.network_routers.lazy_find(ip['parentID']),


Can you explain this bit? Why both network_router and cloud_tenant use the same data from the floating IP?

So ip['parentID] contains ems_ref of corresponding NetworkRouter. Knowing this, you can read the two lines like this:

L73: Assign this FloatingIp to corresponding NetworkRouter - just do lazy lookup by ems_ref.
L74: Assign this FloatingIp to corresponding CloudTenant - but since the JSON we got from API does not contain CloudTenant's ems_ref, we do lazy lookup to get NetworkRouter and copy cloud_tenant from it.

Oh, I missed the fact that you query the network router for its cloud_tenant in the next line. Thanks!

gberginc · 2018-06-08T08:11:51Z

app/models/manageiq/providers/nuage/network_manager/vsd_client/rest.rb

@@ -32,6 +32,13 @@ def append_headers(key, value)
    @headers[key] = value
  end

+  def clear_headers


May I suggest to use a different method name? init_headers, reset_headers or something similar. I had a comment asking why are you clearing the headers here. After I saw this, I understand that you are basically resetting them.

Good point, thanks. In fact I decided to open a separate PR to fix the vsd client itself so that this one can focus on FloatingIp inventoring only.

#99

miha-plesko

Thanks @gberginc

miha-plesko · 2018-06-08T08:19:38Z

app/models/manageiq/providers/nuage/inventory/collector/target_collection.rb

@@ -37,6 +37,10 @@ def network_routers
    @network_routers_map.values.compact
  end

+  def floating_ips
+    [] # TODO(miha-plesko): implement targeted refresh for floating ips


miha-plesko · 2018-06-08T08:26:23Z

app/models/manageiq/providers/nuage/inventory/parser/network_manager.rb

+    collector.floating_ips.each do |ip|
+      persister.floating_ips.find_or_build(ip['ID']).assign_attributes(
+        :address        => ip['address'],
+        :network_router => persister.network_routers.lazy_find(ip['parentID']),


So ip['parentID] contains ems_ref of corresponding NetworkRouter. Knowing this, you can read the two lines like this:

L73: Assign this FloatingIp to corresponding NetworkRouter - just do lazy lookup by ems_ref.
L74: Assign this FloatingIp to corresponding CloudTenant - but since the JSON we got from API does not contain CloudTenant's ems_ref, we do lazy lookup to get NetworkRouter and copy cloud_tenant from it.

miha-plesko · 2018-06-08T09:32:05Z

app/models/manageiq/providers/nuage/network_manager/vsd_client/rest.rb

@@ -32,6 +32,13 @@ def append_headers(key, value)
    @headers[key] = value
  end

+  def clear_headers


Good point, thanks. In fact I decided to open a separate PR to fix the vsd client itself so that this one can focus on FloatingIp inventoring only.

#99

miq-bot · 2018-06-12T09:21:23Z

This pull request is not mergeable. Please rebase and repush.

miha-plesko · 2018-06-12T14:39:11Z

@Ladas I've updated the code to leverage CloudNetwork when relating FloatingIp to NetworkRouter. HOWEVER there are two issues with such approach:

a) on Nuage GUI you can allocate floating IP from external network to a router (which can then allocate it to vPort beneath it). With our model here this is not possible: FloatingIp is connected to external network but MIQ is not aware of what router is it allocated to. So I think we still need to add that additional field to FloatingIp model.

b) on Nuage GUI you can allocate floating IPs from different external networks to the same router. With our model, this is not possible, you can only assign a single one.

We can have a call tomorrow so that I show you how it looks like on Nuage GUI. Would be great to push this through this week so that we can then model also NetworkPorts and be done with inventory upgrade.

Ladas · 2018-06-12T14:58:40Z

@miha-plesko
a) ok, looks like we need network_router_id in FloatingIp

b) so in Nuage, do you do some explicit connection of a Router and External network? Sounds like the connection is done only by allocation the floating IP to router (meaning you can pick from any external network?). So might be enough that the floating_ip has info from what network and router it comes from.

miha-plesko · 2018-06-13T05:36:30Z

Yes @Ladas you're correct about the b) - there is no explicit connection between the router and external netrwork, only FloatingIp itself is aware of what external network did it come from. I'm going to update this PR to match following behavior:

inventory exernal networks as CloudNetwork
inventory NetworkRouters (no connection to CloudNetwork)
inventory FloatingIps and connect them to both NetworkRouter and CloudNetwork

Then I think we're good.

miha-plesko · 2018-06-13T13:54:20Z

There you go @Ladas, I've updated the PR and I think relations between routers, floating ips and external natworks are now in great shape. Looking forward to your review. Will need to wait shema update before we merge.

Ladas

Looks great 👍

With this commit we inventory both FloatingIp and CloudNetwork where the FloatingIp originates from. FloatingIps are directly related to NetworkRouter while NetworkRouter has no direct relation to CloudNetwork (but is connected via FloatingIps). RFE BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1574922 Signed-off-by: Miha Pleško <miha.plesko@xlab.si>

miq-bot · 2018-06-15T10:55:13Z

Checked commit miha-plesko@963e139 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
11 files checked, 5 offenses detected

app/models/manageiq/providers/nuage/inventory/parser/network_manager.rb

❗ - Line 83, Col 9 - Layout/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 84, Col 9 - Layout/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 87, Col 9 - Layout/AlignHash - Align the elements of a hash literal if they span more than one line.

app/models/manageiq/providers/nuage/network_manager/vsd_client.rb

❗ - Line 85, Col 9 - Naming/AccessorMethodName - Do not prefix reader method names with get_.
❗ - Line 89, Col 9 - Naming/AccessorMethodName - Do not prefix reader method names with get_.

miha-plesko · 2018-06-15T10:56:35Z

@Ladas I've commented out the line where we bind FloatingIp to NetworkRouter and put TODO there so that we can, should you agree, merge this PR today. Once the schema PR will get merged I'll uncomment them. Would this work for you?

Ladas · 2018-06-15T12:23:56Z

@miha-plesko sounds good 👍

miha-plesko added the enhancement label Jun 7, 2018

miha-plesko assigned Ladas Jun 7, 2018

miha-plesko requested a review from Ladas June 7, 2018 14:52

miha-plesko mentioned this pull request Jun 7, 2018

Allow FloatingIp to relate directly to NetworkRouter ManageIQ/manageiq-schema#217

Merged

miha-plesko force-pushed the inventory-floating-ips branch from 923af51 to 8aa708f Compare June 8, 2018 07:54

gberginc reviewed Jun 8, 2018

View reviewed changes

miha-plesko changed the title ~~Inventory FloatingIps~~ [WIP] Inventory FloatingIps Jun 8, 2018

miq-bot added the wip label Jun 8, 2018

miha-plesko commented Jun 8, 2018

View reviewed changes

miq-bot added the unmergeable label Jun 12, 2018

miha-plesko force-pushed the inventory-floating-ips branch 2 times, most recently from 4f77b00 to 4d0d682 Compare June 12, 2018 14:21

miha-plesko force-pushed the inventory-floating-ips branch 2 times, most recently from 563d73b to f2d98cf Compare June 13, 2018 13:52

Ladas approved these changes Jun 13, 2018

View reviewed changes

miq-bot removed the unmergeable label Jun 13, 2018

miha-plesko force-pushed the inventory-floating-ips branch from f2d98cf to 963e139 Compare June 15, 2018 10:49

miha-plesko changed the title ~~[WIP] Inventory FloatingIps~~ Inventory FloatingIps Jun 15, 2018

miq-bot removed the wip label Jun 15, 2018

Ladas merged commit c4a0c07 into ManageIQ:master Jun 15, 2018

Ladas added this to the Sprint 88 Ending Jun 18, 2018 milestone Jun 15, 2018

miha-plesko mentioned this pull request Jul 13, 2018

Temporarily remove many-to-many from NetworkRouter #102

Merged

miha-plesko deleted the inventory-floating-ips branch July 24, 2018 08:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inventory FloatingIps #97

Inventory FloatingIps #97

miha-plesko commented Jun 7, 2018 •

edited

Loading

gberginc left a comment

gberginc Jun 8, 2018

miha-plesko Jun 8, 2018

gberginc Jun 8, 2018

miha-plesko Jun 8, 2018

gberginc Jun 8, 2018

gberginc Jun 8, 2018

miha-plesko Jun 8, 2018

miha-plesko left a comment

miha-plesko Jun 8, 2018

miha-plesko Jun 8, 2018

miha-plesko Jun 8, 2018

miq-bot commented Jun 12, 2018

miha-plesko commented Jun 12, 2018

Ladas commented Jun 12, 2018

miha-plesko commented Jun 13, 2018

miha-plesko commented Jun 13, 2018

Ladas left a comment

miq-bot commented Jun 15, 2018

miha-plesko commented Jun 15, 2018

Ladas commented Jun 15, 2018

Inventory FloatingIps #97

Inventory FloatingIps #97

Conversation

miha-plesko commented Jun 7, 2018 • edited Loading

gberginc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miha-plesko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Jun 12, 2018

miha-plesko commented Jun 12, 2018

Ladas commented Jun 12, 2018

miha-plesko commented Jun 13, 2018

miha-plesko commented Jun 13, 2018

Ladas left a comment

Choose a reason for hiding this comment

miq-bot commented Jun 15, 2018

miha-plesko commented Jun 15, 2018

Ladas commented Jun 15, 2018

miha-plesko commented Jun 7, 2018 •

edited

Loading