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/virtualisation/linode-image: init #155426

Merged
merged 10 commits into from
Sep 28, 2022

Conversation

cyntheticfox
Copy link
Contributor

Create both a configuration and image definition file for Linode, which
uses a QEMU-based configuration but without partition tables for some
reason. Largely bases most of the configuration and image generation on the current GCE configuration.

Motivation for this change

I want to easily be able to use NixOS in Linode, so I wrote a configuration and image builder configuration for it. It seems to work based on my testing of local building and pushing and running a simple image in Linode and doing so with Terranix.

Will require adding an interface to nix-community/nixos-generators, but I can pr one easily with this added to nixpkgs.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • 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/)
  • 22.05 Release Notes (or backporting 21.11 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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation 8.has: module (update) This PR changes an existing module in `nixos/` labels Jan 17, 2022
@cyntheticfox
Copy link
Contributor Author

cyntheticfox commented Jan 17, 2022

A couple of notes:

  • I decided against adding Longview to the default configuration due to its explicit dependency on a private API key
  • The disk configuration (/dev/sda as boot and /dev/sdb as swap) is based on the default configuration for images created by Linode through their Wizard and standard "simple" API interfaces
  • Linode images must be uploaded gzipped, and with no partition table (thus requiring boot.loader.grub.forceInstall to be set)
  • Most of this configuration is given in the Linode NixOS configuration guide, but some parts are not -- particularly kernel modules and the partitionless disk scheme. Those which were not given by the guide were found experimentally by following the guide and observing the generated hardware-configuration.nix file.
  • The Linode wizard and standard API will overwrite the shadow file on disk creation to set the root passsword, which can have unexpected/unintended side-effects.
  • I do not know what specifying an authorized ssh key in the wizard or via the API will do.
  • Due to the simplicity of the configuration, I'm also opening to backporting this to 21.11

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 labels Jan 17, 2022
@cyntheticfox
Copy link
Contributor Author

It also should be mentioned that in order to boot properly, as in the guide, the boot kernel must be set to "GRUB2" in the Linode boot configuration, with all boot helpers disabled. I'm not really sure where to mention that though, if at all, since it is kind of out-of-scope of the image itself.

@nixos-discourse
Copy link

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/741

@cyntheticfox
Copy link
Contributor Author

Forgot to mention, I made a Proof-of-Concept a little while ago using Terranix with the noted caveats: houstdav000/terranix-linode-poc

@cyntheticfox
Copy link
Contributor Author

Rebased and changed docs changes to be in 22.11 instead of 22.05. I also confirmed it works with https://github.com/houstdav000/terranix-linode-poc.

@cyntheticfox
Copy link
Contributor Author

cyntheticfox commented Aug 14, 2022

(Unverified commits are because somehow GnuPG decided to sign with a subkey when I explicitly specified the master key. Trying to figure out what's going on)

EDIT: Subkey wasn't there before. Updated my git config and resigned the commits.

Copy link
Member

@lilyinstarlight lilyinstarlight left a comment

Choose a reason for hiding this comment

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

I can confirm this seems to work for me and I'd love to see this merged!

@nixos-discourse
Copy link

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/1115

nixos/modules/virtualisation/linode-image.nix Outdated Show resolved Hide resolved
nixos/modules/virtualisation/linode-image.nix Outdated Show resolved Hide resolved
nixos/modules/virtualisation/linode-image.nix Show resolved Hide resolved
nixos/modules/virtualisation/linode-config.nix Outdated Show resolved Hide resolved
nixos/modules/virtualisation/linode-config.nix Outdated Show resolved Hide resolved
nixos/modules/virtualisation/linode-config.nix Outdated Show resolved Hide resolved
nixos/modules/virtualisation/linode-config.nix Outdated Show resolved Hide resolved
### Set networking config
#
networking = {
usePredictableInterfaceNames = false;
Copy link
Member

Choose a reason for hiding this comment

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

We are sure we want to set this?

Copy link
Member

Choose a reason for hiding this comment

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

Linode recommends turning it off and notes that the problem predictable interfaces tries to solve is irrelevant on their platform with only a single interface, but they do also state that turning it off is optional: https://www.linode.com/docs/guides/install-nixos-on-linode/#disable-predictable-interface-names

The Azure and GCP/GCE image generators for NixOS both turn off predictable names as well as the qemu-vm generator so it may be something users would expect (but I don't know enough to say for sure)

nixos/modules/virtualisation/linode-config.nix Outdated Show resolved Hide resolved
nixos/modules/virtualisation/linode-config.nix Outdated Show resolved Hide resolved
@cyntheticfox cyntheticfox force-pushed the feature/linode-image branch 2 times, most recently from 146dbfb to 506a0f8 Compare August 19, 2022 01:44
@cyntheticfox cyntheticfox requested review from SuperSandro2000 and lilyinstarlight and removed request for SuperSandro2000 August 19, 2022 01:45
@cyntheticfox cyntheticfox requested review from SuperSandro2000 and removed request for lilyinstarlight August 19, 2022 01:45
@cyntheticfox
Copy link
Contributor Author

Weird, can only request from one at a time. Fixed things and rebased. Sorry for spam.

@nixos-discourse
Copy link

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/1139

@lilyinstarlight
Copy link
Member

It looks like all of the review items have been addressed now, aside from the one open question about "predictable interface names" which I think should probably not be changed given it would (ironically) make interface names unpredictable for the config, possibly different for different Linode hypervisor hosts, and there are interface name dependent config items like networking.interfaces.eth0.useDHCP

Did you have any other comments on this PR @SuperSandro2000? (Thank you for the review, by the way!)

@bobby285271 bobby285271 added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Sep 3, 2022
@nixos-discourse
Copy link

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

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

@bobby285271 bobby285271 removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Sep 11, 2022
Create both a configuration and image definition file for Linode, which
uses a QEMU-based configuration but without partition tables for some
reason.
Replace PATH modification with direct package path substitution as
recommended by @lilyinstarlight.
Force diskSize to be a `ints.positive` to ensure only positive disk sizes are
entered.

Added per SuperSandro2000's request.
Trim the comments in the `linode-config.nix` file, removing some blank
line comments and additional leading comment characters for others.
Remove some redundant comments for immediately evident config.
Add comment and link to supporting documentation for post-image-build gzip
task.
Restrict the allowable values for the `compressionLevel` value passed to
GZIP while compressing the built image.
Disable IPv6 privacy extensions, as Linode expects them to be off by
default, adding a comment and link to supporting material.
@cyntheticfox
Copy link
Contributor Author

Should I add myself to the /maintainers/maintainers-list.nix file and add a meta.maintainers to the image? I know it's standard for packages, but I only see maintainers listed on some NixOS modules and virtualization images.

@Mindavi
Copy link
Contributor

Mindavi commented Sep 27, 2022

LGTM. Adding a meta.maintainers would be nice, even if ofborg doesn't look at it (not sure if it does) it's good to know who maintains the things.

My only comment is that some of the things put as comment here might be better off in the commit message(s). But I don't consider that a big issue.

@Mindavi
Copy link
Contributor

Mindavi commented Sep 27, 2022

I think the maintainers change needs to be 2 different commits: https://nixos.org/manual/nixpkgs/stable/#var-meta-maintainers

Signed-off-by: David Houston <houstdav000@gmail.com>
Signed-off-by: David Houston <houstdav000@gmail.com>
@cyntheticfox
Copy link
Contributor Author

Seems arbitrary, but done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants