[ARVADOS] created: 1.1.3-195-ga9065fd
Git user
git at public.curoverse.com
Tue Mar 13 16:25:22 EDT 2018
at a9065fd962827b2cfae7993b550a386821a08dcb (commit)
commit a9065fd962827b2cfae7993b550a386821a08dcb
Author: Tom Clegg <tclegg at veritasgenetics.com>
Date: Tue Mar 13 10:44:15 2018 -0400
12552: Prioritize containers according to top-level requests.
This change ensures:
* a high-priority container and all of its descendants are prioritized
ahead of lower-priority containers and their descendants.
* when two top-level containers are requested using equal priority,
the earlier-submitted container has higher priority.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tclegg at veritasgenetics.com>
diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb
index 2bb71c3..3765266 100644
--- a/services/api/app/models/container.rb
+++ b/services/api/app/models/container.rb
@@ -24,8 +24,8 @@ class Container < ArvadosModel
before_validation :fill_field_defaults, :if => :new_record?
before_validation :set_timestamps
- validates :command, :container_image, :output_path, :cwd, :priority, :presence => true
- validates :priority, numericality: { only_integer: true, greater_than_or_equal_to: 0, less_than_or_equal_to: 1000 }
+ validates :command, :container_image, :output_path, :cwd, :priority, { presence: true }
+ validates :priority, numericality: { only_integer: true, greater_than_or_equal_to: 0 }
validate :validate_state_change
validate :validate_change
validate :validate_lock
@@ -98,29 +98,40 @@ class Container < ArvadosModel
State_transitions
end
+ # Container priority is the highest "computed priority" of any
+ # matching request. The computed priority of a container-submitted
+ # request is the priority of the submitting container. The computed
+ # priority of a user-submitted request is a function of
+ # user-assigned priority and request creation time.
def update_priority!
- 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.
- self.priority = ContainerRequest.
- where(container_uuid: uuid,
- state: ContainerRequest::Committed).
- maximum('priority') || 0
- self.save!
- end
+ return if ![Queued, Locked, Running].include?(state)
+ p = ContainerRequest.
+ where('container_uuid=? and priority>0', uuid).
+ includes(:requesting_container).
+ lock(true).
+ map do |cr|
+ if cr.requesting_container
+ cr.requesting_container.priority
+ else
+ (cr.priority << 50) - (cr.created_at.to_time.to_f * 1000).to_i
+ end
+ end.max || 0
+ update_attributes!(priority: p)
end
def propagate_priority
- if self.priority_changed?
- act_as_system_user do
- # Update the priority of child container requests to match new priority
- # of the parent container.
- ContainerRequest.where(requesting_container_uuid: self.uuid,
- state: ContainerRequest::Committed).each do |cr|
- cr.priority = self.priority
- cr.save
- end
- end
+ return true unless priority_changed?
+ act_as_system_user do
+ # Update the priority of child container requests to match new
+ # priority of the parent container (ignoring requests with no
+ # container assigned, because their priority doesn't matter).
+ ContainerRequest.
+ where(requesting_container_uuid: self.uuid,
+ state: ContainerRequest::Committed).
+ where('container_uuid is not null').
+ includes(:container).
+ map(&:container).
+ map(&:update_priority!)
end
end
diff --git a/services/api/app/models/container_request.rb b/services/api/app/models/container_request.rb
index 7b46994..3587cf0 100644
--- a/services/api/app/models/container_request.rb
+++ b/services/api/app/models/container_request.rb
@@ -11,6 +11,11 @@ class ContainerRequest < ArvadosModel
include WhitelistUpdate
belongs_to :container, foreign_key: :container_uuid, primary_key: :uuid
+ belongs_to :requesting_container, {
+ class_name: 'Container',
+ foreign_key: :requesting_container_uuid,
+ primary_key: :uuid,
+ }
serialize :properties, Hash
serialize :environment, Hash
@@ -276,15 +281,12 @@ class ContainerRequest < ArvadosModel
end
def update_priority
- 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
+ return unless state_changed? || priority_changed? || container_uuid_changed?
+ act_as_system_user do
+ Container.
+ where('uuid in (?)', [self.container_uuid_was, self.container_uuid].compact).
+ lock(true).
+ map(&:update_priority!)
end
end
@@ -299,7 +301,7 @@ class ContainerRequest < ArvadosModel
container = Container.where('auth_uuid=?', token_uuid).order('created_at desc').first
if container
self.requesting_container_uuid = container.uuid
- self.priority = container.priority
+ self.priority = container.priority > 0 ? 1 : 0
end
true
end
diff --git a/services/api/db/migrate/20180313180114_change_container_priority_bigint.rb b/services/api/db/migrate/20180313180114_change_container_priority_bigint.rb
new file mode 100644
index 0000000..d577cbb
--- /dev/null
+++ b/services/api/db/migrate/20180313180114_change_container_priority_bigint.rb
@@ -0,0 +1,5 @@
+class ChangeContainerPriorityBigint < ActiveRecord::Migration
+ def change
+ change_column :containers, :priority, :integer, limit: 8
+ end
+end
diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql
index 798c020..2751114 100644
--- a/services/api/db/structure.sql
+++ b/services/api/db/structure.sql
@@ -348,7 +348,7 @@ CREATE TABLE containers (
output character varying(255),
container_image character varying(255),
progress double precision,
- priority integer,
+ priority bigint,
updated_at timestamp without time zone NOT NULL,
exit_code integer,
auth_uuid character varying(255),
@@ -3066,3 +3066,5 @@ INSERT INTO schema_migrations (version) VALUES ('20180216203422');
INSERT INTO schema_migrations (version) VALUES ('20180228220311');
+INSERT INTO schema_migrations (version) VALUES ('20180313180114');
+
diff --git a/services/api/test/unit/container_request_test.rb b/services/api/test/unit/container_request_test.rb
index 562662e..a2c5279 100644
--- a/services/api/test/unit/container_request_test.rb
+++ b/services/api/test/unit/container_request_test.rb
@@ -11,6 +11,23 @@ class ContainerRequestTest < ActiveSupport::TestCase
include DbCurrentTime
include ContainerTestHelper
+ def with_container_auth(ctr)
+ auth_was = Thread.current[:api_client_authorization]
+ Thread.current[:api_client_authorization] = ApiClientAuthorization.find_by_uuid(ctr.auth_uuid)
+ begin
+ yield
+ ensure
+ Thread.current[:api_client_authorization] = auth_was
+ end
+ end
+
+ def lock_and_run(ctr)
+ act_as_system_user do
+ ctr.update_attributes!(state: Container::Locked)
+ ctr.update_attributes!(state: Container::Running)
+ end
+ end
+
def create_minimal_req! attrs={}
defaults = {
command: ["echo", "foo"],
@@ -143,7 +160,7 @@ class ContainerRequestTest < ActiveSupport::TestCase
assert_equal({"/out" => {"kind"=>"tmp", "capacity"=>1000000}}, c.mounts)
assert_equal "/out", c.output_path
assert_equal({"keep_cache_ram"=>268435456, "vcpus" => 2, "ram" => 30}, c.runtime_constraints)
- assert_equal 1, c.priority
+ assert_operator 0, :<, c.priority
assert_raises(ActiveRecord::RecordInvalid) do
cr.priority = nil
@@ -159,50 +176,17 @@ class ContainerRequestTest < ActiveSupport::TestCase
assert_equal 0, c.priority
end
-
- test "Container request max priority" do
- set_user_from_auth :active
- cr = create_minimal_req!(priority: 5, state: "Committed")
-
- c = Container.find_by_uuid cr.container_uuid
- assert_equal 5, c.priority
-
- cr2 = create_minimal_req!
- cr2.priority = 10
- cr2.state = "Committed"
- cr2.container_uuid = cr.container_uuid
- act_as_system_user do
- cr2.save!
- end
-
- # cr and cr2 have priority 5 and 10, and are being satisfied by
- # the same container c, so c's priority should be
- # max(priority)=10.
- c.reload
- assert_equal 10, c.priority
-
- cr2.update_attributes!(priority: 0)
-
- c.reload
- assert_equal 5, c.priority
-
- cr.update_attributes!(priority: 0)
-
- c.reload
- assert_equal 0, c.priority
- end
-
-
test "Independent container requests" do
set_user_from_auth :active
cr1 = create_minimal_req!(command: ["foo", "1"], priority: 5, state: "Committed")
cr2 = create_minimal_req!(command: ["foo", "2"], priority: 10, state: "Committed")
c1 = Container.find_by_uuid cr1.container_uuid
- assert_equal 5, c1.priority
+ assert_operator 0, :<, c1.priority
c2 = Container.find_by_uuid cr2.container_uuid
- assert_equal 10, c2.priority
+ assert_operator c1.priority, :<, c2.priority
+ c2priority_was = c2.priority
cr1.update_attributes!(priority: 0)
@@ -210,7 +194,7 @@ class ContainerRequestTest < ActiveSupport::TestCase
assert_equal 0, c1.priority
c2.reload
- assert_equal 10, c2.priority
+ assert_equal c2priority_was, c2.priority
end
test "Request is finalized when its container is cancelled" do
@@ -272,14 +256,14 @@ class ContainerRequestTest < ActiveSupport::TestCase
cr = create_minimal_req!(priority: 5, state: "Committed", container_count_max: 1)
c = Container.find_by_uuid cr.container_uuid
- assert_equal 5, c.priority
+ assert_operator 0, :<, c.priority
cr2 = create_minimal_req!
cr2.update_attributes!(priority: 10, state: "Committed", requesting_container_uuid: c.uuid, command: ["echo", "foo2"], container_count_max: 1)
cr2.reload
c2 = Container.find_by_uuid cr2.container_uuid
- assert_equal 10, c2.priority
+ assert_operator 0, :<, c2.priority
act_as_system_user do
c.state = "Cancelled"
@@ -296,37 +280,94 @@ class ContainerRequestTest < ActiveSupport::TestCase
assert_equal 0, c2.priority
end
+ test "child container priority follows same ordering as corresponding top-level ancestors" do
+ findctr = lambda { |cr| Container.find_by_uuid(cr.container_uuid) }
- test "Container makes container request, then changes priority" do
set_user_from_auth :active
- cr = create_minimal_req!(priority: 5, state: "Committed", container_count_max: 1)
- c = Container.find_by_uuid cr.container_uuid
- assert_equal 5, c.priority
-
- cr2 = create_minimal_req!
- cr2.update_attributes!(priority: 5, state: "Committed", requesting_container_uuid: c.uuid, command: ["echo", "foo2"], container_count_max: 1)
- cr2.reload
-
- c2 = Container.find_by_uuid cr2.container_uuid
- assert_equal 5, c2.priority
+ toplevel_crs = [
+ create_minimal_req!(priority: 5, state: "Committed", environment: {"workflow" => "0"}),
+ create_minimal_req!(priority: 5, state: "Committed", environment: {"workflow" => "1"}),
+ create_minimal_req!(priority: 5, state: "Committed", environment: {"workflow" => "2"}),
+ ]
+ parents = toplevel_crs.map(&findctr)
+
+ children = parents.map do |parent|
+ lock_and_run(parent)
+ with_container_auth(parent) do
+ create_minimal_req!(state: "Committed",
+ priority: 1,
+ environment: {"child" => parent.environment["workflow"]})
+ end
+ end.map(&findctr)
+
+ grandchildren = children.reverse.map do |child|
+ lock_and_run(child)
+ with_container_auth(child) do
+ create_minimal_req!(state: "Committed",
+ priority: 1,
+ environment: {"grandchild" => child.environment["child"]})
+ end
+ end.reverse.map(&findctr)
- act_as_system_user do
- c.priority = 10
- c.save!
- end
+ shared_grandchildren = children.map do |child|
+ with_container_auth(child) do
+ create_minimal_req!(state: "Committed",
+ priority: 1,
+ environment: {"grandchild" => "shared"})
+ end
+ end.map(&findctr)
- cr.reload
+ assert_equal shared_grandchildren[0].uuid, shared_grandchildren[1].uuid
+ assert_equal shared_grandchildren[0].uuid, shared_grandchildren[2].uuid
+ shared_grandchild = shared_grandchildren[0]
- cr2.reload
- assert_equal 10, cr2.priority
+ set_user_from_auth :active
- c2.reload
- assert_equal 10, c2.priority
+ # parents should be prioritized by submit time.
+ assert_operator children[0].priority, :>, children[1].priority
+ assert_operator children[1].priority, :>, children[2].priority
+
+ # children should be prioritized in same order as their respective
+ # parents.
+ assert_operator children[0].priority, :>, children[1].priority
+ assert_operator children[1].priority, :>, children[2].priority
+
+ # grandchildren should also be prioritized in the same order,
+ # despite having been submitted in the opposite order.
+ assert_operator grandchildren[0].priority, :>, grandchildren[1].priority
+ assert_operator grandchildren[1].priority, :>, grandchildren[2].priority
+
+ # shared grandchild container should be prioritized above
+ # everything that isn't needed by parents[0], but not above
+ # earlier-submitted descendants of parents[0]
+ assert_operator shared_grandchild.priority, :>, grandchildren[1].priority
+ assert_operator shared_grandchild.priority, :>, children[1].priority
+ assert_operator shared_grandchild.priority, :>, parents[1].priority
+ assert_operator shared_grandchild.priority, :<=, grandchildren[0].priority
+ assert_operator shared_grandchild.priority, :<=, children[0].priority
+ assert_operator shared_grandchild.priority, :<=, parents[0].priority
+
+ # increasing priority of the most recent toplevel container should
+ # reprioritize all of its descendants (including the shared
+ # grandchild) above everything else.
+ toplevel_crs[2].update_attributes!(priority: 72)
+ (parents + children + grandchildren + [shared_grandchild]).map(&:reload)
+ assert_operator shared_grandchild.priority, :>, grandchildren[0].priority
+ assert_operator shared_grandchild.priority, :>, children[0].priority
+ assert_operator shared_grandchild.priority, :>, parents[0].priority
+ assert_operator shared_grandchild.priority, :>, grandchildren[1].priority
+ assert_operator shared_grandchild.priority, :>, children[1].priority
+ assert_operator shared_grandchild.priority, :>, parents[1].priority
+ # ...but the shared container should not have higher priority than
+ # the earlier-submitted descendants of the high-priority workflow.
+ assert_operator shared_grandchild.priority, :<=, grandchildren[2].priority
+ assert_operator shared_grandchild.priority, :<=, children[2].priority
+ assert_operator shared_grandchild.priority, :<=, parents[2].priority
end
[
- ['running_container_auth', 'zzzzz-dz642-runningcontainr', 12],
+ ['running_container_auth', 'zzzzz-dz642-runningcontainr', 1],
['active_no_prefs', nil, 0],
].each do |token, expected, expected_priority|
test "create as #{token} and expect requesting_container_uuid to be #{expected}" do
@@ -792,8 +833,7 @@ class ContainerRequestTest < ActiveSupport::TestCase
c = Container.find_by_uuid(cr.container_uuid)
assert_equal 1, c.priority
- # destroy the cr
- assert_nothing_raised {cr.destroy}
+ cr.destroy
# the cr's container now has priority of 0
c = Container.find_by_uuid(cr.container_uuid)
diff --git a/services/api/test/unit/container_test.rb b/services/api/test/unit/container_test.rb
index 7dffacb..7ee5921 100644
--- a/services/api/test/unit/container_test.rb
+++ b/services/api/test/unit/container_test.rb
@@ -126,10 +126,8 @@ class ContainerTest < ActiveSupport::TestCase
c.priority = 1000
c.save!
- assert_raises(ActiveRecord::RecordInvalid) do
- c.priority = 1001
- c.save!
- end
+ c.priority = 1000 << 50
+ c.save!
end
end
diff --git a/services/crunch-dispatch-slurm/squeue.go b/services/crunch-dispatch-slurm/squeue.go
index 365b4e8..ee79c6f 100644
--- a/services/crunch-dispatch-slurm/squeue.go
+++ b/services/crunch-dispatch-slurm/squeue.go
@@ -91,7 +91,15 @@ func (sqc *SqueueChecker) reniceAll() {
}
sort.Slice(jobs, func(i, j int) bool {
- return jobs[i].wantPriority > jobs[j].wantPriority
+ if jobs[i].wantPriority != jobs[j].wantPriority {
+ return jobs[i].wantPriority > jobs[j].wantPriority
+ } else {
+ // break ties with container uuid --
+ // otherwise, the ordering would change from
+ // one interval to the next, and we'd do many
+ // pointless slurm queue rearrangements.
+ return jobs[i].uuid > jobs[j].uuid
+ }
})
renice := wantNice(jobs, sqc.PrioritySpread)
for i, job := range jobs {
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list