Repository navigation
Conversation
✅ Deploy Preview for urunc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
cmainas
left a comment
There was a problem hiding this comment.
Hello @yug105 , thank you for this. I have added some inline comments but some more generic:
- The block detection should not take place in the create step but in start from the reexec process / monitor process, after pivot and namespaces joining.
- Please us the correct filesystem type (e.g. ext2) and not just ext.
|
|
||
| ### Running without `urunit` | ||
|
|
||
| When the container command is non-empty, its first token is executed as init |
There was a problem hiding this comment.
The container command can not be empty. Most tools will reject such an image.
| exactly as written, with no `PATH` lookup. The path must not contain `:`, | ||
| because the FreeBSD kernel splits `init_path` on colons. The kernel's default | ||
| init search list (`/sbin/init:/sbin/oinit:/sbin/init.bak:/rescue/init`) applies | ||
| only when the command is empty. |
There was a problem hiding this comment.
"the command is empty" - > "init is not specified".
| return "", err | ||
| } | ||
| if n == len(magic) && bytes.Equal(buf, magic) { | ||
| return "ufs", nil |
There was a problem hiding this comment.
The fallback is a FreeBSD specific fiesystem which can not be recognized from nay other guest.
| } { | ||
| buf := make([]byte, len(magic)) | ||
| n, err := f.ReadAt(buf, location+1372) | ||
| if err != nil && err != io.EOF && err != io.ErrUnexpectedEOF { |
There was a problem hiding this comment.
Use the errors package to check the returned errors and not == or !=
| if err != nil { | ||
| uniklog.Warnf("failed to detect filesystem type for explicit block %q: %v", | ||
| resolvedPath, err) | ||
| return result, true |
There was a problem hiding this comment.
The changes in the docs state "If the filesystem cannot be recognized or is unsupported, urunc reports an error.", but here we just log it.
|
|
||
| superblockLocations := []int64{8192, 65536, 262144, 0} | ||
|
|
||
| for _, location := range superblockLocations { |
There was a problem hiding this comment.
nit: This algorithm might report a wrong fs type. An ext4 block image setting UFS1 magic bytes at offset 66908 or 263516 is detected as ufs. Use a kernel implementation (e.g. from FreeBSD or Linux for the detection.
| return "", err | ||
| } | ||
| if n == 2 && bytes.Equal(buf, []byte{0x53, 0xEF}) { | ||
| return "ext2", nil |
There was a problem hiding this comment.
We fall back to ext2 even for filesystems that are ext3/ext4.
| expectedFsType: "ext2", | ||
| }, | ||
| { | ||
| name: "missing image", |
There was a problem hiding this comment.
This does not sound correct. A missing image should not be accepted.
| want: "ufs", | ||
| }, | ||
| { | ||
| name: "UFS2 at 8192 (makefs layout)", |
There was a problem hiding this comment.
It seems there is a mismatch here between FreeBSD kernel and makefs. The FreeBSD kernel expects the UFS2 superblock at 65536. I do not think it is a good idea to add a test for a case that the kernel does not accept.
urunc always passed ufs in vfs.root.mountfrom for a block image inside the container image, so an ext2 image failed to mount as root. Detect the filesystem in FreeBSD's Init, which runs in the start step after the pivot, for both the reexec and the monitor process. UFS follows the superblock locations and a subset of the checks of the FreeBSD kernel. ext2, ext3 and ext4 are told apart by their superblock features, as in util-linux. Only ufs and ext2 are accepted. A missing image, an unrecognized filesystem or an unsupported one is reported as an error. Fixes: urunc-dev#1108 Signed-off-by: yug <yugagarwal105@gmail.com>
The docs described an init path list and an /sbin/init fallback that urunc does not implement. Explain that the first token of the container command is the init, what the guest does not get without urunit, and how urunc detects the filesystem of a block rootfs. Fixes: urunc-dev#1108 Signed-off-by: yug <yugagarwal105@gmail.com>
Signed-off-by: yug <yugagarwal105@gmail.com>
31ac257 to
bced725
Compare
|
Changes based on your review:
A few things I'd like your guidance on:
The UFS2-at-8192 test currently verifies that the image is rejected. Let me know if you'd prefer a different expectation or want me to remove this test. Thanks again for the review. I'd appreciate your thoughts on these points before I finalize the implementation. |
Description
The FreeBSD docs described an
init_pathlist and a/sbin/initfallback that urunc doesn't implement. This PR corrects the docs and fixes block rootfs images being treated as UFS, which caused ext2 images to fail during boot. It detects the filesystem from the superblock, supports UFS1/UFS2 and ext2/ext3/ext4, and uses mountinfo for devmapper. Unknown or unsupported filesystems now return an error instead of defaulting to UFS.Related issues
Fixes #1108
How was this tested?
Unit tests: Covered UFS1/UFS2, ext2, and invalid or missing files.
Boot tests: Tested with QEMU and Firecracker. Ext2 now boots successfully, and UFS continues to work.
Before the fix (upstream
main):After the fix:
End-to-end tests: All four tools (ctr, nerdctl, crictl, docker) passed in default and libcontainer modes, with zero failures across 157 test runs.
Lint: No new issues; the two existing
duplfindings also occur on upstreammain.Performance: Adds approximately 0.06 ms to warm and 0.5 ms to cold container creation; startup time is unaffected.
Lab runs: [Full e2e suite](https://github.com/yug105/urunc-freebsd-lab/actions/runs/39581183764)
· [Filesystem and boot tests](https://github.com/yug105/urunc-freebsd-lab/actions/runs/39576957261)
LLM usage
Claude Opus 5.5 helped me understand the codebase.
Checklist
make lint). No new issues; the twoduplfindings also occur on upstreammain.