[PATCHv4 0/6] issue#382: Fix adding disk submitted with wrong default value
From: Royce Lv <lvroyce@linux.vnet.ibm.com> Royce Lv (6): issue#382 :Change false default value issue#382: Validate form for adding guest cdrom Add mockmodel for vm disk attach Make sure path and volume will not be specified at same time Add testcase for vmstorages create Add translation for vm storage error po/en_US.po | 14 +++++- po/kimchi.pot | 9 ++++ po/pt_BR.po | 18 +++++++- po/zh_CN.po | 17 +++++++- src/kimchi/i18n.py | 1 + src/kimchi/mockmodel.py | 12 +++++- src/kimchi/model/vmstorages.py | 2 + tests/test_model.py | 11 +++++ tests/test_rest.py | 69 ++++++++++++++++++++++++++++-- ui/js/src/kimchi.guest_storage_add.main.js | 40 ++++++++++++++++- ui/pages/i18n.json.tmpl | 7 ++- 11 files changed, 187 insertions(+), 13 deletions(-) -- 1.8.3.2
From: Royce Lv <lvroyce@linux.vnet.ibm.com> Default value for pool and volume were passed when path parameter is set. Fix this wrong behavior by delete the default values. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- ui/js/src/kimchi.guest_storage_add.main.js | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/ui/js/src/kimchi.guest_storage_add.main.js b/ui/js/src/kimchi.guest_storage_add.main.js index 6af00cc..80045d3 100644 --- a/ui/js/src/kimchi.guest_storage_add.main.js +++ b/ui/js/src/kimchi.guest_storage_add.main.js @@ -50,6 +50,16 @@ kimchi.guest_storage_add_main = function() { } }); + if ($(".path-section").hasClass('hidden')) { + $(poolTextbox).val('default'); + $(poolTextbox).change(); + $(pathTextbox).val(""); + } + else { + $(poolTextbox).val(""); + $(volTextbox).val(""); + } + $.each(types, function(index, elem){ if (selectType == elem.value) { var buses = new Array(); @@ -74,8 +84,6 @@ kimchi.guest_storage_add_main = function() { }); } }); - $(poolTextbox).val('default'); - $(poolTextbox).change(); kimchi.select('guest-add-storage-pool-list', options); } }); -- 1.8.3.2
From: Royce Lv <lvroyce@linux.vnet.ibm.com> Add function for cdrom and disk form validation, so that cdrom will not be created when no path given. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- ui/js/src/kimchi.guest_storage_add.main.js | 28 ++++++++++++++++++++++++++++ ui/pages/i18n.json.tmpl | 7 +++++-- 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/ui/js/src/kimchi.guest_storage_add.main.js b/ui/js/src/kimchi.guest_storage_add.main.js index 80045d3..753b070 100644 --- a/ui/js/src/kimchi.guest_storage_add.main.js +++ b/ui/js/src/kimchi.guest_storage_add.main.js @@ -135,6 +135,25 @@ kimchi.guest_storage_add_main = function() { }); }); + var validateCDROM = function(settings) { + if (/^(\/.*)+$/.test(settings['path'])) + return true; + else { + kimchi.message.error.code('KCHVMSTOR0001E'); + return false; + } + } + + var validateDisk = function(settings) { + if (settings['pool'] && settings['vol']) + return true; + else { + kimchi.message.error.code('KCHVMSTOR0002E'); + return false; + } + } + + validator = {cdrom: validateCDROM, disk: validateDisk}; var submitForm = function(event) { if (submitButton.prop('disabled')) { return false; @@ -155,6 +174,15 @@ kimchi.guest_storage_add_main = function() { settings[$(c).attr('name')] = $(c).val(); } }); + // Validate form for cdrom and disk + validateSpecifiedForm = validator[settings['type']]; + if (!validateSpecifiedForm(settings)) { + $(submitButton).prop('disabled', false); + $.each([submitButton, nameTextbox, pathTextbox, poolTextbox, volTextbox], function(i, c) { + $(c).prop('disabled', false); + }); + return false; + } $(submitButton).addClass('loading').text(i18n['KCHVMCD6003M']); kimchi.addVMStorage(settings, function(result) { diff --git a/ui/pages/i18n.json.tmpl b/ui/pages/i18n.json.tmpl index ce23bc4..d765cf2 100644 --- a/ui/pages/i18n.json.tmpl +++ b/ui/pages/i18n.json.tmpl @@ -162,5 +162,8 @@ "KCHPOOL6009E": "$_("This is not a valid Server Name or IP. please, modify it.")", "KCHPOOL6010M": "$_("Looking for available partitions ...")", "KCHPOOL6011M": "$_("No available partitions found.")", - "KCHPOOL6012M": "$_("This storage pool is not persistent. Instead of deactivate, this action will permanently delete it. Would you like to continue?")" -} \ No newline at end of file + "KCHPOOL6012M": "$_("This storage pool is not persistent. Instead of deactivate, this action will permanently delete it. Would you like to continue?")", + + "KCHVMSTOR0001E": "$_("CDROM path need to be a valid local path and cannot be blank.")", + "KCHVMSTOR0002E": "$_("Disk pool or volume cannot be blank.")" +} -- 1.8.3.2
From: Royce Lv <lvroyce@linux.vnet.ibm.com> Add mockmodel for disk attach. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/mockmodel.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py index 05720f4..aecc2a1 100644 --- a/src/kimchi/mockmodel.py +++ b/src/kimchi/mockmodel.py @@ -663,9 +663,13 @@ class MockModel(object): def vmstorages_create(self, vm_name, params): path = params.get('path') - if path.startswith('/') and not os.path.exists(path): + if path and path.startswith('/') and not os.path.exists(path): raise InvalidParameter("KCHVMSTOR0003E", {'value': path}) - + elif params.get('pool'): + try: + self.storagevolume_lookup(params['pool'], params['vol']) + except Exception as e: + raise InvalidParameter("KCHVMSTOR0015E", {'error': e}) dom = self._get_vm(vm_name) dev = params.get('dev', None) if dev and dev in self.vmstorages_get_list(vm_name): @@ -970,6 +974,8 @@ class MockVMStorageDevice(object): def __init__(self, params): self.info = {'dev': params.get('dev'), 'type': params.get('type'), + 'pool': params.get('pool'), + 'vol': params.get('vol'), 'path': params.get('path')} -- 1.8.3.2
From: Royce Lv <lvroyce@linux.vnet.ibm.com> When adding vm storage, volume and path cannot be specified at the same time. Fix it. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/i18n.py | 1 + src/kimchi/mockmodel.py | 2 ++ src/kimchi/model/vmstorages.py | 2 ++ 3 files changed, 5 insertions(+) diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index 90ecc17..91684b6 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -246,6 +246,7 @@ messages = { "KCHVMSTOR0014E": _("Controller type %(type)s limitation of %(limit)s devices reached"), "KCHVMSTOR0015E": _("Cannot lookup disk path information by given pool/volume: %(error)s"), "KCHVMSTOR0016E": _("Volume already been used by other vm"), + "KCHVMSTOR0017E": _("Only one of path or pool/volume can be specified to add a new virtual machine disk"), "KCHREPOS0001E": _("YUM Repository ID must be one word only string."), "KCHREPOS0002E": _("Repository URL must be an http://, ftp:// or file:// URL."), diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py index aecc2a1..a0920e0 100644 --- a/src/kimchi/mockmodel.py +++ b/src/kimchi/mockmodel.py @@ -665,6 +665,8 @@ class MockModel(object): path = params.get('path') if path and path.startswith('/') and not os.path.exists(path): raise InvalidParameter("KCHVMSTOR0003E", {'value': path}) + if path and params.get('pool'): + raise InvalidParameter("KCHVMSTOR0017E") elif params.get('pool'): try: self.storagevolume_lookup(params['pool'], params['vol']) diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py index 8c51716..cd985fa 100644 --- a/src/kimchi/model/vmstorages.py +++ b/src/kimchi/model/vmstorages.py @@ -154,6 +154,8 @@ class VMStoragesModel(object): # Path will never be blank due to API.json verification. # There is no need to cover this case here. params['format'] = 'raw' + if not ('vol' in params) ^ ('path' in params): + raise InvalidParameter("KCHVMSTOR0017E") if params.get('vol'): try: pool = params['pool'] -- 1.8.3.2
From: Royce Lv <lvroyce@linux.vnet.ibm.com> Add testcases to cover vmstorage create fails when pool and path specified at the same time, and vol not specified when pool is passed. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- tests/test_model.py | 11 +++++++++ tests/test_rest.py | 69 +++++++++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 76 insertions(+), 4 deletions(-) diff --git a/tests/test_model.py b/tests/test_model.py index ea38f59..89cadaa 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -184,6 +184,9 @@ class ModelTests(unittest.TestCase): @unittest.skipUnless(utils.running_as_root(), 'Must be run as root') def test_vm_disk(self): + disk_path = '/tmp/existent2.iso' + open(disk_path, 'w').close() + def _attach_disk(bus_type=None): disk_args = {"type": "disk", "pool": pool, @@ -253,6 +256,14 @@ class ModelTests(unittest.TestCase): self.assertEquals(u'disk', disk_info['type']) inst.vmstorage_delete(vm_name, disk) + # Specify pool and path at sametime will fail + disk_args = {"type": "disk", + "bus": "virtio", + "pool": pool, + "vol": vol, + "path": disk_path} + self.assertRaises( + InvalidParameter, inst.vmstorages_create, vm_name, disk_args) # Hot plug 'ide' bus disk does not work inst.vm_start(vm_name) self.assertRaises(InvalidOperation, _attach_disk, 'ide') diff --git a/tests/test_rest.py b/tests/test_rest.py index 18ba66e..2c57ecd 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -457,15 +457,74 @@ class RestTests(unittest.TestCase): resp = self.request('/vms/test-vm/storages', req, 'POST') self.assertEquals(400, resp.status) + # Create temp storage pool + req = json.dumps({'name': 'tmp', + 'capacity': 1024, + 'allocated': 512, + 'path': '/tmp', + 'type': 'dir'}) + resp = self.request('/storagepools', req, 'POST') + self.assertEquals(201, resp.status) + resp = self.request('/storagepools/tmp/activate', req, 'POST') + self.assertEquals(200, resp.status) + + req = json.dumps({'name': "attach-volume", + 'capacity': 1024, + 'allocation': 512, + 'type': 'disk', + 'format': 'raw'}) + resp = self.request('/storagepools/tmp/storagevolumes', + req, 'POST') + self.assertEquals(201, resp.status) + + # Attach cdrom with both path and volume specified + open('/tmp/mock.iso', 'w').close() + req = json.dumps({'dev': 'hdx', + 'type': 'cdrom', + 'pool': 'tmp', + 'vol': 'attach-volume', + 'path': '/tmp/mock.iso'}) + resp = self.request('/vms/test-vm/storages', req, 'POST') + self.assertEquals(400, resp.status) + + # Attach disk with both path and volume specified + req = json.dumps({'dev': 'hdx', + 'type': 'disk', + 'pool': 'tmp', + 'vol': 'attach-volume', + 'path': '/tmp/mock.iso'}) + resp = self.request('/vms/test-vm/storages', req, 'POST') + self.assertEquals(400, resp.status) + + # Attach disk with only pool specified + req = json.dumps({'dev': 'hdx', + 'type': 'cdrom', + 'pool': 'tmp'}) + resp = self.request('/vms/test-vm/storages', req, 'POST') + self.assertEquals(400, resp.status) + + # Attach disk with pool and vol specified + req = json.dumps({'dev': 'hdx', + 'type': 'disk', + 'pool': 'tmp', + 'vol': 'attach-volume'}) + resp = self.request('/vms/test-vm/storages', req, 'POST') + self.assertEquals(201, resp.status) + cd_info = json.loads(resp.read()) + self.assertEquals('hdx', cd_info['dev']) + self.assertEquals('disk', cd_info['type']) + self.assertEquals('tmp', cd_info['pool']) + self.assertEquals('attach-volume', cd_info['vol']) + # Attach a cdrom with existent dev name open('/tmp/existent.iso', 'w').close() - req = json.dumps({'dev': 'hdx', + req = json.dumps({'dev': 'hdk', 'type': 'cdrom', 'path': '/tmp/existent.iso'}) resp = self.request('/vms/test-vm/storages', req, 'POST') self.assertEquals(201, resp.status) cd_info = json.loads(resp.read()) - self.assertEquals('hdx', cd_info['dev']) + self.assertEquals('hdk', cd_info['dev']) self.assertEquals('cdrom', cd_info['type']) self.assertEquals('/tmp/existent.iso', cd_info['path']) # Delete the file and cdrom @@ -482,7 +541,7 @@ class RestTests(unittest.TestCase): # Test GET devs = json.loads(self.request('/vms/test-vm/storages').read()) - self.assertEquals(3, len(devs)) + self.assertEquals(4, len(devs)) # Detach storage cdrom resp = self.request('/vms/test-vm/storages/hdx', '{}', 'DELETE') @@ -490,7 +549,9 @@ class RestTests(unittest.TestCase): # Test GET devs = json.loads(self.request('/vms/test-vm/storages').read()) - self.assertEquals(2, len(devs)) + self.assertEquals(3, len(devs)) + resp = self.request('/storagepools/tmp', {}, 'DELETE') + self.assertEquals(204, resp.status) def test_vm_iface(self): -- 1.8.3.2
From: Royce Lv <lvroyce@linux.vnet.ibm.com> Add corresponding translation for vm storage error Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- po/en_US.po | 14 +++++++++++++- po/kimchi.pot | 9 +++++++++ po/pt_BR.po | 18 +++++++++++++++++- po/zh_CN.po | 17 ++++++++++++++++- 4 files changed, 55 insertions(+), 3 deletions(-) diff --git a/po/en_US.po b/po/en_US.po index 1f274dc..85c2e87 100644 --- a/po/en_US.po +++ b/po/en_US.po @@ -742,6 +742,11 @@ msgstr "" msgid "Controller type %(type)s limitation of %(limit)s devices reached" msgstr "" +msgid "" +"Only one of path or pool/volume can be specified to add a new virtual " +"machine disk" +msgstr "" + msgid "YUM Repository ID must be one word only string." msgstr "" @@ -1276,6 +1281,13 @@ msgid "" "permanently delete it. Would you like to continue?" msgstr "" +msgid "CDROM path need to be a valid local path and cannot be blank." +msgstr "" + +#, fuzzy +msgid "Disk pool or volume cannot be blank." +msgstr "Server name can not be blank." + msgid "" "This will permanently delete the storage pool. Would you like to continue?" msgstr "" @@ -1387,7 +1399,7 @@ msgstr "" msgid "URL to the repository. Supported protocols are http, ftp, and file." msgstr "" -msgid "Repository is a mirror" +msgid "Repository is a mirror." msgstr "" msgid "Distribution" diff --git a/po/kimchi.pot b/po/kimchi.pot index 45aab9a..477b2e1 100755 --- a/po/kimchi.pot +++ b/po/kimchi.pot @@ -742,6 +742,9 @@ msgstr "" msgid "Controller type %(type)s limitation of %(limit)s devices reached" msgstr "" +msgid "Only one of path or pool/volume can be specified to add a new virtual machine disk" +msgstr "" + msgid "YUM Repository ID must be one word only string." msgstr "" @@ -1268,6 +1271,12 @@ msgid "" "permanently delete it. Would you like to continue?" msgstr "" +msgid "CDROM path need to be a valid local path and cannot be blank." +msgstr "" + +msgid "Disk pool or volume cannot be blank." +msgstr "" + msgid "" "This will permanently delete the storage pool. Would you like to continue?" msgstr "" diff --git a/po/pt_BR.po b/po/pt_BR.po index def044f..a5aea66 100644 --- a/po/pt_BR.po +++ b/po/pt_BR.po @@ -825,6 +825,13 @@ msgstr "Especifique o caminho para atualizar o disco da máquina virtual" msgid "Controller type %(type)s limitation of %(limit)s devices reached" msgstr "" +#, fuzzy +msgid "" +"Only one of path or pool/volume can be specified to add a new virtual " +"machine disk" +msgstr "" +"Especifique o tipo e caminho para adicionar um novo disco de máquina virtual" + msgid "YUM Repository ID must be one word only string." msgstr "ID do repositório YUM deve ser uma string com uma palavra." @@ -1379,6 +1386,14 @@ msgid "" "permanently delete it. Would you like to continue?" msgstr "" +#, fuzzy +msgid "CDROM path need to be a valid local path and cannot be blank." +msgstr "O caminho do storage pool não pode ser vazio." + +#, fuzzy +msgid "Disk pool or volume cannot be blank." +msgstr "O nome do storage pool não pode ser vazio." + msgid "" "This will permanently delete the storage pool. Would you like to continue?" msgstr "O storage pool vai ser permanentemente removido. Deseja continuar?" @@ -1494,7 +1509,8 @@ msgstr "Campo obrigatório" msgid "URL to the repository. Supported protocols are http, ftp, and file." msgstr "URL para o repositório. Protocolos suportados são http, ftp e arquivo." -msgid "Repository is a mirror" +#, fuzzy +msgid "Repository is a mirror." msgstr "Repositório é um mirror" msgid "Distribution" diff --git a/po/zh_CN.po b/po/zh_CN.po index 246909b..4be2159 100644 --- a/po/zh_CN.po +++ b/po/zh_CN.po @@ -762,6 +762,12 @@ msgstr "指定更新虚拟机磁盘的路径" msgid "Controller type %(type)s limitation of %(limit)s devices reached" msgstr "" +#, fuzzy +msgid "" +"Only one of path or pool/volume can be specified to add a new virtual " +"machine disk" +msgstr "为新虚拟机添加磁盘时，存储池和路径中只能指定一个" + msgid "YUM Repository ID must be one word only string." msgstr "YUM软件仓库ID必须是只包含一个单词的字符串" @@ -1289,6 +1295,14 @@ msgid "" "permanently delete it. Would you like to continue?" msgstr "" +#, fuzzy +msgid "CDROM path need to be a valid local path and cannot be blank." +msgstr "光驱中光盘路径必须为合法的本地路径且不为空" + +#, fuzzy +msgid "Disk pool or volume cannot be blank." +msgstr "存储池或卷的名称不能为空" + msgid "" "This will permanently delete the storage pool. Would you like to continue?" msgstr "这将永久删除存储池。是否继续？" @@ -1400,7 +1414,8 @@ msgstr "必需的字段" msgid "URL to the repository. Supported protocols are http, ftp, and file." msgstr "软件仓库的URL，支持的协议有http、ftp、和file" -msgid "Repository is a mirror" +#, fuzzy +msgid "Repository is a mirror." msgstr "软件仓库是一个镜像" msgid "Distribution" -- 1.8.3.2
participants (2)
-
Aline Manera -
lvroyce＠linux.vnet.ibm.com