Update model to support network adapters #15371

skovic · 2017-06-14T17:44:11Z

This PR updates the model to support network adapters which will be added as entries to the guest_devices table.

skovic · 2017-06-14T17:44:27Z

@miq-bot add_label wip

skovic · 2017-06-14T18:08:16Z

@miq-bot assign @blomquisg

skovic · 2017-06-22T17:59:21Z

@miq-bot remove_label wip

skovic · 2017-06-27T16:50:29Z

@blomquisg Can you get someone to look at this PR? Thanks

blomquisg · 2017-06-28T19:44:52Z

@skovic it looks like there needs to be an associated set of changes to the DB Schema.

Can you look into that and add the link to the associated PR when that's available.

Talk to @juliancheal about how to get started there if you have questions.

skovic · 2017-06-28T20:51:12Z

@blomquisg @juliancheal The schema changes have already been reviewed and merged. Please see manageiq-schema PR 16

rodneyhbrown7 · 2017-07-19T12:47:46Z

Provider change have also been submitted for review in ManageIQ/manageiq-providers-lenovo#59 @juliancheal can you review?

skovic · 2017-07-24T13:57:41Z

@blomquisg Could you get someone to take a look at this PR? The schema changes are already in. Thanks

blomquisg · 2017-07-24T20:59:19Z

app/models/guest_device.rb

@@ -10,4 +11,7 @@ class GuestDevice < ApplicationRecord

  has_one :network, :foreign_key => "device_id", :dependent => :destroy, :inverse_of => :guest_device
  has_many :miq_scsi_targets, :dependent => :destroy
+
+  has_many :firmwares, :dependent => :destroy
+  has_many :child_devices, -> { where(:parent_device_id => ids) }, :foreign_key => "parent_device_id", :class_name => "GuestDevice", :dependent => :destroy


I wonder if you could do something more bi-directional here, like the Rails docs show.

has_many :child_devices, class_name => "GuestDevice", foreign_key => :parent_device_id, dependent => :destroy belongs_to :parent_device, class_name => "GuestDevice"

This way, you could have a guest device, and list it's child devices, and also ask for it's parent device:

guest_device = GuestDevice.first children = guest_device.child_devices parent = guest_device.parent_device

I think the rest will fall into place with Rails magic.

/cc @Fryguy

@blomquisg I made the changes you suggested, and they seemed to work at first, but after I restarted the evm, one of the ports associated with the guest device was replaced by a duplicate of the other port in the table. For example, suppose we have guest device 1 that has port 1 and port 2. At first, the guest device and its associated ports are populated in the DB correctly. After a evm restart is performed, port 1 disappears from the table and there are now two duplicate port 2 entries with the same address and parent device. Any ideas why this is occurring? Does it have something to do with the save_inventory code? Including @juliancheal @Fryguy

That sounds like a bug in the refresh, honestly.

Can you update the PR with the changes you have in place, and maybe it will shed some light on what's going on.

Also, with the inventory refresher code... are you running what's on master? Or, do you have local changes to the inventory collection? It would help to see that, too.

@blomquisg @juliancheal I updated the PR with the suggested changes. The associated changes in the provider were merged, so they're in master which is what I'm testing with.

@skovic I'm having a look this afternoon, to see if I can find the root of the refresh issue.

@juliancheal Have you had a chance to look into this issue yet? Thanks

@blomquisg Could we just revert to my original changes (undo the change you recommended) and get this PR merged? Our provider changes for the network adapter support have already been merged but these changes have not. This partial implementation is causing the provider to not refresh properly. In addition, this PR is delaying the UI changes for network adapter support from being merged -- the PR is ready to go. We could always make another PR with your suggested changes and have it sit out there while someone investigates the database issues that occurring with the change. What do you think?

@blomquisg I have reverted the guest device model in this PR to use my original changes (which do not have the database refresh problem), and I have moved the implementation of this suggestion to PR #15732. Please consider merging this PR (#15371), so we are unblocked. Thanks

K... Thanks for the update @skovic ... I'll remove my "request for changes" review and take another look.

We can try to figure out what's going on with this in a separate PR/thread.

blomquisg · 2017-07-24T21:01:12Z

db/fixtures/miq_product_features.yml

@@ -6213,6 +6213,26 @@
      :feature_type: view
      :identifier: firmware_show

+# Guest Devices


@dclarizio @Fryguy Do we want a Guest Devices section in the product_features? I'm not sure of all the impact that has.

@blomquisg if these are a new menu item in the navigation, we normally do add them into product_features so a role can be customized to show or not show them. If they are similar to the Firmwares item above, then I'd say yes. If not, then I'm not sure as I am not familiar with this addition.

blomquisg · 2017-07-24T21:12:22Z

@imtayadeway Can you check on the API changes in this PR? Thanks!

juliancheal

LGTM, agree with @blomquisg's comment.

The changes I recommended caused a problem in the refresher ... going to look at that separately and move this PR along.

blomquisg · 2017-08-07T16:54:57Z

app/models/hardware.rb

@@ -19,7 +19,7 @@ class Hardware < ApplicationRecord
  has_many    :guest_devices, :dependent => :destroy
  has_many    :storage_adapters, -> { where "device_type = 'storage'" }, :class_name => "GuestDevice", :foreign_key => :hardware_id
  has_many    :nics, -> { where "device_type = 'ethernet'" }, :class_name => "GuestDevice", :foreign_key => :hardware_id
-  has_many    :ports, -> { where "device_type != 'storage'" }, :class_name => "GuestDevice", :foreign_key => :hardware_id
+  has_many    :ports, -> { where "device_type = 'ethernet port'" }, :class_name => "GuestDevice", :foreign_key => :hardware_id


I'm worried about this, but without any evidence...

The new implementation is much more "correct". I'm just worried that there were some old assumptions built into that previous relationship mapping based on != 'storage' that will come back to bite us.

Honestly, the only thing I can think of doing is letting it through and crossing our fingers.

Oops, I think I spoke to soon.

I can't find a device_type of "ethernet port". This means that whatever used to be classified as a port will no longer be.

And, if this is changed to just ethernet, then it simply overlaps with nics.

I need to look over the different ways we're collecting the device_type information to figure out whatdevice_type ports should really map to.

@blomquisg Are supported device_types defined somewhere? The schema for guest_devices declares the device_type as a string, so it looks like anything will work -- maybe this should be an enumeration instead of a string? The Lenovo provider adds physical ports as a guest_device with a device_type of "ethernet port". Please see ManageIQ/manageiq-providers-lenovo#59

@blomquisg have you found the problem?

…ild device

skovic · 2017-08-08T19:37:34Z

I moved the guest_devices REST API changes to ManageIQ/manageiq-api#7

chargio · 2017-08-10T16:30:28Z

Is there anything missing to have this PR merged? I can't see if that is the case from the comments, or whether this should be merged and an issue created (as the tests are green)

rodneyhbrown7 · 2017-08-21T12:21:04Z

@blomquisg @juliancheal can we merge this PR?

blomquisg · 2017-08-21T14:14:17Z

I need to look back at the device_type issue. I should have an answer today.

blomquisg · 2017-08-21T18:44:54Z

Here's what I'm seeing for guest_device and device_type.

Ovirt
- ethernet
Scvmm
- cdrom
- ethernet
VMware
- ethernet
- storage

Based on this, it looks like the previous implementation would have included cdrom devices in the ports list.

Also, the UI is using the ports relationship to show networking information. From this you can see what type of information the UI is expecting to see on the guest_device. Specifically:

controller_type
device_name
location
address
filename
auto_detect

Here's how I think this should change.

