Try both API versions in `raw_connect` #132

jhernand · 2017-11-06T14:51:20Z

Currently the raw_connect method needs to receive the version of the
API in order to work correctly. If it doesn't receive it it fails
because the method that determines it just doesn't exist (it is an
instance method, but raw_connect is a class method). To avoid that
this patch changes the raw_connect method so that it tries with both
versions of the API.

This patch fixes partially the following bug:

Failed validation when adding RHV provider
https://bugzilla.redhat.com/1509432

jhernand · 2017-11-06T14:52:56Z

@borod108 @masayag @pkliczewski @jntullo please review.

If/when this is merged we will be able to remove the scheme and version options from the call that the UI does to raw_connect.

pkliczewski · 2017-11-06T14:55:52Z

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

+        v4_connection.test(raise_exception=true)
+      rescue Exception => exception
+        v4_error = exception
+      end


Why not to have ensure here as for v3 below? Do we want to keep it open?

I saw the comment. Please ignore

pkliczewski · 2017-11-06T14:58:19Z

@jhernand Can we fix codeclimate comments?

borod108 · 2017-11-06T15:00:44Z

@jhernand why can't we use probing with queue? something like https://github.com/ManageIQ/manageiq-providers-ovirt/pull/111/files#diff-4d8ab13ae69c2eade9d82675975c0defR56

jhernand · 2017-11-06T15:26:39Z

@pkliczewski in the new version of the pull request I tried to address the style issues.

@borod108 probing via a queued request would mean two queued request, as the original call to raw_connect goes already via a queue. The result would be painfully slow.

jhernand · 2017-11-06T15:28:37Z

Bug 1509432 - Failed validation when adding RHV provider

jhernand · 2017-11-06T15:29:17Z

@miq-bot add_label wip

I didn't test this with oVirt 3 yet, so please don't merge.

pkliczewski · 2017-11-06T15:57:09Z

@jhernand Looks good to me, is it still WIP?

jhernand · 2017-11-06T16:00:19Z

@pkliczewski yes, it is work in progress, I need to test that it works correctly with oVirt 3.6.

jhernand · 2017-11-06T17:53:05Z

@miq-bot remove_label wip

Did some important changes to make this work with both versions of the API. I verified that it works correctly now for oVirt 4 and oVirt 3.

However, I also detected that the regression also affects the validation of the details of the C&U metrics endpoint. I am working on that. I'd suggest to merge this as a first step.

oourfali · 2017-11-06T17:56:32Z

@jhernand I'll merge once ci finishes.

jhernand · 2017-11-06T18:21:00Z

