From 297442975eb8958ec2aaf32cb1578e400e1f21d1 Mon Sep 17 00:00:00 2001 From: Lars Lange <9141483+Langleu@users.noreply.github.com> Date: Tue, 25 Jul 2023 02:04:54 +0200 Subject: [PATCH] fix: remove callbacks resulting in scales due to incomplete response (#2671) Co-authored-by: Yusuke Kuoka --- .../actions.summerwind.net/autoscaling.go | 13 ++-- .../autoscaling_test.go | 77 +++++++++++++++---- 2 files changed, 71 insertions(+), 19 deletions(-) diff --git a/controllers/actions.summerwind.net/autoscaling.go b/controllers/actions.summerwind.net/autoscaling.go index ab623051..ea21f953 100644 --- a/controllers/actions.summerwind.net/autoscaling.go +++ b/controllers/actions.summerwind.net/autoscaling.go @@ -118,10 +118,10 @@ func (r *HorizontalRunnerAutoscalerReconciler) suggestReplicasByQueuedAndInProgr } var total, inProgress, queued, completed, unknown int - type callback func() - listWorkflowJobs := func(user string, repoName string, runID int64, fallback_cb callback) { + listWorkflowJobs := func(user string, repoName string, runID int64) { if runID == 0 { - fallback_cb() + // should not happen in reality + r.Log.Info("Detected run with no runID of 0, ignoring the case and not scaling.", "repo_name", repoName, "run_id", runID) return } opt := github.ListWorkflowJobsOptions{ListOptions: github.ListOptions{PerPage: 50}} @@ -139,7 +139,8 @@ func (r *HorizontalRunnerAutoscalerReconciler) suggestReplicasByQueuedAndInProgr opt.Page = resp.NextPage } if len(allJobs) == 0 { - fallback_cb() + // GitHub API can return run with empty job array - should be ignored + r.Log.Info("Detected run with no jobs, ignoring the case and not scaling.", "repo_name", repoName, "run_id", runID) } else { JOB: for _, job := range allJobs { @@ -201,9 +202,9 @@ func (r *HorizontalRunnerAutoscalerReconciler) suggestReplicasByQueuedAndInProgr case "completed": completed++ case "in_progress": - listWorkflowJobs(user, repoName, run.GetID(), func() { inProgress++ }) + listWorkflowJobs(user, repoName, run.GetID()) case "queued": - listWorkflowJobs(user, repoName, run.GetID(), func() { queued++ }) + listWorkflowJobs(user, repoName, run.GetID()) default: unknown++ } diff --git a/controllers/actions.summerwind.net/autoscaling_test.go b/controllers/actions.summerwind.net/autoscaling_test.go index ec0ac79a..ee42f9a4 100644 --- a/controllers/actions.summerwind.net/autoscaling_test.go +++ b/controllers/actions.summerwind.net/autoscaling_test.go @@ -61,8 +61,9 @@ func TestDetermineDesiredReplicas_RepositoryRunner(t *testing.T) { want int err string }{ + // case_0 // Legacy functionality - // 3 demanded, max at 3 + // 0 demanded due to zero runID, min at 2 { repo: "test/valid", min: intPtr(2), @@ -70,9 +71,10 @@ func TestDetermineDesiredReplicas_RepositoryRunner(t *testing.T) { workflowRuns: `{"total_count": 4, "workflow_runs":[{"status":"queued"}, {"status":"in_progress"}, {"status":"in_progress"}, {"status":"completed"}]}"`, workflowRuns_queued: `{"total_count": 1, "workflow_runs":[{"status":"queued"}]}"`, workflowRuns_in_progress: `{"total_count": 2, "workflow_runs":[{"status":"in_progress"}, {"status":"in_progress"}]}"`, - want: 3, + want: 2, }, - // Explicitly speified the default `self-hosted` label which is ignored by the simulator, + // case_1 + // Explicitly specified the default `self-hosted` label which is ignored by the simulator, // as we assume that GitHub Actions automatically associates the `self-hosted` label to every self-hosted runner. // 3 demanded, max at 3 { @@ -80,11 +82,17 @@ func TestDetermineDesiredReplicas_RepositoryRunner(t *testing.T) { labels: []string{"self-hosted"}, min: intPtr(2), max: intPtr(3), - workflowRuns: `{"total_count": 4, "workflow_runs":[{"status":"queued"}, {"status":"in_progress"}, {"status":"in_progress"}, {"status":"completed"}]}"`, - workflowRuns_queued: `{"total_count": 1, "workflow_runs":[{"status":"queued"}]}"`, - workflowRuns_in_progress: `{"total_count": 2, "workflow_runs":[{"status":"in_progress"}, {"status":"in_progress"}]}"`, - want: 3, + 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"]}]}`, + 2: `{"jobs": [{"status": "in_progress", "labels":["self-hosted"]}]}`, + 3: `{"jobs": [{"status": "in_progress", "labels":["self-hosted"]}]}`, + }, + want: 3, }, + // case_2 // 2 demanded, max at 3, currently 3, delay scaling down due to grace period { repo: "test/valid", @@ -97,6 +105,7 @@ func TestDetermineDesiredReplicas_RepositoryRunner(t *testing.T) { workflowRuns_in_progress: `{"total_count": 1, "workflow_runs":[{"status":"in_progress"}]}"`, want: 3, }, + // case_3 // 3 demanded, max at 2 { repo: "test/valid", @@ -107,6 +116,7 @@ func TestDetermineDesiredReplicas_RepositoryRunner(t *testing.T) { workflowRuns_in_progress: `{"total_count": 2, "workflow_runs":[{"status":"in_progress"}, {"status":"in_progress"}]}"`, want: 2, }, + // case_4 // 2 demanded, min at 2 { repo: "test/valid", @@ -117,6 +127,7 @@ func TestDetermineDesiredReplicas_RepositoryRunner(t *testing.T) { workflowRuns_in_progress: `{"total_count": 1, "workflow_runs":[{"status":"in_progress"}]}"`, want: 2, }, + // case_5 // 1 demanded, min at 2 { repo: "test/valid", @@ -127,6 +138,7 @@ func TestDetermineDesiredReplicas_RepositoryRunner(t *testing.T) { workflowRuns_in_progress: `{"total_count": 0, "workflow_runs":[]}"`, want: 2, }, + // case_6 // 1 demanded, min at 2 { repo: "test/valid", @@ -137,6 +149,7 @@ func TestDetermineDesiredReplicas_RepositoryRunner(t *testing.T) { workflowRuns_in_progress: `{"total_count": 1, "workflow_runs":[{"status":"in_progress"}]}"`, want: 2, }, + // case_7 // 1 demanded, min at 1 { repo: "test/valid", @@ -147,6 +160,7 @@ func TestDetermineDesiredReplicas_RepositoryRunner(t *testing.T) { workflowRuns_in_progress: `{"total_count": 0, "workflow_runs":[]}"`, want: 1, }, + // case_8 // 1 demanded, min at 1 { repo: "test/valid", @@ -157,6 +171,7 @@ func TestDetermineDesiredReplicas_RepositoryRunner(t *testing.T) { workflowRuns_in_progress: `{"total_count": 1, "workflow_runs":[{"status":"in_progress"}]}"`, want: 1, }, + // case_9 // fixed at 3 { repo: "test/valid", @@ -166,9 +181,36 @@ func TestDetermineDesiredReplicas_RepositoryRunner(t *testing.T) { workflowRuns: `{"total_count": 4, "workflow_runs":[{"status":"in_progress"}, {"status":"in_progress"}, {"status":"in_progress"}, {"status":"completed"}]}"`, workflowRuns_queued: `{"total_count": 0, "workflow_runs":[]}"`, workflowRuns_in_progress: `{"total_count": 3, "workflow_runs":[{"status":"in_progress"}, {"status":"in_progress"}, {"status":"in_progress"}]}"`, - want: 3, + want: 1, + }, + // Case for empty GitHub Actions reponse - should not trigger scale up + { + description: "GitHub Actions Jobs Array is empty - no scale up", + repo: "test/valid", + min: intPtr(0), + max: intPtr(3), + workflowRuns: `{"total_count": 2, "workflow_runs":[{"status":"queued"}, {"status":"completed"}]}"`, + workflowRuns_queued: `{"total_count": 1, "workflow_runs":[{"status":"queued"}]}"`, + workflowRuns_in_progress: `{"total_count": 0, "workflow_runs":[]}"`, + workflowJobs: map[int]string{ + 1: `{"jobs": []}`, + }, + want: 0, + }, + // Case for hosted GitHub Actions run + { + description: "Hosted GitHub Actions run - no scale up", + repo: "test/valid", + min: intPtr(0), + max: intPtr(3), + workflowRuns: `{"total_count": 2, "workflow_runs":[{"id": 1, "status":"queued"}, {"status":"completed"}]}"`, + workflowRuns_queued: `{"total_count": 1, "workflow_runs":[{"id": 1, "status":"queued"}]}"`, + workflowRuns_in_progress: `{"total_count": 0, "workflow_runs":[]}"`, + workflowJobs: map[int]string{ + 1: `{"jobs": [{"status":"queued"}]}`, + }, + want: 0, }, - { 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", @@ -422,7 +464,8 @@ func TestDetermineDesiredReplicas_OrganizationalRunner(t *testing.T) { want int err string }{ - // 3 demanded, max at 3 + // case_0 + // 0 demanded due to zero runID, min at 2 { org: "test", repos: []string{"valid"}, @@ -431,8 +474,9 @@ func TestDetermineDesiredReplicas_OrganizationalRunner(t *testing.T) { workflowRuns: `{"total_count": 4, "workflow_runs":[{"status":"queued"}, {"status":"in_progress"}, {"status":"in_progress"}, {"status":"completed"}]}"`, workflowRuns_queued: `{"total_count": 1, "workflow_runs":[{"status":"queued"}]}"`, workflowRuns_in_progress: `{"total_count": 2, "workflow_runs":[{"status":"in_progress"}, {"status":"in_progress"}]}"`, - want: 3, + want: 2, }, + // case_1 // 2 demanded, max at 3, currently 3, delay scaling down due to grace period { org: "test", @@ -446,6 +490,7 @@ func TestDetermineDesiredReplicas_OrganizationalRunner(t *testing.T) { workflowRuns_in_progress: `{"total_count": 1, "workflow_runs":[{"status":"in_progress"}]}"`, want: 3, }, + // case_2 // 3 demanded, max at 2 { org: "test", @@ -457,6 +502,7 @@ func TestDetermineDesiredReplicas_OrganizationalRunner(t *testing.T) { workflowRuns_in_progress: `{"total_count": 2, "workflow_runs":[{"status":"in_progress"}, {"status":"in_progress"}]}"`, want: 2, }, + // case_3 // 2 demanded, min at 2 { org: "test", @@ -468,6 +514,7 @@ func TestDetermineDesiredReplicas_OrganizationalRunner(t *testing.T) { workflowRuns_in_progress: `{"total_count": 1, "workflow_runs":[{"status":"in_progress"}]}"`, want: 2, }, + // case_4 // 1 demanded, min at 2 { org: "test", @@ -479,6 +526,7 @@ func TestDetermineDesiredReplicas_OrganizationalRunner(t *testing.T) { workflowRuns_in_progress: `{"total_count": 0, "workflow_runs":[]}"`, want: 2, }, + // case_5 // 1 demanded, min at 2 { org: "test", @@ -512,6 +560,7 @@ func TestDetermineDesiredReplicas_OrganizationalRunner(t *testing.T) { workflowRuns_in_progress: `{"total_count": 1, "workflow_runs":[{"status":"in_progress"}]}"`, want: 1, }, + // case_6 // fixed at 3 { org: "test", @@ -522,8 +571,9 @@ func TestDetermineDesiredReplicas_OrganizationalRunner(t *testing.T) { workflowRuns: `{"total_count": 4, "workflow_runs":[{"status":"in_progress"}, {"status":"in_progress"}, {"status":"in_progress"}, {"status":"completed"}]}"`, workflowRuns_queued: `{"total_count": 0, "workflow_runs":[]}"`, workflowRuns_in_progress: `{"total_count": 3, "workflow_runs":[{"status":"in_progress"},{"status":"in_progress"},{"status":"in_progress"}]}"`, - want: 3, + want: 1, }, + // case_7 // org runner, fixed at 3 { org: "test", @@ -534,8 +584,9 @@ func TestDetermineDesiredReplicas_OrganizationalRunner(t *testing.T) { workflowRuns: `{"total_count": 4, "workflow_runs":[{"status":"in_progress"}, {"status":"in_progress"}, {"status":"in_progress"}, {"status":"completed"}]}"`, workflowRuns_queued: `{"total_count": 0, "workflow_runs":[]}"`, workflowRuns_in_progress: `{"total_count": 3, "workflow_runs":[{"status":"in_progress"},{"status":"in_progress"},{"status":"in_progress"}]}"`, - want: 3, + want: 1, }, + // case_8 // org runner, 1 demanded, min at 1, no repos { org: "test",