diff --git a/multicluster/controllers/multicluster/leader/clusterset_controller.go b/multicluster/controllers/multicluster/leader/clusterset_controller.go index 8741ac41c40..2471af53ace 100644 --- a/multicluster/controllers/multicluster/leader/clusterset_controller.go +++ b/multicluster/controllers/multicluster/leader/clusterset_controller.go @@ -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) } diff --git a/multicluster/controllers/multicluster/leader/clusterset_controller_test.go b/multicluster/controllers/multicluster/leader/clusterset_controller_test.go index da563098407..e8d0696cb29 100644 --- a/multicluster/controllers/multicluster/leader/clusterset_controller_test.go +++ b/multicluster/controllers/multicluster/leader/clusterset_controller_test.go @@ -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", @@ -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", @@ -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) @@ -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) @@ -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{} @@ -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) +} diff --git a/multicluster/controllers/multicluster/member/clusterset_controller.go b/multicluster/controllers/multicluster/member/clusterset_controller.go index d161672625f..633012a4dd7 100644 --- a/multicluster/controllers/multicluster/member/clusterset_controller.go +++ b/multicluster/controllers/multicluster/member/clusterset_controller.go @@ -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) } diff --git a/multicluster/controllers/multicluster/member/clusterset_controller_test.go b/multicluster/controllers/multicluster/member/clusterset_controller_test.go index 8dc1ef1a207..43e07191637 100644 --- a/multicluster/controllers/multicluster/member/clusterset_controller_test.go +++ b/multicluster/controllers/multicluster/member/clusterset_controller_test.go @@ -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", @@ -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)