Change in vdsm[master]: vm: check operation result for vm nic hotunplug

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 20:

Francesco, can you approve this?

I don't think we need the change your requested, but we have a another patch to 
discuss it. I would like to merge this important change with hits tests.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 20
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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]: vm: check operation result for vm nic hotunplug

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 20:

* #1134256::Update tracker: OK
* #1199782::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1134256::OK, public bug
* Check Public Bug::#1199782::OK, public bug
* Check Product::#1134256::OK, Correct product Red Hat Enterprise 
Virtualization Manager
* Check Product::#1199782::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/48473
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 20
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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]: vm: check operation result for vm nic hotunplug

[mmirecki]

Marcin Mirecki has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 20:

xpath property added to base class in:
https://gerrit.ovirt.org/49554

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 20
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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]: vm: check operation result for vm nic hotunplug

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 21:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 21
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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]: vm: check operation result for vm nic hotunplug

[nsoffer]

Nir Soffer has submitted this change and it was merged.

Change subject: vm: check operation result for vm nic hotunplug
..


vm: check operation result for vm nic hotunplug

After detaching a device we need to verify that this device
has actually been detached.

This mechanism was already implemented for disks. This
patch generalizes the disk waiting mechanism so that it
can also be applied to nics.

This patch is similar to:
https://gerrit.ovirt.org/#/c/45138
https://bugzilla.redhat.com/1044466

Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Signed-off-by: Marcin Mirecki 
Bug-Url: https://bugzilla.redhat.com/1134256
Bug-Url: https://bugzilla.redhat.com/1199782
Reviewed-on: https://gerrit.ovirt.org/48473
Reviewed-by: Nir Soffer 
Continuous-Integration: Jenkins CI
Reviewed-by: Francesco Romani 
---
M tests/vmTests.py
M vdsm/virt/vm.py
M vdsm/virt/vmdevices/network.py
M vdsm/virt/vmdevices/storage.py
4 files changed, 43 insertions(+), 20 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/48473
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 21
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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]: vm: check operation result for vm nic hotunplug

[fromani]

Francesco Romani has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 20: Code-Review+2

I see Marcin posted https://gerrit.ovirt.org/#/c/49554/1. I'd be happier with a 
reshuffle, but no big deal, we can now proceed.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 20
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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]: vm: check operation result for vm nic hotunplug

[fromani]

Francesco Romani has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 19: Code-Review+2

even better, thanks

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 19
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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]: vm: check operation result for vm nic hotunplug

[fromani]

Francesco Romani has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 19: Code-Review-1

too quick. I'm still happy with this patch per se, but I want to avoid 
attributes missing from the base class.
Marcin, please let's do one final step:
- post a simple patch adding xpath property to base class (devices.Core), doing 
the right thing (e.g. raise NotImplementedError like is_attached_to in the next 
patch)
- reshuffle this series to have this patch depend on the new one added

This way you can leave this patch unchanged, no score is lost and code is more 
consistent and less surprising.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 19
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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]: vm: check operation result for vm nic hotunplug

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 18:

* #1134256::Update tracker: OK
* #1199782::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1134256::OK, public bug
* Check Public Bug::#1199782::OK, public bug
* Check Product::#1134256::OK, Correct product Red Hat Enterprise 
Virtualization Manager
* Check Product::#1199782::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/48473
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 18
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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]: vm: check operation result for vm nic hotunplug

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 19:

* #1134256::Update tracker: OK
* #1199782::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1134256::OK, public bug
* Check Public Bug::#1199782::OK, public bug
* Check Product::#1134256::OK, Correct product Red Hat Enterprise 
Virtualization Manager
* Check Product::#1199782::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/48473
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 19
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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]: vm: check operation result for vm nic hotunplug

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 17: Code-Review+2 Continuous-Integration+1

CI failures not relevant (mkimage.mkIsoFs)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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]: vm: check operation result for vm nic hotunplug

[mmirecki]

Marcin Mirecki has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 17: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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]: vm: check operation result for vm nic hotunplug

[mmirecki]

Marcin Mirecki has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 16:

(2 comments)

https://gerrit.ovirt.org/#/c/48473/16//COMMIT_MSG
Commit Message:

Line 9:   
> nit: 2 spaces
Done


https://gerrit.ovirt.org/#/c/48473/16/vdsm/virt/vmdevices/network.py
File vdsm/virt/vmdevices/network.py:

Line 194: The path is relative to the root element
Line 195: """
Line 196: return "./devices/interface/mac[@address='%s']" % self.macAddr
Line 197: 
Line 198: def _make_name(self):
> Looks like the failing tests are the new waitForDeviceRemoval tests, since 
Removed from this patch completely.
This should belong to the last patch with the nic test (moved initialization to 
test there)
Line 199: """
Line 200: Creates an interface name if one was not set.
Line 201: There is no guarantee that the interface name will be set in
Line 202: constructor, so we need this to make sure we have a value for 
name.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: check operation result for vm nic hotunplug

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 17:

* #1134256::Update tracker: OK
* #1199782::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1134256::OK, public bug
* Check Public Bug::#1199782::OK, public bug
* Check Product::#1134256::OK, Correct product Red Hat Enterprise 
Virtualization Manager
* Check Product::#1199782::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/48473
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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]: vm: check operation result for vm nic hotunplug

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 16: -Code-Review

(1 comment)

https://gerrit.ovirt.org/#/c/48473/16/vdsm/virt/vmdevices/network.py
File vdsm/virt/vmdevices/network.py:

Line 194: The path is relative to the root element
Line 195: """
Line 196: return "./devices/interface/mac[@address='%s']" % self.macAddr
Line 197: 
Line 198: def _make_name(self):
> We should not introduce an obscure complexity into the already-overcomplex 
Looks like the failing tests are the new waitForDeviceRemoval tests, since the 
code is assuming that an interface has a name, but it does not have one in the 
tests since the class does not initialize its "name" attribute.

Marcin, why not initialize the name in the test, instead of adding this method?
Line 199: """
Line 200: Creates an interface name if one was not set.
Line 201: There is no guarantee that the interface name will be set in
Line 202: constructor, so we need this to make sure we have a value for 
name.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: check operation result for vm nic hotunplug

