Update finalizer of ResourceExport to be domain-qualified #6742

Dyanngg · 2024-10-14T22:07:27Z

multicluster/apis/multicluster/constants/constants.go

multicluster/apis/multicluster/v1alpha1/resourceexport_webhook.go

antoninbas · 2024-10-14T22:18:53Z

multicluster/apis/multicluster/v1alpha1/resourceexport_webhook.go

+	// Note for ResourceExports created before Antrea v2.2, LegacyResourceExportFinalizer may still be present
+	// and needs to be removed before a ResourceExport is deleted.
+	if r.DeletionTimestamp.IsZero() && !stringExistsInSlice(r.Finalizers, constants.DomainQualifiedResourceExportFinalizer) {
+		r.Finalizers = append(r.Finalizers, constants.DomainQualifiedResourceExportFinalizer)


Obviously this predates this PR, but I am surprised to see a webhook being in charge of adding the finalizer. Is there a recommendation to do that somewhere? I usually see it done in the controller itself (Reconcile). cc @luolanzone

Hi @antoninbas I think the Finalizer is not necessarily handled by the reconciler. If I recall it correctly, the main purpose is to stop deleting the ResourceExport during creation, because if it's already converted by the leader controller into ResourceImport, there will be no way to clean up the corresponding info in the ResourceImport when the ResourceExport is deleted by the user.

antoninbas · 2024-10-14T22:35:07Z

multicluster/controllers/multicluster/leader/resourceexport_controller.go

@@ -101,7 +101,7 @@ func (r *ResourceExportReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 	// clean up any replicated resources like ResourceImport.
 	// For more details about using Finalizers, please refer to https://book.kubebuilder.io/reference/using-finalizers.html.
 	if !resExport.DeletionTimestamp.IsZero() {
-		if common.StringExistsInSlice(resExport.Finalizers, constants.ResourceExportFinalizer) {
+		if common.AnyExistsInSlice(resExport.Finalizers, []string{constants.LegacyResourceExportFinalizer, constants.DomainQualifiedResourceExportFinalizer}) {


I am not sure, but would it make sense to update the finalizer as part of the Reconcile (we could do it earlier in the function), so that:

the legacy finalizer can be updated to the new one whenever an object is reconciled, reducing the possibility of the legacy finalizer "sticking around"

the rest of the code only needs to worry about the new finalizer

cc @luolanzone

@antoninbas do you mean to update the Finalizer to the new one in the reconcile? I think we can't guarantee the Finalizer can be updated for all ResourceExports before deletion, we still need to handle the case that both legacy and new one are in the ClusterSet. And this may lead a conflict that one reconciler is updating the Finalizer but another one is deleting it. So I feel we should have only one place to add the Finalizer.

finalizers are usually handled by the controller that owns the resource (that includes adding the finalizer but also processing the finalizer and removing the finalizer), so I am not sure I understand both of your points:

I think we can't guarantee the Finalizer can be updated for all ResourceExports before deletion

And this may lead a conflict that one reconciler is updating the Finalizer but another one is deleting it

Do we have more than one controller / reconciler responsible for the finalizer, or using the finalizer?

Do we have more than one controller / reconciler responsible for the finalizer, or using the finalizer?

I'll try to put my understanding of it here as best as I can, using Service as example:

End users can create a ServiceExport in member clusters. A controller in member cluster [a] will try to create a ResourceExport for this object in the leader cluster. This is currently when a finalizer is put on the resource.

End users can also delete that ServiceExport in member cluster. That controller [a] is also responsible for deleting the ResourceExport in the leader cluster.

A controller [b] in the leader cluster watches for ResourceExports in its own cluster, and reconciles them (by creating corresponding ResourceImports). When it sees an ResourceExport being deleted, it performs necessary cleanups regarding ResourceImports, and then removes the finalizers on ResourceExport and allows it to be deleted.

So if we treat controller [a] solely as a client, not a controller/reconciler, then it's pretty clear that controller [b] actually "owns" ResourceExports. I think @antoninbas's suggestion would actually make sense here:

If we have controller [b] put up finalizers for ResourceExports at the beginning of Reconciles (and making sure the finalizer is set before dealing with ResourceImport logics), then allowing client [a] to create-delete ResourceExports in short succession is actually fine: the leader cluster has not even processed the event yet, so there will not be dangling ResourceImports that contains information of that deleted ResourceExport.

Even if there are ResourceImports in the leader cluster for the same named Service, it will only contain endpoints from ResourceExport created by another member cluster, which would actually have finalizers on it.

In other words, we change the logic to, we add a finalizer to ResourceExports only when we know there are going to be side-effects in the leader cluster, not when the resource is created at apiserver

However @antoninbas do you think this change warrants a separate PR?

finalizers are usually handled by the controller that owns the resource (that includes adding the finalizer but also processing the finalizer and removing the finalizer), so I am not sure I understand both of your points:

I think we can't guarantee the Finalizer can be updated for all ResourceExports before deletion

And this may lead a conflict that one reconciler is updating the Finalizer but another one is deleting it

Do we have more than one controller / reconciler responsible for the finalizer, or using the finalizer?

At the moment, we have the webhook in the leader and the *Exporter controllers in the member cluster to add the Finalizer during ResourceExport creation, the reconciler itself in the leader will remove the Finalizer. The Finalizer in the webhook is a special one for AntreaClusterNetworkPolicyKind since it will be created by the end user if I recall it correctly. The original purpose is to stop the side-affect when the user or "the controller [a]" in the scenario mentioned by @Dyanngg delete the ResourceExport and leads to some dangling ResourceImports.
But you are right, it should be more clear if we can move all Finalizer operations to the leader controller, @Dyanngg could you help to double check and move forward? I think we may need a seperate PR for the change. Thanks.

Yes, we can handle this in a future PR

luolanzone · 2024-10-16T06:44:45Z

/test-multicluster-e2e

XinShuYang · 2024-10-16T08:45:00Z

/test-multicluster-e2e

luolanzone · 2024-10-17T03:02:31Z

/test-multicluster-e2e

luolanzone · 2024-10-17T03:32:39Z

/test-multicluster-e2e

antoninbas

LGTM

multicluster/controllers/multicluster/leader/clusterinfo_export_handler.go

multicluster/controllers/multicluster/leader/resourceexport_controller.go

multicluster/controllers/multicluster/leader/resourceexport_controller_test.go

Dyanngg · 2024-10-17T18:04:56Z

/test-all /test-multicluster-e2e

antoninbas

LGTM

Dyanngg · 2024-10-17T18:45:19Z

/test-all /test-multicluster-e2e

Dyanngg · 2024-10-17T22:09:08Z

/test-multicluster-e2e

multicluster/controllers/multicluster/leader/resourceexport_controller.go

luolanzone

LGTM

Fixes antrea-io#6741 Signed-off-by: Dyanngg <[email protected]>

Dyanngg · 2024-10-21T17:49:32Z

/test-multicluster-e2e All tests were passing before addressing the last comment

antoninbas · 2024-10-22T05:27:47Z

We should probably mention this in the release notes, so I'm adding the label for this

Dyanngg requested review from antoninbas, tnqn, jianjuns and luolanzone October 14, 2024 22:07

antoninbas reviewed Oct 14, 2024

View reviewed changes

luolanzone added the area/multi-cluster Issues or PRs related to multi cluster. label Oct 15, 2024

Dyanngg force-pushed the fix-mc-finalizers branch from 2afa3f2 to d862b03 Compare October 15, 2024 17:43

luolanzone mentioned this pull request Oct 17, 2024

Avoid error log when unmarshalling config for MC controller #6744

Merged

antoninbas reviewed Oct 17, 2024

View reviewed changes

Dyanngg force-pushed the fix-mc-finalizers branch from d862b03 to c4553ac Compare October 17, 2024 18:04

Dyanngg force-pushed the fix-mc-finalizers branch from c4553ac to 900204d Compare October 17, 2024 18:15

antoninbas previously approved these changes Oct 17, 2024

View reviewed changes

luolanzone reviewed Oct 18, 2024

View reviewed changes

multicluster/controllers/multicluster/leader/resourceexport_controller.go Show resolved Hide resolved

luolanzone reviewed Oct 18, 2024

View reviewed changes

Update finalizer of ResourceExport to be domain-qualified

2b93818

Fixes antrea-io#6741 Signed-off-by: Dyanngg <[email protected]>

Dyanngg dismissed antoninbas’s stale review via 2b93818 October 21, 2024 17:48

Dyanngg force-pushed the fix-mc-finalizers branch from 900204d to 2b93818 Compare October 21, 2024 17:48

Dyanngg requested review from luolanzone and antoninbas October 21, 2024 17:49

antoninbas approved these changes Oct 22, 2024

View reviewed changes

antoninbas added the action/release-note Indicates a PR that should be included in release notes. label Oct 22, 2024

antoninbas merged commit 5062aae into antrea-io:main Oct 22, 2024
52 of 59 checks passed

Dyanngg deleted the fix-mc-finalizers branch October 22, 2024 18:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update finalizer of ResourceExport to be domain-qualified #6742

Update finalizer of ResourceExport to be domain-qualified #6742

Dyanngg commented Oct 14, 2024

antoninbas Oct 14, 2024

luolanzone Oct 15, 2024

antoninbas Oct 14, 2024

luolanzone Oct 15, 2024

antoninbas Oct 15, 2024 •

edited

Loading

Dyanngg Oct 15, 2024

Dyanngg Oct 15, 2024

luolanzone Oct 16, 2024

antoninbas Oct 17, 2024

luolanzone commented Oct 16, 2024

XinShuYang commented Oct 16, 2024

luolanzone commented Oct 17, 2024

luolanzone commented Oct 17, 2024

antoninbas left a comment

Dyanngg commented Oct 17, 2024

antoninbas left a comment

Dyanngg commented Oct 17, 2024

Dyanngg commented Oct 17, 2024

luolanzone left a comment

Dyanngg commented Oct 21, 2024

antoninbas commented Oct 22, 2024

Update finalizer of ResourceExport to be domain-qualified #6742

Update finalizer of ResourceExport to be domain-qualified #6742

Conversation

Dyanngg commented Oct 14, 2024

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

antoninbas Oct 15, 2024 • 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

luolanzone commented Oct 16, 2024

XinShuYang commented Oct 16, 2024

luolanzone commented Oct 17, 2024

luolanzone commented Oct 17, 2024

antoninbas left a comment

Choose a reason for hiding this comment

Dyanngg commented Oct 17, 2024

antoninbas left a comment

Choose a reason for hiding this comment

Dyanngg commented Oct 17, 2024

Dyanngg commented Oct 17, 2024

luolanzone left a comment

Choose a reason for hiding this comment

Dyanngg commented Oct 21, 2024

antoninbas commented Oct 22, 2024

antoninbas Oct 15, 2024 •

edited

Loading