Add cleanup tasks to remove stale objects #3950
Conversation
CHANGES/3949.feature
Outdated
| @@ -0,0 +1 @@ | |||
| Added periodically executed cleanup tasks for uploads and temporary files. | |||
There was a problem hiding this comment.
Does it make sense to mention the setting in the changelog as well?
Set the *_PROTECTION_TIME settings to None in order to disable this behaviour.
Besides that, I, personally, would make this behaviour disabled by default for a couple of releases. Just in case.
There was a problem hiding this comment.
Could you also add a note to the upload workflow (I think it is in the pulp_file repository? Not sure if it is even in our docs because we use CLI and uploads are translated to artifacts automatically) that as of now we will remove uncommitted uploads?
|
|
||
| def upload_cleanup(): | ||
| expiration = timezone.now() - timezone.timedelta(minutes=settings.UPLOAD_PROTECTION_TIME) | ||
| qs = Upload.objects.filter(pulp_created__lt=expiration) |
There was a problem hiding this comment.
Having expiration equal to timezone.now() (settings.UPLOAD_PROTECTION_TIME=0) results in pulp_create__lt always being less than expiration, so we will remove uploads even though users opted for not doing that. Is my understanding correct? If yes, we need to fix it.
There was a problem hiding this comment.
Not as such. Having it 0 will unschedule the task, but yes, there may be a window where things go south.
I'll find a different solution.
There was a problem hiding this comment.
It is now checked to be > 0.
Uploads and PulpTemporaryFiles are cleaned up. Set the *_PROTECTION_TIME settings to None in order to disable this behaviour. fixes pulp#3949
|
|
||
| Pulp uses ``uploads`` and ``pulp temporary files`` to pass data from the api to worker tasks. | ||
| These options allow to specify a timeinterval in minutes used for cleaning up stale entries. If | ||
| set to 0, automatic cleanup is disabled, which is the default. |
There was a problem hiding this comment.
sorry for my late 2c, but this a bit contradicts( or confuses?) to our similar setting orphan_protection_time. when that one set to 0 it means the orphans are not protected not that setting/cleanup is disabled .
How can we make this more consistent? The naming for all these 3 settings suggests they work in a similar way.
There was a problem hiding this comment.
It's not a contradiction, but i can see it being confusing. And I must confess, being similar is what inspired me in the first place to use similar names. The biggest difference is probably that we do not have a scheduled task for orphan cleanup (not that this would be impossible...).
Is there any way you think we could improve the documentation here?
There was a problem hiding this comment.
I hear where you're coming from.
Unfortunately, I don't see what can be improved in the documentation. Documentation is well written per se, and if users read it, it's ok even to have behavioral differences between the settings which have similar name.
| @@ -240,8 +240,10 @@ | |||
|
|
|||
| WORKER_TTL = 30 | |||
|
|
|||
| # how long to protect orphan content in minutes | |||
| # how long to protect ephemeral items in minutes | |||
There was a problem hiding this comment.
especially this comment suggests that having UPLOAD_PROTECTION_TIME = 0 uploads are not protected and removed ASAP
Uploads and PulpTemporaryFiles are cleaned up. Set the *_PROTECTION_TIME settings to None in order to disable this behaviour.
fixes #3949