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
Oracle Cloud Infrastructure (OCI) Provider #2367
Oracle Cloud Infrastructure (OCI) Provider #2367
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few questions related to maintainability going forward, but it looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this going to be updated frequently? Is there somewhere we could retrieve this if we needed to recreate/update it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's compiled from the cost estimator APIs, which are subject to change and could not be directly queried. When new shapes are added, this data may need to be recompiled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we document recompiling that data? I just worry that this will go stale and we won't know how to fix it.
pkg/cloud/oracle/product.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we could comment in here the source of the part numbers and how we're going to maintain them?
@@ -0,0 +1,39 @@ | |||
package oracle | |||
|
|||
var oracleRegions = []string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
source of the list?
I'm not very familiar with adding a provider, @AjayTripathy or @Sean-Holcomb, could either of you review this please? |
configs/oracle.json
Outdated
"storage": "0.00005479452", | ||
"defaultLBPrice": "0.0113", | ||
"internetNetworkEgress": "0.0085", | ||
"oraclePricingEndpoint":"https://apexapps.oracle.com/pls/apex/cetools/api/v1/products" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be configurable? When would you want to change it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple situations:
- You have custom pricing data you want to serve
- Your cluster is disconnected from the internet, or otherwise doesn't have access to this URI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does Oracle provide an endpoint with custom pricing in the same format or would the user set it up themselves, If it is Oracle provided is there authorization associated or would the expectation be that they are using a kind of workload identity auth?
For the air gapped example, is the thought that they would have an alternate endpoint available on a vpn or it would just be disable?
As an explanation, I am hammering on this point because that config file is a bit of a mess and I don't want to increase dependencies on it. In the short term I would adding that URL as a const
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Oracle has custom pricing, which requires authentication to access, as it is from your tenancy. Until this integration is done, custom pricing would have to be from a user-supplied URI.
- For scenarios where the default URI is not accessible, users could provide their own endpoint.
Couple thoughts on the config file:
If the config file is too large, what would another means of providing external configurations?
OR, If there's another parameter we can reuse, that might also work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anders-swanson sorry for the slow responses here, we've been on a holiday schedule:)
Many providers have huge config files-- we tokenize them and scan them to extract pricing for just the resources you're running. We should definitely not block this PR on that, though. Would be a great todo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Ajay,
I can move this to an envrionment variable. Is there any concern with overloading environment variables with config values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overloaded env variables with default is preferable to this config file having additional fields added to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good! It may be some time until I can implement the update due to holiday break, but know that it will be done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @anders-swanson checking back in. Still on your radar? I'm hopeful this should be really quick!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, sorry for the delay, I've just returned from winter holidays. I plan to push out these changes tomorrow 🤞
@anders-swanson OpenCost 1.108 is shipping with Cloud Costs, do you want (in a separate PR) to add Oracle support for this too? Once @Sean-Holcomb or @AjayTripathy or one of the other @opencost/opencost-maintainers signs off on a review we'll merge for the next release |
This basically LGTM @anders-swanson -- just see my note about swapping the configuration of the pricing information to an environment variable |
I may be able to integrate cloud costs in a follow-up PR. |
Yep, happy to not block on that! |
@anders-swanson you mentioned in Slack you're still working on getting approval on this. In the meantime could you get the DCO added to your commits? |
Signed-off-by: Anders Swanson <anders.swanson@oracle.com>
Signed-off-by: Anders Swanson <anders.swanson@oracle.com>
cc26d49
to
49bedf0
Compare
Rebased for DCO, thanks! |
pkg/cloud/oracle/provider.go
Outdated
return err | ||
} | ||
if o.RateCardStore == nil { | ||
url := os.Getenv("OCI_PRICING_URL") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you instead do this in https://github.com/opencost/opencost/blob/develop/pkg/env/costmodelenv.go and supply a default for pricing data that would work for most users?
One quick note @anders-swanson then we should be good. |
Signed-off-by: Anders Swanson <anders.swanson@oracle.com>
Quality Gate passedThe SonarCloud Quality Gate passed, but some issues were introduced. 3 New issues |
Oracle Cloud Infrastructure (OCI) provider
What does this PR change?
Does this PR relate to any other PRs?
How will this PR impact users?
Does this PR address any GitHub or Zendesk issues?
How was this PR tested?
Does this PR require changes to documentation?
Oracle Cloud Infrastructure provider opencost-website#178
Have you labeled this PR and its corresponding Issue as "next release" if it should be part of the next OpenCost release? If not, why not?