Nir Soffer has posted comments on this change.
Change subject: vm: implement VM.sdIds as read-only property
......................................................................
Patch Set 4:
(5 comments)
Thanks Fabio!
You should remove the unrelated changes and fix the indentation.
Otherwise this seems fine.
http://gerrit.ovirt.org/#/c/32796/4/tests/vmTests.py
File tests/vmTests.py:
Line 198: expectedXML = expectedXML % conf
Line 199:
Line 200: testVm = vm.Vm(self, conf)
Line 201:
Line 202: output = testVm._buildCmdLine()
This change should not be in your patch, it came from a rebase on master - right?
Line 203:
Line 204: self.assertEqual(re.sub('\n\s*', ' ',
output.strip(' ')),
Line 205: re.sub('\n\s*', ' ',
expectedXML.strip(' ')))
Line 206: finally:
Line 1340: ]
Line 1341:
Line 1342: with FakeVM() as machine:
Line 1343: for drive in drives:
Line 1344: machine._devices[drive.type].append(drive)
I'm not sure that this is the way to add drives, maybe Francesco will suggest a better
way.
Line 1345:
Line 1346: self.assertEqual(machine.sdId, set([domainID]))
Line 1347:
Line 1348: def testGetUnderlyingGraphicsDeviceInfo(self):
http://gerrit.ovirt.org/#/c/32796/4/vdsm/virt/vm.py
File vdsm/virt/vm.py:
Line 2662:
Line 2663: dev._deviceXML = deviceXML
Line 2664: domxml.appendDeviceXML(deviceXML)
Line 2665:
Line 2666: def _buildCmdLine(self):
Should not be in this patch.
Line 2667: domxml = vmxml.Domain(self.conf, self.log, self.arch)
Line 2668: domxml.appendOs()
Line 2669:
Line 2670: if self.arch == caps.Architecture.X86_64:
Line 2880: # we need to complete the initialization, including
Line 2881: # domDependentInit, after the migration is completed.
Line 2882:
Line 2883: if not self.recovering and initDomain:
Line 2884: domxml = hooks.before_vm_start(self._buildCmdLine(), self.conf)
Should not be in this patch.
Line 2885: self.log.debug(domxml)
Line 2886:
Line 2887: if self.recovering:
Line 2888: self._dom = NotifyingVirDomain(
Line 5592: """
Line 5593: Returns a list of the ids of the storage domains in use by the VM.
Line 5594: """
Line 5595: return set(device.domainID for device in self._devices[DISK_DEVICES]
Line 5596: if isVdsmImage(device))
The last line should be indented like this:
return set(first line...
second line...)
Running pep8 tool on your file will tell you that:
pep8 vdsm/virt/vm.py
Line 5597:
Line 5598:
Line 5599: class LiveMergeCleanupThread(threading.Thread):
Line 5600: 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: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Fabio Pliger <fabio.pliger(a)gmail.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Fabio Pliger <fabio.pliger(a)gmail.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes