Rollup of volume fixes.

* Standardizes volume attachment wording.
  * Defensive coding for missing values in attachment dict.
    Fixes bug 1004412 and bug 1004337.
  * Adds polling to Snapshots and Volume Snapshots tables.
    Fixes bug 1005805.
  * Removes an erroneous link on the Volume Snapshots table.
    Fixes bug 1005806.
  * Correts the "type" property on the Images and Snapshots tables.
    Fixes bug 1011910.

This seems to restore all supported functionality related to volumes using
devstack master.

Change-Id: Ie9b7aec06fa1bb7628cd854fb49c02aab14451ea

horizon/api/nova.py

@@ -485,9 +485,14 @@ def volume_get(request, volume_id):
     volume_data = cinderclient(request).volumes.get(volume_id)
 
     for attachment in volume_data.attachments:
-        instance = server_get(request, attachment['server_id'])
+        if "server_id" in attachment:
-        attachment[u'instance_name'] = instance.name
+            instance = server_get(request, attachment['server_id'])
-
+            attachment['instance_name'] = instance.name
+        else:
+            # Nova volume can occasionally send back error'd attachments
+            # the lack a server_id property; to work around that we'll
+            # give the attached instance a generic name.
+            attachment['instance_name'] = _("Unknown instance")
     return volume_data
 
 
@@ -516,9 +521,12 @@ def volume_attach(request, volume_id, instance_id, device):
                                                      device)
 
 
-def volume_detach(request, instance_id, attachment_id):
+def volume_detach(request, instance_id, att_id):
-    novaclient(request).volumes.delete_server_volume(
+    novaclient(request).volumes.delete_server_volume(instance_id, att_id)
-            instance_id, attachment_id)
+
+
+def volume_snapshot_get(request, snapshot_id):
+    return cinderclient(request).volume_snapshots.get(snapshot_id)
 
 
 def volume_snapshot_list(request):

horizon/dashboards/nova/access_and_security/keypairs/forms.py

@@ -24,9 +24,9 @@ from django import shortcuts
 from django.contrib import messages
 from django.core import validators
 from django.utils.translation import ugettext_lazy as _
-from novaclient import exceptions as novaclient_exceptions
 
 from horizon import api
+from horizon import exceptions
 from horizon import forms
 
 
@@ -46,10 +46,9 @@ class CreateKeypair(forms.SelfHandlingForm):
             return shortcuts.redirect(
                     'horizon:nova:access_and_security:keypairs:download',
                     keypair_name=data['name'])
-        except novaclient_exceptions.ClientException, e:
+        except:
-            LOG.exception("ClientException in CreateKeyPair")
+            exceptions.handle(request,
-            messages.error(request,
+                              _('Unable to create keypair.'))
-                           _('Error Creating Keypair: %s') % e.message)
             return shortcuts.redirect(request.build_absolute_uri())
 
 
@@ -66,8 +65,7 @@ class ImportKeypair(forms.SelfHandlingForm):
                                        % data['name'])
             return shortcuts.redirect(
                             'horizon:nova:access_and_security:index')
-        except novaclient_exceptions.ClientException, e:
+        except:
-            LOG.exception("ClientException in ImportKeypair")
+            exceptions.handle(request,
-            messages.error(request,
+                              _('Unable to import keypair.'))
-                           _('Error Importing Keypair: %s') % e.message)
             return shortcuts.redirect(request.build_absolute_uri())

horizon/dashboards/nova/images_and_snapshots/images/tables.py

@@ -77,7 +77,7 @@ class EditImage(tables.LinkAction):
 
 
 def get_image_type(image):
-    return getattr(image.properties, "image_type", "Image")
+    return getattr(image, "properties", {}).get("image_type", _("Image"))
 
 
 def get_format(image):

horizon/dashboards/nova/images_and_snapshots/snapshots/tables.py

@@ -21,7 +21,7 @@ from django.utils.http import urlencode
 from django.utils.translation import ugettext_lazy as _
 
 from horizon import tables
-from ..images.tables import ImagesTable, EditImage, DeleteImage
+from ..images.tables import ImagesTable, EditImage, DeleteImage, UpdateRow
 
 
 LOG = logging.getLogger(__name__)
@@ -55,3 +55,5 @@ class SnapshotsTable(ImagesTable):
         table_actions = (DeleteSnapshot,)
         row_actions = (LaunchSnapshot, EditImage, DeleteSnapshot)
         pagination_param = "snapshot_marker"
+        row_class = UpdateRow
+        status_columns = ["status"]

horizon/dashboards/nova/images_and_snapshots/volume_snapshots/tables.py

@@ -34,11 +34,23 @@ class DeleteVolumeSnapshot(tables.DeleteAction):
         api.volume_snapshot_delete(request, obj_id)
 
 
+class UpdateRow(tables.Row):
+    ajax = True
+
+    def get_data(self, request, snapshot_id):
+        snapshot = api.nova.volume_snapshot_get(request, snapshot_id)
+        return snapshot
+
+
 class VolumeSnapshotsTable(volume_tables.VolumesTableBase):
-    volume_id = tables.Column("volume_id", verbose_name=_("Volume ID"))
+    name = tables.Column("display_name", verbose_name=_("Name"))
+    volume_id = tables.Column("volume_id",
+                              verbose_name=_("Volume ID"))
 
     class Meta:
         name = "volume_snapshots"
         verbose_name = _("Volume Snapshots")
         table_actions = (DeleteVolumeSnapshot,)
         row_actions = (DeleteVolumeSnapshot,)
+        row_class = UpdateRow
+        status_columns = ("status",)

horizon/dashboards/nova/instances_and_volumes/tests.py

@@ -88,7 +88,7 @@ class InstancesAndVolumesViewTest(test.TestCase):
         self.assertContains(res, ">80GB<", 1, 200)
         self.assertContains(res, ">In-Use<", 1, 200)
         self.assertContains(res, ">server_1<", 2, 200)
-        self.assertContains(res, "(/dev/hdn)", 1, 200)
+        self.assertContains(res, "on /dev/hdn", 1, 200)
 
     def test_index_server_list_exception(self):
         self.mox.StubOutWithMock(api, 'server_list')

horizon/dashboards/nova/instances_and_volumes/views.py

@@ -25,10 +25,8 @@ Views for Instances and Volumes.
 import re
 import logging
 
-from django.contrib import messages
 from django.utils.translation import ugettext_lazy as _
 from django.utils.datastructures import SortedDict
-from novaclient import exceptions as novaclient_exceptions
 
 from horizon import api
 from horizon import exceptions
@@ -45,24 +43,28 @@ class IndexView(tables.MultiTableView):
     template_name = 'nova/instances_and_volumes/index.html'
 
     def get_instances_data(self):
-        # Gather our instances
+        if not hasattr(self, "_instances"):
-        try:
+            # Gather our instances
-            instances = self._get_instances()
-        except:
-            instances = []
-            exceptions.handle(self.request, _('Unable to retrieve instances.'))
-        # Gather our flavors and correlate our instances to them
-        if instances:
             try:
-                flavors = api.flavor_list(self.request)
+                instances = self._get_instances()
-                full_flavors = SortedDict([(str(flavor.id), flavor) for \
-                                            flavor in flavors])
-                for instance in instances:
-                    instance.full_flavor = full_flavors[instance.flavor["id"]]
             except:
-                msg = _('Unable to retrieve instance size information.')
+                instances = []
-                exceptions.handle(self.request, msg)
+                exceptions.handle(self.request,
-        return instances
+                                  _('Unable to retrieve instances.'))
+            # Gather our flavors and correlate our instances to them
+            if instances:
+                try:
+                    flavors = api.flavor_list(self.request)
+                    full_flavors = SortedDict([(str(flavor.id), flavor) for \
+                                                flavor in flavors])
+                    for instance in instances:
+                        flavor_id = instance.flavor["id"]
+                        instance.full_flavor = full_flavors[flavor_id]
+                except:
+                    msg = _('Unable to retrieve instance size information.')
+                    exceptions.handle(self.request, msg)
+            self._instances = instances
+        return self._instances
 
     def get_volumes_data(self):
         # Gather our volumes
@@ -83,11 +85,12 @@ class IndexView(tables.MultiTableView):
                     volume.display_description = truncated_string + u'...'
 
                 for att in volume.attachments:
-                    att['instance'] = instances[att['server_id']]
+                    server_id = att.get('server_id', None)
-        except novaclient_exceptions.ClientException, e:
+                    att['instance'] = instances.get(server_id, None)
+        except:
             volumes = []
-            LOG.exception("ClientException in volume index")
+            exceptions.handle(self.request,
-            messages.error(self.request, _('Unable to fetch volumes: %s') % e)
+                              _('Unable to retrieve volume list.'))
         return volumes
 
     def _get_instances(self):

horizon/dashboards/nova/instances_and_volumes/volumes/forms.py

@@ -7,8 +7,6 @@
 Views for managing Nova volumes.
 """
 
-import logging
-
 from django import shortcuts
 from django.contrib import messages
 from django.utils.translation import ugettext_lazy as _
@@ -16,12 +14,9 @@ from django.utils.translation import ugettext_lazy as _
 from horizon import api
 from horizon import forms
 from horizon import exceptions
-from novaclient import exceptions as novaclient_exceptions
 
 from ..instances.tables import ACTIVE_STATES
 
-LOG = logging.getLogger(__name__)
-
 
 class CreateForm(forms.SelfHandlingForm):
     name = forms.CharField(max_length="255", label="Volume Name")
@@ -34,12 +29,10 @@ class CreateForm(forms.SelfHandlingForm):
             api.volume_create(request, data['size'], data['name'],
                               data['description'])
             message = 'Creating volume "%s"' % data['name']
-            LOG.info(message)
             messages.info(request, message)
-        except novaclient_exceptions.ClientException, e:
+        except:
-            LOG.exception("ClientException in CreateVolume")
+            exceptions.handle(request,
-            messages.error(request,
+                              _("Unable to create volume."))
-                           _('Error Creating Volume: %s') % e.message)
         return shortcuts.redirect("horizon:nova:instances_and_volumes:index")
 
 
@@ -76,6 +69,12 @@ class AttachForm(forms.SelfHandlingForm):
         self.fields['instance'].choices = instances
 
     def handle(self, request, data):
+        instance_choices = dict(self.fields['instance'].choices)
+        instance_name = instance_choices.get(data['instance'],
+                                             _("Unknown instance (None)"))
+        # The name of the instance in the choices list has the ID appended to
+        # it, so let's slice that off...
+        instance_name = instance_name.rsplit(" (")[0]
         try:
             api.volume_attach(request,
                               data['volume_id'],
@@ -83,16 +82,14 @@ class AttachForm(forms.SelfHandlingForm):
                               data['device'])
             vol_name = api.volume_get(request, data['volume_id']).display_name
 
-            message = (_('Attaching volume %(vol)s to instance '
+            message = _('Attaching volume %(vol)s to instance '
-                         '%(inst)s at %(dev)s') %
+                         '%(inst)s on %(dev)s.') % {"vol": vol_name,
-                            {"vol": vol_name, "inst": data['instance'],
+                                                    "inst": instance_name,
-                            "dev": data['device']})
+                                                    "dev": data['device']}
-            LOG.info(message)
             messages.info(request, message)
-        except novaclient_exceptions.ClientException, e:
+        except:
-            LOG.exception("ClientException in AttachVolume")
+            exceptions.handle(request,
-            messages.error(request,
+                              _('Unable to attach volume.'))
-                           _('Error attaching volume: %s') % e.message)
         return shortcuts.redirect(
                             "horizon:nova:instances_and_volumes:index")
 
@@ -118,10 +115,9 @@ class CreateSnapshotForm(forms.SelfHandlingForm):
                                        data['description'])
 
             message = _('Creating volume snapshot "%s"') % data['name']
-            LOG.info(message)
             messages.info(request, message)
         except:
             exceptions.handle(request,
-                              _('Error Creating Volume Snapshot: %(exc)s'))
+                              _('Unable to create volume snapshot.'))
 
         return shortcuts.redirect("horizon:nova:images_and_snapshots:index")

horizon/dashboards/nova/instances_and_volumes/volumes/tables.py

@@ -16,12 +16,14 @@
 
 import logging
 
-from django.core.urlresolvers import reverse
+from django.core.urlresolvers import reverse, NoReverseMatch
 from django.template.defaultfilters import title
 from django.utils import safestring
+from django.utils.html import strip_tags
 from django.utils.translation import ugettext_lazy as _
 
 from horizon import api
+from horizon import exceptions
 from horizon import tables
 
 
@@ -83,21 +85,44 @@ def get_size(volume):
     return _("%sGB") % volume.size
 
 
-def get_attachment(volume):
+def get_attachment_name(request, attachment):
-    attachments = []
+    server_id = attachment.get("server_id", None)
-    link = '<a href="%(url)s">%(name)s</a>&nbsp; (%(dev)s)'
+    if "instance" in attachment:
-    # Filter out "empty" attachments which the client returns...
+        name = attachment["instance"].name
-    for attachment in [att for att in volume.attachments if att]:
+    else:
-        url = reverse("%s:instances:detail" % URL_PREFIX,
+        try:
-                      args=(attachment["server_id"],))
+            server = api.nova.server_get(request, server_id)
-        # TODO(jake): Make "instance" the instance name
+            name = server.name
-        vals = {"url": url,
+        except:
-                "name": attachment["instance"].name
+            name = None
-                        if "instance" in attachment else None,
+            exceptions.handle(request, _("Unable to retrieve "
-                "instance": attachment["server_id"],
+                                         "attachment information."))
-                "dev": attachment["device"]}
+    try:
-        attachments.append(link % vals)
+        url = reverse("%s:instances:detail" % URL_PREFIX, args=(server_id,))
-    return safestring.mark_safe(", ".join(attachments))
+        instance = '<a href="%s">%s</a>' % (url, name)
+    except NoReverseMatch:
+        instance = name
+    return instance
+
+
+class AttachmentColumn(tables.Column):
+    """
+    Customized column class that does complex processing on the attachments
+    for a volume instance.
+    """
+    def get_raw_data(self, volume):
+        request = self.table._meta.request
+        link = _('Attached to %(instance)s on %(dev)s')
+        attachments = []
+        # Filter out "empty" attachments which the client returns...
+        for attachment in [att for att in volume.attachments if att]:
+            # When a volume is first attached it may return the server_id
+            # without the server name...
+            instance = get_attachment_name(request, attachment)
+            vals = {"instance": instance,
+                    "dev": attachment["device"]}
+            attachments.append(link % vals)
+        return safestring.mark_safe(", ".join(attachments))
 
 
 class VolumesTableBase(tables.DataTable):
@@ -126,7 +151,7 @@ class VolumesTable(VolumesTableBase):
     name = tables.Column("display_name",
                          verbose_name=_("Name"),
                          link="%s:volumes:detail" % URL_PREFIX)
-    attachments = tables.Column(get_attachment,
+    attachments = AttachmentColumn("attachments",
                                 verbose_name=_("Attached To"))
 
     class Meta:
@@ -141,30 +166,42 @@ class VolumesTable(VolumesTableBase):
 class DetachVolume(tables.BatchAction):
     name = "detach"
     action_present = _("Detach")
-    action_past = _("Detached")
+    action_past = _("Detaching")  # This action is asynchronous.
     data_type_singular = _("Volume")
     data_type_plural = _("Volumes")
     classes = ('btn-danger', 'btn-detach')
 
     def action(self, request, obj_id):
-        instance_id = self.table.get_object_by_id(obj_id)['server_id']
+        attachment = self.table.get_object_by_id(obj_id)
-        api.volume_detach(request, instance_id, obj_id)
+        api.volume_detach(request, attachment.get('server_id', None), obj_id)
 
     def get_success_url(self, request):
         return reverse('%s:index' % URL_PREFIX)
 
 
+class AttachedInstanceColumn(tables.Column):
+    """
+    Customized column class that does complex processing on the attachments
+    for a volume instance.
+    """
+    def get_raw_data(self, attachment):
+        request = self.table._meta.request
+        return safestring.mark_safe(get_attachment_name(request, attachment))
+
+
 class AttachmentsTable(tables.DataTable):
-    instance = tables.Column("instance_name", verbose_name=_("Instance Name"))
+    instance = AttachedInstanceColumn(get_attachment_name,
+                                      verbose_name=_("Instance"))
     device = tables.Column("device")
 
     def get_object_id(self, obj):
         return obj['id']
 
-    def get_object_display(self, obj):
+    def get_object_display(self, attachment):
-        vals = {"dev": obj['device'],
+        instance_name = get_attachment_name(self._meta.request, attachment)
-                "instance": obj['server_id']}
+        vals = {"dev": attachment['device'],
-        return "Attachment %(dev)s on %(instance)s" % vals
+                "instance_name": strip_tags(instance_name)}
+        return _("%(dev)s on instance %(instance_name)s") % vals
 
     def get_object_by_id(self, obj_id):
         for obj in self.data:

horizon/dashboards/nova/instances_and_volumes/volumes/tests.py

@@ -48,6 +48,7 @@ class VolumeViewTests(test.TestCase):
         self.assertEqual(res.status_code, 200)
 
     def test_edit_attachments_attached_volume(self):
+        server = self.servers.first()
         servers = deepcopy(self.servers)
         active_server = deepcopy(self.servers.first())
         active_server.status = 'ACTIVE'
@@ -57,12 +58,14 @@ class VolumeViewTests(test.TestCase):
         volume = deepcopy(self.volumes.first())
         volume.id = "2"
         volume.status = "in-use"
-        volume.attachments = [{"id": "1", "server_id": "3",
+        volume.attachments = [{"id": "1", "server_id": server.id,
                                "device": "/dev/hdn"}]
         volumes.add(volume)
 
         self.mox.StubOutWithMock(api, 'volume_get')
         self.mox.StubOutWithMock(api.nova, 'server_list')
+        self.mox.StubOutWithMock(api.nova, 'server_get')
+        api.nova.server_get(IsA(http.HttpRequest), server.id).AndReturn(server)
         api.volume_get(IsA(http.HttpRequest), volume.id) \
                        .AndReturn(volume)
         api.nova.server_list(IsA(http.HttpRequest)).AndReturn(servers.list())
@@ -104,7 +107,6 @@ class VolumeViewTests(test.TestCase):
         self.assertContains(res, "<dd>40 GB</dd>", 1, 200)
         self.assertContains(res, "<dd>04/01/12 at 10:30:00</dd>", 1, 200)
         self.assertContains(res, "<a href=\"/nova/instances_and_volumes/"
-                            "instances/1/detail\"><strong>server_1</strong> "
+                                 "instances/1/detail\">server_1</a>", 1, 200)
-                            "(1)</a>", 1, 200)
 
         self.assertNoMessages()

horizon/dashboards/nova/templates/nova/instances_and_volumes/instances/_detail_overview.html

@@ -83,11 +83,9 @@
   <hr class="header_rule">
     <dl>
       {% for volume in instance.volumes %}
-        <dt>{% trans "Attached On" %} {{ volume.device }}</dt>
+        <dt>{% trans "Attached To" %}</dt>
         <dd>
-          <a href="{% url horizon:nova:instances_and_volumes:volumes:detail volume.volumeId %}">
+          <a href="{% url horizon:nova:instances_and_volumes:volumes:detail volume.volumeId %}">{{ volume.name }}</a><span> {% trans "on" %} {{ volume.device }}</span>
-             <strong>{{ volume.name }}</strong> ({{ volume.id }})
-          </a>
         </dd>
         {% empty %}
           <dt>{% trans "Volume" %}</dt>

horizon/dashboards/nova/templates/nova/instances_and_volumes/volumes/_detail_overview.html

@@ -38,7 +38,7 @@
       <dt>{% trans "Attached To" %}</dt>
       <dd>
         {% url horizon:nova:instances_and_volumes:instances:detail attachment.server_id as instance_url%}
-        <a href="{{ instance_url }}"><strong>{{ attachment.instance.name }}</strong> ({{ attachment.instance.id }})</a>
+        <a href="{{ instance_url }}">{{ attachment.instance.name }}</a>
         <span> {% trans "on" %} {{ attachment.device }}</span>
       </dd>
   {% empty %}