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
This commit is contained in:
commit
cf6de7dae9
|
@ -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
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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
|
|
@ -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
|
|
@ -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
|
|
@ -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
|
||||
|
|
|
@ -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
|
|
@ -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
|
Loading…
Reference in New Issue