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/lxd: add productionSetup option #71978

Merged
merged 3 commits into from Dec 14, 2019
Merged

nixos/lxd: add productionSetup option #71978

merged 3 commits into from Dec 14, 2019

Conversation

mt-caret
Copy link
Contributor

Motivation for this change

Adds an option to fix errors that may come up for ZFS storage backend users.
See https://discuss.linuxcontainers.org/t/weird-error-for-zfs-storage-backend/6008/3 for details.

Things done

Tested on own machine.

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

@@ -69,8 +81,9 @@ in
ExecStart = "@${pkgs.lxd.bin}/bin/lxd lxd --group lxd";
Type = "simple";
KillMode = "process"; # when stopping, leave the containers alone
LimitMEMLOCK = mkIf cfg.productionSetup "infinity";
LimitNOFILE = mkIf cfg.productionSetup "1048576";
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these service settings should always be added (not only in production). The debian lxd packages seems to do that too https://github.com/lxc/lxd-pkg-ubuntu/blob/dpm-bionic/debian/lxd.service#L15.

Copy link
Contributor Author

@mt-caret mt-caret Dec 6, 2019

Choose a reason for hiding this comment

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

Good point. I've added some stuff from https://github.com/lxc/lxd-pkg-ubuntu/blob/dpm-xenial/debian/lxd.service, how does this look?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks great! I would only rename the option from productionSetup to recommendedSysctlSettings in the style of the recommended* nginx options in nixos: https://nixos.org/nixos/options.html#nginx.reco

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@megheaiulian
Copy link
Contributor

@mt-caret I am not sure who we should be ping to review and merge this ?

@mt-caret
Copy link
Contributor Author

Possibly @wucke13 @Lassulus from #70209 ?

Copy link
Member

@Lassulus Lassulus left a comment

Choose a reason for hiding this comment

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

LGTM

@Lassulus Lassulus merged commit 7358e4f into NixOS:master Dec 14, 2019
@mt-caret mt-caret deleted the lxd-production-setup branch June 4, 2020 07:45
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

3 participants