Add MAX_EXECUTIONS environment variable limit#5272
Conversation
| execution_count += 1 | ||
| max_executions = environment.get_value('MAX_EXECUTIONS') | ||
| if max_executions and execution_count >= int(max_executions): | ||
| logs.info('Reached MAX_EXECUTIONS limit (%s). Exiting.', max_executions) |
There was a problem hiding this comment.
Nit: Can you format using f strings?
| logs.info('Reached MAX_EXECUTIONS limit (%s). Exiting.', max_executions) | |
| logs.info(f'Reached MAX_EXECUTIONS limit {max_executions}. Exiting.') |
We don't really format strings that way in the project.
Besides i think that using the logs.info() method that way would result in the following log:
....
extras :{
max_executions: 5
},
message: "Reached MAX_EXECUTIONS limit (%s). Exiting."
....
There was a problem hiding this comment.
Good catch! Thanks!
- Fixed the `TypeError: info() takes 1 positional argument but 2 were given` exception when max_executions limit is reached. - Ensured environment re-evaluation in the `test_max_executions` unit test. - Mocked `update_task_enabled` correctly during tests.
- Handles ValueError if MAX_EXECUTIONS cannot be parsed as an integer. - Removes the invalid value from the environment to prevent repeated errors. - Adds test coverage for the invalid input case.
- Removed extra indentation by utilizing Python's short-circuit evaluation inside a single try-except block.
| environment._initial_environment = None | ||
| os.environ['MAX_EXECUTIONS'] = '3' | ||
| # Use side_effect to avoid raising an exception so it loops cleanly. | ||
| self.mock.process_command.side_effect = None |
There was a problem hiding this comment.
I guess this works, but do we even need it? And why not use return_value instead?
| return None | ||
| try: | ||
| return int(val) | ||
| except ValueError: |
There was a problem hiding this comment.
I think we should instead loudly fail and exit if the environment variable is wrong. It would surface configuration issues immediately, right before we start the task loop, whereas logging an error gets a bit lost in the noise.
| break | ||
|
|
||
| execution_count += 1 | ||
| max_executions = _get_max_executions() |
There was a problem hiding this comment.
This should move to above the while loop. There is no reason to re-check the environment variable for a new value on every iteration - it should have the same value for the lifetime of the whole process.
This also ties in with loudly failing on invalid values. If we check the value before we even enter the loop, we will immediately surface configuration errors instead of waiting until after we run a task to notice the error.
Finally, consider switching to a for loop instead. It looks like itertools.repeat() would work well here:
max_executions = _get_max_executions()
for _ in itertools.repeat(None, times=max_executions):
...See https://docs.python.org/3/library/itertools.html#itertools.repeat
I guess the downside of the for loop is that it's harder to log the clean exit message.
|
|
||
| def _get_max_executions(): | ||
| """Returns the MAX_EXECUTIONS limit as an int, or None if invalid/unset.""" | ||
| val = environment.get_value('MAX_EXECUTIONS') |
There was a problem hiding this comment.
Ah, and one final comment: max_executions is a bit vague - clusterfuzz also runs fuzzers that have their own concept of executions, execution speed, etc.. Consider max_task_executions to disambiguate.
3 participants
This PR introduces a
MAX_EXECUTIONSenvironment variable that limits the number of tasks the bot will execute before exiting cleanly. It will facilitate testing locally moving forward.This is particularly useful when running a local bot with a
COMMAND_OVERRIDE, allowing developers to restrict the bot to a specific number of iterations rather than looping infinitely.Changes:
execution_counttracker to the maintask_loopinrun_bot.py.MAX_EXECUTIONSis set and reached.run_bot_test.py.