Arvados

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.

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.

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

Test run: developer-run-tests: #1987
Test remainder re-run: developer-run-tests-remainder: #2075

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

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.

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)

Updates at f4cb214f1
Test run: 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?

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!

Updates at 601249b (rebased branch)
Test run: 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!

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!

Updates at 4c417c02b
Test run: 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.

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>'
[...]

Workbench upgrade to Rails 5.2 at 3246c891f - branch 16470-wb-rails-52
Test run: developer-run-tests: #2000

This is ready for review, finally!

Update at 56766d2
Test run: 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.

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.

Updates at 0d08f2b - branch 16470-wb-rails-52 (rebased)
Test run: developer-run-tests: #2011
API Test re-run: 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.

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

The rest LGTM, thanks! Sorry for the delay.