Bug #8907

storage_helper.rb does not check available disk space

Added by kytv 2015-02-15 18:28:20 . Updated 2015-03-21 11:13:53 .

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 2015-02-15 18:34:35

  • 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 2015-02-15 20:14:12

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

#3 Updated by kytv 2015-02-15 21:30:49

  • 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 2015-02-18 10:02:53

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 2015-02-18 11:28:34

  • % 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 2015-02-20 10:01:31

  • 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 2015-02-20 14:04:40

  • 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 2015-02-22 11:47:52

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

Applied in changeset commit:1eec6e8a0cd07c9d43b76e3dcf972e37e3a276ce.

#9 Updated by anonym 2015-02-22 13:18:13

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

#10 Updated by BitingBird 2015-02-24 22:41:17

  • Status changed from Fix committed to Resolved

#11 Updated by intrigeri 2015-03-21 11:13:53