[PATCH v2 0/3] Issue #543
The differences between this and the previous patchset (v1) are: - Patch 3/3 has been updated: generate unique name for all types of storage volumes (not just 'url'); rebased to the latest commit (a378799); reworded commit message. Crístian Viana (3): bugfix: Fix regexp in "kimchi.utils.get_next_clone_name" Support different "tags" when generating a new name in "get_next_clone_name" issue #543: Generate unique names when creating volumes without name src/kimchi/model/storagevolumes.py | 9 ++++++++- src/kimchi/utils.py | 17 ++++++++++------- 2 files changed, 18 insertions(+), 8 deletions(-) -- 1.9.3
The regular expression used in the function "kimchi.utils.get_next_clone_name" has an extra '-' which isn't needed. The following commands, executed from an interactive Python shell, demonstrate the bug:
from kimchi.utils import get_next_clone_name get_next_clone_name(['foo'], 'index', '.html') u'index-clone-1.html' get_next_clone_name(['foo', 'index-clone-1.html'], 'index', '.html') u'index-clone-1.html'
Even though the name 'index-clone-1.html' is provided as an existing name, the function still returns a duplicate value. Fix the regular expression by removing an unnecessary character. The following commands demonstrate the bug fix:
from kimchi.utils import get_next_clone_name get_next_clone_name(['foo'], 'index', '.html') u'index-clone-1.html' get_next_clone_name(['foo', 'index-clone-1.html'], 'index', '.html') u'index-clone-2.html' get_next_clone_name(['foo', 'index-clone-1.html', 'index-clone-2.html'], 'index', '.html') u'index-clone-3.html'
Signed-off-by: Crístian Viana <vianac@linux.vnet.ibm.com> --- src/kimchi/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/kimchi/utils.py b/src/kimchi/utils.py index 989e370..3af701c 100644 --- a/src/kimchi/utils.py +++ b/src/kimchi/utils.py @@ -335,7 +335,7 @@ def get_next_clone_name(all_names, basename, name_suffix=''): re_expr = u'%s-clone-(?P<%s>\d+)' % (basename, re_group_num) if name_suffix != '': - re_expr = u'%s-%s' % (re_expr, name_suffix) + re_expr = u'%s%s' % (re_expr, name_suffix) max_num = 0 re_compiled = re.compile(re_expr) -- 1.9.3
The current implementation of "kimchi.utils.get_next_clone_name" always appends the tag "-clone" in its return values. That may not be suitable for other cases when we want a new resource name without that tag. Add an optional parameter in "get_next_clone_name" so it supports different tags. Signed-off-by: Crístian Viana <vianac@linux.vnet.ibm.com> --- src/kimchi/utils.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/kimchi/utils.py b/src/kimchi/utils.py index 3af701c..bac07df 100644 --- a/src/kimchi/utils.py +++ b/src/kimchi/utils.py @@ -312,10 +312,11 @@ def validate_repo_url(url): raise InvalidParameter("KCHREPOS0002E") -def get_next_clone_name(all_names, basename, name_suffix=''): +def get_next_clone_name(all_names, basename, name_suffix='', + clone_tag='-clone'): """Find the next available name for a cloned resource. - If any resource named "<basename>-clone-<number><name_suffix>" is found + If any resource named "<basename><tag>-<number><name_suffix>" is found in "all_names", use the maximum "number" + 1; else, use 1. Arguments: @@ -325,15 +326,17 @@ def get_next_clone_name(all_names, basename, name_suffix=''): basename -- The name of the original resource. name_suffix -- The resource name suffix (optional). This parameter exist so that a resource named "foo.img" gets the name - "foo-clone-1.img" instead of "foo.img-clone-1". If this parameter + "foo<tag>-1.img" instead of "foo.img<tag>-1". If this parameter is used, the suffix should not be present in "basename". + clone_tag -- The tag which will be appended to the new name (optional). + Default value: "-clone" Return: - A UTF-8 string in the format "<basename>-clone-<number><name_suffix>". + A UTF-8 string in the format "<basename><tag>-<number><name_suffix>". """ re_group_num = 'num' - re_expr = u'%s-clone-(?P<%s>\d+)' % (basename, re_group_num) + re_expr = u'%s%s-(?P<%s>\d+)' % (basename, clone_tag, re_group_num) if name_suffix != '': re_expr = u'%s%s' % (re_expr, name_suffix) @@ -346,7 +349,7 @@ def get_next_clone_name(all_names, basename, name_suffix=''): max_num = max(max_num, int(match.group(re_group_num))) # increments the maximum "clone number" found - new_name = u'%s-clone-%d' % (basename, max_num + 1) + new_name = u'%s%s-%d' % (basename, clone_tag, max_num + 1) if name_suffix != '': new_name = new_name + name_suffix -- 1.9.3
If the user creates a volume without setting its name, Kimchi generates a default value. However, another volume with that generated name may already exist and the volume creation may fail. The following commands demonstrate the issue: $ kimchi_rest /storagepools/default/storagevolumes | grep name "name":"60f46fdc-8ecc-4601-90d6-b3b44d35c337-0.img", "name":"bdd22e7c-7794-4bdf-8d2b-0089242c8b16-0.img", "name":"c16a7e02-6dcf-48c5-83de-1bb2d8a110a8-0.img", $ kimchi_rest -m POST /storagepools/default/storagevolumes '{"url": "http://www.google.com/index.html"}' { "status":"running", "message":"OK", "id":"2", "target_uri":"/storagepools/default/storagevolumes/index.html" } $ kimchi_rest -m POST /storagepools/default/storagevolumes '{"url": "http://www.google.com/index.html"}' { "reason":"KCHVOL0001E: Storage volume index.html already exists", "code":"400 Bad Request", "call_stack":"Traceback (most recent call last):\n File \"/usr/lib/python2.7/site-packages/cherrypy/_cprequest.py\", line 656, in respond\n response.body = self.handler()\n File \"/usr/lib/python2.7/site-packages/cherrypy/lib/encoding.py\", line 188, in __call__\n self.body = self.oldhandler(*args, **kwargs)\n File \"/usr/lib/python2.7/site-packages/cherrypy/_cpdispatch.py\", line 34, in __call__\n return self.callable(*self.args, **self.kwargs)\n File \"/home/vianac/LTC/kimchi/src/kimchi/control/base.py\", line 331, in index\n raise cherrypy.HTTPError(400, e.message)\nHTTPError: (400, u'KCHVOL0001E: Storage volume index.html already exists')\n" Generate a unique name when creating a storage volume without a name. The following commands demonstrate the bug fix: $ kimchi_rest /storagepools/default/storagevolumes | grep name "name":"60f46fdc-8ecc-4601-90d6-b3b44d35c337-0.img", "name":"bdd22e7c-7794-4bdf-8d2b-0089242c8b16-0.img", "name":"c16a7e02-6dcf-48c5-83de-1bb2d8a110a8-0.img", $ kimchi_rest -m POST /storagepools/default/storagevolumes '{"url": "http://www.google.com/index.html"}' { "status":"running", "message":"OK", "id":"1", "target_uri":"/storagepools/default/storagevolumes/index.html" } $ kimchi_rest -m POST /storagepools/default/storagevolumes '{"url": "http://www.google.com/index.html"}' { "status":"running", "message":"OK", "id":"2", "target_uri":"/storagepools/default/storagevolumes/index-1.html" } $ kimchi_rest -m POST /storagepools/default/storagevolumes '{"url": "http://www.google.com/index.html"}' { "status":"running", "message":"OK", "id":"3", "target_uri":"/storagepools/default/storagevolumes/index-2.html" Fix issue #543 ("Download a volume with same basename will result error"). Signed-off-by: Crístian Viana <vianac@linux.vnet.ibm.com> --- src/kimchi/model/storagevolumes.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/kimchi/model/storagevolumes.py b/src/kimchi/model/storagevolumes.py index 406b38b..c33ac8e 100644 --- a/src/kimchi/model/storagevolumes.py +++ b/src/kimchi/model/storagevolumes.py @@ -76,6 +76,8 @@ class StorageVolumesModel(object): except: raise InvalidParameter('KCHVOL0022E', {'url': url}) + all_vol_names = self.get_list(pool_name) + if name is None: # the methods listed in 'REQUIRE_NAME_PARAMS' cannot have # 'name' == None @@ -91,6 +93,11 @@ class StorageVolumesModel(object): name = os.path.basename(params['url']) else: name = 'upload-%s' % int(time.time()) + + if name in all_vol_names: + basename, ext = os.path.splitext(name) + name = get_next_clone_name(all_vol_names, basename, ext, '') + params['name'] = name try: @@ -106,7 +113,7 @@ class StorageVolumesModel(object): if pool_info['state'] == 'inactive': raise InvalidParameter('KCHVOL0003E', {'pool': pool_name, 'volume': name}) - if name in self.get_list(pool_name): + if name in all_vol_names: raise InvalidParameter('KCHVOL0001E', {'name': name}) params['pool'] = pool_name -- 1.9.3
Reviewed-by: Daniel Barboza <danielhb@linux.vnet.ibm.com> On 12/10/2014 03:12 PM, Crístian Viana wrote:
The differences between this and the previous patchset (v1) are:
- Patch 3/3 has been updated: generate unique name for all types of storage volumes (not just 'url'); rebased to the latest commit (a378799); reworded commit message.
Crístian Viana (3): bugfix: Fix regexp in "kimchi.utils.get_next_clone_name" Support different "tags" when generating a new name in "get_next_clone_name" issue #543: Generate unique names when creating volumes without name
src/kimchi/model/storagevolumes.py | 9 ++++++++- src/kimchi/utils.py | 17 ++++++++++------- 2 files changed, 18 insertions(+), 8 deletions(-)
participants (2)
-
Crístian Viana -
Daniel Henrique Barboza