cloud-init-dev team mailing list archive

[Ryan Harper <ryan.harper@xxxxxxxxxxxxx>]

I'm not sure I understand the dropping of instance property from the NoCloud KVM class vs. the LXD one.

Diff comments:

> diff --git a/tests/cloud_tests/execute_basics.py b/tests/cloud_tests/execute_basics.py
> new file mode 100644
> index 0000000..f679b11
> --- /dev/null
> +++ b/tests/cloud_tests/execute_basics.py
> @@ -0,0 +1,45 @@
> +# This file is part of cloud-init. See LICENSE file for license information.
> +"""
> +execute_basics provides higher level operations that utilize only
> +a functional 'execute' and some programs expected to be available.
> +
> +This list of expected programs is currently:
> +  * cat
> +  * sh
> +
> +The functions here expect that 'execute' has a signature as described in
> +Instance class.
> +"""
> +
> +
> +def write_data(execute, remote_path, data):
> +    """Write data to path remote_path."""
> +
> +    # when sh is invoked with '-c', then the first argument is "$0"
> +    # which is commonly understood as the "program name".
> +    # 'write_data' is the program name, and 'remote_path' is '$1'
> +    _, _, rc = execute(
> +        ["sh", "-c", 'exec cat >"$1"', 'write_data', remote_path],

If you wanted to drop cat, you could just use shell 'read';

sh -c 'remote_path=$1; read input_data; echo $input_data >"$remote_path"', program_name, remote_path

> +        stdin=data)
> +
> +    if rc != 0:
> +        raise RuntimeError("Failed to write to '%s'" % remote_path)
> +    return
> +
> +
> +def read_data(execute, remote_path, decode=False):
> +    """read and return the content of 'remote_path'."""
> +
> +    # when sh is invoked with '-c', then the first argument is "$0"
> +    # which is commonly understood as the "program name".
> +    # 'read_data' is the program name, and 'remote_path' is '$1'
> +    stdout, stderr, rc = execute(
> +        ["sh", "-c", 'exec cat "$1"', 'read_data', remote_path])

is the exec needed to get the $0 program-name behavior?  otherwise, I thought exec <foo> means the program name what is exec'ed?

> +    if rc != 0:
> +        raise RuntimeError("Failed to load file '%s'" % remote_path)

nit, s/load/read

> +
> +    if decode:
> +        return stdout.decode()
> +    return stdout
> +
> +# vi: ts=4 expandtab
> diff --git a/tests/cloud_tests/images/base.py b/tests/cloud_tests/images/base.py
> index 0a1e056..5bd640c 100644
> --- a/tests/cloud_tests/images/base.py
> +++ b/tests/cloud_tests/images/base.py
> @@ -47,9 +49,40 @@ class Image(object):
>          """Execute command in image, modifying image."""
>          raise NotImplementedError
>  
> +    def read_data(self, remote_path, decode=False):
> +        """Read data from instance filesystem.
> +
> +        @param remote_path: path in instance
> +        @param decode: return as string
> +        @return_value: data as str or bytes

Is this true?  the input stream converts the bytes? or since it's going to shell it doesn't matter?

> +        """
> +        return execute_basics.read_data(
> +            self.execute, remote_path=remote_path, decode=decode)
> +
> +    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
> +        """
> +        return execute_basics.write_data(self.execute, remote_path, data)
> +
> +    def pull_file(self, remote_path, local_path):
> +        """Copy file at 'remote_path', from instance to 'local_path'.
> +
> +        @param remote_path: path on remote instance
> +        @param local_path: path on local instance
> +        """
> +        with open(local_path, 'wb') as fp:
> +            fp.write(self.read_data(remote_path))
> +
>      def push_file(self, local_path, remote_path):
> -        """Copy file at 'local_path' to instance at 'remote_path'."""
> -        raise NotImplementedError
> +        """Copy file at 'local_path' to instance at 'remote_path'.
> +
> +        @param local_path: path on local instance
> +        @param remote_path: path on remote instance"""
> +        with open(local_path, "rb") as fp:
> +            self.write_data(remote_path, data=fp.read())
>  
>      def run_script(self, *args, **kwargs):
>          """Run script in image, modifying image.
> diff --git a/tests/cloud_tests/images/lxd.py b/tests/cloud_tests/images/lxd.py
> index fd4e93c..fbd5196 100644
> --- a/tests/cloud_tests/images/lxd.py
> +++ b/tests/cloud_tests/images/lxd.py
> @@ -49,15 +49,19 @@ class LXDImage(base.Image):
>          self._pylxd_image = pylxd_image
>  
>      @property
> -    def instance(self):
> -        """Property function."""
> -        if not self._instance:
> -            self._instance = self.platform.launch_container(
> +    def _instance(self):
> +        """Internal use only, returns a instance

I think you want restructure this:

@property
def instance(self):
   if not self._instance:
      self._instance = _img_instance()
   return self._instance

def _img_instance(self)
   """ Internal only """
   <do stuff here to create and start an instance>

> +
> +        This starts an lxc instance from the image, so it is "dirty".
> +        Better would be some way to modify this "at rest".
> +        lxc-pstart would be an option."""
> +        if not self._img_instance:
> +            self._img_instance = self.platform.launch_container(
>                  self.properties, self.config, self.features,
>                  use_desc='image-modification', image_desc=str(self),
>                  image=self.pylxd_image.fingerprint)
> -        self._instance.start()
> -        return self._instance
> +        self._img_instance.start()

I don't think you want the @property accessory to always call start; isn't that only needed the first time you create the _img_instance?

> +        return self._img_instance
>  
>      @property
>      def properties(self):
> @@ -146,18 +150,18 @@ class LXDImage(base.Image):
>  
>      def execute(self, *args, **kwargs):
>          """Execute command in image, modifying image."""
> -        return self.instance.execute(*args, **kwargs)
> +        return self._instance.execute(*args, **kwargs)

this shouldn't be needed, if you call the propery, that takes care of setting up and starting an instance.

>  
>      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)
> +        return self._instance.push_file(local_path, remote_path)

