Arvados

After removing API Server's dependency on protected_attributes and activerecord-deprecated_finders gems, I'm having issues with cure-google-api-client and arvados-cli gems:

[...]
    arvados-cli was resolved to 1.3.1.20190211211047, which depends on
      activesupport (>= 3.2.13, < 5)

    arvados (>= 1.3.1.20190301212059) was resolved to 1.3.1.20190301212059, which depends on
      cure-google-api-client (>= 0.7, < 0.8.9) was resolved to 0.8.7.1, which depends on
        activesupport (>= 3.2, < 5.0)
[...]

Lucas Di Pentima wrote:

After removing API Server's dependency on protected_attributes and activerecord-deprecated_finders gems, I'm having issues with cure-google-api-client and arvados-cli gems:

[...]

I suspect it is just out of caution since a major version number increase could be incompatible.

Forked Ward's google-api-ruby-client repo to github.com/curoverse and made a new branch to adjust the activesupport dependency upper limit to '< 5.1': https://github.com/curoverse/google-api-ruby-client/tree/14873-activesupport5-dependency

Note that I branched out from fix-for-ruby-2.3.7, the branch that seems to have been used to publish the cure-google-api-client-0.8.7.1 gem.

I've manually built & installed the new gem on a ruby 2.5.5 rvm env, and also installed arvados-cli without the activesupport '< 5' dependency. As a result, activesupport 5.0.7.2 was installed and manual tests on the arv command were successful.

Updates at 0e063dfd9 - branch 14873-arvadoscli-gem-activesupport-dep

Bumped arvados-cli upper limit dependency on activesupport from '< 5' to '< 5.1' in order to be able to upgrade to rails 5.

I think we should publish cure-google-api-ruby-client version 0.8.7.2 from #note-10 to be able to run the test suite and do the api server upgrade.

Lucas Di Pentima wrote:

Updates at 0e063dfd9 - branch 14873-arvadoscli-gem-activesupport-dep

Bumped arvados-cli upper limit dependency on activesupport from '< 5' to '< 5.1' in order to be able to upgrade to rails 5.

I think we should publish cure-google-api-ruby-client version 0.8.7.2 from #note-10 to be able to run the test suite and do the api server upgrade.

If we publish it as 0.8.7.2 will that cause all the dependencies to start using it? Is is otherwise backwards compatible?

Both arvados & arvados-cli would ask for it because their dependency line is:

s.add_dependency('cure-google-api-client', '>= 0.7', '< 0.8.9')

OTOH, current arvados-cli also asks for activesupport '< 5' so I think that would avoid upgrading that gem, the rest is the same.

Lucas Di Pentima wrote:

Both arvados & arvados-cli would ask for it because their dependency line is:

[...]

OTOH, current arvados-cli also asks for activesupport '< 5' so I think that would avoid upgrading that gem, the rest is the same.

I see, so long as Ruby's dependency solver respects activesupport '< 5' then it will do the same thing even with activesupport '< 5.1' . Sounds good.

Branch 14873-google-api-client-update (8c0178f2ff01a284ac4bebbc664b9fb7f64cdc38)
Test run: https://ci.curoverse.com/job/developer-run-tests/1139/

Updates arvados and arvados-cli gemspecs to use arvados-google-api-client gem that requests activesupport <5.1.

I also locally ran services/api test suite with these new gem versions by running bundle package to generate vendor/cache directory, then copying the new gem versions to it and then running bundle update arvados[-cli] --local to update the Gemfile.lock, also making sure that those gems were installed on the random gemset created by the test suite. The result: all tests OK.

Lucas Di Pentima wrote:

Branch 14873-google-api-client-update (8c0178f2ff01a284ac4bebbc664b9fb7f64cdc38)
Test run: https://ci.curoverse.com/job/developer-run-tests/1139/

Updates arvados and arvados-cli gemspecs to use arvados-google-api-client gem that requests activesupport <5.1.

I also locally ran services/api test suite with these new gem versions by running bundle package to generate vendor/cache directory, then copying the new gem versions to it and then running bundle update arvados[-cli] --local to update the Gemfile.lock, also making sure that those gems were installed on the random gemset created by the test suite. The result: all tests OK.

This LGTM

Status update¶

Current updates at 8ba20b331fc99252acc29161902017ab98c6a979 - branch 14873-api-rails5-upgrade

Yesterday I unblocked myself from various issues, and was able to get a partially blue test suite run: https://ci.curoverse.com/job/developer-run-tests/1154/

There are several undocumented changes that makes this upgrade more painful that it may look like by reading the official docs. Right now I'm having some unit tests failing but the majority of the failing test come from the functional test suite. The problem that I'm having is that for some reason, minitest seem to flatten nested arrays (that we use on filters), like so:

{filters: [["email", "=", inactive-user@arvados.local]]}

...becomes:

{"filters":[["email"],["="],["inactive-user@arvados.local"]]}

...when received by the controllers. This seems to be happening at the test suite level only. Found some question on SO (https://stackoverflow.com/questions/44119273/rails-5-1-minitest-flattens-array-of-arrays-in-params) but the solution wasn't enough to solve the issue (although the format detected by the controller changed from html to json).

Another issue that I'm having with a unit test is that for some reason the "find_reusable method should select running container by start date" test from container_test.rb (line 326) fails. It seems that the GIN query to ignore those containers with an 'error' key in their runtime_status isn't working as expected. I already tried with the old version and a newer version of pg gem, and it fails the same way. Still investigating.

As a reference for my future reviewer, this blog post reflects most of the pain points I'm experiencing: https://blog.adwyze.com/the-good-the-bad-the-ugly-story-of-a-rails-5-upgrade-c733e727e792

Lucas Di Pentima wrote:

[...]

Another issue that I'm having with a unit test is that for some reason the "find_reusable method should select running container by start date" test from container_test.rb (line 326) fails. It seems that the GIN query to ignore those containers with an 'error' key in their runtime_status isn't working as expected. I already tried with the old version and a newer version of pg gem, and it fails the same way. Still investigating.

It seems that the serializing is done differently between the fixtures and the tests. I tried adding to runtime_status a value from the fixure and later check its value directly from the database, it appears like this:

{"hello": "world"}

...then, I loaded the rails console, got that container record and added a new key to that column, and the result from querying the database directly was:

"{\"hello\":\"world\",\"error\":\"Something went south\"}"

The deserializing process evidently support both ways because all I get from the tests, the debugger and the rails console are hashes, but it seems that there's a difference in how the data is saved and this affects queries that are done by passing SQL command pieces to ActiveRecord.

This test failure seems to be related to the previous comment's finding:

ArvadosModelTest#test_No_HashWithIndifferentAccess_in_database = 0.03 s = F

Failure:
ArvadosModelTest#test_No_HashWithIndifferentAccess_in_database [/media/psf/arvados/services/api/test/unit/arvados_model_test.rb:102]:
--- expected
+++ actual
@@ -1 +1 @@
-"{\"foo\": \"bar\"}" 
+"\"{\\\"foo\\\":\\\"bar\\\"}\"" 

Update at df5f366f4 fixes lots of serialization issues with JSONB columns, and other smaller details.
Unit tests are running OK locally. Jenkins run: https://ci.curoverse.com/job/developer-run-tests/1162/

Status update¶

JSONB columns are auto-detected by the newer rails versions so they shouldn't be declared as serialized because they get serialized twice.
Because of this, I've removed all serialize attrSym, attrClass of the models having JSONB columns.

• With "serialize" statement (current version):
  • Record retrieval: NULL properties field on DB becomes {} on ActiveRecord.
  • Record creation: NULL properties field becomes {} on ActiveRecord.
  • Record update: c.properties = nil becomes {} also.
• Without "serialize" statement (upgraded version):
  • Record retrieval: NULL properties field on DB becomes nil on ActiveRecord
  • Record creation: NULL properties field becomes nil on ActiveRecord
  • Record update: c.properties = nil keeps being nil

To avoid writing a migration to scan and correct every table with a JSONB column without default, I'm trying to solve this with before_validation and after_find callbacks. Some tests do fail with those, I'm trying to see why.

Tables with JSONB columns:¶

• collections
  • properties (without default)
  • storage_classes_desired
  • storage_classes_confirmed
• container_requests
  • properties (without default)
  • secret_mounts
• containers
  • secret_mounts
  • runtime_status
  • runtime_auth_scopes (without default)
• groups
  • properties
• links
  • properties (without default)
• nodes
  • info (without default)
  • properties (without default)

Updates at 97d2da208
Test run: https://ci.curoverse.com/job/developer-run-tests/1170/

• As suggested by Tom, added a couple of JSON custom types that default to [] or {} when assigned nil. Fixed several failing tests
• Dumping hashes and array params as JSON from the controller test helper to avoid minitest to mess the nested structures.

Some tests still pending fixing.

Update at a689825a3
Test run: https://ci.curoverse.com/job/developer-run-tests/1171/

Fixed workbench failing tests by re-adding JSONB backed attributes as Hash & Array types on the discovery doc.

I'm having issues with 2 controller tests. On 66cab5a1f I've modified them to show why they fail. It seems that when doing multiple requests on the same functional test, in some cases the requests don't get through as expected:

• On the groups_controller_test.rb file, the problem that I'm seeing is that the link creation request done before checking that the group is accessible again does create a link, but without the required parameters. I've added a byebug call to the LinkController.create call to see what is being received and the param list is empty. Adding the same link create request as the first request on the test (and then removing that additional link) makes the test pass. I'm not getting why.

• On the jobs_controller_test.rb file, the components aren't being deleted when passing components: {} to the job update call. Instead, it receives the same request as the previous Job update call, including the 'component1' component.

Now I'm working on figuring out why the test/performance/permission_test.rb test fail, it doesn't get the complete group list although I checked that they're being created. Also have a crash on the test/performance/links_index_test.rb involving some type error, but haven't looked into it yet.

(summary of chat) Don't make multiple API calls from a single functional test. Rails functional tests don't work that way. We have some existing tests that try to hack around our own "detect & reject because it doesn't work" code by meddling with @controller, but it's still a bad idea. Split it into multiple test cases that each do a single controller action, or use an integration test.

I expect that will address at least some of the problems you're seeing here.

Happy to report that all tests are passing: https://ci.curoverse.com/job/developer-run-tests/1176/
Updates at a7d2e8bc0 - branch 14873-api-rails5-upgrade

Ready for review!

I've just merged master and adjusted some of the latest changes to be in sync with the rails 5 upgrade at 0a2adc425
Test run: https://ci.curoverse.com/job/developer-run-tests/1178/

Tried starting an arvbox instance with this branch, seems to work correctly.

Seems like we might fix these now:

DEPRECATION WARNING: `config.serve_static_files` is deprecated and will be removed in Rails 5.1.
Please use `config.public_file_server.enabled = true` instead.
 (called from block in <top (required)> at /home/tom/arvados/services/api/config/environments/test.rb:15)
DEPRECATION WARNING: `config.static_cache_control` is deprecated and will be removed in Rails 5.1.
Please use
`config.public_file_server.headers = { 'Cache-Control' => 'public, max-age=3600' }`
instead.
 (called from block in <top (required)> at /home/tom/arvados/services/api/config/environments/test.rb:16)

This is our own fork, https://github.com/curoverse/themes_for_rails -- I think the way it's pinned to a sha1 means we can update it, and update source:services/api/Gemfile.lock and source:apps/workbench/Gemfile.lock when we're ready:

DEPRECATION WARNING: before_filter is deprecated and will be removed in Rails 5.1. Use before_action instead. (called from theme at /home/tom/arvados/services/api/vendor/cache/themes_for_rails-61154877047d/lib/themes_for_rails/action_controller.rb:18)
rake aborted!

This error stops "install services/api" on top of my existing test database, which makes me suspect it will also happen when upgrading a real install. Have you seen this? (I haven't tried to understand/fix it.)

ActiveRecord::NoEnvironmentInSchemaError: 

Environment data not found in the schema. To resolve this issue, run: 

        bin/rails db:environment:set RAILS_ENV=test

Updates at 5f0826e
Test run: https://ci.curoverse.com/job/developer-run-tests/1180/
API server tests re-run: https://ci.curoverse.com/job/developer-run-tests-services-api/1200/ (also did a local run with "--repeat 50", all OK)

• Fixed pending deprecated config items
• Updated our fork of themes_for_rails gem to fix deprecation warnings: https://github.com/curoverse/themes_for_rails/commit/ddf6e592b3b6493ea0c2de7b5d3faa120ed35be0
• Updated themes_for_rails's version dependency on Gemfile.lock

It looks like the change to IntegrationTest causes performance tests to be included in the test suite.

Time spent creating records:
  0.360000   0.030000   0.390000 (  1.283456)
created 6561
Time spent getting group index:
 16.320000   0.490000  16.810000 ( 20.254220)
 15.560000   0.420000  15.980000 ( 19.390378)
 21.140000   0.490000  21.630000 ( 25.254089)
 22.590000   0.510000  23.100000 ( 26.847924)
 23.240000   0.520000  23.760000 ( 27.518683)
PermissionPerfTest#test_groups_index = 120.56 s = .

• reduce the limit to the default (100) or something in between (the linear increase makes me suspect loading the results as AR objects is overwhelming the database timing this is intended to check anyway)
• repeat the "get index" part fewer than 5 times (maybe 2 is enough)

Updates at 7e1bf9eba

Made updates following the above suggestions. Now those tests take 5.8s.

LGTM @ 7e1bf9eba