Merge branch 'master' into 14946-ruby-2.5
authorLucas Di Pentima <ldipentima@veritasgenetics.com>
Tue, 28 May 2019 21:59:16 +0000 (18:59 -0300)
committerLucas Di Pentima <ldipentima@veritasgenetics.com>
Tue, 28 May 2019 21:59:16 +0000 (18:59 -0300)
Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <ldipentima@veritasgenetics.com>

16 files changed:
build/run-library.sh
lib/config/config.default.yml
lib/config/generated_config.go
lib/config/load_test.go
lib/service/cmd.go
sdk/cwl/arvados_cwl/arvcontainer.py
sdk/pam/setup.py
sdk/python/tests/run_test_server.py
services/api/app/models/collection.rb
services/api/app/models/jsonb_type.rb
services/api/config/arvados_config.rb
services/api/lib/update_priority.rb
services/api/test/functional/arvados/v1/collections_controller_test.rb
services/api/test/unit/collection_test.rb
services/api/test/unit/update_priority_test.rb
tools/arvbox/lib/arvbox/docker/service/controller/run

index 141c49e62182a28030ce9690727993dfe06e6610..1aa3e3cfd1147ebe15b0c041637a055b015f3c93 100755 (executable)
@@ -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
index 16d6c61b5e83c49086f163a53ba98dc23793865f..bd4c3521d17bfeba58d851a42768c290ed229f27 100644 (file)
@@ -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.
index 5ee62ee82e7801af4dece12e33a2ab9f76f57f67..d51cb9935fce825450d0995a6d51edb16bd55013 100644 (file)
@@ -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.
index 2bf341fe1deda02369930e52427463ec160e70cc..d72c762b4846ce99ba327a4192d4fadd40f80802 100644 (file)
@@ -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)".*`)
 }
index 4b7341d7294d44a94f6422534dfc8780eab0c7db..955b21f9ea0ef1ac21bc069f67079a767aabac1e 100644 (file)
@@ -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
        }
index ec144abf3ee36adfe5e8594e4cd1219632f79528..f8f1f30f633bd2f7fa9600ae77736ad2634ac3b4 100644 (file)
@@ -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")
index c94f5b41f5b674beaf7b41989d6ec6f0a84329e9..9bc2cef60169b674a51d6ae3cffc24662aaa78df 100755 (executable)
@@ -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',
index 79767c2fa5b007f267ad99a72c5445a909862d05..a36bbfc6fd52eb84446c611ce7c549af300c9828 100644 (file)
@@ -418,10 +418,10 @@ Clusters:
     PostgreSQL:
       ConnectionPool: 32
       Connection:
-        Host: {}
-        DBName: {}
-        User: {}
-        Password: {}
+        host: {}
+        dbname: {}
+        user: {}
+        password: {}
     NodeProfiles:
       "*":
         "arvados-controller":
index 22c260356702dcafb9f6b80dafe3532172b1b23a..775ebdb49486861d73f20b97ba562d77656de765 100644 (file)
@@ -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
index 3f296be55003328f0aa2f81a396256bf89e72297..02746f64d4cc56bafe1ee429a2ecf4d4ec0049c6 100644 (file)
@@ -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
index 8e4151a73e5d1daa7c2e4da89d4bbe0ca5bd96a0..9ea36315f4950c186fd91d3a693f0e84c635c004 100644 (file)
@@ -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"]}"
 
index 21cd74bae67d3cbf94257284aedf05ab20aaad57..c688ac008b44b21944e86b36cdb3abbb15273e12 100644 (file)
@@ -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.
index 089895864adc3f9322eb6f97ca5244af021e876d..4e8b0559aabf519ac9b598e385f1a5432a9a9f85 100644 (file)
@@ -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
index 8cb5bbd5903251a45a7877a4aa2bc8ebc8af1ac3..08d5b1fb72cb9544ba8ae651e0936826462703d3 100644 (file)
@@ -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
index 2d28d3fb690e50c5702635fb772dd5d54bec9ad8..c1f60d91d02467571c8d9c720f0534d8164a9cfd 100644 (file)
@@ -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
index 37052e1233c70879a1b5db928cd243158249a91b..986ad84966b53554550f78db6f52df96000a1793 100755 (executable)
@@ -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