[ARVADOS] updated: 88d5c6563ce416c58817d6268d1f6791437e44e9
Git user
git at public.curoverse.com
Wed May 11 11:20:03 EDT 2016
Summary of changes:
.../arvados/v1/containers_controller.rb | 8 +++
services/api/app/models/container.rb | 13 +++-
services/api/test/fixtures/containers.yml | 6 ++
.../crunch-dispatch-local/crunch-dispatch-local.go | 64 +++++++++++--------
.../crunch-dispatch-local_test.go | 6 +-
.../crunch-dispatch-slurm/crunch-dispatch-slurm.go | 44 +++++++++----
.../crunch-dispatch-slurm_test.go | 19 ++++--
services/crunch-run/crunchrun.go | 48 ++++++++++++--
services/crunch-run/crunchrun_test.go | 73 ++++++++++++++++++++++
9 files changed, 228 insertions(+), 53 deletions(-)
discards 2ab18c1a0957d2085c86c732ef18f72e6a80d90f (commit)
discards 91b1d56098b3c6cb54abaabbec68937fe58191fd (commit)
discards 74189ab0578afbf445a38cbb99bf47d432e3c551 (commit)
discards 8285405a41257da35a482f87add07c7692548703 (commit)
discards 0f8d4bdadd37522d0eb80f071bba8311c76fddf7 (commit)
discards aa76afd84456af352ba78f6e2b2d9e315bb60687 (commit)
via 88d5c6563ce416c58817d6268d1f6791437e44e9 (commit)
via f2915c217609108b4c85e8b31a4aab742ec33304 (commit)
via bc2f49d40040bb7c6838eea91a20ca0fdc766775 (commit)
via 7000465902591c750955bffb0f376b6b4b53f4e1 (commit)
via 23904a9a5c8ea30b2cc771d38c61e80ddd500190 (commit)
via 89849b0d142c4311f26cc93b5200f7c5f1c03449 (commit)
via d0f98f1c9494b52c2cb39453b34383d5c5092b96 (commit)
via a32c69b81296860a30cc33909226d9294f411adf (commit)
via da2e9a5602276637b6780e8d5a64631219eac365 (commit)
via 44f0e83d50f688bf73c336747402d490346f5c34 (commit)
via 48c0b2f90d232d1508f1a6c8214c1b8e33c78795 (commit)
via 37a00e62fa74ce162ee13d16c731dc10218c5df3 (commit)
via 735502484467241d088fd3c2ebbccc0d6a628dc1 (commit)
via b5e2bbb94cd7e121d4b8d83af86ef0ab0c449dcf (commit)
via 1695ccb6279de083eeed80d36d38f607a5c7098d (commit)
via 505c7f34a713906581329417a4e21ac932859926 (commit)
via 9bd0009d81d3bcdb1b1b1b3ff070537b6ff68f54 (commit)
via c217491f9fdc78bd1c665618137c053a852599ac (commit)
via f1adedeba07502273d39084d4ff3645b30067579 (commit)
This update added new revisions after undoing existing revisions. That is
to say, the old revision is not a strict subset of the new revision. This
situation occurs when you --force push a change and generate a repository
containing something like this:
* -- * -- B -- O -- O -- O (2ab18c1a0957d2085c86c732ef18f72e6a80d90f)
\
N -- N -- N (88d5c6563ce416c58817d6268d1f6791437e44e9)
When this happens we assume that you've already had alert emails for all
of the O revisions, and so we here report only the revisions in the N
branch from the common base, B.
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 88d5c6563ce416c58817d6268d1f6791437e44e9
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 f2915c217609108b4c85e8b31a4aab742ec33304
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 bc2f49d40040bb7c6838eea91a20ca0fdc766775
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 7000465902591c750955bffb0f376b6b4b53f4e1
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..baa1478 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,10 +138,9 @@ 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)
@@ -154,6 +152,5 @@ func testWithServerStub(c *C, apiStubResponses map[string]arvadostest.StubRespon
// 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 23904a9a5c8ea30b2cc771d38c61e80ddd500190
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 89849b0d142c4311f26cc93b5200f7c5f1c03449
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 d0f98f1c9494b52c2cb39453b34383d5c5092b96
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