Accept Hash object as result of inter region api invocation #18210

yrudman · 2018-11-15T22:52:12Z

Issue:
Setting retirement date on Service via Centralized Administration generates error in log:

ERROR -- : [InterRegionApiMethodRelay::InterRegionApiMethodRelayError]: Got unexpected API result object Hash  Method:[block in method_missing]
ERROR -- : /var/www/miq/vmdb/app/models/mixins/inter_region_api_method_relay.rb:84:in `exec_api_call'
/var/www/miq/vmdb/app/models/mixins/inter_region_api_method_relay.rb:32:in `block (2 levels) in api_relay_method'
/var/www/miq/vmdb/app/models/mixins/retirement_mixin.rb:16:in `block in retire'

Blocking: #18195 and ManageIQ/manageiq-ui-classic#4874

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1650335

@miq-bot add-label bug, blocker. hammer/yes, gaprindashvili/yes, core

yrudman · 2018-11-15T22:54:44Z

@miq-bot add-label hammer/yes

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1650335

yrudman · 2018-11-16T14:55:23Z

@miq-bot add-label blocker

yrudman · 2018-11-16T14:58:17Z

\cc @gtanzillo @carbonin

yrudman · 2018-11-16T15:03:56Z

@miq-bot remove-label wip

yrudman · 2018-11-16T15:12:58Z

@miq-bot assign @carbonin

miq-bot · 2018-11-16T15:41:32Z

Checked commits yrudman/manageiq@1e89ce1~...65aac7e with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. ⭐

carbonin · 2018-11-16T16:28:04Z

@yrudman did you open a BZ against the API to standardize responses?

/cc @bdunne @abellotti

carbonin · 2018-11-16T16:29:55Z

Also, do we know what happens in this case if we fail to retire the service? Will the client raise an Http error (I assume we will get a 5xx error of some sort) or will the hash contain a failure notification?

yrudman · 2018-11-16T20:07:06Z

Also, do we know what happens in this case if we fail to retire the service? Will the client raise an Http error (I assume we will get a 5xx error of some sort) or will the hash contain a failure notification?

API Client is raising the same error which was raised on remote region:
If API raising "Yura Error something" when executing retire_resource` on remote server,
on global server the same error raised:

irb(main):011:0> s.retire
RuntimeError: Yura Error something
	from /opt/rh/cfme-gemset/gems/manageiq-api-client-0.3.2/lib/manageiq/api/client/connection.rb:111:in `check_response'
	from /opt/rh/cfme-gemset/gems/manageiq-api-client-0.3.2/lib/manageiq/api/client/connection.rb:102:in `send_request'
	from /opt/rh/cfme-gemset/gems/manageiq-api-client-0.3.2/lib/manageiq/api/client/connection.rb:34:in `post'
	from /opt/rh/cfme-gemset/gems/manageiq-api-client-0.3.2/lib/manageiq/api/client/client.rb:65:in `post'
	from /opt/rh/cfme-gemset/gems/manageiq-api-client-0.3.2/lib/manageiq/api/client/resource.rb:60:in `exec_action'
	from /opt/rh/cfme-gemset/gems/manageiq-api-client-0.3.2/lib/manageiq/api/client/resource.rb:46:in `method_missing'
	from /var/www/miq/vmdb/app/models/mixins/inter_region_api_method_relay.rb:76:in `public_send'
	from /var/www/miq/vmdb/app/models/mixins/inter_region_api_method_relay.rb:76:in `exec_api_call'

carbonin · 2018-11-16T20:16:54Z

Okay, that's fine, just make sure to open a BZ against the API so we can remove this in the future.

bdunne · 2018-11-16T20:18:31Z

app/models/mixins/inter_region_api_method_relay.rb

@@ -80,6 +80,11 @@ def self.exec_api_call(region, collection_name, action, api_args = nil, id = nil
      result.attributes
    when ManageIQ::API::Client::Resource
      instance_for_resource(result)
+    when Hash


I think that this is the wrong solution.
Vm retirement returns an action result, which I believe is correct.
However, retiring anything else (see api/base_controller/generic.rb) is returning the resource, which I believe is wrong.
I think retirement of anything should return the action result. @abellotti Any thoughts on this?

carbonin · 2018-11-16T20:29:30Z

@bdunne I agree with you, but I don't think we're going to push an API refactoring for this in time to fix this blocker.

That's why I asked that a BZ be opened to address the API's return value inconsistencies.

bdunne · 2018-11-16T20:39:07Z

I wouldn't call it a refactoring... It's a bug in the API that retiring different things gives you different responses (resource hash vs action result).

a BZ be opened to address the API's return value inconsistencies

Tracking down and fixing all of the API response inconsistencies is an exponentially harder task than fixing the retirement responses and I'm afraid that it will never happen due to the difficulty level.

carbonin · 2018-11-16T20:42:34Z

I think Yuri was also concerned with introducing breaking API changes by backporting the switch from resource to action result.

We could also fix the API for hammer and backport only this to Gaprindashvili. But I don't know what kind of compatibility rules we follow (or don't follow) for the manageiq REST API. @abellotti ?

yrudman · 2018-11-16T20:47:57Z

I've started working on changing return value : ManageIQ/manageiq-api#511.
However, API is out for quite some time and I am not sure if PM should get involved too, since some clients may be affected

…f-inter-region-api-invocation Accept Hash object as result of inter region api invocation (cherry picked from commit 0f85990) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1650335

simaishi · 2018-11-16T21:00:19Z

Hammer backport details:

