From 7cc11a45b94cdf855da343089b28de0c96341621 Mon Sep 17 00:00:00 2001 From: Dhiren Vispute <86131363+dv-msft@users.noreply.github.com> Date: Wed, 23 Aug 2023 15:13:53 -0700 Subject: [PATCH] enable explicit clean-up prior to process exit (#2782) Co-authored-by: Dhiren Vispute --- .github/workflows/cicd.yml | 17 +++++++++++- tests/end_to_end/helpers.h | 2 +- tests/stress/ebpf_mt_stress.h | 4 +++ tests/stress/ebpf_stress_tests.cpp | 5 ++++ tests/stress/km/stress_tests_km.cpp | 10 ++++++- tests/stress/um/stress_tests_um.cpp | 43 +++++++++++++++++++---------- 6 files changed, 64 insertions(+), 17 deletions(-) diff --git a/.github/workflows/cicd.yml b/.github/workflows/cicd.yml index d20ad21bd..9ebb3621d 100644 --- a/.github/workflows/cicd.yml +++ b/.github/workflows/cicd.yml @@ -367,6 +367,21 @@ jobs: fault_injection: true leak_detection: true + # Run a fast multi-threaded stress test pass against the usersim user-mode 'mock' framework. + # Added as a 'per-PR' test to catch usersim regressions and/or run-time usage issues. + quick_user_mode_multi_threaded_stress_test: + needs: regular + uses: ./.github/workflows/reusable-test.yml + with: + name: quick_user_mode_multi_threaded_stress + test_command: .\ebpf_stress_tests_um -tt=8 -td=2 + build_artifact: Build-x64 + environment: windows-2022 + code_coverage: false + leak_detection: false + gather_dumps: true + capture_etw: true + # Additional jobs to run on a schedule only (skip push and pull request). # --------------------------------------------------------------------------- codeql: @@ -416,7 +431,7 @@ jobs: uses: ./.github/workflows/reusable-test.yml with: name: user_mode_multi_threaded_stress - test_command: .\ebpf_stress_tests_um -tt=32 -td=10 + test_command: .\ebpf_stress_tests_um -tt=8 -td=10 build_artifact: Build-x64 environment: windows-2022 code_coverage: false diff --git a/tests/end_to_end/helpers.h b/tests/end_to_end/helpers.h index ddad9b347..bc83006c9 100644 --- a/tests/end_to_end/helpers.h +++ b/tests/end_to_end/helpers.h @@ -115,7 +115,7 @@ typedef class _single_instance_hook : public _hook_helper _single_instance_hook(ebpf_program_type_t program_type, ebpf_attach_type_t attach_type) : _hook_helper{attach_type}, client_binding_context(nullptr), client_data(nullptr), client_dispatch_table(nullptr), link_object(nullptr), client_registration_instance(nullptr), - nmr_binding_handle(nullptr) + nmr_binding_handle(nullptr), nmr_provider_handle(nullptr) { attach_provider_data.supported_program_type = program_type; attach_provider_data.bpf_attach_type = get_bpf_attach_type(&attach_type); diff --git a/tests/stress/ebpf_mt_stress.h b/tests/stress/ebpf_mt_stress.h index 0a964ee58..27d814835 100644 --- a/tests/stress/ebpf_mt_stress.h +++ b/tests/stress/ebpf_mt_stress.h @@ -104,3 +104,7 @@ get_jit_program_attributes(const std::string& program_name); // The query_supported_program_names() call is 'exported' by both the user and kernel mode test suites. const std::vector query_supported_program_names(); + +// The test_process_cleanup() call is 'exported' by both the user and kernel mode test suites. +void +test_process_cleanup(); diff --git a/tests/stress/ebpf_stress_tests.cpp b/tests/stress/ebpf_stress_tests.cpp index 3e66074f6..e960fc043 100644 --- a/tests/stress/ebpf_stress_tests.cpp +++ b/tests/stress/ebpf_stress_tests.cpp @@ -153,4 +153,9 @@ main(int argc, char* argv[]) } session.run(); + + // Tests that run against the 'usersim' framework need explicit clean-up before process + // termination. This clean-up is handled by the OS and/or the in-kernel eBPF components for + // tests that run against the kernel components. + test_process_cleanup(); } diff --git a/tests/stress/km/stress_tests_km.cpp b/tests/stress/km/stress_tests_km.cpp index 66f88e846..08ec0a5f8 100644 --- a/tests/stress/km/stress_tests_km.cpp +++ b/tests/stress/km/stress_tests_km.cpp @@ -44,6 +44,14 @@ query_supported_program_names() return program_names; } +// This function is called by the common test initialization code to perform the requisite clean-up as the last action +// prior to process termination. +void +test_process_cleanup() +{ + // As of now, we don't need to do anything here for kernel mode tests. +} + // Test thread control parameters (# of threads, run duration etc.). static test_control_info _global_test_control_info{0}; @@ -1545,4 +1553,4 @@ TEST_CASE("bindmonitor_tail_call_invoke_program_test", "[native_mt_stress_test]" _print_test_control_info(local_test_control_info); _mt_bindmonitor_tail_call_invoke_program_test(local_test_control_info); -} \ No newline at end of file +} diff --git a/tests/stress/um/stress_tests_um.cpp b/tests/stress/um/stress_tests_um.cpp index ceab5e1b4..92145c4a8 100644 --- a/tests/stress/um/stress_tests_um.cpp +++ b/tests/stress/um/stress_tests_um.cpp @@ -218,11 +218,12 @@ query_supported_program_names() // These objects should be created just _once_ per (test) process. This is only needed for user mode tests that use the // user mode 'mock' framework. Note that these cannot be created globally and _must_ be created in the context of a // a Catch2 test 'session' (Per Catch2 documentation, Catch2's exception framework is apparently not quite ready until -// then). This is an issue in our usage as we make extensive use of Catch2's REQUIRE verification/validation macros -// (based on this framework) during the creation of these objects; -static _test_helper_end_to_end* _test_helper{nullptr}; -static program_info_provider_t* _bind_program_info_provider{nullptr}; -static program_info_provider_t* _xdp_program_info_provider{nullptr}; +// then). This becomes an issue in our usage as we make extensive use of Catch2's REQUIRE verification/validation +// macros (based on this framework) during the creation of these objects; +static std::unique_ptr<_test_helper_end_to_end> _test_helper; +static std::unique_ptr _bind_program_info_provider; +static std::unique_ptr _xdp_program_info_provider; + static test_control_info _test_control_info{0}; static std::once_flag _um_test_init_done; @@ -230,17 +231,20 @@ static void um_test_init() { std::call_once(_um_test_init_done, [&]() { - _test_helper = new _test_helper_end_to_end; - REQUIRE(_test_helper != nullptr); - _test_helper->initialize(); + _test_helper_end_to_end* local_test_helper = new _test_helper_end_to_end; + REQUIRE(local_test_helper != nullptr); + local_test_helper->initialize(); + _test_helper.reset(local_test_helper); - _bind_program_info_provider = new program_info_provider_t(); - REQUIRE(_bind_program_info_provider->initialize(EBPF_PROGRAM_TYPE_BIND) == EBPF_SUCCESS); - REQUIRE(_bind_program_info_provider != nullptr); + program_info_provider_t* local_bind_program_info_provider = new program_info_provider_t(); + REQUIRE(local_bind_program_info_provider != nullptr); + REQUIRE(local_bind_program_info_provider->initialize(EBPF_PROGRAM_TYPE_BIND) == EBPF_SUCCESS); + _bind_program_info_provider.reset(local_bind_program_info_provider); - _xdp_program_info_provider = new program_info_provider_t(); - REQUIRE(_xdp_program_info_provider->initialize(EBPF_PROGRAM_TYPE_XDP) == EBPF_SUCCESS); - REQUIRE(_xdp_program_info_provider != nullptr); + program_info_provider_t* local_xdp_program_info_provider = new program_info_provider_t(); + REQUIRE(local_xdp_program_info_provider != nullptr); + REQUIRE(local_xdp_program_info_provider->initialize(EBPF_PROGRAM_TYPE_XDP) == EBPF_SUCCESS); + _xdp_program_info_provider.reset(local_xdp_program_info_provider); _test_control_info = get_test_control_info(); if (_test_control_info.programs.size()) { @@ -269,6 +273,17 @@ um_test_init() }); } +// This function is called by the common test initialization code to perform the requisite clean-up as the last action +// prior to process termination. +void +test_process_cleanup() +{ + // We need to explicitly 'free' these resources in tests that run against the user-mode 'usersim' framework. + _xdp_program_info_provider.reset(nullptr); + _bind_program_info_provider.reset(nullptr); + _test_helper.reset(nullptr); +} + TEST_CASE("load_attach_detach_unload_sequential_test", "[mt_stress_test]") { um_test_init();