test: remove 'unparallel' build tag in favor of testutils helper#44304
Merged
ti-mo merged 8 commits intocilium:mainfrom Mar 11, 2026
Merged
test: remove 'unparallel' build tag in favor of testutils helper#44304ti-mo merged 8 commits intocilium:mainfrom
ti-mo merged 8 commits intocilium:mainfrom
Conversation
Contributor
Author
|
/test |
57ec4aa to
fee81a3
Compare
Contributor
Author
|
/test |
bimmlerd
approved these changes
Feb 12, 2026
Member
bimmlerd
left a comment
There was a problem hiding this comment.
LGTM, thanks for fighting build tags Timo 🙇
tklauser
approved these changes
Feb 12, 2026
christarazi
approved these changes
Feb 13, 2026
dylandreimerink
approved these changes
Feb 13, 2026
tommyp1ckles
approved these changes
Feb 15, 2026
fee81a3 to
67d6bbe
Compare
Contributor
Author
|
/test |
1 similar comment
Contributor
Author
|
/test |
67d6bbe to
4059666
Compare
Contributor
Author
|
/test |
Contributor
Author
|
/ci-runtime |
4059666 to
892771e
Compare
Contributor
Author
|
/test |
Contributor
Author
|
/ci-runtime |
Contributor
Author
|
/test |
On main, these are currently failing with errors like: ``` logger.go:256: time=2026-03-09T23:29:19.121+01:00 level=ERROR source=/home/cilium/cilium/pkg/datapath/linux/node_ids.go:148 msg="Failed to map node IP address to allocated ID" error="invalid node IP <nil>: ParseAddr(\"<nil>\"): unable to parse IP" nodeID=39855 ipAddr=<nil> spi=0 ``` The recent change to netip for IP addresses broke these, and it's clear they haven't been actively maintained, extended or exercised in a while. If we care about them, we should execute them in CI. Also, the structure is a bit of a mess, so I propose to get rid of them to ease maintenance. Signed-off-by: Timo Beckers <timo@isovalent.com>
Since multiple parts of the code couldn't agree on what to do with fb_tunnels, this commit standardizes on always setting it to 2, as well as managing it through an import side effect to make sure it runs as early during test execution as possible. This reduces the risk of tests loading kernel modules before setting the sysctl. It's tied to the netns package because that's when it usually becomes a problem: creating netns and not getting a clean environment. The tests for renaming fallback devices to cilium_tunl and cilium_ip6tnl were removed as the behaviour in question isn't load-bearing and the Cilium agent itself now also defaults to setting fb_tunnels to 2. Signed-off-by: Timo Beckers <timo@isovalent.com>
We've gone through great lengths to avoid using build tags in tests since it messes with editor tooling, code completion, go-to reference, and notably excludes files from golangci-lint. Remove the 'unparallel' build tag. Signed-off-by: Timo Beckers <timo@isovalent.com>
Another footgun of build tags in tests: code is obscured from linters as well. Signed-off-by: Timo Beckers <timo@isovalent.com>
This commit gives the node tests some long-overdue attention: - All tests get their own netns; this file has been a frequent source of test concurrency bugs/interference - Add test helpers for often-repeated statements that should never fail - Minimize the 'suite' concept and reduce its verbosity - Remove teardown logic; let the kernel take care of this - Introduced the testutils/netlink package for common netlink-related test helpers Signed-off-by: Timo Beckers <timo@isovalent.com> Suggested-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
What it says on the tin, this commit removes some footguns, only runs tests as privileged when needed, uses test helpers, and gets rid of the 'suite' mechanism in favor of simple subtest loops. Signed-off-by: Timo Beckers <timo@isovalent.com>
Signed-off-by: Timo Beckers <timo@isovalent.com>
We've spent considerable effort so far avoiding using build tags in tests to make tooling play nice, keeping the dev experience smooth and friction to a minimum. It's unfortunate that we've regressed, so add a linter to make sure this doesn't happen again without being raised with committers. Signed-off-by: Timo Beckers <timo@isovalent.com>
892771e to
0c2d4d6
Compare
Contributor
Author
|
/test |
1 similar comment
Contributor
Author
|
/test |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Let's avoid regressing in the future, please.
Node and ipsec tests no longer have any side effects in the host netns.
Fixes #42182.
testutils/netnsto setnet.core.fb_tunnels_only_for_init_net=2. It's a backwards-compat measure, not sane behaviour IMO. Then namespaced tests get clean environments out of the box, and there's no risk of new modules getting loaded before netns creation.