daemon: drop the non headless service/endpoint resources #35838

MrFreezeex · 2024-11-07T12:29:50Z

Please ensure your pull request adheres to the following guidelines:

For first time contributors, read Submitting a pull request
All code is covered by unit and/or runtime tests where feasible.
All commits contain a well written commit description including a title,
description and a Fixes: #XXX line if the commit addresses a particular
GitHub issue.
If your commit description contains a Fixes: <commit-id> tag, then
please add the commit author[s] as reviewer[s] to this issue.
All commits are signed off. See the section Developer’s Certificate of Origin
Provide a title or release-note blurb suitable for the release notes.
Are you a user of Cilium? Please add yourself to the Users doc
Thanks for contributing!

Before this PR two types of resources were exposed for services and endpoints: one excluding headless services/endpoints and one including them. This PR drop the non headless resource (and make the necessary changes to accommodate that) to make sure we only start one informer for services and endpoints and lower the load the apiserver.

Fixes: #35797

Remove duplicated watch on services and endpoint in the cilium-agent

MrFreezeex · 2024-11-07T12:31:25Z

/test

MrFreezeex · 2024-11-07T12:49:14Z

All the usage of service/endpoints resources and service cache in the daemon:

Service cache support headless services:

cilium/pkg/k8s/service_cache.go

Line 337 in 6e7ea13

svcID, newService := ParseService(k8sSvc, addrs)

->

cilium/pkg/k8s/service.go

Line 210 in 6e7ea13

headless := false
Policy: services are from service cache

cilium/pkg/policy/k8s/service.go

Line 100 in 6e7ea13

p.svcCache.ForEachService(func(svcID k8s.ServiceID, svc *k8s.Service, eps *k8s.EndpointSlices) bool {

&

cilium/pkg/policy/k8s/watcher.go

Line 35 in 6e7ea13

svcCacheNotifications <-chan k8s.ServiceNotification

. And policy handling doesn't seems to require the frontend IP. Letting headless services be processed should result in CNP being able to process headless services without any new handling.
BGP speaker (metallb integration): Doesn't seems to read the cluster IP at all but rather the IP in the service status, see:

cilium/pkg/bgp/speaker/speaker.go

Line 324 in 6e7ea13

ing := make([]v1.LoadBalancerIngress, len(in.Status.LoadBalancer.Ingress))
Datapath from service cache: The datapath already ignores headless services after getting them from the service cache:

cilium/pkg/k8s/watchers/service.go

Line 213 in 6e7ea13

// Headless services do not need any datapath implementation

&

cilium/pkg/k8s/watchers/service.go

Line 508 in 6e7ea13

// Headless services do not need any datapath implementation

. However the is headless is not entirely correct according to a SIG network test (see the commit on this PR for the related fix)
Redirect policy: Seems to be handling headless services well by ignoring service without frontend ips:

cilium/pkg/redirectpolicy/manager.go

Line 491 in 6e7ea13

ip := rpm.svcCache.GetServiceFrontendIP(*config.serviceID, lb.SVCTypeClusterIP)

MrFreezeex · 2024-11-07T12:55:37Z

/test

MrFreezeex · 2024-11-07T14:32:02Z

/test

MrFreezeex · 2024-11-07T14:48:31Z

/test

MrFreezeex · 2024-11-07T15:13:26Z

/test

MrFreezeex · 2024-11-07T15:43:34Z

/test

MrFreezeex · 2024-11-07T15:44:20Z

/test

MrFreezeex · 2024-11-08T10:52:02Z

/test

MrFreezeex · 2024-11-08T11:43:25Z

/ci-l4lb

MrFreezeex · 2024-11-08T12:18:34Z

I believe this should be ready for reviews. Although, it needs reviews from more groups than sig-k8s/sig-agent, and in particular (according to code owners) @cilium/sig-bgp, @cilium/sig-policy , @cilium/sig-lb (for redirect policy).

I tried to summarize the impact of this change here: #35838 (comment), TLDR: according to my research it should have no impact outside of policies that will start to include headless Services (which might count as an additional feature?). But this needs reviews from all the groups mentioned above to validate that and have their opinion.

Also cc @giorio94 @marseel that found this issue (thanks again for finding it!)

MrFreezeex · 2024-11-08T12:32:48Z

/test

giorio94

Thanks for addressing this! A few minor comments inline.

Redirect policy: Seems to be handling headless services well by ignoring service without frontend ips.

I assume this may be problematic in case of services marked as headless via the label, but associated with a ClusterIP, given that they would have a frontend IP.

And policy handling doesn't seems to require the frontend IP. Letting headless services be processed should result in CNP being able to process headless services without any new handling.

I personally don't have enough context to comment on this.

pkg/k8s/service.go

pkg/k8s/watchers/service.go

pkg/k8s/watchers/service_test.go

giorio94 · 2024-11-11T17:04:01Z

Note that AFAICT the handling of a service is treated as headless if the label has service.kubernetes.io/headless is not super common outside of kube-proxy/associated conformance tests IIUC.

I agree, that looks really a corner case, which is probably not handled consistently by every project.

YutaroHayakawa

BGP looks good to me. Thanks!

Headless services may have frontend IPs if labeled with `service.kubernetes.io/headless` and those should not be taken into account in the datapath. This commit allow to handle the cleanup of such services. Signed-off-by: Arthur Outhenin-Chalandre <arthur@cri.epita.fr>

Before this commit two types of resources were exposed for services and endpoints: one excluding headless services/endpoints and one including them. This commit drop the non headless resource to make sure we only start one informer for services and endpoints and lower the load the apiserver. The consumers of the resources excluding the headless services/endpoint already handles headless services. Signed-off-by: Arthur Outhenin-Chalandre <arthur@cri.epita.fr>

thorn3r

LGTM, thanks for the quick attention 👍

giorio94

Sorry, I just noticed I had not explicitly approved it.

giorio94 · 2024-11-19T11:24:47Z

/test

MrFreezeex · 2024-11-19T13:50:54Z

/ci-e2e-upgrade

For cases when envoyConfig was enabled, we were starting separate informers for services and endpoints. This resulted in a duplicated informers, adding overhead for both cilium-agent and k8s apiserver. Before, existing informer did not handle headless services, which was changed in #35838 so now we can rely on a single informer instead. Signed-off-by: Marcel Zieba <marcel.zieba@isovalent.com>

[ upstream commit a4fecbe ] For cases when envoyConfig was enabled, we were starting separate informers for services and endpoints. This resulted in a duplicated informers, adding overhead for both cilium-agent and k8s apiserver. Before, existing informer did not handle headless services, which was changed in #35838 so now we can rely on a single informer instead. Signed-off-by: Marcel Zieba <marcel.zieba@isovalent.com> Signed-off-by: gray <greyschwinger@gmail.com>

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 Nov 7, 2024

MrFreezeex force-pushed the resources-svc-unfiltered-headless branch from e07aba6 to 5d2f4b7 Compare November 7, 2024 12:54

MrFreezeex force-pushed the resources-svc-unfiltered-headless branch from a6a7dab to 9bbcdbb Compare November 7, 2024 14:48

MrFreezeex changed the title ~~daemon: drop the non headless resource~~ daemon: drop the non headless service/endpoint resources Nov 7, 2024

MrFreezeex force-pushed the resources-svc-unfiltered-headless branch from 9bbcdbb to bed90e7 Compare November 7, 2024 15:11

MrFreezeex force-pushed the resources-svc-unfiltered-headless branch 2 times, most recently from 7fae3b2 to 84337ab Compare November 7, 2024 15:43

MrFreezeex force-pushed the resources-svc-unfiltered-headless branch from 84337ab to 43e5479 Compare November 8, 2024 10:39

MrFreezeex marked this pull request as ready for review November 8, 2024 12:09

MrFreezeex requested review from a team as code owners November 8, 2024 12:09

MrFreezeex requested a review from nathanjsweet November 8, 2024 12:09

MrFreezeex force-pushed the resources-svc-unfiltered-headless branch from 43e5479 to c27f2d6 Compare November 8, 2024 12:31

MrFreezeex requested a review from a team as a code owner November 8, 2024 12:31

MrFreezeex requested a review from thorn3r November 8, 2024 12:31

joestringer added the release-note/misc This PR makes changes that have no direct user impact. label Nov 8, 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 Nov 8, 2024

giorio94 reviewed Nov 11, 2024

View reviewed changes

pkg/k8s/service.go Outdated Show resolved Hide resolved

pkg/k8s/watchers/service.go Show resolved Hide resolved

pkg/k8s/watchers/service_test.go Outdated Show resolved Hide resolved

pkg/k8s/watchers/service_test.go Show resolved Hide resolved

MrFreezeex force-pushed the resources-svc-unfiltered-headless branch from 912ba08 to ed917f4 Compare November 11, 2024 15:16

YutaroHayakawa approved these changes Nov 12, 2024

View reviewed changes

MrFreezeex added 2 commits November 12, 2024 12:44

MrFreezeex force-pushed the resources-svc-unfiltered-headless branch from ed917f4 to 894b91a Compare November 12, 2024 11:44

christarazi approved these changes Nov 12, 2024

View reviewed changes

thorn3r approved these changes Nov 15, 2024

View reviewed changes

nathanjsweet approved these changes Nov 18, 2024

View reviewed changes

giorio94 approved these changes Nov 19, 2024

View reviewed changes

joamaki approved these changes Nov 19, 2024

View reviewed changes

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 19, 2024

tklauser added this pull request to the merge queue Nov 19, 2024

Merged via the queue into cilium:main with commit 4263231 Nov 19, 2024
64 checks passed

rastislavs mentioned this pull request Nov 20, 2024

v1.16 Backports 2024-11-20 #36066

Merged

14 tasks

rastislavs added backport-pending/1.16 The backport for Cilium 1.16.x for this PR is in progress. and removed needs-backport/1.16 This PR / issue needs backporting to the v1.16 branch labels Nov 20, 2024

github-actions bot added backport-done/1.16 The backport for Cilium 1.16.x for this PR is done. and removed backport-pending/1.16 The backport for Cilium 1.16.x for this PR is in progress. labels Nov 26, 2024

cilium-release-bot bot mentioned this pull request Dec 12, 2024

Prepare for release v1.16.5 #36557

Merged

marseel mentioned this pull request Feb 18, 2025

envoy: remove duplicated service/endpointslice informers #37683

Merged

marseel mentioned this pull request Jul 28, 2025

Scalability Regression: Agent Headless Service watch changes in 1.16+ #40763

Closed

daemon: drop the non headless service/endpoint resources #35838

daemon: drop the non headless service/endpoint resources #35838

Uh oh!

Conversation

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

Uh oh!

Uh oh!

MrFreezeex commented Nov 7, 2024

Uh oh!

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

Uh oh!

Uh oh!

MrFreezeex commented Nov 7, 2024

Uh oh!

MrFreezeex commented Nov 7, 2024

Uh oh!

MrFreezeex commented Nov 7, 2024

Uh oh!

MrFreezeex commented Nov 7, 2024

Uh oh!

MrFreezeex commented Nov 7, 2024

Uh oh!

MrFreezeex commented Nov 7, 2024

Uh oh!

MrFreezeex commented Nov 8, 2024

Uh oh!

MrFreezeex commented Nov 8, 2024

Uh oh!

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

Uh oh!

Uh oh!

MrFreezeex commented Nov 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 Nov 11, 2024

Uh oh!

YutaroHayakawa left a comment

Choose a reason for hiding this comment

Uh oh!

thorn3r left a comment

Choose a reason for hiding this comment

Uh oh!

giorio94 left a comment

Choose a reason for hiding this comment

Uh oh!

giorio94 commented Nov 19, 2024

Uh oh!

MrFreezeex commented Nov 19, 2024

Uh oh!

Uh oh!

Uh oh!

MrFreezeex commented Nov 7, 2024 •

edited

Loading

MrFreezeex commented Nov 7, 2024 •

edited

Loading

MrFreezeex commented Nov 8, 2024 •

edited

Loading