[ibarkan]

Ido Barkan has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 16:

(1 comment)

https://gerrit.ovirt.org/#/c/48473/16//COMMIT_MSG
Commit Message:

Line 9:   
nit: 2 spaces


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: check operation result for vm nic hotunplug

[danken]

Dan Kenigsberg has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 16: Code-Review-1

(1 comment)

https://gerrit.ovirt.org/#/c/48473/16/vdsm/virt/vmdevices/network.py
File vdsm/virt/vmdevices/network.py:

Line 194: The path is relative to the root element
Line 195: """
Line 196: return "./devices/interface/mac[@address='%s']" % self.macAddr
Line 197: 
Line 198: def _make_name(self):
> OK, so we should fix the tests, in a later patch.
We should not introduce an obscure complexity into the already-overcomplex 
vmcreate flow just because a test does not provide libvirt's alias to the 
Interface object.

Which tests fail due to this? I'd rather have them fixed now, or at the very 
least - marked with a #TODO.
Line 199: """
Line 200: Creates an interface name if one was not set.
Line 201: There is no guarantee that the interface name will be set in
Line 202: constructor, so we need this to make sure we have a value for 
name.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: check operation result for vm nic hotunplug

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 16:

Waiting for Francesco approval.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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]: vm: check operation result for vm nic hotunplug

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 16: Code-Review+2

Marcin, can you verify?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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]: vm: check operation result for vm nic hotunplug

[fromani]

Francesco Romani has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 16:

(1 comment)

https://gerrit.ovirt.org/#/c/48473/16/vdsm/virt/vmdevices/network.py
File vdsm/virt/vmdevices/network.py:

Line 194: The path is relative to the root element
Line 195: """
Line 196: return "./devices/interface/mac[@address='%s']" % self.macAddr
Line 197: 
Line 198: def _make_name(self):
> During normal execution:
OK, so we should fix the tests, in a later patch.
Line 199: """
Line 200: Creates an interface name if one was not set.
Line 201: There is no guarantee that the interface name will be set in
Line 202: constructor, so we need this to make sure we have a value for 
name.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: check operation result for vm nic hotunplug

[fromani]

Francesco Romani has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 16: Code-Review+2

(1 comment)

fine, but please look a question inside.

https://gerrit.ovirt.org/#/c/48473/16/vdsm/virt/vmdevices/network.py
File vdsm/virt/vmdevices/network.py:

Line 194: The path is relative to the root element
Line 195: """
Line 196: return "./devices/interface/mac[@address='%s']" % self.macAddr
Line 197: 
Line 198: def _make_name(self):
can you provide (later via mail is fine) examples of the flows on which 'name' 
is given and, on which 'name' is NOT given?

We should indeed fix this horrible design as soon as possible.
Line 199: """
Line 200: Creates an interface name if one was not set.
Line 201: There is no guarantee that the interface name will be set in
Line 202: constructor, so we need this to make sure we have a value for 
name.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: check operation result for vm nic hotunplug

[mmirecki]

Marcin Mirecki has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 16:

(1 comment)

https://gerrit.ovirt.org/#/c/48473/16/vdsm/virt/vmdevices/network.py
File vdsm/virt/vmdevices/network.py:

Line 194: The path is relative to the root element
Line 195: """
Line 196: return "./devices/interface/mac[@address='%s']" % self.macAddr
Line 197: 
Line 198: def _make_name(self):
> can you provide (later via mail is fine) examples of the flows on which 'na
During normal execution:
When we start a vm:
vm
_run
_domDependentInit
_getUnderlyingVmDevicesInfo
_getUnderlyingNetworkInterfaceInfo
we assign the name: nic.name = name 
We don't do this in tests
Line 199: """
Line 200: Creates an interface name if one was not set.
Line 201: There is no guarantee that the interface name will be set in
Line 202: constructor, so we need this to make sure we have a value for 
name.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: check operation result for vm nic hotunplug

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 16:

* #1134256::Update tracker: OK
* #1199782::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1134256::OK, public bug
* Check Public Bug::#1199782::OK, public bug
* Check Product::#1134256::OK, Correct product Red Hat Enterprise 
Virtualization Manager
* Check Product::#1199782::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/48473
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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]: vm: check operation result for vm nic hotunplug

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 15: Code-Review-1

(2 comments)

Tow minor issues.

https://gerrit.ovirt.org/#/c/48473/15/vdsm/virt/vmdevices/network.py
File vdsm/virt/vmdevices/network.py:

Line 203: The value created here can still be overriden at a later stage
Line 204: (check vm.py for example)
Line 205: """
Line 206: parts = (getattr(self, "type", ""), getattr(self, "macAddr", 
""))
Line 207: return " " .join(p for p in parts if p)
Unneeded space sneaked int:

" ".join(...


https://gerrit.ovirt.org/#/c/48473/15/vdsm/virt/vmdevices/storage.py
File vdsm/virt/vmdevices/storage.py:

Line 431: """
Line 432: source_key = {
Line 433: DISK_TYPE.FILE: 'file',
Line 434: DISK_TYPE.BLOCK: 'dev',
Line 435: DISK_TYPE.NETWORK: 'name'
I like the previous format, but this is ok.

But if you use this style - one line per item - each line should end with ",". 
This make it easy to move items or add new items - the diffs is always one line.
Line 436: }
Line 437: return ("./devices/disk/source[@%s='%s']" %
Line 438: (source_key[self.diskType], self.path))
Line 439: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: check operation result for vm nic hotunplug

[mmirecki]

Marcin Mirecki has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 13:

(1 comment)

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

Line 42: elif attr == 'network' and value == '':
Line 43: kwargs[attr] = DUMMY_BRIDGE
Line 44: super(Interface, self).__init__(conf, log, **kwargs)
Line 45: self.sndbufParam = False
Line 46: self.name = self._make_name()
> What if name is already set? you just overwritten it with the default value
Done
Line 47: self.is_hostdevice = self.device == hwclass.HOSTDEV
Line 48: self.vlanId = self.specParams.get('vlanid')
Line 49: self._customize()
Line 50: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
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: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: check operation result for vm nic hotunplug

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 15:

* #1134256::Update tracker: OK
* #1199782::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1134256::OK, public bug
* Check Public Bug::#1199782::OK, public bug
* Check Product::#1134256::OK, Correct product Red Hat Enterprise 
Virtualization Manager
* Check Product::#1199782::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/48473
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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]: vm: check operation result for vm nic hotunplug

[fromani]

Francesco Romani has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 12:

(1 comment)

https://gerrit.ovirt.org/#/c/48473/12/vdsm/virt/vmdevices/network.py
File vdsm/virt/vmdevices/network.py:

Line 193: The path is relative to the root element
Line 194: """
Line 195: return "./devices/interface/mac[@address='%s']" % self.macAddr
Line 196: 
Line 197: def _makeName(self):
could be a property (if not please explain why), see my comments in patchset #9
Line 198: name = ''
Line 199: if hasattr(self, 'type'):
Line 200: name += self.type + ' '
Line 201: if hasattr(self, 'macAddr'):


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
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: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: check operation result for vm nic hotunplug

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 9:

(2 comments)

https://gerrit.ovirt.org/#/c/48473/9/vdsm/virt/vmdevices/network.py
File vdsm/virt/vmdevices/network.py:

Line 193: The path is relative to the root element
Line 194: """
Line 195: return "./devices/interface/mac[@address='%s']" % self.macAddr
Line 196: 
Line 197: def _makeName(self):
> why not just make this a property?
I agree with Francesco, property is better.
Line 198: name = ''
Line 199: if hasattr(self, 'type'):
Line 200: name += self.type + ' '
Line 201: if hasattr(self, 'macAddr'):


Line 198: name = ''
Line 199: if hasattr(self, 'type'):
Line 200: name += self.type + ' '
Line 201: if hasattr(self, 'macAddr'):
Line 202: name += self.macAddr + ' '
This will create these funny names, which probably are not what you want:
- "type mac " instead of "type mac"
- "mac " instead of "mac"
- "type " instead of "type"

The generic solution is:

parts = (getattr(self, "type", ""), getattr(self, "macAddr", ""))
return " " .join(p for p in parts if p)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
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: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: check operation result for vm nic hotunplug

[mmirecki]

Marcin Mirecki has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 9:

(3 comments)

https://gerrit.ovirt.org/#/c/48473/9/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2691: while self._isDriveAttached(device):
Line 2692: time.sleep(sleep_time)
Line 2693: if utils.monotonic_time() > deadline:
Line 2694: raise HotunplugTimeout("Timeout detaching device 
%s"
Line 2695:% device.name)
> We support any device now?
We support drive + nic
The method name changes as well
(was in a separate patch for a while due to some misunderstandings)
Line 2696: 
Line 2697: def _isDriveAttached(self, device):
Line 2698: root = ET.fromstring(self._dom.XMLDesc(0))
Line 2699: return bool(root.findall(device.xpath))


https://gerrit.ovirt.org/#/c/48473/9/vdsm/virt/vmdevices/network.py
File vdsm/virt/vmdevices/network.py:

Line 193: The path is relative to the root element
Line 194: """
Line 195: return "./devices/interface/mac[@address='%s']" % self.macAddr
Line 196: 
Line 197: def _makeName(self):
> I agree with Francesco, property is better.
I agree too, but unfortunately not possible.
name is already used as an attribute of Interface, and is being also set 
externally in other places
Why I'm adding make_name is because name is not always set.
Line 198: name = ''
Line 199: if hasattr(self, 'type'):
Line 200: name += self.type + ' '
Line 201: if hasattr(self, 'macAddr'):


Line 198: name = ''
Line 199: if hasattr(self, 'type'):
Line 200: name += self.type + ' '
Line 201: if hasattr(self, 'macAddr'):
Line 202: name += self.macAddr + ' '
> This will create these funny names, which probably are not what you want:
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
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: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: check operation result for vm nic hotunplug

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 13:

* #1134256::Update tracker: OK
* #1199782::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1134256::OK, public bug
* Check Public Bug::#1199782::OK, public bug
* Check Product::#1134256::OK, Correct product Red Hat Enterprise 
Virtualization Manager
* Check Product::#1199782::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/48473
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
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: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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]: vm: check operation result for vm nic hotunplug

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 9:

(1 comment)

https://gerrit.ovirt.org/#/c/48473/9/vdsm/virt/vmdevices/network.py
File vdsm/virt/vmdevices/network.py:

Line 193: The path is relative to the root element
Line 194: """
Line 195: return "./devices/interface/mac[@address='%s']" % self.macAddr
Line 196: 
Line 197: def _makeName(self):
> I agree too, but unfortunately not possible.
So name is sometimes given as argument to __init__, sometimes is not set at 
all, and sometimes being modified by other code?

If we can fix this horrible design, I agree that we should have make_name for 
generating the name if it is missing.

Please add a docstring explaining this.
Line 198: name = ''
Line 199: if hasattr(self, 'type'):
Line 200: name += self.type + ' '
Line 201: if hasattr(self, 'macAddr'):


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
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: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: check operation result for vm nic hotunplug

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 13: Code-Review-1

(1 comment)

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

Line 42: elif attr == 'network' and value == '':
Line 43: kwargs[attr] = DUMMY_BRIDGE
Line 44: super(Interface, self).__init__(conf, log, **kwargs)
Line 45: self.sndbufParam = False
Line 46: self.name = self._make_name()
What if name is already set? you just overwritten it with the default value

