| Message ID | 20200124165335.422-5-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: > Added use of a configuration to tests/vm/basevm.py. > The configuration provides parameters used to configure a VM. > This allows for providing alternate configurations to the VM being > created/launched. cpu, machine, memory, and NUMA configuration are all > examples of configuration which we might want to vary on the VM being created > or launched. > This will for example allow for creating an aarch64 vm. > > Signed-off-by: Robert Foley <robert.foley@linaro.org> > Reviewed-by: Peter Puhov <peter.puhov@linaro.org> > --- > tests/vm/basevm.py | 108 ++++++++++++++++++++++++++++++++++----------- > 1 file changed, 82 insertions(+), 26 deletions(-) > > diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py > index 3b4403ddcb..ec92c8f105 100755 > --- a/tests/vm/basevm.py > +++ b/tests/vm/basevm.py > @@ -32,15 +32,40 @@ import shutil > import multiprocessing > import traceback > > -SSH_KEY = open(os.path.join(os.path.dirname(__file__), > - "..", "keys", "id_rsa")).read() > -SSH_PUB_KEY = open(os.path.join(os.path.dirname(__file__), > - "..", "keys", "id_rsa.pub")).read() > - > +SSH_KEY_FILE = os.path.join(os.path.dirname(__file__), > + "..", "keys", "id_rsa") > +SSH_PUB_KEY_FILE = os.path.join(os.path.dirname(__file__), > + "..", "keys", "id_rsa.pub") > +SSH_KEY = open(SSH_KEY_FILE).read() > +SSH_PUB_KEY = open(SSH_PUB_KEY_FILE).read() Why are we tracking more information about the keyfile than we used to now? Is this because it's harder to embed keys over paths in the config? > + > +# This is the standard configuration. > +# Any or all of these can be overridden by > +# passing in a config argument to the VM constructor. > +DEFAULT_CONFIG = { > + 'cpu' : "max", > + 'machine' : 'pc', > + 'guest_user' : "qemu", > + 'guest_pass' : "qemupass", > + 'root_pass' : "qemupass", > + 'ssh_key_file' : SSH_KEY_FILE, > + 'ssh_pub_key_file': SSH_PUB_KEY_FILE, > + 'memory' : "4G", > + 'extra_args' : [], > + 'dns' : "", > + 'ssh_port' : 0, > + 'install_cmds' : "", > + 'boot_dev_type' : "block", > + 'ssh_timeout' : 1, > +} > +BOOT_DEVICE = { > + 'block' : "-drive file={},if=none,id=drive0,cache=writeback "\ > + "-device virtio-blk,drive=drive0,bootindex=0", > + 'scsi' : "-device virtio-scsi-device,id=scsi "\ > + "-drive file={},format=raw,if=none,id=hd0 "\ > + "-device scsi-hd,drive=hd0,bootindex=0", > +} > class BaseVM(object): > - GUEST_USER = "qemu" > - GUEST_PASS = "qemupass" > - ROOT_PASS = "qemupass" Don't we need these? > > envvars = [ > "https_proxy", > @@ -59,19 +84,26 @@ class BaseVM(object): > poweroff = "poweroff" > # enable IPv6 networking > ipv6 = True > - def __init__(self, debug=False, vcpus=None): > + def __init__(self, debug=False, vcpus=None, config=None): > self._guest = None > + # Allow input config to override defaults. > + self._config = DEFAULT_CONFIG.copy() > + if config != None: > + self._config.update(config) > self._tmpdir = os.path.realpath(tempfile.mkdtemp(prefix="vm-test-", > suffix=".tmp", > dir=".")) > atexit.register(shutil.rmtree, self._tmpdir) > - > + self._config['ssh_key'] = \ > + open(self._config['ssh_key_file']).read().rstrip() > + self._config['ssh_pub_key'] = \ > + open(self._config['ssh_pub_key_file']).read().rstrip() > self._ssh_key_file = os.path.join(self._tmpdir, "id_rsa") > - open(self._ssh_key_file, "w").write(SSH_KEY) > + open(self._ssh_key_file, "w").write(self._config['ssh_key']) > subprocess.check_call(["chmod", "600", self._ssh_key_file]) > > self._ssh_pub_key_file = os.path.join(self._tmpdir, "id_rsa.pub") > - open(self._ssh_pub_key_file, "w").write(SSH_PUB_KEY) > + open(self._ssh_pub_key_file, > "w").write(self._config['ssh_pub_key']) Read as a block I find this confusing: self._config['ssh_key'] = \ open(self._config['ssh_key_file']).read().rstrip() self._config['ssh_pub_key'] = \ open(self._config['ssh_pub_key_file']).read().rstrip() self._ssh_key_file = os.path.join(self._tmpdir, "id_rsa") open(self._ssh_key_file, "w").write(self._config['ssh_key']) subprocess.check_call(["chmod", "600", self._ssh_key_file]) self._ssh_pub_key_file = os.path.join(self._tmpdir, "id_rsa.pub") open(self._ssh_pub_key_file, "w").write(self._config['ssh_pub_key']) We read config['ssh_key_file'] but write out _ssh_pub_key_file which doesn't seem related. <snip>
On Mon, 27 Jan 2020 at 07:26, Alex Bennée <alex.bennee@linaro.org> wrote: > > -SSH_KEY = open(os.path.join(os.path.dirname(__file__), > > - "..", "keys", "id_rsa")).read() > > -SSH_PUB_KEY = open(os.path.join(os.path.dirname(__file__), > > - "..", "keys", "id_rsa.pub")).read() > > - > > +SSH_KEY_FILE = os.path.join(os.path.dirname(__file__), > > + "..", "keys", "id_rsa") > > +SSH_PUB_KEY_FILE = os.path.join(os.path.dirname(__file__), > > + "..", "keys", "id_rsa.pub") > > +SSH_KEY = open(SSH_KEY_FILE).read() > > +SSH_PUB_KEY = open(SSH_PUB_KEY_FILE).read() > > Why are we tracking more information about the keyfile than we used to > now? Is this because it's harder to embed keys over paths in the config? We now allow the user to override the ssh keys. Because of this we need to track the filename separately from the contents of the file, so that we can put this default filename into the DEFAULT_CONFIG below. We also keep the original SSH_PUB_KEY since it is still used by some pre-existing VM scripts, and we did not want to break backwards compatibility for those scripts. Actually upon further inspection, it looks like we can delete SSH_KEY, this is no longer needed. :) > > > + > > +# This is the standard configuration. > > +# Any or all of these can be overridden by > > +# passing in a config argument to the VM constructor. > > +DEFAULT_CONFIG = { > > + 'cpu' : "max", > > + 'machine' : 'pc', > > + 'guest_user' : "qemu", > > + 'guest_pass' : "qemupass", > > + 'root_pass' : "qemupass", > > + 'ssh_key_file' : SSH_KEY_FILE, > > + 'ssh_pub_key_file': SSH_PUB_KEY_FILE, > > + 'memory' : "4G", > > + 'extra_args' : [], > > + 'dns' : "", > > + 'ssh_port' : 0, > > + 'install_cmds' : "", > > + 'boot_dev_type' : "block", > > + 'ssh_timeout' : 1, > > +} > > +BOOT_DEVICE = { > > + 'block' : "-drive file={},if=none,id=drive0,cache=writeback "\ > > + "-device virtio-blk,drive=drive0,bootindex=0", > > + 'scsi' : "-device virtio-scsi-device,id=scsi "\ > > + "-drive file={},format=raw,if=none,id=hd0 "\ > > + "-device scsi-hd,drive=hd0,bootindex=0", > > +} > > class BaseVM(object): > > - GUEST_USER = "qemu" > > - GUEST_PASS = "qemupass" > > - ROOT_PASS = "qemupass" > > Don't we need these? We don't need these since we moved them up to the new DEFAULT_CONFIG. These are essentially the default values now since the user can now override these. We also handle the cases where these are accessed by existing scripts, and ensuring backwards compatibility by referencing these values in the _config (see the code in __getattr__). This actually brings up a good point that I wanted to mention. Our initial plan was to leave the existing VM scripts unchanged. We were thinking that it would also clarify things for a later patch to simply change references to ROOT_PASS, GUEST_USER/ PASS, and SSH_PUB_KEY, within the existing VM scripts (centos, openbsd, etc) to use _config, and then we could get rid of the __getattr__ change entirely. Actually, we could even put it at the end of this series too. I think I will add this change to the next version of this patch unless you think we should keep it separate? > > > > envvars = [ > > "https_proxy", > > @@ -59,19 +84,26 @@ class BaseVM(object): > > poweroff = "poweroff" > > # enable IPv6 networking > > ipv6 = True > > - def __init__(self, debug=False, vcpus=None): > > + def __init__(self, debug=False, vcpus=None, config=None): > > self._guest = None > > + # Allow input config to override defaults. > > + self._config = DEFAULT_CONFIG.copy() > > + if config != None: > > + self._config.update(config) > > self._tmpdir = os.path.realpath(tempfile.mkdtemp(prefix="vm-test-", > > suffix=".tmp", > > dir=".")) > > atexit.register(shutil.rmtree, self._tmpdir) > > - > > + self._config['ssh_key'] = \ > > + open(self._config['ssh_key_file']).read().rstrip() > > + self._config['ssh_pub_key'] = \ > > + open(self._config['ssh_pub_key_file']).read().rstrip() > > self._ssh_key_file = os.path.join(self._tmpdir, "id_rsa") > > - open(self._ssh_key_file, "w").write(SSH_KEY) > > + open(self._ssh_key_file, "w").write(self._config['ssh_key']) > > subprocess.check_call(["chmod", "600", self._ssh_key_file]) > > > > self._ssh_pub_key_file = os.path.join(self._tmpdir, "id_rsa.pub") > > - open(self._ssh_pub_key_file, "w").write(SSH_PUB_KEY) > > + open(self._ssh_pub_key_file, > > "w").write(self._config['ssh_pub_key']) > > Read as a block I find this confusing: > > self._config['ssh_key'] = \ > open(self._config['ssh_key_file']).read().rstrip() > self._config['ssh_pub_key'] = \ > open(self._config['ssh_pub_key_file']).read().rstrip() > self._ssh_key_file = os.path.join(self._tmpdir, "id_rsa") > open(self._ssh_key_file, "w").write(self._config['ssh_key']) > subprocess.check_call(["chmod", "600", self._ssh_key_file]) > > self._ssh_pub_key_file = os.path.join(self._tmpdir, "id_rsa.pub") > open(self._ssh_pub_key_file, "w").write(self._config['ssh_pub_key']) > > We read config['ssh_key_file'] but write out _ssh_pub_key_file which > doesn't seem related. I agree we can clarify this. :) Most of this logic was here previously, we're just adding the parameterization of the keys. This is the current flow: 1) copy the key file (config['ssh_key_file']) to a temporary file (_ssh_pub_key_file) 2) chmod the key file so that ssh is happy with the permissions. Without this chmod ssh will refuse to use the key file. It seems to make sense to add a comment here to clarify all this. It also seems like we could change the name _ssh_pub_key_file to _ssh_tmp_pub_key_file to make it more clear it is a temp file. What do you think, would that be enough to clarify things? Thanks & Regards, -Rob > <snip> > > -- > Alex Bennée
diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py index 3b4403ddcb..ec92c8f105 100755 --- a/tests/vm/basevm.py +++ b/tests/vm/basevm.py @@ -32,15 +32,40 @@ import shutil import multiprocessing import traceback -SSH_KEY = open(os.path.join(os.path.dirname(__file__), - "..", "keys", "id_rsa")).read() -SSH_PUB_KEY = open(os.path.join(os.path.dirname(__file__), - "..", "keys", "id_rsa.pub")).read() - +SSH_KEY_FILE = os.path.join(os.path.dirname(__file__), + "..", "keys", "id_rsa") +SSH_PUB_KEY_FILE = os.path.join(os.path.dirname(__file__), + "..", "keys", "id_rsa.pub") +SSH_KEY = open(SSH_KEY_FILE).read() +SSH_PUB_KEY = open(SSH_PUB_KEY_FILE).read() + +# This is the standard configuration. +# Any or all of these can be overridden by +# passing in a config argument to the VM constructor. +DEFAULT_CONFIG = { + 'cpu' : "max", + 'machine' : 'pc', + 'guest_user' : "qemu", + 'guest_pass' : "qemupass", + 'root_pass' : "qemupass", + 'ssh_key_file' : SSH_KEY_FILE, + 'ssh_pub_key_file': SSH_PUB_KEY_FILE, + 'memory' : "4G", + 'extra_args' : [], + 'dns' : "", + 'ssh_port' : 0, + 'install_cmds' : "", + 'boot_dev_type' : "block", + 'ssh_timeout' : 1, +} +BOOT_DEVICE = { + 'block' : "-drive file={},if=none,id=drive0,cache=writeback "\ + "-device virtio-blk,drive=drive0,bootindex=0", + 'scsi' : "-device virtio-scsi-device,id=scsi "\ + "-drive file={},format=raw,if=none,id=hd0 "\ + "-device scsi-hd,drive=hd0,bootindex=0", +} class BaseVM(object): - GUEST_USER = "qemu" - GUEST_PASS = "qemupass" - ROOT_PASS = "qemupass" envvars = [ "https_proxy", @@ -59,19 +84,26 @@ class BaseVM(object): poweroff = "poweroff" # enable IPv6 networking ipv6 = True - def __init__(self, debug=False, vcpus=None): + def __init__(self, debug=False, vcpus=None, config=None): self._guest = None + # Allow input config to override defaults. + self._config = DEFAULT_CONFIG.copy() + if config != None: + self._config.update(config) self._tmpdir = os.path.realpath(tempfile.mkdtemp(prefix="vm-test-", suffix=".tmp", dir=".")) atexit.register(shutil.rmtree, self._tmpdir) - + self._config['ssh_key'] = \ + open(self._config['ssh_key_file']).read().rstrip() + self._config['ssh_pub_key'] = \ + open(self._config['ssh_pub_key_file']).read().rstrip() self._ssh_key_file = os.path.join(self._tmpdir, "id_rsa") - open(self._ssh_key_file, "w").write(SSH_KEY) + open(self._ssh_key_file, "w").write(self._config['ssh_key']) subprocess.check_call(["chmod", "600", self._ssh_key_file]) self._ssh_pub_key_file = os.path.join(self._tmpdir, "id_rsa.pub") - open(self._ssh_pub_key_file, "w").write(SSH_PUB_KEY) + open(self._ssh_pub_key_file, "w").write(self._config['ssh_pub_key']) self.debug = debug self._stderr = sys.stderr @@ -80,11 +112,14 @@ class BaseVM(object): self._stdout = sys.stdout else: self._stdout = self._devnull + netdev = "user,id=vnet,hostfwd=:127.0.0.1:{}-:22" self._args = [ \ - "-nodefaults", "-m", "4G", - "-cpu", "max", - "-netdev", "user,id=vnet,hostfwd=:127.0.0.1:0-:22" + - (",ipv6=no" if not self.ipv6 else ""), + "-nodefaults", "-m", self._config['memory'], + "-cpu", self._config['cpu'], + "-netdev", + netdev.format(self._config['ssh_port']) + + (",ipv6=no" if not self.ipv6 else "") + + (",dns=" + self._config['dns'] if self._config['dns'] else ""), "-device", "virtio-net-pci,netdev=vnet", "-vnc", "127.0.0.1:0,to=20"] if vcpus and vcpus > 1: @@ -95,6 +130,25 @@ class BaseVM(object): logging.info("KVM not available, not using -enable-kvm") self._data_args = [] + def wait_boot(self, wait_string=None): + """Wait for the standard string we expect + on completion of a normal boot. + The user can also choose to override with an + alternate string to wait for.""" + if wait_string is None: + if self.login_prompt is None: + raise Exception("self.login_prompt not defined") + wait_string = self.login_prompt + self.console_init() + self.console_wait(wait_string) + + def __getattr__(self, name): + # Support direct access to config by key. + # for example, access self._config['cpu'] by self.cpu + if name.lower() in self._config.keys(): + return self._config[name.lower()] + return object.__getattribute__(self, name) + def _download_with_cache(self, url, sha256sum=None, sha512sum=None): def check_sha256sum(fname): if not sha256sum: @@ -126,7 +180,8 @@ class BaseVM(object): "-t", "-o", "StrictHostKeyChecking=no", "-o", "UserKnownHostsFile=" + os.devnull, - "-o", "ConnectTimeout=1", + "-o", + "ConnectTimeout={}".format(self._config["ssh_timeout"]), "-p", self.ssh_port, "-i", self._ssh_key_file] # If not in debug mode, set ssh to quiet mode to # avoid printing the results of commands. @@ -176,15 +231,15 @@ class BaseVM(object): "virtio-blk,drive=%s,serial=%s,bootindex=1" % (name, name)] def boot(self, img, extra_args=[]): - args = self._args + [ - "-device", "VGA", - "-drive", "file=%s,if=none,id=drive0,cache=writeback" % img, - "-device", "virtio-blk,drive=drive0,bootindex=0"] - args += self._data_args + extra_args + boot_dev = BOOT_DEVICE[self._config['boot_dev_type']] + boot_params = boot_dev.format(img) + args = self._args + boot_params.split(' ') + args += self._data_args + extra_args + self._config['extra_args'] + args += ["-device", "VGA"] logging.debug("QEMU args: %s", " ".join(args)) qemu_bin = os.environ.get("QEMU", "qemu-system-" + self.arch) guest = QEMUMachine(binary=qemu_bin, args=args) - guest.set_machine('pc') + guest.set_machine(self._config['machine']) guest.set_console() try: guest.launch() @@ -331,7 +386,6 @@ class BaseVM(object): def shutdown(self): self._guest.shutdown() - def wait(self): self._guest.wait() @@ -379,15 +433,17 @@ def parse_args(vmcls): parser.disable_interspersed_args() return parser.parse_args() -def main(vmcls): +def main(vmcls, config=None): try: + if config == None: + config = {} args, argv = parse_args(vmcls) if not argv and not args.build_qemu and not args.build_image: print("Nothing to do?") return 1 logging.basicConfig(level=(logging.DEBUG if args.debug else logging.WARN)) - vm = vmcls(debug=args.debug, vcpus=args.jobs) + vm = vmcls(debug=args.debug, vcpus=args.jobs, config=config) if args.build_image: if os.path.exists(args.image) and not args.force: sys.stderr.writelines(["Image file exists: %s\n" % args.image,