agent: stop endpoints in parallel on exit

[jaffcheng]

agent: stop endpoints in parallel on exit

Endpoints are stopped on exit signals in an agent cleanup function.
This patch does this in goroutines to speed it up, reduces the probability
of agent exiting timeout, that is, reduces the possibility of pod network
disconnection caused by interrupted regeneration.

Related: #15446
Signed-off-by: Jaff Cheng jaff.cheng.sh@gmail.com

Wait for endpoints to be stopped on agent shutdown

[jaffcheng]

I guess this should be a minor change now.

[aditighag]

This is a nice improvement, but I don't see how this fixes the issue you've tagged.

This happens when cilium-agent is killed (crash/upgrade/operational restart) during endpoint regeneration, more specifically, during synchronizeDirectories function call.

If an agent crashes or killed, then wait groups are not going to add any protection anyway.

In the scenario of RegenerateWithoutDatapath (e.g. one or more identities created or deleted), next dir is empty at this step. So after the following code, we have an empty state dir 1111 and a complete backup 1111_stale
If the agent stops at this point, after the agent restarts, we lose track of this endpoint, tail call maps will get GCed, and the pod will lose connectivity.

The way we take backup across 2 steps (os.Rename is an atomic call) should let us restore from the backup upon agent restart. 🤔 I don't quite follow why "tail call maps will get GC'd" as they would still being referenced in BPF programs.

[jaffcheng]

This is a nice improvement, but I don't see how this fixes the issue you've tagged.

Yes, I've commented in the issue: #15446 (comment), also changed the Fixes tag to Related in commit msg.

The way we take backup across 2 steps (os.Rename is an atomic call) should let us restore from the backup upon agent restart. 🤔 I don't quite follow why "tail call maps will get GC'd" as they would still being referenced in BPF programs.

I think we just ignored backups while restoring endpoints. I'm not sure if this is intended or just an overlook.

cilium/pkg/endpoint/restore.go

Lines 170 to 178 in 52ec2f6

	if _, ok := possibleEPs[ep.ID]; ok {
	// If the endpoint already exists then give priority to the directory
	// that contains an endpoint that didn't fail to be build.
	if strings.HasSuffix(ep.DirectoryPath(), epDirName) {
	possibleEPs[ep.ID] = ep
	}
	} else {
	possibleEPs[ep.ID] = ep
	}

I think maps are GCed because the endpoint is not restored properly

cilium/pkg/datapath/maps/map.go

Lines 113 to 119 in 52ec2f6

	for _, m := range mapPrefix {
	if strings.HasPrefix(filename, m) {
	if endpointID := strings.TrimPrefix(filename, m); endpointID != filename {
	ms.deleteMapIfStale(path, filename, endpointID)
	}
	}
	}

[pchaigno]

I think we just ignored backups while restoring endpoints. I'm not sure if this is intended or just an overlook.

We should probably fix that.

[jaffcheng]

I think we just ignored backups while restoring endpoints. I'm not sure if this is intended or just an overlook.

We should probably fix that.

I'll try to fix that in a separate PR.

[jaffcheng]

Rebased

[pchaigno]

test-me-please

[pchaigno]

Let's see if https://jenkins.cilium.io/job/Cilium-PR-K8s-1.16-net-next/72/testReport/junit/Suite-k8s-1/16/K8sServicesTest_Checks_service_across_nodes_Tests_NodePort_BPF_Tests_with_direct_routing_and_DSR/ is a flake (seems very likely).

test-1.16-netnext