subject:"Change in vdsm\[master\]\: vm\: unit test for vm.

Change in vdsm[master]: vm: unit test for vm._waitForDriveRemoval

2015-12-02 Thread mmirecki

Marcin Mirecki has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 12: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: unit test for vm._waitForDriveRemoval

2015-12-02 Thread nsoffer

Nir Soffer has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 12: Code-Review+2

Thanks Marcin!

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: unit test for vm._waitForDriveRemoval

2015-12-02 Thread nsoffer

Nir Soffer has submitted this change and it was merged.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


vm: unit test for vm._waitForDriveRemoval

Added unit tests for vm._waitForDriveRemoval, in
preparation to make this generic for all devices.

Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Signed-off-by: Marcin Mirecki 
Bug-Url: https://bugzilla.redhat.com/1134256
Reviewed-on: https://gerrit.ovirt.org/48880
Reviewed-by: Francesco Romani 
Continuous-Integration: Jenkins CI
Reviewed-by: Nir Soffer 
---
M lib/vdsm/config.py.in
M tests/vmTests.py
M vdsm/virt/vm.py
3 files changed, 120 insertions(+), 2 deletions(-)

Approvals:
  Nir Soffer: Looks good to me, approved
  Marcin Mirecki: Verified
  Jenkins CI: Passed CI tests
  Francesco Romani: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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]: vm: unit test for vm._waitForDriveRemoval

2015-12-02 Thread automation

gerrit-hooks has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 13:

* #1134256::Update tracker: OK
* Set MODIFIED::bug 1134256#1134256IGNORE, not oVirt classification but 
Red Hat

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: unit test for vm._waitForDriveRemoval

2015-12-02 Thread automation

gerrit-hooks has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 12:

* #1134256::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1134256::OK, public bug
* Check Product::#1134256::OK, Correct product Red Hat Enterprise 
Virtualization Manager
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-3.5 ovirt-3.4 
ovirt-3.3 ovirt-3.2)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: unit test for vm._waitForDriveRemoval

2015-12-01 Thread fromani

Francesco Romani has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 11: Code-Review+2

still looks good after big rebase

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: unit test for vm._waitForDriveRemoval

2015-12-01 Thread nsoffer

Nir Soffer has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 11:

(1 comment)

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

Line 2714: self.log.debug("Waiting for hotunplug to finish")
Line 2715: with utils.stopwatch("Hotunplug disk %s" % drive.name):
Line 2716: deadline = (utils.monotonic_time() +
Line 2717: config.getfloat('vars', 'hotunplug_timeout'))
Line 2718: sleep_time = config.getfloat('vars', 
'hotunplug_check_interval')
> not sure this should be float, but could be fixed later.
It must be for the tests, unless we monkeypatch the utils.monotonic_time() and 
time.sleep() during the tests.
Line 2719: while self._isDriveAttached(drive):
Line 2720: time.sleep(sleep_time)
Line 2721: if utils.monotonic_time() > deadline:
Line 2722: raise HotunplugTimeout("Timeout detaching drive 
%s"


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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]: vm: unit test for vm._waitForDriveRemoval

2015-12-01 Thread fromani

Francesco Romani has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 11:

(1 comment)

added one inside comment, but can be fixed by a later patch

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

Line 2714: self.log.debug("Waiting for hotunplug to finish")
Line 2715: with utils.stopwatch("Hotunplug disk %s" % drive.name):
Line 2716: deadline = (utils.monotonic_time() +
Line 2717: config.getfloat('vars', 'hotunplug_timeout'))
Line 2718: sleep_time = config.getfloat('vars', 
'hotunplug_check_interval')
not sure this should be float, but could be fixed later.
Line 2719: while self._isDriveAttached(drive):
Line 2720: time.sleep(sleep_time)
Line 2721: if utils.monotonic_time() > deadline:
Line 2722: raise HotunplugTimeout("Timeout detaching drive 
%s"


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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]: vm: unit test for vm._waitForDriveRemoval

2015-12-01 Thread automation

gerrit-hooks has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 10:

* #1134256::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1134256::OK, public bug
* Check Product::#1134256::OK, Correct product Red Hat Enterprise 
Virtualization Manager
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-3.5 ovirt-3.4 
ovirt-3.3 ovirt-3.2)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: unit test for vm._waitForDriveRemoval

2015-12-01 Thread automation

gerrit-hooks has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 11:

* #1134256::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1134256::OK, public bug
* Check Product::#1134256::OK, Correct product Red Hat Enterprise 
Virtualization Manager
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-3.5 ovirt-3.4 
ovirt-3.3 ovirt-3.2)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: unit test for vm._waitForDriveRemoval

2015-11-30 Thread nsoffer

Nir Soffer has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 9:

Marcin, can you verify?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: unit test for vm._waitForDriveRemoval

2015-11-30 Thread mmirecki

Marcin Mirecki has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 9: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: unit test for vm._waitForDriveRemoval

2015-11-30 Thread nsoffer

Nir Soffer has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 9:

Marcin, can you rebase?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: unit test for vm._waitForDriveRemoval

2015-11-30 Thread nsoffer

Nir Soffer has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 9:

Thanks Marcin!

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: unit test for vm._waitForDriveRemoval

2015-11-26 Thread nsoffer

Nir Soffer has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 7: Code-Review-1

(3 comments)

https://gerrit.ovirt.org/#/c/48880/7/tests/vmTests.py
File tests/vmTests.py:

Line 1294: # never reach sleep when device is removed in first check, and 
method
Line 1295: # should exit immediately
Line 1296: @MonkeyPatch(vm, "config", make_config([
Line 1297: ("vars", "hotunplug_timeout", "0"),
Line 1298: ("vars", "hotunplug_check_interval", "1")
This value does not effect the test, so we don't have to monkeypatch it in this 
test.
Line 1299: ]))
Line 1300: @MonkeyPatch(utils, "isBlockDevice", lambda x: x == 
"/block_path")
Line 1301: @permutations([[drive_file, FILE_DRIVE_XML],
Line 1302:[drive_network, NETWORK_DRIVE_XML],


https://gerrit.ovirt.org/#/c/48880/7/vdsm/virt/vmdevices/network.py
File vdsm/virt/vmdevices/network.py:

Line 190: """
Line 191: Returns xpath to the device in libvirt dom xml
Line 192: The path is relative to the root element
Line 193: """
Line 194: return "./devices/interface/mac[@address='%s']" % self.macAddr
Not needed, belongs to the next patch.


https://gerrit.ovirt.org/#/c/48880/7/vdsm/virt/vmdevices/storage.py
File vdsm/virt/vmdevices/storage.py:

Line 432: source_key = {DISK_TYPE.FILE: 'file',
Line 433:   DISK_TYPE.BLOCK: 'dev',
Line 434:   DISK_TYPE.NETWORK: 'name'}
Line 435: return "./devices/disk/source[@%s='%s']" \
Line 436:% (source_key[self.diskType], self.path)
Not needed, belongs to the next patch.
Line 437: 
Line 438: 
Line 439: def _getSourceXML(drive):
Line 440: source = vmxml.Element('source')


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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]: vm: unit test for vm._waitForDriveRemoval

2015-11-26 Thread fromani

Francesco Romani has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 7:

(1 comment)

https://gerrit.ovirt.org/#/c/48880/7/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2682: self.log.debug("Waiting for hotunplug to finish")
Line 2683: with utils.stopwatch("Hotunplug disk %s" % drive.name):
Line 2684: deadline = (utils.monotonic_time() +
Line 2685: config.getfloat('vars', 'hotunplug_timeout'))
Line 2686: sleep_time = config.getfloat('vars', 
'hotunplug_check_interval')
not really sure this makes sense as float. OK, it is supported as float, but I 
can't see an use case to wait, leike, 1.5s or 0.723s.
Not very important, however.
Line 2687: while self._isDriveAttached(drive):
Line 2688: time.sleep(sleep_time)
Line 2689: if utils.monotonic_time() > deadline:
Line 2690: raise HotunplugTimeout("Timeout detaching drive 
%s"


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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]: vm: unit test for vm._waitForDriveRemoval

2015-11-26 Thread nsoffer

Nir Soffer has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 7:

(1 comment)

https://gerrit.ovirt.org/#/c/48880/7/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2682: self.log.debug("Waiting for hotunplug to finish")
Line 2683: with utils.stopwatch("Hotunplug disk %s" % drive.name):
Line 2684: deadline = (utils.monotonic_time() +
Line 2685: config.getfloat('vars', 'hotunplug_timeout'))
Line 2686: sleep_time = config.getfloat('vars', 
'hotunplug_check_interval')
> not really sure this makes sense as float. OK, it is supported as float, bu
The only reason is having short sleeps in the tests.
Line 2687: while self._isDriveAttached(drive):
Line 2688: time.sleep(sleep_time)
Line 2689: if utils.monotonic_time() > deadline:
Line 2690: raise HotunplugTimeout("Timeout detaching drive 
%s"


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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]: vm: unit test for vm._waitForDriveRemoval

2015-11-26 Thread mmirecki

Marcin Mirecki has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 7:

(4 comments)

https://gerrit.ovirt.org/#/c/48880/7/tests/vmTests.py
File tests/vmTests.py:

Line 1294: # never reach sleep when device is removed in first check, and 
method
Line 1295: # should exit immediately
Line 1296: @MonkeyPatch(vm, "config", make_config([
Line 1297: ("vars", "hotunplug_timeout", "0"),
Line 1298: ("vars", "hotunplug_check_interval", "1")
> This value does not effect the test, so we don't have to monkeypatch it in 
Both values are not required here
Removing
Line 1299: ]))
Line 1300: @MonkeyPatch(utils, "isBlockDevice", lambda x: x == 
"/block_path")
Line 1301: @permutations([[drive_file, FILE_DRIVE_XML],
Line 1302:[drive_network, NETWORK_DRIVE_XML],


Line 1310: ("vars", "hotunplug_timeout", "1")
This is not required too


https://gerrit.ovirt.org/#/c/48880/7/vdsm/virt/vmdevices/network.py
File vdsm/virt/vmdevices/network.py:

Line 190: """
Line 191: Returns xpath to the device in libvirt dom xml
Line 192: The path is relative to the root element
Line 193: """
Line 194: return "./devices/interface/mac[@address='%s']" % self.macAddr
> Not needed, belongs to the next patch.
Done


https://gerrit.ovirt.org/#/c/48880/7/vdsm/virt/vmdevices/storage.py
File vdsm/virt/vmdevices/storage.py:

Line 432: source_key = {DISK_TYPE.FILE: 'file',
Line 433:   DISK_TYPE.BLOCK: 'dev',
Line 434:   DISK_TYPE.NETWORK: 'name'}
Line 435: return "./devices/disk/source[@%s='%s']" \
Line 436:% (source_key[self.diskType], self.path)
> Not needed, belongs to the next patch.
Done
Line 437: 
Line 438: 
Line 439: def _getSourceXML(drive):
Line 440: source = vmxml.Element('source')


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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]: vm: unit test for vm._waitForDriveRemoval

2015-11-26 Thread nsoffer

Nir Soffer has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 7:

(1 comment)

https://gerrit.ovirt.org/#/c/48880/7/tests/vmTests.py
File tests/vmTests.py:

Line 1294: # never reach sleep when device is removed in first check, and 
method
Line 1295: # should exit immediately
Line 1296: @MonkeyPatch(vm, "config", make_config([
Line 1297: ("vars", "hotunplug_timeout", "0"),
Line 1298: ("vars", "hotunplug_check_interval", "1")
> Both values are not required here
No, if the code is wrong, the tests will timeout after 30 seconds - want them 
to time out sooner, so using 1 second timeout is good.
Line 1299: ]))
Line 1300: @MonkeyPatch(utils, "isBlockDevice", lambda x: x == 
"/block_path")
Line 1301: @permutations([[drive_file, FILE_DRIVE_XML],
Line 1302:[drive_network, NETWORK_DRIVE_XML],


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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]: vm: unit test for vm._waitForDriveRemoval

2015-11-26 Thread automation

gerrit-hooks has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 8:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: unit test for vm._waitForDriveRemoval

2015-11-26 Thread automation

gerrit-hooks has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 9:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: unit test for vm._waitForDriveRemoval

2015-11-26 Thread nsoffer

Nir Soffer has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 9: Code-Review+2

Waiting for Francesco approval.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: unit test for vm._waitForDriveRemoval

2015-11-26 Thread nsoffer

Nir Soffer has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 9:

Marcin, any reason why this is not verified?

Jenkins run the tests and it is happy, if you believe that the test are correct 
you should mark it as verified.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: unit test for vm._waitForDriveRemoval

2015-11-26 Thread fromani

Francesco Romani has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 9: Code-Review+2

impressive work, big thanks!

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: unit test for vm._waitForDriveRemoval

2015-11-25 Thread automation

gerrit-hooks has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 5:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: unit test for vm._waitForDriveRemoval

2015-11-25 Thread automation

gerrit-hooks has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 6:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: unit test for vm._waitForDriveRemoval

2015-11-25 Thread mmirecki

Marcin Mirecki has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 4:

(10 comments)

https://gerrit.ovirt.org/#/c/48880/4/tests/vmTests.py
File tests/vmTests.py:

Line 1263: @MonkeyPatch(vm, "config", make_config([
Line 1264: ("vars", "hotunplug_timeout", "0.5"),
Line 1265: ("vars", "hotunplug_check_interval", "0.1")
Line 1266: ]))
Line 1267: def test_wait_for_drive_removal_timeout(self):
> We don't need to repeat the class name in the tests - use:
Done
Line 1268: drive = Drive({}, log=self.log, index=0, path='test_path', 
iface="")
Line 1269: testvm = TestingVm(FakeVmDom(self.DRIVE_XML, 
times_to_match=9))
Line 1270: 
Line 1271: self.assertRaises(HotunplugTimeout, 
testvm._waitForDriveRemoval, drive)


Line 1266: ]))
Line 1267: def test_wait_for_drive_removal_timeout(self):
Line 1268: drive = Drive({}, log=self.log, index=0, path='test_path', 
iface="")
Line 1269: testvm = TestingVm(FakeVmDom(self.DRIVE_XML, 
times_to_match=9))
Line 1270: 
> Empty line is not needed, this is a very short and simple test - same for o
Done
Line 1271: self.assertRaises(HotunplugTimeout, 
testvm._waitForDriveRemoval, drive)
Line 1272: 
Line 1273: # The timeout hotunplug_check_interval=1 should never be 
reached. We should
Line 1274: # never reach sleep when device is removed in first check, and 
method


Line 1280: def test_wait_for_drive_removal_removed_on_first_check(self):
Line 1281: drive = Drive({}, log=self.log, index=0, path='test_path', 
iface="")
Line 1282: testvm = TestingVm(FakeVmDom(self.DRIVE_XML))
Line 1283: 
Line 1284: testvm._waitForDriveRemoval(drive)
> Check for testvm._dom.xmldesc_fetched == 1
Done
Line 1285: 
Line 1286: @MonkeyPatch(vm, "config", make_config([
Line 1287: ("vars", "hotunplug_timeout", "1"),
Line 1288: ("vars", "hotunplug_check_interval", "0")


Line 1290: def test_wait_for_drive_removal_removed_on_x_check(self):
Line 1291: drive = Drive({}, log=self.log, index=0, path='test_path', 
iface="")
Line 1292: testvm = TestingVm(FakeVmDom(self.DRIVE_XML, 
times_to_match=2))
Line 1293: 
Line 1294: testvm._waitForDriveRemoval(drive)
> Check for testvm._dom.xmldesc_fetched == 3
Done
Line 1295: 
Line 1296: 
Line 1297: class FakeVmDom(object):
Line 1298: 


Line 1293: 
Line 1294: testvm._waitForDriveRemoval(drive)
Line 1295: 
Line 1296: 
Line 1297: class FakeVmDom(object):
> Lets give this a more specific name.
Done
Line 1298: 
Line 1299: DEFAULT_XML = """
Line 1300: 
Line 1301: 


Line 1298: 
Line 1299: DEFAULT_XML = """
Line 1300: 
Line 1301: 
Line 1302: 
> We need other devices in the default xml. For testing drives, we need other
For the sake of tesing, it might be worthwhile to have some of the devices for 
all checks.
Adding some devices with different types.
Line 1303: """
Line 1304: 
Line 1305: def __init__(self, device_xml, times_to_match=0):
Line 1306: self.matches = times_to_match


Line 1301: 
Line 1302: 
Line 1303: """
Line 1304: 
Line 1305: def __init__(self, device_xml, times_to_match=0):
> Lets add a parameter for the domain_xml, the xml that should be returned wh
The times_to_match should never be exceeded. If it is, we have an unexpected 
behaviour. 
Raising an exception for such a case.
Line 1306: self.matches = times_to_match
Line 1307: result_xml = ET.fromstring(self.DEFAULT_XML)
Line 1308: result_xml.find("devices").append(ET.fromstring(device_xml))
Line 1309: self.xml = ET.tostring(result_xml)


Line 1302: 
Line 1303: """
Line 1304: 
Line 1305: def __init__(self, device_xml, times_to_match=0):
Line 1306: self.matches = times_to_match
> Now that we have a counter, we can keep the name as is
Done
Line 1307: result_xml = ET.fromstring(self.DEFAULT_XML)
Line 1308: result_xml.find("devices").append(ET.fromstring(device_xml))
Line 1309: self.xml = ET.tostring(result_xml)
Line 1310: 


Line 1305: def __init__(self, device_xml, times_to_match=0):
Line 1306: self.matches = times_to_match
Line 1307: result_xml = ET.fromstring(self.DEFAULT_XML)
Line 1308: result_xml.find("devices").append(ET.fromstring(device_xml))
Line 1309: self.xml = ET.tostring(result_xml)
> lets call this self.domain_xml_with_device to make it more clear.
Done
Line 1310: 
Line 1311: def XMLDesc(self, flags):
Line 1312: if self.matches == 0:
Line 1313: return self.DEFAULT_XML


Line 1307: result_xml = ET.fromstring(self.DEFAULT_XML)
Line 1308: result_xml.find("devices").append(ET.fromstring(device_xml))
Line 1309: self.xml = ET.tostring(result_xml)
Line 1310: 
Line 1311: def XMLDesc(self,

Change in vdsm[master]: vm: unit test for vm._waitForDriveRemoval

2015-11-25 Thread mmirecki

Marcin Mirecki has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 5:

(2 comments)

https://gerrit.ovirt.org/#/c/48880/5/tests/vmTests.py
File tests/vmTests.py:

Line 1283: 0.1
Made it still shorter.
With 4 tests, it seemed terribly long when set to 1.


Line 1286:  x
Drive actually goes to the file system to check if the device is a block device.
Need to override in tests (hope this is not too dirty).


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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]: vm: unit test for vm._waitForDriveRemoval

2015-11-25 Thread nsoffer

Nir Soffer has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 6:

Nice work!

See the comments.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: unit test for vm._waitForDriveRemoval

2015-11-25 Thread mmirecki

Marcin Mirecki has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 5:

(2 comments)

https://gerrit.ovirt.org/#/c/48880/5/tests/vmTests.py
File tests/vmTests.py:

Line 1279: drive_block = Drive({}, log=logging.getLogger(''), index=0, 
iface="",
Line 1280: path= "/block_path", 
diskType=DISK_TYPE.BLOCK)
Line 1281: 
Line 1282: @MonkeyPatch(vm, "config", make_config([
Line 1283: ("vars", "hotunplug_timeout", "0.1"),
> 0.03 is too short for overloaded ci slaves.
Ok
But you take over the guilt for everyone making a build waiting for 1 (0.25 x 4 
tests) second longer
Line 1284: ("vars", "hotunplug_check_interval", "0.03")
Line 1285: ]))
Line 1286: @MonkeyPatch(utils, "isBlockDevice", lambda x: x == 
"/block_path")
Line 1287: @permutations([[drive_default, FILE_DRIVE_XML],


Line 1338: 
Line 1339: 
Line 1340: 
Line 1341: 
Line 1342: 
> Lets update the xml of the devices to look more like the real xml, see vmSt
Done
Line 1343: 
Line 1344: 
Line 1345: 
Line 1346: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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]: vm: unit test for vm._waitForDriveRemoval

2015-11-25 Thread nsoffer

Nir Soffer has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 5:

(1 comment)

https://gerrit.ovirt.org/#/c/48880/5/tests/vmTests.py
File tests/vmTests.py:

Line 1279: drive_block = Drive({}, log=logging.getLogger(''), index=0, 
iface="",
Line 1280: path= "/block_path", 
diskType=DISK_TYPE.BLOCK)
Line 1281: 
Line 1282: @MonkeyPatch(vm, "config", make_config([
Line 1283: ("vars", "hotunplug_timeout", "0.1"),
> Ok
Some test must be slow for correcness. We can skip these test in normal build 
using @slowtest.
See tests README for the details.

Anothet way is to monkeypatch utils.monotonic_time and time sleep, see for 
example mkimageTests.py. But the test slowly become more complicated than the 
actual code they test.
Line 1284: ("vars", "hotunplug_check_interval", "0.03")
Line 1285: ]))
Line 1286: @MonkeyPatch(utils, "isBlockDevice", lambda x: x == 
"/block_path")
Line 1287: @permutations([[drive_default, FILE_DRIVE_XML],


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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]: vm: unit test for vm._waitForDriveRemoval

2015-11-25 Thread automation

gerrit-hooks has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 7:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: unit test for vm._waitForDriveRemoval

2015-11-25 Thread nsoffer

Nir Soffer has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 5:

(8 comments)

https://gerrit.ovirt.org/#/c/48880/5/tests/vmTests.py
File tests/vmTests.py:

Line 1261: 
Line 1262: 
Line 1263: """
Line 1264: NETWORK_DRIVE_XML = """
Line 1265: 
type="network"
Line 1266: 
Line 1267: """
Line 1268: BLOCK_DRIVE_XML = """
Line 1269: 


Line 1265: 
Line 1266: 
Line 1267: """
Line 1268: BLOCK_DRIVE_XML = """
Line 1269: 
type="block"

See vmStorageTests for example xmls.
Line 1270: 
Line 1271: """
Line 1272: 
Line 1273: drive_default = Drive({}, log=logging.getLogger(''), index=0, 
iface="",


Line 1270: 
Line 1271: """
Line 1272: 
Line 1273: drive_default = Drive({}, log=logging.getLogger(''), index=0, 
iface="",
Line 1274:   path='test_path', )
We can remove the default, we are not testing the Drive class here.
Line 1275: drive_file = Drive({}, log=logging.getLogger(''), index=0, 
iface="",
Line 1276:path='test_path',  diskType=DISK_TYPE.FILE)
Line 1277: drive_network = Drive({}, log=logging.getLogger(''), index=0, 
iface="",
Line 1278:   path='test_path', 
diskType=DISK_TYPE.NETWORK)


Line 1276:path='test_path',  diskType=DISK_TYPE.FILE)
Line 1277: drive_network = Drive({}, log=logging.getLogger(''), index=0, 
iface="",
Line 1278:   path='test_path', 
diskType=DISK_TYPE.NETWORK)
Line 1279: drive_block = Drive({}, log=logging.getLogger(''), index=0, 
iface="",
Line 1280: path= "/block_path", 
diskType=DISK_TYPE.BLOCK)
path is always /rhev/data-center/p/d/images/i/v, (p=poolid, d=domainid, 
i=imageid, v=volumeid) for file and block. For network the path is 
rbd:pool/volume, but it does not really matter. For the tests, using the same 
"test_path" is fine.
Line 1281: 
Line 1282: @MonkeyPatch(vm, "config", make_config([
Line 1283: ("vars", "hotunplug_timeout", "0.1"),
Line 1284: ("vars", "hotunplug_check_interval", "0.03")


Line 1279: drive_block = Drive({}, log=logging.getLogger(''), index=0, 
iface="",
Line 1280: path= "/block_path", 
diskType=DISK_TYPE.BLOCK)
Line 1281: 
Line 1282: @MonkeyPatch(vm, "config", make_config([
Line 1283: ("vars", "hotunplug_timeout", "0.1"),
> Made it still shorter.
0.03 is too short for overloaded ci slaves.

Lets use timeout=0.25, check_internal=0.1 will time out after retries, in the 
worst case after one retry, good enough.
Line 1284: ("vars", "hotunplug_check_interval", "0.03")
Line 1285: ]))
Line 1286: @MonkeyPatch(utils, "isBlockDevice", lambda x: x == 
"/block_path")
Line 1287: @permutations([[drive_default, FILE_DRIVE_XML],


Line 1282: @MonkeyPatch(vm, "config", make_config([
Line 1283: ("vars", "hotunplug_timeout", "0.1"),
Line 1284: ("vars", "hotunplug_check_interval", "0.03")
Line 1285: ]))
Line 1286: @MonkeyPatch(utils, "isBlockDevice", lambda x: x == 
"/block_path")
> Drive actually goes to the file system to check if the device is a block de
Looks ok, the actual code is dirty in this case :-)
Line 1287: @permutations([[drive_default, FILE_DRIVE_XML],
Line 1288:[drive_file, FILE_DRIVE_XML],
Line 1289:[drive_network, NETWORK_DRIVE_XML],
Line 1290:[drive_block, BLOCK_DRIVE_XML]])


Line 1338: 
Line 1339: 
Line 1340: 
Line 1341: 
Line 1342: 
Lets update the xml of the devices to look more like the real xml, see 
vmStorageTests.
Line 1343: 
Line 1344: 
Line 1345: 
Line 1346: 


Line 1356: 
Line 1357: def XMLDesc(self, flags):
Line 1358: self.xmldesc_fetched += 1
Line 1359: if self.xmldesc_fetched > self.times_to_match + 1:
Line 1360: raise BaseException(
You should raise an Exception, Base Exception is not for user code (in general).

But I don't think we need it, it will make it less useful, I thin we should 
just count the fetches and assert about them in the tests.
Line 1361: "_waitForDriveRemoval called XMLDesc too many 
times.")
Line 1362: if self.xmldesc_fetched > self.times_to_match:
Line 1363: return self.DEFAULT_XML
Line 1364: else:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Francesco Romani

Change in vdsm[master]: vm: unit test for vm._waitForDriveRemoval

2015-11-25 Thread mmirecki

Marcin Mirecki has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 5:

(4 comments)

https://gerrit.ovirt.org/#/c/48880/5/tests/vmTests.py
File tests/vmTests.py:

Line 1261: 
Line 1262: 
Line 1263: """
Line 1264: NETWORK_DRIVE_XML = """
Line 1265: 
> type="network"
This is not checked by the code doing the search
Removing this not to cause confusion as to what is really relevant.
Line 1266: 
Line 1267: """
Line 1268: BLOCK_DRIVE_XML = """
Line 1269: 


Line 1265: 
Line 1266: 
Line 1267: """
Line 1268: BLOCK_DRIVE_XML = """
Line 1269: 
> type="block"
same as above
Line 1270: 
Line 1271: """
Line 1272: 
Line 1273: drive_default = Drive({}, log=logging.getLogger(''), index=0, 
iface="",


Line 1270: 
Line 1271: """
Line 1272: 
Line 1273: drive_default = Drive({}, log=logging.getLogger(''), index=0, 
iface="",
Line 1274:   path='test_path', )
> We can remove the default, we are not testing the Drive class here.
Done
Line 1275: drive_file = Drive({}, log=logging.getLogger(''), index=0, 
iface="",
Line 1276:path='test_path',  diskType=DISK_TYPE.FILE)
Line 1277: drive_network = Drive({}, log=logging.getLogger(''), index=0, 
iface="",
Line 1278:   path='test_path', 
diskType=DISK_TYPE.NETWORK)


Line 1356: 
Line 1357: def XMLDesc(self, flags):
Line 1358: self.xmldesc_fetched += 1
Line 1359: if self.xmldesc_fetched > self.times_to_match + 1:
Line 1360: raise BaseException(
> You should raise an Exception, Base Exception is not for user code (in gene
Done
Line 1361: "_waitForDriveRemoval called XMLDesc too many 
times.")
Line 1362: if self.xmldesc_fetched > self.times_to_match:
Line 1363: return self.DEFAULT_XML
Line 1364: else:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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]: vm: unit test for vm._waitForDriveRemoval

2015-11-24 Thread nsoffer

Nir Soffer has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 4: Code-Review-1

(10 comments)

https://gerrit.ovirt.org/#/c/48880/4/tests/vmTests.py
File tests/vmTests.py:

Line 1263: @MonkeyPatch(vm, "config", make_config([
Line 1264: ("vars", "hotunplug_timeout", "0.5"),
Line 1265: ("vars", "hotunplug_check_interval", "0.1")
Line 1266: ]))
Line 1267: def test_wait_for_drive_removal_timeout(self):
We don't need to repeat the class name in the tests - use:

def test_timeout(self):

Same for other tests.

To add tests for other devices, we can use permutations to run all tests with 
all devices without writing additional tests, or since this code does not care 
about the device, we can add one test for each device, testing one of the 
scenarios.
Line 1268: drive = Drive({}, log=self.log, index=0, path='test_path', 
iface="")
Line 1269: testvm = TestingVm(FakeVmDom(self.DRIVE_XML, 
times_to_match=9))
Line 1270: 
Line 1271: self.assertRaises(HotunplugTimeout, 
testvm._waitForDriveRemoval, drive)


Line 1266: ]))
Line 1267: def test_wait_for_drive_removal_timeout(self):
Line 1268: drive = Drive({}, log=self.log, index=0, path='test_path', 
iface="")
Line 1269: testvm = TestingVm(FakeVmDom(self.DRIVE_XML, 
times_to_match=9))
Line 1270: 
Empty line is not needed, this is a very short and simple test - same for other 
tests.
Line 1271: self.assertRaises(HotunplugTimeout, 
testvm._waitForDriveRemoval, drive)
Line 1272: 
Line 1273: # The timeout hotunplug_check_interval=1 should never be 
reached. We should
Line 1274: # never reach sleep when device is removed in first check, and 
method


Line 1280: def test_wait_for_drive_removal_removed_on_first_check(self):
Line 1281: drive = Drive({}, log=self.log, index=0, path='test_path', 
iface="")
Line 1282: testvm = TestingVm(FakeVmDom(self.DRIVE_XML))
Line 1283: 
Line 1284: testvm._waitForDriveRemoval(drive)
Check for testvm._dom.xmldesc_fetched == 1
Line 1285: 
Line 1286: @MonkeyPatch(vm, "config", make_config([
Line 1287: ("vars", "hotunplug_timeout", "1"),
Line 1288: ("vars", "hotunplug_check_interval", "0")


Line 1290: def test_wait_for_drive_removal_removed_on_x_check(self):
Line 1291: drive = Drive({}, log=self.log, index=0, path='test_path', 
iface="")
Line 1292: testvm = TestingVm(FakeVmDom(self.DRIVE_XML, 
times_to_match=2))
Line 1293: 
Line 1294: testvm._waitForDriveRemoval(drive)
Check for testvm._dom.xmldesc_fetched == 3
Line 1295: 
Line 1296: 
Line 1297: class FakeVmDom(object):
Line 1298: 


Line 1293: 
Line 1294: testvm._waitForDriveRemoval(drive)
Line 1295: 
Line 1296: 
Line 1297: class FakeVmDom(object):
Lets give this a more specific name.
Line 1298: 
Line 1299: DEFAULT_XML = """
Line 1300: 
Line 1301: 


Line 1298: 
Line 1299: DEFAULT_XML = """
Line 1300: 
Line 1301: 
Line 1302: 
We need other devices in the default xml. For testing drives, we need other 
drives with different path and different types.

We like to test all disk types: block, file, network.

Since all this is not relevant to removing nics, the default xml should be an 
argument to __init__ - each test will use a relevant xml for the type of device.
Line 1303: """
Line 1304: 
Line 1305: def __init__(self, device_xml, times_to_match=0):
Line 1306: self.matches = times_to_match


Line 1301: 
Line 1302: 
Line 1303: """
Line 1304: 
Line 1305: def __init__(self, device_xml, times_to_match=0):
Lets add a parameter for the domain_xml, the xml that should be returned when 
number of fetches exceeded times_to_match:

self.domain_xml =
Line 1306: self.matches = times_to_match
Line 1307: result_xml = ET.fromstring(self.DEFAULT_XML)
Line 1308: result_xml.find("devices").append(ET.fromstring(device_xml))
Line 1309: self.xml = ET.tostring(result_xml)


Line 1302: 
Line 1303: """
Line 1304: 
Line 1305: def __init__(self, device_xml, times_to_match=0):
Line 1306: self.matches = times_to_match
Now that we have a counter, we can keep the name as is
Line 1307: result_xml = ET.fromstring(self.DEFAULT_XML)
Line 1308: result_xml.find("devices").append(ET.fromstring(device_xml))
Line 1309: self.xml = ET.tostring(result_xml)
Line 1310: 


Line 1305: def __init__(self, device_xml, times_to_match=0):
Line 1306: self.matches = times_to_match
Line 1307: result_xml = ET.fromstring(self.DEFAULT_XML)
Line 1308: result_xml.find("devices").append(ET.fromstring(device_xml))
Line 1309: self.xml = ET.tostring(result_xml)
lets call this self.domain_xml_with_device to make it more clear.
Line 1310: 
Line 1311: def XMLDesc(self,

Change in vdsm[master]: vm: unit test for vm._waitForDriveRemoval

2015-11-24 Thread nsoffer

Nir Soffer has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 3:

(1 comment)

https://gerrit.ovirt.org/#/c/48880/3/tests/vmTests.py
File tests/vmTests.py:

Line 1271: drive = Drive({}, log=self.log, index=0, path='test_path', 
iface="")
Line 1272: testvm = TestingVm(fake.Domain())
Line 1273: 
Line 1274: testvm._dom = FakeVmDom(self.DRIVE_XML)
Line 1275: testvm._waitForDriveRemoval(drive)
> We can count the sleeps in FakeVmDom
Much much better!
Line 1276: 
Line 1277: @MonkeyPatch(config, 'getint', lambda x, y: 0)
Line 1278: def test_wait_for_drive_removal_removed_on_x_check(self):
Line 1279: drive = Drive({}, log=self.log, index=0, path='test_path', 
iface="")


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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]: vm: unit test for vm._waitForDriveRemoval

2015-11-24 Thread mmirecki

Marcin Mirecki has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 3:

(13 comments)

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

Line 145: 
Line 146: ('hotunplug_timeout', '30',
Line 147: 'Time to wait (in seconds) for a VM to detach its disk'),
Line 148: 
Line 149: ('hotunplug_sleep_time', '1',
> Maybe hotunplug_check_interval?
Done
Line 150: 'Sleep time between consecutive checks for VM device 
detach'),
Line 151: 
Line 152: ('vm_watermark_interval', '2',
Line 153: 'How often should we check drive watermark on block 
storage for '


Line 146: ('hotunplug_timeout', '30',
Line 147: 'Time to wait (in seconds) for a VM to detach its disk'),
Line 148: 
Line 149: ('hotunplug_sleep_time', '1',
Line 150: 'Sleep time between consecutive checks for VM device 
detach'),
> Maybe "Time to wait (in seconds) between consecutive checks for device remo
Done
Line 151: 
Line 152: ('vm_watermark_interval', '2',
Line 153: 'How often should we check drive watermark on block 
storage for '
Line 154: 'automatic extension of thin provisioned volumes 
(seconds).'),


https://gerrit.ovirt.org/#/c/48880/3/tests/vmTests.py
File tests/vmTests.py:

Line 1005: 
Line 1006: DRIVE_XML = """
Line 1007: 
Line 1008: 
Line 1009: """
> This is used only by the wait for removal tests, should be in another class
Done
Line 1010: 
Line 1011: @MonkeyPatch(libvirtconnection, 'get', lambda x: 
fake.Connection())
Line 1012: @permutations([[define.NORMAL], [define.ERROR]])
Line 1013: def testTimeOffsetNotPresentByDefault(self, exitCode):


Line 1257: with fake.VM() as testvm:
Line 1258: testvm._dom = fake.Domain(vmId='testvm')
Line 1259: 
self.assertFalse(response.is_error(testvm.acpiShutdown()))
Line 1260: 
Line 1261: @MonkeyPatch(config, 'getint', lambda x, y: 0)
> Nice hack, but too dirty :-)
Done
Line 1262: def test_wait_for_drive_removal_timeout(self):
Line 1263: drive = Drive({}, log=self.log, index=0, path='test_path', 
iface="")
Line 1264: testvm = TestingVm(fake.Domain())
Line 1265: 


Line 1262: def test_wait_for_drive_removal_timeout(self):
Line 1263: drive = Drive({}, log=self.log, index=0, path='test_path', 
iface="")
Line 1264: testvm = TestingVm(fake.Domain())
Line 1265: 
Line 1266: testvm._dom = FakeVmDom(self.DRIVE_XML, 
times_to_return_matching=99)
> Why not create the TestingVm with FakeVmDom() ?
Done
Line 1267: self.assertRaises(HotunplugTimeout, 
testvm._waitForDriveRemoval, drive)
Line 1268: 
Line 1269: @MonkeyPatch(config, 'getint', lambda x, y: 0)
Line 1270: def test_wait_for_drive_removal_removed_on_first_check(self):


Line 1263: drive = Drive({}, log=self.log, index=0, path='test_path', 
iface="")
Line 1264: testvm = TestingVm(fake.Domain())
Line 1265: 
Line 1266: testvm._dom = FakeVmDom(self.DRIVE_XML, 
times_to_return_matching=99)
Line 1267: self.assertRaises(HotunplugTimeout, 
testvm._waitForDriveRemoval, drive)
> To test this, it is enough to do couple of loops. We can override the timeo
Done
Line 1268: 
Line 1269: @MonkeyPatch(config, 'getint', lambda x, y: 0)
Line 1270: def test_wait_for_drive_removal_removed_on_first_check(self):
Line 1271: drive = Drive({}, log=self.log, index=0, path='test_path', 
iface="")


Line 1271: drive = Drive({}, log=self.log, index=0, path='test_path', 
iface="")
Line 1272: testvm = TestingVm(fake.Domain())
Line 1273: 
Line 1274: testvm._dom = FakeVmDom(self.DRIVE_XML)
Line 1275: testvm._waitForDriveRemoval(drive)
> Nice!
We can count the sleeps in FakeVmDom
Adding:
  self.assertEqual(testvm._dom._result_count, 0)
which check how many times we have waited.
Line 1276: 
Line 1277: @MonkeyPatch(config, 'getint', lambda x, y: 0)
Line 1278: def test_wait_for_drive_removal_removed_on_x_check(self):
Line 1279: drive = Drive({}, log=self.log, index=0, path='test_path', 
iface="")


Line 1280: testvm = TestingVm(fake.Domain())
Line 1281: 
Line 1282: testvm._dom = FakeVmDom(self.DRIVE_XML, 
times_to_return_matching=2)
Line 1283: testvm._waitForDriveRemoval(drive)
Line 1284: self.assertEqual(testvm._dom._result_count, 2)
> Like the previous test, with timeout=0.5 (sometimes we run on overloaded sl
Increasing the timeout to 1s.
The 1s should never be reached, as it should exit after 2 matching responses 
from dom.
Line 1285: 
Line 1286: 
Line 1287: class FakeVmDom(object):
Line 1288: 


Line 1285: 
Line 1286: 
Line 1287: class FakeVmDom(object):
Line 1288: 
Line 1289: def __init__(self, device_xml, times_to_return_matching=0):
> This is little

Change in vdsm[master]: vm: unit test for vm._waitForDriveRemoval

2015-11-24 Thread automation

gerrit-hooks has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 4:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: unit test for vm._waitForDriveRemoval

2015-11-24 Thread fromani

Francesco Romani has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 4: Code-Review+1

(1 comment)

Kudos for the nice work and for the updates!
quick review, looks nice. Will do deeper review later

https://gerrit.ovirt.org/#/c/48880/4/vdsm/virt/vmdevices/storage.py
File vdsm/virt/vmdevices/storage.py:

Line 428: """
Line 429: Returns xpath to the device in libvirt dom xml
Line 430: The path is relative to the root element
Line 431: """
Line 432: source_key = {DISK_TYPE.FILE: 'file',
minor: I'd suggest to use

  source_key = {
DISK_TYPE.FILE: 'file',
...
  }
Line 433:   DISK_TYPE.BLOCK: 'dev',
Line 434:   DISK_TYPE.NETWORK: 'name'}
Line 435: return "./devices/disk/source[@%s='%s']" \
Line 436:% (source_key[self.diskType], self.path)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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]: vm: unit test for vm._waitForDriveRemoval

2015-11-23 Thread nsoffer

Nir Soffer has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 3:

(2 comments)

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

Line 145: 
Line 146: ('hotunplug_timeout', '30',
Line 147: 'Time to wait (in seconds) for a VM to detach its disk'),
Line 148: 
Line 149: ('hotunplug_sleep_time', '1',
Maybe hotunplug_check_interval?

Sleep is the current way we implement the check. Tomorrow we can use timer 
instead. The name should describe what we do, not how we do it.
Line 150: 'Sleep time between consecutive checks for VM device 
detach'),
Line 151: 
Line 152: ('vm_watermark_interval', '2',
Line 153: 'How often should we check drive watermark on block 
storage for '


Line 146: ('hotunplug_timeout', '30',
Line 147: 'Time to wait (in seconds) for a VM to detach its disk'),
Line 148: 
Line 149: ('hotunplug_sleep_time', '1',
Line 150: 'Sleep time between consecutive checks for VM device 
detach'),
Maybe "Time to wait (in seconds) between consecutive checks for device removal"
Line 151: 
Line 152: ('vm_watermark_interval', '2',
Line 153: 'How often should we check drive watermark on block 
storage for '
Line 154: 'automatic extension of thin provisioned volumes 
(seconds).'),


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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]: vm: unit test for vm._waitForDriveRemoval

2015-11-23 Thread nsoffer

Nir Soffer has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 3:

(13 comments)

https://gerrit.ovirt.org/#/c/48880/3/tests/vmTests.py
File tests/vmTests.py:

Line 1247
Line 1248
Line 1249
Line 1250
Line 1251
The next tests can be in another test class, as they are very different from 
the tests above. There is no value in having these different tests in the same 
test class.

This will make the constants used by these tests closer to the place we used 
them.

This will also make the test name much nicer:

class TestWaitForRemoval(VdsmTestCase):
def test_timeout(self):
...


Line 1005: 
Line 1006: DRIVE_XML = """
Line 1007: 
Line 1008: 
Line 1009: """
This is used only by the wait for removal tests, should be in another class 
with the relevant tests.
Line 1010: 
Line 1011: @MonkeyPatch(libvirtconnection, 'get', lambda x: 
fake.Connection())
Line 1012: @permutations([[define.NORMAL], [define.ERROR]])
Line 1013: def testTimeOffsetNotPresentByDefault(self, exitCode):


Line 1257: with fake.VM() as testvm:
Line 1258: testvm._dom = fake.Domain(vmId='testvm')
Line 1259: 
self.assertFalse(response.is_error(testvm.acpiShutdown()))
Line 1260: 
Line 1261: @MonkeyPatch(config, 'getint', lambda x, y: 0)
Nice hack, but too dirty :-)

First setting both timeout and sleep to 0 will make it hard to test timeouts, 
since the code will do many unneeded loops, and the results may differ on 
different machines.

Please use testlib.make_config() new config with the value needed for the test, 
and monkey path vm.config.

@MonkeyPatch(vm, "config", make_config([
("section", "key", value),
...
]))
def test_foo():
test code assuming these config values

This make it very clear what values you need to monkey patch, while the lambda 
hack leave the reader wondering, what are you trying to do.
Line 1262: def test_wait_for_drive_removal_timeout(self):
Line 1263: drive = Drive({}, log=self.log, index=0, path='test_path', 
iface="")
Line 1264: testvm = TestingVm(fake.Domain())
Line 1265: 


Line 1262: def test_wait_for_drive_removal_timeout(self):
Line 1263: drive = Drive({}, log=self.log, index=0, path='test_path', 
iface="")
Line 1264: testvm = TestingVm(fake.Domain())
Line 1265: 
Line 1266: testvm._dom = FakeVmDom(self.DRIVE_XML, 
times_to_return_matching=99)
Why not create the TestingVm with FakeVmDom() ?
Line 1267: self.assertRaises(HotunplugTimeout, 
testvm._waitForDriveRemoval, drive)
Line 1268: 
Line 1269: @MonkeyPatch(config, 'getint', lambda x, y: 0)
Line 1270: def test_wait_for_drive_removal_removed_on_first_check(self):


Line 1263: drive = Drive({}, log=self.log, index=0, path='test_path', 
iface="")
Line 1264: testvm = TestingVm(fake.Domain())
Line 1265: 
Line 1266: testvm._dom = FakeVmDom(self.DRIVE_XML, 
times_to_return_matching=99)
Line 1267: self.assertRaises(HotunplugTimeout, 
testvm._waitForDriveRemoval, drive)
To test this, it is enough to do couple of loops. We can override the timeout 
to 0.5, and the sleep to 0.1, and this will fail after 5 tries, no need to 
return 99 times same xml.
Line 1268: 
Line 1269: @MonkeyPatch(config, 'getint', lambda x, y: 0)
Line 1270: def test_wait_for_drive_removal_removed_on_first_check(self):
Line 1271: drive = Drive({}, log=self.log, index=0, path='test_path', 
iface="")


Line 1271: drive = Drive({}, log=self.log, index=0, path='test_path', 
iface="")
Line 1272: testvm = TestingVm(fake.Domain())
Line 1273: 
Line 1274: testvm._dom = FakeVmDom(self.DRIVE_XML)
Line 1275: testvm._waitForDriveRemoval(drive)
Nice!

Here we should test with sleep=0.1 - if we sleep even once, the test will fail 
with timeout.

I would also add a comment, "Should not time out" to help the poor guy looking 
at this six month from now and wondering what are we testing.

Or try block, failing the test if Timeout was raised.
Line 1276: 
Line 1277: @MonkeyPatch(config, 'getint', lambda x, y: 0)
Line 1278: def test_wait_for_drive_removal_removed_on_x_check(self):
Line 1279: drive = Drive({}, log=self.log, index=0, path='test_path', 
iface="")


Line 1280: testvm = TestingVm(fake.Domain())
Line 1281: 
Line 1282: testvm._dom = FakeVmDom(self.DRIVE_XML, 
times_to_return_matching=2)
Line 1283: testvm._waitForDriveRemoval(drive)
Line 1284: self.assertEqual(testvm._dom._result_count, 2)
Like the previous test, with timeout=0.5 (sometimes we run on overloaded 
slaves) and sleep=0.1.
Line 1285: 
Line 1286: 
Line 1287: class FakeVmDom(object):
Line 1288: 


Line 1285: 
Line 1286: 
Line 1287: class FakeVmDom(object):
Line 1288: 
Line 1289: def __init__(self, device_xml,

Change in vdsm[master]: vm: unit test for vm._waitForDriveRemoval

2015-11-23 Thread automation

gerrit-hooks has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 2:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: unit test for vm._waitForDriveRemoval

2015-11-23 Thread mmirecki

Marcin Mirecki has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 1:

(8 comments)

https://gerrit.ovirt.org/#/c/48880/1/tests/vmTests.py
File tests/vmTests.py:

Line 1256: pass
Line 1257: drive = Drive()
Line 1258: drive.name = ''
Line 1259: drive.diskType = DISK_TYPE.FILE
Line 1260: drive.path = 'test_path'
> Why not use a real drive object? See vmStorageTests.py to see how to use re
Done
Line 1261: with fake.VM() as testvm:
Line 1262: testvm._dom = FakeVmDom(999)
Line 1263: testvm._hotunplug_timeout = 1
Line 1264: try:


Line 1257: drive = Drive()
Line 1258: drive.name = ''
Line 1259: drive.diskType = DISK_TYPE.FILE
Line 1260: drive.path = 'test_path'
Line 1261: with fake.VM() as testvm:
> fake.VM is too complex, we cannot use such monster in the tests. I think a 
Done
Line 1262: testvm._dom = FakeVmDom(999)
Line 1263: testvm._hotunplug_timeout = 1
Line 1264: try:
Line 1265: testvm._waitForDriveRemoval(drive)


Line 1261: with fake.VM() as testvm:
Line 1262: testvm._dom = FakeVmDom(999)
Line 1263: testvm._hotunplug_timeout = 1
Line 1264: try:
Line 1265: testvm._waitForDriveRemoval(drive)
> Yes, but 2 seconds is too much.
With both values changed to 0, utils.monotonic_time() >= deadline is true after 
about calling sleep(0) about 2 to 4 times.
Line 1266: raise BaseException("HotunplugTimeout should be 
raised")
Line 1267: except HotunplugTimeout:
Line 1268: pass
Line 1269: 


Line 1264: try:
Line 1265: testvm._waitForDriveRemoval(drive)
Line 1266: raise BaseException("HotunplugTimeout should be 
raised")
Line 1267: except HotunplugTimeout:
Line 1268: pass
> Use self.assertRaises instead.
Done
Line 1269: 
Line 1270: testvm._dom = FakeVmDom()
Line 1271: testvm._hotunplug_timeout = 1
Line 1272: testvm._wait_for_sleep = self._wait_for_sleep


Line 1265: testvm._waitForDriveRemoval(drive)
Line 1266: raise BaseException("HotunplugTimeout should be 
raised")
Line 1267: except HotunplugTimeout:
Line 1268: pass
Line 1269: 
> The code bellow should be in another test - we want to test one scenario pe
Done
Line 1270: testvm._dom = FakeVmDom()
Line 1271: testvm._hotunplug_timeout = 1
Line 1272: testvm._wait_for_sleep = self._wait_for_sleep
Line 1273: testvm._waitForDriveRemoval(drive)


Line 1298: 
Line 1299:   
Line 1300:  
Line 1301: """
Line 1302: return ''
> This is good only for drive test, but we need it also for the nic in the ne
Done
Line 1303: 
Line 1304: 
Line 1305: class ChangingSchedulerDomain(object):
Line 1306: 


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

Line 325: self._vcpuTuneInfo = {}
Line 326: self._numaInfo = {}
Line 327: self._vmJobs = None
Line 328: self._clientPort = ''
Line 329: self._hotunplug_timeout = config.getint('vars', 
'hotunplug_timeout')
> Vm is too big, we cannot add more state.
Done
Line 330: 
Line 331: def _get_lastStatus(self):
Line 332: # note that we don't use _statusLock here. One of the reasons 
is the
Line 333: # non-obvious recursive locking in the following flow:


Line 2689: if utils.monotonic_time() > deadline:
Line 2690: raise HotunplugTimeout("Timeout detaching drive 
%s"
Line 2691:% drive.name)
Line 2692: 
Line 2693: def _wait_for_sleep(self):
> Yes, but again VM is too big, we cannot add such methods for testing. The b
Done
Line 2694: time.sleep(1)
Line 2695: 
Line 2696: def _isDriveAttached(self, drive):
Line 2697: root = ET.fromstring(self._dom.XMLDesc(0))


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Vinzenz Feenstra 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list

Change in vdsm[master]: vm: unit test for vm._waitForDriveRemoval

2015-11-23 Thread automation

gerrit-hooks has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 3:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: unit test for vm._waitForDriveRemoval

2015-11-21 Thread mmirecki

Marcin Mirecki has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 1:

(3 comments)

https://gerrit.ovirt.org/#/c/48880/1/tests/vmTests.py
File tests/vmTests.py:

Line 1255:  
Temporary class. Removed two patches later: https://gerrit.ovirt.org/#/c/48881/


Line 1261: with fake.VM() as testvm:
Line 1262: testvm._dom = FakeVmDom(999)
Line 1263: testvm._hotunplug_timeout = 1
Line 1264: try:
Line 1265: testvm._waitForDriveRemoval(drive)
This will wait about 2 seconds.
Is this ok? Do we allow sleeps in tests?
Line 1266: raise BaseException("HotunplugTimeout should be 
raised")
Line 1267: except HotunplugTimeout:
Line 1268: pass
Line 1269: 


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

Line 2689: if utils.monotonic_time() > deadline:
Line 2690: raise HotunplugTimeout("Timeout detaching drive 
%s"
Line 2691:% drive.name)
Line 2692: 
Line 2693: def _wait_for_sleep(self):
Needed to remove the sleep time in unit tests.
Line 2694: time.sleep(1)
Line 2695: 
Line 2696: def _isDriveAttached(self, drive):
Line 2697: root = ET.fromstring(self._dom.XMLDesc(0))


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: unit test for vm._waitForDriveRemoval

2015-11-21 Thread automation

gerrit-hooks has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 1:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
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: unit test for vm._waitForDriveRemoval

2015-11-21 Thread mmirecki

Marcin Mirecki has uploaded a new change for review.

Change subject: vm: unit test for vm._waitForDriveRemoval
..

vm: unit test for vm._waitForDriveRemoval

Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Signed-off-by: Marcin Mirecki 
---
M tests/vmTests.py
M vdsm/virt/vm.py
2 files changed, 58 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/80/48880/1

diff --git a/tests/vmTests.py b/tests/vmTests.py
index a7d5c63..f8460cd 100644
--- a/tests/vmTests.py
+++ b/tests/vmTests.py
@@ -31,6 +31,7 @@
 from nose.plugins.skip import SkipTest
 
 from virt import vm
+from virt.vm import HotunplugTimeout
 from virt import vmdevices
 from virt import vmexitreason
 from virt.vmdevices import hwclass
@@ -43,6 +44,7 @@
 from vdsm import define
 from vdsm import password
 from vdsm import response
+from virt.vmdevices.storage import DISK_TYPE
 from testlib import VdsmTestCase as TestCaseBase
 from testlib import permutations, expandPermutations
 from testlib import find_xml_element
@@ -1249,6 +1251,56 @@
 testvm._dom = fake.Domain(vmId='testvm')
 self.assertFalse(response.is_error(testvm.acpiShutdown()))
 
+def test_wait_for_drive_removal(self):
+class Drive(object):
+pass
+drive = Drive()
+drive.name = ''
+drive.diskType = DISK_TYPE.FILE
+drive.path = 'test_path'
+with fake.VM() as testvm:
+testvm._dom = FakeVmDom(999)
+testvm._hotunplug_timeout = 1
+try:
+testvm._waitForDriveRemoval(drive)
+raise BaseException("HotunplugTimeout should be raised")
+except HotunplugTimeout:
+pass
+
+testvm._dom = FakeVmDom()
+testvm._hotunplug_timeout = 1
+testvm._wait_for_sleep = self._wait_for_sleep
+testvm._waitForDriveRemoval(drive)
+
+testvm._dom = FakeVmDom(times_to_return_matching=2)
+testvm._waitForDriveRemoval(drive)
+self.assertEqual(testvm._dom._result_count, 2)
+
+def _wait_for_sleep(self):
+pass
+
+
+class FakeVmDom(object):
+
+def __init__(self, times_to_return_matching=0):
+self._times_to_return_matching = times_to_return_matching
+self._result_count = 0
+
+def XMLDesc(self, anything):
+if self._result_count < self._times_to_return_matching:
+self._result_count += 1
+return """
+
+  
+
+  
+  
+
+  
+ 
+"""
+return ''
+
 
 class ChangingSchedulerDomain(object):
 
diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py
index a5eead1..6744781 100644
--- a/vdsm/virt/vm.py
+++ b/vdsm/virt/vm.py
@@ -326,6 +326,7 @@
 self._numaInfo = {}
 self._vmJobs = None
 self._clientPort = ''
+self._hotunplug_timeout = config.getint('vars', 'hotunplug_timeout')
 
 def _get_lastStatus(self):
 # note that we don't use _statusLock here. One of the reasons is the
@@ -2682,13 +2683,16 @@
 self.log.debug("Waiting for hotunplug to finish")
 with utils.stopwatch("Hotunplug disk %s" % drive.name):
 deadline = (utils.monotonic_time() +
-config.getint('vars', 'hotunplug_timeout'))
+self._hotunplug_timeout)
 while self._isDriveAttached(drive):
-time.sleep(1)
+self._wait_for_sleep()
 if utils.monotonic_time() > deadline:
 raise HotunplugTimeout("Timeout detaching drive %s"
% drive.name)
 
+def _wait_for_sleep(self):
+time.sleep(1)
+
 def _isDriveAttached(self, drive):
 root = ET.fromstring(self._dom.XMLDesc(0))
 source_key = {DISK_TYPE.FILE: 'file',


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

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

Change in vdsm[master]: vm: unit test for vm._waitForDriveRemoval

2015-11-21 Thread nsoffer

Nir Soffer has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 1:

General note - please use topic branch for all 3 patches, to make it easier to 
track them.

The easiest way is to create a topic branch and upload the changes using git 
review.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: unit test for vm._waitForDriveRemoval

2015-11-21 Thread nsoffer

Nir Soffer has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
..


Patch Set 1:

(8 comments)

Please work with Francesco on this, this is his domain.

https://gerrit.ovirt.org/#/c/48880/1/tests/vmTests.py
File tests/vmTests.py:

Line 1256: pass
Line 1257: drive = Drive()
Line 1258: drive.name = ''
Line 1259: drive.diskType = DISK_TYPE.FILE
Line 1260: drive.path = 'test_path'
Why not use a real drive object? See vmStorageTests.py to see how to use real 
Drive object in tests.
Line 1261: with fake.VM() as testvm:
Line 1262: testvm._dom = FakeVmDom(999)
Line 1263: testvm._hotunplug_timeout = 1
Line 1264: try:


Line 1257: drive = Drive()
Line 1258: drive.name = ''
Line 1259: drive.diskType = DISK_TYPE.FILE
Line 1260: drive.path = 'test_path'
Line 1261: with fake.VM() as testvm:
fake.VM is too complex, we cannot use such monster in the tests. I think a 
better way to test is TestingVm - see the FreezingTests for example.
Line 1262: testvm._dom = FakeVmDom(999)
Line 1263: testvm._hotunplug_timeout = 1
Line 1264: try:
Line 1265: testvm._waitForDriveRemoval(drive)


Line 1261: with fake.VM() as testvm:
Line 1262: testvm._dom = FakeVmDom(999)
Line 1263: testvm._hotunplug_timeout = 1
Line 1264: try:
Line 1265: testvm._waitForDriveRemoval(drive)
> This will wait about 2 seconds.
Yes, but 2 seconds is too much.

To check for timeout, we need:
 1. check the xml, drive still there
 2. sleep 0 seconds
 3. check the deadline - deadline passed, raise

So we want override the sleep time and the deadline to 0

We need to change the logic so we raise when utils.monotonic_time() >= 
deadline. For real waits, it does not matter, but it make it easier to test.
Line 1266: raise BaseException("HotunplugTimeout should be 
raised")
Line 1267: except HotunplugTimeout:
Line 1268: pass
Line 1269: 


Line 1264: try:
Line 1265: testvm._waitForDriveRemoval(drive)
Line 1266: raise BaseException("HotunplugTimeout should be 
raised")
Line 1267: except HotunplugTimeout:
Line 1268: pass
Use self.assertRaises instead.
Line 1269: 
Line 1270: testvm._dom = FakeVmDom()
Line 1271: testvm._hotunplug_timeout = 1
Line 1272: testvm._wait_for_sleep = self._wait_for_sleep


Line 1265: testvm._waitForDriveRemoval(drive)
Line 1266: raise BaseException("HotunplugTimeout should be 
raised")
Line 1267: except HotunplugTimeout:
Line 1268: pass
Line 1269: 
The code bellow should be in another test - we want to test one scenario per 
test.

I think we need 3 tests:
- timeout
- removed on the first check
- removed on x check (just before the timeout)
Line 1270: testvm._dom = FakeVmDom()
Line 1271: testvm._hotunplug_timeout = 1
Line 1272: testvm._wait_for_sleep = self._wait_for_sleep
Line 1273: testvm._waitForDriveRemoval(drive)


Line 1298: 
Line 1299:   
Line 1300:  
Line 1301: """
Line 1302: return ''
This is good only for drive test, but we need it also for the nic in the next 
version, so we need something more generic.

This object should:
- return always xml with a device
- return xml with a device x times, then return xml without a device

The base xml can be hardcoded in this object. The device can be xml we add into 
the base xml, and remove after x times xml was fetched

Something like:

dom = FakeDomain(device_xml, count)
Line 1303: 
Line 1304: 
Line 1305: class ChangingSchedulerDomain(object):
Line 1306: 


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

Line 325: self._vcpuTuneInfo = {}
Line 326: self._numaInfo = {}
Line 327: self._vmJobs = None
Line 328: self._clientPort = ''
Line 329: self._hotunplug_timeout = config.getint('vars', 
'hotunplug_timeout')
Vm is too big, we cannot add more state.

To modify config, use testlib.fake_config and monkey patch vm.config.
Line 330: 
Line 331: def _get_lastStatus(self):
Line 332: # note that we don't use _statusLock here. One of the reasons 
is the
Line 333: # non-obvious recursive locking in the following flow:


Line 2689: if utils.monotonic_time() > deadline:
Line 2690: raise HotunplugTimeout("Timeout detaching drive 
%s"
Line 2691:% drive.name)
Line 2692: 
Line 2693: def _wait_for_sleep(self):
> Needed to remove the sleep time in unit tests.
Yes, but again VM is too

49 matches

Mail list logo