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

[Robert Foley <robert.foley@linaro.org>]

On Fri, 22 May 2020 at 11:34, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Robert Foley <robert.foley@linaro.org> writes:
<snip>
> >
> > +############################################
> > +# 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.

OK, makes sense, will change to just go for the longer line.

> > +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?

I agree.  The straight up if check is easier to read.  Will change to this.

> <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 reboot was carried over from the way i386 did things.
I have a guess as to why the reboot is there, it is just after the
install of cloud-initramfs-growroot, which does require a reboot.
So I assume that we wanted to grow the root, reboot, and then
begin installation of all the new packages.

However, at this point, it looks like even without installing that package,
(with i386 or aarch64) the root is growing to fill the new size of the image,
so it seems that package and the reboot is no longer needed.
Will plan to remove these as part of making build_image common
(discussed below).

> > +            # 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?

I'm glad you brought this up.  I was noticing the commonality here especially if
we wanted to add another architecture of Ubuntu VM.
For example, we brought up a ppc64 Ubuntu VM script the other day and it really
cried out for creating a common Ubuntu module here since most of the
code is the same.

As suggested, I will plan to create a common Ubuntu module here and share
the build_img code, but have a different set of install commands.

Thanks & Regards,
-Rob

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