Bug #16470

Update to Rails 5.2

Added by Peter Amstutz over 1 year ago. Updated about 1 year ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
08/05/2020
Due date:
% Done:

100%

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

Subtasks

Task #16485: Review 16470-api-rails-52ResolvedTom Clegg

Task #16699: Review 16470-wb-rails-52ResolvedLucas Di Pentima


Related issues

Related to Arvados - Bug #16469: Triage github security alertsClosed

Related to Arvados - Bug #17094: [api] script/rails is broken with 'cannot load such file -- rails/commands/console'Resolved

Related to Arvados - Bug #17528: [Rails] package prints a backtrace on installation when the arvados config.yml file is not present/completeResolved04/15/2021

Associated revisions

Revision 043dd5e0
Added by Lucas Di Pentima about 1 year ago

Merge branch '16470-api-rails-52'
Refs #16470

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <>

Revision 3bcdb624 (diff)
Added by Ward Vandewege about 1 year ago

Stop installing initializers/omniauth.rb in our rails package for the
API server. It has been a dangling symlink for a long time, and Rails
5.2 is no longer forgiving of that.

refs #16470

Arvados-DCO-1.1-Signed-off-by: Ward Vandewege <>

Revision 586e1c55 (diff)
Added by Ward Vandewege about 1 year ago

Actively remove initializers/omniauth.rb in our rails package for the
API server. It has been a dangling symlink for a long time, and Rails
5.2 is no longer forgiving of that.

refs #16470

Arvados-DCO-1.1-Signed-off-by: Ward Vandewege <>

Revision d94f8f7d
Added by Lucas Di Pentima about 1 year ago

Merge branch '16470-wb-rails-52'
Closes #16470

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <>

Revision b000158f (diff)
Added by Lucas Di Pentima about 1 year ago

Reverts previously dropped commit that got reintroduced by a rebase.
Refs #16470

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <>

History

#1 Updated by Peter Amstutz over 1 year ago

  • Related to Bug #16469: Triage github security alerts added

#2 Updated by Peter Amstutz over 1 year ago

  • Assigned To set to Lucas Di Pentima

#3 Updated by Lucas Di Pentima over 1 year ago

  • Target version changed from 2020-06-17 Sprint to 2020-07-01 Sprint

#4 Updated by Lucas Di Pentima over 1 year ago

  • Status changed from New to In Progress

#5 Updated by Lucas Di Pentima over 1 year ago

  • Target version changed from 2020-07-01 Sprint to 2020-07-15

#6 Updated by Lucas Di Pentima over 1 year ago

I've bumped activesupport dependency (to <5.3) and patchlevel number on arvados-google-api-ruby-client to allow the Rails upgrade to 5.2:

https://github.com/arvados/google-api-ruby-client/tree/16470-activesupport52-dependency

I think we need this to be published on rubygems so that test runs on other than my own dev environment, works. I need to be able to run the tests on Jenkins as I want to confirm that some test failures aren't only happening on my machine.

#7 Updated by Lucas Di Pentima over 1 year ago

  • Target version changed from 2020-07-15 to 2020-08-12 Sprint

#8 Updated by Peter Amstutz about 1 year ago

  • Release set to 25

#9 Updated by Lucas Di Pentima about 1 year ago

Update: I'm currently having an issue with the jobs api disabling code, and talking about it on the release 2.1 meeting we came to the conclusion that the jobs, job_task, pipeline & pipeline_instance apis shouldn't be appearing on the discovery document (so the arv command won't see them) and that the jobs api should be disabled by default and have a config knob that would enable it.

#10 Updated by Lucas Di Pentima about 1 year ago

API Server upgrade to Rails 5.2 ready at 7f45a8a - branch 16470-api-rails-52

Test run: https://ci.arvados.org/job/developer-run-tests/1987/
Test remainder re-run: https://ci.arvados.org/job/developer-run-tests-remainder/2075/

Will checkout a separate branch off of this one to continue with workbench's upgrade.

#11 Updated by Lucas Di Pentima about 1 year ago

Note: the jobs API methods mentioned on note-9 will be removed on a separate branch/story as I've tried to do it yesterday and some integration tests failed. As it's not related to the rails upgrade, I prefer not to block this story on that issue.

#12 Updated by Tom Clegg about 1 year ago

This "reload" seems odd, since (a) if we need to reload, doing it after locking the row would make more sense, and (b) with_lock already appears to reload after getting the lock, https://apidock.com/rails/ActiveRecord/Locking/Pessimistic/lock%21 ... I'm guessing we have some code path that raises "Locking a record with unpersisted changes is not supported" in which case it would be better to fix that code path (silently discarding changes seems like it invites bugs that are hard to track down). If that's not feasible, a comment here would be good.

--- a/services/api/app/controllers/arvados/v1/containers_controller.rb
-    @object.with_lock do
+    @object.reload.with_lock do

This (source:services/api/config/application.rb) looks like it could be removed

+# require "active_storage/engine" 

The change to source:services/api/config/initializers/legacy_jobs_api.rb seems to mean config.Containers.JobsAPI.Enable=="auto" now means "true". If so, maybe it would be less confusing if we make "auto" equivalent to "false", change the default value from "auto" to "false", and mention in the release notes (and arvados-server config-check) that "auto" is no longer supported.

Alternatively... how about doing the "auto enable/disable jobs api?" check in a before_action in ApplicationController (perhaps in disable_api_methods itself), and caching the result? It seems like that would preserve the existing behavior without trying to access the database too early in the boot process.

(TBC)

#13 Updated by Lucas Di Pentima about 1 year ago

Updates at f4cb214f1
Test run: https://ci.arvados.org/job/developer-run-tests/1989/

  • Removes the reload call on CR's controller, I think it slipped on my many tries to make the deprecation warning go away when upgrading to 5.1. The tests now on 5.2 pass so no code path makes this happen as it would make rails raise an exception.
  • Removed commented out @require@s for ActiveStorage & ActiveCable, leaving just a comment what both are skipped.
  • Re: the legacy_jobs_api.rb updates, I'm not sure if we're seeing the same code, I'm just catching a specific exception that happens when running rake db:create, the rest of the code is the same, right?
diff --git a/services/api/config/initializers/legacy_jobs_api.rb b/services/api/config/initializers/legacy_jobs_api.rb
index 8f3b3cb5f..2abe40566 100644
--- a/services/api/config/initializers/legacy_jobs_api.rb
+++ b/services/api/config/initializers/legacy_jobs_api.rb
@@ -8,8 +8,13 @@

 require 'enable_jobs_api'

-Server::Application.configure do
-  if ActiveRecord::Base.connection.tables.include?('jobs')
-    check_enable_legacy_jobs_api
+Rails.application.configure do
+  begin
+    if ActiveRecord::Base.connection.tables.include?('jobs')
+      check_enable_legacy_jobs_api
+    end
+  rescue ActiveRecord::NoDatabaseError
+    # Since rails 5.2, all initializers are run by rake tasks (like db:create),
+    # see: https://github.com/rails/rails/issues/32870
   end
 end

Why would it change the config semantics?

#14 Updated by Tom Clegg about 1 year ago

Removes the reload call on CR's controller

Perhaps this similar change should be reverted too? (app/models/collection.rb)

-    # Put aside the changes because with_lock forces a record reload
+    # Put aside the changes because with_lock requires an explicit record reload
     changes = self.changes
     snapshot = nil
-    with_lock do
+    reload.with_lock do

...and more occurrences in test/unit/collection_test.rb, api/app/models/container.rb (4x + comment), app/models/container_request.rb (2x + comment), app/models/job.rb

legacy_jobs_api.rb

Aaah, I see. I didn't understand the comment -- I thought NoDatabaseError was happening at startup, and this was where the "disable crunch1 endpoints entirely" conversation came from. If this only happens during db:create then it makes sense.

Seems like config/storage.yml isn't needed or is ignored since we don't use ActiveStorage (comment in config/application.rb) ... possibly worth deleting the file, or adding a comment that it's not used?

+  root: <%= Rails.root.join("storage") %>

I'm guessing the secret_key_base env var in config/secrets.yml is not actually used because it's overridden by config/arvados_config.rb ... if so, possibly worth adding a comment to that effect? And perhaps it would be slightly safer to set it to rand(1<<255).to_s(36) in prod mode, so that if it doesn't get overridden by the config system in some scenario, it will degrade by rejecting cookies etc., instead of by opening a vulnerability. Or maybe Rails already does this internally / falls back to a different secret if it's empty, in which case it's fine as is.

Rest LGTM. Thanks!

#15 Updated by Lucas Di Pentima about 1 year ago

Updates at 601249b (rebased branch)
Test run: https://ci.arvados.org/job/developer-run-tests/1996/

Tom Clegg wrote:

Perhaps this similar change should be reverted too? (app/models/collection.rb)

In the case of the collection model, the explicit reload is needed because the callback where it's being called is around_update so the instance is not persisted yet at that time.

...and more occurrences in test/unit/collection_test.rb, api/app/models/container.rb (4x + comment), app/models/container_request.rb (2x + comment), app/models/job.rb

Yes, sorry. I've rebased dropping the 2 commits that added those: 618cd9c & 6222401

Seems like config/storage.yml isn't needed or is ignored since we don't use ActiveStorage (comment in config/application.rb) ... possibly worth deleting the file, or adding a comment that it's not used?

Removed.

I'm guessing the secret_key_base env var in config/secrets.yml is not actually used because it's overridden by config/arvados_config.rb ... if so, possibly worth adding a comment to that effect? And perhaps it would be slightly safer to set it to rand(1<<255).to_s(36) in prod mode, so that if it doesn't get overridden by the config system in some scenario, it will degrade by rejecting cookies etc., instead of by opening a vulnerability. Or maybe Rails already does this internally / falls back to a different secret if it's empty, in which case it's fine as is.

Updated following your suggestion, thanks!

#16 Updated by Tom Clegg about 1 year ago

Lucas Di Pentima wrote:

In the case of the collection model, the explicit reload is needed because the callback where it's being called is around_update so the instance is not persisted yet at that time.

There aren't any unpersisted changes at this point (because the updates haven't been applied yet) ... however, this runs after the "stash attributes for logging" code, which reads mutable attrs, and therefore ActiveRecord has already decided the record has unpersisted changes... is that right?

I guess what we really want is "discard_updates_and_lock" -- it seems a bit tragic that we need to reload/deserialize a potentially large row from the database just to convince that method that it's okay to discard the resulting attrs -- but oh well, it doesn't seem worth getting stuck on.

LGTM, thanks!

#17 Updated by Lucas Di Pentima about 1 year ago

Updates at 4c417c02b
Test run: https://ci.arvados.org/job/developer-run-tests/1997/

I've used byebug to do some checking on manage_versioning() and indeed self.changes contains the correct attributes to be updated, but I agree with you that an extra database query isn't needed at all, so I replaced reload with restore_attributes.

#18 Updated by Lucas Di Pentima about 1 year ago

Workbench1 rails upgrade status: Last week, I've already done the 5.0 to 5.1 step, now trying to make 5.2 work, I'm having some issues when running controller tests, keep getting errors like:

---------------------------------------------------------------------------------------------------------------------
ApplicationControllerTest: test_#<CollectionsController:0x00005619c1d30730>_show_method_with_anonymous_config_enabled
---------------------------------------------------------------------------------------------------------------------
#<Minitest::Assertion: unexpected invocation: discovery()>
/media/psf/arvados/apps/workbench/app/models/arvados_base.rb:508:in `api_exists?'
/media/psf/arvados/apps/workbench/app/controllers/application_controller.rb:222:in `ensure_arvados_api_exists'
/home/lucas/.rvm/gems/ruby-2.5.5@arvados-tests-a463ce2708c16631/gems/activesupport-5.2.4.3/lib/active_support/callbacks.rb:426:in `block in make_lambda'
/home/lucas/.rvm/gems/ruby-2.5.5@arvados-tests-a463ce2708c16631/gems/activesupport-5.2.4.3/lib/active_support/callbacks.rb:179:in `block (2 levels) in halting_and_conditional'
/home/lucas/.rvm/gems/ruby-2.5.5@arvados-tests-a463ce2708c16631/gems/actionpack-5.2.4.3/lib/abstract_controller/callbacks.rb:34:in `block (2 levels) in <module:Callbacks>'
[...]

#19 Updated by Lucas Di Pentima about 1 year ago

  • Target version changed from 2020-08-12 Sprint to 2020-08-26 Sprint

#20 Updated by Lucas Di Pentima about 1 year ago

Workbench upgrade to Rails 5.2 at 3246c891f - branch 16470-wb-rails-52
Test run: https://ci.arvados.org/job/developer-run-tests/2000/

This is ready for review, finally!

#21 Updated by Lucas Di Pentima about 1 year ago

Update at 56766d2
Test run: https://ci.arvados.org/job/developer-run-tests-services-api/2096/

Remove preload_all_models.rb initializer, used on development mode, as it was forcing ActiveRecord::Base.connection to be accessed on rake tasks such as db:create and it was failing because the database wasn't created, for example when running a new arvbox instance.
It seems that the problem this initializer was fixing 7 years ago, doesn't exist anymore.

#22 Updated by Lucas Di Pentima about 1 year ago

Now, I'm testing the upgraded workbench on arvbox, and it's failing:

...
2020-08-17_18:54:19.88411 Using capybara 2.5.0
2020-08-17_18:54:19.88415 0:  capybara (2.5.0) from /var/lib/gems/ruby/2.3.0/specifications/capybara-2.5.0.gemspec
2020-08-17_18:54:19.93097 Gem::InstallError: launchy requires Ruby version >= 2.4.0.
2020-08-17_18:54:19.93099 /usr/lib/ruby/2.3.0/rubygems/installer.rb:607:in
2020-08-17_18:54:19.93099 `ensure_required_ruby_version_met'
2020-08-17_18:54:19.93099   /usr/lib/ruby/2.3.0/rubygems/installer.rb:849:in `pre_install_checks'
2020-08-17_18:54:19.93100   /usr/lib/ruby/2.3.0/rubygems/installer.rb:278:in `install'
2020-08-17_18:54:19.93100   /usr/lib/ruby/vendor_ruby/bundler/source/rubygems.rb:144:in `block in install'
2020-08-17_18:54:19.93100 /usr/lib/ruby/vendor_ruby/bundler/rubygems_integration.rb:181:in
2020-08-17_18:54:19.93101 `preserve_paths'
2020-08-17_18:54:19.93101   /usr/lib/ruby/vendor_ruby/bundler/source/rubygems.rb:136:in `install'
2020-08-17_18:54:19.93101   /usr/lib/ruby/vendor_ruby/bundler/installer/gem_installer.rb:55:in `install'
2020-08-17_18:54:19.93101 /usr/lib/ruby/vendor_ruby/bundler/installer/gem_installer.rb:15:in
2020-08-17_18:54:19.93102 `install_from_spec'
2020-08-17_18:54:19.93102 /usr/lib/ruby/vendor_ruby/bundler/installer/parallel_installer.rb:104:in
2020-08-17_18:54:19.93102 `block in worker_pool'
2020-08-17_18:54:19.93102   /usr/lib/ruby/vendor_ruby/bundler/worker.rb:65:in `apply_func'
2020-08-17_18:54:19.93103   /usr/lib/ruby/vendor_ruby/bundler/worker.rb:60:in `block in process_queue'
2020-08-17_18:54:19.93103   /usr/lib/ruby/vendor_ruby/bundler/worker.rb:57:in `loop'
2020-08-17_18:54:19.93103   /usr/lib/ruby/vendor_ruby/bundler/worker.rb:57:in `process_queue'
2020-08-17_18:54:19.93104 /usr/lib/ruby/vendor_ruby/bundler/worker.rb:29:in `block (2 levels) in
2020-08-17_18:54:19.93105 initialize'
2020-08-17_18:54:19.93105
2020-08-17_18:54:19.93105 An error occurred while installing launchy (2.5.0), and Bundler cannot continue.
...

I'll check which ruby version we're using on arvbox.

#23 Updated by Lucas Di Pentima about 1 year ago

Updates at 0d08f2b - branch 16470-wb-rails-52 (rebased)
Test run: https://ci.arvados.org/job/developer-run-tests/2011/
API Test re-run: https://ci.arvados.org/job/developer-run-tests-services-api/2099/

  • Removes duplicated byebug on Gemfile.
  • Pins launchy to avoid requiring ruby >= 2.4.0 (arvbox doesn't have it yet).
  • Rebased against latest version of 16470-api-rails-52 branch.

#24 Updated by Tom Clegg about 1 year ago

It looks like 346d4c6a4eaf280466908e9c56af0c7dae1d8aac was reintroduced here -- was this by accident during rebase?

The rest LGTM, thanks! Sorry for the delay.

#25 Updated by Anonymous about 1 year ago

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

#26 Updated by Ward Vandewege 11 months ago

  • Related to Bug #17094: [api] script/rails is broken with 'cannot load such file -- rails/commands/console' added

#27 Updated by Tom Clegg 6 months ago

  • Related to Bug #17528: [Rails] package prints a backtrace on installation when the arvados config.yml file is not present/complete added

Also available in: Atom PDF