Leave the ports relationship alone (even though it's technically wrong today, because it's going to pull in cdrom as well as ethernet). Then, create a new relationship specifically for physical ethernet ports. I suggest calling the relationship physical_ports and the device_type physical_ports instead of ethernet port. That way, it's clear why it's different from the original ports relationship.

This allows us to treat the two concepts differently. If we combined them today into a single field, and then later decide to treat them differently, it's going to be much harder to split them apart later. If we decide to combine them later, that should be much easier.

/cc @Fryguy @roliveri @juliancheal

Thoughts?

chargio · 2017-08-23T19:30:39Z

Hi @rodneyhbrown7,

Can you comment on the changes @blomquisg is suggesting? There are others PR depending on this one, and it seems that we need to work a little more on this. Is there any approach that would help us to make it faster?

…lationship

skovic · 2017-08-23T20:12:37Z

@blomquisg I believe I have made the changes you requested. Please let me know if I missed anything. Also, as a result of these changes, I had to make an update to the provider in ManageIQ/manageiq-providers-lenovo#73

miq-bot · 2017-08-23T20:13:00Z

Checked commits skovic/manageiq@e34051c~...faed145 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 3 offenses detected

app/models/ems_refresh/save_inventory.rb

❗ - Line 239, Col 7 - Style/Next - Use next to skip iteration.
❗ - Line 256, Col 64 - Style/WordArray - Use %w or %W for an array of words.

app/models/hardware.rb

❗ - Line 23, Col 37 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.

The child_devices added unneeded scope. This scope pulled back all ids from the guest devices table. Since the foreign_key is added to the query, this did not do any harm. But it adds an unneeded constraint an unneeded query. This was introduce in ManageIQ#15371 The way the PR was batted around, it looked like this was not an intentional side effect. Before ====== ```ruby has_many :child_devices, -> { where(:parent_device_id => ids) }, :foreign_key => "parent_device_id" has_many :child_devices, -> { where(:parent_device_id => GuestDevice.ids) }, :foreign_key => "parent_device_id" SELECT "guest_devices".* FROM "guest_devices" WHERE "guest_devices"."parent_device_id" = 26000000000035 AND "guest_devices"."parent_device_id" IN (26000000000032, 26000000000033, 26000000000034, 26000000000035, 26000000000036, 26000000000037, 26000000000038) ``` After ===== ```ruby has_many :child_devices, :foreign_key => "parent_device_id" SELECT "guest_devices".* FROM "guest_devices" WHERE "guest_devices"."parent_device_id" = 26000000000035 ```

miq-bot changed the title ~~Update model to support network adapters~~ [WIP] Update model to support network adapters Jun 14, 2017

miq-bot added the wip label Jun 14, 2017

miq-bot assigned blomquisg Jun 14, 2017

miq-bot changed the title ~~[WIP] Update model to support network adapters~~ Update model to support network adapters Jun 22, 2017

miq-bot removed the wip label Jun 22, 2017

blomquisg requested review from Fryguy and juliancheal June 28, 2017 19:24

skovic force-pushed the network-adapter-model branch from c5d64bf to 56d56eb Compare July 5, 2017 17:03

skovic mentioned this pull request Jul 12, 2017

Add support for displaying network adapter details ManageIQ/manageiq-ui-classic#1654

Merged

skovic force-pushed the network-adapter-model branch from 98801cb to f5b90b3 Compare July 17, 2017 16:34

blomquisg previously requested changes Jul 24, 2017

View reviewed changes

blomquisg requested a review from imtayadeway July 24, 2017 21:12

imtayadeway approved these changes Jul 24, 2017

View reviewed changes

chessbyte added enhancement providers/physical labels Jul 24, 2017

juliancheal approved these changes Jul 25, 2017

View reviewed changes

skovic force-pushed the network-adapter-model branch from f5b90b3 to 3d7b976 Compare July 26, 2017 20:07

skovic force-pushed the network-adapter-model branch from 3d7b976 to 10c1f46 Compare August 4, 2017 17:53

skovic mentioned this pull request Aug 4, 2017

