Refactor Common Area and remove leader election #3964

luolanzone · 2022-07-04T06:32:17Z

Antrea Multi-cluster will support only one leader cluster. There is
no need to set up CommonArea manager and do leader election, so clean up
remote CommonArea manager related codes and refactor member ClusterSet
reconciler to handle connection/disconnection cases between member and
leader clusters
Remove unused ClusterSet webhook
Add schema to validate the number of leader cluster in ClusterSet CR.

Signed-off-by: Lan Luo [email protected]

luolanzone · 2022-07-04T06:32:32Z

/test-multicluster-e2e

codecov-commenter · 2022-07-04T06:54:38Z

Codecov Report

Merging #3964 (9a0f51a) into main (1b4d55e) will increase coverage by 1.56%.
The diff coverage is 50.93%.

❗ Current head 9a0f51a differs from pull request most recent head 9ac61cf. Consider uploading reports for the commit 9ac61cf to get more accurate results

@@            Coverage Diff             @@
##             main    #3964      +/-   ##
==========================================
+ Coverage   64.34%   65.90%   +1.56%     
==========================================
  Files         294      308      +14     
  Lines       43634    44090     +456     
==========================================
+ Hits        28076    29058     +982     
+ Misses      13282    12705     -577     
- Partials     2276     2327      +51

Flag	Coverage Δ		*Carryforward flag
e2e-tests	`60.67% <69.79%> (?)`
kind-e2e-tests	`51.23% <38.03%> (+0.70%)`	⬆️	Carriedforward from 23ef6f7
unit-tests	`44.28% <15.80%> (-0.06%)`	⬇️	Carriedforward from 23ef6f7

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files	Coverage Δ
...ter/apis/multicluster/v1alpha1/clusterset_types.go	`100.00% <ø> (ø)`
...icluster/cmd/multicluster-controller/controller.go	`55.17% <ø> (+46.83%)`	⬆️
pkg/agent/agent_linux.go	`5.36% <0.00%> (+0.22%)`	⬆️
pkg/agent/cniserver/pod_configuration.go	`54.59% <0.00%> (ø)`
pkg/agent/proxy/endpoints.go	`78.57% <ø> (ø)`
pkg/agent/util/net.go	`48.99% <0.00%> (-3.39%)`	⬇️
pkg/agent/util/net_linux.go	`29.12% <0.00%> (-5.08%)`	⬇️
pkg/features/antrea_features.go	`11.11% <ø> (ø)`
pkg/ovs/ovsconfig/ovs_client.go	`46.77% <25.92%> (-1.03%)`	⬇️
pkg/agent/route/route_linux.go	`47.11% <36.84%> (-2.56%)`	⬇️
... and 78 more

luolanzone · 2022-07-04T07:41:02Z

/test-multicluster-e2e
/test-multicluster-integration

luolanzone · 2022-07-05T08:54:22Z

/test-multicluster-integration

multicluster/controllers/multicluster/commonarea/remote_common_area_manager.go

multicluster/controllers/multicluster/memberclusterannounce_controller.go

jianjuns · 2022-07-05T20:54:56Z

multicluster/controllers/multicluster/memberclusterannounce_controller.go

 				memberAnnounce.ClusterID)
-			data.leaderStatus.reason = ReasonNotElectedLeader
+			data.leaderStatus.reason = ReasonNotConnectedLeader


This is confusing. Here, the cluster is a leader but not connected, or it is not a leader at all? We should revise either the message or the reason, depending on which one is true.

It's a temporary status indicates the cluster is not confirmed as the leader yet before validation on the leader is passed. It should be overwritten soon as long as the following validation codes passed. I renamed it as ReasonNotLeader.

multicluster/controllers/multicluster/commonarea/remote_common_area_manager.go

multicluster/controllers/multicluster/commonarea/leader_checker.go

multicluster/apis/multicluster/v1alpha1/clusterset_webhook.go

luolanzone · 2022-07-06T08:36:41Z

/test-multicluster-e2e

multicluster/controllers/multicluster/commonarea/remote_common_area.go

