k8s: Fix data race between ServiceCache and K8sWatcher #25087

joamaki · 2023-04-24T13:44:16Z

correlateEndpoints was modifying a Backend object that was read by
K8sWatcher:

WARNING: DATA RACE
Read at 0x00c000942278 by goroutine 71:
github.com/cilium/cilium/pkg/k8s/watchers.genCartesianProduct()
    /home/chris/code/cilium/cilium/pkg/k8s/watchers/watcher.go:787 +0xb56
github.com/cilium/cilium/pkg/k8s/watchers.datapathSVCs()
    /home/chris/code/cilium/cilium/pkg/k8s/watchers/watcher.go:852 +0x77e
github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).addK8sSVCs()
...
Previous write at 0x00c000942278 by goroutine 70:
github.com/cilium/cilium/pkg/k8s.(*ServiceCache).correlateEndpoints()
    /home/chris/code/cilium/cilium/pkg/k8s/service_cache.go:501 +0x409
github.com/cilium/cilium/pkg/k8s.(*ServiceCache).UpdateService()
    /home/chris/code/cilium/cilium/pkg/k8s/service_cache.go:214 +0x3e4
github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcherSuite).TestChangeSVCPort()
    /home/chris/code/cilium/cilium/pkg/k8s/watchers/watcher_test.go:685 +0x1d09
...

Since the backends are owned by the ServiceCache and allowing to mutate them when holding a lock is something one might expect, fix the issues by making all public ServiceCache methods return a DeepCopy'd *Endpoint and *Backends.

Fix data race affecting the preferred mark in backends, e.g. backends selected by service with affinity set to local. In very rare cases a backend might be missing its preferred status and a non-local backend might be selected.

squeed · 2023-04-24T14:51:42Z

wait, are these objects from the informer cache? what is writing to them?

christarazi · 2023-04-24T18:18:32Z

While the release note is correct in describing the impact of the change, it doesn't describe what behavior / impact it is improving for users. As of now, this release note sounds quite scary for any user, so either we identify what specific symptoms this bug introduces (if we know), or we downgrade the release note to misc.

christarazi · 2023-04-24T18:18:47Z

pkg/k8s/watchers/cilium_envoy_config_test.go

+	//var buf bytes.Buffer
+	//json.Indent(&buf, jsonBytes, "", "\t")
+	//fmt.Printf("JSON spec:\n%s\n", buf.String())


Why not just remove the code?

These 3 lines are in many many places. @jrajahalme Can we clean these up?

I'd guess these are meant for debugging, so let's just remove them and file an issue to clean up the rest as a followup.

Ended up just removing them from everywhere.

christarazi · 2023-04-24T18:19:56Z

Given that #19521 introduced this bug, we'll need to backport to v1.12 and v1.13.

joamaki · 2023-04-25T07:05:15Z

wait, are these objects from the informer cache? what is writing to them?

They're derived from them, but the cache objects are not mutated. What is mutating is the *k8s.Endpoints, or more specifically *loadbalancer.Backend inside *k8s.Endpoints. This is created and owned by ServiceCache which hands it over to K8sWatcher over the events channel. Bug is that the same*Backend is mutated that is handed over to K8sWatcher.

joamaki · 2023-04-25T07:07:01Z

While the release note is correct in describing the impact of the change, it doesn't describe what behavior / impact it is improving for users. As of now, this release note sounds quite scary for any user, so either we identify what specific symptoms this bug introduces (if we know), or we downgrade the release note to misc.

Expanded the comment to mention the impact. Are we ok with this? Could also consider downgrading as the impact is minor.

christarazi · 2023-04-25T16:47:09Z

The release note sounds better now. I don't think we've had any bug reports about backend preferred selection, but then again it's probably hard to pinpoint from a user's perspective that something went wrong there. I think we ship it given we've done our due diligence for as far as we know.

christarazi · 2023-04-25T16:47:37Z

Rebasing to run CI

christarazi · 2023-04-25T16:48:05Z

/test

joamaki · 2023-04-26T12:57:50Z

pkg/k8s/service_cache.go

@@ -531,7 +531,7 @@ func (s *ServiceCache) correlateEndpoints(id ServiceID) (*Endpoints, bool) {

 	// Report the service as ready if a local endpoints object exists or if
 	// external endpoints have been identified
-	return endpoints, hasLocalEndpoints || hasExternalEndpoints
+	return endpoints.DeepCopy(), hasLocalEndpoints || hasExternalEndpoints


Discussed with @squeed that it'd be cleaner to DeepCopy the backend that's mutated rather than the whole thing here.

Ended up flipping this. Now the backends are mutated in-place (with lock held), but all public methods in ServiceCache will always return a copy of the Backends.

joamaki · 2023-05-02T13:35:09Z

/test

Job 'Cilium-PR-K8s-1.24-kernel-5.4' failed:

Click to show.

Test Name

K8sDatapathConfig Host firewall With native routing and endpoint routes

Failure Output

FAIL: Error deleting resource /home/jenkins/workspace/Cilium-PR-K8s-1.24-kernel-5.4/src/github.com/cilium/cilium/test/k8s/manifests/host-policies.yaml: Cannot retrieve "cilium-jh7j5"'s policy revision: cannot get policy revision: ""

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.24-kernel-5.4/1934/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.24-kernel-5.4 so I can create one.

Then please upload the Jenkins artifacts to that issue.

Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed:

Click to show.

Test Name

K8sDatapathCustomCalls Basic test with byte-counter Loads byte-counter and gets consistent values

Failure Output

FAIL: failed to ensure the namespace kube-system exists: signal: killed

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/2028/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.26-kernel-net-next so I can create one.

Then please upload the Jenkins artifacts to that issue.

christarazi · 2023-05-02T19:29:27Z

pkg/k8s/watchers/cilium_envoy_config_test.go

+	//var buf bytes.Buffer
+	//json.Indent(&buf, jsonBytes, "", "\t")
+	//fmt.Printf("JSON spec:\n%s\n", buf.String())


I'd guess these are meant for debugging, so let's just remove them and file an issue to clean up the rest as a followup.

pkg/k8s/service_cache.go

Remove leftover debugging code in envoy-related tests. Signed-off-by: Jussi Maki <jussi@isovalent.com>

correlateEndpoints was modifying a Backend object that was read by K8sWatcher: WARNING: DATA RACE Read at 0x00c000942278 by goroutine 71: github.com/cilium/cilium/pkg/k8s/watchers.genCartesianProduct() /home/chris/code/cilium/cilium/pkg/k8s/watchers/watcher.go:787 +0xb56 github.com/cilium/cilium/pkg/k8s/watchers.datapathSVCs() /home/chris/code/cilium/cilium/pkg/k8s/watchers/watcher.go:852 +0x77e github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).addK8sSVCs() ... Previous write at 0x00c000942278 by goroutine 70: github.com/cilium/cilium/pkg/k8s.(*ServiceCache).correlateEndpoints() /home/chris/code/cilium/cilium/pkg/k8s/service_cache.go:501 +0x409 github.com/cilium/cilium/pkg/k8s.(*ServiceCache).UpdateService() /home/chris/code/cilium/cilium/pkg/k8s/service_cache.go:214 +0x3e4 github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcherSuite).TestChangeSVCPort() /home/chris/code/cilium/cilium/pkg/k8s/watchers/watcher_test.go:685 +0x1d09 ... Since the backends are owned by the ServiceCache and allowing to mutate them when holding a lock is something one might expect, fix the issues by making all public ServiceCache methods return a DeepCopy'd *Endpoint and *Backends. Fixes: cilium#25071 Fixes: aa3c85f ("k8s: Add preferred attribute in Endpoints") Signed-off-by: Jussi Maki <jussi@isovalent.com>

joamaki · 2023-05-05T08:42:46Z

/test

Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed:

Click to show.

Test Name

K8sDatapathServicesTest Checks N/S loadbalancing With host policy Tests NodePort

Failure Output

FAIL: Can not connect to service "tftp://[fd04::12]:31608/hello" from outside cluster (1/10)

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/2090/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.26-kernel-net-next so I can create one.

Then please upload the Jenkins artifacts to that issue.

joamaki · 2023-05-05T11:59:26Z

/test-1.26-net-next

nathanjsweet

I am worried about memory usage, but this all looks good.

youngnick · 2023-05-08T10:38:50Z

Looks like Chart CI Push hit #25274, merging.

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 Apr 24, 2023

joamaki added release-note/bug This PR fixes an issue in a previous release of Cilium. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Apr 24, 2023

joamaki marked this pull request as ready for review April 24, 2023 13:47

joamaki requested a review from a team as a code owner April 24, 2023 13:47

joamaki requested a review from squeed April 24, 2023 13:47

christarazi added sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. kind/bug This is a bug in the Cilium logic. labels Apr 24, 2023

christarazi reviewed Apr 24, 2023

View reviewed changes

christarazi added needs-backport/1.12 needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Apr 24, 2023

maintainer-s-little-helper bot added this to Needs backport from main in 1.13.3 Apr 24, 2023

maintainer-s-little-helper bot added this to Needs backport from main in 1.12.10 Apr 24, 2023

christarazi force-pushed the pr/joamaki/fix-service-cache-endpoints-race branch from 2712e52 to b784ff8 Compare April 25, 2023 16:47

joamaki self-assigned this Apr 26, 2023

joamaki commented Apr 26, 2023

View reviewed changes

joamaki force-pushed the pr/joamaki/fix-service-cache-endpoints-race branch from b784ff8 to 1905319 Compare May 2, 2023 08:19

christarazi approved these changes May 2, 2023

View reviewed changes

squeed reviewed May 2, 2023

View reviewed changes

pkg/k8s/service_cache.go Show resolved Hide resolved

squeed approved these changes May 3, 2023

View reviewed changes

joamaki requested a review from a team as a code owner May 5, 2023 07:29

joamaki requested review from nathanjsweet and jrajahalme May 5, 2023 07:29

joamaki added 2 commits May 5, 2023 10:30

envoy: Remove leftover JSON spec dumping code

5eb0ed3

Remove leftover debugging code in envoy-related tests. Signed-off-by: Jussi Maki <jussi@isovalent.com>

joamaki force-pushed the pr/joamaki/fix-service-cache-endpoints-race branch from 6f1e356 to 4cf827b Compare May 5, 2023 07:30

nathanjsweet approved these changes May 5, 2023

View reviewed changes

joamaki added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 8, 2023

youngnick merged commit bc3028e into cilium:main May 8, 2023
56 of 57 checks passed

YutaroHayakawa mentioned this pull request May 10, 2023

v1.13 Backports 2023-05-10 #25346

Merged

14 tasks

YutaroHayakawa added backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. and removed needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels May 10, 2023

maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.13 in 1.13.3 May 10, 2023

maintainer-s-little-helper bot removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 10, 2023

YutaroHayakawa mentioned this pull request May 10, 2023

v1.12 Backports 2023-05-10 #25348

Merged

7 tasks

YutaroHayakawa added backport-pending/1.12 and removed needs-backport/1.12 labels May 10, 2023

maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.12 in 1.12.10 May 10, 2023

YutaroHayakawa added backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. and removed backport-pending/1.12 labels May 15, 2023

maintainer-s-little-helper bot moved this from Backport pending to v1.12 to Backport done to v1.12 in 1.12.10 May 15, 2023

This was referenced May 16, 2023

Prepare for release v1.12.10 #25490

Merged

Prepare for release v1.13.3 #25514

Merged

julianwiedmann added backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. labels Jul 31, 2023

maintainer-s-little-helper bot moved this from Backport pending to v1.13 to Backport done to v1.13 in 1.13.3 Jul 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

k8s: Fix data race between ServiceCache and K8sWatcher #25087

k8s: Fix data race between ServiceCache and K8sWatcher #25087

joamaki commented Apr 24, 2023 •

edited

squeed commented Apr 24, 2023

christarazi commented Apr 24, 2023

christarazi Apr 24, 2023

joamaki Apr 25, 2023

christarazi May 2, 2023

joamaki May 5, 2023

christarazi commented Apr 24, 2023

joamaki commented Apr 25, 2023

joamaki commented Apr 25, 2023

christarazi commented Apr 25, 2023

christarazi commented Apr 25, 2023

christarazi commented Apr 25, 2023

joamaki Apr 26, 2023

joamaki May 2, 2023

joamaki commented May 2, 2023 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

Test Name

Failure Output

christarazi May 2, 2023

joamaki commented May 5, 2023 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

joamaki commented May 5, 2023

nathanjsweet left a comment

youngnick commented May 8, 2023

k8s: Fix data race between ServiceCache and K8sWatcher #25087

k8s: Fix data race between ServiceCache and K8sWatcher #25087

Conversation

joamaki commented Apr 24, 2023 • edited

squeed commented Apr 24, 2023

christarazi commented Apr 24, 2023

christarazi Apr 24, 2023

Choose a reason for hiding this comment

joamaki Apr 25, 2023

Choose a reason for hiding this comment

christarazi May 2, 2023

Choose a reason for hiding this comment

joamaki May 5, 2023

Choose a reason for hiding this comment

christarazi commented Apr 24, 2023

joamaki commented Apr 25, 2023

joamaki commented Apr 25, 2023

christarazi commented Apr 25, 2023

christarazi commented Apr 25, 2023

christarazi commented Apr 25, 2023

joamaki Apr 26, 2023

Choose a reason for hiding this comment

joamaki May 2, 2023

Choose a reason for hiding this comment

joamaki commented May 2, 2023 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

Test Name

Failure Output

christarazi May 2, 2023

Choose a reason for hiding this comment

joamaki commented May 5, 2023 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

joamaki commented May 5, 2023

nathanjsweet left a comment

Choose a reason for hiding this comment

youngnick commented May 8, 2023

joamaki commented Apr 24, 2023 •

edited

joamaki commented May 2, 2023 •

edited by maintainer-s-little-helper bot

joamaki commented May 5, 2023 •

edited by maintainer-s-little-helper bot