From ae0ce62dc99d233c589a8c61fc0bfd9ce03db08c Mon Sep 17 00:00:00 2001 From: Lukas Hybner Date: Mon, 3 Oct 2022 14:50:02 +0200 Subject: [PATCH 1/3] fix reconcilation when pods was deleted When pods are deleted then podRunnerPairs should be reloaded, so it's better to requeue reconcilation immediately. When new pods are scale up then it takes some time usually a few seconds to sync between pods and runner API and so there is no need to wait whole period (default 1m) to sync. Signed-off-by: Lukas Hybner --- controllers/githubactionrunner_controller.go | 23 +++++++++++++------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/controllers/githubactionrunner_controller.go b/controllers/githubactionrunner_controller.go index c00cb025..b7cc3758 100644 --- a/controllers/githubactionrunner_controller.go +++ b/controllers/githubactionrunner_controller.go @@ -116,13 +116,18 @@ func (r *GithubActionRunnerReconciler) handleScaling(ctx context.Context, instan // safety guard - always look for finalizers in order to unregister runners for pods about to delete // pods could have been deleted by user directly and not through operator - if err = r.handleFinalization(ctx, instance, podRunnerPairs); err != nil { + // if some pods are deleted then it's better to requeue immediately, because we have to reload podRunnerPairs. + numDeleted, err := r.handleFinalization(ctx, instance, podRunnerPairs) + if err != nil { return r.manageOutcome(ctx, instance, err) } + if numDeleted > 0 { + return r.ManageSuccess(ctx, instance) + } if !podRunnerPairs.inSync() { - logger.Info("Pods and runner API not in sync, returning early") - return r.manageOutcome(ctx, instance, nil) + logger.Info("Pods and runner API not in sync, returning early", "Pods", podRunnerPairs.numPods(), "Runners", podRunnerPairs.numRunners()) + return r.ManageSuccessWithRequeue(ctx, instance, 3*time.Second) } if shouldScaleUp(podRunnerPairs, instance) { @@ -362,29 +367,31 @@ func (r *GithubActionRunnerReconciler) unregisterRunner(ctx context.Context, cr } // handleFinalization will remove runner from github based on presence of finalizer -func (r *GithubActionRunnerReconciler) handleFinalization(ctx context.Context, cr *garov1alpha1.GithubActionRunner, list podRunnerPairList) error { +func (r *GithubActionRunnerReconciler) handleFinalization(ctx context.Context, cr *garov1alpha1.GithubActionRunner, list podRunnerPairList) (int, error) { + numDeleted := 0 for _, item := range list.getPodsBeingDeletedOrEvictedOrCompleted() { + numDeleted++ // TODO - cause of failure should be checked more closely, if it does not exist we can ignore it. If it is a comms error we should stick around if err := r.unregisterRunner(ctx, cr, item); err != nil { - return err + return numDeleted, err } if isEvicted(&item.pod) && env.GetBoolDefault(deleteEvictedPodsEnvVarName, true) { logr.FromContextOrDiscard(ctx).Info("Deleting evicted pod", "podname", item.pod.Name) err := r.DeleteResourceIfExists(ctx, &item.pod) if err != nil { - return err + return numDeleted, err } } if isCompleted(&item.pod) { logr.FromContextOrDiscard(ctx).Info("Deleting succeeded pod", "podname", item.pod.Name) err := r.DeleteResourceIfExists(ctx, &item.pod) if err != nil { - return err + return numDeleted, err } } } - return nil + return numDeleted, nil } // tokenForRef returns the token referenced from the GithubActionRunner Spec.TokenRef From 13c5d220b202b5a87d2c5d8286fabfcbcbb7757c Mon Sep 17 00:00:00 2001 From: Lukas Hybner Date: Mon, 3 Oct 2022 15:23:07 +0200 Subject: [PATCH 2/3] Better scale up and down runners We use ephemeral runners which shutdown after pipeline ends and after this new runners starts. So I change behaviour to scale up new runners when idle runners change to busy. This is change meaning minRunners property to mininum idle Runners. This change respect maxRunners property. Signed-off-by: Lukas Hybner --- controllers/githubactionrunner_controller.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/controllers/githubactionrunner_controller.go b/controllers/githubactionrunner_controller.go index b7cc3758..8e7b409d 100644 --- a/controllers/githubactionrunner_controller.go +++ b/controllers/githubactionrunner_controller.go @@ -132,7 +132,8 @@ func (r *GithubActionRunnerReconciler) handleScaling(ctx context.Context, instan if shouldScaleUp(podRunnerPairs, instance) { instance.Status.CurrentSize = podRunnerPairs.numPods() - scale := funk.MaxInt([]int{instance.Spec.MinRunners - podRunnerPairs.numRunners(), 1}) + scale := funk.MaxInt([]int{instance.Spec.MinRunners - podRunnerPairs.numRunners(), instance.Spec.MinRunners - podRunnerPairs.numIdle(), 1}) + scale = funk.MinInt([]int{scale, instance.Spec.MaxRunners - podRunnerPairs.numRunners()}) logger.Info("Scaling up", "numInstances", scale) if err := r.scaleUp(ctx, scale, instance); err != nil { @@ -178,11 +179,11 @@ func (r *GithubActionRunnerReconciler) scaleDown(ctx context.Context, podRunnerP } func shouldScaleUp(podRunnerPairs podRunnerPairList, instance *garov1alpha1.GithubActionRunner) bool { - return podRunnerPairs.numRunners() < instance.Spec.MinRunners || (podRunnerPairs.allBusy() && podRunnerPairs.numRunners() < instance.Spec.MaxRunners) + return podRunnerPairs.numIdle() < instance.Spec.MinRunners && podRunnerPairs.numRunners() < instance.Spec.MaxRunners } func shouldScaleDown(podRunnerPairs podRunnerPairList, instance *garov1alpha1.GithubActionRunner) bool { - return podRunnerPairs.numRunners() > instance.Spec.MaxRunners || (podRunnerPairs.numIdle() > 1 && (podRunnerPairs.numRunners() > instance.Spec.MinRunners)) + return podRunnerPairs.numRunners() > instance.Spec.MaxRunners || podRunnerPairs.numIdle() > instance.Spec.MinRunners } func (r *GithubActionRunnerReconciler) manageOutcome(ctx context.Context, instance *garov1alpha1.GithubActionRunner, issue error) (reconcile.Result, error) { From 5f7beaeb23bee9883d17745d7c1503ba13f61ab7 Mon Sep 17 00:00:00 2001 From: Lukas Hybner Date: Mon, 3 Oct 2022 15:36:04 +0200 Subject: [PATCH 3/3] Logging GitHub API Rate Limit Signed-off-by: Lukas Hybner --- controllers/githubapi/runnerapi.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/controllers/githubapi/runnerapi.go b/controllers/githubapi/runnerapi.go index 3c462b49..76063e74 100644 --- a/controllers/githubapi/runnerapi.go +++ b/controllers/githubapi/runnerapi.go @@ -2,6 +2,7 @@ package githubapi import ( "context" + "github.com/go-logr/logr" "github.com/google/go-github/v47/github" "github.com/gregjones/httpcache" "github.com/palantir/go-githubapp/githubapp" @@ -57,6 +58,7 @@ func (r runnerAPI) getClient(ctx context.Context, organization string, token str // Return all runners for the org func (r runnerAPI) GetRunners(ctx context.Context, organization string, repository string, token string) ([]*github.Runner, error) { + logger := logr.FromContextOrDiscard(ctx) client, err := r.getClient(ctx, organization, token) if err != nil { return nil, err @@ -78,6 +80,7 @@ func (r runnerAPI) GetRunners(ctx context.Context, organization string, reposito if err != nil { return allRunners, err } + logger.Info("GerRunners GitHub Api Rate limit", "Rate", response.Rate) allRunners = append(allRunners, runners.Runners...) if response.NextPage == 0 { break