Test jailer bindmounts to root #5025
Open
+89
−0
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
|
Hello @anthonycorletti , Thank you for your contribution! I'll take a further look at this PR later, but could you fix styling issues first? As instructed in the PR checklist, you can run the coding style test using
Thanks, |
|
You can squash the last two chore commits into a single commit, because the last one ("chore: fix formatting") reverts the change made in the second last ("chore: update gitignore and fix formatting"). |
anthonycorletti
force-pushed
the
main
branch
4 times, most recently
from
7869e8e to
6940c7f
Compare
|
thanks @zulinx86! I addressed your suggestions and cleaned up the commits/ formatting. it looks like my checkstyle is passing on my dev machine but not CI |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5025 +/- ##
=======================================
Coverage 83.13% 83.13%
=======================================
Files 245 245
Lines 26651 26651
=======================================
Hits 22156 22156
Misses 4495 4495
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
yeah, not on my dev machine as well. it looks your commit has a long commit title ( 6940c7f ). Another nitpicking is not to use a title like "test closes #PR_ID". Keeping the commit title and message self-contained as much as possible will help future readers :) |
anthonycorletti
force-pushed
the
main
branch
from
6940c7f to
f99defc
Compare
zulinx86
requested changes
Feb 12, 2025
There was a problem hiding this comment.
It looks what you implemented is different from what we want to check, because your test is just checking file existence from outside jail.
If I would reproduce in shell script what you test, it looks like:
# copying kernel and rootfs
$ echo KERNEL >kernel
$ echo ROOTFS >rootfs
# touching kernel and rootfs inside the jail to be used as mount points
$ sudo touch jail/kernel
$ sudo touch jail/rootfs
# checking the above files are created
$ ls jail/kernel
jail/kernel
$ ls jail/rootfs
jail/rootfs
# bind-mounting to the mount points
$ sudo mount --bind kernel jail/kernel
$ sudo mount --bind rootfs jail/rootfs
# checking the file existence from outside jail
$ ls jail/kernel
jail/kernel
$ ls jail/rootfs
jail/rootfs
# start microvm
# checking the file existence from outside jail
$ ls jail/kernel
jail/kernel
$ ls jail/rootfs
jail/rootfs
What we actually want to test is to check that contents of bind-mounted kernel and rootfs are propagated corrrectly inside the jail. So it's like:
# copying kernel and rootfs
$ echo KERNEL >kernel
$ echo ROOTFS >rootfs
# touching kernel and rootfs inside the jail to be used as mount points
$ sudo touch jail/kernel
$ sudo touch jail/rootfs
# bind-mounting to the mount points
$ sudo mount --bind kernel jail/kernel
$ sudo mount --bind rootfs jail/rootfs
# start microvm
# HERE, mock the jailer and this shell is inside the jail.
$ sudo unshare --mount --propagation unchanged
$ mount --make-rslave /
$ mount --rbind jail jail
$ cd jail/
# Just for simplicity, not pivot_root here.
# contents check
$ cat kernel
KERNEL
$ cat rootfs
ROOTFS
If it were the previous jailer that doesn't support bind-mounts, the result would be that the contents are not propagated properly as follows:
$ echo KERNEL >kernel
$ echo ROOTFS >rootfs
$ sudo touch jail/kernel
$ sudo touch jail/rootfs
$ sudo mount --bind kernel jail/kernel
$ sudo mount --bind rootfs jail/rootfs
# start microvm
# HERE mock preivous jailer (not supporting recursive bind-mounts)
$ sudo unshare --mount --propagation unchanged
# "MS_PRIVATE | MS_REC" instead of "MS_SLAVE | MS_REC"
$ mount --make-rprivate /
# "MS_BIND" instead of "MS_BIND | MS_REC"
$ mount --bind jail jail
$ cd jail/
# Just for simplicity, not pivot_root here.
# the contents of the kernel and rootfs are empty
$ cat kernel
$ cat rootfs
One of my dumb proposals is like:
- Make the default
test_microvm.kernel_fileandtest_microvm.rootfs_fileempty to make the microvm fail to boot if the contents of bind-mounted kernel and rootfs are not propagated correctly. - Bind-mount new kernel and rootfs onto the paths of
test_microvm.kernel_fileandtest_microvm.rootfs_file. - Test the guest inside the microvm boots and responds to remote command execution from host.
This might not work since I don't test it on my side, so please fix as you think best.
It would be super great if you could come up with a better idea!!
Comment on lines
673
to
674
| This is a test for | ||
| https://github.com/firecracker-microvm/firecracker/pull/#1093 |
There was a problem hiding this comment.
Let's make the comment self-contained and not refer to a PR.
.gitignore
Outdated
| @@ -15,3 +15,4 @@ test_results/* | |||
| /resources/linux | |||
| /resources/x86_64 | |||
| /resources/aarch64 | |||
| .venv | |||
There was a problem hiding this comment.
Is this for running the python test on your own virtual env? If so, may I ask why you can't use tools/devtool test?
anthonycorletti
force-pushed
the
main
branch
2 times, most recently
from
e717ef4 to
89c9346
Compare
this test creates two mounts for a rootfs and guest kernel in directories that are mounted in a location that the jailer requires to boot successfully Signed-off-by: Anthony Corletti <[email protected]>
anthonycorletti
force-pushed
the
main
branch
from
89c9346 to
39fedf1
Compare
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.
Changes
Reason
Closes #1099
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.PR Checklist
tools/devtool checkstyleto verify that the PR passes theautomated style checks.
how they are solving the problem in a clear and encompassing way.
in the PR.
CHANGELOG.md.Runbook for Firecracker API changes.
integration tests.
TODO.rust-vmm.