Merge branch 'migration-downtime-tags' into 'master'

Added checks for migration downtime

This adds a set of checks that check/list which migrations require downtime (or not). It also comes with a CI task that fails should a migration not be tagged properly.

Fixes #14545

See merge request !4911

.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 ...'
 
-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.
+  def change
+    ...
+  end
+end
+```
 
-If you don't provide the information it means that a migration is safe to run online.
+It is an error (that is, CI will fail) if the `DOWNTIME` constant is missing
+from a migration class.
 
-### 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

+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

+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

+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

+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

+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