Add CleanupRemoteStorages command #26379

Merged
merged 5 commits into from Jan 24, 2017

Projects

None yet

5 participants

@butonic
Member
butonic commented Oct 14, 2016 edited

Description

# occ sharing:cleanup-remote-storages --help
Usage:
  sharing:cleanup-remote-storages

Options:
  -h, --help            Display this help message
[...]
Help:
 Delete all storage entries that have no matching entries in the shares_external table

Related Issue

cleanup for #15701

  • find the right place where we can delete the storage along with the remote share in #26490

Motivation and Context

remote storages with "shared::" never get deleted. This becomes a problem if a user has reshared an incoming remote share that has been deleted, because that internal re share never gets deleted (fileid still exists in filecache and storage for that file is never deleted). The reshare recipient now gets an errer when trying to access that file/folder.

How Has This Been Tested?

on my personal instance:

18 remote storage(s) need(s) to be checked
8 remote share(s) exist
shared::801c61b92c2aae00412ee21003bd2026 belongs to remote share 11
shared::7fe41a07d3f517a923f4b2b599e72cbb belongs to remote share 12
shared::af712293ab5eb9e6b1745a13818b99fe belongs to remote share 13
shared::c885155131f4487e025943244f4116b1 belongs to remote share 14
shared::113174afdf6e830fc0d7f1c00ec1a581 belongs to remote share 15
shared::6c2632ea130cd1bfc21d839bdc656e62 belongs to remote share 16
shared::cf3b58cc953fdef753867fd4d96a24b7 belongs to remote share 17
shared::0293022aa2a0813ab3f3488ef9336d6b has no matching storage, yet
shared::0ba88523c45c206b91ad8b97141ff020 [23] can be deleted
shared::0d45f25ee68fe0cda20df2139620082e [26] can be deleted
shared::207d9354a47a54e5749b235d0f42aac4 [22] can be deleted
shared::4c783debcc30ff253cf93f2a94a90c08 [27] can be deleted
shared::57ae748a3aa8934484aa70b23675cc93 [35] can be deleted
shared::662f5adeffe1a342f9475066bf94a0f3 [24] can be deleted
shared::99bb21dca2832e9311e3714dd92b8ff2 [18] can be deleted
shared::9b9f0b149480a3baaab8156eb64e5165 [21] can be deleted
shared::ac6f074d3b011e20b0f6a0a5c79b8b4b [20] can be deleted
shared::f7de882bbfcc2cf26780098fda9e1657 [34] can be deleted
shared::fd171b4b7b3c5907f4983e7825e96725 [25] can be deleted

Screenshots (if appropriate):

see above

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

TODO

  • actually delete storages and files
  • what happens when accessing a loca reshare of a remote storage that becomes unavailable (eg. due to network outage)
@mention-bot

@butonic, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nickvergessen, @icewind1991 and @MTGap to be potential reviewers.

@PVince81
Collaborator

This should be a background job too that runs regularily, because these orphaned stuffs are an expected situation, not an exceptional one.

@butonic
Member
butonic commented Oct 14, 2016

well we should delete the storage along with the share, that would prevent oc from picking up the storage in the first place. There might be a lot of filecache entries and I'd prefer to add a specific DeleteStorage job than a reoccurring Job that uses two likes on the storages table and has to load all storages in ram. With a CLI you can test the execution and then actually delete a storage. with an automatic background job you loose that control.

@butonic
Member
butonic commented Oct 14, 2016

We could also try spawnang a backgroud process that does the deletion: http://stackoverflow.com/a/45966

@PVince81
Collaborator

No, I think you're right. We should delete this properly at the time of deletion. There is no point in delying the deletion.

Then the command itself should also be invoked by a repair step so it happens at latest during the next update.

@butonic butonic self-assigned this Oct 26, 2016
@PVince81
Collaborator

@butonic I will soon push a fix for auto-deleting the oc_storages + oc_filecache as a result of "unshare from self". You can focus on the cleanup in this PR here.

@PVince81
Collaborator

@butonic #26490 will make sure such stray entries do not appear again. However we need your repair step / command to cleanup older entries once.

@butonic butonic changed the title from [WIP] Add DeleteOrphanedShares command to Add CleanupRemoteStorages command Oct 27, 2016
@butonic
Member
butonic commented Oct 27, 2016

failing tests unrelated

@@ -27,4 +27,8 @@ Turning the feature off removes shared files and folders on the server for all s
<job>OCA\Files_Sharing\DeleteOrphanedSharesJob</job>
<job>OCA\Files_Sharing\ExpireSharesJob</job>
</background-jobs>
+
+ <commands>
+ <command>OCA\Files_Sharing\Command\CleanupRemoteStorages</command>
@PVince81
PVince81 Oct 27, 2016 Collaborator

