Backfill deployment_merge_requests table

This commit includes migrations to backfill environment_id on
deployment_merge_requests table.

Environment_id is nullable and was introduced to deal with duplicated
environments in MR deployment tracking
https://gitlab.com/gitlab-org/gitlab/issues/199256

Here we backfill the old entries that still have a NULL environment_id

We batch entries with ranges of merge_request_id and schedule some
background migrations.

The migration will delete duplicates and backfill environment_id
Co-authored-by: Yorick Peterse <yorick@yorickpeterse.com>

changelogs/unreleased/ac-backfill-environment_id-on-deployment_merge_requests.yml

+---
+title: backfill environment_id on deployment_merge_requests
+merge_request: 27219
+author:
+type: other

db/post_migrate/20200312134637_backfill_environment_id_on_deployment_merge_requests.rb

+# frozen_string_literal: true
+
+class BackfillEnvironmentIdOnDeploymentMergeRequests < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  BATCH_SIZE = 400
+  DELAY = 1.minute
+
+  disable_ddl_transaction!
+
+  def up
+    max_mr_id = DeploymentMergeRequest
+      .select(:merge_request_id)
+      .distinct
+      .order(merge_request_id: :desc)
+      .limit(1)
+      .pluck(:merge_request_id)
+      .first || 0
+
+    last_mr_id = 0
+    step = 0
+
+    while last_mr_id < max_mr_id
+      stop =
+        DeploymentMergeRequest
+          .select(:merge_request_id)
+          .distinct
+          .where('merge_request_id > ?', last_mr_id)
+          .order(:merge_request_id)
+          .offset(BATCH_SIZE)
+          .limit(1)
+          .pluck(:merge_request_id)
+          .first
+
+      stop ||= max_mr_id
+
+      migrate_in(
+        step * DELAY,
+        'BackfillEnvironmentIdDeploymentMergeRequests',
+        [last_mr_id + 1, stop]
+      )
+
+      last_mr_id = stop
+      step += 1
+    end
+  end
+
+  def down
+    # no-op
+
+    # this migration is designed to delete duplicated data
+  end
+end

db/structure.sql

@@ -13390,6 +13390,7 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200311214912
 20200312053852
 20200312125121
+20200312134637
 20200312160532
 20200312163407
 20200313101649

lib/gitlab/background_migration/backfill_environment_id_deployment_merge_requests.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    # BackfillEnvironmentIdDeploymentMergeRequests deletes duplicates
+    # from deployment_merge_requests table and backfills environment_id
+    class BackfillEnvironmentIdDeploymentMergeRequests
+      def perform(start_mr_id, stop_mr_id)
+        start_mr_id = Integer(start_mr_id)
+        stop_mr_id  = Integer(stop_mr_id)
+
+        ActiveRecord::Base.connection.execute(<<~SQL)
+        DELETE FROM deployment_merge_requests
+        WHERE (deployment_id, merge_request_id) in (
+              SELECT t.deployment_id, t.merge_request_id  FROM (
+                     SELECT mrd.merge_request_id, mrd.deployment_id, ROW_NUMBER() OVER w AS rnum
+                     FROM deployment_merge_requests as mrd
+                          INNER JOIN "deployments" ON "deployments"."id" = "mrd"."deployment_id"
+                     WHERE mrd.merge_request_id BETWEEN #{start_mr_id} AND #{stop_mr_id}
+                     WINDOW w AS (
+                            PARTITION BY merge_request_id, deployments.environment_id
+                            ORDER BY deployments.id
+                     )
+              ) t
+              WHERE t.rnum > 1
+        );
+        SQL
+
+        ActiveRecord::Base.connection.execute(<<~SQL)
+        UPDATE deployment_merge_requests
+        SET environment_id = deployments.environment_id
+        FROM deployments
+        WHERE deployments.id = "deployment_merge_requests".deployment_id
+              AND "deployment_merge_requests".environment_id IS NULL
+              AND "deployment_merge_requests".merge_request_id BETWEEN #{start_mr_id} AND #{stop_mr_id}
+        SQL
+      end
+    end
+  end
+end

spec/finders/merge_requests_finder_spec.rb

@@ -174,15 +174,16 @@ describe MergeRequestsFinder do
             deployment1 = create(
               :deployment,
               project: project_with_repo,
-              sha: project_with_repo.commit.id,
-              merge_requests: [merge_request1, merge_request2]
+              sha: project_with_repo.commit.id
             )
-            create(
+            deployment2 = create(
               :deployment,
               project: project_with_repo,
-              sha: project_with_repo.commit.id,
-              merge_requests: [merge_request3]
+              sha: project_with_repo.commit.id
             )
+            deployment1.link_merge_requests(MergeRequest.where(id: [merge_request1.id, merge_request2.id]))
+            deployment2.link_merge_requests(MergeRequest.where(id: merge_request3.id))
+
             params = { deployment_id: deployment1.id }
             merge_requests = described_class.new(user, params).execute
 

spec/lib/gitlab/background_migration/backfill_environment_id_deployment_merge_requests_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::BackgroundMigration::BackfillEnvironmentIdDeploymentMergeRequests, schema: 20200312134637 do
+  let(:environments)              { table(:environments) }
+  let(:merge_requests)            { table(:merge_requests) }
+  let(:deployments)               { table(:deployments) }
+  let(:deployment_merge_requests) { table(:deployment_merge_requests) }
+  let(:namespaces)                { table(:namespaces) }
+  let(:projects)                  { table(:projects) }
+
+  subject(:migration) { described_class.new }
+
+  it 'correctly backfills environment_id column' do
+    namespace = namespaces.create!(name: 'foo', path: 'foo')
+    project = projects.create!(namespace_id: namespace.id)
+
+    production = environments.create!(project_id: project.id, name: 'production', slug: 'production')
+    staging = environments.create!(project_id: project.id, name: 'staging', slug: 'staging')
+
+    mr = merge_requests.create!(source_branch: 'x', target_branch: 'master', target_project_id: project.id)
+
+    deployment1 = deployments.create!(environment_id: staging.id, iid: 1, project_id: project.id, ref: 'master', tag: false, sha: '123abcdef', status: 1)
+    deployment2 = deployments.create!(environment_id: production.id, iid: 2, project_id: project.id, ref: 'master', tag: false, sha: '123abcdef', status: 1)
+    deployment3 = deployments.create!(environment_id: production.id, iid: 3, project_id: project.id, ref: 'master', tag: false, sha: '123abcdef', status: 1)
+
+    # mr is tracked twice in production through deployment2 and deployment3
+    deployment_merge_requests.create!(deployment_id: deployment1.id, merge_request_id: mr.id)
+    deployment_merge_requests.create!(deployment_id: deployment2.id, merge_request_id: mr.id)
+    deployment_merge_requests.create!(deployment_id: deployment3.id, merge_request_id: mr.id)
+
+    expect(deployment_merge_requests.where(environment_id: nil).count).to eq(3)
+
+    migration.perform(1, mr.id)
+
+    expect(deployment_merge_requests.where(environment_id: nil).count).to be_zero
+    expect(deployment_merge_requests.count).to eq(2)
+
+    production_deployments = deployment_merge_requests.where(environment_id: production.id)
+    expect(production_deployments.count).to eq(1)
+    expect(production_deployments.first.deployment_id).to eq(deployment2.id)
+
+    expect(deployment_merge_requests.where(environment_id: staging.id).count).to eq(1)
+  end
+end

spec/migrations/backfill_environment_id_on_deployment_merge_requests_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20200312134637_backfill_environment_id_on_deployment_merge_requests.rb')
+
+describe BackfillEnvironmentIdOnDeploymentMergeRequests do
+  let(:environments)              { table(:environments) }
+  let(:merge_requests)            { table(:merge_requests) }
+  let(:deployments)               { table(:deployments) }
+  let(:deployment_merge_requests) { table(:deployment_merge_requests) }
+  let(:namespaces)                { table(:namespaces) }
+  let(:projects)                  { table(:projects) }
+
+  let(:migration_worker) { double('BackgroundMigrationWorker') }
+
+  before do
+    stub_const('BackgroundMigrationWorker', migration_worker)
+  end
+
+  it 'schedules nothing when there are no entries' do
+    expect(migration_worker).not_to receive(:perform_in)
+
+    migrate!
+  end
+
+  it 'batches the workload' do
+    stub_const("#{described_class.name}::BATCH_SIZE", 10)
+
+    namespace = namespaces.create!(name: 'foo', path: 'foo')
+    project = projects.create!(namespace_id: namespace.id)
+
+    environment = environments.create!(project_id: project.id, name: 'staging', slug: 'staging')
+
+    # Batching is based on DeploymentMergeRequest.merge_request_id, in order to test it
+    # we must generate more than described_class::BATCH_SIZE merge requests, deployments,
+    # and deployment_merge_requests entries
+    entries = 13
+    expect(entries).to be > described_class::BATCH_SIZE
+
+    # merge requests and deployments bulk generation
+    mrs_params = []
+    deployments_params = []
+    entries.times do |i|
+      mrs_params << { source_branch: 'x', target_branch: 'master', target_project_id: project.id }
+
+      deployments_params << { environment_id: environment.id, iid: i + 1, project_id: project.id, ref: 'master', tag: false, sha: '123abcdef', status: 1 }
+    end
+
+    all_mrs = merge_requests.insert_all(mrs_params)
+    all_deployments = deployments.insert_all(deployments_params)
+
+    # deployment_merge_requests bulk generation
+    dmr_params = []
+    entries.times do |index|
+      mr_id = all_mrs.rows[index].first
+      deployment_id = all_deployments.rows[index].first
+
+      dmr_params << { deployment_id: deployment_id, merge_request_id: mr_id }
+    end
+
+    deployment_merge_requests.insert_all(dmr_params)
+
+    first_batch_limit = dmr_params[described_class::BATCH_SIZE][:merge_request_id]
+    second_batch_limit = dmr_params.last[:merge_request_id]
+
+    expect(migration_worker).to receive(:perform_in)
+                                  .with(
+                                    0,
+                                    'BackfillEnvironmentIdDeploymentMergeRequests',
+                                    [1, first_batch_limit]
+                                  )
+    expect(migration_worker).to receive(:perform_in)
+                                  .with(
+                                    described_class::DELAY,
+                                    'BackfillEnvironmentIdDeploymentMergeRequests',
+                                    [first_batch_limit + 1, second_batch_limit]
+                                  )
+
+    migrate!
+  end
+end

spec/models/merge_request_spec.rb

@@ -3700,41 +3700,41 @@ describe MergeRequest do
   describe '#recent_visible_deployments' do
     let(:merge_request) { create(:merge_request) }
 
-    let(:environment) do
-      create(:environment, project: merge_request.target_project)
-    end
-
     it 'returns visible deployments' do
+      envs = create_list(:environment, 3, project: merge_request.target_project)
+
       created = create(
         :deployment,
         :created,
         project: merge_request.target_project,
-        environment: environment
+        environment: envs[0]
       )
 
       success = create(
         :deployment,
         :success,
         project: merge_request.target_project,
-        environment: environment
+        environment: envs[1]
       )
 
       failed = create(
         :deployment,
         :failed,
         project: merge_request.target_project,
-        environment: environment
+        environment: envs[2]
       )
 
-      merge_request.deployment_merge_requests.create!(deployment: created)
-      merge_request.deployment_merge_requests.create!(deployment: success)
-      merge_request.deployment_merge_requests.create!(deployment: failed)
+      merge_request_relation = MergeRequest.where(id: merge_request.id)
+      created.link_merge_requests(merge_request_relation)
+      success.link_merge_requests(merge_request_relation)
+      failed.link_merge_requests(merge_request_relation)
 
       expect(merge_request.recent_visible_deployments).to eq([failed, success])
     end
 
     it 'only returns a limited number of deployments' do
       20.times do
+        environment = create(:environment, project: merge_request.target_project)
         deploy = create(
           :deployment,
           :success,
@@ -3742,7 +3742,7 @@ describe MergeRequest do
           environment: environment
         )
 
-        merge_request.deployment_merge_requests.create!(deployment: deploy)
+        deploy.link_merge_requests(MergeRequest.where(id: merge_request.id))
       end
 
       expect(merge_request.recent_visible_deployments.count).to eq(10)

spec/requests/api/deployments_spec.rb

@@ -439,7 +439,7 @@ describe API::Deployments do
       let!(:merge_request3) { create(:merge_request, source_project: project2, target_project: project2) }
 
       it 'returns the relevant merge requests linked to a deployment for a project' do
-        deployment.merge_requests << [merge_request1, merge_request2]
+        deployment.link_merge_requests(MergeRequest.where(id: [merge_request1.id, merge_request2.id]))
 
         subject