From 8ccefde99a2ff3c28cb2b6eb7a4a96661faa3ae5 Mon Sep 17 00:00:00 2001 From: Feny Mehta Date: Mon, 8 Jul 2024 16:17:17 +0530 Subject: [PATCH] review Signed-off-by: Feny Mehta --- .../toolchaincluster/healthchecker_test.go | 6 ++-- .../toolchaincluster_controller.go | 31 +++++++++++++------ .../toolchaincluster_controller_test.go | 22 ++++++------- 3 files changed, 35 insertions(+), 24 deletions(-) diff --git a/controllers/toolchaincluster/healthchecker_test.go b/controllers/toolchaincluster/healthchecker_test.go index 2a5aa561..23c0b3ac 100644 --- a/controllers/toolchaincluster/healthchecker_test.go +++ b/controllers/toolchaincluster/healthchecker_test.go @@ -2,6 +2,7 @@ package toolchaincluster import ( "context" + "fmt" "testing" toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" @@ -52,6 +53,7 @@ func TestClusterHealthChecks(t *testing.T) { tctype: "Notfound", apiendpoint: "http://not-found.com", healthcheck: false, + errh: fmt.Errorf("the server could not find the requested resource"), }, } for k, tc := range tests { @@ -71,8 +73,8 @@ func TestClusterHealthChecks(t *testing.T) { //then require.Equal(t, tc.healthcheck, healthcheck) - if tc.tctype == "Notfound" { - require.Error(t, errh) + if tc.errh != nil { + require.EqualError(t, errh, tc.errh.Error()) } else { require.NoError(t, errh) } diff --git a/controllers/toolchaincluster/toolchaincluster_controller.go b/controllers/toolchaincluster/toolchaincluster_controller.go index 1fc33581..48901431 100644 --- a/controllers/toolchaincluster/toolchaincluster_controller.go +++ b/controllers/toolchaincluster/toolchaincluster_controller.go @@ -27,7 +27,7 @@ type Reconciler struct { Client client.Client Scheme *runtime.Scheme RequeAfter time.Duration - CheckHealth func(context.Context, *kubeclientset.Clientset) (bool, error) + checkHealth func(context.Context, *kubeclientset.Clientset) (bool, error) } // SetupWithManager sets up the controller with the Manager. @@ -84,7 +84,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. healthcheckResult := r.getClusterHealthCondition(ctx, clientSet) // update the status of the individual cluster. - if err := r.updateStatus(ctx, toolchainCluster, healthcheckResult...); err != nil { + if err := r.updateStatus(ctx, toolchainCluster, healthcheckResult); err != nil { reqLogger.Error(err, "unable to update cluster status of ToolchainCluster") return reconcile.Result{}, err } @@ -99,22 +99,33 @@ func (r *Reconciler) updateStatus(ctx context.Context, toolchainCluster *toolcha return nil } -func (r *Reconciler) getClusterHealthCondition(ctx context.Context, remoteClusterClientset *kubeclientset.Clientset) []v1alpha1.Condition { +func (r *Reconciler) getClusterHealthCondition(ctx context.Context, remoteClusterClientset *kubeclientset.Clientset) v1alpha1.Condition { - healthcheck, errhealth := r.getClusterHealth(ctx, remoteClusterClientset) - if errhealth != nil || !healthcheck { - return []v1alpha1.Condition{clusterNotReadyCondition()} + ishealthy, err := r.getClusterHealth(ctx, remoteClusterClientset) + if err != nil { + return clusterOfflineCondition(err.Error()) + } else { + if !ishealthy { + return clusterNotReadyCondition() + } + return clusterReadyCondition() } - return []v1alpha1.Condition{clusterReadyCondition()} - } func (r *Reconciler) getClusterHealth(ctx context.Context, remoteClusterClientset *kubeclientset.Clientset) (bool, error) { - if r.CheckHealth != nil { - return r.CheckHealth(ctx, remoteClusterClientset) + if r.checkHealth != nil { + return r.checkHealth(ctx, remoteClusterClientset) } return getClusterHealthStatus(ctx, remoteClusterClientset) } +func clusterOfflineCondition(errMsg string) toolchainv1alpha1.Condition { + return toolchainv1alpha1.Condition{ + Type: toolchainv1alpha1.ConditionReady, + Status: corev1.ConditionFalse, + Reason: toolchainv1alpha1.ToolchainClusterClusterNotReachableReason, + Message: errMsg, + } +} func clusterReadyCondition() toolchainv1alpha1.Condition { return toolchainv1alpha1.Condition{ diff --git a/controllers/toolchaincluster/toolchaincluster_controller_test.go b/controllers/toolchaincluster/toolchaincluster_controller_test.go index fd3b85d9..a1e6a9b8 100644 --- a/controllers/toolchaincluster/toolchaincluster_controller_test.go +++ b/controllers/toolchaincluster/toolchaincluster_controller_test.go @@ -137,11 +137,11 @@ func TestClusterControllerChecks(t *testing.T) { controller, req := prepareReconcile(stable, cl, requeAfter) // when - recresult, err := controller.Reconcile(context.TODO(), req) + recResult, err := controller.Reconcile(context.TODO(), req) // then require.EqualError(t, err, "cluster stable not found in cache") - require.Equal(t, reconcile.Result{}, recresult) + require.Equal(t, reconcile.Result{}, recResult) actualtoolchaincluster := &toolchainv1alpha1.ToolchainCluster{} err = cl.Client.Get(context.TODO(), types.NamespacedName{Name: "stable", Namespace: tcNs}, actualtoolchaincluster) @@ -149,7 +149,7 @@ func TestClusterControllerChecks(t *testing.T) { assertClusterStatus(t, cl, "stable", clusterReadyCondition()) }) - t.Run("error while updating a toolchain cluster status", func(t *testing.T) { + t.Run("error while updating a toolchain cluster status when health-check failed", func(t *testing.T) { // given stable, sec := newToolchainCluster("stable", tcNs, "http://cluster.com", toolchainv1alpha1.ToolchainClusterStatus{}) serr := fmt.Errorf("my test error") @@ -161,7 +161,7 @@ func TestClusterControllerChecks(t *testing.T) { defer reset() controller, req := prepareReconcile(stable, cl, requeAfter) - controller.CheckHealth = func(context.Context, *kubeclientset.Clientset) (bool, error) { + controller.checkHealth = func(context.Context, *kubeclientset.Clientset) (bool, error) { return false, serr } // when @@ -217,7 +217,7 @@ func TestGetClusterHealth(t *testing.T) { defer reset() controller, req := prepareReconcile(stable, cl, requeAfter) - controller.CheckHealth = func(context.Context, *kubeclientset.Clientset) (bool, error) { + controller.checkHealth = func(context.Context, *kubeclientset.Clientset) (bool, error) { return true, nil } @@ -238,7 +238,7 @@ func TestGetClusterHealth(t *testing.T) { defer reset() controller, req := prepareReconcile(stable, cl, requeAfter) - controller.CheckHealth = func(context.Context, *kubeclientset.Clientset) (bool, error) { + controller.checkHealth = func(context.Context, *kubeclientset.Clientset) (bool, error) { return false, nil } @@ -314,12 +314,10 @@ func assertClusterStatus(t *testing.T, cl client.Client, clusterName string, clu 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.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) }