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

[zooko]

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.

[davidsarah]

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

[davidsarah]

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.

[davidsarah]

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

[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.)

[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. (fileutil.get_available_space is implemented in terms of file.get_disk_stats, so it's only necessary to mock the latter.)

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

[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."

[davidsarah]

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

[davidsarah]

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.

[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."

[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.

[warner]

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

[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

[zooko]

I reviewed this patch. +1!

[david-sarah@…]

In 4c592f15054c5a53:

(The changeset message doesn't reference this ticket)

[david-sarah@…]

In c2972e22cb3c7d46:

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

[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

[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