Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 1367039 - Azure cloud discovery does not handle multiple subscriptions
Summary: Azure cloud discovery does not handle multiple subscriptions
Keywords:
Status: CLOSED DUPLICATE of bug 1375285
Alias: None
Product: Red Hat CloudForms Management Engine
Classification: Red Hat
Component: Providers
Version: 5.6.0
Hardware: x86_64
OS: Linux
medium
high
Target Milestone: GA
: 5.7.0
Assignee: Bronagh Sorota
QA Contact: Jeff Teehan
URL:
Whiteboard: provider:azure:discover
Depends On:
Blocks: 1375285
TreeView+ depends on / blocked
 
Reported: 2016-08-15 11:16 UTC by Colin Arnott
Modified: 2016-11-08 04:00 UTC (History)
10 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
: 1375285 (view as bug list)
Environment:
Last Closed: 2016-09-15 19:54:05 UTC
Category: ---
Cloudforms Team: ---


Attachments (Terms of Use)
armrest_service.rb (deleted)
2016-08-15 11:16 UTC, Colin Arnott
no flags Details
cloud-discovery-without-patches.png (deleted)
2016-08-15 11:17 UTC, Colin Arnott
no flags Details
manager_mixin.rb (deleted)
2016-08-15 19:50 UTC, Colin Arnott
no flags Details
cloud-discover-with-patches.png (deleted)
2016-08-15 19:51 UTC, Colin Arnott
no flags Details

Description Colin Arnott 2016-08-15 11:16:13 UTC
Created attachment 1190852 [details]
armrest_service.rb

Description of problem:
The current code expects that there is only one subscription per tenant and returns the subscription associated.  If a tenant has more than one subscription associated with it it will use the first 'active' subscription and run the discovery on that subscription.  For the returned subscription the code iterates through all the regions and determines whether they are in use.  It then creates an entry called Azure-<region>.

In my case, the subscription returned (as I have multiple assigned to the tenant) did not have any active instances so was skipped and why I saw nothing created.

The current code isn't designed to handle my case of multiple subscriptions/tenant.  It would also "fail" for another tenant as the name "Azure-<region>" would already be in use.  I did see the possibility in the code to append an incremented # to handle that case but the names would still be ugly... 


Version-Release number of selected component (if applicable):
cfme-5.6.0.13

How reproducible:
given conditions, easy

Steps to reproduce:
0) have Azure configured as described
1) initiate cloud discovery

Actual results:
no new Azure providers

Expected results:
new Azure providers

Additional info:
I have patched the code to handle this situation.  It now iterates through all the tenants active subscriptions and regions and adds them as needed.  The entries use a naming convention of Azure-<region>-<subscription display name> which is pretty much guaranteed to be unique as subscriptions (and hopefully display names) are unigue regardless of tenant.

I am attaching the code and screenshots of the before and after patching runs...

The basics of the patch are to returns a subscription list and iterate through that.  This required an update to:
    /var/www/miq/vmdb/app/models/manageiq/providers/azure/manager_mixin.rb
    /opt/rh/cfme-gemset/gems/azure-armrest-0.2.7/lib/azure/armrest/armrest_service.rb

Comment 2 Colin Arnott 2016-08-15 11:17:19 UTC
Created attachment 1190853 [details]
cloud-discovery-without-patches.png

Comment 4 Colin Arnott 2016-08-15 19:50:49 UTC
Created attachment 1190981 [details]
manager_mixin.rb

Comment 5 Colin Arnott 2016-08-15 19:51:46 UTC
Created attachment 1190982 [details]
cloud-discover-with-patches.png

Comment 6 Bronagh Sorota 2016-08-18 15:08:00 UTC
Dan,
I have reviewed the manager_mixin.rb but i think you should review the changes they made to armrest_service.rb and determine if they are safe.

thanks
Bronagh

Comment 7 Bronagh Sorota 2016-08-18 17:29:45 UTC
Colin

I visually reviewed the manager_mixin.rb code changes and although they seem safe, I question why they are creating a new ::Azure::Armrest::VirtualMachineService instance for every subscription, the config is the same each time. See line 59 of their manager_mixin.rb file.

Dan is going to review the armrest gem changes.


Jeff,

Is it possible for you to test their implementation on a Darga appliance? Dan is planning on adding multiple subscriptions to our tenant, you could use those credentials to test.

Bronagh

Comment 9 Daniel Berger 2016-08-18 19:07:27 UTC
The modifications to armrest_service.rb are unnecessary.

There is already a 'subscriptions' method that is inherited by all the service classes. However, that does not check to see if they're enabled. Rather than change the armrest gem, I would just add a small check within the discover loop and skip those that are not enabled.

Note that this method was changed to "list_subscriptions" in azure-armrest 0.3.x, with the original method retained for backwards compatibility, though it will emit a deprecation warning.

Comment 10 Colin Arnott 2016-08-19 17:39:50 UTC
I follow your suggestion; would you be willing to provide a test patch with this simplified fix?

Comment 11 Daniel Berger 2016-08-24 14:28:18 UTC
Yes, we can provide a test patch.

Comment 12 Dave Johnson 2016-08-26 19:35:55 UTC
Is this a duplicate to bz 1318356 which talk to the gem supporting multiple subscriptions?

Comment 13 Bronagh Sorota 2016-08-30 14:23:32 UTC
Dave
No, this is not a duplicate. BZ 1318356  is a 5.5.0 ticket and calls for multiple subscription support to be added to CF. This BZ is a 5.6.0 ticket and is specific to discovery, it is also centered around a fix the customer implemented themselves which we will not integrate into the product.

Comment 14 Colin Arnott 2016-09-09 15:16:51 UTC
Is there any status update on the hotfix?

Comment 15 Bronagh Sorota 2016-09-09 15:20:29 UTC
Dan,
can you provide a hot fix for your recommendations?
Thanks
B

Comment 17 Daniel Berger 2016-09-09 15:49:02 UTC
I'm not sure how they should proceed since we decided that a subscription ID is now mandatory, and that we won't iterate over all subscriptions. If they want to match what the product officially supports then they should replace their custom modifications with the following two PR's:

https://github.com/ManageIQ/manageiq/pull/10531
https://github.com/ManageIQ/manageiq/pull/11116

They are relative small and easily integrated but significant.

As to why we did things this way, it was decided that performing discovery for every subscription could potentially hammer the application. Some customers have a lot of subscriptions, and the resulting number of spawned refreshes would be brutal.

That said, I think a separate RFE could be made that requests the option to perform discovery for all subscriptions.

Comment 18 Colin Arnott 2016-09-09 20:48:12 UTC
You are correct, in order to discover appliances for every subscription you have, you will need to manually enter all the subscriptions.

I agree an RFE is needed for this usecase. Would you like me to create it?

Comment 19 Colin Arnott 2016-09-09 20:51:53 UTC
It may be worthwhile to open a docs bug to clarify what a provider in Azure is defined as, because this was a point of confusion for at least me.

Let me know if you think the current docs are sufficient.

Comment 20 Daniel Berger 2016-09-09 20:53:30 UTC
Colin, yes please create a separate RFE.

Comment 21 Bronagh Sorota 2016-09-13 13:21:37 UTC
Colin,
Can this BZ be closed?

Thanks.
Bronagh

Comment 22 Bronagh Sorota 2016-09-13 17:57:42 UTC
Colin,
Can this BZ be closed?

Thanks.
Bronagh

Comment 23 Colin Arnott 2016-09-15 19:54:05 UTC
Yep, closing as this this is a dupe of the new rfe.

*** This bug has been marked as a duplicate of bug 1375285 ***


Note You need to log in before you can comment on or make changes to this bug.