Clean up Endpoint kind of ResourceExport #3652
Conversation
|
/test-multicluster-integration |
Codecov Report
@@ Coverage Diff @@
## main #3652 +/- ##
==========================================
- Coverage 64.63% 57.19% -7.44%
==========================================
Files 278 392 +114
Lines 39360 54807 +15447
==========================================
+ Hits 25439 31347 +5908
- Misses 11945 21003 +9058
- Partials 1976 2457 +481
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| } | ||
|
|
||
| if err := r.Client.Get(ctx, req.NamespacedName, &svcExport); err != nil { | ||
| klog.V(2).ErrorS(err, "Unable to fetch ServiceExport", "serviceexport", req.String()) |
There was a problem hiding this comment.
I thought it's unnecessary to print by default considering the returned error will be recorded by log, I change it to default klog.ErrorS.
| return nil | ||
| } | ||
|
|
||
| if err := r.Client.Get(ctx, req.NamespacedName, &svcExport); err != nil { |
There was a problem hiding this comment.
Do we need a Get call? If we watch ServiceExport, then we should have a Lister to get the ServiceExport from cache?
The same question for other Gets in this file.
There was a problem hiding this comment.
It's limited by the the whole framework which is generated by kube-builder. It doesn't provide a Lister. If we write our own controller to watch events, we may use a Lister.
There was a problem hiding this comment.
Are you saying all kube-builder controllers act like this? But a Get call is expensive.
There was a problem hiding this comment.
Yes, the main logic is in Reconcile(ctx context.Context, req ctrl.Request). https://book.kubebuilder.io/cronjob-tutorial/controller-overview.html
I think the manager optimize the scenario and use cache by default.
Reconcile actually performs the reconciling for a single named object. Our [Request](https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/reconcile?tab=doc#Request) just has a name, but we can use the client to fetch that object from the cache.
There was a problem hiding this comment.
Ok. Seems the client indeed reads from cache.
| } | ||
|
|
||
| if err := r.Client.Get(ctx, req.NamespacedName, &svcExport); err != nil { | ||
| klog.ErrorS(err, "Unable to fetch ServiceExport", "serviceexport", req.String()) |
There was a problem hiding this comment.
Ok, if it is logged in the caller, I am fine not to log here (and we should not log not found as an error if it is not).
There was a problem hiding this comment.
move it after !apierrors.IsNotFound(err)
| return nil | ||
| } | ||
|
|
||
| if err := r.Client.Get(ctx, req.NamespacedName, &svcExport); err != nil { |
There was a problem hiding this comment.
Are you saying all kube-builder controllers act like this? But a Get call is expensive.
If Service is deleted after it's exported successfully, Endpoint kind of ResourceExport is not deleted. Fix this bug to delete both Service and Endpoint kinds of ResourceExport when a Service is deleted but ServiceExport is still kept. Signed-off-by: Lan Luo <luola@vmware.com>
|
/test-multicluster-e2e |
If Service is deleted after it's exported successfully, Endpoint kind
of ResourceExport is not deleted. Fix this bug to delete both Service
and Endpoint kinds of ResourceExport when a Service is deleted but
ServiceExport is still kept.
Signed-off-by: Lan Luo luola@vmware.com