cloud-init-dev team mailing list archive

[Chad Smith <chad.smith@xxxxxxxxxxxxx>]

Thanks for working this josh. Here are a few more more comments. I'm still seeing spurious issues on my laptop trying to run this (I just hit an OOM as well) and NoValidConnectionsError from paramiko. But I have also seen successful runs. I'm +1 on this iteration with various fixes/comments inline. We can iterate and improve as we have cycles.

Diff comments:

> diff --git a/tests/cloud_tests/images/kvm.py b/tests/cloud_tests/images/kvm.py
> new file mode 100644
> index 0000000..ece8727
> --- /dev/null
> +++ b/tests/cloud_tests/images/kvm.py
> @@ -0,0 +1,81 @@
> +# This file is part of cloud-init. See LICENSE file for license information.
> +
> +"""KVM Image Base Class."""
> +
> +from tests.cloud_tests.images import base
> +from tests.cloud_tests.snapshots import kvm as kvm_snapshot
> +
> +
> +class KVMImage(base.Image):
> +    """KVM backed image."""
> +
> +    platform_name = "kvm"
> +
> +    def __init__(self, platform, config, img_path):
> +        """Set up image.
> +
> +        @param platform: platform object
> +        @param config: image configuration
> +        @param img_path: path to the image
> +        """
> +        self.modified = False
> +        self._instance = None
> +        self._img_path = img_path
> +
> +        super(KVMImage, self).__init__(platform, config)
> +
> +    @property
> +    def instance(self):
> +        """Returns an instance of an image."""
> +        if not self._instance:
> +            self._instance = self.platform.create_image(
> +                self.properties, self.config, self.features, self._img_path,
> +                image_desc=str(self), use_desc='image-modification')
> +        return self._instance
> +
> +    @property
> +    def properties(self):
> +        """Dictionary containing: 'arch', 'os', 'version', 'release'."""
> +        return {
> +            'arch': self.config['arch'],
> +            'os': self.config['family'],
> +            'release': self.config['release'],
> +            'version': self.config['version'],
> +        }
> +
> +    def execute(self, *args, **kwargs):
> +        """Execute command in image, modifying image."""
> +        return self.instance.execute(*args, **kwargs)
> +
> +    def push_file(self, local_path, remote_path):
> +        """Copy file at 'local_path' to instance at 'remote_path'."""
> +        return self.instance.push_file(local_path, remote_path)
> +
> +    def run_script(self, *args, **kwargs):
> +        """Run script in image, modifying image.
> +
> +        @return_value: script output
> +        """
> +        return self.instance.run_script(*args, **kwargs)
> +
> +    def snapshot(self):
> +        """Create snapshot of image, block until done."""
> +        instance = self.platform.create_image(
> +            self.properties, self.config, self.features,
> +            self._img_path, image_desc=str(self), use_desc='snapshot')
> +
> +        return kvm_snapshot.KVMSnapshot(
> +            self.platform, self.properties, self.config,
> +            self.features, instance)
> +
> +    def destroy(self):
> +        """Unset path to signal image is no longer used.
> +
> +        The removal of the images and all other items is handled by the
> +        framework. In some cases we want to keep the images, so let the
> +        framework decide whether to keep or destory everything.

s/destory/destroy/

> +        """
> +        self._img_path = None

If all we are doing in setting _img_path = None, I would like to see conditional gates in methods like snapshot to make sure we don't try to create_image based on a path of None (or something the the platform.create_image which would have checked if not src_img_path: raise RuntimeError() or something. Do we expect any of the methods of KVMImage to be called after a destroy() is issued? If not, then maybe each method should be gated by an if self._img_path: check.   Should we also be calling self._instance.destroy() here too or do we expect instances to live longer than the destroyed image? I would have expected image.destroy() to take care of cleanup of _instance references too since it created the instance reference in the first place.

> +        super(KVMImage, self).destroy()
> +
> +# vi: ts=4 expandtab
> diff --git a/tests/cloud_tests/instances/kvm.py b/tests/cloud_tests/instances/kvm.py
> new file mode 100644
> index 0000000..0c504e5
> --- /dev/null
> +++ b/tests/cloud_tests/instances/kvm.py
> @@ -0,0 +1,201 @@
> +# This file is part of cloud-init. See LICENSE file for license information.
> +
> +"""Base KVM instance."""
> +
> +import os
> +import paramiko
> +import shlex
> +import socket
> +import subprocess
> +import time
> +
> +from cloudinit import util as c_util
> +from tests.cloud_tests.instances import base
> +from tests.cloud_tests import util
> +
> +
> +class KVMInstance(base.Instance):
> +    """KVM backed instance."""
> +
> +    platform_name = "kvm"
> +
> +    def __init__(self, platform, name, properties, config, features,
> +                 user_data, meta_data):
> +        """Set up instance.
> +
> +        @param platform: platform object
> +        @param name: image path
> +        @param properties: dictionary of properties
> +        @param config: dictionary of configuration values
> +        @param features: dictionary of supported feature flags
> +        """
> +        self.user_data = user_data
> +        self.meta_data = meta_data
> +        self.ssh_key_file = os.path.join(platform.config['data_dir'],
> +                                         platform.config['private_key'])
> +        self.ssh_port = None
> +        self.pid = None
> +        self.pid_file = None
> +
> +        super(KVMInstance, self).__init__(
> +            platform, name, properties, config, features)
> +
> +    def destroy(self):
> +        """Clean up instance."""
> +        if self.pid:
> +            c_util.subp(['kill', '-9', self.pid])

Is it worth running this is a try/except util.ProcessExecutionError. I get errors like this due to paramiko falling over because it couldn't get a connection. If kill errors out, the process doesn't exist, so we've succeeded in this case and don't need to traceback.

  File "/usr/lib/python3/dist-packages/paramiko/client.py", line 324, in connect
    raise NoValidConnectionsError(errors)

2017-09-07 00:17:38,633 - tests.cloud_tests - ERROR - stage: collect test data for xenial encountered error: Unexpected error while running command.
Command: ['kill', '-9', '7441']
Exit code: 1
Reason: -
Stdout: -
Stderr: kill: (7441): No such process

> +            os.remove(self.pid_file)
> +        super(KVMInstance, self).destroy()

Shall we also unset self.pid too, so if destroy is called twice it doesn't attempt the work it already did?

> +
> +    def execute(self, command, stdout=None, stderr=None, env={},
> +                rcs=None, description=None):
> +        """Execute command in instance.
> +
> +        Assumes functional networking and execution as root with the
> +        target filesystem being available at /.
> +
> +        @param command: the command to execute as root inside the image
> +        @param stdout, stderr: file handles to write output and error to
> +        @param env: environment variables
> +        @param rcs: allowed return codes from command
> +        @param description: purpose of command
> +        @return_value: tuple containing stdout data, stderr data, exit code
> +        """
> +        if self.pid:
> +            return self.ssh(command)
> +        else:
> +            return self.mount_image_callback(command)
> +
> +    def mount_image_callback(self, cmd):
> +        """Run mount-image-callback."""
> +        mic = ('sudo mount-image-callback --system-mounts --system-resolvconf '
> +               '%s -- chroot _MOUNTPOINT_ /bin/sh' % self.name)
> +
> +        out, err = c_util.subp(shlex.split(mic), data=cmd)
> +
> +        return out, err, 0

Having a trailing 0 in your tuple because of the caller likes to format it's responses feels wrong. We should probably just return out, err here and execute should probably just return self.mount_image_callback(command) + (0,)

> +
> +    def generate_seed(self, tmpdir):
> +        """Generate nocloud seed from user-data"""
> +        seed_file = os.path.join(tmpdir, '%s_seed.img' % self.name)
> +        user_data_file = os.path.join(tmpdir, '%s_user_data' % self.name)
> +
> +        if os.path.exists(seed_file):
> +            os.remove(seed_file)

Do we expect re-using instances a bit and cleaning up the seed_file and user-data file? If so, wouldn't we also need to clean /var/log/cloud-init.log and /var/lib/cloud too?

> +        if os.path.exists(user_data_file):
> +            os.remove(user_data_file)
> +
> +        with open(user_data_file, "w") as ud_file:
> +            ud_file.write(self.user_data)
> +
> +        c_util.subp(['cloud-localds', seed_file, user_data_file])
> +
> +        return seed_file
> +
> +    def get_free_port(self):
> +        """Get a free port assigned by the kernel."""
> +        s = socket.socket()
> +        s.bind(('', 0))
> +        num = s.getsockname()[1]
> +        s.close()
> +        return num
> +
> +    def push_file(self, local_path, remote_path, description=''):
> +        """Copy file at 'local_path' to instance at 'remote_path'.
> +
> +        If we have a pid then SSH is up, otherwise, use
> +        mount-image-callback.
> +
> +        @param local_path: path on local instance
> +        @param remote_path: path on remote instance
> +        """
> +        if self.pid:
> +            super(KVMInstance, self).push_file()
> +        else:
> +            cmd = ("sudo mount-image-callback --system-mounts "
> +                   "--system-resolvconf %s -- chroot _MOUNTPOINT_ "
> +                   "/bin/sh -c 'cat - > %s'" % (self.name, remote_path))
> +            local_file = open(local_path)
> +            p = subprocess.Popen(shlex.split(cmd),
> +                                 stdin=local_file,
> +                                 stdout=subprocess.PIPE,
> +                                 stderr=subprocess.PIPE)
> +            p.wait()
> +
> +    def sftp_put(self, path, data):
> +        """SFTP put a file."""
> +        client = self._ssh_connect()
> +        sftp = client.open_sftp()
> +
> +        with sftp.open(path, 'w') as f:
> +            f.write(data)
> +
> +        client.close()
> +
> +    def ssh(self, command):
> +        """Run a command via SSH."""
> +        client = self._ssh_connect()
> +        _, out, err = client.exec_command(command)
> +        exit = out.channel.recv_exit_status()
> +        out = ''.join(out.readlines())
> +        err = ''.join(err.readlines())
> +        client.close()
> +
> +        return out, err, exit
> +
> +    def _ssh_connect(self, hostname='localhost', username='ubuntu',
> +                     banner_timeout=120, retry_attempts=30):
> +        """Connect via SSH."""
> +        private_key = paramiko.RSAKey.from_private_key_file(self.ssh_key_file)
> +        client = paramiko.SSHClient()
> +        client.set_missing_host_key_policy(paramiko.AutoAddPolicy())
> +        while retry_attempts:
> +            try:
> +                client.connect(hostname=hostname, username=username,
> +                               port=self.ssh_port, pkey=private_key,
> +                               banner_timeout=banner_timeout)
> +                return client
> +            except (paramiko.SSHException, TypeError):
> +                time.sleep(1)
> +                retry_attempts = retry_attempts - 1
> +
> +        error_desc = 'Failed command to: %s@%s:%s' % (username, hostname,
> +                                                      self.ssh_port)
> +        raise util.InTargetExecuteError('', '', -1, 'ssh connect',
> +                                        self.name, error_desc)
> +
> +    def start(self, wait=True, wait_for_cloud_init=False):
> +        """Start instance."""
> +        tmpdir = self.platform.config['data_dir']
> +        seed = self.generate_seed(tmpdir)
> +        self.pid_file = os.path.join(tmpdir, '%s.pid' % self.name)
> +        self.ssh_port = self.get_free_port()
> +
> +        cmd = ('./tools/xkvm --disk %s,cache=unsafe --disk %s,cache=unsafe '
> +               '--netdev user,hostfwd=tcp::%s-:22 '
> +               '-- -pidfile %s -vnc none -m 2G -smp 2'
> +               % (self.name, seed, self.ssh_port, self.pid_file))
> +
> +        subprocess.Popen(shlex.split(cmd), close_fds=True,
> +                         stdin=subprocess.PIPE,
> +                         stdout=subprocess.PIPE,
> +                         stderr=subprocess.PIPE)
> +
> +        while not os.path.exists(self.pid_file):
> +            time.sleep(1)
> +
> +        with open(self.pid_file, 'r') as pid_f:
> +            self.pid = pid_f.readlines()[0].strip()
> +
> +        if wait:
> +            self._wait_for_system(wait_for_cloud_init)
> +
> +    def write_data(self, remote_path, data):
> +        """Write data to instance filesystem.
> +
> +        @param remote_path: path in instance
> +        @param data: data to write, either str or bytes
> +        """
> +        self.sftp_put(remote_path, data)
> +
> +# vi: ts=4 expandtab
> diff --git a/tests/cloud_tests/instances/lxd.py b/tests/cloud_tests/instances/lxd.py
> index b9c2cc6..c3a7c13 100644
> --- a/tests/cloud_tests/instances/lxd.py
> +++ b/tests/cloud_tests/instances/lxd.py
> @@ -48,6 +48,7 @@ class LXDInstance(base.Instance):
>          """
>          # ensure instance is running and execute the command
>          self.start()
> +        command = ['/bin/bash', '-c'] + [command]

Why was this needed? shell escaping or something?

>          res = self.pylxd_container.execute(command, environment=env)
>  
>          # get out, exit and err from pylxd return
> diff --git a/tests/cloud_tests/platforms/kvm.py b/tests/cloud_tests/platforms/kvm.py
> new file mode 100644
> index 0000000..70808f2
> --- /dev/null
> +++ b/tests/cloud_tests/platforms/kvm.py
> @@ -0,0 +1,98 @@
> +# This file is part of cloud-init. See LICENSE file for license information.
> +
> +"""Base KVM platform."""
> +import glob
> +import os
> +import shutil
> +
> +from simplestreams import filters
> +from simplestreams import mirrors
> +from simplestreams import objectstores
> +from simplestreams import util as s_util
> +
> +from cloudinit import util as c_util
> +from tests.cloud_tests.images import kvm as kvm_image
> +from tests.cloud_tests.instances import kvm as kvm_instance
> +from tests.cloud_tests.platforms import base
> +from tests.cloud_tests import util
> +
> +
> +class KVMPlatform(base.Platform):
> +    """KVM test platform."""
> +
> +    platform_name = 'kvm'
> +
> +    def __init__(self, config):

If we aren't doing anything specific here, no need to define __init__. The superclass does this for us.

> +        """Set up platform."""
> +        super(KVMPlatform, self).__init__(config)
> +
> +    def get_image(self, img_conf):
> +        """Get image using specified image configuration.
> +
> +        @param img_conf: configuration for image
> +        @return_value: cloud_tests.images instance
> +        """
> +        (url, path) = s_util.path_from_mirror_url(img_conf['mirror_url'], None)
> +
> +        filter = filters.get_filters(['arch=%s' % c_util.get_architecture(),
> +                                      'release=%s' % img_conf['release'],
> +                                      'ftype=disk1.img'])
> +        mirror_config = {'filters': filter,
> +                         'keep_items': False,
> +                         'max_items': 1,
> +                         'checksumming_reader': True,
> +                         'item_download': True
> +                         }
> +
> +        def policy(content, path):
> +            return s_util.read_signed(content, keyring=img_conf['keyring'])
> +
> +        smirror = mirrors.UrlMirrorReader(url,
> +                                          policy=policy)
> +        tstore = objectstores.FileStore(img_conf['mirror_dir'])
> +        tmirror = mirrors.ObjectFilterMirror(config=mirror_config,
> +                                             objectstore=tstore)
> +        tmirror.sync(smirror, path)
> +
> +        search_d = os.path.join(img_conf['mirror_dir'], '**',
> +                                img_conf['release'], '**', '*.img')
> +
> +        images = []
> +        for fname in glob.iglob(search_d, recursive=True):
> +            images.append(fname)
> +
> +        if len(images) != 1:
> +            raise Exception('No unique images found')
> +
> +        image = kvm_image.KVMImage(self, img_conf, images[0])
> +        if img_conf.get('override_templates', False):
> +            image.update_templates(self.config.get('template_overrides', {}),
> +                                   self.config.get('template_files', {}))
> +        return image
> +
> +    def create_image(self, properties, config, features,
> +                     src_img_path, image_desc=None, use_desc=None,
> +                     user_data=None, meta_data=None):
> +        """Create an image
> +
> +        @param src_image_path: image path to launch from

s/src_image_path/src_img_path/

> +        @param properties: image properties
> +        @param config: image configuration
> +        @param features: image features
> +        @param image_desc: description of image being launched
> +        @param use_desc: description of container's use
> +        @return_value: cloud_tests.instances instance
> +        """
> +        name = util.gen_instance_name(image_desc=image_desc, use_desc=use_desc)
> +        img_path = os.path.join(self.config['data_dir'], name + '.qcow2')
> +        shutil.copy2(src_img_path, img_path)
> +
> +        # create copy of the latest image, return that as an instance
> +        return kvm_instance.KVMInstance(self, img_path, properties, config,
> +                                        features, user_data, meta_data)
> +
> +    def destroy(self):

If we aren't doing anything here, no need for the empty subclass method definition, the superclass does this for us anyway.

> +        """Clean up platform data."""
> +        super(KVMPlatform, self).destroy()
> +
> +# vi: ts=4 expandtab


-- 
https://code.launchpad.net/~powersj/cloud-init/+git/cloud-init/+merge/327646
Your team cloud-init commiters is requested to review the proposed merge of ~powersj/cloud-init:cii-kvm into cloud-init:master.