From 3da396178fde0f1a5d42a0ff097c15bf83ebafe1 Mon Sep 17 00:00:00 2001
From: Jason Goodman <jgoodman@gitlab.com>
Date: Mon, 24 Jul 2023 15:27:19 +0000
Subject: [PATCH] Use the namespace storage forks cost factor application
setting
Refactor classes using the placeholder constant
Replace constant with application setting
Extract CostFactor module
---
.../namespace_storage_forks_cost_factor.yml | 8 ++
.../ee/namespace/root_storage_statistics.rb | 2 +-
ee/app/models/ee/project_statistics.rb | 6 +-
.../models/namespaces/storage/cost_factor.rb | 34 +++++++
ee/app/models/namespaces/storage/root_size.rb | 2 -
.../groups/usage_quotas/storage_tab_spec.rb | 4 +-
.../namespace/root_storage_statistics_spec.rb | 12 ++-
ee/spec/models/ee/project_statistics_spec.rb | 3 +-
.../namespaces/storage/cost_factor_spec.rb | 92 +++++++++++++++++++
.../namespaces/storage/root_size_spec.rb | 3 +-
10 files changed, 150 insertions(+), 16 deletions(-)
create mode 100644 config/feature_flags/development/namespace_storage_forks_cost_factor.yml
create mode 100644 ee/app/models/namespaces/storage/cost_factor.rb
create mode 100644 ee/spec/models/namespaces/storage/cost_factor_spec.rb
diff --git a/config/feature_flags/development/namespace_storage_forks_cost_factor.yml b/config/feature_flags/development/namespace_storage_forks_cost_factor.yml
new file mode 100644
index 000000000000..60b4980389be
--- /dev/null
+++ b/config/feature_flags/development/namespace_storage_forks_cost_factor.yml
@@ -0,0 +1,8 @@
+---
+name: namespace_storage_forks_cost_factor
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/126775
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/419181
+milestone: '16.3'
+type: development
+group: group::utilization
+default_enabled: false
diff --git a/ee/app/models/ee/namespace/root_storage_statistics.rb b/ee/app/models/ee/namespace/root_storage_statistics.rb
index fb5e855ad405..ba37798367ab 100644
--- a/ee/app/models/ee/namespace/root_storage_statistics.rb
+++ b/ee/app/models/ee/namespace/root_storage_statistics.rb
@@ -29,7 +29,7 @@ def forks_size_reduction
end
def inverted_cost_factor_for_forks
- 1 - ::Namespaces::Storage::RootSize::COST_FACTOR_FOR_FORKS
+ ::Namespaces::Storage::CostFactor.inverted_cost_factor_for_forks(namespace)
end
override :from_namespace_statistics
diff --git a/ee/app/models/ee/project_statistics.rb b/ee/app/models/ee/project_statistics.rb
index 9485711b2682..95572274a0dd 100644
--- a/ee/app/models/ee/project_statistics.rb
+++ b/ee/app/models/ee/project_statistics.rb
@@ -11,11 +11,7 @@ def cost_factored_storage_size
private
def cost_factor
- if project.forked? && (project.root_ancestor.paid? || !project.private?)
- ::Namespaces::Storage::RootSize::COST_FACTOR_FOR_FORKS
- else
- ::Namespaces::Storage::RootSize::COST_FACTOR
- end
+ ::Namespaces::Storage::CostFactor.cost_factor_for(project)
end
override :storage_size_components
diff --git a/ee/app/models/namespaces/storage/cost_factor.rb b/ee/app/models/namespaces/storage/cost_factor.rb
new file mode 100644
index 000000000000..0ac61f8b15b1
--- /dev/null
+++ b/ee/app/models/namespaces/storage/cost_factor.rb
@@ -0,0 +1,34 @@
+# frozen_string_literal: true
+
+module Namespaces
+ module Storage
+ module CostFactor
+ extend self
+
+ FULL_COST = 1.0
+
+ def cost_factor_for(project)
+ if project.forked? && (project.root_ancestor.paid? || !project.private?)
+ forks_cost_factor(project.root_ancestor)
+ else
+ FULL_COST
+ end
+ end
+
+ def inverted_cost_factor_for_forks(root_namespace)
+ FULL_COST - forks_cost_factor(root_namespace)
+ end
+
+ private
+
+ def forks_cost_factor(root_namespace)
+ if ::Gitlab::CurrentSettings.should_check_namespace_plan? &&
+ ::Feature.enabled?(:namespace_storage_forks_cost_factor, root_namespace)
+ ::Gitlab::CurrentSettings.namespace_storage_forks_cost_factor
+ else
+ FULL_COST
+ end
+ end
+ end
+ end
+end
diff --git a/ee/app/models/namespaces/storage/root_size.rb b/ee/app/models/namespaces/storage/root_size.rb
index cd3768efc211..9e4018bcbdc7 100644
--- a/ee/app/models/namespaces/storage/root_size.rb
+++ b/ee/app/models/namespaces/storage/root_size.rb
@@ -6,8 +6,6 @@ class RootSize
CURRENT_SIZE_CACHE_KEY = 'root_storage_current_size'
EXPIRATION_TIME = 10.minutes
LIMIT_CACHE_NAME = 'root_storage_size_limit'
- COST_FACTOR_FOR_FORKS = 1.0
- COST_FACTOR = 1.0
def initialize(root_namespace)
@root_namespace = root_namespace.root_ancestor # just in case the true root isn't passed
diff --git a/ee/spec/features/groups/usage_quotas/storage_tab_spec.rb b/ee/spec/features/groups/usage_quotas/storage_tab_spec.rb
index 1b33b892d627..e8c877294dfe 100644
--- a/ee/spec/features/groups/usage_quotas/storage_tab_spec.rb
+++ b/ee/spec/features/groups/usage_quotas/storage_tab_spec.rb
@@ -69,7 +69,7 @@
context 'with a cost factor for forks' do
before do
- stub_const('Namespaces::Storage::RootSize::COST_FACTOR_FOR_FORKS', 0.1)
+ stub_ee_application_setting(namespace_storage_forks_cost_factor: 0.1)
end
it 'displays the storage size with the cost factor applied' do
@@ -93,7 +93,7 @@
context 'with a cost factor for forks' do
before do
- stub_const('Namespaces::Storage::RootSize::COST_FACTOR_FOR_FORKS', 0.1)
+ stub_ee_application_setting(namespace_storage_forks_cost_factor: 0.1)
end
it 'displays the total storage size taking into account the cost factor' do
diff --git a/ee/spec/models/ee/namespace/root_storage_statistics_spec.rb b/ee/spec/models/ee/namespace/root_storage_statistics_spec.rb
index db23e12b0ac0..c72f37b4d100 100644
--- a/ee/spec/models/ee/namespace/root_storage_statistics_spec.rb
+++ b/ee/spec/models/ee/namespace/root_storage_statistics_spec.rb
@@ -96,9 +96,13 @@
describe '#cost_factored_storage_size', :saas do
let_it_be(:group) { create(:group_with_plan, plan: :ultimate_plan) }
+ before do
+ stub_ee_application_setting(check_namespace_plan: true)
+ end
+
context 'with a cost factor for forks' do
before do
- stub_const("::Namespaces::Storage::RootSize::COST_FACTOR_FOR_FORKS", 0.05)
+ stub_ee_application_setting(namespace_storage_forks_cost_factor: 0.05)
end
context 'with a free plan' do
@@ -166,7 +170,7 @@
context 'with a fork cost factor of 1' do
before do
- stub_const("::Namespaces::Storage::RootSize::COST_FACTOR_FOR_FORKS", 1.0)
+ stub_ee_application_setting(namespace_storage_forks_cost_factor: 1.0)
end
it 'considers forks to take up their full actual disk storage' do
@@ -179,7 +183,7 @@
context 'with a fork cost factor of 0' do
before do
- stub_const("::Namespaces::Storage::RootSize::COST_FACTOR_FOR_FORKS", 0)
+ stub_ee_application_setting(namespace_storage_forks_cost_factor: 0)
end
it 'considers forks to take up no storage at all' do
@@ -192,7 +196,7 @@
context 'when the cost factor would result in a fractional storage_size' do
before do
- stub_const("::Namespaces::Storage::RootSize::COST_FACTOR_FOR_FORKS", 0.1)
+ stub_ee_application_setting(namespace_storage_forks_cost_factor: 0.1)
end
it 'rounds to the nearest integer' do
diff --git a/ee/spec/models/ee/project_statistics_spec.rb b/ee/spec/models/ee/project_statistics_spec.rb
index a2c775032170..05bf680f9839 100644
--- a/ee/spec/models/ee/project_statistics_spec.rb
+++ b/ee/spec/models/ee/project_statistics_spec.rb
@@ -82,7 +82,8 @@
context 'when there is a cost factor for forks' do
before do
- stub_const('Namespaces::Storage::RootSize::COST_FACTOR_FOR_FORKS', 0.1)
+ stub_ee_application_setting(check_namespace_plan: true)
+ stub_ee_application_setting(namespace_storage_forks_cost_factor: 0.1)
end
where(:plan, :fork_visibility) do
diff --git a/ee/spec/models/namespaces/storage/cost_factor_spec.rb b/ee/spec/models/namespaces/storage/cost_factor_spec.rb
new file mode 100644
index 000000000000..131bbdb570ad
--- /dev/null
+++ b/ee/spec/models/namespaces/storage/cost_factor_spec.rb
@@ -0,0 +1,92 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Namespaces::Storage::CostFactor, feature_category: :consumables_cost_management do
+ let(:full_cost) { 1.0 }
+ let(:forks_cost_factor) { 0.2 }
+ let(:paid_group) { build_group }
+ let(:free_group) { build_group(paid: false) }
+
+ before do
+ stub_ee_application_setting(namespace_storage_forks_cost_factor: forks_cost_factor)
+ stub_ee_application_setting(check_namespace_plan: true)
+ stub_feature_flags(namespace_storage_forks_cost_factor: true)
+ end
+
+ describe '.cost_factor_for' do
+ it 'returns the forks cost factor for a project that is a fork' do
+ project = build_fork(group: paid_group)
+
+ expect(described_class.cost_factor_for(project)).to eq(forks_cost_factor)
+ end
+
+ it 'returns full cost for a project that is not a fork' do
+ project = build_project(group: paid_group)
+
+ expect(described_class.cost_factor_for(project)).to eq(full_cost)
+ end
+
+ it 'returns full cost for a private fork in a free namespace' do
+ project = build_fork(group: free_group, visibility_level: Gitlab::VisibilityLevel::PRIVATE)
+
+ expect(described_class.cost_factor_for(project)).to eq(full_cost)
+ end
+
+ it 'returns full cost when the feature flag is false' do
+ stub_feature_flags(namespace_storage_forks_cost_factor: false)
+ project = build_fork(group: paid_group)
+
+ expect(described_class.cost_factor_for(project)).to eq(full_cost)
+ end
+
+ it 'returns full cost when namespace plans are not checked' do
+ stub_ee_application_setting(check_namespace_plan: false)
+ project = build_fork(group: paid_group)
+
+ expect(described_class.cost_factor_for(project)).to eq(full_cost)
+ end
+ end
+
+ describe '.inverted_cost_factor_for_forks' do
+ it 'returns the inverse of the cost factor for forks' do
+ expect(described_class.inverted_cost_factor_for_forks(paid_group)).to eq(0.8)
+ end
+
+ it 'returns the inverse of full cost when the feature flag is false' do
+ stub_feature_flags(namespace_storage_forks_cost_factor: false)
+
+ expect(described_class.inverted_cost_factor_for_forks(paid_group)).to eq(0)
+ end
+
+ it 'returns the inverse of full cost when namespace plans are not checked' do
+ stub_ee_application_setting(check_namespace_plan: false)
+
+ expect(described_class.inverted_cost_factor_for_forks(paid_group)).to eq(0)
+ end
+ end
+
+ def build_group(paid: true)
+ group = build_stubbed(:group)
+
+ allow(group).to receive(:paid?).and_return(paid)
+
+ group
+ end
+
+ def build_project(group:, visibility_level: Gitlab::VisibilityLevel::PUBLIC)
+ project = build_stubbed(:project, visibility_level: visibility_level)
+
+ allow(project).to receive(:root_ancestor).and_return(group)
+
+ project
+ end
+
+ def build_fork(group:, visibility_level: Gitlab::VisibilityLevel::PUBLIC)
+ project = build_project(group: group, visibility_level: visibility_level)
+
+ allow(project).to receive(:forked?).and_return(true)
+
+ project
+ end
+end
diff --git a/ee/spec/models/namespaces/storage/root_size_spec.rb b/ee/spec/models/namespaces/storage/root_size_spec.rb
index a806b98f25b1..e1ee772af43e 100644
--- a/ee/spec/models/namespaces/storage/root_size_spec.rb
+++ b/ee/spec/models/namespaces/storage/root_size_spec.rb
@@ -212,7 +212,8 @@
end
before do
- stub_const("#{described_class}::COST_FACTOR_FOR_FORKS", 0.05)
+ stub_ee_application_setting(check_namespace_plan: true)
+ stub_ee_application_setting(namespace_storage_forks_cost_factor: 0.05)
end
it 'returns the cost factored storage size' do
--
GitLab