-
Notifications
You must be signed in to change notification settings - Fork 898
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Separate vm store and datastore smartstate scan supports checks #22738
Separate vm store and datastore smartstate scan supports checks #22738
Conversation
app/models/vm_scan/dispatcher.rb
Outdated
if @vm.requires_storage_for_scan? | ||
if @vm.storage.nil? | ||
msg = "Vm [#{@vm.path}] is not located on a storage, aborting job [#{job.guid}]." | ||
queue_signal(job, {:args => [:abort, msg, "error"]}) | ||
return [] | ||
else | ||
unless @vm.storage.supports?(:smartstate_analysis) | ||
msg = @vm.storage.unsupported_reason(:smartstate_analysis) | ||
queue_signal(job, {:args => [:abort, msg, "error"]}) | ||
return [] | ||
end | ||
end | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So https://github.com/ManageIQ/manageiq-providers-ovirt/pull/653/files#diff-24aea1f97754a89593e8311e8ea2bf2d897a159336d1f1c578d17fbb1964506dR10 (I think rightly) moves the storage type check into the VM's supports?(:smartstate_analysis)
, we still need to check @vm.supports?(:smartstate_analysis)
somewhere? Is this done elsewhere?
0ceeeac
to
79c082f
Compare
@miq-bot add_label quinteros/yes? |
79c082f
to
c4e086c
Compare
@@ -246,18 +246,10 @@ def get_eligible_proxies_for_job(job) | |||
return [] | |||
end | |||
|
|||
if @vm.requires_storage_for_scan? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow-up, but I think we can get rid of requires_storage_for_scan?
now?
app/models/storage.rb
Outdated
|
||
supports(:delete) { _("Only storage without VMs and Hosts can be removed") if vms_and_templates.any? || hosts.any? } | ||
|
||
supports_not :smartstate_analysis |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't believe we need explicit supports_not
anymore as of #22751
supports_not :smartstate_analysis |
4538922
to
7ec8278
Compare
From Pull Request: ManageIQ/manageiq#22738
7ec8278
to
2adb6c3
Compare
@miq-bot cross-repo-tests ManageIQ/manageiq-providers-vmware#887 |
From Pull Request: ManageIQ/manageiq#22738
Checked commit nasark@2adb6c3 with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint |
Backported to
|
…_scans Separate vm store and datastore smartstate scan supports checks (cherry picked from commit 2d545d0)
The supports
:smartstate_analysis
block is attempting to do too many things at once with its conditional checks and this is causing issues when differentiating storage scans vs. vm scans with storages. Therefore attempting to simplify the logic by just usingsupports_not
and then adding asupports :smartstate_analysis
to vmware since its the only provider that supports storage scans. RHV/oVirt providers that support vm scans with storages will check the storage type when checking if the vm supports smartstate analysis.Depends:
@miq-bot add_label bug, refactoring
@miq-bot assign @agrare