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

nixos/nextcloud: fix upgrade path from 19.09 to 20.03 #82353

Merged
merged 1 commit into from Mar 26, 2020

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Mar 11, 2020

Motivation for this change

As described in #82056, the upgrade path of nextcloud from 19.09 to 20.03 is broken as we'd have to move two major versions forward which isn't supported by Nextcloud.

This PR introduces versioned attributes for Nextcloud and a few assertions (relying on the stateVersion) to help users with upgrading.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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.

@aanderse
Copy link
Member

@Ma27 does it make sense to just call the packages by their version number? We could default anything with an old stateVersion to nextcloud17 like you suggested, and maybe add a warnings to encourage users to change .package to nextcloud18 after upgrading to nextcloud17? If we entirely remove the nextcloud package and only provide nextcloudXX, keeping warnings (based on .package) up to date I think that might be less confusing. 🤔

@Ma27
Copy link
Member Author

Ma27 commented Mar 12, 2020

Hmm, that's an even better idea 👍

Will update the patches soon :)

@ryantm
Copy link
Member

ryantm commented Mar 12, 2020

If you do it this way, you won't need to worry about nixpkgs-update, because it makes sure the version number is compatible with the attrpath: https://github.com/ryantm/nixpkgs-update/blob/master/src/Version.hs#L35

@bachp
Copy link
Member

bachp commented Mar 12, 2020

What about keeping the nextcloud package, but but making it depend on the state version? This would give a default for people starting with a new installation.

@fpletz
Copy link
Member

fpletz commented Mar 12, 2020

I think @aanderse is on the right track here. After discussing this with @Ma27, we found some edgy use cases we should look out for:

  1. Users who are currently overriding nixpkgs.nextcloud with a custom derivation or are overriding the existing one need a solution when the nextcloud attribute is removed and not used in the NixOS module anymore

    • They would otherwise silently receive our default in the NixOS module instead of their overridden package
    • Possible solutions if nixpkgs.nextcloud is to be removed:
      1. Define nixpkgs.nextcloud to throw an exception which would tell users overriding this derivation that it is was removed. In the NixOS module, check if nixpkgs.nextcloud evaluates. If it does, use it, else use our default logic.
      2. Remove nixpkgs.nextcloud and add an assertion in the module. If nixpkgs.nextcloud is defined but services.nextcloud.package isn't, an error is printed that tells the user to use services.nextcloud.package to override the Nextcloud package used in the module.
    • While the first solution would nicer because it's easier grasp what's going on without the module, it still encourages using nixpkgs.nextcloud to override the package while a better solution exists. In the implementation, we would have to resort to tryEval which would also catch other causes of evaluation errors, i.e. using nixpkgs.nextcloud.overrideAttrs would also be catched and thus the logic would silently assume that this user wants our default even though he has clearly used an override.
    • The latter solution explicitly tells all users who are using overrides and the NixOS module, that they should override the package via the new services.nextcloud.package option. We suggest to go this route.
  2. We should only ship Nextcloud releases that are supported by upstream in our stable releases and master. People who are not updating often (for whatever reason) might skip an intermediate version.

    • Choosing an intermediate Nextcloud release after a new NixOS release based on the stateVersion only works for one intermediate release reliably when users rarely upgrade.
    • We should mention in the documentation that users of the Nextcloud module are supposed to update their system to recent (channel) releases, i.e. every month, so every major version bump reaches them.

So to sum up, we would suggest do the following:

  • Remove nixpkgs.nextcloud and introduce Nextcloud attributes for every supported major branch like nextcloud18
  • Introduce stateVersion dependent default for services.nextcloud.package
  • For case 1: Introduce an assertion as per solution b to inform all users overriding the package to use the module option instead.
  • For case 2: Mention in the documentation that we backport major releases to stable channels and users are supposed to update regularly.

@fpletz
Copy link
Member

fpletz commented Mar 12, 2020

@bachp stateVersion is a NixOS concept/option which should IMHO not influence any attribute in nixpkgs. NixOS configurations and their options depend on nixpkgs but we AFAIK have no precedent for the other direction (other than the explicit nixpkgs.* space in NixOS of course).

@Ma27 Ma27 requested a review from ryantm March 16, 2020 17:54
@Ma27
Copy link
Member Author

Ma27 commented Mar 20, 2020

Rebased to resolve the merge-conflict in the release-notes.

@fpletz @bachp @aanderse @dasJ would it be possible to get another review from you? :)

@aanderse
Copy link
Member

@Ma27 quick glance over it and looks fine 👍

@Ma27
Copy link
Member Author

Ma27 commented Mar 23, 2020

@worldofpeace @disassembler would you mind taking a look at this? This should IMHO land in 20.03 :)

It's impossible to move two major-versions forward when upgrading
Nextcloud. This is an issue when comming from 19.09 (using Nextcloud 16)
and trying to upgrade to 20.03 (using Nextcloud 18 by default).

This patch implements the measurements discussed in NixOS#82056 and NixOS#82353 to
improve the update process and to circumvent similar issues in the
future:

* `pkgs.nextcloud` has been removed in favor of versioned attributes
  (currently `pkgs.nextcloud17` and `pkgs.nextcloud18`). With that
  approach we can safely backport major-releases in the future to
  simplify those upgrade-paths and we can select one of the
  major-releases as default depending on the configuration (helpful to
  decide whether e.g. `pkgs.nextcloud17` or `pkgs.nextcloud18` should be
  used on 20.03 and `master` atm).

* If `system.stateVersion` is older than `20.03`, `nextcloud17` will be
  used (which is one major-release behind v16 from 19.09). When using a
  package older than the latest major-release available (currently v18),
  the evaluation will cause a warning which describes the issue and
  suggests next steps.

  To make those package-selections easier, a new option to define the
  package to be used for the service (namely
  `services.nextcloud.package`) was introduced.

* If `pkgs.nextcloud` exists (e.g. due to an overlay which was used to
  provide more recent Nextcloud versions on older NixOS-releases), an
  evaluation error will be thrown by default: this is to make sure that
  `services.nextcloud.package` doesn't use an older version by accident
  after checking the state-version. If `pkgs.nextcloud` is added
  manually, it needs to be declared explicitly in
  `services.nextcloud.package`.

* The `nixos/nextcloud`-documentation contains a
  "Maintainer information"-chapter  which describes how to roll out new
  Nextcloud releases and how to deal with old (and probably unsafe)
  versions.

Closes NixOS#82056
@ofborg ofborg bot requested a review from bachp March 25, 2020 21:13
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

I approve of the backport to 20.03

@Ma27
Copy link
Member Author

Ma27 commented Mar 25, 2020

Great, thanks a lot for the review! Will merge tomorrow then :)

@Ma27 Ma27 merged commit 89bcf4b into NixOS:master Mar 26, 2020
@Ma27 Ma27 deleted the nextcloud-upgrade-path branch March 26, 2020 10:00
Ma27 added a commit that referenced this pull request Mar 26, 2020
It's impossible to move two major-versions forward when upgrading
Nextcloud. This is an issue when comming from 19.09 (using Nextcloud 16)
and trying to upgrade to 20.03 (using Nextcloud 18 by default).

This patch implements the measurements discussed in #82056 and #82353 to
improve the update process and to circumvent similar issues in the
future:

* `pkgs.nextcloud` has been removed in favor of versioned attributes
  (currently `pkgs.nextcloud17` and `pkgs.nextcloud18`). With that
  approach we can safely backport major-releases in the future to
  simplify those upgrade-paths and we can select one of the
  major-releases as default depending on the configuration (helpful to
  decide whether e.g. `pkgs.nextcloud17` or `pkgs.nextcloud18` should be
  used on 20.03 and `master` atm).

* If `system.stateVersion` is older than `20.03`, `nextcloud17` will be
  used (which is one major-release behind v16 from 19.09). When using a
  package older than the latest major-release available (currently v18),
  the evaluation will cause a warning which describes the issue and
  suggests next steps.

  To make those package-selections easier, a new option to define the
  package to be used for the service (namely
  `services.nextcloud.package`) was introduced.

* If `pkgs.nextcloud` exists (e.g. due to an overlay which was used to
  provide more recent Nextcloud versions on older NixOS-releases), an
  evaluation error will be thrown by default: this is to make sure that
  `services.nextcloud.package` doesn't use an older version by accident
  after checking the state-version. If `pkgs.nextcloud` is added
  manually, it needs to be declared explicitly in
  `services.nextcloud.package`.

* The `nixos/nextcloud`-documentation contains a
  "Maintainer information"-chapter  which describes how to roll out new
  Nextcloud releases and how to deal with old (and probably unsafe)
  versions.

Closes #82056

(cherry picked from commit 702f645)
@Ma27
Copy link
Member Author

Ma27 commented Mar 26, 2020

Backported to 20.03 as d148bb0. Thanks a lot to everyone involved!

@Ma27 Ma27 added 8.has: port to stable A PR already has a backport to the stable release. and removed 9.needs: port to stable A PR needs a backport to the stable release. labels Mar 26, 2020
@Ma27 Ma27 mentioned this pull request Mar 28, 2020
10 tasks
Ma27 added a commit to Ma27/nixpkgs that referenced this pull request Mar 28, 2020
Upgrades Hydra to the latest master/flake branch. To perform this
upgrade, it's needed to do a non-trivial db-migration which provides a
massive performance-improvement[1].

The basic ideas behind multi-step upgrades of services between NixOS versions
have been gathered already[2]. For further context it's recommended to
read this first.

Basically, the following steps are needed:

* Upgrade to a non-breaking version of Hydra with the db-changes
  (columns are still nullable here). If `system.stateVersion` is set to
  something older than 20.03, the package will be selected
  automatically, otherwise `pkgs.hydra-migration` needs to be used.

* Run `hydra-backfill-ids` on the server.

* Deploy either `pkgs.hydra-unstable` (for Hydra master) or
  `pkgs.hydra-flakes` (for flakes-support) to activate the optimization.

The steps are also documented in the release-notes and in the module
using `warnings`.

`pkgs.hydra` has been removed as latest Hydra doesn't compile with
`pkgs.nixStable` and to ensure a graceful migration using the newly
introduced packages.

To verify the approach, a simple vm-test has been added which verifies
the migration steps.

[1] NixOS/hydra#711
[2] NixOS#82353 (comment)
Ma27 added a commit that referenced this pull request Mar 28, 2020
Upgrades Hydra to the latest master/flake branch. To perform this
upgrade, it's needed to do a non-trivial db-migration which provides a
massive performance-improvement[1].

The basic ideas behind multi-step upgrades of services between NixOS versions
have been gathered already[2]. For further context it's recommended to
read this first.

Basically, the following steps are needed:

* Upgrade to a non-breaking version of Hydra with the db-changes
  (columns are still nullable here). If `system.stateVersion` is set to
  something older than 20.03, the package will be selected
  automatically, otherwise `pkgs.hydra-migration` needs to be used.

* Run `hydra-backfill-ids` on the server.

* Deploy either `pkgs.hydra-unstable` (for Hydra master) or
  `pkgs.hydra-flakes` (for flakes-support) to activate the optimization.

The steps are also documented in the release-notes and in the module
using `warnings`.

`pkgs.hydra` has been removed as latest Hydra doesn't compile with
`pkgs.nixStable` and to ensure a graceful migration using the newly
introduced packages.

To verify the approach, a simple vm-test has been added which verifies
the migration steps.

[1] NixOS/hydra#711
[2] #82353 (comment)

(cherry picked from commit bd5324c)
Emantor pushed a commit to Emantor/nixpkgs that referenced this pull request Mar 29, 2020
Upgrades Hydra to the latest master/flake branch. To perform this
upgrade, it's needed to do a non-trivial db-migration which provides a
massive performance-improvement[1].

The basic ideas behind multi-step upgrades of services between NixOS versions
have been gathered already[2]. For further context it's recommended to
read this first.

Basically, the following steps are needed:

* Upgrade to a non-breaking version of Hydra with the db-changes
  (columns are still nullable here). If `system.stateVersion` is set to
  something older than 20.03, the package will be selected
  automatically, otherwise `pkgs.hydra-migration` needs to be used.

* Run `hydra-backfill-ids` on the server.

* Deploy either `pkgs.hydra-unstable` (for Hydra master) or
  `pkgs.hydra-flakes` (for flakes-support) to activate the optimization.

The steps are also documented in the release-notes and in the module
using `warnings`.

`pkgs.hydra` has been removed as latest Hydra doesn't compile with
`pkgs.nixStable` and to ensure a graceful migration using the newly
introduced packages.

To verify the approach, a simple vm-test has been added which verifies
the migration steps.

[1] NixOS/hydra#711
[2] NixOS#82353 (comment)
stigok pushed a commit to stigok/nixpkgs that referenced this pull request Jun 12, 2020
Upgrades Hydra to the latest master/flake branch. To perform this
upgrade, it's needed to do a non-trivial db-migration which provides a
massive performance-improvement[1].

The basic ideas behind multi-step upgrades of services between NixOS versions
have been gathered already[2]. For further context it's recommended to
read this first.

Basically, the following steps are needed:

* Upgrade to a non-breaking version of Hydra with the db-changes
  (columns are still nullable here). If `system.stateVersion` is set to
  something older than 20.03, the package will be selected
  automatically, otherwise `pkgs.hydra-migration` needs to be used.

* Run `hydra-backfill-ids` on the server.

* Deploy either `pkgs.hydra-unstable` (for Hydra master) or
  `pkgs.hydra-flakes` (for flakes-support) to activate the optimization.

The steps are also documented in the release-notes and in the module
using `warnings`.

`pkgs.hydra` has been removed as latest Hydra doesn't compile with
`pkgs.nixStable` and to ensure a graceful migration using the newly
introduced packages.

To verify the approach, a simple vm-test has been added which verifies
the migration steps.

[1] NixOS/hydra#711
[2] NixOS#82353 (comment)

(cherry picked from commit bd5324c)
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

8 participants