[framework] syncthing: fix DSM7 package upgrade

[hgy59]

Description

Fix DSM7 installer

• avoid overwrite of existing files in var folder
• fix recognition of dsm6 var folder

Fixes #5116

Checklist

• Build rule all-supported completed successfully
• New installation of package completed successfully
• Package upgrade completed successfully (Manually install the package again)
• Package functionality was tested
• Any needed documentation is updated/created

Type of change

• Bug fix
• New Package
• Package update
• Includes small framework changes
• This change requires a documentation update (e.g. Wiki)

[hgy59]

@th0ma7 with #4997 you introduced rsync calls to migrate the var folder of spk to the DSM7 var folder. This works as expected except for upgrade of packages created before.

Without this fix, the target/var folder (that was not removed in packages created without the rsync migration) was taken as dsm6 folder and existing config files were overwritten with the default files of the former installation.
As it is not possible to modify already installed packages, the recognition of dsm 6 installation cannot only look for target/var folder but must check whether {package}/var is linked to target/var.

DSM 7 packages created before the rsync solution (like syncthing 1.17.0-22 created at 2021-06-19) do not delete the target/var folder.

[hgy59]

@acolomb DSM 7 package updates are working now. I did intense testing under DSM 7 (including DSM 6 update simulation by recreation of former folder structure).

I am just wondering whether is was a good idea to create the sync folders in the package var folder (home). Before DSM7 this folder is backuped an restored at every package upgrade and it is removed when you uninstall the package.

A better approach would be to use a DSM shared folder.

• shared folders are never removed on uninstallation of a package
• when you try to remove a shared folder in DSM, you get an error telling that the shared folder is in use by the respective package(s).
• you can give dedicated permissions to DSM users for the folders in such a share.
• on DSM < 7 there is no need to backup and restore the folders upon synocommunity package update (no issue with syncthing internal update).

[acolomb]

Thanks @hgy59 for looking into it. Sounds reasonable to me so far.

I don't know what you mean exactly by "creating sync folders". Syncthing can be pointed at ANY existing folder path on the file system. It is not limited to it's "own" sharing volume / folder or whatever. Contrary to e.g. Dropbox where all shared stuff must live under one folder. So if you mean the default location where new shared folders are suggested to be created, that should be pointed at a reasonable location. I don't have enough overview of DSM to judge where that would be.

[hgy59]

So if you mean the default location where new shared folders are suggested to be created, that should be pointed at a reasonable location. I don't have enough overview of DSM to judge where that would be.

Thanks for this information. For my tests I created two folders and for me it looked like the folder path is not editable.
I just created a new DSM shared folder and gave R/W access for the sc-syncthing user. This is working like a charm. So as you said, you have no limitations to choose the sync folders. 😄

[acolomb]

I routinely add the double spaces after a full-stop btw. because it's an accepted norm (one of several) in English writing. And my editor (Emacs) treats it automatically when reflowing text.

[hgy59]

in #5118 I numbered the change log entries, so this gets obsolete.

[acolomb]

Damn, my review was 30 seconds too late! @hgy59

Please check out the comment about SERVICE_CERT_RELOAD before publishing!

[hgy59]

oops I was too fast...

[th0ma7]

@th0ma7 with #4997 you introduced rsync calls to migrate the var folder of spk to the DSM7 var folder. This works as expected except for upgrade of packages created before.

Without this fix, the target/var folder (that was not removed in packages created without the rsync migration) was taken as dsm6 folder and existing config files were overwritten with the default files of the former installation. As it is not possible to modify already installed packages, the recognition of dsm 6 installation cannot only look for target/var folder but must check whether {package}/var is linked to target/var.

DSM 7 packages created before the rsync solution (like syncthing 1.17.0-22 created at 2021-06-19) do not delete the target/var folder.

@hgy59 (late to reply, limited cycles available currently)

I recall testing this throughout but DSM6 to DSM7 isn't an easy thing to fully test. If I understand this correctly, and this applies to synchting (but may apply elsewhere), after upgrading to DSM7, an existing DSM6 syncthing installation would look like this?

/var/packages/synchthing/target -> /volume1/@appstore/synchthing
/var/packages/synchthing/var -> /volume1/@appstore/synchthing/var

While a DSM7 package would normally look like (notice the appstore vs appdata):

/var/packages/synchthing/target -> /volume1/@appstore/synchthing
/var/packages/synchthing/var -> /volume1/@appdata/synchthing

Which could lead at upgrade time that rsync might try to re-sync over itself? Am I painting the right picture?

... Actually no, as var data gets move to a temporary directory it would then overwrite the "new" packaged var content over the new files from the updated package in target/var...

Would that mean:

1. First ensuring that /var/packages/synchthing/var -> /volume1/@appdata/synchthing does exists OR creating it otherwise. Which would mean something such as:

if [ -h /var/packages/synchthing/var -a -d $(readlink /var/packages/synchthing/var) ]; then OK else create symlink.

2. Secondly, move the temporary backup to the "newly" created destination
3. Move along with the rsync as planned.

(re-reading your post, again)

dsm6 folder and existing config files were overwritten with the default files of the former installation

Then rewinding for a bit, if that was the expected behavior, then my PR changed that behavior as updated var files do not overwrite pre-existing configurations but rather creates .new when a file is pre-existing. Re-visiting that could would then lead to something like:

1. step 1: confirm var is ready
2. Do the rsync of target/var to var
3. re-use the rsync method but instead of creating .new create .old instead by moving old configurations over the new ones.

Is there a need for me to review the code change (as already merged)? If so I can try to find additional spare cycles over the week-end and go through the code.

[th0ma7]

Interesting snipet, good catch.

[hgy59]

I recall testing this throughout but DSM6 to DSM7 isn't an easy thing to fully test. If I understand this correctly, and this applies to synchting (but may apply elsewhere),

All packages for DSM 7 are affected, that are created before 15 december 2021 (the date #4997 was merged) and are not updated after this date.

To fully understand what was going wrong, you have to consider the following:

1. only DSM 7 installer is affected
2. the DSM installer extracts the spk content (package.tgz) to the target folder. i.e. the var folder in the package goes to target/var
3. since installer.dsm7: Use rsync to synchronize var directory #4997 the target/var folder is deleted after sync into the new var folder (those are never the same location under DSM 7)
4. before installer.dsm7: Use rsync to synchronize var directory #4997 the target/var folder was not deleted by the DSM 7 installer
5. the temporary directory is used only for DSM6 -> DSM7 migration, the var folder sync goes directly from target/var to the new var folder
6. Important: if we find a target/var folder in preupgrade (2. is not yet performed) it can be a leftover of a DSM 7 installation or from DSM 6
7. The sole error was in preupgrade to take the target/var that was left from a DSM 7 installer as DSM 6 var folder.
8. The correct recognition whether target/var is from DSM 6 in preupgrade fixes it all

sorry for not responding to your detailed questions

[hgy59]

Is there a need for me to review the code change (as already merged)? If so I can try to find additional spare cycles over the week-end and go through the code.

no need for review other than for fully understanding what's happening 😄

[hgy59]

#4702 is probably realted (but it is for DSM 6)