Merge "Fix broken use of pre-existing cloud images" into feature/zuulv3

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