Skip to content

Add occ command to expire the trashbin#25878

Merged
DeepDiver1975 merged 2 commits intomasterfrom
expiry-as-occ-command
Aug 23, 2016
Merged

Add occ command to expire the trashbin#25878
DeepDiver1975 merged 2 commits intomasterfrom
expiry-as-occ-command

Conversation

@DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 added this to the 9.2 milestone Aug 19, 2016
@mention-bot
Copy link

@DeepDiver1975, thanks for your PR! By analyzing the annotation information on this pull request, we identified @schiessle and @LukasReschke to be potential reviewers

@mmattel
Copy link
Contributor

mmattel commented Aug 19, 2016

Great !
Beside expiring the trashbin for a/all users, I would additionally propose the ability to see what and how much data will be deleted. With the current process I just say do it.
Therefore I propose:

  • --dryrun to see what would be deleted and how much data would be freed - without doing it
  • --verbose to see online what is deleted
  • in the results for the user how much actually has been deleted (number and quantity)
  • a summary when finished deleting x files for y users freeing z MB

$maxAge = $this->expiration->getMaxAgeAsTimestamp();
if (!$maxAge) {
$output->writeln("No expiry configured.");
// return;
Copy link
Contributor

Choose a reason for hiding this comment

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

stray return ?

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
Copy link
Contributor

@DeepDiver1975 what's there to discuss ? I heard the primary reason for adding this would be to be able to run OCC commands from cron to ease the load from cron ?

@PVince81
Copy link
Contributor

CC @cdamken

@cdamken
Copy link
Contributor

cdamken commented Aug 22, 2016

Awesome!! Thanks Guys!

@DeepDiver1975 I think this https://github.com/owncloud/enterprise/issues/1279 is would be fixed with this PR too

@cdamken
Copy link
Contributor

cdamken commented Aug 22, 2016

@carlaschroder This should be added in the documentation ;)

@DeepDiver1975
Copy link
Member Author

@carlaschroder This should be added in the documentation ;)

fore sure - once we agree on this ...

@cdamken
Copy link
Contributor

cdamken commented Aug 22, 2016

@DeepDiver1975 how about the versions?, could be added too?

@DeepDiver1975
Copy link
Member Author

how about the versions?, could be added too?

on it ....

@cdamken
Copy link
Contributor

cdamken commented Aug 22, 2016

on it ....

I own you some beers ;)

@DeepDiver1975
Copy link
Member Author

@mmattel thanks for your input - we can enhance this over time in follow up prs.

@mmattel
Copy link
Contributor

mmattel commented Aug 22, 2016

Ok to me,
pls consider that we use a different way defining users in eg files:scan (like I proposed above) than we do here. imho this should be consistant. would make future improvments also be consistant to other commands...


$maxAge = $this->expiration->getMaxAgeAsTimestamp();
if (!$maxAge) {
$output->writeln("No expiry configured.");
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm getting this now. I thought we had defaults ? There was a thing about expiring trashbin items whenever the trashbin size reached 50% of the total available size

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's new behavior, I can see that this command simply reflects the same behavior as the background job, so it's ok. Would be good to get some input from @VicDeo

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I understood the code this whole expiry is purly based on time ... but I maybe I missed something in this mess .....

Copy link
Member

@VicDeo VicDeo Aug 22, 2016

Choose a reason for hiding this comment

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

@PVince81 as per #16300 that I documented in https://github.com/owncloud/core/blob/master/config/config.sample.php#L415 a value of
auto guarantees neither keeping nor deletion. Items might be deleted if a disk usage is over 50% of quota.
While exact values specify number of days to store / to delete after for min/max respectively.
From that point auto value means the mode before retention obligation was implemented. It is limited by quota usage for trashbin and by by number of copies per different time ranges for versions

Copy link
Member

Choose a reason for hiding this comment

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

Anyway the best description was provided by @MTRichards in #16300 for Trashbin and #16301 for Versions.
This is actually a reference spec for retention

@PVince81
Copy link
Contributor

I tried setting "trashbin_retention_obligation" to "auto" but it still says it's unconfigured. A quick debug in the "Expiration" code shows that it keeps the max age to "no obligation". So it looks like the background job wouldn't expire the trashbin based on size even though the doc says it would when set to "auto".
Anyway, this is not related to this PR.

@PVince81
Copy link
Contributor

ok, looks like the size-based behavior is another single-shot expire job that gets scheduled only after a deletion.

@PVince81
Copy link
Contributor

This is the other expire job for size:

| 14 | OC\Command\CommandJob                             | "O:33:\"OCA\\Files_Trashbin\\Command\\Expire\":1:{s:39:\"\u0000OCA\\Files_Trashbin\\Command\\Expire\u0000user\";s:36:\"80cd743a-fcb2-1035-90e4-53fe065c9f7b\";}" |          0 |   1471881096 |           0 |
| 15 | OC\Command\CommandJob                             | "O:33:\"OCA\\Files_Trashbin\\Command\\Expire\":1:{s:39:\"\u0000OCA\\Files_Trashbin\\Command\\Expire\u0000user\";s:36:\"80cde582-fcb2-1035-90e5-53fe065c9f7b\";}" |          0 |   1471881193 |           0 |

Looks like it would be too tricky to add this behavior into this command as well.

Ok ok, I'll now test with numeric values.

@PVince81
Copy link
Contributor

Tested, works 👍

(it was really painful to manipulate timestamps in oc_filecache as I forgot to also re-set the path_hash since the timestamp is also in the filename)

@PVince81
Copy link
Contributor

Tested, works 👍

(it was really painful to manipulate timestamps in oc_filecache as I forgot to also re-set the path_hash since the timestamp is also in the filename)

@PVince81
Copy link
Contributor

PVince81 commented Aug 22, 2016

Ah, that was the trashbin stuff.

Now testing versions, let me put the steps now:

  1. Setup OC with Mysql or MariaDB
  2. Set "versions_retention_obligation" to "auto, 1" in config.php
  3. Open text editor
  4. Edit and save three times to create versions
  5. Go to the data folder of the user in "data/$user/files_versions"
  6. Pick one of the versions file, in my case I picked "test.txt.v1471883185"
  7. Rename the file to "test.txt.v1371883185" (changed one digit)
  8. update oc_filecache set name=replace('v1471883185', 'v1371883185');
  9. (and that's the moment I started to really hate the filecache and its info redundancy)
  10. update oc_filecache set path=replace(path, 'v1471883185', 'v1371883185');
  11. update oc_filecache set path_hash = md5(path) where path_hash <> md5(path);
  12. update oc_filecache set mtime=1371883185 where mtime=1471883185;
  13. select * from oc_filecache where name like '%v1371883185'; => see the entry exists
  14. Run occ versions:expire
  15. select * from oc_filecache where name like '%v1371883185'; => entry is gone and file is gone from data dir

@PVince81
Copy link
Contributor

@DeepDiver1975 fixed a bad copy-paste mistake to make version expire work: 3d618a1

@PVince81
Copy link
Contributor

Both trashbin expire and version expire work now 👍

@butonic
Copy link
Contributor

butonic commented Aug 23, 2016

can we make https://github.com/owncloud/core/blob/master/apps/files_trashbin/lib/BackgroundJob/ExpireTrash.php#L34 reuse the new commend? same with versions? otherwise we just duplicate code.

@butonic
Copy link
Contributor

butonic commented Aug 23, 2016

👍 deduplicating would need further tweaking of the CommandJob. And increase complexity.
backport request to 9.0

@DeepDiver1975
Copy link
Member Author

deduplicating would need further tweaking of the CommandJob.

yes - there are many similarities in the way we implement occ commands, bachgroundjobs and repairsteps. We should think about implementing on single interface which can be reused in all three areas.

@DeepDiver1975 DeepDiver1975 merged commit 4f73fb4 into master Aug 23, 2016
@DeepDiver1975 DeepDiver1975 self-assigned this Aug 23, 2016
@DeepDiver1975 DeepDiver1975 deleted the expiry-as-occ-command branch August 23, 2016 12:03
@DeepDiver1975
Copy link
Member Author

preparing backport ...

DeepDiver1975 added a commit that referenced this pull request Aug 23, 2016
* Add occ command to expire the trashbin

* Fix versions folder in setup check
@DeepDiver1975
Copy link
Member Author

DeepDiver1975 commented Aug 23, 2016

DeepDiver1975 added a commit that referenced this pull request Aug 23, 2016
* Add occ command to expire the trashbin

* Fix versions folder in setup check
@cdamken
Copy link
Contributor

cdamken commented Aug 23, 2016

Nice work! 👍

DeepDiver1975 added a commit that referenced this pull request Aug 23, 2016
* Add occ command to expire the trashbin

* Fix versions folder in setup check
PVince81 pushed a commit that referenced this pull request Aug 24, 2016
…d20964cfd0f7129f998b1

[stable9.1] Add occ command to expire the trashbin (#25878)
PVince81 pushed a commit that referenced this pull request Aug 24, 2016
* Add occ command to expire the trashbin

* Fix versions folder in setup check
PVince81 pushed a commit that referenced this pull request Aug 24, 2016
…be58a2ac5542741378c25

[stable8.2] Add occ command to expire the trashbin (#25878)
@lock
Copy link

lock bot commented Aug 4, 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 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants