Skip to content

Commit

Permalink
Merge branch 'main' into fix-asg-error-messages
Browse files Browse the repository at this point in the history
  • Loading branch information
geofffranks committed May 10, 2024
2 parents 4ee90a1 + 9fc7afa commit 89f1dcc
Show file tree
Hide file tree
Showing 14 changed files with 88 additions and 46 deletions.
2 changes: 1 addition & 1 deletion Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ group :test do
gem 'rspec-its'
gem 'rspec-rails', '~> 6.1.2'
gem 'rspec-wait'
gem 'rubocop', '~> 1.63.4'
gem 'rubocop', '~> 1.63.5'
gem 'rubocop-rails', '~> 2.24'
gem 'rubocop-rspec', '~> 2.26'
gem 'rubocop-sequel', '~> 0.3.4'
Expand Down
8 changes: 4 additions & 4 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -465,7 +465,7 @@ GEM
activesupport (>= 3.0.0)
mustache (~> 1.0, >= 0.99.4)
rspec (~> 3.0)
rubocop (1.63.4)
rubocop (1.63.5)
json (~> 2.3)
language_server-protocol (>= 3.17.0)
parallel (~> 1.10)
Expand All @@ -476,8 +476,8 @@ GEM
rubocop-ast (>= 1.31.1, < 2.0)
ruby-progressbar (~> 1.7)
unicode-display_width (>= 2.4.0, < 3.0)
rubocop-ast (1.31.2)
parser (>= 3.3.0.4)
rubocop-ast (1.31.3)
parser (>= 3.3.1.0)
rubocop-capybara (2.20.0)
rubocop (~> 1.41)
rubocop-factory_bot (2.25.1)
Expand Down Expand Up @@ -656,7 +656,7 @@ DEPENDENCIES
rspec-rails (~> 6.1.2)
rspec-wait
rspec_api_documentation (>= 6.1.0)
rubocop (~> 1.63.4)
rubocop (~> 1.63.5)
rubocop-rails (~> 2.24)
rubocop-rspec (~> 2.26)
rubocop-sequel (~> 0.3.4)
Expand Down
6 changes: 6 additions & 0 deletions app/actions/route_destination_update.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@ def update(destination, message)

destination.save
end

destination.processes.each do |process|
ProcessRouteHandler.new(process).notify_backend_of_route_update
end

destination
end

private
Expand Down
7 changes: 6 additions & 1 deletion app/messages/validators/security_group_rule_validator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ class RulesValidator < ActiveModel::Validator
type
].freeze

MAX_DESTINATIONS_PER_RULE = 6000

def validate(record)
unless record.rules.is_a?(Array)
record.errors.add :rules, 'must be an array'
Expand All @@ -28,7 +30,10 @@ def validate(record)
add_rule_error("protocol must be 'tcp', 'udp', 'icmp', or 'all'", record, index) unless valid_protocol(rule[:protocol])

if valid_destination_type(rule[:destination], record, index)
rule[:destination].split(',', -1).each do |d|
rules = rule[:destination].split(',', -1)
add_rule_error("maximum destinations per rule exceeded - must be under #{MAX_DESTINATIONS_PER_RULE}", record, index) unless rules.length <= MAX_DESTINATIONS_PER_RULE

rules.each do |d|
validate_destination(d, record, index)
end
end
Expand Down
18 changes: 6 additions & 12 deletions db/migrations/20240314131908_add_user_guid_to_jobs_table.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,9 @@

up do
if database_type == :postgres
db = self
alter_table :jobs do
add_column :user_guid, String, size: 255, if_not_exists: true
VCAP::Migration.with_concurrent_timeout(db) do
add_index :user_guid, name: :jobs_user_guid_index, if_not_exists: true, concurrently: true
end
add_column :jobs, :user_guid, String, size: 255, if_not_exists: true
VCAP::Migration.with_concurrent_timeout(self) do
add_index :jobs, :user_guid, name: :jobs_user_guid_index, if_not_exists: true, concurrently: true
end

elsif database_type == :mysql
Expand All @@ -24,13 +21,10 @@

down do
if database_type == :postgres
db = self
alter_table :jobs do
VCAP::Migration.with_concurrent_timeout(db) do
drop_index :user_guid, name: :jobs_user_guid_index, if_exists: true, concurrently: true
end
drop_column :user_guid, if_exists: true
VCAP::Migration.with_concurrent_timeout(self) do
drop_index :jobs, :user_guid, name: :jobs_user_guid_index, if_exists: true, concurrently: true
end
drop_column :jobs, :user_guid, if_exists: true
end

