Merge branch 'revert-841f45b7' into 'master'

Revert "Fix Elastic::MigrationWorker current_migration"

See merge request gitlab-org/gitlab!70405

ee/app/workers/elastic/migration_worker.rb

@@ -87,9 +87,9 @@ module Elastic
     end
 
     def current_migration
-      uncompleted_migrations = Elastic::MigrationRecord.load_versions(completed: false)
+      completed_migrations = Elastic::MigrationRecord.load_versions(completed: true)
 
-      Elastic::DataMigrationService.migrations.find { |migration| uncompleted_migrations.include?(migration.version) }
+      Elastic::DataMigrationService.migrations.find { |migration| !completed_migrations.include?(migration.version) }
     end
 
     def pause_indexing!(migration)

ee/spec/workers/elastic/migration_worker_spec.rb

@@ -22,184 +22,168 @@ RSpec.describe Elastic::MigrationWorker, :elastic do
 
       before do
         stub_ee_application_setting(elasticsearch_indexing: true)
+
+        allow(subject).to receive(:current_migration).and_return(migration)
       end
 
-      context 'an unexecuted migration present' do
+      it 'creates an index if it does not exist' do
+        Gitlab::Elastic::Helper.default.delete_migrations_index
+
+        expect { subject.perform }.to change { Gitlab::Elastic::Helper.default.migrations_index_exists? }.from(false).to(true)
+      end
+
+      context 'no unexecuted migrations' do
         before do
-          allow(subject).to receive(:current_migration).and_return(migration)
+          allow(subject).to receive(:current_migration).and_return(nil)
         end
 
-        it 'creates an index if it does not exist' do
-          Gitlab::Elastic::Helper.default.delete_migrations_index
+        it 'skips execution' do
+          expect(subject).not_to receive(:execute_migration)
 
-          expect { subject.perform }.to change { Gitlab::Elastic::Helper.default.migrations_index_exists? }.from(false).to(true)
+          expect(subject.perform).to be_falsey
         end
+      end
 
-        context 'migration is halted' do
-          using RSpec::Parameterized::TableSyntax
+      context 'migration is halted' do
+        using RSpec::Parameterized::TableSyntax
 
-          where(:pause_indexing, :halted_indexing_unpaused, :unpause) do
-            false | false | false
-            false | true  | false
-            true  | false | true
-            true  | true  | false
+        where(:pause_indexing, :halted_indexing_unpaused, :unpause) do
+          false | false | false
+          false | true  | false
+          true  | false | true
+          true  | true  | false
+        end
+
+        with_them do
+          before do
+            allow(Gitlab::CurrentSettings).to receive(:elasticsearch_pause_indexing?).and_return(true)
+            allow(migration).to receive(:pause_indexing?).and_return(true)
+            migration.save_state!(halted: true, pause_indexing: pause_indexing, halted_indexing_unpaused: halted_indexing_unpaused)
           end
 
-          with_them do
-            before do
-              allow(Gitlab::CurrentSettings).to receive(:elasticsearch_pause_indexing?).and_return(true)
-              allow(migration).to receive(:pause_indexing?).and_return(true)
-              migration.save_state!(halted: true, pause_indexing: pause_indexing, halted_indexing_unpaused: halted_indexing_unpaused)
+          it 'unpauses indexing' do
+            if unpause
+              expect(Gitlab::CurrentSettings).to receive(:update!).with(elasticsearch_pause_indexing: false)
+            else
+              expect(Gitlab::CurrentSettings).not_to receive(:update!)
             end
 
-            it 'unpauses indexing' do
-              if unpause
-                expect(Gitlab::CurrentSettings).to receive(:update!).with(elasticsearch_pause_indexing: false)
-              else
-                expect(Gitlab::CurrentSettings).not_to receive(:update!)
-              end
+            expect(migration).not_to receive(:migrate)
 
-              expect(migration).not_to receive(:migrate)
-
-              subject.perform
-            end
+            subject.perform
           end
         end
+      end
 
-        context 'migration process' do
-          before do
-            allow(migration).to receive(:started?).and_return(started)
-            allow(migration).to receive(:completed?).and_return(completed)
-            allow(migration).to receive(:batched?).and_return(batched)
-          end
-
-          using RSpec::Parameterized::TableSyntax
-
-          # completed is evaluated after migrate method is executed
-          where(:started, :completed, :execute_migration, :batched) do
-            false | false | true  | false
-            false | true  | true  | false
-            false | false | true  | true
-            false | true  | true  | true
-            true  | false | false | false
-            true  | true  | false | false
-            true  | false | true  | true
-            true  | true  | true | true
-          end
-
-          with_them do
-            it 'calls migration only when needed', :aggregate_failures do
-              if execute_migration
-                expect(migration).to receive(:migrate).once
-              else
-                expect(migration).not_to receive(:migrate)
-              end
+      context 'migration process' do
+        before do
+          allow(migration).to receive(:started?).and_return(started)
+          allow(migration).to receive(:completed?).and_return(completed)
+          allow(migration).to receive(:batched?).and_return(batched)
+        end
 
-              expect(migration).to receive(:save!).with(completed: completed)
-              expect(Elastic::DataMigrationService).to receive(:drop_migration_has_finished_cache!).with(migration)
+        using RSpec::Parameterized::TableSyntax
+
+        # completed is evaluated after migrate method is executed
+        where(:started, :completed, :execute_migration, :batched) do
+          false | false | true  | false
+          false | true  | true  | false
+          false | false | true  | true
+          false | true  | true  | true
+          true  | false | false | false
+          true  | true  | false | false
+          true  | false | true  | true
+          true  | true  | true | true
+        end
 
-              subject.perform
+        with_them do
+          it 'calls migration only when needed', :aggregate_failures do
+            if execute_migration
+              expect(migration).to receive(:migrate).once
+            else
+              expect(migration).not_to receive(:migrate)
             end
 
-            it 'handles batched migrations' do
-              if batched && !completed
-                # default throttle_delay is 5.minutes
-                expect( Elastic::MigrationWorker).to receive(:perform_in)
-                  .with(5.minutes)
-              else
-                expect( Elastic::MigrationWorker).not_to receive(:perform_in)
-              end
+            expect(migration).to receive(:save!).with(completed: completed)
+            expect(Elastic::DataMigrationService).to receive(:drop_migration_has_finished_cache!).with(migration)
 
-              subject.perform
-            end
+            subject.perform
           end
 
-          context 'indexing pause' do
-            before do
-              allow(migration).to receive(:pause_indexing?).and_return(true)
+          it 'handles batched migrations' do
+            if batched && !completed
+              # default throttle_delay is 5.minutes
+              expect( Elastic::MigrationWorker).to receive(:perform_in)
+                .with(5.minutes)
+            else
+              expect( Elastic::MigrationWorker).not_to receive(:perform_in)
             end
 
-            let(:batched) { true }
-
-            where(:started, :completed, :expected) do
-              false | false | false
-              true  | false | false
-              true  | true  | true
-            end
-
-            with_them do
-              it 'pauses and unpauses indexing' do
-                expect(Gitlab::CurrentSettings).to receive(:update!).with(elasticsearch_pause_indexing: true)
-                expect(Gitlab::CurrentSettings).to receive(:update!).with(elasticsearch_pause_indexing: false) if expected
+            subject.perform
+          end
+        end
 
-                subject.perform
-              end
-            end
+        context 'indexing pause' do
+          before do
+            allow(migration).to receive(:pause_indexing?).and_return(true)
           end
 
-          context 'checks space required' do
-            let(:helper) { Gitlab::Elastic::Helper.new }
-            let(:started) { false }
-            let(:completed) { false }
-            let(:batched) { false }
+          let(:batched) { true }
 
-            before do
-              allow(Gitlab::Elastic::Helper).to receive(:default).and_return(helper)
-              allow(migration).to receive(:space_requirements?).and_return(true)
-              allow(migration).to receive(:space_required_bytes).and_return(10)
-            end
+          where(:started, :completed, :expected) do
+            false | false | false
+            true  | false | false
+            true  | true  | true
+          end
 
-            it 'halts the migration if there is not enough space' do
-              allow(helper).to receive(:cluster_free_size_bytes).and_return(5)
-              expect(migration).to receive(:halt!)
-              expect(migration).not_to receive(:migrate)
+          with_them do
+            it 'pauses and unpauses indexing' do
+              expect(Gitlab::CurrentSettings).to receive(:update!).with(elasticsearch_pause_indexing: true)
+              expect(Gitlab::CurrentSettings).to receive(:update!).with(elasticsearch_pause_indexing: false) if expected
 
               subject.perform
             end
+          end
+        end
 
-            it 'runs the migration if there is enough space' do
-              allow(helper).to receive(:cluster_free_size_bytes).and_return(20)
-              expect(migration).not_to receive(:halt!)
-              expect(migration).to receive(:migrate).once
-
-              subject.perform
-            end
+        context 'checks space required' do
+          let(:helper) { Gitlab::Elastic::Helper.new }
+          let(:started) { false }
+          let(:completed) { false }
+          let(:batched) { false }
 
-            context 'when migration is already started' do
-              let(:started) { true }
+          before do
+            allow(Gitlab::Elastic::Helper).to receive(:default).and_return(helper)
+            allow(migration).to receive(:space_requirements?).and_return(true)
+            allow(migration).to receive(:space_required_bytes).and_return(10)
+          end
 
-              it 'does not check space requirements' do
-                expect(helper).not_to receive(:cluster_free_size_bytes)
-                expect(migration).not_to receive(:space_required_bytes)
+          it 'halts the migration if there is not enough space' do
+            allow(helper).to receive(:cluster_free_size_bytes).and_return(5)
+            expect(migration).to receive(:halt!)
+            expect(migration).not_to receive(:migrate)
 
-                subject.perform
-              end
-            end
+            subject.perform
           end
-        end
-      end
 
-      context 'no unexecuted migrations' do
-        before do
-          allow(subject).to receive(:current_migration).and_return(nil)
-        end
-
-        it 'skips execution' do
-          expect(subject).not_to receive(:execute_migration)
+          it 'runs the migration if there is enough space' do
+            allow(helper).to receive(:cluster_free_size_bytes).and_return(20)
+            expect(migration).not_to receive(:halt!)
+            expect(migration).to receive(:migrate).once
 
-          expect(subject.perform).to be_falsey
-        end
-      end
+            subject.perform
+          end
 
-      context 'load_versions returns empty array' do
-        before do
-          allow(Elastic::MigrationRecord).to receive(:load_versions).and_return([])
-        end
+          context 'when migration is already started' do
+            let(:started) { true }
 
-        it 'skips execution' do
-          expect(subject).not_to receive(:execute_migration)
+            it 'does not check space requirements' do
+              expect(helper).not_to receive(:cluster_free_size_bytes)
+              expect(migration).not_to receive(:space_required_bytes)
 
-          expect(subject.perform).to be_falsey
+              subject.perform
+            end
+          end
         end
       end
     end