Project

General

Profile

Actions

Bug #9687

closed

[Workbench] A container_request should not be displayed as "successful" if the container exited non-zero.

Added by Radhika Chippada over 7 years ago. Updated over 7 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Workbench
Target version:
Story points:
0.5
Release:
Release relationship:
Auto


Subtasks 1 (0 open1 closed)

Task #9809: Review 9687-container-request-displayResolvedLucas Di Pentima08/29/2016Actions
Actions #1

Updated by Radhika Chippada over 7 years ago

  • Description updated (diff)
Actions #2

Updated by Tom Clegg over 7 years ago

If a container fails, we should have
  • ContainerRequest state: Committed
  • Container state: Complete
  • Container exit code: number != 0

In this example the container has

{
  "state": "Complete",
  "exit_code": 33
}

This state should be rendered in Workbench as a failure.

Actions #3

Updated by Tom Clegg over 7 years ago

  • Subject changed from [Crunch2] The status of a container_request is is success even when the status of one of it's children is failed. to [Workbench] A container_request should not be displayed as "successful" if the container exited non-zero.
  • Category set to Workbench
Actions #4

Updated by Tom Clegg over 7 years ago

  • Story points set to 0.5
Actions #5

Updated by Tom Morris over 7 years ago

  • Assigned To set to Lucas Di Pentima
  • Target version set to 2016-08-31 sprint
Actions #6

Updated by Lucas Di Pentima over 7 years ago

  • Status changed from New to In Progress
Actions #7

Updated by Lucas Di Pentima over 7 years ago

Fixed the issue at the container_work_unit model. Added related fixture and test @ 74b5dd2

Actions #8

Updated by Radhika Chippada over 7 years ago

74b5dd2b6

  • Can you please update the container_work_unit -> state_label method something as follows? Basically, the get_combined already gets the value from the “other” object as needed …
    exit_code = get_combined(:exit_code)
    return "Failed" if (exit_code && exit_code != 0)
    get_combined(:state)
  • Please consider adding “ exit_code: 0” to all other “Complete” container fixtures.
  • Doesn’t hurt to expand the test "state_label should be Failed if container exit_code not 0" to also check that it is “not Failed” when exit code is 0 (even though it is implicitly tested elsewhere)
Actions #9

Updated by Lucas Di Pentima over 7 years ago

Above comments addressed @ 24eee13

Actions #10

Updated by Radhika Chippada over 7 years ago

Lucas: Looking at the test, I can’t really tell what it’s purpose is. I think it can be a bit more self explanatory. Something as follows?

    …
  ].each do |cr_fixture, state, exit_code|
    test “Completed ContainerRequest state = #{state} when exit_code = #{exit_code}” do
       …
       assert_equal exit_code, …
       assert_equal state, …

Rest LGTM. Thanks.

Actions #11

Updated by Lucas Di Pentima over 7 years ago

The test was written that way because I didn't want to add an exit_code accessor method just to support it, I now have updated the test as per your comments, but had to add that method to the model, you can check it at 8da182d .

Actions #12

Updated by Radhika Chippada over 7 years ago

  • Lucas said: The test was written that way because I didn't want to add an exit_code accessor method

Yes, we need this. Also, please add "def exit_code" to work_unit.rb as well (this is to serve as the interface definition of all work units). We do not need to implement this method in all other work_unit typed classes; it will just default to null in the case of other object types.

  • Nit. Now that you have this method, you can use it in the state_label method rather than repeating the logic here
   def state_label
-    exit_code = get_combined(:exit_code)
+    ec = exit_code

Thanks. LGTM

Actions #13

Updated by Lucas Di Pentima over 7 years ago

Merged master into this branch, resolved conflicts at 3678eda .
Running entire suite before merging into master.

Actions #14

Updated by Lucas Di Pentima over 7 years ago

  • Status changed from In Progress to Resolved

Applied in changeset arvados|commit:fbb979710d3cf5e2ee8c46936ac81081ef553b5c.

Actions

Also available in: Atom PDF