Skip to content
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

Fix a bug that ClusterSet status is no longer updated #5338

Merged
merged 1 commit into from
Aug 15, 2023
Merged

Fix a bug that ClusterSet status is no longer updated #5338

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
22 changes: 14 additions & 8 deletions multicluster/controllers/multicluster/leader/clusterset_controller.go
View file
Open in desktop
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}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for my understanding, could you still answer my question if updateStatus() is called periodically and why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jianjuns updateStatus() will check the leader's connectivity status in the member cluster's ClusterSet controller, the status update action will be ignored if there is no change. The same function will check the member's connectivity through MemberClusterAnnounce in the leader cluster and the status will also not be updated if there is no change.
I think the main purpose is reflect the connectivity status between leader and member clusters.

}
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)
jianjuns marked this conversation as resolved.
Show resolved Hide resolved
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
Open in desktop
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
Open in desktop
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
Open in desktop
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)
}
}