From: Lucas Di Pentima Date: Tue, 28 May 2019 21:59:16 +0000 (-0300) Subject: Merge branch 'master' into 14946-ruby-2.5 X-Git-Tag: 2.0.0~308^2~4 X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/ac2785f7f3932ffa4988b6752482f0c1c6883c02?hp=c4107b1da872fe7df4c73de1a1d496bbfa1290dc Merge branch 'master' into 14946-ruby-2.5 Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima --- diff --git a/build/run-library.sh b/build/run-library.sh index 141c49e621..1aa3e3cfd1 100755 --- a/build/run-library.sh +++ b/build/run-library.sh @@ -638,10 +638,13 @@ fpm_build_virtualenv () { done fi - # the libpam module should place this file in the historically correct place - # so as not to break backwards compatibility - if [[ -e "$WORKSPACE/$PKG_DIR/dist/build/usr/share/python2.7/dist/libpam-arvados/lib/security/libpam_arvados.py" ]]; then - COMMAND_ARR+=("usr/share/$python/dist/$PYTHON_PKG/data/lib/security/libpam_arvados.py=/usr/data/lib/security/") + # the libpam module should place a few files in the correct place for the pam + # subsystem + if [[ -e "$WORKSPACE/$PKG_DIR/dist/build/usr/share/$python/dist/$PYTHON_PKG/lib/security/libpam_arvados.py" ]]; then + COMMAND_ARR+=("usr/share/$python/dist/$PYTHON_PKG/lib/security/libpam_arvados.py=/usr/lib/security/") + fi + if [[ -e "$WORKSPACE/$PKG_DIR/dist/build/usr/share/$python/dist/$PYTHON_PKG/share/pam-configs/arvados" ]]; then + COMMAND_ARR+=("usr/share/$python/dist/$PYTHON_PKG/share/pam-configs/arvados=/usr/share/pam-configs/") fi # the python-arvados-cwl-runner package comes with cwltool, expose that version diff --git a/lib/config/config.default.yml b/lib/config/config.default.yml index 16d6c61b5e..bd4c3521d1 100644 --- a/lib/config/config.default.yml +++ b/lib/config/config.default.yml @@ -69,11 +69,11 @@ Clusters: Connection: # All parameters here are passed to the PG client library in a connection string; # see https://www.postgresql.org/docs/current/static/libpq-connect.html#LIBPQ-PARAMKEYWORDS - Host: "" - Port: "" - User: "" - Password: "" - DBName: "" + host: "" + port: "" + user: "" + password: "" + dbname: "" API: # Maximum size (in bytes) allowed for a single API request. This # limit is published in the discovery document for use by clients. diff --git a/lib/config/generated_config.go b/lib/config/generated_config.go index 5ee62ee82e..d51cb9935f 100644 --- a/lib/config/generated_config.go +++ b/lib/config/generated_config.go @@ -75,11 +75,11 @@ Clusters: Connection: # All parameters here are passed to the PG client library in a connection string; # see https://www.postgresql.org/docs/current/static/libpq-connect.html#LIBPQ-PARAMKEYWORDS - Host: "" - Port: "" - User: "" - Password: "" - DBName: "" + host: "" + port: "" + user: "" + password: "" + dbname: "" API: # Maximum size (in bytes) allowed for a single API request. This # limit is published in the discovery document for use by clients. diff --git a/lib/config/load_test.go b/lib/config/load_test.go index 2bf341fe1d..d72c762b48 100644 --- a/lib/config/load_test.go +++ b/lib/config/load_test.go @@ -59,8 +59,8 @@ Clusters: zzzzz: postgresql: connection: - dbname: dbname - host: host + DBName: dbname + Host: host `), ctxlog.TestLogger(c)) c.Check(err, check.ErrorMatches, `Clusters.zzzzz.PostgreSQL.Connection: multiple entries for "(dbname|host)".*`) } diff --git a/lib/service/cmd.go b/lib/service/cmd.go index 4b7341d729..955b21f9ea 100644 --- a/lib/service/cmd.go +++ b/lib/service/cmd.go @@ -10,6 +10,7 @@ import ( "flag" "fmt" "io" + "io/ioutil" "net/http" "net/url" "os" @@ -70,7 +71,10 @@ func (c *command) RunCommand(prog string, args []string, stdin io.Reader, stdout } else if err != nil { return 2 } - cfg, err := config.LoadFile(*configFile, log) + // Logged warnings are discarded for now: the config template + // is incomplete, which causes extra warnings about keys that + // are really OK. + cfg, err := config.LoadFile(*configFile, ctxlog.New(ioutil.Discard, "json", "error")) if err != nil { return 1 } diff --git a/sdk/cwl/arvados_cwl/arvcontainer.py b/sdk/cwl/arvados_cwl/arvcontainer.py index ec144abf3e..f8f1f30f63 100644 --- a/sdk/cwl/arvados_cwl/arvcontainer.py +++ b/sdk/cwl/arvados_cwl/arvcontainer.py @@ -527,6 +527,7 @@ class RunnerContainer(Runner): container = self.arvrunner.api.containers().get( uuid=record["container_uuid"] ).execute(num_retries=self.arvrunner.num_retries) + container["log"] = record["log_uuid"] except Exception: logger.exception("%s while getting runner container", self.arvrunner.label(self)) self.arvrunner.output_callback({}, "permanentFail") diff --git a/sdk/pam/setup.py b/sdk/pam/setup.py index c94f5b41f5..9bc2cef601 100755 --- a/sdk/pam/setup.py +++ b/sdk/pam/setup.py @@ -42,13 +42,6 @@ setup(name='arvados-pam', ('share/pam-configs', ['pam-configs/arvados']), ('share/doc/arvados-pam', ['LICENSE-2.0.txt', 'README.rst']), ('share/doc/arvados-pam/examples', glob.glob('examples/*')), - - # The arvados build scripts used to install data files to - # "/usr/data/*" but now install them to "/usr/*". Here, we - # install an extra copy in the old location so existing pam - # configs can still work. When old systems have had a chance - # to update to the new paths, this line can be removed. - ('data/lib/security', ['lib/libpam_arvados.py']), ], install_requires=[ 'arvados-python-client>=0.1.20150801000000', diff --git a/sdk/python/tests/run_test_server.py b/sdk/python/tests/run_test_server.py index 79767c2fa5..a36bbfc6fd 100644 --- a/sdk/python/tests/run_test_server.py +++ b/sdk/python/tests/run_test_server.py @@ -418,10 +418,10 @@ Clusters: PostgreSQL: ConnectionPool: 32 Connection: - Host: {} - DBName: {} - User: {} - Password: {} + host: {} + dbname: {} + user: {} + password: {} NodeProfiles: "*": "arvados-controller": diff --git a/services/api/app/models/collection.rb b/services/api/app/models/collection.rb index 22c2603567..775ebdb494 100644 --- a/services/api/app/models/collection.rb +++ b/services/api/app/models/collection.rb @@ -33,7 +33,7 @@ class Collection < ArvadosModel validate :past_versions_cannot_be_updated, on: :update after_validation :set_file_count_and_total_size before_save :set_file_names - around_update :manage_versioning + around_update :manage_versioning, unless: :is_past_version? api_accessible :user, extend: :common do |t| t.add :name @@ -281,8 +281,11 @@ class Collection < ArvadosModel sync_past_versions if syncable_updates.any? if snapshot snapshot.attributes = self.syncable_updates - snapshot.manifest_text = snapshot.signed_manifest_text - snapshot.save + leave_modified_by_user_alone do + act_as_system_user do + snapshot.save + end + end end end end @@ -304,7 +307,7 @@ class Collection < ArvadosModel updates = self.syncable_updates Collection.where('current_version_uuid = ? AND uuid != ?', self.uuid_was, self.uuid_was).each do |c| c.attributes = updates - # Use a different validation context to skip the 'old_versions_cannot_be_updated' + # Use a different validation context to skip the 'past_versions_cannot_be_updated' # validator, as on this case it is legal to update some fields. leave_modified_by_user_alone do leave_modified_at_alone do @@ -322,9 +325,17 @@ class Collection < ArvadosModel ['uuid', 'owner_uuid', 'delete_at', 'trash_at', 'is_trashed', 'replication_desired', 'storage_classes_desired'] end + def is_past_version? + # Check for the '_was' values just in case the update operation + # includes a change on current_version_uuid or uuid. + !(new_record? || self.current_version_uuid_was == self.uuid_was) + end + def should_preserve_version? return false unless (Rails.configuration.Collections.CollectionVersioning && versionable_updates?(self.changes.keys)) + return false if self.is_trashed + idle_threshold = Rails.configuration.Collections.PreserveVersionIfIdle if !self.preserve_version_was && (idle_threshold < 0 || @@ -650,9 +661,7 @@ class Collection < ArvadosModel end def past_versions_cannot_be_updated - # We check for the '_was' values just in case the update operation - # includes a change on current_version_uuid or uuid. - if current_version_uuid_was != uuid_was + if is_past_version? errors.add(:base, "past versions cannot be updated") false end @@ -660,7 +669,7 @@ class Collection < ArvadosModel def versioning_metadata_updates valid = true - if (current_version_uuid_was == uuid_was) && current_version_uuid_changed? + if !is_past_version? && current_version_uuid_changed? errors.add(:current_version_uuid, "cannot be updated") valid = false end diff --git a/services/api/app/models/jsonb_type.rb b/services/api/app/models/jsonb_type.rb index 3f296be550..02746f64d4 100644 --- a/services/api/app/models/jsonb_type.rb +++ b/services/api/app/models/jsonb_type.rb @@ -12,6 +12,12 @@ class JsonbType nil end + def changed_in_place?(raw_old_value, value) + # Compare deserialized values for correctness, checking serialized values + # may include changes in ordering, inline whitespaces, etc. + deserialize(raw_old_value) != value + end + def deserialize(value) if value.nil? self.default_value diff --git a/services/api/config/arvados_config.rb b/services/api/config/arvados_config.rb index 8e4151a73e..9ea36315f4 100644 --- a/services/api/config/arvados_config.rb +++ b/services/api/config/arvados_config.rb @@ -171,13 +171,13 @@ arvcfg.declare_config "RemoteClusters.*.Proxy", Boolean, :remote_hosts_via_dns dbcfg = ConfigLoader.new dbcfg.declare_config "PostgreSQL.ConnectionPool", Integer, :pool -dbcfg.declare_config "PostgreSQL.Connection.Host", String, :host -dbcfg.declare_config "PostgreSQL.Connection.Port", String, :port -dbcfg.declare_config "PostgreSQL.Connection.User", String, :username -dbcfg.declare_config "PostgreSQL.Connection.Password", String, :password -dbcfg.declare_config "PostgreSQL.Connection.DBName", String, :database -dbcfg.declare_config "PostgreSQL.Connection.Template", String, :template -dbcfg.declare_config "PostgreSQL.Connection.Encoding", String, :encoding +dbcfg.declare_config "PostgreSQL.Connection.host", String, :host +dbcfg.declare_config "PostgreSQL.Connection.port", String, :port +dbcfg.declare_config "PostgreSQL.Connection.user", String, :username +dbcfg.declare_config "PostgreSQL.Connection.password", String, :password +dbcfg.declare_config "PostgreSQL.Connection.dbname", String, :database +dbcfg.declare_config "PostgreSQL.Connection.template", String, :template +dbcfg.declare_config "PostgreSQL.Connection.encoding", String, :encoding application_config = {} %w(application.default application).each do |cfgfile| @@ -239,16 +239,16 @@ end # rails environments. # if ::Rails.env.to_s == "test" && db_config["test"].nil? - $arvados_config["PostgreSQL"]["Connection"]["DBName"] = "arvados_test" + $arvados_config["PostgreSQL"]["Connection"]["dbname"] = "arvados_test" end -if $arvados_config["PostgreSQL"]["Connection"]["Password"].empty? +if $arvados_config["PostgreSQL"]["Connection"]["password"].empty? raise "Database password is empty, PostgreSQL section is: #{$arvados_config["PostgreSQL"]}" end -dbhost = $arvados_config["PostgreSQL"]["Connection"]["Host"] -if $arvados_config["PostgreSQL"]["Connection"]["Post"] != 0 - dbhost += ":#{$arvados_config["PostgreSQL"]["Connection"]["Post"]}" +dbhost = $arvados_config["PostgreSQL"]["Connection"]["host"] +if $arvados_config["PostgreSQL"]["Connection"]["port"] != 0 + dbhost += ":#{$arvados_config["PostgreSQL"]["Connection"]["port"]}" end # @@ -257,10 +257,10 @@ end # For config migration, we've previously populated the PostgreSQL # section of the config from database.yml # -ENV["DATABASE_URL"] = "postgresql://#{$arvados_config["PostgreSQL"]["Connection"]["User"]}:"+ - "#{$arvados_config["PostgreSQL"]["Connection"]["Password"]}@"+ - "#{dbhost}/#{$arvados_config["PostgreSQL"]["Connection"]["DBName"]}?"+ - "template=#{$arvados_config["PostgreSQL"]["Connection"]["Template"]}&"+ +ENV["DATABASE_URL"] = "postgresql://#{$arvados_config["PostgreSQL"]["Connection"]["user"]}:"+ + "#{$arvados_config["PostgreSQL"]["Connection"]["password"]}@"+ + "#{dbhost}/#{$arvados_config["PostgreSQL"]["Connection"]["dbname"]}?"+ + "template=#{$arvados_config["PostgreSQL"]["Connection"]["template"]}&"+ "encoding=#{$arvados_config["PostgreSQL"]["Connection"]["client_encoding"]}&"+ "pool=#{$arvados_config["PostgreSQL"]["ConnectionPool"]}" diff --git a/services/api/lib/update_priority.rb b/services/api/lib/update_priority.rb index 21cd74bae6..c688ac008b 100644 --- a/services/api/lib/update_priority.rb +++ b/services/api/lib/update_priority.rb @@ -12,14 +12,19 @@ module UpdatePriority # # If container priority=0 but there are committed container requests # for it with priority>0, update priority. - def self.update_priority + # + # Normally, update_priority is a no-op if another thread/process is + # already updating. Test cases that need to check priorities after + # updating can force a (possibly overlapping) update in the current + # thread/transaction by setting the "nolock" flag. See #14878. + def self.update_priority(nolock: false) if !File.owned?(Rails.root.join('tmp')) Rails.logger.warn("UpdatePriority: not owner of #{Rails.root}/tmp, skipping") return end lockfile = Rails.root.join('tmp', 'update_priority.lock') File.open(lockfile, File::RDWR|File::CREAT, 0600) do |f| - return unless f.flock(File::LOCK_NB|File::LOCK_EX) + return unless nolock || f.flock(File::LOCK_NB|File::LOCK_EX) # priority>0 but should be 0: ActiveRecord::Base.connection. diff --git a/services/api/test/functional/arvados/v1/collections_controller_test.rb b/services/api/test/functional/arvados/v1/collections_controller_test.rb index 089895864a..4e8b0559aa 100644 --- a/services/api/test/functional/arvados/v1/collections_controller_test.rb +++ b/services/api/test/functional/arvados/v1/collections_controller_test.rb @@ -1423,4 +1423,20 @@ EOS assert_response :success assert_equal 3, json_response['version'] end + + test "delete collection with versioning enabled" do + Rails.configuration.Collections.CollectionVersioning = true + Rails.configuration.Collections.PreserveVersionIfIdle = 1 # 1 second + + col = collections(:collection_owned_by_active) + assert_equal 2, col.version + assert col.modified_at < Time.now - 1.second + + authorize_with(:active) + post :trash, params: { + id: col.uuid, + } + assert_response :success + assert_equal col.version, json_response['version'], 'Trashing a collection should not create a new version' + end end diff --git a/services/api/test/unit/collection_test.rb b/services/api/test/unit/collection_test.rb index 8cb5bbd590..08d5b1fb72 100644 --- a/services/api/test/unit/collection_test.rb +++ b/services/api/test/unit/collection_test.rb @@ -266,6 +266,67 @@ class CollectionTest < ActiveSupport::TestCase end end + # This test exposes a bug related to JSONB attributes, see #15725. + test "recently loaded collection shouldn't list changed attributes" do + col = Collection.where("properties != '{}'::jsonb").limit(1).first + refute col.properties_changed?, 'Properties field should not be seen as changed' + end + + [ + [ + true, + {'foo'=>'bar', 'lst'=>[1, 3, 5, 7], 'hsh'=>{'baz'=>'qux', 'foobar'=>true, 'hsh'=>{'nested'=>true}}, 'delete_at'=>nil}, + {:foo=>:bar, :lst=>[1, 3, 5, 7], :hsh=>{'baz'=>'qux', :foobar=>true, 'hsh'=>{:nested=>true}}, :delete_at=>nil}, + ], + [ + true, + {'foo'=>'bar', 'lst'=>[1, 3, 5, 7], 'hsh'=>{'baz'=>'qux', 'foobar'=>true, 'hsh'=>{'nested'=>true}}, 'delete_at'=>nil}, + {'delete_at'=>nil, 'foo'=>'bar', 'lst'=>[1, 3, 5, 7], 'hsh'=>{'baz'=>'qux', 'foobar'=>true, 'hsh'=>{'nested'=>true}}}, + ], + [ + true, + {'foo'=>'bar', 'lst'=>[1, 3, 5, 7], 'hsh'=>{'baz'=>'qux', 'foobar'=>true, 'hsh'=>{'nested'=>true}}, 'delete_at'=>nil}, + {'delete_at'=>nil, 'foo'=>'bar', 'lst'=>[1, 3, 5, 7], 'hsh'=>{'foobar'=>true, 'hsh'=>{'nested'=>true}, 'baz'=>'qux'}}, + ], + [ + false, + {'foo'=>'bar', 'lst'=>[1, 3, 5, 7], 'hsh'=>{'baz'=>'qux', 'foobar'=>true, 'hsh'=>{'nested'=>true}}, 'delete_at'=>nil}, + {'foo'=>'bar', 'lst'=>[1, 3, 5, 42], 'hsh'=>{'baz'=>'qux', 'foobar'=>true, 'hsh'=>{'nested'=>true}}, 'delete_at'=>nil}, + ], + [ + false, + {'foo'=>'bar', 'lst'=>[1, 3, 5, 7], 'hsh'=>{'baz'=>'qux', 'foobar'=>true, 'hsh'=>{'nested'=>true}}, 'delete_at'=>nil}, + {'foo'=>'bar', 'lst'=>[1, 3, 7, 5], 'hsh'=>{'baz'=>'qux', 'foobar'=>true, 'hsh'=>{'nested'=>true}}, 'delete_at'=>nil}, + ], + [ + false, + {'foo'=>'bar', 'lst'=>[1, 3, 5, 7], 'hsh'=>{'baz'=>'qux', 'foobar'=>true, 'hsh'=>{'nested'=>true}}, 'delete_at'=>nil}, + {'foo'=>'bar', 'lst'=>[1, 3, 5, 7], 'hsh'=>{'baz'=>'qux', 'foobar'=>true, 'hsh'=>{'nested'=>false}}, 'delete_at'=>nil}, + ], + [ + false, + {'foo'=>'bar', 'lst'=>[1, 3, 5, 7], 'hsh'=>{'baz'=>'qux', 'foobar'=>true, 'hsh'=>{'nested'=>true}}, 'delete_at'=>nil}, + {'foo'=>'bar', 'lst'=>[1, 3, 5, 7], 'hsh'=>{'baz'=>'qux', 'foobar'=>true, 'hsh'=>{'nested'=>true}}, 'delete_at'=>1234567890}, + ], + ].each do |should_be_equal, value_1, value_2| + test "JSONB properties #{value_1} is#{should_be_equal ? '' : ' not'} equal to #{value_2}" do + act_as_user users(:active) do + # Set up initial collection + c = create_collection 'foo', Encoding::US_ASCII + assert c.valid? + c.update_attributes!({'properties' => value_1}) + c.reload + assert c.changes.keys.empty? + c.properties = value_2 + if should_be_equal + assert c.changes.keys.empty?, "Properties #{value_1.inspect} should be equal to #{value_2.inspect}" + else + refute c.changes.keys.empty?, "Properties #{value_1.inspect} should not be equal to #{value_2.inspect}" + end + end + end + end + test "older versions' modified_at indicate when they're created" do Rails.configuration.Collections.CollectionVersioning = true Rails.configuration.Collections.PreserveVersionIfIdle = 0 @@ -334,7 +395,6 @@ class CollectionTest < ActiveSupport::TestCase ['owner_uuid', 'zzzzz-tpzed-d9tiejq69daie8f', 'zzzzz-tpzed-xurymjxw79nv3jz'], ['replication_desired', 2, 3], ['storage_classes_desired', ['hot'], ['archive']], - ['is_trashed', true, false], ].each do |attr, first_val, second_val| test "sync #{attr} with older versions" do Rails.configuration.Collections.CollectionVersioning = true diff --git a/services/api/test/unit/update_priority_test.rb b/services/api/test/unit/update_priority_test.rb index 2d28d3fb69..c1f60d91d0 100644 --- a/services/api/test/unit/update_priority_test.rb +++ b/services/api/test/unit/update_priority_test.rb @@ -10,13 +10,13 @@ class UpdatePriorityTest < ActiveSupport::TestCase uuid = containers(:running).uuid ActiveRecord::Base.connection.exec_query('UPDATE containers SET priority=0 WHERE uuid=$1', 'test-setup', [[nil, uuid]]) assert_equal 0, Container.find_by_uuid(uuid).priority - UpdatePriority.update_priority + UpdatePriority.update_priority(nolock: true) assert_operator 0, :<, Container.find_by_uuid(uuid).priority uuid = containers(:queued).uuid ActiveRecord::Base.connection.exec_query('UPDATE containers SET priority=0 WHERE uuid=$1', 'test-setup', [[nil, uuid]]) assert_equal 0, Container.find_by_uuid(uuid).priority - UpdatePriority.update_priority + UpdatePriority.update_priority(nolock: true) assert_operator 0, :<, Container.find_by_uuid(uuid).priority end @@ -24,7 +24,7 @@ class UpdatePriorityTest < ActiveSupport::TestCase uuid = containers(:running).uuid ActiveRecord::Base.connection.exec_query('DELETE FROM container_requests WHERE container_uuid=$1', 'test-setup', [[nil, uuid]]) assert_operator 0, :<, Container.find_by_uuid(uuid).priority - UpdatePriority.update_priority + UpdatePriority.update_priority(nolock: true) assert_equal 0, Container.find_by_uuid(uuid).priority end end diff --git a/tools/arvbox/lib/arvbox/docker/service/controller/run b/tools/arvbox/lib/arvbox/docker/service/controller/run index 37052e1233..986ad84966 100755 --- a/tools/arvbox/lib/arvbox/docker/service/controller/run +++ b/tools/arvbox/lib/arvbox/docker/service/controller/run @@ -63,10 +63,10 @@ Clusters: Connection: # All parameters here are passed to the PG client library in a connection string; # see https://www.postgresql.org/docs/current/static/libpq-connect.html#LIBPQ-PARAMKEYWORDS - Host: localhost - User: arvados - Password: ${database_pw} - DBName: arvados_${database_env} + host: localhost + user: arvados + password: ${database_pw} + dbname: arvados_${database_env} client_encoding: utf8 API: RailsSessionSecretToken: $secret_token