cloud-init-dev team mailing list archive

[Scott Moser <ssmoser2+ubuntu@xxxxxxxxx>]

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],

shell builtin 'read' is not binary safe.

> +        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])

yeah... the 'exec' of 'cat' is just used because other wise the 'sh' process sits around and waits for 'cat' to exit, and then exits with the exit code of cat.

Thats just pointless. 

(Note, that a very useful function of 'bash' is exec -a, which allows you do do:
   exec -a just-wait-around sleep 60m

so that then 'ps' shows a process named 'just-wait-around'.  Unfortunately that is not a posix function.

$ bash -c 'exec -a just-wat-around sleep 10m' &
[2] 13183
$ ps axw | grep 10m
13183 pts/10   S      0:00 just-wat-around 10m
13185 pts/10   S+     0:00 grep --color=auto 10m

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

fixed.

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

well, if you pass 'decode=True', then it gets decoded. so you get a string back.
otherwise bytes.
i'l update signature.

> +        """
> +        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

no. i expliciltly want the property *not* named 'instance'.
see explanation above.

> +
> +        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()
> +        return self._img_instance
>  
>      @property
>      def properties(self):
> 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
> @@ -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):

i didn't change this. They were there already.
I can drop them.

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