Check min img size #1120

ricab · 2019-10-08T16:57:42Z

Check that the disk size requested on launch is not below the virtual size of the corresponding image. Fixes #701.

Add newline to error messages of external processes. Matching existing format in other cases.

Check that the disk size requested on launch is not below the virtual size of the corresponding image. Fixes #701.

codecov · 2019-10-08T17:24:11Z

Codecov Report

Merging #1120 into master will increase coverage by 0.34%.
The diff coverage is 82.6%.

@@            Coverage Diff             @@
##           master    #1120      +/-   ##
==========================================
+ Coverage    70.9%   71.24%   +0.34%     
==========================================
  Files         200      201       +1     
  Lines        7327     7391      +64     
==========================================
+ Hits         5195     5266      +71     
+ Misses       2132     2125       -7

Impacted Files	Coverage Δ
...rc/platform/backends/qemu/qemu_virtual_machine.cpp	`63.96% <0%> (ø)`	⬆️
...c/platform/backends/shared/linux/backend_utils.cpp	`66.66% <86.36%> (+17.21%)`	⬆️
include/multipass/vm_image_info.h	`0% <0%> (-100%)`	⬇️
src/network/url_downloader.cpp	`55.26% <0%> (-0.19%)`	⬇️
src/daemon/daemon.cpp	`31.33% <0%> (-0.06%)`	⬇️
include/multipass/url_downloader.h	`0% <0%> (ø)`	⬆️
.../multipass/exceptions/aborted_download_exception.h	`0% <0%> (ø)`
src/daemon/default_vm_image_vault.cpp	`78.86% <0%> (+5.03%)`	⬆️
src/daemon/default_vm_image_vault.h	`100% <0%> (+50%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4cb4b93...b5c3fb8. Read the comment docs.

Saviq · 2019-10-09T12:00:51Z

src/platform/backends/shared/linux/backend_utils.cpp

+                                             qemuimg_process->read_all_standard_error()));
+
+    const auto img_info = QString{qemuimg_process->read_all_standard_output()};
+    const auto capture_name = QStringLiteral("size");


This seems a bit excessive… I don't think repeating "size" would be that big of a deal here, and having to substitute this in the pattern below… Your call.

OK, I will do that.

Bahh I totally forgot about this and just noticed this PR never got merged,

Saviq

Could we unit-test this please?

Saviq · 2019-10-09T12:05:47Z

src/platform/backends/shared/linux/backend_utils.cpp

+            throw std::runtime_error(fmt::format("Requested disk ({} bytes) below minimum for this image ({} bytes)",
+                                                 requested_size.in_bytes(), min_size));


I think we should print this human-readable, we can probably assume GBs here, although now I see MemorySize floors, would have to round up for this use case…

I'd be OK with this with a TODO to improve the message later.

I considered that, but then users might get Requested disk (2.2G) below minimum for this image (2.2G), because of approximations. I used bytes to be precise and avoid that.

So, this would happen when a user wanted to save space:

$ multipass launch --disk=1G Requested disk (1G) below minimum for this image (2.2G) $ multipass launch --disk=2.2G # user trying next-best thing Requested disk (2.2G) below minimum for this image (2.2G)

Well of course, which is why I mentioned we'd need to ceil() the minimum, in which case the minimum would be 2.3G.

I see, I did not get what you meant before. OK then, fine by me. It would be imprecise, but I agree it looks friendlier.

MemorySize does not support decimals ATM, so I will add the TODO for now.

ricab · 2019-10-09T12:23:17Z

Could we unit-test this please?

Sure, I am working on it.

Change process counting strategy to accommodate test repetitions, while hopefully improving clarity.

ricab · 2019-10-11T19:55:29Z

Test are in. I still have to address the other review comments.

Employ it in test checking no resizing when below minimum.

To avoid confusion.

ricab · 2019-10-14T14:23:27Z

CI-detected conflict addressed in canonical/multipass-private#187.

Saviq · 2019-10-16T06:43:20Z

@ricab if you rebase / merge this, the CI failure should go away.

ricab · 2019-10-16T10:04:51Z

Bahh new test failure...

Moves backend-util tests into new scheme + some formatting fixes.

Incidentally makes failing test pass (see #1003)!

ricab · 2019-10-16T11:33:11Z

The failure is just another manifestation of #1003. I tried a shot at changing the order of our sources in cmake and it made it go away! At least in my machine, let's see what CI says.

I kept that change as a separate commit on purpose. In summary, the failure in c5eacaf (merge commit) goes away in f502847. Something that will byte us back again for sure, but not really related to this PR.

multipass-ci-bot · 2019-10-23T18:59:14Z

macOS build available: multipass-0.10.0-dev.468+g422367d3.mac-Darwin.pkg
Snap build available: snap refresh multipass --channel edge/pr1120

ricab · 2019-10-24T10:11:51Z

The conflict in CI is handled in canonical/multipass-private#190

Saviq

Thanks, love the coverage :)

Can you just comment on why you didn't go for parameterized tests here? It seems as if test_image_resizing could really itself be a parameterized test?

bors r+

1120: Check min img size r=Saviq a=ricab Check that the disk size requested on launch is not below the virtual size of the corresponding image. Fixes #701. Co-authored-by: Ricardo Abreu <[email protected]>

bors · 2019-10-29T13:53:45Z

Build failed

continuous-integration/travis-ci/push

1120: Check min img size r=Saviq a=ricab Check that the disk size requested on launch is not below the virtual size of the corresponding image. Fixes #701. Co-authored-by: Ricardo Abreu <[email protected]>

ricab · 2019-10-29T16:01:40Z

Thanks! Yeah, the tests took most of the time.

I considered parameterized tests but ended up deciding against. I forget the exact reasoning at the time, but I think the main reasons were:

test_image_resizing is a template: I remember looking briefly and not finding any obvious way to mix parameterized tests with templates
it would not gain me much: despite the many parameters, there weren't many combinations to test and the automated facilities to produce parameter combinations to test would not have been useful
test names would suffer: output would consist of a repeated name plus a load of parameters, instead of a specific, more descriptive, name for each test

Saviq · 2019-10-29T16:19:23Z

* it would not gain me much: despite the many parameters, there weren't many combinations to test and the automated facilities to produce parameter combinations to test would not have been useful

There would be less const autos though ;). But then, less readable because need to remember argument order, I suppose.

* test names would suffer: output would consist of a repeated name plus a load of parameters, instead of a specific, more descriptive, name for each test

FWIW this one can be solved by providing a naming function when instantiating the suite.

Thanks.

ricab · 2019-10-29T16:59:56Z

There would be less const autos though ;). But then, less readable because need to remember argument order, I suppose.

Yeah they weren't necessary here either, I just wanted to make it easier to read.

FWIW this one can be solved by providing a naming function when instantiating the suite.

Interesting! I overlooked that.

TBH, an important reason was that I had already spent a lot of time on these tests and wanted to move on. But I also did not see much advantage. I would remove a bunch of TEST(...) calls, but they would probably have to be replaced with comments for readability :)

polarapfel · 2020-03-06T10:46:36Z

I tried edge/pr1120 and I am still running into the issue with qemu-img being unable to resize the image and timing out.

townsend2010 · 2020-03-06T10:53:20Z

Hi @polarapfel,

It sounds to me the issue you are seeing is unrelated to this PR. Could you please submit a new issue and put in your reproduction steps?

polarapfel · 2020-03-06T10:56:28Z

Sure thing.

polarapfel · 2020-03-06T11:03:35Z

Hey @townsend2010, it's #1408.

ricab added 2 commits October 8, 2019 12:04

[daemon] Improve error formatting

c2eb0ee

Add newline to error messages of external processes. Matching existing format in other cases.

[daemon] Check requested disk against virtual size

1855e53

Check that the disk size requested on launch is not below the virtual size of the corresponding image. Fixes #701.

Saviq reviewed Oct 9, 2019

View reviewed changes

Saviq requested changes Oct 9, 2019

View reviewed changes

ricab added 11 commits October 9, 2019 13:39

[test] Add skeleton for backend-utils testing

605b7af

[test] Image size/resize success case

2367155

[test] Extract qemu-img info simulation, to reuse

f7f5e9e

[test] Confirm no output asked from unending proc

8b9cbf6

[test] Improve param naming

0f87b25

[test] Simplify branching in testing helper

d0cc8d9

[test] Extract img resize simulation, to reuse

c11ed45

[test] Comment tweak

7c82c5c

[test] Relax expectations on process-output reads

b9c5260

[test] Fix failure on repeated tests

ead48e9

Change process counting strategy to accommodate test repetitions, while hopefully improving clarity.

[test] Improve variable naming

2fae3d3

ricab added 12 commits October 14, 2019 10:00

[test] Test qemu-img info crash

5156bc5

[test] Extract exception testing to util assertion

bf541ba

[test] Add more image resizing tests

67ef83e

[test] Start extracting a common resize test util

d5c9f0f

Employ it in test checking no resizing when below minimum.

[test] Use resize testing helper in another test

60276b4

[test] Extract success process state constant

428c12f

[test] Use helper on more image-resizing tests

be15ada

[test] Test reordering

500c07d

[test] Extract crash process state constant

a83a9e0

[test] Add test for image-resizing crash

e905562

[test] Extract failure process state constant

72f420e

[test] Adapt and use helper on crash test

3a8686e

ricab added 4 commits October 14, 2019 10:02

[test] Complete crash test expectations

24b812d

[test] User helper on img-not-found test

393bc39

[test] Avoid unused success result

bad1cb1

To avoid confusion.

[test] Adapt and use helper in remaining test

7a54647

ricab force-pushed the check-min-img-size branch from 6701946 to 7a54647 Compare October 14, 2019 09:04

ricab added 2 commits October 16, 2019 12:21

Merge branch 'master' into check-min-img-size

c5eacaf

Moves backend-util tests into new scheme + some formatting fixes.

[test] List linux-test sources alphabetically

f502847

Incidentally makes failing test pass (see #1003)!

ricab mentioned this pull request Oct 16, 2019

Tests fail when shuffled #1003

Closed

[linux] Address review comments

b5c3fb8

Saviq approved these changes Oct 29, 2019

View reviewed changes

Saviq merged commit b5c3fb8 into master Oct 29, 2019

bors bot deleted the check-min-img-size branch October 29, 2019 14:17

		throw std::runtime_error(fmt::format("Requested disk ({} bytes) below minimum for this image ({} bytes)",
		requested_size.in_bytes(), min_size));

Check min img size #1120

Check min img size #1120

Uh oh!

Conversation

ricab commented Oct 8, 2019

Uh oh!

codecov bot commented Oct 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Saviq left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ricab commented Oct 9, 2019

Uh oh!

ricab commented Oct 11, 2019

Uh oh!

ricab commented Oct 14, 2019

Uh oh!

Saviq commented Oct 16, 2019

Uh oh!

ricab commented Oct 16, 2019

Uh oh!

ricab commented Oct 16, 2019

Uh oh!

multipass-ci-bot commented Oct 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ricab commented Oct 24, 2019

Uh oh!

Saviq left a comment

Choose a reason for hiding this comment

Uh oh!

bors bot commented Oct 29, 2019

Build failed

Uh oh!

ricab commented Oct 29, 2019

Uh oh!

Saviq commented Oct 29, 2019

Uh oh!

ricab commented Oct 29, 2019

Uh oh!

polarapfel commented Mar 6, 2020

Uh oh!

townsend2010 commented Mar 6, 2020

Uh oh!

polarapfel commented Mar 6, 2020

Uh oh!

polarapfel commented Mar 6, 2020

Uh oh!

Uh oh!

codecov bot commented Oct 8, 2019 •

edited

Loading

multipass-ci-bot commented Oct 23, 2019 •

edited

Loading