Change in vdsm[master]: virt: Move Vm._getUnderlyingDriveInfo() out of Vm

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: virt: Move Vm._getUnderlyingDriveInfo() out of Vm
..


Patch Set 11:

(1 comment)

https://gerrit.ovirt.org/#/c/53677/11/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 1723: devices = self._devices
Line 1724: vmdevices.network.Interface.update_device_info(
Line 1725: self, devices[hwclass.NIC])
Line 1726: vmdevices.storage.Drive.update_device_info(
Line 1727: self, devices[hwclass.DISK])
> I don't like them because updating the conf/devices from xml should not be 
> I don't like them because updating the conf/devices from xml should not be 
> the responsibility of the Device class.

I see.  I think it's desirable to group code by devices.  Most device classes 
just create and process domain XML and it wouldn't be good to perform those two 
transformations in separate places.  Properties of a given device should be 
handled in a single place, as a form of encapsulation.

> Also in this design the code for handling the xml is spread all over the 
> place, instead of one module handling this task.

We should generally improve XML handling, it's mess.  If we can do that then 
this shouldn't be a problem.
Line 1728: vmdevices.core.Sound.update_device_info(
Line 1729: self, devices[hwclass.SOUND])
Line 1730: vmdevices.core.Video.update_device_info(
Line 1731: self, devices[hwclass.VIDEO])


-- 
To view, visit https://gerrit.ovirt.org/53677
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib6b372f86b82da7422727f3d084b9afc5505a289
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
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: serialize destroy() and creation

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: vm: serialize destroy() and creation
..


Patch Set 3: Code-Review+1

-- 
To view, visit https://gerrit.ovirt.org/55150
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I234922e46791f5baf46fcb5c790a9d386ac92371
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
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: safer early return if destroyed on startup

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: vm: safer early return if destroyed on startup
..


Patch Set 3: Code-Review+1

-- 
To view, visit https://gerrit.ovirt.org/55151
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I618eb03783d7059ae33d7b0a02542b99c8d8199b
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: virt: Move Vm._getUnderlyingDriveInfo() out of Vm

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: virt: Move Vm._getUnderlyingDriveInfo() out of Vm
..


Patch Set 13: Verified+1

-- 
To view, visit https://gerrit.ovirt.org/53677
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib6b372f86b82da7422727f3d084b9afc5505a289
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: virt: Move Vm._getUnderlyingDriveInfo() out of Vm

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: virt: Move Vm._getUnderlyingDriveInfo() out of Vm
..


Patch Set 11:

(3 comments)

https://gerrit.ovirt.org/#/c/53677/11/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 1723: devices = self._devices
Line 1724: vmdevices.network.Interface.update_device_info(
Line 1725: self, devices[hwclass.NIC])
Line 1726: vmdevices.storage.Drive.update_device_info(
Line 1727: self, devices[hwclass.DISK])
> I don't like these class methods, but it seems to be too late now after you
Why don't you like them? Because they are class methods or for another reason? 
We can make changes in followup patches.
Line 1728: vmdevices.core.Sound.update_device_info(
Line 1729: self, devices[hwclass.SOUND])
Line 1730: vmdevices.core.Video.update_device_info(
Line 1731: self, devices[hwclass.VIDEO])


https://gerrit.ovirt.org/#/c/53677/11/vdsm/virt/vmdevices/storage.py
File vdsm/virt/vmdevices/storage.py:

