Optimize scalability of CiliumIdentity operation by tgraf · Pull Request #11275 · cilium/cilium

tgraf · 2020-04-30T23:28:25Z

This PR changes the garbage collection mechanism of identities to use the CiliumEndpoint information instead of requiring all nodes to maintain status information about who is using a particular identity.

This improves scalability of identity management in CRD mode massively.

Depends on: #11270

…dpoint Rework the CiliumIdentity garbage collector to derive liveness of a CiliumIdentity based on the identity usage of CiliumEndpoint information. As the CiliumEndpoint garbage collector scans endpoints, it will maintain a list of identities with heartbeat signals. As the CiliumIdentity garbage collector runs, it will use the heartbeat signals before removing stale identities. In order to make the gabrage collector with the heartbeat timeout requirements work, the default scan intervals are reduced slightly. Signed-off-by: Thomas Graf <thomas@cilium.io>

With the introduction of the CiliumEndpoint based garbage collection of CiliumIdentities. The status field of the CiliumIdentity can remain unpopulated. This will massively increase the scalability of identity allocation as no PATCHing is required after the identity has been allocated. Signed-off-by: Thomas Graf <thomas@cilium.io>

tgraf · 2020-04-30T23:41:34Z

test-me-please

Signed-off-by: Thomas Graf <thomas@cilium.io>

coveralls · 2020-05-01T06:46:11Z

Coverage increased (+0.1%) to 44.699% when pulling 30ec2e5 on pr/tgraf/optimize-identity-gc into ba3a74b on master.

tgraf · 2020-05-01T06:58:25Z

test-me-please

Looks like the CI is subject to some underlying network issues:

/home/jenkins/workspace/Cilium-PR-Ginkgo-Tests-Validated/runtime-gopath/src/github.com/cilium/cilium/test/ginkgo-ext/scopes.go:431
5 of 1  pings to "8.8.8.8" failed
Expected
    <int>: 5
to be <=
    <int>: 1

tgraf · 2020-05-01T09:50:15Z

test-me-please

aanm · 2020-05-01T12:59:23Z

operator/identity/heartbeat.go

+	defer i.mutex.Unlock()
+
+	for identity, lifesign := range i.lastLifesign {
+		if time.Since(lifesign) > 10*i.timeout {


Why the 10*? can't this live live somewhere else?

10 is a safe assumption for when a life sign is definitely never needed again.

aanm · 2020-05-01T13:02:29Z

operator/k8s_cep_gc.go

@@ -70,6 +71,7 @@ func enableCiliumEndpointSyncGC() {
 					podsCache[pod.Namespace+"/"+pod.Name] = &pod


It worries me a little bit that we are doing an entire Pod List of the cluster every N minutes. This function should be re-written once #11195 is merged.

Is your worry that we do this every 10min instead of 30min now? We have always been doing this so far. This only changes the interval.

We can definitely optimize this. I'm not sure a watcher is always better though. it will pay the cost of churn. Whereas right now we pay the snapshot cost of all pods which can be a lot less or a lot more. It will vary.

I wasn't aware about this listing at all, that's why I made the comment. The watcher will be good because we can optimize it and only keep the pod name and namespace in memory (instead of all fields of the pod). Can be optimized as a follow up.

aanm · 2020-05-01T13:03:49Z

operator/k8s_cep_gc.go

 					// For each CEP we fetched, check if we know about it
 					for _, cep := range ceps.Items {
+						if cep.Status.Identity != nil {
+							identityHeartbeat.MarkAlive(strconv.FormatInt(cep.Status.Identity.ID, 10), timeNow)


Can't this logic be in the CEP watcher? With a reference counter we would be able to know which CEPs are using a particular identity.

Which CEP watcher?

Apparently a non-existing one (I thought we had one in the operator). I was thinking on having a CEP watcher in the operator that would keep a reference counter for the used identities.

The watcher has a downside though. If we ever miss the notification, we won't ever clean up again unless the identity gets re-used and released again. We are also no longer talking about a heartbeat-based mechanism but event notification with the usual pros and cons.

aanm · 2020-05-01T16:48:23Z

operator/k8s_cep_gc.go

@@ -70,6 +71,7 @@ func enableCiliumEndpointSyncGC() {
 					podsCache[pod.Namespace+"/"+pod.Name] = &pod


I wasn't aware about this listing at all, that's why I made the comment. The watcher will be good because we can optimize it and only keep the pod name and namespace in memory (instead of all fields of the pod). Can be optimized as a follow up.

aanm · 2020-05-01T16:51:01Z

operator/k8s_cep_gc.go

 					// For each CEP we fetched, check if we know about it
 					for _, cep := range ceps.Items {
+						if cep.Status.Identity != nil {
+							identityHeartbeat.MarkAlive(strconv.FormatInt(cep.Status.Identity.ID, 10), timeNow)


Apparently a non-existing one (I thought we had one in the operator). I was thinking on having a CEP watcher in the operator that would keep a reference counter for the used identities.

Following the deprecation notice from cilium#11275, remove the field. Signed-off-by: Chris Tarazi <chris@isovalent.com>

Following the deprecation notice from #11275, remove the field. Signed-off-by: Chris Tarazi <chris@isovalent.com>

tgraf added 2 commits May 1, 2020 01:26

tgraf added pending-review kind/performance There is a performance impact of this. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Apr 30, 2020

tgraf requested review from a team as code owners April 30, 2020 23:28

tgraf requested a review from a team April 30, 2020 23:28

tgraf force-pushed the pr/tgraf/optimize-identity-gc branch from 46948c0 to 24abe2f Compare April 30, 2020 23:38

test: Add CiliumIdentity expiry validation

30ec2e5

Signed-off-by: Thomas Graf <thomas@cilium.io>

tgraf force-pushed the pr/tgraf/optimize-identity-gc branch from 24abe2f to 30ec2e5 Compare May 1, 2020 06:19

tgraf requested a review from aanm May 1, 2020 11:36

aanm requested changes May 1, 2020

View reviewed changes

tgraf marked this pull request as draft May 4, 2020 10:26

tgraf removed the pending-review label May 4, 2020

tgraf marked this pull request as ready for review May 6, 2020 14:41

aanm approved these changes May 6, 2020

View reviewed changes

aanm merged commit c0e278a into master May 6, 2020

aanm deleted the pr/tgraf/optimize-identity-gc branch May 6, 2020 14:48

christarazi added a commit to christarazi/cilium that referenced this pull request Sep 2, 2020

k8s: Remove deprecated identity status field

5b12c89

Following the deprecation notice from cilium#11275, remove the field. Signed-off-by: Chris Tarazi <chris@isovalent.com>

christarazi mentioned this pull request Sep 2, 2020

k8s: Remove deprecated identity status field #13053

Merged

christarazi added a commit that referenced this pull request Sep 4, 2020

dropme: k8s: Remove deprecated identity status field

9f4bac9

Following the deprecation notice from #11275, remove the field. Signed-off-by: Chris Tarazi <chris@isovalent.com>

christarazi added a commit that referenced this pull request Sep 7, 2020

dropme: k8s: Remove deprecated identity status field

cccf294

Following the deprecation notice from #11275, remove the field. Signed-off-by: Chris Tarazi <chris@isovalent.com>

nebril pushed a commit that referenced this pull request Sep 7, 2020

k8s: Remove deprecated identity status field

cdeb302

Following the deprecation notice from #11275, remove the field. Signed-off-by: Chris Tarazi <chris@isovalent.com>

mhofstetter mentioned this pull request Mar 22, 2023

k8s api: remove status documentation from CRD CiliumIdentity #24512

Merged

		@@ -70,6 +71,7 @@ func enableCiliumEndpointSyncGC() {
		podsCache[pod.Namespace+"/"+pod.Name] = &pod

Conversation

tgraf commented Apr 30, 2020

Uh oh!

tgraf commented Apr 30, 2020

Uh oh!

coveralls commented May 1, 2020

Uh oh!

tgraf commented May 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgraf commented May 1, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aanm May 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aanm May 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tgraf commented May 1, 2020 •

edited

Loading

aanm May 1, 2020 •

edited

Loading

aanm May 1, 2020 •

edited

Loading