CI: Cilium IPsec upgrade - unexpected packet drops on downgrade

[giorio94]

CI failure

Hit on #29896
Link: https://github.com/cilium/cilium/actions/runs/7258862919/job/19777306148
Sysdump: cilium-sysdump-ipsec-downgrade-1-20231219-094253.zip

 [=] Test [no-unexpected-packet-drops] [3/71]

  [-] Scenario [no-unexpected-packet-drops/no-unexpected-packet-drops]
  🟥 Found unexpected packet drops:
{
  "labels": {
    "direction": "EGRESS",
    "reason": "Missed tail call"
  },
  "name": "cilium_drop_count_total",
  "value": 1
}

[ti-mo]

Related: #28088

[giorio94]

Hit on #30371 (v1.14)
Link: https://github.com/cilium/cilium/actions/runs/7636972870/job/20804933744
Sysdump: cilium-sysdump-ipsec-downgrade-5-20240124-081037.zip

[ti-mo]

@giorio94 Thanks for the report! As far as I can tell, the failed job downgraded to 1.14.5 (I'm assuming this is correct and it's not using unreleased v1.14 tip under the covers!) which doesn't have the latest round of fixes (the new 'Host datapath not ready' drop reason) yet. I remain hopeful that this will fix the issue once and for all. 😄

[bimmlerd]

Hit in #31119
Link: https://github.com/cilium/cilium/actions/runs/8140815226/job/22284010883#step:19:276
sysdump: cilium-sysdump-ipsec-downgrade-1-20240305-085616.7z.zip (sorry for the stupid format, but GH doesn't allow 7z files, and just zip was 30M 😮‍💨)

[giorio94]

Hit on #31013
Link: https://github.com/cilium/cilium/actions/runs/8152624427/job/22282423609
Sysdump: cilium-sysdump-ipsec-downgrade-1-20240305-080723.zip

[ti-mo]

Thanks for the reports, all.

@julianwiedmann FYI, we've found and fixed a few 'real' missed tail calls in the datapath revealed by static analysis on the bytecode: 672fccf.

Are these code paths that could potentially be triggered during up/downgrade tests, resulting in missed tail calls? I'm relatively confident the user space side is sound, even on release branches, so we may need to start looking into datapath itself. #30972 should provide more clarity on 1.16 but I've hit a small blocker and this still needs to be backported. Almost impossible to find the culprit until this lands.

[julianwiedmann]

Thanks for the reports, all.

@julianwiedmann FYI, we've found and fixed a few 'real' missed tail calls in the datapath revealed by static analysis on the bytecode: 672fccf.

Are these code paths that could potentially be triggered during up/downgrade tests, resulting in missed tail calls?

Not for testing in combination with IPsec, unfortunately :/. So we're still chasing ...

I'm relatively confident the user space side is sound, even on release branches, so we may need to start looking into datapath itself. #30972 should provide more clarity on 1.16 but I've hit a small blocker and this still needs to be backported. Almost impossible to find the culprit until this lands.

And there's no way to start debugging with a WIP branch, because we need actual builds for the up/downgrade? Hmpf.

[chancez]

And there's no way to start debugging with a WIP branch, because we need actual builds for the up/downgrade? Hmpf.

We do build and push images and push dev charts to quay for PRs IIRC, so you have some options there.

[giorio94]

Hit on #31399 as well (on downgrade)
Link: https://github.com/cilium/cilium/actions/runs/8285336368/job/22707998046
Sysdump: cilium-sysdump-ipsec-downgrade-1-20240315-141915.zip

[dylandreimerink]

@marqc Just made a discovery with regards to this flake. Quote from a slack thread:

I combined changes from PR/29711+PR/30972 with downgrade test to v1.15+PR/30972
The cilium bpf metrics list shows missed tail call in "bpf_host.c" line 1181
cilium-c969j

{
  "reason": "Missed tail call",
  "direction": "ingress",
  "packets": 124,
  "bytes": 39928,
  "line": 1181,
  "file": "bpf_host.c"
}

cilium-m7rmx
{
  "reason": "Missed tail call",
  "direction": "ingress",
  "packets": 62,
  "bytes": 19964,
  "line": 1181,
  "file": "bpf_host.c"
}

cilium-v4grq
{
  "reason": "Missed tail call",
  "direction": "ingress",
  "packets": 248,
  "bytes": 79856,
  "line": 1181,
  "file": "bpf_host.c"
}

If I'm reading this correctly, then missed tail call is in here:

		ret = tail_call_internal(ctx, from_host ? CILIUM_CALL_IPV4_FROM_HOST :
							  CILIUM_CALL_IPV4_FROM_NETDEV,
					 &ext_err);
		/* We are not returning an error here to always allow traffic to
		 * the stack in case maps have become unavailable.
		 *
		 * Note: Since drop notification requires a tail call as well,
		 * this notification is unlikely to succeed.
		 */
		return send_drop_notify_error_ext(ctx, identity, ret, ext_err,
						  CTX_ACT_OK, METRIC_INGRESS);

So what we know, is:

1. We have a temporary missing tail call (metric increments during program replacement, but stops incrementing once downgrade is done) [link]
2. Its on the ingress path
3. Its in do_netdev in bpf_host.c

So if its ingress. Then from_host must be false. Thus its the CILIUM_CALL_IPV4_FROM_NETDEV tail call that is missing.

But I have yet to come up with an idea how or why that situation might occur. Perhaps someone else has an idea?

[thorn3r]

Hit in https://github.com/cilium/cilium/actions/runs/8524886608/job/23350536331 on #31677
Sysdump:
cilium-sysdump-1-final.zip

[thorn3r]

Hit again in https://github.com/cilium/cilium/actions/runs/8574999134/job/23504841663 on #31677

[giorio94]

One more: https://github.com/cilium/cilium/actions/runs/8630649349/job/23657371018

[thorn3r]

Another hit: https://github.com/cilium/cilium/actions/runs/8974259819/job/24646270999

[github-actions]

This issue has been automatically marked as stale because it has not
had recent activity. It will be closed if no further activity occurs.

[github-actions]

This issue has not seen any activity since it was marked stale.
Closing.