From 4b00df6e6ec5fcf3739b957f86e5f0a9be31ea45 Mon Sep 17 00:00:00 2001 From: Feny Mehta Date: Wed, 10 Apr 2024 16:57:11 +0530 Subject: [PATCH 01/12] KUBESAW-12: Convert the health-check goroutine into ToolchainCluster controller Signed-off-by: Feny Mehta --- .../healthchecker.go | 62 +----------- ...toolchaincluster_healthcheck_controller.go | 95 +++++++++++++++++++ ...aincluster_healthcheck_controller_test.go} | 76 +++++++++++---- 3 files changed, 154 insertions(+), 79 deletions(-) rename controllers/{toolchainclustercache => toolchainclusterhealth}/healthchecker.go (64%) create mode 100644 controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller.go rename controllers/{toolchainclustercache/healthchecker_test.go => toolchainclusterhealth/toolchaincluster_healthcheck_controller_test.go} (69%) diff --git a/controllers/toolchainclustercache/healthchecker.go b/controllers/toolchainclusterhealth/healthchecker.go similarity index 64% rename from controllers/toolchainclustercache/healthchecker.go rename to controllers/toolchainclusterhealth/healthchecker.go index bd1dab3c..b6d40fdf 100644 --- a/controllers/toolchainclustercache/healthchecker.go +++ b/controllers/toolchainclusterhealth/healthchecker.go @@ -1,26 +1,18 @@ -package toolchainclustercache +package toolchainclusterhealth import ( "context" - "fmt" "strings" - "time" toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" - "github.com/codeready-toolchain/toolchain-common/pkg/cluster" "github.com/go-logr/logr" "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/util/wait" kubeclientset "k8s.io/client-go/kubernetes" "sigs.k8s.io/controller-runtime/pkg/client" - logf "sigs.k8s.io/controller-runtime/pkg/log" - "sigs.k8s.io/controller-runtime/pkg/manager" ) -var logger = logf.Log.WithName("toolchaincluster_healthcheck") - const ( healthzOk = "/healthz responded with ok" healthzNotOk = "/healthz responded without ok" @@ -28,13 +20,6 @@ const ( clusterReachableMsg = "cluster is reachable" ) -func StartHealthChecks(ctx context.Context, mgr manager.Manager, namespace string, period time.Duration) { - logger.Info("starting health checks", "period", period) - go wait.Until(func() { - updateClusterStatuses(ctx, namespace, mgr.GetClient()) - }, period, ctx.Done()) -} - type HealthChecker struct { localClusterClient client.Client remoteClusterClient client.Client @@ -42,51 +27,6 @@ type HealthChecker struct { logger logr.Logger } -// updateClusterStatuses checks cluster health and updates status of all ToolchainClusters -func updateClusterStatuses(ctx context.Context, namespace string, cl client.Client) { - clusters := &toolchainv1alpha1.ToolchainClusterList{} - err := cl.List(ctx, clusters, client.InNamespace(namespace)) - if err != nil { - logger.Error(err, "unable to list existing ToolchainClusters") - return - } - if len(clusters.Items) == 0 { - logger.Info("no ToolchainCluster found") - } - - for _, obj := range clusters.Items { - clusterObj := obj.DeepCopy() - clusterLogger := logger.WithValues("cluster-name", clusterObj.Name) - - cachedCluster, ok := cluster.GetCachedToolchainCluster(clusterObj.Name) - if !ok { - clusterLogger.Error(fmt.Errorf("cluster %s not found in cache", clusterObj.Name), "failed to retrieve stored data for cluster") - clusterObj.Status.Conditions = []toolchainv1alpha1.ToolchainClusterCondition{clusterOfflineCondition()} - if err := cl.Status().Update(ctx, clusterObj); err != nil { - clusterLogger.Error(err, "failed to update the status of ToolchainCluster") - } - continue - } - - clientSet, err := kubeclientset.NewForConfig(cachedCluster.RestConfig) - if err != nil { - clusterLogger.Error(err, "cannot create ClientSet for a ToolchainCluster") - continue - } - - healthChecker := &HealthChecker{ - localClusterClient: cl, - remoteClusterClient: cachedCluster.Client, - remoteClusterClientset: clientSet, - logger: clusterLogger, - } - // clusterLogger.Info("getting the current state of ToolchainCluster") - if err := healthChecker.updateIndividualClusterStatus(ctx, clusterObj); err != nil { - clusterLogger.Error(err, "unable to update cluster status of ToolchainCluster") - } - } -} - func (hc *HealthChecker) updateIndividualClusterStatus(ctx context.Context, toolchainCluster *toolchainv1alpha1.ToolchainCluster) error { currentClusterStatus := hc.getClusterHealthStatus(ctx) diff --git a/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller.go b/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller.go new file mode 100644 index 00000000..dcbaf77b --- /dev/null +++ b/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller.go @@ -0,0 +1,95 @@ +package toolchainclusterhealth + +import ( + "context" + "fmt" + "time" + + toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" + "github.com/codeready-toolchain/toolchain-common/pkg/cluster" + "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime" + kubeclientset "k8s.io/client-go/kubernetes" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/manager" + "sigs.k8s.io/controller-runtime/pkg/reconcile" +) + +// NewReconciler returns a new Reconciler +func NewReconciler(mgr manager.Manager, namespace string, timeout time.Duration, requeAfter time.Duration) *Reconciler { + cacheLog := log.Log.WithName("toolchaincluster_health") + clusterCacheService := cluster.NewToolchainClusterService(mgr.GetClient(), cacheLog, namespace, timeout) + return &Reconciler{ + client: mgr.GetClient(), + scheme: mgr.GetScheme(), + clusterCacheService: clusterCacheService, + requeAfter: requeAfter, + } +} + +// SetupWithManager sets up the controller with the Manager. +func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { + return ctrl.NewControllerManagedBy(mgr). + For(&toolchainv1alpha1.ToolchainCluster{}). + Complete(r) +} + +// Reconciler reconciles a ToolchainCluster object +type Reconciler struct { + client client.Client + scheme *runtime.Scheme + clusterCacheService cluster.ToolchainClusterService + requeAfter time.Duration +} + +// Reconcile reads that state of the cluster for a ToolchainCluster object and makes changes based on the state read +// and what is in the ToolchainCluster.Spec. It updates the status of the individual cluster +// Note: +// The Controller will requeue the Request to be processed again if the returned error is non-nil or +// Result.Requeue is true, otherwise upon completion it will remove the work from the queue. +func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.Result, error) { + reqLogger := log.FromContext(ctx).WithName("health") + reqLogger.Info("Reconciling ToolchainCluster") + + // Fetch the ToolchainCluster instance + toolchainCluster := &toolchainv1alpha1.ToolchainCluster{} + err := r.client.Get(ctx, request.NamespacedName, toolchainCluster) + if err != nil { + if errors.IsNotFound(err) { + // Stop monitoring the toolchain cluster as it is deleted + return reconcile.Result{}, nil + } + // Error reading the object - requeue the request. + return reconcile.Result{}, err + } + + cachedCluster, ok := cluster.GetCachedToolchainCluster(toolchainCluster.Name) + if !ok { + err := fmt.Errorf("cluster %s not found in cache", toolchainCluster.Name) + reqLogger.Error(err, "failed to retrieve stored data for cluster") + return reconcile.Result{}, err + } + + clientSet, err := kubeclientset.NewForConfig(cachedCluster.RestConfig) + if err != nil { + reqLogger.Error(err, "cannot create ClientSet for a ToolchainCluster") + return reconcile.Result{}, err + } + + healthChecker := &HealthChecker{ + localClusterClient: r.client, + remoteClusterClient: cachedCluster.Client, + remoteClusterClientset: clientSet, + logger: reqLogger, + } + + //update the status of the individual cluster. + if err := healthChecker.updateIndividualClusterStatus(ctx, toolchainCluster); err != nil { + reqLogger.Error(err, "unable to update cluster status of ToolchainCluster") + return reconcile.Result{}, err + } + + return reconcile.Result{RequeueAfter: r.requeAfter}, nil +} diff --git a/controllers/toolchainclustercache/healthchecker_test.go b/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller_test.go similarity index 69% rename from controllers/toolchainclustercache/healthchecker_test.go rename to controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller_test.go index 422e19db..a07268b8 100644 --- a/controllers/toolchainclustercache/healthchecker_test.go +++ b/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller_test.go @@ -1,8 +1,9 @@ -package toolchainclustercache +package toolchainclusterhealth import ( "context" "testing" + "time" toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" "github.com/codeready-toolchain/toolchain-common/pkg/cluster" @@ -11,9 +12,11 @@ import ( "github.com/stretchr/testify/require" "gopkg.in/h2non/gock.v1" corev1 "k8s.io/api/core/v1" + "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/reconcile" ) func TestClusterHealthChecks(t *testing.T) { @@ -37,37 +40,38 @@ func TestClusterHealthChecks(t *testing.T) { t.Run("ToolchainCluster.status doesn't contain any conditions", func(t *testing.T) { unstable, sec := newToolchainCluster("unstable", tcNs, "http://unstable.com", toolchainv1alpha1.ToolchainClusterStatus{}) - notFound, _ := newToolchainCluster("not-found", tcNs, "http://not-found.com", toolchainv1alpha1.ToolchainClusterStatus{}) - stable, _ := newToolchainCluster("stable", tcNs, "http://cluster.com", toolchainv1alpha1.ToolchainClusterStatus{}) - cl := test.NewFakeClient(t, unstable, notFound, stable, sec) - reset := setupCachedClusters(t, cl, unstable, notFound, stable) + cl := test.NewFakeClient(t, unstable, sec) + reset := setupCachedClusters(t, cl, unstable) defer reset() + service := newToolchainClusterService(t, cl, false) + // given + controller, req := prepareReconcile(unstable, cl, service, 10*time.Second) // when - updateClusterStatuses(context.TODO(), "test-namespace", cl) + _, err := controller.Reconcile(context.TODO(), req) // then + require.NoError(t, err) assertClusterStatus(t, cl, "unstable", notOffline(), unhealthy()) - assertClusterStatus(t, cl, "not-found", offline()) - assertClusterStatus(t, cl, "stable", healthy()) + }) t.Run("ToolchainCluster.status already contains conditions", func(t *testing.T) { unstable, sec := newToolchainCluster("unstable", tcNs, "http://unstable.com", withStatus(healthy())) - notFound, _ := newToolchainCluster("not-found", tcNs, "http://not-found.com", withStatus(notOffline(), unhealthy())) - stable, _ := newToolchainCluster("stable", tcNs, "http://cluster.com", withStatus(offline())) - cl := test.NewFakeClient(t, unstable, notFound, stable, sec) - resetCache := setupCachedClusters(t, cl, unstable, notFound, stable) + cl := test.NewFakeClient(t, unstable, sec) + resetCache := setupCachedClusters(t, cl, unstable) defer resetCache() + service := newToolchainClusterService(t, cl, false) + // given + controller, req := prepareReconcile(unstable, cl, service, 10*time.Second) // when - updateClusterStatuses(context.TODO(), "test-namespace", cl) + _, err := controller.Reconcile(context.TODO(), req) // then + require.NoError(t, err) assertClusterStatus(t, cl, "unstable", notOffline(), unhealthy()) - assertClusterStatus(t, cl, "not-found", offline()) - assertClusterStatus(t, cl, "stable", healthy()) }) t.Run("if no zones nor region is retrieved, then keep the current", func(t *testing.T) { @@ -75,23 +79,31 @@ func TestClusterHealthChecks(t *testing.T) { cl := test.NewFakeClient(t, stable, sec) resetCache := setupCachedClusters(t, cl, stable) defer resetCache() + service := newToolchainClusterService(t, cl, false) + // given + controller, req := prepareReconcile(stable, cl, service, 10*time.Second) // when - updateClusterStatuses(context.TODO(), "test-namespace", cl) + _, err := controller.Reconcile(context.TODO(), req) // then + require.NoError(t, err) assertClusterStatus(t, cl, "stable", healthy()) }) t.Run("if the connection cannot be established at beginning, then it should be offline", func(t *testing.T) { - stable, sec := newToolchainCluster("failing", tcNs, "http://failing.com", toolchainv1alpha1.ToolchainClusterStatus{}) + stable, sec := newToolchainCluster("failing", tcNs, "http://failing.com", toolchainv1alpha1.ToolchainClusterStatus{}) cl := test.NewFakeClient(t, stable, sec) + service := newToolchainClusterService(t, cl, false) + // given + controller, req := prepareReconcile(stable, cl, service, 10*time.Second) // when - updateClusterStatuses(context.TODO(), "test-namespace", cl) + _, err := controller.Reconcile(context.TODO(), req) // then + require.NoError(t, err) assertClusterStatus(t, cl, "failing", offline()) }) } @@ -127,6 +139,21 @@ func newToolchainCluster(name, tcNs string, apiEndpoint string, status toolchain return toolchainCluster, secret } +func newToolchainClusterService(t *testing.T, cl client.Client, withCA bool) cluster.ToolchainClusterService { + return cluster.NewToolchainClusterServiceWithClient(cl, logf.Log, "test-namespace", 3*time.Second, func(config *rest.Config, options client.Options) (client.Client, error) { + if withCA { + assert.False(t, config.Insecure) + assert.Equal(t, []byte("dummy"), config.CAData) + } else { + assert.True(t, config.Insecure) + } + // make sure that insecure is false to make Gock mocking working properly + config.Insecure = false + // reset the dummy certificate + config.CAData = []byte("") + return client.New(config, options) + }) +} func assertClusterStatus(t *testing.T, cl client.Client, clusterName string, clusterConds ...toolchainv1alpha1.ToolchainClusterCondition) { tc := &toolchainv1alpha1.ToolchainCluster{} err := cl.Get(context.TODO(), test.NamespacedName("test-namespace", clusterName), tc) @@ -146,6 +173,19 @@ ExpConditions: } } +func prepareReconcile(toolchainCluster *toolchainv1alpha1.ToolchainCluster, cl *test.FakeClient, service cluster.ToolchainClusterService, requeAfter time.Duration) (Reconciler, reconcile.Request) { + controller := Reconciler{ + client: cl, + scheme: scheme.Scheme, + clusterCacheService: service, + requeAfter: requeAfter, + } + req := reconcile.Request{ + NamespacedName: test.NamespacedName(toolchainCluster.Namespace, toolchainCluster.Name), + } + return controller, req +} + func healthy() toolchainv1alpha1.ToolchainClusterCondition { return toolchainv1alpha1.ToolchainClusterCondition{ Type: toolchainv1alpha1.ToolchainClusterReady, From 84525e0f2bee80c17e6f9ec6aa967235dcee32b9 Mon Sep 17 00:00:00 2001 From: Feny Mehta Date: Thu, 11 Apr 2024 12:13:15 +0530 Subject: [PATCH 02/12] Fixing go-lint Signed-off-by: Feny Mehta --- ...chaincluster_healthcheck_controller_test.go | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller_test.go b/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller_test.go index a07268b8..780a8adc 100644 --- a/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller_test.go +++ b/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller_test.go @@ -38,15 +38,17 @@ func TestClusterHealthChecks(t *testing.T) { Persist(). Reply(404) + requeAfter := 10 * time.Second + withCA := false t.Run("ToolchainCluster.status doesn't contain any conditions", func(t *testing.T) { unstable, sec := newToolchainCluster("unstable", tcNs, "http://unstable.com", toolchainv1alpha1.ToolchainClusterStatus{}) cl := test.NewFakeClient(t, unstable, sec) reset := setupCachedClusters(t, cl, unstable) defer reset() - service := newToolchainClusterService(t, cl, false) + service := newToolchainClusterService(t, cl, withCA) // given - controller, req := prepareReconcile(unstable, cl, service, 10*time.Second) + controller, req := prepareReconcile(unstable, cl, service, requeAfter) // when _, err := controller.Reconcile(context.TODO(), req) @@ -62,9 +64,9 @@ func TestClusterHealthChecks(t *testing.T) { cl := test.NewFakeClient(t, unstable, sec) resetCache := setupCachedClusters(t, cl, unstable) defer resetCache() - service := newToolchainClusterService(t, cl, false) + service := newToolchainClusterService(t, cl, withCA) // given - controller, req := prepareReconcile(unstable, cl, service, 10*time.Second) + controller, req := prepareReconcile(unstable, cl, service, requeAfter) // when _, err := controller.Reconcile(context.TODO(), req) @@ -79,9 +81,9 @@ func TestClusterHealthChecks(t *testing.T) { cl := test.NewFakeClient(t, stable, sec) resetCache := setupCachedClusters(t, cl, stable) defer resetCache() - service := newToolchainClusterService(t, cl, false) + service := newToolchainClusterService(t, cl, withCA) // given - controller, req := prepareReconcile(stable, cl, service, 10*time.Second) + controller, req := prepareReconcile(stable, cl, service, requeAfter) // when _, err := controller.Reconcile(context.TODO(), req) @@ -95,9 +97,9 @@ func TestClusterHealthChecks(t *testing.T) { stable, sec := newToolchainCluster("failing", tcNs, "http://failing.com", toolchainv1alpha1.ToolchainClusterStatus{}) cl := test.NewFakeClient(t, stable, sec) - service := newToolchainClusterService(t, cl, false) + service := newToolchainClusterService(t, cl, withCA) // given - controller, req := prepareReconcile(stable, cl, service, 10*time.Second) + controller, req := prepareReconcile(stable, cl, service, requeAfter) // when _, err := controller.Reconcile(context.TODO(), req) From eb16703f93a405954813350ef98851a4eac15c2c Mon Sep 17 00:00:00 2001 From: Feny Mehta Date: Thu, 11 Apr 2024 16:51:44 +0530 Subject: [PATCH 03/12] Adding some test cases Signed-off-by: Feny Mehta --- ...toolchaincluster_healthcheck_controller.go | 8 ++-- ...haincluster_healthcheck_controller_test.go | 46 ++++++++++++++++++- 2 files changed, 50 insertions(+), 4 deletions(-) diff --git a/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller.go b/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller.go index dcbaf77b..88f2e2a7 100644 --- a/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller.go +++ b/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller.go @@ -64,17 +64,19 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. // Error reading the object - requeue the request. return reconcile.Result{}, err } - + clusterObj := toolchainCluster.DeepCopy() cachedCluster, ok := cluster.GetCachedToolchainCluster(toolchainCluster.Name) if !ok { err := fmt.Errorf("cluster %s not found in cache", toolchainCluster.Name) - reqLogger.Error(err, "failed to retrieve stored data for cluster") + clusterObj.Status.Conditions = []toolchainv1alpha1.ToolchainClusterCondition{clusterOfflineCondition()} + if err := r.client.Status().Update(ctx, clusterObj); err != nil { + reqLogger.Error(err, "failed to update the status of ToolchainCluster") + } return reconcile.Result{}, err } clientSet, err := kubeclientset.NewForConfig(cachedCluster.RestConfig) if err != nil { - reqLogger.Error(err, "cannot create ClientSet for a ToolchainCluster") return reconcile.Result{}, err } diff --git a/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller_test.go b/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller_test.go index 780a8adc..93182220 100644 --- a/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller_test.go +++ b/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller_test.go @@ -2,6 +2,7 @@ package toolchainclusterhealth import ( "context" + "fmt" "testing" "time" @@ -15,6 +16,7 @@ import ( "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" + runtimeclient "sigs.k8s.io/controller-runtime/pkg/client" logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/reconcile" ) @@ -40,9 +42,51 @@ func TestClusterHealthChecks(t *testing.T) { requeAfter := 10 * time.Second withCA := false - t.Run("ToolchainCluster.status doesn't contain any conditions", func(t *testing.T) { + + t.Run("ToolchainCluster not found", func(t *testing.T) { + unstable, sec := newToolchainCluster("unstable", tcNs, "http://unstable.com", toolchainv1alpha1.ToolchainClusterStatus{}) + + cl := test.NewFakeClient(t, sec) + reset := setupCachedClusters(t, cl, unstable) + defer reset() + service := newToolchainClusterService(t, cl, withCA) + // given + controller, req := prepareReconcile(unstable, cl, service, requeAfter) + + // when + _, err := controller.Reconcile(context.TODO(), req) + + // then + require.NoError(t, err) + + }) + + t.Run("Error while getting ToolchainCluster", func(t *testing.T) { unstable, sec := newToolchainCluster("unstable", tcNs, "http://unstable.com", toolchainv1alpha1.ToolchainClusterStatus{}) + cl := test.NewFakeClient(t, sec) + + cl.MockGet = func(ctx context.Context, key runtimeclient.ObjectKey, obj runtimeclient.Object, opts ...runtimeclient.GetOption) error { + if _, ok := obj.(*toolchainv1alpha1.ToolchainCluster); ok { + return fmt.Errorf("mock error") + } + return cl.Client.Get(ctx, key, obj, opts...) + } + + service := newToolchainClusterService(t, cl, withCA) + // given + controller, req := prepareReconcile(unstable, cl, service, requeAfter) + + // when + _, err := controller.Reconcile(context.TODO(), req) + + // then + require.EqualError(t, err, "mock error") + + }) + + t.Run("ToolchainCluster.status doesn't contain any conditions", func(t *testing.T) { + unstable, sec := newToolchainCluster("unstable", tcNs, "http://unstable.com", toolchainv1alpha1.ToolchainClusterStatus{}) cl := test.NewFakeClient(t, unstable, sec) reset := setupCachedClusters(t, cl, unstable) defer reset() From aec2edfa0b4b1d229851b15c6c53e88147558fbf Mon Sep 17 00:00:00 2001 From: Feny Mehta Date: Fri, 12 Apr 2024 17:46:48 +0530 Subject: [PATCH 04/12] adding the test cases as previous Signed-off-by: Feny Mehta --- ...haincluster_healthcheck_controller_test.go | 114 +++++++++++------- 1 file changed, 73 insertions(+), 41 deletions(-) diff --git a/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller_test.go b/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller_test.go index 93182220..2fc90d12 100644 --- a/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller_test.go +++ b/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller_test.go @@ -84,42 +84,21 @@ func TestClusterHealthChecks(t *testing.T) { require.EqualError(t, err, "mock error") }) + t.Run("if the connection cannot be established at beginning, then it should be offline", func(t *testing.T) { - t.Run("ToolchainCluster.status doesn't contain any conditions", func(t *testing.T) { - unstable, sec := newToolchainCluster("unstable", tcNs, "http://unstable.com", toolchainv1alpha1.ToolchainClusterStatus{}) - cl := test.NewFakeClient(t, unstable, sec) - reset := setupCachedClusters(t, cl, unstable) - defer reset() - service := newToolchainClusterService(t, cl, withCA) - // given - controller, req := prepareReconcile(unstable, cl, service, requeAfter) - - // when - _, err := controller.Reconcile(context.TODO(), req) - - // then - require.NoError(t, err) - assertClusterStatus(t, cl, "unstable", notOffline(), unhealthy()) - - }) - - t.Run("ToolchainCluster.status already contains conditions", func(t *testing.T) { - unstable, sec := newToolchainCluster("unstable", tcNs, "http://unstable.com", withStatus(healthy())) - cl := test.NewFakeClient(t, unstable, sec) - resetCache := setupCachedClusters(t, cl, unstable) - defer resetCache() + stable, sec := newToolchainCluster("failing", tcNs, "http://failing.com", toolchainv1alpha1.ToolchainClusterStatus{}) + cl := test.NewFakeClient(t, stable, sec) service := newToolchainClusterService(t, cl, withCA) // given - controller, req := prepareReconcile(unstable, cl, service, requeAfter) + controller, req := prepareReconcile(stable, cl, service, requeAfter) // when _, err := controller.Reconcile(context.TODO(), req) // then require.NoError(t, err) - assertClusterStatus(t, cl, "unstable", notOffline(), unhealthy()) + assertClusterStatus(t, cl, "failing", offline()) }) - t.Run("if no zones nor region is retrieved, then keep the current", func(t *testing.T) { stable, sec := newToolchainCluster("stable", tcNs, "http://cluster.com", withStatus(offline())) cl := test.NewFakeClient(t, stable, sec) @@ -137,21 +116,74 @@ func TestClusterHealthChecks(t *testing.T) { assertClusterStatus(t, cl, "stable", healthy()) }) - t.Run("if the connection cannot be established at beginning, then it should be offline", func(t *testing.T) { - - stable, sec := newToolchainCluster("failing", tcNs, "http://failing.com", toolchainv1alpha1.ToolchainClusterStatus{}) - cl := test.NewFakeClient(t, stable, sec) - service := newToolchainClusterService(t, cl, withCA) - // given - controller, req := prepareReconcile(stable, cl, service, requeAfter) - - // when - _, err := controller.Reconcile(context.TODO(), req) - - // then - require.NoError(t, err) - assertClusterStatus(t, cl, "failing", offline()) - }) + tests := map[string]struct { + tctype string + apiendpoint string + clustercondition1 toolchainv1alpha1.ToolchainClusterCondition + clustercondition2 toolchainv1alpha1.ToolchainClusterCondition + status toolchainv1alpha1.ToolchainClusterStatus + }{ + "UnstableNoCondition": { + tctype: "unstable", + apiendpoint: "http://unstable.com", + clustercondition1: notOffline(), + clustercondition2: unhealthy(), + status: toolchainv1alpha1.ToolchainClusterStatus{}, + }, + "StableNoCondition": { + tctype: "stable", + apiendpoint: "http://cluster.com", + clustercondition1: healthy(), + status: toolchainv1alpha1.ToolchainClusterStatus{}, + }, + "NotFoundNoCondition": { + tctype: "not-found", + apiendpoint: "http://not-found.com", + clustercondition1: offline(), + status: toolchainv1alpha1.ToolchainClusterStatus{}, + }, + "UnstableContainsCondition": { + tctype: "unstable", + apiendpoint: "http://unstable.com", + clustercondition1: notOffline(), + clustercondition2: unhealthy(), + status: withStatus(healthy()), + }, + "StableContainsCondition": { + tctype: "stable", + apiendpoint: "http://cluster.com", + clustercondition1: healthy(), + status: withStatus(healthy()), + }, + "NotFoundContainsCondition": { + tctype: "not-found", + apiendpoint: "http://not-found.com", + clustercondition1: offline(), + status: withStatus(healthy()), + }, + } + for k, tc := range tests { + t.Run(k, func(t *testing.T) { + tctype, sec := newToolchainCluster(tc.tctype, tcNs, tc.apiendpoint, tc.status) + cl := test.NewFakeClient(t, tctype, sec) + reset := setupCachedClusters(t, cl, tctype) + defer reset() + + service := newToolchainClusterService(t, cl, withCA) + //given + controller, req := prepareReconcile(tctype, cl, service, requeAfter) + + //when + _, err := controller.Reconcile(context.TODO(), req) + //then + require.NoError(t, err) + if k == "UnstableNoCondition" || k == "UnstableContainsCondition" { + assertClusterStatus(t, cl, tc.tctype, tc.clustercondition1, tc.clustercondition2) + } else { + assertClusterStatus(t, cl, tc.tctype, tc.clustercondition1) + } + }) + } } func setupCachedClusters(t *testing.T, cl *test.FakeClient, clusters ...*toolchainv1alpha1.ToolchainCluster) func() { From 2171041f8cb61b2ebc930f77b7959bfdefeb9f46 Mon Sep 17 00:00:00 2001 From: Feny Mehta Date: Wed, 17 Apr 2024 14:06:46 +0530 Subject: [PATCH 05/12] Review comments 1 Signed-off-by: Feny Mehta --- ...toolchaincluster_healthcheck_controller.go | 6 ++--- ...haincluster_healthcheck_controller_test.go | 25 ++++++++----------- 2 files changed, 13 insertions(+), 18 deletions(-) diff --git a/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller.go b/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller.go index 88f2e2a7..3bec8162 100644 --- a/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller.go +++ b/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller.go @@ -64,12 +64,12 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. // Error reading the object - requeue the request. return reconcile.Result{}, err } - clusterObj := toolchainCluster.DeepCopy() + cachedCluster, ok := cluster.GetCachedToolchainCluster(toolchainCluster.Name) if !ok { err := fmt.Errorf("cluster %s not found in cache", toolchainCluster.Name) - clusterObj.Status.Conditions = []toolchainv1alpha1.ToolchainClusterCondition{clusterOfflineCondition()} - if err := r.client.Status().Update(ctx, clusterObj); err != nil { + toolchainCluster.Status.Conditions = []toolchainv1alpha1.ToolchainClusterCondition{clusterOfflineCondition()} + if err := r.client.Status().Update(ctx, toolchainCluster); err != nil { reqLogger.Error(err, "failed to update the status of ToolchainCluster") } return reconcile.Result{}, err diff --git a/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller_test.go b/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller_test.go index 2fc90d12..85cf1e7a 100644 --- a/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller_test.go +++ b/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller_test.go @@ -119,46 +119,43 @@ func TestClusterHealthChecks(t *testing.T) { tests := map[string]struct { tctype string apiendpoint string - clustercondition1 toolchainv1alpha1.ToolchainClusterCondition - clustercondition2 toolchainv1alpha1.ToolchainClusterCondition + clusterconditions []toolchainv1alpha1.ToolchainClusterCondition status toolchainv1alpha1.ToolchainClusterStatus }{ "UnstableNoCondition": { tctype: "unstable", apiendpoint: "http://unstable.com", - clustercondition1: notOffline(), - clustercondition2: unhealthy(), + clusterconditions: []toolchainv1alpha1.ToolchainClusterCondition{unhealthy(), notOffline()}, status: toolchainv1alpha1.ToolchainClusterStatus{}, }, "StableNoCondition": { tctype: "stable", apiendpoint: "http://cluster.com", - clustercondition1: healthy(), + clusterconditions: []toolchainv1alpha1.ToolchainClusterCondition{healthy()}, status: toolchainv1alpha1.ToolchainClusterStatus{}, }, "NotFoundNoCondition": { tctype: "not-found", apiendpoint: "http://not-found.com", - clustercondition1: offline(), + clusterconditions: []toolchainv1alpha1.ToolchainClusterCondition{offline()}, status: toolchainv1alpha1.ToolchainClusterStatus{}, }, "UnstableContainsCondition": { tctype: "unstable", apiendpoint: "http://unstable.com", - clustercondition1: notOffline(), - clustercondition2: unhealthy(), + clusterconditions: []toolchainv1alpha1.ToolchainClusterCondition{unhealthy(), notOffline()}, status: withStatus(healthy()), }, "StableContainsCondition": { tctype: "stable", apiendpoint: "http://cluster.com", - clustercondition1: healthy(), + clusterconditions: []toolchainv1alpha1.ToolchainClusterCondition{healthy()}, status: withStatus(healthy()), }, "NotFoundContainsCondition": { tctype: "not-found", apiendpoint: "http://not-found.com", - clustercondition1: offline(), + clusterconditions: []toolchainv1alpha1.ToolchainClusterCondition{offline()}, status: withStatus(healthy()), }, } @@ -177,11 +174,9 @@ func TestClusterHealthChecks(t *testing.T) { _, err := controller.Reconcile(context.TODO(), req) //then require.NoError(t, err) - if k == "UnstableNoCondition" || k == "UnstableContainsCondition" { - assertClusterStatus(t, cl, tc.tctype, tc.clustercondition1, tc.clustercondition2) - } else { - assertClusterStatus(t, cl, tc.tctype, tc.clustercondition1) - } + + assertClusterStatus(t, cl, tc.tctype, tc.clusterconditions...) + }) } } From 5556309acca66ada36ec85143737306870f25313 Mon Sep 17 00:00:00 2001 From: Feny Mehta Date: Wed, 17 Apr 2024 15:48:30 +0530 Subject: [PATCH 06/12] removing unused clustercacheservice Signed-off-by: Feny Mehta --- ...toolchaincluster_healthcheck_controller.go | 17 ++++---- ...haincluster_healthcheck_controller_test.go | 42 +++++-------------- 2 files changed, 18 insertions(+), 41 deletions(-) diff --git a/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller.go b/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller.go index 3bec8162..4cc5fb8c 100644 --- a/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller.go +++ b/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller.go @@ -19,13 +19,11 @@ import ( // NewReconciler returns a new Reconciler func NewReconciler(mgr manager.Manager, namespace string, timeout time.Duration, requeAfter time.Duration) *Reconciler { - cacheLog := log.Log.WithName("toolchaincluster_health") - clusterCacheService := cluster.NewToolchainClusterService(mgr.GetClient(), cacheLog, namespace, timeout) + log.Log.WithName("toolchaincluster_health") return &Reconciler{ - client: mgr.GetClient(), - scheme: mgr.GetScheme(), - clusterCacheService: clusterCacheService, - requeAfter: requeAfter, + client: mgr.GetClient(), + scheme: mgr.GetScheme(), + requeAfter: requeAfter, } } @@ -38,10 +36,9 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { // Reconciler reconciles a ToolchainCluster object type Reconciler struct { - client client.Client - scheme *runtime.Scheme - clusterCacheService cluster.ToolchainClusterService - requeAfter time.Duration + client client.Client + scheme *runtime.Scheme + requeAfter time.Duration } // Reconcile reads that state of the cluster for a ToolchainCluster object and makes changes based on the state read diff --git a/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller_test.go b/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller_test.go index 85cf1e7a..8aecaa3e 100644 --- a/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller_test.go +++ b/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller_test.go @@ -41,7 +41,6 @@ func TestClusterHealthChecks(t *testing.T) { Reply(404) requeAfter := 10 * time.Second - withCA := false t.Run("ToolchainCluster not found", func(t *testing.T) { unstable, sec := newToolchainCluster("unstable", tcNs, "http://unstable.com", toolchainv1alpha1.ToolchainClusterStatus{}) @@ -49,9 +48,8 @@ func TestClusterHealthChecks(t *testing.T) { cl := test.NewFakeClient(t, sec) reset := setupCachedClusters(t, cl, unstable) defer reset() - service := newToolchainClusterService(t, cl, withCA) // given - controller, req := prepareReconcile(unstable, cl, service, requeAfter) + controller, req := prepareReconcile(unstable, cl, requeAfter) // when _, err := controller.Reconcile(context.TODO(), req) @@ -73,9 +71,8 @@ func TestClusterHealthChecks(t *testing.T) { return cl.Client.Get(ctx, key, obj, opts...) } - service := newToolchainClusterService(t, cl, withCA) // given - controller, req := prepareReconcile(unstable, cl, service, requeAfter) + controller, req := prepareReconcile(unstable, cl, requeAfter) // when _, err := controller.Reconcile(context.TODO(), req) @@ -88,9 +85,10 @@ func TestClusterHealthChecks(t *testing.T) { stable, sec := newToolchainCluster("failing", tcNs, "http://failing.com", toolchainv1alpha1.ToolchainClusterStatus{}) cl := test.NewFakeClient(t, stable, sec) - service := newToolchainClusterService(t, cl, withCA) + reset := setupCachedClusters(t, cl, stable) + defer reset() // given - controller, req := prepareReconcile(stable, cl, service, requeAfter) + controller, req := prepareReconcile(stable, cl, requeAfter) // when _, err := controller.Reconcile(context.TODO(), req) @@ -104,9 +102,8 @@ func TestClusterHealthChecks(t *testing.T) { cl := test.NewFakeClient(t, stable, sec) resetCache := setupCachedClusters(t, cl, stable) defer resetCache() - service := newToolchainClusterService(t, cl, withCA) // given - controller, req := prepareReconcile(stable, cl, service, requeAfter) + controller, req := prepareReconcile(stable, cl, requeAfter) // when _, err := controller.Reconcile(context.TODO(), req) @@ -166,9 +163,8 @@ func TestClusterHealthChecks(t *testing.T) { reset := setupCachedClusters(t, cl, tctype) defer reset() - service := newToolchainClusterService(t, cl, withCA) //given - controller, req := prepareReconcile(tctype, cl, service, requeAfter) + controller, req := prepareReconcile(tctype, cl, requeAfter) //when _, err := controller.Reconcile(context.TODO(), req) @@ -212,21 +208,6 @@ func newToolchainCluster(name, tcNs string, apiEndpoint string, status toolchain return toolchainCluster, secret } -func newToolchainClusterService(t *testing.T, cl client.Client, withCA bool) cluster.ToolchainClusterService { - return cluster.NewToolchainClusterServiceWithClient(cl, logf.Log, "test-namespace", 3*time.Second, func(config *rest.Config, options client.Options) (client.Client, error) { - if withCA { - assert.False(t, config.Insecure) - assert.Equal(t, []byte("dummy"), config.CAData) - } else { - assert.True(t, config.Insecure) - } - // make sure that insecure is false to make Gock mocking working properly - config.Insecure = false - // reset the dummy certificate - config.CAData = []byte("") - return client.New(config, options) - }) -} func assertClusterStatus(t *testing.T, cl client.Client, clusterName string, clusterConds ...toolchainv1alpha1.ToolchainClusterCondition) { tc := &toolchainv1alpha1.ToolchainCluster{} err := cl.Get(context.TODO(), test.NamespacedName("test-namespace", clusterName), tc) @@ -246,12 +227,11 @@ ExpConditions: } } -func prepareReconcile(toolchainCluster *toolchainv1alpha1.ToolchainCluster, cl *test.FakeClient, service cluster.ToolchainClusterService, requeAfter time.Duration) (Reconciler, reconcile.Request) { +func prepareReconcile(toolchainCluster *toolchainv1alpha1.ToolchainCluster, cl *test.FakeClient, requeAfter time.Duration) (Reconciler, reconcile.Request) { controller := Reconciler{ - client: cl, - scheme: scheme.Scheme, - clusterCacheService: service, - requeAfter: requeAfter, + client: cl, + scheme: scheme.Scheme, + requeAfter: requeAfter, } req := reconcile.Request{ NamespacedName: test.NamespacedName(toolchainCluster.Namespace, toolchainCluster.Name), From 5e77e0c85cf533be755dd7675c7d90ddbb264313 Mon Sep 17 00:00:00 2001 From: Feny Mehta Date: Wed, 17 Apr 2024 18:42:16 +0530 Subject: [PATCH 07/12] Refactoring the tests Signed-off-by: Feny Mehta --- .../healthchecker_test.go | 176 ++++++++++++++++++ ...toolchaincluster_healthcheck_controller.go | 46 +++-- ...haincluster_healthcheck_controller_test.go | 151 +-------------- 3 files changed, 205 insertions(+), 168 deletions(-) create mode 100644 controllers/toolchainclusterhealth/healthchecker_test.go diff --git a/controllers/toolchainclusterhealth/healthchecker_test.go b/controllers/toolchainclusterhealth/healthchecker_test.go new file mode 100644 index 00000000..00ff6a81 --- /dev/null +++ b/controllers/toolchainclusterhealth/healthchecker_test.go @@ -0,0 +1,176 @@ +package toolchainclusterhealth + +import ( + "context" + "testing" + + toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" + "github.com/codeready-toolchain/toolchain-common/pkg/cluster" + "github.com/codeready-toolchain/toolchain-common/pkg/test" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gopkg.in/h2non/gock.v1" + corev1 "k8s.io/api/core/v1" + kubeclientset "k8s.io/client-go/kubernetes" + "sigs.k8s.io/controller-runtime/pkg/client" + logf "sigs.k8s.io/controller-runtime/pkg/log" +) + +var logger = logf.Log.WithName("toolchaincluster_healthcheck") + +func TestClusterHealthChecks(t *testing.T) { + // given + defer gock.Off() + tcNs := "test-namespace" + gock.New("http://cluster.com"). + Get("healthz"). + Persist(). + Reply(200). + BodyString("ok") + gock.New("http://unstable.com"). + Get("healthz"). + Persist(). + Reply(200). + BodyString("unstable") + gock.New("http://not-found.com"). + Get("healthz"). + Persist(). + Reply(404) + + tests := map[string]struct { + tctype string + apiendpoint string + clusterconditions []toolchainv1alpha1.ToolchainClusterCondition + status toolchainv1alpha1.ToolchainClusterStatus + }{ + //ToolchainCluster.status doesn't contain any conditions + "UnstableNoCondition": { + tctype: "unstable", + apiendpoint: "http://unstable.com", + clusterconditions: []toolchainv1alpha1.ToolchainClusterCondition{unhealthy(), notOffline()}, + status: toolchainv1alpha1.ToolchainClusterStatus{}, + }, + "StableNoCondition": { + tctype: "stable", + apiendpoint: "http://cluster.com", + clusterconditions: []toolchainv1alpha1.ToolchainClusterCondition{healthy()}, + status: toolchainv1alpha1.ToolchainClusterStatus{}, + }, + "NotFoundNoCondition": { + tctype: "not-found", + apiendpoint: "http://not-found.com", + clusterconditions: []toolchainv1alpha1.ToolchainClusterCondition{offline()}, + status: toolchainv1alpha1.ToolchainClusterStatus{}, + }, + //ToolchainCluster.status already contains conditions + "UnstableContainsCondition": { + tctype: "unstable", + apiendpoint: "http://unstable.com", + clusterconditions: []toolchainv1alpha1.ToolchainClusterCondition{unhealthy(), notOffline()}, + status: withStatus(healthy()), + }, + "StableContainsCondition": { + tctype: "stable", + apiendpoint: "http://cluster.com", + clusterconditions: []toolchainv1alpha1.ToolchainClusterCondition{healthy()}, + status: withStatus(healthy()), + }, + "NotFoundContainsCondition": { + tctype: "not-found", + apiendpoint: "http://not-found.com", + clusterconditions: []toolchainv1alpha1.ToolchainClusterCondition{offline()}, + status: withStatus(healthy()), + }, + //if the connection cannot be established at beginning, then it should be offline + "OfflineConnectionNotEstablished": { + tctype: "failing", + apiendpoint: "http://failing.com", + clusterconditions: []toolchainv1alpha1.ToolchainClusterCondition{offline()}, + status: toolchainv1alpha1.ToolchainClusterStatus{}, + }, + //if no zones nor region is retrieved, then keep the current + "NoZoneKeepCurrent": { + tctype: "stable", + apiendpoint: "http://cluster.com", + clusterconditions: []toolchainv1alpha1.ToolchainClusterCondition{healthy()}, + status: withStatus(offline()), + }, + } + for k, tc := range tests { + t.Run(k, func(t *testing.T) { + tctype, sec := newToolchainCluster(tc.tctype, tcNs, tc.apiendpoint, tc.status) + cl := test.NewFakeClient(t, tctype, sec) + reset := setupCachedClusters(t, cl, tctype) + defer reset() + cachedtc, found := cluster.GetCachedToolchainCluster(tctype.Name) + require.True(t, found) + cacheclient, err := kubeclientset.NewForConfig(cachedtc.RestConfig) + require.NoError(t, err) + healthChecker := &HealthChecker{ + localClusterClient: cl, + remoteClusterClient: cachedtc.Client, + remoteClusterClientset: cacheclient, + logger: logger, + } + // when + err = healthChecker.updateIndividualClusterStatus(context.TODO(), tctype) + + //then + require.NoError(t, err) + assertClusterStatus(t, cl, tc.tctype, tc.clusterconditions...) + }) + } +} + +func withStatus(conditions ...toolchainv1alpha1.ToolchainClusterCondition) toolchainv1alpha1.ToolchainClusterStatus { + return toolchainv1alpha1.ToolchainClusterStatus{ + Conditions: conditions, + } +} +func assertClusterStatus(t *testing.T, cl client.Client, clusterName string, clusterConds ...toolchainv1alpha1.ToolchainClusterCondition) { + tc := &toolchainv1alpha1.ToolchainCluster{} + err := cl.Get(context.TODO(), test.NamespacedName("test-namespace", clusterName), tc) + require.NoError(t, err) + assert.Len(t, tc.Status.Conditions, len(clusterConds)) +ExpConditions: + for _, expCond := range clusterConds { + for _, cond := range tc.Status.Conditions { + if expCond.Type == cond.Type { + assert.Equal(t, expCond.Status, cond.Status) + assert.Equal(t, expCond.Reason, cond.Reason) + assert.Equal(t, expCond.Message, cond.Message) + continue ExpConditions + } + } + assert.Failf(t, "condition not found", "the list of conditions %v doesn't contain the expected condition %v", tc.Status.Conditions, expCond) + } +} +func healthy() toolchainv1alpha1.ToolchainClusterCondition { + return toolchainv1alpha1.ToolchainClusterCondition{ + Type: toolchainv1alpha1.ToolchainClusterReady, + Status: corev1.ConditionTrue, + Reason: "ClusterReady", + Message: "/healthz responded with ok", + } +} +func unhealthy() toolchainv1alpha1.ToolchainClusterCondition { + return toolchainv1alpha1.ToolchainClusterCondition{Type: toolchainv1alpha1.ToolchainClusterReady, + Status: corev1.ConditionFalse, + Reason: "ClusterNotReady", + Message: "/healthz responded without ok", + } +} +func offline() toolchainv1alpha1.ToolchainClusterCondition { + return toolchainv1alpha1.ToolchainClusterCondition{Type: toolchainv1alpha1.ToolchainClusterOffline, + Status: corev1.ConditionTrue, + Reason: "ClusterNotReachable", + Message: "cluster is not reachable", + } +} +func notOffline() toolchainv1alpha1.ToolchainClusterCondition { + return toolchainv1alpha1.ToolchainClusterCondition{Type: toolchainv1alpha1.ToolchainClusterOffline, + Status: corev1.ConditionFalse, + Reason: "ClusterReachable", + Message: "cluster is reachable", + } +} diff --git a/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller.go b/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller.go index 4cc5fb8c..b493030f 100644 --- a/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller.go +++ b/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller.go @@ -7,6 +7,7 @@ import ( toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" "github.com/codeready-toolchain/toolchain-common/pkg/cluster" + "github.com/go-logr/logr" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" kubeclientset "k8s.io/client-go/kubernetes" @@ -17,28 +18,44 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" ) +// defaultHealthCheckAndUpdateClusterStatus updates the status of the individual cluster +func defaultHealthCheckAndUpdateClusterStatus(ctx context.Context, localClusterClient client.Client, remoteClusterClient client.Client, remoteClusterClientset *kubeclientset.Clientset, logger logr.Logger, toolchainCluster *toolchainv1alpha1.ToolchainCluster) error { + healthChecker := &HealthChecker{ + localClusterClient: localClusterClient, + remoteClusterClient: remoteClusterClient, + remoteClusterClientset: remoteClusterClientset, + logger: logger, + } + + // update the status of the individual cluster. + return healthChecker.updateIndividualClusterStatus(ctx, toolchainCluster) +} + // NewReconciler returns a new Reconciler func NewReconciler(mgr manager.Manager, namespace string, timeout time.Duration, requeAfter time.Duration) *Reconciler { log.Log.WithName("toolchaincluster_health") return &Reconciler{ - client: mgr.GetClient(), - scheme: mgr.GetScheme(), - requeAfter: requeAfter, + client: mgr.GetClient(), + scheme: mgr.GetScheme(), + requeAfter: requeAfter, + healthCheckAndUpdateClusterStatus: defaultHealthCheckAndUpdateClusterStatus, } } -// SetupWithManager sets up the controller with the Manager. -func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { - return ctrl.NewControllerManagedBy(mgr). - For(&toolchainv1alpha1.ToolchainCluster{}). - Complete(r) -} - // Reconciler reconciles a ToolchainCluster object type Reconciler struct { client client.Client scheme *runtime.Scheme requeAfter time.Duration + + healthCheckAndUpdateClusterStatus func(ctx context.Context, localClusterClient client.Client, remoteClusterClient client.Client, remoteClusterClientset *kubeclientset.Clientset, logger logr.Logger, toolchainCluster *toolchainv1alpha1.ToolchainCluster) error +} + +// SetupWithManager sets up the controller with the Manager. +func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { + return ctrl.NewControllerManagedBy(mgr). + For(&toolchainv1alpha1.ToolchainCluster{}). + Complete(r) } // Reconcile reads that state of the cluster for a ToolchainCluster object and makes changes based on the state read @@ -77,15 +94,8 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. return reconcile.Result{}, err } - healthChecker := &HealthChecker{ - localClusterClient: r.client, - remoteClusterClient: cachedCluster.Client, - remoteClusterClientset: clientSet, - logger: reqLogger, - } - //update the status of the individual cluster. - if err := healthChecker.updateIndividualClusterStatus(ctx, toolchainCluster); err != nil { + if err := defaultHealthCheckAndUpdateClusterStatus(ctx, r.client, cachedCluster.Client, clientSet, reqLogger, toolchainCluster); err != nil { reqLogger.Error(err, "unable to update cluster status of ToolchainCluster") return reconcile.Result{}, err } diff --git a/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller_test.go b/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller_test.go index 8aecaa3e..63c2cf0b 100644 --- a/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller_test.go +++ b/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller_test.go @@ -9,7 +9,6 @@ import ( toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" "github.com/codeready-toolchain/toolchain-common/pkg/cluster" "github.com/codeready-toolchain/toolchain-common/pkg/test" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gopkg.in/h2non/gock.v1" corev1 "k8s.io/api/core/v1" @@ -21,7 +20,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" ) -func TestClusterHealthChecks(t *testing.T) { +func TestClustercontrollerChecks(t *testing.T) { // given defer gock.Off() tcNs := "test-namespace" @@ -81,100 +80,7 @@ func TestClusterHealthChecks(t *testing.T) { require.EqualError(t, err, "mock error") }) - t.Run("if the connection cannot be established at beginning, then it should be offline", func(t *testing.T) { - stable, sec := newToolchainCluster("failing", tcNs, "http://failing.com", toolchainv1alpha1.ToolchainClusterStatus{}) - cl := test.NewFakeClient(t, stable, sec) - reset := setupCachedClusters(t, cl, stable) - defer reset() - // given - controller, req := prepareReconcile(stable, cl, requeAfter) - - // when - _, err := controller.Reconcile(context.TODO(), req) - - // then - require.NoError(t, err) - assertClusterStatus(t, cl, "failing", offline()) - }) - t.Run("if no zones nor region is retrieved, then keep the current", func(t *testing.T) { - stable, sec := newToolchainCluster("stable", tcNs, "http://cluster.com", withStatus(offline())) - cl := test.NewFakeClient(t, stable, sec) - resetCache := setupCachedClusters(t, cl, stable) - defer resetCache() - // given - controller, req := prepareReconcile(stable, cl, requeAfter) - - // when - _, err := controller.Reconcile(context.TODO(), req) - - // then - require.NoError(t, err) - assertClusterStatus(t, cl, "stable", healthy()) - }) - - tests := map[string]struct { - tctype string - apiendpoint string - clusterconditions []toolchainv1alpha1.ToolchainClusterCondition - status toolchainv1alpha1.ToolchainClusterStatus - }{ - "UnstableNoCondition": { - tctype: "unstable", - apiendpoint: "http://unstable.com", - clusterconditions: []toolchainv1alpha1.ToolchainClusterCondition{unhealthy(), notOffline()}, - status: toolchainv1alpha1.ToolchainClusterStatus{}, - }, - "StableNoCondition": { - tctype: "stable", - apiendpoint: "http://cluster.com", - clusterconditions: []toolchainv1alpha1.ToolchainClusterCondition{healthy()}, - status: toolchainv1alpha1.ToolchainClusterStatus{}, - }, - "NotFoundNoCondition": { - tctype: "not-found", - apiendpoint: "http://not-found.com", - clusterconditions: []toolchainv1alpha1.ToolchainClusterCondition{offline()}, - status: toolchainv1alpha1.ToolchainClusterStatus{}, - }, - "UnstableContainsCondition": { - tctype: "unstable", - apiendpoint: "http://unstable.com", - clusterconditions: []toolchainv1alpha1.ToolchainClusterCondition{unhealthy(), notOffline()}, - status: withStatus(healthy()), - }, - "StableContainsCondition": { - tctype: "stable", - apiendpoint: "http://cluster.com", - clusterconditions: []toolchainv1alpha1.ToolchainClusterCondition{healthy()}, - status: withStatus(healthy()), - }, - "NotFoundContainsCondition": { - tctype: "not-found", - apiendpoint: "http://not-found.com", - clusterconditions: []toolchainv1alpha1.ToolchainClusterCondition{offline()}, - status: withStatus(healthy()), - }, - } - for k, tc := range tests { - t.Run(k, func(t *testing.T) { - tctype, sec := newToolchainCluster(tc.tctype, tcNs, tc.apiendpoint, tc.status) - cl := test.NewFakeClient(t, tctype, sec) - reset := setupCachedClusters(t, cl, tctype) - defer reset() - - //given - controller, req := prepareReconcile(tctype, cl, requeAfter) - - //when - _, err := controller.Reconcile(context.TODO(), req) - //then - require.NoError(t, err) - - assertClusterStatus(t, cl, tc.tctype, tc.clusterconditions...) - - }) - } } func setupCachedClusters(t *testing.T, cl *test.FakeClient, clusters ...*toolchainv1alpha1.ToolchainCluster) func() { @@ -197,36 +103,11 @@ func setupCachedClusters(t *testing.T, cl *test.FakeClient, clusters ...*toolcha } } -func withStatus(conditions ...toolchainv1alpha1.ToolchainClusterCondition) toolchainv1alpha1.ToolchainClusterStatus { - return toolchainv1alpha1.ToolchainClusterStatus{ - Conditions: conditions, - } -} - func newToolchainCluster(name, tcNs string, apiEndpoint string, status toolchainv1alpha1.ToolchainClusterStatus) (*toolchainv1alpha1.ToolchainCluster, *corev1.Secret) { toolchainCluster, secret := test.NewToolchainClusterWithEndpoint(name, tcNs, "secret", apiEndpoint, status, map[string]string{"namespace": "test-namespace"}) return toolchainCluster, secret } -func assertClusterStatus(t *testing.T, cl client.Client, clusterName string, clusterConds ...toolchainv1alpha1.ToolchainClusterCondition) { - tc := &toolchainv1alpha1.ToolchainCluster{} - err := cl.Get(context.TODO(), test.NamespacedName("test-namespace", clusterName), tc) - require.NoError(t, err) - assert.Len(t, tc.Status.Conditions, len(clusterConds)) -ExpConditions: - for _, expCond := range clusterConds { - for _, cond := range tc.Status.Conditions { - if expCond.Type == cond.Type { - assert.Equal(t, expCond.Status, cond.Status) - assert.Equal(t, expCond.Reason, cond.Reason) - assert.Equal(t, expCond.Message, cond.Message) - continue ExpConditions - } - } - assert.Failf(t, "condition not found", "the list of conditions %v doesn't contain the expected condition %v", tc.Status.Conditions, expCond) - } -} - func prepareReconcile(toolchainCluster *toolchainv1alpha1.ToolchainCluster, cl *test.FakeClient, requeAfter time.Duration) (Reconciler, reconcile.Request) { controller := Reconciler{ client: cl, @@ -238,33 +119,3 @@ func prepareReconcile(toolchainCluster *toolchainv1alpha1.ToolchainCluster, cl * } return controller, req } - -func healthy() toolchainv1alpha1.ToolchainClusterCondition { - return toolchainv1alpha1.ToolchainClusterCondition{ - Type: toolchainv1alpha1.ToolchainClusterReady, - Status: corev1.ConditionTrue, - Reason: "ClusterReady", - Message: "/healthz responded with ok", - } -} -func unhealthy() toolchainv1alpha1.ToolchainClusterCondition { - return toolchainv1alpha1.ToolchainClusterCondition{Type: toolchainv1alpha1.ToolchainClusterReady, - Status: corev1.ConditionFalse, - Reason: "ClusterNotReady", - Message: "/healthz responded without ok", - } -} -func offline() toolchainv1alpha1.ToolchainClusterCondition { - return toolchainv1alpha1.ToolchainClusterCondition{Type: toolchainv1alpha1.ToolchainClusterOffline, - Status: corev1.ConditionTrue, - Reason: "ClusterNotReachable", - Message: "cluster is not reachable", - } -} -func notOffline() toolchainv1alpha1.ToolchainClusterCondition { - return toolchainv1alpha1.ToolchainClusterCondition{Type: toolchainv1alpha1.ToolchainClusterOffline, - Status: corev1.ConditionFalse, - Reason: "ClusterReachable", - Message: "cluster is reachable", - } -} From 762118cab090c54b42a6c745d299941f410f346c Mon Sep 17 00:00:00 2001 From: Feny Mehta Date: Wed, 17 Apr 2024 18:50:14 +0530 Subject: [PATCH 08/12] Fixing go lint Signed-off-by: Feny Mehta --- .../toolchaincluster_healthcheck_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller.go b/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller.go index b493030f..718a9ca4 100644 --- a/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller.go +++ b/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller.go @@ -32,7 +32,7 @@ func defaultHealthCheckAndUpdateClusterStatus(ctx context.Context, localClusterC } // NewReconciler returns a new Reconciler -func NewReconciler(mgr manager.Manager, namespace string, timeout time.Duration, requeAfter time.Duration) *Reconciler { +func NewReconciler(mgr manager.Manager, requeAfter time.Duration) *Reconciler { log.Log.WithName("toolchaincluster_health") return &Reconciler{ client: mgr.GetClient(), From 3ae200243971c8529b37f94c97b5aaeaaab3d209 Mon Sep 17 00:00:00 2001 From: Feny Mehta Date: Thu, 18 Apr 2024 16:07:29 +0530 Subject: [PATCH 09/12] Just convert to controller Signed-off-by: Feny Mehta --- .../healthchecker_test.go | 1 + ...ller.go => toolchaincluster_controller.go} | 35 +++------ ...go => toolchaincluster_controller_test.go} | 71 ++++++++++++++----- 3 files changed, 67 insertions(+), 40 deletions(-) rename controllers/toolchainclusterhealth/{toolchaincluster_healthcheck_controller.go => toolchaincluster_controller.go} (66%) rename controllers/toolchainclusterhealth/{toolchaincluster_healthcheck_controller_test.go => toolchaincluster_controller_test.go} (60%) diff --git a/controllers/toolchainclusterhealth/healthchecker_test.go b/controllers/toolchainclusterhealth/healthchecker_test.go index 00ff6a81..8c85a3eb 100644 --- a/controllers/toolchainclusterhealth/healthchecker_test.go +++ b/controllers/toolchainclusterhealth/healthchecker_test.go @@ -19,6 +19,7 @@ import ( var logger = logf.Log.WithName("toolchaincluster_healthcheck") func TestClusterHealthChecks(t *testing.T) { + // given defer gock.Off() tcNs := "test-namespace" diff --git a/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller.go b/controllers/toolchainclusterhealth/toolchaincluster_controller.go similarity index 66% rename from controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller.go rename to controllers/toolchainclusterhealth/toolchaincluster_controller.go index 718a9ca4..4b9d9ee9 100644 --- a/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller.go +++ b/controllers/toolchainclusterhealth/toolchaincluster_controller.go @@ -7,38 +7,22 @@ import ( toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" "github.com/codeready-toolchain/toolchain-common/pkg/cluster" - "github.com/go-logr/logr" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" kubeclientset "k8s.io/client-go/kubernetes" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" - "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/reconcile" ) -// defaultHealthCheckAndUpdateClusterStatus updates the status of the individual cluster -func defaultHealthCheckAndUpdateClusterStatus(ctx context.Context, localClusterClient client.Client, remoteClusterClient client.Client, remoteClusterClientset *kubeclientset.Clientset, logger logr.Logger, toolchainCluster *toolchainv1alpha1.ToolchainCluster) error { - healthChecker := &HealthChecker{ - localClusterClient: localClusterClient, - remoteClusterClient: remoteClusterClient, - remoteClusterClientset: remoteClusterClientset, - logger: logger, - } - - // update the status of the individual cluster. - return healthChecker.updateIndividualClusterStatus(ctx, toolchainCluster) -} - // NewReconciler returns a new Reconciler -func NewReconciler(mgr manager.Manager, requeAfter time.Duration) *Reconciler { +func NewReconciler(cl client.Client, scheme runtime.Scheme, requeAfter time.Duration) *Reconciler { log.Log.WithName("toolchaincluster_health") return &Reconciler{ - client: mgr.GetClient(), - scheme: mgr.GetScheme(), - requeAfter: requeAfter, - healthCheckAndUpdateClusterStatus: defaultHealthCheckAndUpdateClusterStatus, + client: cl, + scheme: &scheme, + requeAfter: requeAfter, } } @@ -47,8 +31,6 @@ type Reconciler struct { client client.Client scheme *runtime.Scheme requeAfter time.Duration - - healthCheckAndUpdateClusterStatus func(ctx context.Context, localClusterClient client.Client, remoteClusterClient client.Client, remoteClusterClientset *kubeclientset.Clientset, logger logr.Logger, toolchainCluster *toolchainv1alpha1.ToolchainCluster) error } // SetupWithManager sets up the controller with the Manager. @@ -93,9 +75,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. if err != nil { return reconcile.Result{}, err } - + healthChecker := &HealthChecker{ + localClusterClient: r.client, + remoteClusterClient: cachedCluster.Client, + remoteClusterClientset: clientSet, + logger: reqLogger, + } //update the status of the individual cluster. - if err := defaultHealthCheckAndUpdateClusterStatus(ctx, r.client, cachedCluster.Client, clientSet, reqLogger, toolchainCluster); err != nil { + if err := healthChecker.updateIndividualClusterStatus(ctx, toolchainCluster); err != nil { reqLogger.Error(err, "unable to update cluster status of ToolchainCluster") return reconcile.Result{}, err } diff --git a/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller_test.go b/controllers/toolchainclusterhealth/toolchaincluster_controller_test.go similarity index 60% rename from controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller_test.go rename to controllers/toolchainclusterhealth/toolchaincluster_controller_test.go index 63c2cf0b..dca5ed65 100644 --- a/controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller_test.go +++ b/controllers/toolchainclusterhealth/toolchaincluster_controller_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/require" "gopkg.in/h2non/gock.v1" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" @@ -20,8 +21,11 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" ) +var requeAfter = 10 * time.Second + func TestClustercontrollerChecks(t *testing.T) { // given + defer gock.Off() tcNs := "test-namespace" gock.New("http://cluster.com"). @@ -39,27 +43,26 @@ func TestClustercontrollerChecks(t *testing.T) { Persist(). Reply(404) - requeAfter := 10 * time.Second - t.Run("ToolchainCluster not found", func(t *testing.T) { - unstable, sec := newToolchainCluster("unstable", tcNs, "http://unstable.com", toolchainv1alpha1.ToolchainClusterStatus{}) + NotFound, sec := newToolchainCluster("notfound", tcNs, "http://not-found.com", toolchainv1alpha1.ToolchainClusterStatus{}) cl := test.NewFakeClient(t, sec) - reset := setupCachedClusters(t, cl, unstable) + reset := setupCachedClusters(t, cl, NotFound) defer reset() // given - controller, req := prepareReconcile(unstable, cl, requeAfter) + controller, req := prepareReconcile(NotFound, cl, requeAfter) // when - _, err := controller.Reconcile(context.TODO(), req) + recresult, err := controller.Reconcile(context.TODO(), req) // then - require.NoError(t, err) + require.Equal(t, err, nil) + require.Equal(t, reconcile.Result{Requeue: false, RequeueAfter: 0}, recresult) }) t.Run("Error while getting ToolchainCluster", func(t *testing.T) { - unstable, sec := newToolchainCluster("unstable", tcNs, "http://unstable.com", toolchainv1alpha1.ToolchainClusterStatus{}) + tc, sec := newToolchainCluster("tc", tcNs, "http://tc.com", toolchainv1alpha1.ToolchainClusterStatus{}) cl := test.NewFakeClient(t, sec) @@ -71,13 +74,52 @@ func TestClustercontrollerChecks(t *testing.T) { } // given - controller, req := prepareReconcile(unstable, cl, requeAfter) + controller, req := prepareReconcile(tc, cl, requeAfter) // when - _, err := controller.Reconcile(context.TODO(), req) + recresult, err := controller.Reconcile(context.TODO(), req) // then require.EqualError(t, err, "mock error") + require.Equal(t, reconcile.Result{Requeue: false, RequeueAfter: 0}, recresult) + + }) + + t.Run("reconcile successful and requeued", func(t *testing.T) { + stable, sec := newToolchainCluster("stable", tcNs, "http://cluster.com", toolchainv1alpha1.ToolchainClusterStatus{}) + + cl := test.NewFakeClient(t, stable, sec) + reset := setupCachedClusters(t, cl, stable) + defer reset() + // given + controller, req := prepareReconcile(stable, cl, requeAfter) + + // when + recresult, err := controller.Reconcile(context.TODO(), req) + + // then + require.Equal(t, err, nil) + require.Equal(t, reconcile.Result{RequeueAfter: requeAfter}, recresult) + + }) + + t.Run("toolchain cluster cache not found", func(t *testing.T) { + stable, _ := newToolchainCluster("stable", tcNs, "http://cluster.com", toolchainv1alpha1.ToolchainClusterStatus{}) + + cl := test.NewFakeClient(t, stable) + + // given + controller, req := prepareReconcile(stable, cl, requeAfter) + + // when + _, err := controller.Reconcile(context.TODO(), req) + + // then + require.EqualError(t, err, "cluster stable not found in cache") + actualtoolchaincluster := &toolchainv1alpha1.ToolchainCluster{} + err = cl.Client.Get(context.TODO(), types.NamespacedName{Name: "stable", Namespace: tcNs}, actualtoolchaincluster) + require.NoError(t, err) + assertClusterStatus(t, cl, "stable", offline()) }) @@ -109,13 +151,10 @@ func newToolchainCluster(name, tcNs string, apiEndpoint string, status toolchain } func prepareReconcile(toolchainCluster *toolchainv1alpha1.ToolchainCluster, cl *test.FakeClient, requeAfter time.Duration) (Reconciler, reconcile.Request) { - controller := Reconciler{ - client: cl, - scheme: scheme.Scheme, - requeAfter: requeAfter, - } + controller := NewReconciler(cl, *scheme.Scheme, requeAfter) + req := reconcile.Request{ NamespacedName: test.NamespacedName(toolchainCluster.Namespace, toolchainCluster.Name), } - return controller, req + return *controller, req } From 4ec7dc5e41c5550c6baa0b5c681487e8ff2fb640 Mon Sep 17 00:00:00 2001 From: Feny Mehta Date: Thu, 18 Apr 2024 18:43:19 +0530 Subject: [PATCH 10/12] Updated package name Signed-off-by: Feny Mehta --- .../healthchecker.go | 2 +- .../healthchecker_test.go | 2 +- .../toolchaincluster_controller.go | 12 +----------- .../toolchaincluster_controller_test.go | 11 +++++++---- 4 files changed, 10 insertions(+), 17 deletions(-) rename controllers/{toolchainclusterhealth => toolchaincluster}/healthchecker.go (99%) rename controllers/{toolchainclusterhealth => toolchaincluster}/healthchecker_test.go (99%) rename controllers/{toolchainclusterhealth => toolchaincluster}/toolchaincluster_controller.go (90%) rename controllers/{toolchainclusterhealth => toolchaincluster}/toolchaincluster_controller_test.go (97%) diff --git a/controllers/toolchainclusterhealth/healthchecker.go b/controllers/toolchaincluster/healthchecker.go similarity index 99% rename from controllers/toolchainclusterhealth/healthchecker.go rename to controllers/toolchaincluster/healthchecker.go index b6d40fdf..01d2ead9 100644 --- a/controllers/toolchainclusterhealth/healthchecker.go +++ b/controllers/toolchaincluster/healthchecker.go @@ -1,4 +1,4 @@ -package toolchainclusterhealth +package toolchaincluster import ( "context" diff --git a/controllers/toolchainclusterhealth/healthchecker_test.go b/controllers/toolchaincluster/healthchecker_test.go similarity index 99% rename from controllers/toolchainclusterhealth/healthchecker_test.go rename to controllers/toolchaincluster/healthchecker_test.go index 8c85a3eb..da50b38f 100644 --- a/controllers/toolchainclusterhealth/healthchecker_test.go +++ b/controllers/toolchaincluster/healthchecker_test.go @@ -1,4 +1,4 @@ -package toolchainclusterhealth +package toolchaincluster import ( "context" diff --git a/controllers/toolchainclusterhealth/toolchaincluster_controller.go b/controllers/toolchaincluster/toolchaincluster_controller.go similarity index 90% rename from controllers/toolchainclusterhealth/toolchaincluster_controller.go rename to controllers/toolchaincluster/toolchaincluster_controller.go index 4b9d9ee9..c1aaa78e 100644 --- a/controllers/toolchainclusterhealth/toolchaincluster_controller.go +++ b/controllers/toolchaincluster/toolchaincluster_controller.go @@ -1,4 +1,4 @@ -package toolchainclusterhealth +package toolchaincluster import ( "context" @@ -16,16 +16,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" ) -// NewReconciler returns a new Reconciler -func NewReconciler(cl client.Client, scheme runtime.Scheme, requeAfter time.Duration) *Reconciler { - log.Log.WithName("toolchaincluster_health") - return &Reconciler{ - client: cl, - scheme: &scheme, - requeAfter: requeAfter, - } -} - // Reconciler reconciles a ToolchainCluster object type Reconciler struct { client client.Client diff --git a/controllers/toolchainclusterhealth/toolchaincluster_controller_test.go b/controllers/toolchaincluster/toolchaincluster_controller_test.go similarity index 97% rename from controllers/toolchainclusterhealth/toolchaincluster_controller_test.go rename to controllers/toolchaincluster/toolchaincluster_controller_test.go index dca5ed65..1d869ded 100644 --- a/controllers/toolchainclusterhealth/toolchaincluster_controller_test.go +++ b/controllers/toolchaincluster/toolchaincluster_controller_test.go @@ -1,4 +1,4 @@ -package toolchainclusterhealth +package toolchaincluster import ( "context" @@ -151,10 +151,13 @@ func newToolchainCluster(name, tcNs string, apiEndpoint string, status toolchain } func prepareReconcile(toolchainCluster *toolchainv1alpha1.ToolchainCluster, cl *test.FakeClient, requeAfter time.Duration) (Reconciler, reconcile.Request) { - controller := NewReconciler(cl, *scheme.Scheme, requeAfter) - + controller := Reconciler{ + client: cl, + scheme: scheme.Scheme, + requeAfter: requeAfter, + } req := reconcile.Request{ NamespacedName: test.NamespacedName(toolchainCluster.Namespace, toolchainCluster.Name), } - return *controller, req + return controller, req } From 5d6888b6a711e446f4a038cb1ebab2d2c126e1fd Mon Sep 17 00:00:00 2001 From: Feny Mehta Date: Thu, 18 Apr 2024 21:23:41 +0530 Subject: [PATCH 11/12] Review comments-2 Signed-off-by: Feny Mehta --- .../toolchaincluster/healthchecker_test.go | 2 +- .../toolchaincluster_controller.go | 17 +++++++++-------- .../toolchaincluster_controller_test.go | 11 ++++++----- 3 files changed, 16 insertions(+), 14 deletions(-) diff --git a/controllers/toolchaincluster/healthchecker_test.go b/controllers/toolchaincluster/healthchecker_test.go index da50b38f..773bb4b3 100644 --- a/controllers/toolchaincluster/healthchecker_test.go +++ b/controllers/toolchaincluster/healthchecker_test.go @@ -74,7 +74,7 @@ func TestClusterHealthChecks(t *testing.T) { tctype: "stable", apiendpoint: "http://cluster.com", clusterconditions: []toolchainv1alpha1.ToolchainClusterCondition{healthy()}, - status: withStatus(healthy()), + status: withStatus(offline()), }, "NotFoundContainsCondition": { tctype: "not-found", diff --git a/controllers/toolchaincluster/toolchaincluster_controller.go b/controllers/toolchaincluster/toolchaincluster_controller.go index c1aaa78e..fb564cc1 100644 --- a/controllers/toolchaincluster/toolchaincluster_controller.go +++ b/controllers/toolchaincluster/toolchaincluster_controller.go @@ -18,9 +18,9 @@ import ( // Reconciler reconciles a ToolchainCluster object type Reconciler struct { - client client.Client - scheme *runtime.Scheme - requeAfter time.Duration + Client client.Client + Scheme *runtime.Scheme + RequeAfter time.Duration } // SetupWithManager sets up the controller with the Manager. @@ -36,12 +36,12 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { // The Controller will requeue the Request to be processed again if the returned error is non-nil or // Result.Requeue is true, otherwise upon completion it will remove the work from the queue. func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.Result, error) { - reqLogger := log.FromContext(ctx).WithName("health") + reqLogger := log.FromContext(ctx) reqLogger.Info("Reconciling ToolchainCluster") // Fetch the ToolchainCluster instance toolchainCluster := &toolchainv1alpha1.ToolchainCluster{} - err := r.client.Get(ctx, request.NamespacedName, toolchainCluster) + err := r.Client.Get(ctx, request.NamespacedName, toolchainCluster) if err != nil { if errors.IsNotFound(err) { // Stop monitoring the toolchain cluster as it is deleted @@ -55,7 +55,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. if !ok { err := fmt.Errorf("cluster %s not found in cache", toolchainCluster.Name) toolchainCluster.Status.Conditions = []toolchainv1alpha1.ToolchainClusterCondition{clusterOfflineCondition()} - if err := r.client.Status().Update(ctx, toolchainCluster); err != nil { + if err := r.Client.Status().Update(ctx, toolchainCluster); err != nil { reqLogger.Error(err, "failed to update the status of ToolchainCluster") } return reconcile.Result{}, err @@ -63,10 +63,11 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. clientSet, err := kubeclientset.NewForConfig(cachedCluster.RestConfig) if err != nil { + reqLogger.Error(err, "cannot create ClientSet for the ToolchainCluster") return reconcile.Result{}, err } healthChecker := &HealthChecker{ - localClusterClient: r.client, + localClusterClient: r.Client, remoteClusterClient: cachedCluster.Client, remoteClusterClientset: clientSet, logger: reqLogger, @@ -77,5 +78,5 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. return reconcile.Result{}, err } - return reconcile.Result{RequeueAfter: r.requeAfter}, nil + return reconcile.Result{RequeueAfter: r.RequeAfter}, nil } diff --git a/controllers/toolchaincluster/toolchaincluster_controller_test.go b/controllers/toolchaincluster/toolchaincluster_controller_test.go index 1d869ded..66f143f8 100644 --- a/controllers/toolchaincluster/toolchaincluster_controller_test.go +++ b/controllers/toolchaincluster/toolchaincluster_controller_test.go @@ -23,7 +23,7 @@ import ( var requeAfter = 10 * time.Second -func TestClustercontrollerChecks(t *testing.T) { +func TestClusterControllerChecks(t *testing.T) { // given defer gock.Off() @@ -44,12 +44,12 @@ func TestClustercontrollerChecks(t *testing.T) { Reply(404) t.Run("ToolchainCluster not found", func(t *testing.T) { + // given NotFound, sec := newToolchainCluster("notfound", tcNs, "http://not-found.com", toolchainv1alpha1.ToolchainClusterStatus{}) cl := test.NewFakeClient(t, sec) reset := setupCachedClusters(t, cl, NotFound) defer reset() - // given controller, req := prepareReconcile(NotFound, cl, requeAfter) // when @@ -100,6 +100,7 @@ func TestClustercontrollerChecks(t *testing.T) { // then require.Equal(t, err, nil) require.Equal(t, reconcile.Result{RequeueAfter: requeAfter}, recresult) + assertClusterStatus(t, cl, "stable", healthy()) }) @@ -152,9 +153,9 @@ func newToolchainCluster(name, tcNs string, apiEndpoint string, status toolchain func prepareReconcile(toolchainCluster *toolchainv1alpha1.ToolchainCluster, cl *test.FakeClient, requeAfter time.Duration) (Reconciler, reconcile.Request) { controller := Reconciler{ - client: cl, - scheme: scheme.Scheme, - requeAfter: requeAfter, + Client: cl, + Scheme: scheme.Scheme, + RequeAfter: requeAfter, } req := reconcile.Request{ NamespacedName: test.NamespacedName(toolchainCluster.Namespace, toolchainCluster.Name), From b8fdb47a8e2bc6ebcb3b678d660a63dbebc75691 Mon Sep 17 00:00:00 2001 From: Feny Mehta Date: Fri, 19 Apr 2024 20:59:34 +0530 Subject: [PATCH 12/12] better readability Signed-off-by: Feny Mehta --- .../toolchaincluster/toolchaincluster_controller_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/controllers/toolchaincluster/toolchaincluster_controller_test.go b/controllers/toolchaincluster/toolchaincluster_controller_test.go index 66f143f8..22727789 100644 --- a/controllers/toolchaincluster/toolchaincluster_controller_test.go +++ b/controllers/toolchaincluster/toolchaincluster_controller_test.go @@ -62,6 +62,7 @@ func TestClusterControllerChecks(t *testing.T) { }) t.Run("Error while getting ToolchainCluster", func(t *testing.T) { + // given tc, sec := newToolchainCluster("tc", tcNs, "http://tc.com", toolchainv1alpha1.ToolchainClusterStatus{}) cl := test.NewFakeClient(t, sec) @@ -73,7 +74,6 @@ func TestClusterControllerChecks(t *testing.T) { return cl.Client.Get(ctx, key, obj, opts...) } - // given controller, req := prepareReconcile(tc, cl, requeAfter) // when @@ -86,12 +86,12 @@ func TestClusterControllerChecks(t *testing.T) { }) t.Run("reconcile successful and requeued", func(t *testing.T) { + // given stable, sec := newToolchainCluster("stable", tcNs, "http://cluster.com", toolchainv1alpha1.ToolchainClusterStatus{}) cl := test.NewFakeClient(t, stable, sec) reset := setupCachedClusters(t, cl, stable) defer reset() - // given controller, req := prepareReconcile(stable, cl, requeAfter) // when @@ -105,11 +105,11 @@ func TestClusterControllerChecks(t *testing.T) { }) t.Run("toolchain cluster cache not found", func(t *testing.T) { + // given stable, _ := newToolchainCluster("stable", tcNs, "http://cluster.com", toolchainv1alpha1.ToolchainClusterStatus{}) cl := test.NewFakeClient(t, stable) - // given controller, req := prepareReconcile(stable, cl, requeAfter) // when