Add ClusterClaim deletion validator #4062

luolanzone · 2022-07-29T02:16:37Z

Refine ClusterClaim webhook and add deletion validator to deny deletion
if there is any ClusterSet referring to the ClusterClaim.
Move status data initialization from leader ClusterSet controller to
MemberClusterAnnounce controller and simplify the status:

When the MemberClusterAnnounce is created, the Ready status will be
True and Reason will be Connected.
When the MemberClusterAnnounce is not updated in the agreed period,
the Ready status will be false and Reason will be Disconnected.
When the MemberClusterAnnounce is updated after Disconnected, the Ready
status will be reset to true and Reason will be Connected, and
lastUpdateTime will also be reset.
When the MemberCLusterAnnounce is deleted, the corresponding status
will be removed.

Signed-off-by: Lan Luo luola@vmware.com

luolanzone · 2022-07-29T02:16:54Z

/test-multicluster-e2e

codecov · 2022-07-29T02:31:33Z

Codecov Report

Merging #4062 (25c10a9) into main (024254d) will increase coverage by 0.49%.
The diff coverage is 84.39%.

@@            Coverage Diff             @@
##             main    #4062      +/-   ##
==========================================
+ Coverage   67.51%   68.00%   +0.49%     
==========================================
  Files         297      310      +13     
  Lines       44988    45477     +489     
==========================================
+ Hits        30373    30928     +555     
+ Misses      12238    12125     -113     
- Partials     2377     2424      +47

Flag	Coverage Δ
e2e-tests	`58.46% <59.57%> (?)`
integration-tests	`35.47% <23.25%> (+0.10%)`	⬆️
kind-e2e-tests	`50.03% <ø> (-0.74%)`	⬇️
unit-tests	`44.28% <71.63%> (-0.03%)`	⬇️

Impacted Files	Coverage Δ
...luster-controller/memberclusterannounce_webhook.go	`63.33% <ø> (+8.33%)`	⬆️
...llers/multicluster/member_clusterset_controller.go	`75.87% <ø> (+2.07%)`	⬆️
...md/multicluster-controller/clusterclaim_webhook.go	`74.50% <74.50%> (ø)`
...s/multicluster/memberclusterannounce_controller.go	`74.73% <86.53%> (+10.57%)`	⬆️
...llers/multicluster/leader_clusterset_controller.go	`77.58% <87.50%> (+12.14%)`	⬆️
...lers/multicluster/commonarea/remote_common_area.go	`76.01% <96.15%> (+6.22%)`	⬆️
...icluster/cmd/multicluster-controller/controller.go	`66.66% <100.00%> (+58.80%)`	⬆️
.../flowexporter/connections/conntrack_connections.go	`66.66% <0.00%> (-12.86%)`	⬇️
...kg/controller/networkpolicy/antreanetworkpolicy.go	`74.75% <0.00%> (-12.43%)`	⬇️
pkg/agent/interfacestore/types.go	`61.11% <0.00%> (-8.34%)`	⬇️
... and 62 more

luolanzone · 2022-07-29T07:06:42Z

multicluster/controllers/multicluster/commonarea/remote_common_area.go

 	defer r.mutex.Unlock()
 	r.mutex.Lock()

