Merge lp:~dpigott/lava-dispatcher/add-bootloader-support into lp:lava-dispatcher

[Dave Pigott]

Added in an option to specify a bootloader in the json file. If not specified, it defaults to 'u_boot'

[Fathi Boudra (fboudra)]

It's worth mentioning that 'u_boot' is just a string and can be anything.
You can use linaro-media-create --read-hwpack to get the supported bootloaders and pass a valid string.

[Fathi Boudra (fboudra)]

As noted on https://bugs.launchpad.net/lava-dispatcher/+bug/1057646/comments/5

[Dave Pigott (dpigott)]

I know, and I thought long and hard about which was the right way to go. At the end of the day, the solution was much more flexible this way, since you can also get a return such as:

+--------------------------------------+--------------------------------------+
Supported boards | Supported bootloaders
+--------------------------------------+--------------------------------------+
linaro-vexpress | uefi,u_boot
+--------------------------------------+--------------------------------------+

It would not be possible to decide which one to pick.

This way, the person or process creating and submitting jobs is responsible for picking the right one, and if new ones get added, then those just get done as well.

There will probably be some other work to do in the way the lava dispatcher handles things when interrupting boot, but that's not a problem when sd-mux arrives.

[Dave Pigott (dpigott)]

And actually, since it is indeed a free format string, then if somebody decides to call u_boot "neddy-the-bootloader" we have no way of knowing what sort of bootloader it really is. I guess what I'm advocating is that we should standardise on a naming convention in the hwpacks. Otherwise we are asking for a lot of trouble.

Another key point is that if the dispatcher made a decision about the bootloader, then that goes against someone explicitly trying to specify and test a particular bootloader when we get to that point.

[Andy Doan (doanac)]

seems okay. In general I'm a little uncomfortable with inserting a function parameter rather than appending it to the list. However, it looks like you grep'd through code and fixed all the references where this was affected.

[Andy Doan (doanac)]

On 11/20/2012 07:47 AM, Fathi Boudra wrote:
> It's worth mentioning that 'u_boot' is just a string and can be anything.
> You can use linaro-media-create --read-hwpack to get the supported bootloaders and pass a valid string.

Yes, but that should only matter if we are auto-guessing for the user,
right? Can we just get away with requiring users to provide the
bootloader they want in the event its not u-boot?

[Fathi Boudra (fboudra)]

On 20 November 2012 17:48, Andy Doan wrote:
> On 11/20/2012 07:47 AM, Fathi Boudra wrote:
>>
>> It's worth mentioning that 'u_boot' is just a string and can be anything.
>> You can use linaro-media-create --read-hwpack to get the supported
>> bootloaders and pass a valid string.
>
> Yes, but that should only matter if we are auto-guessing for the user,
> right? Can we just get away with requiring users to provide the bootloader
> they want in the event its not u-boot?

You're already guessing that the default bootloader is U-Boot and its
label is called 'u_boot'.
I don't think you have to guess as the hwpack provides the default
bootloader and also the label.
No interaction with the user here.

[Fathi Boudra (fboudra)]

On 20 November 2012 16:24, Dave Pigott wrote:
> I know, and I thought long and hard about which was the right way to go. At the end of the day, the solution was much more flexible this way, since you can also get a return such as:
>
> +--------------------------------------+--------------------------------------+
> Supported boards | Supported bootloaders
> +--------------------------------------+--------------------------------------+
> linaro-vexpress | uefi,u_boot
> +--------------------------------------+--------------------------------------+
>
> It would not be possible to decide which one to pick.

According to the output, default bootloader is UEFI and the label is 'uefi'.

[Dave Pigott (dpigott)]

Yeah, I checked thoroughly, because I prefer to have function parameters in a logical sequence.

Sent from my Aldis Lamp

On 20 Nov 2012, at 15:46, Andy Doan <email address hidden> wrote:

> seems okay. In general I'm a little uncomfortable with inserting a function parameter rather than appending it to the list. However, it looks like you grep'd through code and fixed all the references where this was affected.
> --
> https://code.launchpad.net/~dpigott/lava-dispatcher/add-bootloader-support/+merge/135151
> You are the owner of lp:~dpigott/lava-dispatcher/add-bootloader-support.

[Dave Pigott (dpigott)]

Ok. So what you're saying is that to support this, we need to accept a selection in the JSON file, and then use the read-hwpack option to determine if it's valid. If nothing is specified we use the first in the list for the matching device type.

That's probably enough to push it into backlog, to be honest. If my change is unsupportable, then let's just throw it away,

On 20 Nov 2012, at 16:01, Fathi Boudra <email address hidden> wrote:

> On 20 November 2012 17:48, Andy Doan wrote:
>> On 11/20/2012 07:47 AM, Fathi Boudra wrote:
>>>
>>> It's worth mentioning that 'u_boot' is just a string and can be anything.
>>> You can use linaro-media-create --read-hwpack to get the supported
>>> bootloaders and pass a valid string.
>>
>> Yes, but that should only matter if we are auto-guessing for the user,
>> right? Can we just get away with requiring users to provide the bootloader
>> they want in the event its not u-boot?
>
> You're already guessing that the default bootloader is U-Boot and its
> label is called 'u_boot'.
> I don't think you have to guess as the hwpack provides the default
> bootloader and also the label.
> No interaction with the user here.
>
> --
> https://code.launchpad.net/~dpigott/lava-dispatcher/add-bootloader-support/+merge/135151
> You are the owner of lp:~dpigott/lava-dispatcher/add-bootloader-support.

[Andy Doan (doanac)]

On 11/20/2012 10:58 AM, Dave Pigott wrote:
> Ok. So what you're saying is that to support this, we need to accept a selection in the JSON file, and then use the read-hwpack option to determine if it's valid. If nothing is specified we use the first in the list for the matching device type.
>
> That's probably enough to push it into backlog, to be honest. If my change is unsupportable, then let's just throw it away,

Fathi and I discussed on IRC. I think what you have now is good. I don't
think we need to do validation of the bootloader parameter. we'll put
the responsibility on the user for now. In the, likely near, future we
can do Fathi's suggesion and validated (or automatically choose) based
on this l-m-c command.

[Dave Pigott (dpigott)]

On 20 Nov 2012, at 17:05, Andy Doan <email address hidden> wrote:

> On 11/20/2012 10:58 AM, Dave Pigott wrote:
>> Ok. So what you're saying is that to support this, we need to accept a selection in the JSON file, and then use the read-hwpack option to determine if it's valid. If nothing is specified we use the first in the list for the matching device type.
>>
>> That's probably enough to push it into backlog, to be honest. If my change is unsupportable, then let's just throw it away,
>
> Fathi and I discussed on IRC. I think what you have now is good. I don't think we need to do validation of the bootloader parameter. we'll put the responsibility on the user for now. In the, likely near, future we can do Fathi's suggesion and validated (or automatically choose) based on this l-m-c command.

Cool.

[Dave Pigott (dpigott)]

On 20 Nov 2012, at 17:07, Andy Doan <email address hidden> wrote:

> On 11/20/2012 10:58 AM, Dave Pigott wrote:
>> Ok. So what you're saying is that to support this, we need to accept a selection in the JSON file, and then use the read-hwpack option to determine if it's valid. If nothing is specified we use the first in the list for the matching device type.
>>
>> That's probably enough to push it into backlog, to be honest. If my change is unsupportable, then let's just throw it away,
>
> Fathi and I discussed on IRC. I think what you have now is good. I don't
> think we need to do validation of the bootloader parameter. we'll put
> the responsibility on the user for now. In the, likely near, future we
> Efaucan do Fathi's suggesion and validated (or automatically choose) based
> on this l-m-c command.
>

Actually, the sensible behaviour is for l-m-c to, by default, use the default boot loader if none is specified, since they have to inspect the hwpack anyway. Then they'll immediately fail if the requested one doesn't exist.

[Andy Doan (doanac)]

On 11/20/2012 11:16 AM, Dave Pigott wrote:
> Actually, the sensible behaviour is for l-m-c to, by default, use the default boot loader if none is specified, since they have to inspect the hwpack anyway. Then they'll immediately fail if the requested one doesn't exist.

yeah, that's what I was trying to say. sorry I wasn't clear.

[Dave Pigott (dpigott)]

review: Approve

=== modified file 'lava_dispatcher/actions/deploy.py'
--- lava_dispatcher/actions/deploy.py	2012-10-24 01:34:07 +0000
+++ lava_dispatcher/actions/deploy.py	2012-11-20 13:38:19 +0000
@@ -54,6 +54,7 @@
             'rootfs': {'type': 'string', 'optional': True},
             'image': {'type': 'string', 'optional': True},
             'rootfstype': {'type': 'string', 'optional': True},
+            'bootloader': {'type': 'string', 'optional': True, 'default': 'u_boot'},
             },
         'additionalProperties': False,
         }
@@ -69,9 +70,9 @@
         elif 'image' not in parameters:
             raise ValueError('must specify image if not specifying a hwpack')
 
-    def run(self, hwpack=None, rootfs=None, image=None, rootfstype='ext3'):
+    def run(self, hwpack=None, rootfs=None, image=None, rootfstype='ext3', bootloader='u_boot'):
         self.client.deploy_linaro(
-            hwpack=hwpack, rootfs=rootfs, image=image, rootfstype=rootfstype)
+            hwpack=hwpack, rootfs=rootfs, image=image, rootfstype=rootfstype, bootloader=bootloader)
 
 
 class cmd_deploy_linaro_android_image(BaseAction):
 
=== modified file 'lava_dispatcher/client/lmc_utils.py'
--- lava_dispatcher/client/lmc_utils.py	2012-09-27 21:59:57 +0000
+++ lava_dispatcher/client/lmc_utils.py	2012-11-20 13:38:19 +0000
@@ -15,7 +15,7 @@
     )
 
 
-def generate_image(client, hwpack_url, rootfs_url, outdir, rootfstype=None):
+def generate_image(client, hwpack_url, rootfs_url, outdir, bootloader='u_boot', rootfstype=None):
     """Generate image from a hwpack and rootfs url
 
     :param hwpack_url: url of the Linaro hwpack to download
@@ -43,8 +43,8 @@
     logging.info("client.device_type = %s" %client.config.device_type)
 
     cmd = ("sudo flock /var/lock/lava-lmc.lck linaro-media-create --hwpack-force-yes --dev %s "
-           "--image-file %s --binary %s --hwpack %s --image-size 3G" %
-           (client.config.lmc_dev_arg, image_file, rootfs_path, hwpack_path))
+           "--image-file %s --binary %s --hwpack %s --image-size 3G --bootloader %s" %
+           (client.config.lmc_dev_arg, image_file, rootfs_path, hwpack_path, bootloader))
     if rootfstype is not None:
         cmd += ' --rootfs ' + rootfstype
     logging.info("Executing the linaro-media-create command")
@@ -53,11 +53,11 @@
     _run_linaro_media_create(cmd)
     return image_file
 
-def generate_fastmodel_image(hwpack, rootfs, odir, size="2000M"):
+def generate_fastmodel_image(hwpack, rootfs, odir, bootloader='u_boot', size="2000M"):
     cmd = ("flock /var/lock/lava-lmc.lck sudo linaro-media-create "
            "--dev fastmodel --output-directory %s --image-size %s "
-           "--hwpack %s --binary %s --hwpack-force-yes" %
-            (odir, size, hwpack, rootfs) )
+           "--hwpack %s --binary %s --hwpack-force-yes --bootloader %s" %
+            (odir, size, hwpack, rootfs, bootloader) )
     logging.info("Generating fastmodel image with: %s" % cmd)
     _run_linaro_media_create(cmd)
 
 
=== modified file 'lava_dispatcher/client/targetdevice.py'
--- lava_dispatcher/client/targetdevice.py	2012-10-26 06:22:05 +0000
+++ lava_dispatcher/client/targetdevice.py	2012-11-20 13:38:19 +0000
@@ -50,7 +50,7 @@
         self.target_device.deploy_android(boot, system, data)
 
     def deploy_linaro(self, hwpack=None, rootfs=None, image=None,
-                      rootfstype='ext3'):
+                      rootfstype='ext3', bootloader='u_boot'):
         if image is None:
             if hwpack is None or rootfs is None:
                 raise CriticalError(
@@ -60,7 +60,7 @@
                     "cannot specify hwpack or rootfs when specifying image")
 
         if image is None:
-            self.target_device.deploy_linaro(hwpack, rootfs)
+            self.target_device.deploy_linaro(hwpack, rootfs, bootloader)
         else:
             self.target_device.deploy_linaro_prebuilt(image)
 
 
=== modified file 'lava_dispatcher/device/fastmodel.py'
--- lava_dispatcher/device/fastmodel.py	2012-11-16 00:47:20 +0000
+++ lava_dispatcher/device/fastmodel.py	2012-11-20 13:38:19 +0000
@@ -113,12 +113,12 @@
 
         self._customize_android()
 
-    def deploy_linaro(self, hwpack=None, rootfs=None):
+    def deploy_linaro(self, hwpack=None, rootfs=None, bootloader='u_boot'):
         hwpack = download_image(hwpack, self.context, decompress=False)
         rootfs = download_image(rootfs, self.context, decompress=False)
         odir = os.path.dirname(rootfs)
 
-        generate_fastmodel_image(hwpack, rootfs, odir)
+        generate_fastmodel_image(hwpack, rootfs, odir, bootloader)
         self._sd_image = '%s/sd.img' % odir
         self._axf = None
         for f in self.config.simulator_axf_files:
 
=== modified file 'lava_dispatcher/device/master.py'
--- lava_dispatcher/device/master.py	2012-11-16 00:44:40 +0000
+++ lava_dispatcher/device/master.py	2012-11-20 13:38:19 +0000
@@ -94,10 +94,10 @@
         # we always leave master image devices powered on
         pass
 
-    def deploy_linaro(self, hwpack, rfs):
+    def deploy_linaro(self, hwpack, rfs, bootloader):
         self.boot_master_image()
 
-        image_file = generate_image(self, hwpack, rfs, self.scratch_dir)
+        image_file = generate_image(self, hwpack, rfs, self.scratch_dir, bootloader)
         (boot_tgz, root_tgz, data) = self._generate_tarballs(image_file)
 
         self._deploy_tarballs(boot_tgz, root_tgz)
 
=== modified file 'lava_dispatcher/device/qemu.py'
--- lava_dispatcher/device/qemu.py	2012-11-15 21:23:20 +0000
+++ lava_dispatcher/device/qemu.py	2012-11-20 13:38:19 +0000
@@ -46,9 +46,9 @@
         super(QEMUTarget, self).__init__(context, config)
         self._sd_image = None
 
-    def deploy_linaro(self, hwpack=None, rootfs=None):
+    def deploy_linaro(self, hwpack=None, rootfs=None, bootloader='u_boot'):
         odir = self.scratch_dir
-        self._sd_image = generate_image(self, hwpack, rootfs, odir)
+        self._sd_image = generate_image(self, hwpack, rootfs, odir, bootloader)
         self._customize_linux(self._sd_image)
 
     def deploy_linaro_prebuilt(self, image):