Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

handle local disk full properly (affects data integrity) #2939

Closed
moscicki opened this issue Mar 10, 2015 · 33 comments

Comments

@moscicki
Copy link
Contributor

commented Mar 10, 2015

Currently the client misses on handling of local disk full condition. This causes random crashes and is susceptible to corrupt data (or sync journal).

Client should require some minimal space being available on the disk hosting the sync folder(s) and enter "idle" mode when there is not enough disk space. For example, google drive pauses operation when free disk space is < ~1GB.

For implementation, one may use existing OS APIs, if not provided by Qt. Python example:

def get_free_space_mb(folder):
    """ Return folder/drive free space (in bytes)
    """
    if platform.system() == 'Windows':
        free_bytes = ctypes.c_ulonglong(0)
        ctypes.windll.kernel32.GetDiskFreeSpaceExW(ctypes.c_wchar_p(folder), None, None, ctypes.pointer(free_bytes))
        return free_bytes.value/1024/1024
    else:
        st = os.statvfs(folder)
        return st.f_bavail * st.f_frsize/1024/1024

This needs some additinal testing on less frequent filesystems (such as Windows DFS).

@dragotin

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2015

I agree, suggest to do it for 1.8.1, @MTRichards

@MTRichards

This comment has been minimized.

Copy link

commented Mar 10, 2015

@moscicki

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2015

I though about some cases:

  • no activity in sync client, disk usage grows independently of the sync client -> a simple periodic check and a check before starting any update activity could suffice
  • disk space going low due to the sync client itself performing downloads -> this requires some check integrated into the propagator itself (to pause it, snapshot the state and then resume it at some later stage)...
  • there could be also a combination of these two factors, in which case a sufficiently high threshold could be enough to cover all real-world scenarios...
  • if we are really paranoid about zero bytes left and the sync journal we may also foresee to have a placeholder file to book the minimal space required to cleanly shutdown sqlite db (not sure if that's an issue)
@dragotin

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2015

I think after the discovery phase (which does not need disk space) we should be able to calculate how much space change need for performing the sync. A decent safety belt can be added and we should be able to estimate if space is sufficient or not.

Notes:

  1. We need to find a efficient, cross platform way of getting the free disk space of the the partition on which the sync directory is located. That might be tricky.
  2. To do it perfectly nice, we would need to do the calculation about the available free space as we sync, as third party processes could eat up the space while we are working. Not sure if that is required...
@dragotin

This comment has been minimized.

@guruz guruz added the bug label Mar 27, 2015

@guruz guruz added this to the 1.8.1 - Bugfix milestone Mar 27, 2015

@ogoffart ogoffart modified the milestones: 1.9 - Multi-account, 1.8.1 - Bugfix Apr 8, 2015

@guruz

This comment has been minimized.

Copy link
Collaborator

commented May 5, 2015

Related #3144

@MTRichards

This comment has been minimized.

Copy link

commented Sep 2, 2015

As a user, I want the desktop client to recognize when the addition of new files and folders added to the server and synced to the desktop will fill the local disk space, to not sync these new folders and files, and to throw a notification of the failure so that the desktop I am using doesn't crash using the desktop client.

Acceptance Criteria:

  • To start, the desktop client will not attempt to use the last 250MB of free disk space on a client
  • When the desktop client identifies that new or updated files downloaded from the server will reduce the free space on the desktop to under 250MB, the desktop client will not sync any files from that point forward until the sync run will fall below 250MB
  • The desktop client will throw a red check mark and the sync run gets a fatal error “The sync client needs to sync XGB/MB of files, but only has room for Y GB/MB. Please free space on the disk before syncing.”
  • The calculations for the disk size needed includes all meaningful consumers of disk by the client (for example, he temp file)
  • At a future date, a different algorithm (% of free disk space for example, or a higher disk limit) may be needed.
    untitled
@MTRichards

This comment has been minimized.

Copy link

commented Sep 3, 2015

@jancborchardt interested in your design ideas here.

@moscicki

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2015

The most important is that we make sure that under no circumstances (such as some other application filling up the disk space while the sync client is idle) leads to corrupted journal or configuration files of the sync client.

@jancborchardt

This comment has been minimized.

Copy link
Member

commented Sep 8, 2015

Yeah, totally agree with your proposal @MTRichards. There definitely needs to be a red x as data is not synced and action from the person is required.

@dragotin

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2015

This is pretty similar to the quota problem mentioned in #3223.

Unfortunately earlier (when we discussed the quota problem before) we agreed to follow a different strategy as described here: IF an individual file exceeds the local space or quota, we do not sync that file, but carry on to try others. And that makes total sense, because there might be the one video with 2.3 GB that does not fit, but the 500 office documents would. In that case we only want to exclude the video and continue with the small ones.

The too large files are handled by the blacklisting, meaning that its retried a few times. If these attempts fail, the error is hidden to not annoy the user, and tried again later.

I suggest to stay with this strategy instead of "the desktop client will not sync any files from that point forward until the sync run will fall below 250MB" as described above.

@jancborchardt

This comment has been minimized.

Copy link
Member

commented Sep 10, 2015

Right, that should be the sync strategy. If a specific file can’t be synced, that should be a red error though.

the error is hidden to not annoy the user, and tried again later.

That doesn’t really work because it might be an important file, no? Files not syncing is an issue which should be visible, and not something we should aim to hide as to »not annoy people«.

@dragotin

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2015

@jancborchardt No. We specifically have the blacklist for exactly that reasoning: Not annoy people with errors of individual files.

@MTRichards

This comment has been minimized.

Copy link

commented Sep 10, 2015

"the desktop client will not sync any files from that point forward until the sync run will fall below 250MB" as described above.

Ok, I think it makes sense to use the same logic. At some point it will run out of room and just stop, but until then try at least.

@ckamm ckamm added the p2-high label Sep 30, 2015

ckamm added a commit to ckamm/owncloud-client that referenced this issue Oct 1, 2015

Sync: An initial diskspace check owncloud#2939
* Before each sync, check that there are at least
  250 MB of space available and abort otherwise.
* Can be overridden with OWNCLOUD_MIN_FREE_SPACE

ckamm added a commit that referenced this issue Oct 1, 2015

Propagator: Download disk space checks #2939
* There's a critical 50 MB threshold under which syncs abort
  (OWNCLOUD_CRITICAL_FREE_SPACE)
* The sync client always keeps 250 MB free
  (OWNCLOUD_FREE_SPACE)

@ckamm ckamm added the ReadyToTest label Oct 29, 2015

@ckamm

This comment has been minimized.

Copy link
Member

commented Oct 29, 2015

@Dianafg76

This comment has been minimized.

Copy link

commented Oct 29, 2015

@ckamm Thanks!!

@Dianafg76

This comment has been minimized.

Copy link

commented Nov 3, 2015

@ckamm I need steps to reproduce this issue, thanks

@Dianafg76 Dianafg76 added the Needs info label Nov 3, 2015

@ckamm

This comment has been minimized.

Copy link
Member

commented Nov 4, 2015

@Dianafg76 I've added the test cases I've been using to the wiki page. Do these seem good to you?

@ckamm ckamm removed the Needs info label Nov 4, 2015

@Dianafg76

This comment has been minimized.

Copy link

commented Nov 5, 2015

@guruz, @cdamken Not sure about how to check this one ☺️
What i have done:
1.- OWNCLOUD_CRITICAL_FREE_SPACE=100 /Applications/owncloud.app/Contents/MacOS/owncloud --logfile -
2.- From the desktop client, I have uploaded a 1GB data

CURRENT BEHAVIOR
The file is being uploaded, no errors

EXPECTED BEHAVIOR
I'd would have expected an error on the client, wouldn't I?

@phil-davis

This comment has been minimized.

Copy link
Contributor

commented Nov 5, 2015

You will need to do the reverse.
a) fill up your local disk, outside of the ownCloud client folder, with files.
b)Set OWNCLOUD_CRITICAL_FREE_SPACE to a number bigger than the free space on the local disk.
c) Put some (big) files on the server and let the client try to download them.