-	if leaderID == common.InvalidClusterID {


The InvalidClusterID will be set only when the ClusterSet is not created and common area is nil or the ClusterSet is deleted and common area is reset to nil. So there is no case to call updateLeaderStatus when the leader ID is InvalidClusterID.

luolanzone · 2022-07-29T08:05:19Z

/test-multicluster-e2e

multicluster/cmd/multicluster-controller/clusterclaim_webhook.go

jianjuns · 2022-08-02T18:09:59Z

multicluster/controllers/multicluster/memberclusterannounce_controller.go

-					data.leaderStatus.reason = ReasonConnectedLeader
-				}
+			if !apierrors.IsNotFound(err) {
+				return ctrl.Result{}, err


We need to unlock. I suggest to set err to nil if apierrors.IsNotFound(err), and break and check err and return after line 135.

Fixed, defined a variable err.

multicluster/controllers/multicluster/memberclusterannounce_controller.go

jianjuns · 2022-08-02T18:12:50Z

multicluster/controllers/multicluster/memberclusterannounce_controller.go

-			// TODO: Add ClusterClaim webhook to make sure it cannot be deleted while ClusterSet is present.
+			// If it's not found error, ClusterClaims were probably deleted during the processing of MemberClusterAnnounce.
+			// Nothing to handle in this case since it means no ClusterSet is referring to it or the ClusterSet is deleted
+			// as well, no status need to be updated.


"and no need to update cluster status"

I actually suggest to remove this sentence.

Removed the whole sentence.

jianjuns · 2022-08-02T18:14:59Z

multicluster/controllers/multicluster/memberclusterannounce_controller.go

+		// Check whether this local cluster is the leader for this member.
+		clusterClaim := &multiclusterv1alpha2.ClusterClaim{}
+		if err := r.Get(context.TODO(), types.NamespacedName{Name: multiclusterv1alpha2.WellKnownClusterClaimID, Namespace: req.Namespace}, clusterClaim); err == nil {
+			if clusterClaim.Value == memberAnnounce.LeaderClusterID {


When possible the leader clusterID does not match? Again, I suggest to remove laderClusterID from memberAnnounce.

It's possible that user creates a member ClusterSet with right access but incorrect leader ClusterID, if we didn't validate the leader ID and allow it to create the member cluster announce, then member cluster will show a status with wrong cluster ID but connected status.

I actually feel leader cluster ID in ClusterSet is not very useful. It is not used anywhere, except these validations. But anyway, we can keep the current way, if you prefer.

jianjuns · 2022-08-02T18:19:42Z

multicluster/controllers/multicluster/memberclusterannounce_controller.go

-			// If err != nil, probably ClusterClaims were deleted during the processing of MemberClusterAnnounce.
-			// Nothing to handle in this case and MemberClusterAnnounce will also be deleted soon.
-			// TODO: Add ClusterClaim webhook to make sure it cannot be deleted while ClusterSet is present.
+			// If it's not found error, ClusterClaims were probably deleted during the processing of MemberClusterAnnounce.


I feel we should revisit the design of Add/RemoveMember() after ClusterSet changes. Why that indirect, but not start/stop memberAnnounce periodic check right after the memberAnnounce is created/deleted in MemberClusterAnnounceReconciler?

Pasted below my earlier comments too.

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?

Yeah, from member cluster's perspective, I feel we don't have to provide leader's ClusterID as long as the access token is correct. We probably need to refactor the whole ClusterSet part including the ClusterSet spec vs ClusterSet status for joined member.

We can decide leader ClusterID later. But please consider 3). The current code is unnecessarily complex.

Sure, I will double check this part.

Hi @jianjuns I have refined this part, the MemberClusterAnnounce is responsible to initialize the data now. Could you help to take another look? thanks.

multicluster/controllers/multicluster/memberclusterannounce_controller.go

jianjuns · 2022-08-06T00:49:34Z

multicluster/controllers/multicluster/memberclusterannounce_controller.go

@@ -114,38 +108,34 @@ func (r *MemberClusterAnnounceReconciler) Reconcile(ctx context.Context, req ctr
 		klog.V(2).InfoS("Reset lastUpdateTime", "cluster", memberAnnounce.ClusterID)


We should remove or rephrase the comment: "// If the member is not found in timerData, it means the member is not added to the ClusterSet or was deleted."?

Done, changed it to If the member is not found in timerData, it means MemberClusterAnnounce is not created or it was deleted.

multicluster/controllers/multicluster/memberclusterannounce_controller.go

jianjuns · 2022-08-06T01:08:35Z

multicluster/controllers/multicluster/memberclusterannounce_controller.go

@@ -114,38 +108,34 @@ func (r *MemberClusterAnnounceReconciler) Reconcile(ctx context.Context, req ctr
 		klog.V(2).InfoS("Reset lastUpdateTime", "cluster", memberAnnounce.ClusterID)


After reading the code more, I wonder should we do the following two:

Merge timerData and memberStatus to one map (and call it memberStatusMap)

Move this block to addMember() (and rename the func to sth like addUpdateMemberStatus())

What you think?

Yes, I am thinking it should work, but If we'd like to include this PR to 1.8, I feel it's better to has a follow up PR to rethink about the whole status part, what's your suggestion?

I am fine to do another PR, but I do feel we should clean up the code. The current implementation is unnecessarily complex, hard to follow and may include bugs due to that.

Sure, I feel we need rethink the design including ClusterClaim/ClusterSet and status.

jianjuns · 2022-08-08T03:58:19Z

multicluster/controllers/multicluster/memberclusterannounce_controller.go

-		}
-		// If MemberClusterAnnounce is deleted, no need to process because RemoveMember must already
-		// be called.
+		// If MemberClusterAnnounce is deleted, no need to process because the Finalizer


If MemberClusterAnnounce is deleted, no further processing is needed, as cleanup must have been done when the Finalizer was removed.

multicluster/controllers/multicluster/memberclusterannounce_controller.go

jianjuns · 2022-08-08T04:02:44Z

multicluster/controllers/multicluster/memberclusterannounce_controller.go

@@ -114,38 +108,34 @@ func (r *MemberClusterAnnounceReconciler) Reconcile(ctx context.Context, req ctr
 		klog.V(2).InfoS("Reset lastUpdateTime", "cluster", memberAnnounce.ClusterID)


I am fine to do another PR, but I do feel we should clean up the code. The current implementation is unnecessarily complex, hard to follow and may include bugs due to that.

multicluster/cmd/multicluster-controller/clusterclaim_webhook.go

multicluster/controllers/multicluster/commonarea/remote_common_area.go

multicluster/controllers/multicluster/memberclusterannounce_controller.go

jianjuns · 2022-08-08T17:37:29Z

multicluster/controllers/multicluster/memberclusterannounce_controller.go

 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}

 	r.mapLock.Lock()
-	// If the member is not found in timerData, it means the member is not added to the ClusterSet or was deleted.
-	if data, ok := r.timerData[common.ClusterID(memberAnnounce.ClusterID)]; ok {
+	// If the member is not found in memberStatus, it means MemberClusterAnnounce is not created


Again, I think this block (from line 94 to 119) should be moved to addMemberStatus (probably rename it to addOrUpdateMemberStatus). Could you check?

jianjuns · 2022-08-08T17:40:25Z

multicluster/controllers/multicluster/memberclusterannounce_controller.go

@@ -163,6 +142,7 @@ func (r *MemberClusterAnnounceReconciler) Reconcile(ctx context.Context, req ctr
 		klog.ErrorS(err, "Failed to add member cluster to ClusterSet", "cluster", memberAnnounce.ClusterID)
 		return ctrl.Result{}, err
 	}
+	r.addMemberStatus(memberID, v1.ConditionTrue)


Should we move it to the end of the func after adding the Finalizer?

I think it's better to place it before adding Finalizer since member is actually connected and able to do export/import etc.

jianjuns · 2022-08-08T17:45:43Z

multicluster/controllers/multicluster/memberclusterannounce_controller.go

@@ -163,6 +142,7 @@ func (r *MemberClusterAnnounceReconciler) Reconcile(ctx context.Context, req ctr
 		klog.ErrorS(err, "Failed to add member cluster to ClusterSet", "cluster", memberAnnounce.ClusterID)


We should ignore "already exists" error in addMemberToClusterSet (in my mind better to add comments on addMemberToClusterSet to explain the behavior)?

Yes, return nil for already exists error.

Oh, I did not see that. Then we should move r.addOrUpdateMemberStatus() to line 113 before "if common.StringExistsInSlice(memberAnnounce.Finalizers, finalizer)"?

I assume we can call the func only once, and remove the call at line 95.

jianjuns · 2022-08-08T17:47:57Z

multicluster/controllers/multicluster/memberclusterannounce_controller.go

@@ -163,6 +142,7 @@ func (r *MemberClusterAnnounceReconciler) Reconcile(ctx context.Context, req ctr
 		klog.ErrorS(err, "Failed to add member cluster to ClusterSet", "cluster", memberAnnounce.ClusterID)
 		return ctrl.Result{}, err
 	}
+	r.addMemberStatus(memberID, v1.ConditionTrue)
 	klog.InfoS("Adding finalizer to MemberClusterAnnounce", "MemberClusterAnnounce", klog.KObj(memberAnnounce))


We will repeatedly log this message for every memberAnnounce update. Should should check if Finalizer is already added?

When the finalizer exists, it will return, code won't reach here.

if common.StringExistsInSlice(memberAnnounce.Finalizers, finalizer) { return ctrl.Result{}, nil }

luolanzone · 2022-08-09T02:40:11Z

/test-multicluster-e2e

jianjuns · 2022-08-09T04:35:40Z

multicluster/controllers/multicluster/memberclusterannounce_controller.go

@@ -163,6 +142,7 @@ func (r *MemberClusterAnnounceReconciler) Reconcile(ctx context.Context, req ctr
 		klog.ErrorS(err, "Failed to add member cluster to ClusterSet", "cluster", memberAnnounce.ClusterID)


Oh, I did not see that. Then we should move r.addOrUpdateMemberStatus() to line 113 before "if common.StringExistsInSlice(memberAnnounce.Finalizers, finalizer)"?

I assume we can call the func only once, and remove the call at line 95.

jianjuns · 2022-08-09T04:37:06Z

multicluster/controllers/multicluster/memberclusterannounce_controller.go

+	// Initialize the member status with unknown status when controller restarts.
+	// Let's background process to check and update the connection status.
+	// v1.ConditionUnknown will be ignored if status already exists in the map.
+	r.addOrUpdateMemberStatus(memberID, v1.ConditionUnknown)


Can we remove this call, and keep only a single call of addOrUpdateMemberStatus(), e.g. at line 112?

jianjuns

Questions and a few nits.

jianjuns · 2022-08-09T18:24:31Z

multicluster/controllers/multicluster/commonarea/remote_common_area.go

-			}
-		}
+
+	if apierrors.IsNotFound(err) {


We can just do:

localClusterMemberAnnounceExists := err == nil

jianjuns · 2022-08-09T18:24:48Z

multicluster/controllers/multicluster/commonarea/remote_common_area.go

 		return err
 	}
+	var localClusterMemberAnnounceExist bool


localClusterMemberAnnounceExist -> localClusterMemberAnnounceExists

multicluster/controllers/multicluster/commonarea/remote_common_area.go

multicluster/controllers/multicluster/memberclusterannounce_controller.go

jianjuns · 2022-08-09T18:38:07Z

multicluster/controllers/multicluster/memberclusterannounce_controller.go

-		LastTransitionTime: metav1.Now(),
-		Message:            "Member created",
-		Reason:             ReasonNeverConnected,
+		Message:            "Member Created",


Question - what is difference between "Member Created" and "Member Connected"? Should we just set the condition to "ConditionTrue + Member Connected"? Or there is a protocol that member should notify leader after it is ready by updating the Announce?

"Member Created" will be used only when controller starts. It will become Member Connected" when member cluster send a MemberClusterAnnounce update. The purpose is handle the case MemberClusterAnnounce aleady exists on leader cluster, but it might be stale and will be cleaned up soon.

But I meant what is the problem if we set the condition to "ConditionTrue + Member Connected" directly here? If the member cluster does not update the Announce, it will be changed to "Disconnected" anyway?

Yes, it will be changed to "Disconnected", I feel both way works. Do you recommend to use Member Connected only?

Does member update the Announce right away after the creation? If yes, I am fine with the current way too; if not, I feel "ConditionTrue + Member Connected" works better, while "ConditionUnknown" is misleading.

It will be updated every 5 seconds if member cluster works well. But you are right, I feel stale MemberClusterAnnounce case will be rare, let me change it to "ConditionTrue + Member Connected".

BTW, do you think 5 seconds may be too frequent?

Yeah, I feel it can be longer, how about 15s?

Was thinking about 10 seconds. 15 seconds sound good to me too.

OK, I updated it to 10s in this PR, it can be quicker to detect failure.

luolanzone · 2022-08-10T02:21:59Z

/test-multicluster-e2e

jianjuns · 2022-08-10T20:25:39Z

multicluster/controllers/multicluster/commonarea/remote_common_area.go


+	localClusterMemberAnnounceExists := !apierrors.IsNotFound(err)


Is it safe to pass nil to apierrors.IsNotFound()? I would suggest:

localClusterMemberAnnounceExists := err == nil

1. Refine ClusterClaim webhook, add deletion validator to deny deletion if there is any ClusterSet referring to it. 2. Move status data initialization from leader ClusterSet controller to MemberClusterAnnounce controller and simplify the status: * When the MemberClusterAnnounce is created, the `Ready` status will be `True` and `Reason` will be `Connected`. * When the MemberClusterAnnounce is not updated in the agreed period, the `Ready` status will be `false` and `Reason` will be `Disconnected`. * When the MemberClusterAnnounce is updated after Disconnected, the `Ready` status will be reset to `true` and `Reason` will be `Connected`, lastUpdateTime will also be reset. * When the MemberCLusterAnnounce is deleted, the corresponding status will be removed. Signed-off-by: Lan Luo <luola@vmware.com>

jianjuns · 2022-08-11T03:03:26Z

/test-multicluster-e2e
/test-integration

jianjuns · 2022-08-11T03:53:35Z

/skip-all

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

luolanzone force-pushed the deletion-webhook-and-cleanup branch from 1584db0 to a3a9658 Compare July 29, 2022 02:35

luolanzone commented Jul 29, 2022

View reviewed changes

luolanzone requested review from hjiajing and jianjuns August 2, 2022 05:52

jianjuns reviewed Aug 2, 2022

View reviewed changes

luolanzone force-pushed the deletion-webhook-and-cleanup branch 4 times, most recently from c0241f1 to 5729c74 Compare August 5, 2022 02:55

jianjuns reviewed Aug 6, 2022

View reviewed changes

luolanzone force-pushed the deletion-webhook-and-cleanup branch from 5729c74 to 04a2198 Compare August 8, 2022 03:30

jianjuns reviewed Aug 8, 2022

View reviewed changes

luolanzone mentioned this pull request Aug 8, 2022

Update Multi-cluster docs and templates for leader cluster auto update #4068

Merged

luolanzone force-pushed the deletion-webhook-and-cleanup branch 4 times, most recently from 9f0db70 to 566c5fa Compare August 8, 2022 10:33

luolanzone mentioned this pull request Aug 8, 2022

Minor fixes for multi-cluster manifests #4087

Merged

jianjuns previously approved these changes Aug 8, 2022

View reviewed changes

luolanzone dismissed jianjuns’s stale review via aa20a88 August 9, 2022 02:34

luolanzone force-pushed the deletion-webhook-and-cleanup branch from 566c5fa to aa20a88 Compare August 9, 2022 02:34

jianjuns reviewed Aug 9, 2022

View reviewed changes

luolanzone mentioned this pull request Aug 9, 2022

Add antctl commands to set up and delete Multi-cluster ClusterSet #3992

Merged

luolanzone force-pushed the deletion-webhook-and-cleanup branch from aa20a88 to 37a2105 Compare August 9, 2022 08:04

jianjuns reviewed Aug 9, 2022

View reviewed changes

luolanzone force-pushed the deletion-webhook-and-cleanup branch from 37a2105 to da508d5 Compare August 10, 2022 02:21

jianjuns previously approved these changes Aug 10, 2022

View reviewed changes

luolanzone dismissed jianjuns’s stale review via 22a172b August 10, 2022 05:31

luolanzone force-pushed the deletion-webhook-and-cleanup branch from da508d5 to 22a172b Compare August 10, 2022 05:31

jianjuns reviewed Aug 10, 2022

View reviewed changes

luolanzone force-pushed the deletion-webhook-and-cleanup branch from 22a172b to 25c10a9 Compare August 11, 2022 01:20

jianjuns approved these changes Aug 11, 2022

View reviewed changes

jianjuns merged commit cb70ad1 into antrea-io:main Aug 11, 2022

		@@ -114,38 +108,34 @@ func (r *MemberClusterAnnounceReconciler) Reconcile(ctx context.Context, req ctr
		klog.V(2).InfoS("Reset lastUpdateTime", "cluster", memberAnnounce.ClusterID)

		@@ -163,6 +142,7 @@ func (r *MemberClusterAnnounceReconciler) Reconcile(ctx context.Context, req ctr
		klog.ErrorS(err, "Failed to add member cluster to ClusterSet", "cluster", memberAnnounce.ClusterID)


		localClusterMemberAnnounceExists := !apierrors.IsNotFound(err)

Add ClusterClaim deletion validator #4062

Add ClusterClaim deletion validator #4062

Conversation

luolanzone commented Jul 29, 2022 • edited by jianjuns Loading

luolanzone commented Jul 29, 2022

codecov bot commented Jul 29, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

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

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

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 Aug 9, 2022

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

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 Aug 10, 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

luolanzone commented Aug 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianjuns commented Aug 11, 2022

jianjuns commented Aug 11, 2022

luolanzone commented Jul 29, 2022 •

edited by jianjuns

Loading

codecov bot commented Jul 29, 2022 •

edited

Loading

jianjuns Aug 10, 2022 •

edited

Loading