Identity based batching race condition Issues #17520

krishgobinath · 2021-10-04T06:26:52Z

Fix race condition issues in Identity based batching

Signed-off-by: Gobinath Krishnamoorthy gobinathk@google.com

aanm · 2021-10-04T10:48:16Z

operator/pkg/ciliumendpointbatch/manager.go

+		// If CEP identity is changed, remove the old CEP from cache and insert
+		// the CEP in a new CEB
+		if c.cebToIdentity[cebName] != cep.IdentityID {
+			c.RemoveCepFromCache(GetCepNameFromCCEP(cep))
+		} else {
+			// add a cep into the ceb
+			c.addCEPtoCEB(cep, cebName)
+			return cebName
+		}


The comment does not match what the code should be doing.

Updated comment.

Weil0ng · 2021-10-04T17:43:02Z

operator/pkg/ciliumendpointbatch/manager.go

+		// If CEP identity is changed, remove the old CEP from cache and insert
+		// the CEP in a new CEB
+		if c.cebToIdentity[cebName] != cep.IdentityID {
+			c.RemoveCepFromCache(GetCepNameFromCCEP(cep))


Don't we also need to remove the CEP from the actual CEB (not just from the cache)?

Updated the comment. RemoveCepFromCache will remove the CEP from local CEB cache and will trigger an UPDATE to k8s-apiserver to sync local CEB with k8s-apiserver.

Where in the code will the CEB be created? What I meant with my previous review was that the else on line 532 seems to be a mistake.

Where in the code will the CEB be created?
Here is what we find or create a CEB for this CEP,
https://github.com/krishgobinath/cilium/blob/ceb-race/operator/pkg/ciliumendpointbatch/manager.go#L539-L564

I'm not following you, what issue you see it on line #532
If the given CEP and CEP in local cache are same, then we should update the local cache, to have the latest value.

If the given CEP and CEP in local cache are same, then we should update the local cache, to have the latest value.

oh I thought this function ended on line 538 I missed it was being added on line 570. Great!

// Queue the CEP in CEB c.addCEPtoCEB(cep, cb.ceb.GetName())

aanm · 2021-10-06T08:44:53Z

operator/pkg/ciliumendpointbatch/manager.go

+		// If CEP identity is changed, remove the old CEP from cache and insert
+		// the CEP in a new CEB
+		if c.cebToIdentity[cebName] != cep.IdentityID {
+			c.RemoveCepFromCache(GetCepNameFromCCEP(cep))


Where in the code will the CEB be created? What I meant with my previous review was that the else on line 532 seems to be a mistake.

aanm · 2021-10-06T08:54:03Z

operator/pkg/ciliumendpointbatch/manager.go

 	// check the given cep is already exists in any of the CEB.
-	// if yes, Update a ceb with the given cep object.
+	// if yes, compare the given CEP Identity with the CEPs stored in CEB.
+	// If they are same UPDATE the CEP in the CEB. This will trigger CEB UPDATE to k8s-apiserver.
+	// If the Identities differ, remove the CEP from the existing CEB
+	// and find a new CEB to batch the given CEP in a CEB. This will trigger following actions,
+	// 1) CEB UPDATE to k8s-apiserver, removing CEP in old CEB
+	// 2) CEB CREATE to k8s-apiserver, inserting the given CEP in a new CEB or
+	// 3) CEB UPDATE to k8s-apiserver, inserting the given CEP in existing CEB


This comment worries me a little bit. It says the CEP is removed from a CEB and added in a new CEB, won't this means that the cilium-agents receiving the CEB events will receive a "delete" followed by a "add/update" for that CEP? This results in the cilium-agent from removing the IP address of that CEP from the ipcache and then re-adding it back which can result in packet drops in the source node.

Unfortunately for Identity based batching mode, if the Identity for the CEP is changed for whatever the reason. This would trigger couple of CEB updates as you described. On the source node we don't delete anything on Ipcache, because source of truth for the cilium-agent is source node. Check here.
https://github.com/krishgobinath/cilium/blob/ceb-race/pkg/k8s/watchers/cilium_endpoint_batch_subscriber.go#L143-L148

That's true for ipcache entries that belong to the local agent but what about for ipcache entries that were populated from information of other nodes?

On remote nodes, yes we may see packet drop issues for a short period of time, eventually that should resolve. That is something we can't avoid here. I think, We should document this behavior in our docs.
@Weil0ng any thoughts here ?

Synced with @krishgobinath offline, I think there are two racing conditions:

Issue 1: similar to #16565. Essentially if the remove from old CEB event arrives after the reinsert into new CEB, we might end up deleting the ip from ipcache, this can be solved in the same way as #17161, aka checking the "owner" CEB at time of deletion. @krishgobinath will add a new commit in this PR to address this racing issue.

Issue 2: this I believe is what's being discussed above. So essentially with batching, a CEP update event is broken into a delete (triggered by removal from old CEB) and an add (trigger by insertion into new CEB) as @aanm initially pointed out. Since we cannot enforce strong guarantee of ordering for different objects (2 different CEBs), I don't think this is easily solvable. But maybe @aanm have some good ideas?
OTOH, since this only happens when you "change"/"add"/"remove" a label of some running resources, I'd say it's relatively rare and I'm okay with documenting it as a known issue.

It is possible on the source node, but there's no guarantee that ADDs will arrive before DELETEs on other nodes, so I'm not sure how much this (sending ADDs before DELETEs) would help?

@Weil0ng couldn't we guarantee it if we would only send the DELETE from the operator after receiving the ADD from the operator?

I think that's a good mitigation method but still, in theory, that only guarantees DELETE will arrive after ADD for the operator node, for other node, it still could be possible that DELETE arrives before ADD but I think that should be very unlikely. On the other hand, I'm not sure how complicated would it be in the code to have such a dependency check between CEP and CEB events? @krishgobinath , if it would greatly complicates the code then maybe just add a default delay in sending DELETE could achieve similar effects.

I'm all for having a mitigation method in place but I still feel we should document the possibility of connection disruption.

@Weil0ng sounds good to me. One other mitigation that just occurred me would be to postpone deletes in the ipcache for 1 minute? This could also allow follow up ADDs to be received by cilium-agent. If ADDs would be received the deletes would be canceled. Might be a little bit too hacky.

Documenting this behavior sounds good, since this will be a beta feature I'm up for documenting this behavior and we can think on a better approach when we switch the feature to GA, what do you think?

sgtm :) I'll document this caveat in #17430.

aanm · 2021-10-11T22:53:42Z

operator/pkg/ciliumendpointbatch/manager.go

+		// If CEP identity is changed, remove the old CEP from cache and insert
+		// the CEP in a new CEB
+		if c.cebToIdentity[cebName] != cep.IdentityID {
+			c.RemoveCepFromCache(GetCepNameFromCCEP(cep))


If the given CEP and CEP in local cache are same, then we should update the local cache, to have the latest value.

oh I thought this function ended on line 538 I missed it was being added on line 570. Great!

// Queue the CEP in CEB c.addCEPtoCEB(cep, cb.ceb.GetName())

aanm · 2021-10-11T23:01:39Z

operator/pkg/ciliumendpointbatch/manager.go

 	// check the given cep is already exists in any of the CEB.
-	// if yes, Update a ceb with the given cep object.
+	// if yes, compare the given CEP Identity with the CEPs stored in CEB.
+	// If they are same UPDATE the CEP in the CEB. This will trigger CEB UPDATE to k8s-apiserver.
+	// If the Identities differ, remove the CEP from the existing CEB
+	// and find a new CEB to batch the given CEP in a CEB. This will trigger following actions,
+	// 1) CEB UPDATE to k8s-apiserver, removing CEP in old CEB
+	// 2) CEB CREATE to k8s-apiserver, inserting the given CEP in a new CEB or
+	// 3) CEB UPDATE to k8s-apiserver, inserting the given CEP in existing CEB


That's true for ipcache entries that belong to the local agent but what about for ipcache entries that were populated from information of other nodes?

Weil0ng · 2021-10-19T20:16:57Z

pkg/k8s/watchers/cilium_endpoint_batch_subscriber.go

 	}
 }
+
+// cepToCEBmap is used to map CiliumEndpoint name to CiliumEndpointBatch name.


I'm confused by this piece, isn't this the same as the one you already introduced in https://github.com/cilium/cilium/pull/17543/files#diff-72fe1386c41a6b05ace9b6247fb3ab9d28b2e37cd7e10f6dfce40a226f115a64?

Those helper function aren't exported to use outside the package. It's better off with separate helper functions.

aanm · 2021-10-20T23:19:02Z

operator/pkg/ciliumendpointbatch/endpointbatch.go

@@ -56,6 +56,11 @@ const (
 	CESControllerWorkQueueQPSLimit = 10
 	// default burst limit value for workqueues.
 	CESControllerWorkQueueBurstLimit = 100
+	// Delayed CES Synctime, CES's are synced with k8s-apiserver after certain delay
+	// Some CES's are delayed to sync with k8s-apiserver.
+	DelayedCESSyncTime = 15 * time.Second


It would be cleaner if this variable name DelayedCESSyncTime would be changed to DelayedCESDeleteSyncTime

Updated, Done.

Fix race condition issues in Identity based batching At runtime if CEP Identity is changed, based on batching mode, in case of Identity batching mode. we may remove the CEP from CEB, re-insert the CEP in new CEB. In this case, change in CEP Identity translates into 1) Remove the CEP from a CEB 2) Insert the CEP in a new CEB Signed-off-by: Gobinath Krishnamoorthy <gobinathk@google.com>

krishgobinath requested a review from a team as a code owner October 4, 2021 06:26

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 Oct 4, 2021

krishgobinath requested review from errordeveloper and removed request for a team October 4, 2021 06:26

maintainer-s-little-helper bot assigned errordeveloper Oct 4, 2021

aanm requested changes Oct 4, 2021

View reviewed changes

Weil0ng self-requested a review October 4, 2021 17:23

maintainer-s-little-helper bot assigned Weil0ng Oct 4, 2021

Weil0ng reviewed Oct 4, 2021

View reviewed changes

krishgobinath force-pushed the ceb-race branch from c4f6cbd to ea71ca7 Compare October 5, 2021 04:56

krishgobinath requested review from Weil0ng and aanm October 5, 2021 04:59

maintainer-s-little-helper bot assigned aanm Oct 5, 2021

Weil0ng approved these changes Oct 6, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned Weil0ng Oct 6, 2021

aanm requested changes Oct 6, 2021

View reviewed changes

krishgobinath requested a review from aanm October 6, 2021 16:10

aanm requested changes Oct 11, 2021

View reviewed changes

krishgobinath force-pushed the ceb-race branch 2 times, most recently from a8b1bb4 to f0843ef Compare October 14, 2021 17:41

krishgobinath requested a review from a team October 14, 2021 17:41

krishgobinath force-pushed the ceb-race branch from f0843ef to 6dca307 Compare October 17, 2021 19:38

Weil0ng mentioned this pull request Oct 18, 2021

Adds concept documentation for CiliumEndpointSlice #17430

Merged

4 tasks

krishgobinath force-pushed the ceb-race branch from 6dca307 to 367cdb8 Compare October 19, 2021 18:24

Weil0ng reviewed Oct 19, 2021

View reviewed changes

krishgobinath force-pushed the ceb-race branch from 367cdb8 to 4aca46f Compare October 19, 2021 20:32

krishgobinath requested a review from a team October 19, 2021 20:32

krishgobinath requested review from a team as code owners October 19, 2021 20:32

krishgobinath requested a review from a team as a code owner October 19, 2021 20:32

krishgobinath force-pushed the ceb-race branch 4 times, most recently from 994418d to a71da8f Compare October 19, 2021 21:28

Weil0ng mentioned this pull request Oct 19, 2021

Implementation of CiliumEndpointBatch (CFP-2021-08) #16931

Closed

15 tasks

krishgobinath force-pushed the ceb-race branch from a71da8f to 058834b Compare October 19, 2021 22:30

krishgobinath requested review from aanm and removed request for a team October 19, 2021 22:30

Weil0ng approved these changes Oct 20, 2021

View reviewed changes

aanm requested changes Oct 20, 2021

View reviewed changes

krishgobinath force-pushed the ceb-race branch from 058834b to 4aa7473 Compare October 21, 2021 05:52

krishgobinath requested a review from aanm October 21, 2021 20:51

aanm approved these changes Oct 22, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned aanm Oct 22, 2021

aanm merged commit 77232c4 into cilium:feature/cep-scalability Oct 22, 2021

krishgobinath mentioned this pull request Oct 25, 2021

Enable CiliumEndpointSlice feature #17658

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Identity based batching race condition Issues #17520

Identity based batching race condition Issues #17520

krishgobinath commented Oct 4, 2021 •

edited

aanm Oct 4, 2021

krishgobinath Oct 5, 2021

Weil0ng Oct 4, 2021

krishgobinath Oct 5, 2021

aanm Oct 6, 2021

krishgobinath Oct 6, 2021 •

edited

aanm Oct 11, 2021

aanm Oct 6, 2021

aanm Oct 6, 2021 •

edited

krishgobinath Oct 6, 2021

aanm Oct 11, 2021 •

edited

krishgobinath Oct 12, 2021 •

edited

Weil0ng Oct 13, 2021

Weil0ng Oct 14, 2021

aanm Oct 18, 2021

Weil0ng Oct 18, 2021

aanm Oct 18, 2021

Weil0ng Oct 18, 2021

aanm Oct 11, 2021

aanm Oct 11, 2021 •

edited

Weil0ng Oct 19, 2021

krishgobinath Oct 19, 2021

aanm Oct 20, 2021

krishgobinath Oct 21, 2021

Identity based batching race condition Issues #17520

Identity based batching race condition Issues #17520

Conversation

krishgobinath commented Oct 4, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krishgobinath Oct 6, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aanm Oct 6, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aanm Oct 11, 2021 • edited

Choose a reason for hiding this comment

krishgobinath Oct 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aanm Oct 11, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krishgobinath commented Oct 4, 2021 •

edited

krishgobinath Oct 6, 2021 •

edited

aanm Oct 6, 2021 •

edited

aanm Oct 11, 2021 •

edited

krishgobinath Oct 12, 2021 •

edited

aanm Oct 11, 2021 •

edited