From ed4d72d102d857e3400a62a86d41d5701dbb7b9d Mon Sep 17 00:00:00 2001 From: Andrew Wong Date: Wed, 30 Aug 2023 18:03:08 -0700 Subject: [PATCH 1/2] test_utils: add scoped config resetter Several tests set configs directly with config::shard_local_config without cleaning them up after. This means that the next test that gets run in the same process may be affected by whatever a previous test had set. This introduces a new scoped wrapper around config::shard_local_config to track what properties may have been updated, so it may reset them upon call to destructor. --- src/v/config/tests/CMakeLists.txt | 1 + src/v/config/tests/scoped_config_test.cc | 40 ++++++++++++++++++++++++ src/v/test_utils/scoped_config.h | 34 ++++++++++++++++++++ 3 files changed, 75 insertions(+) create mode 100644 src/v/config/tests/scoped_config_test.cc create mode 100644 src/v/test_utils/scoped_config.h diff --git a/src/v/config/tests/CMakeLists.txt b/src/v/config/tests/CMakeLists.txt index e40a5c77efca6..7155be68b0e47 100644 --- a/src/v/config/tests/CMakeLists.txt +++ b/src/v/config/tests/CMakeLists.txt @@ -5,6 +5,7 @@ set(srcs config_store_test.cc socket_address_convert_test.cc tls_config_convert_test.cc + scoped_config_test.cc advertised_kafka_api_test.cc seed_server_property_test.cc cloud_credentials_source_test.cc diff --git a/src/v/config/tests/scoped_config_test.cc b/src/v/config/tests/scoped_config_test.cc new file mode 100644 index 0000000000000..d72d3d0f9e3ad --- /dev/null +++ b/src/v/config/tests/scoped_config_test.cc @@ -0,0 +1,40 @@ +// Copyright 2023 Redpanda Data, Inc. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.md +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0 + +#include "config/configuration.h" +#include "test_utils/scoped_config.h" + +#include + +// Test that when multiple test cases update a config using the +// scoped_config, it is reset in between each one. + +SEASTAR_THREAD_TEST_CASE(test_scoped_config_a) { + BOOST_REQUIRE_MESSAGE( + !config::shard_local_cfg().cluster_id().has_value(), + config::shard_local_cfg().cluster_id()->c_str()); + scoped_config cfg; + cfg.get("cluster_id").set_value(std::make_optional("a")); +} + +SEASTAR_THREAD_TEST_CASE(test_scoped_config_b) { + BOOST_REQUIRE_MESSAGE( + !config::shard_local_cfg().cluster_id().has_value(), + config::shard_local_cfg().cluster_id()->c_str()); + scoped_config cfg; + cfg.get("cluster_id").set_value(std::make_optional("b")); +} + +SEASTAR_THREAD_TEST_CASE(test_scoped_config_c) { + BOOST_REQUIRE_MESSAGE( + !config::shard_local_cfg().cluster_id().has_value(), + config::shard_local_cfg().cluster_id()->c_str()); + scoped_config cfg; + cfg.get("cluster_id").set_value(std::make_optional("c")); +} diff --git a/src/v/test_utils/scoped_config.h b/src/v/test_utils/scoped_config.h new file mode 100644 index 0000000000000..410f2ab0bc7e7 --- /dev/null +++ b/src/v/test_utils/scoped_config.h @@ -0,0 +1,34 @@ +/* + * Copyright 2023 Redpanda Data, Inc. + * + * Use of this software is governed by the Business Source License + * included in the file licenses/BSL.md + * + * As of the Change Date specified in that file, in accordance with + * the Business Source License, use of this software will be governed + * by the Apache License, Version 2.0 + */ + +#pragma once + +#include "config/base_property.h" +#include "config/configuration.h" + +// Scoped wrapper around config::shard_local_cfg() that tracks get() calls and, +// upon destructing, resets any properties that were potentially mutated. +class scoped_config { +public: + ~scoped_config() { + for (auto& p : _properties_to_reset) { + config::shard_local_cfg().get(p).reset(); + } + } + + config::base_property& get(std::string_view name) { + _properties_to_reset.emplace_back(name); + return config::shard_local_cfg().get(name); + } + +private: + std::list _properties_to_reset; +}; From d15a801e4315c40a4ef51eefc05ce386a04503c0 Mon Sep 17 00:00:00 2001 From: Andrew Wong Date: Wed, 30 Aug 2023 18:21:06 -0700 Subject: [PATCH 2/2] tests: update some storage fixture tests to reset configs Uses the new scoped_config to reset updated configs to their defaults at the end of each test. This ensures no side effects across tests. --- .../tests/cloud_storage_e2e_test.cc | 53 ++++++++++--------- .../tests/delete_records_e2e_test.cc | 28 +++++----- src/v/cloud_storage/tests/manual_fixture.h | 10 ++-- .../cloud_storage/tests/read_replica_test.cc | 20 +++---- .../storage/tests/storage_e2e_fixture_test.cc | 9 ++-- 5 files changed, 64 insertions(+), 56 deletions(-) diff --git a/src/v/cloud_storage/tests/cloud_storage_e2e_test.cc b/src/v/cloud_storage/tests/cloud_storage_e2e_test.cc index 441e38b7c15ed..e19ada2f2210a 100644 --- a/src/v/cloud_storage/tests/cloud_storage_e2e_test.cc +++ b/src/v/cloud_storage/tests/cloud_storage_e2e_test.cc @@ -19,6 +19,7 @@ #include "kafka/server/tests/produce_consume_utils.h" #include "model/fundamental.h" #include "redpanda/tests/fixture.h" +#include "test_utils/scoped_config.h" #include @@ -45,11 +46,13 @@ class e2e_fixture set_expectations_and_listen({}); wait_for_controller_leadership().get(); } + + scoped_config test_local_cfg; }; FIXTURE_TEST(test_produce_consume_from_cloud, e2e_fixture) { - config::shard_local_cfg() - .cloud_storage_disable_upload_loop_for_tests.set_value(true); + test_local_cfg.get("cloud_storage_disable_upload_loop_for_tests") + .set_value(true); const model::topic topic_name("tapioca"); model::ntp ntp(model::kafka_namespace, topic_name, 0); cluster::topic_properties props; @@ -105,16 +108,15 @@ FIXTURE_TEST(test_produce_consume_from_cloud, e2e_fixture) { FIXTURE_TEST(test_produce_consume_from_cloud_with_spillover, e2e_fixture) { #ifndef _NDEBUG - config::shard_local_cfg() - .cloud_storage_disable_upload_loop_for_tests.set_value(true); - config::shard_local_cfg().cloud_storage_spillover_manifest_size.set_value( - std::make_optional((size_t)0x1000)); + test_local_cfg.get("cloud_storage_disable_upload_loop_for_tests") + .set_value(true); + test_local_cfg.get("cloud_storage_spillover_manifest_size") + .set_value(std::make_optional((size_t)0x1000)); - config::shard_local_cfg().cloud_storage_enable_segment_merging.set_value( - false); + test_local_cfg.get("cloud_storage_enable_segment_merging").set_value(false); - config::shard_local_cfg().enable_metrics_reporter.set_value(false); - config::shard_local_cfg().retention_local_strict.set_value(true); + test_local_cfg.get("enable_metrics_reporter").set_value(false); + test_local_cfg.get("retention_local_strict").set_value(true); const model::topic topic_name("tapioca"); model::ntp ntp(model::kafka_namespace, topic_name, 0); @@ -349,22 +351,20 @@ class cloud_storage_manual_e2e_test wait_for_controller_leadership().get(); // Apply local retention frequently. - config::shard_local_cfg().log_compaction_interval_ms.set_value( - std::chrono::duration_cast(1s)); + test_local_cfg.get("log_compaction_interval_ms") + .set_value(std::chrono::duration_cast(1s)); // We'll control uploads ourselves. - config::shard_local_cfg() - .cloud_storage_enable_segment_merging.set_value(false); - config::shard_local_cfg() - .cloud_storage_disable_upload_loop_for_tests.set_value(true); + test_local_cfg.get("cloud_storage_enable_segment_merging") + .set_value(false); + test_local_cfg.get("cloud_storage_disable_upload_loop_for_tests") + .set_value(true); // Disable metrics to speed things up. - config::shard_local_cfg().enable_metrics_reporter.set_value(false); + test_local_cfg.get("enable_metrics_reporter").set_value(false); // Encourage spilling over. - config::shard_local_cfg() - .cloud_storage_spillover_manifest_max_segments.set_value( - std::make_optional(segs_per_spill)); - config::shard_local_cfg() - .cloud_storage_spillover_manifest_size.set_value( - std::optional{}); + test_local_cfg.get("cloud_storage_spillover_manifest_max_segments") + .set_value(std::make_optional(segs_per_spill)); + test_local_cfg.get("cloud_storage_spillover_manifest_size") + .set_value(std::optional{}); topic_name = model::topic("tapioca"); ntp = model::ntp(model::kafka_namespace, topic_name, 0); @@ -382,6 +382,7 @@ class cloud_storage_manual_e2e_test archiver = &partition->archiver()->get(); } + scoped_config test_local_cfg; model::topic topic_name; model::ntp ntp; cluster::partition* partition; @@ -422,7 +423,7 @@ ss::future check_consume_from_beginning( } // namespace FIXTURE_TEST(test_consume_during_spillover, cloud_storage_manual_e2e_test) { - config::shard_local_cfg().fetch_max_bytes.set_value(size_t{10}); + test_local_cfg.get("fetch_max_bytes").set_value(size_t{10}); const auto records_per_seg = 5; const auto num_segs = 40; tests::remote_segment_generator gen(make_kafka_client().get(), *partition); @@ -470,8 +471,8 @@ FIXTURE_TEST(test_consume_during_spillover, cloud_storage_manual_e2e_test) { FIXTURE_TEST( reclaimable_reported_in_health_report, cloud_storage_manual_multinode_test_base) { - config::shard_local_cfg().retention_local_trim_interval.set_value( - std::chrono::milliseconds(2000)); + test_local_cfg.get("retention_local_trim_interval") + .set_value(std::chrono::milliseconds(2000)); // start a second fixutre and wait for stable setup auto fx2 = start_second_fixture(); diff --git a/src/v/cloud_storage/tests/delete_records_e2e_test.cc b/src/v/cloud_storage/tests/delete_records_e2e_test.cc index 9ef18bda344fd..294e681400c0c 100644 --- a/src/v/cloud_storage/tests/delete_records_e2e_test.cc +++ b/src/v/cloud_storage/tests/delete_records_e2e_test.cc @@ -22,6 +22,7 @@ #include "redpanda/tests/fixture.h" #include "storage/disk_log_impl.h" #include "test_utils/async.h" +#include "test_utils/scoped_config.h" #include @@ -84,23 +85,21 @@ class delete_records_e2e_fixture wait_for_controller_leadership().get(); // Apply local retention frequently. - config::shard_local_cfg().log_compaction_interval_ms.set_value( - std::chrono::duration_cast(1s)); + test_local_cfg.get("log_compaction_interval_ms") + .set_value(std::chrono::duration_cast(1s)); // We'll control uploads ourselves. - config::shard_local_cfg() - .cloud_storage_enable_segment_merging.set_value(false); - config::shard_local_cfg() - .cloud_storage_disable_upload_loop_for_tests.set_value(true); + test_local_cfg.get("cloud_storage_enable_segment_merging") + .set_value(false); + test_local_cfg.get("cloud_storage_disable_upload_loop_for_tests") + .set_value(true); // Disable metrics to speed things up. - config::shard_local_cfg().enable_metrics_reporter.set_value(false); + test_local_cfg.get("enable_metrics_reporter").set_value(false); // Encourage spilling over. - config::shard_local_cfg() - .cloud_storage_spillover_manifest_max_segments.set_value( - std::make_optional(segs_per_spill)); - config::shard_local_cfg() - .cloud_storage_spillover_manifest_size.set_value( - std::optional{}); - config::shard_local_cfg().retention_local_strict.set_value(true); + test_local_cfg.get("cloud_storage_spillover_manifest_max_segments") + .set_value(std::make_optional(segs_per_spill)); + test_local_cfg.get("cloud_storage_spillover_manifest_size") + .set_value(std::optional{}); + test_local_cfg.get("retention_local_strict").set_value(true); topic_name = model::topic("tapioca"); ntp = model::ntp(model::kafka_namespace, topic_name, 0); @@ -131,6 +130,7 @@ class delete_records_e2e_fixture new_archiver.manifest().get_start_kafka_offset_override(), model::offset{}); } + scoped_config test_local_cfg; model::topic topic_name; model::ntp ntp; diff --git a/src/v/cloud_storage/tests/manual_fixture.h b/src/v/cloud_storage/tests/manual_fixture.h index da6cb14db5537..a41bf307b8056 100644 --- a/src/v/cloud_storage/tests/manual_fixture.h +++ b/src/v/cloud_storage/tests/manual_fixture.h @@ -16,6 +16,7 @@ #include "model/fundamental.h" #include "redpanda/tests/fixture.h" #include "storage/disk_log_impl.h" +#include "test_utils/scoped_config.h" class cloud_storage_manual_multinode_test_base : public s3_imposter_fixture @@ -29,10 +30,10 @@ class cloud_storage_manual_multinode_test_base // No expectations: tests will PUT and GET organically. set_expectations_and_listen({}); - config::shard_local_cfg() - .cloud_storage_enable_segment_merging.set_value(false); - config::shard_local_cfg() - .cloud_storage_disable_upload_loop_for_tests.set_value(true); + test_local_cfg.get("cloud_storage_enable_segment_merging") + .set_value(false); + test_local_cfg.get("cloud_storage_disable_upload_loop_for_tests") + .set_value(true); wait_for_controller_leadership().get(); } @@ -53,4 +54,5 @@ class cloud_storage_manual_multinode_test_base get_archival_config(), get_cloud_config(httpd_port_number())); } + scoped_config test_local_cfg; }; diff --git a/src/v/cloud_storage/tests/read_replica_test.cc b/src/v/cloud_storage/tests/read_replica_test.cc index 416ff52eec008..498f2808069c2 100644 --- a/src/v/cloud_storage/tests/read_replica_test.cc +++ b/src/v/cloud_storage/tests/read_replica_test.cc @@ -21,6 +21,7 @@ #include "model/fundamental.h" #include "redpanda/tests/fixture.h" #include "storage/disk_log_impl.h" +#include "test_utils/scoped_config.h" using tests::kafka_consume_transport; @@ -38,17 +39,17 @@ class read_replica_e2e_fixture wait_for_controller_leadership().get(); // Disable metrics to speed things up. - config::shard_local_cfg().enable_metrics_reporter.set_value(false); - config::shard_local_cfg().disable_metrics.set_value(true); - config::shard_local_cfg().disable_public_metrics.set_value(true); + test_local_cfg.get("enable_metrics_reporter").set_value(false); + test_local_cfg.get("disable_metrics").set_value(true); + test_local_cfg.get("disable_public_metrics").set_value(true); // Avoid background work since we'll control uploads ourselves. - config::shard_local_cfg() - .cloud_storage_enable_segment_merging.set_value(false); - config::shard_local_cfg() - .cloud_storage_disable_upload_loop_for_tests.set_value(true); - config::shard_local_cfg() - .cloud_storage_disable_read_replica_loop_for_tests.set_value(true); + test_local_cfg.get("cloud_storage_enable_segment_merging") + .set_value(false); + test_local_cfg.get("cloud_storage_disable_upload_loop_for_tests") + .set_value(true); + test_local_cfg.get("cloud_storage_disable_read_replica_loop_for_tests") + .set_value(true); } std::unique_ptr start_read_replica_fixture() { @@ -66,6 +67,7 @@ class read_replica_e2e_fixture get_archival_config(), get_cloud_config(httpd_port_number())); } + scoped_config test_local_cfg; }; FIXTURE_TEST(test_read_replica_basic_sync, read_replica_e2e_fixture) { diff --git a/src/v/storage/tests/storage_e2e_fixture_test.cc b/src/v/storage/tests/storage_e2e_fixture_test.cc index 3b5d7f3a203e7..abe6761a74d4b 100644 --- a/src/v/storage/tests/storage_e2e_fixture_test.cc +++ b/src/v/storage/tests/storage_e2e_fixture_test.cc @@ -10,6 +10,7 @@ #include "kafka/server/tests/produce_consume_utils.h" #include "redpanda/tests/fixture.h" #include "test_utils/fixture.h" +#include "test_utils/scoped_config.h" #include @@ -20,7 +21,9 @@ using namespace std::chrono_literals; using std::vector; -struct storage_e2e_fixture : public redpanda_thread_fixture {}; +struct storage_e2e_fixture : public redpanda_thread_fixture { + scoped_config test_local_cfg; +}; namespace { @@ -42,8 +45,8 @@ ss::future<> produce_to_fixture( } // namespace FIXTURE_TEST(test_compaction_segment_ms, storage_e2e_fixture) { - config::shard_local_cfg().log_segment_ms_min.set_value( - std::chrono::duration_cast(1ms)); + test_local_cfg.get("log_segment_ms_min") + .set_value(std::chrono::duration_cast(1ms)); const auto topic_name = model::topic("tapioca"); const auto ntp = model::ntp(model::kafka_namespace, topic_name, 0);