multicluster/controllers/multicluster/memberclusterannounce_controller.go

multicluster/controllers/multicluster/commonarea/remote_common_area_manager.go

multicluster/apis/multicluster/v1alpha1/clusterset_types.go

luolanzone · 2022-07-07T06:50:35Z

/test-multicluster-e2e

jianjuns · 2022-07-07T18:32:46Z

multicluster/controllers/multicluster/commonarea/remote_common_area_manager.go

 						rc.Stop()
 					}
 					delete(r.remoteCommonAreas, clusterID)
 				}
 			case <-ticker.C:
-				r.RunLeaderElection()
+				// Check the leader connection status periodically.


I feel we should revisit the relation between remoteCommonAreaManager and RemoteCommonArea. If we just have a single leader cluster, then we should just have one RemoteCommonArea? Why we still need the complex logics of multiple RemoteCommonAreas, and async updates between remoteCommonAreaManager and RemoteCommonArea?

Probably we should think about the implementation from scratch, rather than trying to minimize the changes based on the existing code that wants to cover multiple leaders.

Sure, I will check how to refine this part.

luolanzone · 2022-07-11T09:17:41Z

/test-multicluster-e2e

luolanzone · 2022-07-11T09:53:34Z

/test-multicluster-e2e

multicluster/controllers/multicluster/commonarea/remote_common_area.go

jianjuns · 2022-07-12T00:22:13Z

multicluster/controllers/multicluster/commonarea/remote_common_area.go

+	// Client grants read/write to the Namespace of the cluster that is backing this CommonArea.
+	client.Client
+
+	// GetClusterID returns the clusterID of the cluster accessed by this CommonArea.


Do you mean "cluster ID of the leader cluster"?

yes, comment is updated.

multicluster/controllers/multicluster/commonarea/remote_common_area.go

multicluster/controllers/multicluster/member_clusterset_controller.go

jianjuns · 2022-07-12T00:48:11Z

multicluster/controllers/multicluster/member_clusterset_controller.go

+	}
+}
+
+func (r *MemberClusterSetReconciler) updateActiveLeader(cluster commonarea.RemoteCommonArea) {


I feel we should remove this func, and put the logics into checkLeaderConnection() directly, as the two cases (cluster == nil and cluster != nil) should share no common code.

It's removed now.

jianjuns · 2022-07-12T00:50:05Z

multicluster/controllers/multicluster/member_clusterset_controller.go

+	r.remoteCommonArea = cluster
+	if cluster != nil {
+		if err := cluster.StartWatching(); err != nil {
+			klog.ErrorS(err, "Failed to start watching events")


What will happen? Should we retry StartWatching()？Should we set r.installedLeader.connected = true?

This part has been refined and moved to remote common area now.

jianjuns · 2022-07-12T00:51:08Z

multicluster/controllers/multicluster/member_clusterset_controller.go

-	remoteCommonAreas := r.remoteCommonAreaManager.GetRemoteCommonAreas()
-	if len(remoteCommonAreas) <= 0 {
-		return nil, "", errors.New("ClusterSet has not been set up properly, no available Common Area")
+	if r.remoteCommonArea.IsConnected() {


Here it is possible StartWatching() not called yet. Is that ok?

I think it's OK, StartWatching() is for importer part. This GetRemoteCommonAreaAndLocalID is only used for exporter. Exporter can work as long as the client can talk to leader cluster.

multicluster/controllers/multicluster/member_clusterset_controller.go

luolanzone · 2022-07-12T04:01:31Z

/test-multicluster-e2e

jianjuns · 2022-07-12T20:19:19Z

multicluster/controllers/multicluster/commonarea/remote_common_area.go

+	// Client grants read/write to the Namespace of the cluster that is backing this CommonArea.
+	client.Client
+
+	// GetClusterID returns the clusterID of the leader cluster accessed by this CommonArea.


Remove "accessed by this CommonArea" (do not understand what it means).

jianjuns · 2022-07-12T20:22:43Z

multicluster/controllers/multicluster/commonarea/remote_common_area.go

-		string(r.remoteCommonAreaManager.GetLocalClusterID()),
-		r.remoteCommonAreaManager.GetNamespace(),
+		string(r.GetLocalClusterID()),
+		r.localNamespace,


Was this a bug?

No, RemoteCommonArea doesn't have local Cluster ID/Namespace information, we set it in RemoteCommonAreaManger before. Since I deleted the RemoteCommonAreaManger, the localNamespace is passed to RemoteCommonArea now.

multicluster/controllers/multicluster/member_clusterset_controller.go

multicluster/controllers/multicluster/commonarea/remote_common_area.go

multicluster/controllers/multicluster/member_clusterset_controller.go

jianjuns · 2022-07-13T20:36:01Z

multicluster/controllers/multicluster/member_clusterset_controller.go

 // MemberClusterSetReconciler reconciles a ClusterSet object in the member cluster deployment.
 type MemberClusterSetReconciler struct {
 	client.Client
 	Scheme    *runtime.Scheme
 	Namespace string
-	mutex     sync.RWMutex
+	// commonAreaLock protects the access to RemoteCommonArea which presents the leader cluster.


Remove "which presents the leader cluster"

jianjuns · 2022-07-13T20:41:30Z

multicluster/controllers/multicluster/member_clusterset_controller.go

@@ -250,12 +240,20 @@ func (r *MemberClusterSetReconciler) updateStatus() {
 	status := multiclusterv1alpha1.ClusterSetStatus{}
 	status.TotalClusters = int32(len(r.clusterSetConfig.Spec.Members))
 	status.ObservedGeneration = r.clusterSetConfig.Generation
-	status.ClusterStatuses = r.remoteCommonAreaManager.GetMemberClusterStatues()
+	status.ClusterStatuses = []multiclusterv1alpha1.ClusterStatus{}
+	if r.remoteCommonArea != nil {


We just need to lock from this line to line 251?

You are right, changed the scope.

multicluster/controllers/multicluster/member_clusterset_controller.go

jianjuns · 2022-07-13T20:47:28Z

multicluster/controllers/multicluster/member_clusterset_controller.go

-		secretName := addedLeader.Secret
+		klog.InfoS("ClusterSet update", "old", currentCommonArea.GetClusterID(), "new", newLeader)
+		klog.InfoS("Stopping old RemoteCommonArea", "cluster", clusterID)
+		r.remoteCommonArea.Stop()


Should we check error? I saw you checked Stop() error at line 95. Could you check Stop() can really fail or not?

Nit: consider currentCommonArea.Stop()

multicluster/controllers/multicluster/member_clusterset_controller.go

jianjuns · 2022-07-13T20:51:47Z

multicluster/controllers/multicluster/member_clusterset_controller.go

+	klog.InfoS("Creating RemoteCommonArea", "cluster", clusterID)
+	// Read secret to access the leader cluster. Assume secret is present in the same Namespace as the ClusterSet.
+	secret, err := r.getSecretForLeader(secretName, clusterSet.GetNamespace())
+	if err == nil {


I suggest you change the block to:

if err != nil { klog.ErrorS(err, "Failed to get Secret to create RemoteCommonArea", "secret", secretName, "cluster", clusterID) return err } ... return nil

luolanzone · 2022-07-14T03:26:25Z

/test-multicluster-e2e

jianjuns

LGTM

jianjuns · 2022-07-15T00:12:24Z

/test-all

1. Antrea Multi-cluster will support only one leader cluster. There is no need to set up common area manager and do leader election, so clean up remote common area manager related codes and refactor member clusterset reconciler to handle connection/disconnection cases between member and leader clusters 2. Remove unused clusterset webhook 3. Add schema to valid the number of leader cluster in ClusterSet CR. Signed-off-by: Lan Luo <[email protected]>

luolanzone · 2022-07-18T07:13:11Z

/test-multicluster-e2e
/test-all

@jianjuns I fixed a go.mod issue and do a force push again, could you help to double check if this PR is Ok to merge?
I created another PR #4026 based on this, it would better if you take a look at both, I will do a rebase once this one is merged. Thanks.

luolanzone requested a review from aravindakidambi July 4, 2022 06:32

luolanzone requested a review from jianjuns July 4, 2022 06:33

luolanzone added the area/multi-cluster Issues or PRs related to multi cluster. label Jul 4, 2022

luolanzone force-pushed the remove-leader-election branch from f2b291e to 5336ee7 Compare July 4, 2022 07:08

jianjuns reviewed Jul 5, 2022

View reviewed changes

luolanzone force-pushed the remove-leader-election branch 2 times, most recently from 3abf519 to a31e7f3 Compare July 6, 2022 08:35

jianjuns reviewed Jul 6, 2022

View reviewed changes

luolanzone force-pushed the remove-leader-election branch from a31e7f3 to 9bbea2f Compare July 7, 2022 06:48

jianjuns reviewed Jul 7, 2022

View reviewed changes

luolanzone force-pushed the remove-leader-election branch from 9bbea2f to 4e6bb04 Compare July 11, 2022 09:08

luolanzone changed the title ~~Refactor leader election related codes as a checker~~ Refactor common area and remove leader election Jul 11, 2022

luolanzone force-pushed the remove-leader-election branch from 4e6bb04 to 8837ba8 Compare July 11, 2022 09:16

jianjuns reviewed Jul 12, 2022

View reviewed changes

luolanzone force-pushed the remove-leader-election branch from 8837ba8 to a3c2844 Compare July 12, 2022 04:01

jianjuns reviewed Jul 12, 2022

View reviewed changes

multicluster/controllers/multicluster/member_clusterset_controller.go Outdated Show resolved Hide resolved

multicluster/controllers/multicluster/member_clusterset_controller.go Show resolved Hide resolved

luolanzone force-pushed the remove-leader-election branch from a3c2844 to 1e65ddf Compare July 13, 2022 03:55

luolanzone added this to the Antrea v1.8 release milestone Jul 13, 2022

jianjuns reviewed Jul 13, 2022

View reviewed changes

luolanzone force-pushed the remove-leader-election branch from 1e65ddf to 750a42f Compare July 14, 2022 02:16

jianjuns previously approved these changes Jul 15, 2022

View reviewed changes

jianjuns changed the title ~~Refactor common area and remove leader election~~ Refactor Common Area and remove leader election Jul 15, 2022

luolanzone mentioned this pull request Jul 18, 2022

Remove MemberClusterAnnounce if ClusterSet deleted #4026

Merged

luolanzone dismissed jianjuns’s stale review via 9ac61cf July 18, 2022 07:09

luolanzone force-pushed the remove-leader-election branch from 750a42f to 9ac61cf Compare July 18, 2022 07:09

jianjuns approved these changes Jul 18, 2022

View reviewed changes

jianjuns merged commit 1655f9f into antrea-io:main Jul 18, 2022

luolanzone mentioned this pull request Jul 21, 2022

Auto update ClusterSet and MemberClusterAnnounce in leader cluster #3956

Merged

luolanzone mentioned this pull request Aug 16, 2022

Label Identity replication for MCS #3664

Merged

Refactor Common Area and remove leader election #3964

Refactor Common Area and remove leader election #3964

Conversation

luolanzone commented Jul 4, 2022 • edited by jianjuns Loading

luolanzone commented Jul 4, 2022

codecov-commenter commented Jul 4, 2022 • edited by codecov bot Loading

Codecov Report

luolanzone commented Jul 4, 2022

luolanzone commented Jul 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luolanzone commented Jul 6, 2022

luolanzone commented Jul 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luolanzone commented Jul 11, 2022

luolanzone commented Jul 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luolanzone commented Jul 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luolanzone Jul 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luolanzone commented Jul 14, 2022

jianjuns left a comment

Choose a reason for hiding this comment

jianjuns commented Jul 15, 2022

luolanzone commented Jul 18, 2022

luolanzone commented Jul 4, 2022 •

edited by jianjuns

Loading

codecov-commenter commented Jul 4, 2022 •

edited by codecov bot

Loading

luolanzone Jul 13, 2022 •

edited

Loading