Add method for restricting to accessible storages

[agrare]

Use the host_storage.accessible property to filter storages that cannot
be accessed by the provider due to being disconnected or otherwise
inaccessible.

Depends on:

• Add Accessible to HostStorages manageiq-schema#329
• Parse if a datastore is accessible from a host manageiq-providers-vmware#356

https://bugzilla.redhat.com/show_bug.cgi?id=1668020

[gmcculloug]

cc @tinaafitz

[agrare]

I think it would be good to replace the writable_storages and read_only_storages with associations using these scopes, not sure if in this PR or a followup since it isn't really related

[agrare]

/cc @NickLaMuro since you modified the writable_storages method for performance, wdyt?

[NickLaMuro]

For reference (since I had to go find it), that performance change was done here:

#17354

[NickLaMuro]

Okay, so I think my to cents with this is that you probably want to leave those methods (writable_storages and read_only_storages), and just change them to use the scopes:

 def writable_storages
    if host_storages.loaded? && host_storages.all? { |hs| hs.association(:storage).loaded? }
       host_storages.reject(&:read_only).map(&:storage)
     else
-      storages.where(:host_storages => {:read_only => [false, nil]})
+      storages.writable  # I think my pseudo code here is wrong...
    end
  end

Or, if we did create a writable_storages scope, I would suggest possibly doing the following to maintain the performance benefits of #17354 :

has_many                  :writable_host_storages, -> { writable }, :class_name => "HostStorage"
has_many                  :writable_storages, :through => :writable_host_storages, :source => :storage

# overwrite default reader method
def writable_storages
  if association(:writable_storages).loaded? # use already loaded the relationship (faster)
    association(:writable_storages).reader
  # can use an existing relationship to avoid another query and instantiation of other records (requires loop)
  elsif host_storages.loaded? && host_storages.all? { |hs| hs.association(:storage).loaded? }
    host_storages.reject(&:read_only).map(&:storage)
  else # call the relationship
    association(:writable_storages).reader
  end
end

Let me know if any of this needs a further explanation... trying to avoid a 📖

[agrare]

Alright I'll leave it as is 😄

[miq-bot]

Checked commits agrare/manageiq@afddd94~...62f834e with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 2 offenses detected

app/models/host.rb

• ❗ - Line 50, Col 3 - Rails/InverseOf - Specify an :inverse_of option.

app/models/host_storage.rb

• ❗ - Line 12, Col 5 - Rails/Delegate - Use delegate to define delegations.

[NickLaMuro]

Just some thoughts, mainly regarding the original question asked to me by @agrare

https://github.com/ManageIQ/manageiq/pull/18391/files#r250684300

Take it or leave it.

[NickLaMuro]

Okay, so I think my to cents with this is that you probably want to leave those methods (writable_storages and read_only_storages), and just change them to use the scopes:

 def writable_storages
    if host_storages.loaded? && host_storages.all? { |hs| hs.association(:storage).loaded? }
       host_storages.reject(&:read_only).map(&:storage)
     else
-      storages.where(:host_storages => {:read_only => [false, nil]})
+      storages.writable  # I think my pseudo code here is wrong...
    end
  end

Or, if we did create a writable_storages scope, I would suggest possibly doing the following to maintain the performance benefits of #17354 :

has_many                  :writable_host_storages, -> { writable }, :class_name => "HostStorage"
has_many                  :writable_storages, :through => :writable_host_storages, :source => :storage

# overwrite default reader method
def writable_storages
  if association(:writable_storages).loaded? # use already loaded the relationship (faster)
    association(:writable_storages).reader
  # can use an existing relationship to avoid another query and instantiation of other records (requires loop)
  elsif host_storages.loaded? && host_storages.all? { |hs| hs.association(:storage).loaded? }
    host_storages.reject(&:read_only).map(&:storage)
  else # call the relationship
    association(:writable_storages).reader
  end
end

Let me know if any of this needs a further explanation... trying to avoid a 📖

[gmcculloug]

This change will help with the Provisioning UI and Automate callers to eligible_storages.

Unfortunately the vmware_best_fit_least_utilized placement method in Automate provisioning uses writable_storages directly. (See VM/Provisioning/Placement.class/methods/vmware_best_fit_least_utilized.rb)

@tinaafitz As part of fixing this BZ we should look into changing these calls to use allowed_storages instead.

[gmcculloug]

Overall, I'm good with this change. Added a note about changes we will need to address on the Automate side to fully resolve the reported issue.

Now I just need to figure out when @agrare and @NickLaMuro are actually done discussing the changes. 😕

[NickLaMuro]

Marking as approved to avoid any further confusion from @gmcculloug 😉