[WIP] Refactor guest device model based on review feedback #15732

Closed

blomquisg reviewed Aug 7, 2017

View reviewed changes

skovic added 9 commits August 8, 2017 13:48

Add initial changes to the model to support network adapters

e34051c

Remove extra whitespace

cc697a5

Address rubocop warning

4535e7f

Rename the guest_device self-reference to child_devices

e16c51a

Remove extra whitespace

9aa344d

Update save_guest_devices_inventory to set the hardware ID of each ch…

453a385

…ild device

Fix guest device firmware association

003db32

Add test cases for the guest_devices API

cdb1868

Fix guest device table problem

80a2245

skovic force-pushed the network-adapter-model branch from 10c1f46 to 80a2245 Compare August 8, 2017 18:57

miq-bot removed the unmergeable label Aug 8, 2017

Remove guest_devices from REST API

c9f4efa

Undo 'ports' relationship change and create a new 'physical_ports' re…

faed145

…lationship

blomquisg merged commit 9641a1b into ManageIQ:master Aug 30, 2017

blomquisg added this to the Sprint 68 Ending Sep 4, 2017 milestone Aug 30, 2017

blomquisg mentioned this pull request Aug 30, 2017

Change device_type from 'ethernet port' to 'physical_port' ManageIQ/manageiq-providers-lenovo#73

Merged

skovic mentioned this pull request Aug 30, 2017

Add guest_devices support to the API ManageIQ/manageiq-api#7

Merged

kbrock mentioned this pull request Aug 17, 2023

fix GuestDevice#child_device references #22670

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update model to support network adapters #15371

Update model to support network adapters #15371

skovic commented Jun 14, 2017

skovic commented Jun 14, 2017

skovic commented Jun 14, 2017

skovic commented Jun 22, 2017

skovic commented Jun 27, 2017

blomquisg commented Jun 28, 2017

skovic commented Jun 28, 2017

rodneyhbrown7 commented Jul 19, 2017

skovic commented Jul 24, 2017

blomquisg Jul 24, 2017

juliancheal Jul 25, 2017

skovic Jul 25, 2017

blomquisg Jul 26, 2017

skovic Jul 26, 2017

juliancheal Jul 27, 2017

skovic Jul 31, 2017

skovic Aug 4, 2017

skovic Aug 4, 2017

blomquisg Aug 7, 2017

blomquisg Jul 24, 2017

dclarizio Jul 24, 2017

blomquisg commented Jul 24, 2017

juliancheal left a comment

blomquisg Aug 7, 2017

blomquisg Aug 7, 2017

skovic Aug 7, 2017

chargio Aug 10, 2017

skovic commented Aug 8, 2017

chargio commented Aug 10, 2017

rodneyhbrown7 commented Aug 21, 2017

blomquisg commented Aug 21, 2017

blomquisg commented Aug 21, 2017 •

edited

Loading

chargio commented Aug 23, 2017

skovic commented Aug 23, 2017

miq-bot commented Aug 23, 2017

Update model to support network adapters #15371

Update model to support network adapters #15371

Conversation

skovic commented Jun 14, 2017

skovic commented Jun 14, 2017

skovic commented Jun 14, 2017

skovic commented Jun 22, 2017

skovic commented Jun 27, 2017

blomquisg commented Jun 28, 2017

skovic commented Jun 28, 2017

rodneyhbrown7 commented Jul 19, 2017

skovic commented Jul 24, 2017

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

blomquisg commented Jul 24, 2017

juliancheal 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

skovic commented Aug 8, 2017

chargio commented Aug 10, 2017

rodneyhbrown7 commented Aug 21, 2017

blomquisg commented Aug 21, 2017

blomquisg commented Aug 21, 2017 • edited Loading

chargio commented Aug 23, 2017

skovic commented Aug 23, 2017

miq-bot commented Aug 23, 2017

blomquisg commented Aug 21, 2017 •

edited

Loading