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

2014-09-10 Thread oVirt Jenkins CI Server 
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

2014-09-10 Thread Federico Simoncelli 
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

2014-09-10 Thread nsoffer 
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

2014-09-10 Thread nsoffer 
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

2014-09-11 Thread nsoffer 
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

2014-09-11 Thread oVirt Jenkins CI Server 
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

2014-09-11 Thread nsoffer 
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

2014-09-13 Thread nsoffer 
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

2014-09-13 Thread oVirt Jenkins CI Server 
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

2014-09-13 Thread oVirt Jenkins CI Server 
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

2014-09-13 Thread nsoffer 
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

2014-09-14 Thread oVirt Jenkins CI Server 
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

2014-09-14 Thread nsoffer 
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

2014-09-14 Thread Federico Simoncelli 
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

2014-09-14 Thread oVirt Jenkins CI Server 
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

2014-09-14 Thread nsoffer 
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

2014-09-14 Thread nsoffer 
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

2014-09-14 Thread nsoffer 
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

2014-09-14 Thread oVirt Jenkins CI Server 
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

2014-09-15 Thread Federico Simoncelli 
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

2014-09-15 Thread fromani 
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

2014-09-15 Thread danken 
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

2014-09-15 Thread fromani 
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

2014-09-15 Thread nsoffer 
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

2014-09-15 Thread Federico Simoncelli 
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

2014-09-15 Thread nsoffer 
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

2014-09-15 Thread Federico Simoncelli 
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

2014-09-15 Thread nsoffer 
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

2014-09-15 Thread nsoffer 
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

2014-09-15 Thread fromani 
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

2014-09-15 Thread oVirt Jenkins CI Server 
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

2014-09-15 Thread fromani 
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

2014-09-15 Thread nsoffer 
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

2014-09-15 Thread Federico Simoncelli 
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

2014-09-15 Thread Federico Simoncelli 
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

2014-09-15 Thread danken 
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

2014-09-15 Thread oVirt Jenkins CI Server 
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