Merge lp:~smoser/curtin/trunk.lp1716028-hack-file-locking-in-qemu into lp:~curtin-dev/curtin/trunk
- trunk.lp1716028-hack-file-locking-in-qemu
- Merge into trunk
Status: | Merged | ||||
---|---|---|---|---|---|
Merged at revision: | 539 | ||||
Proposed branch: | lp:~smoser/curtin/trunk.lp1716028-hack-file-locking-in-qemu | ||||
Merge into: | lp:~curtin-dev/curtin/trunk | ||||
Diff against target: |
170 lines (+47/-25) 3 files modified
tests/vmtests/__init__.py (+13/-14) tools/launch (+10/-10) tools/xkvm (+24/-1) |
||||
To merge this branch: | bzr merge lp:~smoser/curtin/trunk.lp1716028-hack-file-locking-in-qemu | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Ryan Harper (community) | Approve | ||
Server Team CI bot | continuous-integration | Approve | |
Review via email: mp+330456@code.launchpad.net |
Commit message
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'.
Description of the change
- 527. By Scott Moser
-
cleanup
Server Team CI bot (server-team-bot) wrote : | # |
- 528. By Scott Moser
-
do not override _QEMU_SUPPORTS_
FILE_LOCKING in environment.
Ryan Harper (raharper) wrote : | # |
+1 on the cls.multipath append of file.locking=off; that's cleaner than putting it in everywhere.
Server Team CI bot (server-team-bot) wrote : | # |
PASSED: Continuous integration, rev:528
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
- 529. By Scott Moser
-
use share-rw presense rather than looking for binary string search.
Scott Moser (smoser) wrote : | # |
Note, i put this branch
https:/
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) wrote : | # |
This looks good. I really like having vmtest pass the locking in and xkvm handling/setting if available.
Server Team CI bot (server-team-bot) wrote : | # |
PASSED: Continuous integration, rev:529
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
Ryan Harper (raharper) wrote : | # |
launch: unrecognized option '--disk,
launch: unrecognized option '--disk,
Ryan Harper (raharper) wrote : | # |
http://
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.
- 530. By Scott Moser
-
xkvm: fix logic bug in xkvm if user provided format, it got set to raw
- 531. By Scott Moser
-
tools/launch: make 'launch' pass --disk to xkvm rather than -drive/-device
- 532. By Scott Moser
-
vmtests: make multipath disks use file.locking=off
- 533. By Scott Moser
-
merge with trunk
Server Team CI bot (server-team-bot) wrote : | # |
PASSED: Continuous integration, rev:532
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
Server Team CI bot (server-team-bot) wrote : | # |
PASSED: Continuous integration, rev:533
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
Ryan Harper (raharper) wrote : | # |
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.
Ryan Harper (raharper) : | # |
- 534. By Scott Moser
-
make multipath versions for both install and boot.
- 535. By Scott Moser
-
fix one last '-drive' usage with equivalent --disk
Server Team CI bot (server-team-bot) wrote : | # |
PASSED: Continuous integration, rev:534
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
- 536. By Scott Moser
-
and the last -drive from tools/launch
Scott Moser (smoser) wrote : | # |
I notice now that
cls.mpath_
is not really the same as waht we did before, which was effectively:
cls.mpath_
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) wrote : | # |
PASSED: Continuous integration, rev:536
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
Ryan Harper (raharper) wrote : | # |
> I notice now that
> cls.mpath_
> is not really the same as waht we did before, which was effectively:
> cls.mpath_
> cls.mpath_
>
> 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) : | # |
Scott Moser (smoser) : | # |
Preview Diff
1 | === modified file 'tests/vmtests/__init__.py' |
2 | --- tests/vmtests/__init__.py 2017-10-11 13:55:42 +0000 |
3 | +++ tests/vmtests/__init__.py 2017-10-19 20:20:00 +0000 |
4 | @@ -699,9 +699,6 @@ |
5 | logger.info('Detected centos, adding default config %s', |
6 | centos_default) |
7 | |
8 | - if cls.multipath: |
9 | - disks = disks * cls.multipath_num_paths |
10 | - |
11 | # set reporting logger |
12 | cls.reporting_log = os.path.join(cls.td.logs, 'webhooks-events.json') |
13 | reporting_logger = CaptureReporting(cls.reporting_log) |
14 | @@ -727,7 +724,7 @@ |
15 | })) |
16 | configs.append(reporting_config) |
17 | |
18 | - cmd.extend(uefi_flags + netdevs + disks + |
19 | + cmd.extend(uefi_flags + netdevs + cls.mpath_diskargs(disks) + |
20 | [ftypes['vmtest.root-image'], "--kernel=%s" % |
21 | ftypes['boot-kernel'], "--initrd=%s" % |
22 | ftypes['boot-initrd'], "--", "curtin", "-vv", "install"] + |
23 | @@ -825,11 +822,6 @@ |
24 | # unlike NVMe disks, we do not want to configure the iSCSI disks |
25 | # via KVM, which would use qemu's iSCSI target layer. |
26 | |
27 | - if cls.multipath: |
28 | - target_disks = target_disks * cls.multipath_num_paths |
29 | - extra_disks = extra_disks * cls.multipath_num_paths |
30 | - nvme_disks = nvme_disks * cls.multipath_num_paths |
31 | - |
32 | # output disk is always virtio-blk, with serial of output_disk.img |
33 | output_disk = '--disk={},driver={},format={},{},{}'.format( |
34 | cls.td.output_disk, 'virtio-blk', |
35 | @@ -840,11 +832,9 @@ |
36 | # create xkvm cmd |
37 | cmd = (["tools/xkvm", "-v", dowait] + |
38 | uefi_flags + netdevs + |
39 | - target_disks + extra_disks + nvme_disks + |
40 | - ["--", "-drive", |
41 | - "file=%s,if=virtio,media=cdrom" % cls.td.seed_disk, |
42 | - "-smp", cls.get_config_smp(), |
43 | - "-m", "1024"]) |
44 | + cls.mpath_diskargs(target_disks + extra_disks + nvme_disks) + |
45 | + ["--disk=file=%s,if=virtio,media=cdrom" % cls.td.seed_disk] + |
46 | + ["--", "-smp", cls.get_config_smp(), "-m", "1024"]) |
47 | |
48 | if not cls.interactive: |
49 | if cls.arch == 's390x': |
50 | @@ -1007,6 +997,15 @@ |
51 | # prepending ./ makes '/root/file' or 'root/file' work as expected. |
52 | return os.path.normpath(os.path.join(cls.td.collect, "./" + path)) |
53 | |
54 | + @classmethod |
55 | + def mpath_diskargs(cls, disks): |
56 | + """make multipath versions of --disk args in disks.""" |
57 | + if not cls.multipath: |
58 | + return disks |
59 | + opt = ",file.locking=off" |
60 | + return ([d if d == "--disk" else d + opt for d in disks] * |
61 | + cls.multipath_num_paths) |
62 | + |
63 | # Misc functions that are useful for many tests |
64 | def output_files_exist(self, files): |
65 | logger.debug('checking files exist: %s', files) |
66 | |
67 | === modified file 'tools/launch' |
68 | --- tools/launch 2017-07-25 15:15:09 +0000 |
69 | +++ tools/launch 2017-10-19 20:20:00 +0000 |
70 | @@ -583,11 +583,10 @@ |
71 | bs_args="${bs_args},physical_block_size=$phybs" |
72 | bs_args="${bs_args},min_io_size=$logbs" |
73 | |
74 | - disk_args=( "${disk_args[@]}" "-drive" |
75 | - "file=${src},if=none,cache=unsafe,format=$fmt,id=drv${id},index=$id" ) |
76 | - |
77 | - disk_args=( "${disk_args[@]}" "-device" |
78 | - "${driver},drive=drv${id},${bs_args}${devopts}" ) |
79 | + t="file=${src},if=none,cache=unsafe,format=$fmt," |
80 | + t="${t}id=drv${id},index=$id," |
81 | + t="${t}driver=${driver},${bs_args}${devopts}" |
82 | + disk_args[${#disk_args[@]}]="--disk=$t" |
83 | |
84 | done |
85 | |
86 | @@ -754,7 +753,7 @@ |
87 | seed="${TEMP_D}/seed.img" |
88 | cloud-localds "$seed" "$udata" "$mdata" || |
89 | { error "failed cloud-localds"; return 1; } |
90 | - seedargs=( "-drive" "file=${seed},if=virtio,media=cdrom" ) |
91 | + seedargs=( "--disk=file=${seed},if=virtio,media=cdrom" ) |
92 | fi |
93 | |
94 | local netargs |
95 | @@ -779,13 +778,14 @@ |
96 | fi |
97 | fi |
98 | # -monitor stdio |
99 | + local bootdisk="--disk=file=$bootimg,id=boot,index=1,cache=unsafe" |
100 | cmd=( |
101 | - xkvm "${pt[@]}" "${netargs[@]}" -- |
102 | + xkvm "${pt[@]}" "${netargs[@]}" |
103 | + "$bootdisk" |
104 | + "${disk_args[@]}" |
105 | + -- |
106 | -smp ${smp} |
107 | -m ${mem} ${serial_args} ${video} |
108 | - -drive "file=$bootimg,if=none,cache=unsafe,format=qcow2,id=boot,index=0" |
109 | - -device "virtio-blk,drive=boot" |
110 | - "${disk_args[@]}" |
111 | "${seedargs[@]}" |
112 | ) |
113 | |
114 | |
115 | === modified file 'tools/xkvm' |
116 | --- tools/xkvm 2016-10-07 19:31:01 +0000 |
117 | +++ tools/xkvm 2017-10-19 20:20:00 +0000 |
118 | @@ -11,6 +11,8 @@ |
119 | # OVS_CLEANUP gets populated with bridge:devname pairs used with ovs |
120 | OVS_CLEANUP=( ) |
121 | MAC_PREFIX="52:54:00:12:34" |
122 | +# allow this to be set externally. |
123 | +_QEMU_SUPPORTS_FILE_LOCKING="${_QEMU_SUPPORTS_FILE_LOCKING}" |
124 | KVM="kvm" |
125 | declare -A KVM_DEVOPTS |
126 | |
127 | @@ -119,6 +121,21 @@ |
128 | return 1 |
129 | } |
130 | |
131 | +qemu_supports_file_locking() { |
132 | + # hackily check if qemu has file.locking in -drive params (LP: #1716028) |
133 | + if [ -z "$_QEMU_SUPPORTS_FILE_LOCKING" ]; then |
134 | + # The only way we could find to check presense of file.locking is |
135 | + # qmp (query-qmp-schema). Simply checking if the virtio-blk driver |
136 | + # supports 'share-rw' is expected to be equivalent and simpler. |
137 | + isdevopt virtio-blk share-rw && |
138 | + _QEMU_SUPPORTS_FILE_LOCKING=true || |
139 | + _QEMU_SUPPORTS_FILE_LOCKING=false |
140 | + debug 1 "qemu supports file locking = ${_QEMU_SUPPORTS_FILE_LOCKING}" |
141 | + fi |
142 | + [ "$_QEMU_SUPPORTS_FILE_LOCKING" = "true" ] |
143 | + return |
144 | +} |
145 | + |
146 | padmac() { |
147 | # return a full mac, given a subset. |
148 | # assume whatever is input is the last portion to be |
149 | @@ -443,7 +460,7 @@ |
150 | out=$(LANG=C qemu-img info "$file") && |
151 | fmt=$(echo "$out" | awk '$0 ~ /^file format:/ { print $3 }') || |
152 | { error "failed to determine format of $file"; return 1; } |
153 | - else |
154 | + elif [ -z "$fmt" ]; then |
155 | fmt=raw |
156 | fi |
157 | if [ -z "$driver" ]; then |
158 | @@ -470,6 +487,12 @@ |
159 | id=*|if=*|driver=*|$file|file=*) continue;; |
160 | fmt=*|format=*) continue;; |
161 | serial=*|bus=*|unit=*|index=*) continue;; |
162 | + file.locking=*) |
163 | + qemu_supports_file_locking || { |
164 | + debug 2 "qemu has no file locking." \ |
165 | + "Dropping '$tok' from: $cur" |
166 | + continue |
167 | + };; |
168 | esac |
169 | isdevopt "$driver" "$tok" && devopts="${devopts},$tok" || |
170 | diskopts="${diskopts},${tok}" |
PASSED: Continuous integration, rev:527 /jenkins. ubuntu. com/server/ job/curtin- ci/616/ /jenkins. ubuntu. com/server/ job/curtin- ci/nodes= metal-amd64/ 616 /jenkins. ubuntu. com/server/ job/curtin- ci/nodes= metal-arm64/ 616 /jenkins. ubuntu. com/server/ job/curtin- ci/nodes= metal-ppc64el/ 616 /jenkins. ubuntu. com/server/ job/curtin- ci/nodes= metal-s390x/ 616
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild: /jenkins. ubuntu. com/server/ job/curtin- ci/616/ rebuild
https:/