fix ACL flakiness - #1968
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughChangesConnectivity probe stability
Possibly related PRs
Suggested reviewers: Mergeability Score: ⚪ Minimal · up to No actionable merge-blocking risk remains; the PR is merge-ready after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
edipascale
added
ci:-upgrade
There was a problem hiding this comment.
Pull request overview
This PR reduces flakiness in ACL-related connectivity tests by lowering probe contention and improving ping loss observability, aligning with Issue #1937’s report of intermittent ICMP loss likely caused by dataplane instability/concurrency at test start.
Changes:
- Add
ping -Oto emit per-sequence timeout lines, improving correlation of loss timing with-Dtimestamps. - Limit NAT test connectivity probing to servers in the two VPCs under test plus a single “outside” control server to reduce concurrent load.
- Run proto/port probes sequentially per (src,dst) pair (icmp → tcp → udp) and serialize ping against the iperf/udp flood semaphore to avoid inducing ICMP loss via test traffic.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/hhfab/testing.go | Enhances ping command flags to improve loss diagnostics (-D -O). |
| pkg/hhfab/rt_nat_tests.go | Reduces NAT test probe scope to decrease concurrent probing load. |
| pkg/hhfab/matrix.go | Serializes per-pair proto/port probes and coordinates ping with iperf/udp concurrency limits. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Release Tests 5 files 20 suites 2h 14m 24s ⏱️ Results for commit 335e719. ♻️ This comment has been updated with latest results. |
pau-hedgehog
left a comment
pau-hedgehog
left a comment
There was a problem hiding this comment.
Diagnosis is right and the fix works. entryOwner gives a pair with proto-port entries entirely to the proto-port phase (pkg/hhfab/matrix.go:379), so for the ACL pair under test the ping in #1937 and the two udp floods were the same goroutine fan-out, with nothing sequencing them. Serializing them is correct.
Build, just lint and go test ./pkg/hhfab/ are clean on d884128.
The main comment is about the flood itself rather than the scheduling of it.
1. The udp probe rate does not belong on virtual switches
The probe sends 10 Mbit/s of 1000-byte datagrams, 1250 pps, for the full IPerfsSeconds (pkg/hhfab/testing.go:3538):
func udpProbeCmd(toIP netip.Addr, port uint16, secs int, timing udpProbeTiming) string {
return fmt.Sprintf("sudo docker exec iperf3 timeout -k 5 %d iperf3 -u -J --connect-timeout %d -c %s -p %d -t %d -b 10M -l 1000",
int(timing.inner.Seconds()), timing.connect.Milliseconds(), toIP.String(), port, secs)
}The verdict it feeds needs almost none of that (pkg/hhfab/testing.go:3498):
const (
udpDenyLossThreshold = 99.0
udpAllowLossThreshold = 90.0
)Allow needs loss under 90 percent, deny needs loss over 99 percent. At -b 1M the probe answers both with the same margin. The rate is not measuring anything, it is only generating load.
And we already say we do not trust this path for throughput on virtual switches (pkg/hhfab/testing.go:2339):
if allVirtual {
if !allCumulusVX {
slog.Warn("All switches are virtual, ignoring IPerf min speed")
// Seems like we're facing some iPerf speed issues on VS so disabling the check for the speed
opts.IPerfsMinSpeed = 0So on VLAB the suite zeroes the throughput floor because the numbers are not trustworthy, then pushes a 10 Mbps flood through that same software dataplane and asserts on what survives alongside it. That flood is what produced #1937. Serializing it removes the collateral damage, lowering it removes the cause. I would like both, and I would rather not carry -b 10M into the release.
2. The new mutual exclusion rests on an unstated invariant
The comment claims a global property (pkg/hhfab/matrix.go:867):
case "icmp":
// ping takes the iperfs semaphore rather than the pings one:
// the udp probe is a 10Mbps flood, and one running anywhere"anywhere" holds only because the semaphore has one slot, set by a default 40 lines away (pkg/hhfab/matrix.go:910):
if opts.IPerfsParallel <= 0 {
opts.IPerfsParallel = 1
}IPerfsParallel is not a flag and rt_base.go leaves it zero, so this is true today. Raise it to 2 for speed at any point and the flake returns with nothing pointing at the change. Either say that in the comment, or take a separate weight-1 semaphore for "a measurement is in flight", so the throughput budget and the quiet window are not the same knob.
Related: server-server pings still take the 50-wide pings semaphore, so the guarantee covers the proto-port phase only. Harmless while the suite pins ServersPerSubnet = 1 and every remaining server-server pair is deny-expected. Worth a line so the scope is on record.
3. Smaller things
- pkg/hhfab/testing.go:3056 still says
"acquiring ping semaphore", which for proto-port icmp is now the iperf semaphore. That string is what a CI failure surfaces. natTestProbeServersis pure and has no test. Three cases pin it: two VPCs only so there is no control server, deterministic control choice, and a server attached to both an under-test and an outside VPC. That last one is why the!underTest[name]guard at pkg/hhfab/rt_nat_tests.go:119 is not dead code, sinceAllEndpointsis keyed per server and VPC.-Ois safe, I checked both readers:parsePingLostSeqsrequiresbytes fromand the summary parser requirespackets transmitted, sono answer yet for icmp_seq=Nis inert to both.
4. Do we want this in the release
Worth deciding explicitly rather than by merge timing. My read is yes, take it. The mechanism was found structurally rather than statistically, so clean repeats would confirm what the code already shows, and at the observed hit rate the repeat count needed to tell a fix from a lucky streak is high. It is test-only code, so a wrong call costs a flaky test rather than a shipped defect, and shipping with the flake means it masks whatever real regression it lands next to.
One full release-test run is still worth having, for something repeats do not answer: whether serializing the proto-port phase behind a single slot moved the suite's wall clock anywhere that matters.
If the release is tight, the two commits are separable. The first is the flake fix and is test timing only. The second permanently changes what the suite asserts, trading 88 redundant pair checks for one isolation control server, and deserves its own ack rather than riding in on the flake fix.
d884128 to
f2abec7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/hhfab/matrix.go`:
- Around line 877-883: Update runMatrixProtoPortPhase so matrix ICMP and UDP
probes are constrained by a dedicated capacity-one gate, regardless of the
configured IPerfsParallel value. Ensure both checkPing and checkUDPPort acquire
and release this gate while preserving their existing probe behavior.
In `@pkg/hhfab/rt_nat_tests.go`:
- Around line 102-127: Update natTestProbeServers to detect when no server
outside vpc1 and vpc2 is available and return the required isolation-control
failure result, including a descriptive error so JUnit records the skip reason;
preserve the existing sorted probe-server selection when an outside server
exists.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5dbf355e-8dd8-46ae-b479-7dfb35a69ead
📒 Files selected for processing (3)
pkg/hhfab/matrix.gopkg/hhfab/rt_nat_tests.gopkg/hhfab/testing.go
f2abec7 to
5098f58
Compare
|
thanks @pau-hedgehog, I've hopefully addressed your comments here. I've moved the server scoping to #1974 and added the suite shuffle there too; both changes are about making the release tests faster and not really related to the fix to the flakiness issue. |
5098f58 to
335e719
Compare
The proto-port phase started one goroutine per (pair, protocol), so a pair's icmp probe ran at the same time as its own tcp/udp probes and as those of the reverse direction. The udp probe sent iperf3 -u -b 10M -l 1000 for the whole IPerfsSeconds window, about 1250pps through the gateway's kernel dataplane, while ping measured that same path and required zero loss. Only the ACL tests are exposed, since they are the sole callers stamping ProtoPort entries and elsewhere runPingIperfPair already sequences a pair's ping and throughput probes. It fits what #1937 reported: RTT rising across a burst, drops at the later seqs, and no route, BFD or config event at the time of the drop. Run a pair's entries in one goroutine, in the (icmp, tcp, udp) order ProtoPortEntries already sorts them into, behind a new weight-1 probes semaphore so one proto-port measurement is in flight at a time. Each probe still takes its own global budget, pings or iperfs, on top of that. The gate is not the iperfs semaphore on purpose: that one is sized by IPerfsParallel, and raising it must not reopen this. Lower the udp probe rate to -b 1M. The verdict needs loss below 90% or above 99% and 1Mbps answers that with the same margin, so the rest of the rate was only load, on a path whose throughput the suite already distrusts enough to zero IPerfsMinSpeed on virtual switches. Pass -O to the measured ping as well: it reports each unanswered seq as it times out, putting the loss on the same clock -D puts the replies on. Those lines carry no "bytes from", so neither the sent/received parser nor parsePingLostSeqs reads them, and the latter gets a fixture with them to keep it that way. Signed-off-by: Emanuele Di Pascale <emanuele@githedgehog.com>
335e719 to
499824e
Compare
Frostman
added
ci:-vlab
Follow-ups to comments that were marked addressed but were not: - record that the proto-port probe semaphore only covers its own phase, since the server-to-server phase keeps pinging concurrently - release that semaphore via defer, so a future early exit between the acquire and the release cannot stall the phase on its single slot - fail loudly when a NAT test finds no servers to probe: an empty source list reads as "no filter" downstream and probes the whole matrix - run Gateway Peering Overlap NAT last in the gateway NAT/ACL suite; it creates an IPv4Namespace and a VPC and re-attaches a server, and without a wipe between tests the rest of the suite inherits whatever its reverts miss - fix the -D -O ping fixture timestamps, which were non-monotonic and duplicated Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Emanuele Di Pascale <emanuele@githedgehog.com>
Follow-ups to comments that were marked addressed but were not: - record that the proto-port probe semaphore only covers its own phase, since the server-to-server phase keeps pinging concurrently - release that semaphore via defer, so a future early exit between the acquire and the release cannot stall the phase on its single slot - fail loudly when a NAT test finds no servers to probe: an empty source list reads as "no filter" downstream and probes the whole matrix - run Gateway Peering Overlap NAT last in the gateway NAT/ACL suite; it creates an IPv4Namespace and a VPC and re-attaches a server, and without a wipe between tests the rest of the suite inherits whatever its reverts miss - fix the -D -O ping fixture timestamps, which were non-monotonic and duplicated Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Emanuele Di Pascale <emanuele@githedgehog.com>
Fix #1937