Fix the size of a virtio device, backed by a block device #1371

kzys · 2019-11-05T21:55:33Z

Reason for This PR

Description of Changes

When a virtio device is backed by a block device, the actual size of the block device file doesn't represent the size as a virtio device.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

PR Checklist

[Author TODO: Meet these criteria. Where there are two options, keep one.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]

All commits in this PR are signed (git commit -s).
Either this PR is linked to an issue, or, the reason for this PR is
clearly provided.
The description of changes is clear and encompassing.
Either no docs need to be updated as part of this PR, or, the required
doc changes are included in this PR. Docs in scope are all *.md files
located either in the repository root, or in the docs/ directory.
Either no code has been touched, or, code-level documentation for touched
code is included in this PR.
Either no API changes are included in this PR, or, the API changes are
reflected in firecracker/swagger.yaml.
Either the changes in this PR have no user impact, or, the changes in
this PR have user impact and have been added to the CHANGELOG.md file.
Either no new unsafe code has been added, or, the newly added unsafe
code is unavoidable and properly documented.

kzys · 2019-11-05T22:04:00Z

What would be a good way to test Ok case? I don't want to hard-code device files such as /dev/nvme8n1p1 here.

iulianbarbu

Thanks for your PR!

As a high-level comment for the way you introduced your bug fix, I would suggest to refactor a little bit with the following considerations:

IMO BLKGETSIZE64 ioctl definition should reside in other place than vmm crate, in the idea that we can place the definition better inside the code base.
If we take a look at net_util crate, inside tap.rs, there are definitions for some ioctls related to TAP.
It would make sense to add a similiar crate, but it does not worth while we have just one ioctl definition, and no code logic or whatsoever.

I would suggest moving the BLKGETSIZE64 ioctl definition into a new file (e.g. block.rs), inside sys_util crate. There is the place for the unit test also (NOT_OK test).

As a final note, it would be really helpful if you can give us any heads up when you decide to provide a PR for an issue, in order to not overlap with the work of someone else who is assigned to that issue. I've just wrote the solution for this issue yesterday, and I was delaying the PR because of the integration tests (OK, NOT_OK), but while we're at it, we can move forward with this PR.

vmm/src/block_device.rs

iulianbarbu · 2019-11-05T22:50:55Z

vmm/src/block_device.rs

+    use std::path::Path;
+
+    #[test]
+    fn test_invalid_file() {


For an OK assert test I would make an integration test.

You can find here: https://github.com/firecracker-microvm/firecracker/blob/master/tests/integration_tests/functional/test_drives.py#L14 a test which does block device rescan for a file backed virtio block device.

I thought about how we should make the OK test before you commited this PR, and I came up with renaming the test from the link to express that it does rescan for a file backed virtio block device and add another similar test which will make rescan for a loop block device backed viritio block device.

The new test should:

Create a loop device which will expose the same file from the test from the link as a block device.

Start Firecracker with this loop device backed virtio block device inserted.

Truncate the file.

Update loop device size on host.

Issue a BlockDeviceRescan to Firecracker, to update the virtio block device capacity accordingly.

Check the virtio block device size, to make sure the size is updated.

What do you think?

Thanks for the guidance. I basically followed your suggestion. Please take a look 706584d.

kzys · 2019-11-06T03:59:05Z

Sorry for taking the issue without let you know beforehand. The issue has been opened for a few weeks and I thought you were working on something else.

I have made separate commits to make review easier. But it would be better to squash all of them into one or two (877f2f0 + other commits) before merging. What do you think?

dhrgit · 2019-11-06T14:45:10Z

vmm/src/lib.rs

+                        .map_err(|_| DriveError::BlockDeviceUpdateFailed)?
+                } else {
+                    metadata.len()
+                };


How about using the fseek(SEEK_END) / ftell() method for getting the file or block dev size? We wouldn't need to seccomp-whitelist another ioctl, and IMO the code would be simpler.

I didn't know that but it works! Would it be better to update the file-backed device path as well? Since metadata() wouldn't need open(), I'd like to only update block.rs to use fseek (std::io::Seek in Rust).

Addressed on 14ae545

This method should work on both files and block devices. With that in mind, do we really need an extra branch in the code to check if the source is a regular file or a block device and an extra function in sys_util?

metadata.len() would be faster...? But yeah, I'm fine inclining open + seek here.

dhrgit · 2019-11-06T14:46:20Z

tests/integration_tests/functional/test_drives.py

+    """Verify that rescan works with a device-backed virtio device."""
+    test_microvm = test_microvm_with_ssh
+    # To access a block device, seccomp_level = 2 is too restrictive
+    test_microvm.jailer.seccomp_level = 1


