19320: Account for AddedScratch in spot instance cost estimates.

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@curii.com>

diff --git a/lib/cloud/azure/azure.go b/lib/cloud/azure/azure.go
index c278005923f88b9301e658417befdc8e3049dbb0..7b170958b6ee69921f9d6186cfeaa79f091eedfb 100644 (file)
--- a/lib/cloud/azure/azure.go
+++ b/lib/cloud/azure/azure.go
@@ -785,7 +785,7 @@ func (ai *azureInstance) Address() string {
        }
 }
 
-func (ai *azureInstance) PriceHistory() []cloud.InstancePrice {
+func (ai *azureInstance) PriceHistory(arvados.InstanceType) []cloud.InstancePrice {
        return nil
 }
 

diff --git a/lib/cloud/ec2/ec2.go b/lib/cloud/ec2/ec2.go
index 2a5eea484590c646e8398762d5692a441f2dc15e..93c308bcb13f328185aaf4aa6e0421bfd4fe62f8 100644 (file)
--- a/lib/cloud/ec2/ec2.go
+++ b/lib/cloud/ec2/ec2.go
@@ -48,6 +48,7 @@ type ec2InstanceSetConfig struct {
        SubnetID                string
        AdminUsername           string
        EBSVolumeType           string
+       EBSPrice                float64
        IAMInstanceProfile      string
        SpotPriceUpdateInterval arvados.Duration
 }
@@ -506,7 +507,7 @@ func (inst *ec2Instance) VerifyHostKey(ssh.PublicKey, *ssh.Client) error {
 // Spot price that is in effect when your Spot Instance is running."
 // (The use of the phrase "is running", as opposed to "was launched",
 // hints that pricing is dynamic.)
-func (inst *ec2Instance) PriceHistory() []cloud.InstancePrice {
+func (inst *ec2Instance) PriceHistory(instType arvados.InstanceType) []cloud.InstancePrice {
        inst.provider.pricesLock.Lock()
        defer inst.provider.pricesLock.Unlock()
        pk := priceKey{
@@ -514,7 +515,16 @@ func (inst *ec2Instance) PriceHistory() []cloud.InstancePrice {
                spot:             aws.StringValue(inst.instance.InstanceLifecycle) == "spot",
                availabilityZone: inst.availabilityZone,
        }
-       return inst.provider.prices[pk]
+       var prices []cloud.InstancePrice
+       for _, price := range inst.provider.prices[pk] {
+               // ceil(added scratch space in GiB)
+               gib := (instType.AddedScratch + 1<<30 - 1) >> 30
+               monthly := inst.provider.ec2config.EBSPrice * float64(gib)
+               hourly := monthly / 30 / 24
+               price.Price += hourly
+               prices = append(prices, price)
+       }
+       return prices
 }
 
 type rateLimitError struct {

diff --git a/lib/cloud/ec2/ec2_test.go b/lib/cloud/ec2/ec2_test.go
index e7534a7b6925c3a13948439ec1e570a0193a43ef..79ee0d04eb6a991c92289d3f85cdcac45892c9ec 100644 (file)
--- a/lib/cloud/ec2/ec2_test.go
+++ b/lib/cloud/ec2/ec2_test.go
@@ -150,7 +150,7 @@ func (e *ec2stub) TerminateInstances(input *ec2.TerminateInstancesInput) (*ec2.T
        return nil, nil
 }
 
-func GetInstanceSet(c *check.C) (cloud.InstanceSet, cloud.ImageID, arvados.Cluster) {
+func GetInstanceSet(c *check.C) (*ec2InstanceSet, cloud.ImageID, arvados.Cluster) {
        cluster := arvados.Cluster{
                InstanceTypes: arvados.InstanceTypeMap(map[string]arvados.InstanceType{
                        "tiny": {
@@ -188,7 +188,7 @@ func GetInstanceSet(c *check.C) (cloud.InstanceSet, cloud.ImageID, arvados.Clust
 
                ap, err := newEC2InstanceSet(exampleCfg.DriverParameters, "test123", nil, logrus.StandardLogger())
                c.Assert(err, check.IsNil)
-               return ap, cloud.ImageID(exampleCfg.ImageIDForTestSuite), cluster
+               return ap.(*ec2InstanceSet), cloud.ImageID(exampleCfg.ImageIDForTestSuite), cluster
        }
        ap := ec2InstanceSet{
                ec2config:     ec2InstanceSetConfig{},
@@ -297,6 +297,7 @@ func (*EC2InstanceSetSuite) TestInstancePriceHistory(c *check.C) {
                }
        }()
 
+       ap.ec2config.EBSPrice = 0.1 // $/GiB/month
        inst1, err := ap.Create(cluster.InstanceTypes["tiny-preemptible"], img, tags, "true", pk)
        c.Assert(err, check.IsNil)
        defer inst1.Destroy()
@@ -328,14 +329,19 @@ func (*EC2InstanceSetSuite) TestInstancePriceHistory(c *check.C) {
        }
 
        for _, inst := range instances {
-               hist := inst.PriceHistory()
+               hist := inst.PriceHistory(arvados.InstanceType{})
                c.Logf("%s price history: %v", inst.ID(), hist)
                c.Check(len(hist) > 0, check.Equals, true)
+
+               histWithScratch := inst.PriceHistory(arvados.InstanceType{AddedScratch: 640 << 30})
+               c.Logf("%s price history with 640 GiB scratch: %v", inst.ID(), histWithScratch)
+
                for i, ip := range hist {
                        c.Check(ip.Price, check.Not(check.Equals), 0.0)
                        if i > 0 {
                                c.Check(ip.StartTime.Before(hist[i-1].StartTime), check.Equals, true)
                        }
+                       c.Check(ip.Price < histWithScratch[i].Price, check.Equals, true)
                }
        }
 }

diff --git a/lib/cloud/interfaces.go b/lib/cloud/interfaces.go
index 7f59049681bc54ef4bd600308cb298ea297f35c8..27cf26152c2962fa1b8d9afeba34f4fad57e2922 100644 (file)
--- a/lib/cloud/interfaces.go
+++ b/lib/cloud/interfaces.go
@@ -102,8 +102,11 @@ type Instance interface {
        // Replace tags with the given tags
        SetTags(InstanceTags) error
 
-       // Get recent price history, if available
-       PriceHistory() []InstancePrice
+       // Get recent price history, if available. The InstanceType is
+       // supplied as an argument so the driver implementation can
+       // account for AddedScratch cost without requesting the volume
+       // attachment information from the provider's API.
+       PriceHistory(arvados.InstanceType) []InstancePrice
 
        // Shut down the node
        Destroy() error

diff --git a/lib/cloud/loopback/loopback.go b/lib/cloud/loopback/loopback.go
index ed2a0050f4f33a07a0b9a25fbdcc8e911547d0d9..8afaa452570c1b79c90989edd8247408f5eb2b54 100644 (file)
--- a/lib/cloud/loopback/loopback.go
+++ b/lib/cloud/loopback/loopback.go
@@ -130,13 +130,13 @@ type instance struct {
        sshService   test.SSHService
 }
 
-func (i *instance) ID() cloud.InstanceID                { return cloud.InstanceID(i.instanceType.ProviderType) }
-func (i *instance) String() string                      { return i.instanceType.ProviderType }
-func (i *instance) ProviderType() string                { return i.instanceType.ProviderType }
-func (i *instance) Address() string                     { return i.sshService.Address() }
-func (i *instance) PriceHistory() []cloud.InstancePrice { return nil }
-func (i *instance) RemoteUser() string                  { return i.adminUser }
-func (i *instance) Tags() cloud.InstanceTags            { return i.tags }
+func (i *instance) ID() cloud.InstanceID                                    { return cloud.InstanceID(i.instanceType.ProviderType) }
+func (i *instance) String() string                                          { return i.instanceType.ProviderType }
+func (i *instance) ProviderType() string                                    { return i.instanceType.ProviderType }
+func (i *instance) Address() string                                         { return i.sshService.Address() }
+func (i *instance) PriceHistory(arvados.InstanceType) []cloud.InstancePrice { return nil }
+func (i *instance) RemoteUser() string                                      { return i.adminUser }
+func (i *instance) Tags() cloud.InstanceTags                                { return i.tags }
 func (i *instance) SetTags(tags cloud.InstanceTags) error {
        i.tags = tags
        return nil

diff --git a/lib/config/config.default.yml b/lib/config/config.default.yml
index a8adaeff8d1134621218cea8459a26b8772a64cf..078dbf494eda04b95bce38a963b06a4365e7292c 100644 (file)
--- a/lib/config/config.default.yml
+++ b/lib/config/config.default.yml
@@ -1400,6 +1400,14 @@ Clusters:
           # calculating container cost estimates.
           SpotPriceUpdateInterval: 24h
 
+          # (ec2) per-GiB-month cost of EBS volumes. Matches
+          # EBSVolumeType. Used to account for AddedScratch when
+          # calculating container cost estimates. Note that
+          # https://aws.amazon.com/ebs/pricing/ defines GB to mean
+          # GiB, so an advertised price $0.10/GB indicates a real
+          # price of $0.10/GiB and can be entered here as 0.10.
+          EBSPrice: 0.10
+
           # (azure) Credentials.
           SubscriptionID: ""
           ClientID: ""
@@ -1453,6 +1461,13 @@ Clusters:
         RAM: 128MiB
         IncludedScratch: 16GB
         AddedScratch: 0
+        # Hourly price ($), used to select node types for containers,
+        # and to calculate estimated container costs. For spot
+        # instances on EC2, this is also used as the maximum price
+        # when launching spot instances, while the estimated container
+        # cost is computed based on the current spot price according
+        # to AWS. On Azure, and on-demand instances on EC2, the price
+        # given here is used to compute container cost estimates.
         Price: 0.1
         Preemptible: false
         # Include this section if the node type includes GPU (CUDA) support

diff --git a/lib/crunchrun/crunchrun.go b/lib/crunchrun/crunchrun.go
index 1f130a5b1003c441023953e862edbfac7ba650c0..507dfc4dd713da6dfdb0410d7d043726dcca5f76 100644 (file)
--- a/lib/crunchrun/crunchrun.go
+++ b/lib/crunchrun/crunchrun.go
@@ -2298,6 +2298,12 @@ func (cr *ContainerRunner) calculateCost(now time.Time) float64 {
        spanEnd := now
        for _, ip := range prices {
                spanStart := ip.StartTime
+               if spanStart.After(now) {
+                       // pricing information from the future -- not
+                       // expected from AWS, but possible in
+                       // principle, and exercised by tests.
+                       continue
+               }
                last := false
                if spanStart.Before(cr.costStartTime) {
                        spanStart = cr.costStartTime
@@ -2309,5 +2315,6 @@ func (cr *ContainerRunner) calculateCost(now time.Time) float64 {
                }
                spanEnd = spanStart
        }
+
        return cost
 }

diff --git a/lib/crunchrun/crunchrun_test.go b/lib/crunchrun/crunchrun_test.go
index a5045863b0e1bc17cf5a372e290a9b8e2716b3df..f4cd6e6090e048282940d87725e4719460010b69 100644 (file)
--- a/lib/crunchrun/crunchrun_test.go
+++ b/lib/crunchrun/crunchrun_test.go
@@ -2119,6 +2119,9 @@ func (s *TestSuite) TestCalculateCost(c *C) {
        cost = cr.calculateCost(now)
        c.Check(cost, Equals, 1.0/2+2.0/4+3.0/4)
 
+       cost = cr.calculateCost(now.Add(-time.Hour / 2))
+       c.Check(cost, Equals, 0.5)
+
        c.Logf("%s", logbuf.String())
        c.Check(logbuf.String(), Matches, `(?ms).*Instance price changed to 1\.00 at 20.* changed to 2\.00 .* changed to 3\.00 .*`)
        c.Check(logbuf.String(), Not(Matches), `(?ms).*changed to 2\.00 .* changed to 2\.00 .*`)

diff --git a/lib/dispatchcloud/test/stub_driver.go b/lib/dispatchcloud/test/stub_driver.go
index bb134e4543649155e16af2d8a0a159ab160e5d99..01af8e6d547931d43a5e72725159c72a7e6dd3ac 100644 (file)
--- a/lib/dispatchcloud/test/stub_driver.go
+++ b/lib/dispatchcloud/test/stub_driver.go
@@ -471,6 +471,6 @@ func copyTags(src cloud.InstanceTags) cloud.InstanceTags {
        return dst
 }
 
-func (si stubInstance) PriceHistory() []cloud.InstancePrice {
+func (si stubInstance) PriceHistory(arvados.InstanceType) []cloud.InstancePrice {
        return nil
 }

diff --git a/lib/dispatchcloud/worker/worker.go b/lib/dispatchcloud/worker/worker.go
index 397a46292981170816ed9bf818284d9c3dff2be8..b2ed6c2bff5b039435944b851a9fe3646c922001 100644 (file)
--- a/lib/dispatchcloud/worker/worker.go
+++ b/lib/dispatchcloud/worker/worker.go
@@ -384,7 +384,7 @@ func (wkr *worker) probeRunning() (running []string, reportsBroken, ok bool) {
        }
        before := time.Now()
        var stdin io.Reader
-       if prices := wkr.instance.PriceHistory(); len(prices) > 0 {
+       if prices := wkr.instance.PriceHistory(wkr.instType); len(prices) > 0 {
                j, _ := json.Marshal(prices)
                stdin = bytes.NewReader(j)
        }