Skip to content
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

deluge 2 upgrade to python 3.10 fix for DSM7 (follow-up #4153) #5398

Merged
merged 31 commits into from
Aug 31, 2022

Conversation

th0ma7
Copy link
Contributor

@th0ma7 th0ma7 commented Aug 20, 2022

Co-Authored-by: hgy59 hpgy59@gmail.com
Co-Authored-by: Christophe David david.christophe@protonmail.com

Description

Upgrade deluge and migrate to DSM7

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)

Testing

  • x64-6.1 migrate/upgrade | install | upgrade
  • x64-7.0 install | upgrade
  • armv7-7.0 install | upgrade

th0ma7 and others added 2 commits August 20, 2022 20:25
Co-Authored-by: hgy59 <hpgy59@gmail.com>
Co-Authored-by: Christophe David <david.christophe@protonmail.com>
@th0ma7 th0ma7 self-assigned this Aug 20, 2022
@th0ma7
Copy link
Contributor Author

th0ma7 commented Aug 22, 2022

Manual daemon startup now works:

# sudo su -s /bin/bash sc-deluge -c '/volume1/@appstore/deluge/env/bin/deluged --config /volume1/@appstore/deluge/var --logfile /volume1/@appstore/deluge/var/deluge.log --loglevel info --pidfile /volume1/@appstore/deluge/var/deluge.pid'
# sudo su -s /bin/bash sc-deluge -c '/volume1/@appstore/deluge/env/bin/deluge-web --config /volume1/@appstore/deluge/var --logfile /volume1/@appstore/deluge/var/deluge-web.log --loglevel info --pidfile /volume1/@appstore/deluge/var/deluge-web.pid'

# ps -fu sc-deluge
UID        PID  PPID  C STIME TTY          TIME CMD
sc-delu+ 13918     1  1 09:01 ?        00:00:00 /volume1/@appstore/deluge/env/bin/python3 /volume1/@appstore/deluge/env/bin/deluged --config /volume1/@appstore/deluge/var --logfile /volume1/@appstore/del
sc-delu+ 17384     1 13 09:02 ?        00:00:00 /volume1/@appstore/deluge/env/bin/python3 /volume1/@appstore/deluge/env/bin/deluge-web --config /volume1/@appstore/deluge/var --logfile /volume1/@appstore/

Along with connecting to default web page http://192.168.x.y:8112/ using deluge as default passwd

@hgy59 finally got a success on this one! Left is changing the code in order to allow starting up multiple daemons.

@th0ma7
Copy link
Contributor Author

th0ma7 commented Aug 22, 2022

DONE:
[x] start both daemon successfully
[x] fixed rencode wheel issue with error [WARNING ][deluge.transfer :130 ] Failed to decompress (25 bytes) and load serialized data with rencode: rencode._rencode.loads() takes no keyword arguments using abi3 cp37 compatibility mode
[x] able to access web interface
[x] able to download torrent (tested using ubuntu & debian) - WOOOOT!
[x] fix PID handling - NON-daemonized mode
[x] fix multi-deamon start|stop - NON-daemonized mode
[x] fix multi-deamon start|stop - DAEMONIZED mode - Tested with tvheadend, dependant on var/${DNAME}.pid
[x] rework setup scripts related to permissions (e.g. DSM6 vs DSM7)

TODO:
none left?

@th0ma7
Copy link
Contributor Author

th0ma7 commented Aug 23, 2022

@hgy59 a second pair of eyes would be appreciated on this PR, mostly on spksrc.service.start-stop-status as this impacts all future packages.

I have completed the changes in order to allow managing multiple daemon for start|stop|status. It currently is tested for non-background mode with PID tracking (e.g. non-daemonized).

Still left are:

  • testing for daemonized mode with no PID tracking to confirm all is also good when using multi-daemons: DONE
  • testing with other packages that nothing breaks at start|stop|status using various scenarios: DONE

I'm getting real close to finally complete this long-standing PR 😄 .

@th0ma7 th0ma7 requested a review from hgy59 August 24, 2022 03:10
@th0ma7
Copy link
Contributor Author

th0ma7 commented Aug 25, 2022