Tests should always be run with seccomp_level = 2 (i.e. the most restrictive whitelist), since this is the recommended way to run Firecracker in production.

I see. The line is not needed anymore due to 14ae545.

iulianbarbu

I am OK with how it looks now. Please consider the comment from the end of the test_rescan_dev, if it would make sense. Basically, I saw that there is a loop device left, which points to a Firecracker session test file (f2 filesystem), which was deleted when test has finished.

tests/integration_tests/functional/test_drives.py

kzys · 2019-11-06T18:19:11Z

Thanks @iulianbarbu. Will rebase the branch once CI is being green.

iulianbarbu

I forgot to mention that although the changes look good to me, in the end, these changes should be structured in commits that do not overlap, ideally. I noticed that changes from the first commit are dissmised by changes from the second commit, and so on. The commits should contain a title which ideally will reflect briefly the crate/area where the changea reside (e.g sys_util: added get_blk_dev_size), and a description, with respect to 50/70 rule. Also, every commit should build without compile errors, and it would be helpful if you can make sure that style test is succesful, which will remove the need for other commits which fix style.

kzys · 2019-11-06T18:58:13Z

Hmm, not so sure about .losetup: /tmp/firecracker_test_session/c0b6bda0-67c2-4223-b003-8b2dae8db548/fsfiles/fs2.ext4: failed to set up loop device: No such file or directory

iulianbarbu · 2019-11-06T19:59:54Z

@kzys , this is just a guess: I suspect that the machine for x86_64 CI is shared by multiple PRs. In this case, running this PR previously on the machine resulted in the creation of a loop device which is now conflicting in some way with a rerun of the integration test. I saw this happening on my local machine, while I was running multiple times test_rescan_dev, before adding the cleanup for the loop device from the host. Maybe you should trigger agian the CI with an idempotent change over the last commit, followed by a push -f, and see the results after.

kzys · 2019-11-06T22:35:46Z

I think the x86 CI host is not having enough loopback devices. Can you check the configuration? You might also need modprobe loop on the host.

kzys · 2019-11-07T17:37:04Z

Synced-up with @aghecenco. The last commit skips the integration test on hosts that don't have loopback devices (incl. the x86_64 CI host). Hope that works...

tests/integration_tests/functional/test_drives.py

kzys · 2019-11-12T19:58:51Z

Seems like the aarch64 failure is coming from #1381, not this PR.

alxiord · 2019-11-13T07:00:16Z

Seems like the aarch64 failure is coming from #1381, not this PR.

The aarch64 check is not required to merge this PR (or others); it’s currently failing due to an unrelated issue and will not be mandatory until that’s fixed. The buildkite-aarch64 check validates each PR on Arm too, so nothing’s broken here.

alxiord · 2019-11-14T13:18:12Z

@kzys the coverage dropped a bit after merging latest upstream changes - can you add a new commit that lowers COVERAGE_TARGET_PCT in test_coverage.py to 84.9% so we can merge your PR?

Hard-linking from /dev/loopN to a jailed directory doesn't work. Signed-off-by: Kazuyoshi Kato <[email protected]>

When a virtio device is backed by a block device, the actual size of the block device file doesn't represent the size as a virtio device. Fixes firecracker-microvm#1316 Signed-off-by: Kazuyoshi Kato <[email protected]>

kzys · 2019-11-14T20:54:24Z

@aghecenco Sure. Rebased and now looks fine.

Instead of calling blockdev(1), this change uses os.File#Seek which would be more effecient. firecracker-microvm/firecracker#1371 Signed-off-by: Kazuyoshi Kato <[email protected]>

kzys force-pushed the fix-1316 branch from a76548f to 551f581 Compare November 5, 2019 21:55

iulianbarbu suggested changes Nov 5, 2019

View reviewed changes

kzys force-pushed the fix-1316 branch from 973a760 to b88f89e Compare November 5, 2019 23:56

dhrgit reviewed Nov 6, 2019

View reviewed changes

sandreim assigned kzys Nov 6, 2019

iulianbarbu reviewed Nov 6, 2019

View reviewed changes

tests/integration_tests/functional/test_drives.py Show resolved Hide resolved

iulianbarbu previously approved these changes Nov 6, 2019

View reviewed changes

kzys dismissed iulianbarbu’s stale review via 51fcf6f November 6, 2019 18:18

iulianbarbu suggested changes Nov 6, 2019

View reviewed changes

kzys force-pushed the fix-1316 branch from 51fcf6f to 22ef221 Compare November 6, 2019 19:15

kzys force-pushed the fix-1316 branch from 35d7314 to 2d8dd18 Compare November 6, 2019 21:30

kzys force-pushed the fix-1316 branch from 4ad0a36 to 2a7679b Compare November 7, 2019 17:29

kzys force-pushed the fix-1316 branch 5 times, most recently from 4795ebb to d8ad616 Compare November 7, 2019 19:24

iulianbarbu reviewed Nov 12, 2019

View reviewed changes

tests/integration_tests/functional/test_drives.py Outdated Show resolved Hide resolved

iulianbarbu reviewed Nov 12, 2019

View reviewed changes

tests/integration_tests/functional/test_drives.py Show resolved Hide resolved

iulianbarbu reviewed Nov 12, 2019

View reviewed changes

tests/integration_tests/functional/test_drives.py Outdated Show resolved Hide resolved

kzys force-pushed the fix-1316 branch from 2e8a813 to 78b8662 Compare November 12, 2019 19:38

kzys force-pushed the fix-1316 branch from 78b8662 to 6a3ef2a Compare November 12, 2019 19:59

iulianbarbu previously approved these changes Nov 13, 2019

View reviewed changes

alxiord previously approved these changes Nov 14, 2019

View reviewed changes

alxiord mentioned this pull request Nov 14, 2019

fix: change logger options from Value to Vec<LogOption> #1376

Merged

8 tasks

kzys added 2 commits November 14, 2019 12:33

tests: support special block devices

49320ac

Hard-linking from /dev/loopN to a jailed directory doesn't work. Signed-off-by: Kazuyoshi Kato <[email protected]>

vmm: fix the size of a virtio device, backed by a block device

32c63f1

When a virtio device is backed by a block device, the actual size of the block device file doesn't represent the size as a virtio device. Fixes firecracker-microvm#1316 Signed-off-by: Kazuyoshi Kato <[email protected]>

kzys dismissed stale reviews from alxiord and iulianbarbu via 32c63f1 November 14, 2019 20:33

kzys force-pushed the fix-1316 branch from 0945a59 to 32c63f1 Compare November 14, 2019 20:33

iulianbarbu approved these changes Nov 15, 2019

View reviewed changes

alxiord approved these changes Nov 15, 2019

View reviewed changes

alxiord merged commit 87eb6ea into firecracker-microvm:master Nov 15, 2019

kzys mentioned this pull request Nov 15, 2019

Consider running our integ tests as part of Firecracker CI firecracker-microvm/firecracker-containerd#228

Open

andreeaflorescu mentioned this pull request Dec 7, 2019

CI skips test_drives.py::test_rescan_dev #1454

Closed

kzys mentioned this pull request Mar 25, 2021

Use os.File#Seek() to get the size of a block device containerd/containerd#5270

Merged

Fix the size of a virtio device, backed by a block device #1371

Fix the size of a virtio device, backed by a block device #1371

Uh oh!

Conversation

kzys commented Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reason for This PR

Description of Changes

License Acceptance

PR Checklist

Uh oh!

kzys commented Nov 5, 2019

Uh oh!

iulianbarbu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

iulianbarbu Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kzys Nov 6, 2019

Choose a reason for hiding this comment

Uh oh!

kzys commented Nov 6, 2019

Uh oh!

dhrgit Nov 6, 2019

Choose a reason for hiding this comment

Uh oh!

kzys Nov 6, 2019

Choose a reason for hiding this comment

Uh oh!

kzys Nov 6, 2019

Choose a reason for hiding this comment

Uh oh!

dhrgit Nov 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kzys Nov 14, 2019

Choose a reason for hiding this comment

Uh oh!

dhrgit Nov 6, 2019

Choose a reason for hiding this comment

Uh oh!

kzys Nov 6, 2019

Choose a reason for hiding this comment

Uh oh!

iulianbarbu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kzys commented Nov 6, 2019

Uh oh!

iulianbarbu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kzys commented Nov 6, 2019

Uh oh!

iulianbarbu commented Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kzys commented Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kzys commented Nov 7, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kzys commented Nov 12, 2019

Uh oh!

alxiord commented Nov 13, 2019

Uh oh!

alxiord commented Nov 14, 2019

Uh oh!

kzys commented Nov 14, 2019

kzys commented Nov 5, 2019 •

edited

Loading

iulianbarbu left a comment •

edited

Loading

iulianbarbu Nov 5, 2019 •

edited

Loading

dhrgit Nov 14, 2019 •

edited

Loading

iulianbarbu left a comment •

edited

Loading

iulianbarbu commented Nov 6, 2019 •

edited

Loading

kzys commented Nov 6, 2019 •

edited

Loading