GTM-52 Fix parsing of GCP pricing and create new node diagnostic pricing API #2083

nikovacevic · 2023-08-11T18:45:55Z

What does this PR change?

Added support for GCP T2D instance types.
Fixes an issue where GCP pricing would fail if there were two nodes of the same family (e.g. e2-small, e2-standard) present.
Adds a new Node Pricing Diagnostic API for detecting issues with node pricing. (This is only supported on GCP to start.)

Does this PR relate to any other PRs?

Required for https://github.com/kubecost/kubecost-cost-model/pull/1694

How will this PR impact users?

These specific node pricing problems will be resolved.

Does this PR address any GitHub or Zendesk issues?

Closes https://kubecost.atlassian.net/browse/GTM-52

How was this PR tested?

Unit tests
Manually, using the new diagnostic API

Here's evidence of the node type at issue, working for currency IDR:

Does this PR require changes to documentation?

No need to document this API just yet. It's internal only.

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?

Yes

nikovacevic · 2023-08-11T22:41:34Z

pkg/cloud/gcp/provider.go

+
+				if (instanceType == "ram" || instanceType == "cpu") && strings.Contains(strings.ToUpper(product.Description), "T2D AMD") {
+					instanceType = "t2dstandard"
+				}
+				if (instanceType == "ram" || instanceType == "cpu") && strings.Contains(strings.ToUpper(product.Description), "T2A ARM") {
+					instanceType = "t2astandard"
+				}
+


This fixes the proximate user issue.

nikovacevic · 2023-08-11T22:42:06Z

pkg/cloud/gcp/provider.go

+							if len(product.PricingInfo) > 0 {
+								lastRateIndex := len(product.PricingInfo[0].PricingExpression.TieredRates) - 1
+								if lastRateIndex >= 0 {
+									nanos = product.PricingInfo[0].PricingExpression.TieredRates[lastRateIndex].UnitPrice.Nanos
+									unitsBaseCurrency, err = strconv.Atoi(product.PricingInfo[0].PricingExpression.TieredRates[lastRateIndex].UnitPrice.Units)
+									if err != nil {
+										return nil, "", fmt.Errorf("error parsing base unit price for instance: %w", err)
+									}
+								} else {
+									continue


This fixes some panics. (We used to check the length of the slice after we'd already accessed [0] haha.)

nikovacevic · 2023-08-11T22:42:41Z

pkg/cloud/gcp/provider.go

 								}
-								break


Removing this, and the other break, allows pricing to work for multiple instance types from the same family.

nikovacevic · 2023-08-11T22:42:46Z

pkg/cloud/gcp/provider.go

 								}
-								break


Removing this, and the other break, allows pricing to work for multiple instance types from the same family.

nikovacevic · 2023-08-11T22:43:43Z

pkg/cloud/gcp/provider.go

-									product = &GCPPricing{}
-									product.Node = &models.Node{
+									log.Debugf("GCP Billing API: key '%s': RAM price: %f", candidateKey, hourlyPrice)
+									pricing := &GCPPricing{}


Setting this to a new variable (i.e. pricing := not product =) prevents nil issues and other failures when the original product data needs to be reused later on.

nikovacevic · 2023-08-11T22:43:52Z

pkg/cloud/gcp/provider.go

-									product = &GCPPricing{}
-									product.Node = &models.Node{
+									log.Debugf("GCP Billing API: key '%s': RAM price: %f", candidateKeyGPU, hourlyPrice)
+									pricing := &GCPPricing{}


Setting this to a new variable (i.e. pricing := not product =) prevents nil issues and other failures when the original product data needs to be reused later on.

nikovacevic · 2023-08-11T22:43:58Z

pkg/cloud/gcp/provider.go

-									product = &GCPPricing{}
-									product.Node = &models.Node{
+									log.Debugf("GCP Billing API: key '%s': CPU price: %f", candidateKey, hourlyPrice)
+									pricing := &GCPPricing{}


Setting this to a new variable (i.e. pricing := not product =) prevents nil issues and other failures when the original product data needs to be reused later on.

nikovacevic · 2023-08-11T22:44:03Z

pkg/cloud/gcp/provider.go

-									product = &GCPPricing{}
-									product.Node = &models.Node{
+									log.Debugf("GCP Billing API: key '%s': CPU price: %f", candidateKeyGPU, hourlyPrice)
+									pricing := &GCPPricing{}


Setting this to a new variable (i.e. pricing := not product =) prevents nil issues and other failures when the original product data needs to be reused later on.

brutal. so this is your mutated loop var issue

Yep. Seems obvious now, but was not fun to find.

nikovacevic · 2023-08-11T22:46:43Z

pkg/cloud/gcp/provider.go

 								}
 								if _, ok := gcpPricingList[candidateKey]; ok {
+									log.Debugf("GCP Billing API: key '%s': RAM price: %f", candidateKey, hourlyPrice)


Unified these logs so that they're more legible and clearly related to each other:

{"level":"debug","time":"2023-08-11T16:46:04-06:00","message":"GCP Billing API: key 'us-central1,a2highgpu,ondemand': RAM price: 0.004237"} {"level":"debug","time":"2023-08-11T16:46:04-06:00","message":"GCP Billing API: key 'us-central1,a2highgpu,ondemand,gpu': RAM price: 0.004237"} {"level":"debug","time":"2023-08-11T16:46:04-06:00","message":"GCP Billing API: key 'us-central1,a2highgpu,ondemand': CPU price: 0.031611"} {"level":"debug","time":"2023-08-11T16:46:04-06:00","message":"GCP Billing API: key 'us-central1,a2highgpu,ondemand,gpu': CPU price: 0.031611"} {"level":"debug","time":"2023-08-11T16:46:04-06:00","message":"GCP Billing API: key 'asia-southeast1,t2dstandard,ondemand': RAM price: 68.205000"} {"level":"debug","time":"2023-08-11T16:46:04-06:00","message":"GCP Billing API: key 'asia-southeast1,t2dstandard,ondemand,gpu': RAM price: 68.205000"} {"level":"debug","time":"2023-08-11T16:46:04-06:00","message":"GCP Billing API: key 'asia-southeast1,t2dstandard,ondemand': CPU price: 508.934997"} {"level":"debug","time":"2023-08-11T16:46:04-06:00","message":"GCP Billing API: key 'asia-southeast1,t2dstandard,ondemand,gpu': CPU price: 508.934997"} {"level":"debug","time":"2023-08-11T16:46:04-06:00","message":"GCP Billing API: key 'us-central1,e2medium,ondemand': CPU price: 327.173848"} {"level":"debug","time":"2023-08-11T16:46:04-06:00","message":"GCP Billing API: key 'us-central1,e2medium,ondemand,gpu': CPU price: 327.173848"} {"level":"debug","time":"2023-08-11T16:46:04-06:00","message":"GCP Billing API: key 'us-central1,e2standard,ondemand': CPU price: 327.173848"} {"level":"debug","time":"2023-08-11T16:46:04-06:00","message":"GCP Billing API: key 'us-central1,e2standard,ondemand,gpu': CPU price: 327.173848"} {"level":"debug","time":"2023-08-11T16:46:04-06:00","message":"GCP Billing API: key 'us-central1,e2medium,ondemand': RAM price: 43.852950"} {"level":"debug","time":"2023-08-11T16:46:04-06:00","message":"GCP Billing API: key 'us-central1,e2medium,ondemand,gpu': RAM price: 43.852950"} {"level":"debug","time":"2023-08-11T16:46:04-06:00","message":"GCP Billing API: key 'us-central1,e2standard,ondemand': RAM price: 43.852950"} {"level":"debug","time":"2023-08-11T16:46:04-06:00","message":"GCP Billing API: key 'us-central1,e2standard,ondemand,gpu': RAM price: 43.852950"}

nikovacevic · 2023-08-11T22:47:21Z

pkg/cloud/gcp/provider.go

@@ -1539,25 +1552,55 @@ func (gcp *GCP) isValidPricingKey(key models.Key) bool {
 }

 // NodePricing returns GCP pricing data for a single node
-func (gcp *GCP) NodePricing(key models.Key) (*models.Node, error) {
+func (gcp *GCP) NodePricing(key models.Key) (*models.Node, models.PricingMetadata, error) {
+	meta := models.PricingMetadata{}


This function is not materially changed. It just has extra lines adding in the metadata for the new diagnostics API.

nikovacevic · 2023-08-11T22:49:23Z

pkg/cloud/gcp/provider_test.go

-// Load a reader object on a portion of a GCP api response
-// Confirm that the resting *GCP object contains the correctly parsed pricing info
-func TestParsePage(t *testing.T) {
+func TestKeyFeatures(t *testing.T) {


This is a new test.

nikovacevic · 2023-08-11T22:51:14Z

pkg/cloud/gcp/provider_test.go

+// tests basic parsing of GCP pricing API responses
+// Load a reader object on a portion of a GCP api response
+// Confirm that the resting *GCP object contains the correctly parsed pricing info
+func TestParsePage(t *testing.T) {


This test has been updated to add more SKUs, and the SKUs themselves were moved to a testing file.

nikovacevic · 2023-08-11T22:51:51Z

pkg/cloud/models/models.go

@@ -273,7 +273,7 @@ type Provider interface {
 	GetAddresses() ([]byte, error)
 	GetDisks() ([]byte, error)
 	GetOrphanedResources() ([]OrphanedResource, error)
-	NodePricing(Key) (*Node, error)
+	NodePricing(Key) (*Node, PricingMetadata, error)


This change just allows us to use the very same function for getting pricing, and reporting metadata about where that data came from.

ameijer · 2023-08-14T13:24:53Z

pkg/cloud/gcp/provider.go

-									product = &GCPPricing{}
-									product.Node = &models.Node{
+									log.Debugf("GCP Billing API: key '%s': CPU price: %f", candidateKeyGPU, hourlyPrice)
+									pricing := &GCPPricing{}


brutal. so this is your mutated loop var issue

pkg/cloud/gcp/provider.go

…ing API Signed-off-by: Niko Kovacevic <nikovacevic@gmail.com>

michaelmdresser

Trusting Alex's review

nikovacevic force-pushed the niko/GTM-52/pricing branch 2 times, most recently from 02f7a10 to 214ebb0 Compare August 11, 2023 22:25

nikovacevic added the v1.106 label Aug 11, 2023

nikovacevic marked this pull request as ready for review August 11, 2023 22:38

nikovacevic force-pushed the niko/GTM-52/pricing branch from 214ebb0 to 933b097 Compare August 11, 2023 22:39

nikovacevic commented Aug 11, 2023

View reviewed changes

nikovacevic force-pushed the niko/GTM-52/pricing branch from 933b097 to efa7cae Compare August 11, 2023 22:44

nikovacevic commented Aug 11, 2023

View reviewed changes

nikovacevic force-pushed the niko/GTM-52/pricing branch from efa7cae to d00b7dc Compare August 11, 2023 22:48

nikovacevic commented Aug 11, 2023

View reviewed changes

nikovacevic added the needs cherry pick label Aug 11, 2023

nikovacevic requested a review from thomasvn August 11, 2023 22:59

ameijer approved these changes Aug 14, 2023

View reviewed changes

GTM-52 Fix parsing of GCP pricing and create new node diagnostic pric…

42976a2

…ing API Signed-off-by: Niko Kovacevic <nikovacevic@gmail.com>

nikovacevic force-pushed the niko/GTM-52/pricing branch from d00b7dc to 42976a2 Compare August 14, 2023 19:27

ameijer approved these changes Aug 14, 2023

View reviewed changes

nikovacevic requested a review from teevans August 14, 2023 22:12

michaelmdresser approved these changes Aug 14, 2023

View reviewed changes

nikovacevic merged commit 820f962 into develop Aug 14, 2023
3 checks passed

nikovacevic deleted the niko/GTM-52/pricing branch August 14, 2023 22:35

nikovacevic mentioned this pull request Aug 14, 2023

Cherry pick: GTM-52 Fix parsing of GCP pricing and create new node diagnostic pricing API #2088

Merged

nikovacevic removed the needs cherry pick label Aug 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GTM-52 Fix parsing of GCP pricing and create new node diagnostic pricing API #2083

GTM-52 Fix parsing of GCP pricing and create new node diagnostic pricing API #2083

nikovacevic commented Aug 11, 2023 •

edited

nikovacevic Aug 11, 2023

nikovacevic Aug 11, 2023

nikovacevic Aug 11, 2023

nikovacevic Aug 11, 2023

nikovacevic Aug 11, 2023

nikovacevic Aug 11, 2023

nikovacevic Aug 11, 2023

nikovacevic Aug 11, 2023

ameijer Aug 14, 2023

nikovacevic Aug 14, 2023

nikovacevic Aug 11, 2023

nikovacevic Aug 11, 2023

nikovacevic Aug 11, 2023

nikovacevic Aug 11, 2023

nikovacevic Aug 11, 2023

ameijer Aug 14, 2023

michaelmdresser left a comment

GTM-52 Fix parsing of GCP pricing and create new node diagnostic pricing API #2083

GTM-52 Fix parsing of GCP pricing and create new node diagnostic pricing API #2083

Conversation

nikovacevic commented Aug 11, 2023 • edited

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?

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?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelmdresser left a comment

Choose a reason for hiding this comment

nikovacevic commented Aug 11, 2023 •

edited