Add features based on supported_api_versions to RedHat provider #11418

borod108 · 2016-09-21T13:47:36Z

Using the SupportedFeaturesMixin to set features that are supported by the RedHat provider based on the API versions.
Using the OvirtSDK4 Probe to find which API versions are supported by the server.
This patch is dependant on OvirtSDK4 4.0.2

borod108 · 2016-09-22T05:09:24Z

Please note rubocop is houting mainly because I moved existing code and it does not like the code that existed.
@blomquisg can you please take a look?
This should go in the next release so there are only 2 weeks to go, and there are at least 2 other patches based on this. Thank you!
@jhernand I would be very grateful for a review.

jhernand · 2016-09-22T09:32:32Z

app/models/manageiq/providers/redhat/infra_manager/api_integration.rb

+  end
+
+  def cache_key
+    "REDHAT_EMS_CACHE_KEY_#{id}"


Can we use OVIRT instead of REDHAT ?

We can, but in the source code all the code related to oVirt is under a folder and name space named "redhat" that is why I used REDHAT. Considering this do you still think it should be OVIRT?

jhernand · 2016-09-22T09:38:08Z

app/models/manageiq/providers/redhat/infra_manager/api_integration.rb

+    %w(default metrics)
+  end
+
+  class Cacher


I find this a bit over engineered. Do we really need this class? Can't we just call Rails.cache.read and Rails.cache.write directly?

@jhernand well I actually tried that at first, then wrote the spec, then re factored, the reason being that because of the need to rebuild the cache based on whether it is older then the last refresh (that is what we discussed with you when designing this) it is not a one liner it require couple of extra methods which I do not think belong in the class directly.
Second point is that this seems to be the first place Rails.cache is used outside of specs. While I think this is the right way to go, perhaps there is a reason no one used it and it will have to change to use some other mechanism. In this case changing the code in the Cacher class which is independent of the infra_menager implementation will be easier.

durandom · 2016-09-22T15:42:45Z

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

-  def supported_auth_types
-    %w(default metrics)
-  end
-
  def supports_authentication?(authtype)


It's not part of this PR, but this method will clash once there is a feature called authentication. Because the SupportsFeatureMixin will add a method supports_authentication? 😟

Maybe you can rename that in another PR?

Thank you! You are absolutely right. I will. Please tell me how to make sure that those PRs will be merged in the right order?

@durandom wait no! thats not true, it will only add methods for the symbols in api3_supported_features and api4_supported_features, https://github.com/ManageIQ/manageiq/pull/11418/files#diff-d6125c21d43685840429020f8c5a57aaR186
other methods will not be effected. So if someone adds "authentication" symbol there, then it might cause a problem. But rightly so - if its not supposed to support authentication from some API version then it should be addressed.

durandom · 2016-09-22T15:45:38Z

app/models/manageiq/providers/redhat/infra_manager/api_integration.rb

+    end
+  end
+
+  def supports_api_version?(version)


same here: supports_api_version? will clash with a feature called api_version

right, this one was actually added in this PR so I will just change it here.

durandom · 2016-09-22T15:48:01Z

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

+  require_nested  :Refresher
+  require_nested  :Template
+  require_nested  :Vm
+  include_concern :ApiIntegration


Is there a reason of moving this to a module? I find it harder to review this way, because I dont know what actually changed.

I am not sure what to do with this, i mean I think it should definitely move to a separate module (actually I think it should be built differently and move to a different class all together) and I could not really work on this when it was all mixed. So I should have moved it first, open a PR on that and only then do the rest. But I did not. If its crucial I will do it now. Is it? @durandom

Well, its not crucial, but it makes it way easier to review and less error prone imho :)
If its not such a big deal and doesnt eat up too much time, yes, please.

I'd leave the connect methods in the manager class, because they are needed for with_provider_connection. But you sure can delegate that to an new class.

And yes, I'm all for proper separation of concerns and adding more classes than just stuffing code into modules.

Oh, and I think such a refactoring PR would get merged pretty fast

@durandom ok added the PR: #11482
When its merged I will rebase this one over it.

durandom · 2016-09-22T15:52:33Z

app/models/manageiq/providers/redhat/infra_manager/api_integration.rb

+    end
+
+    def api4_supported_features 
+      [:reconfigure_vms, :snapshots]


those would need to be added to SupportsFeatureMixin::QUERYABLE_FEATURES
Also those should already have some corresponding validate_ methods in the code, where the UI asks if the manager can handle this operation.

It is https://github.com/ManageIQ/manageiq/pull/11418/files#diff-0613648e8f7d7c13b5aeb5071c90f6d2R73
and the validate exists for the snapshot, and to be added after this patch for reconfigure_vms.

durandom · 2016-09-23T08:45:08Z

app/models/manageiq/providers/redhat/infra_manager/api_integration.rb

+
+  def supported_api_verions_from_sdk
+    with_provider_connection(:path => '', :version => 4, :skip_supported_api_validation => true) do |connection|
+      OvirtSDK4::Probe.probe(connection)


What does probe actually do?
Does it connect and the return a list like %w(v3 v4) ?
Can the result change over time?

Another way would be to store the result in something like models/advanced_settings.rb and check if thats correct upon validate_authentication. This is called whenever sombody clicks the button in the UI or when a worker is started.

Probe connects to server and by sending specific headers and analysing the response to them it figures out which versions of the API the oVirt server supports. Later it returns %w(3, 4)
The result can change over time if the server is upgraded. So the current mechanism is to recheck after every refresh. This is why it is cached.
What will be the advantages of using advanced_settings.rb?

What will be the advantages of using advanced_settings.rb?

you dont need that caching stuff, but store it right in the db

you can examine the setting from the UI

miq-bot · 2016-09-26T20:28:02Z

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

borod108 · 2016-09-27T07:53:37Z

@miq-bot add-label euwe/yes

miq-bot · 2016-09-30T21:47:04Z

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

miq-bot · 2016-10-05T14:07:11Z

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

…sions supported

…on it supports

durandom

apart from reconfigure_vms still in SupportsFeatureMixin I'm good with the state of this.

@chrisarcand can you give your 👍 or 👎 on the cache thing?

durandom · 2016-10-12T10:02:26Z

app/models/mixins/supports_feature_mixin.rb

@@ -76,12 +77,14 @@ module SupportsFeatureMixin
    :provisioning                 => 'Provisioning',
    :reboot_guest                 => 'Reboot Guest Operation',
    :reconfigure                  => 'Reconfiguration',
+    :reconfigure_vms              => 'Reconfigure Virtual Machines',


you'll want to remove this here too

right, was sure I did it, sorry.

chrisarcand · 2016-10-12T12:44:24Z

@durandom Will do; I'll be reviewing the caching stuff in earnest today around 10:30 CST (-600), about 3 hours from now, so this will be merged soon.

chrisarcand

Looking at this closer, caching stuff is fine. My original thought is that the caching is so simple that you could roll your own simple cache but utilizing the Rails file store is fine and any changes I'd do are a little weird because of the fact that this is a mixin. Good for now 👍

durandom · 2016-10-12T17:25:25Z

@chrisarcand thanks for the in depth look

@miq-bot assign @blomquisg

@blomquisg please merge

chessbyte · 2016-10-13T21:10:55Z

Euwe Backport details:

$ git log
commit b6913002e51033a6ca805caab1fa697d191ae023
Author: Greg Blomquist <blomquisg@gmail.com>
Date:   Thu Oct 13 02:26:11 2016 -0400

    Merge pull request #11418 from borod108/rfe/add_cache_to_api_versions

    Add features based on supported_api_versions to RedHat provider
    (cherry picked from commit 855ea0f3238617be8a931794c8783188329a1103)

Add features based on supported_api_versions to RedHat provider (cherry picked from commit 855ea0f)

https://bugzilla.redhat.com/show_bug.cgi?id=1423470 b691300 for PR ManageIQ#11418 was backported before 084ecea for PR ManageIQ#11018, causing some git context to bring over the :terminate key twice.

miq-bot added the dependencies label Sep 21, 2016

borod108 force-pushed the rfe/add_cache_to_api_versions branch from 54bcf1b to b1ae582 Compare September 21, 2016 14:11

chessbyte changed the title ~~Addind features basd on supported_api_versions to RedHat provider~~ Add features based on supported_api_versions to RedHat provider Sep 21, 2016

chessbyte added the providers/rhevm label Sep 21, 2016

chessbyte assigned durandom Sep 21, 2016

chessbyte added refactoring pluggability/providers labels Sep 21, 2016

borod108 mentioned this pull request Sep 22, 2016

Disable snapshots on oVirt for APIs below v4 Bug/1375551 #11441

Closed

jhernand reviewed Sep 22, 2016

View reviewed changes

durandom reviewed Sep 22, 2016

View reviewed changes

durandom suggested changes Sep 22, 2016

View reviewed changes

durandom reviewed Sep 23, 2016

View reviewed changes

borod108 force-pushed the rfe/add_cache_to_api_versions branch 3 times, most recently from 9cfbbbc to 6b81b96 Compare September 25, 2016 09:30

borod108 force-pushed the rfe/add_cache_to_api_versions branch 2 times, most recently from 9a9aa8d to c866cf3 Compare September 27, 2016 07:51

miq-bot added the euwe/yes label Sep 27, 2016

borod108 force-pushed the rfe/add_cache_to_api_versions branch from c866cf3 to e36a68c Compare September 27, 2016 08:09

borod108 force-pushed the rfe/add_cache_to_api_versions branch from e36a68c to ae5dc1b Compare October 3, 2016 08:17

borod108 mentioned this pull request Oct 5, 2016

Update ovirt-engine-sdk gem version to 4.0.2 #11705

Merged

borod108 force-pushed the rfe/add_cache_to_api_versions branch 2 times, most recently from c2930ef to f8c8ecc Compare October 5, 2016 10:53

borod108 mentioned this pull request Oct 5, 2016

add quick_stats support for RHEVM host #10505

Merged

borod108 force-pushed the rfe/add_cache_to_api_versions branch from f8c8ecc to 2ab0898 Compare October 5, 2016 14:30

borod108 force-pushed the rfe/add_cache_to_api_versions branch 5 times, most recently from debe598 to 20e82c8 Compare October 12, 2016 09:27

borod108 added 5 commits October 12, 2016 12:28

adding basic supported_api_versions to provider

aaab988

moving code from infra_manager to api_integration

12764e8

adding SupportFeaturesMixin for diffrent methods depending on API ver…

9f70659

…sions supported

infra_manager now queries the ovirt engine to establish the api versi…

f582b66

…on it supports

improve tests for supported api features

771b232

borod108 force-pushed the rfe/add_cache_to_api_versions branch from 20e82c8 to 1d55545 Compare October 12, 2016 09:28

durandom suggested changes Oct 12, 2016

View reviewed changes

add integration with ovirtsdk

85aa509

borod108 force-pushed the rfe/add_cache_to_api_versions branch from 1d55545 to 85aa509 Compare October 12, 2016 10:41

chrisarcand approved these changes Oct 12, 2016

View reviewed changes

miq-bot assigned blomquisg and unassigned durandom Oct 12, 2016

blomquisg merged commit 855ea0f into ManageIQ:master Oct 13, 2016

blomquisg added this to the Sprint 48 Ending Oct 24, 2016 milestone Oct 13, 2016

chessbyte added euwe/backported and removed euwe/yes labels Oct 13, 2016

chessbyte pushed a commit that referenced this pull request Oct 13, 2016

Merge pull request #11418 from borod108/rfe/add_cache_to_api_versions

b691300

Add features based on supported_api_versions to RedHat provider (cherry picked from commit 855ea0f)

durandom mentioned this pull request Dec 7, 2016

Memory checkbox will not show when VM is not up #12678

Merged

jrafanie mentioned this pull request Feb 20, 2017

[EUWE] Hash key 'terminate' was duplicated due to backport PR order #13987

Merged

Fryguy added the providers/infrastructure label Jan 24, 2020

Fryguy unassigned blomquisg Jan 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add features based on supported_api_versions to RedHat provider #11418

Add features based on supported_api_versions to RedHat provider #11418

borod108 commented Sep 21, 2016 •

edited by miq-bot

Loading

borod108 commented Sep 22, 2016 •

edited

Loading

jhernand Sep 22, 2016

borod108 Sep 23, 2016

jhernand Sep 22, 2016

borod108 Sep 23, 2016

durandom Sep 22, 2016

borod108 Sep 23, 2016

borod108 Sep 23, 2016

durandom Sep 22, 2016

borod108 Sep 23, 2016

durandom Sep 22, 2016

borod108 Sep 23, 2016

durandom Sep 23, 2016

durandom Sep 23, 2016

borod108 Sep 23, 2016

durandom Sep 22, 2016

borod108 Sep 23, 2016

durandom Sep 23, 2016

borod108 Sep 23, 2016

durandom Sep 23, 2016

miq-bot commented Sep 26, 2016

borod108 commented Sep 27, 2016

miq-bot commented Sep 30, 2016

miq-bot commented Oct 5, 2016

durandom left a comment

durandom Oct 12, 2016

borod108 Oct 12, 2016

chrisarcand commented Oct 12, 2016

chrisarcand left a comment

durandom commented Oct 12, 2016

chessbyte commented Oct 13, 2016

Add features based on supported_api_versions to RedHat provider #11418

Add features based on supported_api_versions to RedHat provider #11418

Conversation

borod108 commented Sep 21, 2016 • edited by miq-bot Loading

borod108 commented Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Sep 26, 2016

borod108 commented Sep 27, 2016

miq-bot commented Sep 30, 2016

miq-bot commented Oct 5, 2016

durandom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisarcand commented Oct 12, 2016

chrisarcand left a comment

Choose a reason for hiding this comment

durandom commented Oct 12, 2016

chessbyte commented Oct 13, 2016

borod108 commented Sep 21, 2016 •

edited by miq-bot

Loading

borod108 commented Sep 22, 2016 •

edited

Loading