cloud-init-dev team mailing list archive

[Chad Smith <chad.smith@xxxxxxxxxxxxx>]

Looks good, initial comments and running through tests now.

Diff comments:

> diff --git a/tests/cloud_tests/images/kvm.py b/tests/cloud_tests/images/kvm.py
> new file mode 100644
> index 0000000..008acef
> --- /dev/null
> +++ b/tests/cloud_tests/images/kvm.py
> @@ -0,0 +1,75 @@
> +# 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

since you are documenting so well, can you describe img_path param.

> +        """
> +        self.modified = False
> +        self._instance = None
> +        self._img_path = img_path
> +
> +        super(KVMImage, self).__init__(platform, config)
> +
> +    @property
> +    def instance(self):
> +        """Property function."""

docstring doesn't really apply here, what is the instance property an image instance maybe?

> +        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):
> +        """{} containing: 'arch', 'os', 'version', 'release'."""

s/{}/Dictionary/

> +        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):
> +        """Clean up data associated with image."""
> +        self._img_path = None

Why do we not remove the image at _img_path? Given that we unset it, I expect super(KVMImage, self).destroy() doesn't do any removal.
Can we add a comment about why we do this?

> +        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..c855e07
> --- /dev/null
> +++ b/tests/cloud_tests/instances/kvm.py
> @@ -0,0 +1,222 @@
> +# This file is part of cloud-init. See LICENSE file for license information.
> +
> +"""Base KVM instance."""
> +import os

please add an empty line between docstr and imports

> +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: image properties

Is this a list, dict, tuple etc? Can you describe the type a bit more so we don't have to look it up? same really for config and features. Ultimately this should have been done as well in instances/base.py but I see we don't have that there.

> +        @param config: image config
> +        @param features: 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])
> +            os.remove(self.pid_file)
> +        super(KVMInstance, self).destroy()
> +
> +    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:
> +            out, err, exit = self.ssh(command)
> +        else:
> +            out, err, exit = self.mount_image_callback(command)
> +
> +        # write data to file descriptors if needed
> +        if stdout:
> +            stdout.write(out.readlines())
> +        if stderr:
> +            stderr.write(err.readlines())
> +
> +        # if the command exited with a code not allowed in rcs, then fail
> +        if exit not in (rcs if rcs else (0,)):
> +            error_desc = ('Failed command to: {}'.format(description)
> +                          if description else None)
> +            raise util.InTargetExecuteError(
> +                out, err, exit, command, self.name, error_desc)
> +
> +        return (out, err, exit)
> +
> +    def mount_image_callback(self, command,):
> +        """Run mount-image-callback."""
> +        img_shell = subprocess.Popen(['sudo', 'mount-image-callback',
> +                                      '--system-mounts', '--system-resolvconf',
> +                                      self.name, 'chroot', '_MOUNTPOINT_',
> +                                      '/bin/sh'],
> +                                     stdin=subprocess.PIPE,
> +                                     stdout=subprocess.PIPE,
> +                                     stderr=subprocess.PIPE)
> +        out, err = img_shell.communicate(command.encode())
> +        exit = img_shell.returncode
> +        img_shell.stdin.close()
> +        img_shell.wait()
> +
> +        return out, err, exit
> +
> +    def generate_seed(self, tmpdir):
> +        """Generate nocloud seed from user-data"""
> +        seed_file = os.path.join(tmpdir, 'seed.img')
> +        user_data_file = os.path.join(tmpdir, 'user_data')
> +
> +        if os.path.exists(seed_file):
> +            os.remove(seed_file)
> +        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:
> +            command = ('sudo mount-image-callback --system-mounts '
> +                       '--system-resolvconf %s -- chroot _MOUNTPOINT_ '
> +                       '/bin/sh -ec "cat > %s" < %s'
> +                       % (self.name, remote_path, local_path))
> +            img_shell = subprocess.Popen(command, shell=True,
> +                                         stdin=subprocess.PIPE,
> +                                         stdout=subprocess.PIPE,
> +                                         stderr=subprocess.PIPE)
> +            out, err = img_shell.communicate()
> +            img_shell.stdin.close()
> +            img_shell.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)
> +                break
> +            except paramiko.SSHException:
> +                time.sleep(1)
> +                retry_attempts = retry_attempts - 1
> +
> +        return client
> +
> +    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, 'pid')
> +        self.ssh_port = self.get_free_port()
> +
> +        cmd = ('qemu-system-x86_64 -enable-kvm -m 1024 '
> +               '-pidfile %s -vnc none '
> +               '-drive file=%s,format=qcow2,if=virtio '
> +               '-drive file=%s,format=raw,if=virtio '
> +               '-device virtio-net-pci,netdev=net00 '
> +               '-netdev type=user,id=net00,hostfwd=tcp::%s-:22'
> +               % (self.pid_file, self.name, seed, self.ssh_port))
> +
> +        subprocess.Popen(shlex.split(cmd), close_fds=True)
> +        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()
> +
> +        time.sleep(10)
> +
> +        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/setup_image.py b/tests/cloud_tests/setup_image.py
> index 8053a09..557b4b5 100644
> --- a/tests/cloud_tests/setup_image.py
> +++ b/tests/cloud_tests/setup_image.py
> @@ -188,7 +189,16 @@ def enable_repo(args, image):
>  
>      msg = 'enable repo: "{}" in target'.format(args.repo)
>      LOG.debug(msg)
> -    image.execute(['/bin/sh', '-c', cmd], description=msg)
> +    image.execute(cmd, description=msg)
> +
> +
> +def generate_ssh_keys(args, image):
> +    """Generate SSH keys to be used with image."""
> +    LOG.info('generating SSH keys')
> +    c_util.subp(['ssh-keygen', '-t', 'rsa', '-b', '4096',
> +                 '-f', '%s/id_rsa' % args.data_dir, '-P', '',

Since all we care about is args.data_dir here, can we make that explicit as an generate_ssh_keys param
i.e.generate_ssh_keys(data_dir, image)
Then the docstring is just 
"""Generate SSH Keys for the image in the provided data_dir directory."""


I don't really understand why we pass in image arg here either, we don't use it.

> +                 '-C', 'ubuntu@cloud_test'],
> +                capture=True)
>  
>  
>  def setup_image(args, image):


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