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

Allow ramping down of users #1502

Merged
merged 6 commits into from
Aug 14, 2020
Merged

Allow ramping down of users #1502

merged 6 commits into from
Aug 14, 2020

Conversation

max-rocket-internet
Copy link
Contributor

Resolves: #1488

  • Merge stop_user_instances() into stop_users()
  • Allow stop_rate to be passed to stop_users()
  • Negative or positive value for stop_rate does the same thing
  • Copy same ramping behaviour from spawn_users(), it seems robust
  • If no stop_rate passed to stop_users() then don't "ramp down", just stop all without sleeping
  • Add one extra debug log message when running test is edited with new user_count/hatch_rate values
  • Add test runner to ensure ramping down works

This is my first proper PR for this project so any and all feedback welcome 🙂

I have still yet to update any documentation...

@codecov
Copy link

codecov bot commented Aug 4, 2020

Codecov Report

Merging #1502 into master will decrease coverage by 0.16%.
The diff coverage is 82.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1502      +/-   ##
==========================================
- Coverage   81.44%   81.28%   -0.17%     
==========================================
  Files          27       27              
  Lines        2388     2410      +22     
  Branches      367      372       +5     
==========================================
+ Hits         1945     1959      +14     
- Misses        352      359       +7     
- Partials       91       92       +1     
Impacted Files Coverage Δ
locust/runners.py 81.67% <82.14%> (+0.85%) ⬆️
locust/clients.py 90.19% <0.00%> (-4.91%) ⬇️
locust/event.py 87.50% <0.00%> (-4.40%) ⬇️
locust/user/users.py 95.83% <0.00%> (-2.78%) ⬇️
locust/stats.py 88.96% <0.00%> (-0.06%) ⬇️
locust/user/task.py 96.21% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f5b89d7...dc70aa0. Read the comment docs.

user.stop(self.user_greenlets, force=True)

stop_group.add(user_to_stop._greenlet)
if not stop_group.join(timeout=self.environment.stop_timeout):
Copy link
Collaborator

@cyberw cyberw Aug 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This timeout will potentially prolong the iteration time for this while loop.

So if you had for example stop_rate 1 and stop_timeout 2, you would end up (if no users stopping within the timeout) taking 3 seconds to complete an iteration (giving you a stop rate of 1/3 users per second).

I dont think that is what our users would would expect.

Ideally these things should be done async (I guess you'll have to spawn "killer greenlets" or something)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I see what you mean.

What is we just do stop_group = Group() outside the loop and then stop_group.kill(block=True) after loop is done?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good! Can you add a test for it? I think it is one of those things that could trip us up :) (now or in the future)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can but what exactly is the test? Ramping down with stop_timeout set? How does the test fail? If ramping down doesn't happen? Or it happens too slowly?

If I understand correctly, if stop_timeout is set, we can't really guarantee any ramp down rate accurately, but could do best effort.

@cyberw
Copy link
Collaborator

cyberw commented Aug 8, 2020

If I understand correctly, if stop_timeout is set, we can't really guarantee any ramp down rate accurately, but could do best effort.

Well, in my worst-case example, we should still be stopping roughly 1 user per second, over time (except at the very last step where we last users will take around 1s extra)

For example, if we have 4 users and 2s timeout, it should take 6s to ramp down to zero if no users finish their iteration in time and ramp down is 1/s (and the iterations should be killed after 2 seconds and not sooner)

(the current implementation would take 13s i think)

Also, you can use a faster ramp down rate to make the test take less time than in my example :)

@max-rocket-internet
Copy link
Contributor Author

@cyberw OK I've added a second test under TestStopTimeout that basically repeats the other test but with stop_timeout set. Please have a look and see what you think.

@cyberw
Copy link
Collaborator

cyberw commented Aug 10, 2020

@cyberw OK I've added a second test under TestStopTimeout that basically repeats the other test but with stop_timeout set. Please have a look and see what you think.

This test is too lenient:

self.assertTrue(user_count < 10, "User count has not decreased at all: %i" % user_count)

After 2.1 seconds of ramp down at 2/s and with one second delay I would expect to see 6 users running, any more than that is an error.

You can run higher ramp down rate to shorten the test.

@max-rocket-internet
Copy link
Contributor Author

This test is too lenient

OK fair enough.

I did some testing and did see some variation, like +/- 1 when using whole numbers for sleeping. So I increased the hatch rate to 4 and tightened the sleep times also. This makes the tests run faster and it also very tight with higher hatch rate. If I make it tighter than this I see some failures just from variation.

Copy link
Collaborator

@cyberw cyberw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great now. Only thing left is some documentation + an example.

@max-rocket-internet
Copy link
Contributor Author

Looks great now

🎉

Only thing left is some documentation + an example.

Can you give me a hint here? I've looked through the docs and there's nothing really specifically mentioning hatch rate when decreasing users, decreasing users at all or really anything specific about hatch rate. And what kind of example would I show?

@cyberw
Copy link
Collaborator

cyberw commented Aug 12, 2020

I think for the web gui it is kind of supposed to be self explanatory, so maybe a note there about the hatch rate affecting both up and down ramping (tbh I'm not much of a web user myself).

Maybe there should be a an example similar to the unit test in /examples as well?

(edit: ignore what I previously wrote)

@max-rocket-internet
Copy link
Contributor Author

I think for the web gui it is kind of supposed to be self explanatory, so maybe a note there about the hatch rate affecting both up and down ramping (tbh I'm not much of a web user myself).

OK true. I made some small changes to the wording around starting and editing a test.

Maybe there should be a an example similar to the unit test in /examples as well?

I don't think this is possible? The examples are all various types of locust files and you can't edit a running test in them. Or really do anything to show "ramping down" I don't think. I think this will be addressed in my other PR which we are also close to merging 😉

@cyberw
Copy link
Collaborator

cyberw commented Aug 14, 2020

Nice!

@cyberw cyberw merged commit 19b2dc0 into locustio:master Aug 14, 2020
@max-rocket-internet max-rocket-internet deleted the ramp_down branch August 14, 2020 13:59
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

Successfully merging this pull request may close these issues.

Allow negative hatch rate for ramping down
2 participants