Auto update ClusterSet and MemberClusterAnnounce in leader cluster #3956

hjiajing · 2022-06-30T06:38:32Z

Auto update ClusetSet and MemberClusterAnnounce in the leader cluster.

When a new member cluster is added to the ClusterSet. The ClusterSet in the leader cluster will update the new member cluster in the spec.members.
When a member cluster departs the ClusterSet. The MemberClusterAnnounce in the leader cluster will be deleted. The member cluster will be removed from the spec.members.

hjiajing · 2022-06-30T06:44:33Z

@luolanzone Could you please take a quick review for this WIP PR. Thanks.
I added an annotation "antrea.io/is-member-deleted" in MemberClusterAnnounce because the leader cluster controller need to get ClusterSetID. So the MemberClusterAnnounce will not be deleted by a member cluster.
Once a MemberClusterAnnounce is annotated "deleted", the leader cluster will remove the member in the member list and delete the MemberClusterAnnounce.
In addition, I removed the member check in the webhook. As well as the SA check. I'm concerned about this. If did this, the member will not need SA any more.

codecov-commenter · 2022-06-30T07:30:04Z

Codecov Report

Merging #3956 (e9b2670) into main (13f470e) will decrease coverage by 0.04%.
The diff coverage is 54.61%.

@@            Coverage Diff             @@
##             main    #3956      +/-   ##
==========================================
- Coverage   64.50%   64.46%   -0.05%     
==========================================
  Files         294      294              
  Lines       43726    43827     +101     
==========================================
+ Hits        28206    28253      +47     
- Misses      13236    13270      +34     
- Partials     2284     2304      +20

Flag	Coverage Δ
kind-e2e-tests	`50.88% <ø> (-0.32%)`	⬇️
unit-tests	`44.23% <54.61%> (+0.01%)`	⬆️

Impacted Files	Coverage Δ
multicluster/cmd/multicluster-controller/leader.go	`0.00% <0.00%> (ø)`
...llers/multicluster/member_clusterset_controller.go	`16.30% <0.00%> (-0.15%)`	⬇️
...luster-controller/memberclusterannounce_webhook.go	`55.00% <56.09%> (-0.56%)`	⬇️
...s/multicluster/memberclusterannounce_controller.go	`64.15% <58.02%> (-7.64%)`	⬇️
...lers/multicluster/commonarea/remote_common_area.go	`27.75% <100.00%> (+0.29%)`	⬆️
...agent/flowexporter/connections/deny_connections.go	`65.59% <0.00%> (-19.36%)`	⬇️
pkg/agent/flowexporter/connections/connections.go	`60.60% <0.00%> (-15.16%)`	⬇️
pkg/agent/flowexporter/exporter/exporter.go	`67.82% <0.00%> (-10.90%)`	⬇️
pkg/agent/controller/trafficcontrol/controller.go	`81.08% <0.00%> (-2.00%)`	⬇️
pkg/controller/grouping/controller.go	`65.13% <0.00%> (-1.98%)`	⬇️
... and 20 more

luolanzone · 2022-06-30T08:55:50Z

@hjiajing Could you please fix the unit test failure? https://github.com/antrea-io/antrea/runs/7126092905?check_suite_focus=true

hjiajing · 2022-06-30T09:01:30Z

@luolanzone Sure. I will fix it now

multicluster/cmd/multicluster-controller/memberclusterannounce_webhook.go

multicluster/controllers/multicluster/member_clusterset_controller.go

multicluster/controllers/multicluster/memberclusterannounce_controller.go

multicluster/cmd/multicluster-controller/memberclusterannounce_webhook.go

multicluster/controllers/multicluster/memberclusterannounce_controller.go

luolanzone · 2022-07-04T02:55:13Z

@hjiajing Could you resolve the conflicts? thanks.

multicluster/cmd/multicluster-controller/memberclusterannounce_webhook.go

multicluster/cmd/multicluster-controller/memberclusterannounce_webhook_test.go

multicluster/cmd/multicluster-controller/memberclusterannounce_webhook.go

luolanzone · 2022-07-12T10:05:01Z

multicluster/cmd/multicluster-controller/memberclusterannounce_webhook.go

-				}
-			}
-		}
+	if len(clusterSet.Spec.Leaders) != 1 {


Maybe you can remove this check and add schema validation like this: https://github.com/antrea-io/antrea/pull/3964/files#diff-d91c75a71bf454b846eb3998698d615fadf6108177e9624428572d6440887962R42-R43,
You can refer to the change to add this refinement to this PR, so it won't have any merge dependency between two commits.

And this one?

multicluster/controllers/multicluster/commonarea/remote_common_area.go

multicluster/controllers/multicluster/member_clusterset_controller.go

multicluster/controllers/multicluster/memberclusterannounce_controller.go

multicluster/cmd/multicluster-controller/memberclusterannounce_webhook.go

multicluster/build/yamls/antrea-multicluster-leader-namespaced.yml

jianjuns · 2022-07-12T23:43:57Z

multicluster/controllers/multicluster/memberclusterannounce_controller.go

 	}
+	// If err != nil, probably ClusterClaims were deleted during the processing of MemberClusterAnnounce.


Why we move the error out of the "if" block?

err here should be returned by r.List() at line 131? If so, I do not feel it is relevant to the comment. @luolanzone : could you check?

@jianjuns yes, you are right. I feel original comment and logic doesn't handle the case correctly. If all ClusterClaims are deleted, it should be empty list instead of an error. I think it should retry if there is an error. @hjiajing Could you double check and refine this part? we may leave ClusterClaim webhook part in a new PR.

multicluster/controllers/multicluster/member_clusterset_controller.go

multicluster/controllers/multicluster/memberclusterannounce_controller.go

jianjuns · 2022-07-14T04:30:24Z

multicluster/controllers/multicluster/memberclusterannounce_controller.go

 	}
+	// If err != nil, probably ClusterClaims were deleted during the processing of MemberClusterAnnounce.


We need to handle MemberClusterAnnounce deletion (comments in line 109 are no longer valid?).

multicluster/controllers/multicluster/memberclusterannounce_controller.go

multicluster/cmd/multicluster-controller/memberclusterannounce_webhook.go

multicluster/controllers/multicluster/member_clusterset_controller.go

multicluster/controllers/multicluster/memberclusterannounce_controller.go

jianjuns · 2022-07-22T21:21:29Z

multicluster/controllers/multicluster/memberclusterannounce_controller.go

@@ -108,6 +111,8 @@ func (r *MemberClusterAnnounceReconciler) Reconcile(ctx context.Context, req ctr
 	r.mapLock.Lock()
 	defer r.mapLock.Unlock()

+	// If !ok, it means member not found. If this happens, the MemberClusterAnnounce should soon be deleted.
+	// Nothing to do here.
 	if data, ok := r.timerData[common.ClusterID(memberAnnounce.ClusterID)]; ok {


@luolanzone : I feel we should revisit all these ClusterSet / cluster status code. I have the following question:

Do we really need leader cluster ID in the MemberAnnounce?

Do we still need all status conditions after we remove election?

Add/RemoveMember() need not to be called by ClusterSet changes, after we change to auto-update ClusterSet, but we can start to maintain the timerdata once MemberClusterAnnounce is created?

Could you please look into the code to understand the whole status update logic, and clean up the code?

Maybe you can first summarize what conditions we have, and they are set for what cases.

@luolanzone : a reminder for you.

Sure, I will check this part, thanks for reminding.

multicluster/controllers/multicluster/memberclusterannounce_controller.go

jianjuns

I do not have further comments for this PR, except for a reminder for adding code comments.

@luolanzone : would you take another look before we can merge?

multicluster/controllers/multicluster/member_clusterset_controller.go

jianjuns · 2022-07-25T17:49:29Z

multicluster/controllers/multicluster/memberclusterannounce_controller.go

@@ -108,6 +111,8 @@ func (r *MemberClusterAnnounceReconciler) Reconcile(ctx context.Context, req ctr
 	r.mapLock.Lock()
 	defer r.mapLock.Unlock()

+	// If !ok, it means member not found. If this happens, the MemberClusterAnnounce should soon be deleted.
+	// Nothing to do here.
 	if data, ok := r.timerData[common.ClusterID(memberAnnounce.ClusterID)]; ok {


@luolanzone : a reminder for you.

luolanzone

@hjiajing Could you rebase the PR? A few questions in the comments

luolanzone · 2022-07-26T01:46:54Z

multicluster/cmd/multicluster-controller/leader.go

@@ -63,10 +64,15 @@ func runLeader(o *Options) error {
 	if err = memberClusterStatusManager.SetupWithManager(mgr); err != nil {
 		return fmt.Errorf("error creating MemberClusterAnnounce controller: %v", err)
 	}
+
+	noCachedClient, err := client.New(mgr.GetConfig(), client.Options{Scheme: mgr.GetScheme(), Mapper: mgr.GetRESTMapper()})


Leader is running in a Namespace scope, it's supposed to watch those resources in the same Namespace where the leader controller is running. I didn't see you set the Namespace here.
Do we really need to use no cached client in order to remove the list in the RBAC? I feel it's better to keep the same as other controllers. @jianjuns any comment for this?

I do not know all the implication here, but I would watch only in the same Namespace.

Another question - in RBAC configuration, we do not create a "Role" to restrict the permissions to only the “multcluster" Namespace? @luolanzone

@jianjuns we defined a 'Role' antrea-mc-controller-role in antrea-multicluster-leader-namespaced.yml, Antrea mc-controller will only be able to watch resources under “antrea-multicluster" Namespace.

multicluster/cmd/multicluster-controller/memberclusterannounce_webhook.go

multicluster/controllers/multicluster/member_clusterset_controller.go

jianjuns · 2022-07-26T04:45:19Z

multicluster/cmd/multicluster-controller/leader.go

@@ -63,10 +64,15 @@ func runLeader(o *Options) error {
 	if err = memberClusterStatusManager.SetupWithManager(mgr); err != nil {
 		return fmt.Errorf("error creating MemberClusterAnnounce controller: %v", err)
 	}
+
+	noCachedClient, err := client.New(mgr.GetConfig(), client.Options{Scheme: mgr.GetScheme(), Mapper: mgr.GetRESTMapper()})


I do not know all the implication here, but I would watch only in the same Namespace.

Another question - in RBAC configuration, we do not create a "Role" to restrict the permissions to only the “multcluster" Namespace? @luolanzone

luolanzone

LGTM overall, a few nits

luolanzone · 2022-07-26T08:43:40Z

multicluster/cmd/multicluster-controller/leader.go

@@ -63,10 +64,15 @@ func runLeader(o *Options) error {
 	if err = memberClusterStatusManager.SetupWithManager(mgr); err != nil {
 		return fmt.Errorf("error creating MemberClusterAnnounce controller: %v", err)
 	}
+
+	noCachedClient, err := client.New(mgr.GetConfig(), client.Options{Scheme: mgr.GetScheme(), Mapper: mgr.GetRESTMapper()})


@jianjuns we defined a 'Role' antrea-mc-controller-role in antrea-multicluster-leader-namespaced.yml, Antrea mc-controller will only be able to watch resources under “antrea-multicluster" Namespace.

multicluster/cmd/multicluster-controller/memberclusterannounce_webhook.go

multicluster/build/yamls/antrea-multicluster-leader-namespaced.yml

Signed-off-by: hujiajing <hjiajing@vmware.com>

luolanzone

LGTM overall @jianjuns Could you help to take a look if this is ready to merge? thanks.

jianjuns · 2022-07-28T18:05:13Z

/test-multicluster-e2e
/skip-all

jianjuns · 2022-07-28T18:05:38Z

LGTM overall @jianjuns Could you help to take a look if this is ready to merge? thanks.

No further comment from me.

hjiajing force-pushed the auto-update branch from c009033 to 452e741 Compare June 30, 2022 09:00

luolanzone reviewed Jun 30, 2022

View reviewed changes

hjiajing force-pushed the auto-update branch 4 times, most recently from e55252a to 0ea8255 Compare July 4, 2022 02:54

hjiajing force-pushed the auto-update branch 2 times, most recently from 1b35a35 to 26b7eae Compare July 4, 2022 04:04

hjiajing changed the title ~~[WIP] Auto update ClusterSet and MemberClusterAnnounce in leader cluster~~ Auto update ClusterSet and MemberClusterAnnounce in leader cluster Jul 4, 2022

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

luolanzone reviewed Jul 4, 2022

View reviewed changes

luolanzone requested a review from jianjuns July 4, 2022 07:02

hjiajing force-pushed the auto-update branch from 26b7eae to 75088f9 Compare July 4, 2022 07:30

hjiajing force-pushed the auto-update branch 3 times, most recently from 39c2871 to 2ddbdd6 Compare July 12, 2022 07:02

luolanzone reviewed Jul 12, 2022

View reviewed changes

jianjuns reviewed Jul 12, 2022

View reviewed changes

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

hjiajing force-pushed the auto-update branch from 2ddbdd6 to 5940770 Compare July 13, 2022 09:59

jianjuns reviewed Jul 14, 2022

View reviewed changes

hjiajing force-pushed the auto-update branch 3 times, most recently from a0c5bec to e49b844 Compare July 14, 2022 08:22

jianjuns reviewed Jul 15, 2022

View reviewed changes

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

jianjuns reviewed Jul 21, 2022

View reviewed changes

multicluster/cmd/multicluster-controller/memberclusterannounce_webhook.go Outdated Show resolved Hide resolved

hjiajing force-pushed the auto-update branch 3 times, most recently from bbaa605 to bb7b755 Compare July 22, 2022 06:29

jianjuns reviewed Jul 22, 2022

View reviewed changes

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

hjiajing force-pushed the auto-update branch 4 times, most recently from 309af10 to 8fcbbb6 Compare July 24, 2022 11:57

jianjuns reviewed Jul 25, 2022

View reviewed changes

hjiajing force-pushed the auto-update branch from 8fcbbb6 to 5f2d5e9 Compare July 25, 2022 06:47

jianjuns reviewed Jul 25, 2022

View reviewed changes

luolanzone reviewed Jul 26, 2022

View reviewed changes

hjiajing force-pushed the auto-update branch 3 times, most recently from 2962ce8 to 1e02aa1 Compare July 26, 2022 03:31

jianjuns reviewed Jul 26, 2022

View reviewed changes

hjiajing force-pushed the auto-update branch from 1e02aa1 to 1879d48 Compare July 26, 2022 05:42

luolanzone reviewed Jul 26, 2022

View reviewed changes

hjiajing force-pushed the auto-update branch from 1879d48 to 8656e3e Compare July 26, 2022 14:15

luolanzone reviewed Jul 27, 2022

View reviewed changes

multicluster/cmd/multicluster-controller/memberclusterannounce_webhook.go Show resolved Hide resolved

hjiajing mentioned this pull request Jul 27, 2022

Multi-cluster antctl command refactor #3921

Closed

luolanzone reviewed Jul 27, 2022

View reviewed changes

multicluster/build/yamls/antrea-multicluster-leader-namespaced.yml Show resolved Hide resolved

Auto update ClusterSet in leader cluster

e9b2670

Signed-off-by: hujiajing <hjiajing@vmware.com>

hjiajing force-pushed the auto-update branch from 8656e3e to e9b2670 Compare July 27, 2022 15:11

luolanzone approved these changes Jul 28, 2022

View reviewed changes

jianjuns merged commit 2bf6a9a into antrea-io:main Jul 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto update ClusterSet and MemberClusterAnnounce in leader cluster #3956

Auto update ClusterSet and MemberClusterAnnounce in leader cluster #3956

hjiajing commented Jun 30, 2022

hjiajing commented Jun 30, 2022

codecov-commenter commented Jun 30, 2022 •

edited by codecov bot

Loading

luolanzone commented Jun 30, 2022

hjiajing commented Jun 30, 2022

luolanzone commented Jul 4, 2022

luolanzone Jul 12, 2022

jianjuns Jul 21, 2022

jianjuns Jul 12, 2022

luolanzone Jul 13, 2022

jianjuns Jul 14, 2022

jianjuns Jul 22, 2022 •

edited

Loading

jianjuns Jul 22, 2022

jianjuns Jul 25, 2022

luolanzone Jul 26, 2022

jianjuns left a comment

jianjuns Jul 25, 2022

luolanzone left a comment

luolanzone Jul 26, 2022 •

edited

Loading

jianjuns Jul 26, 2022 •

edited

Loading

luolanzone Jul 26, 2022

jianjuns Jul 26, 2022 •

edited

Loading

luolanzone left a comment

luolanzone Jul 26, 2022

luolanzone left a comment

jianjuns commented Jul 28, 2022

jianjuns commented Jul 28, 2022

		}
		// If err != nil, probably ClusterClaims were deleted during the processing of MemberClusterAnnounce.

Auto update ClusterSet and MemberClusterAnnounce in leader cluster #3956

Auto update ClusterSet and MemberClusterAnnounce in leader cluster #3956

Conversation

hjiajing commented Jun 30, 2022

hjiajing commented Jun 30, 2022

codecov-commenter commented Jun 30, 2022 • edited by codecov bot Loading

Codecov Report

luolanzone commented Jun 30, 2022

hjiajing commented Jun 30, 2022

luolanzone commented Jul 4, 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

jianjuns Jul 22, 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

jianjuns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luolanzone left a comment

Choose a reason for hiding this comment

luolanzone Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

jianjuns Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianjuns Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

luolanzone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luolanzone left a comment

Choose a reason for hiding this comment

jianjuns commented Jul 28, 2022

jianjuns commented Jul 28, 2022

codecov-commenter commented Jun 30, 2022 •

edited by codecov bot

Loading

jianjuns Jul 22, 2022 •

edited

Loading

luolanzone Jul 26, 2022 •

edited

Loading

jianjuns Jul 26, 2022 •

edited

Loading

jianjuns Jul 26, 2022 •

edited

Loading