Code review comment for lp:~doanac/lava-dispatcher/uefi-from-prak

Hey Guys,

I turned this into a proper merge proposal on LP so we have a way to
discuss the changes. My comments are below, basically - I'm pretty sure
we can dramatically simplify this to be a very small change (my guess
<15 lines of code).

On 11/30/2012 09:54 AM, Andy Doan wrote:
> === modified file 'lava_dispatcher/actions/boot_control.py'
> --- lava_dispatcher/actions/boot_control.py 2012-11-20 21:22:17 +0000
> +++ lava_dispatcher/actions/boot_control.py 2012-11-30 15:53:29 +0000
> @@ -70,6 +70,28 @@
> finally:
> self.context.test_data.add_result("boot_image", status)
>
> +### UEFI
> +
> +class cmd_boot_linaro_uefi_image(BaseAction):
> + """ Call client code to boot to the master image
> + """
> +
> + parameters_schema = _boot_schema
> +
> + def run(self,options=[]):
> + client = self.client
> + client.target_device.boot_options = options
> + status = 'pass'
> + try:
> + client.boot_linaro_uefi_image()
> + except:
> + logging.exception("boot_linaro_image failed")
> + status = 'fail'
> + raise CriticalError("Failed to boot uefi test image.")
> + finally:
> + self.context.test_data.add_result("boot_uefi_image",status)
> +
> +### UEFI

I don't think this is needed. See my comments for the deploy logic in
master.py below.

> === modified file 'lava_dispatcher/actions/deploy.py'
> --- lava_dispatcher/actions/deploy.py 2012-11-20 13:34:19 +0000
> +++ lava_dispatcher/actions/deploy.py 2012-11-30 15:53:29 +0000
> @@ -91,6 +91,20 @@
> def run(self, boot, system, data, rootfstype='ext4'):
> self.client.deploy_linaro_android(boot, system, data, rootfstype)
>
> +## UEFI ##
> +class cmd_deploy_linaro_uefi_image(BaseAction):
> + parameters_schema = {
> + 'type': 'object',
> + 'properties' : {
> + 'hwpack': {'type':'string','optional': True},
> + 'rootfs' : {'type':'string','optional': True},
> + },
> + 'additionalProperties': False,
> + }
> +
> + def run(self, hwpack, rootfs):
> + self.client.deploy_linaro_uefi(hwpack,rootfs)
> +## UEFI ##

I don't think this is needed. See my comments for the deploy logic in
master.py below.

> === modified file 'lava_dispatcher/client/base.py'
> --- lava_dispatcher/client/base.py 2012-11-29 08:54:47 +0000
> +++ lava_dispatcher/client/base.py 2012-11-30 15:53:29 +0000
> @@ -402,6 +402,20 @@
> self.setup_proxy(TESTER_PS1_PATTERN)
> logging.info("System is in test image now")
>
> +### UEFI
> + def boot_linaro_uefi_image(self):
> + """
> + Reboot the system to the uefi test image
> + """
> + logging.info("Boot the uefi test image")
> +
> + self._boot_linaro_uefi_image()
> +
> + wait_for_prompt(self.proc,'/bin/sh', timeout=300)
> + logging.info("System is in test image now")
> +
> +### UEFI
> +

I don't think this is needed. See my comments for the deploy logic in
master.py below.

> === modified file 'lava_dispatcher/client/targetdevice.py'
> --- lava_dispatcher/client/targetdevice.py 2012-11-21 22:07:45 +0000
> +++ lava_dispatcher/client/targetdevice.py 2012-11-30 15:53:29 +0000
> @@ -50,6 +50,10 @@
> def deploy_linaro_android(self, boot, system, data, rootfstype='ext4'):
> self.target_device.deploy_android(boot, system, data)
>
> +## UEFI ##
> + def deploy_linaro_uefi(self,hwpack,rootfs):
> + self.target_device.deploy_uefi(hwpack,rootfs)
> +## UEFI ##

I don't think this is needed. See my comments for the deploy logic in
master.py below.

> def deploy_linaro(self, hwpack=None, rootfs=None, image=None,
> rootfstype='ext3', bootloader='u_boot'):
> if image is None:
> @@ -68,6 +72,11 @@
> def _boot_linaro_image(self):
> self.proc = self.target_device.power_on()
>
> +### UEFI
> + def _boot_linaro_uefi_image(self):
> + self.proc = self.target_device.uefi_power_on()
> +
> +### UEFI

I don't think this is needed. See my comments for the deploy logic in
master.py below.

> === modified file 'lava_dispatcher/config.py'
> --- lava_dispatcher/config.py 2012-11-30 01:54:29 +0000
> +++ lava_dispatcher/config.py 2012-11-30 15:53:29 +0000
> @@ -73,6 +73,7 @@
> simulator_version_command = schema.StringOption()
> simulator_command = schema.StringOption()
> simulator_axf_files = schema.ListOption()
> + boot_cmds_uefi = schema.StringOption(fatal=True) ### UEFI

This seems bad, why not just do
+ boot_cmds_uefi = schema.StringOption(default='')
Then you don't have to make a change to device-defaults.conf below.
Also - drop the "#UEFI" comments - we have revision control for a reason.

> === modified file 'lava_dispatcher/default-config/lava-dispatcher/device-defaults.conf'
> --- lava_dispatcher/default-config/lava-dispatcher/device-defaults.conf 2012-11-30 01:54:29 +0000
> +++ lava_dispatcher/default-config/lava-dispatcher/device-defaults.conf 2012-11-30 15:53:29 +0000
> @@ -34,6 +34,11 @@
> # XXX should be called # boot_oe_test_image_commands ?
> boot_cmds_oe =
>
> +
> +### UEFI
> +boot_cmds_uefi =
> +### UEFI
> +

Not needed as described above in config.py

> === modified file 'lava_dispatcher/device/fastmodel.py'

Not sure why you made changes here? This looks like a no-op. I think
they should be dropped.

> === modified file 'lava_dispatcher/device/master.py'

Now for the fun part :)

> --- lava_dispatcher/device/master.py 2012-11-30 01:57:34 +0000
> +++ lava_dispatcher/device/master.py 2012-11-30 15:53:29 +0000
> @@ -70,6 +70,7 @@
> Target.android_deployment_data['boot_cmds'] = 'boot_cmds_android'
> Target.ubuntu_deployment_data['boot_cmds'] = 'boot_cmds'
> Target.oe_deployment_data['boot_cmds'] = 'boot_cmds_oe'
> + Target.uefi_deployment_data['boot_cmds'] = 'boot_cmds_uefi'
>

might not be needed, see below

> # used for tarballcache logic to get proper boot_cmds
> Target.ubuntu_deployment_data['data_type'] = 'ubuntu'
> @@ -78,6 +79,7 @@
> 'android': Target.android_deployment_data,
> 'oe': Target.oe_deployment_data,
> 'ubuntu': Target.ubuntu_deployment_data,
> + 'uefi': Target.uefi_deployment_data,
> }

might not be needed, see below

> @@ -92,6 +94,12 @@
> self._boot_linaro_image()
> return self.proc
>
> +### UEFI
> + def uefi_power_on(self):
> + self._boot_linaro_uefi_image()
> + return self.proc
> +### UEFI
> +

Not needed see below

> @@ -104,6 +112,97 @@
>
> self._deploy_tarballs(boot_tgz, root_tgz)
>
> +## UEFI ##
> + def deploy_uefi(self,hwpack,rootfs):
> + logging.info("[UEFI master.py: deploy_uefi]: %s" % hwpack)
> + logging.info("[UEFI master.py: deploy_uefi]: %s" % rootfs)
> + self.boot_master_image()
> +
> + self._deploy_uefi_tarballs(hwpack,rootfs)

I don't think you need this function or the ones it calls. These deploy
functions, they look identical to the current, _deploy_linaro_bootfs and
_deploy_linaro_rootfs functions we already have in the module. On top of
that, I don't think you even need your own deploy function. You should
be able to use the current deploy_linaro and not even need a new action
defined for UEFI. All you are really doing differently is setting:

  self.deployment_data = Target.uefi_deployment_data

This gets set by our currently deployment logic in:

  lava_dispatcher.device.target:Target._customize_linux

I think you could patch this function to look at boot directory - if it
has UEFI, you could then set deployment_data to use your UEFI boot commands.

I could see a case where you might have an image with both and want to
be able to specify which bootloader to use. In that case we already have
a mechanism for you:

  #for your job's json file:
  {
           "command": "boot_linaro_image",
   "parameters": {
    "options": ["boot_cmds=boot_cmds_uefi"]
   }
  },