Merge lp:~jk0/nova/backup_with_rotation into lp:~hudson-openstack/nova/trunk

[Josh Kearney]

Implement backup with rotation and expose this functionality in the OS API.

[Vish Ishaya (vishvananda)]

All this looks good. Just unsure if this is in the api spec or should be an extension?

[Brian Lamar (blamar)]

Vish/Josh,

I feel like I say this a lot, but this logic doesn't belong in nova/compute/*. If we want the OpenStack API to support backing up images with sweet rotation logic, I would argue it should be implemented as a completely different service.

OpenStack API provides the ability to:
-Backup images
-Query images by creation time/modification time
-Delete a single image or multiple images

Other Service uses these API calls to implement:
-Scheduled deletion of images
-Migration of old images to tape-backup/long term storage/order them on CD-ROMs/whatever

However!

I could be convinced that this logic should go in to glance...because at the very least this isn't a compute task, but an image-store task.

review: Needs Fixing

[Brian Waldon (bcwaldon)]

I think this is a great idea, and a feature that we could really benefit from. However, I'm hesitant to say this belongs in Nova. This feels like a feature in a future "scheduling service" that can execute things at certain times.

If the decision is made to keep this in Nova, I would like to see the OS API functionality implemented as an extension. This isn't part of the v1.1 API specification.

review: Needs Information

[Brian Waldon (bcwaldon)]

After giving it some thought, I'm much more okay with this implementation as a temporary solution. I also don't have a problem with the API change if it is considered part of the admin api. However, being part of the admin api, does that mean that a deployer will have to set up each api node with the admin api on?

Fix up these few things, and it's good to go:

20-25: Can you find a more common place to put this? I feel like we could use this logic in a lot of places.

186, 193: instantiate these classes

214: you might want to specify a limit kwarg here. It defaults to 25

220: There is a branch in MP right now in glance that allows db-level sorting: lp:~rackspace-titan/glance/api-results-ordering

243-245: I don't think this change is necessary

review: Needs Fixing

[Josh Kearney (jk0)]

Thanks for the feedback (changes coming shortly).

> 243-245: I don't think this change is necessary

The pep8 version in the current pip-requires picked this up.

[lp:~jk0/nova/backup_with_rotation]

1220. By Josh Kearney on 2011-06-27

Review feedback.

1221. By Josh Kearney on 2011-06-28

Review feedback.

1222. By Josh Kearney on 2011-06-28

Whoops.

1223. By Josh Kearney on 2011-06-28

OOPS

[Brian Lamar (blamar)]

Lines 24, 53, 63: Can you add a explanation=<message> keyword argument to the bad request? It'll help know what exactly went wrong for the user.

review: Needs Fixing

[lp:~jk0/nova/backup_with_rotation]

1224. By Josh Kearney on 2011-06-28

Review feedback.

[Brian Waldon (bcwaldon)]

Looks like these couple of things still need to be addressed:

20-25: Can you find a more common place to put this? I feel like we could use this logic in a lot of places.

236: The sorting functionality went in today. You can add sort_key and sort_dir keyword params to handle your sorting

[Rick Harris (rconradharris)]

> 20-25: Can you find a more common place to put this? I feel like we could use this logic in a lot of places.

As it's written, this was custom-tailored for this method. We /could/ re-work it to be more generic and use it in other places; the drawback there is that it ends up somewhat defeating the original purpose, DRYing up the code we're modifying.

If we do want to put a version of this code in a separate place, I'd vote that we do that as we're refactoring some of the code that would use it.

[Brian Waldon (bcwaldon)]

> > 20-25: Can you find a more common place to put this? I feel like we could
> use this logic in a lot of places.
>
> As it's written, this was custom-tailored for this method. We /could/ re-work
> it to be more generic and use it in other places; the drawback there is that
> it ends up somewhat defeating the original purpose, DRYing up the code we're
> modifying.
>
> If we do want to put a version of this code in a separate place, I'd vote that
> we do that as we're refactoring some of the code that would use it.

