Story #9617

[Crunch2] [API] Reject ContainerRequests that don't specify valid vcpus constraint

Added by Brett Smith about 5 years ago. Updated about 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
API
Target version:
Start date:
08/01/2016
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
0.5
Release:
Release relationship:
Auto

Description

The runtime_constraints of a ContainerRequest must include a vpcus field. vpcus is a number of CPU cores, so it must be a positive integer. The API server must validate this on every create and update of a ContainerRequest when its state is Committed. If a request tries to set vcpus to an unacceptable value, the server must reject the request with HTTP error code 422, and the response JSON should include an error message close to "runtime_constraints vcpus must be a positive integer."


Subtasks

Task #9650: Review 9617-reject-container-reqs-without-vcpusResolvedRadhika Chippada

Associated revisions

Revision 8fc5b659
Added by Lucas Di Pentima about 5 years ago

Merge branch '9617-reject-container-reqs-without-vcpus'

9617, 9618: Require positive integers on both 'vcpus' and 'ram' runtime constraints when a container request is committed. Closes #9617, #9618

Revision ac4bbb86
Added by Lucas Di Pentima about 5 years ago

Merge branch '9617-reject-container-reqs-without-vcpus'

9617, 9618: Added runtime constraints to fixtures.

refs #9617, #9618

History

#1 Updated by Brett Smith about 5 years ago

  • Target version set to Arvados Future Sprints

#2 Updated by Brett Smith about 5 years ago

  • Subject changed from [Crunch2] [API] Reject ContainerRequests that don't specify vcpus when no default is set to [Crunch2] [API] Reject ContainerRequests that don't specify valid vcpus constraint
  • Description updated (diff)
  • Category set to API

#3 Updated by Brett Smith about 5 years ago

  • Description updated (diff)

#4 Updated by Brett Smith about 5 years ago

  • Story points set to 0.5

#5 Updated by Lucas Di Pentima about 5 years ago

  • Assigned To set to Lucas Di Pentima
  • Target version changed from Arvados Future Sprints to 2016-08-03 sprint

#6 Updated by Lucas Di Pentima about 5 years ago

  • Status changed from New to In Progress

b7f8614e177be3fb989f5ddf17ba474492d19af1

Added validations and wrote the corresponding test. Also, corrected some other tests to conform this new requirement.
This comment is also valid for #9618, did both field validation & tests on the same branch.

#7 Updated by Radhika Chippada about 5 years ago

  • The following update in test/unit/container_request_test.rb caused a concern
   test "Container request commit" do
     set_user_from_auth :active
-    cr = create_minimal_req!(runtime_constraints: {"vcpus" => [2,3]})
+    cr = create_minimal_req!(runtime_constraints: {"vcpus" => 2, "ram" => 30})

In container_request.rb, the method runtime_constraints_for_container uses the first value in the array for a field. Hence, I think the validator should check whether an array or an integer is given for vcpus. If an array is provided, should raise error if the first value is not an integer (seems like the second value is ignored; otherwise, you might want to ensure that it is an integer as well). Also, please revert back this test update.

  • In test "Container request constraints must include valid vcpus and ram fields when committed", can you please add a case where the CR is modified (the existing tests only test create path)
  • Also, please add a test with array for vcpus and non-integer value(s).

Thanks.

#8 Updated by Brett Smith about 5 years ago

Radhika Chippada wrote:

In container_request.rb, the method runtime_constraints_for_container uses the first value in the array for a field. Hence, I think the validator should check whether an array or an integer is given for vcpus. If an array is provided, should raise error if the first value is not an integer (seems like the second value is ignored; otherwise, you might want to ensure that it is an integer as well). Also, please revert back this test update.

Lucas noticed this and asked about this on IRC. Tom and I discussed it with him, and we agreed that at this time, we don't want to provide any support for value ranges (two-element arrays) in these constraints at this time. None of the Crunch v2 implementations currently support them either, so limiting the values to integers for now will ensure that container requests have values that dispatchers actually support, and we can permit ranges in the validations when the dispatchers are extended to support that. The diff you pasted follows up on that discussion fine, AFAICT.

#9 Updated by Lucas Di Pentima about 5 years ago

97c669426fd4a086883d1cf666043af17f99f758

Added tests on container request update. Also added tests for both successful create & update cases

#10 Updated by Radhika Chippada about 5 years ago

Thanks for the update. Just a few suggestions.

  • Rather than create a new CR for each update test, you can reuse the object doing something like:
-    cr = create_minimal_req!(state: "Uncommitted", priority: 1)
-    cr.save!
+    cr = ContainerRequest.find_by_uuid cr.uuid
     cr.state = "Committed" 
  • It would be easier to read the code if cr.state is also within the raises block:
-    cr.state = "Committed" 
     assert_raises(ActiveRecord::RecordInvalid) do
+      cr.state = "Committed" 
       cr.runtime_constraints = {"vcpus" => 1}
       cr.save!
     end
  • It is desirable to include update tests using update_attribute:
+    cr = ContainerRequest.find_by_uuid cr.uuid
+    cr.state = "Committed" 
+    assert_raises(ActiveRecord::RecordInvalid) do
+      cr.update_attributes!(runtime_constraints: {"vcpus" => 1, "ram" => "123"})
+    end

#11 Updated by Lucas Di Pentima about 5 years ago

b6a922bd21b2c9da8861de4c84c2a2ab25666ef2

Thanks for the suggestions, I updated the code and also eliminated repeated lines while trying to maintain readability.

#12 Updated by Radhika Chippada about 5 years ago

Lucas, this is SO MUCH better. Thanks. Just a couple more observations and some nitpicks.

  • Please convert the update_attributes test at line 85 (or add another block similar to "validations on update") to failing tests. Our goal here is to test that it fails.
  • Can you please add another invalid_constraint with a string value, something like {"vcpus" => "1", "ram" => "123"}?
  • We don't need the following at line 68 because we have another test that is invoking create_minimal_req with state "Committed" already, correct?
    cr = create_minimal_req!(state: "Committed", priority: 1)    
    cr.save!
  • Can you please just throw in a line separator between blocks (for example, above the comment "Validations on update" etc) to improve readability

Thanks.

#13 Updated by Lucas Di Pentima about 5 years ago

Radhika Chippada wrote:

Lucas, this is SO MUCH better. Thanks. Just a couple more observations and some nitpicks.

  • Please convert the update_attributes test at line 85 (or add another block similar to "validations on update") to failing tests. Our goal here is to test that it fails.

I'm not sure if I understand correctly. The failing tests are the ones on the invalid_constraint blocks, the idea on this other one was to test a successful case, added an assert to make it more explicit, but not sure if I'm missing something else.

  • Can you please add another invalid_constraint with a string value, something like {"vcpus" => "1", "ram" => "123"}?

Updated.

  • We don't need the following at line 68 because we have another test that is invoking create_minimal_req with state "Committed" already, correct?

Yes, that test case was already included on the "Container request commit" test so, I deleted it.

  • Can you please just throw in a line separator between blocks (for example, above the comment "Validations on update" etc) to improve readability

Done.

Thanks for the observations! You can check the diff @ b0993528fa57bb1866bbd6a2b19822cc75010a66

#14 Updated by Lucas Di Pentima about 5 years ago

73c06c1a5af0e62fb937304fa6bee19225b9d7f6

Test code re-styling as per Radhika's suggestions, thanks!

#15 Updated by Radhika Chippada about 5 years ago

Lucas: this is much better. Thanks.

LGTM

#16 Updated by Lucas Di Pentima about 5 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 0 to 100

Applied in changeset arvados|commit:8fc5b6595fd1d544b5812d0807e9747c6eab5f8d.

#17 Updated by Lucas Di Pentima about 5 years ago

4a1dfd3c16896a873fa32becbdd264aa10ada7af

Added runtime constraints to fixtures.

#18 Updated by Radhika Chippada about 5 years ago

Lucas, thanks for the update.

LGTM

Also available in: Atom PDF