ENG-1706 Fix parsing of AWS OnDemand data for certain instance types (#2765)

* AWS OnDemand Pricing. Only accept lines where marketOption=OnDemand

Signed-off-by: thomasvn <thomasnguyen96@gmail.com>

* Add in debug logging.

Signed-off-by: thomasvn <thomasnguyen96@gmail.com>

* Progress on testing.

Signed-off-by: thomasvn <thomasnguyen96@gmail.com>

* Added regression test for AWS OnDemand Pricing schema.

Signed-off-by: thomasvn <thomasnguyen96@gmail.com>

* Cleanup config used during development.

Signed-off-by: thomasvn <thomasnguyen96@gmail.com>

* Add new concept of "Integration" test to Justfile

Signed-off-by: thomasvn <thomasnguyen96@gmail.com>

---------

Signed-off-by: thomasvn <thomasnguyen96@gmail.com>

justfile

@@ -11,12 +11,15 @@ test-core:
     {{commonenv}} cd ./core && go test ./... -coverprofile=coverage.out
     {{commonenv}} cd ./core && go vet ./...
 
-
 # Run unit tests
 test: test-core
     {{commonenv}} go test ./... -coverprofile=coverage.out
     {{commonenv}} go vet ./...
 
+# Run unit tests and integration tests
+test-integration:
+    {{commonenv}} INTEGRATION=true go test ./... -coverprofile=coverage.out
+
 # Compile a local binary
 build-local:
     cd ./cmd/costmodel && \

pkg/cloud/aws/provider.go

@@ -259,6 +259,7 @@ type AWSProductAttributes struct {
 	InstanceFamily  string `json:"instanceFamily"`
 	CapacityStatus  string `json:"capacitystatus"`
 	GPU             string `json:"gpu"` // GPU represents the number of GPU on the instance
+	MarketOption    string `json:"marketOption"`
 }
 
 // AWSPricingTerms are how you pay for the node: OnDemand, Reserved, or (TODO) Spot
@@ -999,7 +1000,8 @@ func (aws *AWS) populatePricing(resp *http.Response, inputkeys map[string]bool)
 
 				if product.Attributes.PreInstalledSw == "NA" &&
 					(strings.HasPrefix(product.Attributes.UsageType, "BoxUsage") || strings.Contains(product.Attributes.UsageType, "-BoxUsage")) &&
-					product.Attributes.CapacityStatus == "Used" {
+					product.Attributes.CapacityStatus == "Used" &&
+					product.Attributes.MarketOption == "OnDemand" {
 					key := aws.KubeAttrConversion(product.Attributes.RegionCode, product.Attributes.InstanceType, product.Attributes.OperatingSystem)
 					spotKey := key + ",preemptible"
 					if inputkeys[key] || inputkeys[spotKey] { // Just grab the sku even if spot, and change the price later.
@@ -1352,7 +1354,6 @@ func (aws *AWS) createNode(terms *AWSProductTerms, usageType string, k models.Ke
 	}
 	// Throw error if public price is not found
 	if !publicPricingFound {
-		log.Errorf("For node \"%s\", cannot find the following key in OnDemand pricing data \"%s\"", k.ID(), k.Features())
 		return nil, meta, fmt.Errorf("for node \"%s\", cannot find the following key in OnDemand pricing data \"%s\"", k.ID(), k.Features())
 	}
 
@@ -1383,6 +1384,9 @@ func (aws *AWS) NodePricing(k models.Key) (*models.Node, models.PricingMetadata,
 	meta := models.PricingMetadata{}
 
 	terms, ok := aws.Pricing[key]
+	if termsStr, err := json.Marshal(terms); err == nil {
+		log.Debugf("NodePricing: for key \"%s\" found the following OnDemand data: %s", key, string(termsStr))
+	}
 	if ok {
 		return aws.createNode(terms, usageType, k)
 	} else if _, ok := aws.ValidPricingKeys[key]; ok {

pkg/cloud/aws/provider_test.go

@@ -2,9 +2,11 @@ package aws
 
 import (
 	"bytes"
+	"encoding/json"
 	"io"
 	"net/http"
 	"net/url"
+	"os"
 	"reflect"
 	"testing"
 
@@ -102,17 +104,73 @@ func Test_awsKey_getUsageType(t *testing.T) {
 	}
 }
 
+// Test_PricingData_Regression
+//
+// Objective: To test the pricing data download and validate the schema is still
+// as expected
+//
+// These tests may take a long time to complete. It is downloading AWS Pricing
+// data files (~500MB) for each region.
+func Test_PricingData_Regression(t *testing.T) {
+	if os.Getenv("INTEGRATION") == "" {
+		t.Skip("skipping integration tests, set environment variable INTEGRATION")
+	}
+
+	awsRegions := []string{"us-east-1", "eu-west-1"}
+
+	// Check pricing data produced for each region
+	for _, region := range awsRegions {
+		node := v1.Node{}
+		node.SetLabels(map[string]string{"topology.kubernetes.io/region": region})
+
+		awsTest := AWS{}
+		res, _, err := awsTest.getRegionPricing([]*v1.Node{&node})
+		if err != nil {
+			t.Errorf("Failed to download pricing data for region %s: %v", region, err)
+		}
+
+		// Unmarshal pricing data into AWSPricing
+		var pricingData AWSPricing
+		body, err := io.ReadAll(res.Body)
+		if err != nil {
+			t.Errorf("Failed to read pricing data for region %s: %v", region, err)
+		}
+		err = json.Unmarshal(body, &pricingData)
+		if err != nil {
+			t.Errorf("Failed to unmarshal pricing data for region %s: %v", region, err)
+		}
+
+		// ASSERTION. We only anticipate "OnDemand" or "CapacityBlock" in the
+		// pricing data.
+		//
+		// Failing this test does not necessarily mean we have regressed. Just
+		// that we need to revisit this code to ensure OnDemand pricing is still
+		// functioning as expected.
+		for _, product := range pricingData.Products {
+			if product.Attributes.MarketOption != "OnDemand" && product.Attributes.MarketOption != "CapacityBlock" && product.Attributes.MarketOption != "" {
+				t.Errorf("Invalid marketOption for product %s: %s", product.Sku, product.Attributes.MarketOption)
+			}
+		}
+	}
+}
+
 // Test_populate_pricing
 //
 // Objective: To test core pricing population logic for AWS
 //
-//	Case 0: US endpoints
-//	 Take a portion of json returned from ondemand terms in us endpoints
-//	 load the request into the http response and give it to the function
-//	 inspect the resulting aws object after the function returns and validate fields
-//	Case 1: Chinese endpoints
-//	 Same as above US test case, except using CN PV offer codes
-//	 Validate populated fields in AWS object
+// Case 0: US endpoints
+// Take a portion of json returned from ondemand terms in us endpoints load the
+// request into the http response and give it to the function inspect the
+// resulting aws object after the function returns and validate fields
+//
+// Case 1: Ensure marketOption=OnDemand
+// AWS introduced the field marketOption. We need to further filter for
+// marketOption=OnDemand to ensure we are not getting pricing from a line item
+// such as marketOption=CapacityBlock
+//
+// Case 2: Chinese endpoints
+// Same as above US test case, except using CN PV offer codes. Validate
+// populated fields in AWS object
 func Test_populate_pricing(t *testing.T) {
 	awsTest := AWS{
 		ValidPricingKeys: map[string]bool{},
@@ -230,7 +288,7 @@ func Test_populate_pricing(t *testing.T) {
 				  "servicename" : "Amazon Elastic Compute Cloud",
 				  "volumeApiName" : "gp3"
 				}
-			  }
+			}
 		},
 		"terms" : {
 			"OnDemand" : {
@@ -383,7 +441,199 @@ func Test_populate_pricing(t *testing.T) {
 		t.Fatalf("expected parsed pricing did not match actual parsed result (us-east-1)")
 	}
 
-	// Case 1
+	// Case 1 - Only accept `"marketoption":"OnDemand"`
+	inputkeysCase1 := map[string]bool{
+		"us-east-1,p4d.24xlarge,linux": true,
+	}
+	pricingCase1 := `
+	{
+		"formatVersion" : "v1.0",
+		"disclaimer" : "This pricing list is for informational purposes only. All prices are subject to the additional terms included in the pricing pages on http://aws.amazon.com. All Free Tier prices are also subject to the terms included at https://aws.amazon.com/free/",
+		"offerCode" : "AmazonEC2",
+		"version" : "20240528203522",
+		"publicationDate" : "2024-05-28T20:35:22Z",
+		"products" : {
+			"H7NGEAC6UEHNTKSJ" : {
+				"sku" : "H7NGEAC6UEHNTKSJ",
+				"productFamily" : "Compute Instance",
+				"attributes" : {
+					"servicecode" : "AmazonEC2",
+					"location" : "US East (N. Virginia)",
+					"locationType" : "AWS Region",
+					"instanceType" : "p4d.24xlarge",
+					"currentGeneration" : "Yes",
+					"instanceFamily" : "GPU instance",
+					"vcpu" : "96",
+					"physicalProcessor" : "Intel Xeon Platinum 8275L",
+					"clockSpeed" : "3 GHz",
+					"memory" : "1152 GiB",
+					"storage" : "8 x 1000 SSD",
+					"networkPerformance" : "400 Gigabit",
+					"processorArchitecture" : "64-bit",
+					"tenancy" : "Shared",
+					"operatingSystem" : "Linux",
+					"licenseModel" : "No License required",
+					"usagetype" : "BoxUsage:p4d.24xlarge",
+					"operation" : "RunInstances",
+					"availabilityzone" : "NA",
+					"capacitystatus" : "Used",
+					"classicnetworkingsupport" : "false",
+					"dedicatedEbsThroughput" : "19000 Mbps",
+					"ecu" : "345",
+					"enhancedNetworkingSupported" : "No",
+					"gpu" : "8",
+					"gpuMemory" : "NA",
+					"intelAvxAvailable" : "Yes",
+					"intelAvx2Available" : "Yes",
+					"intelTurboAvailable" : "Yes",
+					"marketoption" : "OnDemand",
+					"normalizationSizeFactor" : "192",
+					"preInstalledSw" : "NA",
+					"processorFeatures" : "Intel AVX; Intel AVX2; Intel AVX512; Intel Turbo",
+					"regionCode" : "us-east-1",
+					"servicename" : "Amazon Elastic Compute Cloud",
+					"vpcnetworkingsupport" : "true"
+				}
+			},
+			"YSXJGN78QTXNVGDQ" : {
+				"sku" : "YSXJGN78QTXNVGDQ",
+				"productFamily" : "Compute Instance",
+				"attributes" : {
+					"servicecode" : "AmazonEC2",
+					"location" : "US East (N. Virginia)",
+					"locationType" : "AWS Region",
+					"instanceType" : "p4d.24xlarge",
+					"currentGeneration" : "Yes",
+					"instanceFamily" : "GPU instance",
+					"vcpu" : "96",
+					"physicalProcessor" : "Intel Xeon Platinum 8275L",
+					"clockSpeed" : "3 GHz",
+					"memory" : "1152 GiB",
+					"storage" : "8 x 1000 SSD",
+					"networkPerformance" : "400 Gigabit",
+					"processorArchitecture" : "64-bit",
+					"tenancy" : "Shared",
+					"operatingSystem" : "Linux",
+					"licenseModel" : "No License required",
+					"usagetype" : "BoxUsage:p4d.24xlarge",
+					"operation" : "RunInstances:CB",
+					"availabilityzone" : "NA",
+					"capacitystatus" : "Used",
+					"classicnetworkingsupport" : "false",
+					"dedicatedEbsThroughput" : "19000 Mbps",
+					"ecu" : "345",
+					"enhancedNetworkingSupported" : "No",
+					"gpu" : "8",
+					"gpuMemory" : "NA",
+					"intelAvxAvailable" : "Yes",
+					"intelAvx2Available" : "Yes",
+					"intelTurboAvailable" : "Yes",
+					"marketoption" : "CapacityBlock",
+					"normalizationSizeFactor" : "192",
+					"preInstalledSw" : "NA",
+					"processorFeatures" : "Intel AVX; Intel AVX2; Intel AVX512; Intel Turbo",
+					"regionCode" : "us-east-1",
+					"servicename" : "Amazon Elastic Compute Cloud",
+					"vpcnetworkingsupport" : "true"
+				}
+			}
+		},
+		"terms" : {
+			"OnDemand" : {
+				"H7NGEAC6UEHNTKSJ" : {
+					"H7NGEAC6UEHNTKSJ.JRTCKXETXF" : {
+						"offerTermCode" : "JRTCKXETXF",
+						"sku" : "H7NGEAC6UEHNTKSJ",
+						"effectiveDate" : "2024-05-01T00:00:00Z",
+						"priceDimensions" : {
+							"H7NGEAC6UEHNTKSJ.JRTCKXETXF.6YS6EN2CT7" : {
+								"rateCode" : "H7NGEAC6UEHNTKSJ.JRTCKXETXF.6YS6EN2CT7",
+								"description" : "$32.7726 per On Demand Linux p4d.24xlarge Instance Hour",
+								"beginRange" : "0",
+								"endRange" : "Inf",
+								"unit" : "Hrs",
+								"pricePerUnit" : {
+									"USD" : "32.7726000000"
+								},
+								"appliesTo" : [ ]
+							}
+						},
+						"termAttributes" : { }
+					}
+				},
+				"YSXJGN78QTXNVGDQ" : {
+					"YSXJGN78QTXNVGDQ.JRTCKXETXF" : {
+						"offerTermCode" : "JRTCKXETXF",
+						"sku" : "YSXJGN78QTXNVGDQ",
+						"effectiveDate" : "2024-05-01T00:00:00Z",
+						"priceDimensions" : {
+							"YSXJGN78QTXNVGDQ.JRTCKXETXF.6YS6EN2CT7" : {
+							"rateCode" : "YSXJGN78QTXNVGDQ.JRTCKXETXF.6YS6EN2CT7",
+							"description" : "$0.00 per Capacity Block Linux p4d.24xlarge Instance Hour",
+							"beginRange" : "0",
+							"endRange" : "Inf",
+							"unit" : "Hrs",
+							"pricePerUnit" : {
+								"USD" : "0.0000000000"
+							},
+							"appliesTo" : [ ]
+						}
+					},
+					"termAttributes" : { }
+					}
+				},
+			}
+		},
+		"attributesList" : { }
+	}
+	`
+
+	testResponseCase1 := http.Response{
+		Body: io.NopCloser(bytes.NewBufferString(pricingCase1)),
+		Request: &http.Request{
+			URL: &url.URL{
+				Scheme: "https",
+				Host:   "test-aws-http-endpoint:443",
+			},
+		},
+	}
+
+	awsTest.populatePricing(&testResponseCase1, inputkeysCase1)
+
+	expectedProdTermsInstanceOndemandCase1 := &AWSProductTerms{
+		Sku:     "H7NGEAC6UEHNTKSJ",
+		Memory:  "1152 GiB",
+		Storage: "8 x 1000 SSD",
+		VCpu:    "96",
+		GPU:     "8",
+		OnDemand: &AWSOfferTerm{
+			Sku:           "H7NGEAC6UEHNTKSJ",
+			OfferTermCode: "JRTCKXETXF",
+			PriceDimensions: map[string]*AWSRateCode{
+				"H7NGEAC6UEHNTKSJ.JRTCKXETXF.6YS6EN2CT7": {
+					Unit: "Hrs",
+					PricePerUnit: AWSCurrencyCode{
+						USD: "32.7726000000",
+					},
+				},
+			},
+		},
+	}
+
+	expectedPricingCase1 := map[string]*AWSProductTerms{
+		"us-east-1,p4d.24xlarge,linux":             expectedProdTermsInstanceOndemandCase1,
+		"us-east-1,p4d.24xlarge,linux,preemptible": expectedProdTermsInstanceOndemandCase1,
+	}
+
+	if !reflect.DeepEqual(expectedPricingCase1, awsTest.Pricing) {
+		expectedJsonString, _ := json.MarshalIndent(expectedPricingCase1, "", "  ")
+		resultJsonString, _ := json.MarshalIndent(awsTest.Pricing, "", "  ")
+		t.Logf("Expected: %s", string(expectedJsonString))
+		t.Logf("Result: %s", string(resultJsonString))
+		t.Fatalf("expected parsed pricing did not match actual parsed result (us-east-1)")
+	}
+
+	// Case 2
 	awsCnString := `
 	{
 		"formatVersion" : "v1.0",