From 6105e5711ae0675ba5b8e72a683b18d277b92f1e Mon Sep 17 00:00:00 2001 From: Martin McGrane Date: Fri, 18 Aug 2023 13:43:37 +1000 Subject: [PATCH] Refactor test pool to be less optional (#75) --- README.md | 9 +- src/RAITest.jl | 2 - src/engines.jl | 99 +++----------------- src/testpool.jl | 233 +++++++++++++++++++++++++++++------------------ src/testrel.jl | 4 +- test/runtests.jl | 1 - 6 files changed, 161 insertions(+), 187 deletions(-) diff --git a/README.md b/README.md index 8698da8..0ccb972 100644 --- a/README.md +++ b/README.md @@ -72,14 +72,7 @@ Run multiple tests concurrently with a custom engine provider. using RAITest using Test -# Always use `my_existing_engine_name` as the engine name. This must already be created as -# RAITest will not create the engine itself -set_engine_name_provider!()->"my_existing_engine_name") - -# Releasing the engine does nothing. Controlling the engine use and cleanup is not handled -# by RAITest -set_engine_name_releaser!(name::String)->return) - +set_engine_creater!((name)->create_default_engine(name, "M")) @testset "My tests" begin for i in 1:10 query = "def output = $i ic { output = $i }" diff --git a/src/RAITest.jl b/src/RAITest.jl index 07ed51b..b4c45ed 100644 --- a/src/RAITest.jl +++ b/src/RAITest.jl @@ -16,8 +16,6 @@ export add_test_engine! export set_context! -export set_engine_name_provider! -export set_engine_name_releaser! export set_engine_creater! include("code-util.jl") diff --git a/src/engines.jl b/src/engines.jl index 14d3650..a64ff27 100644 --- a/src/engines.jl +++ b/src/engines.jl @@ -1,12 +1,3 @@ -mutable struct TestEngineProvision - # Returns the name of a provisioned, currently valid, engine - provider::Function - # Sends a notification that the engine represented by a string name is no longer in use - releaser::Function - # Create an engine. This is expected to be used by the provider as needed. - creater::Function -end - function _wait_till_provisioned(engine_name, max_wait_time_s=600) start_time = time() # This should be a rare event, so a coarse-grained period is acceptable @@ -32,96 +23,34 @@ function _wait_till_provisioned(engine_name, max_wait_time_s=600) end """ - create_default_engine(name::String) + create_default_engine(name::String; size::String) -Create an XS engine with default settings and the provided name. +Create an engine with the default configuration. If the engine already exists, return immediately. If not, create the engine then return once the provisioning process is complete, or failed. """ -function create_default_engine(name::String) - size = "XS" +function create_default_engine(name::String; size::String="XS") max_wait_time_s = 600 try - get_engine(get_context(), name; readtimeout=max_wait_time_s) - # The engine already exists so return it immediately - return name - catch - # Engine does not exist yet so we'll need to create it + create_engine(get_context(), name; size, readtimeout=max_wait_time_s) + catch e + # If the status code is 409 then the engine already exists and we can wait for it + # to be ready + if !(e isa HTTPError) || e.status_code != 409 + rethrow() + end end - # Request engine creation - create_engine(get_context(), name; size=size, readtimeout=max_wait_time_s) - # Wait for engine to be provisioned return _wait_till_provisioned(name, max_wait_time_s) end -# Get test engine. If a name is provided, the corresponding engine will be provided. -get_test_engine()::String = TEST_ENGINE_PROVISION.provider() +# Get test engine. +get_test_engine()::String = get_free_test_engine_name() # Release test engine. Notifies the provider that this engine is no longer in use. -release_test_engine(name::String) = TEST_ENGINE_PROVISION.releaser(name) - -""" - set_engine_name_provider!(provider::Function) - -Set a provider for test engine names. - -The provider is called by each test to select an engine to run the test with. The default -provider selects a name from a pool of available test engines. - -# Examples - -``` -set_engine_name_provider!() -> "MyEngine") -set_engine_name_provider!() -> my_custom_engine_selector()) - -``` -""" -function set_engine_name_provider!(provider::Function) - return TEST_ENGINE_PROVISION.provider = provider -end - -""" - set_engine_name_releaser!(releaser::Function) - -Set a releaser for test engine names. - -The releaser will be called after a test has been run. The default releaser -marks an engine in the test engine pool as available for use by another test. - -# Examples - -``` -set_engine_name_releaser!(::String) -> nothing) -set_engine_name_releaser!(name::String) -> my_custom_engine_releaser(name)) -``` -""" -function set_engine_name_releaser!(releaser::Function) - return TEST_ENGINE_PROVISION.releaser = releaser -end - -""" - set_engine_creater!(creater::Function) - -Set a function used to create engines. - -# Examples - -``` - set_engine_creater!(create_default_engine) -``` -""" -function set_engine_creater!(creater::Function) - return TEST_ENGINE_PROVISION.creater = creater -end - -TEST_ENGINE_POOL = TestEnginePool(Dict{String, Int64}(), 0, get_next_engine_name) +release_test_engine(name::String) = release_pooled_test_engine(name) -TEST_ENGINE_PROVISION = TestEngineProvision( - get_pooled_test_engine, - release_pooled_test_engine, - create_default_engine, -) +TEST_ENGINE_POOL = TestEnginePool() diff --git a/src/testpool.jl b/src/testpool.jl index 1e4ea50..cf21b93 100644 --- a/src/testpool.jl +++ b/src/testpool.jl @@ -5,37 +5,30 @@ function release_pooled_test_engine(name::String) if !haskey(TEST_ENGINE_POOL.engines, name) return end - TEST_ENGINE_POOL.engines[name] = 0 - end -end - -function get_pooled_test_engine(engine_name::Option{String}=nothing) - if isnothing(engine_name) - engine_name = get_free_test_engine_name() - end - - # If the engine already exists, return it - is_valid_engine(engine_name) && return engine_name - - # The engine does not exist yet, so create it - try - return TEST_ENGINE_PROVISION.creater(engine_name) - catch - # Provisioning failed - remove the name from the pool and rethrow - delete!(TEST_ENGINE_POOL.engines, engine_name) - rethrow() + TEST_ENGINE_POOL.engines[name] -= 1 + # Sanity check that the threaded world still makes sense + @assert TEST_ENGINE_POOL.engines[name] >= 0 "Engine $name over-released" end end const TEST_SERVER_LOCK = ReentrantLock() const TEST_SERVER_ACQUISITION_LOCK = ReentrantLock() -mutable struct TestEnginePool - engines::Dict{String, Int64} +Base.@kwdef mutable struct TestEnginePool + engines::Dict{String, Int64} = Dict() # This is used to enable unique, simple, naming of engines # Switching to randomly generated UUIDs would be needed if tests are run independently - next_id::Int64 - generator::Function + next_id::Threads.Atomic{Int} = Threads.Atomic{Int}(0) + # Number of tests per engine. Values > 1 invalidate test timing, and require careful + # attention to engine sizing + concurrency::Int64 = 1 + name_generator::Function = get_next_engine_name + # Create an engine. This is expected to be used by the provider as needed. + creater::Function = create_default_engine +end + +function _get_new_id() + return Threads.atomic_add!(TEST_ENGINE_POOL.next_id, 1) end function get_free_test_engine_name()::String @@ -43,14 +36,16 @@ function get_free_test_engine_name()::String # One lock guards name acquisition, forming a queue # The second lock guards modification of the engine pool @lock TEST_SERVER_ACQUISITION_LOCK while true - if (length(TEST_ENGINE_POOL.engines) < 1) - error("No servers available!") - end + @lock TEST_SERVER_LOCK begin + if (length(TEST_ENGINE_POOL.engines) < 1) + error("No servers available!") + end - @lock TEST_SERVER_LOCK for e in TEST_ENGINE_POOL.engines - if e.second == 0 - TEST_ENGINE_POOL.engines[e.first] = Base.Threads.threadid() - return e.first + for e in TEST_ENGINE_POOL.engines + if e.second < TEST_ENGINE_POOL.concurrency + TEST_ENGINE_POOL.engines[e.first] += 1 + return e.first + end end end # Very naive wait protocol @@ -61,32 +56,28 @@ end """ Test if an engine has been created and can be returned via the API. """ -function is_valid_engine(name::String) +function validate_engine(name::String) try - get_engine(get_context(), name) - # The engine exists and does not immediately return an error - return true - catch - # Engine does not exist - return false + response = get_engine(get_context(), name; readtimeout=30) + if response.state == "PROVISIONED" + return true + end + # The engine exists, but is not provisioned + @warn("$engine was not provisioned. Reported state was: $(response.state)") + catch e + if e isa HTTPError + @warn("$engine was not provisioned. Reported error was: $e") + else + rethrow() + end end + return false end function replace_engine(name::String) - @lock TEST_SERVER_LOCK begin - delete!(TEST_ENGINE_POOL.engines, name) - end - # If the engine could not be deleted, notify and continue - try - delete_engine(get_context(), name, readtimeout=30) - catch - @warn("Could not delete engine: ", name) - name = TEST_ENGINE_POOL.generator(TEST_ENGINE_POOL.next_id) - end - - @lock TEST_SERVER_LOCK begin - TEST_ENGINE_POOL.engines[name] = 0 - end + delete_test_engine!(name) + new_name = TEST_ENGINE_POOL.name_generator(_get_new_id()) + add_test_engine!(new_name) end function list_test_engines() @@ -100,33 +91,50 @@ end """ add_test_engine!(name::String) -Add an engine to the pool of test engines +Add an engine to the pool of test engines. The engine will be provisioned if it is not +already. """ function add_test_engine!(name::String) + # Provision the engine if it does not already exist. + try + TEST_ENGINE_POOL.creater(name) + catch + # Provisioning failed. Attempt to delete the engine + delete_test_engine!(name) + @warn("Could not provision engine $name") + return + end @lock TEST_SERVER_LOCK begin engines = TEST_ENGINE_POOL.engines engines[name] = 0 end - return nothing -end - -function get_next_engine_name(id::Int64) - return "julia-sdk-test-$(id)" + return end """ -Engines are provisioned on first use by default. Calling this method will provision -all engines in the current pool. + delete_test_engine!(name::String) + +Delete an engine and remove it from the pool of test engines. The engine will be deleted +whether or not it is in the pool. """ -function provision_all_test_engines() +function delete_test_engine!(name::String) + # Remove the engine from the list of available engines @lock TEST_SERVER_LOCK begin - Threads.@sync for engine in TEST_ENGINE_POOL.engines - Threads.@async get_pooled_test_engine(engine.first) - end + delete!(TEST_ENGINE_POOL.engines, name) + end + # Request engine deletion + try + delete_engine(get_context(), name; readtimeout=30) + catch e + @warn("Could not delete engine $name: ", e) end end +function get_next_engine_name(id::Int64) + return "julia-sdk-test-$(id)" +end + """ resize_test_engine_pool!(size::Int64, generator::Option{Function}=nothing) @@ -146,43 +154,75 @@ resize_test_engine_pool!(10, id->"RAITest-test-\$id") resize_test_engine_pool!(0) ``` """ -function resize_test_engine_pool!(size::Int64, generator::Option{Function}=nothing) +function resize_test_engine_pool!(size::Int64, name_generator::Option{Function}=nothing) if size < 0 size = 0 end - if !isnothing(generator) - TEST_ENGINE_POOL.generator = generator + if !isnothing(name_generator) + TEST_ENGINE_POOL.name_generator = name_generator end + # Add engines if size > length + _create_and_add_engines!(size) + _validate_engine_pool!() + # Remove engines if size < length + _trim_engine_pool!(size) +end + +# Test all engines and remove if they are unavailable or not successfully provisioned +function _validate_engine_pool!() @lock TEST_SERVER_LOCK begin - engines = TEST_ENGINE_POOL.engines - # Add engines while length < size - while (length(engines) < size) - new_name = TEST_ENGINE_POOL.generator(TEST_ENGINE_POOL.next_id) - if haskey(engines, new_name) - throw(ArgumentError("Engine name already exists")) - end - engines[new_name] = 0 - TEST_ENGINE_POOL.next_id += 1 + for engine in keys(TEST_ENGINE_POOL.engines) + validate_engine(engine) && continue + + # The engine was not provisioned or does not exist. Remove it from the pool + @info("Removing failed engine $engine") + delete_test_engine!(engine) + end + end +end + +function _create_and_add_engines!(size::Int64) + new_engine_count = 0 + @lock TEST_SERVER_LOCK begin + new_engine_count = size - length(TEST_ENGINE_POOL.engines) + if new_engine_count < 0 + return + end + end + + new_names = String[] + # Generate new names + for _ in 1:new_engine_count + push!(new_names, TEST_ENGINE_POOL.name_generator(_get_new_id())) + end + + @debug("Provisioning $new_engine_count engines") + @sync for new_name in new_names + @async try + add_test_engine!(new_name) + catch e + @warn("Could not provision engine $new_name:", e) end + end +end + +# Remove engines if size < length(engine_pool) +function _trim_engine_pool!(size::Int64) + @assert size >= 0 + engines_to_delete = String[] + + @lock TEST_SERVER_LOCK begin # Move the first length - size engines to the list of engines to delete - engines_to_delete = String[] - while length(engines) > size - engine_name, _ = pop!(engines) + while length(TEST_ENGINE_POOL.engines) > size + engine_name, _ = pop!(TEST_ENGINE_POOL.engines) push!(engines_to_delete, engine_name) end - # Asynchronously delete the engines - Threads.@sync for engine in engines_to_delete - @info("Deleting engine", engine) - @async try - delete_engine(get_context(), engine, readtimeout=30) - catch e - # The engine may not exist if it hasn't been used yet - # For other errors, we just report the error and delete what we can - @info(e) - end - end + end + + @sync for engine in engines_to_delete + @async delete_test_engine!(engine) end end @@ -193,3 +233,18 @@ function destroy_test_engines!() resize_test_engine_pool!(0) @info("Destroyed all test engine: ") end + +""" + set_engine_creater!(creater::Function) + +Set a function used to create engines. + +# Examples + +``` + set_engine_creater!(create_default_engine) +``` +""" +function set_engine_creater!(creater::Function) + return TEST_ENGINE_POOL.creater = creater +end diff --git a/src/testrel.jl b/src/testrel.jl index 2560380..5980441 100644 --- a/src/testrel.jl +++ b/src/testrel.jl @@ -165,7 +165,7 @@ function Step(; expected_problems::Vector=[], allow_unexpected::Symbol=:warning, expect_abort::Bool=false, - timeout_sec::Int64=1800, + timeout_sec::Int64=300, readonly::Bool=false, ) return Step( @@ -267,7 +267,7 @@ function test_rel(; expected_problems::Vector=[], allow_unexpected::Symbol=:warning, expect_abort::Bool=false, - timeout_sec::Int64=1800, + timeout_sec::Int64=300, broken::Bool=false, clone_db::Option{String}=nothing, engine::Option{String}=nothing, diff --git a/test/runtests.jl b/test/runtests.jl index 837af62..f1509a9 100644 --- a/test/runtests.jl +++ b/test/runtests.jl @@ -50,7 +50,6 @@ if isnothing(RAITest.get_context()) else try resize_test_engine_pool!(2, (i)->"RAITest-test-$i") - provision_all_test_engines() @testset RAITestSet "Basic Integration" begin include("integration.jl")