@guruz

This comment has been minimized.

Copy link
Collaborator

commented Nov 5, 2015

...you can leave out step a) if you set the OWNCLOUD_CRITICAL_FREE_SPACE accordingly :)

@Dianafg76

This comment has been minimized.

Copy link

commented Nov 5, 2015

  1. I set the OWNCLOUD_CRITICAL_FREE_SPACE=100
  2. Go to the Server web
  3. Create a folder a
  4. Go to the folder a
  5. Upload a File more than 100mb
  6. Go to the Desktop client
  7. Download the folder a

CURRENT BEHAVIOR
The file is downloaded without problem

@Dianafg76 Dianafg76 added Needs info and removed ReadyToTest labels Nov 5, 2015

@phil-davis

This comment has been minimized.

Copy link
Contributor

commented Nov 5, 2015

I believe OWNCLOUD_CRITICAL_FREE_SPACE is in MB. So, if you have 100GB free space on your local disk (=100,000MB) then you have to set:
OWNCLOUD_CRITICAL_FREE_SPACE=100000
(or more)
So that the actual free space on your local disk is less than OWNCLOUD_CRITICAL_FREE_SPACE

@Dianafg76

This comment has been minimized.

Copy link

commented Nov 6, 2015

My free space on my local i 737.54 GB drive, so I have to set
OWNCLOUD_CRITICAL_FREE_SPACE=737540
And What is the next step?

@phil-davis

This comment has been minimized.

Copy link
Contributor

commented Nov 6, 2015

Now put some files on the server using the web interface. The client should start to sync and then report that there is not enough space on the local disk.
You might need to increase OWNCLOUD_CRITICAL_FREE_SPACE even more, because it depends how your system measures 737.54 GB - maybe that is 737540 MB or maybe 737.54 * 1024 = 755241 MB - you can make it 800000 to be sure :)

@Dianafg76

This comment has been minimized.

Copy link

commented Nov 6, 2015

@guruz

  1. I set the OWNCLOUD_CRITICAL_FREE_SPACE= 703373
  2. Go to the Server web
  3. Upload some Files
  4. Go to the Desktop client
  5. Waiting to sync

CURRENT BEHAVIOR

The client is started to sync and I don't have the report.
I don't have the report that there is not enough space on the local disk

@phil-davis

This comment has been minimized.

Copy link
Contributor

commented Nov 6, 2015

I think you will need to set OWNCLOUD_CRITICAL_FREE_SPACE to a higher number. OWNCLOUD_CRITICAL_FREE_SPACE needs to be bigger than the free space on your local disk.

@ckamm

This comment has been minimized.

Copy link
Member

commented Nov 10, 2015

@Dianafg76 @phil-davis Actually, OWNCLOUD_CRITICAL_FREE_SPACE is in bytes (also says so here: https://github.com/owncloud/client/wiki/Testing-Scenarios-2.1)

So you want to set it to something like 8589934592 (800GB).

@ckamm ckamm added ReadyToTest and removed Needs info labels Nov 10, 2015

@ckamm

This comment has been minimized.

Copy link
Member

commented Nov 10, 2015

@Dianafg76 In new builds these env variables have been renamed to OWNCLOUD_CRITICAL_FREE_SPACE_BYTES and OWNCLOUD_FREE_SPACE

@phil-davis

This comment has been minimized.

Copy link
Contributor

commented Nov 10, 2015

@ckamm Thanks, from the numbers @Dianafg76 was using at first I just assumed that they were KB - I should read the doc or the code :)

@Dianafg76 Dianafg76 removed the ReadyToTest label Nov 11, 2015

@Dianafg76

This comment has been minimized.

Copy link

commented Nov 11, 2015

I tested this issue and finally is working OK for my.
Many thanks to @ckamm and @phil-davis for helping me
Desktop v ownCloud-2.1.0.2865-nightly20151110.pkg
Server v {"installed":true,"maintenance":false,"version":"8.1.4.0","versionstring":"8.1.4 RC1","edition":""}

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.