Bug #8907

storage_helper.rb does not check available disk space

Added by kytv about 10 years ago. Updated about 10 years ago.

Status:
Resolved
Priority:
Normal
Assignee:
Category:
Test suite
Target version:
Tails_1.3
Start date:
2015-02-15
Due date:
% Done:

100%

Feature Branch:
kytv:test/8907-check-space
Type of work:
Code
Blueprint:

Starter:
Affected tool:
Deliverable for:

Description

The test suite will currently try to create storage devices without checking whether the required disk space is available. Of course, test(s) will eventually fail but it would be preferable to fail much earlier in the process.

I’m currently testing a patch that addresses this.

Subtasks

History

#1 Updated by kytv about 10 years ago

  • Subject changed from storage_helper.rb does not check available disk space before working to storage_helper.rb does not check available disk space

#2 Updated by kytv about 10 years ago

  • Target version changed from Tails_1.3.2 to Tails_1.3

#3 Updated by kytv about 10 years ago

  • Assignee changed from kytv to anonym
  • % Done changed from 20 to 50
  • QA Check set to Ready for QA
  • Feature Branch set to kytv:test/8907-check-space

#4 Updated by anonym about 10 years ago

The branch works fine, but here’s some nitpicking: :)

> + free = `df “#{path}”`

In features/support/helpers/misc_helpers.rb we define cmd_helper(), which asserts that the command succeeds. In this case you check that path exists, which should catch all but truly arcane errors (like bit flips, but just like we should prefer execute_successfully over an un-checked execute, we should consistently use cmd_helper(), imho.

> + avail = convert_to_MiB(get_free_space(‘host’, “#{@pool_path}”), “KiB”)

The embed-expression-inside-string mechanism #{...} isn’t needed here, and just makes the code harder to read.

> + needed = convert_to_MiB(options[:size].to_i, options[:unit]) + 500

Where does this magic number 500 come from? I’m know there’s a good explanation so I think you should introduce an aptly named variable + a comment.

> + assert(avail > needed, “Error creating disk \”#{name}\" in \“#{@pool_path}\”. Need " +

Strictly (and semantically) speaking, we should assert that avail >= needed. :)

#5 Updated by kytv about 10 years ago

  • % Done changed from 50 to 60

anonym wrote:
> The branch works fine, but here’s some nitpicking: :)

Thank you for your continued guidance and explanations.

The magical number’s value is arbitrary. I just wanted some sort of padding or slack space and thought 1GB might be too much but that around 500MB might be enough.

Perhaps the added extrapadding variable and explanation is sufficient. Changes made, tested, pushed.

#6 Updated by anonym about 10 years ago

  • Assignee changed from anonym to kytv
  • QA Check changed from Ready for QA to Dev Needed

kytv wrote:
> The magical number’s value is arbitrary. I just wanted some sort of padding or slack space and thought 1GB might be too much but that around 500MB might be enough.

Makes sense.

> Perhaps the added extrapadding variable and explanation is sufficient. Changes made, tested, pushed.

I think I’d prefer it like this:

--- a/features/support/helpers/storage_helper.rb
+++ b/features/support/helpers/storage_helper.rb
@@ -72,12 +72,13 @@ class VMStorage
     options[:type] ||= "qcow2"
     # Require 'slightly' more space to be available to give a bit more leeway
     # with rounding, temp file creation, etc.
-    extrapadding = 500
-    needed = convert_to_MiB(options[:size].to_i, options[:unit]) + extrapadding
+    reserved = 500
+    needed = convert_to_MiB(options[:size].to_i, options[:unit])
     avail = convert_to_MiB(get_free_space('host', @pool_path), "KiB")
-    assert(avail >= needed, "Error creating disk \"#{name}\" in \"#{@pool_path}\"." +
-                            "Need #{needed} MiB but only #{avail} MiB is available.")
-
+    assert(avail - reserved >= needed,
+           "Error creating disk \"#{name}\" in \"#{@pool_path}\". " \
+           "Need #{needed} MiB but only #{avail} MiB is available of, " \
+           "which #{reserved} MiB is reserved for other temporary files.")
     begin
       old_vol = @pool.lookup_volume_by_name(name)
     rescue Libvirt::RetrieveError


This way it’s less likely to confuse users about where those 500 MiB of free space went. :) What do you think?

#7 Updated by kytv about 10 years ago

  • Assignee changed from kytv to anonym
  • QA Check changed from Dev Needed to Ready for QA

Yes, that’s much better. Applied and pushed.

#8 Updated by Tails about 10 years ago

  • Status changed from In Progress to Fix committed
  • % Done changed from 60 to 100

Applied in changeset commit:1eec6e8a0cd07c9d43b76e3dcf972e37e3a276ce.

#9 Updated by anonym about 10 years ago

  • Assignee deleted (anonym)
  • QA Check changed from Ready for QA to Pass

#10 Updated by BitingBird about 10 years ago

  • Status changed from Fix committed to Resolved

#11 Updated by intrigeri about 10 years ago