From 3c3077a11cf5e2c72fde0d01231029c0e6d2d363 Mon Sep 17 00:00:00 2001 From: Yusuke Kuoka Date: Wed, 29 Jul 2020 21:16:48 +0900 Subject: [PATCH 1/3] Fix crash on startup after the HRDA addition This is a follow-up for #66. The reconciler for the new HorizontalRunnerDeploymentAutoscaler had a terrible flaw that prevented the controller to fail launching due to an error like: ``` indexer conflict: map[field:.metadata.controller:{}] ``` This fixes that, while adding `integration_test.go` to verify its actually fixed and prevent regression in the future. --- .../horizontalrunnerautoscaler_controller.go | 19 +- controllers/integration_test.go | 253 ++++++++++++++++++ 2 files changed, 254 insertions(+), 18 deletions(-) create mode 100644 controllers/integration_test.go diff --git a/controllers/horizontalrunnerautoscaler_controller.go b/controllers/horizontalrunnerautoscaler_controller.go index 5de8575e28..513d4f74e5 100644 --- a/controllers/horizontalrunnerautoscaler_controller.go +++ b/controllers/horizontalrunnerautoscaler_controller.go @@ -130,25 +130,8 @@ func (r *HorizontalRunnerAutoscalerReconciler) Reconcile(req ctrl.Request) (ctrl func (r *HorizontalRunnerAutoscalerReconciler) SetupWithManager(mgr ctrl.Manager) error { r.Recorder = mgr.GetEventRecorderFor("runnerdeployment-controller") - if err := mgr.GetFieldIndexer().IndexField(&v1alpha1.RunnerReplicaSet{}, runnerSetOwnerKey, func(rawObj runtime.Object) []string { - runnerSet := rawObj.(*v1alpha1.RunnerReplicaSet) - owner := metav1.GetControllerOf(runnerSet) - if owner == nil { - return nil - } - - if owner.APIVersion != v1alpha1.GroupVersion.String() || owner.Kind != "RunnerDeployment" { - return nil - } - - return []string{owner.Name} - }); err != nil { - return err - } - return ctrl.NewControllerManagedBy(mgr). - For(&v1alpha1.RunnerDeployment{}). - Owns(&v1alpha1.RunnerReplicaSet{}). + For(&v1alpha1.HorizontalRunnerAutoscaler{}). Complete(r) } diff --git a/controllers/integration_test.go b/controllers/integration_test.go new file mode 100644 index 0000000000..8d42f40518 --- /dev/null +++ b/controllers/integration_test.go @@ -0,0 +1,253 @@ +package controllers + +import ( + "context" + "github.com/summerwind/actions-runner-controller/github/fake" + "time" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/kubernetes/scheme" + ctrl "sigs.k8s.io/controller-runtime" + logf "sigs.k8s.io/controller-runtime/pkg/log" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + + actionsv1alpha1 "github.com/summerwind/actions-runner-controller/api/v1alpha1" +) + +// SetupIntegrationTest will set up a testing environment. +// This includes: +// * creating a Namespace to be used during the test +// * starting all the reconcilers +// * stopping all the reconcilers after the test ends +// Call this function at the start of each of your tests. +func SetupIntegrationTest(ctx context.Context) *corev1.Namespace { + var stopCh chan struct{} + ns := &corev1.Namespace{} + + BeforeEach(func() { + stopCh = make(chan struct{}) + *ns = corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: "testns-" + randStringRunes(5)}, + } + + err := k8sClient.Create(ctx, ns) + Expect(err).NotTo(HaveOccurred(), "failed to create test namespace") + + mgr, err := ctrl.NewManager(cfg, ctrl.Options{}) + Expect(err).NotTo(HaveOccurred(), "failed to create manager") + + replicasetController := &RunnerReplicaSetReconciler{ + Client: mgr.GetClient(), + Scheme: scheme.Scheme, + Log: logf.Log, + Recorder: mgr.GetEventRecorderFor("runnerreplicaset-controller"), + } + err = replicasetController.SetupWithManager(mgr) + Expect(err).NotTo(HaveOccurred(), "failed to setup controller") + + deploymentsController := &RunnerDeploymentReconciler{ + Client: mgr.GetClient(), + Scheme: scheme.Scheme, + Log: logf.Log, + Recorder: mgr.GetEventRecorderFor("runnerdeployment-controller"), + } + err = deploymentsController.SetupWithManager(mgr) + Expect(err).NotTo(HaveOccurred(), "failed to setup controller") + + workflowRuns := `{"total_count": 4, "workflow_runs":[{"status":"in_progress"}, {"status":"in_progress"}, {"status":"in_progress"}, {"status":"completed"}]}"` + + server := fake.NewServer(fake.WithListRepositoryWorkflowRunsResponse(200, workflowRuns)) + defer server.Close() + client := newGithubClient(server) + + autoscalerController := &HorizontalRunnerAutoscalerReconciler{ + Client: mgr.GetClient(), + Scheme: scheme.Scheme, + Log: logf.Log, + GitHubClient: client, + Recorder: mgr.GetEventRecorderFor("horizontalrunnerautoscaler-controller"), + } + err = autoscalerController.SetupWithManager(mgr) + Expect(err).NotTo(HaveOccurred(), "failed to setup controller") + + go func() { + defer GinkgoRecover() + + err := mgr.Start(stopCh) + Expect(err).NotTo(HaveOccurred(), "failed to start manager") + }() + }) + + AfterEach(func() { + close(stopCh) + + err := k8sClient.Delete(ctx, ns) + Expect(err).NotTo(HaveOccurred(), "failed to delete test namespace") + }) + + return ns +} + +var _ = Context("Inside of a new namespace", func() { + ctx := context.TODO() + ns := SetupIntegrationTest(ctx) + + Describe("when no existing resources exist", func() { + + It("should create and scale runners", func() { + name := "example-runnerdeploy" + + { + rs := &actionsv1alpha1.RunnerDeployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: ns.Name, + }, + Spec: actionsv1alpha1.RunnerDeploymentSpec{ + Replicas: intPtr(1), + Template: actionsv1alpha1.RunnerTemplate{ + Spec: actionsv1alpha1.RunnerSpec{ + Repository: "foo/bar", + Image: "bar", + Env: []corev1.EnvVar{ + {Name: "FOO", Value: "FOOVALUE"}, + }, + }, + }, + }, + } + + err := k8sClient.Create(ctx, rs) + + Expect(err).NotTo(HaveOccurred(), "failed to create test RunnerDeployment resource") + + runnerSets := actionsv1alpha1.RunnerReplicaSetList{Items: []actionsv1alpha1.RunnerReplicaSet{}} + + Eventually( + func() int { + err := k8sClient.List(ctx, &runnerSets, client.InNamespace(ns.Name)) + if err != nil { + logf.Log.Error(err, "list runner sets") + } + + return len(runnerSets.Items) + }, + time.Second*5, time.Millisecond*500).Should(BeEquivalentTo(1)) + + Eventually( + func() int { + err := k8sClient.List(ctx, &runnerSets, client.InNamespace(ns.Name)) + if err != nil { + logf.Log.Error(err, "list runner sets") + } + + if len(runnerSets.Items) == 0 { + logf.Log.Info("No runnerreplicasets exist yet") + return -1 + } + + return *runnerSets.Items[0].Spec.Replicas + }, + time.Second*5, time.Millisecond*500).Should(BeEquivalentTo(1)) + } + + { + // We wrap the update in the Eventually block to avoid the below error that occurs due to concurrent modification + // made by the controller to update .Status.AvailableReplicas and .Status.ReadyReplicas + // Operation cannot be fulfilled on runnersets.actions.summerwind.dev "example-runnerset": the object has been modified; please apply your changes to the latest version and try again + Eventually(func() error { + var rd actionsv1alpha1.RunnerDeployment + + err := k8sClient.Get(ctx, types.NamespacedName{Namespace: ns.Name, Name: name}, &rd) + + Expect(err).NotTo(HaveOccurred(), "failed to get test RunnerDeployment resource") + + rd.Spec.Replicas = intPtr(2) + + return k8sClient.Update(ctx, &rd) + }, + time.Second*1, time.Millisecond*500).Should(BeNil()) + + runnerSets := actionsv1alpha1.RunnerReplicaSetList{Items: []actionsv1alpha1.RunnerReplicaSet{}} + + Eventually( + func() int { + err := k8sClient.List(ctx, &runnerSets, client.InNamespace(ns.Name)) + if err != nil { + logf.Log.Error(err, "list runner sets") + } + + return len(runnerSets.Items) + }, + time.Second*5, time.Millisecond*500).Should(BeEquivalentTo(1)) + + Eventually( + func() int { + err := k8sClient.List(ctx, &runnerSets, client.InNamespace(ns.Name)) + if err != nil { + logf.Log.Error(err, "list runner sets") + } + + return *runnerSets.Items[0].Spec.Replicas + }, + time.Second*5, time.Millisecond*500).Should(BeEquivalentTo(2)) + } + + { + rs := &actionsv1alpha1.HorizontalRunnerAutoscaler{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: ns.Name, + }, + Spec: actionsv1alpha1.HorizontalRunnerAutoscalerSpec{ + ScaleTargetRef: actionsv1alpha1.ScaleTargetRef{ + Name: name, + }, + MinReplicas: intPtr(1), + MaxReplicas: intPtr(2), + ScaleDownDelaySecondsAfterScaleUp: nil, + Metrics: nil, + }, + } + + err := k8sClient.Create(ctx, rs) + + Expect(err).NotTo(HaveOccurred(), "failed to create test RunnerDeployment resource") + + runnerSets := actionsv1alpha1.RunnerReplicaSetList{Items: []actionsv1alpha1.RunnerReplicaSet{}} + + Eventually( + func() int { + err := k8sClient.List(ctx, &runnerSets, client.InNamespace(ns.Name)) + if err != nil { + logf.Log.Error(err, "list runner sets") + } + + return len(runnerSets.Items) + }, + time.Second*5, time.Millisecond*500).Should(BeEquivalentTo(1)) + + Eventually( + func() int { + err := k8sClient.List(ctx, &runnerSets, client.InNamespace(ns.Name)) + if err != nil { + logf.Log.Error(err, "list runner sets") + } + + if len(runnerSets.Items) == 0 { + logf.Log.Info("No runnerreplicasets exist yet") + return -1 + } + + return *runnerSets.Items[0].Spec.Replicas + }, + time.Second*5, time.Millisecond*500).Should(BeEquivalentTo(1)) + } + }) + }) +}) From e2164f9946e380563589509eba462b21d24ec27e Mon Sep 17 00:00:00 2001 From: Yusuke Kuoka Date: Sun, 2 Aug 2020 10:34:58 +0900 Subject: [PATCH 2/3] Fix integration test bugs and do verify scaling out --- controllers/integration_test.go | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/controllers/integration_test.go b/controllers/integration_test.go index 8d42f40518..e0dc6102bd 100644 --- a/controllers/integration_test.go +++ b/controllers/integration_test.go @@ -29,6 +29,9 @@ func SetupIntegrationTest(ctx context.Context) *corev1.Namespace { var stopCh chan struct{} ns := &corev1.Namespace{} + workflowRuns := `{"total_count": 5, "workflow_runs":[{"status":"queued"}, {"status":"queued"}, {"status":"in_progress"}, {"status":"in_progress"}, {"status":"completed"}]}"` + server := fake.NewServer(fake.WithListRepositoryWorkflowRunsResponse(200, workflowRuns)) + BeforeEach(func() { stopCh = make(chan struct{}) *ns = corev1.Namespace{ @@ -59,10 +62,6 @@ func SetupIntegrationTest(ctx context.Context) *corev1.Namespace { err = deploymentsController.SetupWithManager(mgr) Expect(err).NotTo(HaveOccurred(), "failed to setup controller") - workflowRuns := `{"total_count": 4, "workflow_runs":[{"status":"in_progress"}, {"status":"in_progress"}, {"status":"in_progress"}, {"status":"completed"}]}"` - - server := fake.NewServer(fake.WithListRepositoryWorkflowRunsResponse(200, workflowRuns)) - defer server.Close() client := newGithubClient(server) autoscalerController := &HorizontalRunnerAutoscalerReconciler{ @@ -86,6 +85,8 @@ func SetupIntegrationTest(ctx context.Context) *corev1.Namespace { AfterEach(func() { close(stopCh) + server.Close() + err := k8sClient.Delete(ctx, ns) Expect(err).NotTo(HaveOccurred(), "failed to delete test namespace") }) @@ -112,7 +113,7 @@ var _ = Context("Inside of a new namespace", func() { Replicas: intPtr(1), Template: actionsv1alpha1.RunnerTemplate{ Spec: actionsv1alpha1.RunnerSpec{ - Repository: "foo/bar", + Repository: "test/valid", Image: "bar", Env: []corev1.EnvVar{ {Name: "FOO", Value: "FOOVALUE"}, @@ -209,7 +210,7 @@ var _ = Context("Inside of a new namespace", func() { Name: name, }, MinReplicas: intPtr(1), - MaxReplicas: intPtr(2), + MaxReplicas: intPtr(3), ScaleDownDelaySecondsAfterScaleUp: nil, Metrics: nil, }, @@ -246,7 +247,7 @@ var _ = Context("Inside of a new namespace", func() { return *runnerSets.Items[0].Spec.Replicas }, - time.Second*5, time.Millisecond*500).Should(BeEquivalentTo(1)) + time.Second*5, time.Millisecond*500).Should(BeEquivalentTo(3)) } }) }) From 50487bbb5425520aba5444348a5a041e6c39a72f Mon Sep 17 00:00:00 2001 From: Yusuke Kuoka Date: Sun, 2 Aug 2020 10:38:15 +0900 Subject: [PATCH 3/3] Fix the HRA controller name --- controllers/horizontalrunnerautoscaler_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/horizontalrunnerautoscaler_controller.go b/controllers/horizontalrunnerautoscaler_controller.go index 513d4f74e5..b4c206d55f 100644 --- a/controllers/horizontalrunnerautoscaler_controller.go +++ b/controllers/horizontalrunnerautoscaler_controller.go @@ -128,7 +128,7 @@ func (r *HorizontalRunnerAutoscalerReconciler) Reconcile(req ctrl.Request) (ctrl } func (r *HorizontalRunnerAutoscalerReconciler) SetupWithManager(mgr ctrl.Manager) error { - r.Recorder = mgr.GetEventRecorderFor("runnerdeployment-controller") + r.Recorder = mgr.GetEventRecorderFor("horizontalrunnerautoscaler-controller") return ctrl.NewControllerManagedBy(mgr). For(&v1alpha1.HorizontalRunnerAutoscaler{}).