diff mbox series

[4/8] tests/vm: Add configuration to basevm.py

Message ID 20200124165335.422-5-robert.foley@linaro.org
State New
Headers show
Series tests/vm: Add support for aarch64 VMs | expand

Commit Message

Robert Foley Jan. 24, 2020, 4:53 p.m. UTC
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(-)

Comments

Alex Bennée Jan. 27, 2020, 12:26 p.m. UTC | #1
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>
Robert Foley Jan. 27, 2020, 1:56 p.m. UTC | #2
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 mbox series

Patch

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,