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

Correct planner's behaviour when there are no capacity left for buffer #94

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

phantasia15
Copy link

Hi, I'm currently using this configuration: DRONE_AGENT_CONCURRENCY=1, DRONE_CAPACITY_BUFFER=1.
When there is only 1 server, 1 running build, I expect the autoscaler to scale the num of servers to 2 but the log say that no capacity changes required.

I took a look the source code and found this:

free := max(capacity-running-p.buffer, 0)

In the above case:

capacity = 1 (only 1 active server)
running = 1 (only 1 active build)
p.buffer = 1
-> free = max(1-1-1,0) = 0
-> diff = ceil(0-0)/1 = 0 
-> Since diff = 0, the autoscaler decides that no changes are required.

I think the problem is that buffer is included in free calculation, but free is forced to be at least 0, instead of -1 in this case.
So I move the buffer into diff calcuation, treating it as a pending build: diff = serverDiff(pending+p.buffer, free, p.cap)). This fixes the above issue while still keeps the logic that free must be at least 0.

@bradrydzewski
Copy link
Member

bradrydzewski commented Jan 7, 2022

thanks, could you please add a unit test that re-creates the state required to reproduce, and that fails without this fix, but passes with the fix? If you look at the unit tests for this file we attempt to test all known states, so this state should definitely be included. Thanks!

Copy link
Member

@bradrydzewski bradrydzewski left a comment

Choose a reason for hiding this comment

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

approval pending unit test

@phantasia15
Copy link
Author

Hi, I've included the failed tests.

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.

3 participants