Skip to content

fix: Remove unuseful condition: Synced #209

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ryanzhang-oss merged 1 commit into from
Jul 29, 2022
Merged

fix: Remove unuseful condition: Synced #209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions pkg/controllers/internalmembercluster/member_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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.
Expand Down
78 changes: 19 additions & 59 deletions pkg/controllers/internalmembercluster/member_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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
Expand All @@ -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) {
Expand All @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion pkg/controllers/membercluster/membercluster_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
37 changes: 0 additions & 37 deletions pkg/utils/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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,
Expand Down Expand Up @@ -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 {
Expand Down