From 18ca760934225620bcd7216cc8e5899ab8e409c1 Mon Sep 17 00:00:00 2001 From: Liqian Luo <13264318+circy9@users.noreply.github.com> Date: Fri, 29 Jul 2022 15:09:21 -0700 Subject: [PATCH] Remove unuseful condition: Synced --- .../member_controller.go | 13 ++-- .../member_controller_test.go | 78 +++++-------------- .../membercluster/membercluster_controller.go | 2 +- pkg/utils/common.go | 37 --------- 4 files changed, 26 insertions(+), 104 deletions(-) diff --git a/pkg/controllers/internalmembercluster/member_controller.go b/pkg/controllers/internalmembercluster/member_controller.go index 330a8ffd0..efc9f137a 100644 --- a/pkg/controllers/internalmembercluster/member_controller.go +++ b/pkg/controllers/internalmembercluster/member_controller.go @@ -25,7 +25,6 @@ import ( "go.goms.io/fleet/apis" fleetv1alpha1 "go.goms.io/fleet/apis/v1alpha1" "go.goms.io/fleet/pkg/metrics" - "go.goms.io/fleet/pkg/utils" ) // Reconciler reconciles a InternalMemberCluster object in the member cluster. @@ -184,7 +183,7 @@ func (r *Reconciler) markInternalMemberClusterHeartbeatReceived(imc apis.Conditi Reason: eventReasonInternalMemberClusterHBReceived, ObservedGeneration: imc.GetGeneration(), } - imc.SetConditions(hearbeatReceivedCondition, utils.ReconcileSuccessCondition()) + imc.SetConditions(hearbeatReceivedCondition) // Hack: We need to get and set again as SetConditions() will ignore new LastTransitionTime if there is no status // change between existing condition and new condition. @@ -201,7 +200,7 @@ func (r *Reconciler) markInternalMemberClusterHealthy(imc apis.ConditionedObj) { Reason: eventReasonInternalMemberClusterHealthy, ObservedGeneration: imc.GetGeneration(), } - imc.SetConditions(internalMemberClusterHealthyCond, utils.ReconcileSuccessCondition()) + imc.SetConditions(internalMemberClusterHealthyCond) } func (r *Reconciler) markInternalMemberClusterUnhealthy(imc apis.ConditionedObj, err error) { @@ -214,7 +213,7 @@ func (r *Reconciler) markInternalMemberClusterUnhealthy(imc apis.ConditionedObj, Message: err.Error(), ObservedGeneration: imc.GetGeneration(), } - imc.SetConditions(internalMemberClusterUnhealthyCond, utils.ReconcileErrorCondition(err)) + imc.SetConditions(internalMemberClusterUnhealthyCond) } func (r *Reconciler) markInternalMemberClusterJoined(imc apis.ConditionedObj) { @@ -226,7 +225,7 @@ func (r *Reconciler) markInternalMemberClusterJoined(imc apis.ConditionedObj) { Reason: eventReasonInternalMemberClusterJoined, ObservedGeneration: imc.GetGeneration(), } - imc.SetConditions(joinSucceedCondition, utils.ReconcileSuccessCondition()) + imc.SetConditions(joinSucceedCondition) } func (r *Reconciler) markInternalMemberClusterLeft(imc apis.ConditionedObj) { @@ -238,7 +237,7 @@ func (r *Reconciler) markInternalMemberClusterLeft(imc apis.ConditionedObj) { Reason: eventReasonInternalMemberClusterLeft, ObservedGeneration: imc.GetGeneration(), } - imc.SetConditions(joinSucceedCondition, utils.ReconcileSuccessCondition()) + imc.SetConditions(joinSucceedCondition) } func (r *Reconciler) markInternalMemberClusterUnknown(imc apis.ConditionedObj) { @@ -250,7 +249,7 @@ func (r *Reconciler) markInternalMemberClusterUnknown(imc apis.ConditionedObj) { Reason: eventReasonInternalMemberClusterUnknown, ObservedGeneration: imc.GetGeneration(), } - imc.SetConditions(joinUnknownCondition, utils.ReconcileSuccessCondition()) + imc.SetConditions(joinUnknownCondition) } // SetupWithManager sets up the controller with the Manager. diff --git a/pkg/controllers/internalmembercluster/member_controller_test.go b/pkg/controllers/internalmembercluster/member_controller_test.go index 6858ea6a1..8340d4fc9 100644 --- a/pkg/controllers/internalmembercluster/member_controller_test.go +++ b/pkg/controllers/internalmembercluster/member_controller_test.go @@ -36,19 +36,12 @@ func TestMarkInternalMemberClusterJoined(t *testing.T) { // check that the correct event is emitted event := <-r.recorder.(*record.FakeRecorder).Events expected := utils.GetEventString(internalMemberCluster, corev1.EventTypeNormal, eventReasonInternalMemberClusterJoined, "internal member cluster has joined") - assert.Equal(t, expected, event, utils.TestCaseMsg, "TestMarkInternalMemberClusterJoined") - // Check expected conditions. - expectedConditions := []metav1.Condition{ - {Type: v1alpha1.ConditionTypeInternalMemberClusterJoin, Status: metav1.ConditionTrue, Reason: eventReasonInternalMemberClusterJoined}, - {Type: utils.ConditionTypeSynced, Status: metav1.ConditionTrue, Reason: utils.ReasonReconcileSuccess}, - } - - for _, expectedCondition := range expectedConditions { - actualCondition := internalMemberCluster.GetCondition(expectedCondition.Type) - assert.Equal(t, "", cmp.Diff(expectedCondition, *(actualCondition), cmpopts.IgnoreTypes(time.Time{})), utils.TestCaseMsg, "TestMarkInternalMemberClusterJoined") - } + // Check expected condition. + expectedCondition := metav1.Condition{Type: v1alpha1.ConditionTypeInternalMemberClusterJoin, Status: metav1.ConditionTrue, Reason: eventReasonInternalMemberClusterJoined} + actualCondition := internalMemberCluster.GetCondition(expectedCondition.Type) + assert.Equal(t, "", cmp.Diff(expectedCondition, *(actualCondition), cmpopts.IgnoreTypes(time.Time{})), utils.TestCaseMsg, "TestMarkInternalMemberClusterJoined") } func TestMarkInternalMemberClusterLeft(t *testing.T) { @@ -60,19 +53,12 @@ func TestMarkInternalMemberClusterLeft(t *testing.T) { // check that the correct event is emitted event := <-r.recorder.(*record.FakeRecorder).Events expected := utils.GetEventString(internalMemberCluster, corev1.EventTypeNormal, eventReasonInternalMemberClusterLeft, "internal member cluster has left") - assert.Equal(t, expected, event, utils.TestCaseMsg, "TestMarkInternalMemberClusterLeft") // Check expected conditions. - expectedConditions := []metav1.Condition{ - {Type: v1alpha1.ConditionTypeInternalMemberClusterJoin, Status: metav1.ConditionFalse, Reason: eventReasonInternalMemberClusterLeft}, - {Type: utils.ConditionTypeSynced, Status: metav1.ConditionTrue, Reason: utils.ReasonReconcileSuccess}, - } - - for _, expectedCondition := range expectedConditions { - actualCondition := internalMemberCluster.GetCondition(expectedCondition.Type) - assert.Equal(t, "", cmp.Diff(expectedCondition, *(actualCondition), cmpopts.IgnoreTypes(time.Time{})), utils.TestCaseMsg, "TestMarkInternalMemberClusterLeft") - } + expectedCondition := metav1.Condition{Type: v1alpha1.ConditionTypeInternalMemberClusterJoin, Status: metav1.ConditionFalse, Reason: eventReasonInternalMemberClusterLeft} + actualCondition := internalMemberCluster.GetCondition(expectedCondition.Type) + assert.Equal(t, "", cmp.Diff(expectedCondition, *(actualCondition), cmpopts.IgnoreTypes(time.Time{})), utils.TestCaseMsg, "TestMarkInternalMemberClusterLeft") } func TestMarkInternalMemberClusterUnknown(t *testing.T) { @@ -84,18 +70,12 @@ func TestMarkInternalMemberClusterUnknown(t *testing.T) { // check that the correct event is emitted event := <-r.recorder.(*record.FakeRecorder).Events expected := utils.GetEventString(internalMemberCluster, corev1.EventTypeNormal, eventReasonInternalMemberClusterUnknown, "internal member cluster join state unknown") - assert.Equal(t, expected, event, utils.TestCaseMsg, "TestMarkInternalMemberClusterUnknown") // Check expected conditions. - expectedConditions := []metav1.Condition{ - {Type: v1alpha1.ConditionTypeInternalMemberClusterJoin, Status: metav1.ConditionUnknown, Reason: eventReasonInternalMemberClusterUnknown}, - {Type: utils.ConditionTypeSynced, Status: metav1.ConditionTrue, Reason: utils.ReasonReconcileSuccess}, - } - for _, expectedCondition := range expectedConditions { - actualCondition := internalMemberCluster.GetCondition(expectedCondition.Type) - assert.Equal(t, "", cmp.Diff(expectedCondition, *(actualCondition), cmpopts.IgnoreTypes(time.Time{})), utils.TestCaseMsg, "TestMarkInternalMemberClusterUnknown") - } + expectedCondition := metav1.Condition{Type: v1alpha1.ConditionTypeInternalMemberClusterJoin, Status: metav1.ConditionUnknown, Reason: eventReasonInternalMemberClusterUnknown} + actualCondition := internalMemberCluster.GetCondition(expectedCondition.Type) + assert.Equal(t, "", cmp.Diff(expectedCondition, *(actualCondition), cmpopts.IgnoreTypes(time.Time{})), utils.TestCaseMsg, "TestMarkInternalMemberClusterUnknown") } func TestMarkInternalMemberClusterHeartbeatReceived(t *testing.T) { @@ -107,19 +87,12 @@ func TestMarkInternalMemberClusterHeartbeatReceived(t *testing.T) { // check that the correct event is emitted event := <-r.recorder.(*record.FakeRecorder).Events expected := utils.GetEventString(internalMemberCluster, corev1.EventTypeNormal, eventReasonInternalMemberClusterHBReceived, "internal member cluster heartbeat received") - assert.Equal(t, expected, event, utils.TestCaseMsg, "TestMarkInternalMemberClusterHeartbeatReceived") // Check expected conditions. - expectedConditions := []metav1.Condition{ - {Type: v1alpha1.ConditionTypeInternalMemberClusterHeartbeat, Status: metav1.ConditionTrue, Reason: eventReasonInternalMemberClusterHBReceived}, - {Type: utils.ConditionTypeSynced, Status: metav1.ConditionTrue, Reason: utils.ReasonReconcileSuccess}, - } - - for _, expectedCondition := range expectedConditions { - actualCondition := internalMemberCluster.GetCondition(expectedCondition.Type) - assert.Equal(t, "", cmp.Diff(expectedCondition, *(actualCondition), cmpopts.IgnoreTypes(time.Time{})), utils.TestCaseMsg, "TestMarkInternalMemberClusterHeartbeatReceived") - } + expectedCondition := metav1.Condition{Type: v1alpha1.ConditionTypeInternalMemberClusterHeartbeat, Status: metav1.ConditionTrue, Reason: eventReasonInternalMemberClusterHBReceived} + actualCondition := internalMemberCluster.GetCondition(expectedCondition.Type) + assert.Equal(t, "", cmp.Diff(expectedCondition, *(actualCondition), cmpopts.IgnoreTypes(time.Time{})), utils.TestCaseMsg, "TestMarkInternalMemberClusterHeartbeatReceived") // Verify last transition time is updated. oldLastTransitionTime := internalMemberCluster.GetCondition(v1alpha1.ConditionTypeInternalMemberClusterHeartbeat).LastTransitionTime @@ -137,19 +110,12 @@ func TestMarkInternalMemberClusterHealthy(t *testing.T) { // check that the correct event is emitted event := <-r.recorder.(*record.FakeRecorder).Events expected := utils.GetEventString(internalMemberCluster, corev1.EventTypeNormal, eventReasonInternalMemberClusterHealthy, "internal member cluster healthy") - assert.Equal(t, expected, event, utils.TestCaseMsg, "TestMarkInternalMemberClusterHealthy") // Check expected conditions. - expectedConditions := []metav1.Condition{ - {Type: v1alpha1.ConditionTypeMemberClusterHealth, Status: metav1.ConditionTrue, Reason: eventReasonInternalMemberClusterHealthy}, - {Type: utils.ConditionTypeSynced, Status: metav1.ConditionTrue, Reason: utils.ReasonReconcileSuccess}, - } - - for _, expectedCondition := range expectedConditions { - actualCondition := internalMemberCluster.GetCondition(expectedCondition.Type) - assert.Equal(t, "", cmp.Diff(expectedCondition, *(actualCondition), cmpopts.IgnoreTypes(time.Time{})), utils.TestCaseMsg, "TestMarkInternalMemberClusterHealthy") - } + expectedCondition := metav1.Condition{Type: v1alpha1.ConditionTypeMemberClusterHealth, Status: metav1.ConditionTrue, Reason: eventReasonInternalMemberClusterHealthy} + actualCondition := internalMemberCluster.GetCondition(expectedCondition.Type) + assert.Equal(t, "", cmp.Diff(expectedCondition, *(actualCondition), cmpopts.IgnoreTypes(time.Time{})), utils.TestCaseMsg, "TestMarkInternalMemberClusterHealthy") } func TestMarkInternalMemberClusterHeartbeatUnhealthy(t *testing.T) { @@ -162,18 +128,12 @@ func TestMarkInternalMemberClusterHeartbeatUnhealthy(t *testing.T) { // check that the correct event is emitted event := <-r.recorder.(*record.FakeRecorder).Events expected := utils.GetEventString(internalMemberCluster, corev1.EventTypeWarning, eventReasonInternalMemberClusterUnhealthy, "internal member cluster unhealthy") - assert.Equal(t, expected, event, utils.TestCaseMsg, "TestMarkInternalMemberClusterHeartbeatUnhealthy") // Check expected conditions. - expectedConditions := []metav1.Condition{ - {Type: v1alpha1.ConditionTypeInternalMemberClusterHealth, Status: metav1.ConditionFalse, Reason: eventReasonInternalMemberClusterUnhealthy, Message: "rand-err-msg"}, - {Type: utils.ConditionTypeSynced, Status: metav1.ConditionFalse, Reason: utils.ReasonReconcileError, Message: "rand-err-msg"}, - } - for _, expectedCondition := range expectedConditions { - actualCondition := internalMemberCluster.GetCondition(expectedCondition.Type) - assert.Equal(t, "", cmp.Diff(expectedCondition, *(actualCondition), cmpopts.IgnoreTypes(time.Time{})), utils.TestCaseMsg, "TestMarkInternalMemberClusterHeartbeatUnhealthy") - } + expectedCondition := metav1.Condition{Type: v1alpha1.ConditionTypeInternalMemberClusterHealth, Status: metav1.ConditionFalse, Reason: eventReasonInternalMemberClusterUnhealthy, Message: "rand-err-msg"} + actualCondition := internalMemberCluster.GetCondition(expectedCondition.Type) + assert.Equal(t, "", cmp.Diff(expectedCondition, *(actualCondition), cmpopts.IgnoreTypes(time.Time{})), utils.TestCaseMsg, "TestMarkInternalMemberClusterHeartbeatUnhealthy") } func TestUpdateInternalMemberClusterWithRetry(t *testing.T) { diff --git a/pkg/controllers/membercluster/membercluster_controller.go b/pkg/controllers/membercluster/membercluster_controller.go index ca4c5894b..b5c15e2f7 100644 --- a/pkg/controllers/membercluster/membercluster_controller.go +++ b/pkg/controllers/membercluster/membercluster_controller.go @@ -420,7 +420,7 @@ func markMemberClusterLeft(recorder record.EventRecorder, mc apis.ConditionedObj Reason: reasonMemberClusterLeft, ObservedGeneration: mc.GetGeneration(), } - mc.SetConditions(leftCondition, utils.ReconcileSuccessCondition()) + mc.SetConditions(leftCondition) } // createRole creates role for member cluster. diff --git a/pkg/utils/common.go b/pkg/utils/common.go index 9bc06f37e..1bc0b2aaa 100644 --- a/pkg/utils/common.go +++ b/pkg/utils/common.go @@ -15,7 +15,6 @@ import ( corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/discovery" "k8s.io/client-go/util/retry" @@ -31,22 +30,6 @@ const ( RoleBindingNameFormat = "fleet-rolebinding-%s" ) -// Condition types. -const ( - // ConditionTypeReady resources are believed to be ready to handle work. - ConditionTypeReady string = "Ready" - - // ConditionTypeSynced resources are believed to be in sync with the - // Kubernetes resources that manage their lifecycle. - ConditionTypeSynced string = "Synced" -) - -// Reasons a resource is or is not synced. -const ( - ReasonReconcileSuccess string = "ReconcileSuccess" - ReasonReconcileError string = "ReconcileError" -) - var ( ClusterResourcePlacementGVR = schema.GroupVersionResource{ Group: fleetv1alpha1.GroupVersion.Group, @@ -97,26 +80,6 @@ var ( } ) -// ReconcileErrorCondition returns a condition indicating that we encountered an -// error while reconciling the resource. -func ReconcileErrorCondition(err error) metav1.Condition { - return metav1.Condition{ - Type: ConditionTypeSynced, - Status: metav1.ConditionFalse, - Reason: ReasonReconcileError, - Message: err.Error(), - } -} - -// ReconcileSuccessCondition returns a condition indicating that we successfully reconciled the resource -func ReconcileSuccessCondition() metav1.Condition { - return metav1.Condition{ - Type: ConditionTypeSynced, - Status: metav1.ConditionTrue, - Reason: ReasonReconcileSuccess, - } -} - func RandSecureInt(limit int64) int64 { nBig, err := rand.Int(rand.Reader, big.NewInt(limit)) if err != nil {