Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make tests safer to run in parallel by changing the Redis key namespace #6283

Merged
merged 7 commits into from
Nov 29, 2024

Conversation

cognifloyd
Copy link
Member

@cognifloyd cognifloyd commented Nov 23, 2024

This PR's commits were extracted from #6273 where I'm working on getting pants+pytest to run integration tests.

When running tests in parallel, the keys stored in redis for one test can mess up the expected redis keys needed by a different test. The community edition of redis has 16 logical databases, but if someone has more cores than that or uses pants remote test execution (with a default of 128 possible remote executors running in parallel), then 16 is not enough.

It turns out, the tooz redis driver already has a config option to support this: namespace. By default, the tooz driver uses _tooz as the key namespace. We can change that by passing a namespace query param in [coordination].url.

So, I updated the tests config to change the namespace using the pants-provided ST2TESTS_PARLLEL_SLOT env var here (building on the ST2TESTS_REDIS_* env vars added in #6245):

def _override_coordinator_opts(noop=False):
driver = None if noop else "zake://"
redis_host = os.environ.get("ST2TESTS_REDIS_HOST", False)
if redis_host:
redis_port = os.environ.get("ST2TESTS_REDIS_PORT", "6379")
# namespace= is the tooz redis driver's key prefix (default is "_tooz")
namespace = f"_st2_test{os.environ.get('ST2TESTS_PARALLEL_SLOT', '')}"
driver = f"redis://{redis_host}:{redis_port}?namespace={namespace}"

I updated tools/launchdev.sh to respect ST2_* env vars (the oslo_config env vars added in #6277) and the ST2TESTS_* env vars (This builds on the refactor from #6276 to make it simpler to update the command used to run each service):

st2/tools/launchdev.sh

Lines 137 to 146 in c394a52

# ST2_* vars directly override conf vars using oslo_config's env var feature.
# The ST2TESTS_* vars are only for tests, and take precedence over ST2_* vars.
if [ -n "${ST2TESTS_SYSTEM_USER}" ]; then
export ST2_SYSTEM_USER__USER="${ST2TESTS_SYSTEM_USER}"
ST2VARS+=("ST2_SYSTEM_USER__USER")
fi
if [ -n "${ST2TESTS_REDIS_HOST}" ] && [ -n "${ST2TESTS_REDIS_PORT}"]; then
export ST2_COORDINATION__URL="redis://${ST2TESTS_REDIS_HOST}:${ST2TESTS_REDIS_PORT}?namespace=_st2_dev"
ST2VARS+=("ST2_COORDINATION__URL")
fi

The redis namespace for tools/launchdev.sh is defined in st2.dev.conf here:

url = redis://127.0.0.1:6379?namespace=_st2_dev

I updated pants-plugins/uses_services to use the new env vars as well, following the patterns established in #6278. pants.toml did not need any additional vars, since it already has the ST2TESTS_REDIS_* env vars:

st2/pants.toml

Lines 251 to 256 in c394a52

# Use these to override Redis connection details
"ST2TESTS_REDIS_HOST",
"ST2TESTS_REDIS_PORT",
# "ST2_COORDINATION__URL", # Tests will override this with one of:
# "redis://{ST2TESTS_REDIS_HOST}:{ST2TESTS_REDIS_PORT}?namespace=_st2_test{ST2TESTS_PARALLEL_SLOT}
# "zake://"

I did, however, add ST2_SYSTEM_USER__USER to [tests].extra_env_vars in pants.toml here:

st2/pants.toml

Lines 240 to 243 in c394a52

# Use this so that the test system does not require the stanley user.
# For example: export ST2TESTS_SYSTEM_USER=${USER}
"ST2TESTS_SYSTEM_USER",
"ST2_SYSTEM_USER__USER",

This will make it safer to run tests in parallel as the
tests will have separate service registration and coordination locks.
To do that, this adds a namespace= query param to the redis url
that includes the slot number that pants creates to allow tests to run in parallel more safely.
The namespace is used by the tooz library's redis driver as a prefix
for all the redis keys. Thus, multiple tests can use the same
redis database without clobbering each other's keys.
These changes were already made for MongoDB and RabbitMQ rules.
This removes a TODO comment and slightly refactors to follow the same convention.
@cognifloyd cognifloyd added this to the pants milestone Nov 23, 2024
@cognifloyd cognifloyd self-assigned this Nov 23, 2024
@pull-request-size pull-request-size bot added the size/M PR that changes 30-99 lines. Good size to review. label Nov 23, 2024
@cognifloyd cognifloyd requested a review from a team November 24, 2024 15:58
This fixes CircleCI.

webtest bumped their dep on waitress to a version that is not available
to all of our interpreters.
Pylons/webtest@090c5b2

The lockfile has a version locked version that does not have this issue.
The pin was already copied to test-requirements.txt, so this just copies
it to the other requirements files as well. This is not a significant
issue because webtest is only required by st2tests.
@cognifloyd cognifloyd merged commit 18241a8 into master Nov 29, 2024
42 checks passed
@cognifloyd cognifloyd deleted the redis-namespace branch November 29, 2024 03:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure: ci/cd pantsbuild size/M PR that changes 30-99 lines. Good size to review. tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants