Re: [Xen-devel] [PATCH 02/11] Introduce cirros tests

[Gémes Géza <geza.gemes@xxxxxxxxx>]

Hi Stefano,

A few answers/comments inline:

On Fri, 24 Mar 2017, Géza Gémes wrote:

Add support for using cirros images in raisin tests

Signed-off-by: Géza Gémes <geza.gemes@xxxxxxxxx>

Thank you! Much, much better, almost ready :)

I have only a couple of suggestions and questions left, see below.

 lib/common-functions.sh            |  23 +++++++
 lib/common-tests.sh                | 126 +++++++++++++++++++++++++++++++++++++
 tests-configs/config-cirros_x86_32 |  13 ++++
 tests-configs/config-cirros_x86_64 |  13 ++++
 4 files changed, 175 insertions(+)
 create mode 100644 tests-configs/config-cirros_x86_32
 create mode 100644 tests-configs/config-cirros_x86_64

diff --git a/lib/common-functions.sh b/lib/common-functions.sh
index d4476f3..d82d7ab 100644
--- a/lib/common-functions.sh
+++ b/lib/common-functions.sh
@@ -439,3 +439,26 @@ function uninstall_package() {
         error_echo "Don't know how to uninstall packages on $DISTRO"
     fi
 }
+
+function get-qemu-img() {
+    set +e
+    QEMU_IMG=`which qemu-img`
+    set -e
+    if [[ -z "$QEMU_IMG" ]]
+    then
+        QEMU_IMG="/usr/lib/xen/bin/qemu-img"
+    fi
+    export QEMU_IMG

we probably want to test that $QEMU_IMG exists (test -f), and print an
error if it doesn't

For QEMU_IMG I would rather check if it is executable, as that is needed.

+}
+
+function get-pvgrub() {
+    ARCH=$1
+    set +e
+    PVGRUB=`which grub-${ARCH}-xen`
+    set -e
+    if [[ -z "$PVGRUB" ]]
+    then
+        PVGRUB="/usr/lib/xen/boot/grub-${ARCH}-xen"
+    fi
+    export PVGRUB

Same here.

Also, I don't see who is calling get-qemu-img and get-pvgrub in this
patch series.

This is strange. I've called them now explicitly.

+}
diff --git a/lib/common-tests.sh b/lib/common-tests.sh
index d346af4..1deaf4f 100644
--- a/lib/common-tests.sh
+++ b/lib/common-tests.sh
@@ -178,3 +178,129 @@ function get_host_initrd() {
         exit 1
     fi
 }
+
+function cirros_network_init() {
+    rootdir=$1
+    ifile=`mktemp`
+    # Create static network config
+    cat >$ifile <<EOF
+auto lo
+iface lo inet loopback
+
+auto eth0
+iface eth0 inet static
+    address 169.254.0.2
+    network 169.254.0.0
+    broadcast 169.254.0.255
+    netmask 255.255.255.0
+EOF
+    $SUDO mv $ifile $rootdir/etc/network/interfaces

This is OK. However, we need to to "mv -f" otherwise it might ask for
confirmation before overwriting.

Done.

+    # Disable cloud-init
+    $SUDO rm -f ${rootdir}/etc/rc3.d/S*cirros*ds*
+    $SUDO rm -f ${rootdir}/etc/rc3.d/S*-cirros-userdata
+}
+
+function get_cirros_kernel() {
+    bootdir=$1
+    basename `find $bootdir -name vmlinuz* 2>/dev/null | head -1`
+}
+
+function get_cirros_initrd() {
+    bootdir=$1
+    basename `find $bootdir -name initrd* 2>/dev/null | head -1`
+}
+
+function cirros_grub_cfg() {
+    rootdir=$1
+    grubroot="`echo $CIRROS_GRUB_CFG | cut -d ')' -f 1`)"
+    grubcfg="`echo $CIRROS_GRUB_CFG | cut -d ')' -f 2`"
+    grubdir=`dirname $grubcfg`
+    bootdir=`dirname $grubdir`
+    tmpgrubcfg=`mktemp`
+    cat > $tmpgrubcfg <<EOF
+root="$grubroot"
+insmod xzio
+insmod gzio
+insmod btrfs
+insmod ext2
+set timeout=1
+set default=0
+menuentry Cirros {
+    linux `echo $bootdir`/`get_cirros_kernel ${rootdir}/${bootdir}` root=/dev/xvda1 ro
+    initrd `echo $bootdir`/`get_cirros_initrd ${rootdir}/${bootdir}`
+}
+EOF
+    $SUDO mv $tmpgrubcfg ${rootdir}/${grubcfg}

mv -f

Done.

+}
+
+function download_cirros_components() {
+    verbose_echo "Downloading cirros components"
+    . tests-configs/config-cirros_$RAISIN_ARCH
+    mkdir -p $CIRROS_DOWNLOADS
+    wget -q -c $CIRROS_KERNEL_URL -P $CIRROS_DOWNLOADS
+    wget -q -c $CIRROS_INITRD_URL -P $CIRROS_DOWNLOADS
+    wget -q -c $CIRROS_ROOTFS_URL -P $CIRROS_DOWNLOADS
+    wget -q -c $CIRROS_DISK_URL -P $CIRROS_DOWNLOADS

If the downloads are already there, does wget do the right thing?  I
guess it does. Otherwise, do we need to test -f each of the files?

wget -c tries to continue the download. If the file is fully downloaded it does nothing. However given your other suggestion to do unzipping right in this function, I'll change that into checking for the existence of the files.

+}
+
+function set_up_cirros_kernel_initrd() {
+    local testdir=$1
+    mkdir -p $testdir
+    download_cirros_components

I would probably move the download_cirros_components call to the test
scripts, for example cirros-separate-kernel-pv-test.

Done.

+    verbose_echo "Setting up kernel and initrd for cirros test"
+    cp $CIRROS_DOWNLOADS/$CIRROS_KERNEL_FILE $testdir
+    cp $CIRROS_DOWNLOADS/$CIRROS_INITRD_FILE $testdir
+}
+
+function set_up_cirros_rootfs() {
+    local testdir=$1
+    mkdir -p $testdir
+    download_cirros_components
+    verbose_echo "Setting up rootfs for cirros test"
+    cp $CIRROS_DOWNLOADS/$CIRROS_ROOTFS_FILE.gz $testdir
+    gunzip $testdir/$CIRROS_ROOTFS_FILE.gz

I would leave the unzipped file under $CIRROS_DOWNLOADS, so that we
don't have to unzip the same rootfs file twice.

I would also test for the existance of the unzipped file here and skip
the whole setup if it is already present. So the first time, we unzip
and setup the network interfaces file; the second time we just test that
the unzipped file exists and return. Would that work?

Done.

+    local cirros_rootfs_loop=`create_loop $testdir/$CIRROS_ROOTFS_FILE`
+    local cirros_rootfs_mntpt=`mktemp -d`
+    $SUDO mount $cirros_rootfs_loop $cirros_rootfs_mntpt
+    cirros_network_init $cirros_rootfs_mntpt
+    $SUDO umount $cirros_rootfs_mntpt
+    $SUDO rmdir $cirros_rootfs_mntpt
+    $SUDO losetup -d $cirros_rootfs_loop
+}
+
+function set_up_cirros_disk() {
+    local testdir=$1
+    mkdir -p $testdir
+    local grub_install=False
+    if [[ $# -gt 1 ]]
+    then
+        grub_install=$2
+    else
+        unset grub_install
+    fi

Is this test actually needed? It doesn't look like anything is calling
set_up_cirros_disk with more than one argument.

The pvgrub test, which needs a grub2 config file (not included by cirros) calls this with two arguments. However as I restructured the code this function is dropped.

+    download_cirros_components
+    verbose_echo "Setting up disk for cirros test"
+    $QEMU_IMG convert -f qcow2 -O raw $CIRROS_DOWNLOADS/$CIRROS_DISK_FILE $testdir/$CIRROS_DISK_FILE

Similarly to the rootfs, I would output the conversion to
$CIRROS_DOWNLOADS to avoid doing it twice. The second time the function
gets called, I would just test for the existance of the raw file and
return. (It is probably best to rename the raw file to something like
$CIRROS_DISK_FILE.raw). In addition, if possible, I would also do the
following setup only the first time around. Does it make sense?

Done

+    local cirros_disk_loop=`$SUDO $BASEDIR/scripts/lopartsetup $testdir/$CIRROS_DISK_FILE | head -1 | cut -d ":" -f 1`
+    local cirros_disk_mntpt=`mktemp -d`
+    $SUDO mount $cirros_disk_loop $cirros_disk_mntpt
+    cirros_network_init $cirros_disk_mntpt
+    if [[ -n $grub_install ]]
+    then
+        cirros_grub_cfg $cirros_disk_mntpt
+    fi
+    $SUDO umount $cirros_disk_mntpt
+    $SUDO rmdir $cirros_disk_mntpt
+    $SUDO losetup -d $cirros_disk_loop

This is just for you information. If you remove the loop device, it
means that you'll end up using the QEMU userspace disk backend.
Otherwise if you keep the loop device around and use it for the backend,
you end up using the in-kernel disk backend, which is typically faster.
In other words, the following:

  disk = [ '$testdir/$CIRROS_DISK_FILE,raw,hda,rw' ]

ends up using the QEMU disk backend, while the following:

  disk = [ '$cirros_disk_loop,raw,hda,rw' ]

ends up using the kernel disk backend.
Either choice is fine for the tests, I just wanted to make sure you knew :)

Thanks for the info. I didn't know. However for simplicity I'll keep the image files.

+}
+
+function tear_down_cirros_test() {
+    testdir=$1
+    if [[ `$SUDO xl vm-list | grep -e "raisin-test" | wc -l` -gt 0 ]]

Why "grep -e"? I think grep is sufficient.

Done

+    then
+        $SUDO xl destroy "raisin-test"
+    fi
+    verbose_echo "$PREPEND deleting environment of cirros test"
+    $SUDO rm -rf $testdir
+}
+
diff --git a/tests-configs/config-cirros_x86_32 b/tests-configs/config-cirros_x86_32
new file mode 100644
index 0000000..5402415
--- /dev/null
+++ b/tests-configs/config-cirros_x86_32
@@ -0,0 +1,13 @@
+CIRROS_ARCH=i386
+CIRROS_BASE_URL="https://download.cirros-cloud.net/"
+CIRROS_VERSION="0.3.5"
+CIRROS_DOWNLOADS=$BASEDIR/downloads
+CIRROS_KERNEL_FILE=cirros-${CIRROS_VERSION}-${CIRROS_ARCH}-kernel
+CIRROS_INITRD_FILE=cirros-${CIRROS_VERSION}-${CIRROS_ARCH}-initramfs
+CIRROS_ROOTFS_FILE=cirros-${CIRROS_VERSION}-${CIRROS_ARCH}-rootfs.img
+CIRROS_DISK_FILE=cirros-${CIRROS_VERSION}-${CIRROS_ARCH}-disk.img
+CIRROS_KERNEL_URL=${CIRROS_BASE_URL}/${CIRROS_VERSION}/${CIRROS_KERNEL_FILE}
+CIRROS_INITRD_URL=${CIRROS_BASE_URL}/${CIRROS_VERSION}/${CIRROS_INITRD_FILE}
+CIRROS_ROOTFS_URL=${CIRROS_BASE_URL}/${CIRROS_VERSION}/${CIRROS_ROOTFS_FILE}.gz
+CIRROS_DISK_URL=${CIRROS_BASE_URL}/${CIRROS_VERSION}/${CIRROS_DISK_FILE}
+CIRROS_GRUB_CFG="(xen/xvda,msdos1)/boot/grub/grub.cfg"

No need for {} around variables. For example, this should work:

  CIRROS_DISK_FILE=cirros-$CIRROS_VERSION-$CIRROS_ARCH-disk.img

But other than that, the two config files look fine.

Done

diff --git a/tests-configs/config-cirros_x86_64 b/tests-configs/config-cirros_x86_64
new file mode 100644
index 0000000..72cf489
--- /dev/null
+++ b/tests-configs/config-cirros_x86_64
@@ -0,0 +1,13 @@
+CIRROS_ARCH=x86_64
+CIRROS_BASE_URL="https://download.cirros-cloud.net/"
+CIRROS_VERSION="0.3.5"
+CIRROS_DOWNLOADS=$BASEDIR/downloads
+CIRROS_KERNEL_FILE=cirros-${CIRROS_VERSION}-${CIRROS_ARCH}-kernel
+CIRROS_INITRD_FILE=cirros-${CIRROS_VERSION}-${CIRROS_ARCH}-initramfs
+CIRROS_ROOTFS_FILE=cirros-${CIRROS_VERSION}-${CIRROS_ARCH}-rootfs.img
+CIRROS_DISK_FILE=cirros-${CIRROS_VERSION}-${CIRROS_ARCH}-disk.img
+CIRROS_KERNEL_URL=${CIRROS_BASE_URL}/${CIRROS_VERSION}/${CIRROS_KERNEL_FILE}
+CIRROS_INITRD_URL=${CIRROS_BASE_URL}/${CIRROS_VERSION}/${CIRROS_INITRD_FILE}
+CIRROS_ROOTFS_URL=${CIRROS_BASE_URL}/${CIRROS_VERSION}/${CIRROS_ROOTFS_FILE}.gz
+CIRROS_DISK_URL=${CIRROS_BASE_URL}/${CIRROS_VERSION}/${CIRROS_DISK_FILE}
+CIRROS_GRUB_CFG="(xen/xvda,msdos1)/boot/grub/grub.cfg"

Cheers,

Geza

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel