diff options
| -rw-r--r-- | .gitlab-ci.yml | 3 | ||||
| -rw-r--r-- | doc/development/migration_style_guide.md | 46 | ||||
| -rw-r--r-- | generator_templates/active_record/migration/create_table_migration.rb | 8 | ||||
| -rw-r--r-- | generator_templates/active_record/migration/migration.rb | 8 | ||||
| -rw-r--r-- | lib/gitlab/downtime_check.rb | 71 | ||||
| -rw-r--r-- | lib/gitlab/downtime_check/message.rb | 28 | ||||
| -rw-r--r-- | lib/tasks/downtime_check.rake | 26 | ||||
| -rw-r--r-- | lib/tasks/gitlab/db.rake | 15 | ||||
| -rw-r--r-- | spec/lib/gitlab/downtime_check/message_spec.rb | 17 | ||||
| -rw-r--r-- | spec/lib/gitlab/downtime_check_spec.rb | 113 |
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 |