openstack
/
python-glanceclient
Code Issues Proposed changes

Add support for location parameters in v2 commands 

Currently glanceclient's v2 commands don't support modification
operations on an image's location attribute - the argparse specification
for the location attribute of the image-update command causes the image
id argument to be included in list of locations and so the command
parsing fails (because it causes the image id to appear to be missing).

Furthermore even if the 'locations' argument were to be accepted by
argparse (e.g. by changing the argument specs and using --id to specify
the image id) the command would still fail because the arguments are
passed directly to the schema which expects the value of the 'locations'
argument to be a valid dictionary (there is nobody to convert the
argument string to a python dictionary that the schema expects).

This commit adds the following location related commands to
glanceclient:
    --location-add: Add a new location to the list of image locations.
    --location-delete: Remove an existing location from the list of
        image locations.
    --location-update: Update the metadata of existing location.

The glanceclient.v2.images.Controller class has been agumented with
three new methods to support the commands listed above:
    - add_location
    - delete_locations
    - update_location

The server has not been modified, i.e. all location related API requests
are passed to the server via HTTP PATCH requests and handled by the
server's image update function.

The v2 'image' and 'shell' related tests have also been supplemented.

Note that in order to use these options the server must be first
configured to expose location related info to the clients (i.e.
'show_multiple_locations' must be set to 'True").

I also added a mailmap entry for myself.

DocImpact
Closes-bug: #1271452
Co-Author: David Koo (koofoss) <david.koo@huawei.com>

Change-Id: Id1f320af05d9344645836359758e4aa227aafc69
changes/39/72539/11
David Koo 2014-02-11 11:06:02 +08:00 committed by Chris Buccella
parent dbefc1a3b1
commit 323d32cc6d
6 changed files with 360 additions and 33 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
.mailmap
View File

@ -1,5 +1,4 @@
# Format is:
# <preferred e-mail> <other e-mail 1>
# <preferred e-mail> <other e-mail 2>
# "man git-shortlog" for reference
<mr.alex.meade@gmail.com> <hatboy112@yahoo.com>
<mr.alex.meade@gmail.com> <alex.meade@rackspace.com>
David Koo <david.koo@huawei.com> <kpublicmail@gmail.com>

10
glanceclient/common/utils.py
View File

@ -325,3 +325,13 @@ def strip_version(endpoint):
if re.match('v\d+\.?\d*', url_bits[-1]):
endpoint = '/'.join(url_bits[:-1])
return endpoint
def print_image(image_obj, max_col_width=None):
ignore = ['self', 'access', 'file', 'schema']
image = dict([item for item in six.iteritems(image_obj)
if item[0] not in ignore])
if str(max_col_width).isdigit():
print_dict(image, max_column_width=max_col_width)
else:
print_dict(image)

93
glanceclient/v2/images.py
View File

@ -13,12 +13,14 @@
# License for the specific language governing permissions and limitations
# under the License.
import json
import six
from six.moves.urllib import parse
import warlock
from glanceclient.common import utils
from glanceclient import exc
from glanceclient.openstack.common import strutils
DEFAULT_PAGE_SIZE = 20
@ -170,3 +172,94 @@ class Controller(object):
# we need to fetch the image again to get a clean history. This is
# an obvious optimization for warlock
return self.get(image_id)
def _get_image_with_locations_or_fail(self, image_id):
image = self.get(image_id)
if getattr(image, 'locations', None) is None:
raise exc.HTTPBadRequest('The administrator has disabled '
'API access to image locations')
return image
def _send_image_update_request(self, image_id, patch_body):
url = '/v2/images/%s' % image_id
hdrs = {'Content-Type': 'application/openstack-images-v2.1-json-patch'}
self.http_client.raw_request('PATCH', url,
headers=hdrs,
body=json.dumps(patch_body))
def add_location(self, image_id, url, metadata):
"""Add a new location entry to an image's list of locations.
It is an error to add a URL that is already present in the list of
locations.
:param image_id: ID of image to which the location is to be added.
:param url: URL of the location to add.
:param metadata: Metadata associated with the location.
:returns: The updated image
"""
image = self._get_image_with_locations_or_fail(image_id)
url_list = [l['url'] for l in image.locations]
if url in url_list:
err_str = 'A location entry at %s already exists' % url
raise exc.HTTPConflict(err_str)
add_patch = [{'op': 'add', 'path': '/locations/-',
'value': {'url': url, 'metadata': metadata}}]
self._send_image_update_request(image_id, add_patch)
return self.get(image_id)
def delete_locations(self, image_id, url_set):
"""Remove one or more location entries of an image.
:param image_id: ID of image from which locations are to be removed.
:param url_set: set of URLs of location entries to remove.
:returns: None
"""
image = self._get_image_with_locations_or_fail(image_id)
current_urls = [l['url'] for l in image.locations]
missing_locs = url_set.difference(set(current_urls))
if missing_locs:
raise exc.HTTPNotFound('Unknown URL(s): %s' % list(missing_locs))
# NOTE: warlock doesn't generate the most efficient patch for remove
# operations (it shifts everything up and deletes the tail elements) so
# we do it ourselves.
url_indices = [current_urls.index(url) for url in url_set]
url_indices.sort(reverse=True)
patches = [{'op': 'remove', 'path': '/locations/%s' % url_idx}
for url_idx in url_indices]
self._send_image_update_request(image_id, patches)
def update_location(self, image_id, url, metadata):
"""Update an existing location entry in an image's list of locations.
The URL specified must be already present in the image's list of
locations.
:param image_id: ID of image whose location is to be updated.
:param url: URL of the location to update.
:param metadata: Metadata associated with the location.
:returns: The updated image
"""
image = self._get_image_with_locations_or_fail(image_id)
url_map = dict([(l['url'], l) for l in image.locations])
if url not in url_map:
raise exc.HTTPNotFound('Unknown URL: %s' % url)
if url_map[url]['metadata'] == metadata:
return image
# NOTE: The server (as of now) doesn't support modifying individual
# location entries. So we must:
# 1. Empty existing list of locations.
# 2. Send another request to set 'locations' to the new list
# of locations.
url_map[url]['metadata'] = metadata
patches = [{'op': 'replace',
'path': '/locations',
'value': p} for p in ([], list(url_map.values()))]
self._send_image_update_request(image_id, patches)
return self.get(image_id)

62
glanceclient/v2/shell.py
View File

@ -19,7 +19,6 @@ from glanceclient import exc
import json
import os
from os.path import expanduser
import six
IMAGE_SCHEMA = None
@ -54,14 +53,11 @@ def do_image_create(gc, args):
fields[key] = value
image = gc.images.create(**fields)
ignore = ['self', 'access', 'file', 'schema']
image = dict([item for item in six.iteritems(image)
if item[0] not in ignore])
utils.print_dict(image)
utils.print_image(image)
@utils.arg('id', metavar='<IMAGE_ID>', help='ID of image to update.')
@utils.schema_args(get_image_schema, omit=['id'])
@utils.schema_args(get_image_schema, omit=['id', 'locations'])
@utils.arg('--property', metavar="<key=value>", action='append',
default=[], help=('Arbitrary property to associate with image.'
' May be used multiple times.'))
@ -85,10 +81,7 @@ def do_image_update(gc, args):
image_id = fields.pop('id')
image = gc.images.update(image_id, remove_properties, **fields)
ignore = ['self', 'access', 'file', 'schema']
image = dict([item for item in six.iteritems(image)
if item[0] not in ignore])
utils.print_dict(image)
utils.print_image(image)
@utils.arg('--page-size', metavar='<SIZE>', default=None, type=int,
@ -125,9 +118,7 @@ def do_image_show(gc, args):
"""Describe a specific image."""
image = gc.images.get(args.id)
ignore = ['self', 'access', 'file', 'schema']
image = dict([item for item in six.iteritems(image) if item[0] not in
ignore])
utils.print_dict(image, max_column_width=int(args.max_column_width))
utils.print_image(image, int(args.max_column_width))
@utils.arg('--image-id', metavar='<IMAGE_ID>', required=True,
@ -263,3 +254,48 @@ def do_image_tag_delete(gc, args):
utils.exit('Unable to delete tag. Specify image_id and tag_value')
else:
gc.image_tags.delete(args.image_id, args.tag_value)
@utils.arg('--url', metavar='<URL>', required=True,
help='URL of location to add.')
@utils.arg('--metadata', metavar='<STRING>', default='{}',
help=('Metadata associated with the location. '
'Must be a valid JSON object (default: %(default)s)'))
@utils.arg('id', metavar='<ID>',
help='ID of image to which the location is to be added.')
def do_location_add(gc, args):
"""Add a location (and related metadata) to an image."""
try:
metadata = json.loads(args.metadata)
except ValueError:
utils.exit('Metadata is not a valid JSON object.')
else:
image = gc.images.add_location(args.id, args.url, metadata)
utils.print_dict(image)
@utils.arg('--url', metavar='<URL>', action='append', required=True,
help='URL of location to remove. May be used multiple times.')
@utils.arg('id', metavar='<ID>',
help='ID of image whose locations are to be removed.')
def do_location_delete(gc, args):
"""Remove locations (and related metadata) from an image."""
image = gc.images.delete_locations(args.id, set(args.url))
@utils.arg('--url', metavar='<URL>', required=True,
help='URL of location to update.')
@utils.arg('--metadata', metavar='<STRING>', default='{}',
help=('Metadata associated with the location. '
'Must be a valid JSON object (default: %(default)s)'))
@utils.arg('id', metavar='<ID>',
help='ID of image whose location is to be updated.')
def do_location_update(gc, args):
"""Update metadata of an image's location."""
try:
metadata = json.loads(args.metadata)
except ValueError:
utils.exit('Metadata is not a valid JSON object.')
else:
image = gc.images.update_location(args.id, args.url, metadata)
utils.print_dict(image)

137
tests/v2/test_images.py
View File

@ -14,11 +14,13 @@
# under the License.
import errno
import json
import testtools
import six
import warlock
from glanceclient import exc
from glanceclient.v2 import images
from tests import utils
@ -303,12 +305,43 @@ fixtures = {
{'images': []},
),
},
'/v2/images/a2b83adc-888e-11e3-8872-78acc0b951d8': {
'GET': (
{},
{
'id': 'a2b83adc-888e-11e3-8872-78acc0b951d8',
'name': 'image-location-tests',
'locations': [{u'url': u'http://foo.com/',
u'metadata': {u'foo': u'foometa'}},
{u'url': u'http://bar.com/',
u'metadata': {u'bar': u'barmeta'}}],
},
),
'PATCH': (
{},
'',
)
},
}
fake_schema = {
'name': 'image',
'properties': {'id': {}, 'name': {}},
'properties': {
'id': {},
'name': {},
'locations': {
'type': 'array',
'items': {
'type': 'object',
'properties': {
'metadata': {'type': 'object'},
'url': {'type': 'string'},
},
'required': ['url', 'metadata'],
},
},
},
'additionalProperties': {'type': 'string'}
}
FakeModel = warlock.model_factory(fake_schema)
@ -626,3 +659,105 @@ class TestController(testtools.TestCase):
params = {'name': 'pong', 'bad_prop': False}
with testtools.ExpectedException(TypeError):
self.controller.update(image_id, **params)
def test_location_ops_when_server_disabled_location_ops(self):
# Location operations should not be allowed if server has not
# enabled location related operations
image_id = '3a4560a1-e585-443e-9b39-553b46ec92d1'
estr = 'The administrator has disabled API access to image locations'
url = 'http://bar.com/'
meta = {'bar': 'barmeta'}
e = self.assertRaises(exc.HTTPBadRequest,
self.controller.add_location,
image_id, url, meta)
self.assertTrue(estr in str(e))
e = self.assertRaises(exc.HTTPBadRequest,
self.controller.delete_locations,
image_id, set([url]))
self.assertTrue(estr in str(e))
e = self.assertRaises(exc.HTTPBadRequest,
self.controller.update_location,
image_id, url, meta)
self.assertTrue(estr in str(e))
def _empty_get(self, image_id):
return ('GET', '/v2/images/%s' % image_id, {}, None)
def _patch_req(self, image_id, patch_body):
c_type = 'application/openstack-images-v2.1-json-patch'
return ('PATCH',
'/v2/images/%s' % image_id,
{'Content-Type': c_type},
json.dumps(patch_body))
def test_add_location(self):
image_id = 'a2b83adc-888e-11e3-8872-78acc0b951d8'
new_loc = {'url': 'http://spam.com/', 'metadata': {'spam': 'ham'}}
add_patch = {'path': '/locations/-', 'value': new_loc, 'op': 'add'}
image = self.controller.add_location(image_id, **new_loc)
self.assertEqual(self.api.calls, [
self._empty_get(image_id),
self._patch_req(image_id, [add_patch]),
self._empty_get(image_id)
])
def test_add_duplicate_location(self):
image_id = 'a2b83adc-888e-11e3-8872-78acc0b951d8'
new_loc = {'url': 'http://foo.com/', 'metadata': {'foo': 'newfoo'}}
err_str = 'A location entry at %s already exists' % new_loc['url']
err = self.assertRaises(exc.HTTPConflict,
self.controller.add_location,
image_id, **new_loc)
self.assertIn(err_str, str(err))
def test_remove_location(self):
image_id = 'a2b83adc-888e-11e3-8872-78acc0b951d8'
url_set = set(['http://foo.com/', 'http://bar.com/'])
del_patches = [{'path': '/locations/1', 'op': 'remove'},
{'path': '/locations/0', 'op': 'remove'}]
image = self.controller.delete_locations(image_id, url_set)
self.assertEqual(self.api.calls, [
self._empty_get(image_id),
self._patch_req(image_id, del_patches)
])
def test_remove_missing_location(self):
image_id = 'a2b83adc-888e-11e3-8872-78acc0b951d8'
url_set = set(['http://spam.ham/'])
err_str = 'Unknown URL(s): %s' % list(url_set)
err = self.assertRaises(exc.HTTPNotFound,
self.controller.delete_locations,
image_id, url_set)
self.assertTrue(err_str in str(err))
def test_update_location(self):
image_id = 'a2b83adc-888e-11e3-8872-78acc0b951d8'
new_loc = {'url': 'http://foo.com/', 'metadata': {'spam': 'ham'}}
fixture_idx = '/v2/images/%s' % (image_id)
orig_locations = fixtures[fixture_idx]['GET'][1]['locations']
loc_map = dict([(l['url'], l) for l in orig_locations])
loc_map[new_loc['url']] = new_loc
mod_patch = [{'path': '/locations', 'op': 'replace',
'value': []},
{'path': '/locations', 'op': 'replace',
'value': list(loc_map.values())}]
image = self.controller.update_location(image_id, **new_loc)
self.assertEqual(self.api.calls, [
self._empty_get(image_id),
self._patch_req(image_id, mod_patch),
self._empty_get(image_id)
])
def test_update_missing_location(self):
image_id = 'a2b83adc-888e-11e3-8872-78acc0b951d8'
new_loc = {'url': 'http://spam.com/', 'metadata': {'spam': 'ham'}}
err_str = 'Unknown URL: %s' % new_loc['url']
err = self.assertRaises(exc.HTTPNotFound,
self.controller.update_location,
image_id, **new_loc)
self.assertTrue(err_str in str(err))

86
tests/v2/test_shell_v2.py
View File

@ -14,6 +14,7 @@
# License for the specific language governing permissions and limitations
# under the License.
import json
import mock
import six
import testtools
@ -53,7 +54,7 @@ class ShellV2Test(testtools.TestCase):
utils.print_dict = mock.Mock()
utils.save_image = mock.Mock()
def _test_with_few_arguments(self, func, func_args, err_msg):
def assert_exits_with_msg(self, func, func_args, err_msg):
with mock.patch.object(utils, 'exit') as mocked_utils_exit:
mocked_utils_exit.return_value = '%s' % err_msg
@ -207,6 +208,59 @@ class ShellV2Test(testtools.TestCase):
utils.print_dict.assert_called_once_with({
'id': 'pass', 'name': 'IMG-01', 'disk_format': 'vhd'})
def test_do_location_add_update_with_invalid_json_metadata(self):
args = self._make_args({'id': 'pass',
'url': 'http://foo/bar',
'metadata': '{1, 2, 3}'})
self.assert_exits_with_msg(test_shell.do_location_add,
args,
'Metadata is not a valid JSON object.')
self.assert_exits_with_msg(test_shell.do_location_update,
args,
'Metadata is not a valid JSON object.')
def test_do_location_add(self):
gc = self.gc
loc = {'url': 'http://foo.com/', 'metadata': {'foo': 'bar'}}
args = self._make_args({'id': 'pass',
'url': loc['url'],
'metadata': json.dumps(loc['metadata'])})
with mock.patch.object(gc.images, 'add_location') as mocked_addloc:
expect_image = {'id': 'pass', 'locations': [loc]}
mocked_addloc.return_value = expect_image
test_shell.do_location_add(self.gc, args)
mocked_addloc.assert_called_once_with('pass',
loc['url'],
loc['metadata'])
utils.print_dict.assert_called_once_with(expect_image)
def test_do_location_delete(self):
gc = self.gc
loc_set = set(['http://foo/bar', 'http://spam/ham'])
args = self._make_args({'id': 'pass', 'url': loc_set})
with mock.patch.object(gc.images, 'delete_locations') as mocked_rmloc:
expect_image = {'id': 'pass', 'locations': []}
test_shell.do_location_delete(self.gc, args)
mocked_rmloc.assert_called_once_with('pass', loc_set)
def test_do_location_update(self):
gc = self.gc
loc = {'url': 'http://foo.com/', 'metadata': {'foo': 'bar'}}
args = self._make_args({'id': 'pass',
'url': loc['url'],
'metadata': json.dumps(loc['metadata'])})
with mock.patch.object(gc.images, 'update_location') as mocked_modloc:
expect_image = {'id': 'pass', 'locations': [loc]}
mocked_modloc.return_value = expect_image
test_shell.do_location_update(self.gc, args)
mocked_modloc.assert_called_once_with('pass',
loc['url'],
loc['metadata'])
utils.print_dict.assert_called_once_with(expect_image)
def test_do_explain(self):
input = {
'page_size': 18,
@ -292,9 +346,9 @@ class ShellV2Test(testtools.TestCase):
args = self._make_args({'image_id': None, 'member_id': 'MEM-01'})
msg = 'Unable to create member. Specify image_id and member_id'
self._test_with_few_arguments(func=test_shell.do_member_create,
func_args=args,
err_msg=msg)
self.assert_exits_with_msg(func=test_shell.do_member_create,
func_args=args,
err_msg=msg)
def test_do_member_update(self):
input = {
@ -322,9 +376,9 @@ class ShellV2Test(testtools.TestCase):
msg = 'Unable to update member. Specify image_id, member_id' \
' and member_status'
self._test_with_few_arguments(func=test_shell.do_member_update,
func_args=args,
err_msg=msg)
self.assert_exits_with_msg(func=test_shell.do_member_update,
func_args=args,
err_msg=msg)
def test_do_member_delete(self):
args = self._make_args({'image_id': 'IMG-01', 'member_id': 'MEM-01'})
@ -337,9 +391,9 @@ class ShellV2Test(testtools.TestCase):
args = self._make_args({'image_id': None, 'member_id': 'MEM-01'})
msg = 'Unable to delete member. Specify image_id and member_id'
self._test_with_few_arguments(func=test_shell.do_member_delete,
func_args=args,
err_msg=msg)
self.assert_exits_with_msg(func=test_shell.do_member_delete,
func_args=args,
err_msg=msg)
def test_image_tag_update(self):
args = self._make_args({'image_id': 'IMG-01', 'tag_value': 'tag01'})
@ -355,9 +409,9 @@ class ShellV2Test(testtools.TestCase):
args = self._make_args({'image_id': None, 'tag_value': 'tag01'})
msg = 'Unable to update tag. Specify image_id and tag_value'
self._test_with_few_arguments(func=test_shell.do_image_tag_update,
func_args=args,
err_msg=msg)
self.assert_exits_with_msg(func=test_shell.do_image_tag_update,
func_args=args,
err_msg=msg)
def test_image_tag_delete(self):
args = self._make_args({'image_id': 'IMG-01', 'tag_value': 'tag01'})
@ -372,6 +426,6 @@ class ShellV2Test(testtools.TestCase):
args = self._make_args({'image_id': 'IMG-01', 'tag_value': None})
msg = 'Unable to delete tag. Specify image_id and tag_value'
self._test_with_few_arguments(func=test_shell.do_image_tag_delete,
func_args=args,
err_msg=msg)
self.assert_exits_with_msg(func=test_shell.do_image_tag_delete,
func_args=args,
err_msg=msg)