| Message ID | 20200124165335.422-6-robert.foley@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tests/vm: Add support for aarch64 VMs | expand |
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?
On Mon, 27 Jan 2020 at 07:38, Alex Bennée <alex.bennee@linaro.org> 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
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'] + 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)) + 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.") + 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" +