same here

>  
>      def run_script(self, *args, **kwargs):
>          """Run script in image, modifying image.
>  
>          @return_value: script output
>          """
> -        return self.instance.run_script(*args, **kwargs)
> +        return self._instance.run_script(*args, **kwargs)

and here

>  
>      def snapshot(self):
>          """Create snapshot of image, block until done."""
> @@ -169,7 +173,7 @@ class LXDImage(base.Image):
>          # clone current instance
>          instance = self.platform.launch_container(
>              self.properties, self.config, self.features,
> -            container=self.instance.name, image_desc=str(self),
> +            container=self._instance.name, image_desc=str(self),

and here?

>              use_desc='snapshot', container_config=conf)
>          # wait for cloud-init before boot_clean_script is run to ensure
>          # /var/lib/cloud is removed cleanly
> diff --git a/tests/cloud_tests/images/nocloudkvm.py b/tests/cloud_tests/images/nocloudkvm.py
> index a7af0e5..5bbd50d 100644
> --- a/tests/cloud_tests/images/nocloudkvm.py
> +++ b/tests/cloud_tests/images/nocloudkvm.py
> @@ -19,24 +21,11 @@ class NoCloudKVMImage(base.Image):
>          @param img_path: path to the image
>          """
>          self.modified = False
> -        self._instance = None
>          self._img_path = img_path
>  
>          super(NoCloudKVMImage, self).__init__(platform, config)
>  
>      @property
> -    def instance(self):

why is this dropped ?

> -        """Returns an instance of an image."""
> -        if not self._instance:
> -            if not self._img_path:
> -                raise RuntimeError()
> -
> -            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 {
> @@ -46,20 +35,30 @@ class NoCloudKVMImage(base.Image):
>              'version': self.config['version'],
>          }
>  
> -    def execute(self, *args, **kwargs):
> +    def execute(self, command, stdin=None, env=None,
> +                rcs=None, description=None):

description and rcs are accepted, but not used?

>          """Execute command in image, modifying image."""
> -        return self.instance.execute(*args, **kwargs)
> +        if isinstance(command, str):
> +            command = ['sh', '-c', command]
>  
> -    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)
> +        return self.mount_image_callback(command, stdin=stdin, env=env)
>  
> -    def run_script(self, *args, **kwargs):
> -        """Run script in image, modifying image.
> +    def mount_image_callback(self, command, stdin=None, env=None):
> +        """Run mount-image-callback."""
>  
> -        @return_value: script output
> -        """
> -        return self.instance.run_script(*args, **kwargs)
> +        env_args = []
> +        if env:
> +            env_args = ['env'] + ["%s=%s" for k, v in env.items()]
> +
> +        mic_chroot = ['sudo', 'mount-image-callback', '--system-mounts',
> +                      '--system-resolvconf', self._img_path,
> +                      '--', 'chroot', '_MOUNTPOINT_']
> +        try:
> +            out, err = c_util.subp(mic_chroot + env_args + list(command),
> +                                   data=stdin)
> +            return (out, err, 0)
> +        except c_util.ProcessExecutionError as e:
> +            return (e.stdout, e.stderr, e.exit_code)
>  
>      def snapshot(self):
>          """Create snapshot of image, block until done."""


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