Dont delete busy runners

[ulich]

Fixes #166

I tested this with our setup and it was working fine. But we only use ephemeral runners.

[ulich]

@samuelb i adjusted the logic from you a little, i believe some things weren't correct in your implementation (decrementing the idleCounter in every case). Let me know if I have missed something.

[npalm]

@ulich thanks for taking the time to create the PR At the moment we hae crafted the first version of the runner there was no API from github available to find the status. By experimenting we found out that we can only delete offline and idel runners. Not active one.

Now github has an API available (already for some time). It would be better to move to a way by rest checking and next removing. However, we should keep in mind that a runner coulbe inactive at the moment of the check and active and the moment of delete.

I could not find particularly good github documention. But there are two fiels available on the API.

• status with the values idle, offline or active
• busy true or false

The exact meaning of busy seems not be defined anywhere. I think we should use tatus instead of busy for the check and delete all non active runners.

What are your thoughts?

[ulich]

You are right that a runner could not be busy at the time of listing but then be busy again when sending the delete request. In this case it would fail.

The other scenario, where the runner is busy at the time of listing but then is no longer busy could also happen, but maybe wouldn't be that bad, then it would be deleted the next time the scale-down lambda runs.

Even when performing another "status-check" request right before sending the delete request, it still could lead to both of the aforementioned situations, just that it's less probable because there is less time in between, especially when there were multiple delete requests in that run. The downside with the additional status check would be quite some API requests more which maybe lead to throttling?

Maybe, as a starting point, we use the approach of this PR which already improves, I'd say 90% of the requests. For the cases where the runner becomes idle after the list request, the downside would be that the runner would run for a few minutes longer until the next scale-down lambda runs. For the cases where the runner becomes busy after the list request, the behavior remains as before, a delete request which leads to an error. Maybe I should revert this line back to how it was before?

[npalm]

You are right that a runner could not be busy at the time of listing but then be busy again when sending the delete request. In this case it would fail.

The other scenario, where the runner is busy at the time of listing but then is no longer busy could also happen, but maybe wouldn't be that bad, then it would be deleted the next time the scale-down lambda runs.

Even when performing another "status-check" request right before sending the delete request, it still could lead to both of the aforementioned situations, just that it's less probable because there is less time in between, especially when there were multiple delete requests in that run. The downside with the additional status check would be quite some API requests more which maybe lead to throttling?

Maybe, as a starting point, we use the approach of this PR which already improves, I'd say 90% of the requests. For the cases where the runner becomes idle after the list request, the downside would be that the runner would run for a few minutes longer until the next scale-down lambda runs. For the cases where the runner becomes busy after the list request, the behavior remains as before, a delete request which leads to an error. Maybe I should revert this line back to how it was before?

What would you think about not using busy but using the the status?

[ulich]

I don't know what is better. I'm also not familiar with these 2 fields from the GitHub API. But yeah, maybe status is better. I'll adapt the PR to check for status != active

[ulich]

regarding the additional status check request: What we also could do is check for the status flag from the list runners response and then, if status != active, before actually sending the delete request, do another get request for the particular runner, to double check that the runner didn't become active in the meantime.

This wouldn't cause extremely many more API requests but also reduce the amount of requests that lead to errors because the runner is active again.

Wdyt?

[samuelb]

What about not using caching when getting the list of runners (listGitHubRunners())? Then there shouldn't be any significant delay between getting the current state of the runner and sending the deletion requests.

[ulich]

As far as I can see, the cache is invalidated when the scale down lambda is invoked. So I don't think this is a huge problem either, or am I missing something?

[krlydm]

Apologize for the duplicated work, but I believe the scale down improvement I made in #1859 is also addressing the problem reported in #166 and implemented by this PR. Feel free to raise any question about it.

[ulich]

I now tested the code checking for status but actually the value for me always was online. So I think the busy flag is the way to go

[npalm]

I now tested the code checking for status but actually the value for me always was online. So I think the busy flag is the way to go

I will ask the GitHub team if the can point to docs or the meaning of busy. I don't like educated guesses

[npalm]

@ulich Thanks, LGTM