Resource exporter/importer and unit test #3024

luolanzone · 2021-11-11T08:01:00Z

This PR is mainly for resource exporter/importer:

exporter
serviceexport_controller is responsible to reconcile ServiceExport resource in member cluster and write
wrapped ResourceExport into leader cluster, meanwhile it will watch Service and Endpoints change via
event mapping and update ResourceExport when exported Service/Endpoints are updated.

resourceexport_controller is responsible to reconcile ResourceExport resources and computes all ResourceExports
from different member clusters into one ResourceExport in leader cluster if the resources are exported
with the same namespaced name and the same kind, for this moment, we only support Service, Endpoints kind.

importer
remote_resourceimport_controller watches leader cluster's ResourceImport events and create corresponding
ServiceImport, Service and Endpoints with AntreaMCServiceAnnotation in member cluster. ServiceImport name will
be the same as exported Service, new created Service and Endpoints will have an antrea multicluster prefix.
stale controller
stale controller is mainly for special cases when resource is deleted during controller restarting
in leader or member cluster, it will be triggered once only when controller starts.

Notes: serviceexport_controller, stale_controller and
resourceimport_controller will run only in member cluster,
resourceexport_controller will run only in leader cluster.

add unit test codes.

This PR is based on #3111, please kindly review top commit.
Signed-off-by: Lan Luo [email protected]

jianjuns

A general question - why we need to label the exported service? Could not we just identify the Services based on ServiceExport?

jianjuns · 2021-11-12T06:56:06Z

multicluster/controllers/multicluster/internal/fake_remote_cluster.go

+	"sigs.k8s.io/controller-runtime/pkg/client"
+)
+
+type fakeRemoteCluster struct {


Please add comments to indicate this is for unit tests.

jianjuns · 2021-11-12T06:57:40Z

multicluster/controllers/multicluster/utils.go

+	"antrea.io/antrea/multicluster/controllers/multicluster/internal"
+)
+
+// we should only have one remote leader cluster for demo purpose


Nit: we -> We

only have -> have only

Why we have "demo" in the comments? If it is a temporary status, say "temporarily" or "at this moment", etc.

jianjuns · 2021-11-12T06:59:57Z

multicluster/controllers/multicluster/utils.go

+
+// we should only have one remote leader cluster for demo purpose
+// so check and return the first remote cluster.
+func getRemoteCluster(remoteMgr *internal.RemoteClusterManager) (internal.RemoteCluster, error) {


I feel not necessary to add a separate file for this func. Can we move it to service_controller.go?

sure, both service_controller.go and serviceexport_controller.go will call it, so I move it into utils.go. let me it move back.

jianjuns · 2021-11-17T00:58:16Z

multicluster/controllers/multicluster/common/helper.go

+import "k8s.io/apimachinery/pkg/types"
+
+const (
+	AntreaMcsLabel        = "antrea.io/multi-cluster"


If we follow our convention to use upper cases for abbrs, then "Mcs" should be "MCS".

Why we use label but not annotation?

the main reason is there is no annotation selector, so I use label.

Sorry, how we use label selectors?

we use labelselector to filter resource export in resourceexport_controller.go, eg https://github.com/antrea-io/antrea/pull/3024/files#diff-4a5af6131176d9b4bd232b0185161872343c0f501be3f061e07a555cc3b2dcb8R168
and
https://github.com/antrea-io/antrea/pull/3024/files#diff-4a5af6131176d9b4bd232b0185161872343c0f501be3f061e07a555cc3b2dcb8R318 etc. I feel in some cases we can use annotation, and some other cases we need label so we can use label selector. for service controller part, I will check if possible to use annotation instead.

I feel ideally we should not add labels in user created resources. It is redundant data, and more importantly we cannot prevent users from removing the labels. If example, they can just apply a yaml to update an existing Services and that will remove labels added by us.

indeed, that make sense, thanks to point out, I will check how to achieve the same feature in a different way.

hi @jianjuns I updated it with an annotation instead of a label, the main purpose of this info is to reduce the service/endpoint update events and watch those exported resource event only https://github.com/antrea-io/antrea/pull/3024/files#diff-1e1d6e2558b5bb86a952c580e877a8a072c499a0c278194afb4d0948ae459d10R407-R443, we can skip adding any annotation and watch all services/endpoints, but I feel it will be a big effort to handle unnecessary services/endpoints change in ServiceExport reconciler. let me know what's your thought. thanks.

But annotation is the same as label that can be removed by users (sorry if my previous comments are not clear). I feel watching all Services and Endpoints is ok, as it is just by one controller in the cluster. AntreaProxy watches all Services and Endpoints in all hosts.

jianjuns · 2021-11-17T00:59:19Z

multicluster/controllers/multicluster/common/helper.go

+	Separator = '/'
+)
+
+// TODO: Use NamespacedName stringer method instead of this. eg nsName.String()


. eg -> , e.g.

jianjuns · 2021-11-17T01:00:26Z

multicluster/controllers/multicluster/common/helper.go

+
+// GetNamespacedName returns the objects name as NamespacedName struct.
+func GetNamespacedName(namespace, name string) types.NamespacedName {
+	return types.NamespacedName{


Do we need a func for this?

yes, it help us to build up a new types.NamespacedName with given strings, not from K8s object directly, so we construct it by ourself, mainly purpose is to avoid duplicate codes, only remote_resourceimport_controller.go use it for now, let me move it from helper to remote_resourceimport_controller.go

But types.NamespacedName{namespace, name} is very simple and just one line.

yeah, we can use types.NamespacedName{namespace, name} but it will give us a warning about k8s.io/apimachinery/pkg/types.NamespacedName composite literal uses unkeyed fieldscomposites, so I feel it's a little bit long to have both key and name.

let me change it back to struct directly, so we have less function

jianjuns · 2021-11-17T01:00:53Z

multicluster/controllers/multicluster/common/helper.go

+	}
+}
+
+func GetPointer(str string) *string {


Why we need a func?

there are a few fields requires string pointer,https://github.com/antrea-io/antrea/pull/3024/files#diff-1e1d6e2558b5bb86a952c580e877a8a072c499a0c278194afb4d0948ae459d10R362-R371, I added this to avoid define a string and get its pointer repeatedly, but looks like only serviceexporter_controller.go use this function, I will move it and name it as getPointer

multicluster/controllers/multicluster/resourceexport_controller.go

codecov-commenter · 2021-11-17T04:40:20Z

Codecov Report

Merging #3024 (faafaf9) into feature/multi-cluster (d36c406) will increase coverage by 1.15%.
The diff coverage is 65.93%.

@@                    Coverage Diff                    @@
##           feature/multi-cluster    #3024      +/-   ##
=========================================================
+ Coverage                  39.76%   40.92%   +1.15%     
=========================================================
  Files                        189      188       -1     
  Lines                      21880    22568     +688     
=========================================================
+ Hits                        8701     9236     +535     
- Misses                     12324    12392      +68     
- Partials                     855      940      +85

Flag	Coverage Δ
unit-tests	`40.92% <65.93%> (+1.15%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...icluster/cmd/multicluster-controller/controller.go	`4.68% <ø> (ø)`
multicluster/cmd/multicluster-controller/member.go	`0.00% <0.00%> (ø)`
...ulticluster/cmd/multicluster-controller/options.go	`8.33% <0.00%> (ø)`
...llers/multicluster/member_clusterset_controller.go	`0.00% <0.00%> (ø)`
...s/multicluster/memberclusterannounce_controller.go	`66.96% <ø> (ø)`
...uster/controllers/multicluster/stale_controller.go	`47.19% <47.19%> (ø)`
...llers/multicluster/core/fake_remote_common_area.go	`61.53% <61.53%> (ø)`
...ntrollers/multicluster/serviceexport_controller.go	`67.61% <68.15%> (+67.61%)`	⬆️
...ers/multicluster/core/resourceimport_controller.go	`68.39% <68.39%> (ø)`
...trollers/multicluster/resourceexport_controller.go	`70.16% <70.61%> (+70.16%)`	⬆️
... and 11 more

luolanzone · 2021-11-17T07:45:15Z

multicluster/config/crd/bases/multicluster.x-k8s.io_serviceexports.yaml

+metadata:
+  name: serviceexports.multicluster.x-k8s.io
+spec:
+  group: multicluster.x-k8s.io


Hi @jianjuns , I am wondering if it's OK to copy serviceexports CRD definition from mcs api repo? or we should move it as own one API group? if we move it to our own group, I think we need to define ServiceExport and ServiceImport objects.

I feel ok to copy the yaml. Let us see if @tnqn has an opinion.

Is it possible to put these copied yamls in a separate directory?

Reminder for the question.

Is it possible to put these copied yamls in a separate directory?

yes, I was thinking to move it in a separate PR when we rename the core package.

jianjuns · 2021-11-17T19:33:59Z

multicluster/controllers/multicluster/common/helper.go

-	AntreaMcsLabel        = "antrea.io/multi-cluster"
-	AntreaMcsAutoGenLabel = "antrea.io/multi-cluster-autogenerated"
-	SourceImportLabel     = "antrea.io/resource-import-name"
+	AntreaMCSAutoGenAnnotation = "antrea.io/multi-cluster-autogenerated"


What you think about "multicluster.antrea.io/imported-service" and "multicluster.antrea.io/resource-import-name"?

ok, will do.

jianjuns · 2021-11-17T19:34:43Z

multicluster/controllers/multicluster/internal/remote_resourceimport_controller.go

@@ -209,7 +209,7 @@ func (r *ResourceImportReconciler) handleResImpUpdateForEndpoints(ctx context.Co
 		r.installedResImports.Add(*resImp)
 		return ctrl.Result{}, nil
 	}
-	if _, ok := ep.Labels[common.AntreaMcsAutoGenLabel]; !ok {
+	if _, ok := ep.Labels[common.AntreaMCSAutoGenAnnotation]; !ok {


Have you done the change to switch from label to annotation, and remove the "multi-cluster" label on exported Services?

yes, @jianjuns , this is a multi-cluster service created by our own codes, so I suppose it should be ok to add a label?

That is ok, but I think you plan to change from label to annotation?

Maybe you like to use label selector for imported services? But you changed the constant name to AntreaMCSAutoGenAnnotation

yeah, my mistake, I think both label or annotation should work, I will check and move it as annotation if it can filter event based on annotation.

Do you watch with filters? I remember watch can have the apiserver side filter the updates with labels. Not sure about annotation.

yes, there is a standalone service_controller.go to do clean up job in case ResourceImport in leader have been removed but member cluster fail to watch the event eg: mcs controller is down accidentally.
controller-runtime provides a predicate to do the filter. I used it like below in service_controller.go, I think annotation should also work with custom predicate funcs, I will try locally first.

func (r *ServiceReconciler) SetupWithManager(mgr ctrl.Manager) error { instance, _ := predicate.LabelSelectorPredicate(metav1.LabelSelector{ MatchLabels: map[string]string{ common.AntreaMCSAutoGenAnnotation: "true", }, }) return ctrl.NewControllerManagedBy(mgr). For(&corev1.Service{}).WithEventFilter(instance). WithOptions(controller.Options{ MaxConcurrentReconciles: common.DefaultWorkerCount, }). Complete(r) }

jianjuns

In commit message:

remote_resourceimport_controller watches leader cluster's ResourceImport events and create corresponding

create -> creates

jianjuns · 2021-11-30T23:36:27Z

multicluster/cmd/multicluster-controller/controller.go

@@ -88,6 +93,11 @@ func run(o *Options) error {
 	} else {
 		o.options.CertDir = certDir
 	}
+	// TODO: These leader checks should go once we have a separate command for leader and member controller
+	if o.leader {
+		// on the leader we want the reconciler to run for a given namspace instead of cluster scope


namesapce -> Namespace

sure, there are some codes from Aravinda's common area, I will leave it to her to address, then I will do a rebase after that. cc @aravindakidambi

Sure, Ill address this

jianjuns · 2021-11-30T23:38:38Z

multicluster/cmd/multicluster-controller/controller.go

+	// TODO: These leader checks should go once we have a separate command for leader and member controller
+	if o.leader {
+		// on the leader we want the reconciler to run for a given namspace instead of cluster scope
+		o.options.Namespace = env.GetPodNamespace()


Question - what will happen if a cluster is both leader and member?

we will provide Namespace based manifests for leader and member, which mean a cluster can run MCS controller both as leader and member at the same time in different Namespace.

jianjuns · 2021-11-30T23:40:41Z

multicluster/cmd/multicluster-controller/controller.go

-	setupLog           = ctrl.Log.WithName("setup")
-	validationWebhooks = []string{"antrea-multicluster-validating-webhook-configuration"}
-	mutationWebhooks   = []string{"antrea-multicluster-mutating-webhook-configuration"}
+	setupLog                      = ctrl.Log.WithName("setup")


@tnqn once asked about using klog. What is the conclusion for that?

sure,we will remove this one @aravindakidambi

jianjuns · 2021-11-30T23:41:56Z

multicluster/cmd/multicluster-controller/controller.go

+		IsMember: o.member,
+	}
+	if err = clusterSetReconciler.SetupWithManager(mgr); err != nil {
+		setupLog.Error(err, "unable to create controller", "controller", "ClusterSet")


We typically do not long if we return the error (which will be logged by the caller too). Could you check if the error logs are necessary?

Sure, will remove them

jianjuns · 2021-11-30T23:47:00Z

multicluster/cmd/multicluster-controller/controller.go

@@ -202,17 +238,32 @@ func createClients(kubeConfig *rest.Config) (
 	return client, aggregatorClient, apiExtensionClient, nil
 }

-func getCAConifg() *certificate.CAConfig {
+func getCAConifg(isLeader bool) *certificate.CAConfig {


I feel it might be better not to call env.GetPodNamespace() everywhere, and so wonder can we call it once in run(), and pass it as a parameter to getCAConfig() and getValidationWebhooks() rather than passing isLeader. No strong opinion. Just for you to consider.

Sure, done.

jianjuns · 2021-12-01T01:47:05Z

multicluster/controllers/multicluster/core/remote_resourceimport_controller.go

+		creationSucceed = true
+	}
+
+	if creationSucceed {


If Service is created in this func, but ServiceImport was created earlier, we should return here too, and not check changes?

jianjuns · 2021-12-01T01:49:32Z

multicluster/controllers/multicluster/core/remote_resourceimport_controller.go

+	// Update MCS logic here
+	if _, ok := svc.Annotations[common.AntreaMCServiceAnnotation]; !ok {
+		klog.InfoS("Service has no desired annotation "+common.AntreaMCServiceAnnotation+", skip update", "service", svcName.String())
+		return ctrl.Result{}, nil


This is when users manually created a Service with MCS prefix, or they removed the annotation on an imported Service? Should we return an error and retry for either case?

sure, I will check this.

I add a step to check conflicts and skip creation if there is the same Service with MCS prefix, are you suggesting that we should always return error if this is a service with MCS prefix but no annotation, so admin can correct it manually?

multicluster/controllers/multicluster/resourceexport_controller.go

jianjuns · 2021-12-01T01:58:21Z

multicluster/controllers/multicluster/resourceexport_controller.go

+			klog.ErrorS(err, "fail to list ResourceExports, retry later")
+			return newResImport, false, err
+		}
+		// when there is only one Service ResourceExport, ResourceImport should reflect the change


How MCSAPI handles this?

they didn't handle this case, MCSAPI repo only create a sample for Service creation.

What you think we return an error and let controller retry (until conflict is resolved)?

I think this kind conflict can only be handled manually, do you mean this kind of situation should be corrected by admin when they do service export, so we can let controller retry after admin resolve conflict?

done, I leave the logic to update ResourceImport when there is only one ResourceExport and for other cases, it will return error. let me know if it's suitable or not.

jianjuns · 2021-12-01T02:05:22Z

multicluster/controllers/multicluster/resourceexport_controller.go

+	switch resExport.Spec.Kind {
+	case common.ServiceKind:
+		resImport, changed, err = r.refreshServiceResourceImport(&resExport, existResImport, createResImport)
+	case common.EndpointsKind:


Today we support exporting only the Endpoints directly, but not externalIP of LB type Services? When later we use externalIP or ClusterIP as the Endpoints, ResourceExportReconciler in the leader cluster should create EndpointImport based on the exported Service spec, or ResourceImportReconciler of member clusters should handle that?

I think ResourceExportReconciler should do this part, I will revisit this later how to minimize the change. thanks.

jianjuns · 2021-12-02T00:33:25Z

multicluster/controllers/multicluster/core/resourceimport_controller.go

@@ -40,12 +42,12 @@ import (

 const (
 	// cached indexer
-	resImportIndexerByNameKind = "resourceimport.by.namekind"
+	resImportIndexer = "resimp.indexer"


Nit - what you think about "name.kind" or even "nk".

done, rename indexer as name.kind

multicluster/controllers/multicluster/core/resourceimport_controller.go

jianjuns · 2021-12-02T00:38:29Z

multicluster/controllers/multicluster/core/resourceimport_controller.go

 		r.installedResImports.Add(*resImp)
+		if len(svcImpObj.Spec.IPs) == 0 {
+			// requeue the event to update ServiceImport's ClusterSetIP
+			return ctrl.Result{}, fmt.Errorf("ServiceImport %s ClusterSetIP is empty", klog.KObj(svcImpObj))


Humm - how long it takes for the retry? Thinking whether we should optimize by watching Services to get the IP faster but I know it is much complexer.

I checked the codes, it seems using the default rate limit here https://github.com/kubernetes/client-go/blob/master/util/workqueue/default_rate_limiters.go#L39, interval should be from 5ms to 10s. if multiple failures happened, the interval will be like baseDelay*2^<num-failures>, baseDelay is 5ms, not sure if this kind of interval is enough or not, I will check if I can add more logic in service_controller to watch Service change, then update the IP

I am still trying to understand if we need a Service controller or not. If not, could we think about a simpler solution to handle this, like some way to requeue the event after a short interval?

service controller only watch those service with mcs annotation, so I suppose it should be OK to have one? I haven't found a valid new way to handle the case when ResourceImport is deleted but event is not handled because of crashed controller in member cluster. I will check if reconciler can run like RunOnce().

@jianjuns about requeue for ClusterSetIP setting, when we return empty result with error, the event will actually be requeued, do you mean the interval controlled by default rate limit is not short enough?

If it is always ms to seconds, that should be good enough.

@jianjuns I removed service_controller and add a standard controller named stale_controller without using kuberbuilder manager framework, it will run only once if no error happens during stale Service/ServiceImport clean up. no unit test code yet, let me know if you have more comments. thanks.

I think better to get @tnqn 's suggestion here. Could you check with him?

sure, I have pinged him directly, let's see if there is comment from @tnqn

jianjuns · 2021-12-02T00:40:28Z

multicluster/controllers/multicluster/core/resourceimport_controller.go

@@ -160,25 +174,27 @@ func (r *ResourceImportReconciler) handleResImpUpdateForService(ctx context.Cont
 		return ctrl.Result{}, err
 	}
 	svcImpObj := getMCServiceImport(resImp)
+	if !reflect.DeepEqual(*svc, corev1.Service{}) {


Should we just check svc.Spec.ClusterIP?

jianjuns · 2021-12-02T00:43:13Z

multicluster/controllers/multicluster/core/resourceimport_controller.go

 	}

-	if creationSucceed {
+	if svcCreated && svcImpCreated {


Should we check both, or just svcImpCreated? At least for r.installedResImports.Add(*resImp) we should not care about svcCreated?

fix via check svcImpNotFound only

jianjuns · 2021-12-02T00:50:19Z

multicluster/controllers/multicluster/core/resourceimport_controller.go

@@ -143,15 +145,27 @@ func (r *ResourceImportReconciler) handleResImpUpdateForService(ctx context.Cont
 	if err != nil && !apierrors.IsNotFound(err) {
 		return ctrl.Result{}, err
 	}
-	var creationSucceed bool
-	mcsObj := getMCSForServiceImport(resImp)
+	if !reflect.DeepEqual(*svc, corev1.Service{}) {


Is it to check if the Service exists or not? If so should we just check IsNotFound, or add a bool variable for that (which can replace svcCreated, svcImpCreated).

done via using svcImpNotFound

jianjuns · 2021-12-02T00:52:54Z

multicluster/controllers/multicluster/core/resourceimport_controller.go

@@ -247,6 +263,13 @@ func (r *ResourceImportReconciler) handleResImpUpdateForEndpoints(ctx context.Co
 	if err != nil && !epNotFound {
 		return ctrl.Result{}, err
 	}
+	if !reflect.DeepEqual(*ep, corev1.Endpoints{}) {


Just check the Endpoints exists or not?

yes, reuse epNotFound now

jianjuns · 2021-12-02T00:55:41Z

multicluster/controllers/multicluster/resourceexport_controller.go

-		// when there is only one Service ResourceExport, ResourceImport should reflect the change
-		// otherwise, it should skip it due to it's impossible to determine which one is desired.
+		// When there is only one Service ResourceExport, ResourceImport should reflect the change
+		// otherwise, it should always return error so controller can retry later if admin fix the issue


How about "assuming users can fix the conflicts"?

jianjuns

In the commit message of "More stale cleanup and remove unnecessary subset info"

add ResourceExport clean up in stale_controller which handle the case
add -> Add, handle -> handles

when controller in member crashed and ServiceExport is deleted during
crash.

crached -> restarts?

multicluster/controllers/multicluster/core/resourceimport_controller.go

jianjuns · 2021-12-15T01:43:37Z

multicluster/controllers/multicluster/stale_controller.go

+	klog.InfoS("Starting StaleController")
+	defer klog.InfoS("Shutting down StaleController")
+
+	if err := c.RunOnce(); err != nil {


But we should do clean up before other controllers start to run, no?

we only clean up those stale resources, eg, remove MC Service and ServiceImport when corresponding ResourceImport does not existing in leader cluster anymore, so it should be fine to no run before other controllers because other controllers will do nothing for a deleted resource which is already gone before reconciling and no delete event at all.

I am thinking about any possible race condition here, e.g. stalecontroller (as it is not atomic to get imports, services, and delete) deletes new Services?

stale controller will get ServiceList from local first, then ResourceImportList from leader, so it means if new derived Service being created but not in ServiceList, we either have new ResourceImport in ResourceImportList or not have it, in first case Service won't be deleted because there is corresponding ResourceImport, in second case, nothing will be done.

jianjuns

Mostly on formatting.

jianjuns · 2021-12-15T23:09:00Z

multicluster/build/yamls/antrea-multicluster-member-only.yml

@@ -1079,6 +1079,217 @@ status:
  conditions: []


I feel we should rename this yml to antrea-multicluster-member.yml. Not necessarily in this PR though.

Sure, I think we can do this rename and move copied yaml to different directory in a new PR.

jianjuns · 2021-12-15T23:10:00Z

multicluster/config/crd/bases/multicluster.x-k8s.io_serviceexports.yaml

+metadata:
+  name: serviceexports.multicluster.x-k8s.io
+spec:
+  group: multicluster.x-k8s.io


Is it possible to put these copied yamls in a separate directory?

multicluster/controllers/multicluster/common/helper.go

multicluster/controllers/multicluster/core/resourceimport_controller.go

multicluster/controllers/multicluster/common/helper.go

multicluster/controllers/multicluster/serviceexport_controller.go

multicluster/controllers/multicluster/stale_controller.go

jianjuns · 2021-12-18T01:45:57Z

multicluster/controllers/multicluster/core/resourceimport_controller.go

+	}
+	if !svcNotFound {
+		// Here we will skip creating derived MC Service when a Service with the same name
+		// is already exists but not previously created by Importer.


is already exists -> already exists but is not

jianjuns · 2021-12-18T01:47:52Z

multicluster/config/crd/bases/multicluster.x-k8s.io_serviceexports.yaml

+metadata:
+  name: serviceexports.multicluster.x-k8s.io
+spec:
+  group: multicluster.x-k8s.io


Reminder for the question.

Is it possible to put these copied yamls in a separate directory?

jianjuns · 2021-12-18T01:52:41Z

multicluster/controllers/multicluster/core/resourceimport_controller.go

+		"service", svcImpName.String(), "resourceimport", klog.KObj(resImp))
+
+	var err error
+	cleanup_serviceimport := func() (ctrl.Result, error) {


cleanupServiceImport

jianjuns · 2021-12-18T01:55:50Z

multicluster/controllers/multicluster/member_clusterset_controller.go

@@ -99,7 +99,7 @@ func (r *MemberClusterSetReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewControllerManagedBy(mgr).


I feel common_area.go in commonarea pkg is not a problem. But would let you decide the appropriate naming here. "core" might be too general and not accurate here.

jianjuns · 2021-12-18T01:57:35Z

multicluster/controllers/multicluster/resourceexport_controller.go

-	client.Client
-	Scheme *runtime.Scheme
+type (
+	// ResourceExportReconciler reconciles a ResourceExport object


Ah, probably we can move the comment here, like "ResourceExportReconciler reconciles ResourceExports in leader cluster"?

jianjuns · 2021-12-18T02:07:18Z

multicluster/controllers/multicluster/resourceexport_controller.go

+
+func (r *ResourceExportReconciler) handleDeleteEvent(ctx context.Context, resImportName types.NamespacedName, resExport *mcsv1alpha1.ResourceExport) error {
+	reList := &mcsv1alpha1.ResourceExportList{}
+	err := r.Client.List(ctx, reList, &client.ListOptions{LabelSelector: getLabelSelector(resExport)})


Ok, thought we can just remove endpoints from the ResourceImport. Not sure that is a better strategy or not.

But could you add some comments?

jianjuns · 2021-12-18T02:14:26Z

multicluster/controllers/multicluster/resourceexport_controller.go

+		return ctrl.Result{}, err
+	}
+
+	resImportName := types.NamespacedName{


Minor suggestion - might be simpler if getResourceImportName() returns a NamespacedName.

good point, refined it.

jianjuns · 2021-12-18T02:19:23Z

multicluster/controllers/multicluster/serviceexport_controller.go

+)
+
+var (
+	leaderNamespace string


I do not mean we must create a new struct to hold these variables, but meant for OO better not to use global variables. I saw you change to pass clusterConfig as func arguments, but originally I was thinking can they be members of ServiceExportReconciler.

multicluster/controllers/multicluster/serviceexport_controller.go

jianjuns · 2021-12-18T02:23:44Z

multicluster/controllers/multicluster/common/helper.go

+)
+
+// TODO: Use NamespacedName stringer method instead of this. e.g. nsName.String()
+func NamespacedName(namespace, name string) string {


namespace -> Namespace

Sorry, this one is my mistake. It should be "namespace" indeed.

luolanzone · 2021-12-20T08:11:45Z

Hi @jianjuns @aravindakidambi I have created a separate PR #3153 to rename core package and move
ServiceExport and ServiceImport CRD manifest to a new forlder. please help to take a look. thanks

jianjuns

LGTM.

jianjuns · 2021-12-20T22:12:42Z

multicluster/controllers/multicluster/common/helper.go

+)
+
+// TODO: Use NamespacedName stringer method instead of this. e.g. nsName.String()
+func NamespacedName(Namespace, name string) string {


Sorry, this one is my mistake. It should be "namespace" indeed.

1. exporter serviceexport_controller is responsible to reconcile ServiceExport resource in member cluster and write wrapped ResourceExport into leader cluster, meanwhile it will watch Service and Endpoints change via event mapping and update ResourceExport when exported Service/Endpoints are updated. resourceexport_controller is responsible to reconcile ResourceExport resources and computes all ResourceExports from different member clusters into one ResourceExport in leader cluster if the resources are exported with the same namespaced name and the same kind, for this moment, we only support Service, Endpoints kind. 2. importer remote_resourceimport_controller watches leader cluster's ResourceImport events and create corresponding ServiceImport, Service and Endpoints with AntreaMCServiceAnnotation in member cluster. ServiceImport name will be the same as exported Service, new created Service and Endpoints will have an antrea multicluster prefix. 3. stale controller stale controller is mainly for special cases when resource is deleted during controller restarting in leader or member cluster, it will be triggered once only when controller starts. Notes: serviceexport_controller, stale_controller and resourceimport_controller will run only in member cluster, resourceexport_controller will run only in leader cluster. Signed-off-by: Lan Luo <[email protected]>

luolanzone · 2021-12-21T01:05:25Z

Hi @jianjuns I fixed the last comment in this PR, Could you help to approve this if it looks good to you? I will rebase the PR#3153 and address your comments on it.

1. exporter serviceexport_controller is responsible to reconcile ServiceExport resource in member cluster and write wrapped ResourceExport into leader cluster, meanwhile it will watch Service and Endpoints change via event mapping and update ResourceExport when exported Service/Endpoints are updated. resourceexport_controller is responsible to reconcile ResourceExport resources and computes all ResourceExports from different member clusters into one ResourceExport in leader cluster if the resources are exported with the same namespaced name and the same kind, for this moment, we only support Service, Endpoints kind. 2. importer remote_resourceimport_controller watches leader cluster's ResourceImport events and create corresponding ServiceImport, Service and Endpoints with AntreaMCServiceAnnotation in member cluster. ServiceImport name will be the same as exported Service, new created Service and Endpoints will have an antrea multicluster prefix. 3. stale controller stale controller is mainly for special cases when resource is deleted during controller restarting in leader or member cluster, it will be triggered once only when controller starts. Notes: serviceexport_controller, stale_controller and resourceimport_controller will run only in member cluster, resourceexport_controller will run only in leader cluster. Signed-off-by: Lan Luo <[email protected]>

luolanzone requested review from jianjuns, aravindakidambi and tnqn November 11, 2021 08:01

luolanzone added the area/multi-cluster Issues or PRs related to multi cluster. label Nov 11, 2021

luolanzone force-pushed the resource-controllers branch 3 times, most recently from 7b380a2 to 646bf65 Compare November 12, 2021 08:06

luolanzone force-pushed the feature/multi-cluster branch from eef025e to adc61a8 Compare November 15, 2021 08:01

luolanzone force-pushed the resource-controllers branch 3 times, most recently from b61b082 to be4ab6e Compare November 16, 2021 02:43

jianjuns reviewed Nov 17, 2021

View reviewed changes

luolanzone force-pushed the resource-controllers branch 2 times, most recently from be5de9c to 479dfc3 Compare November 17, 2021 07:16

luolanzone commented Nov 17, 2021

View reviewed changes

jianjuns reviewed Nov 17, 2021

View reviewed changes

luolanzone force-pushed the feature/multi-cluster branch from 9b32acf to f55dbad Compare November 22, 2021 01:41

luolanzone force-pushed the resource-controllers branch 5 times, most recently from 99a9737 to 839a110 Compare November 24, 2021 09:12

jianjuns reviewed Nov 30, 2021

View reviewed changes

jianjuns reviewed Dec 1, 2021

View reviewed changes

luolanzone force-pushed the resource-controllers branch 3 times, most recently from e76b682 to dff1449 Compare December 1, 2021 09:13

jianjuns reviewed Dec 2, 2021

View reviewed changes

luolanzone changed the title ~~exporter/importer and unit test~~ Resource exporter/importer and unit test Dec 2, 2021

jianjuns reviewed Dec 15, 2021

View reviewed changes

luolanzone force-pushed the resource-controllers branch 2 times, most recently from 8aebad0 to c86cf1a Compare December 15, 2021 02:51

luolanzone mentioned this pull request Dec 15, 2021

Multicluster Integration test #3031

Merged

luolanzone force-pushed the resource-controllers branch 2 times, most recently from 0810d08 to 55254ac Compare December 15, 2021 06:00

jianjuns reviewed Dec 16, 2021

View reviewed changes

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

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

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

luolanzone force-pushed the resource-controllers branch 3 times, most recently from d450c75 to e6ff1e9 Compare December 17, 2021 02:14

jianjuns reviewed Dec 18, 2021

View reviewed changes

luolanzone force-pushed the resource-controllers branch from e6ff1e9 to faafaf9 Compare December 20, 2021 06:47

luolanzone mentioned this pull request Dec 20, 2021

Rename package 'core' to 'commonarea' #3153

Merged

luolanzone force-pushed the resource-controllers branch from faafaf9 to b03f63a Compare December 20, 2021 09:05

jianjuns reviewed Dec 20, 2021

View reviewed changes

luolanzone force-pushed the resource-controllers branch from b03f63a to 1d33ce2 Compare December 21, 2021 01:02

luolanzone merged commit f9e5f48 into antrea-io:feature/multi-cluster Dec 22, 2021

		@@ -99,7 +99,7 @@ func (r *MemberClusterSetReconciler) SetupWithManager(mgr ctrl.Manager) error {
		return ctrl.NewControllerManagedBy(mgr).

Resource exporter/importer and unit test #3024

Resource exporter/importer and unit test #3024

Conversation

luolanzone commented Nov 11, 2021 • edited by jianjuns Loading

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

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 Nov 17, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 17, 2021 • edited Loading

Codecov Report

luolanzone Nov 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianjuns Dec 18, 2021 • 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

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

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 Nov 11, 2021 •

edited by jianjuns

Loading

luolanzone Nov 17, 2021 •

edited

Loading

codecov-commenter commented Nov 17, 2021 •

edited

Loading

luolanzone Nov 17, 2021 •

edited

Loading

jianjuns Dec 18, 2021 •

edited

Loading