Actions-runner-controller: Feature: Add Step Scaling Option For PercentageRunnersBusy Schema

[callum-tait-pbx]

Issue
The PercentageRunnersBusy schema works great for my needs, organisation wide scaling without having to maintain a list of runners, instead we can manage workflow-to-runnerdeployment allocation through github labels rather than server side. This makes the overhead of supporting this solution much lower as it makes it much easier for the runnerdeployments to be self-serve for the developers. Scaling the actual runner count based on a percentage though is not ideal as your scaling very quickly starts to get excessively aggressive.

Solution
I'm basically thinking of AWS style step scaling where we would define the thresholds and then increment the runner count by the defined amount:

---
apiVersion: actions.summerwind.dev/v1alpha1
kind: HorizontalRunnerAutoscaler
metadata:
  name: example-runner-deployment-autoscaler
spec:
  scaleTargetRef:
    name: example-runner-deployment
  minReplicas: 1
  maxReplicas: 3
  scaleDownDelaySecondsAfterScaleOut: 120
  metrics:
  - type: PercentageRunnersBusy
    scaleUpThreshold: '0.75'
    scaleDownThreshold: '0.3'
    scaleUpStepFactor: '2' 
    scaleDownStepFactor: '1'

In this example we would scale up the total number of runners by 2 when 75% of the runners were busy, conversly we would scale down the total number of runners by 1 at 30%. Extending the schema to include this alternative scaling style would be a great addition to this schema.

@ZacharyBenamram @mumoshu

[mumoshu]

@callum-tait-pbx Interesting idea! I agree it would be a good addition.

Instead of scale(Up|Down)StepFactor, can we addscaleUpMin, scaleUpMax, scaleInMin, scaleInMax for limiting number of added/removed runners?

The configuration gets more complex than your suggestion, but I think it serves broader use-cases, and fits nicely with TotalNumberOfQueuedAndInProgressWorkflowRuns too.

[callum-tait-pbx]

It's up to you how you want to do it, I unfortunately can't write any Golang and don't get to use it at work so can't really make any head way with learning it sadly. Reasons I suggested the above construct though:

• In my view the RunnerDeployment top level attributes should be respected regardless of what scaling metric is useds i.e. minReplicas, maxReplicas, scaleDownDelaySecondsAfterScaleOut. I think overiding these attributes in the metrics themselves is confusing and inconsistent design wise, the scaling metrics should be responsible with how to scale and by what amount, not what the constraints are for that RunnerDeployment.
• The metrics attribute is both plural in wording and a type list implying support for multiple scaling metrics within a single RunnerDeployment. I think we don't actually support multiple metrics but if we continue to place the max/min replicas constraints at the global level then we don't break that (if multiple scaling metrics are indeed supported) or don't create a complication down the road if that is something someone wants to introduce later.
• I would keep the yaml as simple as possible as documenting it is actually quite hard and so keeping that simple will hopefully keep the amount of issues raised low (even if that makes the initial dev work longer).
• The solution is used by various people all who are used to the minimum and maximum runner count constraints being a global attribute (through the minReplicas and maxReplicas attributes). Whilst changing that design wouldn't be a breaking change it would be a break from the design philosophy and so confusing.

I can't comment too much on if it would be useful for the TotalNumberOfQueuedAndInProgressWorkflowRuns metric as it isn't a metric I use as I need org wide scaling without naming repos. I thought however the main characteristic of TotalNumberOfQueuedAndInProgressWorkflowRuns was it scaled to the amount of workflows queued (up to the maxReplicas constraints) so scaling in a step fashion, whilst would work, isn't really the intended style of scaling? Different metrics for different styles of scaling? I don't use it though so I could be talking out of my arse here, feel free to disagree! :D

If you think there is value in having step style scaling across all metrics then by all means go for it! Additionally if you think there is a better way of adding support in the YAML then go for it! I would say though that however it is done I wouldn't break the design philosophy of:

• constraints are global attributes
• a metrics job is to define how to scale and by how much (but not what the constraints are)

EDIT

I suppose the last point to make is, in my example YAML:

---
apiVersion: actions.summerwind.dev/v1alpha1
kind: HorizontalRunnerAutoscaler
metadata:
  name: example-runner-deployment-autoscaler
spec:
  scaleTargetRef:
    name: example-runner-deployment
  minReplicas: 1
  maxReplicas: 3
  scaleDownDelaySecondsAfterScaleOut: 120
  metrics:
  - type: PercentageRunnersBusy
    scaleUpThreshold: '0.75'
    scaleDownThreshold: '0.3'
    scaleUpStepFactor: '2' 
    scaleDownStepFactor: '1'

The maxReplicas is set to 3, the scale up process should respect that so if our scale up step factor is set to scale by 2, and we are already running 2 runners then it should scale up the runner count by 1 instead so it doesn't overshoot the maxReplicas configuration. It's sort of implied but I thought I should highlight this point explicitly so it's clear.

EDIT

I've thought about it more and I think having step scaling on both metrics makes a lot of sense actually. The difference between the 2 metrics should be seen as how they derive when to scale i.e. queue depth vs runner busyness. As a result it makes total sense to have similar options around what that scaling looks like on each metric where appropriate. When I was thinking about this I was thinking about the metrics as being unique to each other both in terms of when to scale AND how to scale. Now that I've reflected on this more I think when designing metrics it makes more sense to consider them unique in terms of their data source for when to scale (queue depth for TotalNumberOfQueuedAndInProgressWorkflowRuns vs runner busyness for PercentageRunnersBusy), rather than unique in what that scaling actually looks like. (Again where appropriate, we may still have unique scaling ways on metrics due to the unique data source for each scaling metric).

I stand by my other comments however, if I was to implement step scaling support I would keep the min/max replica attributes and extend the specs for the 2 scaling metrics to include a optional new set of scaling attributes keeping it as simple as possible:

---
apiVersion: actions.summerwind.dev/v1alpha1
kind: HorizontalRunnerAutoscaler
metadata:
  name: example-runner-deployment-autoscaler
spec:
  scaleTargetRef:
    name: example-runner-deployment
  minReplicas: 1
  maxReplicas: 3
  scaleDownDelaySecondsAfterScaleOut: 120
  metrics:
  - type: PercentageRunnersBusy
    scaleUpThreshold: '0.75'
    scaleDownThreshold: '0.3'
    scaleUpStepFactor: '2' 
    scaleDownStepFactor: '1'

---
apiVersion: actions.summerwind.dev/v1alpha1
kind: HorizontalRunnerAutoscaler
metadata:
  name: example-runner-deployment-autoscaler
spec:
  scaleTargetRef:
    name: example-runner-deployment
  minReplicas: 1
  maxReplicas: 3
  metrics:
  - type: TotalNumberOfQueuedAndInProgressWorkflowRuns
    scaleUpStepFactor: '2' 
    scaleDownStepFactor: '1'
    repositoryNames:
    - summerwind/actions-runner-controller

[mumoshu]

@callum-tait-pbx Hey! Thank you so much for the detailed response. I think I agree fully with you.

Almost certainly I should have clarified that what I meant by e.g. scaleUpMax was to limit the number of replicas added in one step, rather than another threshold for total replicas.

That said, I would word scaleUpStepFactor as scaleUpAmount as it sounds a little more natural to me(opinion welcomed because I'm not a native English speaker). My scaleUpMax is should be worded scaleUpAmountMax if we make them consistent. Same for the scaleDown variant.

Anyways,

I would keep the yaml as simple as possible as documenting it is actually quite hard and so keeping that simple will hopefully keep the amount of issues raised low (even if that makes the initial dev work longer).

This makes sense a lot! So I'd probably start with adding scaleUpStepFactor / scaleUpAmount. Hope you like it.

[callum-tait-pbx]

Almost certainly I should have clarified that what I meant by e.g. scaleUpMax was to limit the number of replicas added in one step, rather than another threshold for total replicas.

I thought that might be the case, just wanted to be clear on my end

That said, I would word scaleUpStepFactor as scaleUpAmount as it sounds a little more natural to me(opinion welcomed because I'm not a native English speaker). My scaleUpMax is should be worded scaleUpAmountMax if we make them consistent. Same for the scaleDown variant.

I mainly suggestted scaleUpStepFactor / scaleDownStepFactor as the original scope was just for the PercentageRunnersBusy schema so I was falling in line with the language used for that schema, convention over configuration.

This makes sense a lot! So I'd probably start with adding scaleUpStepFactor / scaleUpAmount. Hope you like it.

I've slept on it and I think the most logical way of doing the yaml is to introduce a new scaling attribute scaleType:. In the case of the PercentageRunnersBusy schema the options would be:

scaleType: step
scaleType: percentage

in the case of TotalNumberOfQueuedAndInProgressWorkflowRuns they would be

scaleType: step
scaleType: queueDepth

Example document

---
apiVersion: actions.summerwind.dev/v1alpha1
kind: HorizontalRunnerAutoscaler
metadata:
  name: example-runner-deployment-autoscaler
spec:
  scaleTargetRef:
    name: example-runner-deployment
  minReplicas: 1
  maxReplicas: 3
  scaleDownDelaySecondsAfterScaleOut: 120
  metrics:
  - type: PercentageRunnersBusy
     scaleType: step
     scaleUpThreshold: '0.75'     # required both step scaling and percentage scaling
     scaleDownThreshold: '0.3'  # required both step scaling and percentage scaling
     scaleUpValue: '2'                 # required both step scaling and percentage scaling
     scaleDownValue: '1'             # required both step scaling and percentage scaling

apiVersion: actions.summerwind.dev/v1alpha1
kind: HorizontalRunnerAutoscaler
metadata:
  name: example-runner-deployment-autoscaler
spec:
  scaleTargetRef:
    name: example-runner-deployment
  minReplicas: 1
  maxReplicas: 3
  scaleDownDelaySecondsAfterScaleOut: 60
  metrics:
  - type: TotalNumberOfQueuedAndInProgressWorkflowRuns
     scaleType: step
     scaleUpValue: '2'         # required for step scaling, not required for queue depth scaling
     scaleDownValue: '1'     # required for step scaling, not required for queue depth scaling
    repositoryNames:
    - summerwind/actions-runner-controller

I think we need to introduce a new attribute to define what type of scaling we are using instead of doing it implicitly which was how I was suggesting previously. If we do this then I would make the scale up and down attributes generic sounding as the scaling factor may differ depending on the style of scaling. For instance with the PercentageRunnersBusy schema we can scale up or down in a step fashion meaning we do a simply increment of the runner count or a percentage fashion meaning we are scaling up or down as a percentage of runner count. As a result I do think using the word Value makes sense so we can use the same attributes regardless of the scaling pattern as the value being applied may not be a simple count. This also opens up for addtional scaling options down the line.

If you do decide to introduce a new attribute then we can either make it optional and have the existing scaling type be the default unless specified. I personally however would opt to make it a required attribute and release a new chart under a major version to indicate a breaking change. I think long term having no default and having to define the scaling style is explicitly simplier in terms of administration, documentation and debugging issues, up to you though!

If you think there is a better way of structuring the yaml or you disagree with the wording feel free to make an executive decision!

EDIT

The below is probably the better way of structuring it? Structure as you see best however, whatever structure is used it should:

• support multiple scaling types for each metric with the ability to extend the number of scaling types down the road without structural changes to the yaml allowing for backwards compatability if new scaling types are added to existing metrics as well as well as new scaling types not conflicting with existing definitions
• be able to assign specific attributes for that metric for the type of scaling chosen
• each scaling type potentially may have a different set of associated attributes with it that are unique to that scaling type

  metrics:
  - type: TotalNumberOfQueuedAndInProgressWorkflowRuns
     scalingConfig:
        type: step
        scaleUpValue: '2'
        scaleDownValue: '1'
    repositoryNames:
    - summerwind/actions-runner-controller
---
  metrics:
  - type: TotalNumberOfQueuedAndInProgressWorkflowRuns
     scalingConfig:
        type: queueDepth
    repositoryNames:
    - summerwind/actions-runner-controller

  metrics:
  - type: PercentageRunnersBusy
     scalingConfig:
        type: step
        scaleUpThreshold: '0.75'
        scaleDownThreshold: '0.3'
        scaleUpValue: '2'
        scaleDownValue: '1'
---
  metrics:
  - type: PercentageRunnersBusy
     scalingConfig:
        type: percentage
        scaleUpThreshold: '0.75'
        scaleDownThreshold: '0.3'
        scaleUpValue: '1.4'
        scaleDownValue: '1.2'

[mumoshu]

@callum-tait-pbx (Sorry if this is becoming bikeshedding but I like bikeshedding) I've searched for the term a bit, and it turned out to me that something named Factor sounds like a number in a product to me, so replicas * scaleUpFactor = desiredReplicas looked ok but replicas + scaleUpStepFactor = desiredReplicas did not.

I'm now inclined to scaleUpValue, scaleUpAmount, or scaleUpAdjustment for the starter.

Along the way, I've studied various autoscaling solutions and surprisingly there are many variations:

• Beanstalk calls them "scale up increment" and "scale down increment"
  
  
  
  • The latter sounds odd to me though. Why increment on scale-down?
  
  
• Azure's scheduled scaling calls them "Operation" + "Instance Count", where the latter can be a percentage or a fixed value.
  
  
  
  • This seems similar to your type: percentage and type: step proposal.
  
  
• AWS's step scaling calls them "step adjustment" (BTW I'm linking to a Pulumi doc because it was easy to read).
  
  
  
  • "Adjustment" sounds good.
  
  
  • But after reading I realized that there can be two or more "steps" in Step Autoscaling. In actions-runner-controller, conceptually, we can have only one "scale up step" and "scale down step". So we can think that "step" is already implied in scaleUpThreshold, scaleUpFactor, and so on. Repeating step like "scaleUpStepWhatever" makes me think why we repeat "step" only in it. Other fields should contain "step" in their names, or all the fields should not contain "step" at all, for consistency.
  
  

[mumoshu]

Regardless of which name we chose for what you've originally proposed as scaleUpStepFactor and scaleDownStepFactor for the short-term, reorganizing the API around the concept of scalingConfig would be a good enhancement in the mid-term. I agree with that. Thank you anyway for putting so much effort into it!

[mumoshu]

@callum-tait-pbx Please see #315 for the MVP of it with scale{Up,Down}Adjustment.

[callum-tait-pbx]

Finally deployed into our test cluster and subsequently production now that v0.18.0 is out.

Works as expected, thanks again for the change @mumoshu 👍 ❤️ 😄

[mumoshu]

@callum-tait-pbx Finally!! Glad to hear it worked ☺️ 🎉