Skip to content

Commit

Permalink
Fix a bug that ClusterSet Status is not updated (#5338)
Browse files Browse the repository at this point in the history 
1. ClusterSetReconciler.Update() now only updates ClusterSet Spec but not
Status. Change it back to update only Status.
2. Update the ClusterSet's ClusterID at the ClusterSet reconciliation.
3. Revise test code for leader ClusterSet controller.

Signed-off-by: Lan Luo <[email protected]>
  • Loading branch information
@luolanzone
luolanzone authored Aug 15, 2023
1 parent 6363bdb commit 160f25f
Show file tree
Hide file tree
Showing 4 changed files with 203 additions and 61 deletions.
22 changes: 14 additions & 8 deletions multicluster/controllers/multicluster/leader/clusterset_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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)
}
Expand Down
151 changes: 106 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,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",
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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{}
Expand Down
21 changes: 13 additions & 8 deletions multicluster/controllers/multicluster/member/clusterset_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down Expand Up @@ -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)
}
Expand Down
70 changes: 70 additions & 0 deletions multicluster/controllers/multicluster/member/clusterset_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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)
}
}

0 comments on commit 160f25f

Please sign in to comment.