Re: [PATCH 5/8] tests/vm: Added configuration file support

["Alex Bennée" <alex.bennee@linaro.org>]

Robert Foley <robert.foley@linaro.org> writes:

> Changes to tests/vm/basevm.py to allow accepting a configuration file
> as a parameter. Allows for specifying VM options such as
> cpu, machine, memory, and arbitrary qemu arguments for specifying options
> such as NUMA configuration.
> Also added an example config_example.yml.
>
> Signed-off-by: Robert Foley <robert.foley@linaro.org>
> Reviewed-by: Peter Puhov <peter.puhov@linaro.org>
> ---
>  tests/vm/basevm.py          | 60 +++++++++++++++++++++++++++++++++++++
>  tests/vm/config_example.yml | 52 ++++++++++++++++++++++++++++++++
>  2 files changed, 112 insertions(+)
>  create mode 100644 tests/vm/config_example.yml
>
> diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py
> index ec92c8f105..08a8989ac0 100755
> --- a/tests/vm/basevm.py
> +++ b/tests/vm/basevm.py
> @@ -31,6 +31,7 @@ import tempfile
>  import shutil
>  import multiprocessing
>  import traceback
> +import yaml
>  
>  SSH_KEY_FILE = os.path.join(os.path.dirname(__file__),
>                 "..", "keys", "id_rsa")
> @@ -396,6 +397,61 @@ class BaseVM(object):
>      def qmp(self, *args, **kwargs):
>          return self._guest.qmp(*args, **kwargs)
>  
> +
> +def parse_config(config, args):
> +    """ Parse yaml config and populate our config structure.
> +        The yaml config allows the user to override the
> +        defaults for VM parameters.  In many cases these
> +        defaults can be overridden without rebuilding the VM."""
> +    if args.config:
> +        config_file = args.config
> +    elif 'QEMU_CONFIG' in os.environ:
> +        config_file = os.environ['QEMU_CONFIG']
> +    else:
> +        return config
> +    if not os.path.exists(config_file):
> +        raise Exception("config file {} does not exist".format(config_file))
> +    with open(config_file) as f:
> +        yaml_dict = yaml.safe_load(f)
> +    if 'target-conf' in yaml_dict:
> +        target_dict = yaml_dict['target-conf']
> +        if 'username' in target_dict and target_dict['username'] != 'root':
> +            config['guest_user'] = target_dict['username']
> +        if 'password' in target_dict:
> +            config['root_pass'] = target_dict['password']
> +            config['guest_pass'] = target_dict['password']

This seems like impedance matching between two dictionaries. Surely it
would be nicer for the config to be read in fully formed and referable by
the rest of the code. We can also change the internal references.

> +        if any (k in target_dict for k in ("ssh_key","ssh_pub_key")) and \
> +           not all (k in target_dict for k in ("ssh_key","ssh_pub_key")):
> +            missing_key = "ssh_pub_key" \
> +              if 'ssh_key' in target_dict else "ssh_key"
> +            raise Exception("both ssh_key and ssh_pub_key required. "
> +                            "{} key is missing.".format(missing_key))

I guess validation has to be done at some time.. but

> +        if 'ssh_key' in target_dict:
> +            config['ssh_key_file'] = target_dict['ssh_key']
> +            if not os.path.exists(config['ssh_key_file']):
> +                raise Exception("ssh key file not found.")
> +        if 'ssh_pub_key' in target_dict:
> +            config['ssh_pub_key_file'] = target_dict['ssh_pub_key']
> +            if not os.path.exists(config['ssh_pub_key_file']):
> +                raise Exception("ssh pub key file not found.")

here we are both munging dictionaries again before checking the data.
Given we bail with an exception I'm now rethinking if it makes sense to
validate up here. It depends on how many places in the code expect to
use this data.

> +        if 'machine' in target_dict:
> +            config['machine'] = target_dict['machine']
> +        if 'qemu_args' in target_dict:
> +            qemu_args = target_dict['qemu_args']
> +            qemu_args = qemu_args.replace('\n', ' ').replace('\r', '')
> +            config['extra_args'] = qemu_args.split(' ')
> +        if 'memory' in target_dict:
> +            config['memory'] = target_dict['memory']
> +        if 'dns' in target_dict:
> +            config['dns'] = target_dict['dns']
> +        if 'cpu' in target_dict:
> +            config['cpu'] = target_dict['cpu']
> +        if 'ssh_port' in target_dict:
> +            config['ssh_port'] = target_dict['ssh_port']
> +        if 'install_cmds' in target_dict:
> +            config['install_cmds'] = target_dict['install_cmds']
> +    return config
> +
>  def parse_args(vmcls):
>  
>      def get_default_jobs():
> @@ -430,6 +486,9 @@ def parse_args(vmcls):
>                        help="Interactively run command")
>      parser.add_option("--snapshot", "-s", action="store_true",
>                        help="run tests with a snapshot")
> +    parser.add_option("--config", "-c", default=None,
> +                      help="Provide config yaml for configuration. "\
> +                           "See config_example.yaml for example.")
>      parser.disable_interspersed_args()
>      return parser.parse_args()
>  
> @@ -441,6 +500,7 @@ def main(vmcls, config=None):
>          if not argv and not args.build_qemu and not args.build_image:
>              print("Nothing to do?")
>              return 1
> +        config = parse_config(config, args)
>          logging.basicConfig(level=(logging.DEBUG if args.debug
>                                     else logging.WARN))
>          vm = vmcls(debug=args.debug, vcpus=args.jobs, config=config)
> diff --git a/tests/vm/config_example.yml b/tests/vm/config_example.yml
> new file mode 100644
> index 0000000000..0a1fec3824
> --- /dev/null
> +++ b/tests/vm/config_example.yml
> @@ -0,0 +1,52 @@
> +#
> +# Example yaml for use by any of the scripts in tests/vm.
> +# Can be provided as an environment variable QEMU_CONFIG
> +#
> +target-conf:
> +
> +    # If any of the below are not provided, we will just use the qemu defaults.
> +
> +    # Login username (has to be sudo enabled)
> +    #username: qemu
> +
> +    # Password is used by root and default login user.
> +    #password: "qemupass"
> +
> +    # If one key is provided, both must be provided.
> +    #ssh_key: /complete/path/of/your/keyfile/id_rsa
> +    #ssh_pub_key: /complete/path/of/your/keyfile/id_rsa.pub
> +
> +    cpu: max
> +    machine: virt,gic_version=3
> +    memory: 16G
> +
> +    # The below is an example for how to configure NUMA topology with
> +    # 4 NUMA nodes and 2 different NUMA distances.
> +    qemu_args: "-smp cpus=16,sockets=2,cores=8
> +                -object memory-backend-ram,size=4G,policy=bind,host-nodes=0,id=ram-node0
> +                -object memory-backend-ram,size=4G,policy=bind,host-nodes=0,id=ram-node1
> +                -object memory-backend-ram,size=4G,policy=bind,host-nodes=1,id=ram-node2
> +                -object memory-backend-ram,size=4G,policy=bind,host-nodes=1,id=ram-node3
> +                -numa node,memdev=ram-node0,cpus=0-3,nodeid=0 -numa node,memdev=ram-node1,cpus=4-7,nodeid=1
> +                -numa node,memdev=ram-node2,cpus=8-11,nodeid=2 -numa node,memdev=ram-node3,cpus=12-15,nodeid=3
> +                -numa dist,src=0,dst=1,val=15 -numa dist,src=2,dst=3,val=15
> +                -numa dist,src=0,dst=2,val=20 -numa dist,src=0,dst=3,val=20
> +                -numa dist,src=1,dst=2,val=20 -numa dist,src=1,dst=3,val=20"
> +
> +    # By default we do not set the DNS.
> +    # You override the defaults by setting the below.
> +    #dns: 1.234.567.89
> +
> +    # By default we will use a "block" device, but
> +    # you can also boot from a "scsi" device.
> +    # Just keep in mind your scripts might need to change
> +    # As you will have /dev/sda instead of /dev/vda (for block device)
> +    #boot_dev_type: "scsi"
> +
> +    # By default the ssh port is not fixed.
> +    # A fixed ssh port makes it easier for automated tests.
> +    #ssh_port: 5555
> +
> +    # To install a different set of packages, provide a command to issue
> +    #install_cmds: "apt-get update ; apt-get build-dep -y qemu"
> +

Having the example is great. It would be nice to see at least one of the
others converted to a config driven approach as well - is the config
driven approach going to reduce duplication across the various bits of
VM configuring python? Should everything be config driven? Are we in
danger of re-inventing an exiting tooling?

-- 
Alex Bennée