feat(scale-down): Update Owner Logic

[mcaulifn]

• Updates scale down logic to look at runner owner when evaluating which runners to terminate
• Changes tags on instance to tag = type and tag = owner instead of tag = type and value = owner
• Isolates scale down tests

NOTE: Runners that do not contain the new tags will be terminated when the scale down lambda runs.

Resolves #1021

[npalm]

Can you also have a thougt about migrartion? Since your change will most likely use the tags to decide which runners to tear down. Existing runners while upgrading will most likely not have the right tag. Maybe a simple solution like a small script to add tags to existing instances can do trick that needs to be executed once after upgrade.

[mcaulifn]

Would it be worthwhile to add a function to deal with runners that got pulled in with the environment tag but then do not have any other tags?

[mcaulifn]

@npalm I am marking this ready for review. I am working on some very small code changes to deal with orphaned runners. That will solve the bug I identified and also address your concerns. I will update the PR description with that callout.

[mcaulifn]

@npalm I created mcaulifn#2 as a way to deal with legacy runners. If the approach is good, I'll merge.

[gertjanmaas]

@mcaulifn did a review on mcaulifn#2 only some small comments

If that's merged in here, it looks good to me.

Nice work and thanks for improving this project!

[mcaulifn]

@gertjanmaas @npalm I think this should be ready. I will be testing it out in our dev environment next week.

[npalm]

@mcaulifn will check the PR early nexrt week as well. @gertjanmaas thanks for checking

[npalm]

If the diffs shows it correctly, it seems orphan runners are removed if not (runnerMinimumTimeExceeded(ec2Runner, minimumRunningTimeInMinutes)). This will remove all runners that are NOT exceeding the minimal time. So potential still executing the user_data script. Orphan runners are all runners that not registred in github but running in aws as ec instance longer then the minimal time.

So removal should be done if NOT ghRunner and minimal time exeeded. So suggest you swap if clauses on line 113 and 114. Next move the terminateOrphan to the else of the most inner if

[mcaulifn]

I don't think the diff is rendering correctly. I copied that section below and added two notes.

      if (ghRunner) {
        if (runnerMinimumTimeExceeded(ec2Runner, minimumRunningTimeInMinutes)) {
          if (idleCounter > 0) {
            idleCounter--;
            console.debug(`Runner '${ec2Runner.instanceId}' will kept idle.`);
          } else {  //If the idle counter is not greater than 0
            await removeRunner(ec2Runner, ghRunner.id);
          }
        // implied else - If the runner min time not exceeded, continue
        }
      } else {
        console.debug(`Runner '${ec2Runner.instanceId}' is orphaned and will be removed.`);
        terminateOrphan(ec2Runner.instanceId);
      }

[npalm]

I still think the code is not correct see below the part int he PR, second part my proposal

      if (ghRunner) { // runner registred in github
        if (runnerMinimumTimeExceeded(ec2Runner, minimumRunningTimeInMinutes)) { // runner is not running minimum time 
          if (idleCounter > 0) {
            idleCounter--;
            console.debug(`Runner '${ec2Runner.instanceId}' will kept idle.`);
          } else {  // enough idle runners so remove
            await removeRunner(ec2Runner, ghRunner.id);
          }
        // implied else - If the runner min time not exceeded, continue
        }
      } else { // runner not registred in gihub, potential not exceedining minimal time, current code (dev) has check.
        console.debug(`Runner '${ec2Runner.instanceId}' is orphaned and will be removed.`);
        terminateOrphan(ec2Runner.instanceId);
      }

Proposed change, we swam the check for minimal time and ghRunner, so if the runner is not a ghRunner we can safely remove the runner still it execeeded the minimal required time to boot.

    if (runnerMinimumTimeExceeded(ec2Runner, minimumRunningTimeInMinutes)) { 
      if (ghRunnr) { 
        if (idleCounter > 0) {
            idleCounter--;
            console.debug(`Runner '${ec2Runner.instanceId}' will kept idle.`);
        } else {  // enough idle runners so remove
            await removeRunner(ec2Runner, ghRunner.id);
        }
      } else { // runner not registered
          console.debug(`Runner '${ec2Runner.instanceId}' is orphaned and will be removed.`);
          terminateOrphan(ec2Runner.instanceId);
        }
      }
    }

[mcaulifn]

if (runnerMinimumTimeExceeded(ec2Runner, minimumRunningTimeInMinutes)) { // runner is not running minimum time

Is the "not" intended here? I removed the ! so it is now checking if the minimum time has been met.

[mcaulifn]

I realized I missed the comment "So potential still executing the user_data script". That shouldn't be the case as the default is 5 minutes and should be enough time to register. It might be worth calling out that this value should be the the user data execution time plus an additional allowance. In our case, with a 60 minute minimum run time, we don't want an orphaned runner hanging around for that long.

[npalm]

@mcaulifn do you agree with my view on the if / else statements above? I see no change in the code yet.

[mcaulifn]

No, I do not. The updated description covers the not enough time to boot scenario. The intention is to remove orphan runners sooner and not leave them online unnecessarily.

[gertjanmaas]

@mcaulifn I looked at the code in your branch and I still believe it is possible to have an EC2 instance booting up, not yet being registered in Github and then being marked as orphan and thus removed.
It might not happen much because the window for this happening is probably small.

I suggest to split the functionalities @npalm mentioned above into two variables:

1. runner_boot_time_in_minutes, make sure the runner has enough time to boot without it being marked as orphan. Default to 5 mins (or whatever the idle time default is now). This time should be big enough to cover the user_data script and registering the runner to Github.
2. minimum_running_time_in_minutes. Revert to previous description.

This way the runners will be marked as orphan faster when a big minimum_running_time_in_minutes is used, but still avoid runners falsely being marked as orphan.

Not sure if this should be a part of this PR.

[mcaulifn]

@gertjanmaas That sounds good to me. Would the check be for runner_boot_time_in_minutes + minimum_running_time_in_minutes? It will need to be separate.

I think it should be a part of this PR as I am changing the logic and that would satisfy both sets of requirements.

[mcaulifn]

OK, I think I have have it solved now 😆

If the runner is identified as orphaned, there is now a check to see if the minimum boot time has passed. If not, it is left alone for this run. I added a new test instance with a launchTime of "now" that should replicate that.

[mcaulifn]

@npalm I deployed locally and I see a legacy runner and new runners being handled correctly.

[mcaulifn]

@gertjanmaas Anything else on this one?

[gertjanmaas]

Looks good to me, tested the default example and works as expected. @npalm do you want to take a look at it?

[npalm]

@gertjanmaas thanks for checking, please feel free to merge