From 3b9f4f18ed2c6b64a0fb9c92ad58006f47435285 Mon Sep 17 00:00:00 2001 From: Lan Luo Date: Thu, 3 Aug 2023 18:10:16 +0800 Subject: [PATCH] Fix a bug that ClusterSet status is no longer updated 1. r.Update() will only update Spec but not status. Revert the change back to update status only. 2. Update the ClusterSet's ClusterID in ClusterSet's reconciler. 3. Refine test codes for leader ClusterSet controller. Signed-off-by: Lan Luo --- .../leader/clusterset_controller.go | 22 ++- .../leader/clusterset_controller_test.go | 151 ++++++++++++------ .../member/clusterset_controller.go | 21 ++- .../member/clusterset_controller_test.go | 70 ++++++++ 4 files changed, 203 insertions(+), 61 deletions(-) diff --git a/multicluster/controllers/multicluster/leader/clusterset_controller.go b/multicluster/controllers/multicluster/leader/clusterset_controller.go index 8741ac41c40..ce6fad715b6 100644 --- a/multicluster/controllers/multicluster/leader/clusterset_controller.go +++ b/multicluster/controllers/multicluster/leader/clusterset_controller.go @@ -92,6 +92,19 @@ func (r *LeaderClusterSetReconciler) Reconcile(ctx context.Context, req ctrl.Req err = fmt.Errorf("local cluster %s is not defined as leader in ClusterSet", r.clusterID) return ctrl.Result{}, err } + + if clusterSet.Spec.ClusterID == "" { + // ClusterID is a required feild, and the empty value case should only happen + // when Antrea Multi-cluster is upgraded from an old version prior to v1.13. + // Here we try to update the ClusterSet's ClusterID when it's configured in an + // existing ClusterClaim. + clusterSet.Spec.ClusterID = string(r.clusterID) + err = r.Update(context.TODO(), clusterSet) + if err != nil { + klog.ErrorS(err, "Failed to update ClusterSet's ClusterID", "clusterset", req.NamespacedName) + return ctrl.Result{}, err + } + } } r.clusterSetConfig = clusterSet.DeepCopy() @@ -199,14 +212,7 @@ func (r *LeaderClusterSetReconciler) updateStatus() { status.Conditions = []mcv1alpha2.ClusterSetCondition{overallCondition} } clusterSet.Status = status - if clusterSet.Spec.ClusterID == "" { - // When the common area is not empty but ClusterID is empty, it means the - // CR was created by an old version of ClusterSet CRD. We can use the ClusterID - // 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) + 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..21e98fa0dd5 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,26 @@ func TestLeaderClusterSetAdd(t *testing.T) { Namespace: "mcs1", }, } +) +func createMockClients(t *testing.T, objects ...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(objects...).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 := createMockClients(t, existingClusterSet) + leaderClusterSetReconcilerUnderTest := LeaderClusterSetReconciler{ Client: fakeRemoteClient, Scheme: scheme, StatusManager: mockStatusManager, } - req := ctrl.Request{ NamespacedName: types.NamespacedName{ Namespace: "mcs1", @@ -83,8 +111,60 @@ func TestLeaderClusterSetAdd(t *testing.T) { assert.Equal(t, "leader1", string(leaderClusterSetReconcilerUnderTest.clusterID)) } +func TestLeaderClusterSetAddWithoutClusterID(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", + }, + } + clusterClaim := &mcv1alpha2.ClusterClaim{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "mcs1", + Name: "id.k8s.io", + }, + Value: "leader1", + } + scheme, fakeRemoteClient, mockStatusManager := createMockClients(t, clusterSetWithoutClusterID, clusterClaim) + leaderClusterSetReconcilerUnderTest := LeaderClusterSetReconciler{ + Client: fakeRemoteClient, + Scheme: scheme, + StatusManager: mockStatusManager, + ClusterCalimCRDAvailable: true, + } + req := ctrl.Request{ + NamespacedName: types.NamespacedName{ + Namespace: "mcs1", + Name: "clusterset1", + }, + } + _, err := leaderClusterSetReconcilerUnderTest.Reconcile(context.TODO(), req) + assert.Equal(t, nil, err) + assert.Equal(t, "clusterset1", string(leaderClusterSetReconcilerUnderTest.clusterSetID)) + assert.Equal(t, "leader1", string(leaderClusterSetReconcilerUnderTest.clusterID)) + + 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) +} + func TestLeaderClusterSetUpdate(t *testing.T) { - TestLeaderClusterSetAdd(t) + scheme, fakeRemoteClient, mockStatusManager := createMockClients(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 +190,13 @@ func TestLeaderClusterSetUpdate(t *testing.T) { } func TestLeaderClusterSetDelete(t *testing.T) { - TestLeaderClusterSetAdd(t) + scheme, fakeRemoteClient, mockStatusManager := createMockClients(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 +217,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 := createMockClients(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{} diff --git a/multicluster/controllers/multicluster/member/clusterset_controller.go b/multicluster/controllers/multicluster/member/clusterset_controller.go index d161672625f..fc1a45c25ef 100644 --- a/multicluster/controllers/multicluster/member/clusterset_controller.go +++ b/multicluster/controllers/multicluster/member/clusterset_controller.go @@ -121,6 +121,18 @@ func (r *MemberClusterSetReconciler) Reconcile(ctx context.Context, req ctrl.Req return ctrl.Result{}, err } r.clusterSetID = common.ClusterSetID(clusterSet.Name) + if clusterSet.Spec.ClusterID == "" { + // ClusterID is a required feild, and the empty value case should only happen + // when Antrea Multi-cluster is upgraded from an old version prior to v1.13. + // Here we try to update the ClusterSet's ClusterID when it's configured in an + // existing ClusterClaim. + clusterSet.Spec.ClusterID = string(r.clusterID) + err = r.Update(context.TODO(), clusterSet) + if err != nil { + klog.ErrorS(err, "Failed to update ClusterSet's ClusterID", "clusterset", req.NamespacedName) + return ctrl.Result{}, err + } + } } r.clusterSetConfig = clusterSet.DeepCopy() @@ -354,14 +366,7 @@ func (r *MemberClusterSetReconciler) updateStatus() { status.Conditions = []mcv1alpha2.ClusterSetCondition{overallCondition} } clusterSet.Status = status - if clusterSet.Spec.ClusterID == "" { - // When the common area is not empty but ClusterID is empty, it means the - // CR was created by an old version of ClusterSet CRD. We can use the ClusterID - // 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) + 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..79cd1dbbb96 100644 --- a/multicluster/controllers/multicluster/member/clusterset_controller_test.go +++ b/multicluster/controllers/multicluster/member/clusterset_controller_test.go @@ -23,7 +23,10 @@ import ( v1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + k8sscheme "k8s.io/client-go/kubernetes/scheme" + ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -252,3 +255,70 @@ func TestMemberCreateOrUpdateRemoteCommonArea(t *testing.T) { assert.Equal(t, nil, err) assert.Equal(t, expectedInstalledLeader, reconciler.installedLeader) } + +func TestLeaderClusterSetAddWithoutClusterID(t *testing.T) { + existingSecret := &v1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "mcs1", + Name: "membertoken", + }, + Data: map[string][]byte{ + "ca.crt": []byte(`12345`), + "token": []byte(`12345`)}, + } + clusterSetWithoutClusterID := &mcv1alpha2.ClusterSet{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "mcs1", + Name: "clusterset1", + Generation: 1, + }, + Spec: mcv1alpha2.ClusterSetSpec{ + Leaders: []mcv1alpha2.LeaderClusterInfo{ + { + ClusterID: "leader1", + Secret: "membertoken", + }}, + Namespace: "mcs1", + }, + } + clusterClaim := &mcv1alpha2.ClusterClaim{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "mcs1", + Name: "id.k8s.io", + }, + Value: "member1", + } + + scheme := runtime.NewScheme() + mcv1alpha2.AddToScheme(scheme) + k8sscheme.AddToScheme(scheme) + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(clusterSetWithoutClusterID, clusterClaim, existingSecret).Build() + + mockCtrl := gomock.NewController(t) + mockManager := mocks.NewMockManager(mockCtrl) + mockManager.EXPECT().GetClient() + mockManager.EXPECT().GetScheme() + getRemoteConfigAndClient = commonarea.FuncGetFakeRemoteConfigAndClient(mockManager) + + reconciler := MemberClusterSetReconciler{ + Client: fakeClient, + ClusterCalimCRDAvailable: true, + } + if _, err := reconciler.Reconcile(common.TestCtx, ctrl.Request{ + NamespacedName: types.NamespacedName{ + Namespace: "mcs1", + Name: "clusterset1", + }, + }); err != nil { + t.Errorf("Member ClusterSet Reconciler should handle add event successfully but got error = %v", err) + } else { + assert.Equal(t, nil, err) + assert.Equal(t, "clusterset1", string(reconciler.clusterSetID)) + assert.Equal(t, "member1", string(reconciler.clusterID)) + + clusterSet := &mcv1alpha2.ClusterSet{} + err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: "clusterset1", Namespace: "mcs1"}, clusterSet) + assert.Equal(t, nil, err) + assert.Equal(t, "member1", clusterSet.Spec.ClusterID) + } +}