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

Make insufficient storage errors visible without attempting too frequent uploads #5537

Closed
ckamm opened this issue Feb 16, 2017 · 10 comments
Closed

Comments

@ckamm
Copy link
Contributor

ckamm commented Feb 16, 2017

This is a follow up to #5346 and owncloud/enterprise#1831.

Before 2.3, 507 Insufficent Storage errors used to be blacklisted. That had the positive effect of making the client not reattempt the upload very frequently. But it also hid the error and sometimes users weren't even aware that there was a storage problem.

For 2.3 we removed the blacklisting. That made the error visible, but lead to too-frequent uploads.

The behavior of the client in situations with insufficient storage space on the server needs to be adjusted. We want users to become aware of the problem while not attempting too many uploads that have a high likelihood of failure.

@ckamm ckamm added this to the 2.4.0 milestone Feb 16, 2017
@ckamm ckamm self-assigned this Feb 16, 2017
ckamm added a commit that referenced this issue Feb 17, 2017
This reverts commit e1f5a49.

Retrying uploads with insufficent storage errors frequently leads to
high server traffic. See #5537 for links and a sketch of a correct
solution.
@ckamm
Copy link
Contributor Author

ckamm commented Feb 21, 2017

Okay, so what do we want to happen if the server has insufficient storage?

The obvious path would be to trust the quota the server reports and prohibit uploads that would exceed the quota and make the whole sync fail with a "Insufficent storage" normal error if that happens.

That means that the client would need to have a model of "free server disk space" that's kept up to date during the sync process. It also means we need to update quota regularly from the server even if the settings dialog is not shown (or do it before the sync?).

@SamuAlfageme
Copy link
Contributor

@ckamm +1 to being reactive to the quota, that would solve collateral problems like: #5346 (comment) (changes in user's quota)

@ckamm
Copy link
Contributor Author

ckamm commented Feb 22, 2017

@SamuAlfageme Actually, using quota changes to trigger syncs is not part of this commit yet.

To do that, we'd need to check quota regularly, even if the settings dialog is not visible. We'd also need to track that there are files that couldn't be synced due to quota limitations. It goes beyond what I wanted to accomplish here.

ckamm added a commit to ckamm/owncloud-client that referenced this issue Feb 22, 2017
* Run a Propfind to determine the available quota before each sync run.
* Keep the available quota up to date with each upload and delete job.
* Don't start upload jobs if there's not enough available space.
* If we (unexpectedly) receive a 507 Insufficent Storage error, adjust
  the expectation of available remote space downwards.
@ckamm
Copy link
Contributor Author

ckamm commented Mar 21, 2017

Unfinished pull request: #5552

Current status: We can have different quotas per folder and currenty don't track them. Fully addressing this needs significantly more work.

@GrendelOnCampus
Copy link

Hello, I understand the problem. You can have your own folder and a share from another user. But if one account is full, you can't upload to that folder. The client does only see the sum and thinks, that he can upload. So far, so good.

Maybe we can realize a solution in 2 steps?

  1. when encountering "Insufficient Storage" error for several times, slowing down the attempts for upload. First a few seconds upto several minutes. Each time let the client put an error message via popup. So we don't fill the logs with more than 80000 tries from just 1 user, as I've seen today. This delay-interval can be decreased again, when the server stops sending error messages about storage.
  2. making the server smart, that it reports the quota for several upload paths so the client can stop uploading the folders, which are full. Restart will only begin with this folder, when the server reports free space again for this particular upload path.

BTW... step 1 can stay active too, even when #2 is realized...

Just my proposal to this issue.

@ckamm
Copy link
Contributor Author

ckamm commented Jul 11, 2017

@DeepDiver1975 @michaelstingl and me spoke about this in detail. Summary:

  • The main issue is seeing 507 errors in the server log.
  • They appear because the client treats a PUT (or chunk-done MOVE) as a cheap way to say "I want to upload if there's enough space". This is correct behavior. The client could do a PROPFIND before each PUT to make running into the error less likely, but the extra request per upload would be significant overhead.
  • There's currently no API to retrieve all "quota-roots". The client has to assume that each folder could have a different quota.
  • When a 507 is seen the problem will not usually resolve itself: some user action (deleting other files, adding storage) is necessary. But we do want the client to sync everything automatically once this intervention has happened. That means the client must retry, poll quota, or something like that.
  • More generally, this is not just about 507s. It's about any error that can come up in regular operation and isn't an error the server admin needs to do something about.

My main suggestion:

  • Don't show 507 errors in owncloud.log: They do not indicate an actionable server error.

An alternate path that's more costly but would also fix the issue:

  • In the client, once you encounter a 507, switch into a PROPFIND-before-upload state.

In addition I'm in the process of improving error display and messaging for these kind of errors in the client. I might also add logic to not retry larger files in the same directory once a 507 has been seen.

ckamm added a commit that referenced this issue Jul 11, 2017
It now produces a summary error message indicating the problem.

Adjust blacklist database table to contain 'errorCategory'. This is
useful for two things:
  - Reestablishing summary messages based on blacklisted errors. For
    example if we don't retry a 507ed file, we still want to show the
    message about space on the server
  - Selectively wiping the blacklist: When we have ui for something like
    "I deleted some files, please retry all files now!", we want to
    delete all blacklist entries of a specific category only.
ckamm added a commit that referenced this issue Jul 11, 2017
Since these errors are blacklisted, it can take up to 24h to retry items
that had a 507 error for a while. This way users can intervene and cause
an upload attempt immediately.
ckamm added a commit that referenced this issue Jul 12, 2017
It now produces a summary error message indicating the problem.

Adjust blacklist database table to contain 'errorCategory'. This is
useful for two things:
  - Reestablishing summary messages based on blacklisted errors. For
    example if we don't retry a 507ed file, we still want to show the
    message about space on the server
  - Selectively wiping the blacklist: When we have ui for something like
    "I deleted some files, please retry all files now!", we want to
    delete all blacklist entries of a specific category only.
ckamm added a commit that referenced this issue Jul 12, 2017
Since these errors are blacklisted, it can take up to 24h to retry items
that had a 507 error for a while. This way users can intervene and cause
an upload attempt immediately.
@ckamm
Copy link
Contributor Author

ckamm commented Jul 12, 2017

The "make 507 errros visible" part is addressed by #5890. It also gives users the ability to say "retry now" when they have solved the problem.

@ckamm
Copy link
Contributor Author

ckamm commented Jul 12, 2017

The PR #5892 skips uploads that would clearly lead to more 507s. It doesn't change the basic approach.

ckamm added a commit that referenced this issue Jul 12, 2017
When we see a 507 error, assume that quota is < uploaded size.
ckamm added a commit that referenced this issue Jul 17, 2017
When we see a 507 error, assume that quota is < uploaded size.
@ckamm
Copy link
Contributor Author

ckamm commented Sep 12, 2017

I think the best approach to avoiding excessive 507 entries in the server error log is to not put them there. See #5537 (comment) . I'd start on a client-side avoidance strategy only if it's strictly necessary.

ckamm added a commit that referenced this issue Sep 15, 2017
When we see a 507 error, assume that quota is < uploaded size.
@ckamm ckamm removed this from the 2.4.0 milestone Oct 6, 2017
@ckamm ckamm added this to the 2.5.0 milestone Oct 6, 2017
@guruz guruz modified the milestones: 2.5.0, 2.6.0 Mar 27, 2018
@ogoffart ogoffart removed this from the 2.6.0 milestone Dec 3, 2018
@ckamm
Copy link
Contributor Author

ckamm commented Mar 13, 2019

I assume this is good now.

@ckamm ckamm closed this as completed Mar 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants