nextcloud module: changing config doesn't propagate to the module

[Ekleog]

Data that was once config and recorded on ${dataDir}/config/config.php doesn't get updated when the configuration changes.

This is a violation of purity. And more than that, it's very surprising when trying to install, hesitating between pgsql and sqlite, and struggling before understanding that the configuration doesn't get used anyway :p

cc @globin @flokli @eqyiel @fpletz

[flokli]

@Ekleog that's a bit tough.

We have a nextcloud-setup unit, that's supposed to do the installation.

We use the nextcloud-occ maintenance:install command to install nextcloud (passing the configured database and adminpass settings as command parameters).

This CLI generates ${cfg.home}/config/config.php, and installs nextcloud.

The script checks the presence of ${cfg.home}/config/config.php, and skips installation on following starts.

If you now change configuration from the module, we won't run the install command again, as the config file already exists, and nextcloud-setup assumes nextcloud is already installed.

[Ekleog]

Hmm… Maybe it would be possible to generate the ${cfg.home}/config/config.php somewhere in the store, and symlink to it? This way the next time nextcloud-setup runs it can in addition check whether config.php points to the correct store path.

Now, I don't really know what nextcloud-occ exactly does, so maybe it's not actually possible to do this because eg. one would need to re-run maintenance:install on certain configuration changes? In which case it'd be hard :/ but at least we can maybe make nextcloud-setup fail or something like that, so that the issue is at least noisy?

[Ma27]

Now, I don't really know what nextcloud-occ exactly does, so maybe it's not actually possible to do this because eg. one would need to re-run maintenance:install on certain configuration changes?

I don't recommend to do this, I just tried that (after upgrading one of my personal systems) and this tries to recreate a database schema which can end in very bad results.

This way the next time nextcloud-setup runs it can in addition check whether config.php points to the correct store path.

Unfortunately I doubt that this will work properly, nextcloud wants to merge this during the runtime. E.g. it adds a installed => true after a successful maintenance:install or adds trusted domains when running the corresponding command.

I'm using this as well now and hitting these issues, unfortunately I don't have useful ideas either ATM, but I'm happy to discuss better solutions :)

[flokli]

Nextcloud edits it's config.php in a lot of different places, so having it generated through Nix currently isn't possible.
nextcloud/server#11075 digs into that already, I added a comments about our use case here too.

[flokli]

@Ma27, @Ekleog to avoid other people running into the reconfiguration issue for now, what about documenting that, maybe in services.nextcloud.enable?

[Ma27]

That's certainly a good thing to do. If nobody else wants to take over, I'd volunteer to do this during the next week as investing time on improving the nextcloud module is one item on my todo list :)

@flokli I was aware of config.php being stateful, but thanks a lot for linking the corresponding ticket! I'll have a look at this as soon as I have sufficient time to :)

[turion]

In the light of this issue, what's the correct way to upgrade nextcloud to a new major version? Do I just pull a newer nixpkgs, upgrade my system and hope that the systemd job will do the right thing?

[Ma27]

The nextcloud-setup service runs an occ upgrade that should apply all changes like database migrations needed for the new Nextcloud version (beware, downgrades aren't possible).

As long as you let Nextcloud do its job, you shouldn't get that much issues. IMHO the actual problem here is the configuration of the software within Nix(OS).

[teto]

just a simple question (could not come to a conclusion after the lengthy thread) but I recently updated my nextcloud via nixops deploy and the update disabled the calendar module. Did I mess up or is that expected ? any way to configure this from nix ?

[turion]

Assuming that you installed the calendar module via the UI, it is installed in /var/lib/nextcloud/store-apps/calendar. The beginning of my config reads:

 'apps_paths' => 
  array (
    0 => 
    array (
      'path' => '/var/lib/nextcloud/apps',
      'url' => '/apps',
      'writable' => false,
    ),
    1 => 
    array (
      'path' => '/var/lib/nextcloud/store-apps',
      'url' => '/store-apps',
      'writable' => true,
    ),
  ),

So in principle nextcloud can edit the apps installed via the web interface (otherwise you couldn't install them via the interface in the first place).

[flokli]

I don't think there's anything actionable here - the nextcloud nixos module is limited by how Nextcloud (imperatively) manages its configuration.

The current state is properly documented in services/web-apps/nextcloud.xml, and we raised our concerns to upstream in nextcloud/server#11075 (comment) - closing this issue, feel free to reopen if there's more to be done.

[Ekleog]

@flokli I think one actionable item would be to have the nextcloud service fail with an explicit error if the configuration being used is not compatible with the configuration defined in the .nix. At least the user would be warned in practice that configuration is not really applied, and not only by the manual that they are relatively unlikely to actually read.

Reopening for now, feel free to reclose if you think that's not a reasonable “solution” :)

[flokli]

The problem is we don't know whether the configuration on disk containing database connection information (${cfg.home}/config/config.php) differs from the one that would be generated through an installation via ${occInstallCmd}- we don't render the configuration on our own, but the installer writes it out as a side-effect.

If I'm not missing something obvious, checking configuration changes would translate to parsing/evaluating config.php on every nextcloud startup as an ExecStartPre, comparing every "interesting" key with the one provided through nixos configuration, and eventually then bailing out if they differ.
I'm not sure if that's a road we'd like to go and keep up to date, while the underlying bug is a missing distinction between configuration and state upstream…

[Ma27]

Since upstream mixes state and config it's questionable whether their configuration format is somehow supposed to be backwards-compatible (as it's only supposed to be used internally by Nextcloud). Hence I'm afraid that this will make i.e. upgrades even harder in case of configuration changes, so 👎 from me.

As the issues are documented in the manual now, folks are warned and I think that this is a better option in contrast to adding more complexity to nextcloud-setup.service.

I suggest we either close this for the reasons @flokli already stated or wait for an appropriate upstream fix.

[Ekleog]

I really don't feel good having stuff in configuration.nix that's actually a disguised command line… :/

Something that would sound nicer to me would be an setup-nextcloud executable that the user would have to manually run, and a bare-bones nextcloud module that does not handle setup or configuration and just makes sure that the systemd unit is setup.

At least the error is obvious (enabling the module without having run the executable would fail), all module options actually mean something, and the impure operation is made explicit.

Either that or, similar to services.postgresql.initialScript, rename all the options so it's explicit that they're used only for initial configuration.

Would one of these solutions make sense to you?

[flokli]

I really don't feel good having stuff in configuration.nix that's actually a disguised command line… :/

Me neither, but currently we don't have much choice…

Something that would sound nicer to me would be an setup-nextcloud executable that the user would have to manually run, and a bare-bones nextcloud module that does not handle setup or configuration and just makes sure that the systemd unit is setup.
At least the error is obvious (enabling the module without having run the executable would fail), all module options actually mean something, and the impure operation is made explicit.

I really like the fact that we can set up the Nextcloud instance in an unattended fashion, without having to worry about manual setting things up.
I really don't want to end with more imperative setup steps than necessary.

Either that or, similar to services.postgresql.initialScript, rename all the options so it's explicit that they're used only for initial configuration.

In theory, we could move some of the options in services.nextcloud.config.* to services.nextcloud.initialConfig.* to make it more obvious what isn't applied continuously. But TBH, I'd rather like to see the state/config mixing fixed upstream than having to introduce more mental complexity here, especially if this gets fixed and we end up with both incarnations.

Really, this issue only occurs while a user changes nextcloud database parameters (mostly while tweaking his initial setup), and not having read the module documentation, where the behaviour is documented.

What about adding the following (subtile) log statement, to point users to the docs when they peek into the logs while things are not working as expected?

diff --git a/nixos/modules/services/web-apps/nextcloud.nix b/nixos/modules/services/web-apps/nextcloud.nix
index 7b3af60ae24..a4665a37f36 100644
--- a/nixos/modules/services/web-apps/nextcloud.nix
+++ b/nixos/modules/services/web-apps/nextcloud.nix
@@ -347,6 +347,8 @@ in {
             # Do not install if already installed
             if [[ ! -e ${cfg.home}/config/config.php ]]; then
               ${occInstallCmd}
+            else
+              echo "${cfg.home}/config/config.php already exists, skipping installation / db reconfiguration (see docs)…"
             fi
 
             ${occ}/bin/nextcloud-occ upgrade

[Ekleog]

I agree that the log statement is the minimum we can do, and would at least give a hint about the way in which things are broken.

Now, I guess the reason we're kind of disagreeing on the path forward is that you're much more hopeful than me that nextcloud will ever fix its stuff -- I'm assuming it'll get fixed in years at best (and even then…) -- and if it's in years then I'd much rather live with initialConfig for years then spend a year deprecating it by making it conflict with new config options than try to live with config from now on and have users fail to understand what is going on.

Now urhm, I'm investigating the module a bit deeper, and… is there any reason we can't put the database configuration etc. in override.config.php? We could then refresh override.config.php on each activation, and it should be enough? If we have a way to couple it with something that automatically runs install iff the database is not ready yet then the setup would actually be… almost perfect?

[Ma27]

Now urhm, I'm investigating the module a bit deeper, and… is there any reason we can't put the database configuration etc. in override.config.php? We could then refresh override.config.php on each activation, and it should be enough? If we have a way to couple it with something that automatically runs install iff the database is not ready yet then the setup would actually be… almost perfect?

After having a deeper look at the sources (nextcloud/server at github, d3332f516ff2b773b75d7269aac8adecb16ea512 (6 commits ahead of v16.0.0RC1)) I guess that this should be possible (I didn't test it yet though).

Configs will be loaded from /var/lib/nextcloud/config using PHP's glob function[1] while all files ending with config.php will be taken into account. Hence override.config.php works as all config arrays will be merged together. This config will be loaded at lib/base.php[2] and is available as OC::$config which is then used in the DI container[3].

If I understand the code correctly, I assume that it should be possible to render all config options in /var/lib/nextcloud/config/override.config.php as config files found by glob will override values from config.php[4].

[1] Config::readData
[2] OC::initPaths
[3] OC\Server
[4] https://github.com/nextcloud/server/blob/d3332f516ff2b773b75d7269aac8adecb16ea512/lib/private/Config.php#L194

[flokli]

@Ma27 Nice catch! This might indeed be possible - we should make very sure that config overrides work the way we think they will - Config::writeData seems to write the merged configuration, and not just the overrides (and merging this with other config in the wrong order can lead to wonderfully weird to debug issues) ;-)

While I'd still prefer nextcloud splitting config and state properly, and being able to cope with read-only (base) config - it might be a direction worth exploring.

What do you think?

[Ma27]

Nice catch! This might indeed be possible

Well, for the record it was actually @Ekleog's idea 😅

Config::writeData seems to write the merged configuration, and not just the overrides (and merging this with other config in the wrong order can lead to wonderfully weird to debug issues) ;-)

I guess so, all the stuff from overrides.config.php is also in my config.php at the moment. I'll probably have sufficient time next weekend to check if I can trigger such a case.

I can't promise that, but unless anybody beats me, I can hopefully prepare a fix for this :)

[bachp]

@Ma27 Did you gain some new insights on this?

[Ma27]

Unfortunately not... I was pretty busy back then. But as this only requires some fiddling in the module and no patching of the actual package, the change shouldn't be too painful, so if you'd like to take over... 😅

[Ma27]

@bachp there's #63900 with a fix now :)