os-nextcloud-backup 1.1 changes modus operandi and causes problems for existing backup schemes

[meyergru]

Important notices
Before you add a new report, we ask you kindly to acknowledge the following:

• I have read the contributing guide lines at https://github.com/opnsense/plugins/blob/master/CONTRIBUTING.md
• I have searched the existing issues, open and closed, and I'm convinced that mine is new.
• When the request is meant for an existing plugin, I've added its name to the title.

Is your feature request related to a problem? Please describe.

This pull request: #4952 updated os-nextcloud-backup from1.0_1 to 1.1 and changed its way of operation in that:

1. The generated filenames do not include the machine name any more. If multiple machines are backup up to the same target, files cannot be attributed to their source any more.
2. Generated filenames do not include a human-readble timestamp any more, so that is much harder to find a suitable restore point in case one knows the date before something went awry.
3. There is not one file per backup any more, such that automatic rules to delete older files after a time limit or only retain a fixed number of files, break.
4. I also had sftp and git backup enabled, which in combination with 3. resulted in two files (symlinks?) named sftp and git were present in the backup folder. I think this has to do with multiple files of all revisions are now being uploaded.

All in all, my impression is that there was an incompatible change introduced which serves a specific purpose, but breaks many others, see also:

https://forum.opnsense.org/index.php?topic=50636.0

Describe the solution you'd like

IMHO, the change should be undone completely or superseded by a means to have the new scheme as an option only, not as the new default on how this thing works.

Describe alternatives you've considered
None

Additional context

You can have the old behaviour back by using:

opnsense-patch -c update fbe231e8f5
opnsense-code -uo sysutils/nextcloud-backup -r 25.7 plugins
pkg lock os-nextcloud-backup-1.0_1
configctl webgui restart

which replaces the new package and locks it until this will be reverted or fixed.

[fichtner]

CC @fabianfrz @Nuadh123

[Nuadh123]

There's nothing stopping people from adding/changing the behaviour of the plugin? It is open source after all. Adding the hostname to the filename is not impossible, and patches are welcome. I scratched an itch I had, and the plugin was mostly abandoned.

The original plugin never made "one file", it made one file each day, regardless of if it was modified or not. I found that annoying, other people might not:)

A UNIX timestamp is not really a "non-sensical number" (and is easily sortable, just like the ISO 8601 date format).

If another plugin keeps changing the main file because it wants to use it as a database, I would talk to the developer of that plugin (not that I'm an authority on what you can use the config file for).

[meyergru]

I think that argument can be opposed. As of now, I see one person advocating for your change (i.e. you) and at least 7 in the thread alone against it, maybe more, when they actually notice the change.

The problem is that the semantics of an existing tool for all of its users changed because of "an itch that you had" - aka "breaking change".

How would you like the semantics being reverted? Suddenly your use case would break, imagine that for a start.

And yes, the plugin made one backup each day before., just like the other backup plugins. Now it makes as many backups as individual changes have been made. Last time I checked, it was 50 changes in one day for me.

If you can read unix timestamps to select a specific day to restore, then good for you, I have to use a tool for that.

But actually, I do not think it was your fault, the maintainer should have noticed the obvious problems.

[fichtner]

I think that argument can be opposed. As of now, I see one person advocating for your change (i.e. you) and at least 7 in the thread alone, maybe more, when they actually notice the change.

Friendly reminder that this isn't a power play. We're here to coordinate changes and then someone will raise a reasonable PR and then we'll review.

FWIW, the design pattern of copying the /conf/backup layout is NOT a bad choice given the file-based constraints of such a backup location, but apparently not what people accustomed to the plugin want to dive into.

Adding a hostname is as easy as adding an additional directory name and timestamps can be converted. We could also only upload the latest backup in the store.

Just some constructive things...

Cheers,
Franco

[meyergru]

That is why I suggested having means to choose the behavior, say, a filename pattern with variables like $hostname and $timestamp / $datetime and an option to choose between "all revisions" or "last version". I just wanted to discuss ideas to have the expected behavior back, which does not exclude having the new one as an option.

[Nuadh123]

----8<----->8----

How would you like the semantics being reverted? Suddenly your use case would break, imagine that for a start.

Not the end of the world, the itch would be back, and I could deal with it, as I did.

----8<----->8----

And yes, the plugin made one backup each day before., just like the other backup plugins. Now it makes as many backups as individual changes have been made. Last time I checked, it was 50 changes in one day for me.

1: the gdrive plugin doesn't, or at least doesn't seem to (disclaimer; I haven't tried it, just briefly looked at the code)
2: take that up with whatever is using the config as a database, not me

Frankly, I'm on your side.. I hate it when things change for no reason. I (or maybe the maintainer/original developer) could've said "good idea, please implement it as a separate plugin".

[Nuadh123]

That is why I suggested having means to choose the behavior, say, a filename pattern with variables like $hostname and $timestamp / $datetime and an option to choose between "all revisions" or "last version". I just wanted to discuss ideas to have the expected behavior back, which does not exclude having the new one as an option.

I like that idea. The easiest would be targetting a directory with the name derived from the hostname of the opnsene instance, since that requires no mapping or checking the contents of files.

I'm no fan of backup systems overwriting files on the target medium, but that's a "me" thing, maybe.

The current implementation just steals whatever is in /conf/, and that is a configurable amount.

If anyone have suggestions, please post suggestions, patches, open PRs, be a good netizen:)

[meyergru]

I am a fan of specify-before-code... ;-)

