Merge branch 'mk/enforce-not-null-package-files-file-store' into 'master'

Geo: Part 1 of Enforce not null packages_package_files.file_store

See merge request gitlab-org/gitlab!38822

changelogs/unreleased/mk-enforce-not-null-package-files-file-store.yml

+---
+title: Add database migrations to ensure Geo replicates all package files when sync
+  object storage is disabled
+merge_request: 38822
+author:
+type: added

db/migrate/20200806004232_add_default_value_for_file_store_to_package_files.rb

+# frozen_string_literal: true
+
+class AddDefaultValueForFileStoreToPackageFiles < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    with_lock_retries do
+      change_column_default :packages_package_files, :file_store, 1
+    end
+  end
+
+  def down
+    with_lock_retries do
+      change_column_default :packages_package_files, :file_store, nil
+    end
+  end
+end

db/post_migrate/20200806004742_add_not_null_constraint_on_file_store_to_package_files.rb

+# frozen_string_literal: true
+
+class AddNotNullConstraintOnFileStoreToPackageFiles < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_not_null_constraint(:packages_package_files, :file_store, validate: false)
+  end
+
+  def down
+    remove_not_null_constraint(:packages_package_files, :file_store)
+  end
+end

db/post_migrate/20200806172909_add_partial_index_on_id_to_package_files.rb

+# frozen_string_literal: true
+
+class AddPartialIndexOnIdToPackageFiles < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  INDEX_NAME = 'index_packages_package_files_file_store_is_null'
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :packages_package_files, :id, where: 'file_store IS NULL', name: INDEX_NAME
+  end
+
+  def down
+    remove_concurrent_index_by_name :packages_package_files, INDEX_NAME
+  end
+end

db/post_migrate/20200806173633_migrate_null_package_files_file_store_to_local_value.rb

+# frozen_string_literal: true
+
+class MigrateNullPackageFilesFileStoreToLocalValue < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  JOB_INTERVAL = 2.minutes + 5.seconds
+  BATCH_SIZE = 5_000
+  MIGRATION = 'SetNullPackageFilesFileStoreToLocalValue'
+
+  disable_ddl_transaction!
+
+  class PackageFile < ActiveRecord::Base
+    self.table_name = 'packages_package_files'
+
+    include ::EachBatch
+  end
+
+  def up
+    # On GitLab.com, there are 2M package files. None have NULL file_store
+    # because they are all object stored. This is a no-op for GitLab.com.
+    #
+    # If a customer had 2M package files with NULL file_store, with batches of
+    # 5000 and a background migration job interval of 2m 5s, then 400 jobs would
+    # be scheduled over 14 hours.
+    #
+    # The index `index_packages_package_files_file_store_is_null` is
+    # expected to be used here and in the jobs.
+    #
+    # queue_background_migration_jobs_by_range_at_intervals is not used because
+    # it would enqueue 18.6K jobs and we have an index for getting these ranges.
+    PackageFile.where(file_store: nil).each_batch(of: BATCH_SIZE) do |batch, index|
+      range = batch.pluck(Arel.sql("MIN(id)"), Arel.sql("MAX(id)")).first
+      delay = index * JOB_INTERVAL
+
+      migrate_in(delay.seconds, MIGRATION, [*range])
+    end
+  end
+
+  def down
+    # noop
+  end
+end

db/schema_migrations/20200806004232

+4b1f048dfaea1887b20fdc421a08ab6206ab9944201e6517e808b27214be926c
\ No newline at end of file

db/schema_migrations/20200806004742

+acc9d1ab79e277e55910b17c5653088721371fa12b2cf1a5677035fe3b422fc8
\ No newline at end of file

db/schema_migrations/20200806172909

+b05401408faabafa4c2ef5fd241fea88f78fd423a3c462be89ccdbd8d988cfc8
\ No newline at end of file

db/schema_migrations/20200806173633

+5467b9c38186a30d333d9ccff0aeb4a06f97f17fec12488aca5e0a619b11831b
\ No newline at end of file

db/structure.sql

@@ -13874,7 +13874,7 @@ CREATE TABLE public.packages_package_files (
     created_at timestamp with time zone NOT NULL,
     updated_at timestamp with time zone NOT NULL,
     size bigint,
-    file_store integer,
+    file_store integer DEFAULT 1,
     file_md5 bytea,
     file_sha1 bytea,
     file_name character varying NOT NULL,
@@ -17699,6 +17699,9 @@ ALTER TABLE public.design_management_designs
 ALTER TABLE public.vulnerability_scanners
     ADD CONSTRAINT check_37608c9db5 CHECK ((char_length(vendor) <= 255)) NOT VALID;
 
+ALTER TABLE public.packages_package_files
+    ADD CONSTRAINT check_4c5e6bb0b3 CHECK ((file_store IS NOT NULL)) NOT VALID;
+
 ALTER TABLE public.merge_request_diffs
     ADD CONSTRAINT check_93ee616ac9 CHECK ((external_diff_store IS NOT NULL)) NOT VALID;
 
@@ -20205,6 +20208,8 @@ CREATE INDEX index_packages_maven_metadata_on_package_id_and_path ON public.pack
 
 CREATE INDEX index_packages_nuget_dl_metadata_on_dependency_link_id ON public.packages_nuget_dependency_link_metadata USING btree (dependency_link_id);
 
+CREATE INDEX index_packages_package_files_file_store_is_null ON public.packages_package_files USING btree (id) WHERE (file_store IS NULL);
+
 CREATE INDEX index_packages_package_files_on_file_store ON public.packages_package_files USING btree (file_store);
 
 CREATE INDEX index_packages_package_files_on_package_id_and_file_name ON public.packages_package_files USING btree (package_id, file_name);

lib/gitlab/background_migration/set_null_package_files_file_store_to_local_value.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    # This class is responsible for migrating a range of package files
+    # with file_store == NULL to 1.
+    #
+    # The index `index_packages_package_files_file_store_is_null` is
+    # expected to be used to find the rows here and in the migration scheduling
+    # the jobs that run this class.
+    class SetNullPackageFilesFileStoreToLocalValue
+      LOCAL_STORE = 1 # equal to ObjectStorage::Store::LOCAL
+
+      # Temporary AR class for package files
+      class PackageFile < ActiveRecord::Base
+        self.table_name = 'packages_package_files'
+      end
+
+      def perform(start_id, stop_id)
+        Packages::PackageFile.where(file_store: nil, id: start_id..stop_id).update_all(file_store: LOCAL_STORE)
+      end
+    end
+  end
+end

spec/lib/gitlab/background_migration/set_null_package_files_file_store_to_local_value_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+# The test setup must begin before
+# 20200806004742_add_not_null_constraint_on_file_store_to_package_files.rb
+# has run, or else we cannot insert a row with `NULL` `file_store` to
+# test against.
+RSpec.describe Gitlab::BackgroundMigration::SetNullPackageFilesFileStoreToLocalValue, schema: 20200806004232 do
+  let!(:packages_package_files) { table(:packages_package_files) }
+  let!(:packages_packages)      { table(:packages_packages) }
+  let!(:projects)               { table(:projects) }
+  let!(:namespaces)             { table(:namespaces) }
+  let!(:namespace)              { namespaces.create!(name: 'foo', path: 'foo') }
+  let!(:project)                { projects.create!(namespace_id: namespace.id) }
+  let!(:package)                { packages_packages.create!(project_id: project.id, name: 'bar', package_type: 1) }
+
+  it 'correctly migrates nil file_store to 1' do
+    file_store_1 = packages_package_files.create!(file_store: 1, file_name: 'foo_1', file: 'foo_1', package_id: package.id)
+    file_store_2 = packages_package_files.create!(file_store: 2, file_name: 'foo_2', file: 'foo_2', package_id: package.id)
+    file_store_nil = packages_package_files.create!(file_store: nil, file_name: 'foo_nil', file: 'foo_nil', package_id: package.id)
+
+    described_class.new.perform(file_store_1.id, file_store_nil.id)
+
+    file_store_1.reload
+    file_store_2.reload
+    file_store_nil.reload
+
+    expect(file_store_1.file_store).to eq(1)   # unchanged
+    expect(file_store_2.file_store).to eq(2)   # unchanged
+    expect(file_store_nil.file_store).to eq(1) # nil => 1
+  end
+end

spec/models/packages/package_file_spec.rb

@@ -58,7 +58,7 @@ RSpec.describe Packages::PackageFile, type: :model do
   end
 
   describe '#update_file_metadata callback' do
-    let_it_be(:package_file) { build(:package_file, :nuget, file_store: nil, size: nil) }
+    let_it_be(:package_file) { build(:package_file, :nuget, size: nil) }
 
     subject { package_file.save! }
 
@@ -67,9 +67,14 @@ RSpec.describe Packages::PackageFile, type: :model do
         .to receive(:update_file_metadata)
         .and_call_original
 
-      expect { subject }
-        .to change { package_file.file_store }.from(nil).to(::Packages::PackageFileUploader::Store::LOCAL)
-        .and change { package_file.size }.from(nil).to(3513)
+      # This expectation uses a stub because we can no longer test a change from
+      # `nil` to `1`, because the field is no longer nullable, and it defaults
+      # to `1`.
+      expect(package_file)
+        .to receive(:update_column)
+        .with(:file_store, ::Packages::PackageFileUploader::Store::LOCAL)
+
+      expect { subject }.to change { package_file.size }.from(nil).to(3513)
     end
   end
 end