Use only cached version for vm import check

[matobet]

When called from UI context (i.e. an appliance with only UI role) the provider
model may not see the actual RHEV instance (by not having the ems_operations
role). This may be a problem in complex production environments so here we do
the safe fallback - when the version is nil we mark the import as not
supported.

Fixes ManageIQ/manageiq-ui-classic#1449
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1553074

[matobet]

@borod108 @jhernand please review

[matobet]

an alternative would be to simply check the provider value .api_version (!nil? && > 4), @jhernand any issues you would see with that?

[jhernand]

If I understand correctly the problem is that the value cached in memory isn't used if called from an appliance that doesn't contain the provider. If that is true then we should fix it. The cached value should be available in any appliance that runs the provider code. @borod108 Isn't the value stored in the memcached instance and available to all workers and appliances?

[matobet]

@jhernand the problem does not occur when the value is present in the cache but on the contrary when it isn't -> the UI appliance just cannot issue provider api calls (that is the privilege of only applicances with the ems_operations role). So the fix should involve only checks dependent on the data available in the DB. Invoking @martinpovolny as a source of higher authority :)

[martinpovolny]

@jhernand in not so uncommon usecase the front-end appliance can not "see" the providers/hypervisors. So it cannot issue any API calls to the provider. This may be due to firewall setup or network layout.

It's source of information about the environment is the database. Generally anything displayed in the UI should come from the database and not directly from the providers.

Another reason for this is performance.

If the UI needs to talk to the backend it does so throught the requests and tasks, not directly. You can busywait in the UI for a task see wait_for_task (sometimes it's needed) but generally you should not do that.

Surely displaying a button on a toolbar is something that should not require any direct interaction with the provider.

[jhernand]

I don't completely understand this, my fault, sure. You say that in this situation it may not be possible to use the provider API, but this modification is in the provider code, which may, at any point, try to use the provider API. Setting the force parameter to false isn't a guarantee that the code won't use the provider API in the future. So please use the version stored in the database instead.

For the future I think that we should consider storing in the database the list of capabilities supported by the provider, so that the UI code doesn't need to figure out capabilities from version numbers.

[matobet]

@jhernand yes this is indeed something that has bitten me more than once during the implementation of the v2v feature: the fact that a method that happens to interact with the provider api can find itself on the call stack reachable from the UI or automate and cause the above problems... @martinpovolny To my knowledge there is nothing in MIQ that would mark parts of code as usable from given roles....

[matobet]

@jhernand simplified to plain api_version comparison (wrapped in Gem::Version as you suggested)

[matobet]

@miq-bot assign @jhernand

[jhernand]

@matobet remember to fix the failing test.

[oourfali]

@matobet let me know when fixed, and then I'll merge it.

[matobet]

@miq-bot add_label fine/yes

[miq-bot]

Checked commit matobet@2e86f81 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

[matobet]

@oourfali fixed tests

[simaishi]

@matobet Is there a BZ for this? Can you please create if it doesn't exist?

[simaishi]

@matobet ping, is this still needed in Fine branch?

[pkliczewski]

@mwperina Please take a look at @simaishi question.

[mwperina]

@jelkosz Tomasi, could you please reply to Satoe questions?

[borod108]

@pkliczewski @mwperina if we use ovirt_services in fine then we need it. Else the UI will just hang.

[jelkosz]

@mwperina
I believe it does not have a BZ since it has been opened only as a github issue: ManageIQ/manageiq-ui-classic#1449
I have opened the BZ https://bugzilla.redhat.com/show_bug.cgi?id=1553074 but did not set the targets - I'll let this to you.

And based on ManageIQ/manageiq-ui-classic#1449 (comment) it is needed in Fine.

[simaishi]

There was a conflict on backport to Fine branch. Looking at the current Fine branch code, it looks like it's ok as is?

  def validate_import_vm
    # The version of the RHV needs to be at least 4.1.5 due to https://bugzilla.redhat.com/1477375
    version_higher_than?('4.1.5')
  end

(changed in ManageIQ/manageiq#15784)

[jelkosz]

@simaishi indeed, the ManageIQ/manageiq#15784 have introduced the version_higher_than? method which is based on the cached version. So you are right, it happens to be already OK in the Fine branch.