Merge ~smoser/cloud-init:fix/cii-kvmimage-preserve-original into cloud-init:master

[Scott Moser]

tests: NoCloudKVMImage do not modify the original local cache image.

The NoCloudKVMImage.execute() would modify the image in /srv/citest
that meant that after the first time you ran a test, the image was
dirty.

The change here is to make the image operate on a qcow backed image.

Also modify Snapshot to then copy the qcow rather
than creating another chained qcow. The reason being that the image
might go away but the snapshot stick around.

Also
 * drop use of 'override_templates' which was only relevant to LXD.
 * NoCloudKVM.create_image() returned an instance before
   now it has create_instance which creates an instance.
 * NoCloudKVMInstance has a 'disk' attribute separate from 'name'

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:b82bed1db430c94df690261f720cceda2e58a235
https://jenkins.ubuntu.com/server/job/cloud-init-ci/539/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/539/rebuild

review: Approve (continuous-integration)

[Joshua Powers (powersj)]

+1 This also makes sure we are able to properly use --preserve-data now with qcow2 files that are ready to boot in the event triage is needed, even if the original file is deleted.

Test with:

python3 -m tests.cloud_tests run --platform nocloud-kvm \
    --verbose --preserve-data --data-dir out --os-name xenial \
    -t modules/write_files -t modules/final_message \
    --deb cloud-init_17.1-41-g76243487-0ubuntu1_all.deb

I appreciate the cleanup of the variable names to be more specific as to what each is.

review: Approve

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:86255ca411f0ea99a13bb96aec6625e61c07008b
https://jenkins.ubuntu.com/server/job/cloud-init-ci/542/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/542/rebuild

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

What's the use case for the "destroy an image without breaking a snapshot" that introduces the copy of the image.

Some more comments inline as well.

[Scott Moser (smoser)]

An image is a mutable thing.
General flow of tests is:
 - get an image
 - update the image (install a new deb)
 - create a snapshot
 - launch a snapshot multiple times

After creating a snapshot of the image the image can reasonably be destroyed.
Just like in a "real cloud".
 - get an image (download a cloud image or boot an instance we can update/modify)
 - update the image
 - create a snapshot (publish to a new ami id)

At the point where we've created a new snapshot we no longer need the image.

As it was previously, if we deleted the Image that the snapshot was created from, then the snapshot (a qcow image) was broken.

[Ryan Harper (raharper)]

> An image is a mutable thing.
> General flow of tests is:
> - get an image
> - update the image (install a new deb)
> - create a snapshot
> - launch a snapshot multiple times
>
> After creating a snapshot of the image the image can reasonably be destroyed.
> Just like in a "real cloud".
> - get an image (download a cloud image or boot an instance we can
> update/modify)
> - update the image
> - create a snapshot (publish to a new ami id)
>
> At the point where we've created a new snapshot we no longer need the image.

Sure, but where in the workflow of our nightly tests will the images be purged,
or where in a developer workflow would the images actually get removed.
Does part of the make cloud-tests or whatever remove images?

>
> As it was previously, if we deleted the Image that the snapshot was created
> from, then the snapshot (a qcow image) was broken.

I'm not terribly concerned about manual deletion of images; Is there some
automatic removal of images that would let a tree rot such that invoking
a cloud-test at a later time would break without the copy?

27ff241... by Scott Moser on 2017-11-27

better error messages on multiple images found

[Scott Moser (smoser)]

the Image object has a destroy() method which is valid to call, and should be called whne you're "done" with the object.
  i = Image(foo)
  i.execute(['apt-get', 'dist-upgrade'])
  s = i.snapshot()
  i.destroy()
  s.launch()
  ...

the result of snapshot needs to be stand-alone, and before this change it was not.
As used above, s.launch() would have failed as it was qcow backed by i.

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:cc9905002cd5956d01c5aa31676be719b0170f5d
https://jenkins.ubuntu.com/server/job/cloud-init-ci/546/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/546/rebuild

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:27ff24127c0bd5074817a199eb373cae5ab83d29
https://jenkins.ubuntu.com/server/job/cloud-init-ci/547/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/547/rebuild

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

> the Image object has a destroy() method which is valid to call, and should be
> called whne you're "done" with the object.
> i = Image(foo)
> i.execute(['apt-get', 'dist-upgrade'])
> s = i.snapshot()
> i.destroy()
> s.launch()
> ...
>
> the result of snapshot needs to be stand-alone, and before this change it was
> not.
> As used above, s.launch() would have failed as it was qcow backed by i.

But is this called in this way via how users and CI interact with the test-suite?
I suspect it's not; and nothing is doing the above or the test-suite would have broken.

[Scott Moser (smoser)]

It looks like as it is right now we do not call destroy on an image before using a snapshot.
But having NoCloud work like other clouds makes sense.

As it is right now we only ever call snapshot wrapped in a context handler of an image.

[Ryan Harper (raharper)]

On Mon, Nov 27, 2017 at 4:31 PM, Scott Moser <email address hidden>
wrote:

>
> It looks like as it is right now we do not call destroy on an image before
> using a snapshot.
> But having NoCloud work like other clouds makes sense.
>

I don't disagree with the goal; however; just as I don't want to upload a
new snapshot
each time I launch a VM, I don't want to copy over the image each time I
launch one in
NoCloud.

Can we model the image manipulation like we would the clouds?

i = Image(foo)
i.execute("modifications here")
s = i.snapshot()
new_image = cloud.upload(s)
new_image.launch()

I think that makes it clear that one *could* delete the original image
second the upload allows for NoCloud to do a copy and return a new path to
the image
which hopefully we could re-use on subsequent runs

i = cloud.get_image("foo")
i.launch()

After the upload, I think it'd be fair for cloud.get_image() to return a
qcow2 image backed against the "uploaded" snapshot which would
avoid the copy and allow the original image to be deleted (not that we have
a use-case for that).

>
> As it is right now we only ever call snapshot wrapped in a context handler
> of an image.
>
> --
> https://code.launchpad.net/~smoser/cloud-init/+git/cloud-
> init/+merge/334147
> Your team cloud-init commiters is requested to review the proposed merge
> of ~smoser/cloud-init:fix/cii-kvmimage-preserve-original into
> cloud-init:master.
>
> _______________________________________________
> Mailing list: https://launchpad.net/~cloud-init-dev
> Post to : <email address hidden>
> Unsubscribe : https://launchpad.net/~cloud-init-dev
> More help : https://help.launchpad.net/ListHelp
>

[Ryan Harper (raharper)]

review: Approve

diff --git a/tests/cloud_tests/images/nocloudkvm.py b/tests/cloud_tests/images/nocloudkvm.py
index 1e7962c..8678b07 100644
--- a/tests/cloud_tests/images/nocloudkvm.py
+++ b/tests/cloud_tests/images/nocloudkvm.py
@@ -4,6 +4,10 @@
 
 from cloudinit import util as c_util
 
+import os
+import shutil
+import tempfile
+
 from tests.cloud_tests.images import base
 from tests.cloud_tests.snapshots import nocloudkvm as nocloud_kvm_snapshot
 
@@ -13,7 +17,7 @@ class NoCloudKVMImage(base.Image):
 
     platform_name = "nocloud-kvm"
 
-    def __init__(self, platform, config, img_path):
+    def __init__(self, platform, config, orig_img_path):
         """Set up image.
 
         @param platform: platform object
@@ -21,7 +25,13 @@ class NoCloudKVMImage(base.Image):
         @param img_path: path to the image
         """
         self.modified = False
-        self._img_path = img_path
+        self._workd = tempfile.mkdtemp(prefix='NoCloudKVMImage')
+        self._orig_img_path = orig_img_path
+        self._img_path = os.path.join(self._workd,
+                                      os.path.basename(self._orig_img_path))
+
+        c_util.subp(['qemu-img', 'create', '-f', 'qcow2',
+                    '-b', orig_img_path, self._img_path])
 
         super(NoCloudKVMImage, self).__init__(platform, config)
 
@@ -61,13 +71,9 @@ class NoCloudKVMImage(base.Image):
         if not self._img_path:
             raise RuntimeError()
 
-        instance = self.platform.create_image(
-            self.properties, self.config, self.features,
-            self._img_path, image_desc=str(self), use_desc='snapshot')
-
         return nocloud_kvm_snapshot.NoCloudKVMSnapshot(
             self.platform, self.properties, self.config,
-            self.features, instance)
+            self.features, self._img_path)
 
     def destroy(self):
         """Unset path to signal image is no longer used.
@@ -77,6 +83,8 @@ class NoCloudKVMImage(base.Image):
         framework decide whether to keep or destroy everything.
         """
         self._img_path = None
+        shutil.rmtree(self._workd)
+
         super(NoCloudKVMImage, self).destroy()
 
 # vi: ts=4 expandtab
diff --git a/tests/cloud_tests/instances/nocloudkvm.py b/tests/cloud_tests/instances/nocloudkvm.py
index cc82580..bc06a79 100644
--- a/tests/cloud_tests/instances/nocloudkvm.py
+++ b/tests/cloud_tests/instances/nocloudkvm.py
@@ -25,12 +25,13 @@ class NoCloudKVMInstance(base.Instance):
     platform_name = "nocloud-kvm"
     _ssh_client = None
 
-    def __init__(self, platform, name, properties, config, features,
-                 user_data, meta_data):
+    def __init__(self, platform, name, image_path, properties, config,
+                 features, user_data, meta_data):
         """Set up instance.
 
         @param platform: platform object
         @param name: image path
+        @param image_path: path to disk image to boot.
         @param properties: dictionary of properties
         @param config: dictionary of configuration values
         @param features: dictionary of supported feature flags
@@ -43,6 +44,7 @@ class NoCloudKVMInstance(base.Instance):
         self.pid = None
         self.pid_file = None
         self.console_file = None
+        self.disk = image_path
 
         super(NoCloudKVMInstance, self).__init__(
             platform, name, properties, config, features)
@@ -145,7 +147,7 @@ class NoCloudKVMInstance(base.Instance):
         self.ssh_port = self.get_free_port()
 
         cmd = ['./tools/xkvm',
-               '--disk', '%s,cache=unsafe' % self.name,
+               '--disk', '%s,cache=unsafe' % self.disk,
                '--disk', '%s,cache=unsafe' % seed,
                '--netdev', ','.join(['user',
                                      'hostfwd=tcp::%s-:22' % self.ssh_port,
diff --git a/tests/cloud_tests/platforms/nocloudkvm.py b/tests/cloud_tests/platforms/nocloudkvm.py
index f1f8187..76cd83a 100644
--- a/tests/cloud_tests/platforms/nocloudkvm.py
+++ b/tests/cloud_tests/platforms/nocloudkvm.py
@@ -55,19 +55,20 @@ class NoCloudKVMPlatform(base.Platform):
         for fname in glob.iglob(search_d, recursive=True):
             images.append(fname)
 
-        if len(images) != 1:
-            raise Exception('No unique images found')
+        if len(images) < 1:
+            raise RuntimeError("No images found under '%s'" % search_d)
+        if len(images) > 1:
+            raise RuntimeError(
+                "Multiple images found in '%s': %s" % (search_d,
+                                                       ' '.join(images)))
 
         image = nocloud_kvm_image.NoCloudKVMImage(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
+    def create_instance(self, properties, config, features,
+                        src_img_path, image_desc=None, use_desc=None,
+                        user_data=None, meta_data=None):
+        """Create an instance
 
         @param src_img_path: image path to launch from
         @param properties: image properties
@@ -82,7 +83,7 @@ class NoCloudKVMPlatform(base.Platform):
         c_util.subp(['qemu-img', 'create', '-f', 'qcow2',
                     '-b', src_img_path, img_path])
 
-        return nocloud_kvm_instance.NoCloudKVMInstance(self, img_path,
+        return nocloud_kvm_instance.NoCloudKVMInstance(self, name, img_path,
                                                        properties, config,
                                                        features, user_data,
                                                        meta_data)
diff --git a/tests/cloud_tests/snapshots/nocloudkvm.py b/tests/cloud_tests/snapshots/nocloudkvm.py
index 0999834..21e908d 100644
--- a/tests/cloud_tests/snapshots/nocloudkvm.py
+++ b/tests/cloud_tests/snapshots/nocloudkvm.py
@@ -2,6 +2,8 @@
 
 """Base NoCloud KVM snapshot."""
 import os
+import shutil
+import tempfile
 
 from tests.cloud_tests.snapshots import base
 
@@ -11,16 +13,19 @@ class NoCloudKVMSnapshot(base.Snapshot):
 
     platform_name = "nocloud-kvm"
 
-    def __init__(self, platform, properties, config, features,
-                 instance):
+    def __init__(self, platform, properties, config, features, image_path):
         """Set up snapshot.
 
         @param platform: platform object
         @param properties: image properties
         @param config: image config
         @param features: supported feature flags
+        @param image_path: image file to snapshot.
         """
-        self.instance = instance
+        self._workd = tempfile.mkdtemp(prefix='NoCloudKVMSnapshot')
+        snapshot = os.path.join(self._workd, 'snapshot')
+        shutil.copyfile(image_path, snapshot)
+        self._image_path = snapshot
 
         super(NoCloudKVMSnapshot, self).__init__(
             platform, properties, config, features)
@@ -40,9 +45,9 @@ class NoCloudKVMSnapshot(base.Snapshot):
                                 self.platform.config['public_key'])
         user_data = self.inject_ssh_key(user_data, key_file)
 
-        instance = self.platform.create_image(
+        instance = self.platform.create_instance(
             self.properties, self.config, self.features,
-            self.instance.name, image_desc=str(self), use_desc=use_desc,
+            self._image_path, image_desc=str(self), use_desc=use_desc,
             user_data=user_data, meta_data=meta_data)
 
         if start:
@@ -68,7 +73,7 @@ class NoCloudKVMSnapshot(base.Snapshot):
 
     def destroy(self):
         """Clean up snapshot data."""
-        self.instance.destroy()
+        shutil.rmtree(self._workd)
         super(NoCloudKVMSnapshot, self).destroy()
 
 # vi: ts=4 expandtab