From dc74c4531725f3047675b333b28d905367a25b0d Mon Sep 17 00:00:00 2001 From: Mario Constanti Date: Wed, 5 Jun 2024 12:37:20 +0200 Subject: [PATCH 1/2] fix: remove unnecessary github api call There are only a few cases, where we get a job information from github where the runner name is not set. For all this cases we do not need to check github API at all because these jobs are never ever get scheduled to a runner: job.Action is: * queued: a queued job is just queued and not scheduled to a runner so we do not get a runner name from the GH API * completed: when conclusion=cancelled|failure github never scheduled the job to a runner and with that we do not get a runner name from the GH API Signed-off-by: Mario Constanti --- runner/pool/pool.go | 82 +-------------------------------------------- 1 file changed, 1 insertion(+), 81 deletions(-) diff --git a/runner/pool/pool.go b/runner/pool/pool.go index be958876..6a7b5632 100644 --- a/runner/pool/pool.go +++ b/runner/pool/pool.go @@ -35,7 +35,6 @@ import ( "github.com/cloudbase/garm-provider-common/util" "github.com/cloudbase/garm/auth" dbCommon "github.com/cloudbase/garm/database/common" - "github.com/cloudbase/garm/metrics" "github.com/cloudbase/garm/params" "github.com/cloudbase/garm/runner/common" garmUtil "github.com/cloudbase/garm/util" @@ -892,40 +891,6 @@ func (r *basePoolManager) addInstanceToProvider(instance params.Instance) error return nil } -func (r *basePoolManager) getRunnerDetailsFromJob(job params.WorkflowJob) (params.RunnerInfo, error) { - runnerInfo := params.RunnerInfo{ - Name: job.WorkflowJob.RunnerName, - Labels: job.WorkflowJob.Labels, - } - - var err error - if job.WorkflowJob.RunnerName == "" { - if job.WorkflowJob.Conclusion == "skipped" || job.WorkflowJob.Conclusion == "cancelled" { - // job was skipped or cancelled before a runner was allocated. No point in continuing. - return params.RunnerInfo{}, fmt.Errorf("job %d was skipped or cancelled before a runner was allocated: %w", job.WorkflowJob.ID, runnerErrors.ErrNotFound) - } - // Runner name was not set in WorkflowJob by github. We can still attempt to - // fetch the info we need, using the workflow run ID, from the API. - slog.InfoContext( - r.ctx, "runner name not found in workflow job, attempting to fetch from API", - "job_id", job.WorkflowJob.ID) - runnerInfo, err = r.GetRunnerInfoFromWorkflow(job) - if err != nil { - return params.RunnerInfo{}, errors.Wrap(err, "fetching runner name from API") - } - } - - _, err = r.store.GetInstanceByName(context.Background(), runnerInfo.Name) - if err != nil { - slog.With(slog.Any("error", err)).ErrorContext( - r.ctx, "could not find runner details", - "runner_name", util.SanitizeLogEntry(runnerInfo.Name)) - return params.RunnerInfo{}, errors.Wrap(err, "fetching runner details") - } - - return runnerInfo, nil -} - // paramsWorkflowJobToParamsJob returns a params.Job from a params.WorkflowJob, and aditionally determines // if the runner belongs to this pool or not. It will always return a valid params.Job, even if it errs out. // This allows us to still update the job in the database, even if we determined that it wasn't necessarily meant @@ -955,6 +920,7 @@ func (r *basePoolManager) paramsWorkflowJobToParamsJob(job params.WorkflowJob) ( CompletedAt: job.WorkflowJob.CompletedAt, Name: job.WorkflowJob.Name, GithubRunnerID: job.WorkflowJob.RunnerID, + RunnerName: job.WorkflowJob.RunnerName, RunnerGroupID: job.WorkflowJob.RunnerGroupID, RunnerGroupName: job.WorkflowJob.RunnerGroupName, RepositoryName: job.Repository.Name, @@ -962,23 +928,6 @@ func (r *basePoolManager) paramsWorkflowJobToParamsJob(job params.WorkflowJob) ( Labels: job.WorkflowJob.Labels, } - runnerName := job.WorkflowJob.RunnerName - if job.Action != "queued" && runnerName == "" { - if job.WorkflowJob.Conclusion != "skipped" && job.WorkflowJob.Conclusion != "cancelled" { - // Runner name was not set in WorkflowJob by github. We can still attempt to fetch the info we need, - // using the workflow run ID, from the API. - // We may still get no runner name. In situations such as jobs being cancelled before a runner had the chance - // to pick up the job, the runner name is not available from the API. - runnerInfo, err := r.getRunnerDetailsFromJob(job) - if err != nil && !errors.Is(err, runnerErrors.ErrNotFound) { - return jobParams, errors.Wrap(err, "fetching runner details") - } - runnerName = runnerInfo.Name - } - } - - jobParams.RunnerName = runnerName - switch r.entity.EntityType { case params.GithubEntityTypeEnterprise: jobParams.EnterpriseID = &asUUID @@ -1968,35 +1917,6 @@ func (r *basePoolManager) ValidateOwner(job params.WorkflowJob) error { return nil } -func (r *basePoolManager) GetRunnerInfoFromWorkflow(job params.WorkflowJob) (params.RunnerInfo, error) { - if err := r.ValidateOwner(job); err != nil { - return params.RunnerInfo{}, errors.Wrap(err, "validating owner") - } - metrics.GithubOperationCount.WithLabelValues( - "GetWorkflowJobByID", // label: operation - r.entity.LabelScope(), // label: scope - ).Inc() - workflow, ghResp, err := r.ghcli.GetWorkflowJobByID(r.ctx, job.Repository.Owner.Login, job.Repository.Name, job.WorkflowJob.ID) - if err != nil { - metrics.GithubOperationFailedCount.WithLabelValues( - "GetWorkflowJobByID", // label: operation - r.entity.LabelScope(), // label: scope - ).Inc() - if ghResp != nil && ghResp.StatusCode == http.StatusUnauthorized { - return params.RunnerInfo{}, errors.Wrap(runnerErrors.ErrUnauthorized, "fetching workflow info") - } - return params.RunnerInfo{}, errors.Wrap(err, "fetching workflow info") - } - - if workflow.RunnerName != nil { - return params.RunnerInfo{ - Name: *workflow.RunnerName, - Labels: workflow.Labels, - }, nil - } - return params.RunnerInfo{}, fmt.Errorf("failed to find runner name from workflow") -} - func (r *basePoolManager) GithubRunnerRegistrationToken() (string, error) { tk, ghResp, err := r.ghcli.CreateEntityRegistrationToken(r.ctx) if err != nil { From b4e7dead1ccd84c4d341aaacf093a48eb15f2949 Mon Sep 17 00:00:00 2001 From: Mario Constanti Date: Wed, 5 Jun 2024 13:48:53 +0200 Subject: [PATCH 2/2] fix: check if runner name is empty and return Signed-off-by: Mario Constanti --- runner/pool/pool.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/runner/pool/pool.go b/runner/pool/pool.go index 6a7b5632..1261ad21 100644 --- a/runner/pool/pool.go +++ b/runner/pool/pool.go @@ -209,6 +209,13 @@ func (r *basePoolManager) HandleWorkflowJob(job params.WorkflowJob) error { return errors.Wrap(err, "converting job to params") } + // If job was not assigned to a runner, we can ignore it. + if jobParams.RunnerName == "" { + slog.InfoContext( + r.ctx, "job never got assigned to a runner, ignoring") + return nil + } + // update instance workload state. if _, err := r.setInstanceRunnerStatus(jobParams.RunnerName, params.RunnerTerminated); err != nil { if errors.Is(err, runnerErrors.ErrNotFound) {