Merge branch '223151-adjust-unique-index-on-alerts' into 'master'

Add less-strict index for alert fingerprints

See merge request gitlab-org/gitlab!36024

app/models/alert_management/alert.rb

@@ -55,8 +55,12 @@ module AlertManagement
     validates :severity,        presence: true
     validates :status,          presence: true
     validates :started_at,      presence: true
-    validates :fingerprint,     uniqueness: { scope: :project }, allow_blank: true
-    validate  :hosts_length
+    validates :fingerprint,     allow_blank: true, uniqueness: {
+      scope: :project,
+      conditions: -> { where.not(status: STATUSES[:resolved]) },
+      message: -> (object, data) { _('Cannot have multiple unresolved alerts') }
+    }, unless: :resolved?
+    validate :hosts_length
 
     enum severity: {
       critical: 0,

changelogs/unreleased/223151-adjust-unique-index-on-alerts.yml

+---
+title: Change Alert fingerprint index to run when status is not resolved
+merge_request: 36024
+author:
+type: changed

db/migrate/20200706035141_adjust_unique_index_alert_management_alerts.rb

+# frozen_string_literal: true
+
+class AdjustUniqueIndexAlertManagementAlerts < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME        = false
+  INDEX_NAME      = 'index_alert_management_alerts_on_project_id_and_fingerprint'
+  NEW_INDEX_NAME  = 'index_partial_am_alerts_on_project_id_and_fingerprint'
+  RESOLVED_STATUS = 2
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index(:alert_management_alerts, %w(project_id fingerprint), where: "status <> #{RESOLVED_STATUS}", name: NEW_INDEX_NAME, unique: true, using: :btree)
+    remove_concurrent_index_by_name :alert_management_alerts, INDEX_NAME
+  end
+
+  def down
+    # Nullify duplicate fingerprints, except for the newest of each match (project_id, fingerprint).
+    query = <<-SQL
+      UPDATE alert_management_alerts am
+      SET fingerprint = NULL
+      WHERE am.created_at <>
+        (SELECT MAX(created_at)
+        FROM alert_management_alerts am2
+        WHERE am.fingerprint = am2.fingerprint AND am.project_id = am2.project_id)
+      AND am.fingerprint IS NOT NULL;
+    SQL
+
+    execute(query)
+
+    remove_concurrent_index_by_name :alert_management_alerts, NEW_INDEX_NAME
+    add_concurrent_index(:alert_management_alerts, %w(project_id fingerprint), name: INDEX_NAME, unique: true, using: :btree)
+  end
+end

db/structure.sql

@@ -18505,8 +18505,6 @@ CREATE INDEX index_alert_management_alerts_on_environment_id ON public.alert_man
 
 CREATE INDEX index_alert_management_alerts_on_issue_id ON public.alert_management_alerts USING btree (issue_id);
 
-CREATE UNIQUE INDEX index_alert_management_alerts_on_project_id_and_fingerprint ON public.alert_management_alerts USING btree (project_id, fingerprint);
-
 CREATE UNIQUE INDEX index_alert_management_alerts_on_project_id_and_iid ON public.alert_management_alerts USING btree (project_id, iid);
 
 CREATE INDEX index_alert_management_alerts_on_prometheus_alert_id ON public.alert_management_alerts USING btree (prometheus_alert_id) WHERE (prometheus_alert_id IS NOT NULL);
@@ -19753,6 +19751,8 @@ CREATE INDEX index_pages_domains_on_verified_at_and_enabled_until ON public.page
 
 CREATE INDEX index_pages_domains_on_wildcard ON public.pages_domains USING btree (wildcard);
 
+CREATE UNIQUE INDEX index_partial_am_alerts_on_project_id_and_fingerprint ON public.alert_management_alerts USING btree (project_id, fingerprint) WHERE (status <> 2);
+
 CREATE UNIQUE INDEX index_partitioned_foreign_keys_unique_index ON public.partitioned_foreign_keys USING btree (to_table, from_table, from_column);
 
 CREATE INDEX index_pat_on_user_id_and_expires_at ON public.personal_access_tokens USING btree (user_id, expires_at);
@@ -23639,6 +23639,7 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200704143633
 20200704161600
 20200706005325
+20200706035141
 20200706154619
 20200706170536
 20200707071941

locale/gitlab.pot

@@ -4140,6 +4140,9 @@ msgstr ""
 msgid "Cannot have multiple Jira imports running at the same time"
 msgstr ""
 
+msgid "Cannot have multiple unresolved alerts"
+msgstr ""
+
 msgid "Cannot import because issues are not available in this project."
 msgstr ""
 

spec/migrations/20200706035141_adjust_unique_index_alert_management_alerts_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'migrate', '20200706035141_adjust_unique_index_alert_management_alerts.rb')
+
+RSpec.describe AdjustUniqueIndexAlertManagementAlerts, :migration do
+  let(:migration) { described_class.new }
+  let(:alerts) { AlertManagement::Alert }
+  let(:project) { create_project }
+  let(:other_project) { create_project }
+  let(:resolved_state) { 2 }
+  let(:triggered_state) { 1 }
+  let!(:existing_alert) { create_alert(project, resolved_state, '1234', 1) }
+  let!(:p2_alert) { create_alert(other_project, resolved_state, '1234', 1) }
+  let!(:p2_alert_diff_fingerprint) { create_alert(other_project, resolved_state, '4567', 2) }
+
+  it 'can reverse the migration' do
+    expect(existing_alert.fingerprint).not_to eq(nil)
+    expect(p2_alert.fingerprint).not_to eq(nil)
+    expect(p2_alert_diff_fingerprint.fingerprint).not_to eq(nil)
+
+    migrate!
+
+    # Adding a second alert with the same fingerprint now that we can
+    second_alert = create_alert(project, triggered_state, '1234', 2)
+    expect(alerts.count).to eq(4)
+
+    schema_migrate_down!
+
+    # We keep the alerts, but the oldest ones fingerprint is removed
+    expect(alerts.count).to eq(4)
+    expect(second_alert.reload.fingerprint).not_to eq(nil)
+    expect(p2_alert.fingerprint).not_to eq(nil)
+    expect(p2_alert_diff_fingerprint.fingerprint).not_to eq(nil)
+    expect(existing_alert.reload.fingerprint).to eq(nil)
+  end
+
+  def namespace
+    @namespace ||= table(:namespaces).create!(name: 'foo', path: 'foo')
+  end
+
+  def create_project
+    table(:projects).create!(namespace_id: namespace.id)
+  end
+
+  def create_alert(project, status, fingerprint, iid)
+    params = {
+      title: 'test',
+      started_at: Time.current,
+      iid: iid,
+      project_id: project.id,
+      status: status,
+      fingerprint: fingerprint
+    }
+    table(:alert_management_alerts).create!(params)
+  end
+end

spec/models/alert_management/alert_spec.rb

@@ -83,21 +83,50 @@ RSpec.describe AlertManagement::Alert do
     end
 
     describe 'fingerprint' do
+      let_it_be(:project) { create(:project) }
       let_it_be(:fingerprint) { 'fingerprint' }
-      let_it_be(:existing_alert) { create(:alert_management_alert, fingerprint: fingerprint) }
       let(:new_alert) { build(:alert_management_alert, fingerprint: fingerprint, project: project) }
 
       subject { new_alert }
 
       context 'adding an alert with the same fingerprint' do
-        context 'same project' do
-          let(:project) { existing_alert.project }
-
-          it { is_expected.not_to be_valid }
+        context 'same project, various states' do
+          using RSpec::Parameterized::TableSyntax
+
+          # We are only validating uniqueness for non-resolved alerts
+          where(:existing_status, :new_status, :valid) do
+            :resolved      | :triggered    | true
+            :resolved      | :acknowledged | true
+            :resolved      | :ignored      | true
+            :resolved      | :resolved     | true
+            :triggered     | :triggered    | false
+            :triggered     | :acknowledged | false
+            :triggered     | :ignored      | false
+            :triggered     | :resolved     | true
+            :acknowledged  | :triggered    | false
+            :acknowledged  | :acknowledged | false
+            :acknowledged  | :ignored      | false
+            :acknowledged  | :resolved     | true
+            :ignored       | :triggered    | false
+            :ignored       | :acknowledged | false
+            :ignored       | :ignored      | false
+            :ignored       | :resolved     | true
+          end
+
+          with_them do
+            let!(:existing_alert) { create(:alert_management_alert, existing_status, fingerprint: fingerprint, project: project) }
+            let(:new_alert) { build(:alert_management_alert, new_status, fingerprint: fingerprint, project: project) }
+
+            if params[:valid]
+              it { is_expected.to be_valid }
+            else
+              it { is_expected.to be_invalid }
+            end
+          end
         end
 
         context 'different project' do
-          let(:project) { create(:project) }
+          let!(:existing_alert) { create(:alert_management_alert, fingerprint: fingerprint) }
 
           it { is_expected.to be_valid }
         end