Merge ~paelzer/qa-regression-testing:fix-libvirt-artful-locking into qa-regression-testing:master

[Christian Ehrhardt ]

Hi,
qemu 2.10 does the right thing to prevent data and metadata clobbering.
It locks image files, and without extra opt-in via special flags does not allow to open image files multiple times.

Some tests we have are currently affected as they attach a disk device.
Then skip to detach it but then attach it again.

Further the disk file gets re-written to be attached again.
All that is no more valid - but OTOH the reasons to skip the detach are gone as well.

So we can properly test the detach again and thereby automatically fix the attach without Failures.

[Christian Ehrhardt  (paelzer)]

Without these fixes the libvirt tests on Artful run into the following issues:

FAIL: test_guest_aa_attach_detach_physical (main.LibvirtTestVirshSystemRoot)

Test attach/detach physical
Traceback (most recent call last):
File "./test-libvirt.py", line 2304, in test_guest_aa_attach_detach_physical
self._virsh_cmd_and_check("attach-disk %s %s sdc --driver=%s" % (self.vm_name, device_disk, driver), check_aa_files=True, tail=True, fn=device
_disk)
File "./test-libvirt.py", line 1831, in _virsh_cmd_and_check
self._run_virsh_cmd(self.qemu_uri, args, None)
File "./test-libvirt.py", line 1740, in _run_virsh_cmd
self.assertEquals(expected, rc, result + str(report))
AssertionError: Got exit code 1, expected 0
error: Failed to attach disk
error: internal error: unable to execute QEMU command 'device_add': Failed to get "write" lock

======================================================================
FAIL: test_guest_aa_attach_detach_physical (main.LibvirtTestVirshSystemRootAppArmor)

Test attach/detach physical
Traceback (most recent call last):
File "./test-libvirt.py", line 2291, in test_guest_aa_attach_detach_physical
self._virsh_cmd_and_check("attach-device %s %s" % (self.vm_name, device_xml), check_aa_files=True, tail=True, fn=device_disk)
File "./test-libvirt.py", line 1831, in _virsh_cmd_and_check
self._run_virsh_cmd(self.qemu_uri, args, None)
File "./test-libvirt.py", line 1740, in _run_virsh_cmd
self.assertEquals(expected, rc, result + str(report))
AssertionError: Got exit code 1, expected 0
error: Failed to attach device from /tmp/tmpipVhxT/device.xml
error: internal error: unable to execute QEMU command 'device_add': Property 'scsi-hd.drive' can't find value 'drive-scsi0-0-0-1'

======================================================================
FAIL: test_guest_cb_attach_detach_virtio (main.LibvirtTestVirshSystemRootAppArmor)

Test attach/detach virtio
Traceback (most recent call last):
File "./test-libvirt.py", line 2346, in test_guest_cb_attach_detach_virtio
self._virsh_cmd_and_check("attach-device %s %s" % (self.vm_name, device_xml), check_aa_files=True, tail=True, fn=device_disk)
File "./test-libvirt.py", line 1831, in _virsh_cmd_and_check
self._run_virsh_cmd(self.qemu_uri, args, None)
File "./test-libvirt.py", line 1740, in _run_virsh_cmd
self.assertEquals(expected, rc, result + str(report))
AssertionError: Got exit code 1, expected 0
error: Failed to attach device from /tmp/tmpIdjdPR/device.xml
error: internal error: unable to execute QEMU command 'device_add': Property 'virtio-blk-device.drive' can't find value 'drive-virtio-disk1'

======================================================================
FAIL: test_guest_aa_attach_detach_physical (main.LibvirtTestVirshSystemNonRootAppArmor)

Test attach/detach physical
Traceback (most recent call last):
File "./test-libvirt.py", line 2291, in test_guest_aa_attach_detach_physical
self._virsh_cmd_and_check("attach-device %s %s" % (self.vm_name, device_xml), check_aa_files=True, tail=True, fn=device_disk)
File "./test-libvirt.py", line 1831, in _virsh_cmd_and_check
self._run_virsh_cmd(self.qemu_uri, args, None)
File "./test-libvirt.py", line 1740, in _run_virsh_cmd
self.assertEquals(...

[Christian Ehrhardt  (paelzer)]

Full run on Artful now completed, no side effects found.
But there seem to be more issues when running the full test, will take a look and merge here as needed.

Oh I see, it now works for the class derived from LibvirtTestVirshSystemRoot but on the apparmor cases like LibvirtTestVirshSystemRootAppArmor the issue you faced in the past with the detach comes up.

Marking this WIP for now until the full set runs.

[Christian Ehrhardt  (paelzer)]

error: internal error: unable to execute QEMU command 'device_add': Property 'scsi-hd.drive' can't find value 'drive-scsi0-0-0-1'

Actually is:
su -c sudo -H -u tkfvcmMz virsh -c qemu:///system attach-device qatest-i386 /tmp/tmp6CrF_y/device.xml

Causing:
apparmor="DENIED" operation="file_lock" profile="libvirt-7d781722-69b7-8801-fe96-caf37b7a8969" name="/tmp/tmp6CrF_y/device_disk.img" pid=1673 comm="qemu" requested_mask="k" denied_mask="k" fsuid=0 ouid=0

We add k in libvirt-aa-helper for image files.
I can see that even in the profile used this is true:
  "/home/ubuntu/qa-regression-testing/scripts/libvirt/qatest/qatest.img" rwk,

Next: libvirt might be missing to add the k perm on the use-case that the test is using to attach the disk?

[Christian Ehrhardt  (paelzer)]

As usual these rules are generated on the hot-add.
So we can't check the rules before.
Instead we need to "intercept" what gets added in the short time the disk gets added.

This shows on:
su -c sudo -H -u tBmzjHwq virsh -c qemu:///system attach-device qatest-i386 /tmp/tmpFLjlVS/device.xml

The rule rendered is:
"/tmp/tmpFLjlVS/device_disk.img" rw,

This is really missing the k flag.
The XML defining the hot-add is:
<disk type='block'>
  <driver name='qemu'/>
  <source dev='/tmp/tmpAoRaMw/device_disk.img'/>
  <target dev='sdb'/>
</disk>

Well this is odd, qemu locks on files.
But this is a file, yet defined as type='block'

So it should actually be like:
<disk type='file'>
  <driver name='qemu'/>
  <source file='/tmp/tmpAoRaMw/device_disk.img'/>
  <target dev='sdb'/>
</disk>

But even with that it is failing, maybe that is a real bug in the addition of the lock perm to aa-helper not considering the attache-device files the same way.
Analysis ongoing ...

[Christian Ehrhardt  (paelzer)]

When I add the same snippet to the main guest definition it gets the rule:
  "/tmp/tmpAoRaMw/device_disk.img" rwk,
Which would be correct, but as we see in the test above it is not generated the same way on attach-device.
The same is true for the type=block device.

That said it is now an issue isolated to libvirt and not this MP/the-tests.
I'll fork a libvirt bug and track it there.
Coming back to the MP once that is unblocked by a fix to libvirt.

[Christian Ehrhardt  (paelzer)]

Since I needed it for the debugging I had some debug helpers that I'll include.
It seems I need that more often than not so it might be wise to add this for everyone.

OTOH with a ppa for bug 1726804 and this fix I'm now able to run all tests correctly again.
Too bad the locking issues I fix int his MP had hidden these real issues for me earlier in this cycle.

Changes pushed - branch now has the fixes plus the enhanced debugging, ready for review again - setting status.

[Jamie Strandboge (jdstrand)]

These debugging options look really nice and I can imagine how they have come in handy. This and the fixes for 17.10 LGTM.

review: Approve

diff --git a/scripts/test-libvirt.py b/scripts/test-libvirt.py
index 58d80ce..f82590d 100755
--- a/scripts/test-libvirt.py
+++ b/scripts/test-libvirt.py
@@ -63,6 +63,14 @@
       If you interrupt the test halfway, you might need to clean up and reset
       the test environment. One way to do this is to run "./test-libvirt reset"
 
+      For extra debugging (way above verbose) you can add environment variables
+      like:
+      $ DEBUG_AA=True sudo ./test-libvirt.py -v
+      The list of such debug variables is:
+      - DEBUG_AA: report apparmor rules
+      - DEBUG_XML: xml snippets that are used
+      - DEBUG_VIRSH: virsh commands that are issues
+
     TODO for migration away from libvirt-aa-secdriver.sh:
      - USB tests (eg analog for libvirt-apparmor.sh foo usb:2,3)
      - remote tests (eg analog for libvirt-apparmor.sh foo remote:192.168.122.203
@@ -92,6 +100,10 @@ copy_image = False
 qemu_user = None
 qemu_savetmpdir = None
 
+DEBUG_VIRSH = os.environ.get('DEBUG_VIRSH', "False")
+DEBUG_AA = os.environ.get('DEBUG_AA', "False")
+DEBUG_XML = os.environ.get('DEBUG_XML', "False")
+
 def checksecmodel(txt):
     dom = xml.dom.minidom.parseString(txt)
     for n in dom.getElementsByTagName('secmodel'):
@@ -1726,13 +1738,12 @@ class LibvirtTestVirshCommon(LibvirtTestCommonWithAbort):
 
     def _run_virsh_cmd(self, connect_uri, args, search, invert=False, user=None, expected=0):
         '''Run virsh command and search for string'''
-        debug = False
 
         if not user:
             global qemu_user
             user = qemu_user
 
-        if debug:
+        if DEBUG_VIRSH != "False":
             print "%s" % (" ".join(['su', '-c', 'sudo -H -u %s virsh -c %s %s' % (user.login, connect_uri, args)]))
 
         rc, report = testlib.cmd(['su', '-c', 'sudo -H -u %s virsh -c %s %s' % (user.login, connect_uri, args)])
@@ -1752,7 +1763,7 @@ class LibvirtTestVirshCommon(LibvirtTestCommonWithAbort):
                 result = "Could not find '%s'\n" % search
                 self.assertTrue(re.search(r'%s' % search, report) != None, result + str(report))
 
-        if debug:
+        if DEBUG_VIRSH != "False":
             print report,
 
         return report
@@ -1865,6 +1876,9 @@ class LibvirtTestVirshCommon(LibvirtTestCommonWithAbort):
         global qemu_user
         self._adjust_perms("%s:%s" % (self.kvm_user, self.kvm_group), self.qemu_tmpdir)
 
+        if DEBUG_XML != "False":
+            with open(self.qemu_xml, 'r') as xml:
+                print xml.read()
         rc, report = testlib.cmd(['su', '-c', 'sudo -H -u %s virsh -c %s define %s' % (qemu_user.login, self.qemu_uri, self.qemu_xml)])
         expected = 0
         result = 'Got exit code %d, expected %d\n' % (rc, expected)
@@ -1891,6 +1905,13 @@ class LibvirtTestVirshCommon(LibvirtTestCommonWithAbort):
         if self.lsb_release['Release'] < 10.04:
             self.vm_uuid = report.splitlines()[1].strip()
 
+        if DEBUG_AA != "False":
+            aa_file = "/etc/apparmor.d/libvirt/libvirt-%s.files" % (self.vm_uuid)
+            if os.path.exists(aa_file):
+                print "Apparmor File: %s" % aa_file
+                with open(aa_file, 'r') as profile:
+                    print profile.read()
+
     def test_aa_initialize(self):
         '''Test initialization'''
         global abort_tests
@@ -1932,6 +1953,9 @@ class LibvirtTestVirshCommon(LibvirtTestCommonWithAbort):
             # Need this for skip_apparmor changes
             self._restart_daemon()
 
+        if DEBUG_XML != "False":
+            with open(self.qemu_xml, 'r') as xml:
+                print xml.read()
         rc, report = testlib.cmd(['su', '-c', 'sudo -H -u %s virsh -c %s define %s' % (qemu_user.login, self.qemu_uri, self.qemu_xml)])
         expected = 0
         result = 'Got exit code %d, expected %d\n' % (rc, expected)
@@ -2287,12 +2311,16 @@ class LibvirtTestVirshGuest(LibvirtTestVirshCommon):
             print "  detach-interface"
             self._run_virsh_cmd(self.qemu_uri, "detach-interface %s network --mac %s" % (self.vm_name, new_mac), None)
 
+        if DEBUG_XML != "False":
+            with open(device_xml, 'r') as xml:
+                print xml.read()
         print "  attach-device (physical)"
         self._virsh_cmd_and_check("attach-device %s %s" % (self.vm_name, device_xml), check_aa_files=True, tail=True, fn=device_disk)
-        if self.lsb_release['Release'] >= 10.10:
+        if self.lsb_release['Release'] >= 10.10 and self.lsb_release['Release'] < 17.10:
             # cannot hot unplug physical block device with qemu in 0.7.7 and higher
             print "  detach-device (physical): (skipped on 10.10 and higher)"
         else:
+            # working again in later releases, >= qemu 2.10 (17.10) required as images are locked and can only be used once
             print "  detach-device (physical)"
             self._virsh_cmd_and_check("detach-device %s %s" % (self.vm_name, device_xml), check_aa_files=True, invert=True, fn=device_disk)
 
@@ -2302,10 +2330,11 @@ class LibvirtTestVirshGuest(LibvirtTestVirshCommon):
             # libvirt 0.9 requires this
             driver = 'qemu'
         self._virsh_cmd_and_check("attach-disk %s %s sdc --driver=%s" % (self.vm_name, device_disk, driver), check_aa_files=True, tail=True, fn=device_disk)
-        if self.lsb_release['Release'] >= 10.10:
+        if self.lsb_release['Release'] >= 10.10 and self.lsb_release['Release'] < 17.10:
             # cannot hot unplug scsi device with qemu in 0.7.7 and higher
             print "  detach-disk (physical): (skipped on 10.10 and higher)"
         else:
+            # working again in later releases, >= qemu 2.10 (17.10) required as images are locked and can only be used once
             print "  detach-disk (physical)"
             self._virsh_cmd_and_check("detach-disk %s sdc" % (self.vm_name), check_aa_files=True, invert=True, fn=device_disk)
 
@@ -2343,6 +2372,9 @@ class LibvirtTestVirshGuest(LibvirtTestVirshCommon):
         max_tries = 4
         for i in range(1, max_tries+1):
             print "  attach-device (virtio #%d)" % i
+            if DEBUG_XML != "False":
+               with open(device_xml, 'r') as xml:
+                   print xml.read()
             self._virsh_cmd_and_check("attach-device %s %s" % (self.vm_name, device_xml), check_aa_files=True, tail=True, fn=device_disk)
             print "  detach-device (virtio #%d)" % i
 
@@ -2394,6 +2426,9 @@ class LibvirtTestVirshGuest(LibvirtTestVirshCommon):
         self._define_and_start_guest()
 
         print "  attach-device (AoE)"
+        if DEBUG_XML != "False":
+           with open(device_xml, 'r') as xml:
+               print xml.read()
         self._virsh_cmd_and_check("attach-device %s %s" % (self.vm_name, device_xml), check_aa_files=True, tail=True, fn=device_aoe)
         print "  detach-device (AoE)"
         # if fail here, might have hit https://launchpad.net/bugs/455832