Extend NetworkManager inventory #9

mancabizjak · 2019-09-20T10:01:59Z

So far the provider's NetworkManager inventory supported only security groups.

With this commit we extend the NetworkManager inventory with cloud networks, cloud subnets and network ports. Cloud subnets and network ports are connected to security groups, while network
ports are in addition connected to VMs.

miha-plesko

I can't believe the amazing quality level of your first PR on MIQ @mancabizjak , it totally feels like you're developing MIQ for months already and not just 5 days. Kudos 👍 Few small comments anyway

miha-plesko · 2019-09-20T10:14:52Z

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

+      )
+      if network.subnets
+        cloud_subnets(network)
+      end


We could capture the just-built network and pass it to the cloud_subnets function so that we don't lazy_find it over and over again:

network_obj = persister.cloud_networks.build(...) cloud_subnets(network_obj, network.subnets) if network.subnets

Oops, missed that. Thanks, done 👍

miha-plesko · 2019-09-20T10:34:26Z

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

+        :ems_ref         => port.id.downcase,
+        :mac_address     => port.mac_address,
+        :device          => port.virtual_machine ? persister.vms.lazy_find(port.virtual_machine.id.downcase) : nil,
+        :security_groups => build_security_groups(port)


I'm not sure we need utility function here because we're just filling in a simple relation. I'd go with a simple IF statement at the top instead:

security_groups = [] if (security_group_id = port.network_security_group&.id&.downcase) security_groups = [persister.security_groups.lazy_find(security_group_id)] end

This is exactly how I used to have it initially, when I hooked security_groups to network ports only. But then I needed the same logic for subnets and I thought it made sense to extract it to a helper function (note that this is also mirrored by the assert_security_groups_binding which is used in both assert_specific_subnet and assert_specific_network_port).

I did modify the helper itself to make use of &. when accessing entity's security group though 👍

miha-plesko · 2019-09-20T10:40:54Z

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

+        :name            => port.name,
+        :ems_ref         => port.id.downcase,
+        :mac_address     => port.mac_address,
+        :device          => port.virtual_machine ? persister.vms.lazy_find(port.virtual_machine.id.downcase) : nil,


Suggested change

:device => port.virtual_machine ? persister.vms.lazy_find(port.virtual_machine.id.downcase) : nil,

:device => persister.vms.lazy_find(port.virtual_machine&.id&.downcase),

miha-plesko · 2019-09-20T10:47:03Z

...odels/manageiq/providers/azure_stack/cloud_manager/vcr_fixtures/full-refresh-deployment.json

+              "addressPrefix": "[variables('subnetPrefix')]",
+              "networkSecurityGroup": {
+                "id": "[resourceId('Microsoft.Network/networkSecurityGroups', variables('networkSecurityGroupName'))]"
+              }


Awesome 👍

miha-plesko · 2019-09-20T10:50:20Z

spec/models/manageiq/providers/azure_stack/cloud_manager/vcr_specs/refresher_spec.rb

+    expect(subnet).not_to be_nil
+    expect(ems_ref_suffix(subnet.ems_ref)).to match(%r{^/providers/microsoft.network/virtualnetworks/[^/]+/subnets/[^/]+$})
+
+    expect(subnet.cloud_network).not_to be_nil


This assertion is actually redundant because we assert the exact value in the next line anyway

Thanks, I removed a similar redundant case from assert_specific_network_port as well.

miha-plesko · 2019-09-20T10:54:48Z

spec/models/manageiq/providers/azure_stack/cloud_manager/vcr_specs/refresher_spec.rb

+    assert_security_groups_binding(network_port)
+  end
+
+  def assert_security_groups_binding(entity)


I like this DRY approach you took here 👍

agrare · 2019-09-20T10:55:18Z

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

+      persister.cloud_subnets.build(
+        :name            => subnet.name,
+        :ems_ref         => subnet.id.downcase,
+        :cloud_network   => persister.cloud_networks.lazy_find(network.id.downcase),


Since this is called right after building the network you can save the lazy_find and pass in the persisted network object directly if you want.
Example:

persister_network = persister.cloud_networks.build() cloud_subnets(persister_network, network.subnets) if network.subnets

Ah @miha-plesko beat me to it #9 (comment)

agrare · 2019-09-20T10:58:40Z

...models/manageiq/providers/azure_stack/inventory/persister/definitions/network_collections.rb

+       security_groups].each do |name|
+      add_collection(network, name)
+
+      add_collection(cloud, :vms) do |builder|


Is this inside the loop above?

Indeed, fixed.

agrare · 2019-09-20T11:05:51Z

I can't believe the amazing quality level of your first PR on MIQ @mancabizjak , it totally feels like you're developing MIQ for months already and not just 5 days. Kudos 👍

I completely agree. This is a great PR for anyone, for your first this is amazing great work.

So far the provider's NetworkManager inventory supported security groups. With this commit we extend NetworkManager inventory with cloud networks, cloud subnets and network ports. Cloud subnets and network ports are connected to security groups, while network ports are in addition connected to VMs. Signed-off-by: Manca Bizjak <[email protected]>

miq-bot · 2019-09-20T12:38:23Z

Checked commit xlab-si@2bfe503 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
8 files checked, 0 offenses detected
Everything looks fine. 🍰

mancabizjak

Thanks a lot for your reviews and encouraging words @miha-plesko @agrare! Please see the changes I made according to your comments.

mancabizjak · 2019-09-20T11:12:56Z

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

+      )
+      if network.subnets
+        cloud_subnets(network)
+      end


Oops, missed that. Thanks, done 👍

mancabizjak · 2019-09-20T11:38:23Z

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

+        :ems_ref         => port.id.downcase,
+        :mac_address     => port.mac_address,
+        :device          => port.virtual_machine ? persister.vms.lazy_find(port.virtual_machine.id.downcase) : nil,
+        :security_groups => build_security_groups(port)


This is exactly how I used to have it initially, when I hooked security_groups to network ports only. But then I needed the same logic for subnets and I thought it made sense to extract it to a helper function (note that this is also mirrored by the assert_security_groups_binding which is used in both assert_specific_subnet and assert_specific_network_port).

I did modify the helper itself to make use of &. when accessing entity's security group though 👍

mancabizjak · 2019-09-20T11:41:03Z

...models/manageiq/providers/azure_stack/inventory/persister/definitions/network_collections.rb

+       security_groups].each do |name|
+      add_collection(network, name)
+
+      add_collection(cloud, :vms) do |builder|


Indeed, fixed.

mancabizjak · 2019-09-20T11:46:24Z

spec/models/manageiq/providers/azure_stack/cloud_manager/vcr_specs/refresher_spec.rb

+    expect(subnet).not_to be_nil
+    expect(ems_ref_suffix(subnet.ems_ref)).to match(%r{^/providers/microsoft.network/virtualnetworks/[^/]+/subnets/[^/]+$})
+
+    expect(subnet.cloud_network).not_to be_nil


Thanks, I removed a similar redundant case from assert_specific_network_port as well.

agrare

👍 LGTM

Extend NetworkManager inventory

miha-plesko suggested changes Sep 20, 2019

View reviewed changes

agrare reviewed Sep 20, 2019

View reviewed changes

mancabizjak force-pushed the extend-network-inventory branch from bc827ac to 2bfe503 Compare September 20, 2019 12:35

mancabizjak commented Sep 20, 2019

View reviewed changes

agrare approved these changes Sep 20, 2019

View reviewed changes

agrare merged commit 2bfe503 into ManageIQ:master Sep 20, 2019

agrare added a commit that referenced this pull request Sep 20, 2019

Merge pull request #9 from xlab-si/extend-network-inventory

4dca041

Extend NetworkManager inventory

agrare self-assigned this Sep 20, 2019

agrare added the enhancement New feature or request label Sep 20, 2019

agrare added this to the Sprint 121 Ending Sep 30, 2019 milestone Sep 20, 2019

mancabizjak deleted the extend-network-inventory branch October 4, 2019 10:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend NetworkManager inventory #9

Extend NetworkManager inventory #9

mancabizjak commented Sep 20, 2019

miha-plesko left a comment

miha-plesko Sep 20, 2019

mancabizjak Sep 20, 2019

miha-plesko Sep 20, 2019

mancabizjak Sep 20, 2019

miha-plesko Sep 20, 2019

miha-plesko Sep 20, 2019

miha-plesko Sep 20, 2019

mancabizjak Sep 20, 2019

miha-plesko Sep 20, 2019

agrare Sep 20, 2019

agrare Sep 20, 2019

agrare Sep 20, 2019

mancabizjak Sep 20, 2019

agrare commented Sep 20, 2019

miq-bot commented Sep 20, 2019

mancabizjak left a comment

mancabizjak Sep 20, 2019

mancabizjak Sep 20, 2019

mancabizjak Sep 20, 2019

mancabizjak Sep 20, 2019

agrare left a comment

	:device => port.virtual_machine ? persister.vms.lazy_find(port.virtual_machine.id.downcase) : nil,
	:device => persister.vms.lazy_find(port.virtual_machine&.id&.downcase),

Extend NetworkManager inventory #9

Extend NetworkManager inventory #9

Conversation

mancabizjak commented Sep 20, 2019

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrare commented Sep 20, 2019

miq-bot commented Sep 20, 2019

mancabizjak 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

agrare left a comment

Choose a reason for hiding this comment