test_utils: add scoped config resetter #13146
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/ci-repeat |
andijcr
reviewed
Aug 31, 2023
|
very cool i ran a search for tests that uses shard_local_cfg I wonder if there is an alternative to change all of them to the |
andijcr
previously approved these changes
Aug 31, 2023
|
for example [[nodiscard]] auto make_local_config_resetter() {
return ss::defer([] {
config::shard_local_cfg().for_each(
[](config::base_property& p) { p.reset(); });
});
} |
Lazin
reviewed
Aug 31, 2023
| public: | ||
| ~scoped_config() { | ||
| for (auto& p : _properties_to_reset) { | ||
| config::shard_local_cfg().get(p).reset(); |
There was a problem hiding this comment.
I think it has to do this on all shards. Something like:
ss::smp::invoke_on_all([&] {
config::shard_local_cfg().get(p).reset();
}).ge();
and same is for setting the value. We have tests which are using more than one shard and setting config parameters (without resetting). For instance throughput_limits_fixture in the produce_consume_test.cc
There was a problem hiding this comment.
Actually the setting part of this seems a bit more involved. I'm not sure it's the right thing to do to always do it on every core. I'm leaning towards forcing test authors to manage this per core with a sharded service or somesuch.
andrwng
force-pushed
the
scoped_config
branch
2 times, most recently
from
1c4224a to
c07fd44
Compare
Yea I'd considered something like this, but opted to track specific configs given the full list might be quite expensive if we start cleaning up every config ubiquitously |
dotnwat
reviewed
Aug 31, 2023
src/v/test_utils/scoped_config.h
Outdated
| ~scoped_config() { | ||
| for (auto p : _properties_to_reset) { | ||
| ss::smp::invoke_on_all([p] { | ||
| config::shard_local_cfg().get(p).reset(); |
There was a problem hiding this comment.
i must be confused about how reset works--it resets it back to a default? i would have expected that it reset it back to it's value at the time get was called so the scoped configs could compose together or deal with cases where a fixture is customizing a config before a test runs.
There was a problem hiding this comment.
That's right. I waffled on this a bit. I considered saving the entire config on construction, but felt like that might be a bit heavyweight; or serializing the original value as yaml or json and reverting, but opted for resetting because it was a simpler implementation.
If you're okay with it, I could rename it to some scoped_config_reset or somesuch if that makes this less surprising.
There was a problem hiding this comment.
i think it's probably ok as. no harm in improving/changing it later if it's solving a need now
src/v/test_utils/scoped_config.h
Outdated
| } | ||
|
|
||
| private: | ||
| std::list<std::string_view> _properties_to_reset; |
There was a problem hiding this comment.
should we save a real string here? as a test author, i think i'm not going to expect to need to manage the lifetime of property strings. for example, wouldn't this be a use-after-free error?
scoped.get(std::string("asdf")).something()
There was a problem hiding this comment.
Sure I can do this, thought it's also just not something in our tree atm.
There was a problem hiding this comment.
ahh i see. yeh, no urgent need then.
There was a problem hiding this comment.
Oh oops, didn't see the response, but just pushed this change
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.
dotnwat
previously approved these changes
Aug 31, 2023
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.
|
Failures were all #13182 |
andijcr
approved these changes
Sep 1, 2023
|
/backport v23.2.x |
|
Failed to run cherry-pick command. I executed the commands below: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Fixes #12839
Backports Required
Release Notes