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.

Sign up for GitHub

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

Jump to bottom

connectivity: add endpointslice clustermesh sync test #2267

Merged
merged 1 commit into from
Apr 17, 2024
Merged

connectivity: add endpointslice clustermesh sync test #2267

merged 1 commit into from
Apr 17, 2024

Conversation

MrFreezeex
Copy link
Member

This adds endpointslice synchronization testing inside a clustermesh. It does that by checking that we can get a DNS answer with a global headless service with endpointslice synchronization enabled across two clusters.

Testing it via DNS ensures that the created EndpointSlice are correctly linked to the headless service.

Related to cilium/cilium#28440 e2e testing

@MrFreezeex MrFreezeex requested a review from a team as a code owner January 28, 2024 16:42
@MrFreezeex MrFreezeex marked this pull request as draft January 28, 2024 16:44
@MrFreezeex MrFreezeex force-pushed the add-endpointslice-sync-test branch from 7d82a98 to 3ed32d4 Compare January 28, 2024 16:45
@MrFreezeex
Copy link
Member Author

MrFreezeex commented Jan 28, 2024 •
edited
Loading

I am converting this to draft as I guess we would need to merge cilium/cilium#28440 first? Unsure of the exact timing here 😅 and not sure if I added the correct pre-requirements to launch this test, checking 1.16.0 + that we have clustermesh seems logical to me but maybe there would be more check before launching this?

I tested that locally and it seems to work fine but not sure how to trigger this in the CI as well as this test an ongoing PR...

[=] Test [endpointslice-mesh] [66/66]
..

✅ All 1 tests (2 actions) successful, 65 tests skipped, 0 scenarios skipped.

@MrFreezeex MrFreezeex force-pushed the add-endpointslice-sync-test branch from 3ed32d4 to 5055852 Compare January 28, 2024 16:50
@MrFreezeex MrFreezeex mentioned this pull request Jan 28, 2024
Merged
8 tasks
@MrFreezeex MrFreezeex force-pushed the add-endpointslice-sync-test branch from 5055852 to 49841b5 Compare March 7, 2024 11:56
@MrFreezeex MrFreezeex force-pushed the add-endpointslice-sync-test branch from 49841b5 to 174b554 Compare March 7, 2024 12:15
@MrFreezeex MrFreezeex force-pushed the add-endpointslice-sync-test branch from 174b554 to 9e8d3aa Compare March 7, 2024 12:40
@MrFreezeex MrFreezeex force-pushed the add-endpointslice-sync-test branch from 9e8d3aa to f825bf6 Compare March 7, 2024 12:56
giorio94
giorio94 requested changes Mar 18, 2024
View reviewed changes
Copy link
Member

@giorio94 giorio94 left a comment

Choose a reason for hiding this comment

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

Thanks! I've left a couple of comments and suggestions inline, but the overall approach looks sensible to me. CI failures appear legitimate (at least a few), as the newly introduce headless service should be skipped in a few more places.

connectivity/check/wait.go Outdated Show resolved Hide resolved
connectivity/tests/endpointslice-clustermesh-sync.go Outdated Show resolved Hide resolved
connectivity/tests/endpointslice-clustermesh-sync.go Outdated Show resolved Hide resolved
connectivity/tests/endpointslice-clustermesh-sync.go Outdated Show resolved Hide resolved
connectivity/builder/endpointslice-clustermesh-sync.go Outdated Show resolved Hide resolved
connectivity/tests/endpointslice-clustermesh-sync.go Outdated Show resolved Hide resolved
connectivity/check/wait.go
Comment on lines +145 to +172
if service.Service.Spec.ClusterIP == corev1.ClusterIPNone {
// If the cluster is headless there is nothing to wait for here
return nil
}
Copy link
Member

Choose a reason for hiding this comment

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

It could make sense to have a wait mechanism for headless services as well (e.g., checking the epslices correctness), but it doesn't seem a likely source of flakiness (given that synchronization requires way less than all the other steps). I'm personally fine with leaving that out for the moment, and adding it later on if we discover we really need it.

@MrFreezeex MrFreezeex force-pushed the add-endpointslice-sync-test branch from f825bf6 to 3e54bc3 Compare March 18, 2024 18:30
@MrFreezeex MrFreezeex force-pushed the add-endpointslice-sync-test branch from 3e54bc3 to b06b740 Compare March 18, 2024 18:41
@MrFreezeex
Copy link
Member Author

as the newly introduce headless service should be skipped in a few more places.

Ah yes now I need to ignore this from some tests and not only from the "deployment" phase, will look into it

@MrFreezeex MrFreezeex force-pushed the add-endpointslice-sync-test branch from b06b740 to 6e2ae67 Compare March 21, 2024 11:45
@MrFreezeex MrFreezeex force-pushed the add-endpointslice-sync-test branch from 6e2ae67 to a9262bb Compare March 21, 2024 11:49
@MrFreezeex MrFreezeex requested a review from giorio94 March 21, 2024 12:36
@MrFreezeex MrFreezeex marked this pull request as ready for review March 21, 2024 14:00
@MrFreezeex MrFreezeex requested review from a team as code owners March 21, 2024 14:00
@MrFreezeex MrFreezeex requested review from bimmlerd and asauber March 21, 2024 14:01
bimmlerd
bimmlerd approved these changes Mar 21, 2024
View reviewed changes
Copy link
Member

@bimmlerd bimmlerd left a comment

Choose a reason for hiding this comment

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

CODEOWNERS lgtm

giorio94
giorio94 reviewed Mar 21, 2024
View reviewed changes
Copy link
Member

@giorio94 giorio94 left a comment

Choose a reason for hiding this comment

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

A couple of final comments inline, then I'll open the PR to test it on cilium/cilium.

connectivity/tests/endpointslice-clustermesh-sync.go Outdated Show resolved Hide resolved
connectivity/check/deployment.go Outdated Show resolved Hide resolved
connectivity/tests/endpointslice-clustermesh-sync.go Outdated Show resolved Hide resolved
@MrFreezeex MrFreezeex force-pushed the add-endpointslice-sync-test branch from a9262bb to 51c4c83 Compare March 21, 2024 18:05
@giorio94 giorio94 mentioned this pull request Mar 22, 2024
Merged
giorio94
giorio94 approved these changes Mar 22, 2024
View reviewed changes
Copy link
Member

@giorio94 giorio94 left a comment

Choose a reason for hiding this comment

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

Thanks!

Additionally tested in cilium/cilium#31551 as well.

@MrFreezeex MrFreezeex force-pushed the add-endpointslice-sync-test branch from 51c4c83 to 3b63b9c Compare March 29, 2024 18:19
@MrFreezeex MrFreezeex force-pushed the add-endpointslice-sync-test branch from 3b63b9c to d8b03d9 Compare March 29, 2024 18:57
@MrFreezeex MrFreezeex force-pushed the add-endpointslice-sync-test branch from d8b03d9 to b44522d Compare April 11, 2024 08:14
@MrFreezeex
connectivity: add endpointslice clustermesh sync test
627ff82 
This adds endpointslice synchronization testing inside a clustermesh.
It does that by checking that we can get a DNS answer with a global
headless service with endpointslice synchronization enabled across two clusters.

Testing it via DNS ensures that the created EndpointSlice are correctly
linked to the headless service.

Signed-off-by: Arthur Outhenin-Chalandre <[email protected]>
@MrFreezeex MrFreezeex force-pushed the add-endpointslice-sync-test branch from b44522d to 627ff82 Compare April 11, 2024 19:45
@MrFreezeex
Copy link
Member Author

MrFreezeex commented Apr 11, 2024 •
edited
Loading

This should be ready 🎉 (I didn't change any code since the reviews, just rebasing for most of the gh actions to pass), the Multicluster action failing seems to be an unrelated flake that could be retried I believe

@giorio94
Copy link
Member

the Multicluster action failing seems to be an unrelated flake that could be retried I believe

Yep, I've triggered a retry

@giorio94 giorio94 added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 15, 2024
@aditighag aditighag merged commit d7fa6d4 into cilium:main Apr 17, 2024
12 of 13 checks passed
@BrewTestBot BrewTestBot mentioned this pull request Apr 26, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@giorio94 giorio94 giorio94 approved these changes

@bimmlerd bimmlerd bimmlerd approved these changes

@christarazi christarazi christarazi approved these changes

@asauber asauber asauber approved these changes

Assignees
No one assigned
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@MrFreezeex @giorio94 @asauber @christarazi @bimmlerd @aditighag