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

Users with fixed_count not being relocated after rebalance #2091

Closed
andydunstall opened this issue May 5, 2022 · 7 comments · Fixed by #2093
Closed

Users with fixed_count not being relocated after rebalance #2091

andydunstall opened this issue May 5, 2022 · 7 comments · Fixed by #2093
Labels

Comments

@andydunstall
Copy link
Contributor

Describe the bug

Running Locust with multiple workers, where UserA has fixed_count = 1 and UserB has no weight attributes.

As expected when starting 4 workers and 8 users, 1 UserA is started with 7 UserBs. Though when the worker UserA is running on is terminated, Locust rebalances such that theres now 8 UserBs and no UserAs. The same issue seems sometimes occur when just scaling up the number of workers, triggering a rebalance.

Adding some logs to the code, looks like self._try_dispatch_fixed is False on the rebalance (dispatch.py#L387) - which is only set to True when it first runs or when the number of users is reduced.

I'm happy have a go at fixing this - though adding issue for visibility.

Steps to reproduce

# locustfile.py
class UserA:
  fixed_count = 1
  # ...

class UserB:
  # ...
  1. Add 4 workers
  2. Add 1 user: This adds a single UserA as expected
  3. Add 7 more users: This adds 7 UserBs as expected
  4. Terminate the worker running the single UserA
  5. After the rebalance now have 8 UserBs and no UserAs
  6. Adding 2 more users (taking total to 10) still has no UserAs
All users spawned: {"UserA": 1, "UserB": 0} (1 total users)
All users spawned: {"UserA": 1, "UserB": 7} (8 total users)
All users spawned: {"UserA": 0, "UserB": 10} (10 total users)

Environment

  • OS: python:3.9 container
  • Locust version: running master
  • Locust command line that you ran: /usr/local/bin/python /usr/local/bin/locust --master --locustfile .../locustfile.py
  • Locust file contents (anonymized if necessary)
class UserA:
  fixed_count = 1
  # ...

class UserB:
  # ...
@cyberw
Copy link
Collaborator

cyberw commented May 5, 2022

@EzR1d3r anything to add? I dont remember but maybe this is a known issue (either way it could be nice to fix)

@EzR1d3r
Copy link
Contributor

EzR1d3r commented May 6, 2022

@cyberw @andydunstall Hi! It is not known issue. I dont remember rebalance in details right now. Does a master request from workers remained users before rebalancing? Overall, changing self._try_dispatch_fixed to True looks reasonable. And I guess, we need a test for this case, may be in locust/test/test_main.py?

@andydunstall
Copy link
Contributor Author

andydunstall commented May 6, 2022

@EzR1d3r looking at setting self._try_dispatch_fixed = True at the start of _prepare_rebalance then hits the issue that _user_gen compares the expected number of fixed count users with the users in self._users_on_workers, so doesn't add any fixed count users

Resetting self._users_on_workers at the start of _prepare_rebalance works, though I don't understand the threading model so not sure if that introduces a race (commit here https://github.com/andydunstall/locust/commit/009268869f5aded52a19a252bcd8be97a835a8df)

Edit: looking though _prepare_rebalance I can't see anywhere it gives up the thread so I don't think theres a race - though I'm new to gevent/greenlet so may be missing something

@EzR1d3r
Copy link
Contributor

EzR1d3r commented May 6, 2022

Ok, I`ll see. But anyway I think its better to make separate test with fixed users insted of modify the exising with only weighted.

@andydunstall
Copy link
Contributor Author

Ok sure I'll add another test for fixed users (I assumed to try to avoid duplication as it would be basically the same test again)

@EzR1d3r
Copy link
Contributor

EzR1d3r commented May 7, 2022

@andydunstall I think your solution is fine. As I remember I didnt add test with fixed_count for rebalacing. You can find my commits for this feature https://github.com/locustio/locust/commits?author=EzR1d3r
I guess we need smth like TestRemoveWorker.test_remove_worker_during_ramp_up but with fixed.

@andydunstall
Copy link
Contributor Author

@EzR1d3r Ok great thanks - I've added a PR here #2093

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants