From 6d3e2e8fe107281315948a624a2ba760fac171f1 Mon Sep 17 00:00:00 2001 From: Andrew Crump Date: Tue, 19 Jul 2022 02:03:59 +0000 Subject: [PATCH] Apply log rate limit to staging tasks - The log rate limit from the application web process is applied to the staging task - The staging log rate limit can be customized when creating a build, for consistency with memory and disk limits [#182311441](https://www.pivotaltracker.com/story/show/182311441) Co-authored-by: Rebecca Roberts --- app/actions/build_create.rb | 13 +++- app/messages/build_create_message.rb | 3 +- app/presenters/v3/build_presenter.rb | 1 + ...22_add_staging_log_rate_limit_to_builds.rb | 7 ++ ...ating_staging_log_rate_limit_calculator.rb | 28 ++++++++ lib/cloud_controller/diego/staging_details.rb | 2 +- .../diego/task_recipe_builder.rb | 1 + spec/request/apps_spec.rb | 4 ++ spec/request/builds_spec.rb | 9 ++- spec/unit/actions/build_create_spec.rb | 24 ++++++- ..._staging_log_rate_limit_calculator_spec.rb | 68 +++++++++++++++++++ .../diego/task_recipe_builder_spec.rb | 2 + .../messages/build_create_message_spec.rb | 29 ++++++++ .../presenters/v3/build_presenter_spec.rb | 2 + 14 files changed, 188 insertions(+), 5 deletions(-) create mode 100644 db/migrations/20220718211322_add_staging_log_rate_limit_to_builds.rb create mode 100644 lib/cloud_controller/backends/quota_validating_staging_log_rate_limit_calculator.rb create mode 100644 spec/unit/lib/cloud_controller/backends/quota_validating_staging_log_rate_limit_calculator_spec.rb diff --git a/app/actions/build_create.rb b/app/actions/build_create.rb index 379f4643ca9..b8c02e53bb5 100644 --- a/app/actions/build_create.rb +++ b/app/actions/build_create.rb @@ -1,3 +1,4 @@ +require 'cloud_controller/backends/quota_validating_staging_log_rate_limit_calculator' require 'cloud_controller/backends/quota_validating_staging_memory_calculator' require 'cloud_controller/backends/staging_disk_calculator' require 'cloud_controller/backends/staging_environment_builder' @@ -25,12 +26,14 @@ class StagingInProgress < BuildError def initialize(user_audit_info: UserAuditInfo.from_context(SecurityContext), memory_limit_calculator: QuotaValidatingStagingMemoryCalculator.new, disk_limit_calculator: StagingDiskCalculator.new, + log_rate_limit_calculator: QuotaValidatingStagingLogRateLimitCalculator.new, environment_presenter: StagingEnvironmentBuilder.new) @user_audit_info = user_audit_info @memory_limit_calculator = memory_limit_calculator @disk_limit_calculator = disk_limit_calculator - @environment_builder = environment_presenter + @log_rate_limit_calculator = log_rate_limit_calculator + @environment_builder = environment_presenter end def create_and_stage(package:, lifecycle:, metadata: nil, start_after_staging: false) @@ -49,6 +52,7 @@ def create_and_stage(package:, lifecycle:, metadata: nil, start_after_staging: f app: package.app, staging_memory_in_mb: staging_details.staging_memory_in_mb, staging_disk_in_mb: staging_details.staging_disk_in_mb, + staging_log_rate_limit: staging_details.staging_log_rate_limit_bytes_per_second, created_by_user_guid: @user_audit_info.user_guid, created_by_user_name: @user_audit_info.user_name, created_by_user_email: @user_audit_info.user_email @@ -117,6 +121,7 @@ def get_staging_details(package, lifecycle) memory_limit = get_memory_limit(lifecycle.staging_message.staging_memory_in_mb, app, space, org) disk_limit = get_disk_limit(lifecycle.staging_message.staging_disk_in_mb, app) + log_rate_limit = get_log_rate_limit(lifecycle.staging_message.staging_log_rate_limit_bytes_per_second, app, space, org) environment_variables = @environment_builder.build(app, space, lifecycle, @@ -128,6 +133,7 @@ def get_staging_details(package, lifecycle) staging_details.package = package staging_details.staging_memory_in_mb = memory_limit staging_details.staging_disk_in_mb = disk_limit + staging_details.staging_log_rate_limit_bytes_per_second = log_rate_limit staging_details.environment_variables = environment_variables staging_details.lifecycle = lifecycle staging_details.isolation_segment = IsolationSegmentSelector.for_space(space) @@ -151,6 +157,11 @@ def get_memory_limit(requested_limit, app, space, org) raise OrgQuotaExceeded.new e.message end + def get_log_rate_limit(requested_limit, app, space, org) + limit = requested_limit || app.newest_web_process&.log_rate_limit + @log_rate_limit_calculator.get_limit(limit, space, org) + end + def logger @logger ||= Steno.logger('cc.action.build_create') end diff --git a/app/messages/build_create_message.rb b/app/messages/build_create_message.rb index cad48bc7d58..c07f0b6bc72 100644 --- a/app/messages/build_create_message.rb +++ b/app/messages/build_create_message.rb @@ -4,7 +4,7 @@ module VCAP::CloudController class BuildCreateMessage < MetadataBaseMessage - register_allowed_keys [:staging_memory_in_mb, :staging_disk_in_mb, :environment_variables, :lifecycle, :package] + register_allowed_keys [:staging_memory_in_mb, :staging_disk_in_mb, :staging_log_rate_limit_bytes_per_second, :environment_variables, :lifecycle, :package] def self.lifecycle_requested? @lifecycle_requested ||= proc { |a| a.requested?(:lifecycle) } @@ -12,6 +12,7 @@ def self.lifecycle_requested? validates :staging_disk_in_mb, numericality: { only_integer: true, greater_than: 0 }, allow_nil: true validates :staging_memory_in_mb, numericality: { only_integer: true, greater_than: 0 }, allow_nil: true + validates :staging_log_rate_limit_bytes_per_second, numericality: { only_integer: true, greater_than_or_equal_to: -1 }, allow_nil: true validates_with NoAdditionalKeysValidator validates_with LifecycleValidator, if: lifecycle_requested? diff --git a/app/presenters/v3/build_presenter.rb b/app/presenters/v3/build_presenter.rb index 855349f0fe7..4676bf39772 100644 --- a/app/presenters/v3/build_presenter.rb +++ b/app/presenters/v3/build_presenter.rb @@ -22,6 +22,7 @@ def to_hash state: build.state, staging_memory_in_mb: build.staging_memory_in_mb, staging_disk_in_mb: build.staging_disk_in_mb, + staging_log_rate_limit_bytes_per_second: build.staging_log_rate_limit, error: error, lifecycle: { type: build.lifecycle_type, diff --git a/db/migrations/20220718211322_add_staging_log_rate_limit_to_builds.rb b/db/migrations/20220718211322_add_staging_log_rate_limit_to_builds.rb new file mode 100644 index 00000000000..13652db0978 --- /dev/null +++ b/db/migrations/20220718211322_add_staging_log_rate_limit_to_builds.rb @@ -0,0 +1,7 @@ +Sequel.migration do + change do + alter_table :builds do + add_column :staging_log_rate_limit, :Bignum, null: false, default: -1 + end + end +end diff --git a/lib/cloud_controller/backends/quota_validating_staging_log_rate_limit_calculator.rb b/lib/cloud_controller/backends/quota_validating_staging_log_rate_limit_calculator.rb new file mode 100644 index 00000000000..a6ec550bfd2 --- /dev/null +++ b/lib/cloud_controller/backends/quota_validating_staging_log_rate_limit_calculator.rb @@ -0,0 +1,28 @@ +module VCAP::CloudController + class QuotaValidatingStagingLogRateLimitCalculator + class SpaceQuotaExceeded < StandardError; end + class OrgQuotaExceeded < StandardError; end + + def get_limit(requested_limit, space, org) + if requested_limit.nil? + requested_limit = -1 + end + + requested_limit = requested_limit.to_i + + space_quota_exceeded!(requested_limit) unless space.has_remaining_log_rate_limit(requested_limit) + org_quota_exceeded!(requested_limit) unless org.has_remaining_log_rate_limit(requested_limit) + requested_limit + end + + private + + def org_quota_exceeded!(staging_log_rate_limit) + raise OrgQuotaExceeded.new("staging requires #{staging_log_rate_limit} bytes per second") + end + + def space_quota_exceeded!(staging_log_rate_limit) + raise SpaceQuotaExceeded.new("staging requires #{staging_log_rate_limit} bytes per second") + end + end +end diff --git a/lib/cloud_controller/diego/staging_details.rb b/lib/cloud_controller/diego/staging_details.rb index 152fa322980..51018788ac4 100644 --- a/lib/cloud_controller/diego/staging_details.rb +++ b/lib/cloud_controller/diego/staging_details.rb @@ -1,7 +1,7 @@ module VCAP::CloudController module Diego class StagingDetails - attr_accessor :staging_guid, :staging_memory_in_mb, :staging_disk_in_mb, :package, + attr_accessor :staging_guid, :staging_memory_in_mb, :staging_disk_in_mb, :staging_log_rate_limit_bytes_per_second, :package, :environment_variables, :lifecycle, :start_after_staging, :isolation_segment end end diff --git a/lib/cloud_controller/diego/task_recipe_builder.rb b/lib/cloud_controller/diego/task_recipe_builder.rb index c6cd9d5bee5..3a0307bb171 100644 --- a/lib/cloud_controller/diego/task_recipe_builder.rb +++ b/lib/cloud_controller/diego/task_recipe_builder.rb @@ -65,6 +65,7 @@ def build_staging_task(config, staging_details) log_guid: staging_details.package.app_guid, log_source: STAGING_LOG_SOURCE, memory_mb: staging_details.staging_memory_in_mb, + log_rate_limit_bytes_per_second: staging_details.staging_log_rate_limit_bytes_per_second, network: generate_network(staging_details.package, Protocol::ContainerNetworkInfo::STAGING), privileged: config.get(:diego, :use_privileged_containers_for_staging), result_file: STAGING_RESULT_FILE, diff --git a/spec/request/apps_spec.rb b/spec/request/apps_spec.rb index 41e5a08f589..b22a1d3a896 100644 --- a/spec/request/apps_spec.rb +++ b/spec/request/apps_spec.rb @@ -1778,6 +1778,7 @@ app: app_model, staging_memory_in_mb: 123, staging_disk_in_mb: 456, + staging_log_rate_limit: 789, created_by_user_name: 'bob the builder', created_by_user_guid: user.guid, created_by_user_email: 'bob@loblaw.com' @@ -1789,6 +1790,7 @@ app: app_model, staging_memory_in_mb: 123, staging_disk_in_mb: 456, + staging_log_rate_limit: 789, created_at: build.created_at - 1.day, created_by_user_name: 'bob the builder', created_by_user_guid: user.guid, @@ -1874,6 +1876,7 @@ 'error' => nil, 'staging_memory_in_mb' => 123, 'staging_disk_in_mb' => 456, + 'staging_log_rate_limit_bytes_per_second' => 789, 'lifecycle' => { 'type' => 'buildpack', 'data' => { @@ -1902,6 +1905,7 @@ 'error' => nil, 'staging_memory_in_mb' => 123, 'staging_disk_in_mb' => 456, + 'staging_log_rate_limit_bytes_per_second' => 789, 'lifecycle' => { 'type' => 'buildpack', 'data' => { diff --git a/spec/request/builds_spec.rb b/spec/request/builds_spec.rb index 219eebbaf60..34d67ee852b 100644 --- a/spec/request/builds_spec.rb +++ b/spec/request/builds_spec.rb @@ -78,6 +78,7 @@ 'state' => 'STAGING', 'staging_memory_in_mb' => 42, 'staging_disk_in_mb' => 42, + 'staging_log_rate_limit_bytes_per_second' => -1, 'metadata' => { 'labels' => { 'release' => 'stable', 'seriouseats.com/potato' => 'mashed' }, 'annotations' => { 'potato' => 'idaho' } }, 'error' => nil, 'lifecycle' => { @@ -347,6 +348,7 @@ created_by_user_email: 'bob@loblaw.com', staging_memory_in_mb: 123, staging_disk_in_mb: 456, + staging_log_rate_limit: 234 ) end let!(:second_build) do @@ -358,7 +360,8 @@ created_by_user_guid: developer.guid, created_by_user_email: 'bob@loblaw.com', staging_memory_in_mb: 789, - staging_disk_in_mb: 12 + staging_disk_in_mb: 12, + staging_log_rate_limit: 345 ) end let(:package) { VCAP::CloudController::PackageModel.make(app_guid: app_model.guid) } @@ -468,6 +471,7 @@ 'state' => 'STAGED', 'staging_memory_in_mb' => 123, 'staging_disk_in_mb' => 456, + 'staging_log_rate_limit_bytes_per_second' => 234, 'error' => nil, 'lifecycle' => { 'type' => 'buildpack', @@ -496,6 +500,7 @@ 'state' => 'STAGED', 'staging_memory_in_mb' => 789, 'staging_disk_in_mb' => 12, + 'staging_log_rate_limit_bytes_per_second' => 345, 'error' => nil, 'lifecycle' => { 'type' => 'buildpack', @@ -557,6 +562,7 @@ app: app_model, staging_memory_in_mb: 123, staging_disk_in_mb: 456, + staging_log_rate_limit: 789, created_by_user_name: 'bob the builder', created_by_user_guid: developer.guid, created_by_user_email: 'bob@loblaw.com' @@ -593,6 +599,7 @@ 'state' => 'STAGED', 'staging_memory_in_mb' => 123, 'staging_disk_in_mb' => 456, + 'staging_log_rate_limit_bytes_per_second' => 789, 'error' => nil, 'lifecycle' => { 'type' => 'buildpack', diff --git a/spec/unit/actions/build_create_spec.rb b/spec/unit/actions/build_create_spec.rb index 5173244a3a2..7b60d748683 100644 --- a/spec/unit/actions/build_create_spec.rb +++ b/spec/unit/actions/build_create_spec.rb @@ -10,12 +10,14 @@ module VCAP::CloudController user_audit_info: user_audit_info, memory_limit_calculator: memory_limit_calculator, disk_limit_calculator: disk_limit_calculator, + log_rate_limit_calculator: log_rate_limit_calculator, environment_presenter: environment_builder ) end let(:memory_limit_calculator) { double(:memory_limit_calculator) } let(:disk_limit_calculator) { double(:disk_limit_calculator) } + let(:log_rate_limit_calculator) { double(:log_rate_limit_calculator) } let(:environment_builder) { double(:environment_builder) } let(:user_audit_info) { UserAuditInfo.new(user_email: 'charles@las.gym', user_guid: '1234', user_name: 'charles') } @@ -24,6 +26,7 @@ module VCAP::CloudController { staging_memory_in_mb: staging_memory_in_mb, staging_disk_in_mb: staging_disk_in_mb, + staging_log_rate_limit_bytes_per_second: staging_log_rate_limit_bytes_per_second, lifecycle: request_lifecycle, }.deep_stringify_keys end @@ -50,7 +53,7 @@ module VCAP::CloudController let(:space) { Space.make } let(:org) { space.organization } let(:app) { AppModel.make(space: space) } - let!(:process) { ProcessModel.make(app: app, memory: 8192, disk_quota: 512) } + let!(:process) { ProcessModel.make(app: app, memory: 8192, disk_quota: 512, log_rate_limit: 7168) } let(:buildpack_git_url) { 'http://example.com/repo.git' } let(:stack) { Stack.default } @@ -65,9 +68,11 @@ module VCAP::CloudController let(:stager) { instance_double(Diego::Stager) } let(:calculated_mem_limit) { 32 } let(:calculated_staging_disk_in_mb) { 64 } + let(:calculated_staging_log_rate_limit) { 96 } let(:staging_memory_in_mb) { 1024 } let(:staging_disk_in_mb) { 2048 } + let(:staging_log_rate_limit_bytes_per_second) { 3072 } let(:environment_variables) { 'random string' } before do @@ -76,6 +81,7 @@ module VCAP::CloudController allow(stager).to receive(:stage) allow(memory_limit_calculator).to receive(:get_limit).with(staging_memory_in_mb, space, org).and_return(calculated_mem_limit) allow(disk_limit_calculator).to receive(:get_limit).with(staging_disk_in_mb).and_return(calculated_staging_disk_in_mb) + allow(log_rate_limit_calculator).to receive(:get_limit).with(staging_log_rate_limit_bytes_per_second, space, org).and_return(calculated_staging_log_rate_limit) allow(environment_builder).to receive(:build).and_return(environment_variables) end @@ -93,6 +99,7 @@ module VCAP::CloudController expect(build.package_guid).to eq(package.guid) expect(build.staging_memory_in_mb).to eq(calculated_mem_limit) expect(build.staging_disk_in_mb).to eq(calculated_staging_disk_in_mb) + expect(build.staging_log_rate_limit).to eq(calculated_staging_log_rate_limit) expect(build.lifecycle_data.to_hash).to eq(lifecycle_data) expect(build.created_by_user_guid).to eq('1234') expect(build.created_by_user_name).to eq('charles') @@ -174,6 +181,7 @@ module VCAP::CloudController expect(staging_details.staging_guid).to eq(build.guid) expect(staging_details.staging_memory_in_mb).to eq(calculated_mem_limit) expect(staging_details.staging_disk_in_mb).to eq(calculated_staging_disk_in_mb) + expect(staging_details.staging_log_rate_limit_bytes_per_second).to eq(calculated_staging_log_rate_limit) expect(staging_details.environment_variables).to eq(environment_variables) expect(staging_details.lifecycle).to eq(lifecycle) expect(staging_details.isolation_segment).to be_nil @@ -210,6 +218,20 @@ module VCAP::CloudController end end + context 'when staging log rate limit is not specified in the message' do + before do + allow(log_rate_limit_calculator).to receive(:get_limit).with(process.log_rate_limit, space, org).and_return(process.log_rate_limit) + end + let(:staging_log_rate_limit_bytes_per_second) { nil } + + it 'uses the app web process log rate limit for staging log rate limit' do + expect(log_rate_limit_calculator).to receive(:get_limit).with(process.log_rate_limit, space, org) + + build = action.create_and_stage(package: package, lifecycle: lifecycle) + expect(build.staging_log_rate_limit).to eq(process.log_rate_limit) + end + end + describe 'isolation segments' do let(:assigner) { VCAP::CloudController::IsolationSegmentAssign.new } let(:isolation_segment_model) { VCAP::CloudController::IsolationSegmentModel.make } diff --git a/spec/unit/lib/cloud_controller/backends/quota_validating_staging_log_rate_limit_calculator_spec.rb b/spec/unit/lib/cloud_controller/backends/quota_validating_staging_log_rate_limit_calculator_spec.rb new file mode 100644 index 00000000000..c24e9af1c49 --- /dev/null +++ b/spec/unit/lib/cloud_controller/backends/quota_validating_staging_log_rate_limit_calculator_spec.rb @@ -0,0 +1,68 @@ +require 'spec_helper' +require 'cloud_controller/backends/quota_validating_staging_log_rate_limit_calculator' + +module VCAP::CloudController + RSpec.describe QuotaValidatingStagingLogRateLimitCalculator do + let(:calculator) { QuotaValidatingStagingLogRateLimitCalculator.new } + + describe '#get_limit' do + let(:space_quota_limit) { 200 } + let(:org_quota_limit) { 200 } + let(:requested_limit) { 100 } + let(:space) { Space.make } + let(:org) { space.organization } + let(:space_quota_definition) { SpaceQuotaDefinition.make(organization: org, log_rate_limit: space_quota_limit) } + let(:quota_definition) { QuotaDefinition.make(log_rate_limit: org_quota_limit) } + + before do + space.space_quota_definition = space_quota_definition + org.quota_definition = quota_definition + space.save + org.save + end + + it 'uses the requested_limit' do + limit = calculator.get_limit(requested_limit, space, org) + expect(limit).to eq(requested_limit) + end + + context 'when the requested_limit is passed as an integer string' do + let(:requested_limit) { '100' } + + it 'uses the requested_limit' do + limit = calculator.get_limit(requested_limit, space, org) + expect(limit).to eq(100) + end + end + + context 'when the requested_limit exceeds the space quota' do + let(:space_quota_limit) { requested_limit - 1 } + + it 'raises a SpaceQuotaExceeded error' do + expect { + calculator.get_limit(requested_limit, space, org) + }.to raise_error(QuotaValidatingStagingLogRateLimitCalculator::SpaceQuotaExceeded, /staging requires 100 bytes per second/) + end + end + + context 'when the requested_limit exceeds the org quota' do + let(:org_quota_limit) { requested_limit - 1 } + + it 'raises a OrgQuotaExceeded error' do + expect { + calculator.get_limit(requested_limit, space, org) + }.to raise_error(QuotaValidatingStagingLogRateLimitCalculator::OrgQuotaExceeded, /staging requires 100 bytes per second/) + end + end + + context 'when the requested_limit is nil' do + let(:requested_limit) { nil } + + it 'uses no limit' do + limit = calculator.get_limit(requested_limit, space, org) + expect(limit).to eq(-1) + end + end + end + end +end diff --git a/spec/unit/lib/cloud_controller/diego/task_recipe_builder_spec.rb b/spec/unit/lib/cloud_controller/diego/task_recipe_builder_spec.rb index 7fdda5cbd51..952a53825ed 100644 --- a/spec/unit/lib/cloud_controller/diego/task_recipe_builder_spec.rb +++ b/spec/unit/lib/cloud_controller/diego/task_recipe_builder_spec.rb @@ -14,6 +14,7 @@ module Diego details.environment_variables = [::Diego::Bbs::Models::EnvironmentVariable.new(name: 'nightshade_fruit', value: 'potato')] details.staging_memory_in_mb = 42 details.staging_disk_in_mb = 51 + details.staging_log_rate_limit_bytes_per_second = 67 details.start_after_staging = true details.lifecycle = lifecycle details.isolation_segment = isolation_segment @@ -152,6 +153,7 @@ module Diego expect(result.memory_mb).to eq(42) expect(result.disk_mb).to eq(51) + expect(result.log_rate_limit_bytes_per_second).to eq(67) expect(result.image_layers).to eq(lifecycle_image_layers) expect(result.cpu_weight).to eq(50) diff --git a/spec/unit/messages/build_create_message_spec.rb b/spec/unit/messages/build_create_message_spec.rb index e3e46df6210..a9d6b1cc46b 100644 --- a/spec/unit/messages/build_create_message_spec.rb +++ b/spec/unit/messages/build_create_message_spec.rb @@ -211,6 +211,35 @@ module VCAP::CloudController end end + describe '#staging_log_rate_limit_bytes_per_second' do + subject(:build_create_message) { BuildCreateMessage.new(params) } + let(:params) { { staging_log_rate_limit_bytes_per_second: -1 } } + + it 'returns the staging_log_rate_limit_bytes_per_second' do + expect(build_create_message.staging_log_rate_limit_bytes_per_second).to eq(-1) + end + + context 'when not provided' do + let(:params) { nil } + + it 'returns nil' do + expect(build_create_message.staging_log_rate_limit_bytes_per_second).to eq(nil) + end + end + + context 'when the value is out of bounds' do + let(:params) do + { + package: { guid: 'some-guid' }, + staging_log_rate_limit_bytes_per_second: -2 + } + end + it 'is invalid' do + expect(build_create_message.valid?).to be false + end + end + end + describe '#environment variables' do subject(:build_create_message) { BuildCreateMessage.new(params) } diff --git a/spec/unit/presenters/v3/build_presenter_spec.rb b/spec/unit/presenters/v3/build_presenter_spec.rb index 11f21d900cd..3c66ef5e2d7 100644 --- a/spec/unit/presenters/v3/build_presenter_spec.rb +++ b/spec/unit/presenters/v3/build_presenter_spec.rb @@ -15,6 +15,7 @@ module VCAP::CloudController::Presenters::V3 app: app, staging_memory_in_mb: 1024, staging_disk_in_mb: 1024, + staging_log_rate_limit: 2048, created_by_user_guid: 'happy user guid', created_by_user_name: 'happier user name', created_by_user_email: 'this user emailed in' @@ -46,6 +47,7 @@ module VCAP::CloudController::Presenters::V3 expect(result[:staging_memory_in_mb]).to eq(1024) expect(result[:staging_disk_in_mb]).to eq(1024) + expect(result[:staging_log_rate_limit_bytes_per_second]).to eq(2048) expect(result[:created_at]).to be_a(Time) expect(result[:updated_at]).to be_a(Time)