Add check for available disk space on launch #1854

katie-knister · 2020-11-25T03:00:01Z

To address #938 I used the std::filesystem library to check for available space in the user’s directory.

Saviq · 2020-11-25T08:34:21Z

Hi @katie-knister, unfortunately to be able to support older systems (like earlier macOS versions), we can't be on the edge of C++, where <filesystem> got introduced.

For our cross-platform needs, we decided on Qt to abstract the underlying system where possible, and QStorageInfo::bytesAvailable should do for this use case.

Saviq · 2020-11-25T08:40:22Z

@katie-knister I also had a look at your code and saw that if the user did not pass a disk size, the default would be used, and that wouldn't be checked for available space.

A better place to put this check would be here:

multipass/src/daemon/daemon.cpp

Lines 609 to 632 in 88618b0

    
           // Computes the final size of an image, but also checks if the value given by the user is bigger than or equal than 
        
           // the size of the image. 
        
           mp::MemorySize compute_final_image_size(const mp::MemorySize image_size, 
        
                                                   mp::optional<mp::MemorySize> command_line_value) 
        
           { 
        
               mp::MemorySize disk_space{}; 
        
               if (!command_line_value) 
        
               { 
        
                   auto default_disk_size_as_struct = mp::MemorySize(mp::default_disk_size); 
        
                   disk_space = image_size < default_disk_size_as_struct ? default_disk_size_as_struct : image_size; 
        
               } 
        
               else if (*command_line_value < image_size) 
        
               { 
        
                   throw std::runtime_error(fmt::format("Requested disk ({} bytes) below minimum for this image ({} bytes)", 
        
                                                        command_line_value->in_bytes(), image_size.in_bytes())); 
        
               } 
        
               else 
        
               { 
        
                   disk_space = *command_line_value; 
        
               } 
        
               return disk_space; 
        
           }

…efault disk sizes as well as user-defined disk sizes. Make check use <QStorageInfo> instead of <filesystem> for increased compatability.

katie-knister · 2020-11-28T01:49:34Z

Hi @Saviq, thanks for the suggestions! I have updated the PR to change where the check is done and use QStorageInfo instead. The build now succeeds and passes 1238/1239 tests in BuildAndTest (Coverage). It fails DNSMasqServer.dnsmasq_fails_and_throws, where a runtime_error is expected but nothing is actually thrown. This is weird because the test seems unrelated to what I changed. Since my change is pretty straightforward and doesn’t involve suppressing any exceptions, I’m unsure what could have caused this, and am wondering if you have any ideas of how to resolve/debug this.

ricab · 2020-11-30T10:10:38Z

Hi @katie-knister , yeah this test keeps coming back to bite us, I don't think it's related to your changes. The test is dependent on a timeout that is apparently still not enough... Thanks for your pull request!

Saviq · 2020-11-30T10:23:42Z

bors try

bors · 2020-11-30T12:09:16Z

try

Build succeeded:

ricab

Hi @katie-knister, thanks again for the PR 🙂

I think your approach is good overall. Using MemorySize for comparison and throwing runtime_error makes sense to me, clear error message too. Unfortunately, locations and snap confinement make it a bit trickier, so I don't think this will work just yet. Details inline.

ricab · 2020-11-30T17:07:46Z

src/daemon/daemon.cpp

        disk_space = *command_line_value;
    }

+    auto available_bytes = QStorageInfo::root().bytesAvailable();


I see two issues with this:

we may be storing images elsewhere, either because multipass is installed in a different filesystem or because MULTIPASS_STORAGE was used

this does not currently work under snap confinement; you can try it yourself by using the PR channel which is built in CI: multipass install/refresh channel=edge/pr1854 multipass; but we're looking for a way to overcome this and will write back when we find it

You also need to cater for QStorageInfo::bytesAvailable returning -1 in case of error.

@ricab @Saviq Thank you for reviewing this! I changed from checking the root's available space to using DaemonConfig::data_directory as @Saviq suggested. I also added a check for if QStorageInfo::bytesAvailable returns -1. The "BuildAndTest" checks don't seem to be running (nothing has updated after over an hour), so I thought I should just go ahead and let you know about these updates to see what you think.

katie-knister · 2020-12-01T03:58:28Z

@ricab Thank you for your review! It sounds like this is blocked for now (if that is not the case, please let me know). I plan to get started on a different “good first issue” in the meantime. Please let me know if this issue becomes unblocked or if you have any recommendations for any other “good first issues” that could be particularly helpful to finish now.

Saviq · 2020-12-01T07:47:37Z

Hi @ricab @katie-knister, I just tried this with --devmode, meaning confinement is disabled for the snap, and the issue persists.

I think in reality, the problem is what @ricab highlighted - you need to check the space where Multipass will actually be writing the image, not on the root filesystem, which, in case of running as a snap, is a read-only mount of the core18 base snap:

 snap run --shell multipass -c "df /"
Filesystem     1K-blocks  Used Available Use% Mounted on
/dev/loop15        56704 56704         0 100% /

So indeed when you check for the root's available space, it's going to be 0.

As a pointer, you likely want to use the QStorageInfo(const QDir &) variant, pointing at DaemonConfig::data_directory:

multipass/src/daemon/daemon_config.h

Line 59 in bc0d9fd

const multipass::Path data_directory;

…dd error check in case call to bytesAvailable fails

ricab · 2020-12-02T12:24:49Z

bors try

bors · 2020-12-02T12:40:18Z

try

Merge conflict.

ricab

Hi @katie-knister, there seems to be a merge conflict now, so we don't get the snap and I couldn't test yet, but the code looks good to me.

One further thing I'd like to request is for tests to the new functionality you're adding (we try to add only tested code as much as possible). Tests for this should probably go around here.

You can't easily mock Qt stuff, but you wouldn't have to: you could check the available space in the test itself, then request just a bit more than that and confirm that the daemon throws the new runtime_error. This may be useful for that. One other test would be the -1 case (you'd have to overwrite the data_directory that is set here with something bogus).

src/daemon/daemon.cpp

codecov · 2020-12-05T20:05:57Z

Codecov Report

Merging #1854 (24bdff5) into master (1f96ef7) will increase coverage by 0.12%.
The diff coverage is 40.00%.

@@            Coverage Diff             @@
##           master    #1854      +/-   ##
==========================================
+ Coverage   77.70%   77.83%   +0.12%     
==========================================
  Files         234      234              
  Lines        8743     8766      +23     
==========================================
+ Hits         6794     6823      +29     
+ Misses       1949     1943       -6

Impacted Files	Coverage Δ
src/daemon/daemon.cpp	`41.87% <40.00%> (+0.97%)`	⬆️
...rc/platform/backends/qemu/qemu_vm_process_spec.cpp	`100.00% <0.00%> (ø)`
src/client/cli/cmd/list_networks.h
src/client/cli/cmd/list_networks.cpp
src/client/cli/cmd/networks.h	`100.00% <0.00%> (ø)`
src/client/cli/cmd/networks.cpp	`18.18% <0.00%> (ø)`
src/client/cli/argparser.cpp	`94.73% <0.00%> (+0.14%)`	⬆️
...tform/backends/libvirt/libvirt_virtual_machine.cpp	`72.63% <0.00%> (+0.38%)`	⬆️
src/client/cli/cmd/exec.cpp	`75.55% <0.00%> (+1.13%)`	⬆️
... and 2 more

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 1f96ef7...24bdff5. Read the comment docs.

katie-knister · 2020-12-07T06:07:20Z

Hi @ricab, thank you for the information on testing! I resolved the merge conflict and added tests. They are all passing. I added another check to the launches_with_correct_disk_size test in a way that should fit with the current structure. However, I made a new test for the -1 check since it required overwriting the data_directory, which is not something we would want to do for every combination of inputs passed to launches_with_correct_disk_size.

ricab

Hi @katie-knister, thanks for the tests. I'm proposing a few simplifications inline.

You did well to separate the -1 check. Actually, I think the other one should be in a separate test too. The idea is that each test should check one property of the intended behavior.

Let me know if you need any help with it.

ricab · 2020-12-07T15:31:43Z

tests/test_daemon.cpp

+    else if (other_command_line_parameters.size() > 0 &&
+             mp::MemorySize(other_command_line_parameters[1]) > mp::MemorySize(available_bytes_str + "B"))
+    {
+        std::stringstream stream;
+        EXPECT_CALL(*mock_factory, create_virtual_machine(_, _)).Times(0);
+        send_command(all_parameters, trash_stream, stream);
+        EXPECT_THAT(stream.str(), AllOf(HasSubstr("Available disk"), HasSubstr("below requested/default size")));
+    }


Hmm, I think this should be in its own test, one where you made sure that the inequality in the if holds (and so didn't need to check it).

Makes sense, I was wondering about that. I switched it to its own test and removed the parts that I added to this test.

ricab · 2020-12-07T16:19:53Z

tests/test_daemon.cpp

+TEST_P(InvalidDataDirectorySuite, launch_fails_with_invalid_data_directory)
+{
+    auto mock_factory = use_a_mock_vm_factory();
+    const auto param = GetParam();
+    const auto& first_command_line_parameter = std::get<0>(param);
+    const auto& other_command_line_parameters = std::get<1>(param);
+    const auto& img_size_str = std::get<2>(param);
+
+    auto mock_image_vault = std::make_unique<NiceMock<mpt::MockVMImageVault>>();
+    ON_CALL(*mock_image_vault.get(), minimum_image_size_for(_)).WillByDefault([&img_size_str](auto...) {
+        return mp::MemorySize{img_size_str};
+    });
+
+    config_builder.vault = std::move(mock_image_vault);
+    config_builder.data_directory = QString("invalid_data_directory");
+    mp::Daemon daemon{config_builder.build()};
+
+    std::vector<std::string> all_parameters{first_command_line_parameter};
+    for (const auto& p : other_command_line_parameters)
+        all_parameters.push_back(p);
+
+    std::stringstream stream;
+    EXPECT_CALL(*mock_factory, create_virtual_machine(_, _)).Times(0);
+    send_command(all_parameters, trash_stream, stream);
+    EXPECT_THAT(stream.str(), HasSubstr("Failed to determine information about the volume containing"));
+}


This test can be a lot simpler than the one you used as inspiration. For instance, you don't need all the parameters, since you use only one value; then you don't need to construct a complex command either; and neither do you need all the mocking. Consider this:

TEST_P(InvalidDataDirectorySuite, launch_fails_with_invalid_data_directory) { auto mock_factory = use_a_mock_vm_factory(); config_builder.data_directory = QString("invalid_data_directory"); mp::Daemon daemon{config_builder.build()}; std::stringstream stream; EXPECT_CALL(*mock_factory, create_virtual_machine(_, _)).Times(0); send_command({GetParam()}, trash_stream, stream); EXPECT_THAT(stream.str(), HasSubstr("Failed to determine information about the volume containing")); }

You'd need to change the param interface to receive only one string. You could then do something very similar for the other test, adding just --disk ... to the command.

Thank you for this information! I was wondering about simplifying this test, but I didn't want to accidentally remove parts that were actually necessary -- so I appreciate you taking the time to discuss how to simplify it. Now I've made the changes you suggested.

ricab · 2020-12-07T16:40:45Z

tests/test_daemon.cpp

                         Combine(Values("test_create", "launch"),
-                                 Values(std::vector<std::string>{}, std::vector<std::string>{"--disk", "4G"}),
+                                 Values(std::vector<std::string>{}, std::vector<std::string>{"--disk", "4G"},
+                                        std::vector<std::string>{"--disk", "9999G"}),


Good point, you can request a very large disk size and assume it's not acceptable. That will allow you to drop the QStorageInfo stuff.

However, 10 terabytes may not be so uncommon in a few years. MemorySize currently uses a long long, which is guaranteed to be at least 64 bits. It's currently signed (something we could change), so we need something very large but below 2^63. Perhaps "999999999G". When 1 exabyte is not enough we need to change the underlying representation anyway 😉

Oh, interesting! Thank you, I've made this change.

…nch_fails_with_invalid_data_directory test

katie-knister · 2020-12-08T06:11:52Z

Hi @ricab, thank you for your feedback on the tests. I just made the changes you suggested.

ricab

Hi @katie-knister, I think there is some confusion regarding the parameterized tests. The principle of your tests is correct, but some parts come from other tests and are not really applicable. I created a draft PR with what I mean because it is simpler and more precise than words.

ricab · 2020-12-10T12:11:46Z

tests/test_daemon.cpp

                                 Values("1G", mp::default_disk_size, "10G")));
+INSTANTIATE_TEST_SUITE_P(Daemon, DiskSpaceRequestedExceedsAvailableSuite,
+                         Combine(Values("test_create", "launch"),
+                                 Values(std::vector<std::string>{"--disk", "999999999G"})));


This is the only value ever used by the test (the vector with the 2 args), so there is no point having it as a parameter.

ricab

OK, this should be ready to merge.

bors merge

1854: Add check for available disk space on launch r=ricab a=katie-knister To address #938 I used the std::filesystem library to check for available space in the user’s directory. Co-authored-by: Katie Knister <[email protected]> Co-authored-by: katie-knister <[email protected]> Co-authored-by: Ricardo Abreu <[email protected]>

bors · 2020-12-14T19:07:56Z

This PR was included in a batch that successfully built, but then failed to merge into master (it was a non-fast-forward update). It will be automatically retried.

bors · 2020-12-14T19:59:29Z

Build succeeded:

This reverts commit 24e4f8b, reversing changes made to b600010.

Add check for available disk space on launch

fb0f1c7

katie-knister force-pushed the fix_#938 branch 2 times, most recently from 714133e to d460bd3 Compare November 25, 2020 03:24

Fix formatting issues for linter

88618b0

katie-knister force-pushed the fix_#938 branch from d460bd3 to 88618b0 Compare November 25, 2020 03:27

katie-knister mentioned this pull request Nov 25, 2020

Check disk space before creating instances #938

Closed

Change location of check for available disk space in order to check d…

cbf4e3e

…efault disk sizes as well as user-defined disk sizes. Make check use <QStorageInfo> instead of <filesystem> for increased compatability.

katie-knister force-pushed the fix_#938 branch from bdafe35 to dec8403 Compare November 26, 2020 06:48

Fix formatting issues for linter

c4406b0

katie-knister force-pushed the fix_#938 branch from dec8403 to c4406b0 Compare November 26, 2020 06:52

dlbeck mentioned this pull request Nov 29, 2020

Suggest -- when exec gets an unrecognised option #531

Closed

bors bot added a commit that referenced this pull request Nov 30, 2020

Try #1854:

5c49165

ricab requested changes Nov 30, 2020

View reviewed changes

Check the space using DaemonConfig::data_directory instead of root; a…

d6140f9

…dd error check in case call to bytesAvailable fails

katie-knister force-pushed the fix_#938 branch from 48105b6 to d6140f9 Compare December 2, 2020 05:07

ricab reviewed Dec 2, 2020

View reviewed changes

src/daemon/daemon.cpp Show resolved Hide resolved

Add tests for checking the disk space available on launch

23a4f2a

katie-knister force-pushed the fix_#938 branch from 5add6ca to 3719323 Compare December 5, 2020 19:49

Merge branch 'master' into fix_#938

23f5425

katie-knister force-pushed the fix_#938 branch from 3719323 to 23f5425 Compare December 5, 2020 19:52

ricab reviewed Dec 7, 2020

View reviewed changes

katie-knister force-pushed the fix_#938 branch from d24aa06 to ac042d9 Compare December 8, 2020 05:03

Separate launches_with_correct_disk_size into two tests; simplify lau…

ded06f7

…nch_fails_with_invalid_data_directory test

katie-knister force-pushed the fix_#938 branch from ac042d9 to ded06f7 Compare December 8, 2020 05:36

[test] Simplify tests for storage space on launch

24bdff5

ricab reviewed Dec 10, 2020

View reviewed changes

ricab approved these changes Dec 14, 2020

View reviewed changes

bors bot merged commit 24e4f8b into canonical:master Dec 14, 2020

Saviq added a commit that referenced this pull request Jan 18, 2021

Revert "Merge #1854"

d762f30

This reverts commit 24e4f8b, reversing changes made to b600010.

Saviq linked an issue Feb 2, 2021 that may be closed by this pull request

[macOS] Can't multipass launch behind CNTLM proxy #1947

Open

Saviq removed a link to an issue Feb 2, 2021

[macOS] Can't multipass launch behind CNTLM proxy #1947

Open

Saviq linked an issue Feb 2, 2021 that may be closed by this pull request

Check disk space before creating instances #938

Closed

Add check for available disk space on launch #1854

Add check for available disk space on launch #1854

Uh oh!

Conversation

katie-knister commented Nov 25, 2020

Uh oh!

Saviq commented Nov 25, 2020

Uh oh!

Saviq commented Nov 25, 2020

Uh oh!

katie-knister commented Nov 28, 2020

Uh oh!

ricab commented Nov 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Saviq commented Nov 30, 2020

Uh oh!

bors bot commented Nov 30, 2020

try

Uh oh!

ricab 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!

katie-knister commented Dec 1, 2020

Uh oh!

Saviq commented Dec 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ricab commented Dec 2, 2020

Uh oh!

bors bot commented Dec 2, 2020

try

Uh oh!

ricab left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Dec 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

katie-knister commented Dec 7, 2020

Uh oh!

ricab 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!

katie-knister commented Dec 8, 2020

Uh oh!

ricab left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ricab left a comment

Choose a reason for hiding this comment

Uh oh!

bors bot commented Dec 14, 2020

Uh oh!

bors bot commented Dec 14, 2020

Uh oh!

Uh oh!

ricab commented Nov 30, 2020 •

edited

Loading

Saviq commented Dec 1, 2020 •

edited

Loading

codecov bot commented Dec 5, 2020 •

edited

Loading