Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test: add the possibility to run a test inside a network namespace #696

Merged
merged 1 commit into from
Jul 31, 2023
Merged

test: add the possibility to run a test inside a network namespace #696

merged 1 commit into from
Jul 31, 2023
+115 −3

Conversation

Tuetuopay
Copy link
Contributor

For tests that do networking operations, this allows to have a clean-state network namespace and interfaces for each test. Mainly, this avoids "device or resource busy" errors when reusing the loopback interface across tests.

@netlify
Copy link

netlify bot commented Jul 27, 2023 •
edited
Loading

✅ Deploy Preview for aya-rs-docs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit c74813f
🔍 Latest deploy log https://app.netlify.com/sites/aya-rs-docs/deploys/64c6e2e5d9903f00084a0196
😎 Deploy Preview https://deploy-preview-696--aya-rs-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

dave-tucker
dave-tucker requested changes Jul 27, 2023
View reviewed changes
Copy link
Member

@dave-tucker dave-tucker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! A couple of nits, but excited to get this merged.

test/integration-test/src/tests/smoke.rs Outdated Show resolved Hide resolved
test/integration-test/src/utils.rs Outdated Show resolved Hide resolved
test/integration-test/src/utils.rs Outdated Show resolved Hide resolved
test/integration-test/src/utils.rs Outdated Show resolved Hide resolved
test/integration-test/src/utils.rs Outdated Show resolved Hide resolved
test/integration-test/src/utils.rs Show resolved Hide resolved
tamird
tamird reviewed Jul 27, 2023
View reviewed changes
test/integration-test/src/utils.rs Outdated Show resolved Hide resolved
test/integration-test/src/utils.rs Show resolved Hide resolved
test/integration-test/src/utils.rs Outdated Show resolved Hide resolved
@Tuetuopay
Copy link
Contributor Author

Tuetuopay commented Jul 27, 2023 •
edited
Loading

we might want to include the pid of the current process in the netns name to avoid concurrent processes from picking the same one.

@tamird great idea. done in 3a44804

tamird
tamird requested changes Jul 27, 2023
View reviewed changes
test/integration-test/src/utils.rs Outdated Show resolved Hide resolved
@Tuetuopay
Copy link
Contributor Author

@tamird I'll remove that script function. Something more ad-hoc will be added in the future when needed (even in my xdp maps pr I don't need it).

Good idea for the netns crate. Especially since it handles the rollback to the original netns, it'll remove the thread we spawn.

As for userspace, I think that iproute2 is ubiquitous enough that it's not a big constraint to require it to run tests. Especially since we use it to up a link, that even busybox's implementation of the ip command can handle. Furthermore, I really don't want to bring the complexity of manual netlink in the codebase for something as simple as upping lo. I'll have a look at the raw netlink code aya already has, and see if it can be added without too much code.

tamird
tamird reviewed Jul 30, 2023
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
test/integration-test/Cargo.toml Outdated Show resolved Hide resolved
aya/src/sys/netlink.rs Show resolved Hide resolved
test/integration-test/src/utils.rs Outdated Show resolved Hide resolved
test/integration-test/src/utils.rs Outdated Show resolved Hide resolved
test/integration-test/src/utils.rs Outdated Show resolved Hide resolved
test/integration-test/src/utils.rs Outdated Show resolved Hide resolved
test/integration-test/src/utils.rs Outdated Show resolved Hide resolved
test/integration-test/src/utils.rs Outdated Show resolved Hide resolved
tamird
tamird approved these changes Jul 30, 2023
View reviewed changes
test/integration-test/src/utils.rs Outdated Show resolved Hide resolved
test/integration-test/src/utils.rs Outdated Show resolved Hide resolved
test/integration-test/src/utils.rs Outdated Show resolved Hide resolved
@Tuetuopay
Copy link
Contributor Author

@tamird last comments taken into account. I squashed all commits in a single one (it ends up pretty short, to not pollute the log with useless commits) and rebased on main.

Thanks for the swift reviews!

@Tuetuopay
test: add the possibility to run a test inside a network namespace
c74813f 
For tests that do networking operations, this allows to have a
clean-state network namespace and interfaces for each test. Mainly, this
avoids "device or resource busy" errors when reusing the loopback
interface across tests.
@tamird
Copy link
Member

tamird commented Jul 30, 2023

@alessandrod PTAL since this technically affects public API.

@tamird tamird added the api/needs-review Makes an API change that needs review label Jul 31, 2023
@mergify mergify bot added aya This is about aya (userspace) test A PR that improves test cases or CI labels Jul 31, 2023
@tamird
Copy link
Member

tamird commented Jul 31, 2023

Per discord discussion we don't need API review here.

@tamird tamird merged commit f705eab into aya-rs:main Jul 31, 2023
21 checks passed
@tamird tamird removed the api/needs-review Makes an API change that needs review label Jul 31, 2023
@Tuetuopay Tuetuopay deleted the tests-netns branch July 31, 2023 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tamird tamird tamird approved these changes

@dave-tucker dave-tucker Awaiting requested review from dave-tucker

Assignees
No one assigned
Labels
aya This is about aya (userspace) test A PR that improves test cases or CI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Tuetuopay @tamird @dave-tucker