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

[Robert Foley]

On Mon, 27 Jan 2020 at 07:38, Alex Bennée <address@hidden> wrote:
> > +        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.

Good point.  Will rework it to avoid this matching.  Basically we can
put the values we
read directly into the config dictionary in one step.
>
> > +        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.

Makes sense.  We will change it to validate in one place just before
we expect to use this data.

> > +    # 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

The example we provided was primarily for aarch64, we will add one or
more examples here for the other VMs to
help provide a ready to use template for providing a config file.

> - 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?

All interesting questions to explore.  Here is my take on this.

One goal we had in mind is to not require a config file for any given
VM.  So in this sense we are
not going in the direction of a config driven approach.
Even the VMs that we added for aarch64 do not require a config file.
The VM scripts will work as is without a config file since the script
itself provides all required defaults.
Our intention was for the config approach to be used to allow
overriding the defaults for any
given VM, to give the flexibility of overriding the parameters.
But on the other hand by not requiring a config file, we make is
simpler and easier to
only override the parameters that the user is interested in.  And also
to limit the cases where
we could generate a non-working VM if the user forgot to provide
certain defaults.

Thanks & Regards,
-Rob