Arvados

13143-secret-mounts @ 4dd3d867e590ff54b9e2c1abeea879bcdf87c529

https://ci.curoverse.com/job/developer-run-tests/640/

There are multiple ContainerRequestsTest failures in the workbench integration tests.

• raise error if same key appears in both mounts and secret_mounts.

13143-secret-mounts @ b77abbed6a747e46b98b1e8478158b27ff320151 https://ci.curoverse.com/job/developer-run-tests/643/

• From the "running" container fixture:

  secret_mounts_md5: <%= Digest::MD5.hexdigest(SafeJSON.dump({'/secret/6x9' => {'kind' => 'text', 'content' => "42\n"}})) %>

This will produce a JSON string with "kind" before "content" which is presumably inconsistent with deep_sort_hash? It looks like the tests for container reuse don't use this fixture.

• The comment is in direct contradiction of what the actual test does:
  

    test "not reuse container with different secret_mounts" do
      secrets = {"/foo" => {"kind" => "binary"}}
      set_user_from_auth :active
      cr1 = create_minimal_req!(state: "Committed", priority: 1, secret_mounts: secrets.dup)
      cr1.save!
      cr2 = create_minimal_req!(state: "Committed", priority: 1, secret_mounts: secrets.dup)
      cr2.save!
      assert_not_nil cr1.container_uuid
      assert_equal cr1.container_uuid, cr2.container_uuid
    end
  

• Similarly the comment is inconsistent with what is actually being tested:
  

    [
      Container::Complete,
      Container::Cancelled,
    ].each do |final_state|
      test "secret_mounts is null after container is #{final_state}" do
        assert_no_secrets_logged
      end
    end
  

• Seems redundant with the test set immediately below it (lines 850-875), which includes testing reuse nil / {} secret_mounts:
  

    test "reuse container with same secret_mounts" do
  

• Should this be SafeJSON.dump(self.class.deep_sort_hash(self.secret_mounts || {}))) ?
  

    def update_secret_mounts_md5
      if self.secret_mounts_changed?
        self.secret_mounts_md5 = Digest::MD5.hexdigest(
          SafeJSON.dump(self.class.deep_sort_hash(self.secret_mounts)))
      end
  

For consistency with "Don't advertise secret_* columns in discovery doc" this should use start_with?('secret_') ?

        YAML.load(file).each do |name, ob|
          ob.reject! { |k, v| k == 'secret_mounts' }
        end

• fix half/un-written tests / remove redundant tests noted in #9
• rearrange hash to make more realistic fixture
• skip secret_* not just secret_mounts

Should this be SafeJSON.dump(self.class.deep_sort_hash(self.secret_mounts || {}))) ?

A serialized attribute like secret_mounts is guaranteed to be the right type (Hash), unless I'm missing something here...

I see I did use "|| {}" in the reuse code, but that was unnecessary -- removed, and added some tests.

So I'm doing some testing with my code rebased on top of the branch. Either I'm doing something wrong, or there's a bug preventing access to the secret_mounts endpoint:

2018-03-12T19:17:51.222533427Z crunch-run dev started
2018-03-12T19:17:51.222551825Z Executing container '2tlax-dz642-fj3rvcqmm1e0c89'
2018-03-12T19:17:51.222619178Z Executing on host '60c7246ce9d1'
2018-03-12T19:17:51.258858659Z error fetching secret_mounts: arvados API server error: Token is not associated with this container. (403: 403 Forbidden) returned by 172.17.0.2:8000
2018-03-12T19:17:51.316341206Z crunch-run finish

