856123 - Self test second stage script should verify preseed file components

Status: RESOLVED FIXED

(Testing Graveyard :: Mozpool, defect)

Description

[Jake Watkins [:dividehex]]

This can be done by generating hashes of the file components at the time the preseed blob is generated and placing those hashes into a json which would be pulled from the imaging (mozpool) server via http similar to way b2g images are fetched.

This would not only allow us to be sure the preseed is intact and current but would also allow us to deploy new preseed components without manually swapping out sdcards.

Comment 1

[Dustin J. Mitchell [:dustin] (he/him)]

I'm hesitant to have self-test automatically *repair* the preseed files, but checking them makes LOTS of sense.

I think checking against a JSON file is a good solution.  That JSON file could be generated by puppet whenever the preseed changes, using a notify and an exec.  Or it could be rebuilt using a script run manually - it won't change that often!

Comment 2

[Jake Watkins [:dividehex]]

(In reply to Dustin J. Mitchell [:dustin] from comment #1)
> I'm hesitant to have self-test automatically *repair* the preseed files, but
> checking them makes LOTS of sense.

I understand the hesitation.  Repair/deploy can be implemented in a separate Please_ action sometime later.  We'll focus on simple checks and logging.

> I think checking against a JSON file is a good solution.  That JSON file
> could be generated by puppet whenever the preseed changes, using a notify
> and an exec.  Or it could be rebuilt using a script run manually - it won't
> change that often!

Either way seems okay to me also.  I'll see the winds blow me. :}

Comment 3

[Jake Watkins [:dividehex]]

Attachment: better selftest

This is a replacement for the current selftest.sh.  It currently contains 4 test cases but additional test cases written can be added by inheriting the class MozpoolSelftest and adding a new configuration dict to the json config file.

This is a sample json for the current pandas

{
    "config": {
        "hardware_type": "Panda ES"
    }, 
    "test_mmc_blk_dev": {
        "mmc_device_list": [
            "mmcblk0", 
            "mmcblk0p1", 
            "mmcblk0p2", 
            "mmcblk0p3", 
            "mmcblk0p4", 
            "mmcblk0p5", 
            "mmcblk0p6"
        ], 
        "priority": 1, 
        "verbose": true
    }, 
    "test_mmc_register": {
        "kernel_sys_mmc_path": "/sys/class/mmc_host/mmc0/mmc0:e624", 
        "priority": 2, 
        "verbose": true
    }, 
    "test_preseed_file_integrity": {
        "boot_partition": "mmcblk0p1", 
        "file_hash_dict": {
            "MLO": "768ba483e7664efc4280c4d5aa2834f26a80d8f5", 
            "boot.scr": "6261fdd19a45db13e6503c5010e3917dbb13eeed", 
            "boot.txt": "1c40f7329f46813bdf74f6f7d71a561e2bd48149", 
            "omap4-panda.dtb": "a91539cc6f656a4b9847bb7cd086c343022aef10", 
            "u-boot.img": "31f3daf95ff8e59f086ae326ec62a3b0664ef9c4"
        }, 
        "priority": 2, 
        "verbose": true
    }, 
    "test_proc_partition_hash": {
        "file_hash": "bdcdaf582e144be3af7535a2f1d93611f515d0ed", 
        "priority": 2, 
        "verbose": true
    }
}

Comment 4

[Jake Watkins [:dividehex]]

Attachment: bug799102-selftest.patch

sorry. meant to put that in as patch form

Comment 5

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 799108 [details] [diff] [review]
bug799102-selftest.patch

Review of attachment 799108 [details] [diff] [review]:
-----------------------------------------------------------------

I like the structure, and testing things more deeply.  All of these comments are pretty minor.  There's also some trailing whitespace, which Bugzilla helpfully highlights in magenta.

::: scripts/selftest/selftest.py
@@ +18,5 @@
> +
> +# grab device id
> +device_id = gethostname()
> +
> +def parser_cmdline():

itym `parse_cmdline`?

@@ +71,5 @@
> +
> +class TestConfigError(Exception):
> +    """Failed to configure test"""
> +
> +class diag_utils(object):

classes should be named with initial caps (with the test cases being a notable exception)

@@ +92,5 @@
> +    testname = 'None'
> +    config_priority = 0
> +    config_verbose = False
> +
> +    def __init__(self):

Since all subclasses have the config passed to their constructor, this constructor should take that argument, too.  This may mean that subclasses don't even need their own constructors..

@@ +117,5 @@
> +        raise TestFailureError
> +    
> +    def _test_info(self, *args, **kwarg):
> +        log_str = '{0}[INFO] {1}'.format(self.testname, args[0])
> +        self._log(log_str)

Why do these take *args/**kwarg and ignore all but args[0]?

@@ +136,5 @@
> +    This test checks that the mmc driver has created the mmc block device file
> +    In some instances, the panda board rom successfully reads the sd card to
> +    exec the boot loader but the kernel fails to recognize it
> +    """
> +    # Each test must have a unique name defined

This comment and that for expected_config_keys should probably be in the parent class.

@@ +255,5 @@
> +        mount_point = self._mount(self.config_boot_partition)
> +        for file_name in self.config_file_hash_dict:
> +            try:
> +                sha1_digest = self.utils.hash_file(os.path.join(mount_point, file_name))
> +            except TestFailureError:

shouldn't this be IOError or OSError?

Comment 6

[Jake Watkins [:dividehex]]

(In reply to Dustin J. Mitchell [:dustin] from comment #5)


> itym `parse_cmdline`?

You are correct. I did mean parse_cmdline
 
> Since all subclasses have the config passed to their constructor, this
> constructor should take that argument, too.  This may mean that subclasses
> don't even need their own constructors..

At some point I had test specific calls in __init__ but those were removed.
And so I'll also remove the subclass constructor altogether

> @@ +117,5 @@
> > +        raise TestFailureError
> > +    
> > +    def _test_info(self, *args, **kwarg):
> > +        log_str = '{0}[INFO] {1}'.format(self.testname, args[0])
> > +        self._log(log_str)
> 
> Why do these take *args/**kwarg and ignore all but args[0]?

This really shouldn't be passing *args/**kwargs and should just be a string arg

> 
> This comment and that for expected_config_keys should probably be in the
> parent class.

Yep. I'll move them to the parent

> 
> @@ +255,5 @@
> > +        mount_point = self._mount(self.config_boot_partition)
> > +        for file_name in self.config_file_hash_dict:
> > +            try:
> > +                sha1_digest = self.utils.hash_file(os.path.join(mount_point, file_name))
> > +            except TestFailureError:
> 
> shouldn't this be IOError or OSError?

Correct, I meant for that to be IOError.

Comment 7

[Jake Watkins [:dividehex]]

Attachment: add new selftest to bmm puppet module

No code changes; just puppet patch to deploy via bmm module

Comment 8

[Jake Watkins [:dividehex]]

Attachment: bug856123-1.patch

- fixes test_proc_partition_hash to glob for mmc0 device filename
- fixes _test_failed args in test_mmc_blk_dev
- in test_mmc_register, external command calls (mount, umount) improved and refactored

Comment 9

[Jake Watkins [:dividehex]]

Ignore the trailing whitespaces.  I forgot to sweep for them before posting

Comment 10

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 801062 [details] [diff] [review]
bug856123-1.patch

Review of attachment 801062 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/bmm/templates/selftest-second-stage.py.erb
@@ +219,5 @@
> +        
> +        # Since the mmc0 path contains a dynamiclly generated filename, we
> +        # glob and return first element. For pandaboards, only one filename
> +        # will ever be generated by the kernel.
> +        kernel_sys_mmc_path = glob.glob(self.config_kernel_sys_mmc_path)[0]

Will this give a useful error if there are zero matches, e.g., if the kernel just doesn't load the driver?

Comment 11

[Jake Watkins [:dividehex]]

(In reply to Dustin J. Mitchell [:dustin] from comment #10)
> Comment on attachment 801062 [details] [diff] [review]
> bug856123-1.patch
> 
> Review of attachment 801062 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: modules/bmm/templates/selftest-second-stage.py.erb
> @@ +219,5 @@
> > +        
> > +        # Since the mmc0 path contains a dynamiclly generated filename, we
> > +        # glob and return first element. For pandaboards, only one filename
> > +        # will ever be generated by the kernel.
> > +        kernel_sys_mmc_path = glob.glob(self.config_kernel_sys_mmc_path)[0]
> 
> Will this give a useful error if there are zero matches, e.g., if the kernel
> just doesn't load the driver?

In the case of the kernel not loading the mmc driver, the test_mmc_blk_dev would catch that first since it has a higher priority and execs before this test. But you are right. This should fail with a usable msg in any case.

try:
    kernel_sys_mmc_path = glob.glob(self.config_kernel_sys_mmc_path)[0]
except IndexError:
    self._test_failed(
        self.config_kernel_sys_mmc_path + ' no filename match found')

Comment 12

[Dustin J. Mitchell [:dustin] (he/him)]

cool - good to commit with that fix :)

Comment 13

[Jake Watkins [:dividehex]]

This was committed.  I'm double checking some of the pandas that had previously failed due to getting hung up on the umount call and it looks like they are passing now.  Yay!

Comment 14

[Jake Watkins [:dividehex]]

I'm calling this done!  It is in production and has been included in the mozpool source repo.  I'll be working on updating the Mozpool documentation over the next week or two since it has become a bit stale over time.