Add a Physical Rack model

[felipedf]

This PR is able to add a PhysicalRack model to ManageIQ.
Today, providers are able to access all physical server directly, although this change relate physical servers to physical racks instead providers, i wanted to keep a possible way to access physical servers through providers so this change won't break most providers out there.

To be merged with: ManageIQ/manageiq-providers-lenovo#147

[miq-bot]

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

[Fryguy]

@agrare Please review.

[Fryguy]

This should no longer be necessary... "default" factories are automatically build now.

[Fryguy]

I could be wrong, but this feels backwards...I would expect the racks to be saved first, and then the servers, since there is a physical_rack_id on the physicals_servers that would need to be resolved. @agrare Does that sound right?

[Fryguy]

Actually, are there servers that aren't in racks at all? If so, that could be where I'm getting confused.

[felipedf]

Yes, we have servers that aren't either in a rack nor in chassis

[Fryguy]

I feel like there's going to be a clash here where the physical_servers table is going to be updated via two different paths with two different sets of data...1) directly from the EMS, and 2) through this relationship, which will probably cause thrashing.

[Fryguy]

@agrare How do we handle this with other tables? I thought we only did the one save from the EMS, but I'm not sure.

[agrare]

So yeah you're gong to have problems:

save_physical_servers([server1, server2])
save_physical_racks(rack)
  # rack has child server 3
  save_physical_servers([server3])
^^ is going to disconnect all servers _not_ in this rack

[agrare]

@felipedf instead of having :physical_servers as a child of both the ems and the rack, just have all physical servers in the top level collection and reference the rack from the physical server.

Example:

:racks => [{:ems_ref => "1"}],
:physical_servers => [{:name => 'server1', :rack => {:ems_ref => "1"}}, {:name => "server1"}]

Save the racks first, then in save_physical_servers pull the new rack id like this:

hash[:rack_id] = hash.delete(:rack).try(:id)

This is assuming there is a rack_id on the physical_servers table

[felipedf]

I ran into this issue, what I tried at first was saving racks and then the servers associated to it, then I tried to save all servers related to the provider.
result: All servers previously added through the rack association were wiped out.

Then I tried to do the opposite, save all servers associated to the provider first, then the ones associated to the rack.
result: they were all there. but one of the behaviors we do want is when I try to access the association provider.physical_servers it will retrieve me all servers managed by that provider, the ones that are inside a rack, chassis, or alone. I tried few thing without success and the solution I found was something like server[ems_id] = ems_id while doing a parse.

@agrare I got to say that I don't know why this trashing happens while use associations in that way. Maybe you can give a hand here. About your suggestion, how would know which server is in which rack using that approach?

[agrare]

You don't want to be saving the same type from multiple places, so instead of

save all servers associated to the provider first, then the ones associated to the rack.

You should just save all servers.

A server can belong to a physical rack so you can put a physical_rack_id in a server record, but that doesn't mean that you should be saving those servers from the physical_rack association.

[agrare]

Sort-of, the actual id will be set at save time, the parser will link the server to the rack hash.

so if you have something like this in your parser:

def parse
  racks, rack_uids = parse_racks(inv[:racks])
  servers = parse_servers(inv[:servers], rack_uids)
end

def parse_racks(inv)
  results = []
  uids = {}

  inv.each do |rack|
    new_result = {
      // parse your rack here
    }

    results << new_result
    uids[new_result[:ems_ref]] = new_result
  end

  return results, uids
end

def parse_servers(inv, rack_uids)
  results = []

  inv.each do |server|
    rack = rack_uids[server[:rack_unique_identifier]]

    new_result = {
      :rack => rack,
    }
    results << new_result
  end

  results
end

This way when you save the racks collection first and update the IDs in the rack hashes you can then get the rack ID when you get to the servers.

[rodneyhbrown7]

We should treat this similar to how we handle the host -> physical_server relationship. The physical_infra provider only saves physical servers and if there is a host that matches the appropriate query; we create an association. In the rack case, we should create all of the racks first. When we parse the physical_server; if there is an appropriate rack instance we create the association at that time.

Rack parsing should not create any physical_server objects. The rack parsing should only create the rack objects. The physical_server parsing should create the association if a rack exists.

[agrare]

If you set the whole rack hash to h[:rack] in the parser, after it is saved and you update the IDs you can get the rack ID directly from h[:rack][:id], see https://github.com/ManageIQ/manageiq/blob/master/app/models/ems_refresh/save_inventory_infra.rb#L136 for an example of how this is done

[felipedf]

I guess I finally got that hehe

[agrare]

Now we're talking :D

[agrare]

Sorry one last nit-pick then this looks good to me, the hash key here :rack should match the fk name so it should be :physical_rack

[felipedf]

Done (:

[agrare]

should be saving physical_racks first now

[agrare]

we shouldn't be saving physical_servers as children of the racks collection anymore

[agrare]

This shouldn't be needed given save_inventory_multi resets the assoc for you https://github.com/ManageIQ/manageiq/blob/master/app/models/ems_refresh/save_inventory_helper.rb#L37

[felipedf]

That would apply to physical_servers as well? I mean the method that saves servers does the same, I did see it and it was weird, but i left there since physical server had it

[agrare]

Yeah I see it in a lot of places, I think a mixture of old code then copy&pasted code.

[felipedf]

I agree

[agrare]

Don't need these here if they're in ExtManagementSystem

[felipedf]

ehhhh... my bad

[agrare]

Actually for this one I think you want it to be dependent => :nullify because physical_servers is a top-level collection and deletion will be done by ems_refresh

[agrare]

Don't think you need a class name here

[miq-bot]

Checked commit felipedf@3e9d09a with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 0 offenses detected
Everything looks fine. 🍪