From a8bfe20d0dbc79616ad69b0e9c1c985ba1887407 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Fri, 24 Jun 2016 18:29:23 +0200 Subject: 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 | 3 +- doc/development/migration_style_guide.md | 46 ++++----- .../migration/create_table_migration.rb | 8 ++ .../active_record/migration/migration.rb | 8 ++ lib/gitlab/downtime_check.rb | 71 +++++++++++++ lib/gitlab/downtime_check/message.rb | 28 +++++ lib/tasks/downtime_check.rake | 26 +++++ lib/tasks/gitlab/db.rake | 15 +++ spec/lib/gitlab/downtime_check/message_spec.rb | 17 ++++ spec/lib/gitlab/downtime_check_spec.rb | 113 +++++++++++++++++++++ 10 files changed, 311 insertions(+), 24 deletions(-) create mode 100644 lib/gitlab/downtime_check.rb create mode 100644 lib/gitlab/downtime_check/message.rb create mode 100644 lib/tasks/downtime_check.rake create mode 100644 spec/lib/gitlab/downtime_check/message_spec.rb create mode 100644 spec/lib/gitlab/downtime_check_spec.rb 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 -- cgit v1.2.3