Skip to content

Commit

Permalink
Fix a bug that ClusterSet status is no longer updated
Browse files Browse the repository at this point in the history 
1. r.Update() will only update Spec but not status, revise the logic
to update the Spec first and retry in next interval to update
ClusterSet's Status.
2. Refine test codes for leader ClusterSet controller.

Signed-off-by: Lan Luo <luola@vmware.com>
  • Loading branch information
@luolanzone
luolanzone committed Aug 1, 2023
1 parent 1d253bf commit 709adb6
Show file tree
Hide file tree
Showing 4 changed files with 150 additions and 60 deletions.
9 changes: 8 additions & 1 deletion multicluster/controllers/multicluster/leader/clusterset_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -205,8 +205,15 @@ func (r *LeaderClusterSetReconciler) updateStatus() {
// from ClusterClaim to update the CR, otherwise, the update will fail due
// to invalid ClusterID.
clusterSet.Spec.ClusterID = string(r.clusterID)
err = r.Update(context.TODO(), clusterSet)
if err != nil {
klog.ErrorS(err, "Failed to update ClusterSet's ClusterID", "name", namespacedName)
}
// Status will be updated in next interval after the ClusterSet spec is updated.
return
}
err = r.Update(context.TODO(), clusterSet)

err = r.Status().Update(context.TODO(), clusterSet)
if err != nil {
klog.ErrorS(err, "Failed to update Status of ClusterSet", "name", namespacedName)
}
Expand Down
138 changes: 93 additions & 45 deletions multicluster/controllers/multicluster/leader/clusterset_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,35 @@ import (
)

var (
fakeRemoteClient client.Client
leaderClusterSetReconcilerUnderTest LeaderClusterSetReconciler
mockStatusManager *MockMemberClusterStatusManager
)

func TestLeaderClusterSetAdd(t *testing.T) {
existingClusterSet := &mcv1alpha2.ClusterSet{
eventTime = time.Date(2021, 12, 12, 12, 12, 12, 0, time.Local)
metaTime = metav1.Time{Time: eventTime}
statuses = []mcv1alpha2.ClusterStatus{
{
ClusterID: "east",
Conditions: []mcv1alpha2.ClusterCondition{
{
LastTransitionTime: metaTime,
Message: "Member Connected",
Reason: "Connected",
Status: v1.ConditionTrue,
Type: mcv1alpha2.ClusterReady,
},
},
},
{
ClusterID: "west",
Conditions: []mcv1alpha2.ClusterCondition{
{
LastTransitionTime: metaTime,
Message: "Member Connected",
Reason: "Disconnected",
Status: v1.ConditionFalse,
Type: mcv1alpha2.ClusterReady,
},
},
},
}
existingClusterSet = &mcv1alpha2.ClusterSet{
ObjectMeta: metav1.ObjectMeta{
Namespace: "mcs1",
Name: "clusterset1",
Expand All @@ -56,20 +78,25 @@ func TestLeaderClusterSetAdd(t *testing.T) {
Namespace: "mcs1",
},
}
)

func createMockClient(t *testing.T, object client.Object) (*runtime.Scheme, client.Client, *MockMemberClusterStatusManager) {
scheme := runtime.NewScheme()
mcv1alpha2.AddToScheme(scheme)
fakeRemoteClient = fake.NewClientBuilder().WithScheme(scheme).
WithObjects(existingClusterSet).Build()
fakeRemoteClient := fake.NewClientBuilder().WithScheme(scheme).
WithObjects(object).Build()

mockCtrl := gomock.NewController(t)
mockStatusManager = NewMockMemberClusterStatusManager(mockCtrl)
leaderClusterSetReconcilerUnderTest = LeaderClusterSetReconciler{
mockStatusManager := NewMockMemberClusterStatusManager(mockCtrl)
return scheme, fakeRemoteClient, mockStatusManager
}
func TestLeaderClusterSetAdd(t *testing.T) {
scheme, fakeRemoteClient, mockStatusManager := createMockClient(t, existingClusterSet)
leaderClusterSetReconcilerUnderTest := LeaderClusterSetReconciler{
Client: fakeRemoteClient,
Scheme: scheme,
StatusManager: mockStatusManager,
}

req := ctrl.Request{
NamespacedName: types.NamespacedName{
Namespace: "mcs1",
Expand All @@ -84,7 +111,13 @@ func TestLeaderClusterSetAdd(t *testing.T) {
}

func TestLeaderClusterSetUpdate(t *testing.T) {
TestLeaderClusterSetAdd(t)
scheme, fakeRemoteClient, mockStatusManager := createMockClient(t, existingClusterSet)
leaderClusterSetReconcilerUnderTest := LeaderClusterSetReconciler{
Client: fakeRemoteClient,
Scheme: scheme,
StatusManager: mockStatusManager,
clusterSetConfig: existingClusterSet.DeepCopy(),
}
clusterSet := &mcv1alpha2.ClusterSet{}
err := fakeRemoteClient.Get(context.TODO(), types.NamespacedName{Name: "clusterset1", Namespace: "mcs1"}, clusterSet)
assert.Equal(t, nil, err)
Expand All @@ -110,7 +143,13 @@ func TestLeaderClusterSetUpdate(t *testing.T) {
}

func TestLeaderClusterSetDelete(t *testing.T) {
TestLeaderClusterSetAdd(t)
scheme, fakeRemoteClient, mockStatusManager := createMockClient(t, existingClusterSet)
leaderClusterSetReconcilerUnderTest := LeaderClusterSetReconciler{
Client: fakeRemoteClient,
Scheme: scheme,
StatusManager: mockStatusManager,
clusterSetConfig: existingClusterSet.DeepCopy(),
}
clusterSet := &mcv1alpha2.ClusterSet{}
err := fakeRemoteClient.Get(context.TODO(), types.NamespacedName{Name: "clusterset1", Namespace: "mcs1"}, clusterSet)
assert.Equal(t, nil, err)
Expand All @@ -131,40 +170,15 @@ func TestLeaderClusterSetDelete(t *testing.T) {
}

func TestLeaderClusterStatus(t *testing.T) {
TestLeaderClusterSetAdd(t)

eventTime := time.Date(2021, 12, 12, 12, 12, 12, 0, time.Local)
metaTime := metav1.Time{Time: eventTime}

statuses := []mcv1alpha2.ClusterStatus{
{
ClusterID: "east",
Conditions: []mcv1alpha2.ClusterCondition{
{
LastTransitionTime: metaTime,
Message: "Member Connected",
Reason: "Connected",
Status: v1.ConditionTrue,
Type: mcv1alpha2.ClusterReady,
},
},
},
{
ClusterID: "west",
Conditions: []mcv1alpha2.ClusterCondition{
{
LastTransitionTime: metaTime,
Message: "Member Connected",
Reason: "Disconnected",
Status: v1.ConditionFalse,
Type: mcv1alpha2.ClusterReady,
},
},
},
scheme, fakeRemoteClient, mockStatusManager := createMockClient(t, existingClusterSet)
leaderClusterSetReconcilerUnderTest := LeaderClusterSetReconciler{
Client: fakeRemoteClient,
Scheme: scheme,
StatusManager: mockStatusManager,
clusterSetConfig: existingClusterSet.DeepCopy(),
}

mockStatusManager.EXPECT().GetMemberClusterStatuses().Return(statuses).Times(1)

leaderClusterSetReconcilerUnderTest.updateStatus()

clusterSet := &mcv1alpha2.ClusterSet{}
Expand Down Expand Up @@ -196,3 +210,37 @@ func TestLeaderClusterStatus(t *testing.T) {
assert.Equal(t, expectedStatus.Conditions[0].Status, actualStatus.Conditions[0].Status)
assert.Equal(t, expectedStatus.Conditions[0].Message, actualStatus.Conditions[0].Message)
}

func TestLeaderClusterStatusWithoutClusterID(t *testing.T) {
clusterSetWithoutClusterID := &mcv1alpha2.ClusterSet{
ObjectMeta: metav1.ObjectMeta{
Namespace: "mcs1",
Name: "clusterset1",
Generation: 1,
},
Spec: mcv1alpha2.ClusterSetSpec{
Leaders: []mcv1alpha2.LeaderClusterInfo{
{
ClusterID: "leader1",
}},
Namespace: "mcs1",
},
}

scheme, fakeRemoteClient, mockStatusManager := createMockClient(t, clusterSetWithoutClusterID)
leaderClusterSetReconcilerUnderTest := LeaderClusterSetReconciler{
Client: fakeRemoteClient,
Scheme: scheme,
StatusManager: mockStatusManager,
clusterID: "leader1",
clusterSetConfig: clusterSetWithoutClusterID.DeepCopy(),
}

mockStatusManager.EXPECT().GetMemberClusterStatuses().Return(statuses).Times(1)
leaderClusterSetReconcilerUnderTest.updateStatus()

clusterSet := &mcv1alpha2.ClusterSet{}
err := fakeRemoteClient.Get(context.TODO(), types.NamespacedName{Name: "clusterset1", Namespace: "mcs1"}, clusterSet)
assert.Equal(t, nil, err)
assert.Equal(t, "leader1", clusterSet.Spec.ClusterID)
}
9 changes: 8 additions & 1 deletion multicluster/controllers/multicluster/member/clusterset_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -360,8 +360,15 @@ func (r *MemberClusterSetReconciler) updateStatus() {
// from ClusterClaim to update the CR, otherwise, the update will fail due
// to invalid ClusterID.
clusterSet.Spec.ClusterID = string(r.clusterID)
err = r.Update(context.TODO(), clusterSet)
if err != nil {
klog.ErrorS(err, "Failed to update ClusterSet's ClusterID", "name", namespacedName)
}
// Status will be updated in next interval after the ClusterSet spec is updated.
return
}
err = r.Update(context.TODO(), clusterSet)

err = r.Status().Update(context.TODO(), clusterSet)
if err != nil {
klog.ErrorS(err, "Failed to update Status of ClusterSet", "name", namespacedName)
}
Expand Down
54 changes: 41 additions & 13 deletions multicluster/controllers/multicluster/member/clusterset_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,24 @@ func TestMemberClusterStatus(t *testing.T) {
ObservedGeneration: 1,
},
}
fakeClient := fake.NewClientBuilder().WithScheme(common.TestScheme).WithObjects(existingClusterSet).Build()
fakeRemoteClient := fake.NewClientBuilder().WithScheme(common.TestScheme).WithObjects(existingClusterSet).Build()
clusterSetWithClusterID := &mcv1alpha2.ClusterSet{
ObjectMeta: metav1.ObjectMeta{
Namespace: "mcs1",
Name: "clusterset1",
Generation: 1,
},
Spec: mcv1alpha2.ClusterSetSpec{
ClusterID: "leader1",
Leaders: []mcv1alpha2.LeaderClusterInfo{
{
ClusterID: "leader1",
}},
Namespace: "mcs1",
},
Status: mcv1alpha2.ClusterSetStatus{
ObservedGeneration: 1,
},
}
conditions := []mcv1alpha2.ClusterCondition{
{
Message: "Member Connected",
Expand Down Expand Up @@ -148,36 +164,48 @@ func TestMemberClusterStatus(t *testing.T) {
}

tests := []struct {
name string
conditions []mcv1alpha2.ClusterCondition
expectedStatus mcv1alpha2.ClusterSetStatus
name string
conditions []mcv1alpha2.ClusterCondition
expectedStatus mcv1alpha2.ClusterSetStatus
existingClusterSet *mcv1alpha2.ClusterSet
}{
{
name: "with no conditions",
conditions: nil,
expectedStatus: expectedStatusNoCondition,
name: "with no conditions",
conditions: nil,
expectedStatus: expectedStatusNoCondition,
existingClusterSet: existingClusterSet,
},
{
name: "with conditions when ClusterSet is without clusterID",
conditions: conditions,
expectedStatus: expectedStatusWithCondition,
existingClusterSet: existingClusterSet,
},
{
name: "with conditions",
conditions: conditions,
expectedStatus: expectedStatusWithCondition,
name: "with conditions when ClusterSet is with clusterID",
conditions: conditions,
expectedStatus: expectedStatusWithCondition,
existingClusterSet: clusterSetWithClusterID,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
fakeClient := fake.NewClientBuilder().WithScheme(common.TestScheme).WithObjects(tt.existingClusterSet).Build()
fakeRemoteClient := fake.NewClientBuilder().WithScheme(common.TestScheme).WithObjects(tt.existingClusterSet).Build()
commonArea := commonarea.NewFakeRemoteCommonArea(fakeRemoteClient, "leader1", common.LocalClusterID, "mcs1", tt.conditions)
reconciler := MemberClusterSetReconciler{
Client: fakeClient,
remoteCommonArea: commonArea,
clusterSetConfig: existingClusterSet,
clusterSetConfig: tt.existingClusterSet,
clusterSetID: "clusterset1",
clusterID: "east",
clusterID: "leader1",
}

reconciler.updateStatus()
clusterSet := &mcv1alpha2.ClusterSet{}

err := fakeClient.Get(context.TODO(), types.NamespacedName{Name: "clusterset1", Namespace: "mcs1"}, clusterSet)
assert.Equal(t, "leader1", clusterSet.Spec.ClusterID)
assert.Equal(t, nil, err)
assert.Equal(t, tt.expectedStatus.ObservedGeneration, clusterSet.Status.ObservedGeneration)
assert.Equal(t, tt.expectedStatus.TotalClusters, clusterSet.Status.TotalClusters)
Expand Down

0 comments on commit 709adb6

Please sign in to comment.