Refactor netns handling, fix flakes, namespace some tests #227
Conversation
This file contains 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
This commit introduces a breaking change to rtnetlink.NetNS. The existing netns implementation had a few problems. It assumed that network namespaces have names, that they would always be pinned to /var/run/netns, and that numeric/integer references are pid references. This made the NetNS type unusable for referring to existing netns by fd, such as ones created by other libraries, or by opening procfs entries directly as demonstrated in the new testutils.NetNS() function. The forced dependency on the `ip` CLI tool also wasn't reasonable for a pure-Go library. Using the old implementation in a scratch/distroless container would quickly run into roadblocks. This commit also removes the functionality of creating and pinning new netns. There are plenty of options out in the Go ecosystem for that, and providing your own is only a few lines of code. Signed-off-by: Timo Beckers <[email protected]>
…ck() ebpf-go provides this out of the box and skips setting the rlimit on kernels that support bpf memory cgroup accounting. Signed-off-by: Timo Beckers <[email protected]>
The flaky tests that were documented in the code are expected. Use the State field to discard entries that can't reasonably be considered in tests. Signed-off-by: Timo Beckers <[email protected]>
When running tests locally, I would frequently hit "too many/little matches, expected 1, actual 0" due to other tests creating and deleting interfaces in the common host netns used by all tests. Neigh entries that fail the interface lookup can't have their Interface fields populated and should be dropped from the result since the interface is no longer there to begin with. Signed-off-by: Timo Beckers <[email protected]>
While running the test suite for testing netns-related changes, I noticed some of the xdp tests started failing because they wanted to create a dummy interface in the host network namespace. Avoid the complexity of managing dummy interfaces altogether by running all tests within their own netns and use the existing lo device that's present by default. Signed-off-by: Timo Beckers <[email protected]>
…nOldKernel There were two implementations of this, so move them to common testutils. Signed-off-by: Timo Beckers <[email protected]>
|
Thank you for these fixes! They are much appreciated. Out of curiosity and you being a user of this library: |
|
@jsimonetti Given the small amount of time (5-6 days) the first release including the netns API has been out, I don't think there's much risk breaking existing users, if that's what your after. 😄 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Please see individual commits for the reasoning behind every change. I did some drive-by cleaning on the more obvious things I could fix quickly, driven by lessons learned working on other libraries.
Please let me know if this should be split up. Most of the changes are in testing and they follow logically, so I thought they made sense as a single PR.