if database_type == :mysql
Expand Down
2 changes: 1 addition & 1 deletion docs/v3/Gemfile
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
source 'http://rubygems.org'

gem 'json', '>= 2.3.0'
gem 'middleman', '~> 4.4'
gem 'middleman', '~> 4.5'
gem 'middleman-autoprefixer', '~> 3.0', '>= 3.0.0'
gem 'middleman-gh-pages', '>= 0.0.3'
gem 'middleman-livereload', '>= 3.4.7'
Expand Down
27 changes: 14 additions & 13 deletions docs/v3/Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ GEM
coffee-script-source (1.12.2)
concurrent-ruby (1.2.3)
contracts (0.17)
dotenv (3.1.0)
dotenv (3.1.1)
em-websocket (0.5.3)
eventmachine (>= 0.12.9)
http_parser.rb (~> 0)
Expand All @@ -27,8 +27,9 @@ GEM
fast_blank (1.0.1)
fastimage (2.3.1)
ffi (1.16.3)
haml (5.2.2)
temple (>= 0.8.0)
haml (6.3.0)
temple (>= 0.8.2)
thor
tilt
hamster (3.0.0)
concurrent-ruby (~> 1.0)
Expand All @@ -47,18 +48,18 @@ GEM
rb-fsevent (~> 0.10, >= 0.10.3)
rb-inotify (~> 0.9, >= 0.9.10)
memoist (0.16.2)
middleman (4.4.3)
middleman (4.5.0)
coffee-script (~> 2.2)
haml (>= 4.0.5, < 6.0)
haml (>= 4.0.5)
kramdown (>= 2.3.0)
middleman-cli (= 4.4.3)
middleman-core (= 4.4.3)
middleman-cli (= 4.5.0)
middleman-core (= 4.5.0)
middleman-autoprefixer (3.0.0)
autoprefixer-rails (~> 10.0)
middleman-core (>= 4.0.0)
middleman-cli (4.4.3)
middleman-cli (4.5.0)
thor (>= 0.17.0, < 2.0)
middleman-core (4.4.3)
middleman-core (4.5.0)
activesupport (>= 6.1, < 7.1)
addressable (~> 2.4)
backports (~> 3.6)
Expand Down Expand Up @@ -125,7 +126,7 @@ GEM
rb-inotify (0.10.1)
ffi (~> 1.0)
redcarpet (3.6.0)
rexml (3.2.5)
rexml (3.2.6)
rouge (3.30.0)
sass (3.7.4)
sass-listen (~> 4.0.0)
Expand All @@ -138,8 +139,8 @@ GEM
sprockets (3.7.2)
concurrent-ruby (~> 1.0)
rack (> 1, < 3)
temple (0.9.1)
thor (1.2.1)
temple (0.10.3)
thor (1.3.1)
tilt (2.0.11)
toml (0.3.0)
parslet (>= 1.8.0, < 3.0.0)
Expand All @@ -157,7 +158,7 @@ PLATFORMS

