Bug#800845: autopkgtest: Add support for nested VMs

[Christian Seiler]

On 03/09/2016 09:29 AM, Martin Pitt wrote:
> Martin Pitt [2016-03-09  8:52 +0100]:
>>> I get (w/ qemu-system 1:2.5+dfsg-4~bpo8+1) with current git master
>>> (no changes):
>>> mount: /dev/vdb1 is write-protected, mounting read-only
>>>
>>> So I can't really reproduce it. :-(
>>
>> I now get the same. I must have wrecked something on Monday, I figure.
> 
> I'm not hallucinating after all. I just tried this again:
> 
>   ./run-from-checkout tests/testpkg-simple/ --shell --- qemu 
> adt-xenial-amd64-cloud.img
> 
> Then ssh'ed in, and ran
> 
>   sudo apt-get install -y kpartx
>   sudo kpartx -av /dev/baseimage
>   sudo mount /dev/mapper/baseimage1 /mnt
>   (no "readonly" warning here)
>   sudo touch /mnt/foo
>   (succeeded!)
>   ls -l /mnt/foo
>   -rw-r--r-- 1 root root 0 Mar  9 08:22 /mnt/foo

Well, this appears to be a bug in kpartx that it does not recognize
that /dev/baseimage is write-protected and thus the dm device is not
marked as write-protected.

If I don't ssh in but connect via minicom, I also see kernel logs,
and I get:

root@autopkgtest:~# mount /dev/mapper/baseimage1 /mnt   


   
[   76.106053] blk_update_request: I/O error, dev vdb, sector 2048
[   76.108045] blk_update_request: I/O error, dev vdb, sector 2048
[   76.109563] Buffer I/O error on dev dm-0, logical block 0, lost sync page 
write
root@autopkgtest:~# [   77.609597] blk_update_request: I/O error, dev vdb, 
sector 4728283
[   77.612648] blk_update_request: I/O error, dev vdb, sector 4728538
[   77.614416] blk_update_request: I/O error, dev vdb, sector 4728793
[   77.616187] blk_update_request: I/O error, dev vdb, sector 4720864
[   77.617945] blk_update_request: I/O error, dev vdb, sector 4721119
[   77.619711] blk_update_request: I/O error, dev vdb, sector 4721374
[   77.621469] blk_update_request: I/O error, dev vdb, sector 4721629
[   77.623222] blk_update_request: I/O error, dev vdb, sector 4721884
[   83.028309] blk_update_request: I/O error, dev vdb, sector 2099200
[   83.029042] Buffer I/O error on dev dm-0, logical block 262144, lost sync 
page write
[   83.029942] JBD2: Error -5 detected when updating journal superblock for 
dm-0-8.
[   83.030781] Aborting journal on device dm-0-8.
[   83.031350] blk_update_request: I/O error, dev vdb, sector 2099200
[   83.032128] blk_update_request: I/O error, dev vdb, sector 2099200
[   83.032832] Buffer I/O error on dev dm-0, logical block 262144, lost sync 
page write
[   83.033726] JBD2: Error -5 detected when updating journal superblock for 
dm-0-8.
sudo touch /mnt/foo
[   88.035532] EXT4-fs (dm-0): previous I/O error to superblock detected
[   88.037457] blk_update_request: I/O error, dev vdb, sector 2048
[   88.039019] blk_update_request: I/O error, dev vdb, sector 2048
[   88.040554] Buffer I/O error on dev dm-0, logical block 0, lost sync page 
write
[   88.043134] EXT4-fs error (device dm-0): ext4_journal_check_start:56: 
Detected aborted journal
[   88.045469] EXT4-fs (dm-0): Remounting filesystem read-only
[   88.046897] EXT4-fs (dm-0): previous I/O error to superblock detected
[   88.048745] blk_update_request: I/O error, dev vdb, sector 2048
[   88.050147] blk_update_request: I/O error, dev vdb, sector 2048
[   88.051528] Buffer I/O error on dev dm-0, logical block 0, lost sync page 
write
touch: cannot touch '/mnt/foo': Read-only file system

(The read-only remount is due to errors=remount-ro being the default
option and me waiting a bit before trying the touch; I can also touch
/mnt/foo if I'm quick enough.)

If I try to mount just /dev/vdb1 (instead of a kpartx mapping), I get
the same message as I already posted, even with a xenial image.

I just searched a bit and found this:
https://www.redhat.com/archives/dm-devel/2013-October/msg00086.html

So it does appear to be a bug in kpartx.

> However, "foo" does not actually appear on the underlying image, so
> maybe it's sometimes using the overlay or something.

No, you just probably ssh'd in and didn't see the I/O errors in the
kernel log. ;-)

So this is completely harmless.

Regards,
Christian



signature.asc
Description: OpenPGP digital signature

Bug#800845: autopkgtest: Add support for nested VMs

[Martin Pitt]

Hello again,

Martin Pitt [2016-03-09  8:52 +0100]:
> > I get (w/ qemu-system 1:2.5+dfsg-4~bpo8+1) with current git master
> > (no changes):
> > mount: /dev/vdb1 is write-protected, mounting read-only
> > 
> > So I can't really reproduce it. :-(
> 
> I now get the same. I must have wrecked something on Monday, I figure.

I'm not hallucinating after all. I just tried this again:

  ./run-from-checkout tests/testpkg-simple/ --shell --- qemu 
adt-xenial-amd64-cloud.img

Then ssh'ed in, and ran

  sudo apt-get install -y kpartx
  sudo kpartx -av /dev/baseimage
  sudo mount /dev/mapper/baseimage1 /mnt
  (no "readonly" warning here)
  sudo touch /mnt/foo
  (succeeded!)
  ls -l /mnt/foo
  -rw-r--r-- 1 root root 0 Mar  9 08:22 /mnt/foo

However, "foo" does not actually appear on the underlying image, so
maybe it's sometimes using the overlay or something.
So whatever it is, it doesn't actually seem to modify the baseimage,
so we can ignore that for now.

Martin

-- 
Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)

Bug#800845: autopkgtest: Add support for nested VMs

[Christian Seiler]

On 03/09/2016 08:52 AM, Martin Pitt wrote:
> sorry for the delay, the flu struck :(

No worries. And I hope you get better soon!

> Christian Seiler [2016-03-07 11:28 +0100]:
>> I've attached a patch that updates the man page a bit to make
>> the current semantics you are using clearer.
> 
> Thanks! I massaged this a bit further -- this is only really relevant
> if a test calls adt-reboot-prepare.

Well, technically if somebody mounts it read-only before and does not
umount it before calling adt-reboot, it may also cause problems, but I
figure the current wording in the man page is enough - because people
really _shouldn't_ do this, because any QEMU instance using the
baseimage should have already been terminated at that point, and I
think we can presume that people using this feature will at least have
some degree of competency.

>> I get (w/ qemu-system 1:2.5+dfsg-4~bpo8+1) with current git master
>> (no changes):
>> mount: /dev/vdb1 is write-protected, mounting read-only
>>
>> So I can't really reproduce it. :-(
> 
> I now get the same. I must have wrecked something on Monday, I figure.

Ok, phew! :)

> So, I think we can finally put this bug to a rest :-)

Yes. :)

I just build-tested the current git master in a clean pbuilder chroot.
No lintian messages except the override with:
lintian -L '>=pedantic/wild-guess'

I've also tried to use adt-run on my preliminary open-iscsi tests, and
no problems there.

So from my perspective, it's all set. :-)

Regards,
Christian

PS: Once you're feeling better, it would be great if you could comment
on my open-iscsi tests (see other thread I CC'd to autopkgtest-devel).



signature.asc
Description: OpenPGP digital signature

Bug#800845: autopkgtest: Add support for nested VMs

[Martin Pitt]

Hey Christian,

sorry for the delay, the flu struck :(

Christian Seiler [2016-03-07 11:28 +0100]:
> I've attached a patch that updates the man page a bit to make
> the current semantics you are using clearer.

Thanks! I massaged this a bit further -- this is only really relevant
if a test calls adt-reboot-prepare.

> I get (w/ qemu-system 1:2.5+dfsg-4~bpo8+1) with current git master
> (no changes):
> mount: /dev/vdb1 is write-protected, mounting read-only
> 
> So I can't really reproduce it. :-(

I now get the same. I must have wrecked something on Monday, I figure.

So, I think we can finally put this bug to a rest :-)

Thanks,

Martin

-- 
Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)

Bug#800845: autopkgtest: Add support for nested VMs

[Christian Seiler]

On 07.03.2016 10:21, Martin Pitt wrote:
> However, there's still one major issue left: Despite the
> "readonly=on", one can actually mount /dev/vdb1 in the VM and write
> files into it! This sounds like a QEMU bug (running
> 1:2.5+dfsg-5ubuntu4 here), but as long as that exists this is
> dangerous as this alters your pristine base images. I already tried to
> add the "readonly=on" to the "device_add", but that's just an "unknown
> property". Unfortunately this stuff isn't documented very well..

So I just tried this on an Ubuntu Wily box, both with the QEMU from
Wily and with the QEMU from Xenial (only upgraded QEMU + deps, didn't
upgrade the entire OS) - and I really cannot reproduce this.

Host kernel: 4.2.0-30-generic
QEMU: 1:2.3+dfsg-5ubuntu9.2 and 1:2.5+dfsg-5ubuntu4
Image: adt-sid.img as generated per adt-virt-qemu(1) manpage
   instructions with vmdebootstrap (exactly, no changes!)
   Tried both writable to user executing QEMU and not
   writable to user executing QEMU.

My Debian machine with which I tried that earlier had:

Host kernel: 4.4.2-3 (from sid)
QEMU: 1:2.5+dfsg-4~bpo8+1
Image: see above

I consistently get (via adt-run --shell, autopkgtest git master, no
changes) in _any_ of these setups:

mount: /dev/vdb1 is write-protected, mounting read-only

(Now I haven't tried the newest kernel on the Ubuntu side, but I'd
_really_ be surprised if that changed anything - especially since
I did try with a recent kernel on Debian with basically the same
QEMU version.)

Regards,
Christian



signature.asc
Description: OpenPGP digital signature

Bug#800845: autopkgtest: Add support for nested VMs

[Christian Seiler]

On 03/07/2016 10:21 AM, Martin Pitt wrote:
> Martin Pitt [2016-03-07  9:27 +0100]:
>> So I'll still look into adding pre-reboot hooks, removing the drive
>> there, and writing the new udev rule into /run/ instead of /etc; this
>> should address the above issues.
> 
> The attached commit does that, and I tested it in a few iterations.
> WDYT?

Seems good to me. It's a bit unintuitive that device_del also
undoes the drive_add bit, but your code is correct.

I've tested this with my open-iscsi nested tests (see my RFC
to pkg-autopkgtest-devel) and it works out of the box. I also
did a bit of testing with --shell and it seems to work just
fine.

I've attached a patch that updates the man page a bit to make
the current semantics you are using clearer.

> However, there's still one major issue left: Despite the
> "readonly=on", one can actually mount /dev/vdb1 in the VM and write
> files into it! This sounds like a QEMU bug (running
> 1:2.5+dfsg-5ubuntu4 here), but as long as that exists this is
> dangerous as this alters your pristine base images. I already tried to
> add the "readonly=on" to the "device_add", but that's just an "unknown
> property". Unfortunately this stuff isn't documented very well..

I get (w/ qemu-system 1:2.5+dfsg-4~bpo8+1) with current git master
(no changes):
mount: /dev/vdb1 is write-protected, mounting read-only

So I can't really reproduce it. :-(

Looking at the changelog of QEMU between Debian and Ubuntu, there's
also nothing there that would affect this - on the Ubuntu side it's
stuff related to CPU flags and defining additional machine types.

Question: what happens if you do a sync() after writing? Does it
really get written or is it just that your kernel doesn't properly
detect the read-only-ness and modifies the page-cache, not
realizing that the subsequent writes will fail? If it's the latter,
since it is documented that it's read-only, I wouldn't consider
that an issue.

If it's the former, there's an easy workaround: keep the readonly
flag regardless, but create another QEMU overlay for the case that
it doesn't work, and throw the overlay away at reboots - that way,
if a guest does write to it, it's their own fault.

(Note that if this really is buggy on your end, this would be a
real problem for additional images specified on the command line,
because those are also added with readonly=on!)

Regards,
Christian
From d1e18afbf5bbab08c9f05e5c9a235cd5e6037822 Mon Sep 17 00:00:00 2001
From: Christian Seiler 
Date: Mon, 7 Mar 2016 11:21:45 +0100
Subject: [PATCH] Make semantics of /dev/baseimage a bit clearer in manpage.

---
 virt-subproc/adt-virt-qemu.1 | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/virt-subproc/adt-virt-qemu.1 b/virt-subproc/adt-virt-qemu.1
index ea8ef21..2124b2c 100644
--- a/virt-subproc/adt-virt-qemu.1
+++ b/virt-subproc/adt-virt-qemu.1
@@ -29,7 +29,11 @@ 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 .
-This allows tests that require nested VMs to reuse the same image.
+This allows tests that require nested VMs to reuse the same image. Be
+aware that the image will not be accessible during reboots of the
+testbed; before requesting a reboot of the testbed, all access to this
+image should cease and may be resumed only after test execution
+continues.
 
 .SH REQUIREMENTS
 .B adt-virt-qemu
-- 
2.1.4



signature.asc
Description: OpenPGP digital signature

Bug#800845: autopkgtest: Add support for nested VMs

[Martin Pitt]

Hello again,

Martin Pitt [2016-03-07  9:27 +0100]:
> So I'll still look into adding pre-reboot hooks, removing the drive
> there, and writing the new udev rule into /run/ instead of /etc; this
> should address the above issues.

The attached commit does that, and I tested it in a few iterations.
WDYT?

However, there's still one major issue left: Despite the
"readonly=on", one can actually mount /dev/vdb1 in the VM and write
files into it! This sounds like a QEMU bug (running
1:2.5+dfsg-5ubuntu4 here), but as long as that exists this is
dangerous as this alters your pristine base images. I already tried to
add the "readonly=on" to the "device_add", but that's just an "unknown
property". Unfortunately this stuff isn't documented very well..

Thanks,

Martin
-- 
Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)


signature.asc
Description: PGP signature

Bug#800845: autopkgtest: Add support for nested VMs

[Martin Pitt]

Hey Christian,

Christian Seiler [2016-03-06 18:18 +0100]:
> I've implemented this in the following way:
> 
>  - touch /run/autopkgtest-update-initramfs-at-reboot in setup_baseimage()
>  - /sbin/autopkgtest-reboot{,-prepare}:
>   - if /run/autopkgtest-update-initramfs-at-reboot exists AND 
>update-initramfs is present, update the initramfs
>  - /usr/share/initramfs-tools/hooks/autopkgtest (created in
>setup_baseimage()):
>   - unconditionally remove /run/autopkgtest-update-initramfs-at-reboot
> Because a initramfs-tools hook will only be called if an initramfs
> is generated - but if it's generated anyway, we might as well
> remove the flag file, because then we can save ourselves a lot of
> work.

Thanks. I applied [1] this with a few niggles ("anyway" -> "already",
PEP-8, not mix "[] || foo" and "if; then" in the same command).
I also redirected the u-i output to /dev/null, as this is run within
the context of the *test* and thus clutters up or even breaks tests
(those that don't have "allow-stderr) -- see all three
QemuRunner.test_reboot_* test cases which got broken by this. This of
course now means that *if* the command fails then the test will also
fail without any noticeable output :-(

I can't say that this makes me happy, due to the performance impact on
most real-life test runs (--apt-upgrade), not supporting r/o testbeds,
code leaking into common testbed code, and the initramfs update
running in test context. But this at least gets master back to a
reasonably working state.

So I'll still look into adding pre-reboot hooks, removing the drive
there, and writing the new udev rule into /run/ instead of /etc; this
should address the above issues.

As there was quite some back-and-forth in master, I now rebased the
recent commits to make them easier to review/understand. Sorry world
for making the next git pull a bit inconvenient!

Thanks,

Martin

[1] 
http://anonscm.debian.org/cgit/autopkgtest/autopkgtest.git/commit/?id=bc57502f1bcb

-- 
Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)


signature.asc
Description: PGP signature

Bug#800845: autopkgtest: Add support for nested VMs

[Martin Pitt]

Hello Christian,

Christian Seiler [2016-03-06 18:32 +0100]:
> But setup_baseimage() doesn't do that. It's only run the first time
> after QEMU is started - it's NOT run every boot. Just look at where
> it's hooked in. :-)

Right, sorry -- brain not working on a Sunday evening, obviously :-)
It is re-run after every revert only,  but that's fine.

> (That was the point: I only wanted to add it once so I don't have to
> deal with reboots - otherwise we need to intercept reboots and
> remove the drive and all these nasty things that I didn't want to
> deal with.)

I'll still keep this on my list, though. Rebuilding the initramfs
remains being expensive, and there could be cases where it fails.

Martin
-- 
Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)

Bug#800845: autopkgtest: Add support for nested VMs

[Christian Seiler]

Hi Martin,

On 03/06/2016 06:26 PM, Martin Pitt wrote:
> Also, is it actually safe to add the base image *again* after a reboot
> if it already is added? Won't you have two drives then, and
> /dev/baseimage randomly points to one of it?

But setup_baseimage() doesn't do that. It's only run the first time
after QEMU is started - it's NOT run every boot. Just look at where
it's hooked in. :-)

(That was the point: I only wanted to add it once so I don't have to
deal with reboots - otherwise we need to intercept reboots and
remove the drive and all these nasty things that I didn't want to
deal with.)

>> What you _could_ do is create a flag file in /run and update the reboot
>> script to update the initramfs before rebooting if that flag file is
>> set - that way only if a reboot is needed will the initramfs be updated
>> and hence you optimize for the common case where that's not needed.
> 
> That's one way to mitigate this, although it still sucks as it would
> run on *every* reboot,

No, only on the first reboot, because then the initramfs is generated
and doesn't need to be regenerated.

> We also need that
> flag file on the host side to avoid re-adding the drive to the QEMU
> monitor again on each reboot.

No, we don't, see the other patch I sent. I did use --shell on various
trivial tests to verify the presence / absence of that flag file at
various points in time.

> Maybe a better way to avoid this would be to introduce the concept of
> a pre-boot hook in adt-virt-*, which could then simply remove the
> drive via the QEMU monitor.

Why? It's not necessary.

So current git master + my flag file patch + debhelper (>= 9) works
just fine for me (and I did test it quite a bit) AND is (except for
the override) completely lintian clean, even with my lintian setting.

Regards,
Christian



signature.asc
Description: OpenPGP digital signature

Bug#800845: autopkgtest: Add support for nested VMs

[Martin Pitt]

Hello Christian,

Christian Seiler [2016-03-06 16:29 +0100]:
> But that does NOT mean that the UUID clashes don't happen. From my
> experience testing this, the UUID clashes would result in ~50% 
> the time having /dev/disk/by-uuid/XXX being a symlink to /dev/vdb1
> while the other 50% it would be /dev/vda1.

Ah, this would be because on a reboot the drive is already added to
QEMU, so indeed we don't have the same situation.

> Now funnily enough for mounting the root file system in the
> initramfs, this doesn't appear to have any impact whatsoever (not
> sure why, though, haven' investigated that)

The initramfs only mounts the root fs read-only, so it doesn't really
matter which one it picks. But this might indeed clash as soon as the
real userspace takes over and tries to mount it r/w, and that's not
possible with /dev/baseimage. So in order to exploit that we'd need to
run the udev rule before systemd-remount-fs.service.

Also, is it actually safe to add the base image *again* after a reboot
if it already is added? Won't you have two drives then, and
/dev/baseimage randomly points to one of it?

> What you _could_ do is create a flag file in /run and update the reboot
> script to update the initramfs before rebooting if that flag file is
> set - that way only if a reboot is needed will the initramfs be updated
> and hence you optimize for the common case where that's not needed.

That's one way to mitigate this, although it still sucks as it would
run on *every* reboot, and for some tests like systemd that adds up
quickly! -- so, we need a more persistent flag file. We also need that
flag file on the host side to avoid re-adding the drive to the QEMU
monitor again on each reboot.

Maybe a better way to avoid this would be to introduce the concept of
a pre-boot hook in adt-virt-*, which could then simply remove the
drive via the QEMU monitor. Then the next hook_open() would re-add it,
and everyone is happy. :-)

Thanks,

Martin
-- 
Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)


signature.asc
Description: PGP signature

Bug#800845: autopkgtest: Add support for nested VMs

[Christian Seiler]

On 03/06/2016 06:17 PM, Martin Pitt wrote:
> Christian Seiler [2016-03-06 16:54 +0100]:
>> I've attached a patch that fixes the following spelling mistakes,
> 
> Thanks, applied.
> 
>> P: autopkgtest source: package-uses-old-debhelper-compat-version 8
>> I: autopkgtest source: missing-debian-source-format
>> I: autopkgtest source: vcs-field-uses-insecure-uri vcs-browser 
>> http://anonscm.debian.org/gitweb/?p=autopkgtest/autopkgtest.git
>> I: autopkgtest source: vcs-field-uses-insecure-uri vcs-git 
>> git://anonscm.debian.org/autopkgtest/autopkgtest.git
> 
> Fixed in git too.

Thanks.

However, this causes:

W: autopkgtest source: package-needs-versioned-debhelper-build-depends 9

You're going to need a Build-Depends: debhelper (>= 9) in there. ;-)

Regards,
Christian



signature.asc
Description: OpenPGP digital signature

Bug#800845: autopkgtest: Add support for nested VMs

[Martin Pitt]

Hello Christian,

Christian Seiler [2016-03-06 16:54 +0100]:
> I've attached a patch that fixes the following spelling mistakes,

Thanks, applied.

> P: autopkgtest source: package-uses-old-debhelper-compat-version 8
> I: autopkgtest source: missing-debian-source-format
> I: autopkgtest source: vcs-field-uses-insecure-uri vcs-browser 
> http://anonscm.debian.org/gitweb/?p=autopkgtest/autopkgtest.git
> I: autopkgtest source: vcs-field-uses-insecure-uri vcs-git 
> git://anonscm.debian.org/autopkgtest/autopkgtest.git

Fixed in git too.

Martin
-- 
Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)


signature.asc
Description: PGP signature

Bug#800845: autopkgtest: Add support for nested VMs

[Christian Seiler]

Hi Martin,

On 03/06/2016 04:29 PM, Christian Seiler wrote:
> What you _could_ do is create a flag file in /run and update the reboot
> script to update the initramfs before rebooting if that flag file is
> set - that way only if a reboot is needed will the initramfs be updated
> and hence you optimize for the common case where that's not needed.

So this took me longer than I hoped, because a kernel update on my host
screwed up QEMU's support for rebooting VMs in general - and it was a
real pain to figure that out.

Now that that works again:

I've implemented this in the following way:

 - touch /run/autopkgtest-update-initramfs-at-reboot in setup_baseimage()
 - /sbin/autopkgtest-reboot{,-prepare}:
  - if /run/autopkgtest-update-initramfs-at-reboot exists AND 
   update-initramfs is present, update the initramfs
 - /usr/share/initramfs-tools/hooks/autopkgtest (created in
   setup_baseimage()):
  - unconditionally remove /run/autopkgtest-update-initramfs-at-reboot
Because a initramfs-tools hook will only be called if an initramfs
is generated - but if it's generated anyway, we might as well
remove the flag file, because then we can save ourselves a lot of
work.

So basically, IF a reboot is requested AND no initramfs was generated
since, THEN and ONLY THEN will update-initramfs be called. So we minimize
how often it's required to actually generate it.

Tested it against my update open-iscsi package tests (see my RFC mail),
and it works without a hitch.

Patch is attached.

Hopefully that satisfies your performance requirements?

Regards,
Christian
From f4652b649883e2d11aeb7354e3dcf4e52135e5e1 Mon Sep 17 00:00:00 2001
From: Christian Seiler 
Date: Sun, 6 Mar 2016 17:04:24 +0100
Subject: [PATCH] adt-virt-qemu: regenerate initramfs only on reboots

The initramfs needs to be regenerated due to UUID collisions with the
baseimage drive, but we only need to do that if the testbed is
rebooted. So in turn just set a flag file and only regenerate the
initramfs when a reboot is requested.
---
 lib/adt_testbed.py |  6 ++
 virt-subproc/adt-virt-qemu | 14 ++
 2 files changed, 20 insertions(+)

diff --git a/lib/adt_testbed.py b/lib/adt_testbed.py
index 2e6dffd..30ee075 100644
--- a/lib/adt_testbed.py
+++ b/lib/adt_testbed.py
@@ -142,6 +142,9 @@ class Testbed:
 self.execute(['sh', '-ecC', '''[ ! -e /tmp/autopkgtest-reboot ] || exit 0; '''
   '''/bin/echo -e '#!/bin/sh -e\\n'''
   '''[ -n "$1" ] || { echo "Usage: $0 " >&2; exit 1; }\\n'''
+  '''[ ! -e "/run/autopkgtest-update-initramfs-at-reboot" ] || {'''
+  '''  if type update-initramfs >/dev/null 2>&1; then '''
+  '''update-initramfs -k all -u; fi; }\\n'''
   '''echo "$1" > /run/autopkgtest-reboot-mark\\n'''
   '''test_script_pid=$(cat /tmp/adt_test_script_pid)\\n'''
   '''p=$PPID; while true; do read _ c _ pp _ < /proc/$p/stat;'''
@@ -154,6 +157,9 @@ class Testbed:
 self.execute(['sh', '-ecC', '''[ ! -e /tmp/autopkgtest-reboot-prepare ] || exit 0; '''
   '''/bin/echo -e '#!/bin/sh -e\\n'''
   '''[ -n "$1" ] || { echo "Usage: $0 " >&2; exit 1; }\\n'''
+  '''[ ! -e "/run/autopkgtest-update-initramfs-at-reboot" ] || {'''
+  '''  if type update-initramfs >/dev/null 2>&1; then '''
+  '''update-initramfs -k all -u; fi; }\\n'''
   '''echo "$1" > /run/autopkgtest-reboot-prepare-mark\\n'''
   '''test_script_pid=$(cat /tmp/adt_test_script_pid)\\n'''
   '''kill -KILL $test_script_pid\\n'''
diff --git a/virt-subproc/adt-virt-qemu b/virt-subproc/adt-virt-qemu
index 2116be1..a0f1339 100755
--- a/virt-subproc/adt-virt-qemu
+++ b/virt-subproc/adt-virt-qemu
@@ -208,6 +208,20 @@ def setup_baseimage():
 term.send(b'udevadm settle --exit-if-exists=/dev/baseimage\n')
 VirtSubproc.expect(term, b'#', 10)
 
+# The initramfs needs to be regenerated to include the new udev
+# rules, but this is only necesssary if the testbed is rebooted, so
+# just touch a flag file.
+term.send(b'touch /run/autopkgtest-update-initramfs-at-reboot\n')
+VirtSubproc.expect(term, b'#', 10)
+
+# Add an update-initramfs hook that removes the flag file, because
+# if the test case calls update-initramfs anyway, we don't need to
+# do so ourselves.
+term.send(b'''if [ -d /usr/share/initramfs-tools/hooks ] ; then '''
+  b'''printf '#!/bin/sh\\nrm -f %s\\nexit 0\\n' "/run/autopkgtest-update-initramfs-at-reboot" '''
+  b'''> /usr/share/initramfs-tools/hooks/autopkgtest; '''
+  b'''chmod +x /usr/share/initramfs-tools/hooks/autopkgtest; fi\n''')

Bug#800845: autopkgtest: Add support for nested VMs

[Christian Seiler]

On 03/06/2016 04:29 PM, Christian Seiler wrote:
>>> While test-building I also noticed quite a few lintian informational
>>> notices about the manpages (mainly hyphen-used-as-minus-sign)
>>
>> Weird, I don't see them here, I just get the ones below and the too
>> old Standards-Version.
> 
> Ok, I'll send a patch soon, if you can wait with the release for
> another hour or so. ;-)

Ok, so I test-built autopkgtest in both Jessie and sid chroots, and the
Jessie chroots gave my hyphen-used-as-minus-sign, whereas the sid ones
don't, I just noticed.

I did a bit of digging and found:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=785353

So basically the tag is completely useless and one shouldn't care about
it anymore.

I've attached a patch that fixes the following spelling mistakes,
however:

I: autopkgtest: spelling-error-in-manpage usr/share/man/man1/adt-build-lxc.1.gz 
SYNOPSYS SYNOPSIS
I: autopkgtest: spelling-error-in-manpage usr/share/man/man1/adt-build-lxd.1.gz 
SYNOPSYS SYNOPSIS
I: autopkgtest: spelling-error-in-manpage 
usr/share/man/man1/adt-buildvm-ubuntu-cloud.1.gz SYNOPSYS SYNOPSIS
I: autopkgtest: spelling-error-in-manpage usr/share/man/man1/adt-run.1.gz 
SYNOPSYS SYNOPSIS
I: autopkgtest: spelling-error-in-manpage usr/share/man/man1/adt-run.1.gz 
preceeding preceding
I: autopkgtest: spelling-error-in-manpage 
usr/share/man/man1/adt-virt-chroot.1.gz SYNOPSYS SYNOPSIS
I: autopkgtest: spelling-error-in-manpage usr/share/man/man1/adt-virt-lxc.1.gz 
SYNOPSYS SYNOPSIS
I: autopkgtest: spelling-error-in-manpage usr/share/man/man1/adt-virt-lxd.1.gz 
SYNOPSYS SYNOPSIS
I: autopkgtest: spelling-error-in-manpage usr/share/man/man1/adt-virt-null.1.gz 
SYNOPSYS SYNOPSIS
I: autopkgtest: spelling-error-in-manpage usr/share/man/man1/adt-virt-qemu.1.gz 
SYNOPSYS SYNOPSIS
I: autopkgtest: spelling-error-in-manpage 
usr/share/man/man1/adt-virt-schroot.1.gz SYNOPSYS SYNOPSIS
I: autopkgtest: spelling-error-in-manpage usr/share/man/man1/adt-virt-ssh.1.gz 
SYNOPSYS SYNOPSIS

Also note that I get:

P: autopkgtest source: package-uses-old-debhelper-compat-version 8
I: autopkgtest source: missing-debian-source-format
I: autopkgtest source: vcs-field-uses-insecure-uri vcs-browser 
http://anonscm.debian.org/gitweb/?p=autopkgtest/autopkgtest.git
I: autopkgtest source: vcs-field-uses-insecure-uri vcs-git 
git://anonscm.debian.org/autopkgtest/autopkgtest.git

I'm not going to send a patch for these, but to make your life easier
for the Vcs URIs, here are the debian/control fields for those:

Vcs-Git: https://anonscm.debian.org/git/autopkgtest/autopkgtest.git
Vcs-Browser: https://anonscm.debian.org/gitweb/?p=autopkgtest/autopkgtest.git

(Other than that, the package is lintian clean.)

Regards,
Christian
From 4eb56702a757e0654775785f1794d7c4fa5ef420 Mon Sep 17 00:00:00 2001
From: Christian Seiler 
Date: Sun, 6 Mar 2016 16:47:16 +0100
Subject: [PATCH] Fix spelling errors in manpages.

---
 runner/adt-run.1 | 4 ++--
 tools/adt-build-lxc.1| 2 +-
 tools/adt-build-lxd.1| 2 +-
 tools/adt-buildvm-ubuntu-cloud.1 | 2 +-
 virt-subproc/adt-virt-chroot.1   | 2 +-
 virt-subproc/adt-virt-lxc.1  | 2 +-
 virt-subproc/adt-virt-lxd.1  | 2 +-
 virt-subproc/adt-virt-null.1 | 2 +-
 virt-subproc/adt-virt-qemu.1 | 2 +-
 virt-subproc/adt-virt-schroot.1  | 2 +-
 virt-subproc/adt-virt-ssh.1  | 2 +-
 11 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/runner/adt-run.1 b/runner/adt-run.1
index 57a1fc6..1274ec9 100644
--- a/runner/adt-run.1
+++ b/runner/adt-run.1
@@ -1,7 +1,7 @@
 .TH adt\-run 1 2014 autopkgtest
 .SH NAME
 adt\-run \- test an installed binary package using the source package's tests
-.SH SYNOPSYS
+.SH SYNOPSIS
 .B adt\-run
 .IR options ...
 .B \-\-\-
@@ -114,7 +114,7 @@ is a file (*.click), install given click package into testbed. If it is a click
 name (like "com.example.myapp"), assume it is already installed in the testbed
 and read the manifest from it.
 
-Run click package tests from the preceeding
+Run click package tests from the preceding
 .BR --click-source .
 If a click source directory is not specified explicitly, it will be downloaded
 according to the manifest's
diff --git a/tools/adt-build-lxc.1 b/tools/adt-build-lxc.1
index 24bf448..6deef8d 100644
--- a/tools/adt-build-lxc.1
+++ b/tools/adt-build-lxc.1
@@ -2,7 +2,7 @@
 .SH NAME
 adt-build-lxc \- Create or update autopkgtest container for adt\-virt-lxc
 
-.SH SYNOPSYS
+.SH SYNOPSIS
 .B adt-build-lxc
 .I distribution release
 .RI [ architecture ]
diff --git a/tools/adt-build-lxd.1 b/tools/adt-build-lxd.1
index ba20a27..5fb2d70 100644
--- a/tools/adt-build-lxd.1
+++ b/tools/adt-build-lxd.1
@@ -2,7 +2,7 @@
 .SH NAME
 adt-build-lxd \- Create or update autopkgtest container for adt\-virt-lxd
 
-.SH SYNOPSYS
+.SH SYNOPSIS
 .B adt-build-lxd
 .I image
 
diff --git a/tools/adt-buildvm-ubuntu-cloud.1 b/tools/adt-buildvm-ubuntu-cloud.1
index b992f32..fdce40c 100644
--- 

Bug#800845: autopkgtest: Add support for nested VMs

[Christian Seiler]

Hi Martin,

On 03/06/2016 04:19 PM, Martin Pitt wrote:
> Christian Seiler [2016-03-04 23:36 +0100]:
>> There's a small bug in the changes you made to my patch: logging.warning
>> should be adtlog.warning. Also, technically you missed a return path in
>> get_cpuflag(), since /proc/cpuinfo could contain no line that starts with
>> 'flags'.
> 
> Indeed, thanks! I made that more robust by always returning [] in the
> end:
> 
>   
> http://anonscm.debian.org/cgit/autopkgtest/autopkgtest.git/commit/?id=748453b095

Looks good, haven't tested it though.

>> While test-building I also noticed quite a few lintian informational
>> notices about the manpages (mainly hyphen-used-as-minus-sign)
> 
> Weird, I don't see them here, I just get the ones below and the too
> old Standards-Version.

Ok, I'll send a patch soon, if you can wait with the release for
another hour or so. ;-)

(Btw. I run lintian -L '>=pedantic/wild-guess', so maybe that's
why you don't see them.)

> There's however one issue which I'd like to address before release.
> Adding "update-initramfs -u" to adt-virt-qemu increased the test time
> quite dramatically. The tests/testpkg-simple/ no-op test now takes 28s
> instead of 20s here. I wonder, do we really need this? A reboot
> without an update initrd should not be affected by this: the
> /dev/baseimage node does not exist at all, thus there can't be any
> UUID clash. It only gets added after booting (and rebooting too), so
> having the initrd know about this is not required at all.
> 
> I pushed
> 
>   
> http://anonscm.debian.org/cgit/autopkgtest/autopkgtest.git/commit/?id=f5b9cabe6
> 
> now to fix this. I want to give you a chance to review this before I
> push the red release button, though. :-)

This is wrong. The udev rules do two things:

 - link_priority
 - /dev/baseimage symlink

If you don't update the initramfs, /dev/baseimage won't exist, true.
But that does NOT mean that the UUID clashes don't happen. From my
experience testing this, the UUID clashes would result in ~50% 
the time having /dev/disk/by-uuid/XXX being a symlink to /dev/vdb1
while the other 50% it would be /dev/vda1. Now funnily enough for
mounting the root file system in the initramfs, this doesn't appear
to have any impact whatsoever (not sure why, though, haven'
investigated that), but it definitely _will_ have an impact for any
other file systems. Which means that if you have a test image that
has multiple partitions (which vmdebootstrap doesn't generate, but
the docs of adt-virt-qemu do mention that _any_ VM image should do),
this _will_ break those cases.

What you _could_ do is create a flag file in /run and update the reboot
script to update the initramfs before rebooting if that flag file is
set - that way only if a reboot is needed will the initramfs be updated
and hence you optimize for the common case where that's not needed.

Regards,
Christian



signature.asc
Description: OpenPGP digital signature

Bug#800845: autopkgtest: Add support for nested VMs

[Martin Pitt]

Hey Christian,

Christian Seiler [2016-03-04 23:36 +0100]:
> There's a small bug in the changes you made to my patch: logging.warning
> should be adtlog.warning. Also, technically you missed a return path in
> get_cpuflag(), since /proc/cpuinfo could contain no line that starts with
> 'flags'.

Indeed, thanks! I made that more robust by always returning [] in the
end:

  
http://anonscm.debian.org/cgit/autopkgtest/autopkgtest.git/commit/?id=748453b095

> While test-building I also noticed quite a few lintian informational
> notices about the manpages (mainly hyphen-used-as-minus-sign)

Weird, I don't see them here, I just get the ones below and the too
old Standards-Version.

> (Also note that lintian gives me:
> W: autopkgtest: executable-not-elf-or-script 
> usr/share/autopkgtest/setup-commands/ubuntu-touch-session
> W: autopkgtest: executable-not-elf-or-script 
> usr/share/autopkgtest/setup-commands/ro-apt
> W: autopkgtest: executable-not-elf-or-script 
> usr/share/autopkgtest/setup-commands/ro-apt-update
> There are no shebangs in there: is that intentional?)

They aren't needed, but to quiesce lintian I added them now.

There's however one issue which I'd like to address before release.
Adding "update-initramfs -u" to adt-virt-qemu increased the test time
quite dramatically. The tests/testpkg-simple/ no-op test now takes 28s
instead of 20s here. I wonder, do we really need this? A reboot
without an update initrd should not be affected by this: the
/dev/baseimage node does not exist at all, thus there can't be any
UUID clash. It only gets added after booting (and rebooting too), so
having the initrd know about this is not required at all.

I pushed

  
http://anonscm.debian.org/cgit/autopkgtest/autopkgtest.git/commit/?id=f5b9cabe6

now to fix this. I want to give you a chance to review this before I
push the red release button, though. :-)

Thanks,

Martin

-- 
Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)


signature.asc
Description: PGP signature

Bug#800845: autopkgtest: Add support for nested VMs

[Christian Seiler]

Hi,

On 03/04/2016 09:31 PM, Martin Pitt wrote:
> Fixed the fd leak, some code style, PEP-8, and massaged the changelog:
> 
>   
> http://anonscm.debian.org/cgit/autopkgtest/autopkgtest.git/commit/?id=3a9cb0b9

There's a small bug in the changes you made to my patch: logging.warning
should be adtlog.warning. Also, technically you missed a return path in
get_cpuflag(), since /proc/cpuinfo could contain no line that starts with
'flags'.

I've attached a patch that fixes that for me.

While test-building I also noticed quite a few lintian informational
notices about the manpages (mainly hyphen-used-as-minus-sign), but also
some spelling errors. If you want, I can provide a patch that fixes
all of those.

(Also note that lintian gives me:
W: autopkgtest: executable-not-elf-or-script 
usr/share/autopkgtest/setup-commands/ubuntu-touch-session
W: autopkgtest: executable-not-elf-or-script 
usr/share/autopkgtest/setup-commands/ro-apt
W: autopkgtest: executable-not-elf-or-script 
usr/share/autopkgtest/setup-commands/ro-apt-update
There are no shebangs in there: is that intentional?)

Regards,
Christian
diff --git a/virt-subproc/adt-virt-qemu b/virt-subproc/adt-virt-qemu
index 5ae01b0..10c2b33 100755
--- a/virt-subproc/adt-virt-qemu
+++ b/virt-subproc/adt-virt-qemu
@@ -440,8 +440,9 @@ def get_cpuflag():
 adtlog.debug('Detected KVM capable AMD host CPU, enabling nested KVM')
 return ['-cpu', 'kvm64,+svm,+lahf_lm']
 return []
+return []
 except IOError as e:
-logging.warning('Cannot read /proc/cpuinfo to detect CPU flags: %s' % e)
+adtlog.warning('Cannot read /proc/cpuinfo to detect CPU flags: %s' % e)
 # fetching CPU flags isn't critical (only used to enable nested KVM),
 # so don't fail here
 return []


signature.asc
Description: OpenPGP digital signature

Bug#800845: autopkgtest: Add support for nested VMs

[Martin Pitt]

Hello Christian,

Christian Seiler [2016-03-04 21:53 +0100]:
> Question: I think we should also define a canonical way of
> detecting baseimage support. I propose to use:
> 
> [ -e /dev/baseimage ]
> 
> Because that is very agnostic to details: if for example some cloud
> runner also wants to support this in the future, but provides access
> to a file instead of a block device, they can symlink that to
> /dev/baseimage and this test would still catch that. Using -b would
> be suboptimal.

A file symlink in /dev/ would be rather strange, and implementing that
with a single file in schroot/containers/ssh would be both inefficient
and inconvenient (bind mounts or similar are much better there). But
for the time being tests can use that indeed.

Martin


-- 
Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)


signature.asc
Description: PGP signature

Bug#800845: autopkgtest: Add support for nested VMs

[Christian Seiler]

Hi Martin,

On 03/04/2016 09:31 PM, Martin Pitt wrote:
> Christian Seiler [2016-03-04 18:17 +0100]:
>> 0001-Support-nested-KVM-by-default-by-emulating-a-CPU-wit.patch
> 
> Fixed the fd leak,

Oops, sorry! ;-)

(I really should work with python more regularly.)

>   
> http://anonscm.debian.org/cgit/autopkgtest/autopkgtest.git/commit/?id=3a9cb0b9
>   
> http://anonscm.debian.org/cgit/autopkgtest/autopkgtest.git/commit/?id=a46c7a69
>   
> http://anonscm.debian.org/cgit/autopkgtest/autopkgtest.git/commit/?id=b6557eb02

Thanks for committing this! :-)

> Thank you! I'm running the full testsuite now, do some finishing
> touches, and will do a new upload RSN.

Ok, then I'm going to write some tests that use this feature now.
Thanks a lot for making this possible.

Question: I think we should also define a canonical way of
detecting baseimage support. I propose to use:

[ -e /dev/baseimage ]

Because that is very agnostic to details: if for example some cloud
runner also wants to support this in the future, but provides access
to a file instead of a block device, they can symlink that to
/dev/baseimage and this test would still catch that. Using -b would
be suboptimal.

Do you agree?

Regards,
Christian



signature.asc
Description: OpenPGP digital signature

Bug#800845: autopkgtest: Add support for nested VMs

[Martin Pitt]

Control: tag -1 pending

Hey Christian,

Christian Seiler [2016-03-04 18:17 +0100]:
> 0001-Support-nested-KVM-by-default-by-emulating-a-CPU-wit.patch

Fixed the fd leak, some code style, PEP-8, and massaged the changelog:

  http://anonscm.debian.org/cgit/autopkgtest/autopkgtest.git/commit/?id=3a9cb0b9

> 0002-setup-testbed-reduce-grub-timeout-on-Debian-systems.patch

Committed with refined changelog:

  http://anonscm.debian.org/cgit/autopkgtest/autopkgtest.git/commit/?id=a46c7a69

> 0003-adt-virt-qemu-make-i386-arch-work-by-default.patch

Similar PEP-8/code style massaging, committed:

  
http://anonscm.debian.org/cgit/autopkgtest/autopkgtest.git/commit/?id=b6557eb02

Thank you! I'm running the full testsuite now, do some finishing
touches, and will do a new upload RSN.

Martin

-- 
Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)


signature.asc
Description: PGP signature

Bug#800845: autopkgtest: Add support for nested VMs

[Martin Pitt]

Hey Christian,

Christian Seiler [2016-03-04  1:19 +0100]:
> I've thought about this a bit more and came to the conclusion that it'd
> be best to run udevadm settle in the VM after adding the disks to QEMU.

Excellent, thank you! I like this approach now, it's clean, reasonably
race-free, and simple.

I added a (rather shallow admittedly) test, adjusted the changelog and
some grammar fixes:

  
http://anonscm.debian.org/cgit/autopkgtest/autopkgtest.git/commit/?id=f5fd792cf06017

I'll respond to the other thread about the -cpu and Restrictions:
issues; so not adding a "pending" tag just yet.

Thanks for your work and bearing with me!

Martin

-- 
Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)


signature.asc
Description: PGP signature

Bug#800845: autopkgtest: Add support for nested VMs

[Martin Pitt]

Hello Christian,

Christian Seiler [2016-03-03  2:01 +0100]:
> > Maybe this is all trying to be overly abstract, and what we really
> > want is just "Restrictions: virt-{qemu,lxc,lxd,schroot,...}"?
> 
> No, it isn't. Cloning this bug to track the SKIP stuff.

Ack, since this is now tracked in #816564, let's drop the thread in
this bug.

> >> 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?

How so? Host hardware having different capabilities is not a bug :-)
But I'd like to abstract away from this as much as possible for
production runs, i. e. make the test environment predictable as much
as we can.

> 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.

That's correct, yes. But that doesn't mean that this is intended, just
way harder to avoid :-) We do run LXC tests for armhf and s390x where
we don't yet have QEMU support, and indeed we have much more jitter
there. But for i386/amd64/ppc64el we use Openstack instances (which
are effectively QEMU).

> > 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. :-)

Ah, I didn't have that in mind, thanks for the heads-up.

> 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.

That sounds like a reasonable first start. So if the host has "vmx",
use SandyBridge, if it has "svm" use Opteron_G5, and if it has
neither, just continue to use whatever QEMU defaults to?

That said, at least on the machines I tested, nested kvm does work in
principle (maybe some subtle bugs) with the default -cpu too.

> This is a tangent, but wasn't somebody working on qemu testbeds for
> the Debian CI?

I'm not aware of that. However, it's more realistic to use the ssh
runner with the nova setup script, given that production debci already
runs in the cloud. (This won't be able to use this nesting trick,
though).

Thanks,

Martin

-- 
Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)


signature.asc
Description: PGP signature

Bug#800845: autopkgtest: Add support for nested VMs

[Martin Pitt]

Hey Christian,

Christian Seiler [2016-03-04 12:54 +0100]:
>  - if x86_64 and default QEMU command:
> - if vmx in CPU flags:
>  use -cpu kvm64,+vmx,+lahf_lm
> - else if svm in CPU flags:
>  use -cpu kvm64,+svm,+lahf_lm
>  - otherwise: don't pass -cpu
> 
> That way, you have the same CPU on any hardware, with the exception
> of the vendor-specific VT extensions.
> 
> If you're agreeable to that, I'll prepare a patch.

Sounds great!

> > That said, at least on the machines I tested, nested kvm does work in
> > principle (maybe some subtle bugs) with the default -cpu too.
> 
> No, it doesn't. Nested QEMU does (and hence adt-virt-qemu will run
> now that you merged baseimage support), but the inner QEMU is not
> using KVM then.

It does for me:

ubuntu@autopkgtest:~$ egrep 'model name|flags' /proc/cpuinfo
model name  : QEMU Virtual CPU version 2.4.0
flags   : fpu de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov 
pse36 clflush mmx fxsr sse sse2 syscall nx lm rep_glusood nopl pni
vmx cx16 x2apic popcnt hypervisor lahf_lm tpr_shadow vnmi flexpriority
ept vpid
ubuntu@autopkgtest:~$ lsmod|grep kvm
kvm_intel 172032  0
kvm   536576  1 kvm_intel
irqbypass  16384  1 kvm
ubuntu@autopkgtest:~$ ls -l /dev/kvm 
crw--- 1 root root 10, 232 Mar  4 13:02 /dev/kvm

> (Unless you have a QEMU that defaults to some other CPU type by
> default OR you explicitly set the CPU type, perhaps in some
> configuration you forgot about.)

Indeed it seems Ubuntu's qemu is modified accordingly
(https://launchpad.net/ubuntu/+source/qemu/1:2.5+dfsg-5ubuntu1):

- d/p/ubuntu/expose-vmx_qemu64cpu.patch: enable nested kvm by default
  in qemu64 cpu type.

This is

  
http://anonscm.debian.org/cgit/pkg-qemu/qemu.git/tree/debian/patches/ubuntu/expose-vmx_qemu64cpu.patch?h=ubuntu-dev

That explains why, sorry for being dense there.

> >> This is a tangent, but wasn't somebody working on qemu testbeds for
> >> the Debian CI?
> > 
> > I'm not aware of that. However, it's more realistic to use the ssh
> > runner with the nova setup script, given that production debci already
> > runs in the cloud. (This won't be able to use this nesting trick,
> > though).
> 
> This is kind of a bummer for me, because I'd really like to have
> continuous integration for actual functionality that relies on
> nested VMs. But in principle qemu should run in the cloud, right?

In most Openstack ones at least, yes. There are other nova-compute
backends like libvirt or lxc, though. Also, debci is running on Amazon
EC2 (which doesn't use Openstack), but I guess it's qemu there anyway.

> So it's only a question of whether it's somehow possible to get
> a working base image in there?

Right. Openstack/EC2 hide this pretty well, so there's no way to get
the base image into the instance directly :-( It's of course possible
to download a public cloud image in the test, but that's much less
convenient.

Thanks,

Martin

-- 
Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)


signature.asc
Description: PGP signature

Bug#800845: autopkgtest: Add support for nested VMs

[Christian Seiler]

Hi Martin,

On 04.03.2016 13:16, Martin Pitt wrote:
> Indeed it seems Ubuntu's qemu is modified accordingly
> (https://launchpad.net/ubuntu/+source/qemu/1:2.5+dfsg-5ubuntu1):
> 
> - d/p/ubuntu/expose-vmx_qemu64cpu.patch: enable nested kvm by default
>   in qemu64 cpu type.

Oh, that explains it. I tried it under Debian, where this patch isn't
applied. :)

>> This is kind of a bummer for me, because I'd really like to have
>> continuous integration for actual functionality that relies on
>> nested VMs. But in principle qemu should run in the cloud, right?
> 
> In most Openstack ones at least, yes. There are other nova-compute
> backends like libvirt or lxc, though.

But I expect QEMU to run in e.g. LXC, at least if /dev/kvm is available.
(Haven't tried it though.) I mean, QEMU runs as a normal user, so I
don't see how LXC would interfere. (Except for device group permissions
for /dev/kvm, but that can be remedied.)

Also, at least in my case, I require isolation-machine anyway, because
the server I set up is also kernel-related, so I need VMs. In other
cases it could be the case though that QEMU within LXC would be
sufficient for testing the software.

>> So it's only a question of whether it's somehow possible to get
>> a working base image in there?
> 
> Right. Openstack/EC2 hide this pretty well, so there's no way to get
> the base image into the instance directly :-( It's of course possible
> to download a public cloud image in the test, but that's much less
> convenient.

Yes. Ok, let's take it one step at a time: first get nested VM support
working in general for the QEMU runner, and then discuss this with the
people working on the automatic CI parts of that. (One could also make
the case that for the few packages that really need nested VM support -
it's not going to be more than a couple of dozen I guess - you could
just create a dedicated Jenkins job for those that uses the QEMU runner.
Maybe that's just the simplest solution.)

Regards,
Christian



signature.asc
Description: OpenPGP digital signature

Bug#800845: autopkgtest: Add support for nested VMs

[Christian Seiler]

On 04.03.2016 13:16, Martin Pitt wrote:
> Christian Seiler [2016-03-04 12:54 +0100]:
>>  - if x86_64 and default QEMU command:
>> - if vmx in CPU flags:
>>  use -cpu kvm64,+vmx,+lahf_lm
>> - else if svm in CPU flags:
>>  use -cpu kvm64,+svm,+lahf_lm
>>  - otherwise: don't pass -cpu
>>
>> That way, you have the same CPU on any hardware, with the exception
>> of the vendor-specific VT extensions.
>>
>> If you're agreeable to that, I'll prepare a patch.
> 
> Sounds great!

I've attached three patches:

0001-Support-nested-KVM-by-default-by-emulating-a-CPU-wit.patch

  This addresses the KVM issue. I've only done this for x86_64,
  because I don't think VMX extensions exist for pure x86_32 CPUs.
  Also, you can use an i386 VM image with x86_64, so I don't think this
  is an issue.

  Works fine here, both under Debian and Ubuntu.

The other two patches are unrelated, but I noticed them while working
on the code:

0002-setup-testbed-reduce-grub-timeout-on-Debian-systems.patch

  On Debian systems, grub has a default timeout of 5s - which means that
  boot of VM images is always delayed by that amount. This patch reduces
  that timeout to 1s to speed things up. I'm unsure about decreasing it
  to zero, because on Debian I know of now way of getting a grub menu if
  you really need it if the timeout is zero. But if you think a timeout
  of 0 is fine, feel free to change this. (I tested both locally, 0 also
  works.)

0003-adt-virt-qemu-make-i386-arch-work-by-default.patch

  On pure 32bit x86 systems adt-virt-qemu is currently broken, because
  os.uname()[4] is i686 typically, but the qemu binary ends in -i386.
  I've added a generic system to map architecture names. because I can
  easily imagine this also occurs for some other architectures.

  Reproducer is in the commit message (needs 32bit kernel + userland in
  VM image though!).

Regards,
Christian
From 245c2e8aa68b06bb83be6fad125a749690cc2a15 Mon Sep 17 00:00:00 2001
From: Christian Seiler 
Date: Fri, 4 Mar 2016 17:13:18 +0100
Subject: [PATCH 1/3] Support nested KVM by default by emulating a CPU with
 VMX/SVM support on x86_64.

Implemented support for nested KVM by default on x86_64, if the default
qemu command was specified and no CPU type was explicitly requested by
the user.
---
 debian/changelog |  2 ++
 virt-subproc/adt-virt-qemu   | 28 +++-
 virt-subproc/adt-virt-qemu.1 | 20 
 3 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/debian/changelog b/debian/changelog
index 4d38b67..dc00b57 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -20,6 +20,8 @@ autopkgtest (3.19.4) UNRELEASED; urgency=medium
   [ Christian Seiler ]
   * adt-virt-qemu: Provide read-only version of the VM image to the test as
 /dev/baseimage, for tests that want to run nested QEMU. (Closes: #800845)
+  * Support nested KVM by default by emulating a CPU with VMX/SVM support on
+x86_64.
 
  -- Martin Pitt   Tue, 23 Feb 2016 18:21:51 +0100
 
diff --git a/virt-subproc/adt-virt-qemu b/virt-subproc/adt-virt-qemu
index 9759c09..d53c94e 100755
--- a/virt-subproc/adt-virt-qemu
+++ b/virt-subproc/adt-virt-qemu
@@ -52,10 +52,11 @@ p_qemu = None
 ssh_port = None
 ssh_port_lock = None
 normal_user = None
+qemu_cmd_default = None
 
 
 def parse_args():
-global args
+global args, qemu_cmd_default
 
 parser = ArgumentParser()
 
@@ -416,6 +417,22 @@ sys.exit(rc == 255 and 253 or rc)
 VirtSubproc.bomb('failed to connect to VM')
 
 
+def get_cpuflags():
+try:
+for line in open('/proc/cpuinfo', 'r'):
+if line.startswith('flags\t') or line.startswith('flags '):
+line = line.split()
+else:
+continue
+if line[1] != ':':
+continue
+return line[2:]
+return []
+except (IOError, OSError):
+# fetching CPU flags isn't critical (only used to enable nested KVM),
+# so don't fail here
+return []
+
 def find_free_port(start):
 '''Find an unused port in the range [start, start+50)'''
 
@@ -508,6 +525,15 @@ def hook_open():
 
 if os.path.exists('/dev/kvm'):
 argv.append('-enable-kvm')
+# Enable nested KVM by default on x86_64
+if os.uname()[4] == 'x86_64' and args.qemu_command == qemu_cmd_default and (not args.qemu_options or not '-cpu' in args.qemu_options.split()):
+cpuflags = get_cpuflags()
+if 'vmx' in cpuflags:
+argv.append('-cpu')
+argv.append('kvm64,+vmx,+lahf_lm')
+elif 'svm' in cpuflags:
+argv.append('-cpu')
+argv.append('kvm64,+svm,+lahf_lm')
 
 # pass through option to qemu
 if args.qemu_options:
diff --git a/virt-subproc/adt-virt-qemu.1 b/virt-subproc/adt-virt-qemu.1
index 5e3ec3d..17ef25b 100644
--- a/virt-subproc/adt-virt-qemu.1
+++ 

Bug#800845: autopkgtest: Add support for nested VMs

[Christian Seiler]

Hi again,

On 03/03/2016 10:18 PM, Christian Seiler wrote:
> I've created a patch that does just that and tested it: with
> setup-testbed from current git master it properly installs the udev
> rule, *then* adds the drive, and we have:
> 
>  - /dev/baseimage exists
>  - /dev/disk/by-{part,}uuid/* don't point to the baseimage disk.
> 
> Patch is attached.

I've thought about this a bit more and came to the conclusion that it'd
be best to run udevadm settle in the VM after adding the disks to QEMU.
On my system without load this was not an issue, but if the system where
this is run on is under a bit of load, not waiting for udev may result
in the tests thinking that /dev/baseimage doesn't exist and thus
skipping the nested tests.

I've updated the patch to let udev settle. See attachment to this email.

> Would still like to hear a comment about the CPU issue.

Regards,
Christian
From 9909a0449237e821237fcfcf3d9a94d35208c121 Mon Sep 17 00:00:00 2001
From: Christian Seiler 
Date: Thu, 3 Mar 2016 22:10:27 +0100
Subject: [PATCH] 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. adt-virt-qemu installs
udev rules that create a symbolic link /dev/baseimage for that drive
the first time the testbed is booted. 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 |  4 
 virt-subproc/adt-virt-qemu   | 29 +
 virt-subproc/adt-virt-qemu.1 |  5 +
 3 files changed, 38 insertions(+)

diff --git a/debian/changelog b/debian/changelog
index 36ee620..85a5571 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,9 @@ 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   Tue, 23 Feb 2016 18:21:51 +0100
 
 autopkgtest (3.19.3) unstable; urgency=medium
diff --git a/virt-subproc/adt-virt-qemu b/virt-subproc/adt-virt-qemu
index 965e3e8..e11ab14 100755
--- a/virt-subproc/adt-virt-qemu
+++ b/virt-subproc/adt-virt-qemu
@@ -175,6 +175,34 @@ def login_tty_and_setup_shell():
 term.send(b'\nexit\n')
 VirtSubproc.expect(term, b'\nlogout', 10)
 
+def setup_baseimage():
+'''setup /dev/baseimage in VM'''
+
+term = VirtSubproc.get_unix_socket(os.path.join(workdir, 'ttyS1'))
+
+# Setup udev rules for /dev/baseimage; set link_priority to -1024 so
+# that the duplicate UUIDs of the partitions will have no effect.
+term.send(b'''mkdir -p -m 0755 /etc/udev/rules.d ; printf '# Created by adt-virt-qemu\\n%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"' > /etc/udev/rules.d/61-baseimage.rules\n''')
+VirtSubproc.expect(term, b'#', 10)
+# Reload udev to make sure the rules take effect (udev only auto-
+# rereads rules every 3 seconds)
+term.send(b'udevadm control -R\n')
+VirtSubproc.expect(term, b'#', 60)
+# Update the initramfs to include the new udev rules (to support
+# reboots properly)
+term.send(b'update-initramfs -k all -u\n')
+VirtSubproc.expect(term, b'#', 60)
+
+monitor = VirtSubproc.get_unix_socket(os.path.join(workdir, 'monitor'))
+
+# Add the base image as an additional drive
+monitor.send(('drive_add 0 file=%s,if=none,readonly=on,serial=BASEIMAGE,id=drive-baseimage\n' % args.image[0]).encode())
+VirtSubproc.expect(monitor, b'(qemu)', 10)
+monitor.send(b'device_add virtio-blk-pci,drive=drive-baseimage,id=virtio-baseimage\n')
+VirtSubproc.expect(monitor, b'(qemu)', 10)
+
+term.send(b'udevadm settle --exit-if-exists=/dev/baseimage\n')
+VirtSubproc.expect(term, b'#', 10)
 
 def setup_shared(shared_dir):
 '''Set up shared dir'''
@@ -501,6 +529,7 @@ def hook_open():
 # files; let QEMU run with the deleted inode
 os.unlink(overlay)
 setup_shell()
+setup_baseimage()
 setup_shared(shareddir)
 setup_config(shareddir)
 make_auxverb(shareddir)
diff --git a/virt-subproc/adt-virt-qemu.1 b/virt-subproc/adt-virt-qemu.1
index 81bb11f..2de5e3e 100644
--- 

Bug#800845: autopkgtest: Add support for nested VMs

[Christian Seiler]

Hi Martin,

On 03/04/2016 11:56 AM, Martin Pitt wrote:
> Christian Seiler [2016-03-04  1:19 +0100]:
>> I've thought about this a bit more and came to the conclusion that it'd
>> be best to run udevadm settle in the VM after adding the disks to QEMU.
> 
> Excellent, thank you! I like this approach now, it's clean, reasonably
> race-free, and simple.
> 
> I added a (rather shallow admittedly) test, adjusted the changelog and
> some grammar fixes:
> 
>   
> http://anonscm.debian.org/cgit/autopkgtest/autopkgtest.git/commit/?id=f5fd792cf06017

Fantastic. That was the major show-stopper for me writing tests
that require nested VMs. :-)

> I'll respond to the other thread about the -cpu and Restrictions:
> issues; so not adding a "pending" tag just yet.

Note that for the restrictions I cloned this bug, so discussion
about that should go in #816564. The CPU stuff I'd still like
to track in this bug, though.

> Thanks for your work and bearing with me!

I think this is one of the problems where once you figure out
what the best solution is, it seems obvious, but it takes some
time and discussion to actually get to it. And I think what we
arrived at is far simpler and more elegant than what I had in
mind initially when I described my use case. The only regret I
have in this regard is that we didn't have a chance to hash
this out at DebConf (not your fault, there was just too much
going on there in general), otherwise we might have been able
to get here quite a bit quicker. ;-)

Regards,
Christian



signature.asc
Description: OpenPGP digital signature

Bug#800845: autopkgtest: Add support for nested VMs

[Christian Seiler]

On 03/04/2016 12:11 PM, Martin Pitt wrote:
>>> 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?
> 
> How so? Host hardware having different capabilities is not a bug :-)
> But I'd like to abstract away from this as much as possible for
> production runs, i. e. make the test environment predictable as much
> as we can.

Ok, so it kind of depends on how you view testing. There are two
ways of looking at this: my perspective when I wrote that was
that variations in the hardware base will allow you to find bugs
in your software more easily - whereas you can also have the
perspective that tests are primarily for regression testing.

I think both views are valid.

>> 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.
> 
> That sounds like a reasonable first start. So if the host has "vmx",
> use SandyBridge, if it has "svm" use Opteron_G5, and if it has
> neither, just continue to use whatever QEMU defaults to?

I actually have a better idea: QEMU can set individual CPU flags.
I just tried the following:

--qemu-options='-cpu kvm64,+vmx'

The booted instance contains /dev/kvm on my system and the kvm_intel
driver is loaded (without me having to do so explicitly) and running
QEMU inside with KVM support works.

Technically, we should also add +lahf_lm there, because that's also
part of what KVM needs to do virtualization properly. Since this
feature is tied to virtualization, I don't think there are CPUs out
there that support vmx/svm but not lahf_lm. (But even if: QEMU will
then just print a warning and continue, it's not fatal.)

So we could maybe do the following:

 - if x86_64 and default QEMU command:
- if vmx in CPU flags:
 use -cpu kvm64,+vmx,+lahf_lm
- else if svm in CPU flags:
 use -cpu kvm64,+svm,+lahf_lm
 - otherwise: don't pass -cpu

That way, you have the same CPU on any hardware, with the exception
of the vendor-specific VT extensions.

If you're agreeable to that, I'll prepare a patch.

> That said, at least on the machines I tested, nested kvm does work in
> principle (maybe some subtle bugs) with the default -cpu too.

No, it doesn't. Nested QEMU does (and hence adt-virt-qemu will run
now that you merged baseimage support), but the inner QEMU is not
using KVM then.

Try that on your system (with current autopkgtest):

 - adt-run $SOMEPKG --shell --- adt-virt-qemu /path/to/image
 - Log in to that instance

Then try doing:

  modprobe kvm_intel

or

  modprobe kvm_amd

(Depending on your hardware.)

Either will fail because of missing CPU flags. /dev/kvm will not
exist.

(Unless you have a QEMU that defaults to some other CPU type by
default OR you explicitly set the CPU type, perhaps in some
configuration you forgot about.)

>> This is a tangent, but wasn't somebody working on qemu testbeds for
>> the Debian CI?
> 
> I'm not aware of that. However, it's more realistic to use the ssh
> runner with the nova setup script, given that production debci already
> runs in the cloud. (This won't be able to use this nesting trick,
> though).

This is kind of a bummer for me, because I'd really like to have
continuous integration for actual functionality that relies on
nested VMs. But in principle qemu should run in the cloud, right?
So it's only a question of whether it's somehow possible to get
a working base image in there?

Regards,
Christian



signature.asc
Description: OpenPGP digital signature

Bug#800845: autopkgtest: Add support for nested VMs

[Christian Seiler]

Hi,

replying to IRC:

>  chris_se: I think the setup should go into adt-virt-qemu, not 
> setup-testbed
>  setup-testbed is used for lxc or openstack instances too
>  and it's not a strict requirement to run it
>  chris_se: but I'm fine with mopping this up myself
>  chris_se: i. e. install the rule and call udevadm trigger to run it
>  so that it works without rebooting

So yes, I think you're right that it's better for adt-virt-qemu to
configure this, but I think we can even do it better:

 - DON'T add the drive to the qemu command
 - create the rule
 - udevadm control -R (because otherwise we might fall in the 3s
   window where udev doesn't reload the config)
 - update-initramfs -k all -u (to support reboots)
 - use qemu's monitor to add the drive

This way, the wrong symlinks will never be created (because the
drive won't exist at initial boot without the udev rule) and we can
guarantee that this doesn't cause any weird problems.

I've created a patch that does just that and tested it: with
setup-testbed from current git master it properly installs the udev
rule, *then* adds the drive, and we have:

 - /dev/baseimage exists
 - /dev/disk/by-{part,}uuid/* don't point to the baseimage disk.

Patch is attached.

Would still like to hear a comment about the CPU issue.

Regards,
Christian
From e2aaf0c2d0386d62bc9929f0b84e00cdc706c8f3 Mon Sep 17 00:00:00 2001
From: Christian Seiler 
Date: Thu, 3 Mar 2016 22:10:27 +0100
Subject: [PATCH] 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. adt-virt-qemu installs
udev rules that create a symbolic link /dev/baseimage for that drive
the first time the testbed is booted. 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 |  4 
 virt-subproc/adt-virt-qemu   | 25 +
 virt-subproc/adt-virt-qemu.1 |  5 +
 3 files changed, 34 insertions(+)

diff --git a/debian/changelog b/debian/changelog
index 36ee620..85a5571 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,9 @@ 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   Tue, 23 Feb 2016 18:21:51 +0100
 
 autopkgtest (3.19.3) unstable; urgency=medium
diff --git a/virt-subproc/adt-virt-qemu b/virt-subproc/adt-virt-qemu
index 965e3e8..d676708 100755
--- a/virt-subproc/adt-virt-qemu
+++ b/virt-subproc/adt-virt-qemu
@@ -175,6 +175,30 @@ def login_tty_and_setup_shell():
 term.send(b'\nexit\n')
 VirtSubproc.expect(term, b'\nlogout', 10)
 
+def setup_baseimage():
+'''setup /dev/baseimage in VM'''
+
+term = VirtSubproc.get_unix_socket(os.path.join(workdir, 'ttyS1'))
+
+# Setup udev rules for /dev/baseimage; set link_priority to -1024 so
+# that the duplicate UUIDs of the partitions will have no effect.
+term.send(b'''mkdir -p -m 0755 /etc/udev/rules.d ; printf '# Created by adt-virt-qemu\\n%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"' > /etc/udev/rules.d/61-baseimage.rules\n''')
+VirtSubproc.expect(term, b'#', 10)
+# Reload udev to make sure the rules take effect (udev only auto-
+# rereads rules every 3 seconds)
+term.send(b'udevadm control -R\n')
+# Update the initramfs to include the new udev rules (to support
+# reboots properly)
+term.send(b'update-initramfs -k all -u\n')
+VirtSubproc.expect(term, b'#', 60)
+
+monitor = VirtSubproc.get_unix_socket(os.path.join(workdir, 'monitor'))
+
+# Add the base image as an additional drive
+monitor.send(('drive_add 0 file=%s,if=none,readonly=on,serial=BASEIMAGE,id=drive-baseimage\n' % args.image[0]).encode())
+VirtSubproc.expect(monitor, b'(qemu)', 10)
+monitor.send(b'device_add virtio-blk-pci,drive=drive-baseimage,id=virtio-baseimage\n')
+VirtSubproc.expect(monitor, b'(qemu)', 10)
 
 def setup_shared(shared_dir):
 '''Set up shared dir'''
@@ -501,6 +525,7 @@ def hook_open():
 # files; let QEMU run with the deleted inode
 os.unlink(overlay)
 

Bug#800845: autopkgtest: Add support for nested VMs

[Christian Seiler]

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 

Bug#800845: autopkgtest: Add support for nested VMs

[Martin Pitt]

Hey Christian,

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!

> 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.

>   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?

>   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.

> 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.

> 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.

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 do like the "only specify -cpu if we use the default qemu" part
though.

>   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.

> 0003-Add-needs-baseimage-restriction.patch

I already said that I feel that this is quite a stretch of what
Restrictions: should do -- they should apply to any kind of testbed,
and this concept doesn't generalize at all to schroot, lxc, ssh, etc.

On second thought maybe a more generic name would be
"needs-nested-virt"? qemu, LXC/LXD, and schroot can be nested in
principle (although we don't yet implement it for the latter ones),
while ssh and null would never provide this capability. LXC/schroots
wouldn't serve images, but you could e. g. provide a read-only bind
mount inside the container/schroot with the root fs.

But -- on third thought :-) -- Even with that tests will still be
specific to which adt-virt-* you use, as you have to do completely
different steps (and even have completely different test depends!) for
doing the nesting. So in the end, your test will always have a
/dev/baseimage with -qemu, and it can't work anyway with any other
runner. So for example, the tests will never run on Debian's and
Ubuntu's production CI.

>   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

Bug#800845: autopkgtest: Add support for nested VMs

[Christian Seiler]

Hi Martin,

So I've spent some time again on this and have separated this into
three patches:

0001-adt-virt-qemu-Implement-support-for-nested-base-imag.patch

  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.

  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.

  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, hence my patch doesn't set the base
  image in that case, though you may specify one explicitly via the
  command line.

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

  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.

0003-Add-needs-baseimage-restriction.patch

  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.

  (Of course, without a base image, one could do vmdebootstrap within
  the outer VM, but that would be a huge waste of resources, without
  a base image the only sensible thing to do is to skip the tests. And
  I'd really prefer .)

All patches also update the documentation. Note that since autopkgtest
specification is now part of Debian policy, if you agree to my third
patch, an update to the policy package would also be required, which
should probably only follow after this has been in use for a while, so
that we could still change it if we determine some problems. I'd be
willing to take care of that part.

I've tested this with Debian sid and Ubuntu Xenial images (created via
vmdebootstrap and adt-buildvm-ubuntu-cloud, as per the manpage) and it
works on both.

CAVEAT though:

  The first patch that adds the base image as an additional read-only
  drive has a slight problem if old images are used: since UUIDs are
  the same for filesystems on the main virtio drive and the base image
  "drive" (because udev processing of the base image "drive" is not
  disabled), partitions on both drives will have the same UUID, and
  apparently (at least on sid) udev selects the second drive for the
  symlinks, so /dev/disk/by-uuid/* will point to /dev/vdb*.

  This is not a problem for the default images (even those that were
  created from older versions), because they only contain one partition
  and the initiramfs is not affected, but if someone has multiple
  partitions in their test image, this will cause problems.

  I don't think this is a huge deal, beacuse you have to regenerate VM
  base images for ADT regularly anyway, but this is something to keep
  in mind (and maybe requires a NEWS entry?).

  I don't have a good solution for that other than to disable the base
  image logic unless explicitly enabled - but I'd really like to see
  this on by default.

I'd really like to see this moving forward.

Thoughts?

Regards,
Christian
From 5ee53e4ce023521b43a67708a9c533a9c3a5b02a Mon Sep 17 00:00:00 2001
From: Christian Seiler 
Date: Fri, 26 Feb 2016 16:14:29 +0100
Subject: [PATCH 1/3] 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.

By default, a copy of the main image is passed to the VM, but it may
be overwritten by the --nested-baseimage command line parameter. Note
that this parameter becomes mandatory for supporting this if more than
one image is speicifed on the regular command line.

setup-testbed is modified to modify 60-persistent-storage.rules: the
base image that is passed into the system should not be scanned for
file system UUIDs etc., because they are likely to be duplicates of the
UUIDs of the image that was booted from.

Closes: #800845
---
 debian/changelog |  5 +
 setup-commands/setup-testbed | 16 
 

Bug#800845: autopkgtest: Add support for nested VMs

[Martin Pitt]

Hey Christian,

thanks for working on this! Indeed I once had a patch to adt-virt-qemu
which would make its own image available as /dev/qemu_image or
something such, but I can't find it now for the life of me.

Christian Seiler [2015-10-04 13:51 +0200]:
> as per our discussion on the autopkgtest mailing list [1], I'd like to
> be able to have autopkgtest support nested VMs for testing of network
> clients in the kernel such as NFS, CIFS, iSCSI, NBD, etc.

Back then I actually had that mostly working, except that the nested
qemu was really unstable. But in later releases QEMU got much beteter,
and your trick to set the emulated CPU to the host CPU might just be
the remaining brick :-)

>  - have a copy of the qcow2 base image in the test environment

OOI, is this merely an implementation detail, or do you really need
qcow2? If we'd export this as a block device (symlink /dev/qemu_image
-> /dev/xdb or whatever it will actually end up like), you avoid
introducing an assumption about the "outside" image format. This block
device would be readonly, so that you can only use it in overlay mode
for the nested QEMU.

> I've attached two patches that implement the necessary changes to
> autopkgtest.

I'll comment on the actual patches, that's easier.

> Note that nested KVM will also require a module option to be set.
> (nested=1 for either the kvm_intel or kvm_amd module.) Setting
> -cpu host has no negative side effects even if nested=0 is set on
> the host - then the kvm_* modules will not load in the VM and KVM
> will simply be not available - same as without -cpu host.

Where does this need to be set, on the host, or in the outer QEMU
instance? The patch doesn't cover this bit, and it should at least be
documented.

> --- a/doc/README.package-tests.rst
> +++ b/doc/README.package-tests.rst
> @@ -202,6 +202,26 @@ needs-recommends
>  Enable installation of recommended packages in apt for the test
>  dependencies. This does not affect build dependencies.
>  
> +needs-qcow2-baseimage
> +The test needs to have a read-only qcow2 base image available so it
> +may create an overlay and start a qemu/KVM virtual machine inside
> +the test environment.

This is a way too specific restriction that I want to make this
official and unchangeable API for eternity. If you want to do the same
with LXC or nspawn etc., we'd have to come up with similarly
complicated and specific names.

Something like "needs-nesting" would be more general, but it's still
not sufficient to tell the testbed everything it needs to know, in
particular where the base image is. With QEMU and LXC exporting a
block device to the testbed should work. But unless you are
super-careful, tests which make use of that will be written for one
specific adt runner (i. e. -qemu).

Can we start small with not declaring a particular restriction (and
corresponding test bed capabilitiy), and instead the test just skips
itself if it doesn't find $ADT_QCOW2_BASEIMAGE, or /dev/qemu_image,
etc.?

> --- a/virt-subproc/adt-virt-qemu
> +++ b/virt-subproc/adt-virt-qemu
> @@ -80,6 +80,8 @@ def parse_args():
>  help='Enable debugging output')
>  parser.add_argument('--qemu-options',
>  help='Pass through arguments to QEMU command.')
> +parser.add_argument('--nested-qcow2-baseimage',
> +help='qcow2 VM base image for use inside the VM 
> (nested VMs)')

I don't think we need this. This should be really cheap to set up, so
just always make it available.

>  def prepare_overlay():
> @@ -274,6 +278,10 @@ EOF
>  def make_auxverb(shared_dir):
>  '''Create auxverb script'''
>  
> +envvars = ''
> +if args.nested_qcow2_baseimage != None:
> +envvars += 'export ADT_QCOW2_BASEIMAGE=/dev/vd%c ; ' % chr(ord('a') 
> + len(args.image))

As I said above, could this instead be added as an extra readonly
-drive, and you add an extra command to provide a symlink
/dev/qemu_image to it? Then the test doesn't have to make an
assumption about the image format any more, and this can be changed in
the future.

> @@ -475,6 +485,13 @@ 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))
> +if args.nested_qcow2_baseimage != None:
> +# export base image as drive (it's the easiest way to get
> +# it into QEMU), but make sure format=raw is set, because
> +# we want the VM to be able to see it as a qcow2 image and
> +# not have QEMU interpret it
> +argv.append('-drive')
> +argv.append('file=%s,if=virtio,index=%i,readonly,format=raw' % 
> (args.nested_qcow2_baseimage, len(args.image)))

Right, like that, but as a real drive, not a block device which isn't
actually one. :-)

>  def hook_capabilities():
> -global normal_user
> +global normal_user, args
>  caps = ['revert', 

Bug#800845: autopkgtest: Add support for nested VMs

[Christian Seiler]

Hi Martin,

Am 2015-10-05 16:20, schrieb Martin Pitt:

Christian Seiler [2015-10-04 13:51 +0200]:
as per our discussion on the autopkgtest mailing list [1], I'd like 
to
be able to have autopkgtest support nested VMs for testing of 
network

clients in the kernel such as NFS, CIFS, iSCSI, NBD, etc.


Back then I actually had that mostly working, except that the nested
qemu was really unstable. But in later releases QEMU got much 
beteter,

and your trick to set the emulated CPU to the host CPU might just be
the remaining brick :-)


It's not really necessary to make QEMU work. If you don't set the CPU
type properly, QEMU will emulate a generic CPU without the instruction
set that's required for KVM, so the kernel inside the VM will simply
think the CPU doesn't support KVM and /dev/kvm won't be available. Then
QEMU will simply fall back to non-KVM usage, which is slower, but still
works.

Note that QEMU-inside-KVM is bad albeit still acceptable, while
QEMU-inside-QEMU is horrendous when it comes to performance.


 - have a copy of the qcow2 base image in the test environment


OOI, is this merely an implementation detail, or do you really need
qcow2? If we'd export this as a block device (symlink /dev/qemu_image
-> /dev/xdb or whatever it will actually end up like), you avoid
introducing an assumption about the "outside" image format. This 
block

device would be readonly, so that you can only use it in overlay mode
for the nested QEMU.


Ok, this is completely my bad. I thought that the base image for a
qcow2 overlay had to be a qcow2 image itself - now that I've checked
that's obviously not the case. Sorry about that.

So yes, I agree that we should just let QEMU interpret the base image
and export the drive directly.

Note that nested KVM will also require a module option to be 
set.

(nested=1 for either the kvm_intel or kvm_amd module.) Setting
-cpu host has no negative side effects even if nested=0 is set 
on
the host - then the kvm_* modules will not load in the VM and 
KVM

will simply be not available - same as without -cpu host.


Where does this need to be set, on the host, or in the outer QEMU
instance? The patch doesn't cover this bit, and it should at least be
documented.


On the bare metal host. Just create e.g.
/etc/modprobe.d/nested_kvm.conf
with the following contents:
options kvm_intel nested=1
options kvm_amd   nested=1

Don't know about s390 virtualization (the other supported KVM platform)
and don't know about ARM (where people were talking about supporting
KVM eventually, but isn't completed yet as far as I know).


--- a/doc/README.package-tests.rst
+++ b/doc/README.package-tests.rst
@@ -202,6 +202,26 @@ needs-recommends
 Enable installation of recommended packages in apt for the test
 dependencies. This does not affect build dependencies.

+needs-qcow2-baseimage
+The test needs to have a read-only qcow2 base image available 
so it
+may create an overlay and start a qemu/KVM virtual machine 
inside

+the test environment.


This is a way too specific restriction that I want to make this
official and unchangeable API for eternity. If you want to do the 
same

with LXC or nspawn etc., we'd have to come up with similarly
complicated and specific names.

Something like "needs-nesting" would be more general, but it's still
not sufficient to tell the testbed everything it needs to know, in
particular where the base image is. With QEMU and LXC exporting a
block device to the testbed should work. But unless you are
super-careful, tests which make use of that will be written for one
specific adt runner (i. e. -qemu).

Can we start small with not declaring a particular restriction (and
corresponding test bed capabilitiy), and instead the test just skips
itself if it doesn't find $ADT_QCOW2_BASEIMAGE, or /dev/qemu_image,
etc.?


Ok, so skip the QCOW2, see above. But having a restriction is a good
thing here, because unless I'm completely mistaken, autopkgtest will
only consider a test skipped if a restriction can't be fulfilled. This
means that if I just do "yay, exit 0" in the outer KVM if the base
image is not present, then the test will be marked as succeeded and we
won't actually get the information that the test was in fact skipped.
(If I'm wrong about that sorry, then please correct me.)

But what about "needs-qcow2-baseimage" -> "needs-baseimage" (+ not
qcow2 but raw format, see above). This guarantees that either a file or
block device is present that contains a disk image (including partition
table) that contains a base image for VMs and/or containers. With
systemd-nspwan you can actually boot disk images as containers, so I
think this is sufficiently generic.

Btw. technically you wouldn't even need to skip the test if the image
is not there, you could just install vmdebootstrap and creeate a new
image inside the VM - the only problem is that that will take forever.


--- a/virt-subproc/adt-virt-qemu
+++ b/virt-subproc/adt-virt-qemu
@@ -80,6 

Bug#800845: autopkgtest: Add support for nested VMs

[Christian Seiler]

Package: autopkgtest
Version: 3.17.2
Severity: wishlist
Tags: patch

Hello Martin,

as per our discussion on the autopkgtest mailing list [1], I'd like to
be able to have autopkgtest support nested VMs for testing of network
clients in the kernel such as NFS, CIFS, iSCSI, NBD, etc.

The logic would be to

 - have the test environment run a simple server that the client may
   connect to
 - have a copy of the qcow2 base image in the test environment
 - create a QEMU/KVM VM in the test environment that then tests the
   client (this can again be done with adt-run inside the test env)

I've attached two patches that implement the necessary changes to
autopkgtest. They are quire minimal in fact:

 1. Have adt-virt-qemu add an additional drive to the VM that
maps (read-only) to the qcow2 base image (but without QEMU
interpreting the image, just passing it through via format=raw).

This allows the drive to be used as a base image, e.g.
qemu-img create -f qcow2 -b /dev/vdb overlay.img
(Running KVM on top of something like that works, btw., in case
you were wondering - you can easily try that by creating a loop
device, i.e. losetup --show -f base.img and then doing
adt-run ... --- adt-virt-qemu /dev/loopX.)

 2. Have adt-virt-qemu provide an environment variable to the tests
so that they may make use of it. (ADT_QCOW2_BASEIMAGE)

 3. Have adt-virt-qemu export the capability provides-qcow2-baseimage.

 4. Add new restriction needs-qcow2-baseimage that is checked against
that capability. (The base image logic is done unconditionally by
adt-virt-qemu, regardless of the restriction. In principle this
would allow people to just do vmdebootstrap inside the VM if the
base image were not to be exported.)

 5. If in KVM mode, add -cpu host option to the emulator command, since
that is required (but not sufficient) for nested KVM to work.

Note that nested KVM will also require a module option to be set.
(nested=1 for either the kvm_intel or kvm_amd module.) Setting
-cpu host has no negative side effects even if nested=0 is set on
the host - then the kvm_* modules will not load in the VM and KVM
will simply be not available - same as without -cpu host.

On the other hand, nested KVM is nice-to-have, but not required for
a nested VM - the VM inside could be a non-accelerated QEMU VM.

Once could in principle also add support for nested KVM to adt-virt-lxc
by simply allowing the administrator to specify an additional qcow2
base image on the command line. That could be done in an additional
step.

I've updated the documentation to reflect the base image changes.

It would be great if that could be added to autopkgtest.

Thanks!

Regards,
Christian
From d091f958e1a61cbf4d3296c4267d011ba3c6dbd4 Mon Sep 17 00:00:00 2001
From: Christian Seiler 
Date: Sun, 4 Oct 2015 12:25:39 +0200
Subject: [PATCH 1/2] Add 'needs-qcow2-baseimage' restriction

Add restrictions that allows tests to require a qcow2 base image be
present inside the test environment, so that e.g. nested VM tests are
possible.
---
 doc/README.package-tests.rst | 20 
 lib/testdesc.py  |  9 -
 virt-subproc/adt-virt-qemu   | 25 ++---
 virt-subproc/adt-virt-qemu.1 | 10 ++
 4 files changed, 60 insertions(+), 4 deletions(-)

diff --git a/doc/README.package-tests.rst b/doc/README.package-tests.rst
index e1127cb..5872617 100644
--- a/doc/README.package-tests.rst
+++ b/doc/README.package-tests.rst
@@ -202,6 +202,26 @@ needs-recommends
 Enable installation of recommended packages in apt for the test
 dependencies. This does not affect build dependencies.
 
+needs-qcow2-baseimage
+The test needs to have a read-only qcow2 base image available so it
+may create an overlay and start a qemu/KVM virtual machine inside
+the test environment.
+
+This is useful for testing network client packages that require
+kernel support (NFS, CIFS, iSCSI, NBD, etc.): the external testing
+environment sets up a minimalistic server environment and then
+starts a virtual machine that tests the client.
+
+While currently only adt-virt-qemu supports this, this options is
+independent of the isolation level. If the setup of the server also
+needs a specific isolation level, that should be specified
+additionally.
+
+The environment variable ADT_QCOW2_BASEIMAGE will be set to the
+absolute path of the qcow2 base image. If the test environment
+supports it, this variable will be available irrespective of
+whether this restriction was added to the test or not.
+
 Defined features
 
 
diff --git a/lib/testdesc.py b/lib/testdesc.py
index 260b2fa..33fdb7f 100644
--- a/lib/testdesc.py
+++ b/lib/testdesc.py
@@ -42,7 +42,8 @@ import adtlog
 
 known_restrictions = ['rw-build-tree', 'breaks-testbed', 'needs-root',
   'build-needed', 

Bug#800845: autopkgtest: Add support for nested VMs

[Christian Seiler]

Hi,

just an additional note:

with these changes, I've successfully managed to write autopkgtests for
open-iscsi that do the following (not pushed to git yet because I need
to know whether you'll accept this patch):

debian/tests/control:
Tests: simple
Restrictions: needs-root, breaks-testbed, isolation-machine, 
needs-qcow2-baseimage, allow-stderr
Depends: targetcli, qemu-system, qemu-utils, autopkgtest, python3, 
python3-netifaces

debian/tests/simple:
  - sets up iSCSI target in test env
  - runs adt-run with adt-virt-qemu with the ADT_QCOW2_BASEIMAGE (in
this case /dev/vdb) to run the tests for open-iscsi in a nested
VM (using --override-control debian/tests/nested/control);
also sets --env ISCSI_TARGET_IP=... etc. for the nested test

debian/tests/nested/control:
Tests: install, login, sysvinit-login
Tests-Directory: debian/tests/nested
Restrictions: needs-root, isolation-machine, breaks-testbed
Depends: open-iscsi

debian/tests/nested/$testname:
  - tests the iSCSI initiator (client) by connecting to
ISCSI_TARGET_IP etc. and seeing if the kernel makes the right
block devices available

So the patch I've posted here for autopkgtest is not just theoretical,
it actually works.

Regards,
Christian



signature.asc
Description: OpenPGP digital signature