[PATCH 0/3] Fix: Do not pass 'bus' param when attaching disk
From: Royce Lv <lvroyce@linux.vnet.ibm.com> We passed 'IDE' for default value when attaching cdrom, but 'IDE' bus is not supported on some platform(e.g. power), In fact, kimchi has a embeded scheme to decide which bus is proper, so let backend to decide which bus to use when attaching disk. Royce Lv (3): Delete 'bus' param from backend Delete 'bus' selection from UI Update testcases for bus type decision making src/kimchi/API.json | 6 ------ src/kimchi/model/vmstorages.py | 3 +-- tests/test_model.py | 34 ++++++++++++++++++------------ ui/js/src/kimchi.guest_storage_add.main.js | 28 ------------------------ ui/pages/guest-storage-add.html.tmpl | 13 ------------ 5 files changed, 22 insertions(+), 62 deletions(-) -- 1.8.3.2
From: Royce Lv <lvroyce@linux.vnet.ibm.com> Since we pick the best bus type according to distro/version/arch when creating template instead of giving users choice, this logic can be applied to add vm disk logic. So delete 'bus' param from api and let kimchi decide the best bus type to use. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/API.json | 6 ------ src/kimchi/model/vmstorages.py | 3 +-- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/src/kimchi/API.json b/src/kimchi/API.json index 4b432a2..640ed10 100644 --- a/src/kimchi/API.json +++ b/src/kimchi/API.json @@ -500,12 +500,6 @@ "type": "string", "pattern": "^((/)|(http)[s]?:|[t]?(ftp)[s]?:)+.*$", "error": "KCHVMSTOR0003E" - }, - "bus": { - "description": "Target bus type of attached device", - "type": "string", - "pattern": "^scsi|ide|virtio$", - "error": "KCHVMSTOR0005E" } } }, diff --git a/src/kimchi/model/vmstorages.py b/src/kimchi/model/vmstorages.py index cd985fa..c15744b 100644 --- a/src/kimchi/model/vmstorages.py +++ b/src/kimchi/model/vmstorages.py @@ -171,8 +171,7 @@ class VMStoragesModel(object): params['format'] = vol_info['format'] params['path'] = vol_info['path'] params['src_type'] = _check_path(params['path']) - params.setdefault( - 'bus', _get_device_bus(params['type'], dom)) + params['bus'] = _get_device_bus(params['type'], dom) if (params['bus'] not in HOTPLUG_TYPE and DOM_STATE_MAP[dom.info()[0]] != 'shutoff'): raise InvalidOperation('KCHVMSTOR0011E') -- 1.8.3.2
From: Royce Lv <lvroyce@linux.vnet.ibm.com> Do not expose 'bus' selection in UI and let kimchi choose for them. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- ui/js/src/kimchi.guest_storage_add.main.js | 28 ---------------------------- ui/pages/guest-storage-add.html.tmpl | 13 ------------- 2 files changed, 41 deletions(-) diff --git a/ui/js/src/kimchi.guest_storage_add.main.js b/ui/js/src/kimchi.guest_storage_add.main.js index 753b070..17cb991 100644 --- a/ui/js/src/kimchi.guest_storage_add.main.js +++ b/ui/js/src/kimchi.guest_storage_add.main.js @@ -19,21 +19,17 @@ kimchi.guest_storage_add_main = function() { var types = [{ label: 'cdrom', value: 'cdrom', - bus: ['ide'] }, { label: 'disk', value: 'disk', - bus: ['virtio', 'ide'] }]; kimchi.select('guest-storage-type-list', types); - kimchi.select('guest-storage-bus-list', [{label: 'ide', value: 'ide'}]); var storageAddForm = $('#form-guest-storage-add'); var submitButton = $('#guest-storage-button-add'); var nameTextbox = $('input[name="dev"]', storageAddForm); var typeTextbox = $('input[name="type"]', storageAddForm); - var busTextbox = $('input[name="bus"]', storageAddForm); var pathTextbox = $('input[name="path"]', storageAddForm); var poolTextbox = $('input[name="pool"]', storageAddForm); var volTextbox = $('input[name="vol"]', storageAddForm); @@ -59,18 +55,6 @@ kimchi.guest_storage_add_main = function() { $(poolTextbox).val(""); $(volTextbox).val(""); } - - $.each(types, function(index, elem){ - if (selectType == elem.value) { - var buses = new Array(); - $.each(elem.bus, function (index, elem) { - buses[index] = {label: elem, value: elem}; - }); - $('#guest-storage-bus').selectMenu("setData", buses); - $('#guest-storage-bus-label').text(buses[0].value); - $('#guest-storage-bus-type').val(buses[0].value); - } - }); }); kimchi.listStoragePools(function(result) { @@ -122,17 +106,6 @@ kimchi.guest_storage_add_main = function() { $(value).addClass('hidden'); } }); - - $.each(types, function(index, elem){ - if (selectType == elem.value) { - var buses = new Array(); - $.each(elem.bus, function (index, elem) { - buses[index] = {label: elem, value: elem}; - }); - $('#guest-storage-bus').selectMenu("setData", buses); - $('#guest-storage-bus-label').text(buses[0].value); - } - }); }); var validateCDROM = function(settings) { @@ -163,7 +136,6 @@ kimchi.guest_storage_add_main = function() { var settings = { vm: kimchi.selectedGuest, type: typeTextbox.val(), - bus: busTextbox.val() }; $(submitButton).prop('disabled', true); diff --git a/ui/pages/guest-storage-add.html.tmpl b/ui/pages/guest-storage-add.html.tmpl index 6574dcf..08bcd88 100644 --- a/ui/pages/guest-storage-add.html.tmpl +++ b/ui/pages/guest-storage-add.html.tmpl @@ -53,19 +53,6 @@ </div> </div> </section> - <section class="form-section"> - <h2>3. $_("Device Bus")</h2> - <div class="field"> - <div class="btn dropdown popable" id="guest-storage-bus"> - <input id="guest-storage-bus-type" name="bus" value='ide' type="hidden" /> - <span class="text" id="guest-storage-bus-label"></span> - <span class="arrow"></span> - <div class="popover"> - <ul class="select-list" id="guest-storage-bus-list" data-target="guest-storage-bus-type" data-label="guest-storage-bus-label"></ul> - </div> - </div> - </div> - </section> <div class="volume-section hidden"> <section class="form-section"> <h2>4. $_("Storage Pool")</h2> -- 1.8.3.2
From: Royce Lv <lvroyce@linux.vnet.ibm.com> Test kimchi will find right type of bus when attaching disks, update test for ide bus does not support hot plug. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- tests/test_model.py | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/tests/test_model.py b/tests/test_model.py index cab8288..696c0e5 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -188,14 +188,10 @@ class ModelTests(unittest.TestCase): disk_path = '/tmp/existent2.iso' open(disk_path, 'w').close() - def _attach_disk(bus_type=None): + def _attach_disk(expect_bus='virtio'): disk_args = {"type": "disk", "pool": pool, "vol": vol} - if bus_type: - disk_args['bus'] = bus_type - else: - bus_type = 'virtio' disk = inst.vmstorages_create(vm_name, disk_args) storage_list = inst.vmstorages_get_list(vm_name) self.assertEquals(prev_count + 1, len(storage_list)) @@ -204,7 +200,7 @@ class ModelTests(unittest.TestCase): disk_info = inst.vmstorage_lookup(vm_name, disk) self.assertEquals(u'disk', disk_info['type']) self.assertEquals(vol_path, disk_info['path']) - self.assertEquals(bus_type, disk_info['bus']) + self.assertEquals(expect_bus, disk_info['bus']) return disk inst = model.Model(objstore_loc=self.tmp_store) @@ -251,6 +247,7 @@ class ModelTests(unittest.TestCase): # Hot plug a disk inst.vm_start(vm_name) disk = _attach_disk() + # VM disk still there after powered off inst.vm_poweroff(vm_name) disk_info = inst.vmstorage_lookup(vm_name, disk) @@ -259,21 +256,32 @@ class ModelTests(unittest.TestCase): # 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 + + old_distro_iso = self.iso_path + 'rhel4_8.iso' + iso_gen.construct_fake_iso(old_distro_iso, True, '4.8', 'rhel') + + vm_name = 'kimchi-ide-bus-vm' + params = {'name': 'old_distro_template', 'disks': [], 'cdrom': old_distro_iso} + inst.templates_create(params) + rollback.prependDefer(inst.template_delete, 'old_distro_template') + params = {'name': vm_name, 'template': '/templates/old_distro_template'} + inst.vms_create(params) + rollback.prependDefer(inst.vm_delete, vm_name) + + # Attach will choose IDE bus for old distro + disk = _attach_disk('ide') + inst.vmstorage_delete('kimchi-ide-bus-vm', disk) + + # Hot plug IDE bus disk does not work inst.vm_start(vm_name) - self.assertRaises(InvalidOperation, _attach_disk, 'ide') + self.assertRaises(InvalidOperation, _attach_disk) inst.vm_poweroff(vm_name) - # Cold plug 'ide' bus disk can work - disk = _attach_disk() - inst.vmstorage_delete(vm_name, disk) - @unittest.skipUnless(utils.running_as_root(), 'Must be run as root') def test_vm_cdrom(self): inst = model.Model(objstore_loc=self.tmp_store) -- 1.8.3.2
The patch set looks good and it's working in general (read as, automatically choose the correct bus), except by the device parameter used in the new disk attached. In all my tests (x86 and Power), all the disks attached is set to use 'hda', instead of the sequential device - if already exists a 'hda' device the next one must be 'hdb'. Also, even for SCSI and Virtio, the device set up is 'hda', instead of 'vd?' or 'sd?' respectively. -- Paulo Ricardo Paz Vital <pvital@linux.vnet.ibm.com> IBM Linux Technology Center On Fri, 2014-08-01 at 16:19 +0800, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
We passed 'IDE' for default value when attaching cdrom, but 'IDE' bus is not supported on some platform(e.g. power), In fact, kimchi has a embeded scheme to decide which bus is proper, so let backend to decide which bus to use when attaching disk.
Royce Lv (3): Delete 'bus' param from backend Delete 'bus' selection from UI Update testcases for bus type decision making
src/kimchi/API.json | 6 ------ src/kimchi/model/vmstorages.py | 3 +-- tests/test_model.py | 34 ++++++++++++++++++------------ ui/js/src/kimchi.guest_storage_add.main.js | 28 ------------------------ ui/pages/guest-storage-add.html.tmpl | 13 ------------ 5 files changed, 22 insertions(+), 62 deletions(-)
On 08/04/2014 01:45 PM, Paulo Ricardo Paz Vital wrote:
The patch set looks good and it's working in general (read as, automatically choose the correct bus), except by the device parameter used in the new disk attached.
In all my tests (x86 and Power), all the disks attached is set to use 'hda', instead of the sequential device - if already exists a 'hda' device the next one must be 'hdb'.
Also, even for SCSI and Virtio, the device set up is 'hda', instead of 'vd?' or 'sd?' respectively.
Those problems are not related to this patch set. Royce, please send a separate patch to fix the problems mentioned by Paulo.
participants (3)
-
Aline Manera -
lvroyce＠linux.vnet.ibm.com -
Paulo Ricardo Paz Vital