Add clusterID annotation to ServiceExport/Import #3359
Conversation
|
/test-multicluster-e2e |
luolanzone
requested review from
bangqipropel,
jianjuns,
hjiajing and
Dyanngg
Codecov Report
@@ Coverage Diff @@
## main #3359 +/- ##
==========================================
+ Coverage 60.85% 60.86% +0.01%
==========================================
Files 268 268
Lines 26723 26753 +30
==========================================
+ Hits 16263 16284 +21
- Misses 8649 8655 +6
- Partials 1811 1814 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| Namespace: resImp.Spec.Namespace, | ||
| Name: resImp.Spec.Name, | ||
| Namespace: resImp.Spec.Namespace, | ||
| Annotations: map[string]string{"localClusterID": clusterID}, |
There was a problem hiding this comment.
Can we use format similar to Antrea and K8s's well know annotations? There should be a prefix defining its scope, and words should be connected with "-". For instances:
node.antrea.io/mac-address
service.antrea.io/external-ip-pool
service.kubernetes.io/topology-aware-hints
There was a problem hiding this comment.
done, add a new annotation const AntreaMCClusterIDAnnotation = "multicluster.antrea.io/local-cluster-id" in helper.go
There was a problem hiding this comment.
done, fix it and unit test. thanks.
|
|
||
| if err = r.updateSvcExportAnnotation(svcExport); err != nil { | ||
| // Ignore the error since it's not critical and we can update in next event. | ||
| klog.V(2).ErrorS(err, "failed to update ServiceExport annotation", "serviceexport", klog.KObj(&svcExport)) |
There was a problem hiding this comment.
But with V(2), the error will never be caught if there is something wrong leading to the failure permanently.
And first word in log message should be capitalized.
There was a problem hiding this comment.
done. there are quite a lot capitalization issue in other logs and files, I will submit a separate PR for it.
luolanzone
force-pushed
the
add-clusterid
branch
2 times, most recently
from
f73144b
to
ae0f937
Compare
| func checkAnnotation(t *testing.T, svcImport *k8smcsapi.ServiceImport) { | ||
| id, ok := svcImport.Annotations[common.AntreaMCClusterIDAnnotation] | ||
| if id != localClusterID || !ok { | ||
| t.Errorf("latest ServiceImport annotation should be "+localClusterID+" but got %v", id) |
There was a problem hiding this comment.
why don't use the expected cluster IP but an uncommon place holder?
t.Errorf("latest ServiceImport annotation should be %v but got %v", localClusterID, id)
Or
assert.Equal(t, localClusterID, id, "Unexpected ServiceImport clusterID annotation")
| @@ -378,12 +383,21 @@ func (r *ServiceExportReconciler) handleEndpointDeleteEvent(ctx context.Context, | |||
| return nil | |||
| } | |||
|
|
|||
| func (r *ServiceExportReconciler) updateSvcExportAnnotation(svcExport k8smcsv1alpha1.ServiceExport) error { | |||
There was a problem hiding this comment.
nit: this should use pointer, otherwise there would be an unnecessary copy.
| func checkAnnotation(t *testing.T, svcExport *k8smcsv1alpha1.ServiceExport) { | ||
| id, ok := svcExport.Annotations[common.AntreaMCClusterIDAnnotation] | ||
| if id != localClusterID || !ok { | ||
| t.Errorf("latest ServiceExport annotation should be "+localClusterID+" but got %v", id) |
Add local cluster ID as an annotation to ServiceExport/Import which help to simplify the logic to group ServiceExport/Import in Antctl. Signed-off-by: Lan Luo <luola@vmware.com>
luolanzone
force-pushed
the
add-clusterid
branch
from
ae0f937
to
08fabf7
Compare
|
/skip-networkpolicy |
Add local cluster ID as an annotation to ServiceExport/Import which help to simplify the logic to group ServiceExport/Import in Antctl. Signed-off-by: Lan Luo <luola@vmware.com>
Add local cluster ID as an annotation to ServiceExport/Import
which help to simplify the logic to group ServiceExport/Import in
Antctl.
Signed-off-by: Lan Luo luola@vmware.com