Change in vdsm[master]: Remove the _preparedDrives dictionary from vm

[Federico Simoncelli]

Federico Simoncelli has uploaded a new change for review.

Change subject: Remove the _preparedDrives dictionary from vm
......................................................................

Remove the _preparedDrives dictionary from vm

The _preparedDrives dictionary was a fix for bz670599 introduced with
the commit 7aa57fe. Currently prepareVolumePath is not relying on the
resource manager anymore and therefore the race fix is superfluous.

In this patch:
* remove the _preparedDrives dictionary and its lock (_volPrepareLock)
* make prepareVolumePath and teardownVolumePath public

Change-Id: I8d310f216987b7ded89e71752464934f70c051ad
---
M vdsm/clientIF.py
M vdsm/libvirtvm.py
M vdsm/vm.py
3 files changed, 26 insertions(+), 47 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/23/1123/1
--
To view, visit http://gerrit.ovirt.org/1123
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I8d310f216987b7ded89e71752464934f70c051ad
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: Remove the _preparedDrives dictionary from vm
......................................................................

Patch Set 1: I would prefer that you didn't submit this

(1 inline comment)

....................................................
File vdsm/vm.py
Line 573:             drive['path'] = self.cif.prepareVolumePath(drive)
Maybe you need self.destroyed here

--
To view, visit http://gerrit.ovirt.org/1123
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8d310f216987b7ded89e71752464934f70c051ad
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: Remove the _preparedDrives dictionary from vm
......................................................................

Patch Set 2: Looks good to me, but someone else must approve

--
To view, visit http://gerrit.ovirt.org/1123
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8d310f216987b7ded89e71752464934f70c051ad
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: Remove the _preparedDrives dictionary from vm
......................................................................

Patch Set 3: Looks good to me, but someone else must approve

--
To view, visit http://gerrit.ovirt.org/1123
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8d310f216987b7ded89e71752464934f70c051ad
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky

Saggi Mizrahi has posted comments on this change.

Change subject: Remove the _preparedDrives dictionary from vm
......................................................................

Patch Set 3: Looks good to me, but someone else must approve

--
To view, visit http://gerrit.ovirt.org/1123
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8d310f216987b7ded89e71752464934f70c051ad
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Remove the _preparedDrives dictionary from vm
......................................................................

Patch Set 4: I would prefer that you didn't submit this

(2 inline comments)

....................................................
File vdsm/vm.py
Line 575:                 # a race with _cleanup
but the race still exists: if destroy() is called right after self.destroyed is checked, _cleanup may be quicker than preparePaths, try to teardown drive, swallow the failure, and we are going to prepare drive in line 577.

We end up with a leaked volume (which is not the end of the world, just ugly).

Line 861:                                drive, exc_info=True)
is exc_info=True intentional?

--
To view, visit http://gerrit.ovirt.org/1123
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8d310f216987b7ded89e71752464934f70c051ad
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Remove the _preparedDrives dictionary from vm
......................................................................

Patch Set 4:

above are minor issues, but I'd like to hear your opinion.

--
To view, visit http://gerrit.ovirt.org/1123
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8d310f216987b7ded89e71752464934f70c051ad
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: Remove the _preparedDrives dictionary from vm
......................................................................

Patch Set 4: I would prefer that you didn't submit this

(2 inline comments)

....................................................
File vdsm/vm.py
Line 575:                 # a race with _cleanup
Yep, you right. It looks like we still need self._volPrepareLock between prepare and _cleanup

Line 861:                                drive, exc_info=True)
You are right it shouldn't be True.

--
To view, visit http://gerrit.ovirt.org/1123
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8d310f216987b7ded89e71752464934f70c051ad
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: Remove the _preparedDrives dictionary from vm
......................................................................

Patch Set 5: I would prefer that you didn't submit this

Look at comments on previous patchset

--
To view, visit http://gerrit.ovirt.org/1123
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8d310f216987b7ded89e71752464934f70c051ad
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Remove the _preparedDrives dictionary from vm
......................................................................

Patch Set 4: (1 inline comment)

....................................................
File vdsm/vm.py
Line 575:                 # a race with _cleanup
Argh! I knew we shouldn't have trusted destroyed and we needed a lock here! Grr. My fault for being easily convinced.

--
To view, visit http://gerrit.ovirt.org/1123
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8d310f216987b7ded89e71752464934f70c051ad
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: Remove the _preparedDrives dictionary from vm
......................................................................

Patch Set 6: Looks good to me, but someone else must approve

(1 inline comment)

Just minor typo in commit message

....................................................
Commit Message
Line 14: * remove the _preparedDrives dictionary and its lock (_volPrepareLock)
lock still here

--
To view, visit http://gerrit.ovirt.org/1123
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8d310f216987b7ded89e71752464934f70c051ad
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: Remove the _preparedDrives dictionary from vm
......................................................................

Patch Set 7: Looks good to me, approved

--
To view, visit http://gerrit.ovirt.org/1123
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8d310f216987b7ded89e71752464934f70c051ad
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Remove the _preparedDrives dictionary from vm
......................................................................

Patch Set 7: Verified

--
To view, visit http://gerrit.ovirt.org/1123
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8d310f216987b7ded89e71752464934f70c051ad
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: Remove the _preparedDrives dictionary from vm
......................................................................

Remove the _preparedDrives dictionary from vm

The _preparedDrives dictionary was a fix for bz670599 introduced with
the commit 7aa57fe. Currently prepareVolumePath is not relying on the
resource manager anymore and therefore the race fix is superfluous.

In this patch:
* make prepareVolumePath and teardownVolumePath public

Change-Id: I8d310f216987b7ded89e71752464934f70c051ad
---
M vdsm/clientIF.py
M vdsm/libvirtvm.py
M vdsm/vm.py
3 files changed, 31 insertions(+), 48 deletions(-)

Approvals:
  Dan Kenigsberg: Verified
  Igor Lvovsky: Looks good to me, approved

--
To view, visit http://gerrit.ovirt.org/1123
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I8d310f216987b7ded89e71752464934f70c051ad
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi