Skip to content

Commit

Permalink
Fix label support for TotalNumberOfQueuedAndInProgressWorkflowRuns me…
Browse files Browse the repository at this point in the history 
…tric (#1390)

In #1373 we made two mistakes:

- We mistakenly checked if all the runner labels are included in the job labels and only after that it marked the target as eligible for scale. It should definitely be the opposite!
- We mistakenly checked for the existence of `self-hosted` labe l in the job. [Although it should be a good practice to explicitly say `runs-on: ["self-hosted", "custom-label"]`](https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#example-using-labels-for-runner-selection), that's not a requirement so we should code accordingly.

The consequence of those two mistakes was that, for example, jobs with `self-hosted` + `custom` labels didn't result in scaling runner with `self-hosted` + `custom` + `custom2`. This should fix that.

Ref #1056
Ref #1373
  • Loading branch information
@mumoshu
mumoshu authored Apr 27, 2022
1 parent 059481b commit 4053ab3
Show file tree
Hide file tree
Showing 2 changed files with 96 additions and 21 deletions.
18 changes: 12 additions & 6 deletions controllers/autoscaling.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,17 +140,23 @@ func (r *HorizontalRunnerAutoscalerReconciler) suggestReplicasByQueuedAndInProgr
} else {
JOB:
for _, job := range allJobs {
labels := make(map[string]struct{}, len(job.Labels))
for _, l := range job.Labels {
labels[l] = struct{}{}
runnerLabels := make(map[string]struct{}, len(st.labels))
for _, l := range st.labels {
runnerLabels[l] = struct{}{}
}

if _, ok := labels["self-hosted"]; !ok {
if len(job.Labels) == 0 {
// This shouldn't usually happen
r.Log.Info("Detected job with no labels, which is not supported by ARC. Skipping anyway.", "labels", job.Labels, "run_id", job.GetRunID(), "job_id", job.GetID())
continue JOB
}

for _, l := range st.labels {
if _, ok := labels[l]; !ok {
for _, l := range job.Labels {
if l == "self-hosted" {
continue
}

if _, ok := runnerLabels[l]; !ok {
continue JOB
}
}
Expand Down
99 changes: 84 additions & 15 deletions controllers/autoscaling_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,23 @@ func TestDetermineDesiredReplicas_RepositoryRunner(t *testing.T) {
},

{
description: "Job-level autoscaling with no label (imply self-hosted, 5 requested from 3 workflows)",
description: "Job-level autoscaling with no explicit runner label (runners have implicit self-hosted, requested self-hosted, 5 jobs from 3 workflows)",
repo: "test/valid",
min: intPtr(2),
max: intPtr(10),
workflowRuns: `{"total_count": 4, "workflow_runs":[{"id": 1, "status":"queued"}, {"id": 2, "status":"in_progress"}, {"id": 3, "status":"in_progress"}, {"status":"completed"}]}"`,
workflowRuns_queued: `{"total_count": 1, "workflow_runs":[{"id": 1, "status":"queued"}]}"`,
workflowRuns_in_progress: `{"total_count": 2, "workflow_runs":[{"id": 2, "status":"in_progress"}, {"id": 3, "status":"in_progress"}]}"`,
workflowJobs: map[int]string{
1: `{"jobs": [{"status":"queued", "labels":["self-hosted"]}, {"status":"queued", "labels":["self-hosted"]}]}`,
2: `{"jobs": [{"status": "in_progress", "labels":["self-hosted"]}, {"status":"completed", "labels":["self-hosted"]}]}`,
3: `{"jobs": [{"status": "in_progress", "labels":["self-hosted"]}, {"status":"queued", "labels":["self-hosted"]}]}`,
},
want: 5,
},

{
description: "Skipped job-level autoscaling with no explicit runner label (runners have implicit self-hosted, requested self-hosted+custom, 0 jobs from 3 workflows)",
repo: "test/valid",
min: intPtr(2),
max: intPtr(10),
Expand All @@ -182,11 +198,11 @@ func TestDetermineDesiredReplicas_RepositoryRunner(t *testing.T) {
2: `{"jobs": [{"status": "in_progress", "labels":["self-hosted", "custom"]}, {"status":"completed", "labels":["self-hosted", "custom"]}]}`,
3: `{"jobs": [{"status": "in_progress", "labels":["self-hosted", "custom"]}, {"status":"queued", "labels":["self-hosted", "custom"]}]}`,
},
want: 5,
want: 2,
},

{
description: "Skipped job-level autoscaling with no label (imply self-hosted, 0 requested from 3 workflows)",
description: "Skipped job-level autoscaling with no label (runners have implicit self-hosted, jobs had no labels, 0 jobs from 3 workflows)",
repo: "test/valid",
min: intPtr(2),
max: intPtr(10),
Expand All @@ -202,7 +218,7 @@ func TestDetermineDesiredReplicas_RepositoryRunner(t *testing.T) {
},

{
description: "Job-level autoscaling with default runner label (5 requested from 3 workflows)",
description: "Skipped job-level autoscaling with default runner label (runners have self-hosted only, requested self-hosted+custom, 0 jobs from 3 workflows)",
repo: "test/valid",
labels: []string{"self-hosted"},
min: intPtr(2),
Expand All @@ -215,11 +231,11 @@ func TestDetermineDesiredReplicas_RepositoryRunner(t *testing.T) {
2: `{"jobs": [{"status": "in_progress", "labels":["self-hosted", "custom"]}, {"status":"completed", "labels":["self-hosted", "custom"]}]}`,
3: `{"jobs": [{"status": "in_progress", "labels":["self-hosted", "custom"]}, {"status":"queued", "labels":["self-hosted", "custom"]}]}`,
},
want: 5,
want: 2,
},

{
description: "Skipped job-level autoscaling with custom runner label",
description: "Skipped job-level autoscaling with custom runner label (runners have custom2, requested self-hosted+custom, 0 jobs from 5 workflows",
repo: "test/valid",
labels: []string{"custom2"},
min: intPtr(2),
Expand All @@ -236,7 +252,7 @@ func TestDetermineDesiredReplicas_RepositoryRunner(t *testing.T) {
},

{
description: "Skipped job-level autoscaling with default runner label",
description: "Skipped job-level autoscaling with default runner label (runners have self-hosted, requested managed-runner-label, 0 jobs from 3 runs)",
repo: "test/valid",
labels: []string{"self-hosted"},
min: intPtr(2),
Expand All @@ -253,7 +269,7 @@ func TestDetermineDesiredReplicas_RepositoryRunner(t *testing.T) {
},

{
description: "Job-level autoscaling with default + custom runner label (5 requested from 3 workflows)",
description: "Job-level autoscaling with default + custom runner label (runners have self-hosted+custom, requested self-hosted+custom, 5 jobs from 3 workflows)",
repo: "test/valid",
labels: []string{"self-hosted", "custom"},
min: intPtr(2),
Expand All @@ -270,7 +286,7 @@ func TestDetermineDesiredReplicas_RepositoryRunner(t *testing.T) {
},

{
description: "Job-level autoscaling with custom runner label (5 requested from 3 workflows)",
description: "Job-level autoscaling with custom runner label (runners have custom, requested self-hosted+custom, 5 jobs from 3 workflows)",
repo: "test/valid",
labels: []string{"custom"},
min: intPtr(2),
Expand Down Expand Up @@ -533,7 +549,42 @@ func TestDetermineDesiredReplicas_OrganizationalRunner(t *testing.T) {
},

{
description: "Skipped job-level autoscaling (imply self-hosted, but jobs lack self-hosted labels, 0 requested from 3 workflows)",
description: "Job-level autoscaling (runners have implicit self-hosted, requested self-hosted, 5 jobs from 3 runs)",
org: "test",
repos: []string{"valid"},
min: intPtr(2),
max: intPtr(10),
workflowRuns: `{"total_count": 4, "workflow_runs":[{"id": 1, "status":"queued"}, {"id": 2, "status":"in_progress"}, {"id": 3, "status":"in_progress"}, {"status":"completed"}]}"`,
workflowRuns_queued: `{"total_count": 1, "workflow_runs":[{"id": 1, "status":"queued"}]}"`,
workflowRuns_in_progress: `{"total_count": 2, "workflow_runs":[{"id": 2, "status":"in_progress"}, {"id": 3, "status":"in_progress"}, {"status":"completed"}]}"`,
workflowJobs: map[int]string{
1: `{"jobs": [{"status":"queued", "labels":["self-hosted"]}, {"status":"queued", "labels":["self-hosted"]}]}`,
2: `{"jobs": [{"status": "in_progress", "labels":["self-hosted"]}, {"status":"completed", "labels":["self-hosted"]}]}`,
3: `{"jobs": [{"status": "in_progress", "labels":["self-hosted"]}, {"status":"queued", "labels":["self-hosted"]}]}`,
},
want: 5,
},

{
description: "Job-level autoscaling (runners have explicit self-hosted, requested self-hosted, 5 jobs from 3 runs)",
org: "test",
repos: []string{"valid"},
labels: []string{"self-hosted"},
min: intPtr(2),
max: intPtr(10),
workflowRuns: `{"total_count": 4, "workflow_runs":[{"id": 1, "status":"queued"}, {"id": 2, "status":"in_progress"}, {"id": 3, "status":"in_progress"}, {"status":"completed"}]}"`,
workflowRuns_queued: `{"total_count": 1, "workflow_runs":[{"id": 1, "status":"queued"}]}"`,
workflowRuns_in_progress: `{"total_count": 2, "workflow_runs":[{"id": 2, "status":"in_progress"}, {"id": 3, "status":"in_progress"}, {"status":"completed"}]}"`,
workflowJobs: map[int]string{
1: `{"jobs": [{"status":"queued", "labels":["self-hosted"]}, {"status":"queued", "labels":["self-hosted"]}]}`,
2: `{"jobs": [{"status": "in_progress", "labels":["self-hosted"]}, {"status":"completed", "labels":["self-hosted"]}]}`,
3: `{"jobs": [{"status": "in_progress", "labels":["self-hosted"]}, {"status":"queued", "labels":["self-hosted"]}]}`,
},
want: 5,
},

{
description: "Skipped job-level autoscaling (jobs lack labels, 0 requested from 3 workflows)",
org: "test",
repos: []string{"valid"},
min: intPtr(2),
Expand All @@ -550,7 +601,7 @@ func TestDetermineDesiredReplicas_OrganizationalRunner(t *testing.T) {
},

{
description: "Job-level autoscaling (imply self-hosted, 5 requested from 3 workflows)",
description: "Skipped job-level autoscaling (runners have valid and implicit self-hosted, requested self-hosted+custom, 0 jobs from 3 runs)",
org: "test",
repos: []string{"valid"},
min: intPtr(2),
Expand All @@ -563,11 +614,11 @@ func TestDetermineDesiredReplicas_OrganizationalRunner(t *testing.T) {
2: `{"jobs": [{"status": "in_progress", "labels":["self-hosted", "custom"]}, {"status":"completed", "labels":["self-hosted", "custom"]}]}`,
3: `{"jobs": [{"status": "in_progress", "labels":["self-hosted", "custom"]}, {"status":"queued", "labels":["self-hosted", "custom"]}]}`,
},
want: 5,
want: 2,
},

{
description: "Job-level autoscaling (specified self-hosted, 5 requested from 3 workflows)",
description: "Skipped job-level autoscaling (runners have self-hosted, requested self-hosted+custom, 0 jobs from 3 workflows)",
org: "test",
repos: []string{"valid"},
labels: []string{"self-hosted"},
Expand All @@ -581,11 +632,11 @@ func TestDetermineDesiredReplicas_OrganizationalRunner(t *testing.T) {
2: `{"jobs": [{"status": "in_progress", "labels":["self-hosted", "custom"]}, {"status":"completed", "labels":["self-hosted", "custom"]}]}`,
3: `{"jobs": [{"status": "in_progress", "labels":["self-hosted", "custom"]}, {"status":"queued", "labels":["self-hosted", "custom"]}]}`,
},
want: 5,
want: 2,
},

{
description: "Job-level autoscaling (specified custom, 5 requested from 3 workflows)",
description: "Job-level autoscaling (runners have custom, requested self-hosted+custom, 5 requested from 3 workflows)",
org: "test",
repos: []string{"valid"},
labels: []string{"custom"},
Expand All @@ -602,6 +653,24 @@ func TestDetermineDesiredReplicas_OrganizationalRunner(t *testing.T) {
want: 5,
},

{
description: "Job-level autoscaling (runners have custom, requested custom, 5 requested from 3 workflows)",
org: "test",
repos: []string{"valid"},
labels: []string{"custom"},
min: intPtr(2),
max: intPtr(10),
workflowRuns: `{"total_count": 4, "workflow_runs":[{"id": 1, "status":"queued"}, {"id": 2, "status":"in_progress"}, {"id": 3, "status":"in_progress"}, {"status":"completed"}]}"`,
workflowRuns_queued: `{"total_count": 1, "workflow_runs":[{"id": 1, "status":"queued"}]}"`,
workflowRuns_in_progress: `{"total_count": 2, "workflow_runs":[{"id": 2, "status":"in_progress"}, {"id": 3, "status":"in_progress"}, {"status":"completed"}]}"`,
workflowJobs: map[int]string{
1: `{"jobs": [{"status":"queued", "labels":["custom"]}, {"status":"queued", "labels":["custom"]}]}`,
2: `{"jobs": [{"status": "in_progress", "labels":["custom"]}, {"status":"completed", "labels":["custom"]}]}`,
3: `{"jobs": [{"status": "in_progress", "labels":["custom"]}, {"status":"queued", "labels":["custom"]}]}`,
},
want: 5,
},

{
description: "Skipped job-level autoscaling (specified custom2, 0 requested from 3 workflows)",
org: "test",
Expand Down

0 comments on commit 4053ab3

Please sign in to comment.