Refactored backup rotate.

6 files changed, 93 insertions, 43 deletions

diff --git a/nova/api/openstack/images.py b/nova/api/openstack/images.py
index 5f88ede96..c535e4e26 100644
--- a/nova/api/openstack/images.py
+++ b/nova/api/openstack/images.py
@@ -118,19 +118,25 @@ class Controller(object):
         except KeyError:
             raise webob.exc.HTTPBadRequest()
 
+        image_name = get_param("name")
+
         if image_type == "snapshot":
-            image_name = get_param("name")
-            image = self._compute_service.snapshot(context, server_id,
-                    image_name)
-        elif image_type in ("daily", "weekly"):
+            image = self._compute_service.snapshot(
+                        context, server_id, image_name)
+        elif image_type == "backup":
+            # NOTE(sirp): Unlike snapshot, backup is not a customer facing
+            # API call; rather, it's used by the internal backup scheduler
             if not FLAGS.allow_admin_api:
                 raise webob.exc.HTTPBadRequest()
 
+            backup_type = get_param("backup_type")
             rotation = int(get_param("rotation"))
-            image = self._compute_service.backup(context, server_id,
-                    image_type, rotation)
+
+            image = self._compute_service.backup(
+                        context, server_id, image_name,
+                        backup_type, rotation)
         else:
-            LOG.error(_("Invalid image_type '%s' passed" % image_type))
+            LOG.error(_("Invalid image_type '%s' passed") % image_type)
             raise webob.exc.HTTPBadRequest()
 
         return dict(image=self.get_builder(req).build(image, detail=True))
diff --git a/nova/compute/api.py b/nova/compute/api.py
index c0cb2e18a..9c6f0ef9d 100644
--- a/nova/compute/api.py
+++ b/nova/compute/api.py
@@ -701,32 +701,38 @@ class API(base.Base):
         raise exception.Error(_("Unable to find host for Instance %s")
                                 % instance_id)
 
-    def backup(self, context, instance_id, backup_type, rotation):
+    def backup(self, context, instance_id, name, backup_type, rotation):
         """Backup the given instance
 
-            instance_id - int - id representing the instance
-            backup_type - str - whether it's 'daily' or 'weekly'
-            rotation - int - number of backups to keep around
-        """
+        :param instance_id: nova.db.sqlalchemy.models.Instance.Id
+        :param name: name of the backup or snapshot
         name = backup_type  # daily backups are called 'daily'
-        recv_meta = self._snapshot(context, instance_id, name, backup_type,
-                                   rotation=rotation)
+        :param rotation: int representing how many backups to keep around;
+            None if rotation shouldn't be used (as in the case of snapshots)
+        """
+        recv_meta = self._create_image(context, instance_id, name, 'backup',
+                            backup_type=backup_type, rotation=rotation)
         return recv_meta
 
     def snapshot(self, context, instance_id, name):
         """Snapshot the given instance.
 
+        :param instance_id: nova.db.sqlalchemy.models.Instance.Id
+        :param name: name of the backup or snapshot
+        
         :returns: A dict containing image metadata
         """
-        return self._snapshot(context, instance_id, name, 'snapshot')
+        return self._create_image(context, instance_id, name, 'snapshot')
 
-    def _snapshot(self, context, instance_id, name, image_type, rotation=None):
-        """Snapshot an instance on this host.
+    def _create_image(self, context, instance_id, name, image_type,
+                      backup_type=None, rotation=None):
+        """Create snapshot or backup for an instance on this host.
 
         :param context: security context
         :param instance_id: nova.db.sqlalchemy.models.Instance.Id
         :param name: string for name of the snapshot
-        :param image_type: snapshot | daily | weekly
+        :param image_type: snapshot | backup
+        :param backup_type: daily | weekly
         :param rotation: int representing how many backups to keep around;
             None if rotation shouldn't be used (as in the case of snapshots)
         """
@@ -734,12 +740,13 @@ class API(base.Base):
         properties = {'instance_uuid': instance['uuid'],
                       'user_id': str(context.user_id),
                       'image_state': 'creating',
-                      'image_type': image_type}
+                      'image_type': image_type,
+                      'backup_type': backup_type}
         sent_meta = {'name': name, 'is_public': False,
                      'status': 'creating', 'properties': properties}
         recv_meta = self.image_service.create(context, sent_meta)
         params = {'image_id': recv_meta['id'], 'image_type': image_type,
-                  'rotation': rotation}
+                  'backup_type': backup_type, 'rotation': rotation}
         self._cast_compute_message('snapshot_instance', context, instance_id,
                                    params=params)
         return recv_meta
diff --git a/nova/compute/manager.py b/nova/compute/manager.py
index ca66d0387..1458ea41f 100644
--- a/nova/compute/manager.py
+++ b/nova/compute/manager.py
@@ -476,13 +476,15 @@ class ComputeManager(manager.SchedulerDependentManager):
 
     @exception.wrap_exception
     def snapshot_instance(self, context, instance_id, image_id,
-                          image_type='snapshot', rotation=None):
+                          image_type='snapshot', backup_type=None,
+                          rotation=None):
         """Snapshot an instance on this host.
 
         :param context: security context
         :param instance_id: nova.db.sqlalchemy.models.Instance.Id
         :param image_id: glance.db.sqlalchemy.models.Image.Id
-        :param image_type: snapshot | daily | weekly
+        :param image_type: snapshot | backup
+        :param backup_type: daily | weekly
         :param rotation: int representing how many backups to keep around;
             None if rotation shouldn't be used (as in the case of snapshots)
         """
@@ -504,13 +506,21 @@ class ComputeManager(manager.SchedulerDependentManager):
                        'expected: %(running)s)') % locals())
 
         self.driver.snapshot(instance_ref, image_id)
-        if rotation and image_type == 'snapshot':
+        
+        if image_type == 'snapshot' and rotation:
             raise exception.ImageRotationNotAllowed
-        elif rotation:
-            instance_uuid = instance_ref['uuid']
-            self.rotate_backups(context, instance_uuid, image_type, rotation)
+        elif image_type == 'backup':
+            if rotation:
+                instance_uuid = instance_ref['uuid']
+                self.rotate_backups(context, instance_uuid, backup_type,
+                                    rotation)
+            else:
+                raise exception.RotationRequiredForBackup
+        else:
+            raise Exception(_('Image type not recognized %s') % image_type)
+
 
-    def rotate_backups(self, context, instance_uuid, image_type, rotation):
+    def rotate_backups(self, context, instance_uuid, backup_type, rotation):
         """Delete excess backups associated to an instance.
 
         Instances are allowed a fixed number of backups (the rotation number);
@@ -519,12 +529,13 @@ class ComputeManager(manager.SchedulerDependentManager):
 
         :param context: security context
         :param instance_uuid: string representing uuid of instance
-        :param image_type: snapshot | daily | weekly
+        :param backup_type: daily | weekly
         :param rotation: int representing how many backups to keep around;
             None if rotation shouldn't be used (as in the case of snapshots)
         """
         image_service = nova.image.get_default_image_service()
-        filters = {'property-image_type': image_type,
+        filters = {'property-image_type': 'backup',
+                   'property-backup_type': backup_type,
                    'property-instance_uuid': instance_uuid}
         images = image_service.detail(context, filters=filters)
         num_images = len(images)
diff --git a/nova/exception.py b/nova/exception.py
index a548a638c..f3893d239 100644
--- a/nova/exception.py
+++ b/nova/exception.py
@@ -553,6 +553,10 @@ class ImageRotationNotAllowed(NovaException):
     message = _("Rotation is not allowed for snapshots")
 
 
+class RotationRequiredForBackup(NovaException):
+    message = _("Rotation param is required for backup image_type")
+
+
 #TODO(bcwaldon): EOL this exception!
 class Duplicate(NovaException):
     pass
diff --git a/nova/tests/api/openstack/fakes.py b/nova/tests/api/openstack/fakes.py
index 0a2584910..ad9c5067c 100644
--- a/nova/tests/api/openstack/fakes.py
+++ b/nova/tests/api/openstack/fakes.py
@@ -147,10 +147,11 @@ def stub_out_compute_api_snapshot(stubs):
 
 
 def stub_out_compute_api_backup(stubs):
-    def backup(self, context, instance_id, backup_type, rotation):
+    def backup(self, context, instance_id, name, backup_type, rotation):
         return dict(id='123', status='ACTIVE',
                     properties=dict(instance_id='123',
-                                    image_type=backup_type,
+                                    name=name,
+                                    backup_type=backup_type,
                                     rotation=rotation))
     stubs.Set(nova.compute.API, 'backup', backup)
 
diff --git a/nova/tests/api/openstack/test_images.py b/nova/tests/api/openstack/test_images.py
index 0fad044f1..8ad08080a 100644
--- a/nova/tests/api/openstack/test_images.py
+++ b/nova/tests/api/openstack/test_images.py
@@ -983,11 +983,7 @@ class ImageControllerWithGlanceServiceTest(test.TestCase):
         self.assertEqual(200, response.status_int)
 
     def test_create_snapshot_no_name(self):
-        """Name is required for snapshots
-
-        If an image_type isn't passed, we default to image_type=snapshot,
-        thus `name` is required
-        """
+        """Name is required for snapshots"""
         body = dict(image=dict(serverId='123'))
         req = webob.Request.blank('/v1.0/images')
         req.method = 'POST'
@@ -996,11 +992,19 @@ class ImageControllerWithGlanceServiceTest(test.TestCase):
         response = req.get_response(fakes.wsgi_app())
         self.assertEqual(400, response.status_int)
 
-    def test_create_backup_no_name_with_rotation(self):
-        """Name isn't required for backups, but rotation is.
+    def test_create_backup_no_name(self):
+        """Name is also required for backups"""
+        body = dict(image=dict(serverId='123', image_type='backup',
+                               backup_type='daily', rotation=1))
+        req = webob.Request.blank('/v1.0/images')
+        req.method = 'POST'
+        req.body = json.dumps(body)
+        req.headers["content-type"] = "application/json"
+        response = req.get_response(fakes.wsgi_app())
+        self.assertEqual(400, response.status_int)
 
-        The reason name isn't required is because it defaults to the
-        image_type.
+    def test_create_backup_with_rotation_and_backup_type(self):
+        """The happy path for creating backups
 
         Creating a backup is an admin-only operation, as opposed to snapshots
         which are available to anybody.
@@ -1009,8 +1013,9 @@ class ImageControllerWithGlanceServiceTest(test.TestCase):
         FLAGS.allow_admin_api = True
 
         # FIXME(sirp): should the fact that backups are admin_only be a FLAG
-        body = dict(image=dict(serverId='123', image_type='daily',
-                               rotation=1))
+        body = dict(image=dict(serverId='123', image_type='backup',
+                               name='Backup 1',
+                               backup_type='daily', rotation=1))
         req = webob.Request.blank('/v1.0/images')
         req.method = 'POST'
         req.body = json.dumps(body)
@@ -1024,7 +1029,23 @@ class ImageControllerWithGlanceServiceTest(test.TestCase):
         FLAGS.allow_admin_api = True
 
         # FIXME(sirp): should the fact that backups are admin_only be a FLAG
-        body = dict(image=dict(serverId='123', image_type='daily'))
+        body = dict(image=dict(serverId='123', name='daily',
+                               image_type='backup', backup_type='daily'))
+        req = webob.Request.blank('/v1.0/images')
+        req.method = 'POST'
+        req.body = json.dumps(body)
+        req.headers["content-type"] = "application/json"
+        response = req.get_response(fakes.wsgi_app())
+        self.assertEqual(400, response.status_int)
+
+    def test_create_backup_no_backup_type(self):
+        """Backup Type (daily or weekly) is required for backup requests"""
+        # FIXME(sirp): teardown needed?
+        FLAGS.allow_admin_api = True
+
+        # FIXME(sirp): should the fact that backups are admin_only be a FLAG
+        body = dict(image=dict(serverId='123', name='daily',
+                               image_type='backup', rotation=1))
         req = webob.Request.blank('/v1.0/images')
         req.method = 'POST'
         req.body = json.dumps(body)