qemu-vm: fix script syntax and VM args #34409

steveej · 2018-01-30T08:18:58Z

Motivation for this change

The target vmWithBootLoader is broken for my configuration. The VM hangs after pivoting to the real rootfs, waiting on boot (/dev/vdb2) forever.
An empty line (without ) in between the arguments for qemu will have the remainder arguments ignored
The network parameter vlan is deprecated, instead qemu suggests to use the -netdev

Please let me know if you need me to split into separate commits.

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-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 nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

Mic92 · 2018-01-30T09:14:04Z

Is this the same code used in nixos-rebuild build-vm-with-bootloader?

steveej · 2018-01-31T22:12:03Z

@Mic92 Yes

Mic92 · 2018-01-31T22:41:21Z

I tried the following:

{ lib, config, pkgs, fetchgit, ... }:
{
  users.users.root.initialPassword = "root";
  systemd.services."serial-getty@ttyS0".enable = true;
  networking.firewall.enable = false;
}

$ nixos-rebuild -I nixos-config=./test-vm.nix build-vm-with-bootloader
building Nix...
building the system configuration...
these derivations will be built:
  /nix/store/6z05y9a1bqi5n51rxc2pmcz2bqr584w2-nixos-boot-disk.drv
  /nix/store/h70k8wn3b5rn3r066lwdaq4fpkdyhq7a-run-nixos-vm.drv
  /nix/store/b6sg27fqhw32ll7fb7rp2al0xy27jjya-nixos-vm.drv
building '/nix/store/6z05y9a1bqi5n51rxc2pmcz2bqr584w2-nixos-boot-disk.drv'...
Formatting '/nix/store/z0l74v7na2027xbfdq6anci097vjp33s-nixos-boot-disk/disk.img', fmt=qcow2 size=41943040 cluster_size=65536 lazy_refcounts=off refcount_bits=16
QEMU 2.11.0 monitor - type 'help' for more information
(qemu) qemu-system-x86_64: -serial pty: char device redirected to /dev/pts/0 (label serial0)
builder for '/nix/store/6z05y9a1bqi5n51rxc2pmcz2bqr584w2-nixos-boot-disk.drv' failed with exit code 1
cannot build derivation '/nix/store/h70k8wn3b5rn3r066lwdaq4fpkdyhq7a-run-nixos-vm.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/b6sg27fqhw32ll7fb7rp2al0xy27jjya-nixos-vm.drv': 1 dependencies couldn't be built
error: build of '/nix/store/b6sg27fqhw32ll7fb7rp2al0xy27jjya-nixos-vm.drv' failed

The same error happens on current master.
So I assume there are also some other command line arguments that changed beside the ones you apparently fixed.
How can I test your pull request?

steveej · 2018-05-11T07:14:58Z

The same error happens on current master.

I hadn't noticed that before pushing. I actually ran my tests against an outdated branch and only cherry-picked the commit. I was able to reproduce the issue on master and might be able to push a fix on the weekend.

steveej · 2018-09-16T19:46:40Z

I have outlined the changes required for 18.03 which make it work for me here: steveej-forks/nixpkgs-channels#1

I'm building this with the following script:

#!/usr/bin/env bash
set -x
rm $(hostname).qcow2
set -e

nix-build -K --show-trace ${HOME}/src/github/NixOS/nixpkgs-channels/nixos \
    -I nixos-config=${HOME}/src/steveej/nix-expressions/nixos-configuration/steveej-laptop/configuration.nix \
    -A vmWithBootLoader

./result/bin/run-*-vm

I will try to reproduce this on master too and update this PR

steveej · 2018-09-23T10:14:48Z

@aszlig Any pointers on how to design a nixos/test which executes the script produced by the vmWithBootLoader derivation and makes use of the test-driver?

steveej · 2018-09-23T22:22:02Z

@Mic92 the easiest way to test this would be to do the following while you're in the working tree with this branch: nixos-rebuild -I nixos-config=./test-vm.nix -I nixpkgs=$PWD build-vm-with-bootloader && ./result/bin/run-nixos-vm. You'll find that without the commit in this PR the VM will not boot.

Mic92 · 2018-09-24T10:59:55Z

Thanks!

Mic92 · 2018-09-24T11:00:17Z

@xeji do we want this on release-18.09?

dezgeg · 2018-09-24T11:15:42Z

pkgs/build-support/vm/default.nix

@@ -196,9 +197,17 @@ rec {
    ${qemuBinary qemu} \
      -nographic -no-reboot \
      -device virtio-rng-pci \
+      ${if "$diskInterface" == "scsi" then '' \
+        \ # FIXME: /dev/sda is not created within the VM


How about using virtio-scsi-pci?

Good idea, should be more efficent than lsi53c895a.

xeji · 2018-09-24T13:24:31Z

@Mic92 let me run some tests, I will backport unless there are issues.

xeji · 2018-09-24T16:06:05Z

backported to 18.09: 174e19d

eadwu · 2018-09-24T23:38:18Z

This seems to break config.system.build.virtualBoxOVA for me.

          nix-build "<nixpkgs/nixos>" \
            -A config.system.build.virtualBoxOVA \
            -I nixos-config=vm.nix

vm.nix

{ config, pkgs, lib, ... }:

{
  imports =
    [
      <nixpkgs/nixos/modules/virtualisation/virtualbox-image.nix>
    ];

  virtualbox = {
    baseImageSize = 64 * 1024;
    memorySize = 2048;
  };

  nixpkgs = {
    config = {
      allowUnfree = true;
    };
  };
}

error: while evaluating the attribute 'diskInterface' of the derivation 'nixos-ova-19.03.git.703d5ff-x86_64-linux' at $HOME/Downloads/nixpkgs/pkgs/stdenv/generic/make-derivation.nix:177:11:
attribute 'diskInterface' missing, at $HOME/Downloads/nixpkgs/pkgs/build-support/vm/default.nix:316:24

…ixes" This reverts commit 21b29cd, reversing changes made to 4a9ca1d.

matthewbauer · 2018-09-25T02:21:47Z

Yep same error on Hydra for nixos.ova

aszlig · 2018-09-25T04:04:35Z

nixos/modules/virtualisation/qemu-vm.nix

+    ''${if isSCSI then
+        "-device lsi53c895a -device ${diskInterface}-hd,${commonInterfaceArgs}"
+      else
+        "-device virtio-blk-pci,scsi=off,${commonInterfaceArgs}"} ''


Why the '' here? (if isSCSI then ... else ...) should be enough.

aszlig · 2018-09-25T04:13:40Z

nixos/modules/virtualisation/qemu-vm.nix

          ${if cfg.useBootLoader then ''
-            ${mkDiskIfaceDriveFlag "0" "file=$NIX_DISK_IMAGE,cache=writeback,werror=report"} \
-            ${mkDiskIfaceDriveFlag "1" "file=$TMPDIR/disk.img,media=disk"} \
+            -boot menu=on \


Why do we need this? If the user wants to have a boot menu, they can still provide additional arguments via command line or within the configuration.

aszlig · 2018-09-25T04:18:48Z

nixos/modules/virtualisation/qemu-vm.nix

-            ''}
-          '' else ''
-            ${mkDiskIfaceDriveFlag "0" "file=$NIX_DISK_IMAGE,cache=writeback,werror=report"} \
+            \''}


This makes it pretty much unreadable, why not ${optionalString cfg.useEFIBoot "-pflash $TMPDIR/bios.bin"} \?

aszlig · 2018-09-25T04:25:19Z

pkgs/build-support/vm/default.nix

+        -device lsi53c895a \
+        -device scsi-hd,drive=hd,id=scsi1,bootindex=1 \
+        ''${diskImage:+-drive file=$diskImage,media=disk,if=none,id=hd,cache=unsafe,werror=report} \
+      '' else '' \


Note that when you leave out the \, the first newline is stripped, so this just adds another superfluous \ to the table for which I don't see any reason here.

aszlig · 2018-09-25T04:26:54Z

pkgs/build-support/vm/default.nix

+      '' else '' \
+        -drive file=$diskImage,media=disk,if=none,id=hd \
+        -device virtio-blk-pci,scsi=off,drive=hd,id=virtio0,bootindex=1 \
+      \''}


Why not add the backslash after the }?

aszlig · 2018-09-25T04:32:39Z

pkgs/build-support/vm/default.nix

    requiredSystemFeatures = [ "kvm" ];
    builder = "${bash}/bin/sh";
    args = ["-e" (vmRunCommand qemuCommandLinux)];
    origArgs = args;
    origBuilder = builder;
+    diskInterface = "${moreArgs.diskInterface}";


Note that this is not only called by just the qemu-vm module, so you really need to take care of falling back to a default value.

aszlig · 2018-09-25T04:43:46Z

Thanks for the contribution, however:

IMHO this makes the module less readable overall, because it adds another way to handle the backslashes. Ideally we might want to get rid of all that nested antiquotation and either use an attribute set or list to build those arguments.

Apart from that, the commit message provides no detail whatsoever about why you did certain things. Unfortunately it already got merged, so I'd suggest to provide more details for commits containing substantial changes in the future :-) Ideally there should be one commit for each logical step.

xeji · 2018-09-25T09:10:52Z

I have reverted this on 18.09 in 294060d
As for master, I'm not sure what's better: revert and try again, or fix the current state?

srhb · 2018-09-25T09:11:38Z

@xeji Ah, I just reverted it on master, you got to release-18.09 before me. I think we should fix the evaluation error before continuing on both branches.

This reverts commit f777d2b. cc #34409 This breaks evaluation of the tested job: attribute 'diskInterface' missing, at /nix/store/5k9kk52bv6zsvsyyvpxhm8xmwyn2yjvx-source/pkgs/build-support/vm/default.nix:316:24

aszlig · 2018-09-25T09:30:33Z

@srhb: The eval error would be gone by adding diskInterface ? "virtio" to the function args of runInLinuxVM and use that in the attribute set, but that would not really fix anything.

In practice however more fixing needs to be done, for example if you look at https://github.com/steveeJ/nixpkgs/blob/f777d2b719be0db4cb7819d5b6dbdd121db29a37/pkgs/build-support/vm/default.nix#L200, the code is essentially a no-op which always falls back to the else branch. I'm not sure what the original author has intended here and actually we shouldn't have something like this in runInLinuxVM at all, because it already allows passing arbitrary QEMU args, so there really is no need to add a special case for diskInterface.

srhb · 2018-09-25T09:37:26Z

@aszlig I didn't mean (or want) to try and make quickfixes to the existing commit just in order to get eval going again. The revert is fine until we can get it sorted out properly, imo. :)

GrahamcOfBorg added 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Jan 30, 2018

virtualization/qemu-vm: fix and improve virtio/scsi switching

f777d2b

steveej force-pushed the steveej-qemu-vm-fixes branch from 5c35ed0 to f777d2b Compare September 22, 2018 22:09

GrahamcOfBorg added 6.topic: nixos 8.has: module (update) labels Sep 22, 2018

Mic92 merged commit 21b29cd into NixOS:master Sep 24, 2018

dezgeg reviewed Sep 24, 2018

View reviewed changes

steveej deleted the steveej-qemu-vm-fixes branch September 24, 2018 12:19

eadwu added a commit to eadwu/nixpkgs that referenced this pull request Sep 25, 2018

Revert "Merge pull request NixOS#34409 from steveeJ/steveej-qemu-vm-f…

59b77f3

…ixes" This reverts commit 21b29cd, reversing changes made to 4a9ca1d.

aszlig reviewed Sep 25, 2018

View reviewed changes

xeji mentioned this pull request Sep 25, 2018

ISO/OVA closure size reductions #47296

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

qemu-vm: fix script syntax and VM args #34409

qemu-vm: fix script syntax and VM args #34409

steveej commented Jan 30, 2018

Mic92 commented Jan 30, 2018

steveej commented Jan 31, 2018

Mic92 commented Jan 31, 2018 •

edited

steveej commented May 11, 2018

steveej commented Sep 16, 2018

steveej commented Sep 23, 2018

steveej commented Sep 23, 2018

Mic92 commented Sep 24, 2018

Mic92 commented Sep 24, 2018

dezgeg Sep 24, 2018

xeji Sep 24, 2018

xeji commented Sep 24, 2018

xeji commented Sep 24, 2018

eadwu commented Sep 24, 2018 •

edited

matthewbauer commented Sep 25, 2018

aszlig Sep 25, 2018

aszlig Sep 25, 2018

aszlig Sep 25, 2018

aszlig Sep 25, 2018

aszlig Sep 25, 2018 •

edited

aszlig Sep 25, 2018

aszlig commented Sep 25, 2018

xeji commented Sep 25, 2018

srhb commented Sep 25, 2018

aszlig commented Sep 25, 2018 •

edited

srhb commented Sep 25, 2018

qemu-vm: fix script syntax and VM args #34409

qemu-vm: fix script syntax and VM args #34409

Conversation

steveej commented Jan 30, 2018

Motivation for this change

Things done

Mic92 commented Jan 30, 2018

steveej commented Jan 31, 2018

Mic92 commented Jan 31, 2018 • edited

steveej commented May 11, 2018

steveej commented Sep 16, 2018

steveej commented Sep 23, 2018

steveej commented Sep 23, 2018

Mic92 commented Sep 24, 2018

Mic92 commented Sep 24, 2018

dezgeg Sep 24, 2018

Choose a reason for hiding this comment

xeji Sep 24, 2018

Choose a reason for hiding this comment

xeji commented Sep 24, 2018

xeji commented Sep 24, 2018

eadwu commented Sep 24, 2018 • edited

matthewbauer commented Sep 25, 2018

aszlig Sep 25, 2018

Choose a reason for hiding this comment

aszlig Sep 25, 2018

Choose a reason for hiding this comment

aszlig Sep 25, 2018

Choose a reason for hiding this comment

aszlig Sep 25, 2018

Choose a reason for hiding this comment

aszlig Sep 25, 2018 • edited

Choose a reason for hiding this comment

aszlig Sep 25, 2018

Choose a reason for hiding this comment

aszlig commented Sep 25, 2018

xeji commented Sep 25, 2018

srhb commented Sep 25, 2018

aszlig commented Sep 25, 2018 • edited

srhb commented Sep 25, 2018

Mic92 commented Jan 31, 2018 •

edited

eadwu commented Sep 24, 2018 •

edited

aszlig Sep 25, 2018 •

edited

aszlig commented Sep 25, 2018 •

edited