Fix/autoscaling

[skeith45]

Description

The pull request is to fix the auto-scaling configuration using object variable rather than individual variables.

Two variables had wrong typing so they were being put in config.toml even if set to -1 which should have resulted in those to return empty strings but they were filled with the values has -1 each.

${runners_off_peak_idle_count}
${runners_off_peak_idle_time}

The 3 spaces for the object-based auto-scaling config was shifted from "template/runner-config.tpl" to "template/runners_machine_autoscaling.tpl" since that particular space placement made it so only the first set of
"[[runners.machine.autoscaling]]" were preceded by 3 spaces

Finally, line 3 of "template/runners_machine_autoscaling.tpl" was missing surrounding double-quotes that would also yield invalid configuration.

Migrations required

NO (Shouldn't be at least seeing as setting up auto-scaling using the object variable is currently not working)

Verification

None of the examples currently use the object configuration method rather than the method marked as deprecated.

The parameter used to test was :

runners_machine_autoscaling = [
    {
      timezone   = var.timezone
      idle_count = 0
      idle_time  = 60
      periods = ["* * 0-7,18-23 * * mon-fri *"]
    },
    {
      timezone   = var.timezone
      idle_count = 0
      idle_time  = 60
      periods = ["* * * * * sat,sun *"]
    }
  ]

Documentation

pre-commit script were run and passed

[npalm]

@skeith45 thanks for your contribution. I put it on my backlog to check the PR in the next days.

[npalm]

First of al thanks for the contribution! I just checked your change, A few questions and remarks

• I have deployed the deafult example, with the change in this PR the runner agent was not starting. I think the problem is introduced via the formatting of the toml for auto scaling group. Please can you check once again?
• The PR needs to rebased, due to conflicts with other merged pr's
• Just wondering which version of tf docs are you using. Since resources are added to the readme.

[skeith45]

I'll recheck for the toml. A bit odd that its not working cause I use the same changes that are in this PR in a repo that is used to deploy runners that are currently in-use. Curious if there could be differences due to terraform version? I'm currently using the latest 0.12 version of terraform.

Regarding the conflict, I followed the contributing doc and used master as the starting point but I see that the other pull request got merged into develop and master has yet to change. Is it safe to assume I should using develop as a base? If so it might be pertinent to update the contributing documentation.

Regarding the tf docs version. I'm using tf docs version 0.11.0 (Windows version to be precise, don't know if it makes any difference). Installed it via chocolatey; I see that there is a version 0.11.2 available however.

[npalm]

I'll recheck for the toml. A bit odd that its not working cause I use the same changes that are in this PR in a repo that is used to deploy runners that are currently in-use. Curious if there could be differences due to terraform version? I'm currently using the latest 0.12 version of terraform.

Did specify in your cae the following variable? runners_machine_autoscaling as is done in the default example?

  # Deprecated, replaced by runners_machine_autoscaling
  # runners_off_peak_periods    = "[\"* * 0-9,17-23 * * mon-fri *\", \"* * * * * sat,sun *\"]"
  # runners_off_peak_timezone   = var.timezone
  # runners_off_peak_idle_count = 0
  # runners_off_peak_idle_time  = 60
  runners_machine_autoscaling = [
    {
      periods    = ["\"* * 0-9,17-23 * * mon-fri *\"", "\"* * * * * sat,sun *\""]
      idle_count = 0
      idle_time  = 60
      timezone   = var.timezone
    }
  ]

see https://github.com/npalm/terraform-aws-gitlab-runner/blob/532eb0b3ecc20502693ad87747274d32aaa8869e/examples/runner-default/main.tf#L81

Regarding the conflict, I followed the contributing doc and used master as the starting point but I see that the other pull request got merged into develop and master has yet to change. Is it safe to assume I should using develop as a base? If so it might be pertinent to update the contributing documentation.

Sorry for the confusion, and I wil upate the contribution guide. Please use the default branch develop.

Regarding the tf docs version. I'm using tf docs version 0.11.0 (Windows version to be precise, don't know if it makes any difference). Installed it via chocolatey; I see that there is a version 0.11.2 available however.

My mistake, it seems my link to the terraform-dcos installation wasn't correct. Generation of the resources was added to version 11. So that is all good.

[skeith45]

I put in the content of the variable inside the PR main post (changed the value to 9 to 5 in the comment here however) :

runners_machine_autoscaling = [
    {
      timezone   = var.timezone
      idle_count = 0
      idle_time  = 60
      periods = ["* *0-9,17-23 * * mon-fri *"]
    },
    {
      timezone   = var.timezone
      idle_count = 0
      idle_time  = 60
      periods = ["* * * * * sat,sun *"]
    }
  ]

This makes the template file generate two [[runners.machine.autoscaling]] blocks which is how its done in the gitlab runner documentation : Autoscaling Documentation

I guess I should probably remove the join call in the template cause that doesn't work with this method of configuring autoscaling as far as I'm aware.

[skeith45]

My bad, the period is an array of strings, so I guess it should work if you put multiple periods in it.
But the current code didn't surround the periods with double quotes.
And didn't allow for multiple autoscaling blocks.

I'll try to make sure the multiple period string within same object still work at least. Maybe I broke that.

[npalm]

I think you aprroach is actually better. This avoid the user has to avoid escaping the double quotes. I just checked the defaullt example and set thre periods to:

periods    = ["* * 0-9,17-23 * * mon-fri *", "* * * * * sat,sun *"]

This is working, drawback is that is breaking backwards compatiblity. But we could add a clear direction for migration. Do you see any good option to remain backwards compatible.

[skeith45]

I added backward compatibility. It makes the period handling of the template a bit harder to read but it makes it compatible with both methods of writing the autoscaling object.

I didn't test it in an actual runner but I did test the output of the template using the variable below which tests both old and new version of the period. The output is the same for both old and new. (whether you add escaped double quotes or not you'll get the same output)

runners_machine_autoscaling = [
  {
    periods    = ["\"* * 0-9,17-23 * * mon-fri *\"", "\"* * * * * sat,sun *\""]
    idle_count = 0
    idle_time  = 60
    timezone   = "Europe/Amsterdam"
  },
  {
    periods    = ["\"* * * * * sat,sun *\""]
    idle_count = 0
    idle_time  = 60
    timezone   = "Europe/Amsterdam"
  },
  {
    periods    = ["* * 0-9,17-23 * * mon-fri *","* * * * * sat,sun *"]
    idle_count = 0
    idle_time  = 60
    timezone   = "Europe/Amsterdam"
  },
  {
    periods    = ["* * * * * sat,sun *"]
    idle_count = 0
    idle_time  = 60
    timezone   = "Europe/Amsterdam"
  }
]

[npalm]

Thanks for all the work!

[semantic-releaser]

🎉 This PR is included in version 4.23.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