diff options
-rw-r--r-- | app/models/application_setting.rb | 7 | ||||
-rw-r--r-- | app/models/ci/build.rb | 10 | ||||
-rw-r--r-- | app/models/ci/runner.rb | 3 | ||||
-rw-r--r-- | app/models/concerns/ignorable_columns.rb | 45 | ||||
-rw-r--r-- | app/models/deploy_key.rb | 3 | ||||
-rw-r--r-- | app/models/epic.rb | 4 | ||||
-rw-r--r-- | app/models/issue.rb | 4 | ||||
-rw-r--r-- | app/models/merge_request.rb | 4 | ||||
-rw-r--r-- | app/models/project.rb | 3 | ||||
-rw-r--r-- | app/models/project_ci_cd_setting.rb | 5 | ||||
-rw-r--r-- | doc/development/database_review.md | 1 | ||||
-rw-r--r-- | doc/development/what_requires_downtime.md | 58 | ||||
-rw-r--r-- | lib/gitlab/database/obsolete_ignored_columns.rb | 9 | ||||
-rw-r--r-- | lib/tasks/db_obsolete_ignored_columns.rake | 5 | ||||
-rw-r--r-- | rubocop/cop/ignored_columns.rb | 20 | ||||
-rw-r--r-- | rubocop/rubocop.rb | 1 | ||||
-rw-r--r-- | spec/lib/gitlab/database/obsolete_ignored_columns_spec.rb | 21 | ||||
-rw-r--r-- | spec/models/concerns/ignorable_columns_spec.rb | 88 | ||||
-rw-r--r-- | spec/rubocop/cop/ignored_columns_spec.rb | 22 |
19 files changed, 272 insertions, 41 deletions
diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 72605af433f..c681d941e35 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -5,12 +5,9 @@ class ApplicationSetting < ApplicationRecord include CacheMarkdownField include TokenAuthenticatable include ChronicDurationAttribute + include IgnorableColumns - # Only remove this >= %12.6 and >= 2019-12-01 - self.ignored_columns += %i[ - pendo_enabled - pendo_url - ] + ignore_columns :pendo_enabled, :pendo_url, remove_after: '2019-12-01', remove_with: '12.6' add_authentication_token_field :runners_registration_token, encrypted: -> { Feature.enabled?(:application_settings_tokens_optional_encryption, default_enabled: true) ? :optional : :required } add_authentication_token_field :health_check_access_token diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 5192f85ad8d..caa4478c848 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -13,17 +13,11 @@ module Ci include Importable include Gitlab::Utils::StrongMemoize include HasRef + include IgnorableColumns BuildArchivedError = Class.new(StandardError) - self.ignored_columns += %i[ - artifacts_file - artifacts_file_store - artifacts_metadata - artifacts_metadata_store - artifacts_size - commands - ] + ignore_columns :artifacts_file, :artifacts_file_store, :artifacts_metadata, :artifacts_metadata_store, :artifacts_size, :commands, remove_after: '2019-12-15', remove_with: '12.7' belongs_to :project, inverse_of: :builds belongs_to :runner diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index c4a4410e8fc..3f409b8bb22 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -8,6 +8,7 @@ module Ci include ChronicDurationAttribute include FromUnion include TokenAuthenticatable + include IgnorableColumns add_authentication_token_field :token, encrypted: -> { Feature.enabled?(:ci_runners_tokens_optional_encryption, default_enabled: true) ? :optional : :required } @@ -35,7 +36,7 @@ module Ci FORM_EDITABLE = %i[description tag_list active run_untagged locked access_level maximum_timeout_human_readable].freeze - self.ignored_columns += %i[is_shared] + ignore_column :is_shared, remove_after: '2019-12-15', remove_with: '12.6' has_many :builds has_many :runner_projects, inverse_of: :runner, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent diff --git a/app/models/concerns/ignorable_columns.rb b/app/models/concerns/ignorable_columns.rb new file mode 100644 index 00000000000..744a1f0b5f3 --- /dev/null +++ b/app/models/concerns/ignorable_columns.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +module IgnorableColumns + extend ActiveSupport::Concern + + ColumnIgnore = Struct.new(:remove_after, :remove_with) do + def safe_to_remove? + Date.today > remove_after + end + + def to_s + "(#{remove_after}, #{remove_with})" + end + end + + class_methods do + # Ignore database columns in a model + # + # Indicate the earliest date and release we can stop ignoring the column with +remove_after+ (a date string) and +remove_with+ (a release) + def ignore_columns(*columns, remove_after:, remove_with:) + raise ArgumentError, 'Please indicate when we can stop ignoring columns with remove_after (date string YYYY-MM-DD), example: ignore_columns(:name, remove_after: \'2019-12-01\', remove_with: \'12.6\')' unless remove_after =~ Gitlab::Regex.utc_date_regex + raise ArgumentError, 'Please indicate in which release we can stop ignoring columns with remove_with, example: ignore_columns(:name, remove_after: \'2019-12-01\', remove_with: \'12.6\')' unless remove_with + + self.ignored_columns += columns.flatten # rubocop:disable Cop/IgnoredColumns + + columns.flatten.each do |column| + self.ignored_columns_details[column.to_sym] = ColumnIgnore.new(Date.parse(remove_after), remove_with) + end + end + + alias_method :ignore_column, :ignore_columns + + def ignored_columns_details + unless defined?(@ignored_columns_details) + IGNORE_COLUMN_MUTEX.synchronize do + @ignored_columns_details ||= superclass.try(:ignored_columns_details)&.dup || {} + end + end + + @ignored_columns_details + end + + IGNORE_COLUMN_MUTEX = Mutex.new + end +end diff --git a/app/models/deploy_key.rb b/app/models/deploy_key.rb index 22ab326a0ab..19216281e48 100644 --- a/app/models/deploy_key.rb +++ b/app/models/deploy_key.rb @@ -2,6 +2,7 @@ class DeployKey < Key include FromUnion + include IgnorableColumns has_many :deploy_keys_projects, inverse_of: :deploy_key, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :projects, through: :deploy_keys_projects @@ -10,7 +11,7 @@ class DeployKey < Key scope :are_public, -> { where(public: true) } scope :with_projects, -> { includes(deploy_keys_projects: { project: [:route, :namespace] }) } - self.ignored_columns += %i[can_push] + ignore_column :can_push, remove_after: '2019-12-15', remove_with: '12.6' accepts_nested_attributes_for :deploy_keys_projects diff --git a/app/models/epic.rb b/app/models/epic.rb index 01ef8bd100e..8222bbf9656 100644 --- a/app/models/epic.rb +++ b/app/models/epic.rb @@ -3,7 +3,9 @@ # Placeholder class for model that is implemented in EE # It reserves '&' as a reference prefix, but the table does not exists in CE class Epic < ApplicationRecord - self.ignored_columns += %i[milestone_id] + include IgnorableColumns + + ignore_column :milestone_id, remove_after: '2019-12-15', remove_with: '12.7' def self.link_reference_pattern nil diff --git a/app/models/issue.rb b/app/models/issue.rb index 7e5a94fc0a1..4a17c93e7a2 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -14,6 +14,7 @@ class Issue < ApplicationRecord include TimeTrackable include ThrottledTouch include LabelEventable + include IgnorableColumns DueDateStruct = Struct.new(:title, :name).freeze NoDueDate = DueDateStruct.new('No Due Date', '0').freeze @@ -68,8 +69,7 @@ class Issue < ApplicationRecord scope :counts_by_state, -> { reorder(nil).group(:state_id).count } - # Only remove after 2019-12-22 and with %12.7 - self.ignored_columns += %i[state] + ignore_column :state, remove_with: '12.7', remove_after: '2019-12-22' after_commit :expire_etag_cache after_save :ensure_metrics, unless: :imported? diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index b0d030c78b7..a25d1ccccca 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -17,6 +17,7 @@ class MergeRequest < ApplicationRecord include FromUnion include DeprecatedAssignee include ShaAttribute + include IgnorableColumns sha_attribute :squash_commit_sha @@ -239,8 +240,7 @@ class MergeRequest < ApplicationRecord with_state(:opened).where(auto_merge_enabled: true) end - # Only remove after 2019-12-22 and with %12.7 - self.ignored_columns += %i[state] + ignore_column :state, remove_with: '12.7', remove_after: '2019-12-22' after_save :keep_around_commit diff --git a/app/models/project.rb b/app/models/project.rb index 3177a5b83f9..b452f05c590 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -31,6 +31,7 @@ class Project < ApplicationRecord include FeatureGate include OptionallySearch include FromUnion + include IgnorableColumns extend Gitlab::Cache::RequestCache extend Gitlab::ConfigHelper @@ -64,7 +65,7 @@ class Project < ApplicationRecord # TODO: remove once GitLab 12.5 is released # https://gitlab.com/gitlab-org/gitlab/issues/34638 - self.ignored_columns += %i[merge_requests_require_code_owner_approval] + ignore_column :merge_requests_require_code_owner_approval, remove_after: '2019-12-01', remove_with: '12.6' default_value_for :archived, false default_value_for :resolve_outdated_diff_discussions, false diff --git a/app/models/project_ci_cd_setting.rb b/app/models/project_ci_cd_setting.rb index d089a004d3d..b292d39dae7 100644 --- a/app/models/project_ci_cd_setting.rb +++ b/app/models/project_ci_cd_setting.rb @@ -1,9 +1,10 @@ # frozen_string_literal: true class ProjectCiCdSetting < ApplicationRecord - # TODO: remove once GitLab 12.7 is released + include IgnorableColumns # https://gitlab.com/gitlab-org/gitlab/issues/36651 - self.ignored_columns += %i[merge_trains_enabled] + ignore_column :merge_trains_enabled, remove_with: '12.7', remove_after: '2019-12-22' + belongs_to :project, inverse_of: :ci_cd_settings # The version of the schema that first introduced this model/table. diff --git a/doc/development/database_review.md b/doc/development/database_review.md index f3c19002417..38785897361 100644 --- a/doc/development/database_review.md +++ b/doc/development/database_review.md @@ -101,6 +101,7 @@ and details for a database reviewer: - Check consistency with `db/schema.rb` and that migrations are [reversible](migration_style_guide.md#reversibility) - Check queries timing (If any): Queries executed in a migration need to fit comfortably within `15s` - preferably much less than that - on GitLab.com. + - For column removals, make sure the column has been [ignored in a previous release](what_requires_downtime.md#dropping-columns) - Check [background migrations](background_migrations.md): - Establish a time estimate for execution on GitLab.com. - They should only be used when migrating data in larger tables. diff --git a/doc/development/what_requires_downtime.md b/doc/development/what_requires_downtime.md index 00371057d3c..9e43758a4aa 100644 --- a/doc/development/what_requires_downtime.md +++ b/doc/development/what_requires_downtime.md @@ -37,11 +37,19 @@ information on how to use this method. ## Dropping Columns Removing columns is tricky because running GitLab processes may still be using -the columns. To work around this you will need two separate merge requests and -releases: one to ignore and then remove the column, and one to remove the ignore -rule. +the columns. To work around this safely, you will need three steps in three releases: -### Step 1: Ignoring The Column +1. Ignoring the column (release M) +1. Dropping the column (release M+1) +1. Removing the ignore rule (release M+2) + +The reason we spread this out across three releases is that dropping a column is +a destructive operation that can't be rolled back easily. + +Following this procedure helps us to make sure there are no deployments to GitLab.com +and upgrade processes for self-hosted installations that lump together any of these steps. + +### Step 1: Ignoring the column (release M) The first step is to ignore the column in the application code. This is necessary because Rails caches the columns and re-uses this cache in various @@ -50,18 +58,46 @@ places. This can be done by defining the columns to ignore. For example, to igno ```ruby class User < ApplicationRecord - self.ignored_columns += %i[updated_at] + include IgnorableColumns + ignore_column :updated_at, remove_with: '12.7', remove_after: '2019-12-22' end ``` -Once added you should create a _post-deployment_ migration that removes the -column. Both these changes should be submitted in the same merge request. +Multiple columns can be ignored, too: + +```ruby +ignore_columns %i[updated_at created_at], remove_with: '12.7', remove_after: '2019-12-22' +``` + +We require indication of when it is safe to remove the column ignore with: + +- `remove_with`: set to a GitLab release typically two releases (M+2) after adding the + column ignore. +- `remove_after`: set to a date after which we consider it safe to remove the column + ignore, typically within the development cycle of release M+2. + +This information allows us to reason better about column ignores and makes sure we +don't remove column ignores too early for both regular releases and deployments to GitLab.com. For +example, this avoids a situation where we deploy a bulk of changes that include both changes +to ignore the column and subsequently remove the column ignore (which would result in a downtime). + +In this example, the change to ignore the column went into release 12.5. + +### Step 2: Dropping the column (release M+1) + +Continuing our example, dropping the column goes into a _post-deployment_ migration in release 12.6: + +```ruby + remove_column :user, :updated_at +``` + +### Step 3: Removing the ignore rule (release M+2) -### Step 2: Removing The Ignore Rule +With the next release, in this example 12.7, we set up another merge request to remove the ignore rule. +This removes the `ignore_column` line and - if not needed anymore - also the inclusion of `IgnoreableColumns`. -Once the changes from step 1 have been released & deployed you can set up a -separate merge request that removes the ignore rule. This merge request can -simply remove the `self.ignored_columns` line. +This should only get merged with the release indicated with `remove_with` and once +the `remove_after` date has passed. ## Renaming Columns diff --git a/lib/gitlab/database/obsolete_ignored_columns.rb b/lib/gitlab/database/obsolete_ignored_columns.rb index 6266b6a4b65..ad5473f1b74 100644 --- a/lib/gitlab/database/obsolete_ignored_columns.rb +++ b/lib/gitlab/database/obsolete_ignored_columns.rb @@ -23,8 +23,15 @@ module Gitlab private def ignored_columns_safe_to_remove_for(klass) - ignored = klass.ignored_columns.map(&:to_s) + ignores = ignored_and_not_present(klass).each_with_object({}) do |col, h| + h[col] = klass.ignored_columns_details[col.to_sym] + end + + ignores.select { |_, i| i&.safe_to_remove? } + end + def ignored_and_not_present(klass) + ignored = klass.ignored_columns.map(&:to_s) return [] if ignored.empty? schema = klass.connection.schema_cache.columns_hash(klass.table_name) diff --git a/lib/tasks/db_obsolete_ignored_columns.rake b/lib/tasks/db_obsolete_ignored_columns.rake index 184e407f28c..00f60231f4f 100644 --- a/lib/tasks/db_obsolete_ignored_columns.rake +++ b/lib/tasks/db_obsolete_ignored_columns.rake @@ -8,7 +8,10 @@ task 'db:obsolete_ignored_columns' => :environment do puts 'The following `ignored_columns` are obsolete and can be removed:' list.each do |name, ignored_columns| - puts "- #{name}: #{ignored_columns.join(', ')}" + puts "#{name}:" + ignored_columns.each do |column, removal| + puts " - #{column.ljust(30)} Remove after #{removal.remove_after} with #{removal.remove_with}" + end end puts <<~TEXT diff --git a/rubocop/cop/ignored_columns.rb b/rubocop/cop/ignored_columns.rb new file mode 100644 index 00000000000..14bcfa04ae1 --- /dev/null +++ b/rubocop/cop/ignored_columns.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + # Cop that blacklists the usage of Group.public_or_visible_to_user + class IgnoredColumns < RuboCop::Cop::Cop + MSG = 'Use `IgnoredColumns` concern instead of adding to `self.ignored_columns`.' + + def_node_matcher :ignored_columns?, <<~PATTERN + (send (self) :ignored_columns) + PATTERN + + def on_send(node) + return unless ignored_columns?(node) + + add_offense(node, location: :expression) + end + end + end +end diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb index 49d582bf034..9c9948e2a61 100644 --- a/rubocop/rubocop.rb +++ b/rubocop/rubocop.rb @@ -54,3 +54,4 @@ require_relative 'cop/group_public_or_visible_to_user' require_relative 'cop/inject_enterprise_edition_module' require_relative 'cop/graphql/authorize_types' require_relative 'cop/graphql/descriptions' +require_relative 'cop/ignored_columns' diff --git a/spec/lib/gitlab/database/obsolete_ignored_columns_spec.rb b/spec/lib/gitlab/database/obsolete_ignored_columns_spec.rb index 6d38f7f1b95..b3826666b18 100644 --- a/spec/lib/gitlab/database/obsolete_ignored_columns_spec.rb +++ b/spec/lib/gitlab/database/obsolete_ignored_columns_spec.rb @@ -8,21 +8,27 @@ describe Gitlab::Database::ObsoleteIgnoredColumns do end class SomeAbstract < MyBase + include IgnorableColumns + self.abstract_class = true self.table_name = 'projects' - self.ignored_columns += %i[unused] + ignore_column :unused, remove_after: '2019-01-01', remove_with: '12.0' end class B < MyBase + include IgnorableColumns + self.table_name = 'issues' - self.ignored_columns += %i[id other] + ignore_column :id, :other, remove_after: '2019-01-01', remove_with: '12.0' + ignore_column :not_used_but_still_ignored, remove_after: Date.today.to_s, remove_with: '12.1' end class A < SomeAbstract - self.ignored_columns += %i[id also_unused] + ignore_column :also_unused, remove_after: '2019-02-01', remove_with: '12.1' + ignore_column :not_used_but_still_ignored, remove_after: Date.today.to_s, remove_with: '12.1' end class C < MyBase @@ -35,8 +41,13 @@ describe Gitlab::Database::ObsoleteIgnoredColumns do describe '#execute' do it 'returns a list of class names and columns pairs' do expect(subject.execute).to eq([ - ['Testing::A', %w(unused also_unused)], - ['Testing::B', %w(other)] + ['Testing::A', { + 'unused' => IgnorableColumns::ColumnIgnore.new(Date.parse('2019-01-01'), '12.0'), + 'also_unused' => IgnorableColumns::ColumnIgnore.new(Date.parse('2019-02-01'), '12.1') + }], + ['Testing::B', { + 'other' => IgnorableColumns::ColumnIgnore.new(Date.parse('2019-01-01'), '12.0') + }] ]) end end diff --git a/spec/models/concerns/ignorable_columns_spec.rb b/spec/models/concerns/ignorable_columns_spec.rb new file mode 100644 index 00000000000..55efa1b5fda --- /dev/null +++ b/spec/models/concerns/ignorable_columns_spec.rb @@ -0,0 +1,88 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe IgnorableColumns do + let(:record_class) do + Class.new(ApplicationRecord) do + include IgnorableColumns + end + end + + subject { record_class } + + it 'adds columns to ignored_columns' do + expect do + subject.ignore_columns(:name, :created_at, remove_after: '2019-12-01', remove_with: '12.6') + end.to change { subject.ignored_columns }.from([]).to(%w(name created_at)) + end + + it 'adds columns to ignored_columns (array version)' do + expect do + subject.ignore_columns(%i[name created_at], remove_after: '2019-12-01', remove_with: '12.6') + end.to change { subject.ignored_columns }.from([]).to(%w(name created_at)) + end + + it 'requires remove_after attribute to be set' do + expect { subject.ignore_columns(:name, remove_after: nil, remove_with: 12.6) }.to raise_error(ArgumentError, /Please indicate/) + end + + it 'requires remove_after attribute to be set' do + expect { subject.ignore_columns(:name, remove_after: "not a date", remove_with: 12.6) }.to raise_error(ArgumentError, /Please indicate/) + end + + it 'requires remove_with attribute to be set' do + expect { subject.ignore_columns(:name, remove_after: '2019-12-01', remove_with: nil) }.to raise_error(ArgumentError, /Please indicate/) + end + + describe '.ignored_columns_details' do + shared_examples_for 'storing removal information' do + it 'storing removal information' do + subject.ignore_column(columns, remove_after: '2019-12-01', remove_with: '12.6') + + [columns].flatten.each do |column| + expect(subject.ignored_columns_details[column].remove_after).to eq(Date.parse('2019-12-01')) + expect(subject.ignored_columns_details[column].remove_with).to eq('12.6') + end + end + end + + context 'with single column' do + let(:columns) { :name } + it_behaves_like 'storing removal information' + end + + context 'with array column' do + let(:columns) { %i[name created_at] } + it_behaves_like 'storing removal information' + end + + it 'defaults to empty Hash' do + expect(subject.ignored_columns_details).to eq({}) + end + end + + describe IgnorableColumns::ColumnIgnore do + subject { described_class.new(remove_after, remove_with) } + + let(:remove_with) { double } + + describe '#safe_to_remove?' do + context 'after remove_after date has passed' do + let(:remove_after) { Date.parse('2019-01-10') } + + it 'returns true (safe to remove)' do + expect(subject.safe_to_remove?).to be_truthy + end + end + + context 'before remove_after date has passed' do + let(:remove_after) { Date.today } + + it 'returns false (not safe to remove)' do + expect(subject.safe_to_remove?).to be_falsey + end + end + end + end +end diff --git a/spec/rubocop/cop/ignored_columns_spec.rb b/spec/rubocop/cop/ignored_columns_spec.rb new file mode 100644 index 00000000000..64437765018 --- /dev/null +++ b/spec/rubocop/cop/ignored_columns_spec.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'rubocop' +require 'rubocop/rspec/support' +require_relative '../../../rubocop/cop/ignored_columns' + +describe RuboCop::Cop::IgnoredColumns do + include CopHelper + + subject(:cop) { described_class.new } + + it 'flags the use of destroy_all with a local variable receiver' do + inspect_source(<<~RUBY) + class Foo < ApplicationRecord + self.ignored_columns += %i[id] + end + RUBY + + expect(cop.offenses.size).to eq(1) + end +end |