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/auto-upgrade: offer the possibility to define a reboot window during which the system may be automatically rebooted #77622

Open
wants to merge 3 commits into
base: master
from

Conversation

@R-VdP
Copy link

@R-VdP R-VdP commented Jan 13, 2020

Motivation for this change

Some systems should not be rebooted at just any time. If the upgrade process takes too long, for instance because of a slow internet connection, or if the upgrade service is ran during production hours, we want to allow to define a window outside of which a reboot will not be performed.
The system will then reboot on the next run of the upgrade service which finishes inside the reboot window.

E.g. we can run the update service twice per week, once during the night and once during the day, but reboots are only allowed during the night. By doing so, a system that is usually shut down during the night will still receive updates and systems that are turned on 24/7 can be rebooted outside of production hours.

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.
@R-VdP
Copy link
Author

@R-VdP R-VdP commented Jan 13, 2020

@danbst, could you review this?

@danbst
danbst approved these changes Jan 14, 2020
@R-VdP
Copy link
Author

@R-VdP R-VdP commented Jan 25, 2020

@danbst, thanks! Anything else needs to happen to have this merged?

@danbst
Copy link
Contributor

@danbst danbst commented Jan 27, 2020

@R-VdP sorry, I wait for more 👀 . But I don't quite know whom to ping for another review.
I'll redirect this to 20.03 release managers @NixOS/nixos-release-managers

@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Feb 4, 2020

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

https://discourse.nixos.org/t/what-are-your-goals-for-20-03/4773/14

@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Feb 7, 2020

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

https://discourse.nixos.org/t/prs-already-reviewed/2617/92

@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Feb 7, 2020

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

https://discourse.nixos.org/t/nixos-20-03-feature-freeze/5655/29

@R-VdP R-VdP requested review from worldofpeace and disassembler Feb 8, 2020
@rnhmjoj
Copy link
Contributor

@rnhmjoj rnhmjoj commented Feb 8, 2020

I haven't fully tried it yet but it looks all right.
I wrote a NixOS test for this but unfortunately it doesn't fully work: since the VM has no internet access Nix can't rebuild the system. I hoped it could pick derivations from the host nix store but it doesn't really work. If somebody wants to try fixing it it's here: rnhmjoj@01d1284

Copy link
Member

@Infinisil Infinisil left a comment

This doesn't show correct behavior when the time span goes through midnight. E.g. with lower = "23:00", upper = "02:00" and let's say it runs on Tuesday 01:00, then the condition makes it only reboot if "Tuesday 01:00 is before Tuesday 02:00 and after Tuesday 23:00", which won't be the case.

@R-VdP
Copy link
Author

@R-VdP R-VdP commented Feb 11, 2020

Something went horribly wrong when pushing new changes, I guess I did something wrong while trying to rebase the commits. I did a force push to clear up the mess.

@Infinisil, I made a change to address your issue, we now only look at the hour independently of the day. Do you agree that this fixes the issue that you raised?

@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Feb 11, 2020

@R-VdP This happens to be a well known bug in GitHub for nixpkgs, we're pretty used to the "ping issue". Don't sweat it 😄

@R-VdP R-VdP requested a review from Infinisil Feb 11, 2020
date = "/run/current-system/sw/bin/date";
readlink = "/run/current-system/sw/bin/readlink";
shutdown = "/run/current-system/sw/bin/shutdown";
Comment on lines 116 to 118

This comment has been minimized.

@Atemu

Atemu Feb 15, 2020
Member

I think it'd be better to make these paths relative to pkgs.coreutils and pkgs.systemd, not the current system.

This comment has been minimized.

Make the paths relative to the values obtained from pkgs instead of the current system.
nixos-rebuild = "${config.system.build.nixos-rebuild}/bin/nixos-rebuild";
in
nixos-rebuild = "${config.system.build.nixos-rebuild}/bin/nixos-rebuild";
date = "${pkgs.coreutils}/bin/date";

This comment has been minimized.

@JohnAZoidberg

JohnAZoidberg Feb 16, 2020
Member

Shouldn't date, readlink and shutdown already be available, since coreutils is in the unit's path?

This comment has been minimized.

@R-VdP

R-VdP Feb 16, 2020
Author

I guess. I figured it wouldn't hurt to make the paths explicit, but I'm fine with removing this if that's the way things are usually done.

@R-VdP
Copy link
Author

@R-VdP R-VdP commented Mar 16, 2020

This doesn't show correct behavior when the time span goes through midnight. E.g. with lower = "23:00", upper = "02:00" and let's say it runs on Tuesday 01:00, then the condition makes it only reboot if "Tuesday 01:00 is before Tuesday 02:00 and after Tuesday 23:00", which won't be the case.

@Infinisil: Could you re-review and approve if I sufficiently addressed your comments? Thanks!

'';
${optionalString (cfg.rebootWindow != null) ''
elif [[ "''${current_time}" < "${cfg.rebootWindow.lower}" ]] || \
[[ "''${current_time}" > "${cfg.rebootWindow.upper}" ]]; then

This comment has been minimized.

@Infinisil

Infinisil Mar 18, 2020
Member

Still wrong. Example is lower = 12:00, upper = 6:00, current = 18:00 -> This is within the window, but the code doesn't do it then.

After a bit of thinking, I'd say the best way to correctly implement this is to: Determine if lower > upper, and if so, flip them around. Then check whether lower < cur < upper, and toggle this result if lower > upper from before. If the result is true, do the reboot, otherwise don't.

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

Successfully merging this pull request may close these issues.

None yet

10 participants
You can’t perform that action at this time.