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

Restarting the engine unconditionally resets the max_concurrency values #27

Open
0xDEC0DE opened this issue Nov 28, 2023 · 2 comments
Open

Comments

@0xDEC0DE
Copy link
Contributor

This one is a bit complicated, but:

Preamble

A test case script:

import time
import spinach
sp = spinach.Engine(spinach.RedisBroker())

@sp.task(name='nap', max_retries=1, max_concurrency=8)
def nap(job_id):
    print(f"{job_id:3} Zzzz...")
    time.sleep(5)

batch = spinach.Batch()
for i in range(32):
    batch.schedule('nap', i)

sp.schedule_batch(batch)
sp.start_workers(32, stop_when_queue_empty=True)

Steps to reproduce

  • docker-compose -f spinach/tests/docker-compose.yml up -d
  • Run the script above, it should process the queue 8-at-a-time
  • Run redis-cli and execute the command HSET spinach/_max_concurrency nap 32
  • Run the script again

Expected result

The second run of the script runs all 32 tasks simultaneously, like we told it to

Actual behavior

The second run of the script processes the queue 8-at-a-time

Miscellany

  • This is not a bug; it is 100% intentional behavior, with comments in the set_concurrency_keys.lua script explaining that it is done on purpose.
  • Our app has several Spinach tasks with max_concurrency set to "baseline" values, and Operations occasionally has a need to tune the values up or down, for reasons.
  • This is highly inconvenient for things like flask_spinach running under Gunicorn; when Gunicorn cycles out workers, the new workers start and destroy any runtime adjustments to the Redis that operators may have made; and it does so with no warning.
  • Conversely, it is highly convenient if an Operator wants to, e.g., set a "burst" value for a Spinach task, and immediately forget about it; the software will put itself back on its own.
  • The previous behavior can be simulated by putting a TTL on the _max_concurrency key, but Issue Jobs without concurrency limits generate current_concurrency hash keys in redis #15 makes it not work very well.
  • Swinging the concurrency adjustments through the app, rather than fidgeting with Redis underneath it, is also an obvious dodge to this issue.

Any thoughts and insights on how to best address this would be appreciated.

@bigjools
Copy link
Collaborator

This is not a bug; it is 100% intentional behavior, with comments in the set_concurrency_keys.lua script explaining that it is done on purpose.

This is the only place that sets up the key, so naturally it's going to override the current value if one was set from a different client.

I'd advocate for a second key that allows user to set their own limits, effectively as an override for the code's own limit. We can amend the Lua script so it looks for that key first and falls back to the real one. Users could potentially add a TTL on the second key to get the "temporary behaviour" action.

0xDEC0DE pushed a commit to 0xDEC0DE/spinach that referenced this issue Nov 28, 2023
- Leave already-set concurrency values alone, always.
- Fix some unit tests that were throwing `InvalidJobSignatureError` errors
- Fix incompatibilties with tox >= 4.0

Fixes: Issue NicolasLM#27
@bigjools
Copy link
Collaborator

In fact I'd say let's add a tool script to do this, so that the user doesn't need to know about our Redis keys. Something like:
spinach max_concurrency myjobname 32 1m
to set myjobname job to 32 concurrency for 1 minute.

0xDEC0DE pushed a commit to 0xDEC0DE/spinach that referenced this issue Feb 28, 2024
Leave already-set concurrency values alone, always.

Fixes: Issue NicolasLM#27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants