daemon: Initialize k8sCachesSynced channel before calling Initk8sSubsystem()

[jrajahalme]

InitK8sSubsystem() starts all k8s watchers concurrently, some of which do
call into K8sCacheIsSynced() via ipcache/metadata.InjectLabels(), and
possibly also from elsewhere. Initialize k8sCachesSynced before any
watchers are started to make this access safe. This fixes data race
detected by race detection builds.

Fixes: #19614
Fixes: #19556
Signed-off-by: Jarno Rajahalme jarno@isovalent.com

[jrajahalme]

While backporting this does not seem necessary at this point, it is possible that later backports will introduce the fixed data race to release branches. Adding backport labels to avoid introducing the fixed data race in any future backports.

[jrajahalme]

/test

[jrajahalme]

/test-race

[joestringer]

Thanks! I think that this is most important for v1.11 since it's mostly the ipcache that depends on this, but the watcher -> ipcache logic was only introduced in that version. I'm not sure if we have data race pipelines for the older releases to confirm though 🤔

[aditighag]

This looks like a regression? Please add the label, and Fixes: tag pointing to the offending commit.

[joestringer]

I think it's a little bit hard to say; this code was always like this (in terms of starting to run the k8s watchers before initializing the "k8s synced" channel). Until v1.11, Cilium didn't have any logic that caused watchers to trigger logic that waits for k8s to be synced. This was introduced as part of commit 2b17d4d ("ipcache, policy: Inject labels from identity metadata"). However, at that time, the ipcache also waited for the local identity allocator to be initialized, and I believe that some other logic was ensuring that the k8s synced channel was initialized before the local identity allocator was initialized. This meant that the race condition did not occur. Only recently in commit 2e5f35b ("identity: Initialize local identity allocator early") this additional constraint was dropped, revealing the underlying initialization race. So I don't think we have any releases with this regression in it, it's only on master, but PR #19556 introduced the race condition and it will be backported. Hence we should also backport this PR to match.

[joestringer]

k8s-1.23-kernel-net-next-race hit a provisioning error, re-running.

[joestringer]

/test-net-next-race

Job 'Cilium-PR-K8s-GKE' failed:

Click to show.

Test Name

K8sDatapathConfig Host firewall With native routing and endpoint routes

Failure Output

FAIL: Failed to reach 10.128.0.19:80 from testclient-mrvw2

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

[jrajahalme]

@aditighag Figured out with Joe that #19556 revealed this data race, so added Fixes 19556.

[christarazi]

LGTM

[jrajahalme]

Updated commit message title to be shorter to keep bpf checks happy.

[jrajahalme]

/test-race

[jrajahalme]

test-1.21-5.4 hit known flake #16928

[jrajahalme]

/test-1.21

[jrajahalme]

/test-1.23-net-next

[jrajahalme]

/test-gke

[jrajahalme]

/test-1.21-5.4

[jrajahalme]

Some unrelated flakes, all race tests passed.

[jrajahalme]

@joestringer Thanks for the merge, and feel free to drop the backport labels for 1.10 and 1.9 at will. Those backports would only be defensive against possible later backports hitting this issue.

[joestringer]

v1.9 will become EOL soon when we release v1.12, so I'll drop there. We can do best-effort for v1.10 backport, I think the risk is low but it could end up helping us avoid digging through this kind of thing in future. If it turns out that the v1.10 backport doesn't cleanly work, then we can drop that one too.

[aditighag]

v1.9 will become EOL soon when we release v1.12, so I'll drop there. We can do best-effort for v1.10 backport, I think the risk is low but it could end up helping us avoid digging through this kind of thing in future. If it turns out that the v1.10 backport doesn't cleanly work, then we can drop that one too.

@jrajahalme @joestringer I tried resolving the conflicts, but there are other changes in this code path that differ from upstream/master so I dropped the backport on v1.10. Resetting the labels accordingly.

[joestringer]

@aditighag OK, fair enough. I'm not too worried about this for v1.10. I'll just drop the backport label.