Re: [PATCH v7 09/12] tests/vm: Added a new script for ubuntu.aarch64.

[Alex Bennée]

Robert Foley <address@hidden> writes:

> ubuntu.aarch64 provides a script to create an Ubuntu 18.04 VM.
> Another new file is also added aarch64vm.py, which is a module with
> common methods used by aarch64 VMs, such as how to create the
> flash images.
>
> Signed-off-by: Robert Foley <address@hidden>
> Reviewed-by: Peter Puhov <address@hidden>
> ---
>  configure                 |  20 +++++++
>  tests/vm/Makefile.include |  11 ++++
>  tests/vm/aarch64vm.py     | 106 ++++++++++++++++++++++++++++++++++
>  tests/vm/basevm.py        |  12 ++++
>  tests/vm/ubuntu.aarch64   | 117 ++++++++++++++++++++++++++++++++++++++
>  5 files changed, 266 insertions(+)
>  create mode 100644 tests/vm/aarch64vm.py
>  create mode 100755 tests/vm/ubuntu.aarch64
>
> diff --git a/configure b/configure
> index 89d11aa5d4..d38db335dd 100755
> --- a/configure
> +++ b/configure
> @@ -411,6 +411,7 @@ prefix="/usr/local"
>  mandir="\${prefix}/share/man"
>  datadir="\${prefix}/share"
>  firmwarepath="\${prefix}/share/qemu-firmware"
> +efi_aarch64_arg=
>  qemu_docdir="\${prefix}/share/doc/qemu"
>  bindir="\${prefix}/bin"
>  libdir="\${prefix}/lib"
> @@ -1099,6 +1100,8 @@ for opt do
>    ;;
>    --firmwarepath=*) firmwarepath="$optarg"
>    ;;
> +  --efi-aarch64=*) efi_aarch64_arg="$optarg"
> +  ;;
>    --host=*|--build=*|\
>    --disable-dependency-tracking|\
>    --sbindir=*|--sharedstatedir=*|\
> @@ -1753,6 +1756,7 @@ Advanced options (experts only):
>    --sysconfdir=PATH        install config in PATH$confsuffix
>    --localstatedir=PATH     install local state in PATH (set at runtime on 
> win32)
>    --firmwarepath=PATH      search PATH for firmware files
> +  --efi-aarch64=PATH       PATH of efi file to use for aarch64 VMs.
>    --with-confsuffix=SUFFIX suffix for QEMU data inside 
> datadir/libdir/sysconfdir [$confsuffix]
>    --with-pkgversion=VERS   use specified string as sub-version of the package
>    --enable-debug           enable common debug build options
> @@ -3548,6 +3552,20 @@ EOF
>    fi
>  fi
>  
> +############################################
> +# efi-aarch64 probe
> +# Check for efi files needed by aarch64 VMs.
> +# By default we will use the efi included with QEMU.
> +# Allow user to override the path for efi also.
> +qemu_efi_aarch64=$PWD/pc-bios/edk2-aarch64-code.fd

as you only define this once there is no harm in just having a long line
bellow rather than running the potential confusion when looking at the
variables.

> +for fd in $efi_aarch64_arg $qemu_efi_aarch64
> +do
> +    if test -f $fd; then
> +        efi_aarch64=$fd
> +        break
> +    fi
> +done

This only detects the pc-bios bundled version of edk on a directory
which has already been built. Maybe we need to do a straight forward:

  if not test -f $efi_aarch64; then
      if test -f $SRC/pc-bios/edk2-aarch64-code.fd.bz2; then
          # valid after build
          efi_aarch64=$PWD/pc-bios/edk2-aarch64-code.fd
      else
          efi_aarch64=""
      fi
  fi

what do you think?

<snip>
> +
> +    def build_image(self, img):
> +        os_img = self._download_with_cache(self.image_link)
> +        img_tmp = img + ".tmp"
> +        subprocess.check_call(["cp", "-f", os_img, img_tmp])
> +        subprocess.check_call(["qemu-img", "resize", img_tmp, "+50G"])
> +        ci_img = self.gen_cloud_init_iso()
> +
> +        self.boot(img_tmp, extra_args = ["-cdrom", ci_img])
> +        if self.debug:
> +            self.wait_boot()
> +        # First command we issue is fix for slow ssh login.
> +        self.wait_ssh(wait_root=True,
> +                      cmd="chmod -x /etc/update-motd.d/*")
> +        # Wait for cloud init to finish
> +        self.wait_ssh(wait_root=True,
> +                      cmd="ls /var/lib/cloud/instance/boot-finished")
> +        self.ssh_root("touch /etc/cloud/cloud-init.disabled")
> +        # Disable auto upgrades.
> +        # We want to keep the VM system state stable.
> +        self.ssh_root('sed -ie \'s/"1"/"0"/g\' 
> /etc/apt/apt.conf.d/20auto-upgrades')
> +        # If the user chooses *not* to do the second phase,
> +        # then we will jump right to the graceful shutdown
> +        if self._config['install_cmds'] != "":
> +            self.ssh_root("sync")
> +            # Shutdown and then boot it again.
> +            # Allows us to know for sure it is booting (not shutting down)
> +            # before we call wait_ssh().
> +            self.graceful_shutdown()
> +            self.boot(img_tmp)
> +            if self.debug:
> +                self.wait_boot()
> +            self.wait_ssh(wait_root=True)
> +            self.wait_ssh(wait_root=True, cmd="locale")

Why do we need to shutdown before proceeding with the install commands?
I see ubuntu.i386 does it as well although with a slightly hackier
approach.

> +            # The previous update sometimes doesn't survive a reboot, so do 
> it again
> +            self.ssh_root("sed -ie s/^#\ deb-src/deb-src/g 
> /etc/apt/sources.list")
> +
> +            # Issue the install commands.
> +            # This can be overriden by the user in the config .yml.
> +            install_cmds = self._config['install_cmds'].split(',')
> +            for cmd in install_cmds:
> +                self.ssh_root(cmd)
> +        self.graceful_shutdown()
> +        self.wait()
> +        os.rename(img_tmp, img)
> +        return 0

How come we are diverging from the ubuntu.i386 install here? You've
moved all the complications for aarch64 into a it's own handling so
these steps are almost but not quite the same. Couldn't the ubuntu
build_img code be common and then just have a slightly different set of
install commands?

> +
> +if __name__ == "__main__":
> +    defaults = aarch64vm.get_config_defaults(UbuntuAarch64VM, DEFAULT_CONFIG)
> +    sys.exit(basevm.main(UbuntuAarch64VM, defaults))


-- 
Alex Bennée