make nixos.stateVersion mandatory #65314

aristidb · 2019-07-23T20:12:54Z

Motivation for this change

In #61321 a necessary migration was only run if the stateVersion is older than 19.09. The default value of cfg.release (= 19.09) prevented it from being run on systems without explicit stateVersion.

In this particular case, maybe using stateVersion could have been avoided, but in general, it is not safe to have a system without explicitly set stateVersion.

All systems installed in the last few years should have it set automatically, but there are surely still a lot of configs out there with it not set.

(While stateVersion as it is currently designed has other problems, like mentioned in #50112, the goal here is to just do the minimal change to get systems safe.)

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
Built on platform(s)
- NixOS
- [N/A] macOS
- [N/A] 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 nix-review --run "nix-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)
Ensured that relevant documentation is up to date
Fits CONTRIBUTING.md.

infinisil

I suggest adding an assertion like this to provide a much better error message than the generic "Option is used but not defined"

{
  assertions = [
    {
      assertion = options.system.stateVersion.isDefined;
      message = "`system.stateVersion', which protects against future state migrations, "
        + "is not defined. Set it to its previous default value of \"${cfg.release}\".";
    }
  ];
}

infinisil · 2019-07-31T23:21:36Z

CC @edolstra @oxij @Ekleog @srhb @samueldr

Ekleog

Having already seen people getting badly bitten by this, I'm all 👍 for this change!

Ekleog · 2019-08-01T04:07:43Z

nixos/lib/testing.nix

@@ -188,6 +188,7 @@ in rec {
            networking.hostName = "client";
            nix.readOnlyStore = false;
            virtualisation.writableStore = false;
+            system.stateVersion = "19.09"; # TODO: determine automatically


I think this TODO ought to be resolved before merging this, as otherwise we'll forget it after 19.09 branch-off, and we'll start testing outdated versions of postgresql on the next postgresql bump (or similar thing that would involve bumping stateVersion)

Giving it a bit more thought: either that or just add bumping this to the release process checklist, maybe -- definitely need to record the existence of this TODO somewhere at least, though.

srhb · 2019-08-01T04:19:55Z

@infinisil Regarding the assertion: Maybe replace the last paragraph. We want to avoid a situation where a user accidentally deletes the setting, gets that assertion message, then puts it back with a release possibly several versions ahead of their system's compatibility at the suggestion of the text.

infinisil · 2019-08-01T13:17:03Z

@srhb Yeah, but I'm not sure what would be a good replacement for it. Ideally I'd like to say

Set it to the NixOS version you initially installed, but only if you recently updated and notice problems. If you're sure that you don't need an older stateVersion by checking all things you would have to manually upgrade, you can set it to 19.09

Not sure how this can be phrased better..

srhb · 2019-08-01T13:58:42Z

@infinisil Punting a bit, but how about referring to the option documentation and we can try to improve on that? Better than having to cook up a great explanation in multiple places. :)

infinisil · 2019-08-01T14:32:35Z

I took the liberty to push a better stateVersion description to this PR in 548d427, how does it sound?

oxij · 2019-08-02T11:40:46Z

But I expect @edolstra to revert this. See #39447 and 1f0b692#commitcomment-28997477 and 4d1332e.

infinisil · 2019-08-02T13:48:39Z

Actually, it's kind of bad UX if we now make stateVersion mandatory, but at some later point deprecate that option in favor of per-module stateVersion's as described in #50112 (comment) (but with stateVersion's declared in configuration.nix, not in /var/lib/nixos)

So it's arguably better if we first figure out the per-module stateVersion, and only after that make it mandatory. This also means a lot of users won't ever see a warning for it being unset, because they're not using any modules that need it.

But then again, figuring out per-module stateVersion will take some time, likely with an RFC, and until that point users who don't have the global stateVersion set are at risk of breakage

oxij · 2019-08-05T15:17:09Z

But then again, figuring out per-module `stateVersion` will take some time, likely with an RFC, and until that point users who don't have the global `stateVersion` set are at risk of breakage

Yes. Which is why I think for now we should just copy-paste the warning from 1f0b692 (maybe with minor modifications describing what will break if they won't set it) to all the modules that actually depend on `stateVersion`. Users that don't have those modules enabled won't feel a thing, the rest could be affected by the issue, so, I say, let them see it. Thoughts?

infinisil · 2019-08-05T16:01:00Z

That sure would be the least-invasive solution for now, but copy-pasting it around doesn't sound very nice. Maybe an internal option system.warnStateVersionUnset which modules like postgres can set to true would be better. I like this as a temporary solution

samueldr · 2019-08-05T16:46:18Z

To be more end-user friendly, the internal setting could be a list, system.modulesRequiringState, and modules requiring state append to it. This would allow printing "The modules xxx, yyy and zzz require stateVersion to be set`. An empty list would mean nothing requires a stateful version to be set...

Though, this is all well, but with this coming update, most, if not all, systems will require stateVersion to be set because of how it ends up being used for systemd-timesyncd. (#64922) So, in a way, it's now de facto required. Unless a solution is figured out before the 19.09 fork/release, to handle the issue in another way.

infinisil · 2019-08-05T21:40:14Z

I see. So it might be best to just go ahead with the original idea of this PR and make stateVersion mandatory. With a good assertion error this shouldn't be a big problem.

oxij · 2019-08-05T22:36:10Z

One can use NixOS without `systemd-timesyncd`. There are, at the very least, `ntpd` and `chrony` modules doing the same thing (the latter one is much simpler source-vise, yet also much more advanced, with automatic clock drift calculations and etc, btw). Thus, I still vote for per-module warning (regardless of the actual implementation, the list proposal above sounds nice, though).

danbst · 2019-08-07T16:13:09Z

I'm contra to adding mandatory options. I'm also against of having asserts about filesystems."/" and boot.loader. We should remove those out for

$ nix build -f ./nixos system --arg configuration '{ services.postgresql.enable = true; }'

to just build.

It should be nixos-install or nixos-rebuild who should check whether stateVersion is configured correctly.

infinisil · 2019-08-09T12:28:07Z

@oxij Always requiring stateVersion has the benefit that a module that initially didn't need it and was enabled by the user will continue evaluating successfully even if it eventually does need it. If this was done with the module list thing, people could suddenly have to set stateVersion when they update their NixOS.

I feel like this is a band-aid like situation, we should just rip it off in one swoop by making stateVersion mandatory, which it should've been from the beginning. Once everybody has it set, we won't have to worry it again, no maintenance cost either.

@danbst That would be neat yeah, but I think that's something for the future. For most users whether the stateVersion check is done in the module or in nixos-rebuild doesn't make a difference, so there's no big cost to going with the former for now.

oxij · 2019-08-10T05:07:24Z

@infinisil Firstly, I think everybody here agrees than a global `stateVersion` is a hack which should be eventually removed. Secondly, modules using `stateVersion` should check it explicitly, if they don't I would consider it a bug. Hence

If this was done with the module list thing,

(or any other listed non-mandatory method)

people could suddenly have to set `stateVersion` when they update their NixOS.

but only if they use something that depends on it, which is the point.

we should just rip it off in one swoop by making `stateVersion` mandatory

Firstly, concerning `systemd-timesync` use of `stateVersion` in `nixos/modules/system/boot/timesyncd.nix`, I don't see why that piece of code can't be made unconditional. It looks like a migration script that will work for both old and new version of the package to me. (Since the old version of `systemd-timesyncd` can just regenerate the old directory, there's nothing of essence stored there anyway. Though, if you boot into an older release it might create some junk on the second migration, but that seems easy to fix with one more conditional around the line doing `mv`.) Which brings us back to the point that most systems should not, and with `systemd-timesyncd` fixed would not, ever use `stateVersion`. Currently `stateVersion` is used in conditional expressions in just a handful of services. Most uses of `stateVersion` in `nixos` tree are assignments to it to "avoid warnings during eval" and similar reasons, which is rather meh, IMHO. In short, I think that making it mandatory is counterproductive. By making it mandatory we will just make it harder to remove. After some more thinking about this I reverted to the opinion that copy-pasting edited warnings from 1f0b692 to modules using it should be better than any new infrastructure. Those copy-pasted pieces will just eventually turn into warnings/assertions for modularized `stateVersion`. I.e. I vote for simple copy-paste. @danbst Well, adding some runtime checks that would check that on-disk `stateVersion` is equal to `stateVersion` of `configuration.nix` would be useful, but I don't see how we could ever remove it from `configuration.nix` eval altogether. Because, in general, we can't just migrate on-disk service state a-la database schema migration because users can boot into older generations of their NixOS systems and booting into a newer one without explicit `stateVersion` could cause backward-incompatible migration for the older ones. (Which, I claim, is not the case for `systemd-timesyncd` issue above, especially since it does not store anything of essence in its state anyway.)

infinisil · 2019-08-10T12:38:42Z

On second thought, yeah actually that doesn't sound half bad @oxij, having a warning copy-pasted to every module that uses stateVersion until the per-module version is implemented. It's the only solution that doesn't need new infrastructure (which would eventually have to be removed again).

dasJ · 2019-09-01T12:58:42Z

To make the switch less painful, I think we should handle this like a deprecation (basically deprecating omittance of stateVersion).
So IMO, another PR (for 19.09) which emits a warning would be more appropriate. This PR could go into 20.03, then.

stale · 2020-06-01T11:03:20Z

Thank you for your contributions.
This has been automatically marked as stale because it has had no activity for 180 days.
If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.
Here are suggestions that might help resolve this more quickly:

Search for maintainers and people that previously touched the
related code and @ mention them in a comment.
Ask on the NixOS Discourse. 3. Ask on the #nixos channel on
irc.freenode.net.

dasJ · 2020-06-01T12:03:48Z

still important to me

stale · 2021-06-05T00:56:10Z

I marked this as stale due to inactivity. → More info

lucasew · 2023-08-07T19:29:37Z

Is this still relevant?

oxij · 2023-08-09T07:56:42Z

Yes, as an issue, it is still relevant.

Enzime · 2023-10-22T09:54:11Z

Duplicate of #149877

make nixos.stateVersion mandatory

5b5e4d4

ofborg bot added 6.topic: nixos 8.has: module (update) labels Jul 23, 2019

attempt to fix nixos tests (crude first version)

b6ff51e

infinisil requested changes Jul 31, 2019

View reviewed changes

Ekleog requested changes Aug 1, 2019

View reviewed changes

nixos/misc: Improve stateVersion description

548d427

infinisil mentioned this pull request Aug 12, 2019

systemd-timesyncd fails at step STATE_DIRECTORY on semi-clean install #64922

Closed

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 1, 2020

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 1, 2020

ryantm added the 2.status: merge conflict label Oct 3, 2020

ryantm added 2.status: merge conflict and removed 2.status: merge conflict labels Oct 3, 2020

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 5, 2021

roberth mentioned this pull request Jun 5, 2022

nixos/version: Require a stateVersion #176295

Open

13 tasks

matthiasbeyer mentioned this pull request Oct 27, 2022

trace that stateVersion is not set, but it is? #191622

Open

infinisil added the significant Novel ideas, large API changes, notable refactorings, issues with RFC potential, etc. label Apr 19, 2023

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 19, 2023

wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 19, 2024

wegank marked this pull request as draft March 20, 2024 13:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make nixos.stateVersion mandatory #65314

make nixos.stateVersion mandatory #65314

aristidb commented Jul 23, 2019

infinisil left a comment

infinisil commented Jul 31, 2019

Ekleog left a comment

Ekleog Aug 1, 2019

Ekleog Aug 1, 2019

srhb commented Aug 1, 2019

infinisil commented Aug 1, 2019 •

edited

srhb commented Aug 1, 2019

infinisil commented Aug 1, 2019

oxij commented Aug 2, 2019 via email

infinisil commented Aug 2, 2019 •

edited

oxij commented Aug 5, 2019 via email

infinisil commented Aug 5, 2019

samueldr commented Aug 5, 2019

infinisil commented Aug 5, 2019

oxij commented Aug 5, 2019 via email

danbst commented Aug 7, 2019 •

edited

infinisil commented Aug 9, 2019

oxij commented Aug 10, 2019 via email

infinisil commented Aug 10, 2019

dasJ commented Sep 1, 2019

stale bot commented Jun 1, 2020

dasJ commented Jun 1, 2020

stale bot commented Jun 5, 2021

lucasew commented Aug 7, 2023

oxij commented Aug 9, 2023

Enzime commented Oct 22, 2023

make nixos.stateVersion mandatory #65314

Are you sure you want to change the base?

make nixos.stateVersion mandatory #65314

Conversation

aristidb commented Jul 23, 2019

Motivation for this change

Things done

infinisil left a comment

Choose a reason for hiding this comment

infinisil commented Jul 31, 2019

Ekleog left a comment

Choose a reason for hiding this comment

Ekleog Aug 1, 2019

Choose a reason for hiding this comment

Ekleog Aug 1, 2019

Choose a reason for hiding this comment

srhb commented Aug 1, 2019

infinisil commented Aug 1, 2019 • edited

srhb commented Aug 1, 2019

infinisil commented Aug 1, 2019

oxij commented Aug 2, 2019 via email

infinisil commented Aug 2, 2019 • edited

oxij commented Aug 5, 2019 via email

infinisil commented Aug 5, 2019

samueldr commented Aug 5, 2019

infinisil commented Aug 5, 2019

oxij commented Aug 5, 2019 via email

danbst commented Aug 7, 2019 • edited

infinisil commented Aug 9, 2019

oxij commented Aug 10, 2019 via email

infinisil commented Aug 10, 2019

dasJ commented Sep 1, 2019

stale bot commented Jun 1, 2020

dasJ commented Jun 1, 2020

stale bot commented Jun 5, 2021

lucasew commented Aug 7, 2023

oxij commented Aug 9, 2023

Enzime commented Oct 22, 2023

infinisil commented Aug 1, 2019 •

edited

infinisil commented Aug 2, 2019 •

edited

danbst commented Aug 7, 2019 •

edited