Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add serviceexport and resourceexport controllers #22

Merged
merged 1 commit into from
Oct 13, 2021

Conversation

luolanzone
Copy link
Collaborator

  1. add serviceexport_controller, for each ServiceExport CR, it will have two
    ResourceExports in leader cluster, one kind is Service, another one is Endpoints,
    the name format is ---.
  2. resourceexport_controller will watch resourceexport event
    and extract service/endpoints from resourceexport and wrapp
    them from member clusters into one ResourceImport if source service's
    Namespace and Name are the same
  3. disable auth proxy since we didn't add manager_auth_proxy_patch.yaml in default kustomization.yaml
  4. regenerate multi-cluster.yaml

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

SourceServiceType = "SourceServiceType"
)

var LeaderNameSpace string
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var LeaderNameSpace string
var LeaderNamespace string

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Scheme *runtime.Scheme
K8sClient kubernetes.Interface
LeaderK8sClient kubernetes.Interface
K8smcsCrdClient k8smcsversioned.Interface
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
K8smcsCrdClient k8smcsversioned.Interface
K8sMcsCrdClient k8smcsversioned.Interface

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


func svcInfoKeyFunc(obj interface{}) (string, error) {
svc := obj.(*svcInfo)
return svc.namespace + svc.name, nil
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can use the NamespacedName func to return such keys

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, add a func NamespacedName in helper.go and call it here.


func epInfoKeyFunc(obj interface{}) (string, error) {
ep := obj.(*epInfo)
return ep.namespace + ep.name, nil
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

return ep.namespace + ep.name, nil
}

func epIndexerByLabelFunc(obj interface{}) ([]string, error) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should the labels be normalized in a string to make sure it always returns a predictable string

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, sort the key first, then append them as a string.

}
clusterID := LocalClusterID

key := req.Namespace + req.Name
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here too you could use the namespacedName func

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

//+kubebuilder:rbac:groups="",resources=services,verbs=get;list;watch;update
//+kubebuilder:rbac:groups="",resources=endpoints,verbs=get;list;watch;update
//+kubebuilder:rbac:groups="",resources=nodes,verbs=get;list;watch

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

general comment for the package: maybe add docstrings for each func and methods to explain what is being done.. more comments in the code .. the better

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

1. add serviceexport_controller, for each ServiceExport CR, it will have two
ResourceExports in leader cluster, one kind is Service, another one is Endpoints,
the name format is <clusterid>-<namespace>-<servicename>-<kind>.
2. resourceexport_controller will watch resourceexport event
and extract service/endpoints from resourceexport and wrapp
them from member clusters into one ResourceImport if source service's
Namespace and Name are the same
3. disable auth proxy since we didn't add manager_auth_proxy_patch.yaml in default kustomization.yaml
4. regenerate multi-cluster.yaml

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

resImportName := getResourceImportName(&existRe)
if len(reList.Items) == 0 {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i know what we are doing here .. but would be good to add comments for someone not aware of why we need to check the resExport list before cleaning up the relevant resImp

if existRe.Spec.Kind == ServiceKind {
return ctrl.Result{}, nil
}
uniqueIndex := existRe.Spec.Namespace + existRe.Spec.Name + existRe.Spec.ClusterID + EndpointsKind
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this sounds like could be a func too..

_ = log.FromContext(ctx)
klog.V(2).Infof("reconcile for %s", req.NamespacedName)
var resExport mcsv1alpha1.ResourceExport
if err := r.Client.Get(ctx, req.NamespacedName, &resExport); err != nil {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we break up the Reconcile func to deleteResExport() add/updateResExport() ? to make it easier to read

for eg following block of code is for delete event ..

klog.Infof("local ClusterID is %s", clusterID)

var svcExport k8smcsv1alpha1.ServiceExport
key := NamespacedName(req.Namespace, req.Name)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i noticed req has NamespacedName

@abhiraut
Copy link
Owner

some of these were old comments.. merging it and we can follow up on TODOs

@abhiraut abhiraut merged commit a81dc58 into abhiraut:staging/multi-cluster Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants