Control: clone -1 -2 Control: retitle -2 Control: block -2 by -1 Properly support skipping nested VM tests in incompatible virt-subprocs Control: severity -2 wishlist Control: tags -2 - patch
Hi Martin,
Moving this to the front:
>> I split this out from the first patch, because this is what you
>> didn't like the first time around. I still think a new restriction is
>> a good idea, in order to be able to distinguish between skipped tests
>> and tests that were actually successful.
>
> Right, getting proper SKIPs is desirable. But the new restriction
> would otherwise be rather pointless.
>
> Maybe this is all trying to be overly abstract, and what we really
> want is just "Restrictions: virt-{qemu,lxc,lxd,schroot,...}"? Given
> that tests must be written in a very adt-virt-* specific way, they
> should also be able to just plain say which runner they are compatible
> with.
>
> Let's get the other two patches in shape first, formalizing this new
> restriction isn't a blocker AFAICS, right?
No, it isn't. Cloning this bug to track the SKIP stuff.
On 03/02/2016 11:52 PM, Martin Pitt wrote:
> Christian Seiler [2016-02-26 17:00 +0100]:
>> So I've spent some time again on this and have separated this into
>> three patches:
>
> Thanks for getting back to this!
I want to get this done, so I can actually start working on proper
integration tests. :)
>> 0001-adt-virt-qemu-Implement-support-for-nested-base-imag.patch
>
> We are homing in for this one, it just still looks a bit hackish to
> me:
>
>> This adds the image specified to adt-virt-qemu as an additional read
>> only virtio block device (with serial BASEIMAGE) to the VM (in
>> contrast to my first patch, this is the actual data, not a qcow2
>> image).
>>
>> It sets the ADT_BASEIMAGE environment variable to that device.
>
> I'm not sure why we need *both* that environment variable *and*
> /dev/baseimage. This doesn't generalize very well to other runners
> anyway. I'd be fine with just /dev/baseimage.
Yes, thinking about it some more, /dev/baseimage is completely
sufficient.
>> It also does two things with udev in setup-testbed:
>>
>> 1) it creates an udev rule to create a symlink /dev/baseimage to
>> the new image (ADT_BASEIMAGE remains the absolute path for now,
>> though)
>>
>> 2) it modifies 60-persistent-storage.rules to skip blkid processing
>> of UUID etc. for the base image, since the base image is likely
>> the same as the main image, so UUIDs would match. Otherwise the
>> /dev/disk/by-uuid symlinks would point to the base image drive.
>
> This is quite "eww".. Isn't it enough to add
> OPTIONS+="link_priority=-1000" to 61-baseimage.rules so that the
> symlinks of the real root device always win?
Oh, I didn't know about link_priority (but it's documented, so my bad).
Just tried it (removed the 60-...rules hack, added the link priority),
and it works.
>> The same as with my first patch: if multiple images are specified on
>> the adt-virt-qemu command line, it is impossible to determine what
>> an approrpriate base image is
>
> It is -- it must be the first one. Any subsequent ones can only be
> readonly auxiliary ones, they don't get an overlay, etc. The only
> practical application for this is to provide an iso9660 image for
> cloud-init if you want to use off-the-shelf cloud images. But this
> isn't necessary with adt-buildvm-ubuntu-cloud or vmdebootstrap, and I
> think for this case you can safely ignore any additional drives.
Ah ok, thinking about it: all additional drives are read-only anyway,
so they can't really be part of the file system that does stuff (such
as /usr, because in test setups that needs to be r/w). Sorry, I was
thinking far too generically.
>> hence my patch doesn't set the base image in that case, though you
>> may specify one explicitly via the command line.
>
> I don't mind whether or not it sets /dev/baseimage in this case -- I
> think it can't hurt really. But a new CLI option is over the top IMO,
> can we please just drop this?
>
> So I think just adding 61-baseimage.rules, the extra -drive, and
> documentation in the manpage about /dev/baseimage should be all that's
> needed.
I've attached an updated patch that does just this.
>> 0002-adt-virt-qemu-Use-host-CPU-type-by-default.patch
>>
>> Passes -cpu host to QEMU by default _unless_ --qemu-command= is
>> specified. (If qemu-command is specified, it might be the case that
>> someone wants to run a fully emulated different architecture, and
>> then -cpu host will fail spectacularly.)
>
> As I already wrote before, I really don't like this as a default, as
> it introduces unpredictability into testbeds. We've seen tests that
> behave differently with different CPU models (X.org's LLVM
> software render and mesa for example), and getting random test
> passes/failures depending on which hw you run it on is really
> non-obvious.
Isn't this then a bug of the testbed? Also note that if you run those
tests (e.g. mesa) in e.g. LXC - or on a dedicated machine (doesn't
Ubuntu have some bare metal machines where tests requiring machine
isolation are run on?), you get the same issues.
Also note that the CPU vendor id is not replaced by QEMU - so while
the CPU features (flags in /proc/cpuinfo) correspond to the emulated
CPU, the vendor name (GenuineIntel or AuthenticAMD) remains that of
the host - at least in KVM mode.
> However, I'd be totally fine with changing the default to
> "-cpu SandyBridge". This is what nova-compute-qemu uses by default
> (apparently), so this both provides a reasonably rich CPU architecture
> and increases compatibility to test runs on a cloud instance, and
> keeps predictability. WDYT?
I don't have AMD hardware to test here, but if I try to do it the
other way around, e.g. "-cpu Opteron_G5" on my Haswell box, nested
KVM doesn't work. Problem is that AMD and Intel have completely
different KVM support - AMD has something called 'svm', Intel calls
it's technology 'vmx'. And if we start to emulate SandyBridge,
nested KVM won't work on AMD at all. (CPU features are disabled if
the host CPU doesn't support them. There are some warnings present.)
That's why I went with -cpu host - it's very nice and simple. :-)
Of course, we could pick SandyBridge for Intel and something else
for AMD (no idea what would be a good idea there, I haven't followed
AMD for a while) and reduce the possible CPUs to just 2.
>> This can be overridden by the --qemu-cpu option for adt-virt-qemu,
>> which then replaces the value of QEMU's -cpu parameter. If
>> --qemu-cpu=qemu-default is specified, the -cpu option is dropped
>> completely.
>
> This option should be used so seldomly that I think
> --qemu-options='-cpu blah' is fine.
Ok, just checked: QEMU is fine with -cpu twice, takes the last one.
Then it's not a problem, should drop that option.
> So for example, the tests will never run on Debian's and
> Ubuntu's production CI.
This is a tangent, but wasn't somebody working on qemu testbeds for
the Debian CI?
>> [CAVEAT for base image stuff]
>> The first patch that adds the base image as an additional read-only
>> drive has a slight problem if old images are used
>
> What are "old" images?
So let's say you create an image with the commands from autopkgtest
3.19.3 (current sid), that doesn't have the udev rules to make sure
that the UUIDs don't get reused, then you might run into a problem.
So one needs to make sure to recreate the images with the new udev
rules to be on the safe side.
Regards,
Christian
From cf1ad072163003e5df2aa9e328e02f9b3a778fd6 Mon Sep 17 00:00:00 2001
From: Christian Seiler <christ...@iwakd.de>
Date: Thu, 3 Mar 2016 01:17:21 +0100
Subject: [PATCH 1/2] adt-virt-qemu: Implement support for nested base images.
This adds initial support for nested base images to be passed into the
test environment, so that nested VMs may be used in tests.
A read-only copy of the first image without the overlay is passed to
the VM with a hardware serial number BASEIMAGE. setup-testbed installs
udev rules that create a symbolic link /dev/baseimage for that drive.
Also, the symlink priority for that drive is lowered, because the same
file system UUIDs will be present on both the first drive and the
readonly baseimage drive.
Closes: #800845
---
debian/changelog | 5 +++++
setup-commands/setup-testbed | 11 +++++++++++
virt-subproc/adt-virt-qemu | 2 ++
virt-subproc/adt-virt-qemu.1 | 20 ++++++++++++++++++++
4 files changed, 38 insertions(+)
diff --git a/debian/changelog b/debian/changelog
index 36ee620..6a3107b 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,5 +1,6 @@
autopkgtest (3.19.4) UNRELEASED; urgency=medium
+ [ Martin Pitt ]
* setup-commands/setup-testbed: Ensure that removing cruft does not remove
cloud-init. (LP: #1539126)
* setup-commands/setup-testbed: Purge lxd and lxc.
@@ -14,6 +15,10 @@ autopkgtest (3.19.4) UNRELEASED; urgency=medium
lxc-start-ephemeral got deprecated by that. This now supports reboots in
ephemeral mode.
+ [ Christian Seiler ]
+ * adt-virt-qemu: Implement support for nested base images.
+ (Closes: #800845)
+
-- Martin Pitt <mp...@debian.org> Tue, 23 Feb 2016 18:21:51 +0100
autopkgtest (3.19.3) unstable; urgency=medium
diff --git a/setup-commands/setup-testbed b/setup-commands/setup-testbed
index b7f586f..bcdbad3 100755
--- a/setup-commands/setup-testbed
+++ b/setup-commands/setup-testbed
@@ -142,6 +142,17 @@ if [ -z "${ADT_IS_SETUP_COMMAND:-}" ]; then
fi
fi
+# set up base image udev rules
+if [ -z "${ADT_IS_SETUP_COMMAND:-}" ]; then
+ # Create /dev/baseimage symlink for the base image, and
+ # make sure that the link_priority is low, so that the
+ # drive has a lower priority for udev symlinks. (Because
+ # the same base image as for the main drive is reused.)
+ printf '%s\n%s\n' 'KERNEL=="vd*[!0-9]", ENV{ID_SERIAL}=="BASEIMAGE", OPTIONS+="link_priority=-1024", SYMLINK+="baseimage", MODE="0664"' \
+ 'KERNEL=="vd*[0-9]", ENV{ID_SERIAL}=="BASEIMAGE", OPTIONS+="link_priority=-1024"' \
+ > "$root/etc/udev/rules.d/61-baseimage.rules"
+fi
+
# go-faster apt/dpkg
echo "Acquire::Languages \"none\";" > "$root"/etc/apt/apt.conf.d/90nolanguages
echo 'force-unsafe-io' > "$root"/etc/dpkg/dpkg.cfg.d/autopkgtest
diff --git a/virt-subproc/adt-virt-qemu b/virt-subproc/adt-virt-qemu
index 965e3e8..d9ae8c8 100755
--- a/virt-subproc/adt-virt-qemu
+++ b/virt-subproc/adt-virt-qemu
@@ -476,6 +476,8 @@ def hook_open():
for i, image in enumerate(args.image[1:]):
argv.append('-drive')
argv.append('file=%s,if=virtio,index=%i,readonly' % (image, i + 1))
+ argv.append('-drive')
+ argv.append('file=%s,if=virtio,index=%i,readonly,serial=BASEIMAGE' % (args.image[0], len(args.image)))
if os.path.exists('/dev/kvm'):
argv.append('-enable-kvm')
diff --git a/virt-subproc/adt-virt-qemu.1 b/virt-subproc/adt-virt-qemu.1
index 81bb11f..ed888e6 100644
--- a/virt-subproc/adt-virt-qemu.1
+++ b/virt-subproc/adt-virt-qemu.1
@@ -26,6 +26,12 @@ does
the given images, but will instead create a temporary overlay for the
primary image, and add all other images as read-only.
+The first image without the overlay is always added as an additional
+read-only hard drive, which will be available for tests as
+.IR /dev/baseimage ,
+assuming the correct udev rules are present within the image. This allows
+for tests that require nested VMs to reuse the same image.
+
.SH REQUIREMENTS
.B adt-virt-qemu
assumes that you have already prepared a suitable Debian based QEMU image (see
@@ -45,6 +51,20 @@ with specified user and password. This will then be used to start a root
shell on ttyS1, to reduce this to the first case and to not assume that
ttyS0 stays operational throughout the whole test.
+.IP * 4
+and has the following udev rule installed as
+.IR /etc/udev/rules.d/61-baseimage.rules :
+
+.RS
+.EX
+KERNEL=="vd*[!0-9]", ENV{ID_SERIAL}=="BASEIMAGE", \\
+ OPTIONS+="link_priority=-1024", SYMLINK+="baseimage", \\
+ MODE="0664"
+KERNEL=="vd*[0-9]", ENV{ID_SERIAL}=="BASEIMAGE", \\
+ OPTIONS+="link_priority=-1024"
+.EE
+.RE
+
.SH OPTIONS
.TP
--
2.1.4
signature.asc
Description: OpenPGP digital signature
