Add support for shared disks #21
Conversation
run
Outdated
| -a '//devices/disk[last()]/address' -t attr -n unit -v $sidx \ | ||
| "$xml" | ||
| fi | ||
| if [ "$bus" != "scsi" ]; then |
There was a problem hiding this comment.
There is a formatting error.
You need to move the code block to the right.
| @@ -259,6 +259,7 @@ parse_options(){ | |||
| exit 1 | |||
| fi | |||
|
|
|||
| DISK_OWNER=$OWNER | |||
There was a problem hiding this comment.
Why do we need this variable?
There was a problem hiding this comment.
This variable is needed to separate NODE and OWNER since below NODE is added to OWNER. This is used on line 701: VOLUMES+=("disk-${disk[0]}-$DISK_OWNER:$disk_path:raw:${disk[1]}:${disk[0]}"). This is necessary so that scsi disks (shared) have the same image for two VMs.
run
Outdated
| -a '//devices/disk[last()]/address' -t attr -n unit -v $sidx \ | ||
| "$xml" | ||
| fi | ||
| if [ "$bus" != "scsi" ]; then |
There was a problem hiding this comment.
I think it's best to check the equality.
run
Outdated
| disk_path="$USR_CACHE_DIR/$VMNAME/disk-${disk[0]}.qcow2" | ||
| log qemu-img create -f qcow2 "$disk_path" ${disk[2]} | ||
| VOLUMES+=("disk-${disk[0]}-$OWNER:$disk_path:qcow2:${disk[1]}:${disk[0]}") | ||
| disk_path="$USR_CACHE_DIR/$VMNAME/disk-${disk[0]}.raw" |
There was a problem hiding this comment.
You can never use a qcow2 image for a shared-write disk as it will cause corruption of the qcow2 metadata. Use raw format for shared disks.
run
Outdated
| -a '//devices/disk[last()]/target' -t attr -n bus -v $bus \ | ||
| -a '//devices/disk[last()]/target' -t attr -n dev -v $dev \ | ||
| "$xml" | ||
| else |
There was a problem hiding this comment.
Please add a comment to the pull request:
What's changed here?
Why is this changed?
There was a problem hiding this comment.
The xml description of the VM changes here. This is necessary so that scisi disks can be described as shared.
|
Please add an example to README.md |
DenisValitov
force-pushed
the
shared_disk
branch
3 times, most recently
from
341ce36 to
3a448b2
Compare
DenisValitov
force-pushed
the
shared_disk
branch
from
d708ce3 to
6e702e3
Compare
DenisValitov
force-pushed
the
shared_disk
branch
from
f395a58 to
08fbdd6
Compare
DenisValitov
force-pushed
the
shared_disk
branch
from
08fbdd6 to
0240778
Compare
| @@ -96,6 +96,8 @@ options: | |||
| -f, --force stop running VM | |||
| -c, --clean remove kernel and initramfs volumes after VM | |||
| shutdown | |||
| --shared using scsi disks as shared disk (The qcow2 | |||
There was a problem hiding this comment.
It wasn't clear from the PR why qcow2 can't be used for shared disk, but the ticket somewhat explains that:
https://bugzilla.redhat.com/show_bug.cgi?id=1511480
It's still weird that qemu has issues with shared disks for qcow2.
| @@ -225,6 +227,9 @@ parse_options(){ | |||
| elif [ "$1" == "-K" -o "$1" == "--keep" ]; then | |||
| [ -t 1 ] && KOPT+=("ktest.keep") | |||
| shift | |||
| elif [ "$1" == "--shared" ]; then | |||
There was a problem hiding this comment.
If wonder if we should make it an attribute of a DISKSPEC rather than global setting for all disks.
| -a '//devices/disk[last()]/target' -t attr -n bus -v $bus \ | ||
| -a '//devices/disk[last()]/target' -t attr -n dev -v $dev \ | ||
| -s '//devices/disk[last()]' -t elem -n shareable -v '' \ | ||
| -s '//devices/disk[last()]' -t elem -n vendor -v 'YADRO' \ |
There was a problem hiding this comment.
vendor/product belongs to diskspec too
| -a '//devices/disk[last()]/address' -t attr -n type -v 'drive' \ | ||
| -a '//devices/disk[last()]/address' -t attr -n unit -v $sidx \ | ||
| "$xml" | ||
| xml ed --inplace \ |
There was a problem hiding this comment.
why did you change the hunk?
| "$xml" | ||
|
|
||
| if [ -n "$SHARED" ] && [ "$bus" == "scsi" ]; then | ||
| xml ed --inplace \ |
There was a problem hiding this comment.
Please drop the hunk altogether and append required parameters for sharable disk below
if [ "$bus" == "scsi" ]; then
<...set up scsi controller...>
if [ -n "$SHARED" ]; then
<...set up settings for sharable disk...>
fi
if <...vendor is provided in diskspec..>; then
<setup vendor>
fi
if <...product is provided in diskspec...>; then
<...setup product...>
fi
fi
That would reduce amount of unneeded code churn and make the change much easier to review.
| @@ -703,6 +741,7 @@ if [ -n "$GDB_ADDRESS" ]; then | |||
| xml::gdb::set "$XML_NAME" "$GDB_ADDRESS" | |||
| fi | |||
|
|
|||
| while [ -n "$NODE" ] && [ "$NODE" -ne "0" ] && ! [[ -f $USR_CACHE_DIR/ktest-0-$DISK_OWNER/start.log ]]; do sleep 1; done | |||
There was a problem hiding this comment.
It would hang indefinitely if the disk doesn't come up. Also the dependency on the start log is questionable.
What is ktest-0?
Added support for shared drives when the --shared is enabled. The xml description of the VM changes here. This is necessary so that scisi disks can be described as shared.
The qcow2 format is also replaced with raw. This is necessary because you cannot use a qcow2 image for a shared write disks, as this will corrupt the qcow2 metadata.