Change in vdsm[master]: vm: implement VM.sdIds as read-only property

nsoffer at redhat.com nsoffer at redhat.com
Thu Sep 11 06:06:04 UTC 2014


Nir Soffer has posted comments on this change.

Change subject: vm: implement VM.sdIds as read-only property
......................................................................


Patch Set 1:

(6 comments)

Nice!

http://gerrit.ovirt.org/#/c/32796/1/tests/vmTests.py
File tests/vmTests.py:

Line 1303:                 "specParams": {
Line 1304:                     "ioTune": {
Line 1305:                         "total_bytes_sec": 9999,
Line 1306:                         "total_iops_sec": 9999}
Line 1307:                 }},
Please fix the indentation of the closing braces:

    a: {
        b: {
            c: "v"
        }
    }
Line 1308:                 log=self.log,
Line 1309:                 index=0,
Line 1310:                 device="hdd",
Line 1311:                 path="/dev/dummy",


Line 1315: 
Line 1316:         # Make the drive look like a VDSM volume
Line 1317:         required = ('domainID', 'imageID', 'poolID', 'volumeID')
Line 1318:         for p in required:
Line 1319:             setattr(drives[0], p, "1")
Why do we need this loop here? I think you can create the drive with this values like a read drive is created.
Line 1320: 
Line 1321:         with FakeVM() as machine:
Line 1322:             dom = FakeDomain()
Line 1323:             machine._dom = dom


Line 1321:         with FakeVM() as machine:
Line 1322:             dom = FakeDomain()
Line 1323:             machine._dom = dom
Line 1324:             ids = []
Line 1325:             for drive in drives:
> I prefer that the expected output is explicitly written instead of re-gener
+1
Line 1326:                 ids.append(drive.domainId)
Line 1327:                 machine._devices[drive.type].append(drive)
Line 1328: 
Line 1329:             self.assertEqual(machine.sdId, ids)


Line 1325:             for drive in drives:
Line 1326:                 ids.append(drive.domainId)
Line 1327:                 machine._devices[drive.type].append(drive)
Line 1328: 
Line 1329:             self.assertEqual(machine.sdId, ids)
We don't care about drives order, use a set().
Line 1330: 
Line 1331:     def testGetUnderlyingGraphicsDeviceInfo(self):
Line 1332:         port = '6000'
Line 1333:         tlsPort = '6001'


http://gerrit.ovirt.org/#/c/32796/1/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2867:             # So, to get proper device objects during VM recovery flow
Line 2868:             # we must to have updated conf before VM run
Line 2869:             self.saveState()
Line 2870:         else:
Line 2871:             pass
> I think this block can be removed entirely.
+1
Line 2872:             # Note that we do not start disk stats collection here since
Line 2873:             # in the recovery flow irs may not be ready yet.
Line 2874:             # Disk stats collection is started from clientIF at the end
Line 2875:             # of the recovery process.


Line 5597:     def sdIds(self):
Line 5598:         """
Line 5599:         Returns a list of the ids of the storage domains in use by the VM.
Line 5600:         """
Line 5601:         return [device.domainID for device in self._devices[DISK_DEVICES]]
> Not all the disks are vdsm images, therefore not all of them have domainID.
We don't care about the order of devices, and number of disk in each domain, so you should return a set().
Line 5602: 
Line 5603: 
Line 5604: class LiveMergeCleanupThread(threading.Thread):
Line 5605:     def __init__(self, vm, jobId, drive, doPivot):


-- 
To view, visit http://gerrit.ovirt.org/32796
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie445d4689aa562b00229844007ffa05eaecebdcb
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Fabio Pliger <fabio.pliger at gmail.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list