#1884 User distribution should happen when new workers comes in #1886

tyge68 · 2021-09-15T10:46:22Z

Adding a new argument to enable automatic rebalancing for new workers (after ramp up completed)
Log message added when rebalancing is triggered

To be checked how to test such code change.

cyberw · 2021-09-15T11:45:29Z

I only had a quick look, but I dont see anything bad about this.

mboutet · 2021-09-15T13:15:09Z

report.html

@@ -0,0 +1,353 @@
+<!DOCTYPE html>


Was this file committed inadvertently?

mboutet · 2021-09-15T13:24:20Z

I know I said yesterday that the background task was my preferred solution, but looking back at the code, I remembered I started to implement it in a more efficient fashion.

It is more efficient because rebalancing only takes place whenever a new worker joins or if a worker goes missing instead of always rebalancing even if there's nothing to rebalance. It is also faster to react because it acts immediately upon a worker joining or missing.

The logic for a worker joining goes here:

locust/locust/runners.py

Lines 880 to 883 in bb77344

    
           # if self.state == STATE_RUNNING or self.state == STATE_SPAWNING: 
        
           #     # TODO: Necessary now that UsersDispatcher handles that? 
        
           #     # balance the load distribution when new client joins 
        
           #     self.start(self.target_user_count, self.spawn_rate)

The logic for a worker that goes missing goes here:

locust/locust/runners.py

Line 834 in bb77344

# TODO: If status is `STATE_RUNNING`, call self.start()

mboutet · 2021-09-15T14:04:29Z

Also note that because of a regression I introduced in #1809 and that was reported in #1883, the target_user_count on the master is currently not updated until the end of the ramp-up/down. This should be fixed in an upcoming PR. I mention it because it means it could potentially cause a rebalancing to be messed up.

tyge68 · 2021-09-15T14:05:07Z

@mboutet indeed it would make more sense to implement it in the event, currently I have added a new integration test, it would currently fail without my changes, but I will try to fix it in the event code instead.

mboutet · 2021-09-15T14:15:35Z

I saw in one of your latest commit that you removed the --enable-rebalancing flag. However, I believe it is still needed because some people might not want the rebalancing behaviour.

tyge68 · 2021-09-15T14:17:30Z

@mboutet ok sure I will add it back

tyge68 · 2021-09-15T14:32:46Z

@cyberw @mboutet I think it should be ready for final review now, let me know if the last changes and the added test is good enough.

mboutet · 2021-09-15T14:37:26Z

Can you address the comment I left: #1886 (comment)

mboutet · 2021-09-15T14:38:11Z

locust/runners.py

@@ -637,6 +638,9 @@ def on_quitting(environment, **kw):

        self.environment.events.quitting.add_listener(on_quitting)

+    def rebalancingEnabled(self):


Suggested change

def rebalancingEnabled(self):

@property

def rebalancing_enabled(self) -> bool:

mboutet · 2021-09-15T14:38:34Z

locust/runners.py

-                #     # TODO: Necessary now that UsersDispatcher handles that?
-                #     # balance the load distribution when new client joins
-                #     self.start(self.target_user_count, self.spawn_rate)
+                if self.rebalancingEnabled() and self.state == STATE_RUNNING and self.spawning_completed:


Suggested change

if self.rebalancingEnabled() and self.state == STATE_RUNNING and self.spawning_completed:

if self.rebalancing_enabled and self.state == STATE_RUNNING and self.spawning_completed:

mboutet · 2021-09-15T14:40:27Z

locust/argument_parser.py

+        action="store_true",
+        default=False,
+        dest="enable_rebalancing",
+        help="Allow to automatically rebalance users if new workers are added after ramp up completed.",


Suggested change

help="Allow to automatically rebalance users if new workers are added after ramp up completed.",

help="Allow to automatically rebalance users if new workers are added or removed during a test run.",

mboutet · 2021-09-15T14:47:34Z

@tyge68, the re-balancing also needs to be implemented when a worker goes missing:

locust/locust/runners.py

Line 834 in bb77344

# TODO: If status is `STATE_RUNNING`, call self.start()

Also, I'll have to revisit my code, because according to this:

locust/locust/runners.py

Lines 873 to 875 in a278fb3

    
           if not self._users_dispatcher.dispatch_in_progress and self.state == STATE_RUNNING: 
        
               # TODO: Test this situation 
        
               self.start(self.target_user_count, self.spawn_rate)

I already implemented the rebalance when a worker joins. Apparently, it doesn't work, otherwise @tyge68 would have not reported the issue. I'll try to look into this soon.

mboutet · 2021-09-15T15:01:05Z

locust/test/test_runners.py

@@ -717,7 +717,7 @@ def addWorker():
            def checkRebalancedTrue():


In Python, snake_case is preferred. There's some old library such as unittest that uses camelCase (i.e. self.assertTrue), but this is only because the standard library needs to keep backward compatibility.

# This is the 1st commit message: locustio#1884 User distribution should happen when new workers comes in # This is the commit message locustio#2: locustio#1884 reformatting # This is the commit message locustio#3: locustio#1884 Make rebalance interval configurable # This is the commit message locustio#4: locustio#1884 Adding integration test for rebalanced # This is the commit message locustio#5: locustio#1884 revert change # This is the commit message locustio#6: locustio#1886 Simplify solution # This is the commit message locustio#7: locustio#1886 adding missing dependency # This is the commit message locustio#8: locustio#1884 Adding back enable rebalancing option # This is the commit message locustio#9: locustio#1884 adding extra comment the test # This is the commit message locustio#10: locustio#1884 renaming as suggested # This is the commit message locustio#11: locustio#1884 update help description # This is the commit message locustio#12: locustio#1884 handling of missing workers to rebalance

…comes in

tyge68 · 2021-09-15T16:45:11Z

@mboutet I have applied the last suggested changes regarding the snake_case, and cleanup the commit history.
you can recheck the new test eventually , that would also be a good one to try to fix the original line of code that should have worked.

cyberw · 2021-09-18T09:09:54Z

LGTM, except for the remaining camelCase method (checkRebalancedTrue) in the test

tyge68 · 2021-09-20T08:12:30Z

@cyberw indeed, fixed casing issue :)

cyberw · 2021-09-20T13:06:43Z

Just waiting for any comments from @mboutet ...

mboutet · 2021-09-20T13:11:48Z

Sorry for not getting back sooner. The changes and the test look good. I haven't tested the branch locally though.

mboutet · 2021-09-20T13:13:49Z

Thanks @tyge68

cyberw · 2021-09-20T13:14:19Z

Thanks! I'll trust the tests to report any errors (and the poor sods running latest master/prerelease version :)

mboutet · 2021-09-20T13:15:14Z

I'm one of them 😜

mboutet reviewed Sep 15, 2021

View reviewed changes

report.html Outdated

@@ -0,0 +1,353 @@

<!DOCTYPE html>

Copy link

Contributor

mboutet Sep 15, 2021

Choose a reason for hiding this comment

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

Was this file committed inadvertently?

mboutet mentioned this pull request Sep 15, 2021

User distribution should happen when new workers comes in #1884

Closed

tyge68 added a commit to tyge68/locust that referenced this pull request Sep 15, 2021

locustio#1886 Simplify solution

9bd3895

tyge68 added a commit to tyge68/locust that referenced this pull request Sep 15, 2021

locustio#1886 adding missing dependency

8cba567

tyge68 marked this pull request as ready for review September 15, 2021 14:31

mboutet reviewed Sep 15, 2021

View reviewed changes

tyge68 force-pushed the issue/1884 branch 2 times, most recently from 9e888fd to 2b09b79 Compare September 15, 2021 16:30

Issue locustio#1884 User distribution should happen when new workers …

84838a6

…comes in

tyge68 force-pushed the issue/1884 branch from 2b09b79 to 84838a6 Compare September 15, 2021 16:36

Issue locustio#1884 Turning to snake case style

9ec166b

cyberw merged commit 87b6dbc into locustio:master Sep 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#1884 User distribution should happen when new workers comes in #1886

#1884 User distribution should happen when new workers comes in #1886

tyge68 commented Sep 15, 2021

cyberw commented Sep 15, 2021

mboutet Sep 15, 2021

mboutet commented Sep 15, 2021

mboutet commented Sep 15, 2021 •

edited

Loading

tyge68 commented Sep 15, 2021

mboutet commented Sep 15, 2021

tyge68 commented Sep 15, 2021

tyge68 commented Sep 15, 2021

mboutet commented Sep 15, 2021

mboutet Sep 15, 2021

mboutet Sep 15, 2021

mboutet Sep 15, 2021

mboutet commented Sep 15, 2021 •

edited

Loading

mboutet Sep 15, 2021

tyge68 commented Sep 15, 2021

cyberw commented Sep 18, 2021

tyge68 commented Sep 20, 2021

cyberw commented Sep 20, 2021

mboutet commented Sep 20, 2021

mboutet commented Sep 20, 2021

cyberw commented Sep 20, 2021

mboutet commented Sep 20, 2021

		@@ -637,6 +638,9 @@ def on_quitting(environment, **kw):

		self.environment.events.quitting.add_listener(on_quitting)

		def rebalancingEnabled(self):

	def rebalancingEnabled(self):
	@property
	def rebalancing_enabled(self) -> bool:

	if self.rebalancingEnabled() and self.state == STATE_RUNNING and self.spawning_completed:
	if self.rebalancing_enabled and self.state == STATE_RUNNING and self.spawning_completed:

	help="Allow to automatically rebalance users if new workers are added after ramp up completed.",
	help="Allow to automatically rebalance users if new workers are added or removed during a test run.",

		@@ -717,7 +717,7 @@ def addWorker():
		def checkRebalancedTrue():

#1884 User distribution should happen when new workers comes in #1886

#1884 User distribution should happen when new workers comes in #1886

Conversation

tyge68 commented Sep 15, 2021

cyberw commented Sep 15, 2021

mboutet Sep 15, 2021

Choose a reason for hiding this comment

mboutet commented Sep 15, 2021

mboutet commented Sep 15, 2021 • edited Loading

tyge68 commented Sep 15, 2021

mboutet commented Sep 15, 2021

tyge68 commented Sep 15, 2021

tyge68 commented Sep 15, 2021

mboutet commented Sep 15, 2021

mboutet Sep 15, 2021

Choose a reason for hiding this comment

mboutet Sep 15, 2021

Choose a reason for hiding this comment

mboutet Sep 15, 2021

Choose a reason for hiding this comment

mboutet commented Sep 15, 2021 • edited Loading

mboutet Sep 15, 2021

Choose a reason for hiding this comment

tyge68 commented Sep 15, 2021

cyberw commented Sep 18, 2021

tyge68 commented Sep 20, 2021

cyberw commented Sep 20, 2021

mboutet commented Sep 20, 2021

mboutet commented Sep 20, 2021

cyberw commented Sep 20, 2021

mboutet commented Sep 20, 2021

mboutet commented Sep 15, 2021 •

edited

Loading

mboutet commented Sep 15, 2021 •

edited

Loading