Skip to content

nixos/qemu-vm: default memorySize 384 -> 1024#146804

Merged
Artturin merged 1 commit intoNixOS:masterfrom
Artturin:qemudefaultmem
Nov 21, 2021
Merged

nixos/qemu-vm: default memorySize 384 -> 1024#146804
Artturin merged 1 commit intoNixOS:masterfrom
Artturin:qemudefaultmem

Conversation

@Artturin
Copy link
Member

@Artturin Artturin commented Nov 20, 2021

the default hasn't been changed since 2009
this can improve our test performances

Motivation for this change
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/)
  • 21.11 Release Notes (or backporting 21.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@Artturin Artturin mentioned this pull request Nov 20, 2021
Merged
13 tasks
@Artturin
Copy link
Member Author

Artturin commented Nov 20, 2021 •
edited
Loading

Opinions on setting it to 2047? 2047 instead of 2048 because i saw this comment
nixos/tests/nexus.nix: { virtualisation.memorySize = 2047; # qemu-system-i386 has a 2047M limit
https://bugs.launchpad.net/maas-test/+bug/1257891

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Nov 20, 2021
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Nov 20, 2021
@ck3d
Copy link
Contributor

ck3d commented Nov 20, 2021

I would update the default only, when all tests have to set higher memory sizes.
But if we really wan to update, then we should also remove all memory size configurations which are less than the new value.

@Artturin
Copy link
Member Author

Artturin commented Nov 20, 2021

But if we really wan to update, then we should also remove all memory size configurations which are less than the new value.

I am going to do that

@Artturin Artturin marked this pull request as draft November 20, 2021 23:18
@Artturin Artturin marked this pull request as ready for review November 21, 2021 01:11
@Artturin Artturin requested a review from WilliButz as a code owner November 21, 2021 01:11
@github-actions github-actions bot added 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: pantheon The Pantheon desktop environment 6.topic: qt/kde Object-oriented framework for GUI creation 6.topic: xfce The Xfce Desktop Environment labels Nov 21, 2021
@mohe2015
Copy link
Contributor

mohe2015 commented Nov 21, 2021

I would think that this should not increase physical memory usage if the VM is not using it. Because then I think it's fine and could lead to a speedup

@Artturin
Copy link
Member Author

Artturin commented Nov 21, 2021

I would think that this should not increase physical memory usage if the VM is not using it. Because then I think it's fine and could lead to a speedup

yeah it only increases the max amount the vm can use but doesn't reserve it all

@talyz
Copy link
Contributor

talyz commented Nov 21, 2021

Opinions on setting it to 2047? 2047 instead of 2048 because i saw this comment nixos/tests/nexus.nix: { virtualisation.memorySize = 2047; # qemu-system-i386 has a 2047M limit https://bugs.launchpad.net/maas-test/+bug/1257891

Sounds good to me, considering it shouldn't consume more memory for tests that don't use it.

@Artturin Artturin requested a review from roberth November 21, 2021 14:54
roberth
roberth approved these changes Nov 21, 2021
View reviewed changes
Copy link
Member

@roberth roberth left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is very sensible.

I should note that VM memory usage probably expands quite quickly because of the disk cache, but I find it hard to imagine that even reserving 1GB per physical core would not be feasible today. So 👍

This is similar to #144788 but for memory instead of disk.

@Artturin
nixos/qemu-vm: default memorySize 384 -> 1024
54ece05 
the default hasn't been changed since 2009
this can improve our test performances

nixos/tests: remove explicit memorySize <1024

1024MiB is now the default
@Artturin
Copy link
Member Author

Artturin commented Nov 21, 2021

missed some memorySize <1024's

@Artturin Artturin merged commit 53edfe1 into NixOS:master Nov 21, 2021
@Artturin Artturin deleted the qemudefaultmem branch November 21, 2021 15:48
@Artturin Artturin mentioned this pull request Nov 21, 2021
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@roberth roberth roberth approved these changes

@WilliButz WilliButz Awaiting requested review from WilliButz

Assignees

No one assigned

Labels

6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: pantheon The Pantheon desktop environment 6.topic: qt/kde Object-oriented framework for GUI creation 6.topic: xfce The Xfce Desktop Environment 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 on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Artturin @ck3d @mohe2015 @talyz @roberth