From 8a923bcc1bbc5b55bfc33fdce93b3c7a4dad8ce7 Mon Sep 17 00:00:00 2001 From: Marc Riegel Date: Thu, 23 Nov 2017 11:19:23 +0100 Subject: [PATCH] Fixed broken instance-info test; Added areness of tenancy and effectivity of the offer provided by the aws pricing api --- .../cloudprovider/aws/api/instance_info.go | 102 ++++++++++++------ .../aws/api/instance_info_test.go | 9 +- 2 files changed, 72 insertions(+), 39 deletions(-) diff --git a/cluster-autoscaler/cloudprovider/aws/api/instance_info.go b/cluster-autoscaler/cloudprovider/aws/api/instance_info.go index aa74acf084f4..bb8749e9e772 100644 --- a/cluster-autoscaler/cloudprovider/aws/api/instance_info.go +++ b/cluster-autoscaler/cloudprovider/aws/api/instance_info.go @@ -28,8 +28,12 @@ import ( "time" ) -const instanceInfoCacheMaxAge = time.Hour * 6 -const awsPricingAPIURLTemplate = "https://pricing.us-east-1.amazonaws.com/offers/v1.0/aws/AmazonEC2/current/%s/index.json" +const ( + instanceInfoCacheMaxAge = time.Hour * 6 + awsPricingAPIURLTemplate = "https://pricing.us-east-1.amazonaws.com/offers/v1.0/aws/AmazonEC2/current/%s/index.json" + instanceOperatingSystemLinux = "Linux" + instanceTenancyShared = "Shared" +) // InstanceInfo holds AWS EC2 instance information type InstanceInfo struct { @@ -106,56 +110,86 @@ func (s *instanceInfoService) sync(availabilityZone string) error { } instances := make([]InstanceInfo, 0) + now := time.Now() for _, product := range response.Products { sku := product.SKU attr := product.Attributes - if attr.InstanceType != "" { - i := InstanceInfo{ - InstanceType: attr.InstanceType, - } + // TODO find better solution for the case of windows installations for instance. + if attr.OperatingSystem != instanceOperatingSystemLinux { + continue + } - var err error - if attr.Memory != "" && attr.Memory != "NA" { - if i.MemoryMb, err = parseMemory(attr.Memory); err != nil { - return fmt.Errorf("parser error %v", err) - } + // We do actually only support Shared tenancy instances. + // See for more information: http://docs.aws.amazon.com/AWSEC2/latest/UserGuide/instance-purchasing-options.html + if attr.Tenancy != instanceTenancyShared { + continue + } + + if len(attr.InstanceType) == 0 { + continue + } + + i := InstanceInfo{ + InstanceType: attr.InstanceType, + } + + var err error + if attr.Memory != "" && attr.Memory != "NA" { + if i.MemoryMb, err = parseMemory(attr.Memory); err != nil { + return fmt.Errorf("parser error %v", err) } + } - if attr.VCPU != "" { - if i.VCPU, err = parseCPU(attr.VCPU); err != nil { - return fmt.Errorf("parser error %v", err) - } + if attr.VCPU != "" { + if i.VCPU, err = parseCPU(attr.VCPU); err != nil { + return fmt.Errorf("parser error %v", err) } - if attr.GPU != "" { - if i.GPU, err = parseCPU(attr.GPU); err != nil { - return fmt.Errorf("parser error %v", err) - } + } + if attr.GPU != "" { + if i.GPU, err = parseCPU(attr.GPU); err != nil { + return fmt.Errorf("parser error %v", err) + } + } + + for priceSKU, offers := range response.Terms.OnDemand { + if priceSKU != sku { + continue } - for priceSKU, offers := range response.Terms.OnDemand { - if priceSKU != sku { + var lastOfferTime time.Time + var lastOfferPrice float64 + + for _, offer := range offers { + if offer.EffectiveDate.After(now) { continue } - for _, offer := range offers { - for _, price := range offer.PriceDimensions { - if price.EndRange != "Inf" || price.Unit != "Hrs" { - continue - } - p, err := strconv.ParseFloat(price.PricePerUnit.USD, 64) - if err != nil { - return fmt.Errorf("error parsing price for SKU %s [%s] %v", sku, price.PricePerUnit.USD, err) - } - - i.OnDemandPrice = p + for _, price := range offer.PriceDimensions { + if price.EndRange != "Inf" || price.Unit != "Hrs" { + continue + } + p, err := strconv.ParseFloat(price.PricePerUnit.USD, 64) + if err != nil { + return fmt.Errorf("error parsing price for SKU %s [%s] %v", sku, price.PricePerUnit.USD, err) + } + + if p == 0.0 { + continue + } + + if lastOfferTime.IsZero() || lastOfferTime.After(offer.EffectiveDate) { + lastOfferTime = offer.EffectiveDate + lastOfferPrice = p } } } - instances = append(instances, i) + i.OnDemandPrice = lastOfferPrice } + + instances = append(instances, i) } bucket.Clear() @@ -252,6 +286,7 @@ type productOffers map[string]productOffer type productOffer struct { OfferTermCode string `json:"offerTermCode"` + EffectiveDate time.Time `json:"effectiveDate"` SKU string `json:"sku"` PriceDimensions map[string]productPriceDimension `json:"priceDimensions"` } @@ -275,6 +310,7 @@ type product struct { } type productAttributes struct { + Tenancy string `json:"tenancy"` InstanceType string `json:"instanceType"` VCPU string `json:"vcpu"` Memory string `json:"memory"` diff --git a/cluster-autoscaler/cloudprovider/aws/api/instance_info_test.go b/cluster-autoscaler/cloudprovider/aws/api/instance_info_test.go index 470454794f36..701f45114c74 100644 --- a/cluster-autoscaler/cloudprovider/aws/api/instance_info_test.go +++ b/cluster-autoscaler/cloudprovider/aws/api/instance_info_test.go @@ -48,9 +48,6 @@ func TestInstanceInfoService_DescribeInstanceInfo(t *testing.T) { usWestOneURL, err := url.Parse(fmt.Sprintf(awsPricingAPIURLTemplate, "us-west-1")) assert.NoError(t, err) - //usWestTwoURL, err := url.Parse(fmt.Sprintf(awsPricingAPIURLTemplate, "us-west-2")) - //assert.NoError(t, err) - mc := &mockClient{m: make(map[string]mockResponse)} mc.m[usEastOneURL.Path] = mockResponse{pricingBody, 200} mc.m[usWestOneURL.Path] = mockResponse{[]byte("some non-json stuff"), 200} @@ -69,7 +66,7 @@ func TestInstanceInfoService_DescribeInstanceInfo(t *testing.T) { "m4.xlarge", "us-east-1", false, - 0, + 0.2, 4, }, { // error case: unknown availability zone @@ -103,9 +100,9 @@ func TestInstanceInfoService_DescribeInstanceInfo(t *testing.T) { assert.Error(t, err, fmt.Sprintf("case %d", n)) } else { assert.NoError(t, err, fmt.Sprintf("case %d", n)) - assert.Equal(t, tc.expectOnDemandPrice, info.OnDemandPrice) - assert.Equal(t, tc.expectCPU, info.VCPU) assert.Equal(t, tc.instanceType, info.InstanceType) + assert.Equal(t, tc.expectCPU, info.VCPU) + assert.Equal(t, tc.expectOnDemandPrice, info.OnDemandPrice) } }