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.

Sign up for GitHub

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

Jump to bottom

Add randomizedDelaySec, persistent options to nix-gc and auto-upgrade #107805

Closed
wants to merge 3 commits into from
Closed

Add randomizedDelaySec, persistent options to nix-gc and auto-upgrade #107805

wants to merge 3 commits into from

Conversation

tex
Copy link
Contributor

@tex tex commented Dec 28, 2020

Motivation for this change

I am complaining about auto-upgrade (and nix-gc) not correctly working in laptop environment where laptop is turned on and off unpredictable causing those timers never expire and those services never executed.

Here is my proposal to fix it. Please advice, comment. Since I am changing existing API I guess it is not for direct delivery (unless it is), but I think that current situation is making those services unusable on laptops. Since it is auto upgrade that could lead to security issues as user might expect getting automatic updates while he is really not.

#15689
#75861

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.

@xaverdh
Copy link
Contributor

xaverdh commented Dec 29, 2020

It should be possible to keep both apis in parallel, at least for now?

@tex tex force-pushed the gc_and_auto_upgrade_interval branch from bace3f8 to cbdcaee Compare December 29, 2020 11:36
@tex
Copy link
Contributor Author

tex commented Dec 29, 2020

Good idea. I changed it, so if user already has set it's own dates then it will use them. But defaults are changes to work in laptop environment (and that doesn't break servers). Also updated comments a bit to indicate that dates accepts fixed date/time or intervals. And the Persistent is important to make intervals works on laptops.

@tex tex changed the title Use interval, randomizedDelaySec, persistent options in nix-gc and auto-upgrade Add randomizedDelaySec, persistent options to nix-gc and auto-upgrade Dec 29, 2020
@tex tex force-pushed the gc_and_auto_upgrade_interval branch from cbdcaee to 8afc357 Compare December 29, 2020 11:40
bbjubjub2494 added a commit to bbjubjub2494/devos that referenced this pull request Dec 29, 2020
@bbjubjub2494
Try out NixOS/nixpkgs#107805
1871928
bbjubjub2494
bbjubjub2494 suggested changes Dec 29, 2020
View reviewed changes
Copy link
Member

@bbjubjub2494 bbjubjub2494 left a comment

Choose a reason for hiding this comment

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

I like the idea. I made nix-gc persistent on the side for the same reason, but it's nicer to have a proper fix. I will test both modules on my infrastructure this week.

Your commit doesn't follow CONTRIBUTING.md. Ideally, you should split it into two, namely

  • nixos/nix-gc: add persistent and randomizeDelaySec options
  • nixos/auto-upgrade: add persistent option

bbjubjub2494
bbjubjub2494 suggested changes Dec 29, 2020
View reviewed changes
Copy link
Member

@bbjubjub2494 bbjubjub2494 left a comment

Choose a reason for hiding this comment

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

Sorry for the traffic, I fumbled posting those comments apparently

nixos/modules/services/misc/nix-gc.nix Outdated Show resolved Hide resolved
nixos/modules/tasks/auto-upgrade.nix Outdated Show resolved Hide resolved
bbjubjub2494 added a commit to bbjubjub2494/devos that referenced this pull request Dec 29, 2020
@bbjubjub2494
Try out NixOS/nixpkgs#107805
a5fec5c
@tex
Copy link
Contributor Author

tex commented Dec 29, 2020

I see you config:

  nix.gc.automatic = true;
  nix.gc.options = "--delete-older-than 1w";
  nix.gc.dates = "Sat 03:15";
  systemd.timers.nix-gc.timerConfig.Persistent = true;

Oh, I see I could set Persistent directly in my configuration.nix! :) I didn't know it can be so easy :)

But fix it in core is definitely better, at least for users and discoverability of the configuration.

Will split it to two merge requests and apply your suggested changes.

tex and others added 2 commits December 29, 2020 22:59
@tex
Update nixos/modules/services/misc/nix-gc.nix
42afdbd 
Co-authored-by: Louis Bettens <lourkeur@users.noreply.github.com>
@tex
Update nixos/modules/tasks/auto-upgrade.nix
79ffa88 
Co-authored-by: Louis Bettens <lourkeur@users.noreply.github.com>
This was referenced Dec 30, 2020
Merged
Merged
@tex
Copy link
Contributor Author

tex commented Dec 30, 2020

Created two merge requests according to your suggestion. Closing this one.

@tex tex closed this Dec 30, 2020
@tex tex deleted the gc_and_auto_upgrade_interval branch December 30, 2020 11:11
bbjubjub2494 added a commit to bbjubjub2494/devos that referenced this pull request Dec 31, 2020
@bbjubjub2494
Revert "Try out NixOS/nixpkgs#107805"
f9f777e 
This reverts commit a5fec5c.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bbjubjub2494 bbjubjub2494 bbjubjub2494 requested changes

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tex @xaverdh @bbjubjub2494