nixos/syncthing: turn into RFC42 style settings

[Lassulus]

Description of changes

alternative to #233377

fix #232679

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 23.05 Release Notes (or backporting 22.11 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

[ncfavier]

This looks ad-hoc, fragile, and inefficient. I don't like it.

[Lassulus]

why inefficient? it fixes the check and does skip running the service, also the tristates in the options actually make more sense then having a default value which could be tied to a specific version. IMHO this is the most correct way this can be achieved without extending the freeform types.

[doronbehar]

I think this is the best possible fix proposed so far.

[RaitoBezarius]

Though this fix is going to break the people who expected/use those defaults, isn't it?

[doronbehar]

Though this fix is going to break the people who expected/use those defaults, isn't it?

I don't think so. Syncthing retains settings between reboots, and those users will just continue have the same settings, but they will be defined imperatively from now on.

[Lassulus]

Though this fix is going to break the people who expected/use those defaults, isn't it?

those default only were there for like 2 weeks and they will still be there afterwards for the people having them now.

[ncfavier]

How does syncthing handle null values?

[Lassulus]

How does syncthing handle null values?

it should ignore them because they are invalid. Since I couldn't find anything in the docs I just tested this manually:

I first took the default config and looked at the value options.limitBandwidthInLan: which is false.

I dumped the config, changed the value to null, PUT the new config, was still false. then I changed it to true, PUT it, was true afterwards, after PUTing null again it was still true. So this seems like the perfect behavior for us

We should probably take care when introducing options in settings.options which could take null as a valid value. but right now thats not the case. We could also filter the nix null values before converting them to JSON

[ncfavier]

We'll revisit this when there's no emergency. Reverted for now.

[Lassulus]

alright, have added a revert of the revert here so we can continue after branch off

[doronbehar]

How does syncthing handle null values?

it should ignore them because they are invalid.

That sounds great. To further improve this, we can filter those null values prior to generating the script.

We'll revisit this when there's no emergency. Reverted for now.

I was, and still optimistic we can reach an agreement until the branch off - we are all responsive and engaged. Hence my response is a polite 👎.

Also, there was a lot more to be said. For instance, I think that my PR might have also fixed the issue for @RaitoBezarius - as it makes the script iterate each attribute in settings individually.

[RaitoBezarius]

I was, and still optimistic we can reach an agreement until the branch off - we are all responsive and engaged. Hence my response is a polite -1.

Also, there was a lot more to be said. For instance, I think that #230196 might have also fixed the issue for @RaitoBezarius - as it makes the script iterate each attribute in settings individually.

I apologize for this @doronbehar but we were extremely close to the merge window when those changes got pulled in, with a subpar review, breaking the rule on breaking changes, without proper test coverage for systemService and basically with a 48 hours timelimit depending on me (one of the release manager) to test things…

While I do appreciate your efforts and the fact you were very engaged & responsive. We were not able to get out of the trouble with the multiples opened fixes which takes time to test and for which there is no consensus on what is the right thing.

The most important thing for a release is not to ship things that were tested by one person after breakage saying "it works on my machine" but that we take the time to find the right fix and the right semantics for what we want, especially to avoid over-breakages by piling up on fixes which apparently covers up for the previous issues / cornercases.

Do not put it on @ncfavier ; I would have merged the revert before the branch-off if no reasonable solution was not found at that moment, to be completely fair with you.

And do not put it on yourselves, that's the nixpkgs game, our goal is to stabilize the release, if those fixes would create more issues somehow: you would have been dragged into some unpleasant situation ; now you can take the time to figure out this during unstable, if it fixes new bugs that should be backported according to policies, those changes can end up in stable. I think it's a good thing.

[doronbehar]

The most important thing for a release is not to ship things that were tested by one person after breakage saying "it works on my machine" but that we take the time to find the right fix and the right semantics for what we want, especially to avoid over-breakages by piling up on fixes which apparently covers up for the previous issues / cornercases.

You have a point. Though I am rather sure the solution would have worked for you. Still, giving this more time to get cooked in unstable won't hurt anyone :) 👍.

[RaitoBezarius]

Though I am rather sure the solution would have worked for you.

I am certain too, but I am an advanced NixOS user, I don't care about me :P. I have to care about the others. :)

[doronbehar]

I think this PR can be marked as ready for review, since the release-23.05 branch off has already been done.

[Lassulus]

@ncfavier do you have a rough estimate when you will come around to review this or #234262 ? right now this is in limbo and I'm not aware of any more regressions with this PR

[ncfavier]

@infinisil may I ask for your opinion? I'm not sure how to best implement RFC 42 here; the proposed solutions leave me deeply unsatisfied and I can't seem to articulate why or suggest alternatives.

Particular points I'm iffy about:

• moving devices, folders option trees down under settings (they seem like important options that should be easily accessible)
• removing watch, watchDelay, rescanInterval in favour of their API-internal names fsWatcherEnabled, fsWatcherDelayS, rescanIntervalS (ditto, I guess)
• the horrible cleanedConfig hack; I've suggested looking at options.services.syncthing.settings.isDefined before to determine whether to enable the syncthing-init service, does that seem like a better idea?

[Lassulus]

Just to write down my thought process and different tradeoffs I thought about while implementing these changes:

* moving `devices`, `folders` option trees down under `settings` (they seem like important options that should be easily accessible)

since .settings is a freeFormType and describes the API endpoint, having these outside of settings would cause some weird behavior where people could define folders/devices twice, once through services.syncthing.folders and through services.syncthing.settings.folders.
We could strip the folders from settings but that would make it more obscure and we would stray away from the API.

Ideally I want people to dump their current config and just put that into settings to make it declarative.

* removing `watch`, `watchDelay`, `rescanInterval` in favour of their API-internal names `fsWatcherEnabled`, `fsWatcherDelayS`, `rescanIntervalS` (ditto, I guess)

this is similar to the previous issue, but I don't see a quick solution. If we turn folders into a freeForm people could define rescanIntervalS directly and we would need to handle to conflict with our current/previous rescalInterval.
But not sure which one should win in that scenario?

Also the current settings has a default which ideally would need to keep in sync with upstream if the default changes there.
Also there is no way to just ignore the value completely and have it set manually in the webinterface, since it would be override with the default value of the option. The new code would let people have folders declaratively but be able to change folder settings manually.

* the horrible `cleanedConfig` hack; I've suggested looking at `options.services.syncthing.settings.isDefined` before to determine whether to enable the syncthing-init service, does that seem like a better idea?

I looked into isDefined, but if a freeForm has options with default values (it breaks without those default values in my tests) it seems to always be true. Also stripping empty sets and nulls gives us the ability to have documentation for certain options without forcing them to a default value by simply setting them to null. Other alternatives I tried out made the check more complex and gave us less features with more edgecases to handle in the future. Like when people add more option.

[infinisil]

Part 2 of RFC 42 encourages adding extra options beside settings when it makes sense. So I agree with @ncfavier that devices, watch, etc. would make sense to keep as options.

@Lassulus it shouldn't be a problem if these extra options are implemented in terms of settings, something like this:

settings.fsWatcherDelayS = cfg.watchDelay;

the horrible cleanedConfig hack; I've suggested looking at options.services.syncthing.settings.isDefined before to determine whether to enable the syncthing-init service, does that seem like a better idea?

I think isDefined is something that should never be used for NixOS modules, because it leads to cases where things can change depending on the priority of options, even though all options are set to the same value, which is very unintuitive.

Most likely this is also the reason upstream why the cleanedConfig hack is necessary here, because upstream config behavior changes depending on whether the default is used or the value is set explicitly, even though the values might be the same. This could be caused by the upstream config not providing any way to set the default.

So I think ideally this should be fixed in upstream so that we don't need the cleanedConfig hack here, but until then I think it's fine to have it. I like it better than having to rely on isDefined.

[ncfavier]

(isDefined wouldn't work anyway, as @Lassulus points out, so we would have to use something like options.services.syncthing.settings.highestPrio > (mkOptionDefault {}).priority, but I imagine your concerns apply all the same?)

[infinisil]

@ncfavier Yeah I'd discourage priority-based behavior in general

[doronbehar]

BTW I'd really like to get this merged so that I can get #230196 going as well...

[Lassulus]

I'm currently traveling so a bit limited on time. Not sure what the current consensus is. I guess the cleanedConfig "hack" is still the best way to handle the config. We could think about having folders and devices outside of settings, but not sure what the upside of that would be? I like having all the declarative settings inside one option and the documentation renders the same no matter where we have it.

I didn't understand the point of configuring settings in terms of their aliases, since doing something like this:

settings.fsWatcherDelayS = cfg.watchDelay;

would override setting fsWatcherDelayS always with the default value of watchDelay. so we would need to change watchDelay into an nullOr int and then do something like this:

settings.fsWatcherDelayS = lib.mkIf (settings.fsWatcherDelayS == null && cfg.watchDelay != null) cfg.watchDelay;

which would cause a infinite recursion? Or we could just override fsWatcherDelay with watchDelay if the later is configured everytime with:

settings.fsWatcherDelayS = lib.mkIf (cfg.watchDelay != null) cfg.watchDelay

but here I don't like the silent ignoring of fsWatcherDelayS

[doronbehar]

I didn't understand the point of configuring settings in terms of their aliases

Me too, but if others will insist, perhaps we can force the user to use (in this example) watchDelay and not fsWatcherDelayS, by triggering a warning or throwing an error if the later is used.

I'll be able to live with the above suggestion, but I still think it's not too bad to ask our users to change some of these settings in order to conform to upstream's terminology - making us and the NixOS modules' users' maintenance burden easier

[doronbehar]

Rebased, and solved merge conflicts after #239427 , hopefully without mistakes.

[doronbehar]

@ofborg test syncthing-no-settings

[doronbehar]

So what is the status of this?

[Lassulus]

well I tried to bump some people but basically we are in review hell since @nbp still seems unhappy with the solution? not sure exactly how to proceed. But the current solution still seems like the best we can come up with. having devices and folders outside of settings would be misleading since they end up in settings anyway. and discoverability should be the same since documentation is generated either way?

I would be very happy if we could merge this and fix stuff later if it comes up. but even the last regressions before the channel bump were mainly because syncthing-init ran in weird circumstances where it should not.

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-in-distress/3604/62

[ncfavier]

How about making the type of settings nullOr (submodule ...) and defaulting to null? That way the check is just cfg.settings == null.

[Lassulus]

Ah we tried that already but the default value of settings gets overriden by the default values of the sub options

[Lassulus]

since communication with @ncfavier seems kinda stuck (please don't take it as an offence) and there are no other voices against merging this, I will self merge this. We are already like 1/3 of the time to a new release and there seems no forward movement and the closer we come to a release the bigger the fallout we have to fix last minute if something unexpected happens (again?)