Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gather AWS instances details from AWS's Price List API #397

Open
AlexanderZagaynov opened this issue Jan 17, 2018 · 20 comments
Open

Gather AWS instances details from AWS's Price List API #397

AlexanderZagaynov opened this issue Jan 17, 2018 · 20 comments

Comments

@AlexanderZagaynov
Copy link
Contributor

AlexanderZagaynov commented Jan 17, 2018

Instances details are currently hard-coded in the https://github.com/ManageIQ/manageiq-providers-amazon/blob/master/app/models/manageiq/providers/amazon/instance_types.rb#L12 file.

We can get them (either dynamically or at build time) following the Amazon's Price List API instruction: https://docs.aws.amazon.com/awsaccountbilling/latest/aboutv2/using-ppslong.html

P.S. It can be useful also to use price list changes notifications: https://docs.aws.amazon.com/awsaccountbilling/latest/aboutv2/price-notification.html

@AlexanderZagaynov
Copy link
Contributor Author

Related issue: #302

@djberg96
Copy link
Contributor

djberg96 commented Jun 7, 2018

@AlexanderZagaynov Seems it's partially done. What's the current status? Also, the two doc links appear to be dead.

@djberg96
Copy link
Contributor

@AlexanderZagaynov FYI in version 3 of the AWS SDK for Ruby, there's an Aws::Pricing::Client you can use that will simplify your life.

@AlexanderZagaynov
Copy link
Contributor Author

@djberg96 @Ladas it doesn't makes sense:

  • it requires credentials and IAM role setup
  • it returns the same data, as from those JSON files, but filtered (and we don't need any filters there)

https://docs.aws.amazon.com/awsaccountbilling/latest/aboutv2/using-pelong.html

@bronaghs
Copy link

@AlexanderZagaynov - Why do we not need filters? If we filter on "Compute Instance" we do not have to pull a ton of data we otherwise would have to pull.

I was able to get the product information without modifying my AWS credentials because I have admin access. We need to find out what roles users are given access to today when setting up the ManageIQ user in AWS. Enabling an IAM role should not stop us from using the Aws::Pricing:: Client module.

@Ladas
Copy link
Contributor

Ladas commented Jul 19, 2018

@AlexanderZagaynov IAM role is ok, since we already require like 10-20 roles (if you don't have admin)

If it's returning the same data, I would rather go with the API and refresh approach, since then it just stays fresh for the customer automatically. While now we need to release for every new flavor, or customer needs to type it in manually to settings. So the experience with refresh is always better.

@AlexanderZagaynov
Copy link
Contributor Author

@Ladas the thing was just to replace one parser to another, and not define values in the code file, but in the yaml instead. What you asking about - is to change entire approach, which is definitely out of the current PR's scope.

@agrare
Copy link
Member

agrare commented Jul 19, 2018

@AlexanderZagaynov if the current approach is working well enough and we might be able to get the flavors from the API (which we weren't able to do before) I agree with @Ladas getting flavors from the API would be preferable even if it is out of the scope of this PR.

@bronaghs
Copy link

hi All
FYI Alex and I discussed this offline. He is going to make a new PR using the Aws::Pricing::Client module where he will process instance types like we process all other inventory types.

@AlexanderZagaynov
Copy link
Contributor Author

@agrare @bronaghs (cc @Fryguy) we discussed it with @Ladas right now, and make conclusion that there should be at least two PRs within another BZ/issue:

  • upgrade aws-sdk gem to v3
  • refactor Inventory::Parser to not use hardcoded instance types (flavors), but fetch them dynamically during the regular refresh

I'm going to simplify my current PR to just move hardcoded constants to the yaml file, and update it with a new flavors. It should be enough for now, considering further changes.

@bronaghs
Copy link

@AlexanderZagaynov - why do we still need your current PR if we use the AWS SDK?

@djberg96
Copy link
Contributor

Only reason would be that we need something until we actually upgrade to v3. Unless we're going to require v3 specifically for the rake task, but that might get confusing. But, I'll leave that up to y'all to resolve.

@bronaghs
Copy link

I think this should be the order:

  1. Prove out in a prototype that we can get what we need from the Aws::Pricing:: Client module.
  2. Upgrade the AWS provider to use the Aws-Sdk V3 ( we need this anyway since V2 was released in 2015 and we should not fall so far behind)
  3. Update the AWS Refresh code to gather instance types like we gather other inventory types
  4. If any special configuration is needed on the IAM roles then consider a documentation update.

@Ladas
Copy link
Contributor

Ladas commented Jul 20, 2018

@bronaghs updating the current yaml file would make sense, if we would be backporting it farther than the solution using the client v3? So, what are the backport plans? :-)

@Ladas
Copy link
Contributor

Ladas commented Nov 29, 2018

@AlexanderZagaynov @djberg96 @bronaghs FYI, in topology inventory I am using the query RedHatInsights/topological_inventory-amazon@9160963#diff-937a6d3aaf6ea0718b23227a3e3a2a6fR27

Since the JSON has over 100k flavor objects in it, i'd be huge in memory and we'd need to fetch 500MB or so. With a query, I can filter it down to 164 unique instance types now. So definitely +1 for the query approach as @djberg96 said

@miq-bot
Copy link
Member

miq-bot commented Jun 3, 2019

This issue has been automatically marked as stale because it has not been updated for at least 6 months.

If you can still reproduce this issue on the current release or on master, please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions!

@miq-bot miq-bot added the stale label Jun 3, 2019
@djberg96
Copy link
Contributor

@AlexanderZagaynov, @Ladas Now that we've upgraded to v3, what's the word?

@Ladas
Copy link
Contributor

Ladas commented Nov 22, 2019

@djberg96 topological inventory is using the API for a while and it works ok (filtering out just what we need)

@miq-bot
Copy link
Member

miq-bot commented Jun 11, 2020

This issue has been automatically marked as stale because it has not been updated for at least 3 months.

If you can still reproduce this issue on the current release or on master, please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the traige process documentation.

@miq-bot miq-bot added the stale label Jun 11, 2020
@gtanzillo gtanzillo removed the stale label Jun 29, 2020
@djberg96
Copy link
Contributor

djberg96 commented Jul 1, 2020

#614

@djberg96 djberg96 removed their assignment Jan 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants