Add ID references to PhysicalDisks and Canisters

[EsdrasVP]

This PR is able to:

• Add Canister reference into PhysicalDisks
• Add ems_ref to PhysicalDisks

[agrare]

These don't need to be in separate migrations

[EsdrasVP]

@agrare Changed it to be in only one migration.

[agrare]

You can use add_reference e.g. add_reference :physical_disks, :canister, :type => :bigint ref

[EsdrasVP]

@agrare In this case, this affect the way we use canister_id on save_inventory_physical_infra, right?

[agrare]

Not sure what you mean

[EsdrasVP]

@agrare What I asked is if this affects the way we update the associated IDs for PhysicalDisks on save_inventory_physical_infra, more specifically on h[:canister_id] = h.delete(:canister).try(:[], :id), or if canister_id column will still exist if we use add_reference and nothing else has to change.

[agrare]

add_reference will result in a canister_id column

[agrare]

@EsdrasVP what was the primary unique reference in physical_disks if there wasn't a uid_ems?
I had assumed it was "location" and that it was relative to a physical_storage.

Addind a uid_ems implies like a physical disk can be uniquely identified across all providers, and an ems_ref would imply a physical disk can be uniquely identified under a single provider and should have an ems_id also.

[agrare]

@EsdrasVP wait a minute, looking at your physical infra save_inventory and the find_key for physical_disks is :physical_storage_id ???

save_inventory_multi(physical_storage.physical_disks, hashes, :use_association, [:physical_storage_id])

Are you every seeing more than one disk on a physical storage today?

[EsdrasVP]

@EsdrasVP what was the primary unique reference in physical_disks if there wasn't a uid_ems?
I had assumed it was "location" and that it was relative to a physical_storage.

Addind a uid_ems implies like a physical disk can be uniquely identified across all providers, and an ems_ref would imply a physical disk can be uniquely identified under a single provider and should have an ems_id also.

@agrare The thing is there was no unique identifier on a PhysicalDisk, actually. As you said, the closest field that we can considerate a unique one is location, but it doesn't always exist on the response that we parse. We stumbled across a similar problem on PhysicalNetworkPorts, where we had to create an unique field to be used as identifier.

[EsdrasVP]

@EsdrasVP wait a minute, looking at your physical infra save_inventory and the find_key for physical_disks is :physical_storage_id ???

save_inventory_multi(physical_storage.physical_disks, hashes, :use_association, [:physical_storage_id])

Are you every seeing more than one disk on a physical storage today?

@agrare Yes, currently there is at least 12 disk slots on the storage. So what we parse could be an array of physical disks. Also, I changed it to adapt to this implementation here.

[agrare]

Yes, currently there is at least 12 disk slots on the storage.

No I mean looking at save_inventory_physical_infra I bet that there is only ever a single physical_disk because the find_key is :physical_storage_id.

Looking at the specs for physical storage/disks in your provider repo there are 0 checks for anything related to physical disks in the refresher_spec. There are checks on the parser but the storage being checked in assert_specific_storage doesn't have any physical disks.

Did you test a refresh and confirm that you got 1-12 physical disks on a physical storage?

The thing is there was no unique identifier on a PhysicalDisk, actually.

These are things that need to be figured out the first time we add the schema. I don't see it in the PR but I talked to you offline and believed that you were using [physical_storage, location] as the unique reference.

I'm okay adding this but really this should have been there from the start.

What is the content of uid_ems going to be from the parser? Is it something that we can populate here with a data migration?

If not now there are databases with records that we probably cannot update to a consistent state without running a refresh.

[agrare]

Going to put a block on this pending adequate specs in ManageIQ/manageiq-providers-lenovo#235 (review) and we have confirmed this works end-to-end.

[EsdrasVP]

No I mean looking at save_inventory_physical_infra I bet that there is only ever a single physical_disk because the find_key is :physical_storage_id.

Looking at the specs for physical storage/disks in your provider repo there are 0 checks for anything related to physical disks in the refresher_spec. There are checks on the parser but the storage being checked in assert_specific_storage doesn't have any physical disks.

Did you test a refresh and confirm that you got 1-12 physical disks on a physical storage?

@agrare The storage I test on assert_specific_storage have disks, the field that represent them on our response is the field "drives" (yeah, don't know why it's misspelled) and it's on mock_storages. What's missing there is the assertion of at least one of those disks, so I'll add this like we did on refresh_parser_spec.

These are things that need to be figured out the first time we add the schema. I don't see it in the PR but I talked to you offline and believed that you were using [physical_storage, location] as the unique reference.

Sorry for the confusion, I must have interpreted it wrong because I thought that the physical_storage_id was being passed as a physical disk parent reference, and not as an unique identifier for a physical disk.

What is the content of uid_ems going to be from the parser? Is it something that we can populate here with a data migration?

Since there is no unique identifier on a physical disk, we decided to use a concatenation of the physical disk parent uuid/serial_number with the disk index. I'm still trying to reach the team responsible for the response that we parse to confirm that, so that's why I let the PRs as WIP.

[agrare]

The storage I test on assert_specific_storage have disks

Then they aren't being saved:

[107, 116] in /home/agrare/src/manageiq/manageiq-providers-lenovo/spec/models/manageiq/providers/lenovo/physical_infra_manager/refresher_spec.rb
   107: 
   108:   def assert_specific_storage
   109:     storage = PhysicalStorage.find_by(:ems_ref => "208000C0FF2683AF")
   110: 
   111:     byebug
=> 112:     expect(storage.name).to eq("S3200-1")
   113:     expect(storage.uid_ems).to eq("208000C0FF2683AF")
   114:     expect(storage.ems_ref).to eq("208000C0FF2683AF")
   115:     expect(storage.access_state).to eq("Online")
   116:     expect(storage.access_state).to eq("Online")
(byebug) storage.physical_disks.count
0

Sorry for the confusion, I must have interpreted it wrong because I thought that the physical_storage_id was being passed as a physical disk parent reference, and not as an unique identifier for a physical disk.

Nothing wrong with confusion but this would have been found out by either a quick test against a live system and confirming that physical_storage.physical_disks.count is what you expect it to be, or by good spec tests.

[EsdrasVP]

Then they aren't being saved:

@agrare They were not being saved on the second turn of full refresh on refresher_spec. When running the tests here I noticed that the reference for a physical disk's storage parent was being deleted by h[:physical_storage_id] = h.delete(:physical_storage).try(:[], :id), so the disks were saved on the inventory but with physical_storage_id as nil. The same problem might be happening with canisters, I still have to confirm that, and if so we have to open a PR to fix it.

[slemrmartin]

for graph refresh (ManageIQ/manageiq-providers-lenovo#236),
I'm using [physical_storage_id, serial_number] as a unique identifier (ManageIQ/manageiq#18063)

In case of canisters I think there can be used GET /canisters
http://sysmgt.lenovofiles.com/help/index.jsp?topic=%2Fcom.lenovo.lxca_restapis.doc%2Frest_api_canisters_get.html&cp=1_21_3_3_3_0_0
and pair it with "parent", because this request contains UUID of canister (current request GET /storage don't). I think it will be easier to implement into graph refresh because we can use lazy_find there.

@EsdrasVP I think there can be uid_ems column for Canisters and ems_ref is not necessary for Lenovo.

About PhysicalDisk:

Since there is no unique identifier on a physical disk, we decided to use a concatenation of the physical disk parent uuid/serial_number with the disk index.

It seems like redundant information. @agrare Is there significant speed or other improvement when manager_ref will be precomputed as @EsdrasVP wrote? There is no GET /physical_disks so they can't be inserted/updated alone

[agrare]

@slemrmartin yeah I agree it seems redundant to have the parent uuid in the unique ref when these already belong_to a physical_storage or canister.

I'd be okay adding an ems_ref to physical_disks and just also storing the serial number there in the lenovo parser since other physical infra providers might have different unique values other than serial number.

[EsdrasVP]

In case of canisters I think there can be used GET /canisters
http://sysmgt.lenovofiles.com/help/index.jsp?topic=%2Fcom.lenovo.lxca_restapis.doc%2Frest_api_canisters_get.html&cp=1_21_3_3_3_0_0
and pair it with "parent", because this request contains UUID of canister (current request GET /storage don't). I think it will be easier to implement into graph refresh because we can use lazy_find there.

@slemrmartin I agree, it would be much better to do that, we would only have to change parser and inventory saving a little bit. I'll ask the team what they think of that, but for me that's fine.

@slemrmartin yeah I agree it seems redundant to have the parent uuid in the unique ref when these already belong_to a physical_storage or canister.

I used the parent UUID in cases where serial_number isn't returned (which, bizarrely, is a possibility). But if ems_ref can be nil on those cases, I remove this redundancy. What do you think @agrare ?

[agrare]

@EsdrasVP ems_ref can't be nil but it also can't be parent_uuid if there are more than one disk under a parent which there can be right?

[EsdrasVP]

@EsdrasVP ems_ref can't be nil but it also can't be parent_uuid if there are more than one disk under a parent which there can be right?

@agrare Yeah, that's why I made Physical Disk ems_ref be the concatenation of its parent_uuid and an index here, which makes it unique inside a storage system. I don't like to make this kind of work around at all, but it's what we can do given what the API returns. Do you have any suggestions?

[agrare]

@EsdrasVP so is parent_uuid the uid of the storage system or something else? This might be better discusses in the manageiq-providers-lenovo parser PR since from a schema point of view we just need some ems_ref which is unique under the parent.

[EsdrasVP]

@EsdrasVP so is parent_uuid the uid of the storage system or something else? This might be better discusses in the manageiq-providers-lenovo parser PR since from a schema point of view we just need some ems_ref which is unique under the parent.

@agrare Yeah, it is the Physical Storage uid. We can migrate the discussion to the other PR then.

[miq-bot]

Checked commit EsdrasVP@08ab2a8 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍪

[agrare]

This is generic enough that I'm okay with it, we can work through the issues with the parser on that PR