[ARVADOS] updated: 3ede205bf0e5af7a88e04009cd66ff111e0729c3

Git user git at public.curoverse.com
Thu May 12 11:45:07 EDT 2016


Summary of changes:
 sdk/go/arvadosclient/arvadosclient.go              |   2 +-
 sdk/go/arvadostest/fixtures.go                     |   3 +
 .../v1/api_client_authorizations_controller.rb     |  10 +-
 .../arvados/v1/containers_controller.rb            |  15 ++
 services/api/app/models/container.rb               | 116 +++++---
 services/api/app/models/container_request.rb       |  62 +++--
 services/api/config/routes.rb                      |   5 +-
 .../20160506175108_add_auths_to_container.rb       |   6 +
 ...9143250_add_auth_and_lock_to_container_index.rb |  19 ++
 services/api/db/structure.sql                      |  12 +-
 services/api/lib/current_api_client.rb             |   6 +
 services/api/lib/whitelist_update.rb               |   4 +-
 .../test/fixtures/api_client_authorizations.yml    |   6 +
 services/api/test/fixtures/container_requests.yml  |  13 +
 services/api/test/fixtures/containers.yml          |   4 +-
 .../api_client_authorizations_controller_test.rb   |  13 +
 .../arvados/v1/containers_controller_test.rb       |  52 ++++
 services/api/test/test_helper.rb                   |   4 +
 services/api/test/unit/container_request_test.rb   |   8 +-
 services/api/test/unit/container_test.rb           | 295 +++++++++------------
 .../crunch-dispatch-local/crunch-dispatch-local.go | 146 ++++++----
 .../crunch-dispatch-local_test.go                  |  19 +-
 .../crunch-dispatch-slurm/crunch-dispatch-slurm.go | 227 ++++++++++++----
 .../crunch-dispatch-slurm_test.go                  |  32 ++-
 24 files changed, 702 insertions(+), 377 deletions(-)
 create mode 100644 services/api/db/migrate/20160506175108_add_auths_to_container.rb
 create mode 100644 services/api/db/migrate/20160509143250_add_auth_and_lock_to_container_index.rb
 create mode 100644 services/api/test/fixtures/container_requests.yml
 create mode 100644 services/api/test/functional/arvados/v1/containers_controller_test.rb

       via  3ede205bf0e5af7a88e04009cd66ff111e0729c3 (commit)
       via  408acd0b02c1d6f246b3723a9344c68c17d836b2 (commit)
       via  f1b2f6bf13b5e1a21d429a5b412e82fe84771cdd (commit)
       via  e348da8c60b35adae6bc9fdf388cebc8aee7dc6b (commit)
       via  b8be976adfabb17718dee8c569129cdf28036380 (commit)
       via  4e2763883588ac691da65ee316a52a052c002aa7 (commit)
       via  8e5206a5b1910ee7bc1d0a45af754ce507a7f237 (commit)
       via  55923446d946f11f03243ace3281b54ac5a4e80b (commit)
       via  14e317f75f2e3ecee53d78012eddaa59ce9e2712 (commit)
       via  1b4fa5760aab91a8422dc5d84c73bd627ff1dc51 (commit)
      from  12bfd7a65c6635a882cb2e5e419321db100b9d56 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.


commit 3ede205bf0e5af7a88e04009cd66ff111e0729c3
Merge: 12bfd7a 408acd0
Author: Tom Clegg <tom at curoverse.com>
Date:   Thu May 12 11:34:51 2016 -0400

    Merge branch '8128-crunch2-auth-api'
    
    closes #8128


commit 408acd0b02c1d6f246b3723a9344c68c17d836b2
Author: Tom Clegg <tom at curoverse.com>
Date:   Thu May 12 10:23:31 2016 -0400

    8128: Fix test race.

diff --git a/services/crunch-dispatch-local/crunch-dispatch-local.go b/services/crunch-dispatch-local/crunch-dispatch-local.go
index 848d723..4023870 100644
--- a/services/crunch-dispatch-local/crunch-dispatch-local.go
+++ b/services/crunch-dispatch-local/crunch-dispatch-local.go
@@ -131,10 +131,14 @@ func dispatchLocal(pollInterval time.Duration, crunchRunCommand string) {
 		return
 	}
 
-	for i := 0; i < len(containers.Items); i++ {
-		log.Printf("About to run queued container %v", containers.Items[i].UUID)
+	for _, c := range containers.Items {
+		log.Printf("About to run queued container %v", c.UUID)
 		// Run the container
-		go run(containers.Items[i].UUID, crunchRunCommand, pollInterval)
+		waitGroup.Add(1)
+		go func(c Container) {
+			run(c.UUID, crunchRunCommand, pollInterval)
+			waitGroup.Done()
+		}(c)
 	}
 }
 
@@ -187,10 +191,6 @@ func run(uuid string, crunchRunCommand string, pollInterval time.Duration) {
 
 	log.Printf("Starting container %v", uuid)
 
-	// Add this crunch job to waitGroup
-	waitGroup.Add(1)
-	defer waitGroup.Done()
-
 	updateState(uuid, "Running")
 
 	cmdExited := make(chan struct{})

commit f1b2f6bf13b5e1a21d429a5b412e82fe84771cdd
Author: Tom Clegg <tom at curoverse.com>
Date:   Thu May 12 09:08:14 2016 -0400

    8128: Fix flaky test: pipe the "echo UUID" script to sh, not to "echo UUID".

diff --git a/services/crunch-dispatch-slurm/crunch-dispatch-slurm_test.go b/services/crunch-dispatch-slurm/crunch-dispatch-slurm_test.go
index fe4cba6..3dfb7d5 100644
--- a/services/crunch-dispatch-slurm/crunch-dispatch-slurm_test.go
+++ b/services/crunch-dispatch-slurm/crunch-dispatch-slurm_test.go
@@ -77,7 +77,7 @@ func (s *TestSuite) Test_doMain(c *C) {
 	}(sbatchCmd)
 	sbatchCmd = func(container Container) *exec.Cmd {
 		sbatchCmdLine = sbatchFunc(container).Args
-		return exec.Command("echo", container.UUID)
+		return exec.Command("sh")
 	}
 
 	// Override striggerCmd

commit e348da8c60b35adae6bc9fdf388cebc8aee7dc6b
Author: Tom Clegg <tom at curoverse.com>
Date:   Wed May 11 11:01:28 2016 -0400

    8128: Use row lock during Container update, add comments.

diff --git a/services/api/app/controllers/arvados/v1/containers_controller.rb b/services/api/app/controllers/arvados/v1/containers_controller.rb
index 3580397..21ee7ef 100644
--- a/services/api/app/controllers/arvados/v1/containers_controller.rb
+++ b/services/api/app/controllers/arvados/v1/containers_controller.rb
@@ -11,4 +11,12 @@ class Arvados::V1::ContainersController < ApplicationController
     @object = @object.auth
     show
   end
+
+  # Updates use row locking to resolve races between multiple
+  # dispatchers trying to lock the same container.
+  def update
+    @object.with_lock do
+      super
+    end
+  end
 end
diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb
index 845374e..4c77008 100644
--- a/services/api/app/models/container.rb
+++ b/services/api/app/models/container.rb
@@ -146,18 +146,27 @@ class Container < ArvadosModel
   end
 
   def validate_lock
+    # If the Container is already locked by someone other than the
+    # current api_client_auth, disallow all changes -- except
+    # priority, which needs to change to reflect max(priority) of
+    # relevant ContainerRequests.
     if locked_by_uuid_was
       if locked_by_uuid_was != Thread.current[:api_client_authorization].uuid
-        # Notably, prohibit changing state or locked_by_uuid:
         check_update_whitelist [:priority]
       end
     end
 
     if [Locked, Running].include? self.state
-      need_lock = Thread.current[:api_client_authorization].uuid
+      # If the Container was already locked, locked_by_uuid must not
+      # changes. Otherwise, the current auth gets the lock.
+      need_lock = locked_by_uuid_was || Thread.current[:api_client_authorization].uuid
     else
       need_lock = nil
     end
+
+    # The caller can provide a new value for locked_by_uuid, but only
+    # if it's exactly what we expect. This allows a caller to perform
+    # an update like {"state":"Unlocked","locked_by_uuid":null}.
     if self.locked_by_uuid_changed?
       if self.locked_by_uuid != need_lock
         return errors.add :locked_by_uuid, "can only change to #{need_lock}"

commit b8be976adfabb17718dee8c569129cdf28036380
Author: Tom Clegg <tom at curoverse.com>
Date:   Tue May 10 10:45:05 2016 -0400

    8128: Add arvados.v1.api_client_authorizations.current

diff --git a/sdk/go/arvadosclient/arvadosclient.go b/sdk/go/arvadosclient/arvadosclient.go
index b67eaa5..8cdfa48 100644
--- a/sdk/go/arvadosclient/arvadosclient.go
+++ b/sdk/go/arvadosclient/arvadosclient.go
@@ -273,7 +273,7 @@ func newAPIServerError(ServerAddress string, resp *http.Response) APIServerError
 // Returns a non-nil error if an error occurs making the API call, the
 // API responds with a non-successful HTTP status, or an error occurs
 // parsing the response body.
-func (c ArvadosClient) Call(method string, resourceType string, uuid string, action string, parameters Dict, output interface{}) error {
+func (c ArvadosClient) Call(method, resourceType, uuid, action string, parameters Dict, output interface{}) error {
 	reader, err := c.CallRaw(method, resourceType, uuid, action, parameters)
 	if reader != nil {
 		defer reader.Close()
diff --git a/services/api/app/controllers/arvados/v1/api_client_authorizations_controller.rb b/services/api/app/controllers/arvados/v1/api_client_authorizations_controller.rb
index 5229d80..76acc70 100644
--- a/services/api/app/controllers/arvados/v1/api_client_authorizations_controller.rb
+++ b/services/api/app/controllers/arvados/v1/api_client_authorizations_controller.rb
@@ -1,8 +1,9 @@
 class Arvados::V1::ApiClientAuthorizationsController < ApplicationController
   accept_attribute_as_json :scopes, Array
-  before_filter :current_api_client_is_trusted
+  before_filter :current_api_client_is_trusted, :except => [:current]
   before_filter :admin_required, :only => :create_system_auth
-  skip_before_filter :render_404_if_no_object, :only => :create_system_auth
+  skip_before_filter :render_404_if_no_object, :only => [:create_system_auth, :current]
+  skip_before_filter :find_object_by_uuid, :only => [:create_system_auth, :current]
 
   def self._create_system_auth_requires_parameters
     {
@@ -40,6 +41,11 @@ class Arvados::V1::ApiClientAuthorizationsController < ApplicationController
     super
   end
 
+  def current
+    @object = Thread.current[:api_client_authorization]
+    show
+  end
+
   protected
 
   def default_orders
diff --git a/services/api/config/routes.rb b/services/api/config/routes.rb
index 4cea874..ed8f8d8 100644
--- a/services/api/config/routes.rb
+++ b/services/api/config/routes.rb
@@ -15,6 +15,7 @@ Server::Application.routes.draw do
     namespace :v1 do
       resources :api_client_authorizations do
         post 'create_system_auth', on: :collection
+        get 'current', on: :collection
       end
       resources :api_clients
       resources :authorized_keys
diff --git a/services/api/test/functional/arvados/v1/api_client_authorizations_controller_test.rb b/services/api/test/functional/arvados/v1/api_client_authorizations_controller_test.rb
index 9f0f555..37e690e 100644
--- a/services/api/test/functional/arvados/v1/api_client_authorizations_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/api_client_authorizations_controller_test.rb
@@ -168,4 +168,17 @@ class Arvados::V1::ApiClientAuthorizationsControllerTest < ActionController::Tes
     }
     assert_response 403
   end
+
+  test "get current token" do
+    authorize_with :active
+    get :current
+    assert_response :success
+    assert_equal(json_response['api_token'],
+                 api_client_authorizations(:active).api_token)
+  end
+
+  test "get current token, no auth" do
+    get :current
+    assert_response 401
+  end
 end
diff --git a/services/crunch-dispatch-slurm/crunch-dispatch-slurm.go b/services/crunch-dispatch-slurm/crunch-dispatch-slurm.go
index 8eefd35..53e4705 100644
--- a/services/crunch-dispatch-slurm/crunch-dispatch-slurm.go
+++ b/services/crunch-dispatch-slurm/crunch-dispatch-slurm.go
@@ -103,15 +103,12 @@ type apiClientAuthorizationList struct {
 // This is because, once one or more crunch jobs are running,
 // we would need to wait for them complete.
 func runQueuedContainers(pollInterval, priorityPollInterval int, crunchRunCommand, finishCommand string) {
-	var authList apiClientAuthorizationList
-	err := arv.List("api_client_authorizations", map[string]interface{}{
-		"filters": [][]interface{}{{"api_token", "=", arv.ApiToken}},
-	}, &authList)
-	if err != nil || len(authList.Items) != 1 {
-		log.Printf("Error getting my token UUID: %v (%d)", err, len(authList.Items))
+	var auth apiClientAuthorization
+	err := arv.Call("GET", "api_client_authorizations", "", "current", nil, &auth)
+	if err != nil {
+		log.Printf("Error getting my token UUID: %v", err)
 		return
 	}
-	auth := authList.Items[0]
 
 	ticker := time.NewTicker(time.Duration(pollInterval) * time.Second)
 	for {
diff --git a/services/crunch-dispatch-slurm/crunch-dispatch-slurm_test.go b/services/crunch-dispatch-slurm/crunch-dispatch-slurm_test.go
index 7fd20c1..fe4cba6 100644
--- a/services/crunch-dispatch-slurm/crunch-dispatch-slurm_test.go
+++ b/services/crunch-dispatch-slurm/crunch-dispatch-slurm_test.go
@@ -141,7 +141,7 @@ func (s *TestSuite) Test_doMain(c *C) {
 
 func (s *MockArvadosServerSuite) Test_APIErrorGettingContainers(c *C) {
 	apiStubResponses := make(map[string]arvadostest.StubResponse)
-	apiStubResponses["/arvados/v1/api_client_authorizations"] = arvadostest.StubResponse{200, string(`{"items":[{"uuid":"` + arvadostest.Dispatch1AuthUUID + `"}]}`)}
+	apiStubResponses["/arvados/v1/api_client_authorizations/current"] = arvadostest.StubResponse{200, `{"uuid":"` + arvadostest.Dispatch1AuthUUID + `"}`}
 	apiStubResponses["/arvados/v1/containers"] = arvadostest.StubResponse{500, string(`{}`)}
 
 	testWithServerStub(c, apiStubResponses, "echo", "Error getting list of queued containers")

commit 4e2763883588ac691da65ee316a52a052c002aa7
Author: Tom Clegg <tom at curoverse.com>
Date:   Mon May 9 15:33:05 2016 -0400

    8128: Add runtime tokens for containers, and locks for multiple dispatchers

diff --git a/services/api/app/controllers/arvados/v1/containers_controller.rb b/services/api/app/controllers/arvados/v1/containers_controller.rb
index 04a5ed0..3580397 100644
--- a/services/api/app/controllers/arvados/v1/containers_controller.rb
+++ b/services/api/app/controllers/arvados/v1/containers_controller.rb
@@ -4,4 +4,11 @@ class Arvados::V1::ContainersController < ApplicationController
   accept_attribute_as_json :runtime_constraints, Hash
   accept_attribute_as_json :command, Array
 
+  def auth
+    if @object.locked_by_uuid != Thread.current[:api_client_authorization].uuid
+      raise ArvadosModel::PermissionDeniedError.new("Not locked by your token")
+    end
+    @object = @object.auth
+    show
+  end
 end
diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb
index a014552..845374e 100644
--- a/services/api/app/models/container.rb
+++ b/services/api/app/models/container.rb
@@ -16,9 +16,12 @@ class Container < ArvadosModel
   validates :command, :container_image, :output_path, :cwd, :priority, :presence => true
   validate :validate_state_change
   validate :validate_change
+  validate :validate_lock
+  after_validation :assign_auth
   after_save :handle_completed
 
   has_many :container_requests, :foreign_key => :container_uuid, :class_name => 'ContainerRequest', :primary_key => :uuid
+  belongs_to :auth, :class_name => 'ApiClientAuthorization', :foreign_key => :auth_uuid, :primary_key => :uuid
 
   api_accessible :user, extend: :common do |t|
     t.add :command
@@ -37,6 +40,7 @@ class Container < ArvadosModel
     t.add :runtime_constraints
     t.add :started_at
     t.add :state
+    t.add :auth_uuid
   end
 
   # Supported states for a container
@@ -61,27 +65,17 @@ class Container < ArvadosModel
   end
 
   def update_priority!
-    if [Queued, Running].include? self.state
+    if [Queued, Locked, Running].include? self.state
       # Update the priority of this container to the maximum priority of any of
       # its committed container requests and save the record.
-      max = 0
-      ContainerRequest.where(container_uuid: uuid).each do |cr|
-        if cr.state == ContainerRequest::Committed and cr.priority > max
-          max = cr.priority
-        end
-      end
-      self.priority = max
+      self.priority = ContainerRequest.
+        where(container_uuid: uuid,
+              state: ContainerRequest::Committed).
+        maximum('priority')
       self.save!
     end
   end
 
-  def locked_by_uuid
-    # Stub to permit a single dispatch to recognize its own containers
-    if current_user.is_admin
-      Thread.current[:api_client_authorization].andand.uuid
-    end
-  end
-
   protected
 
   def fill_field_defaults
@@ -151,6 +145,52 @@ class Container < ArvadosModel
     check_update_whitelist permitted
   end
 
+  def validate_lock
+    if locked_by_uuid_was
+      if locked_by_uuid_was != Thread.current[:api_client_authorization].uuid
+        # Notably, prohibit changing state or locked_by_uuid:
+        check_update_whitelist [:priority]
+      end
+    end
+
+    if [Locked, Running].include? self.state
+      need_lock = Thread.current[:api_client_authorization].uuid
+    else
+      need_lock = nil
+    end
+    if self.locked_by_uuid_changed?
+      if self.locked_by_uuid != need_lock
+        return errors.add :locked_by_uuid, "can only change to #{need_lock}"
+      end
+    end
+    self.locked_by_uuid = need_lock
+  end
+
+  def assign_auth
+    if self.auth_uuid_changed?
+      return errors.add :auth_uuid, 'is readonly'
+    end
+    if not [Locked, Running].include? self.state
+      # don't need one
+      self.auth.andand.update_attributes(expires_at: db_current_time)
+      self.auth = nil
+      return
+    elsif self.auth
+      # already have one
+      return
+    end
+    cr = ContainerRequest.
+      where('container_uuid=? and priority>0', self.uuid).
+      order('priority desc').
+      first
+    if !cr
+      return errors.add :auth_uuid, "cannot be assigned because priority <= 0"
+    end
+    self.auth = ApiClientAuthorization.
+      create!(user_id: User.find_by_uuid(cr.modified_by_user_uuid).id,
+              api_client_id: 0)
+  end
+
   def handle_completed
     # This container is finished so finalize any associated container requests
     # that are associated with this container.
diff --git a/services/api/app/models/container_request.rb b/services/api/app/models/container_request.rb
index acb751c..6353132 100644
--- a/services/api/app/models/container_request.rb
+++ b/services/api/app/models/container_request.rb
@@ -78,32 +78,32 @@ class ContainerRequest < ArvadosModel
     self.cwd ||= "."
   end
 
-  # Turn a container request into a container.
+  # Create a new container (or find an existing one) to satisfy this
+  # request.
   def resolve
-    # In the future this will do things like resolve symbolic git and keep
-    # references to content addresses.
-    Container.create!({ :command => self.command,
-                        :container_image => self.container_image,
-                        :cwd => self.cwd,
-                        :environment => self.environment,
-                        :mounts => self.mounts,
-                        :output_path => self.output_path,
-                        :runtime_constraints => self.runtime_constraints })
+    # TODO: resolve symbolic git and keep references to content
+    # addresses.
+    c = act_as_system_user do
+      Container.create!(command: self.command,
+                        container_image: self.container_image,
+                        cwd: self.cwd,
+                        environment: self.environment,
+                        mounts: self.mounts,
+                        output_path: self.output_path,
+                        runtime_constraints: self.runtime_constraints)
+    end
+    self.container_uuid = c.uuid
   end
 
   def set_container
-    if self.container_uuid_changed?
-      if not current_user.andand.is_admin and not self.container_uuid.nil?
-        errors.add :container_uuid, "can only be updated to nil."
-      end
-    else
-      if self.state_changed?
-        if self.state == Committed and (self.state_was == Uncommitted or self.state_was.nil?)
-          act_as_system_user do
-            self.container_uuid = self.resolve.andand.uuid
-          end
-        end
-      end
+    if (container_uuid_changed? and
+        not current_user.andand.is_admin and
+        not container_uuid.nil?)
+      errors.add :container_uuid, "can only be updated to nil."
+      return false
+    end
+    if state_changed? and state == Committed and container_uuid.nil?
+      resolve
     end
   end
 
@@ -158,16 +158,14 @@ class ContainerRequest < ArvadosModel
   end
 
   def update_priority
-    if [Committed, Final].include? self.state and (self.state_changed? or
-                                                   self.priority_changed? or
-                                                   self.container_uuid_changed?)
-      [self.container_uuid_was, self.container_uuid].each do |cuuid|
-        unless cuuid.nil?
-          c = Container.find_by_uuid cuuid
-          act_as_system_user do
-            c.update_priority!
-          end
-        end
+    if self.state_changed? or
+        self.priority_changed? or
+        self.container_uuid_changed?
+      act_as_system_user do
+        Container.
+          where('uuid in (?)',
+                [self.container_uuid_was, self.container_uuid].compact).
+          map(&:update_priority!)
       end
     end
   end
diff --git a/services/api/config/routes.rb b/services/api/config/routes.rb
index c85a3fc..4cea874 100644
--- a/services/api/config/routes.rb
+++ b/services/api/config/routes.rb
@@ -28,7 +28,9 @@ Server::Application.routes.draw do
       end
       resources :humans
       resources :job_tasks
-      resources :containers
+      resources :containers do
+        get 'auth', on: :member
+      end
       resources :container_requests
       resources :jobs do
         get 'queue', on: :collection
diff --git a/services/api/db/migrate/20160506175108_add_auths_to_container.rb b/services/api/db/migrate/20160506175108_add_auths_to_container.rb
new file mode 100644
index 0000000..d714a49
--- /dev/null
+++ b/services/api/db/migrate/20160506175108_add_auths_to_container.rb
@@ -0,0 +1,6 @@
+class AddAuthsToContainer < ActiveRecord::Migration
+  def change
+    add_column :containers, :auth_uuid, :string
+    add_column :containers, :locked_by_uuid, :string
+  end
+end
diff --git a/services/api/db/migrate/20160509143250_add_auth_and_lock_to_container_index.rb b/services/api/db/migrate/20160509143250_add_auth_and_lock_to_container_index.rb
new file mode 100644
index 0000000..4329ac0
--- /dev/null
+++ b/services/api/db/migrate/20160509143250_add_auth_and_lock_to_container_index.rb
@@ -0,0 +1,19 @@
+class AddAuthAndLockToContainerIndex < ActiveRecord::Migration
+  Columns_were = ["uuid", "owner_uuid", "modified_by_client_uuid", "modified_by_user_uuid", "state", "log", "cwd", "output_path", "output", "container_image"]
+  Columns = Columns_were + ["auth_uuid", "locked_by_uuid"]
+  def up
+    begin
+      remove_index :containers, :name => 'containers_search_index'
+    rescue
+    end
+    add_index(:containers, Columns, name: "containers_search_index")
+  end
+
+  def down
+    begin
+      remove_index :containers, :name => 'containers_search_index'
+    rescue
+    end
+    add_index(:containers, Columns_were, name: "containers_search_index")
+  end
+end
diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql
index 3ec420c..4bf4a17 100644
--- a/services/api/db/structure.sql
+++ b/services/api/db/structure.sql
@@ -339,7 +339,9 @@ CREATE TABLE containers (
     progress double precision,
     priority integer,
     updated_at timestamp without time zone NOT NULL,
-    exit_code integer
+    exit_code integer,
+    auth_uuid character varying(255),
+    locked_by_uuid character varying(255)
 );
 
 
@@ -1472,7 +1474,7 @@ CREATE INDEX container_requests_search_index ON container_requests USING btree (
 -- Name: containers_search_index; Type: INDEX; Schema: public; Owner: -; Tablespace: 
 --
 
-CREATE INDEX containers_search_index ON containers USING btree (uuid, owner_uuid, modified_by_client_uuid, modified_by_user_uuid, state, log, cwd, output_path, output, container_image);
+CREATE INDEX containers_search_index ON containers USING btree (uuid, owner_uuid, modified_by_client_uuid, modified_by_user_uuid, state, log, cwd, output_path, output, container_image, auth_uuid, locked_by_uuid);
 
 
 --
@@ -2583,4 +2585,8 @@ INSERT INTO schema_migrations (version) VALUES ('20160208210629');
 
 INSERT INTO schema_migrations (version) VALUES ('20160209155729');
 
-INSERT INTO schema_migrations (version) VALUES ('20160324144017');
\ No newline at end of file
+INSERT INTO schema_migrations (version) VALUES ('20160324144017');
+
+INSERT INTO schema_migrations (version) VALUES ('20160506175108');
+
+INSERT INTO schema_migrations (version) VALUES ('20160509143250');
\ No newline at end of file
diff --git a/services/api/lib/current_api_client.rb b/services/api/lib/current_api_client.rb
index 2e78612..fbd4ef5 100644
--- a/services/api/lib/current_api_client.rb
+++ b/services/api/lib/current_api_client.rb
@@ -124,12 +124,18 @@ module CurrentApiClient
   end
 
   def act_as_user user
+    #auth_was = Thread.current[:api_client_authorization]
     user_was = Thread.current[:user]
     Thread.current[:user] = user
+    #Thread.current[:api_client_authorization] = ApiClientAuthorization.
+    #  where('user_id=? and scopes is null', user.id).
+    #  order('expires_at desc').
+    #  first
     begin
       yield
     ensure
       Thread.current[:user] = user_was
+      #Thread.current[:api_client_authorization] = auth_was
     end
   end
 
diff --git a/services/api/test/fixtures/api_client_authorizations.yml b/services/api/test/fixtures/api_client_authorizations.yml
index 485b6d1..d6c0e40 100644
--- a/services/api/test/fixtures/api_client_authorizations.yml
+++ b/services/api/test/fixtures/api_client_authorizations.yml
@@ -273,7 +273,7 @@ fuse:
 
 dispatch1:
   uuid: zzzzz-gj3su-k9dvestay1plssr
-  api_client: trusted
+  api_client: untrusted
   user: system_user
   api_token: kwi8oowusvbutahacwk2geulqewy5oaqmpalczfna4b6bb0hfw
   expires_at: 2038-01-01 00:00:00
\ No newline at end of file
diff --git a/services/api/test/fixtures/container_requests.yml b/services/api/test/fixtures/container_requests.yml
new file mode 100644
index 0000000..c9f3427
--- /dev/null
+++ b/services/api/test/fixtures/container_requests.yml
@@ -0,0 +1,13 @@
+queued:
+  owner_uuid: zzzzz-tpzed-xurymjxw79nv3jz
+  state: Committed
+  priority: 1
+  created_at: 2016-01-11 11:11:11.111111111 Z
+  updated_at: 2016-01-11 11:11:11.111111111 Z
+  modified_at: 2016-01-11 11:11:11.111111111 Z
+  modified_by_user_uuid: zzzzz-tpzed-xurymjxw79nv3jz
+  container_image: test
+  cwd: test
+  output_path: test
+  command: ["echo", "hello"]
+  container_uuid: zzzzz-dz642-queuedcontainer
diff --git a/services/api/test/fixtures/containers.yml b/services/api/test/fixtures/containers.yml
index aa7ad31..b804c80 100644
--- a/services/api/test/fixtures/containers.yml
+++ b/services/api/test/fixtures/containers.yml
@@ -1,6 +1,6 @@
 queued:
   uuid: zzzzz-dz642-queuedcontainer
-  owner_uuid: zzzzz-tpzed-d9tiejq69daie8f
+  owner_uuid: zzzzz-tpzed-000000000000000
   state: Queued
   priority: 1
   created_at: 2016-01-11 11:11:11.111111111 Z
@@ -16,7 +16,7 @@ queued:
 
 completed:
   uuid: zzzzz-dz642-compltcontainer
-  owner_uuid: zzzzz-tpzed-d9tiejq69daie8f
+  owner_uuid: zzzzz-tpzed-000000000000000
   state: Complete
   priority: 1
   created_at: 2016-01-11 11:11:11.111111111 Z
diff --git a/services/api/test/functional/arvados/v1/containers_controller_test.rb b/services/api/test/functional/arvados/v1/containers_controller_test.rb
new file mode 100644
index 0000000..d9f7d96
--- /dev/null
+++ b/services/api/test/functional/arvados/v1/containers_controller_test.rb
@@ -0,0 +1,52 @@
+require 'test_helper'
+
+class Arvados::V1::ContainersControllerTest < ActionController::TestCase
+  test 'create' do
+    authorize_with :system_user
+    post :create, {
+      container: {
+        command: ['echo', 'hello'],
+        container_image: 'test',
+        output_path: 'test',
+      },
+    }
+    assert_response :success
+  end
+
+  [Container::Queued, Container::Complete].each do |state|
+    test "cannot get auth in #{state} state" do
+      authorize_with :dispatch1
+      get :auth, id: containers(:queued).uuid
+      assert_response 403
+    end
+  end
+
+  test 'cannot get auth with wrong token' do
+    authorize_with :dispatch1
+    c = containers(:queued)
+    assert c.update_attributes(state: Container::Locked), show_errors(c)
+
+    authorize_with :system_user
+    get :auth, id: c.uuid
+    assert_response 403
+  end
+
+  test 'get auth' do
+    authorize_with :dispatch1
+    c = containers(:queued)
+    assert c.update_attributes(state: Container::Locked), show_errors(c)
+    get :auth, id: c.uuid
+    assert_response :success
+    assert_operator 32, :<, json_response['api_token'].length
+    assert_equal 'arvados#apiClientAuthorization', json_response['kind']
+  end
+
+  test 'no auth in container response' do
+    authorize_with :dispatch1
+    c = containers(:queued)
+    assert c.update_attributes(state: Container::Locked), show_errors(c)
+    get :show, id: c.uuid
+    assert_response :success
+    assert_nil json_response['auth']
+  end
+end
diff --git a/services/api/test/test_helper.rb b/services/api/test/test_helper.rb
index 25ab286..ef08c72 100644
--- a/services/api/test/test_helper.rb
+++ b/services/api/test/test_helper.rb
@@ -36,6 +36,10 @@ module ArvadosTestSupport
   def auth(api_client_auth_name)
     {'HTTP_AUTHORIZATION' => "OAuth2 #{api_token(api_client_auth_name)}"}
   end
+
+  def show_errors model
+    return lambda { model.errors.full_messages.inspect }
+  end
 end
 
 class ActiveSupport::TestCase
diff --git a/services/api/test/unit/container_test.rb b/services/api/test/unit/container_test.rb
index 84713c2..9cc0981 100644
--- a/services/api/test/unit/container_test.rb
+++ b/services/api/test/unit/container_test.rb
@@ -1,22 +1,34 @@
 require 'test_helper'
 
 class ContainerTest < ActiveSupport::TestCase
-  def minimal_new
-    c = Container.new
-    c.command = ["echo", "foo"]
-    c.container_image = "img"
-    c.output_path = "/tmp"
-    c
-  end
-
-  def show_errors c
-    return lambda { c.errors.full_messages.inspect }
+  include DbCurrentTime
+
+  DEFAULT_ATTRS = {
+    command: ['echo', 'foo'],
+    container_image: 'img',
+    output_path: '/tmp',
+    priority: 1,
+  }
+
+  def minimal_new attrs={}
+    cr = ContainerRequest.new DEFAULT_ATTRS.merge(attrs)
+    act_as_user users(:active) do
+      cr.save!
+    end
+    c = Container.new DEFAULT_ATTRS.merge(attrs)
+    act_as_system_user do
+      c.save!
+      assert cr.update_attributes(container_uuid: c.uuid,
+                                  state: ContainerRequest::Committed,
+                                  ), show_errors(cr)
+    end
+    return c, cr
   end
 
   def check_illegal_updates c, bad_updates
     bad_updates.each do |u|
       refute c.update_attributes(u), u.inspect
-      refute c.valid?
+      refute c.valid?, u.inspect
       c.reload
     end
   end
@@ -28,6 +40,8 @@ class ContainerTest < ActiveSupport::TestCase
                               {environment: {"FOO" => "BAR"}},
                               {mounts: {"FOO" => "BAR"}},
                               {output_path: "/tmp3"},
+                              {locked_by_uuid: "zzzzz-gj3su-027z32aux8dg2s1"},
+                              {auth_uuid: "zzzzz-gj3su-017z32aux8dg2s1"},
                               {runtime_constraints: {"FOO" => "BAR"}}]
   end
 
@@ -39,7 +53,6 @@ class ContainerTest < ActiveSupport::TestCase
   def check_no_change_from_cancelled c
     check_illegal_modify c
     check_bogus_states c
-
     check_illegal_updates c, [{ priority: 3 },
                               { state: Container::Queued },
                               { state: Container::Locked },
@@ -49,13 +62,11 @@ class ContainerTest < ActiveSupport::TestCase
 
   test "Container create" do
     act_as_system_user do
-      c = minimal_new
-      c.environment = {}
-      c.mounts = {"BAR" => "FOO"}
-      c.output_path = "/tmp"
-      c.priority = 1
-      c.runtime_constraints = {}
-      c.save!
+      c, _ = minimal_new(environment: {},
+                      mounts: {"BAR" => "FOO"},
+                      output_path: "/tmp",
+                      priority: 1,
+                      runtime_constraints: {})
 
       check_illegal_modify c
       check_bogus_states c
@@ -67,89 +78,100 @@ class ContainerTest < ActiveSupport::TestCase
   end
 
   test "Container running" do
-    act_as_system_user do
-      c = minimal_new
-      c.save!
+    c, _ = minimal_new priority: 1
 
-      check_illegal_updates c, [{state: Container::Running},
-                                {state: Container::Complete}]
+    set_user_from_auth :dispatch1
+    check_illegal_updates c, [{state: Container::Running},
+                              {state: Container::Complete}]
 
-      c.update_attributes! state: Container::Locked
-      c.update_attributes! state: Container::Running
+    c.update_attributes! state: Container::Locked
+    c.update_attributes! state: Container::Running
 
-      check_illegal_modify c
-      check_bogus_states c
+    check_illegal_modify c
+    check_bogus_states c
 
-      check_illegal_updates c, [{state: Container::Queued}]
-      c.reload
+    check_illegal_updates c, [{state: Container::Queued}]
+    c.reload
 
-      c.update_attributes! priority: 3
-    end
+    c.update_attributes! priority: 3
   end
 
   test "Lock and unlock" do
-    act_as_system_user do
-      c = minimal_new
-      c.save!
-      assert_equal Container::Queued, c.state
+    c, cr = minimal_new priority: 0
 
-      refute c.update_attributes(state: Container::Running), "not locked"
-      c.reload
-      refute c.update_attributes(state: Container::Complete), "not locked"
-      c.reload
+    set_user_from_auth :dispatch1
+    assert_equal Container::Queued, c.state
 
-      assert c.update_attributes(state: Container::Locked), show_errors(c)
-      assert c.update_attributes(state: Container::Queued), show_errors(c)
+    refute c.update_attributes(state: Container::Locked), "no priority"
+    c.reload
+    assert cr.update_attributes priority: 1
 
-      refute c.update_attributes(state: Container::Running), "not locked"
-      c.reload
+    refute c.update_attributes(state: Container::Running), "not locked"
+    c.reload
+    refute c.update_attributes(state: Container::Complete), "not locked"
+    c.reload
 
-      assert c.update_attributes(state: Container::Locked), show_errors(c)
-      assert c.update_attributes(state: Container::Running), show_errors(c)
+    assert c.update_attributes(state: Container::Locked), show_errors(c)
+    assert c.locked_by_uuid
+    assert c.auth_uuid
 
-      refute c.update_attributes(state: Container::Locked), "already running"
-      c.reload
-      refute c.update_attributes(state: Container::Queued), "already running"
-      c.reload
+    assert c.update_attributes(state: Container::Queued), show_errors(c)
+    refute c.locked_by_uuid
+    refute c.auth_uuid
 
-      assert c.update_attributes(state: Container::Complete), show_errors(c)
-    end
+    refute c.update_attributes(state: Container::Running), "not locked"
+    c.reload
+    refute c.locked_by_uuid
+    refute c.auth_uuid
+
+    assert c.update_attributes(state: Container::Locked), show_errors(c)
+    assert c.update_attributes(state: Container::Running), show_errors(c)
+    assert c.locked_by_uuid
+    assert c.auth_uuid
+
+    auth_uuid_was = c.auth_uuid
+
+    refute c.update_attributes(state: Container::Locked), "already running"
+    c.reload
+    refute c.update_attributes(state: Container::Queued), "already running"
+    c.reload
+
+    assert c.update_attributes(state: Container::Complete), show_errors(c)
+    refute c.locked_by_uuid
+    refute c.auth_uuid
+
+    auth_exp = ApiClientAuthorization.find_by_uuid(auth_uuid_was).expires_at
+    assert_operator auth_exp, :<, db_current_time
   end
 
   test "Container queued cancel" do
-    act_as_system_user do
-      c = minimal_new
-      c.save!
-      assert c.update_attributes(state: Container::Cancelled), show_errors(c)
-      check_no_change_from_cancelled c
-    end
+    c, _ = minimal_new
+    set_user_from_auth :dispatch1
+    assert c.update_attributes(state: Container::Cancelled), show_errors(c)
+    check_no_change_from_cancelled c
   end
 
   test "Container locked cancel" do
-    act_as_system_user do
-      c = minimal_new
-      c.save!
-      assert c.update_attributes(state: Container::Locked), show_errors(c)
-      assert c.update_attributes(state: Container::Cancelled), show_errors(c)
-      check_no_change_from_cancelled c
-    end
+    c, _ = minimal_new
+    set_user_from_auth :dispatch1
+    assert c.update_attributes(state: Container::Locked), show_errors(c)
+    assert c.update_attributes(state: Container::Cancelled), show_errors(c)
+    check_no_change_from_cancelled c
   end
 
   test "Container running cancel" do
-    act_as_system_user do
-      c = minimal_new
-      c.save!
-      c.update_attributes! state: Container::Queued
-      c.update_attributes! state: Container::Locked
-      c.update_attributes! state: Container::Running
-      c.update_attributes! state: Container::Cancelled
-      check_no_change_from_cancelled c
-    end
+    c, _ = minimal_new
+    set_user_from_auth :dispatch1
+    c.update_attributes! state: Container::Queued
+    c.update_attributes! state: Container::Locked
+    c.update_attributes! state: Container::Running
+    c.update_attributes! state: Container::Cancelled
+    check_no_change_from_cancelled c
   end
 
   test "Container create forbidden for non-admin" do
     set_user_from_auth :active_trustedclient
-    c = minimal_new
+    c = Container.new DEFAULT_ATTRS
     c.environment = {}
     c.mounts = {"BAR" => "FOO"}
     c.output_path = "/tmp"
@@ -161,16 +183,14 @@ class ContainerTest < ActiveSupport::TestCase
   end
 
   test "Container only set exit code on complete" do
-    act_as_system_user do
-      c = minimal_new
-      c.save!
-      c.update_attributes! state: Container::Locked
-      c.update_attributes! state: Container::Running
+    c, _ = minimal_new
+    set_user_from_auth :dispatch1
+    c.update_attributes! state: Container::Locked
+    c.update_attributes! state: Container::Running
 
-      check_illegal_updates c, [{exit_code: 1},
-                                {exit_code: 1, state: Container::Cancelled}]
+    check_illegal_updates c, [{exit_code: 1},
+                              {exit_code: 1, state: Container::Cancelled}]
 
-      assert c.update_attributes(exit_code: 1, state: Container::Complete)
-    end
+    assert c.update_attributes(exit_code: 1, state: Container::Complete)
   end
 end

commit 8e5206a5b1910ee7bc1d0a45af754ce507a7f237
Author: Tom Clegg <tom at curoverse.com>
Date:   Thu May 5 17:50:44 2016 -0400

    8128: Update crunch-dispatch-local to use new Locked state.

diff --git a/services/crunch-dispatch-local/crunch-dispatch-local.go b/services/crunch-dispatch-local/crunch-dispatch-local.go
index e05c0c5..848d723 100644
--- a/services/crunch-dispatch-local/crunch-dispatch-local.go
+++ b/services/crunch-dispatch-local/crunch-dispatch-local.go
@@ -72,7 +72,7 @@ func doMain() error {
 	}(sigChan)
 
 	// Run all queued containers
-	runQueuedContainers(*pollInterval, *priorityPollInterval, *crunchRunCommand)
+	runQueuedContainers(time.Duration(*pollInterval)*time.Second, time.Duration(*priorityPollInterval)*time.Second, *crunchRunCommand)
 
 	// Finished dispatching; interrupt any crunch jobs that are still running
 	for _, cmd := range runningCmds {
@@ -91,8 +91,8 @@ func doMain() error {
 // Any errors encountered are logged but the program would continue to run (not exit).
 // This is because, once one or more crunch jobs are running,
 // we would need to wait for them complete.
-func runQueuedContainers(pollInterval, priorityPollInterval int, crunchRunCommand string) {
-	ticker := time.NewTicker(time.Duration(pollInterval) * time.Second)
+func runQueuedContainers(pollInterval, priorityPollInterval time.Duration, crunchRunCommand string) {
+	ticker := time.NewTicker(pollInterval)
 
 	for {
 		select {
@@ -107,9 +107,10 @@ func runQueuedContainers(pollInterval, priorityPollInterval int, crunchRunComman
 
 // Container data
 type Container struct {
-	UUID     string `json:"uuid"`
-	State    string `json:"state"`
-	Priority int    `json:"priority"`
+	UUID         string `json:"uuid"`
+	State        string `json:"state"`
+	Priority     int    `json:"priority"`
+	LockedByUUID string `json:"locked_by_uuid"`
 }
 
 // ContainerList is a list of the containers from api
@@ -118,7 +119,7 @@ type ContainerList struct {
 }
 
 // Get the list of queued containers from API server and invoke run for each container.
-func dispatchLocal(priorityPollInterval int, crunchRunCommand string) {
+func dispatchLocal(pollInterval time.Duration, crunchRunCommand string) {
 	params := arvadosclient.Dict{
 		"filters": [][]string{[]string{"state", "=", "Queued"}},
 	}
@@ -133,88 +134,117 @@ func dispatchLocal(priorityPollInterval int, crunchRunCommand string) {
 	for i := 0; i < len(containers.Items); i++ {
 		log.Printf("About to run queued container %v", containers.Items[i].UUID)
 		// Run the container
-		go run(containers.Items[i].UUID, crunchRunCommand, priorityPollInterval)
+		go run(containers.Items[i].UUID, crunchRunCommand, pollInterval)
 	}
 }
 
+func updateState(uuid, newState string) error {
+	err := arv.Update("containers", uuid,
+		arvadosclient.Dict{
+			"container": arvadosclient.Dict{"state": newState}},
+		nil)
+	if err != nil {
+		log.Printf("Error updating container %s to '%s' state: %q", uuid, newState, err)
+	}
+	return err
+}
+
 // Run queued container:
-// Set container state to locked (TBD)
+// Set container state to Locked
 // Run container using the given crunch-run command
 // Set the container state to Running
 // If the container priority becomes zero while crunch job is still running, terminate it.
-func run(uuid string, crunchRunCommand string, priorityPollInterval int) {
-	cmd := exec.Command(crunchRunCommand, uuid)
+func run(uuid string, crunchRunCommand string, pollInterval time.Duration) {
+	if err := updateState(uuid, "Locked"); err != nil {
+		return
+	}
 
+	cmd := exec.Command(crunchRunCommand, uuid)
 	cmd.Stdin = nil
 	cmd.Stderr = os.Stderr
 	cmd.Stdout = os.Stderr
+
+	// Add this crunch job to the list of runningCmds only if we
+	// succeed in starting crunch-run.
+	runningCmdsMutex.Lock()
 	if err := cmd.Start(); err != nil {
-		log.Printf("Error running container for %v: %q", uuid, err)
+		log.Printf("Error starting crunch-run for %v: %q", uuid, err)
+		runningCmdsMutex.Unlock()
+		updateState(uuid, "Queued")
 		return
 	}
-
-	// Add this crunch job to the list of runningCmds
-	runningCmdsMutex.Lock()
 	runningCmds[uuid] = cmd
 	runningCmdsMutex.Unlock()
 
-	log.Printf("Started container run for %v", uuid)
+	defer func() {
+		setFinalState(uuid)
+
+		// Remove the crunch job from runningCmds
+		runningCmdsMutex.Lock()
+		delete(runningCmds, uuid)
+		runningCmdsMutex.Unlock()
+	}()
+
+	log.Printf("Starting container %v", uuid)
 
 	// Add this crunch job to waitGroup
 	waitGroup.Add(1)
 	defer waitGroup.Done()
 
-	// Update container status to Running
-	err := arv.Update("containers", uuid,
-		arvadosclient.Dict{
-			"container": arvadosclient.Dict{"state": "Running"}},
-		nil)
-	if err != nil {
-		log.Printf("Error updating container state to 'Running' for %v: %q", uuid, err)
-	}
+	updateState(uuid, "Running")
+
+	cmdExited := make(chan struct{})
 
-	// A goroutine to terminate the runner if container priority becomes zero
-	priorityTicker := time.NewTicker(time.Duration(priorityPollInterval) * time.Second)
+	// Kill the child process if container priority changes to zero
 	go func() {
-		for _ = range priorityTicker.C {
+		ticker := time.NewTicker(pollInterval)
+		defer ticker.Stop()
+		for {
+			select {
+			case <-cmdExited:
+				return
+			case <-ticker.C:
+			}
 			var container Container
 			err := arv.Get("containers", uuid, nil, &container)
 			if err != nil {
-				log.Printf("Error getting container info for %v: %q", uuid, err)
-			} else {
-				if container.Priority == 0 {
-					priorityTicker.Stop()
-					cmd.Process.Signal(os.Interrupt)
-				}
+				log.Printf("Error getting container %v: %q", uuid, err)
+				continue
+			}
+			if container.Priority == 0 {
+				log.Printf("Sending SIGINT to pid %d to cancel container %v", cmd.Process.Pid, uuid)
+				cmd.Process.Signal(os.Interrupt)
 			}
 		}
 	}()
 
-	// Wait for the crunch job to exit
+	// Wait for crunch-run to exit
 	if _, err := cmd.Process.Wait(); err != nil {
 		log.Printf("Error while waiting for crunch job to finish for %v: %q", uuid, err)
 	}
+	close(cmdExited)
 
-	// Remove the crunch job to runningCmds
-	runningCmdsMutex.Lock()
-	delete(runningCmds, uuid)
-	runningCmdsMutex.Unlock()
-
-	priorityTicker.Stop()
+	log.Printf("Finished container run for %v", uuid)
+}
 
-	// The container state should be 'Complete'
+func setFinalState(uuid string) {
+	// The container state should now be 'Complete' if everything
+	// went well. If it started but crunch-run didn't change its
+	// final state to 'Running', fix that now. If it never even
+	// started, cancel it as unrunnable. (TODO: Requeue instead,
+	// and fix tests so they can tell something happened even if
+	// the final state is Queued.)
 	var container Container
-	err = arv.Get("containers", uuid, nil, &container)
-	if container.State == "Running" {
-		log.Printf("After crunch-run process termination, the state is still 'Running' for %v. Updating it to 'Complete'", uuid)
-		err = arv.Update("containers", uuid,
-			arvadosclient.Dict{
-				"container": arvadosclient.Dict{"state": "Complete"}},
-			nil)
-		if err != nil {
-			log.Printf("Error updating container state to Complete for %v: %q", uuid, err)
-		}
+	err := arv.Get("containers", uuid, nil, &container)
+	if err != nil {
+		log.Printf("Error getting final container state: %v", err)
+	}
+	fixState := map[string]string{
+		"Running": "Complete",
+		"Locked": "Cancelled",
+	}
+	if newState, ok := fixState[container.State]; ok {
+		log.Printf("After crunch-run process termination, the state is still '%s' for %v. Updating it to '%s'", container.State, uuid, newState)
+		updateState(uuid, newState)
 	}
-
-	log.Printf("Finished container run for %v", uuid)
 }
diff --git a/services/crunch-dispatch-local/crunch-dispatch-local_test.go b/services/crunch-dispatch-local/crunch-dispatch-local_test.go
index 3ec1e2e..e3ab3a4 100644
--- a/services/crunch-dispatch-local/crunch-dispatch-local_test.go
+++ b/services/crunch-dispatch-local/crunch-dispatch-local_test.go
@@ -4,12 +4,11 @@ import (
 	"git.curoverse.com/arvados.git/sdk/go/arvadosclient"
 	"git.curoverse.com/arvados.git/sdk/go/arvadostest"
 
-	"io/ioutil"
+	"bytes"
 	"log"
 	"net/http"
 	"net/http/httptest"
 	"os"
-	"strings"
 	"syscall"
 	"testing"
 	"time"
@@ -101,7 +100,7 @@ func (s *MockArvadosServerSuite) Test_APIErrorUpdatingContainerState(c *C) {
 	apiStubResponses["/arvados/v1/containers/zzzzz-dz642-xxxxxxxxxxxxxx1"] =
 		arvadostest.StubResponse{500, string(`{}`)}
 
-	testWithServerStub(c, apiStubResponses, "echo", "Error updating container state")
+	testWithServerStub(c, apiStubResponses, "echo", "Error updating container zzzzz-dz642-xxxxxxxxxxxxxx1 to 'Locked' state")
 }
 
 func (s *MockArvadosServerSuite) Test_ContainerStillInRunningAfterRun(c *C) {
@@ -122,7 +121,7 @@ func (s *MockArvadosServerSuite) Test_ErrorRunningContainer(c *C) {
 	apiStubResponses["/arvados/v1/containers/zzzzz-dz642-xxxxxxxxxxxxxx3"] =
 		arvadostest.StubResponse{200, string(`{"uuid":"zzzzz-dz642-xxxxxxxxxxxxxx3", "state":"Running", "priority":1}`)}
 
-	testWithServerStub(c, apiStubResponses, "nosuchcommand", "Error running container for zzzzz-dz642-xxxxxxxxxxxxxx3")
+	testWithServerStub(c, apiStubResponses, "nosuchcommand", "Error starting crunch-run for zzzzz-dz642-xxxxxxxxxxxxxx3")
 }
 
 func testWithServerStub(c *C, apiStubResponses map[string]arvadostest.StubResponse, crunchCmd string, expected string) {
@@ -139,21 +138,19 @@ func testWithServerStub(c *C, apiStubResponses map[string]arvadostest.StubRespon
 		Retries:   0,
 	}
 
-	tempfile, err := ioutil.TempFile(os.TempDir(), "temp-log-file")
-	c.Check(err, IsNil)
-	defer os.Remove(tempfile.Name())
-	log.SetOutput(tempfile)
+	buf := bytes.NewBuffer(nil)
+	log.SetOutput(buf)
+	defer log.SetOutput(os.Stderr)
 
 	go func() {
 		time.Sleep(2 * time.Second)
 		sigChan <- syscall.SIGTERM
 	}()
 
-	runQueuedContainers(1, 1, crunchCmd)
+	runQueuedContainers(time.Second, time.Second, crunchCmd)
 
 	// Wait for all running crunch jobs to complete / terminate
 	waitGroup.Wait()
 
-	buf, _ := ioutil.ReadFile(tempfile.Name())
-	c.Check(strings.Contains(string(buf), expected), Equals, true)
+	c.Check(buf.String(), Matches, `(?ms).*`+expected+`.*`)
 }

commit 55923446d946f11f03243ace3281b54ac5a4e80b
Author: Tom Clegg <tom at curoverse.com>
Date:   Thu May 5 17:15:51 2016 -0400

    8128: Update crunch-dispatch-slurm to use new Locked state.

diff --git a/sdk/go/arvadostest/fixtures.go b/sdk/go/arvadostest/fixtures.go
index bebef79..84a3bff 100644
--- a/sdk/go/arvadostest/fixtures.go
+++ b/sdk/go/arvadostest/fixtures.go
@@ -13,6 +13,9 @@ const (
 	FooBarDirCollection   = "zzzzz-4zz18-foonbarfilesdir"
 	FooPdh                = "1f4b0bc7583c2a7f9102c395f4ffc5e3+45"
 	HelloWorldPdh         = "55713e6a34081eb03609e7ad5fcad129+62"
+
+	Dispatch1Token    = "kwi8oowusvbutahacwk2geulqewy5oaqmpalczfna4b6bb0hfw"
+	Dispatch1AuthUUID = "zzzzz-gj3su-k9dvestay1plssr"
 )
 
 // A valid manifest designed to test various edge cases and parsing
diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb
index 5856edd..a014552 100644
--- a/services/api/app/models/container.rb
+++ b/services/api/app/models/container.rb
@@ -27,6 +27,7 @@ class Container < ArvadosModel
     t.add :environment
     t.add :exit_code
     t.add :finished_at
+    t.add :locked_by_uuid
     t.add :log
     t.add :mounts
     t.add :output
@@ -74,6 +75,13 @@ class Container < ArvadosModel
     end
   end
 
+  def locked_by_uuid
+    # Stub to permit a single dispatch to recognize its own containers
+    if current_user.is_admin
+      Thread.current[:api_client_authorization].andand.uuid
+    end
+  end
+
   protected
 
   def fill_field_defaults
diff --git a/services/api/test/fixtures/api_client_authorizations.yml b/services/api/test/fixtures/api_client_authorizations.yml
index f99a9fb..485b6d1 100644
--- a/services/api/test/fixtures/api_client_authorizations.yml
+++ b/services/api/test/fixtures/api_client_authorizations.yml
@@ -271,3 +271,9 @@ fuse:
   api_token: 4nagbkv8eap0uok7pxm72nossq5asihls3yn5p4xmvqx5t5e7p
   expires_at: 2038-01-01 00:00:00
 
+dispatch1:
+  uuid: zzzzz-gj3su-k9dvestay1plssr
+  api_client: trusted
+  user: system_user
+  api_token: kwi8oowusvbutahacwk2geulqewy5oaqmpalczfna4b6bb0hfw
+  expires_at: 2038-01-01 00:00:00
\ No newline at end of file
diff --git a/services/crunch-dispatch-slurm/crunch-dispatch-slurm.go b/services/crunch-dispatch-slurm/crunch-dispatch-slurm.go
index f45c2a1..8eefd35 100644
--- a/services/crunch-dispatch-slurm/crunch-dispatch-slurm.go
+++ b/services/crunch-dispatch-slurm/crunch-dispatch-slurm.go
@@ -1,6 +1,7 @@
 package main
 
 import (
+	"bufio"
 	"flag"
 	"fmt"
 	"git.curoverse.com/arvados.git/sdk/go/arvadosclient"
@@ -86,6 +87,15 @@ func doMain() error {
 	return nil
 }
 
+type apiClientAuthorization struct {
+	UUID     string `json:"uuid"`
+	APIToken string `json:"api_token"`
+}
+
+type apiClientAuthorizationList struct {
+	Items []apiClientAuthorization `json:"items"`
+}
+
 // Poll for queued containers using pollInterval.
 // Invoke dispatchSlurm for each ticker cycle, which will run all the queued containers.
 //
@@ -93,12 +103,21 @@ func doMain() error {
 // This is because, once one or more crunch jobs are running,
 // we would need to wait for them complete.
 func runQueuedContainers(pollInterval, priorityPollInterval int, crunchRunCommand, finishCommand string) {
-	ticker := time.NewTicker(time.Duration(pollInterval) * time.Second)
+	var authList apiClientAuthorizationList
+	err := arv.List("api_client_authorizations", map[string]interface{}{
+		"filters": [][]interface{}{{"api_token", "=", arv.ApiToken}},
+	}, &authList)
+	if err != nil || len(authList.Items) != 1 {
+		log.Printf("Error getting my token UUID: %v (%d)", err, len(authList.Items))
+		return
+	}
+	auth := authList.Items[0]
 
+	ticker := time.NewTicker(time.Duration(pollInterval) * time.Second)
 	for {
 		select {
 		case <-ticker.C:
-			dispatchSlurm(priorityPollInterval, crunchRunCommand, finishCommand)
+			dispatchSlurm(auth, time.Duration(priorityPollInterval)*time.Second, crunchRunCommand, finishCommand)
 		case <-doneProcessing:
 			ticker.Stop()
 			return
@@ -112,6 +131,7 @@ type Container struct {
 	State              string           `json:"state"`
 	Priority           int              `json:"priority"`
 	RuntimeConstraints map[string]int64 `json:"runtime_constraints"`
+	LockedByUUID       string           `json:"locked_by_uuid"`
 }
 
 // ContainerList is a list of the containers from api
@@ -119,10 +139,11 @@ type ContainerList struct {
 	Items []Container `json:"items"`
 }
 
-// Get the list of queued containers from API server and invoke run for each container.
-func dispatchSlurm(priorityPollInterval int, crunchRunCommand, finishCommand string) {
+// Get the list of queued containers from API server and invoke run
+// for each container.
+func dispatchSlurm(auth apiClientAuthorization, pollInterval time.Duration, crunchRunCommand, finishCommand string) {
 	params := arvadosclient.Dict{
-		"filters": [][]string{[]string{"state", "=", "Queued"}},
+		"filters": [][]interface{}{{"state", "in", []string{"Queued", "Locked"}}},
 	}
 
 	var containers ContainerList
@@ -132,16 +153,33 @@ func dispatchSlurm(priorityPollInterval int, crunchRunCommand, finishCommand str
 		return
 	}
 
-	for i := 0; i < len(containers.Items); i++ {
-		log.Printf("About to submit queued container %v", containers.Items[i].UUID)
-		// Run the container
-		go run(containers.Items[i], crunchRunCommand, finishCommand, priorityPollInterval)
+	for _, container := range containers.Items {
+		if container.State == "Locked" {
+			if container.LockedByUUID != auth.UUID {
+				// Locked by a different dispatcher
+				continue
+			} else if checkMine(container.UUID) {
+				// I already have a goroutine running
+				// for this container: it just hasn't
+				// gotten past Locked state yet.
+				continue
+			}
+			log.Printf("WARNING: found container %s already locked by my token %s, but I didn't submit it. "+
+				"Assuming it was left behind by a previous dispatch process, and waiting for it to finish.",
+				container.UUID, auth.UUID)
+			setMine(container.UUID, true)
+			go func() {
+				waitContainer(container, pollInterval)
+				setMine(container.UUID, false)
+			}()
+		}
+		go run(container, crunchRunCommand, finishCommand, pollInterval)
 	}
 }
 
 // sbatchCmd
 func sbatchFunc(container Container) *exec.Cmd {
-	memPerCPU := math.Ceil((float64(container.RuntimeConstraints["ram"])) / (float64(container.RuntimeConstraints["vcpus"]*1048576)))
+	memPerCPU := math.Ceil((float64(container.RuntimeConstraints["ram"])) / (float64(container.RuntimeConstraints["vcpus"] * 1048576)))
 	return exec.Command("sbatch", "--share", "--parsable",
 		"--job-name="+container.UUID,
 		"--mem-per-cpu="+strconv.Itoa(int(memPerCPU)),
@@ -162,17 +200,19 @@ var striggerCmd = striggerFunc
 func submit(container Container, crunchRunCommand string) (jobid string, submitErr error) {
 	submitErr = nil
 
-	// Mark record as complete if anything errors out.
 	defer func() {
-		if submitErr != nil {
-			// This really should be an "Error" state, see #8018
-			updateErr := arv.Update("containers", container.UUID,
-				arvadosclient.Dict{
-					"container": arvadosclient.Dict{"state": "Complete"}},
-				nil)
-			if updateErr != nil {
-				log.Printf("Error updating container state to 'Complete' for %v: %q", container.UUID, updateErr)
-			}
+		// If we didn't get as far as submitting a slurm job,
+		// unlock the container and return it to the queue.
+		if submitErr == nil {
+			// OK, no cleanup needed
+			return
+		}
+		err := arv.Update("containers", container.UUID,
+			arvadosclient.Dict{
+				"container": arvadosclient.Dict{"state": "Queued"}},
+			nil)
+		if err != nil {
+			log.Printf("Error unlocking container %s: %v", container.UUID, err)
 		}
 	}()
 
@@ -205,6 +245,7 @@ func submit(container Container, crunchRunCommand string) (jobid string, submitE
 	stdoutChan := make(chan []byte)
 	go func() {
 		b, _ := ioutil.ReadAll(stdoutReader)
+		stdoutReader.Close()
 		stdoutChan <- b
 		close(stdoutChan)
 	}()
@@ -212,6 +253,7 @@ func submit(container Container, crunchRunCommand string) (jobid string, submitE
 	stderrChan := make(chan []byte)
 	go func() {
 		b, _ := ioutil.ReadAll(stderrReader)
+		stderrReader.Close()
 		stderrChan <- b
 		close(stderrChan)
 	}()
@@ -246,18 +288,35 @@ func finalizeRecordOnFinish(jobid, containerUUID, finishCommand, apiHost, apiTok
 	err := cmd.Run()
 	if err != nil {
 		log.Printf("While setting up strigger: %v", err)
+		// BUG: we drop the error here and forget about it. A
+		// human has to notice the container is stuck in
+		// Running state, and fix it manually.
 	}
 }
 
-// Run a queued container.
-// Set container state to locked (TBD)
-// Submit job to slurm to execute crunch-run command for the container
-// If the container priority becomes zero while crunch job is still running, cancel the job.
-func run(container Container, crunchRunCommand, finishCommand string, priorityPollInterval int) {
+// Run a queued container: [1] Set container state to locked. [2]
+// Execute crunch-run as a slurm batch job. [3] waitContainer().
+func run(container Container, crunchRunCommand, finishCommand string, pollInterval time.Duration) {
+	setMine(container.UUID, true)
+	defer setMine(container.UUID, false)
+
+	// Update container status to Locked. This will fail if
+	// another dispatcher (token) has already locked it. It will
+	// succeed if *this* dispatcher has already locked it.
+	err := arv.Update("containers", container.UUID,
+		arvadosclient.Dict{
+			"container": arvadosclient.Dict{"state": "Locked"}},
+		nil)
+	if err != nil {
+		log.Printf("Error updating container state to 'Locked' for %v: %q", container.UUID, err)
+		return
+	}
+
+	log.Printf("About to submit queued container %v", container.UUID)
 
 	jobid, err := submit(container, crunchRunCommand)
 	if err != nil {
-		log.Printf("Error queuing container run: %v", err)
+		log.Printf("Error submitting container %s to slurm: %v", container.UUID, err)
 		return
 	}
 
@@ -267,9 +326,9 @@ func run(container Container, crunchRunCommand, finishCommand string, priorityPo
 	}
 	finalizeRecordOnFinish(jobid, container.UUID, finishCommand, arv.ApiServer, arv.ApiToken, insecure)
 
-	// Update container status to Running, this is a temporary workaround
-	// to avoid resubmitting queued containers because record locking isn't
-	// implemented yet.
+	// Update container status to Running. This will fail if
+	// another dispatcher (token) has already locked it. It will
+	// succeed if *this* dispatcher has already locked it.
 	err = arv.Update("containers", container.UUID,
 		arvadosclient.Dict{
 			"container": arvadosclient.Dict{"state": "Running"}},
@@ -277,31 +336,100 @@ func run(container Container, crunchRunCommand, finishCommand string, priorityPo
 	if err != nil {
 		log.Printf("Error updating container state to 'Running' for %v: %q", container.UUID, err)
 	}
+	log.Printf("Submitted container %v to slurm", container.UUID)
+	waitContainer(container, pollInterval)
+}
 
-	log.Printf("Submitted container run for %v", container.UUID)
-
-	containerUUID := container.UUID
+// Wait for a container to finish. Cancel the slurm job if the
+// container priority changes to zero before it ends.
+func waitContainer(container Container, pollInterval time.Duration) {
+	log.Printf("Monitoring container %v started", container.UUID)
+	defer log.Printf("Monitoring container %v finished", container.UUID)
+
+	pollTicker := time.NewTicker(pollInterval)
+	defer pollTicker.Stop()
+	for _ = range pollTicker.C {
+		var updated Container
+		err := arv.Get("containers", container.UUID, nil, &updated)
+		if err != nil {
+			log.Printf("Error getting container %s: %q", container.UUID, err)
+			continue
+		}
+		if updated.State == "Complete" || updated.State == "Cancelled" {
+			return
+		}
+		if updated.Priority != 0 {
+			continue
+		}
 
-	// A goroutine to terminate the runner if container priority becomes zero
-	priorityTicker := time.NewTicker(time.Duration(priorityPollInterval) * time.Second)
-	go func() {
-		for _ = range priorityTicker.C {
-			var container Container
-			err := arv.Get("containers", containerUUID, nil, &container)
-			if err != nil {
-				log.Printf("Error getting container info for %v: %q", container.UUID, err)
-			} else {
-				if container.Priority == 0 {
-					log.Printf("Canceling container %v", container.UUID)
-					priorityTicker.Stop()
-					cancelcmd := exec.Command("scancel", "--name="+container.UUID)
-					cancelcmd.Run()
-				}
-				if container.State == "Complete" {
-					priorityTicker.Stop()
-				}
+		// Priority is zero, but state is Running or Locked
+		log.Printf("Canceling container %s", container.UUID)
+
+		err = exec.Command("scancel", "--name="+container.UUID).Run()
+		if err != nil {
+			log.Printf("Error stopping container %s with scancel: %v", container.UUID, err)
+			if inQ, err := checkSqueue(container.UUID); err != nil {
+				log.Printf("Error running squeue: %v", err)
+				continue
+			} else if inQ {
+				log.Printf("Container %s is still in squeue; will retry", container.UUID)
+				continue
 			}
 		}
-	}()
 
+		err = arv.Update("containers", container.UUID,
+			arvadosclient.Dict{
+				"container": arvadosclient.Dict{"state": "Cancelled"}},
+			nil)
+		if err != nil {
+			log.Printf("Error updating state for container %s: %s", container.UUID, err)
+			continue
+		}
+
+		return
+	}
+}
+
+func checkSqueue(uuid string) (bool, error) {
+	cmd := exec.Command("squeue", "--format=%j")
+	sq, err := cmd.StdoutPipe()
+	if err != nil {
+		return false, err
+	}
+	cmd.Start()
+	defer cmd.Wait()
+	scanner := bufio.NewScanner(sq)
+	found := false
+	for scanner.Scan() {
+		if scanner.Text() == uuid {
+			found = true
+		}
+	}
+	if err := scanner.Err(); err != nil {
+		return false, err
+	}
+	return found, nil
+}
+
+var mineMutex sync.RWMutex
+var mineMap = make(map[string]bool)
+
+// Goroutine-safely add/remove uuid to the set of "my" containers,
+// i.e., ones for which this process has a goroutine running.
+func setMine(uuid string, t bool) {
+	mineMutex.Lock()
+	if t {
+		mineMap[uuid] = true
+	} else {
+		delete(mineMap, uuid)
+	}
+	mineMutex.Unlock()
+}
+
+// Check whether there is already a goroutine running for this
+// container.
+func checkMine(uuid string) bool {
+	mineMutex.RLocker().Lock()
+	defer mineMutex.RLocker().Unlock()
+	return mineMap[uuid]
 }
diff --git a/services/crunch-dispatch-slurm/crunch-dispatch-slurm_test.go b/services/crunch-dispatch-slurm/crunch-dispatch-slurm_test.go
index e58b9e4..7fd20c1 100644
--- a/services/crunch-dispatch-slurm/crunch-dispatch-slurm_test.go
+++ b/services/crunch-dispatch-slurm/crunch-dispatch-slurm_test.go
@@ -4,8 +4,8 @@ import (
 	"git.curoverse.com/arvados.git/sdk/go/arvadosclient"
 	"git.curoverse.com/arvados.git/sdk/go/arvadostest"
 
+	"bytes"
 	"fmt"
-	"io/ioutil"
 	"log"
 	"math"
 	"net/http"
@@ -52,6 +52,7 @@ func (s *TestSuite) SetUpTest(c *C) {
 	if err != nil {
 		c.Fatalf("Error making arvados client: %s", err)
 	}
+	os.Setenv("ARVADOS_API_TOKEN", arvadostest.Dispatch1Token)
 }
 
 func (s *TestSuite) TearDownTest(c *C) {
@@ -89,10 +90,12 @@ func (s *TestSuite) Test_doMain(c *C) {
 			apiHost, apiToken, apiInsecure).Args
 		go func() {
 			time.Sleep(5 * time.Second)
-			arv.Update("containers", containerUUID,
-				arvadosclient.Dict{
-					"container": arvadosclient.Dict{"state": "Complete"}},
-				nil)
+			for _, state := range []string{"Running", "Complete"} {
+				arv.Update("containers", containerUUID,
+					arvadosclient.Dict{
+						"container": arvadosclient.Dict{"state": state}},
+					nil)
+			}
 		}()
 		return exec.Command("echo", "strigger")
 	}
@@ -122,7 +125,7 @@ func (s *TestSuite) Test_doMain(c *C) {
 	c.Check(sbatchCmdLine, DeepEquals, sbatchCmdComps)
 
 	c.Check(striggerCmdLine, DeepEquals, []string{"strigger", "--set", "--jobid=zzzzz-dz642-queuedcontainer\n", "--fini",
-		"--program=/usr/bin/crunch-finish-slurm.sh " + os.Getenv("ARVADOS_API_HOST") + " 4axaw8zxe0qm22wa6urpp5nskcne8z88cvbupv653y1njyi05h 1 zzzzz-dz642-queuedcontainer"})
+		"--program=/usr/bin/crunch-finish-slurm.sh " + os.Getenv("ARVADOS_API_HOST") + " " + arvadostest.Dispatch1Token + " 1 zzzzz-dz642-queuedcontainer"})
 
 	// There should be no queued containers now
 	err = arv.List("containers", params, &containers)
@@ -138,6 +141,7 @@ func (s *TestSuite) Test_doMain(c *C) {
 
 func (s *MockArvadosServerSuite) Test_APIErrorGettingContainers(c *C) {
 	apiStubResponses := make(map[string]arvadostest.StubResponse)
+	apiStubResponses["/arvados/v1/api_client_authorizations"] = arvadostest.StubResponse{200, string(`{"items":[{"uuid":"` + arvadostest.Dispatch1AuthUUID + `"}]}`)}
 	apiStubResponses["/arvados/v1/containers"] = arvadostest.StubResponse{500, string(`{}`)}
 
 	testWithServerStub(c, apiStubResponses, "echo", "Error getting list of queued containers")
@@ -157,18 +161,18 @@ func testWithServerStub(c *C, apiStubResponses map[string]arvadostest.StubRespon
 		Retries:   0,
 	}
 
-	tempfile, err := ioutil.TempFile(os.TempDir(), "temp-log-file")
-	c.Check(err, IsNil)
-	defer os.Remove(tempfile.Name())
-	log.SetOutput(tempfile)
+	buf := bytes.NewBuffer(nil)
+	log.SetOutput(buf)
+	defer log.SetOutput(os.Stderr)
 
 	go func() {
-		time.Sleep(2 * time.Second)
+		for i := 0; i < 80 && !strings.Contains(buf.String(), expected); i++ {
+			time.Sleep(100 * time.Millisecond)
+		}
 		sigChan <- syscall.SIGTERM
 	}()
 
 	runQueuedContainers(2, 1, crunchCmd, crunchCmd)
 
-	buf, _ := ioutil.ReadFile(tempfile.Name())
-	c.Check(strings.Contains(string(buf), expected), Equals, true)
+	c.Check(buf.String(), Matches, `(?ms).*`+expected+`.*`)
 }

commit 14e317f75f2e3ecee53d78012eddaa59ce9e2712
Author: Tom Clegg <tom at curoverse.com>
Date:   Thu May 5 15:46:20 2016 -0400

    8128: Add Locked state to Container model.

diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb
index 787047d..5856edd 100644
--- a/services/api/app/models/container.rb
+++ b/services/api/app/models/container.rb
@@ -42,6 +42,7 @@ class Container < ArvadosModel
   States =
     [
      (Queued = 'Queued'),
+     (Locked = 'Locked'),
      (Running = 'Running'),
      (Complete = 'Complete'),
      (Cancelled = 'Cancelled')
@@ -49,7 +50,8 @@ class Container < ArvadosModel
 
   State_transitions = {
     nil => [Queued],
-    Queued => [Running, Cancelled],
+    Queued => [Locked, Cancelled],
+    Locked => [Queued, Running, Cancelled],
     Running => [Complete, Cancelled]
   }
 
@@ -102,47 +104,40 @@ class Container < ArvadosModel
   end
 
   def validate_change
-    permitted = []
+    permitted = [:state]
 
     if self.new_record?
-      permitted.push :owner_uuid, :command, :container_image, :cwd, :environment,
-                     :mounts, :output_path, :priority, :runtime_constraints, :state
+      permitted.push(:owner_uuid, :command, :container_image, :cwd,
+                     :environment, :mounts, :output_path, :priority,
+                     :runtime_constraints)
     end
 
     case self.state
-    when Queued
-      # permit priority change only.
+    when Queued, Locked
       permitted.push :priority
 
     when Running
+      permitted.push :priority, :progress
       if self.state_changed?
-        # At point of state change, can set state and started_at
-        permitted.push :state, :started_at
-      else
-        # While running, can update priority and progress.
-        permitted.push :priority, :progress
+        permitted.push :started_at
       end
 
     when Complete
-      if self.state_changed?
-        permitted.push :state, :finished_at, :output, :log, :exit_code
-      else
-        errors.add :state, "cannot update record"
+      if self.state_was == Running
+        permitted.push :finished_at, :output, :log, :exit_code
       end
 
     when Cancelled
-      if self.state_changed?
-        if self.state_was == Running
-          permitted.push :state, :finished_at, :output, :log
-        elsif self.state_was == Queued
-          permitted.push :state, :finished_at
-        end
-      else
-        errors.add :state, "cannot update record"
+      case self.state_was
+      when Running
+        permitted.push :finished_at, :output, :log
+      when Queued, Locked
+        permitted.push :finished_at
       end
 
     else
-      errors.add :state, "invalid state"
+      # The state_transitions check will add an error message for this
+      return false
     end
 
     check_update_whitelist permitted
diff --git a/services/api/lib/whitelist_update.rb b/services/api/lib/whitelist_update.rb
index a81f992..8fccd0f 100644
--- a/services/api/lib/whitelist_update.rb
+++ b/services/api/lib/whitelist_update.rb
@@ -2,7 +2,7 @@ module WhitelistUpdate
   def check_update_whitelist permitted_fields
     attribute_names.each do |field|
       if not permitted_fields.include? field.to_sym and self.send((field.to_s + "_changed?").to_sym)
-        errors.add field, "illegal update of field"
+        errors.add field, "cannot be modified in this state"
       end
     end
   end
@@ -10,7 +10,7 @@ module WhitelistUpdate
   def validate_state_change
     if self.state_changed?
       unless state_transitions[self.state_was].andand.include? self.state
-        errors.add :state, "invalid state change from #{self.state_was} to #{self.state}"
+        errors.add :state, "cannot change from #{self.state_was} to #{self.state}"
         return false
       end
     end
diff --git a/services/api/test/unit/container_request_test.rb b/services/api/test/unit/container_request_test.rb
index d0def57..701147c 100644
--- a/services/api/test/unit/container_request_test.rb
+++ b/services/api/test/unit/container_request_test.rb
@@ -306,18 +306,18 @@ class ContainerRequestTest < ActiveSupport::TestCase
     assert_equal "Committed", cr.state
 
     c = Container.find_by_uuid cr.container_uuid
-    assert_equal "Queued", c.state
+    assert_equal Container::Queued, c.state
 
     act_as_system_user do
-      c.state = "Running"
-      c.save!
+      c.update_attributes! state: Container::Locked
+      c.update_attributes! state: Container::Running
     end
 
     cr.reload
     assert_equal "Committed", cr.state
 
     act_as_system_user do
-      c.state = "Complete"
+      c.update_attributes! state: Container::Complete
       c.save!
     end
 
diff --git a/services/api/test/unit/container_test.rb b/services/api/test/unit/container_test.rb
index a25f2af..84713c2 100644
--- a/services/api/test/unit/container_test.rb
+++ b/services/api/test/unit/container_test.rb
@@ -9,91 +9,42 @@ class ContainerTest < ActiveSupport::TestCase
     c
   end
 
-  def check_illegal_modify c
-    c.reload
-    c.command = ["echo", "bar"]
-    assert_raises(ActiveRecord::RecordInvalid) do
-      c.save!
-    end
-
-    c.reload
-    c.container_image = "img2"
-    assert_raises(ActiveRecord::RecordInvalid) do
-      c.save!
-    end
-
-    c.reload
-    c.cwd = "/tmp2"
-    assert_raises(ActiveRecord::RecordInvalid) do
-      c.save!
-    end
-
-    c.reload
-    c.environment = {"FOO" => "BAR"}
-    assert_raises(ActiveRecord::RecordInvalid) do
-      c.save!
-    end
-
-    c.reload
-    c.mounts = {"FOO" => "BAR"}
-    assert_raises(ActiveRecord::RecordInvalid) do
-      c.save!
-    end
+  def show_errors c
+    return lambda { c.errors.full_messages.inspect }
+  end
 
-    c.reload
-    c.output_path = "/tmp3"
-    assert_raises(ActiveRecord::RecordInvalid) do
-      c.save!
+  def check_illegal_updates c, bad_updates
+    bad_updates.each do |u|
+      refute c.update_attributes(u), u.inspect
+      refute c.valid?
+      c.reload
     end
+  end
 
-    c.reload
-    c.runtime_constraints = {"FOO" => "BAR"}
-    assert_raises(ActiveRecord::RecordInvalid) do
-      c.save!
-    end
+  def check_illegal_modify c
+    check_illegal_updates c, [{command: ["echo", "bar"]},
+                              {container_image: "img2"},
+                              {cwd: "/tmp2"},
+                              {environment: {"FOO" => "BAR"}},
+                              {mounts: {"FOO" => "BAR"}},
+                              {output_path: "/tmp3"},
+                              {runtime_constraints: {"FOO" => "BAR"}}]
   end
 
   def check_bogus_states c
-    c.reload
-    c.state = nil
-    assert_raises(ActiveRecord::RecordInvalid) do
-      c.save!
-    end
-
-    c.reload
-    c.state = "Flubber"
-    assert_raises(ActiveRecord::RecordInvalid) do
-      c.save!
-    end
+    check_illegal_updates c, [{state: nil},
+                              {state: "Flubber"}]
   end
 
-  def check_no_change_from_complete c
+  def check_no_change_from_cancelled c
     check_illegal_modify c
     check_bogus_states c
 
-    c.reload
-    c.priority = 3
-    assert_raises(ActiveRecord::RecordInvalid) do
-      c.save!
-    end
-
-    c.reload
-    c.state = "Queued"
-    assert_raises(ActiveRecord::RecordInvalid) do
-      c.save!
-    end
-
-    c.reload
-    c.state = "Running"
-    assert_raises(ActiveRecord::RecordInvalid) do
-      c.save!
-    end
-
-    c.reload
-    c.state = "Complete"
-    assert_raises(ActiveRecord::RecordInvalid) do
-      c.save!
-    end
+    check_illegal_updates c, [{ priority: 3 },
+                              { state: Container::Queued },
+                              { state: Container::Locked },
+                              { state: Container::Running },
+                              { state: Container::Complete }]
   end
 
   test "Container create" do
@@ -120,58 +71,79 @@ class ContainerTest < ActiveSupport::TestCase
       c = minimal_new
       c.save!
 
-      c.reload
-      c.state = "Complete"
-      assert_raises(ActiveRecord::RecordInvalid) do
-        c.save!
-      end
+      check_illegal_updates c, [{state: Container::Running},
+                                {state: Container::Complete}]
 
-      c.reload
-      c.state = "Running"
-      c.save!
+      c.update_attributes! state: Container::Locked
+      c.update_attributes! state: Container::Running
 
       check_illegal_modify c
       check_bogus_states c
 
+      check_illegal_updates c, [{state: Container::Queued}]
       c.reload
-      c.state = "Queued"
-      assert_raises(ActiveRecord::RecordInvalid) do
-        c.save!
-      end
 
-      c.reload
-      c.priority = 3
-      c.save!
+      c.update_attributes! priority: 3
     end
   end
 
-  test "Container queued cancel" do
+  test "Lock and unlock" do
     act_as_system_user do
       c = minimal_new
       c.save!
+      assert_equal Container::Queued, c.state
 
+      refute c.update_attributes(state: Container::Running), "not locked"
+      c.reload
+      refute c.update_attributes(state: Container::Complete), "not locked"
       c.reload
-      c.state = "Cancelled"
-      c.save!
 
-      check_no_change_from_complete c
+      assert c.update_attributes(state: Container::Locked), show_errors(c)
+      assert c.update_attributes(state: Container::Queued), show_errors(c)
+
+      refute c.update_attributes(state: Container::Running), "not locked"
+      c.reload
+
+      assert c.update_attributes(state: Container::Locked), show_errors(c)
+      assert c.update_attributes(state: Container::Running), show_errors(c)
+
+      refute c.update_attributes(state: Container::Locked), "already running"
+      c.reload
+      refute c.update_attributes(state: Container::Queued), "already running"
+      c.reload
+
+      assert c.update_attributes(state: Container::Complete), show_errors(c)
     end
   end
 
-  test "Container running cancel" do
+  test "Container queued cancel" do
     act_as_system_user do
       c = minimal_new
       c.save!
+      assert c.update_attributes(state: Container::Cancelled), show_errors(c)
+      check_no_change_from_cancelled c
+    end
+  end
 
-      c.reload
-      c.state = "Running"
+  test "Container locked cancel" do
+    act_as_system_user do
+      c = minimal_new
       c.save!
+      assert c.update_attributes(state: Container::Locked), show_errors(c)
+      assert c.update_attributes(state: Container::Cancelled), show_errors(c)
+      check_no_change_from_cancelled c
+    end
+  end
 
-      c.reload
-      c.state = "Cancelled"
+  test "Container running cancel" do
+    act_as_system_user do
+      c = minimal_new
       c.save!
-
-      check_no_change_from_complete c
+      c.update_attributes! state: Container::Queued
+      c.update_attributes! state: Container::Locked
+      c.update_attributes! state: Container::Running
+      c.update_attributes! state: Container::Cancelled
+      check_no_change_from_cancelled c
     end
   end
 
@@ -192,28 +164,13 @@ class ContainerTest < ActiveSupport::TestCase
     act_as_system_user do
       c = minimal_new
       c.save!
+      c.update_attributes! state: Container::Locked
+      c.update_attributes! state: Container::Running
 
-      c.reload
-      c.state = "Running"
-      c.save!
-
-      c.reload
-      c.exit_code = 1
-      assert_raises(ActiveRecord::RecordInvalid) do
-        c.save!
-      end
+      check_illegal_updates c, [{exit_code: 1},
+                                {exit_code: 1, state: Container::Cancelled}]
 
-      c.reload
-      c.exit_code = 1
-      c.state = "Cancelled"
-      assert_raises(ActiveRecord::RecordInvalid) do
-        c.save!
-      end
-
-      c.reload
-      c.exit_code = 1
-      c.state = "Complete"
-      c.save!
+      assert c.update_attributes(exit_code: 1, state: Container::Complete)
     end
   end
 end

commit 1b4fa5760aab91a8422dc5d84c73bd627ff1dc51
Author: Tom Clegg <tom at curoverse.com>
Date:   Thu Apr 28 11:16:50 2016 -0400

    8128: De-dup container unit tests

diff --git a/services/api/test/unit/container_test.rb b/services/api/test/unit/container_test.rb
index 0cac6ac..a25f2af 100644
--- a/services/api/test/unit/container_test.rb
+++ b/services/api/test/unit/container_test.rb
@@ -1,6 +1,14 @@
 require 'test_helper'
 
 class ContainerTest < ActiveSupport::TestCase
+  def minimal_new
+    c = Container.new
+    c.command = ["echo", "foo"]
+    c.container_image = "img"
+    c.output_path = "/tmp"
+    c
+  end
+
   def check_illegal_modify c
     c.reload
     c.command = ["echo", "bar"]
@@ -90,10 +98,7 @@ class ContainerTest < ActiveSupport::TestCase
 
   test "Container create" do
     act_as_system_user do
-      c = Container.new
-      c.command = ["echo", "foo"]
-      c.container_image = "img"
-      c.cwd = "/tmp"
+      c = minimal_new
       c.environment = {}
       c.mounts = {"BAR" => "FOO"}
       c.output_path = "/tmp"
@@ -112,10 +117,7 @@ class ContainerTest < ActiveSupport::TestCase
 
   test "Container running" do
     act_as_system_user do
-      c = Container.new
-      c.command = ["echo", "foo"]
-      c.container_image = "img"
-      c.output_path = "/tmp"
+      c = minimal_new
       c.save!
 
       c.reload
@@ -145,10 +147,7 @@ class ContainerTest < ActiveSupport::TestCase
 
   test "Container queued cancel" do
     act_as_system_user do
-      c = Container.new
-      c.command = ["echo", "foo"]
-      c.container_image = "img"
-      c.output_path = "/tmp"
+      c = minimal_new
       c.save!
 
       c.reload
@@ -161,10 +160,7 @@ class ContainerTest < ActiveSupport::TestCase
 
   test "Container running cancel" do
     act_as_system_user do
-      c = Container.new
-      c.command = ["echo", "foo"]
-      c.container_image = "img"
-      c.output_path = "/tmp"
+      c = minimal_new
       c.save!
 
       c.reload
@@ -181,10 +177,7 @@ class ContainerTest < ActiveSupport::TestCase
 
   test "Container create forbidden for non-admin" do
     set_user_from_auth :active_trustedclient
-    c = Container.new
-    c.command = ["echo", "foo"]
-    c.container_image = "img"
-    c.cwd = "/tmp"
+    c = minimal_new
     c.environment = {}
     c.mounts = {"BAR" => "FOO"}
     c.output_path = "/tmp"
@@ -197,10 +190,7 @@ class ContainerTest < ActiveSupport::TestCase
 
   test "Container only set exit code on complete" do
     act_as_system_user do
-      c = Container.new
-      c.command = ["echo", "foo"]
-      c.container_image = "img"
-      c.output_path = "/tmp"
+      c = minimal_new
       c.save!
 
       c.reload

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


hooks/post-receive
-- 




More information about the arvados-commits mailing list