Merge lp:~smoser/curtin/trunk.lp1716028-hack-file-locking-in-qemu into lp:~curtin-dev/curtin/trunk

[Scott Moser]

vmtest: Support newer qemu and multipath.

If you pass the same backing device for a disk to qemu, newer
versions (2.10+) will fail. In order to allow this, it requires
you to pass 'file.locking=off'.

This change makes the multipath tests add a 'file.locking=off' argument
to the disks that are actually intended to be added more than once.
xkvm is modified to remove that flag if the qemu being used does not
support it.

In order to accomplish this:
a.) make launch use xkvm '--disk' for all its disks rather than
    passing -drive and -device. This is good in that we know have
    a single way of passing disks around.
b.) fix a logic bug in xkvm when --disk= specified format= on it.

vmtest and launch now only use '--disk=' args, which are interpreted
by xkvm rather than using a mix of '--disk=' and '-drive'.

[lp:~smoser/curtin/trunk.lp1716028-hack-file-locking-in-qemu]

527. By Scott Moser on 2017-09-08

cleanup

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:527
https://jenkins.ubuntu.com/server/job/curtin-ci/616/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/616
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/616
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/616
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/616

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/616/rebuild

review: Approve (continuous-integration)

[lp:~smoser/curtin/trunk.lp1716028-hack-file-locking-in-qemu]

528. By Scott Moser on 2017-09-08

do not override _QEMU_SUPPORTS_FILE_LOCKING in environment.

[Ryan Harper (raharper)]

+1 on the cls.multipath append of file.locking=off; that's cleaner than putting it in everywhere.

review: Approve

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:528
https://jenkins.ubuntu.com/server/job/curtin-ci/617/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/617
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/617
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/617
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/617

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/617/rebuild

review: Approve (continuous-integration)

[lp:~smoser/curtin/trunk.lp1716028-hack-file-locking-in-qemu]

529. By Scott Moser on 2017-09-12

use share-rw presense rather than looking for binary string search.

[Scott Moser (smoser)]

Note, i put this branch
 https://code.launchpad.net/~smoser/curtin/trunk.lp1716028-better-file-locking/
it has some code that i think is cleaner to turn --disk into -drive and -device
support, but I ended up shelving it because we couldn't use share-rw anyway
due to that not really working with -drive (see comments in bug).

[Ryan Harper (raharper)]

This looks good. I really like having vmtest pass the locking in and xkvm handling/setting if available.

review: Approve

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:529
https://jenkins.ubuntu.com/server/job/curtin-ci/619/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/619
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/619
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/619
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/619

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/619/rebuild

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

launch: unrecognized option '--disk,file.locking=off'
launch: unrecognized option '--disk,file.locking=off'

[Ryan Harper (raharper)]

http://paste.ubuntu.com/25522510/

This helps get it on the disk parameter as a devopt; but xkvm puts those on under -device parameters to qemu, however, file.locking is a -drive parameter.

review: Needs Fixing

[lp:~smoser/curtin/trunk.lp1716028-hack-file-locking-in-qemu]

530. By Scott Moser on 2017-10-19

xkvm: fix logic bug in xkvm if user provided format, it got set to raw

531. By Scott Moser on 2017-10-19

tools/launch: make 'launch' pass --disk to xkvm rather than -drive/-device

532. By Scott Moser on 2017-10-19

vmtests: make multipath disks use file.locking=off

533. By Scott Moser on 2017-10-19

merge with trunk

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:532
https://jenkins.ubuntu.com/server/job/curtin-ci/660/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/660
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/660
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/660
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/660

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/660/rebuild

review: Approve (continuous-integration)

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:533
https://jenkins.ubuntu.com/server/job/curtin-ci/661/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/661
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/661
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/661
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/661

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/661/rebuild

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

I really want to have xkvm just always disable file.locking (if a qemu has it enabled).

But I do understand that it's reasonable for the test to declare that
it doesn't want/need file locking.

[lp:~smoser/curtin/trunk.lp1716028-hack-file-locking-in-qemu]

534. By Scott Moser on 2017-10-19

make multipath versions for both install and boot.

535. By Scott Moser on 2017-10-19

fix one last '-drive' usage with equivalent --disk

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:534
https://jenkins.ubuntu.com/server/job/curtin-ci/662/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/662
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/662
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/662
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/662

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/662/rebuild

review: Approve (continuous-integration)

[lp:~smoser/curtin/trunk.lp1716028-hack-file-locking-in-qemu]

536. By Scott Moser on 2017-10-19

and the last -drive from tools/launch

[Scott Moser (smoser)]

I notice now that
 cls.mpath_args(cls.mpath_diskargs(target_disks + extra_disks + nvme_disks)
is not really the same as waht we did before, which was effectively:
cls.mpath_diskargs(target_disks) + cls.mpath_diskargs(extra_disks) + cls.mpath_diskargs(nvme_disks)

the difference is really just in order, effectively
new: [a,b,c] * 2 = [a, b, c, a, b, c]
where before we had
 [a] * 2 + b * 2 + c * 2 = [a, a, b, b, c, c]

it all should really come out the same just different order of -drive options/-device options.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:536
https://jenkins.ubuntu.com/server/job/curtin-ci/663/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/663
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/663
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/663
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/663

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/663/rebuild

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

> I notice now that
> cls.mpath_args(cls.mpath_diskargs(target_disks + extra_disks + nvme_disks)
> is not really the same as waht we did before, which was effectively:
> cls.mpath_diskargs(target_disks) + cls.mpath_diskargs(extra_disks) +
> cls.mpath_diskargs(nvme_disks)
>
> the difference is really just in order, effectively
> new: [a,b,c] * 2 = [a, b, c, a, b, c]
> where before we had
> [a] * 2 + b * 2 + c * 2 = [a, a, b, b, c, c]
>
> it all should really come out the same just different order of -drive
> options/-device options.

You're right that the order is different, but I think you've got them backwards:

For the multi-list-join path:

cls.mpath_disks([a, b] + [c, d] + [e, f])

We used to get:
[a, b, a, b, c, d, c, d, e, f, e, f]

and with this new code we'll get:

[a, a, b, b, c, c, d, d, e, e, f, f]

Since we iterate over the combined list and multiply out each element of the list times the multiplier.

I *think* we'll be fine, specifically in the multipath case, the devices are paired by their serial number/model not by their placement on the PCI bus.

[Ryan Harper (raharper)]

review: Approve

=== modified file 'tests/vmtests/__init__.py'
--- tests/vmtests/__init__.py	2017-10-11 13:55:42 +0000
+++ tests/vmtests/__init__.py	2017-10-19 20:20:00 +0000
@@ -699,9 +699,6 @@
             logger.info('Detected centos, adding default config %s',
                         centos_default)
 
-        if cls.multipath:
-            disks = disks * cls.multipath_num_paths
-
         # set reporting logger
         cls.reporting_log = os.path.join(cls.td.logs, 'webhooks-events.json')
         reporting_logger = CaptureReporting(cls.reporting_log)
@@ -727,7 +724,7 @@
                 }))
         configs.append(reporting_config)
 
-        cmd.extend(uefi_flags + netdevs + disks +
+        cmd.extend(uefi_flags + netdevs + cls.mpath_diskargs(disks) +
                    [ftypes['vmtest.root-image'], "--kernel=%s" %
                        ftypes['boot-kernel'], "--initrd=%s" %
                     ftypes['boot-initrd'], "--", "curtin", "-vv", "install"] +
@@ -825,11 +822,6 @@
         # unlike NVMe disks, we do not want to configure the iSCSI disks
         # via KVM, which would use qemu's iSCSI target layer.
 
-        if cls.multipath:
-            target_disks = target_disks * cls.multipath_num_paths
-            extra_disks = extra_disks * cls.multipath_num_paths
-            nvme_disks = nvme_disks * cls.multipath_num_paths
-
         # output disk is always virtio-blk, with serial of output_disk.img
         output_disk = '--disk={},driver={},format={},{},{}'.format(
             cls.td.output_disk, 'virtio-blk',
@@ -840,11 +832,9 @@
         # create xkvm cmd
         cmd = (["tools/xkvm", "-v", dowait] +
                uefi_flags + netdevs +
-               target_disks + extra_disks + nvme_disks +
-               ["--", "-drive",
-                "file=%s,if=virtio,media=cdrom" % cls.td.seed_disk,
-                "-smp",  cls.get_config_smp(),
-                "-m", "1024"])
+               cls.mpath_diskargs(target_disks + extra_disks + nvme_disks) +
+               ["--disk=file=%s,if=virtio,media=cdrom" % cls.td.seed_disk] +
+               ["--", "-smp",  cls.get_config_smp(), "-m", "1024"])
 
         if not cls.interactive:
             if cls.arch == 's390x':
@@ -1007,6 +997,15 @@
         # prepending ./ makes '/root/file' or 'root/file' work as expected.
         return os.path.normpath(os.path.join(cls.td.collect, "./" + path))
 
+    @classmethod
+    def mpath_diskargs(cls, disks):
+        """make multipath versions of --disk args in disks."""
+        if not cls.multipath:
+            return disks
+        opt = ",file.locking=off"
+        return ([d if d == "--disk" else d + opt for d in disks] *
+                cls.multipath_num_paths)
+
     # Misc functions that are useful for many tests
     def output_files_exist(self, files):
         logger.debug('checking files exist: %s', files)
 
=== modified file 'tools/launch'
--- tools/launch	2017-07-25 15:15:09 +0000
+++ tools/launch	2017-10-19 20:20:00 +0000
@@ -583,11 +583,10 @@
               bs_args="${bs_args},physical_block_size=$phybs"
               bs_args="${bs_args},min_io_size=$logbs"
 
-        disk_args=( "${disk_args[@]}" "-drive"
-            "file=${src},if=none,cache=unsafe,format=$fmt,id=drv${id},index=$id" )
-
-        disk_args=( "${disk_args[@]}" "-device"
-            "${driver},drive=drv${id},${bs_args}${devopts}" )
+        t="file=${src},if=none,cache=unsafe,format=$fmt,"
+        t="${t}id=drv${id},index=$id,"
+        t="${t}driver=${driver},${bs_args}${devopts}"
+        disk_args[${#disk_args[@]}]="--disk=$t"
 
     done
 
@@ -754,7 +753,7 @@
         seed="${TEMP_D}/seed.img"
         cloud-localds "$seed" "$udata" "$mdata" ||
             { error "failed cloud-localds"; return 1; }
-        seedargs=( "-drive" "file=${seed},if=virtio,media=cdrom" )
+        seedargs=( "--disk=file=${seed},if=virtio,media=cdrom" )
     fi
 
     local netargs
@@ -779,13 +778,14 @@
         fi
     fi
     # -monitor stdio
+    local bootdisk="--disk=file=$bootimg,id=boot,index=1,cache=unsafe"
     cmd=(
-        xkvm "${pt[@]}" "${netargs[@]}" --
+        xkvm "${pt[@]}" "${netargs[@]}" 
+        "$bootdisk"
+        "${disk_args[@]}"
+        --
         -smp ${smp}
         -m ${mem} ${serial_args} ${video}
-        -drive "file=$bootimg,if=none,cache=unsafe,format=qcow2,id=boot,index=0"
-        -device "virtio-blk,drive=boot"
-        "${disk_args[@]}"
         "${seedargs[@]}"
         )
 
 
=== modified file 'tools/xkvm'
--- tools/xkvm	2016-10-07 19:31:01 +0000
+++ tools/xkvm	2017-10-19 20:20:00 +0000
@@ -11,6 +11,8 @@
 # OVS_CLEANUP gets populated with bridge:devname pairs used with ovs
 OVS_CLEANUP=( )
 MAC_PREFIX="52:54:00:12:34"
+# allow this to be set externally.
+_QEMU_SUPPORTS_FILE_LOCKING="${_QEMU_SUPPORTS_FILE_LOCKING}"
 KVM="kvm"
 declare -A KVM_DEVOPTS
 
@@ -119,6 +121,21 @@
     return 1
 }
 
+qemu_supports_file_locking() {
+    # hackily check if qemu has file.locking in -drive params (LP: #1716028)
+    if [ -z "$_QEMU_SUPPORTS_FILE_LOCKING" ]; then
+        # The only way we could find to check presense of file.locking is
+        # qmp (query-qmp-schema).  Simply checking if the virtio-blk driver
+        # supports 'share-rw' is expected to be equivalent and simpler.
+        isdevopt virtio-blk share-rw &&
+            _QEMU_SUPPORTS_FILE_LOCKING=true ||
+            _QEMU_SUPPORTS_FILE_LOCKING=false
+        debug 1 "qemu supports file locking = ${_QEMU_SUPPORTS_FILE_LOCKING}"
+    fi
+    [ "$_QEMU_SUPPORTS_FILE_LOCKING" = "true" ]
+    return
+}
+
 padmac() {
     # return a full mac, given a subset.
     # assume whatever is input is the last portion to be
@@ -443,7 +460,7 @@
             out=$(LANG=C qemu-img info "$file") &&
                 fmt=$(echo "$out" | awk '$0 ~ /^file format:/ { print $3 }') ||
                 { error "failed to determine format of $file"; return 1; }
-        else
+        elif [ -z "$fmt" ]; then
             fmt=raw
         fi
         if [ -z "$driver" ]; then
@@ -470,6 +487,12 @@
                 id=*|if=*|driver=*|$file|file=*) continue;;
                 fmt=*|format=*) continue;;
                 serial=*|bus=*|unit=*|index=*) continue;;
+                file.locking=*)
+                    qemu_supports_file_locking || {
+                        debug 2 "qemu has no file locking." \
+                            "Dropping '$tok' from: $cur"
+                        continue
+                    };;
             esac
             isdevopt "$driver" "$tok" && devopts="${devopts},$tok" ||
                 diskopts="${diskopts},${tok}"