[arvados] created: 2.1.0-2818-gf5512fd7f

git repository hosting git at public.arvados.org
Mon Aug 8 15:24:41 UTC 2022


        at  f5512fd7f9abe306740af464551938461033a935 (commit)


commit f5512fd7f9abe306740af464551938461033a935
Author: Tom Clegg <tom at curii.com>
Date:   Mon Aug 8 11:24:30 2022 -0400

    18205: Add container cost accounting fields.
    
    Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>

diff --git a/doc/api/methods/container_requests.html.textile.liquid b/doc/api/methods/container_requests.html.textile.liquid
index 15fa207b1..11f4f34fc 100644
--- a/doc/api/methods/container_requests.html.textile.liquid
+++ b/doc/api/methods/container_requests.html.textile.liquid
@@ -62,6 +62,7 @@ table(table table-bordered table-condensed).
 |runtime_auth_scopes|array of string|The scopes associated with the auth token used to run this container.||
 |output_storage_classes|array of strings|The storage classes that will be used for the log and output collections of this container request|default is ["default"]|
 |output_properties|hash|User metadata properties to set on the output collection.  The output collection will also have default properties "type" ("intermediate" or "output") and "container_request" (the uuid of container request that produced the collection).|
+|cumulative_cost|number|Estimated cost of the cloud VMs used to satisfy the request, including retried attempts and completed subrequests, but not including reused containers.|0 if container was reused or VM price information was not available.|
 
 h2(#priority). Priority
 
diff --git a/doc/api/methods/containers.html.textile.liquid b/doc/api/methods/containers.html.textile.liquid
index 43163c555..e6891621c 100644
--- a/doc/api/methods/containers.html.textile.liquid
+++ b/doc/api/methods/containers.html.textile.liquid
@@ -61,6 +61,8 @@ Generally this will contain additional keys that are not present in any correspo
 |interactive_session_started|boolean|Indicates whether @arvados-client shell@ has been used to run commands in the container, which may have altered the container's behavior and output.||
 |output_storage_classes|array of strings|The storage classes that will be used for the log and output collections of this container||
 |output_properties|hash|User metadata properties to set on the output collection.|
+|cost|number|Estimated cost of the cloud VM used to run the container.|0 if not available.|
+|subrequests_cost|number|Total estimated cumulative cost of container requests submitted by this container.|0 if not available.|
 
 h2(#container_states). Container states
 
diff --git a/lib/config/load.go b/lib/config/load.go
index fbd01488a..9269ddf27 100644
--- a/lib/config/load.go
+++ b/lib/config/load.go
@@ -448,6 +448,7 @@ func (ldr *Loader) setLoopbackInstanceType(cfg *arvados.Config) error {
 			RAM:             hostram,
 			Scratch:         scratch,
 			IncludedScratch: scratch,
+			Price:           1.0,
 		}}
 		cfg.Clusters[id] = cc
 	}
diff --git a/lib/controller/integration_test.go b/lib/controller/integration_test.go
index b0ec4293a..4c49c007e 100644
--- a/lib/controller/integration_test.go
+++ b/lib/controller/integration_test.go
@@ -1245,6 +1245,8 @@ func (s *IntegrationSuite) runContainer(c *check.C, clusterID string, token stri
 			time.Sleep(time.Second / 2)
 		}
 	}
+	c.Logf("cr.CumulativeCost == %f", cr.CumulativeCost)
+	c.Check(cr.CumulativeCost, check.Not(check.Equals), 0.0)
 	if expectExitCode >= 0 {
 		c.Check(ctr.State, check.Equals, arvados.ContainerStateComplete)
 		c.Check(ctr.ExitCode, check.Equals, expectExitCode)
diff --git a/lib/crunchrun/crunchrun.go b/lib/crunchrun/crunchrun.go
index eadf22876..ee9115d8d 100644
--- a/lib/crunchrun/crunchrun.go
+++ b/lib/crunchrun/crunchrun.go
@@ -140,6 +140,7 @@ type ContainerRunner struct {
 	MkArvClient   func(token string) (IArvadosClient, IKeepClient, *arvados.Client, error)
 	finalState    string
 	parentTemp    string
+	costStartTime time.Time
 
 	keepstoreLogger  io.WriteCloser
 	keepstoreLogbuf  *bufThenWrite
@@ -1457,6 +1458,10 @@ func (runner *ContainerRunner) UpdateContainerFinal() error {
 	if runner.finalState == "Complete" && runner.OutputPDH != nil {
 		update["output"] = *runner.OutputPDH
 	}
+	var it arvados.InstanceType
+	if j := os.Getenv("InstanceType"); j != "" && json.Unmarshal([]byte(j), &it) == nil && it.Price > 0 {
+		update["cost"] = it.Price * time.Now().Sub(runner.costStartTime).Seconds() / time.Hour.Seconds()
+	}
 	return runner.DispatcherArvClient.Update("containers", runner.Container.UUID, arvadosclient.Dict{"container": update}, nil)
 }
 
@@ -1489,6 +1494,7 @@ func (runner *ContainerRunner) Run() (err error) {
 	runner.CrunchLog.Printf("Using FUSE mount: %s", v)
 	runner.CrunchLog.Printf("Using container runtime: %s", runner.executor.Runtime())
 	runner.CrunchLog.Printf("Executing container: %s", runner.Container.UUID)
+	runner.costStartTime = time.Now()
 
 	hostname, hosterr := os.Hostname()
 	if hosterr != nil {
diff --git a/sdk/go/arvados/container.go b/sdk/go/arvados/container.go
index 466221fe1..45f92017c 100644
--- a/sdk/go/arvados/container.go
+++ b/sdk/go/arvados/container.go
@@ -38,6 +38,8 @@ type Container struct {
 	RuntimeToken              string                 `json:"runtime_token"`
 	AuthUUID                  string                 `json:"auth_uuid"`
 	Log                       string                 `json:"log"`
+	Cost                      float64                `json:"cost"`
+	SubrequestsCost           float64                `json:"subrequests_cost"`
 }
 
 // ContainerRequest is an arvados#container_request resource.
@@ -77,6 +79,7 @@ type ContainerRequest struct {
 	ContainerCount          int                    `json:"container_count"`
 	OutputStorageClasses    []string               `json:"output_storage_classes"`
 	OutputProperties        map[string]interface{} `json:"output_properties"`
+	CumulativeCost          float64                `json:"cumulative_cost"`
 }
 
 // Mount is special behavior to attach to a filesystem path or device.
diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb
index 43af0721c..3e3f73b83 100644
--- a/services/api/app/models/container.rb
+++ b/services/api/app/models/container.rb
@@ -83,6 +83,8 @@ class Container < ArvadosModel
     t.add :interactive_session_started
     t.add :output_storage_classes
     t.add :output_properties
+    t.add :cost
+    t.add :subrequests_cost
   end
 
   # Supported states for a container
@@ -478,8 +480,9 @@ class Container < ArvadosModel
 
   def validate_change
     permitted = [:state]
-    progress_attrs = [:progress, :runtime_status, :log, :output, :output_properties, :exit_code]
     final_attrs = [:finished_at]
+    progress_attrs = [:progress, :runtime_status, :subrequests_cost, :cost,
+                      :log, :output, :output_properties, :exit_code]
 
     if self.new_record?
       permitted.push(:owner_uuid, :command, :container_image, :cwd,
@@ -516,7 +519,7 @@ class Container < ArvadosModel
       when Running
         permitted.push :finished_at, *progress_attrs
       when Queued, Locked
-        permitted.push :finished_at, :log, :runtime_status
+        permitted.push :finished_at, :log, :runtime_status, :cost
       end
 
     else
@@ -719,6 +722,7 @@ class Container < ArvadosModel
               cr.with_lock do
                 leave_modified_by_user_alone do
                   # Use row locking because this increments container_count
+                  cr.cumulative_cost += self.cost + self.subrequests_cost
                   cr.container_uuid = c.uuid
                   cr.save!
                 end
diff --git a/services/api/app/models/container_request.rb b/services/api/app/models/container_request.rb
index 911603590..47e2769a8 100644
--- a/services/api/app/models/container_request.rb
+++ b/services/api/app/models/container_request.rb
@@ -33,6 +33,7 @@ class ContainerRequest < ArvadosModel
   serialize :scheduling_parameters, Hash
 
   after_find :fill_container_defaults_after_find
+  after_initialize { @state_was_when_initialized = self.state_was } # see finalize_if_needed
   before_validation :fill_field_defaults, :if => :new_record?
   before_validation :fill_container_defaults
   validates :command, :container_image, :output_path, :cwd, :presence => true
@@ -80,6 +81,7 @@ class ContainerRequest < ArvadosModel
     t.add :use_existing
     t.add :output_storage_classes
     t.add :output_properties
+    t.add :cumulative_cost
   end
 
   # Supported states for a container request
@@ -173,6 +175,30 @@ class ContainerRequest < ArvadosModel
   def finalize!
     container = Container.find_by_uuid(container_uuid)
     if !container.nil?
+      # We don't want to add the container cost if the container was
+      # already finished when this CR was committed. But we are
+      # running in an after_save hook after a lock/reload, so
+      # state_was has already been updated to Committed regardless.
+      # Hence the need for @state_was_when_initialized.
+      if @state_was_when_initialized == Committed
+        # Add the final container cost to our cumulative cost (which
+        # may already be non-zero from previous attempts if
+        # container_count_max > 1).
+        self.cumulative_cost += container.cost + container.subrequests_cost
+      end
+
+      # Add our cumulative cost to the subrequests_cost of the
+      # requesting container, if any.
+      if self.requesting_container_uuid
+        Container.where(
+          uuid: self.requesting_container_uuid,
+          state: Container::Running,
+        ).each do |c|
+          c.subrequests_cost += self.cumulative_cost
+          c.save!
+        end
+      end
+
       update_collections(container: container)
 
       if container.state == Container::Complete
@@ -461,7 +487,7 @@ class ContainerRequest < ArvadosModel
 
     case self.state
     when Committed
-      permitted.push :priority, :container_count_max, :container_uuid
+      permitted.push :priority, :container_count_max, :container_uuid, :cumulative_cost
 
       if self.priority.nil?
         self.errors.add :priority, "cannot be nil"
@@ -478,7 +504,7 @@ class ContainerRequest < ArvadosModel
     when Final
       if self.state_was == Committed
         # "Cancel" means setting priority=0, state=Committed
-        permitted.push :priority
+        permitted.push :priority, :cumulative_cost
 
         if current_user.andand.is_admin
           permitted.push :output_uuid, :log_uuid
diff --git a/services/api/db/migrate/20220804133317_add_cost_to_containers.rb b/services/api/db/migrate/20220804133317_add_cost_to_containers.rb
new file mode 100644
index 000000000..188187e39
--- /dev/null
+++ b/services/api/db/migrate/20220804133317_add_cost_to_containers.rb
@@ -0,0 +1,11 @@
+# Copyright (C) The Arvados Authors. All rights reserved.
+#
+# SPDX-License-Identifier: AGPL-3.0
+
+class AddCostToContainers < ActiveRecord::Migration[5.2]
+  def change
+    add_column :containers, :cost, :float, null: false, default: 0
+    add_column :containers, :subrequests_cost, :float, null: false, default: 0
+    add_column :container_requests, :cumulative_cost, :float, null: false, default: 0
+  end
+end
diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql
index c5f6d567b..42fb8b3aa 100644
--- a/services/api/db/structure.sql
+++ b/services/api/db/structure.sql
@@ -481,7 +481,8 @@ CREATE TABLE public.container_requests (
     secret_mounts jsonb DEFAULT '{}'::jsonb,
     runtime_token text,
     output_storage_classes jsonb DEFAULT '["default"]'::jsonb,
-    output_properties jsonb DEFAULT '{}'::jsonb
+    output_properties jsonb DEFAULT '{}'::jsonb,
+    cumulative_cost double precision DEFAULT 0.0 NOT NULL
 );
 
 
@@ -545,7 +546,9 @@ CREATE TABLE public.containers (
     gateway_address character varying,
     interactive_session_started boolean DEFAULT false NOT NULL,
     output_storage_classes jsonb DEFAULT '["default"]'::jsonb,
-    output_properties jsonb DEFAULT '{}'::jsonb
+    output_properties jsonb DEFAULT '{}'::jsonb,
+    cost double precision DEFAULT 0.0 NOT NULL,
+    subrequests_cost double precision DEFAULT 0.0 NOT NULL
 );
 
 
@@ -3182,6 +3185,7 @@ INSERT INTO "schema_migrations" (version) VALUES
 ('20220301155729'),
 ('20220303204419'),
 ('20220401153101'),
-('20220505112900');
+('20220505112900'),
+('20220804133317');
 
 
diff --git a/services/api/test/unit/container_request_test.rb b/services/api/test/unit/container_request_test.rb
index e5c008518..e6db41217 100644
--- a/services/api/test/unit/container_request_test.rb
+++ b/services/api/test/unit/container_request_test.rb
@@ -231,11 +231,12 @@ class ContainerRequestTest < ActiveSupport::TestCase
 
     act_as_system_user do
       Container.find_by_uuid(cr.container_uuid).
-        update_attributes!(state: Container::Cancelled)
+        update_attributes!(state: Container::Cancelled, cost: 1.25)
     end
 
     cr.reload
     assert_equal "Final", cr.state
+    assert_equal 1.25, cr.cumulative_cost
     assert_equal users(:active).uuid, cr.modified_by_user_uuid
   end
 
@@ -261,12 +262,14 @@ class ContainerRequestTest < ActiveSupport::TestCase
     log_pdh = 'fa7aeb5140e2848d39b416daeef4ffc5+45'
     act_as_system_user do
       c.update_attributes!(state: Container::Complete,
+                           cost: 1.25,
                            output: output_pdh,
                            log: log_pdh)
     end
 
     cr.reload
     assert_equal "Final", cr.state
+    assert_equal 1.25, cr.cumulative_cost
     assert_equal users(:active).uuid, cr.modified_by_user_uuid
 
     assert_not_nil cr.output_uuid
@@ -788,6 +791,7 @@ class ContainerRequestTest < ActiveSupport::TestCase
     prev_container_uuid = cr.container_uuid
 
     act_as_system_user do
+      c.update_attributes!(cost: 0.5, subrequests_cost: 1.25)
       c.update_attributes!(state: Container::Cancelled)
     end
 
@@ -800,6 +804,9 @@ class ContainerRequestTest < ActiveSupport::TestCase
 
     c = act_as_system_user do
       c = Container.find_by_uuid(cr.container_uuid)
+      c.update_attributes!(state: Container::Locked)
+      c.update_attributes!(state: Container::Running)
+      c.update_attributes!(cost: 0.125)
       c.update_attributes!(state: Container::Cancelled)
       c
     end
@@ -809,6 +816,7 @@ class ContainerRequestTest < ActiveSupport::TestCase
     assert_equal "Final", cr.state
     assert_equal prev_container_uuid, cr.container_uuid
     assert_not_equal cr2.container_uuid, cr.container_uuid
+    assert_equal 1.875, cr.cumulative_cost
   end
 
   test "Retry on container cancelled with runtime_token" do
@@ -1511,4 +1519,63 @@ class ContainerRequestTest < ActiveSupport::TestCase
     end
   end
 
+  test "Cumulative cost includes retried attempts but not reused containers" do
+    set_user_from_auth :active
+    cr = create_minimal_req!(priority: 5, state: "Committed", container_count_max: 3)
+    c = Container.find_by_uuid cr.container_uuid
+    act_as_system_user do
+      c.update_attributes!(state: Container::Locked)
+      c.update_attributes!(state: Container::Running)
+      c.update_attributes!(state: Container::Cancelled, cost: 3)
+    end
+    cr.reload
+    assert_equal 3, cr.cumulative_cost
+
+    c = Container.find_by_uuid cr.container_uuid
+    lock_and_run c
+    c.reload
+    assert_equal 0, c.subrequests_cost
+
+    # cr2 is a child/subrequest
+    cr2 = with_container_auth(c) do
+      create_minimal_req!(priority: 10, state: "Committed", container_count_max: 1, command: ["echo", "foo2"])
+    end
+    assert_equal c.uuid, cr2.requesting_container_uuid
+    c2 = Container.find_by_uuid cr2.container_uuid
+    act_as_system_user do
+      c2.update_attributes!(state: Container::Locked)
+      c2.update_attributes!(state: Container::Running)
+      logc = Collection.new(owner_uuid: system_user_uuid,
+                            manifest_text: ". ef772b2f28e2c8ca84de45466ed19ee9+7815 0:0:arv-mount.txt\n")
+      logc.save!
+      c2.update_attributes!(state: Container::Complete,
+                            exit_code: 0,
+                            output: '1f4b0bc7583c2a7f9102c395f4ffc5e3+45',
+                            log: logc.portable_data_hash,
+                            cost: 7)
+    end
+    c.reload
+    assert_equal 7, c.subrequests_cost
+
+    # cr3 is an identical child/subrequest, will reuse c2
+    cr3 = with_container_auth(c) do
+      create_minimal_req!(priority: 10, state: "Committed", container_count_max: 1, command: ["echo", "foo2"])
+    end
+    assert_equal c.uuid, cr3.requesting_container_uuid
+    c3 = Container.find_by_uuid cr3.container_uuid
+    assert_equal c2.uuid, c3.uuid
+    assert_equal Container::Complete, c3.state
+    c.reload
+    assert_equal 7, c.subrequests_cost
+
+    act_as_system_user do
+      c.update_attributes!(state: Container::Complete, exit_code: 0, cost: 9)
+    end
+
+    c.reload
+    assert_equal 7, c.subrequests_cost
+    cr.reload
+    assert_equal 3+7+9, cr.cumulative_cost
+  end
+
 end

-----------------------------------------------------------------------


hooks/post-receive
-- 




More information about the arvados-commits mailing list