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

[Nir Soffer]

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&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Fabio Pliger <fabio.pliger(a)gmail.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes