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

nextcloud module: init #47159

Merged
merged 3 commits into from Oct 3, 2018
Merged

nextcloud module: init #47159

merged 3 commits into from Oct 3, 2018

Conversation

eqyiel
Copy link
Contributor

@eqyiel eqyiel commented Sep 22, 2018

Motivation for this change

This is #44994 with the requested changes from reviews.

I don't think I have permission to push to that branch so I'm doing it this way.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Fits CONTRIBUTING.md.

@eqyiel
Copy link
Contributor Author

eqyiel commented Sep 22, 2018

@Mic92
Copy link
Member

Mic92 commented Sep 22, 2018

@GrahamcOfBorg test nextcloud

@GrahamcOfBorg
Copy link

No attempt on x86_64-linux (full log)

The following builds were skipped because they don't evaluate on x86_64-linux: tests.nextcloud

Partial log (click to expand)

while evaluating the attribute 'isDefined' at /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/builder-0-gustav.ewr1.nix.ci/lib/modules.nix:374:5:
while evaluating the attribute 'values' at /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/builder-0-gustav.ewr1.nix.ci/lib/modules.nix:362:9:
while evaluating the attribute 'values' at /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/builder-0-gustav.ewr1.nix.ci/lib/modules.nix:458:7:
while evaluating anonymous function at /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/builder-0-gustav.ewr1.nix.ci/lib/modules.nix:348:28, called from /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/builder-0-gustav.ewr1.nix.ci/lib/modules.nix:348:17:
while evaluating 'dischargeProperties' at /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/builder-0-gustav.ewr1.nix.ci/lib/modules.nix:416:25, called from /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/builder-0-gustav.ewr1.nix.ci/lib/modules.nix:349:62:
while evaluating the attribute 'value' at /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/builder-0-gustav.ewr1.nix.ci/lib/modules.nix:232:58:
while evaluating the attribute 'config.script' at /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/builder-0-gustav.ewr1.nix.ci/nixos/modules/services/web-apps/nextcloud.nix:255:11:
while evaluating the attribute 'text' of the derivation 'nextcloud-autoconfig.php' at /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/builder-0-gustav.ewr1.nix.ci/pkgs/stdenv/generic/make-derivation.nix:177:11:
cannot coerce null to a string, at /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/builder-0-gustav.ewr1.nix.ci/nixos/modules/services/web-apps/nextcloud.nix:236:20

@GrahamcOfBorg
Copy link

No attempt on aarch64-linux (full log)

The following builds were skipped because they don't evaluate on aarch64-linux: tests.nextcloud

Partial log (click to expand)

while evaluating the attribute 'isDefined' at /var/lib/gc-of-borg/nix-test-rs-23/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/grahamc-aarch64-community-23/lib/modules.nix:374:5:
while evaluating the attribute 'values' at /var/lib/gc-of-borg/nix-test-rs-23/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/grahamc-aarch64-community-23/lib/modules.nix:362:9:
while evaluating the attribute 'values' at /var/lib/gc-of-borg/nix-test-rs-23/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/grahamc-aarch64-community-23/lib/modules.nix:458:7:
while evaluating anonymous function at /var/lib/gc-of-borg/nix-test-rs-23/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/grahamc-aarch64-community-23/lib/modules.nix:348:28, called from /var/lib/gc-of-borg/nix-test-rs-23/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/grahamc-aarch64-community-23/lib/modules.nix:348:17:
while evaluating 'dischargeProperties' at /var/lib/gc-of-borg/nix-test-rs-23/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/grahamc-aarch64-community-23/lib/modules.nix:416:25, called from /var/lib/gc-of-borg/nix-test-rs-23/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/grahamc-aarch64-community-23/lib/modules.nix:349:62:
while evaluating the attribute 'value' at /var/lib/gc-of-borg/nix-test-rs-23/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/grahamc-aarch64-community-23/lib/modules.nix:232:58:
while evaluating the attribute 'config.script' at /var/lib/gc-of-borg/nix-test-rs-23/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/grahamc-aarch64-community-23/nixos/modules/services/web-apps/nextcloud.nix:255:11:
while evaluating the attribute 'text' of the derivation 'nextcloud-autoconfig.php' at /var/lib/gc-of-borg/nix-test-rs-23/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/grahamc-aarch64-community-23/pkgs/stdenv/generic/make-derivation.nix:177:11:
cannot coerce null to a string, at /var/lib/gc-of-borg/nix-test-rs-23/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/grahamc-aarch64-community-23/nixos/modules/services/web-apps/nextcloud.nix:236:20