@SynoCommunity/developers I need some help for DSM7, related to permission management.

When installing on DSM7 it fails miserably (I mean it says installation error). After investigating through the logs it seems related to permission management.

In this case, both DSM7 NAS I'm testing against are vanilla images, imaged from DSM7 natively (one virtual x64 and one physical armv7) using default configurations.

Issues I see while testing deluge are:

  1. group sc-download doesn't get created (nor user associated to)
  2. group synocommunity exists, but no user is attached to it (from /etc/group although manual id says otherwise).
  3. access to /volume1/downloads is restricted (e.g. default permissions)

Output from synoscgi.log:

2022-08-25T08:23:05-04:00 DSM7 synoscgi_SYNO.Core.Package.Installation_1_install[15388]: pkgverify.cpp:299 Failed to verify package, spk=[/volume1/@tmp/upload_tmp.152780] result=[{"action":"prepare","beta":false,"betaIncoming":false,"error":{"code":289,"description":"spk is not from synology"},"installReboot":false,"package":"deluge","packageName":"Deluge","stage":"prepare","success":false,"version":"2.1.1-15"}]
2022-08-25T08:23:06-04:00 DSM7 synoscgi_SYNO.Core.Package.Installation_1_install[15491]: share_default_get.c:347 failed to alloc share for [/volume1/downloads], errno=0x1200
2022-08-25T08:23:06-04:00 DSM7 synoscgi_SYNO.Core.Package.Installation_1_install[15491]: share_default_share_create.c:21 Failed to get defualt status for /volume1/downloads
2022-08-25T08:23:06-04:00 DSM7 synoscgi_SYNO.Core.Package.Installation_1_install[15491]: data_share.cpp:323 Failed to create /volume1/downloads [0x1200 share_alloc.c:68]
2022-08-25T08:23:07-04:00 DSM7 synoscgi_SYNO.Core.Package.Installation_1_install[15388]: pkginstall.cpp:1324 Failed to acquire resource after install deluge [0x2000 bdb_get.c:40]
2022-08-25T08:25:14-04:00 DSM7 synoscgi_SYNO.Core.Package.Installation_1_install[15388]: pkginstall.cpp:1058 Failed to install package, spk=[/volume1/@tmp/upload_tmp.152780] result=[{"action":"install","beta":false,"betaIncoming":false,"broken_by":"install_corruption","error":{"code":276,"description":"failed to acquire postinst worker","worker_msg":[]},"finished":true,"installReboot":false,"installing":true,"language":"fre","last_stage":"postreplace","package":"deluge","packageName":"Deluge","pid":15388,"scripts":[{"code":0,"message":"","type":"preinst"},{"code":0,"message":"","type":"postinst"}],"spk":"/volume1/@tmp/upload_tmp.152780","stage":"install_failed","status":"broken","status_code":150,"status_description":"failed to complete installation","success":false,"username":"th0ma7","version":"2.1.1-15"}]

Auto-generated work-x64-7.0/conf/resource:

{
  "port-config": {
    "protocol-file": "app/deluge.sc"
  },
  "data-share": {
    "shares": [
      {
        "name": "{{wizard_download_dir}}",
        "permission": {
          "rw": [
            "sc-deluge"
          ]
        }
      }
    ]
  }
}

id output from DSM7:

$ sudo su -s /bin/bash sc-deluge -c 'id'
uid=280533(sc-deluge) gid=207066(synocommunity) groups=207066(synocommunity),999(synopkgs)

In comparison on my DSM6 where it works like a charm:

  1. user sc-deluge is properly added to sc-download
  2. group synocommunity does not exists
  3. group deluge does exists
  4. access to /volume/downloads was already taken care of probably a long time ago.
    id output from DSM7:
$ sudo su -s /bin/bash sc-deluge -c 'id'
uid=280533(sc-deluge) gid=169500(deluge) groups=169500(deluge),65538(sc-download)

I've re-read the https://github.com/SynoCommunity/spksrc/wiki/Permission-Management and didn't find the right answer (or I misread). My guess is that I need a src/conf/privilege file describing what's needed but I couldn't find any example related to sc-download and al.

Help would be really much appreciated.

@Safihre
Copy link
Contributor

Safihre commented Aug 25, 2022

What you are seeing is exactly as they should be on DSM 7.. All the nice stuff we could do on DSM6, we cannot do anymore on DSM7.
The user has to manually make sure there is a group which has all the system-internal-accounts of the download programs that need to share files.
You can see that for example for my package SABnzbd the logic for DSM 7 doesn't do any permissions anymore:
https://github.com/SynoCommunity/spksrc/blob/master/spk/sabnzbd/src/service-setup.sh
Otherwise indeed it fails.

Sucks for the user, but it's safer because malicious packages can no longer hijack permissions all over the disk.

@th0ma7
Copy link
Contributor Author

th0ma7 commented Aug 25, 2022

Thnx @Safihre , it hapens that I just hit sabnzbd and I might use that as a starting point for DSM7.

else
date >> ${LOG_FILE}
fi
# Keep logs by default
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not change such a default behaviour!

  • packages might write a lot of log output
  • log is never deleted and old log is kept on package updates
  • logs are neigther rotated nor compressed by our generic service support
  • to report issues, the log since service start is sufficient and I assume that users will include the full log to issues reported here

If you have services that really should keep the logs, it would be better, we could write logs to /var/log/packages were those might get rotated (not verified yet).
IMHO the option SVC_QUIET = no is more for package development than for production. Currently no package defines SVC_QUIET, this shows that deleting the logs on service start is a good default behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll revert to the original behavior. Although while playing with the package I find it really anoying to remove thel previous log, always. The best would be to incorporate a logrotate by default. Page 107 for Syslog Config mentions something about that similar to (but that's for another time and PR):

"syslog-config": {
"logrotate-relpath": "etc/<appname>-logrotate.conf"
}

Beyond that, any thought otherwise on the proposed changes?

@th0ma7
Copy link
Contributor Author

th0ma7 commented Aug 27, 2022

@hgy59 this now LGTM and ready for review.

I'll resume testing on my DSM7 armv7 tomorrow as there is no error but downloads do not start. I wonder if its a configuration issue or just having too many deluge running on my LAN for testing and thus colliding on my router... Beyond that, running smoothly on DSM6 and DSM7 for x64.

EDIT: Issue found! This is cause by sabnzbd that isn't doing a mkdir -p -m 0775 to allow group synocommuniity read+write access in the downloads sub-directories. Will patch this part of this PR.

@th0ma7 th0ma7 requested a review from hgy59 August 27, 2022 22:50
@th0ma7
Copy link
Contributor Author

th0ma7 commented Aug 28, 2022

While playing with multiple install + upgrade on DSM6 I ended noticing something wrong with the setting up of the permissions which led into creating 40+ ACL entries on my /volume1/download directory:

2022/08/28 08:42:43	ACL version: 1 
2022/08/28 08:42:43	Archive: has_ACL,is_support_ACL 
2022/08/28 08:42:43	Owner: [admin(user)] 
2022/08/28 08:42:43	--------------------- 
2022/08/28 08:42:43		 [0] group:media:deny:rwxpdDaARWcCo:fd--  (level:0)
2022/08/28 08:42:43		 [1] group:administrators:allow:rwxpdDaARWc--:fd--  (level:0)
2022/08/28 08:42:43		 [2] group:sc-download:allow:r-x---a-R----:---n  (level:0)
2022/08/28 08:42:43		 [3] group:sc-download:allow:rwxpdDaARWcC-:fd--  (level:0)
2022/08/28 08:42:43		 [4] group:sc-download:allow:r-x---a-R----:---n  (level:0)
2022/08/28 08:42:43		 [5] group:sc-download:allow:rwxpdDaARWcC-:fd--  (level:0)
2022/08/28 08:42:43		 [6] group:sc-download:allow:rwxpdDaARWcC-:fd--  (level:0)
2022/08/28 08:42:43		 [7] group:sc-download:allow:r-x---a-R----:---n  (level:0)
2022/08/28 08:42:43		 [8] group:sc-download:allow:rwxpdDaARWcC-:fd--  (level:0)
2022/08/28 08:42:43		 [9] group:sc-download:allow:r-x---a-R----:---n  (level:0)
2022/08/28 08:42:43		 [10] group:sc-download:allow:rwxpdDaARWcC-:fd--  (level:0)
2022/08/28 08:42:43		 [11] group:sc-download:allow:r-x---a-R----:---n  (level:0)
2022/08/28 08:42:43		 [12] group:sc-download:allow:rwxpdDaARWcC-:fd--  (level:0)
2022/08/28 08:42:43		 [13] group:sc-download:allow:rwxpdDaARWcC-:fd--  (level:0)
2022/08/28 08:42:43		 [14] group:sc-download:allow:rwxpdDaARWcC-:fd--  (level:0)
2022/08/28 08:42:43		 [15] group:sc-download:allow:rwxpdDaARWcC-:fd--  (level:0)
2022/08/28 08:42:43		 [16] group:sc-download:allow:rwxpdDaARWcC-:fd--  (level:0)
2022/08/28 08:42:43		 [17] group:sc-download:allow:rwxpdDaARWcC-:fd--  (level:0)
2022/08/28 08:42:43		 [18] group:sc-download:allow:rwxpdDaARWcC-:fd--  (level:0)
2022/08/28 08:42:43		 [19] group:sc-download:allow:rwxpdDaARWcC-:fd--  (level:0)
2022/08/28 08:42:43		 [20] group:sc-download:allow:rwxpdDaARWcC-:fd--  (level:0)
2022/08/28 08:42:43		 [21] group:sc-download:allow:rwxpdDaARWcC-:fd--  (level:0)
2022/08/28 08:42:43		 [22] group:sc-download:allow:rwxpdDaARWcC-:fd--  (level:0)
2022/08/28 08:42:43		 [23] user:-------:allow:rwxpdDaARWc--:fd--  (level:0)
2022/08/28 08:42:43		 [24] user:----------:allow:rwxpdDaARWc--:fd--  (level:0)
2022/08/28 08:42:43		 [25] group:sc-download:allow:rwxpdDaARWc--:fd--  (level:0)
2022/08/28 08:42:43		 [26] group:sc-download:allow:r-x---a-R----:---n  (level:0)
2022/08/28 08:42:43		 [27] group:sc-download:allow:r-x---a-R----:---n  (level:0)
2022/08/28 08:42:43		 [28] group:sc-download:allow:r-x---a-R----:---n  (level:0)
2022/08/28 08:42:43		 [29] group:sc-download:allow:r-x---a-R----:---n  (level:0)
2022/08/28 08:42:43		 [30] group:sc-download:allow:r-x---a-R----:---n  (level:0)
2022/08/28 08:42:43		 [31] group:sc-download:allow:r-x---a-R----:---n  (level:0)
2022/08/28 08:42:43		 [32] group:sc-download:allow:r-x---a-R----:---n  (level:0)
2022/08/28 08:42:43		 [33] group:sc-download:allow:r-x---a-R----:---n  (level:0)
2022/08/28 08:42:43		 [34] group:sc-download:allow:r-x---a-R----:---n  (level:0)
2022/08/28 08:42:43		 [35] group:sc-download:allow:r-x---a-R----:---n  (level:0)
2022/08/28 08:42:43		 [36] group:sc-download:allow:r-x---a-R----:---n  (level:0)
2022/08/28 08:42:43		 [37] group:sc-download:allow:r-x---a-R----:---n  (level:0)
2022/08/28 08:42:43		 [38] group:sc-download:allow:r-x---a-R----:---n  (level:0)
2022/08/28 08:42:43		 [39] group:sc-download:allow:r-x---a-R----:---n  (level:0)
2022/08/28 08:42:43		 [40] group:sc-download:allow:r-x---a-R----:---n  (level:0)
2022/08/28 08:42:43		 [41] group:sc-download:allow:r-x---a-R----:---n  (level:0)
2022/08/28 08:42:43		 [42] group:sc-download:allow:r-x---a-R----:---n  (level:0)
2022/08/28 08:42:43		 [43] group:sc-download:allow:r-x---a-R----:---n  (level:0)
2022/08/28 08:42:43		 [44] group:sc-download:allow:r-x---a-R----:---n  (level:0)
2022/08/28 08:42:43	grep: mode": No such file or directory
2022/08/28 08:42:43	Granting 'sc-download' group permissions on /volume1/download

Notice that the important part of the installer log is:

2022/08/28 08:42:43	grep: mode": No such file or directory
2022/08/28 08:42:43	Granting 'sc-download' group permissions on /volume1/download/complete

There might have been a /bin/ash update during the DSM 6.x era leading to not be interpreting the following properly:

if [ ! "$(some \"thing\")" ]

which needs to be converting \" into ""

A patch is on the way as well for fixing this.

@th0ma7
Copy link
Contributor Author

th0ma7 commented Aug 28, 2022

@hgy59 Now fully complete and ready for merge.

@th0ma7
Copy link
Contributor Author

th0ma7 commented Aug 28, 2022

@Ber-Flo Feel free to give this a shot for testing. Wait for updated build to be available in the "Checks" summary page.

@Ber-Flo
Copy link

Ber-Flo commented Aug 28, 2022

@Ber-Flo Feel free to give this a shot for testing. Wait for updated build to be available in the "Checks" summary page.

x64-7.0 is working like a charm on DSM 7.1-42661, just had to install python3.2 and enable sc-deluge read/write rights for my target folder on DSM.

Thank you so much, it's the only DSM torrent client supporting socks5 proxy.

Merci :)

@th0ma7
Copy link
Contributor Author

th0ma7 commented Aug 28, 2022

x64-7.0 is working like a charm on DSM 7.1-42661, just had to install python3.2 and enable sc-deluge read/write rights for my target folder on DSM.

@Ber-Flo can you please confirm, you mention python3.2 but really that should be python 3.10...

Thank you so much, it's the only DSM torrent client supporting socks5 proxy.

Merci :)

Au plaisir :)

@Ber-Flo
Copy link

Ber-Flo commented Aug 29, 2022

x64-7.0 is working like a charm on DSM 7.1-42661, just had to install python3.2 and enable sc-deluge read/write rights for my target folder on DSM.

@Ber-Flo can you please confirm, you mention python3.2 but really that should be python 3.10...

3.10 not 3.20 indeed

@th0ma7 th0ma7 merged commit 85a79b1 into SynoCommunity:master Aug 31, 2022
@th0ma7 th0ma7 deleted the deluge-py310-try2 branch August 31, 2022 00:08
mkdir -p "${shared_folder}/incomplete"
mkdir -p "${shared_folder}/complete"
mkdir -p "${shared_folder}/watch"
install -m 0775 -o ${EFF_USER} -g ${GROUP} -d "${shared_folder}/incomplete"
Copy link
Contributor

Choose a reason for hiding this comment

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

@hgy59 Does this not break ACL permissions by forcing Linux-style permissions?

Copy link
Contributor Author

@th0ma7 th0ma7 Aug 31, 2022

Choose a reason for hiding this comment

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

The end result is identical to the mkdir being theses sub-directories are in Linux mode with no ACL under DSM7. As such it needs the 0775 + ${GROUP} to be set properly otherwise it won't work for other packages.

EDIT: Note that this was tested and behavior discovered on vanilla DSM7 on both x64 and armv7. I believe the workaround would be using the resources file by adapting the jq call in spksrc.service.mk so it can handle multiple arguments, even some relative to wizard variable in the Makefile such as:

SERVICE_WIZARD_SHARE  = wizard_download_dir
SERVICE_WIZARD_SHARE += wizard_download_dir/incomplete
SERVICE_WIZARD_SHARE += wizard_download_dir/complete
SERVICE_WIZARD_SHARE += wizard_download_dir/watch

I briefly looked at it and clearly my jq skillset is lacking... so I left it as is 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I will test if this is needed at all. Maybe those folders don't need to be created during install, because SABnzbd also creates them at startup. I think when SAB creates them at startup, they will be created in the right way with correct ACL.

@th0ma7 th0ma7 added status/published Published and activated (may take up to 48h until visible in DSM package manager) and removed status/needs-review labels Aug 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/published Published and activated (may take up to 48h until visible in DSM package manager)
Projects
None yet
4 participants