Merge branch '18792-background-migration' into 'master'

Background migration to copy projects.container_registry_enabled to project_features.container_registry_access_level [RUN ALL RSPEC]

See merge request gitlab-org/gitlab!55327

app/models/project.rb

@@ -95,6 +95,9 @@ class Project < ApplicationRecord
 
   before_save :ensure_runners_token
 
+  # https://api.rubyonrails.org/v6.0.3.4/classes/ActiveRecord/AttributeMethods/Dirty.html#method-i-will_save_change_to_attribute-3F
+  before_update :set_container_registry_access_level, if: :will_save_change_to_container_registry_enabled?
+
   after_save :update_project_statistics, if: :saved_change_to_namespace_id?
 
   after_save :create_import_state, if: ->(project) { project.import? && project.import_state.nil? }
@@ -2555,6 +2558,20 @@ class Project < ApplicationRecord
 
   private
 
+  def set_container_registry_access_level
+    # changes_to_save = { 'container_registry_enabled' => [value_before_update, value_after_update] }
+    value = changes_to_save['container_registry_enabled'][1]
+
+    access_level =
+      if value
+        ProjectFeature::ENABLED
+      else
+        ProjectFeature::DISABLED
+      end
+
+    project_feature.update!(container_registry_access_level: access_level)
+  end
+
   def find_service(services, name)
     services.find { |service| service.to_param == name }
   end

app/models/project_feature.rb

@@ -43,6 +43,8 @@ class ProjectFeature < ApplicationRecord
     end
   end
 
+  before_create :set_container_registry_access_level
+
   # Default scopes force us to unscope here since a service may need to check
   # permissions for a project in pending_delete
   # http://stackoverflow.com/questions/1540645/how-to-disable-default-scope-for-a-belongs-to
@@ -87,6 +89,15 @@ class ProjectFeature < ApplicationRecord
 
   private
 
+  def set_container_registry_access_level
+    self.container_registry_access_level =
+      if project&.container_registry_enabled
+        ENABLED
+      else
+        DISABLED
+      end
+  end
+
   # Validates builds and merge requests access level
   # which cannot be higher than repository access level
   def repository_children_level

changelogs/unreleased/18792-background-migration.yml

+---
+title: Add a background migration to copy projects.container_registry_enabled to project_features.container_registry_access_level
+merge_request: 55327
+author:
+type: added

db/post_migrate/20210226120851_move_container_registry_enabled_to_project_features.rb

+# frozen_string_literal: true
+
+class MoveContainerRegistryEnabledToProjectFeatures < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  BATCH_SIZE = 50_000
+  MIGRATION = 'MoveContainerRegistryEnabledToProjectFeature'
+
+  disable_ddl_transaction!
+
+  class Project < ActiveRecord::Base
+    include EachBatch
+    self.table_name = 'projects'
+  end
+
+  def up
+    queue_background_migration_jobs_by_range_at_intervals(Project, MIGRATION, 2.minutes, batch_size: BATCH_SIZE)
+  end
+
+  def down
+    # no-op
+  end
+end

db/schema_migrations/20210226120851

+ced4e314f2653ff56a946d334b4cb12085825b8d21ceea42cb686bef688b714c
\ No newline at end of file

lib/gitlab/background_migration/move_container_registry_enabled_to_project_feature.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    # This migration moves projects.container_registry_enabled values to
+    # project_features.container_registry_access_level for the projects within
+    # the given range of ids.
+    class MoveContainerRegistryEnabledToProjectFeature
+      MAX_BATCH_SIZE = 1_000
+
+      module Migratable
+        # Migration model namespace isolated from application code.
+        class ProjectFeature < ActiveRecord::Base
+          ENABLED = 20
+          DISABLED = 0
+        end
+      end
+
+      def perform(from_id, to_id)
+        (from_id..to_id).each_slice(MAX_BATCH_SIZE) do |batch|
+          process_batch(batch.first, batch.last)
+        end
+      end
+
+      private
+
+      def process_batch(from_id, to_id)
+        ActiveRecord::Base.connection.execute(update_sql(from_id, to_id))
+
+        logger.info(message: "#{self.class}: Copied container_registry_enabled values for projects with IDs between #{from_id}..#{to_id}")
+      end
+
+      # For projects that have a project_feature:
+      # Set project_features.container_registry_access_level to ENABLED (20) or DISABLED (0)
+      #   depending if container_registry_enabled is true or false.
+      def update_sql(from_id, to_id)
+        <<~SQL
+        UPDATE project_features
+        SET container_registry_access_level = (CASE p.container_registry_enabled
+                                              WHEN true THEN #{ProjectFeature::ENABLED}
+                                              WHEN false THEN #{ProjectFeature::DISABLED}
+                                              ELSE #{ProjectFeature::DISABLED}
+                                              END)
+        FROM projects p
+        WHERE project_id = p.id AND
+        project_id BETWEEN #{from_id} AND #{to_id}
+        SQL
+      end
+
+      def logger
+        @logger ||= Gitlab::BackgroundMigration::Logger.build
+      end
+    end
+  end
+end

spec/lib/gitlab/background_migration/move_container_registry_enabled_to_project_feature_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::BackgroundMigration::MoveContainerRegistryEnabledToProjectFeature, :migration, schema: 2021_02_26_120851 do
+  let(:enabled) { 20 }
+  let(:disabled) { 0 }
+
+  let(:namespaces) { table(:namespaces) }
+  let(:project_features) { table(:project_features) }
+  let(:projects) { table(:projects) }
+
+  let(:namespace) { namespaces.create!(name: 'user', path: 'user') }
+  let!(:project1) { projects.create!(namespace_id: namespace.id) }
+  let!(:project2) { projects.create!(namespace_id: namespace.id) }
+  let!(:project3) { projects.create!(namespace_id: namespace.id) }
+  let!(:project4) { projects.create!(namespace_id: namespace.id) }
+
+  # pages_access_level cannot be null.
+  let(:non_null_project_features) { { pages_access_level: enabled } }
+  let!(:project_feature1) { project_features.create!(project_id: project1.id, **non_null_project_features) }
+  let!(:project_feature2) { project_features.create!(project_id: project2.id, **non_null_project_features) }
+  let!(:project_feature3) { project_features.create!(project_id: project3.id, **non_null_project_features) }
+
+  describe '#perform' do
+    before do
+      project1.update!(container_registry_enabled: true)
+      project2.update!(container_registry_enabled: false)
+      project3.update!(container_registry_enabled: nil)
+      project4.update!(container_registry_enabled: true)
+    end
+
+    it 'copies values to project_features' do
+      expect(project1.container_registry_enabled).to eq(true)
+      expect(project2.container_registry_enabled).to eq(false)
+      expect(project3.container_registry_enabled).to eq(nil)
+      expect(project4.container_registry_enabled).to eq(true)
+
+      expect(project_feature1.container_registry_access_level).to eq(disabled)
+      expect(project_feature2.container_registry_access_level).to eq(disabled)
+      expect(project_feature3.container_registry_access_level).to eq(disabled)
+
+      expect_next_instance_of(Gitlab::BackgroundMigration::Logger) do |logger|
+        expect(logger).to receive(:info)
+          .with(message: "#{described_class}: Copied container_registry_enabled values for projects with IDs between #{project1.id}..#{project4.id}")
+
+        expect(logger).not_to receive(:info)
+      end
+
+      subject.perform(project1.id, project4.id)
+
+      expect(project1.reload.container_registry_enabled).to eq(true)
+      expect(project2.reload.container_registry_enabled).to eq(false)
+      expect(project3.reload.container_registry_enabled).to eq(nil)
+      expect(project4.container_registry_enabled).to eq(true)
+
+      expect(project_feature1.reload.container_registry_access_level).to eq(enabled)
+      expect(project_feature2.reload.container_registry_access_level).to eq(disabled)
+      expect(project_feature3.reload.container_registry_access_level).to eq(disabled)
+    end
+
+    context 'when no projects exist in range' do
+      it 'does not fail' do
+        expect(project1.container_registry_enabled).to eq(true)
+        expect(project_feature1.container_registry_access_level).to eq(disabled)
+
+        expect { subject.perform(-1, -2) }.not_to raise_error
+
+        expect(project1.container_registry_enabled).to eq(true)
+        expect(project_feature1.container_registry_access_level).to eq(disabled)
+      end
+    end
+
+    context 'when projects in range all have nil container_registry_enabled' do
+      it 'does not fail' do
+        expect(project3.container_registry_enabled).to eq(nil)
+        expect(project_feature3.container_registry_access_level).to eq(disabled)
+
+        expect { subject.perform(project3.id, project3.id) }.not_to raise_error
+
+        expect(project3.container_registry_enabled).to eq(nil)
+        expect(project_feature3.container_registry_access_level).to eq(disabled)
+      end
+    end
+  end
+end

spec/migrations/move_container_registry_enabled_to_project_features_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20210226120851_move_container_registry_enabled_to_project_features.rb')
+
+RSpec.describe MoveContainerRegistryEnabledToProjectFeatures, :migration do
+  let(:namespace) { table(:namespaces).create!(name: 'gitlab', path: 'gitlab-org') }
+
+  let!(:projects) do
+    [
+      table(:projects).create!(namespace_id: namespace.id, name: 'project 1'),
+      table(:projects).create!(namespace_id: namespace.id, name: 'project 2'),
+      table(:projects).create!(namespace_id: namespace.id, name: 'project 3'),
+      table(:projects).create!(namespace_id: namespace.id, name: 'project 4')
+    ]
+  end
+
+  before do
+    stub_const("#{described_class.name}::BATCH_SIZE", 3)
+  end
+
+  around do |example|
+    Sidekiq::Testing.fake! do
+      freeze_time do
+        example.call
+      end
+    end
+  end
+
+  it 'schedules jobs for ranges of projects' do
+    migrate!
+
+    expect(described_class::MIGRATION)
+      .to be_scheduled_delayed_migration(2.minutes, projects[0].id, projects[2].id)
+
+    expect(described_class::MIGRATION)
+      .to be_scheduled_delayed_migration(4.minutes, projects[3].id, projects[3].id)
+  end
+
+  it 'schedules jobs according to the configured batch size' do
+    expect { migrate! }.to change { BackgroundMigrationWorker.jobs.size }.by(2)
+  end
+end

spec/models/project_feature_spec.rb

@@ -187,4 +187,30 @@ RSpec.describe ProjectFeature do
       expect(described_class.required_minimum_access_level_for_private_project(:issues)).to eq(Gitlab::Access::GUEST)
     end
   end
+
+  describe 'container_registry_access_level' do
+    context 'when the project is created with container_registry_enabled false' do
+      it 'creates project with DISABLED container_registry_access_level' do
+        project = create(:project, container_registry_enabled: false)
+
+        expect(project.project_feature.container_registry_access_level).to eq(described_class::DISABLED)
+      end
+    end
+
+    context 'when the project is created with container_registry_enabled true' do
+      it 'creates project with ENABLED container_registry_access_level' do
+        project = create(:project, container_registry_enabled: true)
+
+        expect(project.project_feature.container_registry_access_level).to eq(described_class::ENABLED)
+      end
+    end
+
+    context 'when the project is created with container_registry_enabled nil' do
+      it 'creates project with DISABLED container_registry_access_level' do
+        project = create(:project, container_registry_enabled: nil)
+
+        expect(project.project_feature.container_registry_access_level).to eq(described_class::DISABLED)
+      end
+    end
+  end
 end

spec/models/project_spec.rb

@@ -2202,6 +2202,44 @@ RSpec.describe Project, factory_default: :keep do
     end
   end
 
+  describe '#set_container_registry_access_level' do
+    let_it_be_with_reload(:project) { create(:project) }
+
+    it 'updates project_feature', :aggregate_failures do
+      # Simulate an existing project that has container_registry enabled
+      project.update_column(:container_registry_enabled, true)
+      project.project_feature.update_column(:container_registry_access_level, ProjectFeature::DISABLED)
+
+      expect(project.container_registry_enabled).to eq(true)
+      expect(project.project_feature.container_registry_access_level).to eq(ProjectFeature::DISABLED)
+
+      project.update!(container_registry_enabled: false)
+
+      expect(project.container_registry_enabled).to eq(false)
+      expect(project.project_feature.container_registry_access_level).to eq(ProjectFeature::DISABLED)
+
+      project.update!(container_registry_enabled: true)
+
+      expect(project.container_registry_enabled).to eq(true)
+      expect(project.project_feature.container_registry_access_level).to eq(ProjectFeature::ENABLED)
+    end
+
+    it 'rollsback both projects and project_features row in case of error', :aggregate_failures do
+      project.update_column(:container_registry_enabled, true)
+      project.project_feature.update_column(:container_registry_access_level, ProjectFeature::DISABLED)
+
+      expect(project.container_registry_enabled).to eq(true)
+      expect(project.project_feature.container_registry_access_level).to eq(ProjectFeature::DISABLED)
+
+      allow(project).to receive(:valid?).and_return(false)
+
+      expect { project.update!(container_registry_enabled: false) }.to raise_error(ActiveRecord::RecordInvalid)
+
+      expect(project.reload.container_registry_enabled).to eq(true)
+      expect(project.project_feature.reload.container_registry_access_level).to eq(ProjectFeature::DISABLED)
+    end
+  end
+
   describe '#has_container_registry_tags?' do
     let(:project) { build(:project) }