Line 443: dom = ET.fromstring(xml_string)
Line 444: return bool(dom.findall(self._xpath))
Line 445: 
Line 446: @classmethod
Line 447: def _getDriveIdentification(cls, dom):
> I don't see any use of the cls argument - why is this a classmethod?
Done
Line 448: sources = dom.getElementsByTagName('source')
Line 449: if sources:
Line 450: devPath = (sources[0].getAttribute('file') or
Line 451:sources[0].getAttribute('dev') or


Line 457: alias = 
dom.getElementsByTagName('alias')[0].getAttribute('name')
Line 458: return alias, devPath, name
Line 459: 
Line 460: @classmethod
Line 461: def update_device_info(cls, vm, device_conf):
> Same, I don't see any use of this class, so it should be a function in this
Each device type has its own update_device_info method, so it should be in the 
corresponding class. We can argue whether it should be classmethod or 
staticmethod, but conceptually it's not a separate function.
Line 462: # FIXME!  We need to gather as much info as possible from the 
libvirt.
Line 463: # In the future we can return this real data to management 
instead of
Line 464: # vm's conf
Line 465: for x in vm.domain.get_device_elements('disk'):


-- 
To view, visit https://gerrit.ovirt.org/53677
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib6b372f86b82da7422727f3d084b9afc5505a289
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
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: use proper threading.Event()s

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: vm: use proper threading.Event()s
..


Patch Set 3: Code-Review+1

(Assuming you fix the commit message.)

-- 
To view, visit https://gerrit.ovirt.org/54792
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1b4e0c266e327306d9712b00dbf028f3bb096426
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
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: improve safety between startup and shutdown

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: vm: improve safety between startup and shutdown
..


Patch Set 8:

(3 comments)

I can't say there is something clearly wrong with the change, but it looks a 
bit suspicious to me. While it probably shouldn't cause regressions, I'm not 
sure it handles the problem completely. Maybe just some source comment is 
missing, maybe something should be done a bit differently. I don't know now.

https://gerrit.ovirt.org/#/c/44989/8/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 251: vmdevices.graphics.initLegacyConf(self.conf)
Line 252: self.cif = cif
Line 253: self.log = SimpleLogAdapter(self.log, {"vmId": 
self.conf['vmId']})
Line 254: self._destroyed = threading.Event()
Line 255: self._destroyRequested = threading.Event()
We shouldn't use camel case for attribute names, right?
Line 256: self._recovery_file = recovery.File(self.conf['vmId'])
Line 257: self._monitorResponse = 0
Line 258: self.memCommitted = 0
Line 259: self._consoleDisconnectAction = 
ConsoleDisconnectAction.LOCK_SCREEN


Line 1306: try:
Line 1307: self.guestAgent.stop()
Line 1308: except Exception:
Line 1309: pass
Line 1310: self._created.set()
Setting this at two quite different places worries me a bit. E.g. didn't we 
forget about another place?
Line 1311: self.saveState()
Line 1312: if event_data:
Line 1313: self.send_status_event(**event_data)
Line 1314: 


Line 1765: self._created.set()
Line 1766: 
Line 1767: def _domDependentInit(self):
Line 1768: if self._destroyRequested.is_set():
Line 1769: raise 
DestroyedOnStartupError(vmexitreason.DESTROYED_ON_STARTUP)
What happens if destroy gets called after this check? Is there anything 
preventing it? Or is it harmless?
Line 1770: 
Line 1771: if not self._dom.connected:
Line 1772: raise 
MissingLibvirtDomainError(vmexitreason.LIBVIRT_START_FAILED)
Line 1773: 


-- 
To view, visit https://gerrit.ovirt.org/44989
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8718f58f1d255d9e603db75aa1f256c03c300f3a
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Arik Hadas 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Michal Skrivanek 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Vinzenz Feenstra 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: virt: Move Vm._getUnderlyingHostDeviceInfo() out of Vm

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: virt: Move Vm._getUnderlyingHostDeviceInfo() out of Vm
..


Patch Set 12:

Some test code added to better cover the moved code.

-- 
To view, visit https://gerrit.ovirt.org/53620
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I789d7d2349fc24b48e52260de98fb3d66b168bf7
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
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: improve safety between startup and shutdown

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: vm: improve safety between startup and shutdown
..


Patch Set 8:

(2 comments)

https://gerrit.ovirt.org/#/c/44989/8/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 251: vmdevices.graphics.initLegacyConf(self.conf)
Line 252: self.cif = cif
Line 253: self.log = SimpleLogAdapter(self.log, {"vmId": 
self.conf['vmId']})
Line 254: self._destroyed = threading.Event()
Line 255: self._destroyRequested = threading.Event()
> though call. On one hand, we should move (as fast as possible) towards pep8
OK.
Line 256: self._recovery_file = recovery.File(self.conf['vmId'])
Line 257: self._monitorResponse = 0
Line 258: self.memCommitted = 0
Line 259: self._consoleDisconnectAction = 
ConsoleDisconnectAction.LOCK_SCREEN


Line 1765: self._created.set()
Line 1766: 
Line 1767: def _domDependentInit(self):
Line 1768: if self._destroyRequested.is_set():
Line 1769: raise 
DestroyedOnStartupError(vmexitreason.DESTROYED_ON_STARTUP)
> The key change here is the introduction of the "created" event.
I see, I got confused by something, perhaps thinking about too many things at 
once. Thanks for explanation, it's fine for me.
Line 1770: 
Line 1771: if not self._dom.connected:
Line 1772: raise 
MissingLibvirtDomainError(vmexitreason.LIBVIRT_START_FAILED)
Line 1773: 


-- 
To view, visit https://gerrit.ovirt.org/44989
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8718f58f1d255d9e603db75aa1f256c03c300f3a
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Arik Hadas 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Michal Skrivanek 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Vinzenz Feenstra 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: virt: Add `vm' argument to underlying_device_info methods

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: virt: Add `vm' argument to underlying_device_info methods
..


Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/53676/1//COMMIT_MSG
Commit Message:

Line 11: instead of adding more other arguments.  Perhaps we could remove some 
of
Line 12: the other underlying_device_info arguments when we've got `vm'.  
Perhaps
Line 13: we should take a different approach.  It's for discussion, but for now
Line 14: we take this path to make storage device handling movable out of Vm
Line 15: class.
> In general I don't think it is a good idea.
Thinking about it a bit more I don't think there is anything fundamentally 
wrong in passing `vm' argument here with its current public interfaces. But we 
need to keep device_conf argument instead of exposing the devices configuration.

I think this way is fine here. We may try some redesign in future, but we 
shouldn't try to do that here. Moreover we might want to postpone the activity 
until we move to etree as that can imply some improvements to domain XML 
handling etc.

So I'm still going to delete this patch.
Line 16: 
Line 17: Change-Id: Ide9e8d105d6c14101f9ce367409af729a075b2df


-- 
To view, visit https://gerrit.ovirt.org/53676
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ide9e8d105d6c14101f9ce367409af729a075b2df
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: virt: clientIF: extract vmContainer into a module

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: virt: clientIF: extract vmContainer into a module
..


Patch Set 6: Code-Review-1

(1 comment)

https://gerrit.ovirt.org/#/c/53101/6/lib/vdsm/virt/vmdict.py
File lib/vdsm/virt/vmdict.py:

Line 84: with _lock:
Line 85: if vm.id in _vms:
Line 86: del _vms[vm.id]
Line 87: else:
Line 88: raise UnknownVM()
> You have a point here, but we this 'dict' thingy has to be a singleton, and
I agree with Martin, (re)implementing dictionary this way is not a good idea. 
And I don't agree this is the most pythonic way to define a singleton. Martin's 
suggestion to make the dictionary class private and export just its (single) 
instance created here is the simplest way to get the singleton. Using 
metaclasses or decorators/wrappers are other options, depending on 
circumstances.
Line 89: 
Line 90: 
Line 91: @utils.traceback()
Line 92: def dispatch_eio_cont(libvirt_vms, sdUUID):


-- 
To view, visit https://gerrit.ovirt.org/53101
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iacd2ae6c5e9ca6a73c0fed978c78c9ebb001c46d
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: virt: New method Vm.get_devices

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: virt: New method Vm.get_devices
..


Patch Set 1: Code-Review-1

(1 comment)

https://gerrit.ovirt.org/#/c/54090/1/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 5070: def getNicDevices(self):
Line 5071: return self._devices[hwclass.NIC]
Line 5072: 
Line 5073: def get_devices(self, hw_class):
Line 5074: return self._devices[hw_class]
> fine as long as we make sure we pass a copy of the data, to skip future hea
Hm, good note. We need to modify the returned object in update_*_info methods, 
so copying doesn't work here. Perhaps we shouldn't introduce this method and we 
should explicitly pass the device object to update_*_info methods instead.
Line 5075: 
Line 5076: def getBalloonDevicesConf(self):
Line 5077: for dev in self.conf['devices']:
Line 5078: if dev['type'] == hwclass.BALLOON:


-- 
To view, visit https://gerrit.ovirt.org/54090
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6a7c32806501df8672718c588b2f62087ff4e085
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: debian: add supervdsm_api

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: debian: add supervdsm_api
..


Patch Set 1: Code-Review+1

-- 
To view, visit https://gerrit.ovirt.org/54466
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2779caab6a4ec877faf9d0ba5141ca91cfadffb3
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: virt: Move Vm._getUnderlyingDriveInfo() out of Vm

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: virt: Move Vm._getUnderlyingDriveInfo() out of Vm
..


Patch Set 7:

(1 comment)

https://gerrit.ovirt.org/#/c/53677/7/tests/devices/data/testComplexVm.xml
File tests/devices/data/testComplexVm.xml:

Line 142:   /dev/random
Line 143:   
Line 144:   
Line 145: 
Line 146: 
> Could you please remind me why this is needed?
This piece of XML is needed to get the moved code at least partially covered by 
the tests. If it is not present the top loop in Drive.update_device_info just 
skips.
Line 147:   
Line 148:   
Line 149:   
Line 150:   


-- 
To view, visit https://gerrit.ovirt.org/53677
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib6b372f86b82da7422727f3d084b9afc5505a289
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: virt: Don't use Vm device configuration in clientIF

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: virt: Don't use Vm device configuration in clientIF
..


Patch Set 7: Verified+1

Verified by running a VM from Engine, migrating it to another host and back and 
shutting it down.

-- 
To view, visit https://gerrit.ovirt.org/53482
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idd0f7c99b4df18d27dfad36bf2275540bc8b958c
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: virt: New method Vm.get_devices

[mzamazal]

Milan Zamazal has abandoned this change.

Change subject: virt: New method Vm.get_devices
..


Abandoned

Not a good idea, discarded.

-- 
To view, visit https://gerrit.ovirt.org/54090
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: abandon
Gerrit-Change-Id: I6a7c32806501df8672718c588b2f62087ff4e085
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: virt: Move Vm._getUnderlyingDeviceAddress() to vmxml.py

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: virt: Move Vm._getUnderlyingDeviceAddress() to vmxml.py
..


Patch Set 5:

(1 comment)

https://gerrit.ovirt.org/#/c/53615/5/vdsm/virt/vmxml.py
File vdsm/virt/vmxml.py:

Line 62: alias = aliasElement[0].getAttribute('name')
Line 63: yield deviceXML, alias
Line 64: 
Line 65: 
Line 66: def device_address(devXml, index=0):
> minor: let's reformat in pep8 style. It is fine for me if you do now or int
> minor: let's reformat in pep8 style.

Done in a followup patch.

> also, let's make sure this code is covered by tests

It is covered.
Line 67: """
Line 68: Obtain device's address from libvirt
Line 69: """
Line 70: address = {}


-- 
To view, visit https://gerrit.ovirt.org/53615
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9868ca1df6650b1262e6f30d639a08b1f38304d
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: virt: Move Vm._getUnderlyingUnknownDeviceInfo() out of Vm

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: virt: Move Vm._getUnderlyingUnknownDeviceInfo() out of Vm
..


Patch Set 7:

> if you add a new module you need to update also the Makefile.am, vdsm.spec.in 
> and the corresponding debian data

Done.

-- 
To view, visit https://gerrit.ovirt.org/53678
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I50d160317f91db92bb6a8ae22a45d9574d1434b3
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: virt: Move Vm._getUnderlyingGraphicsDeviceInfo() out of Vm

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: virt: Move Vm._getUnderlyingGraphicsDeviceInfo() out of Vm
..


Patch Set 8:

(1 comment)

https://gerrit.ovirt.org/#/c/53619/8/vdsm/virt/vmdevices/graphics.py
File vdsm/virt/vmdevices/graphics.py:

Line 173: if tlsPort:
Line 174: dev['tlsPort'] = tlsPort
Line 175: break
Line 176: 
Line 177: # the first graphic device is duplicated in the legacy conf
> let's add (maybe in a future patch) a comment to remind us that this should
IMO the comment is not worth a separate patch, added here.
Line 178: updateLegacyConf({'devices': vm.conf['devices']})
Line 179: 
Line 180: 
Line 181: def isSupportedDisplayType(vmParams):


-- 
To view, visit https://gerrit.ovirt.org/53619
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1aff4f57905d0f2d1ee60a2ed963c843feee4540
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: virt: Use PEP8 identifiers in vmxml.device_address

[mzamazal]

Milan Zamazal has uploaded a new change for review.

Change subject: virt: Use PEP8 identifiers in vmxml.device_address
..

virt: Use PEP8 identifiers in vmxml.device_address

This is a trivial change just renaming two local variables.

Change-Id: I75ec0bcef132bd8b5bd40bb5f4859578014e2728
Signed-off-by: Milan Zamazal 
---
M vdsm/virt/vmxml.py
1 file changed, 4 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/11/54511/1

diff --git a/vdsm/virt/vmxml.py b/vdsm/virt/vmxml.py
index 8e65d3a..8f2a812 100644
--- a/vdsm/virt/vmxml.py
+++ b/vdsm/virt/vmxml.py
@@ -63,19 +63,19 @@
 yield deviceXML, alias
 
 
-def device_address(devXml, index=0):
+def device_address(dev_xml, index=0):
 """
 Obtain device's address from libvirt
 """
 address = {}
-adrXml = devXml.getElementsByTagName('address')[index]
+adr_xml = dev_xml.getElementsByTagName('address')[index]
 # Parse address to create proper dictionary.
 # Libvirt device's address definition is:
 # PCI = {'type':'pci', 'domain':'0x', 'bus':'0x00',
 #'slot':'0x0c', 'function':'0x0'}
 # IDE = {'type':'drive', 'controller':'0', 'bus':'0', 'unit':'0'}
-for key in adrXml.attributes.keys():
-address[key.strip()] = adrXml.getAttribute(key).strip()
+for key in adr_xml.attributes.keys():
+address[key.strip()] = adr_xml.getAttribute(key).strip()
 
 return address
 


-- 
To view, visit https://gerrit.ovirt.org/54511
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I75ec0bcef132bd8b5bd40bb5f4859578014e2728
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: virt: Make Vm.devMapFromDevSpecMap() private

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: virt: Make Vm.devMapFromDevSpecMap() private
..


Patch Set 7: Verified+1

Verified by running a VM from Engine, migrating it to another host and back and 
shutting it down.

-- 
To view, visit https://gerrit.ovirt.org/53484
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3a1d8d8d80313a8b4648a255413c26689d1c4657
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: virt: Make Vm.devSpecMapFromConf() private

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: virt: Make Vm.devSpecMapFromConf() private
..


Patch Set 7: Verified+1

Verified by running a VM from Engine, migrating it to another host and back and 
shutting it down.

-- 
To view, visit https://gerrit.ovirt.org/53483
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6dffb82bde715453ff4ae3eae3b82b4890f04f8b
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: virt: Move Vm._getUnderlyingUnknownDeviceInfo() out of Vm

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: virt: Move Vm._getUnderlyingUnknownDeviceInfo() out of Vm
..


Patch Set 7:

(1 comment)

https://gerrit.ovirt.org/#/c/53678/7/vdsm/virt/vmdevices/common.py
File vdsm/virt/vmdevices/common.py:

Line 1: #
> let me think a bit more about this name. Since I don't have better suggesti
I'm fine with the name as it is. Maybe Martin can come with a better idea?
Line 2: # Copyright 2008-2016 Red Hat, Inc.
Line 3: #
Line 4: # This program is free software; you can redistribute it and/or modify
Line 5: # it under the terms of the GNU General Public License as published by


-- 
To view, visit https://gerrit.ovirt.org/53678
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I50d160317f91db92bb6a8ae22a45d9574d1434b3
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: fix handling virt-v2v fail on stream close

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: v2v: fix handling virt-v2v fail on stream close
..


Patch Set 2: Code-Review+1

(2 comments)

OK, I just suggest improving the commit message.

https://gerrit.ovirt.org/#/c/55477/2//COMMIT_MSG
Commit Message:

Line 3: AuthorDate: 2016-03-28 15:38:16 +0300
Line 4: Commit: Shahar Havivi 
Line 5: CommitDate: 2016-03-30 15:53:58 +0300
Line 6: 
Line 7: v2v: fix handling virt-v2v fail on stream close
Handle closed stream when virt-v2v fails (?)
Line 8: 
Line 9: when proc.stdout.read() is close it doesn't raise an error but returns
Line 10: ''.
Line 11: 


PS2, Line 9: close
... closed ...


-- 
To view, visit https://gerrit.ovirt.org/55477
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia166c1aa03a8d62168034cd581be80ef5a3dc69e
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[ovirt-3.6]: virt: clean and modernize the destroy() path

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: virt: clean and modernize the destroy() path
..


Patch Set 1:

(2 comments)

https://gerrit.ovirt.org/#/c/55534/1/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 3853:   self.conf['vmId'])
Line 3854: else:
Line 3855: self.log.warning(
Line 3856: "Failed to destroy VM '%s' gracefully 
(error=%i)",
Line 3857: self.id, e.get_error_code())
Is it better to log e.get_error_code() or e.get_error_message()?
Line 3858: if e.get_error_code() == 
libvirt.VIR_ERR_OPERATION_FAILED:
Line 3859: return self._destroyVmForceful()
Line 3860: return response.error('destroyErr')
Line 3861: return response.success()


Line 3865: self._dom.destroy()
Line 3866: except libvirt.libvirtError as e:
Line 3867: self.log.warning(
Line 3868: "Failed to destroy VM '%s' forcefully (error=%i)",
Line 3869: self.id, e.get_error_code())
Ditto.
Line 3870: return response.error('destroyErr')
Line 3871: return response.success()
Line 3872: 
Line 3873: def deleteVm(self):


-- 
To view, visit https://gerrit.ovirt.org/55534
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I46296fe7ac6c13e064014298148f518ea6b1e1d8
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.6
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[ovirt-3.6]: virt: clean and modernize the destroy() path

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: virt: clean and modernize the destroy() path
..


Patch Set 1: Code-Review+1

Oh, it's a backport.

-- 
To view, visit https://gerrit.ovirt.org/55534
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I46296fe7ac6c13e064014298148f518ea6b1e1d8
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.6
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: vdsm.spec: Require new libvirt on RHEL

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: vdsm.spec: Require new libvirt on RHEL
..


Patch Set 2: Verified+1

Verified that the rpm package installs on current CentOS and a VM with 
non-ASCII characters starts.

-- 
To view, visit https://gerrit.ovirt.org/54547
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ea25acd1bec9ce7f6f26225f0917d5e38f65508
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[ovirt-3.6]: spec: bump libvirt requirement

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: spec: bump libvirt requirement
..


Patch Set 1: Code-Review+1

-- 
To view, visit https://gerrit.ovirt.org/55570
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7c231a33161d91470e98165311f408cf6fa41d38
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.6
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: vdsm.spec: Require new libvirt on RHEL

[mzamazal]

Milan Zamazal has abandoned this change.

Change subject: vdsm.spec: Require new libvirt on RHEL
..


Abandoned

Duplicate of http://gerrit.ovirt.org/54796

-- 
To view, visit https://gerrit.ovirt.org/54547
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: abandon
Gerrit-Change-Id: I2ea25acd1bec9ce7f6f26225f0917d5e38f65508
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: tests: add tests for sampling.VMBulkSampler

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: tests: add tests for sampling.VMBulkSampler
..


Patch Set 48:

(1 comment)

https://gerrit.ovirt.org/#/c/40053/48/tests/virt/bulk_sampling_test.py
File tests/virt/bulk_sampling_test.py:

PS48, Line 82: when=0, duration=10
> They are not arbitrary indeed: for most of the test I need to start with an
Please add the explanation. I wonder whether we really need such a long 
duration -- even when the tests are marked as slow, they shouldn't be 
unnecessarily slow.


-- 
To view, visit https://gerrit.ovirt.org/40053
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id66dbd420ca29d08ae4063dc83b858be34b8940f
Gerrit-PatchSet: 48
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: hostdev: add is_assignable flag

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: hostdev: add is_assignable flag
..


Patch Set 3:

(1 comment)

I can't judge the technical side, but codewise fine, except for wondering about 
one style issue.

https://gerrit.ovirt.org/#/c/56291/4/lib/vdsm/hostdev.py
File lib/vdsm/hostdev.py:

Line 152: if name != 'computer':
Line 153: params['parent'] = devXML.find('parent').text
Line 154: 
Line 155: params['is_assignable'] = str(_pci_header_type(name) == 0).lower()
Line 156: 
I'd prefer if...else for clarity. Also, must params values be strings, couldn't 
we just set the boolean value here without converting it to the string and back?
Line 157: try:
Line 158: driver_name = devXML.find('./driver/name').text
Line 159: except AttributeError:
Line 160: # No driver exposed by libvirt/sysfs.


-- 
To view, visit https://gerrit.ovirt.org/56291
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6fd0d39e777c6bc0f3175b737ace9be92df4cca0
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Betak 
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: virt: Don't set connected attribute in if already...

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: virt: Don't set connected attribute in  if already set
..


Patch Set 4: Verified+1

Verified by installing the hook as described in the referenced bug and checking 
that I can open two remote viewers from the engine.

-- 
To view, visit https://gerrit.ovirt.org/56224
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7c49b26675f06a4914e97d1d85f2355ee6a083c
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Vinzenz Feenstra 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: migrations: change convergence schedule from time to iterations

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: migrations: change convergence schedule from time to iterations
..


Patch Set 3: Code-Review-1

(6 comments)

The code seems to be OK to me. But please fix errors and confusions in the 
commit message -- it's already difficult to understand the concept itself and 
it's even more difficult to understand it with the errors.

https://gerrit.ovirt.org/#/c/56558/3//COMMIT_MSG
Commit Message:

PS3, Line 12: than
... then ...


Line 18: In the first iteration there can be no stalling, qemu only copies 
memory and
Line 19: does not look at how much of it changed.
Line 20: 
Line 21: After this period of time it checks if it can move the rest of the VMs 
memory
Line 22: when pauses it for the dowtime. If not, it starts a new iteration. 
This new
when *it* pauses it for the dow*n*time. ... ?
Line 23: iteration starts copying all the dirtyed memory which can be a lot. 
And the
Line 24: whole second iteration the lowmark of the memory will be lower than 
the current
Line 25: remaining thus being threated as stalling. But this does not actually 
mean we
Line 26: want to enlarge the downtime during the iteration since we don't know 
if qemu


PS3, Line 23: dirtyed
... dirtied ...


PS3, Line 23: And the
: whole second iteration the lowmark of the memory will be lower 
than the current
: remaining
I can't parse this sentence.


PS3, Line 25: threated
... treated ... ?


PS3, Line 42: migration either
: progressing inside one iteration or the next iteration but it 
was so fast
: that it went under the remembered value.
Difficult to understand. How about: ... migration is either progressing within 
the same iteration, or it is already in the next iteration but the progress was 
fast enough to get below the remembered value.


-- 
To view, visit https://gerrit.ovirt.org/56558
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6f87c954031842c35c99888c228a34ec7f19d800
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Jelinek 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Tomas Jelinek 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: hostdev: report additional information in 'scsi' device

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: hostdev: report additional information in 'scsi' device
..


Patch Set 7: Code-Review-1

(1 comment)

https://gerrit.ovirt.org/#/c/56038/7/lib/vdsm/hostdev.py
File lib/vdsm/hostdev.py:

Line 204: device_params = matching_devices.values()[0]
Line 205: except IndexError:
Line 206: raise
Line 207: else:
Line 208: return device_params
Do I overlook something or could this function be simplified as

  for device in list_by_caps(device_cap).values():
  if is_parent(device, parent_name):
  return device
  else:
  raise IndexError

Either it can or the tests should be improved as they pass with my version. :-)
Line 209: 
Line 210: params = {}
Line 211: 
Line 212: scsi_name = device_xml.find('name').text


-- 
To view, visit https://gerrit.ovirt.org/56038
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I789f49c9abca2dff1487acf3e8a04ae1407956f4
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: virt: Don't fail when existingConnAction is unset for a SPIC...

[mzamazal]

Milan Zamazal has uploaded a new change for review.

Change subject: virt: Don't fail when existingConnAction is unset for a SPICE 
device
..

virt: Don't fail when existingConnAction is unset for a SPICE device

When a ticket for a SPICE device is set, existingConnAction parameter is
sent by Engine as well.  This parameter is used to set `connected'
attribute of  element.  But setting this parameter when
updating the ticket is basically useless.  It may be actually even
harmful, e.g. when a user sets `connected' attribute value to `keep' in
a Vdsm hook and Engine overrides it to `disconnect' without any good
reason.  See the referenced bug.

This should be fixed on the Engine side.  One approach may be not to
send the parameter at all when updating the SPICE ticket.  However, Vdsm
crashes in such a case.  This patch fixes Vdsm problems when the
parameter is not present, making future changes in Engine possible.

Change-Id: I2fde83ceb994eaafd2aa956d1fa82ce6cb16094c
Bug-Url: https://bugzilla.redhat.com/1060573
Signed-off-by: Milan Zamazal 
---
M tests/vmOperationsTests.py
M vdsm/virt/vm.py
2 files changed, 25 insertions(+), 12 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/36/56836/1

diff --git a/tests/vmOperationsTests.py b/tests/vmOperationsTests.py
index 7c65428..82ad7b0 100644
--- a/tests/vmOperationsTests.py
+++ b/tests/vmOperationsTests.py
@@ -129,17 +129,26 @@
 self.assertEqual(testvm.getStats()['timeOffset'],
  str(self.BASE_OFFSET + self.UPDATE_OFFSETS[-1]))
 
-def testUpdateSingleDeviceGraphics(self):
+@permutations([['disconnect'], [None]])
+def testUpdateSingleDeviceGraphics(self, connected):
+if connected:
+graphics_params = _GRAPHICS_DEVICE_PARAMS
+connected_attribute = ' connected="disconnect"'
+else:
+graphics_params = {k: v for k, v in _GRAPHICS_DEVICE_PARAMS.items()
+   if k != 'existingConnAction'}
+connected_attribute = ''
 devXmls = (
-'',
+'' % (connected_attribute,),
 '')
 for device, devXml in zip(self.GRAPHIC_DEVICES, devXmls):
 domXml = '''
 
 
 ''' % device['device']
-self._verifyDeviceUpdate(device, device, domXml, devXml)
+self._verifyDeviceUpdate(device, device, domXml, devXml,
+ graphics_params)
 
 def testUpdateMultipleDeviceGraphics(self):
 devXmls = (
@@ -153,23 +162,26 @@
 '''
 for device, devXml in zip(self.GRAPHIC_DEVICES, devXmls):
 self._verifyDeviceUpdate(
-device, self.GRAPHIC_DEVICES, domXml, devXml)
+device, self.GRAPHIC_DEVICES, domXml, devXml,
+_GRAPHICS_DEVICE_PARAMS)
 
-def _updateGraphicsDevice(self, testvm, device_type):
+def _updateGraphicsDevice(self, testvm, device_type, graphics_params):
 def _check_ticket_params(domXML, conf, params):
 self.assertEqual(params, _TICKET_PARAMS)
 
 with MonkeyPatchScope([(hooks, 'before_vm_set_ticket',
 _check_ticket_params)]):
 params = {'graphicsType': device_type}
-params.update(_GRAPHICS_DEVICE_PARAMS)
+params.update(graphics_params)
 return testvm.updateDevice(params)
 
-def _verifyDeviceUpdate(self, device, allDevices, domXml, devXml):
+def _verifyDeviceUpdate(self, device, allDevices, domXml, devXml,
+graphics_params):
 with fake.VM(devices=allDevices) as testvm:
 testvm._dom = fake.Domain(domXml)
 
-self._updateGraphicsDevice(testvm, device['device'])
+self._updateGraphicsDevice(testvm, device['device'],
+   graphics_params)
 
 self.assertEquals(testvm._dom.devXml, devXml)
 
@@ -312,7 +324,8 @@
 domain.updateDeviceFlags = _fail
 testvm._dom = domain
 
-res = self._updateGraphicsDevice(testvm, device)
+res = self._updateGraphicsDevice(testvm, device,
+ _GRAPHICS_DEVICE_PARAMS)
 
 self.assertEqual(res,
  response.error('ticketErr', message))
diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py
index 8ac6242..1148d8e 100644
--- a/vdsm/virt/vm.py
+++ b/vdsm/virt/vm.py
@@ -2302,7 +2302,7 @@
 if graphics:
 result = self._setTicketForGraphicDev(
 graphics, params['password'], params['ttl'],
-params['existingConnAction'], params['params'])
+params.get('existingConnAction'), params['params'])
 

Change in vdsm[master]: virt: Don't set connected attribute in if already...

[mzamazal]

Milan Zamazal has abandoned this change.

Change subject: virt: Don't set connected attribute in  if already set
..


Abandoned

Abandoned in favor of https://gerrit.ovirt.org/56836 + some Engine side 
solution.

-- 
To view, visit https://gerrit.ovirt.org/56224
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: abandon
Gerrit-Change-Id: Ie7c49b26675f06a4914e97d1d85f2355ee6a083c
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal 
Gerrit-Reviewer: Arik Hadas 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Vinzenz Feenstra 
Gerrit-Reviewer: gerrit-hooks 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: migrations: change convergence schedule from time to iterations

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: migrations: change convergence schedule from time to iterations
..


Patch Set 4: -Code-Review

Thanks, the commit message is much better now.

I'm still a bit worried about a scenario where each iteration makes a small 
progress. In such a, perhaps rare, situation we may miss many or all iterations 
and can remain waiting for a long time. Do we mind or do I miss something?

-- 
To view, visit https://gerrit.ovirt.org/56558
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6f87c954031842c35c99888c228a34ec7f19d800
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Jelinek 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Tomas Jelinek 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: migrations: enhance legacy downtime alg

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: migrations: enhance legacy downtime alg
..


Patch Set 3:

(6 comments)

https://gerrit.ovirt.org/#/c/56561/3//COMMIT_MSG
Commit Message:

PS3, Line 12: during first
... during the first ...


PS3, Line 15: change the way and params the wait for next step is calculated so
I can't parse this sentence.


https://gerrit.ovirt.org/#/c/56561/3/lib/vdsm/config.py.in
File lib/vdsm/config.py.in:

Line 112: 
Line 113: ('migration_downtime_delay', '100',
Line 114: 'This value is used on the source host to define the 
delay before '
Line 115: 'setting/increasing the downtime of a migration. '
Line 116: 'The value is per GiB of RAM. A minimum of twice this 
value is '
When we are on it, we could probably add to the description the time unit of 
this value (seconds?).
Line 117: 'used for VMs with less than 2 GiB of RAM'),
Line 118: 
Line 119: ('migration_downtime_steps', '5',
Line 120: 'Incremental steps used to reach migration_downtime.'),


Line 113: ('migration_downtime_delay', '100',
Line 114: 'This value is used on the source host to define the 
delay before '
Line 115: 'setting/increasing the downtime of a migration. '
Line 116: 'The value is per GiB of RAM. A minimum of twice this 
value is '
Line 117: 'used for VMs with less than 2 GiB of RAM'),
Really??
Line 118: 
Line 119: ('migration_downtime_steps', '5',
Line 120: 'Incremental steps used to reach migration_downtime.'),
Line 121: 


https://gerrit.ovirt.org/#/c/56561/3/vdsm/virt/migration.py
File vdsm/virt/migration.py:

Line 501: 
Line 502: delay_per_gib = config.getint('vars', 
'migration_downtime_delay')
Line 503: memSize = int(vm.conf['memSize'])
Line 504: self._wait = min(delay_per_gib *
Line 505:  (memSize + 1023) / 1024 / self._steps, 60)
I'm confused about migration_downtime_delay units and the whole formula here. 
I'm having trouble to match the numbers with the commit message. We reach the 
60 seconds limit here much sooner than on 2 GB with delay_per_gib == 100, don't 
we? And don't we want to divide 60 by self._steps as well?
Line 506: if self._steps > 1:
Line 507: self._exponentialDowntimes = \
Line 508: list(exponential_downtime(downtime, self._steps))
Line 509: else:


Line 503: memSize = int(vm.conf['memSize'])
Line 504: self._wait = min(delay_per_gib *
Line 505:  (memSize + 1023) / 1024 / self._steps, 60)
Line 506: if self._steps > 1:
Line 507: self._exponentialDowntimes = \
Please don't use camelCase for attribute names, use underscores.
Line 508: list(exponential_downtime(downtime, self._steps))
Line 509: else:
Line 510: self._exponentialDowntimes = [downtime]
Line 511: 


-- 
To view, visit https://gerrit.ovirt.org/56561
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5d18a77c91a1344110afef1577e310faab3ffe5b
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Jelinek 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Tomas Jelinek 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: virt: graphics: enforce spice default mode

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: virt: graphics: enforce spice default mode
..


Patch Set 2: Code-Review+1

-- 
To view, visit https://gerrit.ovirt.org/56746
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I169e7c4a76717dda8aeacbdb20ee031f453ed4fa
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: Detect VM with snapshots

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: v2v: Detect VM with snapshots
..


Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/56574/1/lib/vdsm/v2v.py
File lib/vdsm/v2v.py:

Line 899: 
Line 900: 
Line 901: def _add_has_snapshots(vm, params):
Line 902: try:
Line 903: ret = vm.hasCurrentSnapshot()
> Does this method exist? I can see it only in tests.
Ah, found, it's a libvirt object, sorry.
Line 904: except libvirt.libvirtError:
Line 905: logging.exception("Error checking for existing snapshots")
Line 906: else:
Line 907: params['has_snapshots'] = ret > 0


-- 
To view, visit https://gerrit.ovirt.org/56574
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9aa4de2faff92625cd0de8e3ae2a10a2d58aa823
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Vinzenz Feenstra 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: Detect VM with snapshots

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: v2v: Detect VM with snapshots
..


Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/56574/1/lib/vdsm/v2v.py
File lib/vdsm/v2v.py:

Line 899: 
Line 900: 
Line 901: def _add_has_snapshots(vm, params):
Line 902: try:
Line 903: ret = vm.hasCurrentSnapshot()
Does this method exist? I can see it only in tests.
Line 904: except libvirt.libvirtError:
Line 905: logging.exception("Error checking for existing snapshots")
Line 906: else:
Line 907: params['has_snapshots'] = ret > 0


-- 
To view, visit https://gerrit.ovirt.org/56574
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9aa4de2faff92625cd0de8e3ae2a10a2d58aa823
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Vinzenz Feenstra 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: Detect VM with snapshots

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: v2v: Detect VM with snapshots
..


Patch Set 3: Code-Review+1

-- 
To view, visit https://gerrit.ovirt.org/56574
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9aa4de2faff92625cd0de8e3ae2a10a2d58aa823
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Tomas Golembiovsky 
Gerrit-Reviewer: Vinzenz Feenstra 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: hostdev/sr-iov: use device setup instead of detach

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: hostdev/sr-iov: use device setup instead of detach
..


Patch Set 13: Code-Review-1

(2 comments)

Documentation issues.

https://gerrit.ovirt.org/#/c/55137/13/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2135: dev_object = vmdevices.hostdevice.HostDevice(self.conf, 
self.log,
Line 2136:  **dev_spec)
Line 2137: dev_objects.append(dev_object)
Line 2138: try:
Line 2139: dev_object.setup()
HostDevice.setup docstring should be updated as well.
Line 2140: except libvirt.libvirtError:
Line 2141: # We couldn't detach one of the devices. Halt.
Line 2142: self.log.exception('Could not detach a device from a 
host.')
Line 2143: return response.error('hostdevDetachErr')


https://gerrit.ovirt.org/#/c/55137/13/vdsm/virt/vmdevices/network.py
File vdsm/virt/vmdevices/network.py:

Line 181: def setup(self):
Line 182: """
Line 183: Detach the device from the host. This method *must* be
Line 184: called before getXML in order to populate _deviceParams.
Line 185: """
I find this combination of the method name and the docstring confusing. I 
suggest rewording the docstring to emphasize the logical purpose of the method 
instead of the implementation. It should explain what the method is useful for; 
for instance if its only purpose is to be called before getXML, why isn't it 
called from getXML directly and is exposed as a public interface?
Line 186: if self.is_hostdevice:
Line 187: logging.debug('Detaching device %s from the host.' % 
self.device)
Line 188: detach_detachable(self.hostdev)
Line 189: 


-- 
To view, visit https://gerrit.ovirt.org/55137
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1c97af2ea9f17ef38f9dbb4f41e5f9d1da9eebaa
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: hostdev: use setup instead of detach in hotplug

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: hostdev: use setup instead of detach in hotplug
..


Patch Set 3: Code-Review-1

This is confusing. Why do we call the method HostDevice.setup when its 
docstring speaks about detaching instead?

-- 
To view, visit https://gerrit.ovirt.org/56338
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I96853249e32c7b15c6a1c11b5a1ab385d7c4c827
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: hostdev: report storage model as product

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: hostdev: report storage model as product
..


Patch Set 3: Code-Review+1

Just thinking whether we should check for `product' presence before we override 
it with model, to be future-proof. But if it doesn't make sense for any reason 
then it's fine for me as it is.

-- 
To view, visit https://gerrit.ovirt.org/56123
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8258988363faa00a9a32aa637dbed06ab82da55f
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: hostdev: use libvirt flags to select capability

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: hostdev: use libvirt flags to select capability
..


Patch Set 4:

(2 comments)

https://gerrit.ovirt.org/#/c/56037/4//COMMIT_MSG
Commit Message:

PS4, Line 10: capability
... capabilities ...


https://gerrit.ovirt.org/#/c/56037/4/lib/vdsm/hostdev.py
File lib/vdsm/hostdev.py:

Line 213: """
Line 214: devices = {}
Line 215: flags = 0
Line 216: if caps:
Line 217: flags |= sum([_LIBVIRT_DEVICE_FLAGS[cap] for cap in caps])
Simple assignment should be enough here, right?  Or perhaps we can simply use

  flags = sum([_LIBVIRT_DEVICE_FLAGS[cap] for cap in caps or []])

instead of all the three lines?
Line 218: libvirt_devices = _get_devices_from_libvirt(flags)
Line 219: 
Line 220: for devName, params in libvirt_devices.items():
Line 221: devices[devName] = {'params': params}


-- 
To view, visit https://gerrit.ovirt.org/56037
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia40fff8ba0bace1272666825d6c4fce18ac9a1f0
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: hostdev: expose parameters needed to support hotunplug

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: hostdev: expose parameters needed to support hotunplug
..


Patch Set 19: Code-Review+1

(1 comment)

Better now.

> I don't feel like adding xpath test in this patch. It is good idea for future 
> patch, with other devices following the trend.

The trend of adding tests or the trend of postponing addition tests? ;-)

https://gerrit.ovirt.org/#/c/54939/19/vdsm/virt/vmdevices/hostdevice.py
File vdsm/virt/vmdevices/hostdevice.py:

Line 59: return {'base': '0x', 'padding': ''}
Line 60: return {'base': '0x', 'padding': '02'}
Line 61: return {'base': '', 'padding': ''}
Line 62: 
Line 63: address_format_string = '[@{key}=\'{base}{value:{padding}}\']'
Why not using double quotes for the string to avoid the need of backslashes?
Line 64: address = ''.join([address_format_string.format(
Line 65: key=key, value=int(value), **_padding(key)) for
Line 66: key, value in self.hostAddress.items()])
Line 67: 


-- 
To view, visit https://gerrit.ovirt.org/54939
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I09e879051b1d47e48e9ae73c1f7d9bfbea8f1237
Gerrit-PatchSet: 19
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: hostdev: expose generic scsi driver char device in device pa...

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: hostdev: expose generic scsi driver char device in device params
..


Patch Set 7: Code-Review+1

-- 
To view, visit https://gerrit.ovirt.org/55022
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I91e6793e946310b57e3004cbebc286cc6e00a724
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: hostdev: expose parameters needed to support hotunplug

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: hostdev: expose parameters needed to support hotunplug
..


Patch Set 17:

(3 comments)

https://gerrit.ovirt.org/#/c/54939/17/vdsm/virt/vmdevices/hostdevice.py
File vdsm/virt/vmdevices/hostdevice.py:

Line 48: logging.debug('Detaching device %s from the host.' % 
self.device)
Line 49: self._deviceParams = detach_detachable(self.device)
Line 50: 
Line 51: @property
Line 52: def _xpath(self):
Why @property?
Line 53: """
Line 54: Returns xpath to the device in libvirt dom xml
Line 55: The path is relative to the root element
Line 56: """


Line 52: def _xpath(self):
Line 53: """
Line 54: Returns xpath to the device in libvirt dom xml
Line 55: The path is relative to the root element
Line 56: """
Missing dots at the ends of the sentences.
Line 57: def _padding(key):
Line 58: if CAPABILITY_TO_XML_ATTR[
Line 59: self._deviceParams['capability']] == 'pci':
Line 60: if key == 'domain':


Line 66: 
Line 67: return ('./devices/hostdev/source/address{}'.format(
Line 68: ''.join(['[@{key}=\'{base}{value:{padding}}\']'.format(
Line 69: key=key, value=int(value), **_padding(key)) for
Line 70: key, value in self.hostAddress.items()])))
Am I the only one who finds this unreadable?  How about something like

  attributes = ["[@{key}='{base}{value:{padding}}']"
.format(key=key, value=int(value), **_padding(key))
for key, value in self.hostAddress.items()]
  return './devices/hostdev/source/address' + ''.join(attributes)
Line 71: 
Line 72: def is_attached_to(self, xml_string):
Line 73: dom = ET.fromstring(xml_string)
Line 74: return bool(dom.findall(self._xpath))


-- 
To view, visit https://gerrit.ovirt.org/54939
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I09e879051b1d47e48e9ae73c1f7d9bfbea8f1237
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: hostdev: add vdsClient hotunplug command

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: hostdev: add vdsClient hotunplug command
..


Patch Set 18: Code-Review+1

-- 
To view, visit https://gerrit.ovirt.org/54940
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie00e5510b5306d689536b24091411332c8b80e75
Gerrit-PatchSet: 18
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: hostdev/sr-iov: use device setup instead of detach

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: hostdev/sr-iov: use device setup instead of detach
..


Patch Set 14: Code-Review+1

(1 comment)

https://gerrit.ovirt.org/#/c/55137/14/vdsm/virt/vmdevices/hostdevice.py
File vdsm/virt/vmdevices/hostdevice.py:

Line 39: 
Line 40: def setup(self):
Line 41: """
Line 42: Detach the device from the host.
Line 43: """
Hm, I find it still confusing, perhaps it's better to omit the docstring when 
the method is already documented in the superclass.  But it's not a blocker for 
me, maybe maintainers can give us a clue.
Line 44: logging.debug('Detaching device %s from the host.' % 
self.device)
Line 45: self._deviceParams = detach_detachable(self.device)
Line 46: 
Line 47: def getXML(self):


-- 
To view, visit https://gerrit.ovirt.org/55137
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1c97af2ea9f17ef38f9dbb4f41e5f9d1da9eebaa
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: hostdev: expose hotplug via vdsClient

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: hostdev: expose hotplug via vdsClient
..


Patch Set 17: Code-Review+1

-- 
To view, visit https://gerrit.ovirt.org/54938
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I46fb415c39d04b94428703ac19da505a2cf2800d
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: hostdev: expose parameters needed to support hotunplug

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: hostdev: expose parameters needed to support hotunplug
..


Patch Set 18: Code-Review-1

Some cosmetic issues. And how about adding a simple test to check we build the 
xpath expression correctly?

-- 
To view, visit https://gerrit.ovirt.org/54939
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I09e879051b1d47e48e9ae73c1f7d9bfbea8f1237
Gerrit-PatchSet: 18
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: virt: Set timeout on boot menu

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: virt: Set timeout on boot menu
..


Patch Set 4:

After some discussions we decided that we can omit the configuration option 
completely. One must enable the boot menu to be able to see it and then it 
makes sense to provide longer opportunity to catch on the menu than the default 
3 seconds. 10 seconds may be a reasonable value, not too short and not too 
long. There is no urgent need to expand this thing further, e.g. to the engine 
side, so let's leave it that simple.

-- 
To view, visit https://gerrit.ovirt.org/56393
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f3501e8500e366e785f5a8ddfdf78fd34c997a2
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal 
Gerrit-Reviewer: Arik Hadas 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Michal Skrivanek 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: virt: Set timeout on boot menu

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: virt: Set timeout on boot menu
..


Patch Set 5: Verified+1

(1 comment)

https://gerrit.ovirt.org/#/c/56393/4//COMMIT_MSG
Commit Message:

PS4, Line 12: more a
> more
Done


-- 
To view, visit https://gerrit.ovirt.org/56393
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f3501e8500e366e785f5a8ddfdf78fd34c997a2
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal 
Gerrit-Reviewer: Arik Hadas 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Michal Skrivanek 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: small test improvement

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: v2v: small test improvement
..


Patch Set 1: Code-Review+1

-- 
To view, visit https://gerrit.ovirt.org/56694
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I89c46efc9836fe0f0ef680084f1921ef3948055f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Vinzenz Feenstra 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: virt: Set timeout on boot menu

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: virt: Set timeout on boot menu
..


Patch Set 4: Verified+1

Verified by running a VM and checking that the boot menu prompt lasts 
accordingly longer.

-- 
To view, visit https://gerrit.ovirt.org/56393
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f3501e8500e366e785f5a8ddfdf78fd34c997a2
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal 
Gerrit-Reviewer: Arik Hadas 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Michal Skrivanek 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: virt: Set timeout on boot menu

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: virt: Set timeout on boot menu
..


Patch Set 3:

(1 comment)

https://gerrit.ovirt.org/#/c/56393/3/lib/vdsm/config.py.in
File lib/vdsm/config.py.in:

Line 240: 
Line 241: ('boot_menu_timeout', '10',
Line 242: 'Boot menu timeout in seconds. '
Line 243: 'Minimum value is 0, maximum value is 65; if a different 
value '
Line 244: 'is given then it is adjusted to this range.'),
> Vdsm conf is not the place for this, this should be a per-vm settings contr
Well, increased boot menu timeout is a feature that's not strictly needed, but 
it was requested and may be convenient for some users. So we can provide it but 
we probably don't want to pollute the Web user interface with it. We can simply 
hard code the 10 s value but it was suggested in the bug report to provide a 
Vdsm configuration option to be able to override it. Can you suggest a better 
solution for this situation?
Line 245: ]),
Line 246: 
Line 247: # Section: [rpc]
Line 248: ('rpc', [


-- 
To view, visit https://gerrit.ovirt.org/56393
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f3501e8500e366e785f5a8ddfdf78fd34c997a2
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal 
Gerrit-Reviewer: Arik Hadas 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: hostdev: add is_assignable flag

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: hostdev: add is_assignable flag
..


Patch Set 4: Code-Review+1

-- 
To view, visit https://gerrit.ovirt.org/56291
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6fd0d39e777c6bc0f3175b737ace9be92df4cca0
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Betak 
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: hostdev: get is_assignable from libvirt when available

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: hostdev: get is_assignable from libvirt when available
..


Patch Set 2: Code-Review+1

-- 
To view, visit https://gerrit.ovirt.org/56299
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I80560c0202a494afd15795156d618104b644c4de
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Betak 
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: hostdev: use libvirt flags to select capability

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: hostdev: use libvirt flags to select capability
..


Patch Set 5: Code-Review+1

(Although I still insist on that there is a grammar error in the commit 
message.)

-- 
To view, visit https://gerrit.ovirt.org/56037
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia40fff8ba0bace1272666825d6c4fce18ac9a1f0
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: hostdev: expose generic scsi driver char device in device pa...

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: hostdev: expose generic scsi driver char device in device params
..


Patch Set 6:

(1 comment)

https://gerrit.ovirt.org/#/c/55022/6/lib/vdsm/hostdev.py
File lib/vdsm/hostdev.py:

Line 183: except AttributeError:
Line 184: # Is not scsi char device.
Line 185: pass
Line 186: else:
Line 187: params['udev_path'] = udev_path
Does it mean this is *scsi* char device here? Or don't we mind (if this is the 
case then it's not clear from the commit message)?
Line 188: 
Line 189: iommu_group = caps.find('iommuGroup')
Line 190: if iommu_group is not None:
Line 191: params['iommu_group'] = iommu_group.attrib['number']


-- 
To view, visit https://gerrit.ovirt.org/55022
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I91e6793e946310b57e3004cbebc286cc6e00a724
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: hostdev: report additional information in 'scsi' device

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: hostdev: report additional information in 'scsi' device
..


Patch Set 5:

(1 comment)

https://gerrit.ovirt.org/#/c/56038/5/lib/vdsm/hostdev.py
File lib/vdsm/hostdev.py:

Line 159: key: value for key, value in
Line 160: list_by_caps(device_cap).items() if
Line 161: value['params']['parent'] == parent_name}
Line 162: except KeyError:
Line 163: raise
Do we really want to fail here if *any* of the devices returned by 
list_by_caps() doesn't have a parent?
Line 164: 
Line 165: try:
Line 166: device_params = matching_devices.values()[0]
Line 167: except IndexError:


-- 
To view, visit https://gerrit.ovirt.org/56038
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I789f49c9abca2dff1487acf3e8a04ae1407956f4
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: virt: Don't set connected attribute in if already...

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: virt: Don't set connected attribute in  if already set
..


Patch Set 4:

(1 comment)

https://gerrit.ovirt.org/#/c/56224/4//COMMIT_MSG
Commit Message:

Line 19: 
Line 20: In theory, it prevents changing the attribute value from Engine after
Line 21: the VM has been started.  But we don't support such a feature now and
Line 22: we're probably not going to add it in a foreseeable future, so this
Line 23: limitation shouldn't matter.
> This smells like the wrong place to fix. If we want to support shared sessi
Because we officially don't support this feature. Multiple SPICE connections 
may not be reliable and we don't want to support or advertise them. But if 
users want to experiment with them via Vdsm hooks on their own risks, we 
shouldn't block them.
Line 24: 
Line 25: Change-Id: Ie7c49b26675f06a4914e97d1d85f2355ee6a083c
Line 26: Bug-Url: https://bugzilla.redhat.com/1060573


-- 
To view, visit https://gerrit.ovirt.org/56224
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7c49b26675f06a4914e97d1d85f2355ee6a083c
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal 
Gerrit-Reviewer: Arik Hadas 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Vinzenz Feenstra 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: migration: Enable lazy setting of incoming/outgoing limits

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: migration: Enable lazy setting of incoming/outgoing limits
..


Patch Set 31:

(2 comments)

https://gerrit.ovirt.org/#/c/53305/31/lib/api/vdsm-api.yml
File lib/api/vdsm-api.yml:

Line 2913: type: boolean
Line 2914: 
Line 2915: -   defaultvalue: null
Line 2916: description: maximum number of outgoing migrations
Line 2917:  Must be > 0, values <= 0 are silently 
ignored.
Negative values are not silently ignored. But we declare `type: uint' here so 
mentioning just zero value should be enough.
Line 2918: name: outgoingLimit
Line 2919: type: uint
Line 2920: added: '4.0'
Line 2921: 


https://gerrit.ovirt.org/#/c/53305/31/vdsm/API.py
File vdsm/API.py:

Line 585: :type params: dict
Line 586: :param incomingLimit: maximum number of incoming migrations 
to set
Line 587: before the migration is started.
Line 588: Must be > 0, values <= 0 are silently ignored.
Line 589: :type incomingLimit: int
uint, not int
Line 590: """
Line 591: self.log.debug('Migration create')
Line 592: 
Line 593: if incomingLimit:


-- 
To view, visit https://gerrit.ovirt.org/53305
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I79ab97f15788e4024c94d051e4aade713d760acf
Gerrit-PatchSet: 31
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Betak 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Betak 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: tests: Prevent multiple invocations of makecerts.sh

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: tests: Prevent multiple invocations of makecerts.sh
..


Patch Set 1: Code-Review+1

-- 
To view, visit https://gerrit.ovirt.org/57344
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5553057e0c0a66ccb96aa099a6608002f42d513f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Vinzenz Feenstra 
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: devices: fix behaviour with balloon model=none

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: vm: devices: fix behaviour with balloon model=none
..


Patch Set 1:

To clarify: We must be careful about balloon devices because if libvirt doesn't 
find one in the domain XML then it creates one. We must explicitly pass 
model='none' device to disable balloon devices. But in 
Balloon.update_device_info, there is probably nothing useful we can retrieve 
from a none device, so we can skip it (for simplicity, otherwise there is 
nothing wrong with your change).

-- 
To view, visit https://gerrit.ovirt.org/54440
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iab96f47118148a67135286a3a9b1998840efce18
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
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: devices: fix behaviour with balloon model=none

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: vm: devices: fix behaviour with balloon model=none
..


Patch Set 1:

Couldn't we simply ignore balloon devices with model='none'?

-- 
To view, visit https://gerrit.ovirt.org/54440
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iab96f47118148a67135286a3a9b1998840efce18
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: utils: Making try block smaller in tobool()

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: utils: Making try block smaller in tobool()
..


Patch Set 1: Code-Review-1

Martin is right, the additional restriction to ValueError is not safe.

-- 
To view, visit https://gerrit.ovirt.org/57549
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9c7326798fc5a2c8c1491fb7433657fa460495be
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Vinzenz Feenstra 
Gerrit-Reviewer: gerrit-hooks 
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: devices: fix behaviour with balloon model=none

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: vm: devices: fix behaviour with balloon model=none
..


Patch Set 2:

(2 comments)

https://gerrit.ovirt.org/#/c/54440/2//COMMIT_MSG
Commit Message:

Line 5: CommitDate: 2016-05-17 14:35:00 +0200
Line 6: 
Line 7: vm: devices: fix behaviour with balloon model=none
Line 8: 
Line 9: TODO: learn why this doesn't happen in the creation path.
Because it (for me) happens only when libvirtd gets restarted during VM life.
Line 10: 
Line 11: In the recovery flow, should Vdsm recover a VM configured
Line 12: with balloon device model=none
Line 13: 


Line 8: 
Line 9: TODO: learn why this doesn't happen in the creation path.
Line 10: 
Line 11: In the recovery flow, should Vdsm recover a VM configured
Line 12: with balloon device model=none
Again, probably only if libvirtd is restarted or other problem happens with 
libvirtd/QEMU.
Line 13: 
Line 14: 
Line 15: 
Line 16: The recovery fails with


-- 
To view, visit https://gerrit.ovirt.org/54440
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iab96f47118148a67135286a3a9b1998840efce18
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: storage: Make _readspeed_regex compatible with more dd outputs

[mzamazal]

Milan Zamazal has abandoned this change.

Change subject: storage: Make _readspeed_regex compatible with more dd outputs
..


Abandoned

This change is not needed, it's already handled in the patches referred by Nir 
above.

-- 
To view, visit https://gerrit.ovirt.org/57513
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: abandon
Gerrit-Change-Id: I40e94305aa18000f2ba74463d71df552cac2cbf9
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: utils: Properly handle int argument in tobool().

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: utils: Properly handle int argument in tobool().
..


Patch Set 3: Code-Review+1

-- 
To view, visit https://gerrit.ovirt.org/57511
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1740f82f5fa5eb50c319b46ce428bbb9f8c0c15e
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Tomas Golembiovsky 
Gerrit-Reviewer: Vinzenz Feenstra 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: schema: Marked optional fields in ExternalVmInfo.

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: schema: Marked optional fields in ExternalVmInfo.
..


Patch Set 1: Code-Review+1

-- 
To view, visit https://gerrit.ovirt.org/57417
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8e2e7f81ba39be2616f732de1deff477a1ca6c65
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: Tomas Golembiovsky 
Gerrit-Reviewer: Tomas Jelinek 
Gerrit-Reviewer: Vinzenz Feenstra 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: Lazy loading of external VMs info

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: v2v: Lazy loading of external VMs info
..


Patch Set 1:

(3 comments)

https://gerrit.ovirt.org/#/c/57418/1/lib/vdsm/utils.py
File lib/vdsm/utils.py:

Line 413: if s is None:
Line 414: return False
Line 415: if type(s) == bool:
Line 416: return s
Line 417: if str(s).lower() == 'true':
Why do you need this change?
Line 418: return True
Line 419: return bool(int(s))
Line 420: except:
Line 421: return False


https://gerrit.ovirt.org/#/c/57418/1/lib/vdsm/v2v.py
File lib/vdsm/v2v.py:

Line 166: _add_vm(conn, vms, vm)
Line 167: else:
Line 168: vms.append({'vmName': vm.name(),
Line 169: 'status': 'Up' if vm.ID() > -1 else 
'Down'})
Line 170: 
> maybe _add_vm can handle the "names_only"...
... and we are already inconsistent with vm.ID() vs. vm.isActive() without 
explanation.
Line 171: return {'status': doneCode, 'vmList': vms}
Line 172: 
Line 173: 
Line 174: def convert_external_vm(uri, username, password, vminfo, job_id, irs):


https://gerrit.ovirt.org/#/c/57418/1/tests/v2vTests.py
File tests/v2vTests.py:

Line 328: def _connect(uri, username, passwd):
Line 329: return MockVirConnect(vms=self._vms)
Line 330: 
Line 331: vmIDs = [1, 3]
Line 332: names = [vm.name for vm in VM_SPECS if vm.id in vmIDs]
I'd suggest adding a non-existent name here to check that nothing bad happens.
Line 333: 
Line 334: with MonkeyPatchScope([(libvirtconnection, 'open_connection',
Line 335: _connect)]):
Line 336: vms = v2v.get_external_vms('esx://mydomain', 'user',


-- 
To view, visit https://gerrit.ovirt.org/57418
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3dc6fc4573b2c0b1c03ed87025452e14af2fc566
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: Tomas Golembiovsky 
Gerrit-Reviewer: Tomas Jelinek 
Gerrit-Reviewer: Vinzenz Feenstra 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: build: Make sure run_tests*.sh scripts are executable

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: build: Make sure run_tests*.sh scripts are executable
..


Patch Set 2:

Tomas Golembiovsky was kind to resolve the puzzle for us:
- We can simply replace top_srcdir "manually" in tests/Makefile.am.
- We can merge the two *.in rules if we don't mind handling *.sh using a shell 
condition inside.

-- 
To view, visit https://gerrit.ovirt.org/55949
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc1e3dc8ace7f69801b765262352903020cc8aef
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[ovirt-3.6]: hostdev: add is_assignable flag

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: hostdev: add is_assignable flag
..


Patch Set 3: Code-Review+1

-- 
To view, visit https://gerrit.ovirt.org/57506
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6fd0d39e777c6bc0f3175b737ace9be92df4cca0
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.6
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: Lazy loading of external VMs info

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: v2v: Lazy loading of external VMs info
..


Patch Set 2:

(1 comment)

Just one PEP8 issue, otherwise fine for me now.

https://gerrit.ovirt.org/#/c/57418/2/lib/vdsm/v2v.py
File lib/vdsm/v2v.py:

Line 167: else:
Line 168: # The reason we use vm.ID() instead of vm.isActive() 
here is
Line 169: # because ID is already available after 
_list_domains(),
Line 170: # but vm.isActive() requires another API call to the
Line 171: # hypervisor.
Beware of the final whitespace, PEP8 tools are going to complain.
Line 172: vms.append({'vmName': vm.name(),
Line 173: 'status': 'Up' if vm.ID() > -1 else 
'Down'})
Line 174: 
Line 175: return {'status': doneCode, 'vmList': vms}


-- 
To view, visit https://gerrit.ovirt.org/57418
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3dc6fc4573b2c0b1c03ed87025452e14af2fc566
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: Tomas Golembiovsky 
Gerrit-Reviewer: Tomas Jelinek 
Gerrit-Reviewer: Vinzenz Feenstra 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: utils: Fix bug in tobool()

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: utils: Fix bug in tobool()
..


Patch Set 1: Code-Review-1

Let's be a bit careful. We've had issues with string<->unicode conversions 
involving non-ASCII characters in the past. For this reason, it'd be better to 
add isinstance(s,basestring) check before using `lower' rather than the `str' 
conversion.

(And I dislike the original implementation of this function as well as its use 
too so I don't mind any harsher action to it.)

-- 
To view, visit https://gerrit.ovirt.org/57511
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1740f82f5fa5eb50c319b46ce428bbb9f8c0c15e
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Vinzenz Feenstra 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: Lazy loading of external VMs info

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: v2v: Lazy loading of external VMs info
..


Patch Set 3: Code-Review+1

-- 
To view, visit https://gerrit.ovirt.org/57418
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3dc6fc4573b2c0b1c03ed87025452e14af2fc566
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: Tomas Golembiovsky 
Gerrit-Reviewer: Tomas Jelinek 
Gerrit-Reviewer: Vinzenz Feenstra 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: storage: Make _readspeed_regex compatible with more dd outputs

[mzamazal]

Milan Zamazal has uploaded a new change for review.

Change subject: storage: Make _readspeed_regex compatible with more dd outputs
..

storage: Make _readspeed_regex compatible with more dd outputs

Some versions of dd, e.g. the current one in Debian testing, produce
outputs that don't match the current regexp.  For instance:

  460 bytes copied, 0.000234846 s, 2.0 MB/s

or

  4096 bytes (4.1 kB, 4.0 KiB) copied, 0.00150043 s, 2.7 MB/s

This patch makes _readspeed_regex compatible with those outputs.

Change-Id: I40e94305aa18000f2ba74463d71df552cac2cbf9
Signed-off-by: Milan Zamazal 
---
M lib/vdsm/storage/misc.py
M tests/miscTests.py
2 files changed, 6 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/13/57513/1

diff --git a/lib/vdsm/storage/misc.py b/lib/vdsm/storage/misc.py
index fab8dd7..3876135 100644
--- a/lib/vdsm/storage/misc.py
+++ b/lib/vdsm/storage/misc.py
@@ -193,7 +193,8 @@
 return str(ctime)
 
 _readspeed_regex = re.compile(
-"(?P\d+) bytes? \([\de\-.]+ [kMGT]*B\) copied, "
+"(?P\d+) bytes?"
+"( \([\de\-.]+ [kMGT]*B(, [\de\-.]+ [KMGTi]*B)?\))? copied, "
 "(?P[\de\-.]+) s, "
 "([\de\-.]+|Infinity) [kMGT]*B/s"
 )
diff --git a/tests/miscTests.py b/tests/miscTests.py
index 8359833..9b563d8 100644
--- a/tests/miscTests.py
+++ b/tests/miscTests.py
@@ -858,6 +858,10 @@
  "512", "1"),
 ("524288 bytes (512e3 B) copied, 1 s, 512e3 B/s",
  "524288", "1"),
+("4096 bytes (4.1 kB, 4.0 KiB) copied, 1 s, 4 kB/s",
+ "4096", "1"),
+("460 bytes copied, 1 s, 460 B/s",
+ "460", "1"),
 ("517 bytes (517 B) copied, 0 s, Infinity B/s",
  "517", "0")
 ])


-- 
To view, visit https://gerrit.ovirt.org/57513
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I40e94305aa18000f2ba74463d71df552cac2cbf9
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[ovirt-3.6]: hostdev: add is_assignable flag

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: hostdev: add is_assignable flag
..


Patch Set 4: Code-Review+1

-- 
To view, visit https://gerrit.ovirt.org/57506
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6fd0d39e777c6bc0f3175b737ace9be92df4cca0
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.6
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
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: devices: fix behaviour with balloon model=none

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: vm: devices: fix behaviour with balloon model=none
..


Patch Set 1:

We've got a bug for this now: https://bugzilla.redhat.com/1335840

-- 
To view, visit https://gerrit.ovirt.org/54440
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iab96f47118148a67135286a3a9b1998840efce18
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: hostdev: teardown non-scsi devices

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: hostdev: teardown non-scsi devices
..


Patch Set 6: Code-Review+1

Without understanding all the possible implications, it looks basically fine to 
me. (And I wouldn't make it more complicated unless there is a reason to do so.)

-- 
To view, visit https://gerrit.ovirt.org/57375
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Icc8b8780203fcd29afa37dabc4594e2767245359
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: hostdev: fix rmAppropriateSCSIDevice in reattach

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: hostdev: fix rmAppropriateSCSIDevice in reattach
..


Patch Set 5: Code-Review+1

-- 
To view, visit https://gerrit.ovirt.org/57374
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I90e0e38bc86fa52bb654934b03f080490ba23fd8
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: migrations: enhance legacy downtime alg

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: migrations: enhance legacy downtime alg
..


Patch Set 5:

(2 comments)

Thanks for clarifications. I'm still confused about migration_downtime_delay 
description, see the inline comment.

https://gerrit.ovirt.org/#/c/56561/5/lib/vdsm/config.py.in
File lib/vdsm/config.py.in:

Line 113: ('migration_downtime_delay', '100',
Line 114: 'This value is used on the source host to define the 
delay before '
Line 115: 'setting/increasing the downtime of a migration. '
Line 116: 'The value is seconds per GiB of RAM divided by '
Line 117: 'migration_downtime_steps. The resulting maximum is 60 
seconds.'),
I find the whole description still confusing (and quite complicated now). The 
option value defines basically the total downtime thread time in seconds per 
GiB, right? But its description here actually corresponds to the step value 
(20), right?
Line 118: 
Line 119: ('migration_downtime_steps', '5',
Line 120: 'Incremental steps used to reach migration_downtime.'),
Line 121: 


https://gerrit.ovirt.org/#/c/56561/3/vdsm/virt/migration.py
File vdsm/virt/migration.py:

Line 501: base = (downtime - offset) ** (1 / float(steps - 1))
Line 502: 
Line 503: for i in range(steps):
Line 504: yield int(offset + base ** i)
Line 505: 
> about small VMs:
You're right, I was confused about DowntimeThread time and step time. See my 
additional comment in config.py.in.
Line 506: 
Line 507: class DowntimeThread(threading.Thread):
Line 508: def __init__(self, vm, downtime, steps):
Line 509: super(DowntimeThread, self).__init__()


-- 
To view, visit https://gerrit.ovirt.org/56561
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5d18a77c91a1344110afef1577e310faab3ffe5b
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Jelinek 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Tomas Jelinek 
Gerrit-Reviewer: gerrit-hooks 
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: devices: fix behaviour with balloon model=none

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: vm: devices: fix behaviour with balloon model=none
..


Patch Set 1:

For me, the balloon alias gets lost after libvirtd restart. Just restarting 
Vdsm doesn't cause the problem.

-- 
To view, visit https://gerrit.ovirt.org/54440
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iab96f47118148a67135286a3a9b1998840efce18
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: migration: Enable lazy setting of incoming/outgoing limits

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: migration: Enable lazy setting of incoming/outgoing limits
..


Patch Set 32: Code-Review+1

-- 
To view, visit https://gerrit.ovirt.org/53305
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I79ab97f15788e4024c94d051e4aade713d760acf
Gerrit-PatchSet: 32
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Betak 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Betak 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: virt: graphics: enforce spice default mode

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: virt: graphics: enforce spice default mode
..


Patch Set 4: Code-Review+1

-- 
To view, visit https://gerrit.ovirt.org/56746
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I169e7c4a76717dda8aeacbdb20ee031f453ed4fa
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Michal Skrivanek 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: migration: Enable lazy setting of incoming/outgoing limits

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: migration: Enable lazy setting of incoming/outgoing limits
..


Patch Set 25:

(2 comments)

https://gerrit.ovirt.org/#/c/53305/30/vdsm/API.py
File vdsm/API.py:

Line 586: params['vmId'] = self._UUID
Line 587: result = self.create(params)
Line 588: if result['status']['code']:
Line 589: self.log.debug('Migration create - Failed')
Line 590: # for compatibility with < 4.0 src that could not handle 
the
What if incomingLimit is 0?
Line 591: # retry error code
Line 592: is_old_source = not incomingLimit
Line 593: is_retry_error = response.is_error(result, 'migrateLimit')
Line 594: if is_old_source and is_retry_error:


https://gerrit.ovirt.org/#/c/53305/25/vdsm/virt/migration.py
File vdsm/virt/migration.py:

Line 320:  -1, -1) # int1, int2
Line 321: raise e
Line 322: 
Line 323: def _update_outgoing_limit(self):
Line 324: if self._outgoingLimit:
What if 0?
Line 325: self.log.debug('Setting outgoing migration limit to %s',
Line 326:self._outgoingLimit)
Line 327: SourceThread.ongoingMigrations.bound = self._outgoingLimit
Line 328: 


-- 
To view, visit https://gerrit.ovirt.org/53305
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I79ab97f15788e4024c94d051e4aade713d760acf
Gerrit-PatchSet: 25
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Betak 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Betak 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: migration: Enable lazy setting of incoming/outgoing limits

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: migration: Enable lazy setting of incoming/outgoing limits
..


Patch Set 30: Code-Review-1

(2 comments)

Just a documentation issue, please fix it if possible to avoid confusion.

https://gerrit.ovirt.org/#/c/53305/25/vdsm/API.py
File vdsm/API.py:

Line 573: except KeyError:
Line 574: return errCode['noVM']
Line 575: return v.migrateCancel()
Line 576: 
Line 577: def migrationCreate(self, params, incomingLimit=None):
It's uint according to the schema.
Line 578: """
Line 579: Start a migration-destination VM.
Line 580: 
Line 581: :param params: parameters of new VM, to be passed to


https://gerrit.ovirt.org/#/c/53305/30/vdsm/API.py
File vdsm/API.py:

Line 586: :type incomingLimit: int
Line 587: """
Line 588: self.log.debug('Migration create')
Line 589: 
Line 590: if incomingLimit:
> Should be a illegal value, so it should be OK that the setting is skipped.
OK, then it should be documented in the schema and the docstrings that 
incomingLimit (and outgoingLimit?) must be positive numbers.
Line 591: self.log.debug('Setting incoming migration limit to %s',
Line 592:incomingLimit)
Line 593: migration.incomingMigrations.bound = incomingLimit
Line 594: 


-- 
To view, visit https://gerrit.ovirt.org/53305
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I79ab97f15788e4024c94d051e4aade713d760acf
Gerrit-PatchSet: 30
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Betak 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Betak 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Piotr Kliczewski 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: migrations: enhance legacy downtime alg

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: migrations: enhance legacy downtime alg
..


Patch Set 6: Code-Review+1

-- 
To view, visit https://gerrit.ovirt.org/56561
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5d18a77c91a1344110afef1577e310faab3ffe5b
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Jelinek 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Tomas Jelinek 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: migration: log the convergence schedule only if provided

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: migration: log the convergence schedule only if provided
..


Patch Set 1: Code-Review+1

-- 
To view, visit https://gerrit.ovirt.org/57370
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id34f02deea261cd7e09197771950316efc09ebd5
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Jelinek 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: test: validate that OVS tests runs as root

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: test: validate that OVS tests runs as root
..


Patch Set 1: Code-Review+1

It fixes the problem I had with `make rpm' in Vdsm.

-- 
To view, visit https://gerrit.ovirt.org/56952
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I04404531ad85ca357dce3ed72ca7fd3f2be9c533
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Petr Horáček 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Edward Haas 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: virt: Don't fail when existingConnAction is unset for a SPIC...

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: virt: Don't fail when existingConnAction is unset for a SPICE 
device
..


Patch Set 3: Verified+1

Verified by successfully opening a SPICE console with a patched Engine not 
sending existingConnAction parameter.

Rerun-Hooks: all

-- 
To view, visit https://gerrit.ovirt.org/56836
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2fde83ceb994eaafd2aa956d1fa82ce6cb16094c
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Vinzenz Feenstra 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: migrations: change convergence schedule from time to iterations

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: migrations: change convergence schedule from time to iterations
..


Patch Set 4: Code-Review+1

To clarify the last Tomáš's comment: We primarily care about small VMs. There 
is a good probability to catch at least some iterations with them so the 
algorithm should generally work on them. As of my concern about continuous but 
too small progress without reaching the desired amount of memory, it's more 
likely to happen with large VMs. But we don't care about them that much as 
their migrations are expected to take long time anyway. So this patch should 
correspond to our goals. (Please correct me if there is anything wrong above.)

-- 
To view, visit https://gerrit.ovirt.org/56558
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6f87c954031842c35c99888c228a34ec7f19d800
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Jelinek 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Tomas Jelinek 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: migrations: change convergence schedule from time to iterations

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: migrations: change convergence schedule from time to iterations
..


Patch Set 4:

My last comment was wrong. The patch is actually targeted at large VMs. My 
confusion was from misinterpreting data_remaining value. It contains the amount 
of memory to copy in the current iteration not counting memory already copied 
and dirtied in the meantime (so it's not a total amount of dirty memory, it's 
just a pointer to current iteration progress). Then this algorithm should work 
fine indeed.

-- 
To view, visit https://gerrit.ovirt.org/56558
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6f87c954031842c35c99888c228a34ec7f19d800
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Jelinek 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Tomas Jelinek 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: virt: Don't fail when existingConnAction is unset for a SPIC...

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: virt: Don't fail when existingConnAction is unset for a SPICE 
device
..


Patch Set 5: Verified+1

-- 
To view, visit https://gerrit.ovirt.org/56836
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2fde83ceb994eaafd2aa956d1fa82ce6cb16094c
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Vinzenz Feenstra 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: virt: Don't fail when existingConnAction is unset for a SPIC...

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: virt: Don't fail when existingConnAction is unset for a SPICE 
device
..


Patch Set 4:

(2 comments)

https://gerrit.ovirt.org/#/c/56836/3/lib/api/vdsmapi-schema.json
File lib/api/vdsmapi-schema.json:

Line 7402: # @password:The desired connection password
Line 7403: #
Line 7404: # @ttl: The number of seconds before the password 
expires
Line 7405: #
Line 7406: # @existingConnAction:  #optional Indicate what to do with any 
existing connections
> please add a line like
Done
Line 7407: #   (made optional in version 4.18.0)
Line 7408: #
Line 7409: # @params:  Arbitrary key:val pairs that will be passed 
to hooks
Line 7410: #


https://gerrit.ovirt.org/#/c/56836/3/tests/vmOperationsTests.py
File tests/vmOperationsTests.py:

PS3, Line 146: graphics_params = dict(_GRAPHICS_DEVICE_PARAMS)
 : del graphics_params['existingConnAction']
> silly nit: I think
I couldn't decide which version is better, so you resolved it. :-) Done.


-- 
To view, visit https://gerrit.ovirt.org/56836
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2fde83ceb994eaafd2aa956d1fa82ce6cb16094c
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Vinzenz Feenstra 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: vmtests: use assertXMLEqual in assertBuildCmdline

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: vmtests: use assertXMLEqual in assertBuildCmdline
..


Patch Set 1: Code-Review+1

-- 
To view, visit https://gerrit.ovirt.org/56973
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If38342d837374c51a862814288803915e4ac3f07
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: virt: delay appendFeatures and appendClock

[mzamazal]

Milan Zamazal has posted comments on this change.

Change subject: virt: delay appendFeatures and appendClock
..


Patch Set 1: Code-Review+1

-- 
To view, visit https://gerrit.ovirt.org/56974
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4c3a719b57978a7f7adc0f0845c706e3c525574b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches