nixos/proxmox-lxc: allow importing module without activation

[Silver-Golden]

Description of changes

A small patch to allow importing proxmox-lxc without activating it.
We have several servers, some physical, some lxc that share a base config.
We recently accidently imported lxc config into a physical server which caused several issues.

enable is set to true to not break any exisitng import of this module.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 23.11 Release Notes (or backporting 23.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

[Silver-Golden]

@illustris You seem to be the original creator of this module, would you be able to take a look at this?

[illustris]

This behavior is intentional. This module is modeled similar to the ec2 module and other target-specific modules, and is not meant to be imported by default. Instead of adding an enable option to this module, I recommend adding an option to your own shared base config module. Something like:

{config, lib, modulesPath, ...}
{
  options.myCustomNamespace.isLXC = lib.mkEnableOption "PVE LXC module"
  imports = [
    # your other imports
  ] ++ lib.optionals config.myCustomNamespace.isLXC [
    "${modulesPath}/virtualisation/proxmox-lxc.nix"
  ];
}

[Silver-Golden]

@illustris that is what I first tried, but that only leads to infinite recursion.
I had found a few possible routes I tried before going down this route (https://www.google.com/search?q=nixos+conditionally+import)
Am I correct in understanding that the imports are imported before the options are evaluated?

(oh and the sample above leads to infinite recursion)

[illustris]

Can you share samples of your code? Something minimal that reproduces the infinite recursion. You can also verify that this is not a module-specific issue by replacing proxmox-lxc.nix with amazon-image.nix. You should still run into the same infinite recursion.

[Silver-Golden]

The minimal example is basically what ye posted.

{ modulesPath,  config,  lib, ...}: {
  imports = [] ++ (lib.optional config.silver.lxc [
    (modulesPath + "/virtualisation/proxmox-lxc.nix")
  ]);

  options.silver.lxc = lib.mkOption {
    type = lib.types.bool;
    default = true;
    description = lib.mdDoc "Is this a Linux Container?";
  };
  config = {};
}

[silver@helios:~/nixos]$ sudo nixos-rebuild switch --flake '.#helios'
warning: Git tree '/home/silver/nixos' is dirty
error: infinite recursion encountered

       at /nix/store/v4vrwzcjk2aa9nv7l22j157l6v3dd0vn-source/lib/modules.nix:239:21:

          238|           (regularModules ++ [ internalModule ])
          239|           ({ inherit lib options config specialArgs; } // specialArgs);
             |                     ^
          240|         in mergeModules prefix (reverseList collected);
(use '--show-trace' to show detailed location information)

We have several servers, most running LXC and some physical.
For teh physical ones we have teh hardware-configuration.nix that ye would expect.
All of our machienes much have some shared config (nameservers, access control etc).
We could duplicate the imports and config, but will lead to them being desynced.
So the goal of this is to guard against lxc configs ever appearing on a physical server.

[illustris]

You're right, because of how the imports are evaluated, there is no way to make the import optional. I would still recommend you explicitly import the module on the LXC targets, but I see no harm in merging this.

[illustris]

Suggested change

	description = lib.mdDoc "Whether to enable the ProxmoxLXC.";
	description = lib.mdDoc "Whether to enable the Proxmox VE LXC module.";

[Silver-Golden]

Done

[Silver-Golden]

Noticed thison my backlog again.
Do we need to get a second person to review?

From what I can see there should be no conflict with the other more recent changes

[illustris]

Shouldn't need another review, since this is a relatively simple non-breaking change. I would have merged if I had access. You can try posting this in the "PRs already reviewed" thread on discourse.

[nixos-discourse]

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/4068

[Silver-Golden]

hey @SuperSandro2000

Sorry fir the ping.
I noticed ye reviewed/merged in #307163, would you be able to take a quick look at this?

[nixos-discourse]

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

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

[SuperSandro2000]

Should we make a release not entry for this?

[illustris]

There is no change in the default behaviour of importing the module. This change just gives you the option to not activate the module by setting enable to false. I don't think it needs an entry in the release notes.

[github-actions]

Successfully created backport PR for release-24.05:

• [Backport release-24.05] nixos/proxmox-lxc: allow importing module without activation #328837