fix(network): close tap device fd after creation#831
Conversation
✅ Deploy Preview for urunc canceled.
|
bf9b5f5 to
1e77783
Compare
|
hi @cmainas , as discussed, i opened a separate issue and PR for the tap device fd leak:
one thing i want to flag honestly about the PR: i could not get the e2e tests to pass locally, but i verified this is not caused by my change. i ran the actual failure is:
this also reproduces outside the test suite with a plain nerdctl run of harbor.nbfc.io/nubificus/urunc/chttp-firecracker-linux-aarch64:latest on unchanged main. the same image boots fine on the older main i was previously based on (8928c4c), so something between 8928c4c and 609e07e changed the behavior. so on the PR: build, unit tests and lint all pass, and the fix itself was verified in the API-based boot prototype where it actually manifests (EBUSY on the tap device, fixed after the change). i left the e2e checkbox unticked and explained this in the PR description. |
cmainas
left a comment
cmainas
left a comment
There was a problem hiding this comment.
Hello @Anamika1608 ,
thank you for the PR, I have added one comment. Also, please add yourself in https://github.com/urunc-dev/urunc/blob/main/.github/contributors.yaml
From the logs it seems that runc is used instead of urunc. Can you verify the version of urunc with |
1e77783 to
bda6460
Compare
yeah, you were right it was because of the stale vendor file i had, i deleted that one and generated the new one, and it passes completely now. |
cmainas
left a comment
cmainas
left a comment
There was a problem hiding this comment.
Thank you @Anamika1608 for the changes,
we still need to check for errors even in the failure path and update the error message we return or at least log the closing fd error.
|
Hello @Anamika1608 , the changes look good, but please follow the correct structure of the PR template. |
yes, i have updated the pr description. |
Great, let's squash the commits too and as soon as all checks complete we can merge. |
createTapDevice relies on a later syscall.Exec into the monitor to
close the tap device fd that netlink.LinkAdd opened (O_CLOEXEC). In any
flow where urunc keeps running instead of exec-ing away, the fd stays
open and, since the tap device is single-queue, blocks the monitor from
attaching to it ("Resource busy").
Close the fd explicitly after the owner/group ioctls (handling the
close error), and add the author to contributors.
Signed-off-by: Anamika Aggarwal <anamikaagg18@gmail.com>
61fa441 to
185f2a1
Compare
done, i have merged all in one commit |
2 participants
Description
createTapDevicerelies on a latersyscall.Execinto the monitor to close the tap device fd thatnetlink.LinkAddopened (O_CLOEXEC). In any flow where urunc keeps running instead of exec-ing away, the fd stays open for as long as urunc runs, and since the tap device is single-queue, it blocks the monitor from attaching to the device ("Resource busy").Close the fd explicitly right after the owner/group ioctls. The device is created with
TUNSETPERSIST, so it survives the close.Related issues
How was this tested?
go build ./...andgo test ./internal/... ./pkg/...pass (Ubuntu 24.04 aarch64 VM with KVM, Firecracker v1.7.0).make lintreports no findings in the changed file./network-interfacescall fails with EBUSY; with this change, the guest boots and serves traffic.main.make test_nerdctlis not runnable in my environment (aarch64 VM with only Firecracker installed; the suite needs docker, all five monitors, devmapper, and amd64 test images), so the e2e checkbox is left unticked.LLM usage
claude code (opus 4.8) for the understanding of codebase, approach decisions and reviews
Checklist
make lint).make test_ctr,make test_nerdctl,make test_docker,make test_crictl).