LAVA development

Before you start, ensure you’ve read:

Run the unit tests

Extra dependencies are required to run the tests. On Debian based distributions, you can install lava-dev.

To run the tests, use the ci-run script:

$ ./ci-run

There is never any need to use sudo for unit tests, it causes lots of complications by changing file permissions in your local git clone.

Static code analysis

It is essential to run pep8 --ignore E501 routinely on your local changes as ./ci-run will fail on any PEP8 errors. All automated tests occur using Debian Stretch.

It is important to run tools like pylint3, particularly when adding new files, to check for missing or unused imports. Other analysis tools should also be used, for example from within your IDE.

Functional testing

Unit tests cannot replicate all tests required on LAVA code, some tests will need to be run with real devices under test. On Debian based distributions, see Developer package build. See Writing Tests for information on writing LAVA test jobs to test particular device functionality.

Make your changes

  • Follow PEP8 style for Python code.
  • Use one topic branch for each logical change.
  • Include new unit tests in the proposed merge request.
  • Write good commit messages.
    • Describe why the change was made, not what files were changed. The commit message should reflect your intention, not the contents of the commit.
    • Avoid putting documentation into the commit message. Keep the commit message to a reasonable length (about 10 to 12 lines at most). Include changes to the existing documentation in your commit.
    • Usage examples need to go into the documentation, not the commit message. Everything which is intended to help users to add this support to their own test jobs must be in the documentation.
    • Avoid duplicating or summarising the documentation in the commit message, reviewers will be reading the documentation as well.
    • Use comments in the code in preference to detailed commit messages.

Blackening the source code

LAVA is adopting black as the formatter for LAVA source code, in a gradual manner. black is only available for Debian Buster and later - it is not suitable for backporting to Stretch. However, Buster is starting the release freeze process ready for a release in Q2 2019. If you are running Buster on your development machine already, or have Buster packages available to you, you can install black using apt as usual. You can also choose to install black from https://github.com/ambv/black if it is not available for your OS.

Initially, black is to only be applied to:

When changing files with black:

  1. Avoid changing a file with black if the file is already part of an existing merge request. Formatting changes must be secondary to functional changes.
  2. The objective is not to run black on lots of files at once, that just leads to large and noisy merge requests and churn. Change at most two or three files at a time. (This advice can be relaxed for unit test files as simply running the unit tests in the CI is sufficient to show that the formatting change has not affected functionality. Even then, avoid changing more than a few hundred lines per merge request.)
  3. When changing files with black, add the filename to the tracking file in share/black.list. In time, this will be used to keep those formatting changes when the file is touched again with a merge request.

Add some unit tests

Some changes will always need additional unit tests and reviews will not be merged without this support. The purpose is to ensure that future changes in the codebase have some assurance that existing support has not been affected. The intent is that as much as possible of the test job and device configuration is covered by at least one unit test. Some examples include:

  1. Changes to an existing jinja2 device-type template which change the output YAML of the device configuration need a unit test to show that the change is being included.
  2. Adding a new deployment or boot method needs unit tests (including sample test jobs) which check that all validate() functions work correctly and particular tests checking for the specific details of the new method.
  3. Adding a change to an existing deployment or boot method which changes the construction of the pipeline based on test job or device configuration. Unit tests will be required to show that the change is being made.

Reviewers may ask for unit test support for any change, so talk to us during development. You can also use an WIP: prefix in your git commit to indicate that the change is not ready for merging but is ready for comments.

lava_dispatcher

Whenever new functionality is added to lava_dispatcher, especially a new Strategy class, there must be some new unit tests added to allow some assurance that the new classes continue to operate as expected as the rest of the codebase continues to develop. There are a lot of examples in the current unit tests.

  1. Start with a sample test job which is known to work. Copy that into lava_dispatcher/tests/sample_jobs. The URLs in that sample job will need to be valid URLs but do not need to be working files. (This sample_job is not being submitted to run on a device, it is only being used to check that the construction of the pipeline is valid.) If you need files which other sample jobs do not use then we can help with that by putting files onto images.validation.linaro.org.

  2. Use the updated Factory support to generate the device configuration directly from the lava_scheduler_app templates.

    If a suitable device dictionary does not already exist in lava_scheduler_app/tests/devices, a new one can be added to support the unit tests.

  3. Add a function to a suitable Factory class to use the device config file to create a device and use the parser to create a Job instance by following the examples in the existing unit tests

  4. Create the pipeline ref by following the readme.txt in the pipeline_ref directory. The simplest way to create a single new pipeline reference file is to add one line to the new unit test function:

    self.update_ref = True
    

    Run the unit test and the pipeline reference will be created. Remove the line before committing for review or the ./ci-run check will fail.

    This file acts as a description of the classes involved in the pipeline which has been constructed from the supplied test job and device configuration. Validating it in the unit tests ensures that later development does not invalidate the new code by accidentally removing or adding unexpected actions.

  5. In the new function, use the pipeline_refs README to add a check that the pipeline reference continues to reflect the pipeline which has been constructed by the parser.

Note

unit tests do not typically check any of the run function code. Do as much checking as is practical in the validate functions of all the new classes. For example, if run relies on a parameter being set, check for that parameter in validate and check that the value of that parameter is correct based on the sample job and the supplied device configuration.

lava_scheduler_app

Some parts of lava_scheduler_app are easier to test than others. New device-type templates need to have specific unit tests added to lava_scheduler_app/tests/test_templates or one of the relevant specialist template unit test files. Follow the examples and make sure that if the new template adds new items then those items are checked for existence and validity in the new function which tests the new template.

$ python3 -m unittest -vcf lava_scheduler_app.tests.test_fastboot_templates

$ python3 -m unittest -vcf lava_scheduler_app.tests.test_qemu_templates

$ python3 -m unittest -vcf lava_scheduler_app.tests.test_uboot_templates

If you are adding or modifying documentation in lava-server, make sure that the documentation builds cleanly:

$ make -C doc/v2 clean
$ make -C doc/v2 html

For other parts of lava-server, follow the examples of the existing unit tests and talk to us.

Re-run the unit tests

Make sure that your changes do not cause any failures in the unit tests:

$ ./ci-run

Wherever possible, always add new unit tests for new code.

Testing local changes

For any sufficiently large change, building and installing a new package on a local instance is recommended. Ensure that the test instance is already running the most recent production release.

If the test instance has a separate worker, ensure that the master and the worker always have precisely the same code applied. For some changes, it may be necessary to have a test instance which is a clone of a production instance, complete with devices. Never make live changes to a production instance. (This is why integrating new device types into LAVA requires multiple devices.)

Once your change is working successfully:

  1. Ensure that your local branch is clean - check for left over debug code.
  2. Ensure that your local branch has been rebased against current master
  3. Build and install a package from the master branch. If you have added any new files in your local change, make sure these have been removed. Reproduce the original bug or problem.
  4. Build and install a package from your local branch and repeat the tests.

lava_dispatcher

Changes to most files in lava_dispatcher can be symlinked or copied into the packaged locations. e.g.:

$ PYTHONDIR=/usr/lib/python3/dist-packages/
$ sudo cp <path_to_file> $PYTHONDIR/<path_to_file>

Note

The path used for PYTHONDIR has changed with the LAVA runtime support moving to Python3 in 2018.4.

There is no need to copy files used solely by the unit tests.

Changes to files in ./etc/ will require restarting the relevant service.

Changes to files in ./lava/dispatcher/ will need the lava-slave service to be restarted but changes to ./lava_dispatcher/ will not.

  • When adding or modifying run, validate, populate or cleanup functions, always ensure that super is called appropriately, for example:

    super().validate()
    
    connection = super().run(connection, max_end_time)
    
  • When adding or modifying run functions in subclasses of Action, always ensure that each return point out of the run function returns the connection object:

    return connection
    
  • When adding new classes, use hyphens, -, as separators in self.name, not underscores, _. The function will fail if underscore or whitespace is used. Action names need to all be lowercase and describe something about what the action does at runtime. More information then needs to be added to the self.summary and an extended sentence in self.description.

    self.name = 'do-something-at-runtime'
    
  • Use namespaces for all dynamic data. Parameters of actions are immutable. Use the namespace functions when an action needs to store dynamic data, for example the location of files which have been downloaded to temporary directories, Do not access self.data directly (except for use in iterators). Use the get and set primitives, for example:

    set_namespace_data(action='boot', label='shared', key='boot-result', value=res)
    
    image_arg = self.get_namespace_data(action='download-action', label=label, key='image_arg')
    

lava-server

Changes to device-type templates and device dictionaries take effect immediately, so simply submitting a test job will pick up the latest version of the code in /etc/lava-server/dispatcher-config/device-types/. Make changes to the templates in lava_scheduler_app/tests/device-types/. Check them using the test_all_templates test, and only then copy the updates into /etc/lava-server/dispatcher-config/device-types/ when the tests pass.

Changes to django templates can be applied immediately by copying the template into the packaged path, e.g. html files in lava_scheduler_app/templates/lava_scheduler_app/ can be copied or symlinked to /usr/lib/python3/dist-packages/lava_scheduler_app/templates/lava_scheduler_app/

Note

The path changed when the LAVA runtime support moved to Python3 with the 2018.4 release.

Changes to python code generally require copying the files and restarting the lava-server-gunicorn service before the changes will be applied:

sudo service lava-server-gunicorn restart

Changes to lava_scheduler_app/models.py, lava_scheduler_app/db_utils.py or lava_results_app/dbutils will require restarting the lava-master service:

sudo service lava-master restart

Changes to files in ./etc/ will require restarting the relevant service. If multiple services are affected, it is normally best to build and install a new package.

Database migrations are a complex area - read up on the django documentation for migrations. Instead of python ./manage.py, use sudo lava-server manage.

lava-server-doc

Documentation files in doc/v2 can be built locally in the git checkout using make:

make -C doc/v2 clean
make -C doc/v2 html

Files can then be checked in a web browser using the file:// url scheme and the _build/html/ subdirectory. For example: file:///home/neil/code/lava/lava-server/doc/v2/_build/html/first_steps.html

Some documentation changes can add images, example test jobs, test definitions and other files. Depending on the type of file, it may be necessary to make changes to the packaging, so talk to us before making such changes.

Documentation is written in RST, so the RST Primer is essential reading when modifying the documentation.

  1. Keep all documentation paragraphs wrapped to 80 columns.
  2. Strip trailing whitespace from all modified files.
  3. When you build your changes from clean, make sure there are no warning or error messages from the build.
  4. Use en_GB unless referring to elements of code which use en_US.
  5. Use syntax highlighting for code and check the rendered page. For example, code-block:: shell relates to the contents of shell scripts, not the output of commands or scripts in a shell (those should use code-block:: none)
  6. Wherever possible, pull in code samples from working example files so that these can be checked for accuracy on staging before future releases.

Send your commits for review

From each topic branch, just run:

git push
  1. each merge request is reviewed and approved individually and
  2. later commits will depend on earlier commits, so if a later commit is approved and the one before it is not, the later commit will not be merged until the earlier one is approved.
  3. you are responsible for rebasing your branch(es) against updates on master and this can become more difficult when there are multiple commits on one local branch.
  4. Fixes from comments or unit test failures in one review are not acceptable as separate merge requests.
  5. It is common for merge requests to go through repeated cycles of comments and updates. This is not a reflection on the usefulness of the change or on any particular contributors, it is a natural evolution of the code. Comments may reflect changes being made in other parallel reviews or reviews merged whilst this change was being reviewed. Contributors may be added to other reviews where the team consider this to be useful for feedback or where the documentation is being updated in areas which relate to your change. The number of comments per review is no indication of the quality of that review and does not affect when the review would be merged.
  6. It is common for changes to develop merge conflicts during the review process as other reviews are merged. You are responsible for fixing all merge conflicts in your merge requests.
  7. All merge requests must pass all CI tests.

Therefore the recommendations are:

  1. Always use a separate local branch per change and a new commit for changes on that branch each time branch gets pushed until it is merged.
  2. Think carefully about whether to base one local branch on another local branch. This is recommended when one change logically extends an earlier change and makes it a lot easier than having multiple commits on a single branch.
  3. Keep all your branches up to date with master regularly. It is much better to resolve merge conflicts one change at a time instead of having multiple merge commits all in the one rebase operation when the merge request is finally ready to be merged. GitLab will show a message if a rebase is required but you can also simply rebase your local branch before pushing any new changes.
  4. Check gitlab periodically and ensure that you address all comments on the review.

Adding reviewers

The lava group is automatically added as approver for every merge request.

Optionally, you can put WIP: at the start of your git commit message and then amend the message when the request is ready to merge.

Handling your local branches

After placing a few reviews, there will be a number of local branches. To keep the list of local branches under control, the local branches can be easily deleted after the merge. Note: git will warn if the branch has not already been merged when used with the lower case -d option. This is a useful check that you are deleting a merged branch and not an unmerged one, so work with git to help your workflow.

$ git checkout bugfix
$ git rebase master
$ git checkout master
$ git branch -d bugfix

If the final command fails, check the status of the review of the branch. If you are completely sure the branch should still be deleted or if the review of this branch was abandoned, use the -D option instead of -d and repeat the command.

Future proofing

All developers are encouraged to write code with future changes in mind, so that it is easy to do a technology upgrade. This includes watching for errors and warnings generated by dependency packages, as well as upgrading and migrating to newer APIs as a normal part of development.

This is particularly true for Django where the lava-server package needs to retain support for multiple django versions as well as monitoring for deprecation warnings in the newest django version. Where necessary, write code for different versions and separate with:

import django
if django.VERSION > (1, 8):
    pass  # newer code
else:
    pass  # older compatibility code

Use templates to generate device configuration

One of the technical reasons to merge the lava-dispatcher and lava-server source trees into a single source is to allow lava-dispatcher to use the output of the lava-server templates in development. Further changes are being made in this area to provide a common module but it is already possible to build a lava_dispatcher unit test which pulls device configuration directly from the templates in lava_scheduler_app. This removes the problem of static YAML files in lava_dispatcher/devices getting out of date compared to the actual YAML created by changes in the templates.

The YAML device configuration is generated from a device dictionary in lava_scheduler_app which extends a template in lava_scheduler_app - the same template which is used at runtime on LAVA instances. Any change to the template or device dictionary is immediately reflected in the YAML sent to the lava_dispatcher unit test.

import unittest
from lava_dispatcher.tests.test_basic import Factory, StdoutTestCase
from lava_dispatcher.tests.utils import infrastructure_error_multi_paths

class TestFastbootDeploy(StdoutTestCase):  # pylint: disable=too-many-public-methods

    def setUp(self):
        super().setUp()
        self.factory = Factory()

    @unittest.skipIf(infrastructure_error_multi_paths(
        ['lxc-info', 'img2simg', 'simg2img']),
        "lxc or img2simg or simg2img not installed")
    def test_lxc_api(self):
        job = self.factory.create_job('d02-01.jinja2', 'sample_jobs/grub-ramdisk.yaml')

Database migrations

The LAVA team recommend using Debian stable but also support testing and unstable which have a newer version of python-django.

Database migrations on Debian Jessie and later are managed within django. Support for python-django-south has been dropped. Only django migration types should be included in any reviews which involve a database migration.

Once modified, the updated models.py file needs to be copied into the system location for the relevant extension, e.g. lava_scheduler_app. This is a step which needs to be done by the developer - developer packages cannot be installed cleanly and unit tests will likely fail until the migration has been created and applied.

On Debian Jessie and later:

$ sudo lava-server manage makemigrations lava_scheduler_app

The migration file will be created in /usr/lib/python3/dist-packages/lava_scheduler_app/migrations/ (which is why sudo is required) and will need to be copied into your git working copy and added to the review.

The migration is applied using:

$ sudo lava-server manage migrate lava_scheduler_app

See django docs for more information.

Python 3.x

Python3 support in LAVA is related to a number of factors:

  • Forthcoming LTS releases of django which will remove support for python2.7
  • Debian Jessie is now unsupported and development has moved to Stretch.
  • Transition within Debian to full python3 support.

https://lists.lavasoftware.org/pipermail/lava-announce/2017-June/000032.html

https://lists.lavasoftware.org/pipermail/lava-announce/2018-January/000046.html

lava-dispatcher and lava-server now fully support python3, runtime and testing. Code changes to either codebase must be Python3 compatible.

All reviews run the lava-dispatcher and lava-server unit tests against python 3.x and changes must pass all unit tests.

The ./ci-run script for lava-dispatcher and lava-server can run the unit tests using Python3:

./ci-run -a

Some additional Python3 dependencies will be required. In particular, python3-django-auth-ldap and python3-django-testscenarios will need to be installed from stretch-backports.

Warning

Django wil be dropping python2.7 support with the 2.2LTS release, frozen instances of LAVA will not be able to use django updates after that point.

XML-RPC changes

Each of the installed django apps in lava-server are able to expose functionality using XML-RPC.

from linaro_django_xmlrpc.models import ExposedAPI

class SomeAPI(ExposedAPI):
  1. The docstring must include the full user-facing documentation of each function exposed through the API.

  2. Authentication should be supported using the base class support:

    self._authenticate()
    
  3. Catch exceptions for all errors, SubmissionException, DoesNotExist and others, then re-raise as xmlrpc.client.Fault.

  4. Move as much of the work into the relevant app as possible, either in models.py or in dbutils.py. Wherever possible, re-use existing functions with wrappers for error handling in the API code.

Instance settings

/etc/lava-server/instance.conf is principally for V1 configuration. V2 uses this file only for the database connection settings on the master, instance name and the lavaserver user.

Most settings for the instance are handled inside django using /etc/lava-server/settings.conf. (For historical reasons, this file uses JSON syntax.)

Pylint3

Pylint is a tool that checks for errors in Python code, tries to enforce a coding standard and looks for bad code smells. We encourage developers to run LAVA code through pylint and fix warnings or errors shown by pylint to maintain a good score. For more information about code smells, refer to Martin Fowler’s refactoring book. LAVA developers stick on to PEP 008 (aka Guido’s style guide) across all the LAVA component code.

pylint3 does need to be used with some caution, the messages produced should not be followed blindly. It can be very useful for spotting unused imports, unused variables and other issues. To simplify the pylint output, some warnings are recommended to be disabled:

$ pylint3 -d line-too-long -d missing-docstring

Note

Docstrings should still be added wherever a docstring would be useful.

Many developers use a ~/.pylintrc file which already includes a sample list of warnings to disable. Other warnings frequently disabled in ~/.pylintrc include:

too-many-locals,
too-many-ancestors,
too-many-arguments,
too-many-instance-attributes,
too-many-nested-blocks,
too-many-return-statements,
too-many-branches,
too-many-statements,
too-few-public-methods,
wrong-import-order,
ungrouped-imports,

pylint also supports local disabling of warnings and there are many examples of:

variable = func_call()  # pylint: disable=

There is a pylint-django plugin available in unstable and testing and whilst it improves the pylint output for the lava-server codebase, it still has a high level of false indications, particularly when extending an existing model.

pep8

In order to check for PEP 008 compliance the following command is recommended:

$ pep8 --ignore E501

pep8 can be installed in Debian based systems as follows:

$ apt install pep8

Unit-tests

LAVA has set of unit tests which the developers can run on a regular basis for each change they make in order to check for regressions if any. Most of the LAVA components such as lava-server, lava-dispatcher, lava-tool have unit tests.

Extra dependencies are required to run the tests. On Debian based distributions, you need to install lava-dev and python3-django-testscenarios.

To run the tests, use the ci-run / ci-build scripts:

$ ./ci-run

See also

Preparing for LAVA development, Dependencies required to run unit tests and Testing the design for examples of how to run individual unit tests or all unit tests within a class or module.

LAVA database model visualization

LAVA database models can be visualized with the help of django_extensions along with tools such as pydot. In Debian based systems install the following packages to get the visualization of LAVA database models:

$ apt install python-django-extensions python-pydot

Once the above packages are installed successfully, use the following command to get the visualization of lava-server models in PNG format:

$ sudo lava-server manage graph_models --pydot -a -g -o lava-server-model.png

More documentation about graph models is available in https://django-extensions.readthedocs.io/en/latest/graph_models.html

Other useful features from django_extensions are as follows:

  • shell_plus - similar to the built-in “shell” but autoloads all models

  • validate_templates - check templates for rendering errors:

    $ sudo lava-server manage validate_templates
    
  • runscript - run arbitrary scripts inside lava-server environment:

    $ sudo lava-server manage runscript fix_user_names --script-args=all
    

Developer access to django shell

Default configurations use a side-effect of the logging behaviour to restrict access to the lava-server manage operations which typical Django apps expose through the manage.py interface. This is because lava-server manage shell provides read-write access to the database, so the command requires sudo.

On developer machines, this can be unnecessary. Set the location of the django log to a new location to allow easier access to the management commands to simplify debugging and to be able to run a Django Python Console inside a development environment. In /etc/lava-server/settings.conf add:

"DJANGO_LOGFILE": "/tmp/django.log"

Note

settings.conf is JSON syntax, so ensure that the previous line ends with a comma and that the resulting file validates as JSON. Use JSONLINT

The new location needs to be writable by the lavaserver user (for use by localhost) and by the developer user (but would typically be writeable by anyone).