|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 02/11] Introduce cirros tests
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
> +}
> +
> +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.
> +}
> 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.
> + # 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
> +}
> +
> +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?
> +}
> +
> +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.
> + 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?
> + 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.
> + 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?
> + 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 :)
> +}
> +
> +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.
> + 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.
> 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"_______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |