[v1.12] Backport of #26242 (don't hold EP lock while generating policy)

[bimmlerd]

• endpoint: don't hold the endpoint lock while generating policy #26242 (@squeed)

Once this PR is merged, a GitHub action will update the labels of these PRs:

 26242

Rationale

In the following, the notation SelectorCache > NameManager means that the selector cache lock is held while the NameManager lock is being acquired. When a lock is held when another is acquired, it creates an edge in the graph of lock dependencies. If there is a cycle in lock dependencies, a wait cycle (or deadlock) is possible.

In this case, the wait cycle in question is the following:

IPCache > EP > NameManager > IPCache

The IPCache > EP link stems from the following stack. Note that EP here stands for any endpoint, as they are all locked in sequence. I believe this is related to replacing the kube API server.

github.com/cilium/cilium/pkg/endpointmanager.(*EndpointManager).UpdatePolicyMaps() <-- takes _all_ EP locks
github.com/cilium/cilium/pkg/ipcache.(*IPCache).UpdatePolicyMaps() <-- t
github.com/cilium/cilium/pkg/ipcache.(*IPCache).removeLabelsFromIPs(...) <-- takes IPCache lock
github.com/cilium/cilium/pkg/ipcache.(*IPCache).RemoveLabelsExcluded(...)
github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).handleKubeAPIServerServiceEPChanges(...)
github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).addKubeAPIServerServiceEPSliceV1(...)
github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).updateK8sEndpointSliceV1(...)

The next link in the chain, EP > NameManager, occurs in the path of EP regeneration when dealing with FQDN selectors. Specifically, the SelectorCache wants to lock NameManager early (due to lock ordering constraints, cf 0a07c9b).

github.com/cilium/cilium/pkg/fqdn.(*NameManager).Lock() <--- NameManager lock
github.com/cilium/cilium/pkg/policy.(*SelectorCache).AddFQDNSelector(...)
github.com/cilium/cilium/pkg/policy.(*L4Filter).cacheFQDNSelector(...)
github.com/cilium/cilium/pkg/policy.(*L4Filter).cacheFQDNSelectors(...)
github.com/cilium/cilium/pkg/policy.createL4Filter(...)
github.com/cilium/cilium/pkg/policy.createL4EgressFilter(...)
github.com/cilium/cilium/pkg/policy.mergeEgressPortProto(...)
github.com/cilium/cilium/pkg/policy.mergeEgress.func1(...)
github.com/cilium/cilium/pkg/policy/api.PortRules.Iterate(...)
github.com/cilium/cilium/pkg/policy.mergeEgress(...)
github.com/cilium/cilium/pkg/policy.(*rule).resolveEgressPolicy(...)
github.com/cilium/cilium/pkg/policy.ruleSlice.resolveL4EgressPolicy(...)
github.com/cilium/cilium/pkg/policy.(*Repository).resolvePolicyLocked(...)
github.com/cilium/cilium/pkg/policy.(*PolicyCache).updateSelectorPolicy(...)
github.com/cilium/cilium/pkg/policy.(*PolicyCache).UpdatePolicy(...)
github.com/cilium/cilium/pkg/endpoint.(*Endpoint).regeneratePolicy(...)
github.com/cilium/cilium/pkg/endpoint.(*Endpoint).runPreCompilationSteps(...) <--- takes EP lock
github.com/cilium/cilium/pkg/endpoint.(*Endpoint).regenerateBPF(...)

Finally, NameManager > IPCache stems from the incremental policy path used by FQDN. UpdateGenerateDNS locks the NameManager for the duration of the method, and calls into IPCache for CIDR allocation.

github.com/cilium/cilium/pkg/ipcache.(*IPCache).AllocateCIDRs(...) <--- rlocks metadata, locks IPCache
github.com/cilium/cilium/pkg/ipcache.(*IPCache).AllocateCIDRsForIPs(...)
github.com/cilium/cilium/daemon/cmd.cachingIdentityAllocator.AllocateCIDRsForIPs(...)
github.com/cilium/cilium/daemon/cmd.identitiesForFQDNSelectorIPs(...)
github.com/cilium/cilium/daemon/cmd.(*Daemon).updateSelectors(...)
github.com/cilium/cilium/pkg/fqdn.(*NameManager).UpdateGenerateDNS(...) <--- locks the NameManager
github.com/cilium/cilium/daemon/cmd.(*Daemon).notifyOnDNSMsg(...)

[bimmlerd]

/test-backport-1.12

[bimmlerd]

CI triage:

• test-1.16-4.9 failed with endpoint 732 SecurityIdentity changed during policy regeneration logged at error severity. This, AFAICT, is now expected to happen and shouldn't fail the test. Rerunning, might look into fixing this upstream.
• test-1.24-net-next failed in K8sServicesTest Checks N/S loadbalancing Tests with XDP, direct routing, Hybrid and Random with a curl TFTP request failing. The service state of cilium looks as expected, and there is no policy drop of the flow in the hubble logs. I'm chalking this up to kubectl exec 'curl' failing, and will rerun.

[bimmlerd]

/test-1.24-net-next

[bimmlerd]

/test-1.16-4.9

[squeed]

I recall seeing the error message SecurityIdentity changed during policy regeneration during development of this PR, and I did something to prevent it from happening. It is escaping me, however, what exactly the fix was.

[squeed]

I did a lot of debugging, and I determined that the error message popped up because the endpoint in question (actually the local node's endpoint) changed labels precisely during regeneration. Bad luck. I believe it is safe to ignore this error message in the tests.