2018-03-12_19:22:07.10099 Error 1520882527+b1dde1fb: 403
2018-03-12_19:22:07.10152 {"method":"GET","path":"/arvados/v1/api_client_authorizations/2tlax-gj3su-pm6h67l2fclc4zp","format":"html","controller":"Arvados::V1::ApiClientAuthorizationsController","action":"show","status":403,"duration":0.64,"view":0.22,"db":0.0,"request_id":"req-71uxf8k9f6rxgiiad6i7","client_ipaddr":"172.17.0.2","client_auth":"2tlax-gj3su-681l6ikc3nfl2wk","params":{"include_trash":"true","select":"[\"uuid\"]"},"@timestamp":"2018-03-12T19:22:07.101339806Z","@version":"1","message":"[403] GET /arvados/v1/api_client_authorizations/2tlax-gj3su-pm6h67l2fclc4zp (Arvados::V1::ApiClientAuthorizationsController#show)"}

2018-03-12_19:22:07.17888 Error 1520882527+aaf2ed81: 403
2018-03-12_19:22:07.17935 {"method":"GET","path":"/arvados/v1/containers/2tlax-dz642-fj3rvcqmm1e0c89/secret_mounts","format":"html","controller":"Arvados::V1::ContainersController","action":"secret_mounts","status":403,"duration":3.93,"view":0.2,"db":0.68,"request_id":"req-aeb7gxw0r64xi4t722qz","client_ipaddr":"172.17.0.2","client_auth":"2tlax-gj3su-kpice4el961d1oh","params":{},"@timestamp":"2018-03-12T19:22:07.179140090Z","@version":"1","message":"[403] GET /arvados/v1/containers/2tlax-dz642-fj3rvcqmm1e0c89/secret_mounts (Arvados::V1::ContainersController#secret_mounts)"}

Should the containers controller have this?

  accept_attribute_as_json :scheduling_parameters, Hash

I realized what I'm doing wrong in crunch-run, it needs to use the container token, not the dispatcher token.

Added accept_attribute_as_json, 13143-secret-mounts @ b20e9017fae4dc9876f4ffee56add1c58f3e3f21

  1) Failure:
ContainerTest#test_find_reusable_method_should_select_complete_over_running_container [/usr/src/arvados/services/api/test/unit/container_test.rb:339]:
Expected nil to not be nil.

  2) Failure:
ContainerTest#test_find_reusable_method_should_select_higher_priority_queued_container [/usr/src/arvados/services/api/test/unit/container_test.rb:161]:
Expected nil to not be nil.

  3) Failure:
ContainerTest#test_find_reusable_method_should_select_latest_completed_container [/usr/src/arvados/services/api/test/unit/container_test.rb:189]:
Expected nil to not be nil.

  4) Failure:
ContainerTest#test_find_reusable_method_should_select_locked_container_most_likely_to_start_sooner [/usr/src/arvados/services/api/test/unit/container_test.rb:297]:
Expected nil to not be nil.

  5) Failure:
ContainerTest#test_find_reusable_method_should_select_locked_over_queued_container [/usr/src/arvados/services/api/test/unit/container_test.rb:368]:
Expected nil to not be nil.

  6) Failure:
ContainerTest#test_find_reusable_method_should_select_running_container_by_progress [/usr/src/arvados/services/api/test/unit/container_test.rb:276]:
Expected nil to not be nil.

  7) Failure:
ContainerTest#test_find_reusable_method_should_select_running_container_by_start_date [/usr/src/arvados/services/api/test/unit/container_test.rb:252]:
Expected nil to not be nil.

  8) Failure:
ContainerTest#test_find_reusable_method_should_select_running_over_failed_container [/usr/src/arvados/services/api/test/unit/container_test.rb:318]:
Expected nil to not be nil.

  9) Failure:
ContainerTest#test_find_reusable_method_should_select_running_over_locked_container [/usr/src/arvados/services/api/test/unit/container_test.rb:355]:
Expected nil to not be nil.

Submitted here

https://ci.curoverse.com/job/developer-run-tests/647/

13143-secret-mounts @ f04ac7ef46a09cdb80a03cfb5b9fefea67602cd5 https://ci.curoverse.com/job/developer-run-tests/648/

This LGTM