#1384 closed defect (fixed)

use storage/shares/ instead of storage/ to detect available space

Reported by: zooko Owned by: zooko
Priority: minor Milestone: 1.9.0
Component: code-nodeadmin Version: 1.8.2
Keywords: usability configuration defaults storage reviewed Cc:
Launchpad Bug:

Description

Gwern on IRC had this issue after they made their storage/shares/ be a symlink to an external hard disk (on my suggestion). Once they reported the issue then I realized that many storage servers that I am running (for volunteergrid, among others) have this same issue.

I think we should examine the partition which has storage/shares/ instead of the one which has storage/ when determining how much disk space is available.

Here's a patch:

--- old-bothw/src/allmydata/storage/server.py   2011-03-30 17:12:13.000000000 -0600
+++ new-bothw/src/allmydata/storage/server.py   2011-03-30 17:12:13.000000000 -0600
@@ -192,7 +192,7 @@
 
         if self.readonly_storage:
             return 0
-        return fileutil.get_available_space(self.storedir, self.reserved_space)
+        return fileutil.get_available_space(self.sharedir, self.reserved_space)
 
     def allocated_size(self):
         space = 0

I can't think of a useful way to unit test this patch! So, I'm marking it as review-needed.

Attachments (4)

fix-1384.darcs.patch (15.9 KB) - added by davidsarah at 2011-07-19T03:30:26Z.
src/allmydata/storage/server.py: use the filesystem of storage/shares/, rather than storage/, to calculate remaining space. fixes #1384
news-1384.darcs.patch (19.6 KB) - added by davidsarah at 2011-07-19T03:31:17Z.
NEWS for: use the filesystem of storage/shares/, rather than storage/, to calculate remaining space. refs #1384 (Note that this has an artificial dependency on another patch that touches NEWS.)
test-1384.darcs.patch (17.3 KB) - added by davidsarah at 2011-07-19T19:54:52Z.
test for: use the filesystem of storage/shares/, rather than storage/, to calculate remaining space. refs #1384
test-1384.2.darcs.patch (31.1 KB) - added by davidsarah at 2011-08-09T20:17:57Z.
test_storage.py: test that we are using the filesystem of storage/shares/, rather than storage/, to calculate remaining space, and that the HTML status output reflects the values returned by fileutil.get_disk_stats. This version works with older versions of the mock library. refs #1384

Download all attachments as: .zip

Change History (19)

comment:1 Changed at 2011-03-30T23:53:07Z by davidsarah

  • Owner set to davidsarah
  • Status changed from new to assigned

+1 on this patch. I'll test it manually.

comment:2 Changed at 2011-07-19T03:29:47Z by davidsarah

  • Milestone changed from undecided to 1.9.0

There was an instance of fileutil.get_disk_stats(self.storedir, self.reserved_space) that also needed to be changed to self.sharedir in order to report the remaining space correctly. Thanks to T_X on IRC for testing this.

Last edited at 2011-07-19T03:36:36Z by davidsarah (previous) (diff)

Changed at 2011-07-19T03:30:26Z by davidsarah

src/allmydata/storage/server.py: use the filesystem of storage/shares/, rather than storage/, to calculate remaining space. fixes #1384

Changed at 2011-07-19T03:31:17Z by davidsarah

NEWS for: use the filesystem of storage/shares/, rather than storage/, to calculate remaining space. refs #1384 (Note that this has an artificial dependency on another patch that touches NEWS.)

comment:3 follow-up: Changed at 2011-07-19T03:40:27Z by davidsarah

  • Keywords test-needed added

We could, and probably should, test this by mocking fileutil.get_disk_stats, and checking that the server reports the correct space and accepts/refuses immutable shares correctly. (fileutil.get_available_space is implemented in terms of fileutil.get_disk_stats, so it's only necessary to mock the latter.)

Last edited at 2011-07-19T03:40:58Z by davidsarah (previous) (diff)

comment:4 Changed at 2011-07-19T04:10:30Z by T_X

I was having the same issue and with

a) using the two patches and

b) and mounting instead of symlinking:

  • creating a raw, large file with dd on the desired disk (dd if=/dev/urandom of=/home/ftp/tahoe.img bs=1k count=250M)
  • losetup /dev/loop0 /home/ftp/tahoe.img
  • mount /dev/loop0 ~/.tahoe/storage/shares

...everything worked fine for me! tahoe-lafs was then picking the correct disk space and matched df -h ~/.tahoe/storage/shares.

Thanks for the support and patches!

comment:5 in reply to: ↑ 3 ; follow-up: Changed at 2011-07-19T15:43:14Z by davidsarah

Replying to davidsarah:

We could, and probably should, test this by mocking fileutil.get_disk_stats, and checking that the server reports the correct space and accepts/refuses immutable shares correctly.

Actually to test this patch, we only need to mock fileutil.get_disk_stats to check that it is called with the correct directory. We're short of time to write the test, and this approach is simpler.

Note that the NEWS patch says "This allows storage/shares/, rather than storage/, to be a symlink to another filesystem.". Per T_X's comment:4, it should say "... to be the mount point for another filesystem."

Changed at 2011-07-19T19:54:52Z by davidsarah

test for: use the filesystem of storage/shares/, rather than storage/, to calculate remaining space. refs #1384

comment:6 in reply to: ↑ 5 Changed at 2011-07-19T20:00:15Z by davidsarah

  • Keywords test-needed removed
  • Owner davidsarah deleted
  • Status changed from assigned to new

Replying to davidsarah:

Replying to davidsarah:

We could, and probably should, test this by mocking fileutil.get_disk_stats, and checking that the server reports the correct space and accepts/refuses immutable shares correctly.

Actually to test this patch, we only need to mock fileutil.get_disk_stats to check that it is called with the correct directory. We're short of time to write the test, and this approach is simpler.

I ended up implementing the more comprehensive test, but excluding the check that we accept/refuse immutable shares based on the remaining space. It does check that the get_available_space method of the StorageServer returns the right value.

comment:7 Changed at 2011-07-19T20:02:57Z by zooko

  • Owner set to zooko
  • Status changed from new to assigned

comment:8 Changed at 2011-07-19T22:13:28Z by davidsarah

According to my tests, os.statvfs does follow symlinks on Linux (and presumably other Unices). So the NEWS can say "This allows storage/shares/, rather than storage/, to be a mount point or a symlink pointing to another filesystem."

Last edited at 2011-07-19T22:14:09Z by davidsarah (previous) (diff)

comment:9 Changed at 2011-08-09T18:43:04Z by warner

I get a test failure in the new test_status_right_disk_stats:

[ERROR]
Traceback (most recent call last):
  File "build/bdist.macosx-10.6-universal/egg/mock.py", line 562, in patched
    
  File "/Users/warner2/stuff/tahoe/trunk-1384/src/allmydata/test/test_storage.py", line 2556, in test_status_right_disk_stats
    ss = StorageServer(basedir, "\x00" * 20, reserved_space=1*GB)
  File "/Users/warner2/stuff/tahoe/trunk-1384/src/allmydata/storage/server.py", line 72, in __init__
    if self.get_available_space() is None:
  File "/Users/warner2/stuff/tahoe/trunk-1384/src/allmydata/storage/server.py", line 207, in get_available_space
    return fileutil.get_available_space(self.sharedir, self.reserved_space)
  File "/Users/warner2/stuff/tahoe/trunk-1384/src/allmydata/util/fileutil.py", line 415, in get_available_space
    return get_disk_stats(whichdir, reserved_space)['avail']
exceptions.TypeError: 'Mock' object is unsubscriptable

allmydata.test.test_storage.WebStatus.test_status_right_disk_stats

My guess is that the mocked get_disk_stats() isn't ready to be called (because .side_effect wasn't added) by the time StorageServer.__init__ invokes it.

Maybe it'd be better to make call_get_disk_stats just add whichdir to a set, then at the end of the test assert that the set has just one element and that it's equal to expecteddir. Then set up the mock's .side_effect before constructing the StorageServer object.

comment:10 Changed at 2011-08-09T19:34:05Z by warner

oh, I forgot to mention that my tree was using mock version 0.7.0b4 .

Changed at 2011-08-09T20:17:57Z by davidsarah

test_storage.py: test that we are using the filesystem of storage/shares/, rather than storage/, to calculate remaining space, and that the HTML status output reflects the values returned by fileutil.get_disk_stats. This version works with older versions of the mock library. refs #1384

comment:11 Changed at 2011-08-09T20:56:11Z by zooko

  • Keywords reviewed added; review-needed removed

I reviewed this patch. +1!

comment:12 Changed at 2011-08-10T04:26:21Z by david-sarah@…

In 4c592f15054c5a53:

(The changeset message doesn't reference this ticket)

comment:13 Changed at 2011-08-10T04:26:25Z by david-sarah@…

  • Resolution set to fixed
  • Status changed from assigned to closed

In c2972e22cb3c7d46:

src/allmydata/storage/server.py: use the filesystem of storage/shares/, rather than storage/, to calculate remaining space. fixes #1384

comment:14 Changed at 2011-08-10T17:28:05Z by david-sarah@…

In [5153/ticket393-MDMF-2]:

test_storage.py: test that we are using the filesystem of storage/shares/, rather than storage/, to calculate remaining space, and that the HTML status output reflects the values returned by fileutil.get_disk_stats. This version works with older versions of the mock library. refs #1384

comment:15 Changed at 2011-08-10T17:28:05Z by david-sarah@…

In [5154/ticket393-MDMF-2]:

src/allmydata/storage/server.py: use the filesystem of storage/shares/, rather than storage/, to calculate remaining space. fixes #1384

Note: See TracTickets for help on using tickets.