Associate ph server to host #37
Conversation
Use actice record procedures to assign relationship.
Use actice record procedures to assign relationship.
walteraa
changed the title
…vo/manageiq-providers-lenovo into associate_ph_server_to_host
…_ph_server_to_host
|
Reopen to rebuild. |
|
Reopen to rebuild. |
|
@juliancheal @rodneyhbrown7 I am having migrations erros when I try to execute the refresh process. Is there any PR referring to migrations that solve these issues? Is missing only test if the refresh process will assign the physical server ID in host. |
walteraa
changed the title
|
Walter what errors are you seeing in the refresh process? |
|
|
|
Do you have the PR 14430 installed? |
|
@rodneyhbrown7 I'm running MIQ core from |
| def save_host_relationship(node) | ||
| # Assign a physicalserver and host if server already exists and | ||
| # some host match with physical Server's serial number | ||
| server = PhysicalServer.find_by(:uuid => node.uuid, :ems_id => @ems.id) |
There was a problem hiding this comment.
:uuid is not part of the PhysicalServer definition. I think this needs to be changed to find_by(:ems_ref => node.uuid
There was a problem hiding this comment.
Was the :uuid removed from Physical server?
There was a problem hiding this comment.
I changed it to :ems_ref.
| # Filled in later conditionally on what's available | ||
| :computer_system => {:hardware => {:networks => [], :firmwares => []}} | ||
| } | ||
| new_result[:hardware] = get_hardwares(node) |
There was a problem hiding this comment.
The hardware element is not associated to the physical server directly. Hardware is associated to ComputerSystem which is associated to the PhysicalServer (see L80) . Rails will not resolve this assignment.
There was a problem hiding this comment.
This assignment come from @juliancheal's branch. What do you think about @rodneyhbrown7's concern, Julian?
There was a problem hiding this comment.
If you look at #34 This assignment is now new_result[:computer_system][:hardware] = get_hardwares(node). It may be worth rebasing this PR from 34 before it is merged. I was testing this yesterday and the assignment that was there before threw an exception.
There was a problem hiding this comment.
Ok, I'll do a rebase, @rodneyhbrown7, thanks!
| } | ||
| } | ||
| # Filled in later conditionally on what's available | ||
| :computer_system => {:hardware => {:networks => [], :firmwares => []}} |
There was a problem hiding this comment.
This change will be functional but removes formatting that was added previously. Rubocop will probably complain about laying this out on a single line like this.
There was a problem hiding this comment.
@rodneyhbrown7 I did this change because rubocop was warning about the previous structure made by Julian. Rubocop doesn't warn about this format.
There was a problem hiding this comment.
Ok. Good. Thanks Walter.
|
This pull request is not mergeable. Please rebase and repush. |
| return node.uuid, new_result | ||
| end | ||
|
|
||
| def save_host_relationship(node) |
There was a problem hiding this comment.
I believe we should just be finding hosts here, but not saving them. All saving should be done in ManageIQ in SaveInventory.
There was a problem hiding this comment.
@juliancheal I don't figured out how to send a save host to MIQ core from here. I mean, the ID of relationship was saved in Physical server, as you guys defined, so, once we found the Host ID and the Physical server ID which matches considering the LXCA association(serial_number and service_tag equals), how to do that in Core?
There was a problem hiding this comment.
I think you do the look up and assignments in the refresher as you have already implemented, but you remove the line that saves the server instance. I think that the existing code in save_ems_physical_infra_inventory (https://github.com/ManageIQ/manageiq/blob/master/app/models/ems_refresh/save_inventory_physical_infra.rb#L8 ) should save the details without any additional changes. This needs to be verified.
There was a problem hiding this comment.
Hey @rodneyhbrown7 , I figured out this process(which use the core to save the objects) before, it isn't the problem. The problem is that the relationship is saved in the Host object, so to use the ems_refresh/save_inventory_physical_infra.rb is unfeasible considering the structure proposed by RH guys.
The only way that we can use this, is converting the Host object found in a Hash, adding it as a "sub hash" in the Physical server hash and setting up a child key to host in save_physical_server method in ems_refresh/save_inventory_physical_infra.rb file.
But it looks like a workaround.
There was a problem hiding this comment.
I don't think it is a work around. I think that is the way this is intended to work. I believe all we need to do is remove the save! operation in the refresher and then add the :host as a child key in save_physical_server. This is working for me locally.
There was a problem hiding this comment.
Ok @rodneyhbrown7, I'll do that as I described.
Now Host object is passing to Physical server Hash. Before was passing a Hash, but the save method throws an error.
There isn't necessity search a physical server. Then, just need find a host previously saved in database to create the relationship.
|
Checked commits https://github.com/lenovo/manageiq-providers-lenovo/compare/da38bd3de9757209bedd2479f2dd4232071c066f~...2a8728de5465820b3212695b30f604424ad7fbb8 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
|
@juliancheal can you merge this PR? @rodneyhbrown7 |
|
LGTM 👍 |
Now the relationship is associated in Provider.
The relationship will be assign only if the physical server exists previously in Database and if exists a host . So, the relationship will be done in the second refresh for each Physical server.
This PR depends of #34 and maybe some change should be done after merge.