clustermesh: fix remote service deletion on endpointslicesync #32961

MrFreezeex · 2024-06-07T13:14:47Z

This fixes remote service deletion in endpointslicesync (see second commit).

And it also fixes tests by using EventuallyWithT instead of waiting for the controller queue to be empty. The queue being empty does not signify that the reconciliation is done as the controller pops elements when the reconciliation starts and not when it ends.

cc @giorio94
Fixes #32931

giorio94

Thanks. I've left a couple of comments inline.

pkg/clustermesh/endpointslicesync/endpointslice_test.go

MrFreezeex · 2024-06-08T00:24:00Z

Thanks for the feedback and finding the issue + the root cause of the test @giorio94 🙏.

I unfortunately had to remove one of the test involving a remote service and no local service because I cannot really create an endpointslice locally and check that it's removed because this deletion is made through the normal ownerreferences.

MrFreezeex · 2024-06-08T00:24:04Z

/test

giorio94

A couple of final comments, but overall looks good to me, thanks!

pkg/clustermesh/endpointslicesync/endpointslice_test.go

pkg/clustermesh/endpointslicesync/service_informer.go

giorio94 · 2024-06-10T09:56:00Z

I unfortunately had to remove one of the test involving a remote service and no local service because I cannot really create an endpointslice locally and check that it's removed because this deletion is made through the normal ownerreferences.

How about asserting in one of the tests that the resulting endpointslice has the correct owner reference? That would give us confidence that k8s will then take care of removing the stale endpointslices.

MrFreezeex · 2024-06-10T12:39:55Z

/test

MrFreezeex · 2024-06-10T13:41:42Z

/test

giorio94

Thanks, just a couple of nits inline. I've re-run the stress test, and hit no failures after 5k runs.

pkg/clustermesh/endpointslicesync/endpointslice_test.go

pkg/k8s/resource_ctors.go

pkg/clustermesh/endpointslicesync/endpointslice_test.go

MrFreezeex · 2024-06-11T08:49:46Z

/test

pkg/clustermesh/endpointslicesync/endpointslice_test.go

MrFreezeex · 2024-06-11T09:00:43Z

/test

giorio94 · 2024-06-13T12:42:19Z

@MrFreezeex One of the unit test failures are legitimate:

 --- FAIL: Test_meshEndpointSlice_Reconcile/Create_service_then_global_service (0.01s)

I0611 09:13:26.678082   49847 endpointslice_controller.go:296] "Starting endpoint slice controller"
I0611 09:13:26.678103   49847 shared_informer.go:313] Waiting for caches to sync for endpoint_slice
I0611 09:13:26.678110   49847 shared_informer.go:320] Caches are synced for endpoint_slice
    endpointslice_test.go:161: 
        	Error Trace:	/home/runner/work/cilium/cilium/pkg/clustermesh/endpointslicesync/endpointslice_test.go:161
        	Error:      	Not equal: 
        	            	expected: "service"
        	            	actual  : "Service"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-service
        	            	+Service
        	Test:       	Test_meshEndpointSlice_Reconcile/Create_service_then_global_service

MrFreezeex · 2024-06-13T12:49:00Z

@MrFreezeex One of the unit test failures are legitimate:

 --- FAIL: Test_meshEndpointSlice_Reconcile/Create_service_then_global_service (0.01s)

I0611 09:13:26.678082   49847 endpointslice_controller.go:296] "Starting endpoint slice controller"
I0611 09:13:26.678103   49847 shared_informer.go:313] Waiting for caches to sync for endpoint_slice
I0611 09:13:26.678110   49847 shared_informer.go:320] Caches are synced for endpoint_slice
    endpointslice_test.go:161: 
        	Error Trace:	/home/runner/work/cilium/cilium/pkg/clustermesh/endpointslicesync/endpointslice_test.go:161
        	Error:      	Not equal: 
        	            	expected: "service"
        	            	actual  : "Service"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-service
        	            	+Service
        	Test:       	Test_meshEndpointSlice_Reconcile/Create_service_then_global_service

Woops indeed my bad!

MrFreezeex · 2024-06-13T12:49:05Z

/test

giorio94 · 2024-06-14T15:43:44Z

@MrFreezeex, could you please rebase this PR? That should fix the failing tests

MrFreezeex · 2024-06-14T15:51:00Z

/test

MrFreezeex · 2024-06-15T12:02:18Z

/ci-ingress

MrFreezeex · 2024-06-15T12:02:29Z

/ci-ginkgo

MrFreezeex · 2024-06-15T12:02:39Z

/ci-eks

MrFreezeex · 2024-06-15T12:02:50Z

/ci-ipsec-upgrade

MrFreezeex · 2024-06-16T13:03:35Z

/ci-ingress

aanm · 2024-06-17T07:50:07Z

@MrFreezeex can you rebase? it looks there are a couple of conflicting files.

Use EventuallyWithT instead of waiting for the controller queue to be empty. The queue being empty does not signify that the reconciliation is done as the controller pops elements when the reconciliation starts and not when it ends. Suggested-by: Marco Iorio <marco.iorio@isovalent.com> Signed-off-by: Arthur Outhenin-Chalandre <arthur@cri.epita.fr>

This commit fixes the case when a remote service gets deleted while the local service remains. Previously the Reconcile on the controller wasn't called because we weren't returning the deleted Service in the service informer List method which may leave some EndpointSlice in the cluster that should be deleted. In an usual Kubernetes environment this is not an issue since the OwnerReference should be deleting the EndpointSlices as well. In our case the actual Service still exist because we have this mechanism of creating "virtual" service by adding the cluster name in suffix to trigger a reconcile on each remote cluster. To fix that we are now returning the combination of all the local Services and the remote clusters instead of returning all the remote services. This allows to trigger a reconcile on all the possible services including some of them that don't exist which would make the Get method of the Service informer to return a not found error which will then trigger a deletion via our cleanup hook. Signed-off-by: Arthur Outhenin-Chalandre <arthur@cri.epita.fr>

Signed-off-by: Arthur Outhenin-Chalandre <arthur@cri.epita.fr>

MrFreezeex · 2024-06-17T09:12:35Z

/test

MrFreezeex requested a review from a team as a code owner June 7, 2024 13:14

MrFreezeex requested a review from thorn3r June 7, 2024 13:14

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 7, 2024

giorio94 added area/clustermesh Relates to multi-cluster routing functionality in Cilium. release-note/misc This PR makes changes that have no direct user impact. labels Jun 7, 2024

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 7, 2024

giorio94 requested changes Jun 7, 2024

View reviewed changes

MrFreezeex force-pushed the endpointslice-sync-race-test-fix branch from 47aae40 to 8bf26e9 Compare June 8, 2024 00:12

MrFreezeex changed the title ~~clustermesh: use EventuallyWithT in endpointslicemeshsync~~ clustermesh: fix remote service deletion on endpointslicesync Jun 8, 2024

MrFreezeex force-pushed the endpointslice-sync-race-test-fix branch from 8bf26e9 to ce016b6 Compare June 8, 2024 00:17

MrFreezeex requested a review from giorio94 June 8, 2024 00:24

giorio94 reviewed Jun 10, 2024

View reviewed changes

MrFreezeex force-pushed the endpointslice-sync-race-test-fix branch 2 times, most recently from e41827b to 6e91b95 Compare June 10, 2024 11:07

MrFreezeex requested a review from a team as a code owner June 10, 2024 12:37

MrFreezeex requested a review from nathanjsweet June 10, 2024 12:37

MrFreezeex force-pushed the endpointslice-sync-race-test-fix branch from 6639d0a to 922c957 Compare June 10, 2024 13:41

giorio94 approved these changes Jun 11, 2024

View reviewed changes

giorio94 removed the request for review from thorn3r June 11, 2024 08:03

MrFreezeex force-pushed the endpointslice-sync-race-test-fix branch from 922c957 to ea9d527 Compare June 11, 2024 08:48

giorio94 reviewed Jun 11, 2024

View reviewed changes

pkg/clustermesh/endpointslicesync/endpointslice_test.go Show resolved Hide resolved

MrFreezeex force-pushed the endpointslice-sync-race-test-fix branch from ea9d527 to caf7894 Compare June 11, 2024 09:00

MrFreezeex force-pushed the endpointslice-sync-race-test-fix branch from caf7894 to 16fdde9 Compare June 13, 2024 12:48

MrFreezeex force-pushed the endpointslice-sync-race-test-fix branch from 16fdde9 to 057adb4 Compare June 14, 2024 15:50

aanm added the kind/bug This is a bug in the Cilium logic. label Jun 17, 2024

MrFreezeex added 3 commits June 17, 2024 10:51

clustermesh: add namespace index to service resource

217acee

Signed-off-by: Arthur Outhenin-Chalandre <arthur@cri.epita.fr>

MrFreezeex force-pushed the endpointslice-sync-race-test-fix branch from 057adb4 to 217acee Compare June 17, 2024 08:52

aanm enabled auto-merge June 17, 2024 10:48

aanm approved these changes Jun 17, 2024

View reviewed changes

aanm added this pull request to the merge queue Jun 17, 2024

Merged via the queue into cilium:main with commit b7237b1 Jun 17, 2024

clustermesh: fix remote service deletion on endpointslicesync #32961

clustermesh: fix remote service deletion on endpointslicesync #32961

Uh oh!

Conversation

MrFreezeex commented Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

giorio94 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MrFreezeex commented Jun 8, 2024

Uh oh!

MrFreezeex commented Jun 8, 2024

Uh oh!

giorio94 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

giorio94 commented Jun 10, 2024

Uh oh!

MrFreezeex commented Jun 10, 2024

Uh oh!

MrFreezeex commented Jun 10, 2024

Uh oh!

giorio94 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MrFreezeex commented Jun 11, 2024

Uh oh!

Uh oh!

MrFreezeex commented Jun 11, 2024

Uh oh!

giorio94 commented Jun 13, 2024

Uh oh!

MrFreezeex commented Jun 13, 2024

Uh oh!

MrFreezeex commented Jun 13, 2024

Uh oh!

giorio94 commented Jun 14, 2024

Uh oh!

MrFreezeex commented Jun 14, 2024

Uh oh!

MrFreezeex commented Jun 15, 2024

Uh oh!

MrFreezeex commented Jun 15, 2024

Uh oh!

MrFreezeex commented Jun 15, 2024

Uh oh!

MrFreezeex commented Jun 15, 2024

Uh oh!

MrFreezeex commented Jun 16, 2024

Uh oh!

aanm commented Jun 17, 2024

Uh oh!

MrFreezeex commented Jun 17, 2024

Uh oh!

Uh oh!

MrFreezeex commented Jun 7, 2024 •

edited

Loading