Added checks for migration downtime

These new checks can be used to check if migrations require downtime or
not (as tagged by their authors). In CI this compares the current branch
with master so migrations added by merge requests are automatically
verified.

To check the migrations added since a Git reference simply run:

    bundle exec rake gitlab:db:downtime_check[GIT_REF]

.gitlab-ci.yml

@@ -148,7 +148,7 @@ spinach 9 10: *spinach-knapsack
 .spinach-knapsack-ruby23: &spinach-knapsack-ruby23
   <<: *spinach-knapsack
   <<: *ruby-23
-  
+
 rspec 0 20 ruby23: *rspec-knapsack-ruby23
 rspec 1 20 ruby23: *rspec-knapsack-ruby23
 rspec 2 20 ruby23: *rspec-knapsack-ruby23
@@ -196,6 +196,7 @@ rake flog: *exec
 rake flay: *exec
 rake db:migrate:reset: *exec
 license_finder: *exec
+rake downtime_check: *exec
 
 bundler:audit:
   stage: test

doc/development/migration_style_guide.md

@@ -11,7 +11,8 @@ migrations are written carefully, can be applied online and adhere to the style
 Migrations should not require GitLab installations to be taken offline unless
 _absolutely_ necessary. If a migration requires downtime this should be
 clearly mentioned during the review process as well as being documented in the
-monthly release post.
+monthly release post. For more information see the "Downtime Tagging" section
+below.
 
 When writing your migrations, also consider that databases might have stale data
 or inconsistencies and guard for that. Try to make as little assumptions as possible
@@ -20,35 +21,34 @@ about the state of the database.
 Please don't depend on GitLab specific code since it can change in future versions.
 If needed copy-paste GitLab code into the migration to make it forward compatible.
 
-## Comments in the migration
+## Downtime Tagging
 
-Each migration you write needs to have the two following pieces of information
-as comments.
+Every migration must specify if it requires downtime or not, and if it should
+require downtime it must also specify a reason for this. To do so, add the
+following two constants to the migration class' body:
 
-### Online, Offline, errors?
+* `DOWNTIME`: a boolean that when set to `true` indicates the migration requires
+  downtime.
+* `DOWNTIME_REASON`: a String containing the reason for the migration requiring
+  downtime. This constant **must** be set when `DOWNTIME` is set to `true`.
 
-First, you need to provide information on whether the migration can be applied:
+For example:
 
-1. online without errors (works on previous version and new one)
-2. online with errors on old instances after migrating
-3. online with errors on new instances while migrating
-4. offline (needs to happen without app servers to prevent db corruption)
-
-For example: 
-
-```
-# Migration type: online without errors (works on previous version and new one)
+```ruby
 class MyMigration < ActiveRecord::Migration
-...
+  DOWNTIME = true
+  DOWNTIME_REASON = 'This migration requires downtime because ...'
+
+  def change
+    ...
+  end
+end
 ```
 
-It is always preferable to have a migration run online. If you expect the migration
-to take particularly long (for instance, if it loops through all notes),
-this is valuable information to add.
+It is an error (that is, CI will fail) if the `DOWNTIME` constant is missing
+from a migration class.
 
-If you don't provide the information it means that a migration is safe to run online.
-
-### Reversibility
+## Reversibility
 
 Your migration should be reversible. This is very important, as it should
 be possible to downgrade in case of a vulnerability or bugs.
@@ -100,7 +100,7 @@ value of `10` you'd write the following:
 class MyMigration < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
   disable_ddl_transaction!
-  
+
   def up
     add_column_with_default(:projects, :foo, :integer, default: 10)
   end

generator_templates/active_record/migration/create_table_migration.rb

@@ -4,6 +4,14 @@
 class <%= migration_class_name %> < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  # When a migration requires downtime you **must** uncomment the following
+  # constant and define a short and easy to understand explanation as to why the
+  # migration requires downtime.
+  # DOWNTIME_REASON = ''
+
   # When using the methods "add_concurrent_index" or "add_column_with_default"
   # you must disable the use of transactions as these methods can not run in an
   # existing transaction. When using "add_concurrent_index" make sure that this

generator_templates/active_record/migration/migration.rb

@@ -4,6 +4,14 @@
 class <%= migration_class_name %> < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  # When a migration requires downtime you **must** uncomment the following
+  # constant and define a short and easy to understand explanation as to why the
+  # migration requires downtime.
+  # DOWNTIME_REASON = ''
+
   # When using the methods "add_concurrent_index" or "add_column_with_default"
   # you must disable the use of transactions as these methods can not run in an
   # existing transaction. When using "add_concurrent_index" make sure that this

lib/gitlab/downtime_check.rb

@@ -0,0 +1,71 @@
+module Gitlab
+  # Checks if a set of migrations requires downtime or not.
+  class DowntimeCheck
+    # The constant containing the boolean that indicates if downtime is needed
+    # or not.
+    DOWNTIME_CONST = :DOWNTIME
+
+    # The constant that specifies the reason for the migration requiring
+    # downtime.
+    DOWNTIME_REASON_CONST = :DOWNTIME_REASON
+
+    # Checks the given migration paths and returns an Array of
+    # `Gitlab::DowntimeCheck::Message` instances.
+    #
+    # migrations - The migration file paths to check.
+    def check(migrations)
+      migrations.map do |path|
+        require(path)
+
+        migration_class = class_for_migration_file(path)
+
+        unless migration_class.const_defined?(DOWNTIME_CONST)
+          raise "The migration in #{path} does not specify if it requires " \
+            "downtime or not"
+        end
+
+        if online?(migration_class)
+          Message.new(path)
+        else
+          reason = downtime_reason(migration_class)
+
+          unless reason
+            raise "The migration in #{path} requires downtime but no reason " \
+              "was given"
+          end
+
+          Message.new(path, true, reason)
+        end
+      end
+    end
+
+    # Checks the given migrations and prints the results to STDOUT/STDERR.
+    #
+    # migrations - The migration file paths to check.
+    def check_and_print(migrations)
+      check(migrations).each do |message|
+        puts message.to_s # rubocop: disable Rails/Output
+      end
+    end
+
+    # Returns the class for the given migration file path.
+    def class_for_migration_file(path)
+      File.basename(path, File.extname(path)).split('_', 2).last.camelize.
+        constantize
+    end
+
+    # Returns true if the given migration can be performed without downtime.
+    def online?(migration)
+      migration.const_get(DOWNTIME_CONST) == false
+    end
+
+    # Returns the downtime reason, or nil if none was defined.
+    def downtime_reason(migration)
+      if migration.const_defined?(DOWNTIME_REASON_CONST)
+        migration.const_get(DOWNTIME_REASON_CONST)
+      else
+        nil
+      end
+    end
+  end
+end

lib/gitlab/downtime_check/message.rb

@@ -0,0 +1,28 @@
+module Gitlab
+  class DowntimeCheck
+    class Message
+      attr_reader :path, :offline, :reason
+
+      OFFLINE = "\e[32moffline\e[0m"
+      ONLINE = "\e[31monline\e[0m"
+
+      # path - The file path of the migration.
+      # offline - When set to `true` the migration will require downtime.
+      # reason - The reason as to why the migration requires downtime.
+      def initialize(path, offline = false, reason = nil)
+        @path = path
+        @offline = offline
+        @reason = reason
+      end
+
+      def to_s
+        label = offline ? OFFLINE : ONLINE
+
+        message = "[#{label}]: #{path}"
+        message += ": #{reason}" if reason
+
+        message
+      end
+    end
+  end
+end

lib/tasks/downtime_check.rake

@@ -0,0 +1,26 @@
+desc 'Checks if migrations in a branch require downtime'
+task downtime_check: :environment do
+  # First we'll want to make sure we're comparing with the right upstream
+  # repository/branch.
+  current_branch = `git rev-parse --abbrev-ref HEAD`.strip
+
+  # Either the developer ran this task directly on the master branch, or they're
+  # making changes directly on the master branch.
+  if current_branch == 'master'
+    if defined?(Gitlab::License)
+      repo = 'gitlab-ee'
+    else
+      repo = 'gitlab-ce'
+    end
+
+    `git fetch https://gitlab.com/gitlab-org/#{repo}.git --depth 1`
+
+    compare_with = 'FETCH_HEAD'
+  # The developer is working on a different branch, in this case we can just
+  # compare with the master branch.
+  else
+    compare_with = 'master'
+  end
+
+  Rake::Task['gitlab:db:downtime_check'].invoke(compare_with)
+end

lib/tasks/gitlab/db.rake

@@ -46,5 +46,20 @@ namespace :gitlab do
         Rake::Task['db:seed_fu'].invoke
       end
     end
+
+    desc 'Checks if migrations require downtime or not'
+    task :downtime_check, [:ref] => :environment do |_, args|
+      abort 'You must specify a Git reference to compare with' unless args[:ref]
+
+      require 'shellwords'
+
+      ref = Shellwords.escape(args[:ref])
+
+      migrations = `git diff #{ref}.. --name-only -- db/migrate`.lines.
+        map { |file| Rails.root.join(file.strip).to_s }.
+        select { |file| File.file?(file) }
+
+      Gitlab::DowntimeCheck.new.check_and_print(migrations)
+    end
   end
 end

spec/lib/gitlab/downtime_check/message_spec.rb

@@ -0,0 +1,17 @@
+require 'spec_helper'
+
+describe Gitlab::DowntimeCheck::Message do
+  describe '#to_s' do
+    it 'returns an ANSI formatted String for an offline migration' do
+      message = described_class.new('foo.rb', true, 'hello')
+
+      expect(message.to_s).to eq("[\e[32moffline\e[0m]: foo.rb: hello")
+    end
+
+    it 'returns an ANSI formatted String for an online migration' do
+      message = described_class.new('foo.rb')
+
+      expect(message.to_s).to eq("[\e[31monline\e[0m]: foo.rb")
+    end
+  end
+end

spec/lib/gitlab/downtime_check_spec.rb

@@ -0,0 +1,113 @@
+require 'spec_helper'
+
+describe Gitlab::DowntimeCheck do
+  subject { described_class.new }
+  let(:path) { 'foo.rb' }
+
+  describe '#check' do
+    before do
+      expect(subject).to receive(:require).with(path)
+    end
+
+    context 'when a migration does not specify if downtime is required' do
+      it 'raises RuntimeError' do
+        expect(subject).to receive(:class_for_migration_file).
+          with(path).
+          and_return(Class.new)
+
+        expect { subject.check([path]) }.
+          to raise_error(RuntimeError, /it requires downtime/)
+      end
+    end
+
+    context 'when a migration requires downtime' do
+      context 'when no reason is specified' do
+        it 'raises RuntimeError' do
+          stub_const('TestMigration::DOWNTIME', true)
+
+          expect(subject).to receive(:class_for_migration_file).
+            with(path).
+            and_return(TestMigration)
+
+          expect { subject.check([path]) }.
+            to raise_error(RuntimeError, /no reason was given/)
+        end
+      end
+
+      context 'when a reason is specified' do
+        it 'returns an Array of messages' do
+          stub_const('TestMigration::DOWNTIME', true)
+          stub_const('TestMigration::DOWNTIME_REASON', 'foo')
+
+          expect(subject).to receive(:class_for_migration_file).
+            with(path).
+            and_return(TestMigration)
+
+          messages = subject.check([path])
+
+          expect(messages).to be_an_instance_of(Array)
+          expect(messages[0]).to be_an_instance_of(Gitlab::DowntimeCheck::Message)
+
+          message = messages[0]
+
+          expect(message.path).to eq(path)
+          expect(message.offline).to eq(true)
+          expect(message.reason).to eq('foo')
+        end
+      end
+    end
+  end
+
+  describe '#check_and_print' do
+    it 'checks the migrations and prints the results to STDOUT' do
+      stub_const('TestMigration::DOWNTIME', true)
+      stub_const('TestMigration::DOWNTIME_REASON', 'foo')
+
+      expect(subject).to receive(:require).with(path)
+
+      expect(subject).to receive(:class_for_migration_file).
+        with(path).
+        and_return(TestMigration)
+
+      expect(subject).to receive(:puts).with(an_instance_of(String))
+
+      subject.check_and_print([path])
+    end
+  end
+
+  describe '#class_for_migration_file' do
+    it 'returns the class for a migration file path' do
+      expect(subject.class_for_migration_file('123_string.rb')).to eq(String)
+    end
+  end
+
+  describe '#online?' do
+    it 'returns true when a migration can be performed online' do
+      stub_const('TestMigration::DOWNTIME', false)
+
+      expect(subject.online?(TestMigration)).to eq(true)
+    end
+
+    it 'returns false when a migration can not be performed online' do
+      stub_const('TestMigration::DOWNTIME', true)
+
+      expect(subject.online?(TestMigration)).to eq(false)
+    end
+  end
+
+  describe '#downtime_reason' do
+    context 'when a reason is defined' do
+      it 'returns the downtime reason' do
+        stub_const('TestMigration::DOWNTIME_REASON', 'hello')
+
+        expect(subject.downtime_reason(TestMigration)).to eq('hello')
+      end
+    end
+
+    context 'when a reason is not defined' do
+      it 'returns nil' do
+        expect(subject.downtime_reason(Class.new)).to be_nil
+      end
+    end
+  end
+end