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/proxmox-image: add additionalSpace, bootSize and diskSize options #238735

Merged
merged 1 commit into from
Jul 27, 2023

Conversation

MayNiklas
Copy link
Member

Description of changes

Since make-disk-image.nix has options to define the size of the EFI partition as well as the size of the whole disk, I think we should also add those options to proxmox-image. This makes it possible, to generate a disk image which already has the wanted size without expanding it afterwards. Since the additional space isn't used, the image size shouldn't increase (compression is being used).

, # The size of the disk, in megabytes.
# if "auto" size is calculated based on the contents copied to it and
# additionalSpace is taken into account.
diskSize ? "auto"
, # additional disk space to be added to the image if diskSize "auto"
# is used
additionalSpace ? "512M"
, # size of the boot partition, is only used if partitionTableType is
# either "efi" or "hybrid"
# This will be undersized slightly, as this is actually the offset of
# the end of the partition. Generally it will be 1MiB smaller.
bootSize ? "256M"

I used the same defaults as make-disk-image.nix.

In case any additional changes to my PR are wanted - feel free to tell me!
I'm more than happy to discuss the specific implementation.

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/)
  • 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.

@illustris
Copy link
Contributor

Thanks! I will test this change tonight.

Copy link
Contributor

@illustris illustris left a comment

Choose a reason for hiding this comment

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

Tested, works. Just have one small style suggestion.

@@ -234,6 +253,8 @@ with lib;
mkdir -p $out/nix-support
echo "file vma $out/vzdump-qemu-${cfg.filenameSuffix}.vma.zst" >> $out/nix-support/hydra-build-products
'';
bootSize = cfg.qemuConf.bootSize;
diskSize = cfg.qemuConf.diskSize;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
diskSize = cfg.qemuConf.diskSize;
inherit (cfg.qemuConf) diskSize bootSize;

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds great!
Didn't know this would be possible!
What do you think about also adding additionalSpace ?
I just realised I could add this option as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, that would be useful to have too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, that would be useful to have too.

  • added it as well
  • forced pushed the changes with better fitting commit message
  • changed name of PR
  • tested the additionalSpace option -> worked just fine

@MayNiklas MayNiklas requested a review from illustris June 21, 2023 12:14
@MayNiklas MayNiklas changed the title nixos/proxmox-image: add bootSize and diskSize options nixos/proxmox-image: add additionalSpace, bootSize and diskSize options Jun 21, 2023
Copy link
Contributor

@SebTM SebTM left a comment

Choose a reason for hiding this comment

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

LGTM :)

@emilylange emilylange merged commit 49c07cd into NixOS:master Jul 27, 2023
8 checks passed
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.

5 participants