-
Notifications
You must be signed in to change notification settings - Fork 89
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
Adding support for capacity buffer #39
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get why you changed the drone config, but on the first view that LGTM
I would be ok with changing the default behavior so that when the autoscaler starts, it immediately creates the min number of servers set via DRONE_POOL_MIN. This seems to be how most people expect it to work anyway and it slightly simplifies the implementation by not adding a new configuration parameter. |
While this change might accidentally accomplish that (I wasn't actually even thinking of that when I made this), it really is more to prevent builds queuing and to proactively spin up a new instance ahead of time to have it ready before builds are actually waiting. It won't stop all queuing, but it will add a buffer to reduce what I am hoping is the majority of it. As I think this over though, I could accomplish a similar outcome by increasing the |
Switched it from Thinking that might be more clear what this is used for? |
@tboerger, sorry I have a habit of just running |
@jones2026 ok I think I get it. Is the goal to enable always having a little extra capacity instead of the exact capacity? For example:
Am I understanding correctly? And does this cover all the permutations? Lets make sure we have a unit test for each permutation as well. Thanks for the pull request, and just to let you know, I'm going to be traveling today and tomorrow so my replies may be delayed. |
@bradrydzewski no worries, I honestly didn't expect any replies today! That is exactly what I was going for, except the current implementation in my PR is around server capacity (i.e. concurrency * number of servers) and not the number of actual servers. Do you think it would be clearer to switch it to number of warm server instances instead of spare capacity? (Once we decide whether we think it's best for standby servers or standby capacity I will add all the permutations to the test) |
Sorry for the delayed reply. I went back and forth on this. I think both approaches could work just fine. I think capacity is more granular and therefore makes more sense. In terms of variable names, I think using something like I think overall this looks good. Once we have the additional unit tests in place we should be all set :) |
@bradrydzewski I updated the variable name and added tests for the other permutations you mentioned above. Let me know if you see any other issues. |
Thanks for this. I also updated the documentation accordingly: |
This is to enable the drone autoscaler to have standby capacity ready so you can have warm instances before scaling is needed. This will help avoid builds waiting on nodes to be provisioned and should not affect the normal operation of the autoscaler if you do not want to use this feature.
@tboerger or @bradrydzewski let me know if you have any concerns or suggestions