consolidate supported storage types #22258

kbrock · 2022-12-01T01:36:25Z

needed for:

IsoDatastore to Storage support manageiq-ui-classic#8548

I only need the first change, which is low risk. The second one is a clean up that may not be necessary or deemed too risky.

Change:

Rename method supports? to storage_type_supported_for_ssa?
I introduced a conflict with SupportsFeatureMixin in Cap&U split up capture_infra_targets to allow tuning #9447
Make the list of supported storage_type values consistent.
The method above was used in metrics capture Storage.scan and capture_storage_targets
Similar but different lists are found in vm_scan, and Vm.path.

@agrare Regarding the second item:

Is there a reason why those 3 lists of supported storage_type values should be different?
What is the best way to determine the proper list? (I chose to include all)

storage_type	Storage.scan	Vm.path	vm_scan
VSAN		y	y
VMFS	y	y	y
NAS		y	y
NFS	y	y	y
NFS41	y	y	y
ISCSI	y		y
DIR			y
FCP	y		y
CSVFS		y	y
NTFS		y	y
GLUSTERFS	y		y

/cc @nasark

kbrock · 2022-12-01T19:13:51Z

update:

fixed spacing cop
fixed constant/array cop (sorry, took a few tries)
fixed specs to be consistent
fixed typo for storage_type vs store_type

agrare · 2022-12-01T19:15:16Z

app/models/manageiq/providers/infra_manager/metrics_capture.rb

@@ -78,7 +78,7 @@ def capture_host_targets(ems)
  # @return [Array<Storage>] supported storages
  # hosts preloaded storages and tags
  def capture_storage_targets(hosts)
-    hosts.flat_map(&:storages).uniq.select { |s| Storage.supports?(s.store_type) & s.perf_capture_enabled? }
+    hosts.flat_map(&:storages).uniq.select { |s| s.storage_type_supported? & s.perf_capture_enabled? }


I was a little confused by this at first, I think because it sounds like we won't create the storage if it isn't supported.

I think this would more accurately be described as storage_type_supported_for_scan? or _for_ssa?. Maybe even add the storage type check into supports?(:smartstate_analysis) which we already have?

kbrock · 2022-12-01T19:24:22Z

update:

renamed storage_type_supported? to storage_type_supported_for_ssa?

app/models/vm_or_template.rb

kbrock · 2022-12-02T15:10:10Z

I think this would more accurately be described as storage_type_supported_for_scan? or _for_ssa?. Maybe even add the storage type check into supports?(:smartstate_analysis) which we already have?

@agrare I'll look into:

class Storage
  supports :smartstate_analysis do
    if !ext_management_system&.class&.supports?(:smartstate_analysis) || !storage_type_supported_for_ssa?
      _("Smartstate Analysis cannot be performed on selected Datastore")
    elsif !ext_management_system&.authentication_status_ok?
      _("There are no EMSs with valid credentials for this Datastore")
    end
  end
end

Do you want explicit wording for it?
I'll look to see if we can remove some of these checks if we have a not supports for smartstate

agrare · 2022-12-05T15:37:35Z

Do you want explicit wording for it?

@kbrock up to you, if we can give a more specific reason why we don't support it without much downside I don't see why not

We are triggering queue_signal for every flow, both for error and success cases. So detecting if the queue_signal is called doesn't mean anything. The positive cases were passing because either the vms were not updated or it was not calling dispatch at all. The negative cases were passing because an authorization was missing, which is not actually what was under test. The host now has authentication so we can do the tests the ems now has authentication so we can do more tests The vms are updated properly (it was not saving the storage before) The vms all have an ems (so they have authentications are scannable) The storages and repo_storage have an ems The tests are stating the signal they are expecting (not necessary) removed constants from specs (constants are defined in the global, not just in this spec) reference ems instead of ems id in factory

…eatureMixin conflict BTW/ This method name is a little tricky. The field is store_type but the constant is SUPPORTED_STORAGE_TYPES

The listed of supported storage types was different between metrics capture, vm_scan, and vm path This made them consistent.

I had replaced storage.storage_type_supports_ssa? calls to just use supports(:smartstate_analysis) but I wasn't sure if that was correct for a few. But I did keep ems authentication changes - they seemed like they may make introduce some false negatives in some areas of the code

kbrock · 2022-12-06T23:30:08Z

update

ensured objects created by factory/helper had ems references

Sorry took so many tries, the errors were not always obvious what was breaking the tests.

kbrock · 2022-12-06T23:33:24Z

app/models/manageiq/providers/infra_manager/metrics_capture.rb

@@ -78,7 +78,7 @@ def capture_host_targets(ems)
  # @return [Array<Storage>] supported storages
  # hosts preloaded storages and tags
  def capture_storage_targets(hosts)
-    hosts.flat_map(&:storages).uniq.select { |s| Storage.supports?(s.store_type) & s.perf_capture_enabled? }


so while we introduced storage_type_supported_for_ssa?, I rolled it into supports?(:smartstate_analysis) and basically removed all the calls.

Here, I felt perf_capture_enabled? wasn't part of supports, so I kept this separate.

miq-bot · 2022-12-06T23:34:40Z

Checked commits kbrock/manageiq@dbf6c93~...ac06484 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
10 files checked, 0 offenses detected
Everything looks fine. 🍪

By default, a storage should be valid for smart state Sure, it also needs an ems and others, but we definitely need a good store_type value This is a followup to the recent supports change ManageIQ#22258 / 28fa0a6

kbrock requested review from agrare and Fryguy as code owners December 1, 2022 01:36

nasark mentioned this pull request Dec 1, 2022

IsoDatastore to Storage support ManageIQ/manageiq-ui-classic#8548

Merged

3 tasks

kbrock force-pushed the storage_supports branch 3 times, most recently from 4770c21 to c6e97ef Compare December 1, 2022 19:13

agrare reviewed Dec 1, 2022

View reviewed changes

kbrock force-pushed the storage_supports branch from c6e97ef to c1aafde Compare December 1, 2022 19:23

agrare reviewed Dec 1, 2022

View reviewed changes

app/models/vm_or_template.rb Outdated Show resolved Hide resolved

kbrock force-pushed the storage_supports branch from c1aafde to b9e045e Compare December 1, 2022 21:06

agrare approved these changes Dec 1, 2022

View reviewed changes

agrare self-assigned this Dec 1, 2022

agrare added refactoring providers labels Dec 1, 2022

kbrock force-pushed the storage_supports branch from b9e045e to bcae267 Compare December 5, 2022 14:54

nasark mentioned this pull request Dec 6, 2022

Subclass IsoDatastore and transfer from core ManageIQ/manageiq-providers-ovirt#621

Merged

9 tasks

agrare closed this Dec 6, 2022

agrare reopened this Dec 6, 2022

kbrock added 4 commits December 6, 2022 18:25

rename method supports? to storage_type_supported? to avoid SupportsF…

57aead5

…eatureMixin conflict BTW/ This method name is a little tricky. The field is store_type but the constant is SUPPORTED_STORAGE_TYPES

make supported storage types consistent

2fde639

The listed of supported storage types was different between metrics capture, vm_scan, and vm path This made them consistent.

kbrock force-pushed the storage_supports branch from bcae267 to ac06484 Compare December 6, 2022 23:26

kbrock commented Dec 6, 2022

View reviewed changes

agrare approved these changes Dec 7, 2022

View reviewed changes

agrare merged commit 28fa0a6 into ManageIQ:master Dec 7, 2022

kbrock deleted the storage_supports branch December 7, 2022 14:33

kbrock mentioned this pull request Dec 9, 2022

set a valid Storage.store_type #22273

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

consolidate supported storage types #22258

consolidate supported storage types #22258

kbrock commented Dec 1, 2022 •

edited

Loading

kbrock commented Dec 1, 2022

agrare Dec 1, 2022

kbrock commented Dec 1, 2022

kbrock commented Dec 2, 2022 •

edited

Loading

agrare commented Dec 5, 2022

kbrock commented Dec 6, 2022

kbrock Dec 6, 2022

miq-bot commented Dec 6, 2022

consolidate supported storage types #22258

consolidate supported storage types #22258

Conversation

kbrock commented Dec 1, 2022 • edited Loading

kbrock commented Dec 1, 2022

agrare Dec 1, 2022

Choose a reason for hiding this comment

kbrock commented Dec 1, 2022

kbrock commented Dec 2, 2022 • edited Loading

agrare commented Dec 5, 2022

kbrock commented Dec 6, 2022

kbrock Dec 6, 2022

Choose a reason for hiding this comment

miq-bot commented Dec 6, 2022

kbrock commented Dec 1, 2022 •

edited

Loading

kbrock commented Dec 2, 2022 •

edited

Loading