Skip to content

Add support for shared disks #21

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add support for shared disks #21

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6e702e3
Add support for shared disks
Aug 18, 2020
0240778
Optimized time for creating and uploading discs for --shared mode
Sep 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 71 additions & 32 deletions run
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

@roolebo roolebo Oct 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

format is also replaced with raw.)
-G, --gdb [HOST]:PORT Enable QEMU GDB stub via HOST:PORT
-k, --kopt PARAM pass a kernel option
-K, --keep don't shutdown VM immediately
Expand Down Expand Up @@ -225,6 +227,9 @@ parse_options(){
elif [ "$1" == "-K" -o "$1" == "--keep" ]; then
[ -t 1 ] && KOPT+=("ktest.keep")
shift
elif [ "$1" == "--shared" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If wonder if we should make it an attribute of a DISKSPEC rather than global setting for all disks.

SHARED=1
shift
elif [ "$1" == "--disable-kdump" ]; then
DISABLE_KDUMP=y
shift
Expand Down Expand Up @@ -259,6 +264,7 @@ parse_options(){
exit 1
fi

DISK_OWNER=$OWNER
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this variable?

Copy link
Author

@DenisValitov DenisValitov Aug 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

KERNEL="${POSITIONAL[0]}"
[ -r "$KERNEL" ] || die "Can't read kernel: $KERNEL"
[ -n "$ENTRY_POINT" ] && KOPT+=("ktest.start=$ENTRY_POINT")
Expand Down Expand Up @@ -394,28 +400,52 @@ make_xml(){
local bus=scsi
((sidx++))
fi
xml ed --inplace \
-s //devices -t elem -n disk -v '' \
-a '//devices/disk[last()]' -t attr -n type -v volume \
-a '//devices/disk[last()]' -t attr -n device -v disk \
-s '//devices/disk[last()]' -t elem -n driver -v '' \
-a '//devices/disk[last()]/driver' -t attr -n name -v qemu \
-a '//devices/disk[last()]/driver' -t attr -n type -v "${v[2]}" \
-s '//devices/disk[last()]' -t elem -n source -v '' \
-a '//devices/disk[last()]/source' -t attr -n pool -v "$4" \
-a '//devices/disk[last()]/source' -t attr -n volume -v "${v[0]}" \
-s '//devices/disk[last()]' -t elem -n target -v '' \
-a '//devices/disk[last()]/target' -t attr -n bus -v $bus \
-a '//devices/disk[last()]/target' -t attr -n dev -v $dev \
"$xml"

if [ -n "$SHARED" ] && [ "$bus" == "scsi" ]; then
xml ed --inplace \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

-s //devices -t elem -n disk -v '' \
-a '//devices/disk[last()]' -t attr -n type -v file \
-a '//devices/disk[last()]' -t attr -n device -v disk \
-a '//devices/disk[last()]' -t attr -n snapshot -v no \
-s '//devices/disk[last()]' -t elem -n driver -v '' \
-a '//devices/disk[last()]/driver' -t attr -n name -v qemu \
-a '//devices/disk[last()]/driver' -t attr -n type -v "${v[2]}" \
-a '//devices/disk[last()]/driver' -t attr -n cache -v writeback \
-a '//devices/disk[last()]/driver' -t attr -n error_policy -v stop \
-s '//devices/disk[last()]' -t elem -n source -v '' \
-a '//devices/disk[last()]/source' -t attr -n file -v /var/lib/libvirt/images/"${v[0]}" \
-s '//devices/disk[last()]' -t elem -n backingStore -v '' \
-s '//devices/disk[last()]' -t elem -n target -v '' \
-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' \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vendor/product belongs to diskspec too

-s '//devices/disk[last()]' -t elem -n product -v 'shared_disk' \
"$xml"
else
xml ed --inplace \
-s //devices -t elem -n disk -v '' \
-a '//devices/disk[last()]' -t attr -n type -v volume \
-a '//devices/disk[last()]' -t attr -n device -v disk \
-s '//devices/disk[last()]' -t elem -n driver -v '' \
-a '//devices/disk[last()]/driver' -t attr -n name -v qemu \
-a '//devices/disk[last()]/driver' -t attr -n type -v "${v[2]}" \
-s '//devices/disk[last()]' -t elem -n source -v '' \
-a '//devices/disk[last()]/source' -t attr -n pool -v "$4" \
-a '//devices/disk[last()]/source' -t attr -n volume -v "${v[0]}" \
-s '//devices/disk[last()]' -t elem -n target -v '' \
-a '//devices/disk[last()]/target' -t attr -n bus -v $bus \
-a '//devices/disk[last()]/target' -t attr -n dev -v $dev \
"$xml"
fi

if [ "$bus" == "scsi" ]; then
xml ed --inplace \
-s '//devices/disk[last()]' -t elem -n address -v '' \
-a '//devices/disk[last()]/address' -t attr -n controller -v '0' \
-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 \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did you change the hunk?

-s '//devices/disk[last()]' -t elem -n address -v '' \
-a '//devices/disk[last()]/address' -t attr -n controller -v '0' \
-a '//devices/disk[last()]/address' -t attr -n type -v 'drive' \
-a '//devices/disk[last()]/address' -t attr -n unit -v $sidx \
"$xml"
fi

if [ -n "${v[4]}" ]; then
Expand Down Expand Up @@ -464,15 +494,17 @@ make_xml(){
}

vm::vol::upload(){
if ! logq virsh vol-info "$1" --pool "$POOL"; then
notify "Create volume $1 on $POOL"
local size=$(wc -c < "$2")
log virsh vol-create-as --format "$3" "$POOL" "$1" $size
fi
if cache::file::outdated "$2" "$HOST" "$POOL" "$1" || [ -n "$FORCE" ]; then
notify "Upload $2 to hypervisor"
log virsh vol-upload "$1" "$2" --pool "$POOL"
cache::file::update "$2" "$HOST" "$POOL" "$1"
if [ "$4" != "scsi" ] || [ -z "$NODE" ] || [ "$NODE" == "0" ] || [ -z "$SHARED" ]; then
if ! logq virsh vol-info "$1" --pool "$POOL"; then
notify "Create volume $1 on $POOL"
local size=$(wc -c < "$2")
log virsh vol-create-as --format "$3" "$POOL" "$1" $size
fi
if cache::file::outdated "$2" "$HOST" "$POOL" "$1" || [ -n "$FORCE" ]; then
notify "Upload $2 to hypervisor"
log virsh vol-upload "$1" "$2" --pool "$POOL"
cache::file::update "$2" "$HOST" "$POOL" "$1"
fi
fi
}

Expand Down Expand Up @@ -660,9 +692,15 @@ done

for d in "${DISKS[@]}"; do
IFS=':' read -ra disk <<< "$d"
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]}")
if [ -n "$SHARED" ]; then
disk_path="$USR_CACHE_DIR/$VMNAME/disk-${disk[0]}.raw"
log qemu-img create -f raw "$disk_path" ${disk[2]}
VOLUMES+=("disk-${disk[0]}-$DISK_OWNER:$disk_path:raw:${disk[1]}:${disk[0]}")
else
disk_path="$USR_CACHE_DIR/$VMNAME/disk-${disk[0]}.qcow"
log qemu-img create -f qcow2 "$disk_path" ${disk[2]}
VOLUMES+=("disk-${disk[0]}-$OWNER:$disk_path:qcow2:${disk[1]}:${disk[0]}")
fi
done

notify "Discover hypervisor..."
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would hang indefinitely if the disk doesn't come up. Also the dependency on the start log is questionable.

What is ktest-0?

ktest::lock
trap ktest::exit EXIT
TAKEN_PORTS="$USR_CACHE_DIR/$VMNAME/port.list"
Expand Down