$ git log -1
commit 96f2924c85b7ad09eb5095f0688eb64b0e5ac1b5
Author: Nick Carboni <ncarboni@redhat.com>
Date:   Fri Nov 16 15:16:59 2018 -0500

    Merge pull request #18210 from yrudman/accept-hash-object-as-result-of-inter-region-api-invocation
    
    Accept Hash object as result of inter region api invocation
    
    (cherry picked from commit 0f8599059c5cb192be92e3dc7fbb10382874ba56)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1650335

yrudman · 2018-11-16T21:02:06Z

I think we should proceed for now with this solution (even if it looks wrong) since it is closing blockers for hammer and gaprindashvili and discuss/decide when we want to do breaking changes on API

yrudman · 2018-11-16T21:21:40Z

@carbonin @bdunne API's return value inconsistencies BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1650696

…f-inter-region-api-invocation Accept Hash object as result of inter region api invocation (cherry picked from commit 0f85990) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1650691

simaishi · 2018-11-19T14:04:58Z

Gaprindashvili backport details:

$ git log -1
commit 3a18916dbc986e58a0c385663314057498580f9f
Author: Nick Carboni <ncarboni@redhat.com>
Date:   Fri Nov 16 15:16:59 2018 -0500

    Merge pull request #18210 from yrudman/accept-hash-object-as-result-of-inter-region-api-invocation
    
    Accept Hash object as result of inter region api invocation
    
    (cherry picked from commit 0f8599059c5cb192be92e3dc7fbb10382874ba56)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1650691

yrudman mentioned this pull request Nov 15, 2018

[GAPRINDASHVILI]Fixed retire_now functionality for Service requested via Centralized Administration #18195

Merged

This comment has been minimized.

Sign in to view

miq-bot added bug core gaprindashvili/yes labels Nov 15, 2018

miq-bot added the hammer/yes label Nov 15, 2018

yrudman mentioned this pull request Nov 15, 2018

Rescue error messages when executing retire action ManageIQ/manageiq-ui-classic#4874

Merged

miq-bot added the wip label Nov 16, 2018

yrudman added 2 commits November 16, 2018 08:56

accept hash object as result of inter-region api invocation

1e89ce1

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1650335

rspec testing that exec_api_call accepts Hash as result object

65aac7e

yrudman force-pushed the accept-hash-object-as-result-of-inter-region-api-invocation branch from 59b2870 to 65aac7e Compare November 16, 2018 13:57

miq-bot added the blocker label Nov 16, 2018

yrudman changed the title ~~[WIP] Accept hash object as result of inter region api invocation~~ Accept hash object as result of inter region api invocation Nov 16, 2018

yrudman changed the title ~~Accept hash object as result of inter region api invocation~~ Accept Hash object as result of inter region api invocation Nov 16, 2018

yrudman mentioned this pull request Nov 16, 2018

[WIP] Return ActionResult instead of Hash when retiring Service ManageIQ/manageiq-api#511

Closed

miq-bot removed the wip label Nov 16, 2018

miq-bot assigned carbonin Nov 16, 2018

carbonin approved these changes Nov 16, 2018

View reviewed changes

carbonin merged commit 0f85990 into ManageIQ:master Nov 16, 2018

carbonin added this to the Sprint 99 Ending Nov 19, 2018 milestone Nov 16, 2018

bdunne reviewed Nov 16, 2018

View reviewed changes

yrudman deleted the accept-hash-object-as-result-of-inter-region-api-invocation branch November 16, 2018 20:32

simaishi added hammer/backported and removed hammer/yes labels Nov 16, 2018

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Nov 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accept Hash object as result of inter region api invocation #18210

Accept Hash object as result of inter region api invocation #18210

yrudman commented Nov 15, 2018 •

edited

Loading

This comment has been minimized.

yrudman commented Nov 15, 2018

yrudman commented Nov 16, 2018

yrudman commented Nov 16, 2018

yrudman commented Nov 16, 2018

yrudman commented Nov 16, 2018

miq-bot commented Nov 16, 2018

carbonin commented Nov 16, 2018

carbonin commented Nov 16, 2018

yrudman commented Nov 16, 2018

carbonin commented Nov 16, 2018

bdunne Nov 16, 2018

carbonin commented Nov 16, 2018

bdunne commented Nov 16, 2018

carbonin commented Nov 16, 2018

yrudman commented Nov 16, 2018

simaishi commented Nov 16, 2018

yrudman commented Nov 16, 2018

yrudman commented Nov 16, 2018

simaishi commented Nov 19, 2018

Accept Hash object as result of inter region api invocation #18210

Accept Hash object as result of inter region api invocation #18210

Conversation

yrudman commented Nov 15, 2018 • edited Loading

This comment has been minimized.

yrudman commented Nov 15, 2018

yrudman commented Nov 16, 2018

yrudman commented Nov 16, 2018

yrudman commented Nov 16, 2018

yrudman commented Nov 16, 2018

miq-bot commented Nov 16, 2018

carbonin commented Nov 16, 2018

carbonin commented Nov 16, 2018

yrudman commented Nov 16, 2018

carbonin commented Nov 16, 2018

bdunne Nov 16, 2018

Choose a reason for hiding this comment

carbonin commented Nov 16, 2018

bdunne commented Nov 16, 2018

carbonin commented Nov 16, 2018

yrudman commented Nov 16, 2018

simaishi commented Nov 16, 2018

yrudman commented Nov 16, 2018

yrudman commented Nov 16, 2018

simaishi commented Nov 19, 2018

yrudman commented Nov 15, 2018 •

edited

Loading