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
Created attachment 1190853 [details] cloud-discovery-without-patches.png
Created attachment 1190981 [details] manager_mixin.rb
Created attachment 1190982 [details] cloud-discover-with-patches.png
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
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
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.
I follow your suggestion; would you be willing to provide a test patch with this simplified fix?
Yes, we can provide a test patch.
Is this a duplicate to bz 1318356 which talk to the gem supporting multiple subscriptions?
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.
Is there any status update on the hotfix?
Dan, can you provide a hot fix for your recommendations? Thanks B
Added Polarion Testcase https://polarion.engineering.redhat.com/polarion/#/project/RHCF3/workitem?id=RHCF3-11863
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.
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?
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.
Colin, yes please create a separate RFE.
Colin, Can this BZ be closed? Thanks. Bronagh
Yep, closing as this this is a dupe of the new rfe. *** This bug has been marked as a duplicate of bug 1375285 ***