Skip to content

Add occ command dav:cleanup-chunks to remove all upload folders which…#28587

Merged
DeepDiver1975 merged 1 commit intomasterfrom
chunking-cleanup
Oct 9, 2017
Merged

Add occ command dav:cleanup-chunks to remove all upload folders which…#28587
DeepDiver1975 merged 1 commit intomasterfrom
chunking-cleanup

Conversation

@DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented Aug 4, 2017

… are older then 2 days

Description

Related Issue

fixes #26981

How Has This Been Tested?

  • create an upload folder via dav
  • set the mtime in database to an older value then 2 days ago
  • run the command
  • see the foller being killed

Screenshots (if appropriate):

bildschirmfoto von 2017-08-04 16-27-00

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.

}

protected function execute(InputInterface $input, OutputInterface $output) {
$cutOffTime = new \DateTime('2 days ago');
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok that's clever. The original problem was that iterating over ALL users would take a long time.

But this assumes that someone will run this command at least every day. If they miss one day, the command becomes useless.

I suggest to add CLI arguments to:

  • make it possible to run it for all users regardless of time
  • specify number of days

Copy link
Contributor

Choose a reason for hiding this comment

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

It removes chunks older than 2 days, so you can run it once a week, once a month., whenever you want to clean up the space. It will clean up all old crud.

But yes, the 2 days could be a parameter with default of 2 - that way:

  • if you know you have some chunks to cleanup then you can specify 1 day or even 0 days (if you know current activity is stopped)
  • if you have some users with huge uploads that run for ages, you can specify a larger number of days so as not to annoy them

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, of course...

@guruz
Copy link
Contributor

guruz commented Aug 4, 2017

Should this be hooked by default into oC's cron? So that it "just works" for normal installations, not only for hardcore admins running those commands?

@PVince81
Copy link
Contributor

PVince81 commented Aug 4, 2017

@guruz we likely can't plug it into the background job / cron because for some setups it is likely that the job itself will take several days to finish when iterating over millions of users... This is why we had this other discussion about "marking users for expiration" here: #14646 (comment).

The other alternative, iterating over the folders will not work either as it needs to be done on API / storage level which then needs you to iterate over all storages. (no magic find command on disk because of object store where stuff is not on disk)

@DeepDiver1975
Copy link
Member Author

bildschirmfoto von 2017-08-07 12-46-51

@PVince81
Copy link
Contributor

PVince81 commented Aug 7, 2017

"junks" 😱

@DeepDiver1975
Copy link
Member Author

"junks" 😱

take it as an review assessment test: achievement unlocked: conscientious-pr-reviewer

@PVince81
Copy link
Contributor

PVince81 commented Aug 7, 2017

@DeepDiver1975 please add unit test.

Moving to "planned" for next release for now

@PVince81 PVince81 modified the milestones: planned, development Aug 7, 2017
@felixboehm felixboehm added the p2-high Escalation, on top of current planning, release blocker label Oct 5, 2017
@DeepDiver1975
Copy link
Member Author

@DeepDiver1975 please add unit test.

due to the usage of filesystem setup code unit testing this is not easy/possible.
maybe some integration tests can help?

we any how need a proper concept on testing our commands.
maybe use https://symfony.com/doc/current/console.html#testing-commands

@DeepDiver1975
Copy link
Member Author

now with command tester 🎉

Copy link
Contributor

Choose a reason for hiding this comment

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

s/older then/older than/
and in the expected test result

Copy link
Member Author

Choose a reason for hiding this comment

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

thx

$this->commandTester->execute([]);
$output = $this->commandTester->getDisplay();
$this->assertContains("Cleaning chunks older than 2 days", $output);
$this->assertContains('Cleaning chunks for dav-clean-chunks-user', $output);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you somehow verify that the chunks were actually removed from disk ?

Copy link
Member Author

Choose a reason for hiding this comment

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

let me think about this .....

$this->loginAsUser($this->user->getUID());

// generate old chunks
$view = new View("/$userId/uploads");
Copy link
Member Author

Choose a reason for hiding this comment

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

@PVince81 now with real deletion of the upload folder

Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

@PVince81
Copy link
Contributor

My change requires a change to the documentation.

@DeepDiver1975 please raise/link to documentation ticket. We need to make sure admins know they need to add this to their cron.

@DeepDiver1975
Copy link
Member Author

@DeepDiver1975 please raise/link to documentation ticket. We need to make sure admins know they need to add this to their cron.

done

@PVince81
Copy link
Contributor

@settermjd
Copy link
Contributor

@DeepDiver1975, how about https://github.com/mikey179/vfsStream for assisting with testing?

@DeepDiver1975
Copy link
Member Author

interesting idea .... I try not to forget .... 😉

settermjd added a commit to owncloud-archive/documentation that referenced this pull request Nov 10, 2017
settermjd added a commit to owncloud-archive/documentation that referenced this pull request Nov 11, 2017
@lock
Copy link

lock bot commented Aug 2, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

4 - To release p2-high Escalation, on top of current planning, release blocker status/STALE

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ChunkingNG: Stray chunks are never deleted

7 participants