Skip to content

Commit

Permalink
review
Browse files Browse the repository at this point in the history 
Signed-off-by: Feny Mehta <[email protected]>
  • Loading branch information
@fbm3307
fbm3307 committed Jul 4, 2024
1 parent 98893bb commit a61b21a
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 93 deletions.
4 changes: 1 addition & 3 deletions controllers/toolchaincluster/healthchecker_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,11 @@ func TestClusterHealthChecks(t *testing.T) {
tctype: "stable",
apiendpoint: "http://cluster.com",
healthcheck: true,
errh: nil,
},
"HealthNotOkayButNoError": {
tctype: "unstable",
apiendpoint: "http://unstable.com",
healthcheck: false,
errh: nil,
},
"ErrorWhileDoingHealth": {
tctype: "Notfound",
Expand Down Expand Up @@ -76,7 +74,7 @@ func TestClusterHealthChecks(t *testing.T) {
if tc.tctype == "Notfound" {
require.Error(t, errh)
} else {
require.Equal(t, tc.errh, errh)
require.NoError(t, errh)
}

})
Expand Down
59 changes: 12 additions & 47 deletions controllers/toolchaincluster/toolchaincluster_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,8 @@ import (
toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1"
"github.com/codeready-toolchain/toolchain-common/pkg/cluster"
"github.com/codeready-toolchain/toolchain-common/pkg/condition"
"github.com/pkg/errors"
corev1 "k8s.io/api/core/v1"
kerrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
kubeclientset "k8s.io/client-go/kubernetes"
"k8s.io/client-go/tools/clientcmd"
Expand Down Expand Up @@ -63,7 +61,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.
cachedCluster, ok := cluster.GetCachedToolchainCluster(toolchainCluster.Name)
if !ok {
err := fmt.Errorf("cluster %s not found in cache", toolchainCluster.Name)
if err := r.updateStatus(ctx, toolchainCluster, clusterOfflineCondition()); err != nil {
if err := r.updateStatus(ctx, toolchainCluster, clusterNotReadyCondition()); err != nil {
reqLogger.Error(err, "unable to update cluster status of ToolchainCluster")
}
return reconcile.Result{}, err
Expand Down Expand Up @@ -96,19 +94,16 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.
func (r *Reconciler) updateStatus(ctx context.Context, toolchainCluster *toolchainv1alpha1.ToolchainCluster, currentconditions ...toolchainv1alpha1.Condition) error {
toolchainCluster.Status.Conditions = condition.AddOrUpdateStatusConditionsWithLastUpdatedTimestamp(toolchainCluster.Status.Conditions, currentconditions...)
if err := r.Client.Status().Update(ctx, toolchainCluster); err != nil {
return errors.Wrapf(err, "Failed to update the status of cluster %s", toolchainCluster.Name)
return fmt.Errorf("failed to update the status of cluster - %s "+err.Error(), toolchainCluster.Name)
}
return nil
}

func (r *Reconciler) getClusterHealthCondition(ctx context.Context, remoteClusterClientset *kubeclientset.Clientset) []v1alpha1.Condition {

healthcheck, errhealth := r.getClusterHealth(ctx, remoteClusterClientset)
if errhealth != nil {
return []v1alpha1.Condition{clusterOfflineCondition()}
}
if !healthcheck {
return []v1alpha1.Condition{clusterNotReadyCondition(), clusterNotOfflineCondition()}
if errhealth != nil || !healthcheck {
return []v1alpha1.Condition{clusterNotReadyCondition()}
}
return []v1alpha1.Condition{clusterReadyCondition()}

Expand All @@ -122,50 +117,20 @@ func (r *Reconciler) getClusterHealth(ctx context.Context, remoteClusterClientse
}

func clusterReadyCondition() toolchainv1alpha1.Condition {
currentTime := metav1.Now()
return toolchainv1alpha1.Condition{
Type: toolchainv1alpha1.ConditionReady,
Status: corev1.ConditionTrue,
Reason: toolchainv1alpha1.ToolchainClusterClusterReadyReason,
Message: healthzOk,
LastUpdatedTime: &currentTime,
LastTransitionTime: currentTime,
Type: toolchainv1alpha1.ConditionReady,
Status: corev1.ConditionTrue,
Reason: toolchainv1alpha1.ToolchainClusterClusterReadyReason,
Message: healthzOk,
}
}

func clusterNotReadyCondition() toolchainv1alpha1.Condition {
currentTime := metav1.Now()
return toolchainv1alpha1.Condition{
Type: toolchainv1alpha1.ConditionReady,
Status: corev1.ConditionFalse,
Reason: toolchainv1alpha1.ToolchainClusterClusterNotReadyReason,
Message: healthzNotOk,
LastUpdatedTime: &currentTime,
LastTransitionTime: currentTime,
}
}

func clusterOfflineCondition() toolchainv1alpha1.Condition {
currentTime := metav1.Now()
return toolchainv1alpha1.Condition{
Type: toolchainv1alpha1.ToolchainClusterOffline,
Status: corev1.ConditionTrue,
Reason: toolchainv1alpha1.ToolchainClusterClusterNotReachableReason,
Message: clusterNotReachableMsg,
LastUpdatedTime: &currentTime,
LastTransitionTime: currentTime,
}
}

func clusterNotOfflineCondition() toolchainv1alpha1.Condition {
currentTime := metav1.Now()
return toolchainv1alpha1.Condition{
Type: toolchainv1alpha1.ToolchainClusterOffline,
Status: corev1.ConditionFalse,
Reason: toolchainv1alpha1.ToolchainClusterClusterReachableReason,
Message: clusterReachableMsg,
LastUpdatedTime: &currentTime,
LastTransitionTime: currentTime,
Type: toolchainv1alpha1.ConditionReady,
Status: corev1.ConditionFalse,
Reason: toolchainv1alpha1.ToolchainClusterClusterNotReadyReason,
Message: healthzNotOk,
}
}

Expand Down
55 changes: 12 additions & 43 deletions controllers/toolchaincluster/toolchaincluster_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ func TestClusterControllerChecks(t *testing.T) {

t.Run("reconcile successful and requeued", func(t *testing.T) {
// given
stable, sec := newToolchainCluster("stable", tcNs, "http://cluster.com", withStatus(healthy()))
stable, sec := newToolchainCluster("stable", tcNs, "http://cluster.com", withStatus(clusterReadyCondition()))

cl := test.NewFakeClient(t, stable, sec)
reset := setupCachedClusters(t, cl, stable)
Expand All @@ -104,15 +104,15 @@ func TestClusterControllerChecks(t *testing.T) {
// then
require.Equal(t, err, nil)
require.Equal(t, reconcile.Result{RequeueAfter: requeAfter}, recresult)
assertClusterStatus(t, cl, "stable", healthy())
assertClusterStatus(t, cl, "stable", clusterReadyCondition())
})

t.Run("toolchain cluster cache not found", func(t *testing.T) {
// given
unstable, _ := newToolchainCluster("unstable", tcNs, "http://cluster.com", toolchainv1alpha1.ToolchainClusterStatus{})

cl := test.NewFakeClient(t, unstable)
unstable.Status.Conditions = condition.AddOrUpdateStatusConditionsWithLastUpdatedTimestamp(unstable.Status.Conditions, clusterOfflineCondition())
unstable.Status.Conditions = condition.AddOrUpdateStatusConditionsWithLastUpdatedTimestamp(unstable.Status.Conditions, clusterNotReadyCondition())
controller, req := prepareReconcile(unstable, cl, requeAfter)

// when
Expand All @@ -123,12 +123,12 @@ func TestClusterControllerChecks(t *testing.T) {
actualtoolchaincluster := &toolchainv1alpha1.ToolchainCluster{}
err = cl.Client.Get(context.TODO(), types.NamespacedName{Name: "unstable", Namespace: tcNs}, actualtoolchaincluster)
require.NoError(t, err)
assertClusterStatus(t, cl, "unstable", offline())
assertClusterStatus(t, cl, "unstable", clusterNotReadyCondition())
})

t.Run("error while updating a toolchain cluster status on cache not found", func(t *testing.T) {
// given
stable, _ := newToolchainCluster("stable", tcNs, "http://cluster.com", withStatus(healthy()))
stable, _ := newToolchainCluster("stable", tcNs, "http://cluster.com", withStatus(clusterReadyCondition()))

cl := test.NewFakeClient(t, stable)
cl.MockStatusUpdate = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.UpdateOption) error {
Expand All @@ -146,12 +146,12 @@ func TestClusterControllerChecks(t *testing.T) {
actualtoolchaincluster := &toolchainv1alpha1.ToolchainCluster{}
err = cl.Client.Get(context.TODO(), types.NamespacedName{Name: "stable", Namespace: tcNs}, actualtoolchaincluster)
require.NoError(t, err)
assertClusterStatus(t, cl, "stable", healthy())
assertClusterStatus(t, cl, "stable", clusterReadyCondition())
})

t.Run("error while updating a toolchain cluster status", func(t *testing.T) {
// given
stable, sec := newToolchainCluster("stable", tcNs, "http://cluster.com", withStatus(healthy()))
stable, sec := newToolchainCluster("stable", tcNs, "http://cluster.com", toolchainv1alpha1.ToolchainClusterStatus{})
serr := fmt.Errorf("my test error")
cl := test.NewFakeClient(t, stable, sec)
cl.MockStatusUpdate = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.UpdateOption) error {
Expand All @@ -168,9 +168,8 @@ func TestClusterControllerChecks(t *testing.T) {
recresult, err := controller.Reconcile(context.TODO(), req)

// then
require.EqualError(t, err, fmt.Sprintf("Failed to update the status of cluster %s: %v", stable.Name, serr))
require.EqualError(t, err, fmt.Sprintf("failed to update the status of cluster - %s %v", stable.Name, serr))
require.Equal(t, reconcile.Result{}, recresult)
assertClusterStatus(t, cl, "stable", healthy())
})

t.Run("migrates connection settings to kubeconfig in secret", func(t *testing.T) {
Expand Down Expand Up @@ -211,7 +210,7 @@ func TestClusterControllerChecks(t *testing.T) {
func TestGetClusterHealth(t *testing.T) {
t.Run("Check health default", func(t *testing.T) {
// given
stable, sec := newToolchainCluster("stable", "test-namespace", "http://cluster.com", withStatus(healthy()))
stable, sec := newToolchainCluster("stable", "test-namespace", "http://cluster.com", withStatus(clusterReadyCondition()))

cl := test.NewFakeClient(t, stable, sec)
reset := setupCachedClusters(t, cl, stable)
Expand All @@ -228,11 +227,11 @@ func TestGetClusterHealth(t *testing.T) {
// then
require.Equal(t, err, nil)
require.Equal(t, reconcile.Result{RequeueAfter: requeAfter}, recresult)
assertClusterStatus(t, cl, "stable", healthy())
assertClusterStatus(t, cl, "stable", clusterReadyCondition())
})
t.Run("get health condition when health obtained is false ", func(t *testing.T) {
// given
stable, sec := newToolchainCluster("stable", "test-namespace", "http://cluster.com", withStatus(healthy()))
stable, sec := newToolchainCluster("stable", "test-namespace", "http://cluster.com", withStatus(clusterReadyCondition()))

cl := test.NewFakeClient(t, stable, sec)
reset := setupCachedClusters(t, cl, stable)
Expand All @@ -249,7 +248,7 @@ func TestGetClusterHealth(t *testing.T) {
// then
require.Equal(t, err, nil)
require.Equal(t, reconcile.Result{RequeueAfter: requeAfter}, recresult)
assertClusterStatus(t, cl, "stable", notOffline(), unhealthy())
assertClusterStatus(t, cl, "stable", clusterNotReadyCondition())
})
}
func TestComposeKubeConfig(t *testing.T) {
Expand Down Expand Up @@ -325,33 +324,3 @@ 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.Condition {
return toolchainv1alpha1.Condition{
Type: toolchainv1alpha1.ConditionReady,
Status: corev1.ConditionTrue,
Reason: "ClusterReady",
Message: "/healthz responded with ok",
}
}
func unhealthy() toolchainv1alpha1.Condition {
return toolchainv1alpha1.Condition{Type: toolchainv1alpha1.ConditionReady,
Status: corev1.ConditionFalse,
Reason: "ClusterNotReady",
Message: "/healthz responded without ok",
}
}
func offline() toolchainv1alpha1.Condition {
return toolchainv1alpha1.Condition{Type: toolchainv1alpha1.ToolchainClusterOffline,
Status: corev1.ConditionTrue,
Reason: "ClusterNotReachable",
Message: "cluster is not reachable",
}
}
func notOffline() toolchainv1alpha1.Condition {
return toolchainv1alpha1.Condition{Type: toolchainv1alpha1.ToolchainClusterOffline,
Status: corev1.ConditionFalse,
Reason: "ClusterReachable",
Message: "cluster is reachable",
}
}

0 comments on commit a61b21a

Please sign in to comment.