Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

ownCloud: Update PHP FPM in DSM 6 #5920

Merged
merged 9 commits into from
Nov 21, 2023

Conversation

mreid-tt
Copy link
Contributor

@mreid-tt mreid-tt commented Nov 8, 2023

Description

This is a follow-on from #5912 which fixes the PHP FPM handling in DSM 6 by making all functions run as the http user. Code has also been added to revert modification to PHP template defaults from previous versions.

Fixes #

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

@mreid-tt mreid-tt force-pushed the owncloud-update branch 3 times, most recently from 8d6d016 to ea00f03 Compare November 8, 2023 07:56
@mreid-tt mreid-tt self-assigned this Nov 8, 2023
@mreid-tt mreid-tt mentioned this pull request Nov 8, 2023
6 tasks
@mreid-tt mreid-tt requested a review from hgy59 November 8, 2023 08:29
@mreid-tt mreid-tt force-pushed the owncloud-update branch 2 times, most recently from 7c2df48 to 03be053 Compare November 8, 2023 16:03
@mreid-tt
Copy link
Contributor Author

mreid-tt commented Nov 8, 2023

@hgy59, the main intent of this PR is to convert the behaviour of the DSM 6 deployment of the web interface. Previously, my original workaround tried to mirror the DSM 7 behaviour where we have all the files owned by the package user sc-owncloud. This unfortunately also required the modification of the PHP FPM to also run as this user.

When considering a review of the work in #5862 I realised that having other packages follow this same path would create a major issue since the PHP FPM can only run as one user account. Given that by default it runs as http, I modified the code for the data share and the executables in addition to the files to all run under this user in DSM 6.

What made the process more complex is the handling of previous deployments which ran as the package user account. This required the implementation of a migration strategy from the old to the new user account (including the data share), as well as mitigations for any backups previously executed with the old account.

Finally, what became challenging was that after making changes to the web interface in DSM 6, I would usually trigger a restart of Apache to refresh the server including the PHP FPM configurations. I found that once there were instances of PHP FPM configurations outside of the one for this package, the installer would wait endlessly for that other package to stop which didn't work. As a rough workaround, I've added a check for this and the installer log includes a warning that the DSM needs to be rebooted for the new configs to be fully loaded.

The end user experience if this occurs is that the web interface either does not load or throws a PHP extension error. I admit this is not ideal and perhaps we could have something in the wizard to warn about this but I know this would add even more bulk to the wizard which I was hesitant to do. I also considered that there may not be many users that run combinations of ownCloud and Selfoss so it may not come up that often.

Let me know your thoughts on the approach.

EDIT: I've added a check for multiple PHP FPM configurations in the installation wizard which will advise users that a restart is required after installing the package if multiple configurations are present.

@mreid-tt
Copy link
Contributor Author

@hgy59, I'd like to publish this version and then deactivate version 10.13.2-12. let me know if you have any objections moving forward given the summary above.

@mreid-tt mreid-tt removed the request for review from hgy59 November 21, 2023 17:20
@mreid-tt mreid-tt merged commit 37c5640 into SynoCommunity:master Nov 21, 2023
@mreid-tt mreid-tt deleted the owncloud-update branch November 21, 2023 17:20
@mreid-tt mreid-tt added status/published Published and activated (may take up to 48h until visible in DSM package manager) and removed status/to-publish labels Nov 21, 2023
@mreid-tt
Copy link
Contributor Author

@hgy59, I've successfully published the latest update, and it's visible in my NAS. Could you please remove version 10.13.2-12 from the repository entirely? I've already deactivated it, but I'd appreciate it if you could take care of the removal when you have some free time. Thanks!

@hgy59
Copy link
Contributor

hgy59 commented Nov 22, 2023

@mreid-tt I prefer to wait another day or two until the current version is proved to be visible in the package center of DSM on the devices.
This helps to avoid discussions when someone wants to installal the version 10.13.2-12 as newest and gets download errors (my devices currently still show this version as newest in package center).

@hgy59
Copy link
Contributor

hgy59 commented Nov 23, 2023

@mreid-tt deleted owncloud 10.13.2-12 since all of my devices show 10.13.2-13 in the package center.

@mreid-tt
Copy link
Contributor Author

@mreid-tt deleted owncloud 10.13.2-12 since all of my devices show 10.13.2-13 in the package center.

Thanks @hgy59, appreciate it.

@mreid-tt mreid-tt mentioned this pull request Dec 9, 2023
6 tasks
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
status/published Published and activated (may take up to 48h until visible in DSM package manager)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants