Nir Soffer has uploaded a new change for review.
Change subject: virt: Use Drive.diskType instead of networkDev and blockDev ......................................................................
virt: Use Drive.diskType instead of networkDev and blockDev
Now that we have explicit diskType we don't need to use the networkDev and blockDev properties. This is very useful when we set libvirt disk type property, or want to check for certain disk type.
Change-Id: Id68bc74b3d788dc82fc61bf8c3de5a52164d0989 Signed-off-by: Nir Soffer nsoffer@redhat.com --- M tests/vmStorageTests.py M vdsm/virt/vm.py 2 files changed, 21 insertions(+), 28 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/72/40472/1
diff --git a/tests/vmStorageTests.py b/tests/vmStorageTests.py index 8e8f7bc..dfe0991 100644 --- a/tests/vmStorageTests.py +++ b/tests/vmStorageTests.py @@ -268,74 +268,69 @@ def test_cdrom(self): conf = drive_config(device='cdrom') drive = Drive({}, self.log, **conf) - self.assertFalse(drive.networkDev) - self.assertFalse(drive.blockDev) + self.assertEqual(drive.diskType, DISK_TYPE.FILE)
def test_floppy(self): conf = drive_config(device='floppy') drive = Drive({}, self.log, **conf) - self.assertFalse(drive.networkDev) - self.assertFalse(drive.blockDev) + self.assertEqual(drive.diskType, DISK_TYPE.FILE)
def test_network_disk(self): conf = drive_config(diskType=DISK_TYPE.NETWORK) drive = Drive({}, self.log, **conf) - self.assertTrue(drive.networkDev) - self.assertFalse(drive.blockDev) + self.assertEqual(drive.diskType, DISK_TYPE.NETWORK)
@MonkeyPatch(utils, 'isBlockDevice', lambda path: True) def test_block_disk(self): conf = drive_config(device='disk') drive = Drive({}, self.log, **conf) - self.assertFalse(drive.networkDev) - self.assertTrue(drive.blockDev) + self.assertEqual(drive.diskType, DISK_TYPE.BLOCK)
@MonkeyPatch(utils, 'isBlockDevice', lambda path: False) def test_file_disk(self): conf = drive_config(device='disk') drive = Drive({}, self.log, **conf) - self.assertFalse(drive.networkDev) - self.assertFalse(drive.blockDev) + self.assertEqual(drive.diskType, DISK_TYPE.FILE)
@MonkeyPatch(utils, 'isBlockDevice', lambda path: False) def test_migrate_from_file_to_block(self): conf = drive_config(path='/filedomain/volume') drive = Drive({}, self.log, **conf) - self.assertFalse(drive.blockDev) + self.assertEqual(drive.diskType, DISK_TYPE.FILE) # Migrate drive to block domain... utils.isBlockDevice = lambda path: True drive.path = "/blockdomain/volume" - self.assertTrue(drive.blockDev) + self.assertEqual(drive.diskType, DISK_TYPE.BLOCK)
@MonkeyPatch(utils, 'isBlockDevice', lambda path: True) def test_migrate_from_block_to_file(self): conf = drive_config(path='/blockdomain/volume') drive = Drive({}, self.log, **conf) - self.assertTrue(drive.blockDev) + self.assertEqual(drive.diskType, DISK_TYPE.BLOCK) # Migrate drive to file domain... utils.isBlockDevice = lambda path: False drive.path = "/filedomain/volume" - self.assertFalse(drive.blockDev) + self.assertEqual(drive.diskType, DISK_TYPE.FILE)
@MonkeyPatch(utils, 'isBlockDevice', lambda path: True) def test_migrate_from_block_to_network(self): conf = drive_config(path='/blockdomain/volume') drive = Drive({}, self.log, **conf) - self.assertTrue(drive.blockDev) + self.assertEqual(drive.diskType, DISK_TYPE.BLOCK) # Migrate drive to network disk... drive.path = "pool/volume" drive.diskType = DISK_TYPE.NETWORK - self.assertFalse(drive.blockDev) + self.assertEqual(drive.diskType, DISK_TYPE.NETWORK)
@MonkeyPatch(utils, 'isBlockDevice', lambda path: True) def test_migrate_network_to_block(self): conf = drive_config(diskType=DISK_TYPE.NETWORK, path='pool/volume') drive = Drive({}, self.log, **conf) - self.assertTrue(drive.networkDev) + self.assertEqual(drive.diskType, DISK_TYPE.NETWORK) # Migrate drive to block domain... drive.path = '/blockdomain/volume' drive.diskType = None - self.assertTrue(drive.blockDev) + self.assertEqual(drive.diskType, DISK_TYPE.BLOCK)
@expandPermutations diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py index 78b55c9..a6e742e 100644 --- a/vdsm/virt/vm.py +++ b/vdsm/virt/vm.py @@ -1862,7 +1862,7 @@ def _changeDisk(self, diskDeviceXmlElement): diskType = diskDeviceXmlElement.getAttribute('type')
- if diskType not in ['file', 'block']: + if diskType not in (DISK_TYPE.BLOCK, DISK_TYPE.FILE): return
diskSerial = diskDeviceXmlElement. \ @@ -1871,13 +1871,12 @@ for vmDrive in self._devices[hwclass.DISK]: if vmDrive.serial == diskSerial: # update the type - diskDeviceXmlElement.setAttribute( - 'type', 'block' if vmDrive.blockDev else 'file') + diskDeviceXmlElement.setAttribute('type', vmDrive.diskType)
# update the path + attr = 'dev' if vmDrive.diskType == DISK_TYPE.BLOCK else 'file' diskDeviceXmlElement.getElementsByTagName('source')[0]. \ - setAttribute('dev' if vmDrive.blockDev else 'file', - vmDrive.path) + setAttribute(attr, vmDrive.path)
# update the format (the disk might have been collapsed) diskDeviceXmlElement.getElementsByTagName('driver')[0]. \ @@ -2773,7 +2772,7 @@ # we specify type='block' and dev=path for block volumes but we # always speficy the file=path for backwards compatibility. args = {'type': sourceType, 'file': newPath} - if sourceType == 'block': + if sourceType == DISK_TYPE.BLOCK: args['dev'] = newPath disk.appendChildWithArgs('source', **args) return disk @@ -2881,7 +2880,7 @@ newDrives[vmDevName]["format"] = "cow"
# We need to keep track of the drive object because we cannot - # safely access the blockDev property until after prepareVolumePath + # safely access the diskType property until after prepareVolumePath vmDrives[vmDevName] = vmDrive
# If all the drives are the current ones, return success @@ -2905,9 +2904,8 @@ _rollbackDrives(preparedDrives) return errCode['snapshotErr']
- snapType = 'block' if vmDrives[vmDevName].blockDev else 'file' snapelem = _diskSnapshot(vmDevName, newDrives[vmDevName]["path"], - snapType) + vmDrives[vmDevName].diskType) disks.appendChild(snapelem)
snap.appendChild(disks) @@ -4553,7 +4551,7 @@ sourceXML = find_element_by_name(diskXML, 'source') if not sourceXML: break - sourceAttr = ('file', 'dev')[drive.blockDev] + sourceAttr = 'dev' if drive.diskType == DISK_TYPE.BLOCK else 'file' path = sourceXML.getAttribute(sourceAttr)
# TODO: Allocation information is not available in the XML. Switch
automation@ovirt.org has posted comments on this change.
Change subject: virt: Use Drive.diskType instead of networkDev and blockDev ......................................................................
Patch Set 1:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
automation@ovirt.org has posted comments on this change.
Change subject: virt: Use Drive.diskType instead of networkDev and blockDev ......................................................................
Patch Set 2:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
automation@ovirt.org has posted comments on this change.
Change subject: virt: Use Drive.diskType instead of networkDev and blockDev ......................................................................
Patch Set 3:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
automation@ovirt.org has posted comments on this change.
Change subject: virt: Use Drive.diskType instead of networkDev and blockDev ......................................................................
Patch Set 4:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Francesco Romani has posted comments on this change.
Change subject: virt: Use Drive.diskType instead of networkDev and blockDev ......................................................................
Patch Set 4: Code-Review+1
(1 comment)
the patch looks good. I have concerns about disks being possibly misidentified because the introduction of the network disk type, but this patch makes the code no worse in this regard.
https://gerrit.ovirt.org/#/c/40472/4/vdsm/virt/vm.py File vdsm/virt/vm.py:
Line 1873: # update the type Line 1874: diskDeviceXmlElement.setAttribute('type', vmDrive.diskType) Line 1875: Line 1876: # update the path Line 1877: attr = 'dev' if vmDrive.diskType == DISK_TYPE.BLOCK else 'file' not related to this (good) change, but regarding this topic branch.
I'm not sure there is any guarantee that we will never get there with vmDrive.diskType == DISK_TYPE.NETWORK, hence triggering a bug. Line 1878: diskDeviceXmlElement.getElementsByTagName('source')[0]. \ Line 1879: setAttribute(attr, vmDrive.path) Line 1880: Line 1881: # update the format (the disk might have been collapsed)
Nir Soffer has posted comments on this change.
Change subject: virt: Use Drive.diskType instead of networkDev and blockDev ......................................................................
Patch Set 4:
(1 comment)
https://gerrit.ovirt.org/#/c/40472/4/vdsm/virt/vm.py File vdsm/virt/vm.py:
Line 1873: # update the type Line 1874: diskDeviceXmlElement.setAttribute('type', vmDrive.diskType) Line 1875: Line 1876: # update the path Line 1877: attr = 'dev' if vmDrive.diskType == DISK_TYPE.BLOCK else 'file'
not related to this (good) change, but regarding this topic branch.
This validation is indeed missing in the current code. I will add code failing loudly if we get a network disk where we expect only file or block. Line 1878: diskDeviceXmlElement.getElementsByTagName('source')[0]. \ Line 1879: setAttribute(attr, vmDrive.path) Line 1880: Line 1881: # update the format (the disk might have been collapsed)
automation@ovirt.org has posted comments on this change.
Change subject: virt: Use Drive.diskType instead of networkDev and blockDev ......................................................................
Patch Set 5:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
automation@ovirt.org has posted comments on this change.
Change subject: virt: Use Drive.diskType instead of networkDev and blockDev ......................................................................
Patch Set 6:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Jenkins CI RO has abandoned this change.
Change subject: virt: Use Drive.diskType instead of networkDev and blockDev ......................................................................
Abandoned
Abandoned due to no activity - please restore if still relevant
gerrit-hooks has posted comments on this change.
Change subject: virt: Use Drive.diskType instead of networkDev and blockDev ......................................................................
Patch Set 6:
* Update tracker: IGNORE, no Bug-Url found
Nir Soffer has restored this change.
Change subject: virt: Use Drive.diskType instead of networkDev and blockDev ......................................................................
Restored
vdsm-patches@lists.fedorahosted.org