IDK if all NextCloud providers allow to create subdirectories - if not, a directory name as prefix could cause problems.

The string for filename derivation could also be constructed from checkboxes only, like:

a. Include Hostname
b. Use datetime (YYYYMM...) instead of Unix timetamps
c. Save latest version only

With all options set, you would have the old behavior. Also, that would prevent user errors with specifying filename expressions.

[Nuadh123]

----8<----->8----

IDK if all NextCloud providers allow to create subdirectories - if not, a directory name as prefix could cause problems.

I can't think of any reason you cannot create directories inside directories if you want to. The original plugin created the full path in one go (at least if the comment in the code is to trust). Checking and creating as you traverse the full path is always an option if you need to.

The string for filename derivation could also be constructed from checkboxes only, like:

a. Include Hostname b. Use datetime (YYYYMM...) instead of Unix timetamps c. Save latest version only

This complicates the plugin a whole lot, if you want to reduce the number of unneeded copies of the files. Either you need to save a map of /conf/backup/filename to $remote_filename somewhere, compare file contents, or just do as it used to do; upload current config to a new file whenever the backup is running.

With all options set, you would have the old behavior. Also, that would prevent user errors with specifying filename expressions.

My suggestion would be to have checkbox:

• save backups in subdir named after hostname

have option box (choose one of two):

• snapshot config every day, and name file according to date (going back to the old way)
• copy system config backups (as it is now)

the checkbox option is of course possible to do today; by specifying a target directory with the name you want. For people with multiple machines and having configs distributed by puppet or the likes, I can see the fancy for a more generic approach to having the directories named something sensible, even with a config that looks identical from a deployment pov.

[muchachagrande]

May be this is a dumb suggestion, but what about using the file time stamp to generate the backup name. This way, if the configuration didn't change, the file name wont either.

[Nuadh123]

May be this is a dumb suggestion, but what about using the file time stamp to generate the backup name. This way, if the configuration didn't change, the file name wont either.

This is how it currently works, except for using a UNIX timestamp. Renaming is not very complex, so that's a valid strategy.

[fabianfrz]

As this is already as a discussion in progress, I can give some insights in the questions:

The generated filenames do not include the machine name any more. If multiple machines are backup up to the same target, files cannot be attributed to their source any more.

The orignal file name was something I came up to be usable and at the same time unique.

Generated filenames do not include a human-readble timestamp any more, so that is much harder to find a suitable restore point in case one knows the date before something went awry.

Nextclould should show the upload time human readable. However it is true that it is not very comfortable if you sync them to somewhere else and there the upload timestamp does not exist. Also the original filename caused a lot of drama because the timestamp caused problems with the Windows sync client. I actually don't know if this has been already fixed in the Nextcloud client.

Also I guess that the hostname should be part of the file names again if that is the biggest issue. However different directories should already have been possible in the previous version. So from my point of view, the best option would be to allow config-hostname-xxx to have them also sortable.

ISO date in UTC would be actually a good option but one thing is a problem: Columns were the problem in a previous version of the plugin.

There is not one file per backup any more, such that automatic rules to delete older files after a time limit or only retain a fixed number of files, break.

The original implementation was that nexclould can schedule automatic deletion of files after x days.

I also had sftp and git backup enabled, which in combination with 3. resulted in two files (symlinks?) named sftp and git were present in the backup folder. I think this has to do with multiple files of all revisions are now being uploaded.

If you look at the plugins, they create a repository or other working directory in the /config directory.

• https://github.com/opnsense/plugins/blob/master/sysutils/git-backup/src/opnsense/mvc/app/library/OPNsense/Backup/Git.php#L138
• https://github.com/opnsense/plugins/blob/master/sysutils/sftp-backup/src/opnsense/mvc/app/library/OPNsense/Backup/Sftp.php#L184

@Nuadh123 Maybe a good idea would be to exclude the /conf/backup directory?

[Nuadh123]

--------8<---------snip--------->8--------

@Nuadh123 Maybe a good idea would be to exclude the /conf/backup directory?

the current implementation reads the content by "scandir('/conf/backup/');", it doesn't discriminate on types of content in the directory. it should probably only copy files, yes

[meyergru]

Yes, that would probably help with problem no. 4 from the report.

[Nuadh123]

#5186