@Mic92
Copy link
Member

Mic92 commented Sep 22, 2018

The test does not work yet:

cannot coerce null to a string, at /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/builder-0-gustav.ewr1.nix.ci/nixos/modules/services/web-apps/nextcloud.nix:236:20

@eqyiel
Copy link
Contributor Author

eqyiel commented Sep 23, 2018

@Mic92 sorry about that.

Can I do this too?

@GrahamcOfBorg test nextcloud

@flokli
Copy link
Contributor

flokli commented Sep 23, 2018

@GrahamcOfBorg test nextcloud

@Ekleog
Copy link
Member

Ekleog commented Sep 23, 2018

@eqyiel You need to be known by ofborg for this. This can be done by opening a PR similar to NixOS/ofborg#239 to the ofborg repository :) That said, you can also test locally, with eg. nix-build nixos/tests/nextcloud.nix.

@flokli
Copy link
Contributor

flokli commented Sep 23, 2018

@eqyiel I was giving this another run, together with #47220 I got nextcloud 14 to be installed, backed by a MySQL server :-)

I didn't yet test redis and memcached, but can we use php72Packages? It seems to be supported in both nextcloud 13 and 14, according to their docs: https://docs.nextcloud.com/server/14/admin_manual/installation/source_installation.html

We could then also stop setting services.phpfpm.phpPackage, as it points to pkgs.php (which points to pkgs.php72 currently), or set it to pkgs.php72 there, too.
Personally, I'd prefer to not set it in nextcloud at all, as the module seems to be written with still supporting other phpfpm pools, so the decision shouldn't be done in the nextcloud module.

@GrahamcOfBorg
Copy link

Failure on aarch64-linux (full log)

Attempted: tests.nextcloud

Partial log (click to expand)

nextcloud: exit status 22
nextcloud: output:
error: command `curl -sSf http://nextcloud/login' did not succeed (exit code 22)
command `curl -sSf http://nextcloud/login' did not succeed (exit code 22)
cleaning up
killing nextcloud (pid 631)
vde_switch: EOF on stdin, cleaning up and exiting
vde_switch: Could not remove ctl dir '/build/vde1.ctl': Directory not empty
builder for '/nix/store/ipdlchp4krphj1mpzpmsl5blifrjgsxp-vm-test-run-nextcloud.drv' failed with exit code 255
error: build of '/nix/store/ipdlchp4krphj1mpzpmsl5blifrjgsxp-vm-test-run-nextcloud.drv' failed

@GrahamcOfBorg
Copy link

Timed out, unknown build status on x86_64-linux (full log)

Attempted: tests.nextcloud

Partial log (click to expand)

cannot build derivation '/nix/store/bpi99khc6zcsrgnlkpys94qj7v2mszan-etc.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/991y23jvs92cwwmkgzbmc7sqvlw4r6wr-stage-1-init.sh.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/r5i8wrm2yzn5irk8n2br1l5zlvvzpjvg-initrd.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/vk9plkimifk8ysnv9126l9qcsl55j6x1-nixos-system-nextcloud-19.03.git.5149f62.drv': 5 dependencies couldn't be built
cannot build derivation '/nix/store/iyihfnfhcd1dl3mzcl7iv0rdzfhn0c18-closure-info.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/bnipz4685i1r5kbak5nzizsymia57zhf-run-nixos-vm.drv': 2 dependencies couldn't be built
cannot build derivation '/nix/store/p3mdmysdhskwzq6c8l3kz3xyaygzqflm-nixos-vm.drv': 2 dependencies couldn't be built
cannot build derivation '/nix/store/5rirrjjrf4gdhkl91wkl4lsz8cjs4yy2-nixos-test-driver-nextcloud.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/q29rf7gyla6maf6gamr1bmxp8v2f0rvl-vm-test-run-nextcloud.drv': 1 dependencies couldn't be built
error: build of '/nix/store/q29rf7gyla6maf6gamr1bmxp8v2f0rvl-vm-test-run-nextcloud.drv' failed

nixos/modules/services/web-apps/nextcloud.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/nextcloud.nix Outdated Show resolved Hide resolved
default = {
"short_open_tag" = "Off";
"expose_php" = "Off";
"disable_functions" = "pcntl_alarm,pcntl_fork,pcntl_waitpid,pcntl_wait,pcntl_wifexited,pcntl_wifstopped,pcntl_wifsignaled,pcntl_wexitstatus,pcntl_wtermsig,pcntl_wstopsig,pcntl_signal,pcntl_signal_dispatch,pcntl_get_last_error,pcntl_strerror,pcntl_sigprocmask,pcntl_sigwaitinfo,pcntl_sigtimedwait,pcntl_exec,pcntl_getpriority,pcntl_setpriority";
Copy link
Contributor

Choose a reason for hiding this comment

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

This will cause nextcloud-occ to complain

The process control (PCNTL) extensions are required in case you want to interrupt long running commands - see http://php.net/manual/en/book.pcntl.php
on each run, as that's baked into nextcloud-occs config as well.

@fpletz @globin : Why was that disabled?

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've removed this line, but I'd like to know why too!

[ -e ${cfg.home}/config/config.php ] && exit 0

ln -sf ${autoConfig} ${cfg.home}/config/autoconfig.php
chown -R nextcloud:nginx ${cfg.home}/config ${cfg.home}/data ${cfg.home}/store-apps
Copy link
Contributor

@flokli flokli Sep 23, 2018

Choose a reason for hiding this comment

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

The initial request currently does the whole nextcloud installation. This ran into timeouts:

nextcloud# [  140.665158] nginx[690]: 2018/09/23 13:55:18 [error] 691#691: *1 upstream timed out (110: Connection timed out) while reading response header from upstream, client: 192.168.1.1, server: nextcloud, request: "GET /login HTTP/1.1", upstream: "fastcgi://unix:/run/phpfpm/nextcloud", host: "nextcloud"

It might help to run the nextcloud installation via nextcloud-occ inside this nextcloud-setup unit - On the first HTTP request, everything will be setup already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea. The problem I'm having now is that the tests pass interactively (run with nix-build nixos/tests/nextcloud.nix -A driver --show-trace -I nixpkgs=. and running testScript on the CLI) but not with nix-build nixos/tests/nextcloud.nix --show-trace -I nixpkgs=.. I'm not sure what that's about.

nextcloud# [   21.213558] Nextcloud[575]: {PHP} mkdir(): Read-only file system at /nix/store/6bzrcj4ryhilnz3xsh1l94r5aykybm3w-nextcloud-14.0.0/lib/private/Setup.php#300
nextcloud# [   21.257576] 1jgyw0ca6flrapn5bh55nrrsjwdixk2a-unit-script-nextcloud-setup-start[520]: Set an admin password.
nextcloud# [   21.269205] 1jgyw0ca6flrapn5bh55nrrsjwdixk2a-unit-script-nextcloud-setup-start[520]: Can't create or write into the data directory /nix/store/6bzrcj4ryhilnz3xsh1l94r5aykybm3w-nextcloud-14.0.0/data
nextcloud# [   21.289701] sudo[557]: pam_unix(sudo:session): session closed for user nextcloud
nextcloud# [   21.549481] systemd[1]: nextcloud-setup.service: Main process exited, code=exited, status=1/FAILURE
nextcloud# [   21.554644] systemd[1]: nextcloud-setup.service: Failed with result 'exit-code'.

Copy link
Member

Choose a reason for hiding this comment

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

One cannot use the autoconfig.php with that AFAICS

Copy link
Contributor

Choose a reason for hiding this comment

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

Uff, I need to investigate.

Other thing: The nextcloud-setup script is idempotent, and seems to be executed on each boot - so we should check with occ if nextcloud is already installed, and skip the installation in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like this is true: nextcloud/server#8524

I don't know how to resolve this except to increase the gateway timeout, so I'll do that.

@eqyiel
Copy link
Contributor Author

eqyiel commented Sep 23, 2018

@flokli thanks for the thorough review. I should be able to do this tonight (UTC+9:30)

@flokli
Copy link
Contributor

flokli commented Sep 24, 2018

@eqyiel thanks!

ln -sf ${autoConfig} ${cfg.home}/config/autoconfig.php
chown -R nextcloud:nginx ${cfg.home}/config ${cfg.home}/data ${cfg.home}/store-apps

${occ}/bin/nextcloud-occ maintenance:install
Copy link
Member

Choose a reason for hiding this comment

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

cool. This means migration is done automatically? Profit!

Copy link
Member

Choose a reason for hiding this comment

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

As mentioned below, this does not work, which is why it was done during the first request with the help of the autoconfig.php.

@globin
Copy link
Member

globin commented Sep 24, 2018

The maintenance:install does not work:

nextcloud# [    8.736117] Nextcloud[543]: {PHP} mkdir(): Read-only file system at /nix/store/lw7vbr6dqqgbar0zyvaa6c6vb8aardk8-nextcloud-14.0.0/lib/private/Setup.php#300
nextcloud# [    8.766589] sudo[531]: pam_unix(sudo:session): session closed for user nextcloud
nextcloud# [    8.768410] wm9iwqxgx0711hq76g1pflkbffq0h2in-unit-script-nextcloud-setup-start[496]: Set an admin password.
nextcloud# [    8.771361] wm9iwqxgx0711hq76g1pflkbffq0h2in-unit-script-nextcloud-setup-start[496]: Can't create or write into the data directory /nix/store/lw7vbr6dqqgbar0zyvaa6c6vb8aardk8-nextcloud-14.0.0/data
nextcloud# [    8.783804] systemd[1]: nextcloud-setup.service: Main process exited, code=exited, status=1/FAILURE
nextcloud# [    8.788311] systemd[1]: nextcloud-setup.service: Failed with result 'exit-code'.

@eqyiel
Copy link
Contributor Author

eqyiel commented Sep 24, 2018

Increasing the timeout seems to do it for now.

@flokli
Copy link
Contributor

flokli commented Sep 25, 2018

@eqyiel I found the issue!

For some reasons, before nextcloud is installed, occ doesn't seem to parse the configuration file, so all values stayed at default, which is why installation failed with the error above.

It's possible to set these options manually, see https://docs.nextcloud.com/server/14/admin_manual/configuration_server/occ_command.html#command-line-installation-label .

I did a POC in my nextcloud branch. I didn't test anything other than the vm test, especially the passFile bits and using another database needs some testing - could you have a look?

@eqyiel
Copy link
Contributor Author

eqyiel commented Sep 30, 2018

I think this is good now, should I squash it into two commits (module init and package version bump)?

@Ekleog
Copy link
Member

Ekleog commented Sep 30, 2018

I have been following the discussion, and wonder: do we know how well this module will handle upgrades? If I understand #47159 (comment) correctly, upgrading currently requires manual intervention, which would be unfortunate for NixOS. Would it be possible to automate it, or at least enforce the package version through stateVersion so that things don't break on upgrades?

@flokli
Copy link
Contributor

flokli commented Sep 30, 2018

@Ekleog nah, that was most likely a messed up state from the time we used autoconfig.

{occ}/bin/nextcloud-occ upgrade takes care of doing the migrations needed during an upgrade, and it's a no-op in case nothing is to do.

I was able to upgrade from 14.0.0 to 14.0.1 (in case that introduced DB migrations) using this module.

What's currently not tracked is upgrades of additionally installed "apps", but I'm not sure about the tooling for those anyhow. @globin, @fpletz, what are your thoughts on that? You seem to be using the saml app… Should we add support for installing apps through this nixos module in a followup PR?

@eqyiel 👍 for squahing this into three commits (module init, package bump, sendmail path), and then this should be good to go - and TBH, I'd love to see this backported for 18.09 as well :-)

@eqyiel
Copy link
Contributor Author

eqyiel commented Sep 30, 2018

Squashed!

@flokli
Copy link
Contributor

flokli commented Sep 30, 2018

@eqyiel you can actually use the Co-authored-by: keyword in the commit description:
https://blog.github.com/2018-01-29-commit-together-with-co-authors/

fpletz and others added 3 commits October 1, 2018 02:07
Co-authored-by: Franz Pletz <fpletz@fnordicwalking.de>
Co-authored-by: Robin Gloster <mail@glob.in>
Co-authored-by: Janne Heß <janne@hess.ooo>
Co-authored-by: Florian Klink <flokli@flokli.de>
Co-authored-by: Robin Gloster <mail@glob.in>
@eqyiel
Copy link
Contributor Author

eqyiel commented Sep 30, 2018

@flokli done 👌

@nyanloutre
Copy link
Member

@eqyiel seems like you are missing the two empty lines before the co-author options : https://help.github.com/articles/creating-a-commit-with-multiple-authors/#creating-co-authored-commits-on-the-command-line

@Mic92
Copy link
Member

Mic92 commented Oct 1, 2018

At least github seem to have detected it.

@nyanloutre
Copy link
Member

OK sorry, seems to be the phone version not showing correctly

@flokli flokli mentioned this pull request Oct 1, 2018
9 tasks
@eqyiel
Copy link
Contributor Author

eqyiel commented Oct 1, 2018

@nyanloutre my bad, I'll fix it, but I won't get a chance to until tomorrow.

@globin
Copy link
Member

globin commented Oct 1, 2018

@eqyiel it's fine, the mobile interface simply doesn't display it correctly.

@flokli
Copy link
Contributor

flokli commented Oct 1, 2018 via email

Copy link
Member

@fpletz fpletz left a comment

Choose a reason for hiding this comment

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

Thanks for the improvements! LGTM 👍

@eqyiel
Copy link
Contributor Author

eqyiel commented Oct 3, 2018

I'm getting mixed messages here 😅

@Mic92 Mic92 merged commit 6a995e9 into NixOS:master Oct 3, 2018
@Mic92
Copy link
Member

Mic92 commented Oct 3, 2018

Works perfectly just migrated an old installation.

@Mic92
Copy link
Member

Mic92 commented Oct 3, 2018

Regarding backport please open a pull request, so we can also run tests. I am not sure about backporting major upgrades though.

@eqyiel
Copy link
Contributor Author

eqyiel commented Oct 3, 2018

@Mic92 thanks!

@eqyiel eqyiel deleted the nextcloud branch October 3, 2018 23:49
extraTrustedDomains = mkOption {
type = types.listOf types.str;
default = [];
description = ''
Copy link
Member

Choose a reason for hiding this comment

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

Would it not make sense to also put extraTrustedDomains into services.nginx.virtualHosts.<hostname>.serverAliases?
At least this is what I would expected this option do.

Copy link
Member

Choose a reason for hiding this comment

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

I think there should be a clear distinction there. It might be the desired behavior in 99.9% of all cases, but especially with HTTP servers, some people have pretty wild configs, so this reduces flexibility.

What do you think? Wait until someone complains or leave it flexible?
Of course, there is always the option to configure nginx manually...

Copy link
Member

Choose a reason for hiding this comment

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

The least we can do is to add it to the description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants