Change in vdsm[master]: vm: implement VM.sdIds as read-only property
oVirt Jenkins CI Server has posted comments on this change. Change subject: vm: implement VM.sdIds as read-only property .. Patch Set 1: No Builds Executed http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11452/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12396/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/1651/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12241/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. -- 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 Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: implement VM.sdIds as read-only property
Federico Simoncelli has posted comments on this change. Change subject: vm: implement VM.sdIds as read-only property .. Patch Set 1: (7 comments) http://gerrit.ovirt.org/#/c/32796/1//COMMIT_MSG Commit Message: Line 5: CommitDate: 2014-09-10 23:27:17 +0200 Line 6: Line 7: vm: implement VM.sdIds as read-only property Line 8: Line 9: Changed the VM object sdIds list attribute. It used to be a list that kept a copy of the storage domains in use in an additional list (with all the difficulties of keeping the list in sync). That list has been replaced with a property that returns the storage domains in use getting them directly from the the vm devices in use. Commit message should respect the 72 columns limit. Line 10: Line 11: Change-Id: Ie445d4689aa562b00229844007ffa05eaecebdcb Line 12: Bug-Url: https://bugzilla.redhat.com/1094518 http://gerrit.ovirt.org/#/c/32796/1/tests/vmTests.py File tests/vmTests.py: Line 1308: log=self.log, Line 1309: index=0, Line 1310: device="hdd", Line 1311: path="/dev/dummy", Line 1312: type=vm.DISK_DEVICES, I don't recall, can't you add domainID (etc) here? Line 1313: iface="ide") Line 1314: ] Line 1315: Line 1316: # Make the drive look like a VDSM volume Line 1309: index=0, Line 1310: device="hdd", Line 1311: path="/dev/dummy", Line 1312: type=vm.DISK_DEVICES, Line 1313: iface="ide") Maybe you could add another drive that doesn't contain domainID to check that we're not failing. (Code as it is now will crash iiuc) Line 1314: ] Line 1315: Line 1316: # Make the drive look like a VDSM volume Line 1317: required = ('domainID', 'imageID', 'poolID', 'volumeID') 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") Please use uuids for consistency. 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-generated with the same algorithm used in the code. Line 1326: ids.append(drive.domainId) Line 1327: machine._devices[drive.type].append(drive) Line 1328: Line 1329: self.assertEqual(machine.sdId, ids) 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. 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. 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 Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: implement VM.sdIds as read-only property
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": , Line 1306: "total_iops_sec": } 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 Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: implement VM.sdIds as read-only property
Nir Soffer has posted comments on this change. Change subject: vm: implement VM.sdIds as read-only property .. Patch Set 1: (1 comment) http://gerrit.ovirt.org/#/c/32796/1//COMMIT_MSG Commit Message: Line 8: Line 9: Changed the VM object sdIds list attribute. It used to be a list that kept a copy of the storage domains in use in an additional list (with all the difficulties of keeping the list in sync). That list has been replaced with a property that returns the storage domains in use getting them directly from the the vm devices in use. Line 10: Line 11: Change-Id: Ie445d4689aa562b00229844007ffa05eaecebdcb Line 12: Bug-Url: https://bugzilla.redhat.com/1094518 This is not enough to close this bug, so better make this Relates-To: instead of Bug-Url: Unless you plan a series of patches redesigning the current mess. -- 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 Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: implement VM.sdIds as read-only property
Nir Soffer has posted comments on this change. Change subject: vm: implement VM.sdIds as read-only property .. Patch Set 1: (1 comment) http://gerrit.ovirt.org/#/c/32796/1/vdsm/virt/vm.py File vdsm/virt/vm.py: 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]] > Ok. Is domainID enough to stablish this? In this case that line can be chan Possible, but the blessed way is using isVdsmImage(device). And note that we need a set, not a list: return set(device.domainID for device in ... if isVdsmImage(device)) 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 Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Fabio Pliger Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: implement VM.sdIds as read-only property
oVirt Jenkins CI Server has posted comments on this change. Change subject: vm: implement VM.sdIds as read-only property .. Patch Set 2: No Builds Executed http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11473/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12417/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/1659/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12262/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. -- 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: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Fabio Pliger Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Fabio Pliger Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: implement VM.sdIds as read-only property
Nir Soffer has posted comments on this change. Change subject: vm: implement VM.sdIds as read-only property .. Patch Set 2: I don't see any changes in the code except the reformatting of the commit message - did you submit this by mistake? -- 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: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Fabio Pliger Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Fabio Pliger Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: implement VM.sdIds as read-only property
Nir Soffer has posted comments on this change. Change subject: vm: implement VM.sdIds as read-only property .. Patch Set 2: Fabio, did you try git review? it is much easier to use than manually pushing to gerrit. Here is quick instructions on how to use it: 1. Install it yum install git-review 2. Edit your .git/config, and add a gerrit remote [remote "gerrit"] url = http://gerrit.ovirt.org/p/vdsm.git pushurl = ssh://myusern...@gerrit.ovirt.org:29418/vdsm.git fetch = +refs/heads/*:refs/remotes/gerrit/* 3. Push a new version of your patch git review Please join #vdsm on irc.freenode.net if you need more help -- 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: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Fabio Pliger Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Fabio Pliger Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: implement VM.sdIds as read-only property
oVirt Jenkins CI Server has posted comments on this change. Change subject: vm: implement VM.sdIds as read-only property .. Patch Set 3: No Builds Executed http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11500/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12444/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/1662/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12289/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. -- 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: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Fabio Pliger Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Fabio Pliger Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: implement VM.sdIds as read-only property
oVirt Jenkins CI Server has posted comments on this change. Change subject: vm: implement VM.sdIds as read-only property .. Patch Set 4: No Builds Executed http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11501/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12445/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/1663/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12290/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. -- 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 Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Fabio Pliger Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: implement VM.sdIds as read-only property
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 Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Fabio Pliger Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: implement VM.sdIds as read-only property
oVirt Jenkins CI Server has posted comments on this change. Change subject: vm: implement VM.sdIds as read-only property .. Patch Set 5: No Builds Executed http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11515/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12459/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/1664/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12304/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. -- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Fabio Pliger Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Fabio Pliger Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: implement VM.sdIds as read-only property
Nir Soffer has posted comments on this change. Change subject: vm: implement VM.sdIds as read-only property .. Patch Set 5: (2 comments) A deeper cleanup is required in vm.py. Otherwise this is fine. Thanks Fabio! http://gerrit.ovirt.org/#/c/32796/5/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 1918: # A destroy request has been issued, exit early Line 1919: break Line 1920: drive['path'] = self.cif.prepareVolumePath(drive, self.id) Line 1921: if drive['device'] == 'disk' and isVdsmImage(drive): Line 1922: domains.append(drive['domainID']) You left behind a rather useless list of domains. Now that we don't append this list to the self.sdIds, we don't need to maintain the "domains" list. Line 1923: else: Line 1924: # Now we got all the resources we needed Line 1925: self.startDisksStatsCollection() Line 1926: Line 3538: diskParams = params.get('drive', {}) Line 3539: diskParams['path'] = self.cif.prepareVolumePath(diskParams) Line 3540: vdsmImg = isVdsmImage(diskParams) Line 3541: Line 3542: if vdsmImg: After you removed the other use of "vdsmImg" (was in line 3552), we are left with one usage, which makes this temporary pointless, and less readable (vdsmImg vs isVdsmImage(diskParams)). Please remove unneeded temporary. Line 3543: self._normalizeVdsmImg(diskParams) Line 3544: self._createTransientDisk(diskParams) Line 3545: Line 3546: self.updateDriveIndex(diskParams) -- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Fabio Pliger Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Fabio Pliger Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: implement VM.sdIds as read-only property
Federico Simoncelli has posted comments on this change. Change subject: vm: implement VM.sdIds as read-only property .. Patch Set 5: Let's get this fixed asap so we can merge it tomorrow. -- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Fabio Pliger Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Fabio Pliger Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: implement VM.sdIds as read-only property
oVirt Jenkins CI Server has posted comments on this change. Change subject: vm: implement VM.sdIds as read-only property .. Patch Set 6: No Builds Executed http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11517/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12461/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/1665/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12306/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. -- 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: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Fabio Pliger Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Fabio Pliger Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: implement VM.sdIds as read-only property
Nir Soffer has posted comments on this change. Change subject: vm: implement VM.sdIds as read-only property .. Patch Set 6: Code-Review+1 Looks great - Thanks Fabio! -- 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: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Fabio Pliger Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Fabio Pliger Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: implement VM.sdIds as read-only property
Nir Soffer has posted comments on this change. Change subject: vm: implement VM.sdIds as read-only property .. Patch Set 7: Fix typo in the tests, running make check is useful. Tests pass now. -- 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: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Fabio Pliger Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Fabio Pliger Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: implement VM.sdIds as read-only property
Nir Soffer has posted comments on this change. Change subject: vm: implement VM.sdIds as read-only property .. Patch Set 7: Code-Review+1 -- 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: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Fabio Pliger Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Fabio Pliger Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: implement VM.sdIds as read-only property
oVirt Jenkins CI Server has posted comments on this change. Change subject: vm: implement VM.sdIds as read-only property .. Patch Set 7: Build Failed http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11519/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12463/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/1666/ : There was an infra issue, please contact in...@ovirt.org http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12308/ : SUCCESS -- 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: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Fabio Pliger Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Fabio Pliger Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: implement VM.sdIds as read-only property
Federico Simoncelli has posted comments on this change. Change subject: vm: implement VM.sdIds as read-only property .. Patch Set 7: Code-Review+2 LGTM. Yeela can you take a look and check if everything is correct? Fabio, if you tried this patch and you're confident enough that it's not introducing any regression you should hit "Verified". Thanks! -- 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: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Fabio Pliger Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Fabio Pliger Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: implement VM.sdIds as read-only property
Francesco Romani has posted comments on this change. Change subject: vm: implement VM.sdIds as read-only property .. Patch Set 7: Code-Review+1 -- 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: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Fabio Pliger Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Fabio Pliger Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: implement VM.sdIds as read-only property
Dan Kenigsberg has posted comments on this change. Change subject: vm: implement VM.sdIds as read-only property .. Patch Set 7: Code-Review-1 (2 comments) http://gerrit.ovirt.org/#/c/32796/7/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 2872 Line 2873 Line 2874 Line 2875 Line 2876 this comment is unrelated to sdIds maintenance - please keep it. It was added very recently. Line 5588: def sdIds(self): Line 5589: """ Line 5590: Returns a list of the ids of the storage domains in use by the VM. Line 5591: """ Line 5592: return set(device.domainID for device in self._devices[DISK_DEVICES] The removed code had an additional check for drive['device'] == 'disk' I do not think it was correct - ISO domains should have been accounted for. However, this change extend from mere refactoring. Please confirm in the commit message that it was intentional; placing it in its own patch makes sense, too. Line 5593:if isVdsmImage(device)) Line 5594: Line 5595: Line 5596: class LiveMergeCleanupThread(threading.Thread): -- 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: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Fabio Pliger Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Fabio Pliger Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: implement VM.sdIds as read-only property
Francesco Romani has posted comments on this change. Change subject: vm: implement VM.sdIds as read-only property .. Patch Set 7: -Code-Review (1 comment) http://gerrit.ovirt.org/#/c/32796/7/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 2872 Line 2873 Line 2874 Line 2875 Line 2876 > this comment is unrelated to sdIds maintenance - please keep it. It was add I spent quite some time figuring out my score to this patch for this comment alone. I can get away with this removal (hence my +1) since the else clause is now gone. On the other hand, it's also true that the removal of this comment doesn't belong to this patch, and this documents a quite dark corner of VDSM (got bitten from this quite recently), and we can keep this by just de-indent the comment block of one level. Other than that, patch looks fine. -- 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: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Fabio Pliger Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Fabio Pliger Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: implement VM.sdIds as read-only property
Nir Soffer has posted comments on this change. Change subject: vm: implement VM.sdIds as read-only property .. Patch Set 7: (2 comments) http://gerrit.ovirt.org/#/c/32796/7/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 2872 Line 2873 Line 2874 Line 2875 Line 2876 > this comment is unrelated to sdIds maintenance - please keep it. It was add The comment was needed when we were adding domain ids, and *not* starting disk stats collection. Now the only place doing this is preparePath, so this comment is not much useful any more. Line 5588: def sdIds(self): Line 5589: """ Line 5590: Returns a list of the ids of the storage domains in use by the VM. Line 5591: """ Line 5592: return set(device.domainID for device in self._devices[DISK_DEVICES] > The removed code had an additional check for Lets keep the pointless 'disk' check, and remove it globally in another patch from all other places. Line 5593:if isVdsmImage(device)) Line 5594: Line 5595: Line 5596: class LiveMergeCleanupThread(threading.Thread): -- 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: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Fabio Pliger Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Fabio Pliger Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: implement VM.sdIds as read-only property
Federico Simoncelli has posted comments on this change. Change subject: vm: implement VM.sdIds as read-only property .. Patch Set 7: -Code-Review Let's take care of the last comments. -- 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: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Fabio Pliger Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Fabio Pliger Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: implement VM.sdIds as read-only property
Nir Soffer has posted comments on this change. Change subject: vm: implement VM.sdIds as read-only property .. Patch Set 7: (2 comments) http://gerrit.ovirt.org/#/c/32796/7/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 2872 Line 2873 Line 2874 Line 2875 Line 2876 > The comment was needed when we were adding domain ids, and *not* starting d I'm ok with leaving this comment. Add a title to make it more clear e.g. # recovery flow - ... Line 5588: def sdIds(self): Line 5589: """ Line 5590: Returns a list of the ids of the storage domains in use by the VM. Line 5591: """ Line 5592: return set(device.domainID for device in self._devices[DISK_DEVICES] > so new check in the inner loop should become device['device'] == 'disk' and Right, like the old check when collecting domain ids. Line 5593:if isVdsmImage(device)) Line 5594: Line 5595: Line 5596: class LiveMergeCleanupThread(threading.Thread): -- 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: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Fabio Pliger Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Fabio Pliger Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: implement VM.sdIds as read-only property
Federico Simoncelli has posted comments on this change. Change subject: vm: implement VM.sdIds as read-only property .. Patch Set 7: (1 comment) http://gerrit.ovirt.org/#/c/32796/7/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 2872 Line 2873 Line 2874 Line 2875 Line 2876 > I'm ok with leaving this comment. Add a title to make it more clear e.g. # IMO comments goes before: if not self.recovering: -- 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: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Fabio Pliger Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Fabio Pliger Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: implement VM.sdIds as read-only property
Nir Soffer has posted comments on this change. Change subject: vm: implement VM.sdIds as read-only property .. Patch Set 7: (1 comment) http://gerrit.ovirt.org/#/c/32796/7/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 2872 Line 2873 Line 2874 Line 2875 Line 2876 > IMO comments goes before: We have different flows here, so better have a comment for each flow. Lets not make it harder for Fabio, just keep the structure that was before. -- 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: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Fabio Pliger Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Fabio Pliger Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: implement VM.sdIds as read-only property
Nir Soffer has posted comments on this change. Change subject: vm: implement VM.sdIds as read-only property .. Patch Set 7: (1 comment) http://gerrit.ovirt.org/#/c/32796/7/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 2872 Line 2873 Line 2874 Line 2875 Line 2876 > should I wait for a confirmation on this before resending the patch? No, send the version that you like. -- 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: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Fabio Pliger Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Fabio Pliger Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: implement VM.sdIds as read-only property
Francesco Romani has posted comments on this change. Change subject: vm: implement VM.sdIds as read-only property .. Patch Set 7: (1 comment) http://gerrit.ovirt.org/#/c/32796/7/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 2872 Line 2873 Line 2874 Line 2875 Line 2876 > should I wait for a confirmation on this before resending the patch? I'd do the following: - amend the comment to read like # In the recovery flow we do not start disk stats collection here # since irs may not be ready yet. (the rest follows unchanged) - de-indent the comment block of one level, put it at the same level of the above if: if not self.recovering: # code unchanged # In the recovery flow... This is IMO the simplest modification which makes sense. -- 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: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Fabio Pliger Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Fabio Pliger Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: implement VM.sdIds as read-only property
oVirt Jenkins CI Server has posted comments on this change. Change subject: vm: implement VM.sdIds as read-only property .. Patch Set 8: No Builds Executed http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11532/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12476/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/1669/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12321/ : To avoid overloading the infrastructure, a whitelist for running gerrit triggered jobs has been set in place, if you feel like you should be in it, please contact infra at ovirt dot org. -- 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: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Fabio Pliger Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Fabio Pliger Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: implement VM.sdIds as read-only property
Francesco Romani has posted comments on this change. Change subject: vm: implement VM.sdIds as read-only property .. Patch Set 8: Code-Review+1 good enough for me. Nice work, thanks! -- 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: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Fabio Pliger Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Fabio Pliger Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: implement VM.sdIds as read-only property
Nir Soffer has posted comments on this change. Change subject: vm: implement VM.sdIds as read-only property .. Patch Set 8: Code-Review+1 Thanks Fabio and sorry for the bike-shedding. -- 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: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Fabio Pliger Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Fabio Pliger Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: implement VM.sdIds as read-only property
Federico Simoncelli has posted comments on this change. Change subject: vm: implement VM.sdIds as read-only property .. Patch Set 8: Code-Review+2 -- 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: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Fabio Pliger Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Fabio Pliger Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: implement VM.sdIds as read-only property
Federico Simoncelli has posted comments on this change. Change subject: vm: implement VM.sdIds as read-only property .. Patch Set 8: Fabio please hit "verified" if you verified that this is not introducing a regression. -- 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: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Fabio Pliger Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Fabio Pliger Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: implement VM.sdIds as read-only property
Dan Kenigsberg has submitted this change and it was merged. Change subject: vm: implement VM.sdIds as read-only property .. vm: implement VM.sdIds as read-only property Changed the VM object sdIds list attribute. It used to be a list that kept a copy of the storage domains in use in an additional list (with all the difficulties of keeping the list in sync). That list has been replaced with a property that returns the storage domains in use getting them directly from the the vm devices in use. Change-Id: Ie445d4689aa562b00229844007ffa05eaecebdcb Relates-To: https://bugzilla.redhat.com/1094518 Signed-off-by: Fabio Pliger Reviewed-on: http://gerrit.ovirt.org/32796 Reviewed-by: Francesco Romani Reviewed-by: Nir Soffer Reviewed-by: Federico Simoncelli --- M tests/vmTests.py M vdsm/virt/vm.py 2 files changed, 69 insertions(+), 23 deletions(-) Approvals: Fabio Pliger: Verified; Looks good to me, but someone else must approve Nir Soffer: Looks good to me, but someone else must approve Federico Simoncelli: Looks good to me, approved Francesco Romani: Looks good to me, but someone else must approve -- To view, visit http://gerrit.ovirt.org/32796 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ie445d4689aa562b00229844007ffa05eaecebdcb Gerrit-PatchSet: 9 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Fabio Pliger Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Fabio Pliger Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: implement VM.sdIds as read-only property
oVirt Jenkins CI Server has posted comments on this change. Change subject: vm: implement VM.sdIds as read-only property .. Patch Set 9: Build Failed http://jenkins.ovirt.org/job/vdsm_master_create-rpms_merged/1853/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_create-rpms_merged_test_debug/194/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_verify-error-codes_merged/5827/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/3986/ : FAILURE -- 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: 9 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Fabio Pliger Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Fabio Pliger Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches