Reduce unnecessary churn and probes on ACL/NAT tests by edipascale · Pull Request #1974 · githedgehog/fabricator

edipascale · 2026-08-13T16:40:32Z

for these test scenarios which only have two VPCs, select only the two servers that belong to them, plus a third server not in the VPCs to ensure we are not leaking anything (down from however many servers there are in the topology)
move the tests to a separate suite without wipeBetweenTests, since it is not needed here; this means not running setup-vpcs every time, so no cycle of hhnet cleanup + DHCP etc. to get the exact same assignments as before

coderabbitai · 2026-08-13T16:40:44Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d0591226-b05b-4edc-93ae-07d9ebd5f2e4

📥 Commits

Reviewing files that changed from the base of the PR and between 335e719 and 2845e9f.

📒 Files selected for processing (5)

pkg/hhfab/matrix_test.go
pkg/hhfab/rt_base.go
pkg/hhfab/rt_gw_nat_acl_suite.go
pkg/hhfab/rt_multi_vpc_single_subnet_suite.go
pkg/hhfab/rt_nat_tests.go

💤 Files with no reviewable changes (1)

pkg/hhfab/rt_multi_vpc_single_subnet_suite.go

📝 Walkthrough

Walkthrough

The PR adds deterministic NAT probe selection, moves NAT and ACL cases into a dedicated gateway suite, and runs that suite in the normal release-test flow. It also adds table-driven coverage for probe selection.

Changes

NAT and ACL release tests

Layer / File(s)	Summary
NAT probe selection and coverage `pkg/hhfab/rt_nat_tests.go`, `pkg/hhfab/matrix_test.go`	NAT tests use servers from the tested VPCs and at most one external server. Probe ordering is deterministic, and table-driven tests cover endpoint ordering and VPC membership.
Gateway suite composition `pkg/hhfab/rt_gw_nat_acl_suite.go`, `pkg/hhfab/rt_multi_vpc_single_subnet_suite.go`	A dedicated gateway NAT/ACL suite aggregates NAT, external NAT, and gateway peering ACL cases. The multi-VPC single-subnet suite no longer registers those cases.
Normal release-test execution `pkg/hhfab/rt_base.go`	The gateway suite runs in normal mode, while on-ready mode remains isolated. Its results and failures are included in reporting and aggregate failure handling.

Possibly related PRs

githedgehog/fabricator#1910: Related changes to NAT/ACL release-test suite registration and NAT test handling.

Suggested reviewers: pau-hedgehog

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly summarizes the primary optimization: reducing unnecessary work and probes in ACL and NAT tests.
Description check	✅ Passed	The description directly explains reducing probe scope and moving ACL/NAT tests to a separate suite without repeated cleanup.

_{Comment @coderabbitai help to get the list of available commands.}

github-actions · 2026-08-13T23:45:07Z

Release Tests

11 files + 6 55 suites +35 3h 56m 31s ⏱️ + 1h 42m 7s
58 tests ± 0 40 ✅ - 1 17 💤 ± 0 1 ❌ +1
638 runs +348 260 ✅ +119 377 💤 +228 1 ❌ +1

For more details on these failures, see this check.

Results for commit 2845e9f. ± Comparison against base commit 335e719.

♻️ This comment has been updated with latest results.

edipascale · 2026-08-14T08:14:38Z

time savings for the vlab gw jobs compared to the base:

edipascale · 2026-08-14T09:00:10Z

@coderabbitai review

coderabbitai · 2026-08-14T09:00:16Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

pau-hedgehog

Validated with repeated full CI runs on this branch:
https://github.com/githedgehog/fabricator/actions/workflows/ci.yaml?query=branch%3Aema%2Ffaster-acl-and-nat-rts+actor%3Apau-hedgehog

Three comments inline, none blocking.

Frostman

just a rebase

Every NAT and ACL test peers the first two VPCs, then probed the whole matrix: with one server per VPC that is ~90 ordered pairs, of which 2 are the subject. The other 88 only restate that unpeered VPCs cannot talk, and they cost up to PingsParallel concurrent ssh sessions and probes while the pair under test is being measured. Narrow Sources/Destinations to the servers of the two peered VPCs plus one server outside them, kept as an isolation control so a peering that leaks past its VPCs still fails a test. This narrows the curl phase to those servers too. Unit-test the selection, including the case of a server attached to both a tested and an untested VPC, which is what the dedup guard is for. Signed-off-by: Emanuele Di Pascale <emanuele@githedgehog.com>

The NAT, external NAT and ACL tests all sat in the multi-VPC single-subnet suite, which runs with wipeBetweenTests set, so each of the 28 was preceded by a full VPC setup: every server ran hhnet cleanup and took a fresh DHCP lease, and the gateway peering was deleted and recreated instead of updated. That is the churn the probes in #1937 raced. None of them needs it. They only create their own peerings, return no reverts, and DoSetupPeerings deletes every peering absent from the spec they pass, so the previous test's peerings are gone either way. Move them to their own suite, run before wipeBetweenTests is set, which leaves one setup for the whole group instead of 28. 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>

edipascale mentioned this pull request Aug 13, 2026

fix ACL flakiness #1968

Merged

edipascale self-assigned this Aug 13, 2026

edipascale added ci:+release Enable VLAB release tests ci:+hlab Enable hybrid VLAB tests labels Aug 13, 2026

edipascale force-pushed the ema/faster-acl-and-nat-rts branch from 31b0666 to 2845e9f Compare August 14, 2026 06:14

edipascale marked this pull request as ready for review August 14, 2026 06:14

edipascale requested review from a team as code owners August 14, 2026 06:14

pau-hedgehog previously approved these changes Aug 14, 2026

View reviewed changes

Comment thread pkg/hhfab/rt_nat_tests.go

Comment thread pkg/hhfab/rt_gw_nat_acl_suite.go

Comment thread pkg/hhfab/rt_nat_tests.go

Frostman previously approved these changes Aug 14, 2026

View reviewed changes

Frostman dismissed stale reviews from pau-hedgehog and themself via b4942c1 August 14, 2026 22:32

Frostman force-pushed the ema/faster-acl-and-nat-rts branch from 2845e9f to b4942c1 Compare August 14, 2026 22:32

Frostman added ci:-vlab Disable VLAB tests ci:-upgrade Disable VLAB upgrade tests and removed ci:+release Enable VLAB release tests ci:+hlab Enable hybrid VLAB tests labels Aug 14, 2026

Frostman approved these changes Aug 14, 2026

View reviewed changes

Base automatically changed from ema/fix-acl-flakiness to master August 14, 2026 22:46

edipascale added 2 commits August 14, 2026 15:46

Frostman force-pushed the ema/faster-acl-and-nat-rts branch from b4942c1 to c6ffe57 Compare August 14, 2026 22:46

Frostman merged commit efb762f into master Aug 14, 2026
31 of 38 checks passed

Frostman deleted the ema/faster-acl-and-nat-rts branch August 14, 2026 22:59

Navigation Menu

Uh oh!

Reduce unnecessary churn and probes on ACL/NAT tests - #1974

Reduce unnecessary churn and probes on ACL/NAT tests#1974
Frostman merged 2 commits into
masterfrom
ema/faster-acl-and-nat-rts

edipascale commented Aug 13, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Aug 13, 2026 •

edited

Loading

Walkthrough

Changes

Uh oh!

github-actions Bot commented Aug 13, 2026 •

edited

Loading

Uh oh!

edipascale commented Aug 14, 2026

Uh oh!

edipascale commented Aug 14, 2026

Uh oh!

coderabbitai Bot commented Aug 14, 2026 •

edited

Loading

Uh oh!

pau-hedgehog left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Frostman left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

edipascale commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

github-actions Bot commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Release Tests

Uh oh!

edipascale commented Aug 14, 2026

Uh oh!

edipascale commented Aug 14, 2026

Uh oh!

coderabbitai Bot commented Aug 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pau-hedgehog left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Frostman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

edipascale commented Aug 13, 2026 •

edited

Loading

coderabbitai Bot commented Aug 13, 2026 •

edited

Loading

github-actions Bot commented Aug 13, 2026 •

edited

Loading

coderabbitai Bot commented Aug 14, 2026 •

edited

Loading