DEPENDENCIES
json (>= 2.3.0)
middleman (~> 4.4)
middleman (~> 4.5)
middleman-autoprefixer (~> 3.0, >= 3.0.0)
middleman-gh-pages (>= 0.0.3)
middleman-livereload (>= 3.4.7)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ curl "https://api.example.org/v3/organization_quotas/[guid]" \
"total_reserved_ports": 4
},
"domains": {
"total_private_domains": 7
"total_domains": 7
}
}'
```
Expand Down
15 changes: 10 additions & 5 deletions lib/cloud_controller/db.rb
Original file line number Diff line number Diff line change
Expand Up @@ -224,15 +224,20 @@ def self.uuid_function(migration)
# Concurrent migrations can take a long time to run, so this helper can be used to override 'max_migration_statement_runtime_in_seconds' for a specific migration.
# REF: https://www.postgresql.org/docs/current/sql-createindex.html#SQL-CREATEINDEX-CONCURRENTLY
def self.with_concurrent_timeout(db, &block)
concurrent_timeout_seconds = VCAP::CloudController::Config.config&.get(:migration_psql_concurrent_statement_timeout_in_seconds) || PSQL_DEFAULT_STATEMENT_TIMEOUT

if concurrent_timeout_seconds && db.database_type == :postgres
concurrent_timeout_in_seconds = VCAP::CloudController::Config.config&.get(:migration_psql_concurrent_statement_timeout_in_seconds)
concurrent_timeout_in_milliseconds = if concurrent_timeout_in_seconds.nil? || concurrent_timeout_in_seconds <= 0
PSQL_DEFAULT_STATEMENT_TIMEOUT
else
concurrent_timeout_in_seconds * 1000
end

if db.database_type == :postgres
original_timeout = db.fetch("select setting from pg_settings where name = 'statement_timeout'").first[:setting]
db.run("SET statement_timeout TO #{concurrent_timeout_seconds * 1000}")
db.run("SET statement_timeout TO #{concurrent_timeout_in_milliseconds}")
end
block.call
ensure
db.run("SET statement_timeout TO #{original_timeout}") if original_timeout && db.database_type == :postgres
db.run("SET statement_timeout TO #{original_timeout}") if original_timeout
end

def self.logger
Expand Down
3 changes: 2 additions & 1 deletion lib/cloud_controller/runners/puma_runner.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ def initialize(config, app, logger, periodic_updater, request_logs)
end

conf.workers(config.get(:puma, :workers) || 1)
conf.threads(0, config.get(:puma, :max_threads) || 1)
num_threads = config.get(:puma, :max_threads) || 1
conf.threads(num_threads, num_threads)

# In theory there shouldn't be any open connections when shutting down Puma as they have either been gracefully
# drained or forcefully terminated (after cc.nginx_drain_timeout) by Nginx. Puma has some built-in (i.e. not
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
skip if db.database_type != :postgres
expect { Sequel::Migrator.run(db, tmp_migrations_dir, allow_missing_migration_files: true) }.not_to raise_error
expect(db).to have_received(:run).exactly(2).times
expect(db).to have_received(:run).with(/SET statement_timeout TO \d+/).twice
expect(db).to have_received(:run).with('SET statement_timeout TO 1899000').once
expect(db).to have_received(:run).with('SET statement_timeout TO 30000').once
end
end
21 changes: 17 additions & 4 deletions spec/unit/actions/route_destination_update_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,20 @@ module VCAP::CloudController
subject(:destination_update) { RouteDestinationUpdate }

describe '#update' do
let!(:destination) { RouteMappingModel.make({ protocol: 'http1' }) }
let(:process) { ProcessModelFactory.make(state: 'STARTED') }
let!(:destination) { RouteMappingModel.make({ protocol: 'http1', app_guid: process.app.guid, process_type: process.type }) }
let(:process_route_handler) { instance_double(ProcessRouteHandler, notify_backend_of_route_update: nil) }

let(:message) do
VCAP::CloudController::RouteDestinationUpdateMessage.new({
protocol: 'http2'
})
VCAP::CloudController::RouteDestinationUpdateMessage.new(
{
protocol: 'http2'
}
)
end

before do
allow(ProcessRouteHandler).to receive(:new).with(process).and_return(process_route_handler)
end

it 'updates the destination record' do
Expand All @@ -20,6 +28,11 @@ module VCAP::CloudController
expect(updated_destination.protocol).to eq 'http2'
end

it 'notifies the backend of route updates' do
RouteDestinationUpdate.update(destination, message)
expect(process_route_handler).to have_received(:notify_backend_of_route_update)
end

context 'when the given protocol is incompatible' do
context 'for tcp route' do
let(:routing_api_client) { double('routing_api_client', router_group:) }
Expand Down
3 changes: 2 additions & 1 deletion spec/unit/lib/cloud_controller/runners/puma_runner_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ module VCAP::CloudController
subject

expect(puma_launcher.config.final_options[:workers]).to eq(num_workers)
expect(puma_launcher.config.final_options[:min_threads]).to eq(0)
expect(puma_launcher.config.final_options[:min_threads]).to eq(max_threads)
expect(puma_launcher.config.final_options[:max_threads]).to eq(max_threads)
end

Expand All @@ -75,6 +75,7 @@ module VCAP::CloudController
subject

expect(puma_launcher.config.final_options[:workers]).to eq(1)
expect(puma_launcher.config.final_options[:min_threads]).to eq(1)
expect(puma_launcher.config.final_options[:max_threads]).to eq(1)
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,23 @@ def self.name
expect(subject.errors.full_messages).to include expected_error
end
end


context 'more than 6000 destinations per rule' do
let(:rules) do
[
{
protocol: 'all',
destination: (['192.168.1.3'] * 7000).join(',')
}
]
end

it 'throws an error' do
expect(subject).not_to be_valid
expect(subject.errors.full_messages).to include 'Rules[0]: maximum destinations per rule exceeded - must be under 6000'
end
end

context 'empty destinations in the front are invalid' do
let(:rules) do
[
Expand Down

0 comments on commit 89f1dcc

Please sign in to comment.