Commit 31aacc1b authored by Marius Bobin's avatar Marius Bobin

Merge branch 'remove-ff-ci-use-new-monthly-minutes' into 'master'

Remove FF ci_use_new_monthly_minutes

See merge request gitlab-org/gitlab!83274
parents f0ff58c1 9d0566ff
---
name: ci_skip_legacy_extra_minutes_recalculation
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/78476
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/341730
milestone: '14.8'
type: development
group: group::pipeline execution
default_enabled: false
......@@ -66,9 +66,7 @@ module Ci
end
def should_recalculate_purchased_minutes?
namespace.new_monthly_ci_minutes_enabled? &&
enabled? &&
any_purchased?
enabled? && any_purchased?
end
end
end
......
......@@ -97,10 +97,8 @@ module Ci
when_new_strategy.call
elsif @tracking_strategy == :legacy
when_legacy_strategy.call
elsif namespace.new_monthly_ci_minutes_enabled?
when_new_strategy.call
else
when_legacy_strategy.call
when_new_strategy.call
end
end
end
......
......@@ -257,12 +257,6 @@ module EE
end
end
def new_monthly_ci_minutes_enabled?
strong_memoize(:new_monthly_ci_minutes_enabled) do
::Feature.enabled?(:ci_use_new_monthly_minutes, self, default_enabled: :yaml)
end
end
# The same method name is used also at project level
def shared_runners_minutes_limit_enabled?
any_project_with_shared_runners_enabled? && ci_minutes_quota.enabled?
......
......@@ -366,12 +366,8 @@ module EE
!disable_overriding_approvers_per_merge_request
end
def ci_minutes_used(namespace_actor)
if namespace_actor.new_monthly_ci_minutes_enabled?
def ci_minutes_used
ci_minutes_usage.amount_used.to_i
else
shared_runners_seconds.to_i / 60
end
end
def shared_runners_available?
......
......@@ -36,12 +36,8 @@ module Ci
private
# ensure that recalculation of extra shared runners minutes occurs in the same
# transaction as the reset of the namespace statistics. If the transaction fails
# none of the changes apply but the numbers still remain consistent with each other.
def reset_ci_minutes!(namespaces)
Namespace.transaction do
recalculate_extra_shared_runners_minutes_limits!(namespaces)
reset_shared_runners_seconds!(namespaces)
reset_ci_minutes_notifications!(namespaces)
end
......@@ -57,49 +53,6 @@ module Ci
}
end
# This service is responsible for the logic that recalculates the extra shared runners
# minutes including how to deal with the cases where shared_runners_minutes_limit is `nil`.
# We prefer to keep the queries here rather than scatter them across classes.
# rubocop: disable CodeReuse/ActiveRecord
def recalculate_extra_shared_runners_minutes_limits!(namespaces)
return if Feature.enabled?(:ci_use_new_monthly_minutes, default_enabled: :yaml)
return if Feature.enabled?(:ci_skip_legacy_extra_minutes_recalculation, default_enabled: :yaml)
namespaces
.joins(:namespace_statistics)
.where(namespaces_arel[:extra_shared_runners_minutes_limit].gt(0))
.where(actual_shared_runners_minutes_limit.gt(0))
.where(namespaces_statistics_arel[:shared_runners_seconds].gt(actual_shared_runners_minutes_limit * 60))
.update_all("extra_shared_runners_minutes_limit = #{extra_minutes_left.to_sql} FROM namespace_statistics")
end
# rubocop: enable CodeReuse/ActiveRecord
def extra_minutes_left
shared_minutes_limit = actual_shared_runners_minutes_limit + namespaces_arel[:extra_shared_runners_minutes_limit]
used_minutes = arel_function("round", [namespaces_statistics_arel[:shared_runners_seconds] / Arel::Nodes::SqlLiteral.new('60.0')])
arel_function("greatest", [shared_minutes_limit - used_minutes, 0])
end
def actual_shared_runners_minutes_limit
namespaces_arel.coalesce(
namespaces_arel[:shared_runners_minutes_limit],
[::Gitlab::CurrentSettings.shared_runners_minutes.presence, 0].compact
)
end
def namespaces_arel
Namespace.arel_table
end
def namespaces_statistics_arel
NamespaceStatistics.arel_table
end
def arel_function(name, attrs)
Arel::Nodes::NamedFunction.new(name, attrs)
end
# rubocop: disable CodeReuse/ActiveRecord
def reset_shared_runners_seconds!(namespaces)
NamespaceStatistics
......
......@@ -31,29 +31,21 @@ module Ci
namespace_usage.update!(notification_level: current_alert_percentage)
if namespace.new_monthly_ci_minutes_enabled?
CiMinutesUsageMailer.notify(namespace, recipients).deliver_later
end
elsif notification.running_out?
return if namespace_usage.usage_notified?(current_alert_percentage)
namespace_usage.update!(notification_level: current_alert_percentage)
if namespace.new_monthly_ci_minutes_enabled?
CiMinutesUsageMailer.notify_limit(namespace, recipients, current_alert_percentage).deliver_later
end
end
end
def legacy_notify
if legacy_notification.no_remaining_minutes?
return if namespace.last_ci_minutes_notification_at
namespace.update_columns(last_ci_minutes_notification_at: Time.current)
unless namespace.new_monthly_ci_minutes_enabled?
CiMinutesUsageMailer.notify(namespace, recipients).deliver_later
end
elsif legacy_notification.running_out?
current_alert_percentage = legacy_notification.stage_percentage
......@@ -61,10 +53,6 @@ module Ci
return if namespace.last_ci_minutes_usage_notification_level == current_alert_percentage
namespace.update_columns(last_ci_minutes_usage_notification_level: current_alert_percentage)
unless namespace.new_monthly_ci_minutes_enabled?
CiMinutesUsageMailer.notify_limit(namespace, recipients, current_alert_percentage).deliver_later
end
end
end
......
......@@ -66,7 +66,7 @@
= project_icon(project, alt: '', class: 'avatar project-avatar s20')
%strong= link_to project.full_name, project
%td
= project.ci_minutes_used(namespace)
= project.ci_minutes_used
- if projects.blank?
%tr
%td{ colspan: 2 }
......
---
name: ci_use_new_monthly_minutes
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/71180
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/341730
milestone: '14.4'
type: development
group: group::pipeline execution
default_enabled: false
......@@ -94,25 +94,20 @@ RSpec.describe Ci::Minutes::Limit do
describe '#recalculate_remaining_purchased_minutes!' do
subject { limit.recalculate_remaining_purchased_minutes! }
where(:purchased_minutes, :namespace_monthly_limit, :previous_amount_used, :ff_enabled, :expected_purchased_limit) do
200 | 400 | 0 | true | 200 # no minutes used
200 | 0 | 0 | true | 200 # monthly limit disabled
0 | 0 | 0 | true | 0 # monthly limit disabled and no purchased minutes
200 | 400 | nil | true | 200 # no previous month usage
200 | 400 | 300 | true | 200 # previous usage < monthly limit
200 | 400 | 500 | true | 100 # previous usage > monthly limit => purchased minutes reduced
200 | 400 | 500 | false | 200 # same as above but FF disabled
0 | 400 | 500 | true | 0 # no purchased minutes = nothing reduced
200 | 400 | 600 | true | 0 # previous usage == total limit => purchased minutes reduced
200 | 400 | 600 | false | 200 # same as above but FF disabled
200 | 400 | 800 | true | 0 # previous usage > total limit => purchased minutes reduced but not negative
200 | 400 | 800 | false | 200 # same as above but FF disabled
where(:purchased_minutes, :namespace_monthly_limit, :previous_amount_used, :expected_purchased_limit) do
200 | 400 | 0 | 200 # no minutes used
200 | 0 | 0 | 200 # monthly limit disabled
0 | 0 | 0 | 0 # monthly limit disabled and no purchased minutes
200 | 400 | nil | 200 # no previous month usage
200 | 400 | 300 | 200 # previous usage < monthly limit
200 | 400 | 500 | 100 # previous usage > monthly limit => purchased minutes reduced
0 | 400 | 500 | 0 # no purchased minutes = nothing reduced
200 | 400 | 600 | 0 # previous usage == total limit => purchased minutes reduced
200 | 400 | 800 | 0 # previous usage > total limit => purchased minutes reduced but not negative
end
with_them do
before do
namespace.clear_memoization(:new_monthly_ci_minutes_enabled)
if previous_amount_used
create(:ci_namespace_monthly_usage,
namespace: namespace,
......@@ -124,8 +119,6 @@ RSpec.describe Ci::Minutes::Limit do
date: Ci::Minutes::NamespaceMonthlyUsage.beginning_of_month(3.months.ago),
amount_used: 5_000)
end
stub_feature_flags(ci_use_new_monthly_minutes: ff_enabled)
end
it 'has the expected purchased minutes' do
......
......@@ -85,14 +85,6 @@ RSpec.describe Ci::Minutes::NamespaceMonthlyUsage do
end
end
context 'when ci_use_new_monthly_minutes feature flag is disabled' do
before do
stub_feature_flags(ci_use_new_monthly_minutes: false)
end
it_behaves_like 'does not update the additional minutes'
end
context 'when limit is disabled' do
before do
namespace.update!(
......
......@@ -109,20 +109,16 @@ RSpec.describe Ci::Minutes::Quota do
end
context 'with tracking_strategy' do
where(:minutes_used, :legacy_minutes_used, :tracking_strategy, :ff_enabled, :expected_minutes) do
0 | 100 | nil | true | 0
0 | 100 | nil | false | 100
0 | 100 | :new | true | 0
0 | 100 | :new | false | 0
0 | 100 | :legacy | true | 100
0 | 100 | :legacy | false | 100
where(:minutes_used, :legacy_minutes_used, :tracking_strategy, :expected_minutes) do
0 | 100 | nil | 0
0 | 100 | :new | 0
0 | 100 | :legacy | 100
end
with_them do
let(:quota) { described_class.new(namespace, tracking_strategy: tracking_strategy) }
before do
stub_feature_flags(ci_use_new_monthly_minutes: ff_enabled)
namespace.namespace_statistics.update!(shared_runners_seconds: legacy_minutes_used.minutes)
end
......@@ -268,15 +264,5 @@ RSpec.describe Ci::Minutes::Quota do
expect(reset_date).to eq(Date.new(2021, 07, 14))
end
end
context 'when feature flag ci_use_new_monthly_minutes is disabled' do
before do
stub_feature_flags(ci_use_new_monthly_minutes: false)
end
it 'corresponds to the current time' do
expect(reset_date).to eq(Date.new(2021, 07, 14))
end
end
end
end
......@@ -752,22 +752,6 @@ RSpec.describe Namespace do
end
end
describe '#new_monthly_ci_minutes_enabled?' do
subject { namespace.new_monthly_ci_minutes_enabled? }
context 'when feature flag ci_use_new_monthly_minutes is enabled' do
it { is_expected.to be_truthy }
end
context 'when feature flag ci_use_new_monthly_minutes is disabled' do
before do
stub_feature_flags(ci_use_new_monthly_minutes: false)
end
it { is_expected.to be_falsy }
end
end
describe '#shared_runners_minutes_limit_enabled?' do
subject { namespace.shared_runners_minutes_limit_enabled? }
......
......@@ -1298,40 +1298,20 @@ RSpec.describe Project do
end
describe '#ci_minutes_used' do
subject { project.ci_minutes_used(project.namespace) }
subject { project.ci_minutes_used }
context 'when CI minutes have not been used' do
it { is_expected.to be_zero }
context 'when ci_use_new_monthly_minutes feature flag is disabled' do
before do
stub_feature_flags(ci_use_new_monthly_minutes: false)
end
it { is_expected.to be_zero }
end
end
context 'when CI minutes have been used' do
let(:minutes_used) { 70.3 }
# this difference in minutes is purely to test that the value
# comes from the expected table
let(:legacy_minutes_used) { 60.3 }
before do
create(:project_statistics, project: project, shared_runners_seconds: legacy_minutes_used.minutes)
create(:ci_project_monthly_usage, project: project, amount_used: minutes_used )
end
it { is_expected.to eq(70) }
context 'when ci_use_new_monthly_minutes feature flag is disabled' do
before do
stub_feature_flags(ci_use_new_monthly_minutes: false)
end
it { is_expected.to eq(60) }
end
end
end
......
......@@ -44,7 +44,6 @@ RSpec.describe Ci::Minutes::BatchResetService do
let(:ids_range) { (project_namespace.id..namespace_5.id) }
let(:namespaces_exceeding_minutes) { [namespace_1, namespace_2, namespace_3] }
let(:namespaces_not_exceeding_minutes) { [namespace_4, namespace_5] }
it 'resets minutes in batches for the given range and ignores project namespaces' do
expect(service).to receive(:reset_ci_minutes!).with([namespace_1, namespace_2, namespace_3])
......@@ -53,7 +52,6 @@ RSpec.describe Ci::Minutes::BatchResetService do
subject
end
context 'when feature flags ci_use_new_monthly_minutes and ci_skip_legacy_extra_minutes_recalculation are enabled' do
it 'resets CI minutes but does not recalculate purchased minutes for the namespace exceeding the monthly minutes' do
subject
......@@ -69,47 +67,6 @@ RSpec.describe Ci::Minutes::BatchResetService do
expect(namespace.last_ci_minutes_usage_notification_level).to be_nil
end
end
end
context 'when feature flag ci_use_new_monthly_minutes and ci_skip_legacy_extra_minutes_recalculation are disabled' do
before do
stub_feature_flags(
ci_use_new_monthly_minutes: false,
ci_skip_legacy_extra_minutes_recalculation: false)
end
it 'resets CI minutes and recalculates purchased minutes for the namespace exceeding the monthly minutes' do
subject
namespaces_exceeding_minutes.each do |namespace|
namespace.reset
expect(namespace.extra_shared_runners_minutes_limit).to eq 30
expect(namespace.namespace_statistics.shared_runners_seconds).to eq 0
expect(namespace.namespace_statistics.shared_runners_seconds_last_reset).to be_present
expect(ProjectStatistics.find_by(namespace: namespace).shared_runners_seconds).to eq 0
expect(ProjectStatistics.find_by(namespace: namespace).shared_runners_seconds_last_reset).to be_present
expect(namespace.last_ci_minutes_notification_at).to be_nil
expect(namespace.last_ci_minutes_usage_notification_level).to be_nil
end
end
end
it 'resets CI minutes but does not recalculate purchased minutes for the namespace not exceeding the monthly minutes' do
subject
namespaces_not_exceeding_minutes.each do |namespace|
namespace.reset
expect(namespace.extra_shared_runners_minutes_limit).to eq 50
expect(namespace.namespace_statistics.shared_runners_seconds).to eq 0
expect(namespace.namespace_statistics.shared_runners_seconds_last_reset).to be_present
expect(ProjectStatistics.find_by(namespace: namespace).shared_runners_seconds).to eq 0
expect(ProjectStatistics.find_by(namespace: namespace).shared_runners_seconds_last_reset).to be_present
expect(namespace.last_ci_minutes_notification_at).to be_nil
expect(namespace.last_ci_minutes_usage_notification_level).to be_nil
end
end
context 'when an ActiveRecordError is raised' do
before do
......@@ -137,36 +94,5 @@ RSpec.describe Ci::Minutes::BatchResetService do
end
end
end
context 'when global shared_runners_minutes setting is nil and namespaces have no limits' do
using RSpec::Parameterized::TableSyntax
where(:global_limit, :namespace_limit) do
nil | 0
nil | nil
0 | 0
0 | nil
end
with_them do
let!(:namespace) { create_namespace_with_project(100.minutes, namespace_limit) }
let(:ids_range) { [namespace.id] }
before do
allow(::Gitlab::CurrentSettings).to receive(:shared_runners_minutes).and_return(global_limit)
stub_feature_flags(
ci_use_new_monthly_minutes: false,
ci_skip_legacy_extra_minutes_recalculation: false)
end
it 'does not recalculate purchased minutes for any namespaces' do
subject
namespace.reset
expect(namespace.extra_shared_runners_minutes_limit).to eq 50
end
end
end
end
end
......@@ -10,43 +10,27 @@ RSpec.describe Ci::Minutes::EmailNotificationService do
subject { described_class.new(project).execute }
where(:ff_enabled, :monthly_minutes_limit, :minutes_used, :current_notification_level, :new_notification_level, :legacy_minutes_used, :legacy_current_notification_level, :legacy_new_notification_level, :result) do
where(:monthly_minutes_limit, :minutes_used, :current_notification_level, :new_notification_level, :legacy_minutes_used, :legacy_current_notification_level, :legacy_new_notification_level, :result) do
# when legacy and new tracking usage matches
true | 1000 | 500 | 100 | 100 | 500 | 100 | 100 | [false]
false | 1000 | 500 | 100 | 100 | 500 | 100 | 100 | [false]
true | 1000 | 800 | 100 | 30 | 800 | 100 | 30 | [true, 30]
false | 1000 | 800 | 100 | 30 | 800 | 100 | 30 | [true, 30]
true | 1000 | 800 | 30 | 30 | 800 | 30 | 30 | [false]
false | 1000 | 800 | 30 | 30 | 800 | 30 | 30 | [false]
true | 1000 | 950 | 100 | 5 | 950 | 100 | 5 | [true, 5]
false | 1000 | 950 | 100 | 5 | 950 | 100 | 5 | [true, 5]
true | 1000 | 950 | 30 | 5 | 950 | 30 | 5 | [true, 5]
false | 1000 | 950 | 30 | 5 | 950 | 30 | 5 | [true, 5]
true | 1000 | 950 | 5 | 5 | 950 | 5 | 5 | [false]
false | 1000 | 950 | 5 | 5 | 950 | 5 | 5 | [false]
true | 1000 | 1000 | 100 | 0 | 1000 | 100 | 0 | [true, 0]
false | 1000 | 1000 | 100 | 0 | 1000 | 100 | 0 | [true, 0]
true | 1000 | 1000 | 30 | 0 | 1000 | 30 | 0 | [true, 0]
false | 1000 | 1000 | 30 | 0 | 1000 | 30 | 0 | [true, 0]
true | 1000 | 1000 | 5 | 0 | 1000 | 5 | 0 | [true, 0]
false | 1000 | 1000 | 5 | 0 | 1000 | 5 | 0 | [true, 0]
true | 1000 | 1001 | 5 | 0 | 1001 | 5 | 0 | [true, 0]
false | 1000 | 1001 | 5 | 0 | 1001 | 5 | 0 | [true, 0]
true | 1000 | 1000 | 0 | 0 | 1000 | 0 | 0 | [false]
false | 1000 | 1000 | 0 | 0 | 1000 | 0 | 0 | [false]
true | 0 | 1000 | 100 | 100 | 1000 | 100 | 100 | [false]
false | 0 | 1000 | 100 | 100 | 1000 | 100 | 100 | [false]
1000 | 500 | 100 | 100 | 500 | 100 | 100 | [false]
1000 | 800 | 100 | 30 | 800 | 100 | 30 | [true, 30]
1000 | 800 | 30 | 30 | 800 | 30 | 30 | [false]
1000 | 950 | 100 | 5 | 950 | 100 | 5 | [true, 5]
1000 | 950 | 30 | 5 | 950 | 30 | 5 | [true, 5]
1000 | 950 | 5 | 5 | 950 | 5 | 5 | [false]
1000 | 1000 | 100 | 0 | 1000 | 100 | 0 | [true, 0]
1000 | 1000 | 30 | 0 | 1000 | 30 | 0 | [true, 0]
1000 | 1000 | 5 | 0 | 1000 | 5 | 0 | [true, 0]
1000 | 1001 | 5 | 0 | 1001 | 5 | 0 | [true, 0]
1000 | 1000 | 0 | 0 | 1000 | 0 | 0 | [false]
0 | 1000 | 100 | 100 | 1000 | 100 | 100 | [false]
# when legacy and new tracking usage doesn't match we send notifications
# based on the feature flag.
true | 1000 | 500 | 100 | 100 | 800 | 100 | 30 | [false]
false | 1000 | 500 | 100 | 100 | 800 | 100 | 30 | [true, 30]
true | 1000 | 800 | 100 | 30 | 500 | 100 | 100 | [true, 30]
false | 1000 | 800 | 100 | 30 | 500 | 100 | 100 | [false]
true | 1000 | 950 | 100 | 5 | 800 | 100 | 30 | [true, 5]
false | 1000 | 950 | 100 | 5 | 800 | 100 | 30 | [true, 30]
true | 1000 | 950 | 100 | 5 | 1001 | 30 | 0 | [true, 5]
false | 1000 | 950 | 100 | 5 | 1001 | 30 | 0 | [true, 0]
1000 | 500 | 100 | 100 | 800 | 100 | 30 | [false]
1000 | 800 | 100 | 30 | 500 | 100 | 100 | [true, 30]
1000 | 950 | 100 | 5 | 800 | 100 | 30 | [true, 5]
1000 | 950 | 100 | 5 | 1001 | 30 | 0 | [true, 5]
end
with_them do
......@@ -104,8 +88,6 @@ RSpec.describe Ci::Minutes::EmailNotificationService do
end
before do
stub_feature_flags(ci_use_new_monthly_minutes: ff_enabled)
if namespace.namespace_statistics
namespace.namespace_statistics.update!(shared_runners_seconds: legacy_minutes_used.minutes)
else
......@@ -249,14 +231,6 @@ RSpec.describe Ci::Minutes::EmailNotificationService do
let(:recipients) { [user.email] }
it_behaves_like 'matches the expectation'
context 'when feature flag ci_use_new_monthly_minutes is disabled' do
before do
stub_feature_flags(ci_use_new_monthly_minutes: false)
end
it_behaves_like 'matches the expectation'
end
end
context 'when on group' do
......@@ -269,14 +243,6 @@ RSpec.describe Ci::Minutes::EmailNotificationService do
end
it_behaves_like 'matches the expectation'
context 'when feature flag ci_use_new_monthly_minutes is disabled' do
before do
stub_feature_flags(ci_use_new_monthly_minutes: false)
end
it_behaves_like 'matches the expectation'
end
end
end
end
......
......@@ -66,33 +66,11 @@ RSpec.describe Ci::BatchResetMinutesWorker do
let(:namespace) { last_namespace }
end
context 'when ci_use_new_monthly_minutes is enabled' do
it 'does not recalculate purchased minutes for the namespace exceeding the monthly minutes' do
subject
expect(first_namespace.reset.extra_shared_runners_minutes_limit).to eq 50
end
end
context 'when ci_use_new_monthly_minutes and ci_skip_legacy_extra_minutes_recalculation are disabled' do
before do
stub_feature_flags(
ci_use_new_monthly_minutes: false,
ci_skip_legacy_extra_minutes_recalculation: false)
end
it 'recalculates purchased minutes for the namespace exceeding the monthly minutes' do
subject
expect(first_namespace.reset.extra_shared_runners_minutes_limit).to eq 30
end
it 'does not recalculate purchased minutes for the namespace not exceeding the monthly minutes' do
subject
expect(last_namespace.reset.extra_shared_runners_minutes_limit).to eq 50
end
end
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment