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

Add global enable switches for nixos and home-manager #171

Closed
wants to merge 1 commit into from

Conversation

0xf09f95b4
Copy link

@0xf09f95b4 0xf09f95b4 commented Feb 23, 2024

In #138 and #167, requests for some global enable option were made.

I've also found some use-cases where I'd like to disable all side-effects of the impermanence modules for specific configurations while globally importing the modules. Though setting environment.persistence to {} generally works, it has two drawbacks:

  • You can't declare environment.persistence and then easily globally disable them somewhere else, without forcing it to {} (see Easy way to disable #138).
  • Some side-effects always remain on the system (such as empty activation scripts).

This PR adds two additional options to impermanence:

  • boot.impermanence.enable: This switch is enabled by default and, if disabled, deactivates the entire module.
  • home.impermanence.enable: This switch does the same for home-manager.nix

I'd rather have this module be disabled by default (as most nixos modules are) but this would break every current install when updating which would be very bad. Instead, these new options are enabled by default, which means current installs would not be affected at all by these changes.

I put the nixos switch into boot because I felt that this module mostly affects the boot process (though some systemd services are started as well). I'm open to any suggestions for better placement.

@Kreyren
Copy link

Kreyren commented Feb 23, 2024

I'd rather have this module be disabled by default (as most nixos modules are) but this would break every current install when updating which would be very bad. Instead, these new options are enabled by default, which means current installs would not be affected at all by these changes. -- @0xf09f95b4 (#171 (comment))

I would argue for these to be set to false by default as in the worst case the user will have to reboot and change configuration and in the present usecase it seems that it would make a permanent breaking changes to all my systems that don't have impermenance integrated in a reliable way yet https://github.com/Kreyren/nixos-config/tree/aaf809b0212d63ad694054139a74dc97c32ff497

@Kreyren
Copy link

Kreyren commented Mar 22, 2024

ping @0xf09f95b4 @talyz could this be merged or brainstormed? This feature is a major roadblock for my nixos configuration atm

@0xf09f95b4
Copy link
Author

I'm ready to get this merged ;) I'm currently just using my fork for my projects.
I don't think I can make the decision if impermanence should be disabled by default though. I still see the danger of breaking existing configurations.

@talyz
Copy link
Collaborator

talyz commented Apr 7, 2024

For the NixOS module, there's already an enable option that can be set for each declared persistent storage path, i.e. environment.persistence."/persistent".enable. It seems to be undocumented, though. It should be added to the home-manager module as well.

@0xf09f95b4
Copy link
Author

It might seem a bit nitpicky, but disabling all storage paths is not completely side-effect free, even environment.persistence = {} still leads to a few empty activation script files added to the system.

I'd still really love a global enable/disable switch for impermanence.

Adding individual enable options to the home-manager module would be a good idea regardless.

@talyz
Copy link
Collaborator

talyz commented Apr 16, 2024

It shouldn't be a problem to make it side-effect free, and if we do, I don't see the need for global switches.

@Kreyren
Copy link

Kreyren commented May 19, 2024

environment.persistence."/persistent".enable

It shouldn't be a problem to make it side-effect free, and if we do, I don't see the need for global switches.

On a more complicated setup that is using various directories it's really pain to manage and i want to implement a logic that would on demand enable/disable impermanence per system, so the proposed options would make programming logic practically usable on complex setups.

Copy link

@Kreyren Kreyren left a comment

Choose a reason for hiding this comment

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

Propose for alternative management of the default option

Whether to enable impermanence for home-manager.
Defaults to "true".
'';
default = true;
Copy link

Choose a reason for hiding this comment

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

We could use true here, but add a warning to all new deployments and then change that to false after a reasonable amount of time? Same for the option below.

Kreyren added a commit to Arcanyx-org/impermanence that referenced this pull request May 28, 2024
@ElvishJerricco
Copy link

ElvishJerricco commented Jun 7, 2024

@talyz

It shouldn't be a problem to make it side-effect free, and if we do, I don't see the need for global switches.

Well, it's not uncommon one module to set options for another module in case the other module is enabled. In order to do this, you really need an enable switch to check in a mkIf, or you need it to be that the options you set will have no effect if the user has chosen to disable the other module.

Neither method is possible with impermanence right now, short of the user knowing to do environment.persistence = lib.mkForce {};

@talyz
Copy link
Collaborator

talyz commented Jun 9, 2024

But we already have enable switches, they're just on a submodule level instead of a module one. Why would this not be enough?

Usage example:

{
  config = lib.mkIf config.environment.persistence.main.enable {
    ...
  }
}

or with more than one persistent storage path

{
  config = lib.mkIf (config.environment.persistence.main.enable && config.environment.persistence.backup.enable) {
    ...
  }
}

@talyz
Copy link
Collaborator

talyz commented Jun 9, 2024

It might seem a bit nitpicky, but disabling all storage paths is not completely side-effect free, even environment.persistence = {} still leads to a few empty activation script files added to the system.

Should be solved by #189. Please check it out.

@bddvlpr
Copy link

bddvlpr commented Jun 9, 2024

For the NixOS module, there's already an enable option that can be set for each declared persistent storage path, i.e. environment.persistence."/persistent".enable. It seems to be undocumented, though. It should be added to the home-manager module as well.

To follow up on this, is there a specific reason this isn't implemented in home-manager or has it just not been developed yet?

@talyz
Copy link
Collaborator

talyz commented Jun 9, 2024

To follow up on this, is there a specific reason this isn't implemented in home-manager or has it just not been developed yet?

No reason, it just hasn't been developed yet.

@ElvishJerricco
Copy link

But we already have enable switches, they're just on a submodule level instead of a module one. Why would this not be enough?

I'm not following. The point I made was that a module would like to set environment.persistence."/foo" so that it only takes effect if the config is using impermanence at all. Right now it's not possible to mkIf that kind of condition. So a module either configures /foo to be an impermanence directory or it doesn't, and it cannot decide whether or not to do that based on whether impermanence is in use.

@talyz
Copy link
Collaborator

talyz commented Jun 12, 2024

There's no need for mkIf in that case: the module can just go ahead and always set the options in environment.persistence."/foo"; whether that configuration will be active can be decided by other modules through environment.persistence."/foo".enable.

@ElvishJerricco
Copy link

I think that's missing the point. If an external module X creates several environment.persistent entries, then I the user of X have to be aware of exactly which entries X made and include the appropriate enable = false lines for each of them in any config that I don't want persistence for. That is extremely error prone and un-ergonomic. As is, the best way to accomplish this is for X to have its own usePersistence module option, which is just a global enable switch made by someone else instead.

@Kreyren
Copy link

Kreyren commented Jun 20, 2024

I forked the repo and implemented this commit, referencing the real life usecase: https://github.com/search?q=repo%3ANiXium-org%2FNiXium+boot.impermanence.enable&type=code

So that now i can declaratively rebuild any system with impermanence and without it on demand.

@talyz
Copy link
Collaborator

talyz commented Jun 22, 2024

@ElvishJerricco In what scenario would an external module create persistentStoragePaths? If it would, you would have bigger issues anyway, such as providing backing storage for them. Disregarding that, it would also be pretty strange for an external module to create many different persistentStoragePaths as most people only need one or maybe two of them and the reason you would need more would certainly be outside the scope of the module anyway.

@Kreyren For your use, you could just use environment.persistence."/nix/persist/system".enable instead - just set it to false for your non-impermanence systems.

@Kreyren
Copy link

Kreyren commented Jun 24, 2024

@Kreyren For your use, you could just use environment.persistence."/nix/persist/system".enable instead - just set it to false for your non-impermanence systems. -- @talyz (#171 (comment))

The problem is that environment.persistent.<path> has a variable so automating that is a problem as in real-life usage you standardize /persistence for non-disko, but for disko setup it's more convinient to use /nix/persist to be able to e.g. just declare the whole /nix as BTRFS with @persist sub-volume (other filesystem setups have different needs depending on the projected data type that they will hold).

Then for users i am providing /nix/persist/users through system and for invidual users using /nix/persist/users/<user> which gets enabled per user so the check for mkIf config.environment.persistent.<path>.enable becomes unusable with complexity as i would have to do add checks for each user to the condition.

Additionally I expect the setup on system-level to get significantly more complicated with integrating Arcanyx-org/NiXium#16

@ElvishJerricco
Copy link

@talyz I'm pretty tired of explaining the use case, and it's frustrating that your answer continues to be "I wouldn't be in that scenario".

It's standard practice for a module to have a global enable flag. The amount of discussion on this PR is ridiculous. I'm out.

@talyz
Copy link
Collaborator

talyz commented Jun 25, 2024

@ElvishJerricco I was hoping that informing about the current enable options and how to use them would be enough to keep this discussion short, but obviously not. Instead, I'll do what I was hoping to avoid spending time on and explain my current stance on this and how to use the module in different scenarios at length.

My current stance

  • Having a global enable switch doesn't fit into the current design. This is made pretty obvious by this PR putting the option in boot.impermanence when the other config is in environment.persistence. As the top level is a submodule where you declare new and unrelated storage contexts, that's where it makes sense to enable/disable them as well.
  • It has been claimed multiple times here that it's standard practice for modules to have global enable switches and that false would be the correct default for them, but this mostly applies to services and not even all services have them. The modules aren't services and shouldn't be thought of as such either. Think of them more as part of the environment configuration, closer to environment.etc.
  • Tying into both of the above, it would be confusing to have a global switch (especially one that is false by default), since that's not what you would expect from similar modules. There's no environment.useEtc, systemd.tmpfiles.enable or fileSystems.enable.
  • There's nothing wrong with using lib.mkForce {} when you want to force disable all persistence settings! In fact, I think it conveys the intention better and feels less hacky than a detached boot.impermanence setting.

Usage in different, progressively more niche, scenarios

  • In the standard use case
    • You would need one or possibly two entries under environment.persistence - one for backed-up data and one for cache / data you're okay with losing.
    • Just set the enable switches directly.
  • For a multi-user system where every user should have their data stored separately, but configuration should be shared
    • Generate one environment.persistence.<user> entry per user.
    • Condition environment.persistence.<user>.enable = lib.mkDefault on the deciding factor for why a system should use persistence
    • or add your own option for it, if you're making your own internal module / the deciding factor is random.
    • This way, you can even decide to turn on or off persistence for individual users per system
    • or have instances of the system that only persist system files, but not user files.
  • You're making an external module that sets up partitions and storage + optionally wraps the persistence module
    • Expose your own option to enable / disable persistence! Yes, this is the right thing to do:
      • Well designed modules offer options to turn on/off usage of optional external services - the user shouldn't have to read the source and set these themselves.
      • What if the user just wants to disable the persistence offered by the module, but persist things outside of it?
  • You have declared 20 different entries under environment.persistence, but most systems will only use 3
    • Set environment.persistence.<whatever>.enable = lib.mkDefault false for each entry when declaring it.
    • Enable only the ones you need for each system.

@ElvishJerricco
Copy link

This is exactly what I'm talking about. This is a real "you're holding it wrong" moment.

@talyz
Copy link
Collaborator

talyz commented Jun 25, 2024

I've presented my take on this and shown how the current option can be used. I don't think there's much more I can do or add here, and my patience for your demands is running out.

@talyz talyz closed this Jun 25, 2024
@nix-community nix-community locked and limited conversation to collaborators Jun 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants