Fix broken use of pre-existing cloud images

The existing code to check for a label being ready was using the label
name to query the cloud for image existence. The label name and the
name used to communicate with the remote cloud are not necessarily the
same.

Change the config reader to put the ProviderCloudImage into the Label's
cloud_image attribute, which allows validating that the cloud-image
referenced in a label definition matches the name of an existing
ProviderCloudImage.

Add accessor properties on the ProviderCloudImage to get the thing we
pass to the cloud (external) and the human readable name version of the
name of the image on the remote cloud for logging (image_name).

Note: This will need to be reworked a bit with the upcoming pluggable
config. The name, image_id, image_name split is done the way it is
currently because of how shade works and so is likely a feature of an
OpenStackProviderCloudImage not a general CloudImage. (or maybe I'm
wrong and it's a general thing, but I don't think I'm wrong)

Change-Id: I8411c627f9136339d1b0eb35632d6b2a27ab7a81
Co-Authored-By: Monty Taylor <mordred@inaugust.com>
Co-Authored-By: David Shrewsbury <shrewsbury.dave@gmail.com>

doc/source/configuration.rst

@@ -542,8 +542,10 @@ Example configuration::
     Refers to provider's diskimages, see :ref:`provider_diskimages`.
 
   ``cloud-image``
-    Refers to an externally managed image name or id already existing on the
-    provider, see :ref:`provider_cloud_images`.
+    Refers to the name of an externally managed image in the cloud that already
+    exists on the provider. The value of ``cloud-image`` should match the
+    ``name`` of a previously configured entry from the ``cloud-images`` section
+    of the provider. See :ref:`provider_cloud_images`.
 
 **at least one of**
 

nodepool/config.py

@@ -87,6 +87,19 @@ class ProviderCloudImage(ConfigValue):
     def __repr__(self):
         return "<ProviderCloudImage %s>" % self.name
 
+    @property
+    def external(self):
+        '''External identifier to pass to the cloud.'''
+        if self.image_id:
+            return dict(id=self.image_id)
+        else:
+            return self.image_name or self.name
+
+    @property
+    def external_name(self):
+        '''Human readable version of external.'''
+        return self.image_id or self.image_name or self.name
+
 
 class Label(ConfigValue):
     def __repr__(self):
@@ -290,7 +303,17 @@ def loadConfig(config_path):
                     pl.diskimage = newconfig.diskimages[diskimage]
                 else:
                     pl.diskimage = None
-                pl.cloud_image = label.get('cloud-image', None)
+                cloud_image_name = label.get('cloud-image', None)
+                if cloud_image_name:
+                    cloud_image = p.cloud_images.get(cloud_image_name, None)
+                    if not cloud_image:
+                        raise ValueError(
+                            "cloud-image %s does not exist in provider %s"
+                            " but is referenced in label %s" %
+                            (cloud_image_name, p.name, pl.name))
+                else:
+                    cloud_image = None
+                pl.cloud_image = cloud_image
                 pl.min_ram = label.get('min-ram', 0)
                 pl.flavor_name = label.get('flavor-name', None)
                 pl.key_name = label.get('key-name')

nodepool/driver/openstack/handler.py

@@ -62,7 +62,6 @@ class NodeLauncher(threading.Thread, stats.StatsReporter):
             self._diskimage = self._provider.diskimages[self._label.diskimage.name]
         else:
             self._diskimage = None
-        self._cloud_image = self._provider.cloud_images.get(self._label.cloud_image, None)
 
     def logConsole(self, server_id, hostname):
         if not self._label.console_log:
@@ -97,22 +96,11 @@ class NodeLauncher(threading.Thread, stats.StatsReporter):
 
         else:
             # launch using unmanaged cloud image
-            config_drive = self._cloud_image.config_drive
+            config_drive = self._label.cloud_image.config_drive
 
-            # These are different values for zk, but it's all the same
-            # for cloud-images.
-            # image_external is what we use for OpenStack.
-            # image_id is what we record in the node for zk.
-            # image_name is what we log, so matches the config.
-            image_external = self._cloud_image.name
-            if self._cloud_image.image_id:
-                image_external = dict(id=self._cloud_image.image_id)
-            elif self._cloud_image.image_name:
-                image_external = self._cloud_image.image_name
-            else:
-                image_external = self._cloud_image.name
-            image_id = self._cloud_image.name
-            image_name = self._cloud_image.name
+            image_external = self._label.cloud_image.external
+            image_id = self._label.cloud_image.name
+            image_name = self._label.cloud_image.name
 
             # TODO(tobiash): support username also for unmanaged cloud images
             username = None
@@ -308,13 +296,12 @@ class OpenStackNodeRequestHandler(NodeRequestHandler):
         for label in self.request.node_types:
 
             if self.pool.labels[label].cloud_image:
-                img = self.pool.labels[label].cloud_image
-                if not self.manager.labelReady(img):
+                if not self.manager.labelReady(self.pool.labels[label]):
                     return False
             else:
-                img = self.pool.labels[label].diskimage.name
-
-                if not self.zk.getMostRecentImageUpload(img, self.provider.name):
+                if not self.zk.getMostRecentImageUpload(
+                        self.pool.labels[label].diskimage.name,
+                        self.provider.name):
                     return False
         return True
 

nodepool/driver/openstack/provider.py

@@ -273,8 +273,20 @@ class OpenStackProvider(Provider):
         with shade_inner_exceptions():
             return self._client.get_image(image_id)
 
-    def labelReady(self, image_id):
-        return self.getImage(image_id)
+    def labelReady(self, label):
+        if not label.cloud_image:
+            return False
+        image = self.getImage(label.cloud_image.external)
+        if not image:
+            self.log.warning(
+                "Provider %s is configured to use %s as the"
+                " cloud-image for label %s and that"
+                " cloud-image could not be found in the"
+                " cloud." % (self.provider.name,
+                             label.cloud_image.external_name,
+                             label.name))
+            return False
+        return True
 
     def uploadImage(self, image_name, filename, image_type=None, meta=None,
             md5=None, sha256=None):

nodepool/launcher.py

@@ -796,13 +796,14 @@ class NodePool(threading.Thread):
                 # Provider doesn't manage images, assuming label is ready
                 return True
             for pool_label in pool.labels.values():
-                if pool_label.cloud_image:
-                    manager = self.getProviderManager(pool.provider.name)
-                    if manager.labelReady(pool_label.cloud_image):
+                if pool_label.diskimage:
+                    if self.zk.getMostRecentImageUpload(
+                            pool_label.diskimage.name, pool.provider.name):
+                        return True
+                else:
+                    manager = self.getProviderManager(pool.provider.name)
+                    if manager.labelReady(pool_label):
                         return True
-                elif self.zk.getMostRecentImageUpload(pool_label.diskimage.name,
-                                                      pool.provider.name):
-                    return True
         return False
 
     def createMinReady(self):

nodepool/tests/fixtures/unmanaged_image_provider_name.yaml

@@ -0,0 +1,28 @@
+elements-dir: .
+images-dir: '{images_dir}'
+
+zookeeper-servers:
+  - host: {zookeeper_host}
+    port: {zookeeper_port}
+    chroot: {zookeeper_chroot}
+
+labels:
+  - name: fake-label
+    min-ready: 1
+
+providers:
+  - name: fake-provider
+    cloud: fake
+    driver: fake
+    region-name: fake-region
+    rate: 0.0001
+    cloud-images:
+      - name: fake-image
+        image-name: provider-named-image
+    pools:
+      - name: main
+        max-servers: 96
+        labels:
+          - name: fake-label
+            cloud-image: fake-image
+            min-ram: 8192

nodepool/tests/test_launcher.py

@@ -647,6 +647,22 @@ class TestLauncher(tests.DBTestCase):
         nodes = self.waitForNodes('fake-label')
         self.assertEqual(len(nodes), 1)
 
+    def test_unmanaged_image_provider_name(self):
+        """
+        Test node launching using an unmanaged image referencing the
+        image name as known by the provider.
+        """
+        configfile = self.setup_config('unmanaged_image_provider_name.yaml')
+        pool = self.useNodepool(configfile, watermark_sleep=1)
+
+        pool.start()
+        self.wait_for_config(pool)
+        manager = pool.getProviderManager('fake-provider')
+        manager._client.create_image(name="provider-named-image")
+
+        nodes = self.waitForNodes('fake-label')
+        self.assertEqual(len(nodes), 1)
+
     def test_paused_gets_declined(self):
         """Test that a paused request, that later gets declined, unpauses."""