summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorYorick Peterse <yorickpeterse@gmail.com>2016-06-24 18:29:23 +0200
committerYorick Peterse <yorickpeterse@gmail.com>2016-07-20 12:41:56 +0200
commita8bfe20d0dbc79616ad69b0e9c1c985ba1887407 (patch)
tree289641ee727f146a13393aa77043c1d3bc01e47b
parentd6bd412be4e3063c5f8844ef2c15f736f173b2f1 (diff)
downloadgitlab-ce-a8bfe20d0dbc79616ad69b0e9c1c985ba1887407.tar.gz
Added checks for migration downtimemigration-downtime-tags
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]
-rw-r--r--.gitlab-ci.yml3
-rw-r--r--doc/development/migration_style_guide.md46
-rw-r--r--generator_templates/active_record/migration/create_table_migration.rb8
-rw-r--r--generator_templates/active_record/migration/migration.rb8
-rw-r--r--lib/gitlab/downtime_check.rb71
-rw-r--r--lib/gitlab/downtime_check/message.rb28
-rw-r--r--lib/tasks/downtime_check.rake26
-rw-r--r--lib/tasks/gitlab/db.rake15
-rw-r--r--spec/lib/gitlab/downtime_check/message_spec.rb17
-rw-r--r--spec/lib/gitlab/downtime_check_spec.rb113
10 files changed, 311 insertions, 24 deletions
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index ff8aa351226..f566dfd76e9 100644
--- a/.gitlab-ci.yml
+++ b/.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
diff --git a/doc/development/migration_style_guide.md b/doc/development/migration_style_guide.md
index e2ca46504e7..b8fab3aaff7 100644
--- a/doc/development/migration_style_guide.md
+++ b/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
diff --git a/generator_templates/active_record/migration/create_table_migration.rb b/generator_templates/active_record/migration/create_table_migration.rb
index 27acc75dcc4..aad8626a720 100644
--- a/generator_templates/active_record/migration/create_table_migration.rb
+++ b/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
diff --git a/generator_templates/active_record/migration/migration.rb b/generator_templates/active_record/migration/migration.rb
index 06bdea11367..825bc8bdf61 100644
--- a/generator_templates/active_record/migration/migration.rb
+++ b/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
diff --git a/lib/gitlab/downtime_check.rb b/lib/gitlab/downtime_check.rb
new file mode 100644
index 00000000000..ab9537ed7d7
--- /dev/null
+++ b/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
diff --git a/lib/gitlab/downtime_check/message.rb b/lib/gitlab/downtime_check/message.rb
new file mode 100644
index 00000000000..4446e921e0d
--- /dev/null
+++ b/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
diff --git a/lib/tasks/downtime_check.rake b/lib/tasks/downtime_check.rake
new file mode 100644
index 00000000000..30a2e9be5ce
--- /dev/null
+++ b/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
diff --git a/lib/tasks/gitlab/db.rake b/lib/tasks/gitlab/db.rake
index 7230b9485be..0ec19e1a625 100644
--- a/lib/tasks/gitlab/db.rake
+++ b/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
diff --git a/spec/lib/gitlab/downtime_check/message_spec.rb b/spec/lib/gitlab/downtime_check/message_spec.rb
new file mode 100644
index 00000000000..93094cda776
--- /dev/null
+++ b/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
diff --git a/spec/lib/gitlab/downtime_check_spec.rb b/spec/lib/gitlab/downtime_check_spec.rb
new file mode 100644
index 00000000000..42d895e548e
--- /dev/null
+++ b/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