From f999af7cc8a087a7281686156c282f090b208310 Mon Sep 17 00:00:00 2001 From: Alper Akgun Date: Wed, 10 Jan 2024 14:46:24 +0300 Subject: [PATCH 1/2] Enforce workspaces_quota and workspaces_per_user_quota Coming from agent config while creating the workspace Changelog: added --- .../remote_development_agent_config.rb | 7 +++- ee/app/models/remote_development/workspace.rb | 24 +++++++++++ .../remote_development/workspace_spec.rb | 42 +++++++++++++++++++ locale/gitlab.pot | 6 +++ 4 files changed, 77 insertions(+), 2 deletions(-) diff --git a/ee/app/models/remote_development/remote_development_agent_config.rb b/ee/app/models/remote_development/remote_development_agent_config.rb index 63171b708b7440..fc46e59a2b49ab 100644 --- a/ee/app/models/remote_development/remote_development_agent_config.rb +++ b/ee/app/models/remote_development/remote_development_agent_config.rb @@ -6,6 +6,8 @@ class RemoteDevelopmentAgentConfig < ApplicationRecord # https://gitlab.com/gitlab-org/gitlab/-/issues/410045#note_1385602915 include IgnorableColumns + UNLIMITED_QUOTA = -1 + ignore_column :max_workspaces, remove_with: '16.8', remove_after: '2023-12-22' ignore_column :max_workspaces_per_user, remove_with: '16.8', remove_after: '2023-12-22' @@ -28,8 +30,9 @@ class RemoteDevelopmentAgentConfig < ApplicationRecord validates :max_resources_per_workspace, json_schema: { filename: 'remote_development_agent_configs_workspace_container_resources' } validates :max_resources_per_workspace, 'remote_development/workspace_container_resources': true - validates :workspaces_quota, numericality: { only_integer: true, greater_than_or_equal_to: -1 } - validates :workspaces_per_user_quota, numericality: { only_integer: true, greater_than_or_equal_to: -1 } + validates :workspaces_quota, numericality: { only_integer: true, greater_than_or_equal_to: UNLIMITED_QUOTA } + validates :workspaces_per_user_quota, + numericality: { only_integer: true, greater_than_or_equal_to: UNLIMITED_QUOTA } # noinspection RubyResolve - likely due to https://handbook.gitlab.com/handbook/tools-and-tips/editors-and-ides/jetbrains-ides/tracked-jetbrains-issues/#ruby-25400 before_validation :prevent_dns_zone_update, if: ->(record) { record.persisted? && record.dns_zone_changed? } diff --git a/ee/app/models/remote_development/workspace.rb b/ee/app/models/remote_development/workspace.rb index 49036df610d112..0dc3cbdaa2b001 100644 --- a/ee/app/models/remote_development/workspace.rb +++ b/ee/app/models/remote_development/workspace.rb @@ -6,6 +6,7 @@ class Workspace < ApplicationRecord include Sortable # noinspection RubyResolve - https://handbook.gitlab.com/handbook/tools-and-tips/editors-and-ides/jetbrains-ides/tracked-jetbrains-issues/#ruby-31542 include RemoteDevelopment::Workspaces::States + include ::Gitlab::Utils::StrongMemoize MAX_HOURS_BEFORE_TERMINATION_LIMIT = 120 @@ -18,6 +19,7 @@ class Workspace < ApplicationRecord # noinspection RailsParamDefResolve - https://handbook.gitlab.com/handbook/tools-and-tips/editors-and-ides/jetbrains-ides/tracked-jetbrains-issues/#ruby-31542 has_one :remote_development_agent_config, through: :agent, source: :remote_development_agent_config + alias_attribute :agent_config, :remote_development_agent_config has_many :workspace_variables, class_name: 'RemoteDevelopment::WorkspaceVariable', inverse_of: :workspace delegate :dns_zone, to: :remote_development_agent_config, prefix: false, allow_nil: false @@ -39,6 +41,7 @@ class Workspace < ApplicationRecord validates :max_hours_before_termination, numericality: { less_than_or_equal_to: MAX_HOURS_BEFORE_TERMINATION_LIMIT } validate :enforce_permanent_termination + validate :enforce_quotas, on: :create scope :with_desired_state_updated_more_recently_than_last_response_to_agent, -> do where('desired_state_updated_at >= responded_to_agent_at').or(where(responded_to_agent_at: nil)) @@ -59,6 +62,11 @@ class Workspace < ApplicationRecord actual_state: RemoteDevelopment::Workspaces::States::TERMINATED ) end + scope :without_terminated_actual_state, -> do + where.not( + actual_state: RemoteDevelopment::Workspaces::States::TERMINATED + ) + end # noinspection RubyResolve - https://handbook.gitlab.com/handbook/tools-and-tips/editors-and-ides/jetbrains-ides/tracked-jetbrains-issues/#ruby-32287 before_save :touch_desired_state_updated_at, if: ->(workspace) do @@ -91,6 +99,22 @@ def enforce_permanent_termination errors.add(:desired_state, "is 'Terminated', and cannot be updated. Create a new workspace instead.") end + # rubocop:disable Layout/LineLength -- Long messages for UI + def enforce_quotas + if exceeds_workspaces_per_user_quota? + errors.add :base, + format(s_('RemoteDevelopment|You cannot create a workspace because you already have "%{count}" existing workspaces for the given agent with a per user quota of "%{quota}" workspaces'), + count: actual_workspaces_count_for_current_user_and_agent, + quota: agent_config.workspaces_per_user_quota) + elsif exceeds_workspaces_quota? + errors.add :base, + format(s_('RemoteDevelopment|You cannot create a workspace because there are already "%{count}" existing workspaces for the given agent with a total quota of "%{quota}" workspaces'), + count: actual_workspaces_count_for_current_agent, + quota: agent_config.workspaces_quota) + end + end + # rubocop:enable Layout/LineLength -- Long messages for UI + def touch_desired_state_updated_at # noinspection RubyResolve - https://handbook.gitlab.com/handbook/tools-and-tips/editors-and-ides/jetbrains-ides/tracked-jetbrains-issues/#ruby-31542 self.desired_state_updated_at = Time.current.utc diff --git a/ee/spec/models/remote_development/workspace_spec.rb b/ee/spec/models/remote_development/workspace_spec.rb index 836fca2809be10..146dce4e8bf1a9 100644 --- a/ee/spec/models/remote_development/workspace_spec.rb +++ b/ee/spec/models/remote_development/workspace_spec.rb @@ -156,5 +156,47 @@ .to include("is 'Terminated', and cannot be updated. Create a new workspace instead.") end end + + describe '#enforce_quotas' do + subject(:workspace) do + build(:workspace, + user: user, + agent: agent, + project: project, + personal_access_token: personal_access_token, desired_state: desired_state) + end + + before do + allow(workspace).to receive(:exceeds_workspaces_per_user_quota?).and_return(false) + allow(workspace).to receive(:exceeds_workspaces_quota?).and_return(false) + end + + it 'does not add base errors when quotas are not exceeded' do + workspace.validate + expect(workspace.errors[:base]).to be_empty + end + + it 'adds base error when per user quota exceeded' do + allow(workspace).to receive(:agent_config).and_return(instance_double( + ::RemoteDevelopment::RemoteDevelopmentAgentConfig, workspaces_per_user_quota: 5)) + allow(workspace).to receive(:actual_workspaces_count_for_current_user_and_agent).and_return(6) + allow(workspace).to receive(:exceeds_workspaces_per_user_quota?).and_return(true) + workspace.validate + message = "You cannot create a workspace because you already have \"6\" \ +existing workspaces for the given agent with a per user quota of \"5\" workspaces" + expect(workspace.errors[:base]).to include(message) + end + + it 'adds base error when total quota exceeded' do + allow(workspace).to receive(:agent_config).and_return(instance_double( + ::RemoteDevelopment::RemoteDevelopmentAgentConfig, workspaces_quota: 3)) + allow(workspace).to receive(:actual_workspaces_count_for_current_agent).and_return(3) + allow(workspace).to receive(:exceeds_workspaces_quota?).and_return(true) + workspace.validate + message = "You cannot create a workspace because there are already \"3\" \ +existing workspaces for the given agent with a total quota of \"3\" workspaces" + expect(workspace.errors[:base]).to include(message) + end + end end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index b41845620a1193..27092459c2a7e8 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -40542,6 +40542,12 @@ msgstr "" msgid "RemoteDevelopment|Workspaces" msgstr "" +msgid "RemoteDevelopment|You cannot create a workspace because there are already \"%{count}\" existing workspaces for the given agent with a total quota of \"%{quota}\" workspaces" +msgstr "" + +msgid "RemoteDevelopment|You cannot create a workspace because you already have \"%{count}\" existing workspaces for the given agent with a per user quota of \"%{quota}\" workspaces" +msgstr "" + msgid "Remove" msgstr "" -- GitLab From ff4a4a604b8aea8fbf29fd6989736d65dc597f53 Mon Sep 17 00:00:00 2001 From: Alper Akgun Date: Mon, 15 Jan 2024 12:59:00 +0300 Subject: [PATCH 2/2] Desired state and renamings --- ee/app/models/remote_development/workspace.rb | 48 ++++- .../remote_development/workspace_spec.rb | 178 +++++++++++++++++- 2 files changed, 214 insertions(+), 12 deletions(-) diff --git a/ee/app/models/remote_development/workspace.rb b/ee/app/models/remote_development/workspace.rb index 0dc3cbdaa2b001..ce311cba6b5159 100644 --- a/ee/app/models/remote_development/workspace.rb +++ b/ee/app/models/remote_development/workspace.rb @@ -19,7 +19,6 @@ class Workspace < ApplicationRecord # noinspection RailsParamDefResolve - https://handbook.gitlab.com/handbook/tools-and-tips/editors-and-ides/jetbrains-ides/tracked-jetbrains-issues/#ruby-31542 has_one :remote_development_agent_config, through: :agent, source: :remote_development_agent_config - alias_attribute :agent_config, :remote_development_agent_config has_many :workspace_variables, class_name: 'RemoteDevelopment::WorkspaceVariable', inverse_of: :workspace delegate :dns_zone, to: :remote_development_agent_config, prefix: false, allow_nil: false @@ -62,11 +61,6 @@ class Workspace < ApplicationRecord actual_state: RemoteDevelopment::Workspaces::States::TERMINATED ) end - scope :without_terminated_actual_state, -> do - where.not( - actual_state: RemoteDevelopment::Workspaces::States::TERMINATED - ) - end # noinspection RubyResolve - https://handbook.gitlab.com/handbook/tools-and-tips/editors-and-ides/jetbrains-ides/tracked-jetbrains-issues/#ruby-32287 before_save :touch_desired_state_updated_at, if: ->(workspace) do @@ -79,6 +73,43 @@ def desired_state_updated_more_recently_than_last_response_to_agent? desired_state_updated_at >= responded_to_agent_at end + def workspaces_count_for_current_user_and_agent + Workspace + .desired_state_not_terminated + .by_user_ids(user_id) + .by_agent_ids(cluster_agent_id) + .count + end + strong_memoize_attr :workspaces_count_for_current_user_and_agent + + def workspaces_count_for_current_agent + Workspace + .desired_state_not_terminated + .by_agent_ids(cluster_agent_id) + .count + end + strong_memoize_attr :workspaces_count_for_current_agent + + def exceeds_workspaces_per_user_quota? + return unless remote_development_agent_config + + quota = remote_development_agent_config.workspaces_per_user_quota + return true if quota == 0 + return false if quota == -1 + + workspaces_count_for_current_user_and_agent >= quota + end + + def exceeds_workspaces_quota? + return unless remote_development_agent_config + + quota = remote_development_agent_config.workspaces_quota + return true if quota == 0 + return false if quota == -1 + + workspaces_count_for_current_agent >= quota + end + private def validate_agent_config_presence @@ -101,15 +132,16 @@ def enforce_permanent_termination # rubocop:disable Layout/LineLength -- Long messages for UI def enforce_quotas + agent_config = remote_development_agent_config if exceeds_workspaces_per_user_quota? errors.add :base, format(s_('RemoteDevelopment|You cannot create a workspace because you already have "%{count}" existing workspaces for the given agent with a per user quota of "%{quota}" workspaces'), - count: actual_workspaces_count_for_current_user_and_agent, + count: workspaces_count_for_current_user_and_agent, quota: agent_config.workspaces_per_user_quota) elsif exceeds_workspaces_quota? errors.add :base, format(s_('RemoteDevelopment|You cannot create a workspace because there are already "%{count}" existing workspaces for the given agent with a total quota of "%{quota}" workspaces'), - count: actual_workspaces_count_for_current_agent, + count: workspaces_count_for_current_agent, quota: agent_config.workspaces_quota) end end diff --git a/ee/spec/models/remote_development/workspace_spec.rb b/ee/spec/models/remote_development/workspace_spec.rb index 146dce4e8bf1a9..cf8453a91e9879 100644 --- a/ee/spec/models/remote_development/workspace_spec.rb +++ b/ee/spec/models/remote_development/workspace_spec.rb @@ -157,6 +157,176 @@ end end + describe "#workspaces_count_for_current_user_and_agent" do + let_it_be(:user1) { create(:user) } + let_it_be(:user2) { create(:user) } + let_it_be(:user3) { create(:user) } + let_it_be(:agent1, reload: true) { create(:ee_cluster_agent, :with_remote_development_agent_config) } + let_it_be(:agent2, reload: true) { create(:ee_cluster_agent, :with_remote_development_agent_config) } + let_it_be(:workspace1) do + create(:workspace, user: user1, agent: agent1, desired_state: ::RemoteDevelopment::Workspaces::States::RUNNING) + end + + let_it_be(:workspace2) do + create(:workspace, user: user2, agent: agent1, + desired_state: ::RemoteDevelopment::Workspaces::States::TERMINATED) + end + + let_it_be(:workspace3) do + create(:workspace, user: user1, agent: agent1, desired_state: ::RemoteDevelopment::Workspaces::States::STOPPED) + end + + let_it_be(:workspace4) do + create(:workspace, user: user2, agent: agent2, + desired_state: ::RemoteDevelopment::Workspaces::States::TERMINATED) + end + + let_it_be(:workspace5) do + create(:workspace, user: user3, agent: agent2, desired_state: ::RemoteDevelopment::Workspaces::States::RUNNING) + end + + it "returns the correct count for the current user and agent" do + expect(workspace1.workspaces_count_for_current_user_and_agent).to eq(2) + expect(workspace2.workspaces_count_for_current_user_and_agent).to eq(0) + expect(workspace4.workspaces_count_for_current_user_and_agent).to eq(0) + expect(workspace5.workspaces_count_for_current_user_and_agent).to eq(1) + end + end + + describe "#workspaces_count_for_current_agent" do + let_it_be(:agent1, reload: true) { create(:ee_cluster_agent, :with_remote_development_agent_config) } + let_it_be(:agent2, reload: true) { create(:ee_cluster_agent, :with_remote_development_agent_config) } + let_it_be(:workspace1) do + create(:workspace, agent: agent1, desired_state: ::RemoteDevelopment::Workspaces::States::RUNNING) + end + + let_it_be(:workspace2) do + create(:workspace, agent: agent1, desired_state: ::RemoteDevelopment::Workspaces::States::TERMINATED) + end + + let_it_be(:workspace3) do + create(:workspace, agent: agent1, desired_state: ::RemoteDevelopment::Workspaces::States::STOPPED) + end + + let_it_be(:workspace4) do + create(:workspace, agent: agent2, desired_state: ::RemoteDevelopment::Workspaces::States::TERMINATED) + end + + let_it_be(:workspace5) do + create(:workspace, agent: agent2, desired_state: ::RemoteDevelopment::Workspaces::States::RUNNING) + end + + it "returns the correct count for the current agent" do + expect(workspace1.workspaces_count_for_current_agent).to eq(2) + expect(workspace4.workspaces_count_for_current_agent).to eq(1) + expect(workspace5.workspaces_count_for_current_agent).to eq(1) + end + end + + describe "#exceeds_workspaces_per_user_quota?" do + let(:workspace) { create(:workspace) } + + context "when remote_development_agent_config is nil" do + it "returns false" do + workspace.remote_development_agent_config = nil + expect(workspace.exceeds_workspaces_per_user_quota?).to be nil + end + end + + context "when remote_development_agent_config is present" do + context "when workspaces_per_user_quota is 0" do + before do + allow(workspace).to receive(:remote_development_agent_config).and_return(instance_double( + RemoteDevelopment::RemoteDevelopmentAgentConfig, workspaces_per_user_quota: 0)) + end + + it "returns true" do + expect(workspace.exceeds_workspaces_per_user_quota?).to be true + end + end + + context "when workspaces_per_user_quota is -1" do + before do + allow(workspace).to receive(:remote_development_agent_config).and_return(instance_double( + RemoteDevelopment::RemoteDevelopmentAgentConfig, workspaces_per_user_quota: -1)) + end + + it "returns false" do + expect(workspace.exceeds_workspaces_per_user_quota?).to be false + end + end + + context "when workspaces_per_user_quota is greater than 0" do + before do + allow(workspace).to receive(:remote_development_agent_config).and_return(instance_double( + RemoteDevelopment::RemoteDevelopmentAgentConfig, workspaces_per_user_quota: 2)) + end + + it "returns true if the workspaces count for current user and agent is greater than or equal to the quota" do + allow(workspace).to receive(:workspaces_count_for_current_user_and_agent).and_return(3) + expect(workspace.exceeds_workspaces_per_user_quota?).to be true + end + + it "returns false if the workspaces count for current user and agent is less than the quota" do + allow(workspace).to receive(:workspaces_count_for_current_user_and_agent).and_return(1) + expect(workspace.exceeds_workspaces_per_user_quota?).to be false + end + end + end + end + + describe "#exceeds_workspaces_quota?" do + let(:workspace) { create(:workspace) } + + context "when remote_development_agent_config is nil" do + it "returns false" do + workspace.remote_development_agent_config = nil + expect(workspace.exceeds_workspaces_quota?).to be nil + end + end + + context "when remote_development_agent_config is present" do + context "when workspaces_quota is 0" do + before do + allow(workspace).to receive(:remote_development_agent_config).and_return(instance_double( + RemoteDevelopment::RemoteDevelopmentAgentConfig, workspaces_quota: 0)) + end + + it "returns true" do + expect(workspace.exceeds_workspaces_quota?).to be true + end + end + + context "when workspaces_quota is -1" do + before do + allow(workspace).to receive(:remote_development_agent_config).and_return(instance_double( + RemoteDevelopment::RemoteDevelopmentAgentConfig, workspaces_quota: -1)) + end + + it "returns false" do + expect(workspace.exceeds_workspaces_quota?).to be false + end + end + + context "when workspaces_quota is greater than 0" do + before do + allow(workspace).to receive(:remote_development_agent_config).and_return(instance_double( + RemoteDevelopment::RemoteDevelopmentAgentConfig, workspaces_quota: 2)) + end + + it "returns true if the workspaces count for the current agent is greater than or equal to the quota" do + allow(workspace).to receive(:workspaces_count_for_current_agent).and_return(3) + expect(workspace.exceeds_workspaces_quota?).to be true + end + + it "returns false if the workspaces count for the current agent is less than the quota" do + allow(workspace).to receive(:workspaces_count_for_current_agent).and_return(1) + expect(workspace.exceeds_workspaces_quota?).to be false + end + end + end + end + describe '#enforce_quotas' do subject(:workspace) do build(:workspace, @@ -177,9 +347,9 @@ end it 'adds base error when per user quota exceeded' do - allow(workspace).to receive(:agent_config).and_return(instance_double( + allow(workspace).to receive(:remote_development_agent_config).and_return(instance_double( ::RemoteDevelopment::RemoteDevelopmentAgentConfig, workspaces_per_user_quota: 5)) - allow(workspace).to receive(:actual_workspaces_count_for_current_user_and_agent).and_return(6) + allow(workspace).to receive(:workspaces_count_for_current_user_and_agent).and_return(6) allow(workspace).to receive(:exceeds_workspaces_per_user_quota?).and_return(true) workspace.validate message = "You cannot create a workspace because you already have \"6\" \ @@ -188,9 +358,9 @@ end it 'adds base error when total quota exceeded' do - allow(workspace).to receive(:agent_config).and_return(instance_double( + allow(workspace).to receive(:remote_development_agent_config).and_return(instance_double( ::RemoteDevelopment::RemoteDevelopmentAgentConfig, workspaces_quota: 3)) - allow(workspace).to receive(:actual_workspaces_count_for_current_agent).and_return(3) + allow(workspace).to receive(:workspaces_count_for_current_agent).and_return(3) allow(workspace).to receive(:exceeds_workspaces_quota?).and_return(true) workspace.validate message = "You cannot create a workspace because there are already \"3\" \ -- GitLab