Project

General

Profile

Actions

Idea #14873

closed

[API] Update to Rails 5

Added by Tom Morris about 5 years ago. Updated almost 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
API
Target version:
Story points:
2.0
Release relationship:
Auto

Description

When Rails 6 is released in May 2019, Ralis 4.x will become unsupported, so we need to be supporting Rails 5 and associated dependencies in a stable release before then.

https://weblog.rubyonrails.org/2018/12/20/timeline-for-the-release-of-Rails-6-0/


Subtasks 2 (0 open2 closed)

Task #15004: Review 14873-google-api-client-updateResolvedPeter Amstutz03/20/2019Actions
Task #15031: Review 14873-api-rails5-upgradeClosedLucas Di Pentima04/04/2019Actions

Related issues

Related to Arvados - Idea #14824: postgresql 10.x compatibility ResolvedActions
Blocks Arvados - Idea #14946: Update to Ruby 2.5 - 2.3 is going EOLResolvedLucas Di Pentima05/31/2019Actions
Blocks Arvados - Idea #13484: Support multiple load-balanced API server nodesNewWard VandewegeActions
Actions #1

Updated by Eric Biagiotti about 5 years ago

  • Related to Idea #14824: postgresql 10.x compatibility added
Actions #2

Updated by Tom Morris about 5 years ago

  • Description updated (diff)
  • Release set to 15
Actions #3

Updated by Tom Morris about 5 years ago

  • Description updated (diff)
Actions #4

Updated by Lucas Di Pentima about 5 years ago

  • Blocks Idea #14946: Update to Ruby 2.5 - 2.3 is going EOL added
Actions #5

Updated by Peter Amstutz about 5 years ago

  • Blocks Idea #13484: Support multiple load-balanced API server nodes added
Actions #7

Updated by Lucas Di Pentima about 5 years ago

  • Subject changed from Update to Rails 5 to [API] Update to Rails 5
  • Category set to API
  • Status changed from New to In Progress
  • Assigned To set to Lucas Di Pentima
  • Target version changed from To Be Groomed to 2019-03-27 Sprint
Actions #8

Updated by Lucas Di Pentima about 5 years ago

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)
[...]
Actions #9

Updated by Peter Amstutz about 5 years ago

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.

Actions #10

Updated by Lucas Di Pentima about 5 years ago

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.

Actions #11

Updated by Lucas Di Pentima about 5 years ago

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.

Actions #12

Updated by Lucas Di Pentima about 5 years ago

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.

Actions #13

Updated by Peter Amstutz about 5 years ago

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?

Actions #14

Updated by Lucas Di Pentima about 5 years ago

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.

Actions #15

Updated by Peter Amstutz about 5 years ago

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.

Actions #16

Updated by Lucas Di Pentima about 5 years ago

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.

Actions #17

Updated by Peter Amstutz about 5 years ago

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

Actions #18

Updated by Lucas Di Pentima about 5 years ago

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

Actions #19

Updated by Lucas Di Pentima about 5 years ago

  • Target version changed from 2019-03-27 Sprint to 2019-04-10 Sprint
Actions #20

Updated by Tom Morris about 5 years ago

  • Story points set to 2.0
Actions #21

Updated by Lucas Di Pentima about 5 years ago

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.

Actions #22

Updated by Lucas Di Pentima about 5 years ago

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\\\"}\"" 
Actions #23

Updated by Lucas Di Pentima almost 5 years ago

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/

Actions #24

Updated by Lucas Di Pentima almost 5 years ago

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.

The side-effect of this, is that default values on those attributes don't work as before, for example with Collection class properties (not declared with default value at structure.sql):
  • 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)
Actions #25

Updated by Lucas Di Pentima almost 5 years ago

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.

Actions #26

Updated by Lucas Di Pentima almost 5 years ago

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.

Actions #27

Updated by Lucas Di Pentima almost 5 years ago

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.

Actions #28

Updated by Tom Clegg almost 5 years ago

(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.

Actions #29

Updated by Lucas Di Pentima almost 5 years ago

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!

Actions #30

Updated by Lucas Di Pentima almost 5 years ago

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/

Actions #31

Updated by Lucas Di Pentima almost 5 years ago

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

Actions #32

Updated by Tom Clegg almost 5 years ago

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
Actions #33

Updated by Lucas Di Pentima almost 5 years ago

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)

Actions #34

Updated by Tom Clegg almost 5 years ago

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 = .
This test alone nearly doubles the overall suite time. Could we...
  • 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)
Actions #35

Updated by Lucas Di Pentima almost 5 years ago

Updates at 7e1bf9eba

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

Actions #36

Updated by Tom Clegg almost 5 years ago

LGTM @ 7e1bf9eba

Actions #37

Updated by Lucas Di Pentima almost 5 years ago

  • Status changed from In Progress to Resolved
Actions

Also available in: Atom PDF