Sounds reasonable.

[lp:~jk0/nova/backup_with_rotation]

1225. By Josh Kearney on 2011-06-28

Let glance handle sorting.

1226. By Josh Kearney on 2011-06-28

Merged trunk

1227. By Josh Kearney on 2011-06-28

Merged trunk

[Brian Waldon (bcwaldon)]

Josh, I think it might be a good idea to explicitly add sort_key/sort_dir params to protect from future changes. I would like the peace of mind to know we won't get screwed here if the default sort key/direction change. Is that reasonable?

[lp:~jk0/nova/backup_with_rotation]

1228. By Josh Kearney on 2011-06-28

Review feedback

[Josh Kearney (jk0)]

> Josh, I think it might be a good idea to explicitly add sort_key/sort_dir
> params to protect from future changes. I would like the peace of mind to know
> we won't get screwed here if the default sort key/direction change. Is that
> reasonable?

Yep. All set.

[Brian Waldon (bcwaldon)]

> > Josh, I think it might be a good idea to explicitly add sort_key/sort_dir
> > params to protect from future changes. I would like the peace of mind to
> know
> > we won't get screwed here if the default sort key/direction change. Is that
> > reasonable?
>
> Yep. All set.

Awesome. Great work, guys.

review: Approve

[Brian Lamar (blamar)]

I really want to help you guys get this in, but what are your thoughts on making this inline with the other admin API functions? Currently nova/api/openstack/__init__.py loads routes if FLAGS.allow_admin_api is set, could we maybe add:

server_members['backup'] = 'POST'

and then have the backup logic go in the servers controller? My main point here is that we're backing up a server...not an image right? It seems odd to say "backup this image" because an image is something you start a server from. Also, it seems more 'secure' to not even load the routes when allow_admin_api flag isn't set, like the existing admin API methods.

[Josh Kearney (jk0)]

> I really want to help you guys get this in, but what are your thoughts on
> making this inline with the other admin API functions? Currently
> nova/api/openstack/__init__.py loads routes if FLAGS.allow_admin_api is set,
> could we maybe add:
>
> server_members['backup'] = 'POST'
>
> and then have the backup logic go in the servers controller? My main point
> here is that we're backing up a server...not an image right? It seems odd to
> say "backup this image" because an image is something you start a server from.
> Also, it seems more 'secure' to not even load the routes when allow_admin_api
> flag isn't set, like the existing admin API methods.

From a Glance perspective everything is considered an image (base images, snapshots, backups, etc), so we felt most comfortable including this portion of the code in images. Same idea in python-novaclient. For example:

Snapshot: nova image-create <instance ID> <snapshot name>

Backup: nova image-create <instance ID> <backup name> --image-type backup --backup-type <daily|weeky> --rotation <arbitrary number>

So for the most part, the only differences between these two are the image_type, backup_type and rotation properties in glance.

I do like what you're suggesting, but would this refactor be better suited in its own branch / BP?

[Brian Lamar (blamar)]

Yeah, can you maybe make a blueprint to look at this as a whole in Nova? Nova should be instance-focused and Glance should be image-focused. It's been stated multiple times that /images in Nova is going away so we need to have a blueprint to show us the way.

This looks good and passes all of the normal test suites.

review: Approve

=== modified file 'nova/api/openstack/images.py'
--- nova/api/openstack/images.py	2011-06-24 12:01:51 +0000
+++ nova/api/openstack/images.py	2011-06-28 23:51:36 +0000
@@ -90,31 +90,67 @@
         return webob.exc.HTTPNoContent()
 
     def create(self, req, body):
-        """Snapshot a server instance and save the image.
+        """Snapshot or backup a server instance and save the image.
+
+        Images now have an `image_type` associated with them, which can be
+        'snapshot' or the backup type, like 'daily' or 'weekly'.
+
+        If the image_type is backup-like, then the rotation factor can be
+        included and that will cause the oldest backups that exceed the
+        rotation factor to be deleted.
 
         :param req: `wsgi.Request` object
         """
+        def get_param(param):
+            try:
+                return body["image"][param]
+            except KeyError:
+                raise webob.exc.HTTPBadRequest(explanation="Missing required "
+                        "param: %s" % param)
+
         context = req.environ['nova.context']
         content_type = req.get_content_type()
 
         if not body:
             raise webob.exc.HTTPBadRequest()
 
+        image_type = body["image"].get("image_type", "snapshot")
+
         try:
             server_id = self._server_id_from_req(req, body)
-            image_name = body["image"]["name"]
         except KeyError:
             raise webob.exc.HTTPBadRequest()
 
+        image_name = get_param("name")
         props = self._get_extra_properties(req, body)
 
-        image = self._compute_service.snapshot(context, server_id,
-                                               image_name, props)
+        if image_type == "snapshot":
+            image = self._compute_service.snapshot(
+                        context, server_id, image_name,
+                        extra_properties=props)
+        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(
+                        explanation="Admin API Required")
+
+            backup_type = get_param("backup_type")
+            rotation = int(get_param("rotation"))
+
+            image = self._compute_service.backup(
+                        context, server_id, image_name,
+                        backup_type, rotation, extra_properties=props)
+        else:
+            LOG.error(_("Invalid image_type '%s' passed") % image_type)
+            raise webob.exc.HTTPBadRequest(explanation="Invalue image_type: "
+                   "%s" % image_type)
+
         return dict(image=self.get_builder(req).build(image, detail=True))
 
     def get_builder(self, request):
         """Indicates that you must use a Controller subclass."""
-        raise NotImplementedError
+        raise NotImplementedError()
 
     def _server_id_from_req(self, req, data):
         raise NotImplementedError()

=== modified file 'nova/compute/api.py'
--- nova/compute/api.py	2011-06-24 12:07:33 +0000
+++ nova/compute/api.py	2011-06-28 23:51:36 +0000
@@ -711,19 +711,60 @@
         raise exception.Error(_("Unable to find host for Instance %s")
                                 % instance_id)
 
+    def backup(self, context, instance_id, name, backup_type, rotation,
+               extra_properties=None):
+        """Backup the given instance
+
+        :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'
+        :param rotation: int representing how many backups to keep around;
+            None if rotation shouldn't be used (as in the case of snapshots)
+        :param extra_properties: dict of extra image properties to include
+        """
+        recv_meta = self._create_image(context, instance_id, name, 'backup',
+                            backup_type=backup_type, rotation=rotation,
+                            extra_properties=extra_properties)
+        return recv_meta
+
     def snapshot(self, context, instance_id, name, extra_properties=None):
         """Snapshot the given instance.
 
+        :param instance_id: nova.db.sqlalchemy.models.Instance.Id
+        :param name: name of the backup or snapshot
+        :param extra_properties: dict of extra image properties to include
+
         :returns: A dict containing image metadata
         """
-        properties = {'instance_id': str(instance_id),
+        return self._create_image(context, instance_id, name, 'snapshot',
+                                  extra_properties=extra_properties)
+
+    def _create_image(self, context, instance_id, name, image_type,
+                      backup_type=None, rotation=None, extra_properties=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 | 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)
+        :param extra_properties: dict of extra image properties to include
+
+        """
+        instance = db.api.instance_get(context, instance_id)
+        properties = {'instance_uuid': instance['uuid'],
                       'user_id': str(context.user_id),
-                      'image_state': 'creating'}
+                      'image_state': 'creating',
+                      'image_type': image_type,
+                      'backup_type': backup_type}
         properties.update(extra_properties or {})
         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']}
+        params = {'image_id': recv_meta['id'], 'image_type': image_type,
+                  'backup_type': backup_type, 'rotation': rotation}
         self._cast_compute_message('snapshot_instance', context, instance_id,
                                    params=params)
         return recv_meta

=== modified file 'nova/compute/manager.py'
--- nova/compute/manager.py	2011-06-28 20:37:05 +0000
+++ nova/compute/manager.py	2011-06-28 23:51:36 +0000
@@ -46,6 +46,7 @@
 
 from nova import exception
 from nova import flags
+import nova.image
 from nova import log as logging
 from nova import manager
 from nova import network
@@ -506,8 +507,19 @@
         self._update_state(context, instance_id)
 
     @exception.wrap_exception
-    def snapshot_instance(self, context, instance_id, image_id):
-        """Snapshot an instance on this host."""
+    def snapshot_instance(self, context, instance_id, image_id,
+                          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 | 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)
+        """
         context = context.elevated()
         instance_ref = self.db.instance_get(context, instance_id)
 
@@ -527,6 +539,65 @@
 
         self.driver.snapshot(instance_ref, image_id)
 
+        if image_type == 'snapshot':
+            if rotation:
+                raise exception.ImageRotationNotAllowed()
+        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, backup_type, rotation):
+        """Delete excess backups associated to an instance.
+
+        Instances are allowed a fixed number of backups (the rotation number);
+        this method deletes the oldest backups that exceed the rotation
+        threshold.
+
+        :param context: security context
+        :param instance_uuid: string representing uuid of instance
+        :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)
+        """
+        # NOTE(jk0): Eventually extract this out to the ImageService?
+        def fetch_images():
+            images = []
+            marker = None
+            while True:
+                batch = image_service.detail(context, filters=filters,
+                        marker=marker, sort_key='created_at', sort_dir='desc')
+                if not batch:
+                    break
+                images += batch
+                marker = batch[-1]['id']
+            return images
+
+        image_service = nova.image.get_default_image_service()
+        filters = {'property-image_type': 'backup',
+                   'property-backup_type': backup_type,
+                   'property-instance_uuid': instance_uuid}
+
+        images = fetch_images()
+        num_images = len(images)
+        LOG.debug(_("Found %(num_images)d images (rotation: %(rotation)d)"
+                    % locals()))
+        if num_images > rotation:
+            # NOTE(sirp): this deletes all backups that exceed the rotation
+            # limit
+            excess = len(images) - rotation
+            LOG.debug(_("Rotating out %d backups" % excess))
+            for i in xrange(excess):
+                image = images.pop()
+                image_id = image['id']
+                LOG.debug(_("Deleting image %d" % image_id))
+                image_service.delete(context, image_id)
+
     @exception.wrap_exception
     @checks_instance_lock
     def set_admin_password(self, context, instance_id, new_pass=None):

=== modified file 'nova/db/sqlalchemy/migrate_repo/versions/027_add_provider_firewall_rules.py'
--- nova/db/sqlalchemy/migrate_repo/versions/027_add_provider_firewall_rules.py	2011-06-28 22:26:08 +0000
+++ nova/db/sqlalchemy/migrate_repo/versions/027_add_provider_firewall_rules.py	2011-06-28 23:51:36 +0000
@@ -58,7 +58,7 @@
         Column('to_port', Integer()),
         Column('cidr',
                String(length=255, convert_unicode=False, assert_unicode=None,
-                      unicode_error=None, _warn_on_bytestring=False)))
+                    unicode_error=None, _warn_on_bytestring=False)))
 
 
 def upgrade(migrate_engine):

=== modified file 'nova/exception.py'
--- nova/exception.py	2011-06-28 15:13:52 +0000
+++ nova/exception.py	2011-06-28 23:51:36 +0000
@@ -558,6 +558,14 @@
     message = _("Unable to use global role %(role_id)s")
 
 
+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

=== modified file 'nova/tests/api/openstack/fakes.py'
--- nova/tests/api/openstack/fakes.py	2011-06-28 00:23:19 +0000
+++ nova/tests/api/openstack/fakes.py	2011-06-28 23:51:36 +0000
@@ -147,6 +147,16 @@
     stubs.Set(nova.compute.API, 'snapshot', snapshot)
 
 
+def stub_out_compute_api_backup(stubs):
+    def backup(self, context, instance_id, name, backup_type, rotation,
+               extra_properties=None):
+        props = dict(instance_id=instance_id, instance_ref=instance_id,
+                     backup_type=backup_type, rotation=rotation)
+        props.update(extra_properties or {})
+        return dict(id='123', status='ACTIVE', name=name, properties=props)
+    stubs.Set(nova.compute.API, 'backup', backup)
+
+
 def stub_out_glance_add_image(stubs, sent_to_glance):
     """
     We return the metadata sent to glance by modifying the sent_to_glance dict

=== modified file 'nova/tests/api/openstack/test_images.py'
--- nova/tests/api/openstack/test_images.py	2011-06-24 12:01:51 +0000
+++ nova/tests/api/openstack/test_images.py	2011-06-28 23:51:36 +0000
@@ -340,6 +340,7 @@
         self.fixtures = self._make_image_fixtures()
         fakes.stub_out_glance(self.stubs, initial_fixtures=self.fixtures)
         fakes.stub_out_compute_api_snapshot(self.stubs)
+        fakes.stub_out_compute_api_backup(self.stubs)
 
     def tearDown(self):
         """Run after each test."""
@@ -364,10 +365,10 @@
         response_list = response_dict["images"]
 
         expected = [{'id': 123, 'name': 'public image'},
-                    {'id': 124, 'name': 'queued backup'},
-                    {'id': 125, 'name': 'saving backup'},
-                    {'id': 126, 'name': 'active backup'},
-                    {'id': 127, 'name': 'killed backup'},
+                    {'id': 124, 'name': 'queued snapshot'},
+                    {'id': 125, 'name': 'saving snapshot'},
+                    {'id': 126, 'name': 'active snapshot'},
+                    {'id': 127, 'name': 'killed snapshot'},
                     {'id': 129, 'name': None}]
 
         self.assertDictListMatch(response_list, expected)
@@ -617,14 +618,14 @@
         },
         {
             'id': 124,
-            'name': 'queued backup',
+            'name': 'queued snapshot',
             'updated': self.NOW_API_FORMAT,
             'created': self.NOW_API_FORMAT,
             'status': 'QUEUED',
         },
         {
             'id': 125,
-            'name': 'saving backup',
+            'name': 'saving snapshot',
             'updated': self.NOW_API_FORMAT,
             'created': self.NOW_API_FORMAT,
             'status': 'SAVING',
@@ -632,14 +633,14 @@
         },
         {
             'id': 126,
-            'name': 'active backup',
+            'name': 'active snapshot',
             'updated': self.NOW_API_FORMAT,
             'created': self.NOW_API_FORMAT,
             'status': 'ACTIVE'
         },
         {
             'id': 127,
-            'name': 'killed backup',
+            'name': 'killed snapshot',
             'updated': self.NOW_API_FORMAT,
             'created': self.NOW_API_FORMAT,
             'status': 'FAILED',
@@ -684,7 +685,7 @@
         },
         {
             'id': 124,
-            'name': 'queued backup',
+            'name': 'queued snapshot',
             'serverRef': "http://localhost:8774/v1.1/servers/42",
             'updated': self.NOW_API_FORMAT,
             'created': self.NOW_API_FORMAT,
@@ -706,7 +707,7 @@
         },
         {
             'id': 125,
-            'name': 'saving backup',
+            'name': 'saving snapshot',
             'serverRef': "http://localhost:8774/v1.1/servers/42",
             'updated': self.NOW_API_FORMAT,
             'created': self.NOW_API_FORMAT,
@@ -729,7 +730,7 @@
         },
         {
             'id': 126,
-            'name': 'active backup',
+            'name': 'active snapshot',
             'serverRef': "http://localhost:8774/v1.1/servers/42",
             'updated': self.NOW_API_FORMAT,
             'created': self.NOW_API_FORMAT,
@@ -751,7 +752,7 @@
         },
         {
             'id': 127,
-            'name': 'killed backup',
+            'name': 'killed snapshot',
             'serverRef': "http://localhost:8774/v1.1/servers/42",
             'updated': self.NOW_API_FORMAT,
             'created': self.NOW_API_FORMAT,
@@ -969,18 +970,103 @@
         self.assertEqual(res.status_int, 404)
 
     def test_create_image(self):
-
-        body = dict(image=dict(serverId='123', name='Backup 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(200, response.status_int)
+        body = dict(image=dict(serverId='123', name='Snapshot 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(200, response.status_int)
+
+    def test_create_snapshot_no_name(self):
+        """Name is required for snapshots"""
+        body = dict(image=dict(serverId='123'))
+        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_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)
+
+    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.
+        """
+        # 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', 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)
+        req.headers["content-type"] = "application/json"
+        response = req.get_response(fakes.wsgi_app())
+        self.assertEqual(200, response.status_int)
+
+    def test_create_backup_no_rotation(self):
+        """Rotation 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', 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)
+        req.headers["content-type"] = "application/json"
+        response = req.get_response(fakes.wsgi_app())
+        self.assertEqual(400, response.status_int)
+
+    def test_create_image_with_invalid_image_type(self):
+        """Valid image_types are snapshot | daily | weekly"""
+        # 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', image_type='monthly',
+                               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)
 
     def test_create_image_no_server_id(self):
 
-        body = dict(image=dict(name='Backup 1'))
+        body = dict(image=dict(name='Snapshot 1'))
         req = webob.Request.blank('/v1.0/images')
         req.method = 'POST'
         req.body = json.dumps(body)
@@ -990,7 +1076,7 @@
 
     def test_create_image_v1_1(self):
 
-        body = dict(image=dict(serverRef='123', name='Backup 1'))
+        body = dict(image=dict(serverRef='123', name='Snapshot 1'))
         req = webob.Request.blank('/v1.1/images')
         req.method = 'POST'
         req.body = json.dumps(body)
@@ -1024,7 +1110,7 @@
 
     def test_create_image_v1_1_xml_serialization(self):
 
-        body = dict(image=dict(serverRef='123', name='Backup 1'))
+        body = dict(image=dict(serverRef='123', name='Snapshot 1'))
         req = webob.Request.blank('/v1.1/images')
         req.method = 'POST'
         req.body = json.dumps(body)
@@ -1038,7 +1124,7 @@
             <image
                    created="None"
                    id="123"
-                   name="Backup 1"
+                   name="Snapshot 1"
                    serverRef="http://localhost/v1.1/servers/123"
                    status="ACTIVE"
                    updated="None"
@@ -1057,7 +1143,7 @@
 
     def test_create_image_v1_1_no_server_ref(self):
 
-        body = dict(image=dict(name='Backup 1'))
+        body = dict(image=dict(name='Snapshot 1'))
         req = webob.Request.blank('/v1.1/images')
         req.method = 'POST'
         req.body = json.dumps(body)
@@ -1084,19 +1170,21 @@
                     status='active', properties={})
         image_id += 1
 
-        # Backup for User 1
+        # Snapshot for User 1
         server_ref = 'http://localhost:8774/v1.1/servers/42'
-        backup_properties = {'instance_ref': server_ref, 'user_id': '1'}
+        snapshot_properties = {'instance_ref': server_ref, 'user_id': '1'}
         for status in ('queued', 'saving', 'active', 'killed'):
-            add_fixture(id=image_id, name='%s backup' % status,
+            add_fixture(id=image_id, name='%s snapshot' % status,
                         is_public=False, status=status,
-                        properties=backup_properties)
+                        properties=snapshot_properties)
             image_id += 1
 
-        # Backup for User 2
-        other_backup_properties = {'instance_id': '43', 'user_id': '2'}
-        add_fixture(id=image_id, name='someone elses backup', is_public=False,
-                    status='active', properties=other_backup_properties)
+        # Snapshot for User 2
+        other_snapshot_properties = {'instance_id': '43', 'user_id': '2'}
+        add_fixture(id=image_id, name='someone elses snapshot',
+                    is_public=False, status='active',
+                    properties=other_snapshot_properties)
+
         image_id += 1
 
         # Image without a name