Please don't merge, checking the connection works, but using it doesn't :-( .

agrare · 2017-11-06T19:58:21Z

@jhernand confirmed this works for validation but not for normal connections. Looks like just fallout from moving a number of methods to class methods, let me know if there is anything I can do to help.

jhernand · 2017-11-06T20:05:37Z

@agrare would be nice if you can take a look and help find the right Ruby way to call the resolve_ip_address method from both instance and class methods. The typical self.resolve_ip_address doesn't work. I guess that is related to the fact that the methods are defined in a concern, and using the class_methods mechanism. I am trying now to call them with self.class.resolve_ip_address, but I have to admit that I am a bit lost.

agrare · 2017-11-06T20:14:54Z

@jhernand you're right about self.class.resolve_ip_address but it wasn't working because they're private class methods.

Try with this patch:

diff --git a/app/models/manageiq/providers/redhat/infra_manager/api_integration.rb b/app/models/manageiq/providers/redhat/infra_manager/api_integration.rb
index d917e43..5c7d6e0 100644
--- a/app/models/manageiq/providers/redhat/infra_manager/api_integration.rb
+++ b/app/models/manageiq/providers/redhat/infra_manager/api_integration.rb
@@ -41,8 +41,8 @@ module ManageIQ::Providers::Redhat::InfraManager::ApiIntegration
     # Starting with version 4 of oVirt authentication doesn't work when using directly the IP address, it requires
     # the fully qualified host name, so if we received an IP address we try to convert it into the corresponding
     # host name:
-    if resolve_ip_addresses?
-      resolved = resolve_ip_address(connect_options[:server])
+    if self.class.resolve_ip_addresses?
+      resolved = self.class.resolve_ip_address(connect_options[:server])
       if resolved != connect_options[:server]
         _log.info("IP address '#{connect_options[:server]}' has been resolved to host name '#{resolved}'.")
         default_endpoint.hostname = resolved
@@ -266,8 +266,8 @@ module ManageIQ::Providers::Redhat::InfraManager::ApiIntegration
       # the fully qualified host name, so if we received an IP address we try to convert it into the corresponding
       # host name:
       resolved = server
-      if resolve_ip_addresses?
-        resolved = resolve_ip_address(server)
+      if self.class.resolve_ip_addresses?
+        resolved = self.class.resolve_ip_address(server)
         if resolved != server
           _log.info("IP address '#{server}' has been resolved to host name '#{resolved}'.")
         end
@@ -391,8 +391,6 @@ module ManageIQ::Providers::Redhat::InfraManager::ApiIntegration
       href && href.split("/").last
     end
 
-    private
-
     #
     # Checks if IP address to host name resolving is enabled.
     #

jhernand · 2017-11-06T20:24:42Z

@agrare self.class seems to work, even if I don't remove the private, and I really would like to preserve the private. Unit tests are still failing, but I am getting close. Thanks!

agrare · 2017-11-06T20:32:38Z

@jhernand I had an issue with them being private when calling #connect

>> ExtManagementSystem.first.connect
NoMethodError: private method `resolve_ip_addresses?' called for #<Class:0x0055ca75d2e5c0>
	from /var/lib/gems/2.3.0/gems/activerecord-5.0.6/lib/active_record/dynamic_matchers.rb:21:in `method_missing'
	from /home/agrare/workspace/redhat/manageiq/plugins/manageiq-providers-ovirt/app/models/manageiq/providers/redhat/infra_manager/api_integration.rb:44:in `connect'

Is this logic that we could move into the raw_connect_v4 method? The comment states it starts in version 4.

jhernand · 2017-11-06T21:01:02Z

@agrare the name resolution logic can't be moved to the raw_connect_v4 method. The reason is that the name resolution is needed in order to work with version 4 of the the oVirt server, regardless of what version of the API is used. Version 4 of the server supports versions 3 and 4 of the API, version 3 of the server only supports version 3 of the API. But the name resolution has to be done before knowing the version of the server.

jhernand · 2017-11-07T08:28:22Z

@agrare you were right about the need to remove the private, unfortunately. I didn't find other way to make this work than removing it and using self.class to call those methods. It is ugly, but works. I marked them with @api private, hopefully future readers will understand.

agrare

This looks good to me, just need to get the tests passing.

Currently the `raw_connect` method needs to receive the version of the API in order to work correctly. If it doesn't receive it it fails because the method that determines it just doesn't exist (it is an instance method, but `raw_connect` is a class method). To avoid that this patch changes the `raw_connect` method so that it tries with both versions of the API. This patch fixes partially the following bug: Failed validation when adding RHV provider https://bugzilla.redhat.com/1509432

miq-bot · 2017-11-07T14:14:36Z

Checked commit https://github.com/jhernand/manageiq-providers-ovirt/commit/30bf6a128a42b11813f383abf04861900111f25e with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
8 files checked, 0 offenses detected
Everything looks fine. 👍

jhernand · 2017-11-07T14:27:38Z

Dear reviewers, this is now, to the best of my understanding, working with oVirt 3 and oVirt 4, and it passes the tests. Please re-review.

agrare

👍 thanks so much @jhernand !

…nnect Try both API versions in `raw_connect` (cherry picked from commit cb185fc) https://bugzilla.redhat.com/show_bug.cgi?id=1510504

simaishi · 2017-11-07T15:30:35Z

Gaprindashvili backport details:

$ git log -1
commit 3bc6ba4b1f11bf7c8574e9a378dec8cbbde362ea
Author: Adam Grare <[email protected]>
Date:   Tue Nov 7 09:32:13 2017 -0500

    Merge pull request #132 from jhernand/try_both_api_versions_in_raw_connect
    
    Try both API versions in `raw_connect`
    (cherry picked from commit cb185fc11567f66ad71d2287e0bc99e9507420a5)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1510504

pkliczewski reviewed Nov 6, 2017

View reviewed changes

jhernand changed the title ~~Try both API versions in raw_connect~~ [WIP] Try both API versions in raw_connect Nov 6, 2017

miq-bot added the wip label Nov 6, 2017

jhernand mentioned this pull request Nov 6, 2017

Always use HTTPS to interact with oVirt ManageIQ/manageiq-ui-classic#2633

Merged

jhernand changed the title ~~[WIP] Try both API versions in raw_connect~~ Try both API versions in raw_connect Nov 6, 2017

miq-bot removed the wip label Nov 6, 2017

chessbyte requested a review from agrare November 6, 2017 19:34

jhernand mentioned this pull request Nov 7, 2017

Check metrics details from raw_connect #134

Merged

agrare reviewed Nov 7, 2017

View reviewed changes

agrare added the bug label Nov 7, 2017

agrare self-assigned this Nov 7, 2017

agrare approved these changes Nov 7, 2017

View reviewed changes

agrare merged commit cb185fc into ManageIQ:master Nov 7, 2017

agrare added the gaprindashvili/yes label Nov 7, 2017

agrare modified the milestone: Sprint 73 Ending Nov 13, 2017 Nov 7, 2017

simaishi pushed a commit that referenced this pull request Nov 7, 2017

Merge pull request #132 from jhernand/try_both_api_versions_in_raw_co…

3bc6ba4

…nnect Try both API versions in `raw_connect` (cherry picked from commit cb185fc) https://bugzilla.redhat.com/show_bug.cgi?id=1510504

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Nov 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try both API versions in `raw_connect` #132

Try both API versions in `raw_connect` #132

jhernand commented Nov 6, 2017

jhernand commented Nov 6, 2017

pkliczewski Nov 6, 2017

pkliczewski Nov 6, 2017

pkliczewski commented Nov 6, 2017

borod108 commented Nov 6, 2017

jhernand commented Nov 6, 2017

jhernand commented Nov 6, 2017

jhernand commented Nov 6, 2017

pkliczewski commented Nov 6, 2017

jhernand commented Nov 6, 2017

jhernand commented Nov 6, 2017

oourfali commented Nov 6, 2017

jhernand commented Nov 6, 2017

agrare commented Nov 6, 2017

jhernand commented Nov 6, 2017

agrare commented Nov 6, 2017

jhernand commented Nov 6, 2017

agrare commented Nov 6, 2017

jhernand commented Nov 6, 2017

jhernand commented Nov 7, 2017

agrare left a comment

miq-bot commented Nov 7, 2017

jhernand commented Nov 7, 2017

agrare left a comment

simaishi commented Nov 7, 2017

Try both API versions in raw_connect #132

Try both API versions in raw_connect #132

Conversation

jhernand commented Nov 6, 2017

jhernand commented Nov 6, 2017

pkliczewski Nov 6, 2017

Choose a reason for hiding this comment

pkliczewski Nov 6, 2017

Choose a reason for hiding this comment

pkliczewski commented Nov 6, 2017

borod108 commented Nov 6, 2017

jhernand commented Nov 6, 2017

jhernand commented Nov 6, 2017

jhernand commented Nov 6, 2017

pkliczewski commented Nov 6, 2017

jhernand commented Nov 6, 2017

jhernand commented Nov 6, 2017

oourfali commented Nov 6, 2017

jhernand commented Nov 6, 2017

agrare commented Nov 6, 2017

jhernand commented Nov 6, 2017

agrare commented Nov 6, 2017

jhernand commented Nov 6, 2017

agrare commented Nov 6, 2017

jhernand commented Nov 6, 2017

jhernand commented Nov 7, 2017

agrare left a comment

Choose a reason for hiding this comment

miq-bot commented Nov 7, 2017

jhernand commented Nov 7, 2017

agrare left a comment

Choose a reason for hiding this comment

simaishi commented Nov 7, 2017

Try both API versions in `raw_connect` #132

Try both API versions in `raw_connect` #132