This should be something like:

if not hasattr(self, 'name'):
self.name = self._make_name()

This will be good enough for now.

The real solution is to have a proper default value mechanism for all 
attributes that may be unspecified.
Line 47: self.is_hostdevice = self.device == hwclass.HOSTDEV
Line 48: self.vlanId = self.specParams.get('vlanid')
Line 49: self._customize()
Line 50: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
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: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: check operation result for vm nic hotunplug

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 13:

(1 comment)

https://gerrit.ovirt.org/#/c/48473/13/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)
Please replace the \ here with () - not in 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/48473
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
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: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: check operation result for vm nic hotunplug

[mmirecki]

Marcin Mirecki has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 13:

(2 comments)

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

Line 42: elif attr == 'network' and value == '':
Line 43: kwargs[attr] = DUMMY_BRIDGE
Line 44: super(Interface, self).__init__(conf, log, **kwargs)
Line 45: self.sndbufParam = False
Line 46: self.name = self._make_name()
> What if name is already set? you just overwritten it with the default value
Done
Line 47: self.is_hostdevice = self.device == hwclass.HOSTDEV
Line 48: self.vlanId = self.specParams.get('vlanid')
Line 49: self._customize()
Line 50: 


https://gerrit.ovirt.org/#/c/48473/13/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)
> Please replace the \ here with () - not in 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/48473
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
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: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: check operation result for vm nic hotunplug

[mmirecki]

Marcin Mirecki has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 9:

(2 comments)

https://gerrit.ovirt.org/#/c/48473/9/vdsm/virt/vmdevices/network.py
File vdsm/virt/vmdevices/network.py:

Line 193: The path is relative to the root element
Line 194: """
Line 195: return "./devices/interface/mac[@address='%s']" % self.macAddr
Line 196: 
Line 197: def _makeName(self):
> So name is sometimes given as argument to __init__, sometimes is not set at
Done
Line 198: name = ''
Line 199: if hasattr(self, 'type'):
Line 200: name += self.type + ' '
Line 201: if hasattr(self, 'macAddr'):


Line 193: The path is relative to the root element
Line 194: """
Line 195: return "./devices/interface/mac[@address='%s']" % self.macAddr
Line 196: 
Line 197: def _makeName(self):
> So name is sometimes given as argument to __init__, sometimes is not set at
Done
Line 198: name = ''
Line 199: if hasattr(self, 'type'):
Line 200: name += self.type + ' '
Line 201: if hasattr(self, 'macAddr'):


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
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: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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: check operation result for vm nic hotunplug

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 14:

* #1134256::Update tracker: OK
* #1199782::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1134256::OK, public bug
* Check Public Bug::#1199782::OK, public bug
* Check Product::#1134256::OK, Correct product Red Hat Enterprise 
Virtualization Manager
* Check Product::#1199782::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/48473
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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]: vm: check operation result for vm nic hotunplug

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 10:

* #1134256::Update tracker: OK
* #1199782::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1134256::OK, public bug
* Check Public Bug::#1199782::OK, public bug
* Check Product::#1134256::OK, Correct product Red Hat Enterprise 
Virtualization Manager
* Check Product::#1199782::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/48473
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
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: 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: check operation result for vm nic hotunplug

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 9:

(1 comment)

https://gerrit.ovirt.org/#/c/48473/9/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2691: while self._isDriveAttached(device):
Line 2692: time.sleep(sleep_time)
Line 2693: if utils.monotonic_time() > deadline:
Line 2694: raise HotunplugTimeout("Timeout detaching device 
%s"
Line 2695:% device.name)
> why is this rename useful?
We support any device now?
Line 2696: 
Line 2697: def _isDriveAttached(self, device):
Line 2698: root = ET.fromstring(self._dom.XMLDesc(0))
Line 2699: return bool(root.findall(device.xpath))


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
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: 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: check operation result for vm nic hotunplug

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 12:

* #1134256::Update tracker: OK
* #1199782::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1134256::OK, public bug
* Check Public Bug::#1199782::OK, public bug
* Check Product::#1134256::OK, Correct product Red Hat Enterprise 
Virtualization Manager
* Check Product::#1199782::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/48473
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
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: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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]: vm: check operation result for vm nic hotunplug

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 10: Code-Review-1

This should be squashed with https://gerrit.ovirt.org/49038

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
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: 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: check operation result for vm nic hotunplug

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 11:

* #1134256::Update tracker: OK
* #1199782::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1134256::OK, public bug
* Check Public Bug::#1199782::OK, public bug
* Check Product::#1134256::OK, Correct product Red Hat Enterprise 
Virtualization Manager
* Check Product::#1199782::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/48473
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
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: Ido Barkan 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki 
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]: vm: check operation result for vm nic hotunplug

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 10: Code-Review+1

Waiting for Francesco approval.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
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: 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: check operation result for vm nic hotunplug

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 8:

* #1134256::Update tracker: OK
* #1199782::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1134256::OK, public bug
* Check Public Bug::#1199782::OK, public bug
* Check Product::#1134256::OK, Correct product Red Hat Enterprise 
Virtualization Manager
* Check Product::#1199782::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/48473
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 8
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: 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: check operation result for vm nic hotunplug

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 9:

* #1134256::Update tracker: OK
* #1199782::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1134256::OK, public bug
* Check Public Bug::#1199782::OK, public bug
* Check Product::#1134256::OK, Correct product Red Hat Enterprise 
Virtualization Manager
* Check Product::#1199782::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/48473
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
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: 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: check operation result for vm nic hotunplug

[fromani]

Francesco Romani has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 9:

(2 comments)

https://gerrit.ovirt.org/#/c/48473/9/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2669: def _waitForDriveRemoval(self, device):
 : """
 : As stated in libvirt documentary, after detaching a device 
using
 : virDomainDetachDeviceFlags, we need to verify that this 
device
 : has actually been detached:
 : 
libvirt.org/html/libvirt-libvirt-domain.html#virDomainDetachDeviceFlags
 : 
 : This function waits for the device to be detached.
 : 
 : Currently we use virDomainDetachDevice. However- That 
function behaves
 : the same in that matter. (Currently it is not documented at 
libvirt's
 : API docs- but after contacting libvirt's guys it turned out 
that this
 : is true. Bug 1257280 opened for fixing the documentation.)
 : TODO: remove this comment when the documentation will be 
fixed.
 : 
 : :param device: Device to wait for
 : """
 : self.log.debug("Waiting for hotunplug to finish")
 : with utils.stopwatch("Hotunplug device %s" % device.name):
 : deadline = (utils.monotonic_time() +
 : config.getfloat('vars', 'hotunplug_timeout'))
 : sleep_time = config.getfloat('vars', 
'hotunplug_check_interval')
 : while self._isDriveAttached(device):
 : time.sleep(sleep_time)
 : if utils.monotonic_time() > deadline:
 : raise HotunplugTimeout("Timeout detaching device 
%s"
 :% device.name)
why is this rename useful?


https://gerrit.ovirt.org/#/c/48473/9/vdsm/virt/vmdevices/network.py
File vdsm/virt/vmdevices/network.py:

Line 193: The path is relative to the root element
Line 194: """
Line 195: return "./devices/interface/mac[@address='%s']" % self.macAddr
Line 196: 
Line 197: def _makeName(self):
why not just make this a property?


  @property
  def name(self):
name = ''
if hasattr(self, 'type'):
  name += self.type + ' '
if hasattr(self, 'macAddr'):
  name += self.macAddr + ' '
Line 198: name = ''
Line 199: if hasattr(self, 'type'):
Line 200: name += self.type + ' '
Line 201: if hasattr(self, 'macAddr'):


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
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: 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: check operation result for vm nic hotunplug

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 6:

* #1134256::Update tracker: OK
* #1199782::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1134256::OK, public bug
* Check Public Bug::#1199782::OK, public bug
* Check Product::#1134256::OK, Correct product Red Hat Enterprise 
Virtualization Manager
* Check Product::#1199782::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/48473
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 6
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: 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: check operation result for vm nic hotunplug

[mmirecki]

Marcin Mirecki has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 5:

(2 comments)

https://gerrit.ovirt.org/#/c/48473/5/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2681: API docs- but after contacting libvirt's guys it turned out 
that this
Line 2682: is true. Bug 1257280 opened for fixing the documentation.)
Line 2683: TODO: remove this comment when the documentation will be 
fixed.
Line 2684: 
Line 2685: :param device: Device to be removed
> Device to wait for
Done
Line 2686: """
Line 2687: self.log.debug("Waiting for hotunplug to finish")
Line 2688: with utils.stopwatch("Hotunplug device %s" % device.name):
Line 2689: deadline = (utils.monotonic_time() +


Line 2687: self.log.debug("Waiting for hotunplug to finish")
Line 2688: with utils.stopwatch("Hotunplug device %s" % device.name):
Line 2689: deadline = (utils.monotonic_time() +
Line 2690: self._hotunplug_timeout)
Line 2691: while self._is_device_attached(device):
> This is much nicer, but lets do this in a separate patch, renaming also _wa
Done
Line 2692: self._wait_for_sleep()
Line 2693: if utils.monotonic_time() > deadline:
Line 2694: raise HotunplugTimeout("Timeout detaching device 
%s"
Line 2695:% device.name)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 5
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: 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: check operation result for vm nic hotunplug

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 7:

* #1134256::Update tracker: OK
* #1199782::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1134256::OK, public bug
* Check Public Bug::#1199782::OK, public bug
* Check Product::#1134256::OK, Correct product Red Hat Enterprise 
Virtualization Manager
* Check Product::#1199782::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/48473
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 7
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: 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: check operation result for vm nic hotunplug

[mmirecki]

Marcin Mirecki has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 4:

(4 comments)

https://gerrit.ovirt.org/#/c/48473/4/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2665: self._cleanupDrives(drive)
Line 2666: 
Line 2667: return {'status': doneCode, 'vmList': self.status()}
Line 2668: 
Line 2669: def _waitForDeviceRemoval(self, device):
> This can be good point to add a test.
Unit test added in:
https://gerrit.ovirt.org/#/c/48880/
Line 2670: """
Line 2671: As stated in libvirt documentary, after detaching a device 
using
Line 2672: virDomainDetachDeviceFlags, we need to verify that this 
device
Line 2673: has actually been detached:


Line 2692: if utils.monotonic_time() > deadline:
Line 2693: raise HotunplugTimeout("Timeout detaching device 
%s"
Line 2694:% device.name)
Line 2695: 
Line 2696: def _is_device_attached(self, device):
> This method is clumsy. When method starts with is, it should be about the o
Done in:
https://gerrit.ovirt.org/#/c/48881/
Line 2697: root = ET.fromstring(self._dom.XMLDesc(0))
Line 2698: return bool(root.findall(device.xpath))
Line 2699: 
Line 2700: def _readPauseCode(self):


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

Line 425: 
Line 426: @property
Line 427: def xpath(self):
Line 428: """
Line 429: Returns an xpath to the device in libvirt dom xml
> an xpath?
Done
Line 430: The path is relative to the root element"""
Line 431: source_key = {DISK_TYPE.FILE: 'file',
Line 432:   DISK_TYPE.BLOCK: 'dev',
Line 433:   DISK_TYPE.NETWORK: 'name'}


Line 426: @property
Line 427: def xpath(self):
Line 428: """
Line 429: Returns an xpath to the device in libvirt dom xml
Line 430: The path is relative to the root element"""
> Use this format for docstrings:
Done
Line 431: source_key = {DISK_TYPE.FILE: 'file',
Line 432:   DISK_TYPE.BLOCK: 'dev',
Line 433:   DISK_TYPE.NETWORK: 'name'}
Line 434: return "./devices/disk/source[@%s='%s']" \


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 4
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: 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: check operation result for vm nic hotunplug

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 5:

* #1134256::Update tracker: OK
* #1199782::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1134256::OK, public bug
* Check Public Bug::#1199782::OK, public bug
* Check Product::#1134256::OK, Correct product Red Hat Enterprise 
Virtualization Manager
* Check Product::#1199782::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/48473
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 5
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: 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: check operation result for vm nic hotunplug

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 5:

(2 comments)

I think this is ready after removing the unneeded changes, and updating the 
tests when we finish the tests.

https://gerrit.ovirt.org/#/c/48473/5/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2681: API docs- but after contacting libvirt's guys it turned out 
that this
Line 2682: is true. Bug 1257280 opened for fixing the documentation.)
Line 2683: TODO: remove this comment when the documentation will be 
fixed.
Line 2684: 
Line 2685: :param device: Device to be removed
Device to wait for
Line 2686: """
Line 2687: self.log.debug("Waiting for hotunplug to finish")
Line 2688: with utils.stopwatch("Hotunplug device %s" % device.name):
Line 2689: deadline = (utils.monotonic_time() +


Line 2687: self.log.debug("Waiting for hotunplug to finish")
Line 2688: with utils.stopwatch("Hotunplug device %s" % device.name):
Line 2689: deadline = (utils.monotonic_time() +
Line 2690: self._hotunplug_timeout)
Line 2691: while self._is_device_attached(device):
This is much nicer, but lets do this in a separate patch, renaming also 
_waitForDeviceRemoval. Lets keep this patch minimal, only what we must change 
to make it work also for other devices.
Line 2692: self._wait_for_sleep()
Line 2693: if utils.monotonic_time() > deadline:
Line 2694: raise HotunplugTimeout("Timeout detaching device 
%s"
Line 2695:% device.name)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 5
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: 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: check operation result for vm nic hotunplug

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 4:

* #1134256::Update tracker: OK
* #1199782::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1134256::OK, public bug
* Check Public Bug::#1199782::OK, public bug
* Check Product::#1134256::OK, Correct product Red Hat Enterprise 
Virtualization Manager
* Check Product::#1199782::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/48473
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 4
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: 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: check operation result for vm nic hotunplug

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 3:

(4 comments)

Nice! but we must have tests before doing this refactoring.

https://gerrit.ovirt.org/#/c/48473/3/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2236: self.saveState()
Line 2237: 
Line 2238: try:
Line 2239: self._dom.detachDevice(nicXml)
Line 2240: mac = nicParams['macAddr']
Why do we need this variable here?
Line 2241: self._waitForDeviceRemoval(nic)
Line 2242: except HotunplugTimeout as e:
Line 2243: self.log.error("%s", e)
Line 2244: return response.error('hotunplugNic', "%s" % e)


Line 2682: is true. Bug 1257280 opened for fixing the documentation.)
Line 2683: TODO: remove this comment when the documentation will be 
fixed.
Line 2684: 
Line 2685: :param device_name: Device name for logging purposes
Line 2686: :device_search_patch XPath query for the detached device
Please update - we get only one parameter now.
Line 2687: """
Line 2688: self.log.debug("Waiting for hotunplug to finish")
Line 2689: with utils.stopwatch("Hotunplug device %s" % device.name):
Line 2690: deadline = (utils.monotonic_time() +


https://gerrit.ovirt.org/#/c/48473/3/vdsm/virt/vmdevices/network.py
File vdsm/virt/vmdevices/network.py:

Line 184: else:
Line 185: raise Exception('Tried to detach a non host device: %s' % 
(
Line 186: self.conf,))
Line 187: 
Line 188: def get_device_path(self):
Lets call it xpath and make it a readonly property:

@property
def xpath(self):
return "./devices/interface/mac[@address='%s']" % self.macAddr

Now we can do this in client code:

isDeviceAttached(device):
return bool(root.findall(device.xpath))
Line 189: """
Line 190: Returns an xpath to the device in libvirt dom xml
Line 191: The path is relative to the root element"""


https://gerrit.ovirt.org/#/c/48473/3/vdsm/virt/vmdevices/storage.py
File vdsm/virt/vmdevices/storage.py:

Line 422: 
Line 423: if hasattr(self, 'specParams') and 'ioTune' in 
self.specParams:
Line 424: self._validateIoTuneParams(self.specParams['ioTune'])
Line 425: 
Line 426: def get_device_path(self):
Should be a property, see my comment in vmdevices/network.py
Line 427: """
Line 428: Returns an xpath to the device in libvirt dom xml
Line 429: The path is relative to the root element"""
Line 430: source_key = {DISK_TYPE.FILE: 'file',


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 3
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: 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: check operation result for vm nic hotunplug

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 3: Code-Review-1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 3
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: 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: check operation result for vm nic hotunplug

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 3:

* #1134256::Update tracker: OK
* #1199782::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1134256::OK, public bug
* Check Public Bug::#1199782::OK, public bug
* Check Product::#1134256::OK, Correct product Red Hat Enterprise 
Virtualization Manager
* Check Product::#1199782::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/48473
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 3
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: 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: check operation result for vm nic hotunplug

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 4:

(4 comments)

https://gerrit.ovirt.org/#/c/48473/4/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2665: self._cleanupDrives(drive)
Line 2666: 
Line 2667: return {'status': doneCode, 'vmList': self.status()}
Line 2668: 
Line 2669: def _waitForDeviceRemoval(self, device):
This can be good point to add a test.

- Mock vm._dom so it will return fake fake xml from a list of xmls.
- call waitForDriveRemoval (this test should come before this patch)
- Mock the configuration so we timeout after 1 second
- Change the time.sleep(1) to get the sleep delay from the config - in a 
separte patch before the test patch.
- Mock the sleep time to 0.1 seconds

- Test one xml without the device for testing the special case of quick removal
- The last xml in the list will be one without the device for testing 
successful removal.
- The last xml will contain the device for testing timeut.

The only change needed to adapt to this patch is a rename and add tests for nic.
Line 2670: """
Line 2671: As stated in libvirt documentary, after detaching a device 
using
Line 2672: virDomainDetachDeviceFlags, we need to verify that this 
device
Line 2673: has actually been detached:


Line 2692: if utils.monotonic_time() > deadline:
Line 2693: raise HotunplugTimeout("Timeout detaching device 
%s"
Line 2694:% device.name)
Line 2695: 
Line 2696: def _is_device_attached(self, device):
This method is clumsy. When method starts with is, it should be about the 
object (the vm), not about the argument.

What if we move it to the device?

device.is_attached_to(self._dom)

Should be in another patch, after this one, since it is not required to make 
this reusable.
Line 2697: root = ET.fromstring(self._dom.XMLDesc(0))
Line 2698: return bool(root.findall(device.xpath))
Line 2699: 
Line 2700: def _readPauseCode(self):


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

Line 425: 
Line 426: @property
Line 427: def xpath(self):
Line 428: """
Line 429: Returns an xpath to the device in libvirt dom xml
an xpath?
Line 430: The path is relative to the root element"""
Line 431: source_key = {DISK_TYPE.FILE: 'file',
Line 432:   DISK_TYPE.BLOCK: 'dev',
Line 433:   DISK_TYPE.NETWORK: 'name'}


Line 426: @property
Line 427: def xpath(self):
Line 428: """
Line 429: Returns an xpath to the device in libvirt dom xml
Line 430: The path is relative to the root element"""
Use this format for docstrings:

"""
Text...
"""

Same for other modules.
Line 431: source_key = {DISK_TYPE.FILE: 'file',
Line 432:   DISK_TYPE.BLOCK: 'dev',
Line 433:   DISK_TYPE.NETWORK: 'name'}
Line 434: return "./devices/disk/source[@%s='%s']" \


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 4
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: 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: check operation result for vm nic hotunplug

[fromani]

Francesco Romani has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 2:

Marcin, please address comments from Nir. In followup patches, we can make 
waitForDriveRemoval reusable and add tests for it. I can take care of this 
(albeit with fairly low prio due to other things preempting).

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 2
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: 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: check operation result for vm nic hotunplug

[mmirecki]

Marcin Mirecki has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 2:

(1 comment)

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

Line 14: can also be applied to nics.
Line 15: 
Line 16: This patch is similar to:
Line 17: https://gerrit.ovirt.org/#/c/45138
Line 18: https://bugzilla.redhat.com/1044466
> The storage patch adding this feature was solving a horrible issue. How sev
We fail to remove the nic on vds, while the engine thinks that the nic is 
removed. The most severe consequence is that the stray nic still occupies a pci 
slot, so adding a new nic very often fails if trying to plug in to the same 
slot. 
We are also running into a risk of amassing a number of such devices which 
would exhaust the pci slots available.
Line 19: 
Line 20: Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Line 21: Signed-off-by: Marcin Mirecki 
Line 22: Bug-Url: https://bugzilla.redhat.com/1134256


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 2
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: 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: check operation result for vm nic hotunplug

[fromani]

Francesco Romani has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 2:

+1 for the concept, codewise please address the comments from Nir.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 2
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: 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: check operation result for vm nic hotunplug

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 2:

I would like to see a separate patch adding tests for current code, and on top 
of it patch making waitForDrive reusable.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 2
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: 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: check operation result for vm nic hotunplug

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 2: Code-Review-1

(3 comments)

Please write tests for this code before you change it.

https://gerrit.ovirt.org/#/c/48473/2/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2250: try:
Line 2251: self._dom.detachDevice(nicXml)
Line 2252: mac = nicParams['macAddr']
Line 2253: self._waitForDeviceRemoval("NIC (mac='" + mac + "')",
Line 2254:
self._construct_nic_search_path(mac))
Having common waitForDeviceRemoval makes sense, but the arguments to this 
function are too strange.

The arguments should be something like predicate function that return True when 
the device is removed, and a device dict/object.

Something like this:

self._waitForDeviceRemoval(self.drive_attached, drive)

self._waitForDeviceRemoval(self.nic_attached, nic)

Or maybe just the object you want to wait for. This object should have
a method is_attached, accepting the domain xml.
Line 2255: except HotunplugTimeout as e:
Line 2256: self.log.error("%s", e)
Line 2257: return response.error('hotunplugNic', "%s" % e)
Line 2258: except libvirt.libvirtError as e:


Line 2711: source_key = {DISK_TYPE.FILE: 'file',
Line 2712:   DISK_TYPE.BLOCK: 'dev',
Line 2713:   DISK_TYPE.NETWORK: 'name'}
Line 2714: return "./devices/disk/source[@%s='%s']" \
Line 2715:% (source_key[drive.diskType], drive.path)
This should be a method or property of Drive object.
Line 2716: 
Line 2717: def _construct_nic_search_path(self, mac):
Line 2718: return "./devices/interface/mac[@address='%s']" % mac
Line 2719: 


Line 2714: return "./devices/disk/source[@%s='%s']" \
Line 2715:% (source_key[drive.diskType], drive.path)
Line 2716: 
Line 2717: def _construct_nic_search_path(self, mac):
Line 2718: return "./devices/interface/mac[@address='%s']" % mac
This should be a method or property of a Nic object.
Line 2719: 
Line 2720: def _is_device_attached(self, device_search_path):
Line 2721: root = ET.fromstring(self._dom.XMLDesc(0))
Line 2722: return bool(root.findall(device_search_path))


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 2
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: 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: check operation result for vm nic hotunplug

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 2:

(1 comment)

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

Line 14: can also be applied to nics.
Line 15: 
Line 16: This patch is similar to:
Line 17: https://gerrit.ovirt.org/#/c/45138
Line 18: https://bugzilla.redhat.com/1044466
The storage patch adding this feature was solving a horrible issue. How severe 
is the issue with nics?
Line 19: 
Line 20: Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Line 21: Signed-off-by: Marcin Mirecki 
Line 22: Bug-Url: https://bugzilla.redhat.com/1134256


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 2
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: 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: check operation result for vm nic hotunplug

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 2:

* #1134256::Update tracker: OK
* #1199782::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1134256::OK, public bug
* Check Public Bug::#1199782::OK, public bug
* Check Product::#1134256::OK, Correct product Red Hat Enterprise 
Virtualization Manager
* Check Product::#1199782::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/48473
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
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: check operation result for vm nic hotunplug

[mmirecki]

Marcin Mirecki has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 2: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 2
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: 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: check operation result for vm nic hotunplug

[mmirecki]

Marcin Mirecki has uploaded a new change for review.

Change subject: vm: check operation result for vm nic hotunplug
..

vm: check operation result for vm nic hotunplug

After detaching a device  we need to verify that this device
has actually been detached.

This mechanism was already implemented for disks. This
patch generalizes the disk waiting mechanism so that it
can also be applied to nics.

This patch is similar to:
https://gerrit.ovirt.org/#/c/45138
https://bugzilla.redhat.com/1044466

Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Signed-off-by: Marcin Mirecki 
Bug-Url: https://bugzilla.redhat.com/1134256
Bug-Url: https://bugzilla.redhat.com/1199782
---
M vdsm/virt/vm.py
1 file changed, 26 insertions(+), 12 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/73/48473/1

diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py
index a5eead1..6a9e318 100644
--- a/vdsm/virt/vm.py
+++ b/vdsm/virt/vm.py
@@ -2237,6 +2237,12 @@
 
 try:
 self._dom.detachDevice(nicXml)
+mac = nicParams['macAddr']
+self._waitForDeviceRemoval("NIC (mac='" + mac + "')",
+   self._construct_nic_search_path(mac))
+except HotunplugTimeout as e:
+self.log.error("%s", e)
+return response.error('hotunplugNic', "%s" % e)
 except libvirt.libvirtError as e:
 self.log.exception("Hotunplug failed")
 if e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN:
@@ -2636,7 +2642,8 @@
 params=drive.custom)
 try:
 self._dom.detachDevice(driveXml)
-self._waitForDriveRemoval(drive)
+self._waitForDeviceRemoval("disk %s" % drive.name,
+   
self._construct_drive_search_path(drive))
 except HotunplugTimeout as e:
 self.log.error("%s", e)
 return response.error('hotunplugDisk', "%s" % e)
@@ -2662,14 +2669,14 @@
 
 return {'status': doneCode, 'vmList': self.status()}
 
-def _waitForDriveRemoval(self, drive):
+def _waitForDeviceRemoval(self, device_name, device_search_path):
 """
 As stated in libvirt documentary, after detaching a device using
 virDomainDetachDeviceFlags, we need to verify that this device
 has actually been detached:
 libvirt.org/html/libvirt-libvirt-domain.html#virDomainDetachDeviceFlags
 
-This function waits for the disk device to be detached.
+This function waits for the device to be detached.
 
 Currently we use virDomainDetachDevice. However- That function behaves
 the same in that matter. (Currently it is not documented at libvirt's
@@ -2677,25 +2684,32 @@
 is true. Bug 1257280 opened for fixing the documentation.)
 TODO: remove this comment when the documentation will be fixed.
 
-:param drive: The drive that should be detached.
+:param device_name: Device name for logging purposes
+:device_search_patch XPath query for the detached device
 """
 self.log.debug("Waiting for hotunplug to finish")
-with utils.stopwatch("Hotunplug disk %s" % drive.name):
+with utils.stopwatch("Hotunplug device %s" % device_name):
 deadline = (utils.monotonic_time() +
 config.getint('vars', 'hotunplug_timeout'))
-while self._isDriveAttached(drive):
+while self._is_device_attached(device_search_path):
 time.sleep(1)
 if utils.monotonic_time() > deadline:
-raise HotunplugTimeout("Timeout detaching drive %s"
-   % drive.name)
+raise HotunplugTimeout("Timeout detaching device %s"
+   % device_name)
 
-def _isDriveAttached(self, drive):
-root = ET.fromstring(self._dom.XMLDesc(0))
+def _construct_drive_search_path(self, drive):
 source_key = {DISK_TYPE.FILE: 'file',
   DISK_TYPE.BLOCK: 'dev',
   DISK_TYPE.NETWORK: 'name'}
-return bool(root.findall("./devices/disk/source[@%s='%s']" %
- (source_key[drive.diskType], drive.path)))
+return "./devices/disk/source[@%s='%s']" \
+   % (source_key[drive.diskType], drive.path)
+
+def _construct_nic_search_path(self, mac):
+return "./devices/interface/mac[@address='%s']" % mac
+
+def _is_device_attached(self, device_search_path):
+root = ET.fromstring(self._dom.XMLDesc(0))
+return bool(root.findall(device_search_path))
 
 def _readPauseCode(self):
 state, reason = self._dom.state(0)


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

Change in vdsm[master]: vm: check operation result for vm nic hotunplug

[automation]

automat...@ovirt.org has posted comments on this change.

Change subject: vm: check operation result for vm nic hotunplug
..


Patch Set 1:

* #1134256::Update tracker: OK
* #1199782::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1134256::OK, public bug
* Check Public Bug::#1199782::OK, public bug
* Check Product::#1134256::OK, Correct product Red Hat Enterprise 
Virtualization Manager
* Check Product::#1199782::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/48473
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3b2c839cbb4733aecc8d5a0e9a1ae691e14ac3
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches