[Merge] ~patviafore/livecd-rootfs/+git/livecd-rootfs:linux_kvm_image into livecd-rootfs:ubuntu/master

Robert C Jennings robert.jennings at canonical.com
Tue Jun 4 20:12:38 UTC 2019


Review: Needs Fixing

Looks good.  I've added a few comments for missing code mostly.

Diff comments:

> diff --git a/live-build/functions b/live-build/functions
> index 72fdb05..2bdbb62 100644
> --- a/live-build/functions
> +++ b/live-build/functions
> @@ -877,3 +877,17 @@ is_live_layer () {
>      done
>      return 1
>  }
> +
> +replace_kernel () { 
> +    mountpoint=$1
> +    new_kernel=$2
> +
> +    # Install custom kernel (N.B. the trailing + retains linux-base during this
> +    # operation)
> +    env DEBIAN_FRONTEND=noninteractive chroot "${mountpoint}" apt-get \
> +        remove --purge --assume-yes '^linux-.*' 'linux-base+'
> +    env DEBIAN_FRONTEND=noninteractive chroot "${mountpoint}" apt-get \
> +        update --assume-yes
> +    env DEBIAN_FRONTEND=noninteractive chroot "${mountpoint}" apt-get \
> +        install --assume-yes "${new_kernel}" 

Can you add 'apt-get autoremove --purge --assume-yes' and see if that does any work?  I think we want that command here and I'm curious if there's anything that does get removed when you do that.  Thanks.

> +}
> \ No newline at end of file
> diff --git a/live-build/ubuntu-cpc/hooks.d/base/kvm-image.binary b/live-build/ubuntu-cpc/hooks.d/base/kvm-image.binary
> new file mode 100644
> index 0000000..ded54c6
> --- /dev/null
> +++ b/live-build/ubuntu-cpc/hooks.d/base/kvm-image.binary
> @@ -0,0 +1,59 @@
> +#!/bin/bash -ex

Please add 'u' to the cmdline flags.  We want to error out on unset variables.

> +# vi: ts=4 expandtab
> +#
> +# Generate KVM images

Add an 'echo "Building KVM image" here just to make it easy to pick out in debug output.  Also, you can replaces 'images' with 'image' above to be accurate.

> +#
> +
> +case ${SUBPROJECT:-} in
> +    minimized)
> +        echo "Skipping minimized $0 builds"
> +        exit 0
> +        ;;
> +    *)
> +        ;;
> +esac
> +
> +# Only allow amd64 builds for now
> +case $ARCH in
> +        amd64)
> +            ;;
> +        *)
> +            echo "Linux KVM images are not supported for $ARCH yet.";
> +            exit 0;;
> +esac
> +
> +. config/functions
> +
> +mount_d=$(mktemp -d)
> +
> +create_derivative "disk" "kvm" #sets ${derivative_img}
> +mount_disk_image ${derivative_img} ${mount_d}
> +
> +# unmount disk image and remove created folders on exit
> +cleanup_kvm() {
> +    if [ -d "$mount_d" ]; then
> +        umount_disk_image "$mount_d"
> +    fi
> +    rm -rf ${mount_d} ${derivative_img}
> +}
> +trap cleanup_kvm EXIT
> +
> +chroot ${mount_d} apt-get update

This should be unnecessary as replace_kernel does this.

> +
> +divert_grub "${mount_d}"
> +replace_kernel ${mount_d} "linux-kvm"
> +undivert_grub "${mount_d}"
> +
> +#setup grub correctly
> +env DEBIAN_FRONTEND=noninteractive chroot "${mount_d}" update-grub
> +replace_grub_root_with_label "${mount_d}"
> +
> +# Remove initramfs for kvm image
> +env DEBIAN_FRONTEND=noninteractive chroot "${mount_d}" apt-get \
> +    purge -y initramfs-tools busybox-initramfs
> +

Add an 'apt-get clean' to remove indices.

> +env DEBIAN_FRONTEND=noninteractive chroot "${mount_d}" rm \
> +    -rf /boot/initrd.img-* /boot/initrd.img
> +
> +create_manifest ${mount_d} livecd.ubuntu-cpc.kvm.manifest
> +convert_to_qcow2 binary/boot/disk-kvm.ext4 livecd.ubuntu-cpc.kvm.img 

You haven't torn down the disk image by calling cleanup_kvm (it's still mounted by 'mount_disk_image') and so you shouldn't convert before you do that to avoid corruption.

Also, remove the source file to save space for other hooks that will run (binary/boot/disk-kvm.ext4).

Lastly, the output name... Let's give ourselves some room to build other kvm variations by calling this livecd.ubuntu-cpc.disk-kvm.img. (Or something)

> \ No newline at end of file


-- 
https://code.launchpad.net/~patviafore/livecd-rootfs/+git/livecd-rootfs/+merge/368350
Your team Ubuntu Core Development Team is requested to review the proposed merge of ~patviafore/livecd-rootfs/+git/livecd-rootfs:linux_kvm_image into livecd-rootfs:ubuntu/master.



More information about the Ubuntu-reviews mailing list