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

Check the file in the primary storage and allow remove it from the #37067

Merged
merged 3 commits into from
Mar 26, 2020

Conversation

jvillafanez
Copy link
Member

cache if it's missing

Description

Files in the primary storage could be deleted outside of ownCloud, leaving information in the ownCloud's file cache. This command intends to check if the target file can be read from the backend storage, and it not, it allows you to remove the information cached.

Main goal is to clean the cache for objectstores (objectstore and files_primary_S3 apps) as primary storage, but the behaviour has been homogenized, so it can be used for local storage as long as the same conditions apply.

Note that removing files directly from the primary storage isn't supported and shouldn't happen. As such, the cases where you need to run this command should be extremely weird. This is why we only provide a command to check for only one file instead of scanning for the whole ownCloud's FS

Related Issue

https://github.com/owncloud/enterprise/issues/3801

Motivation and Context

Provide a tool to clean up stored information in the ownCloud's filecache in case a file disappears from the primary storage

How Has This Been Tested?

Check both with local storage and objectstore app (files_primary_S3 not tested but expected to behave the same as with objectstore)

  1. Upload some files into the primary storage through ownCloud
  2. Remove one of those files directly from the storage (bypassing ownCloud)
  3. Run the command

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

@update-docs
Copy link

update-docs bot commented Mar 3, 2020

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@codecov
Copy link

codecov bot commented Mar 3, 2020

Codecov Report

Merging #37067 into master will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #37067      +/-   ##
============================================
+ Coverage     64.75%   64.78%   +0.02%     
- Complexity    19135    19145      +10     
============================================
  Files          1270     1271       +1     
  Lines         74909    74957      +48     
  Branches       1329     1328       -1     
============================================
+ Hits          48511    48559      +48     
- Misses        26007    26008       +1     
+ Partials        391      390       -1     
Flag Coverage Δ Complexity Δ
#javascript 54.18% <ø> (+0.01%) 0.00 <ø> (ø)
#phpunit 65.96% <100.00%> (+0.02%) 19145.00 <9.00> (+10.00)
Impacted Files Coverage Δ Complexity Δ
apps/files_external/lib/Lib/Storage/Google.php 0.00% <0.00%> (ø) 0.00% <0.00%> (ø%)
apps/files/lib/Command/CheckCache.php 100.00% <0.00%> (ø) 9.00% <0.00%> (?%)
core/js/shareitemmodel.js 80.51% <0.00%> (+0.41%) 0.00% <0.00%> (ø%)
apps/files/lib/Command/TransferOwnership.php 81.57% <0.00%> (+0.75%) 43.00% <0.00%> (+1.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 923be3d...dceb5e5. Read the comment docs.

return self::ERROR_NOT_A_FILE;
}

$stream = @$node->fopen('rb'); // errors are expected if the file is missing
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to check if we want to suppress the errors or not. I assume the admin knows what file is giving problems as well as the reason behind those problems, so there is no reason to log the same thing again.
Note that, taking into account the current implementation of the objectstore, we can't know the reason why the file can't be opened. We're assuming here that the file is missing, but it might happen that there is no connection or wrong permissions; in this case, we rely on the admin to use or not this command knowing that the file (and/or the filecache infor) might be deleted.

@micbar
Copy link
Contributor

micbar commented Mar 6, 2020

@jvillafanez LGTM from a code POV

@cdamken @pako81 @enbrnz Could support / consulting give some feedback on this feature please?

@micbar micbar merged commit 9fdba83 into master Mar 26, 2020
@delete-merged-branch delete-merged-branch bot deleted the check_cache_primary branch March 26, 2020 09:12
@mmattel
Copy link
Contributor

mmattel commented Mar 26, 2020

You "forgot" to create a doc issue 👎
These things could be easily missed and forgotten...
I added one for you, see link.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants