-
Notifications
You must be signed in to change notification settings - Fork 704
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
allow to exclude previews from backup upon instance backup #5892
allow to exclude previews from backup upon instance backup #5892
Conversation
40bc5d4
to
eeef048
Compare
Edit: I added |
5e354ea
to
0e471d4
Compare
Follow up of: nextcloud#5550 Signed-off-by: Paul <[email protected]>
0e471d4
to
ce5ac63
Compare
@szaimen Is there a way to test the changes on my side on my production instance? I tried looking into building the whole AIO thing but there doesn't seem to be an easy way to do that. The actual docker build pipeline seem to be defined in the I vaguely tried to pull the CI workflow change it to push to ghcr.io (to my own fork) instead of docker.io but the references to docker.io look farther reaching than just in the CI pipelines so I quickly gave up… |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @devnoname120 thanks a lot for the contribution and sorry for my late reply!
I unfortunately have some problems with this as in my opinion, the backup should always represent a full backup with intact database and file system and basically including all data needed to restore the same state without exception. This also includes the previews as their path is also written into the database. Allowing to exclude the previews means that we always need to scan the database upon restore which is hacky imho.
Also, maybe we could find a better way that allows to exclude previews and more without having to add an option to the interface for each thing? This could probably be implemented like this: #4751. Would you be able to work on this?
Please let me know what you think about my thoughts!
@szaimen I understand your point of view and I think that it's very sensible. From my side the motivation underlying this PR is that every day my server is down for 20 minutes due to the daily backup, and the absolutely huge number of thumbnail files is responsible for a good part of it. Furthermore, navigating my backups is extremely slow (almost a minute for the initial loading) — again due to these thumbnail files. Note that my Nextcloud instance and its data are fully stored on an SSD so there is not much I can do on that front. I'm not entirely sure about the point of backing up and restoring thumbnails as they are generated on the fly anyway so I think most users would want to exclude them (note that I didn't make it the default). While a more general approach for excluding things from backups would definitely be useful, I think that thumbnails are the killer use-case because of their massive footprint and unclear utility. I don't think of another obvious target that would have such a considerable impact. Finally, given that the thumbnail exclusion on restore is already implemented I think that mirroring the implementation for the backup side is reasonable as it makes the experience consistent. It does not mean that a more general exclusion feature won't be implemented at a later point, but this is additional work that I cannot take on my plate right now unfortunately. What do you think? Edit: I just ran the command |
I see. However this PR is very far from being finished as it still misses quite a few places in order to work correctly. The much easier and more forward-thinking solution would be to allow |
Could you expand on what is missing? I was under the impression that I got everything right |
I will not give further hints on the implementation, as this feature is not going to be merged, sorry. Please have a look at #4751 if you want to make it possible to ignore the folder during backup. |
I feel very frustrated because of the considerable waste of time and I'm not sure anyone will implement what you're asking for anytime soon, so big opportunity cost here. I will close this PR but I think it's sad. |
Follow up of: #5550
@szaimen I haven't tested the changes yet as I don't have the right setup… Is there any chance you could test them? Thanks!