I have the feeling that we're moving more and more away from background jobs... will background jobs even make sense in the future then ? Or maybe we need to have hybrids: commands that can optionally be run as background job and maybe have a UI for it.

(just as a side note, not relevant for the review)

CC @DeepDiver1975

@PVince81
PVince81 Oct 27, 2016 Collaborator

Hmm, if we consider this to be an exceptional situation then a command is fine.
It is only exceptional if we manage to finish #26490 which should delete the entry directly at the right time.

@butonic
butonic Oct 28, 2016 edited Member

the nice thing about commands is that they can be scheduled more granularily via cron than our internal background jobs, eg any cleanup/exdiry job can be scheduled to run at 2am where no one is using the instance.

@PVince81
PVince81 Oct 28, 2016 Collaborator

yes, but with the drawback that if an admin forgot to schedule it they won't run by default and crap will pile up in the DB (for commands that are supposed to be run regularily)

@PVince81
PVince81 Nov 7, 2016 Collaborator

@butonic any update ?

@PVince81

Tested with LDAP user, works.

We should also delete the matching oc_filecache entry at least.
Also see my other minor comments.

+use Symfony\Component\Console\Output\OutputInterface;
+
+/**
+ * Delete all storage entries that have no matching entries in the shares_external table.
@PVince81
PVince81 Oct 27, 2016 Collaborator

[optional] "all storage entries" => actually "all 'shared::' storage entries"

+ ->setName('sharing:cleanup-remote-storages')
+ ->setDescription('Show or delete storage entries that have no matching entries in the shares_external table')
+ ->addOption(
+ 'dry-run',
@PVince81
PVince81 Oct 27, 2016 Collaborator

[optional] if "dry-run" is for "Show" then maybe we don't need to specify it in the description. Just call it "Cleanup storage entries that..."

+ }
+
+ public function deleteStorage ($id, $numericId, OutputInterface $output) {
+ $queryBuilder = \OC::$server->getDatabaseConnection()->getQueryBuilder();
@PVince81
PVince81 Oct 27, 2016 Collaborator

$this->connection, you already injected it

+ 'id',
+ $queryBuilder->createNamedParameter($id, IQueryBuilder::PARAM_STR),
+ IQueryBuilder::PARAM_STR)
+ );
@PVince81
PVince81 Oct 27, 2016 Collaborator

We should also delete the matching oc_filecache entries here as they are part of the storage. (on the API level, calling clear on the ICache instance does both).

+ );
+ $output->write("deleting $id [$numericId] ... ");
+ $count = $queryBuilder->execute();
+ $output->writeln("deleted $count");
@PVince81
PVince81 Oct 27, 2016 Collaborator

[minor] there will always be only "1" here, not sure if we should log it. But you're right that we want a message that confirms that something has been deleted or not.

+ ->from('storages')
+ ->where($queryBuilder->expr()->like(
+ 'id',
+ $queryBuilder->createNamedParameter('shared::________________________________', IQueryBuilder::PARAM_STR),
@PVince81
PVince81 Oct 27, 2016 Collaborator

Huh ?! That confused me for a minute. Ok, that's a LIKE regexp.
Maybe add a code comment that says we match "shared::" + 32 chars

+ )
+ ->andWhere($queryBuilder->expr()->notLike(
+ 'id',
+ $queryBuilder->createNamedParameter('shared::/%', IQueryBuilder::PARAM_STR),
@PVince81
PVince81 Oct 27, 2016 Collaborator

good call, just to be safe

+
+ public function getRemoteShareIds() {
+
+ $queryBuilder = \OC::$server->getDatabaseConnection()->getQueryBuilder();
@PVince81
PVince81 Oct 27, 2016 Collaborator

$this->connection

+ *
+ */
+
+namespace OCA\Files\Tests\Command;
@PVince81
PVince81 Oct 27, 2016 Collaborator

OCA\Files_sharing\Tests\Command ? Also update @package in PHPDoc

@@ -27,4 +27,8 @@ Turning the feature off removes shared files and folders on the server for all s
<job>OCA\Files_Sharing\DeleteOrphanedSharesJob</job>
<job>OCA\Files_Sharing\ExpireSharesJob</job>
</background-jobs>
+
+ <commands>
+ <command>OCA\Files_Sharing\Command\CleanupRemoteStorages</command>
@PVince81
PVince81 Oct 27, 2016 Collaborator

Hmm, if we consider this to be an exceptional situation then a command is fine.
It is only exceptional if we manage to finish #26490 which should delete the entry directly at the right time.

+ ->method('writeln')
+ ->with('deleted 1');
+
+ $this->command->execute($input, $output);
@PVince81
PVince81 Oct 28, 2016 Collaborator

shouldn't we also check the database to make sure the oc_storages and oc_filecache entries are gone after testing ? πŸ˜„

@butonic
butonic Oct 28, 2016 Member

hm if the count is 1 ... the storage should be gone.

@PVince81
PVince81 Oct 28, 2016 edited Collaborator

that is, if we trust the tested code isn't lying πŸ˜‰
Does it cover both oc_storages and oc_filecache deletion ?

Also in the test we should ideally have regular shares and test at the end that they still exist to cover the part that excludes regular shares.
This would align with how we wrote other repair step tests.

@PVince81
PVince81 Nov 9, 2016 Collaborator

@butonic any update ? Do you need help to finish this ?

@PVince81
Collaborator

Thanks for the update.
I also just noticed that in the unit test we don't verify that oc_storages and oc_filecache are gone.

Going to retest this.

@PVince81
Collaborator

Tested, works.

@PVince81 PVince81 referenced this pull request Nov 7, 2016
Closed

Delete storage when unsharing remote share from self #26490

3 of 13 tasks complete
@DeepDiver1975
Member
Test Name

Duration

Age
 OCA\Files_Sharing\Tests\Command\CleanupRemoteStoragesTest.testCleanup
 Stack Trace

OCA\Files_Sharing\Tests\Command\CleanupRemoteStoragesTest::testCleanup
Expectation failed for method name is equal to <string:write> when invoked at sequence index 9
Parameter 0 for invocation Symfony\Component\Console\Output\OutputInterface::write('deleting files for storage 258 ... ', false, 0) does not match expected value.
Failed asserting that 'deleting files for storage 258 ... ' starts with "deleting shared::af712293ab5eb9e6a1745a13818b99fe".

/var/lib/jenkins/workspace/owncloud-core_core_PR-26379-LFWSIQ5TFLURDDOMQTBVQD2VJPS5XENDYFNE5NDG2LUZ3JU3V4LQ/apps/files_sharing/lib/Command/CleanupRemoteStorages.php:129
/var/lib/jenkins/workspace/owncloud-core_core_PR-26379-LFWSIQ5TFLURDDOMQTBVQD2VJPS5XENDYFNE5NDG2LUZ3JU3V4LQ/apps/files_sharing/lib/Command/CleanupRemoteStorages.php:118
/var/lib/jenkins/workspace/owncloud-core_core_PR-26379-LFWSIQ5TFLURDDOMQTBVQD2VJPS5XENDYFNE5NDG2LUZ3JU3V4LQ/apps/files_sharing/lib/Command/CleanupRemoteStorages.php:87
/var/lib/jenkins/workspace/owncloud-core_core_PR-26379-LFWSIQ5TFLURDDOMQTBVQD2VJPS5XENDYFNE5NDG2LUZ3JU3V4LQ/apps/files_sharing/tests/Command/CleanupRemoteStoragesTest.php:181

    2 ms    1
 Test\Comments\ManagerTest.testGetComment
 Stack Trace

Test\Comments\ManagerTest::testGetComment
Failed asserting that two DateTime objects are equal.
--- Expected
+++ Actual
@@ @@
 2016-10-28T07:55:57+0000

/var/lib/jenkins/workspace/owncloud-core_core_PR-26379-LFWSIQ5TFLURDDOMQTBVQD2VJPS5XENDYFNE5NDG2LUZ3JU3V4LQ/tests/lib/Comments/ManagerTest.php:110

    0 ms    2
 Test\Comments\ManagerTest.testSaveNew
 Stack Trace

Test\Comments\ManagerTest::testSaveNew
Failed asserting that two DateTime objects are equal.
--- Expected
+++ Actual
@@ @@
 2016-10-28T07:55:57+0000

/var/lib/jenkins/workspace/owncloud-core_core_PR-26379-LFWSIQ5TFLURDDOMQTBVQD2VJPS5XENDYFNE5NDG2LUZ3JU3V4LQ/tests/lib/Comments/ManagerTest.php:372

    2 ms    2
 Test\Comments\ManagerTest.testSetMarkRead
 Stack Trace

Test\Comments\ManagerTest::testSetMarkRead
Failed asserting that two DateTime objects are equal.
--- Expected
+++ Actual
@@ @@
 2016-10-28T07:55:57+0000

/var/lib/jenkins/workspace/owncloud-core_core_PR-26379-LFWSIQ5TFLURDDOMQTBVQD2VJPS5XENDYFNE5NDG2LUZ3JU3V4LQ/tests/lib/Comments/ManagerTest.php:576

    0 ms    2
 Test\Share20\DefaultShareProviderTest.testCreateLinkShare
 Stack Trace

Test\Share20\DefaultShareProviderTest::testCreateLinkShare
Failed asserting that two DateTime objects are equal.
--- Expected
+++ Actual
@@ @@
 2016-10-28T07:56:12+0000

/var/lib/jenkins/workspace/owncloud-core_core_PR-26379-LFWSIQ5TFLURDDOMQTBVQD2VJPS5XENDYFNE5NDG2LUZ3JU3V4LQ/tests/lib/Share20/DefaultShareProviderTest.php:747
@PVince81
Collaborator

@butonic please fix the test

@PVince81
Collaborator

@butonic can you fix the test or do you want someone else to take over ? Would be good to finish this

@butonic
Member
butonic commented Dec 16, 2016

Sorry, too many fires ... someone pls take over.

@butonic butonic removed their assignment Dec 16, 2016
@PVince81
Collaborator

Sorry, too many fires ... someone pls take over.

That's fine, just say so so we know you're not working on it.

@PVince81
Collaborator

I've fixed and improved the tests.

butonic and others added some commits Oct 12, 2016
@butonic @PVince81 butonic Add CleanupRemoteStorages command 5f14dc7
@butonic @PVince81 butonic cleanup files, address review 6d86987
@PVince81 PVince81 Fix CleanupRemoteStoragesTest tests
Fix test expectation.
Added files count to check filecache deletion.
f2345f4
@PVince81
Collaborator

Rebased. Let's hope the tests will pass now.

@PVince81
Collaborator

Orrrrrrghhhhhacle

@PVince81 PVince81 Sort by numeric id for deterministic test results
1d0c377
@PVince81
Collaborator

Again ? 😒

11:44:26 1) OCA\Files_Sharing\Tests\Command\CleanupRemoteStoragesTest::testCleanup
11:44:26 Expectation failed for method name is equal to <string:writeln> when invoked at sequence index 2
11:44:26 Parameter 0 for invocation Symfony\Component\Console\Output\OutputInterface::writeln('shared::efe3b456112c3780da615...are 46', 0) does not match expected value.
11:44:26 Failed asserting that 'shared::efe3b456112c3780da6155d3a9b9141c belongs to remote share 46' starts with "shared::7b4a322b22f9d0047c38d77d471ce3cf belongs to remote share".
11:44:26 
11:44:26 /var/lib/jenkins/workspace/owncloud-core_core_PR-26379-LFWSIQ5TFLURDDOMQTBVQD2VJPS5XENDYFNE5NDG2LUZ3JU3V4LQ/apps/files_sharing/lib/Command/CleanupRemoteStorages.php:71
11:44:26 /var/lib/jenkins/workspace/owncloud-core_core_PR-26379-LFWSIQ5TFLURDDOMQTBVQD2VJPS5XENDYFNE5NDG2LUZ3JU3V4LQ/apps/files_sharing/tests/Command/CleanupRemoteStoragesTest.php:239
@PVince81
Collaborator

Looks like it's impossible to fix without fully rewriting the tests.

Oh well... Let's rewrite them in a way that makes more sense: instead of checking the text output and trusting that the job is done, we'll simply make sure that the data that needed to be killed is gone from the DB and the rest is still there.

@PVince81 PVince81 Removed precise order test and added storage check
Remove inaccurate removal message check which has a different order on
Oracle.

Added more checks to confirm that existing storages still exist.
4eb5860
@PVince81
Collaborator

Fix here 4eb5860

@PVince81
Collaborator

Tests pass now, @butonic please review tests and clarify backport requirement

@butonic butonic added this to the 9.0.8 milestone Jan 24, 2017
@butonic
Member
butonic commented Jan 24, 2017

backport to 9.0.x requested

@butonic
Member
butonic commented Jan 24, 2017

πŸ‘ good to go

@PVince81 PVince81 merged commit aa92a65 into master Jan 24, 2017

4 checks passed

Scrutinizer 9 updated code elements
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@PVince81 PVince81 deleted the cleanupremotestorages branch Jan 24, 2017
@PVince81
Collaborator

stable9.1: #27009
stable9: #27010

@PVince81
Collaborator

Apparently this stores everything in memory, not good. Raised here #27017

@SergioBertolinSG
Member

When using this command I have seen that it doesn't detect if remote shares were unshared.

My steps (using stable9.1):

  1. Import a share from a public link.
  2. Remove the public share in the sharer server.
  3. Run the command cleanup-remote-storages (nothing is deleted).
  4. Access the shared folder with recipient and see it dissapear.
  5. Run the command cleanup-remote-storages. Now database entries are deleted.

should this command be able to detect if a remote share has been unshared?

@SergioBertolinSG
Member

As discussed with @PVince81 this command should not do any remote connections.

@PVince81
Collaborator

To clarify, the command only clears oc_storages entries that have no more matching oc_share_external entries. The entries in oc_share_external are usually already cleared by some other logic in the code.

So the orphaned entries need to be cleaned up later by this command.

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